[RFC,11/13] can: slcan: add ethtool support to reset adapter errors
diff mbox series

Message ID 20220607094752.1029295-12-dario.binacchi@amarulasolutions.com
State New
Headers show
Series
  • can: slcan: extend supported features
Related show

Commit Message

Dario Binacchi June 7, 2022, 9:47 a.m. UTC 
This patch adds a private flag to the slcan driver to switch the
"err-rst-on-open" setting on and off.

"err-rst-on-open" on  - Reset error states on opening command

"err-rst-on-open" off - Don't reset error states on opening command
                        (default)

The setting can only be changed if the interface is down:

    ip link set dev can0 down
    ethtool --set-priv-flags can0 err-rst-on-open {off|on}
    ip link set dev can0 up

Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
---

 drivers/net/can/slcan/Makefile        |  1 +
 drivers/net/can/slcan/slcan-core.c    | 36 +++++++++++++++
 drivers/net/can/slcan/slcan-ethtool.c | 65 +++++++++++++++++++++++++++
 drivers/net/can/slcan/slcan.h         | 18 ++++++++
 4 files changed, 120 insertions(+)
 create mode 100644 drivers/net/can/slcan/slcan-ethtool.c
 create mode 100644 drivers/net/can/slcan/slcan.h

Comments

Marc Kleine-Budde June 7, 2022, 10:52 a.m. UTC | #1 
On 07.06.2022 11:47:50, Dario Binacchi wrote:
> This patch adds a private flag to the slcan driver to switch the
> "err-rst-on-open" setting on and off.
> 
> "err-rst-on-open" on  - Reset error states on opening command
> 
> "err-rst-on-open" off - Don't reset error states on opening command
>                         (default)
> 
> The setting can only be changed if the interface is down:
> 
>     ip link set dev can0 down
>     ethtool --set-priv-flags can0 err-rst-on-open {off|on}
>     ip link set dev can0 up
> 
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

I'm a big fan of bringing the device into a well known good state during
ifup. What would be the reasons/use cases to not reset the device?

Marc
Dario Binacchi June 8, 2022, 4:33 p.m. UTC | #2 
Hi Marc,

On Tue, Jun 7, 2022 at 12:52 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 07.06.2022 11:47:50, Dario Binacchi wrote:
> > This patch adds a private flag to the slcan driver to switch the
> > "err-rst-on-open" setting on and off.
> >
> > "err-rst-on-open" on  - Reset error states on opening command
> >
> > "err-rst-on-open" off - Don't reset error states on opening command
> >                         (default)
> >
> > The setting can only be changed if the interface is down:
> >
> >     ip link set dev can0 down
> >     ethtool --set-priv-flags can0 err-rst-on-open {off|on}
> >     ip link set dev can0 up
> >
> > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>
> I'm a big fan of bringing the device into a well known good state during
> ifup. What would be the reasons/use cases to not reset the device?

Because by default either slcand and slcan_attach don't reset the error states,
but you must use the `-f' option to do so. So,  I followed this use case.

Thanks and regards,
Dario
>
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |
Marc Kleine-Budde June 9, 2022, 6:38 a.m. UTC | #3 
On 08.06.2022 18:33:08, Dario Binacchi wrote:
> Hi Marc,
> 
> On Tue, Jun 7, 2022 at 12:52 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >
> > On 07.06.2022 11:47:50, Dario Binacchi wrote:
> > > This patch adds a private flag to the slcan driver to switch the
> > > "err-rst-on-open" setting on and off.
> > >
> > > "err-rst-on-open" on  - Reset error states on opening command
> > >
> > > "err-rst-on-open" off - Don't reset error states on opening command
> > >                         (default)
> > >
> > > The setting can only be changed if the interface is down:
> > >
> > >     ip link set dev can0 down
> > >     ethtool --set-priv-flags can0 err-rst-on-open {off|on}
> > >     ip link set dev can0 up
> > >
> > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> >
> > I'm a big fan of bringing the device into a well known good state during
> > ifup. What would be the reasons/use cases to not reset the device?
> 
> Because by default either slcand and slcan_attach don't reset the
> error states, but you must use the `-f' option to do so. So, I
> followed this use case.

Is this a CAN bus error state, like Bus Off or some controller (i.e. non
CAN related) error?

regards,
Marc
Dario Binacchi June 9, 2022, 7:24 a.m. UTC | #4 
Hi Marc,

On Thu, Jun 9, 2022 at 8:38 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 08.06.2022 18:33:08, Dario Binacchi wrote:
> > Hi Marc,
> >
> > On Tue, Jun 7, 2022 at 12:52 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > >
> > > On 07.06.2022 11:47:50, Dario Binacchi wrote:
> > > > This patch adds a private flag to the slcan driver to switch the
> > > > "err-rst-on-open" setting on and off.
> > > >
> > > > "err-rst-on-open" on  - Reset error states on opening command
> > > >
> > > > "err-rst-on-open" off - Don't reset error states on opening command
> > > >                         (default)
> > > >
> > > > The setting can only be changed if the interface is down:
> > > >
> > > >     ip link set dev can0 down
> > > >     ethtool --set-priv-flags can0 err-rst-on-open {off|on}
> > > >     ip link set dev can0 up
> > > >
> > > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> > >
> > > I'm a big fan of bringing the device into a well known good state during
> > > ifup. What would be the reasons/use cases to not reset the device?
> >
> > Because by default either slcand and slcan_attach don't reset the
> > error states, but you must use the `-f' option to do so. So, I
> > followed this use case.
>
> Is this a CAN bus error state, like Bus Off or some controller (i.e. non
> CAN related) error?

The help option of slcan_attach and slcand prints " -f (read status
flags with 'F\\r' to reset error states)\n"
I looked at the sources of the adapter I am using (USBtin, which uses
the mcp2515 controller). The 'F'
command reads the EFLG register (0x2d) without resetting the RX0OVR
and RX1OVR overrun bits.
The error states reset is done by 'f <subcmd>' command, that is not
managed by slcan_attach/slcand.

        switch (subcmd) {
            case 0x0: // Disable status reporting
                mcp2515_write_register(MCP2515_REG_CANINTE, 0x00);
                return CR;
            case 0x1: // Enable status reporting
                mcp2515_write_register(MCP2515_REG_CANINTE, 0x20); //
ERRIE interrupt to INT pin
                return CR;
            case 0x2: // Clear overrun errors
                mcp2515_write_register(MCP2515_REG_EFLG, 0x00);
                return CR;
            case 0x3: // Reinit/reset MCP2515 to clear all errors
                if (state == STATE_CONFIG) {
                    mcp2515_init();
                    return CR;
                }
                break;
        }


Thanks and regards,
Dario

>
> regards,
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |
Marc Kleine-Budde June 9, 2022, 8:01 a.m. UTC | #5 
On 09.06.2022 09:24:14, Dario Binacchi wrote:
> > > > I'm a big fan of bringing the device into a well known good state during
> > > > ifup. What would be the reasons/use cases to not reset the device?
> > >
> > > Because by default either slcand and slcan_attach don't reset the
> > > error states, but you must use the `-f' option to do so. So, I
> > > followed this use case.
> >
> > Is this a CAN bus error state, like Bus Off or some controller (i.e. non
> > CAN related) error?
> 
> The help option of slcan_attach and slcand prints " -f (read status
> flags with 'F\\r' to reset error states)\n" I looked at the sources of
> the adapter I am using (USBtin, which uses the mcp2515 controller).
> The 'F' command reads the EFLG register (0x2d) without resetting the
> RX0OVR and RX1OVR overrun bits.

The Lawicel doc [1] says 'F' is to read the status flags not to clear
it. However commit 7ef581fec029 ("slcan_attach: added '-f' commandline
option to read status flags") [2] suggests that there are some adapters
that the reading of the status flag clears the errors. IMHO the 'F'
command should be send unconditionally during open.

[1] http://www.can232.com/docs/can232_v3.pdf
[2] https://github.com/linux-can/can-utils/commit/7ef581fec0298a228baa71372ea5667874fb4a56

> The error states reset is done by 'f <subcmd>' command, that is not
> managed by slcan_attach/slcand.

Is the 'f' command is non-standard?

>         switch (subcmd) {
>             case 0x0: // Disable status reporting
>                 mcp2515_write_register(MCP2515_REG_CANINTE, 0x00);
>                 return CR;
>             case 0x1: // Enable status reporting
>                 mcp2515_write_register(MCP2515_REG_CANINTE, 0x20); //
> ERRIE interrupt to INT pin
>                 return CR;
>             case 0x2: // Clear overrun errors
>                 mcp2515_write_register(MCP2515_REG_EFLG, 0x00);
>                 return CR;
>             case 0x3: // Reinit/reset MCP2515 to clear all errors
>                 if (state == STATE_CONFIG) {
>                     mcp2515_init();
>                     return CR;
>                 }
>                 break;
>         }

Marc
Dario Binacchi June 9, 2022, 8:52 a.m. UTC | #6 
Hi Marc,

On Thu, Jun 9, 2022 at 10:01 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 09.06.2022 09:24:14, Dario Binacchi wrote:
> > > > > I'm a big fan of bringing the device into a well known good state during
> > > > > ifup. What would be the reasons/use cases to not reset the device?
> > > >
> > > > Because by default either slcand and slcan_attach don't reset the
> > > > error states, but you must use the `-f' option to do so. So, I
> > > > followed this use case.
> > >
> > > Is this a CAN bus error state, like Bus Off or some controller (i.e. non
> > > CAN related) error?
> >
> > The help option of slcan_attach and slcand prints " -f (read status
> > flags with 'F\\r' to reset error states)\n" I looked at the sources of
> > the adapter I am using (USBtin, which uses the mcp2515 controller).
> > The 'F' command reads the EFLG register (0x2d) without resetting the
> > RX0OVR and RX1OVR overrun bits.
>
> The Lawicel doc [1] says 'F' is to read the status flags not to clear
> it. However commit 7ef581fec029 ("slcan_attach: added '-f' commandline
> option to read status flags") [2] suggests that there are some adapters
> that the reading of the status flag clears the errors. IMHO the 'F'
> command should be send unconditionally during open.

When in doubt I would follow the slcand / slcan_attach approach, that don't
send the 'F \ r' command by default. We would be more backward compatible
as regards the sequence of commands to be sent to the controller.

>
> [1] http://www.can232.com/docs/can232_v3.pdf
> [2] https://github.com/linux-can/can-utils/commit/7ef581fec0298a228baa71372ea5667874fb4a56
>
> > The error states reset is done by 'f <subcmd>' command, that is not
> > managed by slcan_attach/slcand.
>
> Is the 'f' command is non-standard?

It's possible.

Thanks and regards,
Dario

>
> >         switch (subcmd) {
> >             case 0x0: // Disable status reporting
> >                 mcp2515_write_register(MCP2515_REG_CANINTE, 0x00);
> >                 return CR;
> >             case 0x1: // Enable status reporting
> >                 mcp2515_write_register(MCP2515_REG_CANINTE, 0x20); //
> > ERRIE interrupt to INT pin
> >                 return CR;
> >             case 0x2: // Clear overrun errors
> >                 mcp2515_write_register(MCP2515_REG_EFLG, 0x00);
> >                 return CR;
> >             case 0x3: // Reinit/reset MCP2515 to clear all errors
> >                 if (state == STATE_CONFIG) {
> >                     mcp2515_init();
> >                     return CR;
> >                 }
> >                 break;
> >         }
>
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |
Marc Kleine-Budde June 10, 2022, 10:51 a.m. UTC | #7 
On 09.06.2022 10:52:03, Dario Binacchi wrote:
> > > The help option of slcan_attach and slcand prints " -f (read status
> > > flags with 'F\\r' to reset error states)\n" I looked at the sources of
> > > the adapter I am using (USBtin, which uses the mcp2515 controller).
> > > The 'F' command reads the EFLG register (0x2d) without resetting the
> > > RX0OVR and RX1OVR overrun bits.
> >
> > The Lawicel doc [1] says 'F' is to read the status flags not to clear
> > it. However commit 7ef581fec029 ("slcan_attach: added '-f' commandline
> > option to read status flags") [2] suggests that there are some adapters
> > that the reading of the status flag clears the errors. IMHO the 'F'
> > command should be send unconditionally during open.
> 
> When in doubt I would follow the slcand / slcan_attach approach, that don't
> send the 'F \ r' command by default. We would be more backward compatible
> as regards the sequence of commands to be sent to the controller.

Ok, keep it this way.

Marc

Patch
diff mbox series 

diff --git a/drivers/net/can/slcan/Makefile b/drivers/net/can/slcan/Makefile
index 2e84f7bf7617..8a88e484ee21 100644
--- a/drivers/net/can/slcan/Makefile
+++ b/drivers/net/can/slcan/Makefile
@@ -4,3 +4,4 @@  obj-$(CONFIG_CAN_SLCAN) += slcan.o
 
 slcan-objs :=
 slcan-objs += slcan-core.o
+slcan-objs += slcan-ethtool.o
diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
index d63d270d21da..b813a59534a3 100644
--- a/drivers/net/can/slcan/slcan-core.c
+++ b/drivers/net/can/slcan/slcan-core.c
@@ -57,6 +57,8 @@ 
 #include <linux/can/dev.h>
 #include <linux/can/skb.h>
 
+#include "slcan.h"
+
 MODULE_ALIAS_LDISC(N_SLCAN);
 MODULE_DESCRIPTION("serial line CAN interface");
 MODULE_LICENSE("GPL");
@@ -98,6 +100,8 @@  struct slcan {
 #define SLF_INUSE		0		/* Channel in use            */
 #define SLF_ERROR		1               /* Parity, etc. error        */
 #define SLF_XCMD		2               /* Command transmission      */
+	unsigned long           cmd_flags;      /* Command flags             */
+#define CF_ERR_RST		0               /* Reset errors on open      */
 	wait_queue_head_t       xcmd_wait;      /* Wait queue for commands   */
 						/* transmission              */
 };
@@ -117,6 +121,28 @@  static const struct can_bittiming_const slcan_bittiming_const = {
 	.brp_inc = 1,
 };
 
+bool slcan_err_rst_on_open(struct net_device *ndev)
+{
+	struct slcan *sl = netdev_priv(ndev);
+
+	return !!test_bit(CF_ERR_RST, &sl->cmd_flags);
+}
+
+int slcan_enable_err_rst_on_open(struct net_device *ndev, bool on)
+{
+	struct slcan *sl = netdev_priv(ndev);
+
+	if (netif_running(ndev))
+		return -EBUSY;
+
+	if (on)
+		set_bit(CF_ERR_RST, &sl->cmd_flags);
+	else
+		clear_bit(CF_ERR_RST, &sl->cmd_flags);
+
+	return 0;
+}
+
  /************************************************************************
   *			SLCAN ENCAPSULATION FORMAT			 *
   ************************************************************************/
@@ -483,6 +509,15 @@  static int slc_open(struct net_device *dev)
 	if (sl->can.bittiming.bitrate == 0) {
 		sl->can.bittiming.bitrate = -1UL;
 	} else {
+		if (test_bit(CF_ERR_RST, &sl->cmd_flags)) {
+			err = slcan_transmit_cmd(sl, "F\r");
+			if (err) {
+				netdev_err(sl->dev,
+					   "failed to send error command 'F\\r'\n");
+				return err;
+			}
+		}
+
 		err = slcan_transmit_cmd(sl, "O\r");
 		if (err) {
 			netdev_err(dev, "failed to send open command 'O\\r'\n");
@@ -645,6 +680,7 @@  static struct slcan *slc_alloc(void)
 
 	snprintf(dev->name, sizeof(dev->name), "slcan%d", i);
 	dev->netdev_ops = &slc_netdev_ops;
+	slcan_set_ethtool_ops(dev);
 	sl = netdev_priv(dev);
 
 	/* Initialize channel control data */
diff --git a/drivers/net/can/slcan/slcan-ethtool.c b/drivers/net/can/slcan/slcan-ethtool.c
new file mode 100644
index 000000000000..bf0afdc4e49d
--- /dev/null
+++ b/drivers/net/can/slcan/slcan-ethtool.c
@@ -0,0 +1,65 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/* Copyright (c) 2022 Amarula Solutions, Dario Binacchi <dario.binacchi@amarulasolutions.com>
+ *
+ */
+
+#include <linux/can/dev.h>
+#include <linux/ethtool.h>
+#include <linux/kernel.h>
+#include <linux/netdevice.h>
+#include <linux/platform_device.h>
+
+#include "slcan.h"
+
+static const char slcan_priv_flags_strings[][ETH_GSTRING_LEN] = {
+#define SLCAN_PRIV_FLAGS_ERR_RST_ON_OPEN BIT(0)
+	"err-rst-on-open",
+};
+
+static void slcan_get_strings(struct net_device *ndev, u32 stringset, u8 *data)
+{
+	switch (stringset) {
+	case ETH_SS_PRIV_FLAGS:
+		memcpy(data, slcan_priv_flags_strings,
+		       sizeof(slcan_priv_flags_strings));
+	}
+}
+
+static u32 slcan_get_priv_flags(struct net_device *ndev)
+{
+	u32 flags = 0;
+
+	if (slcan_err_rst_on_open(ndev))
+		flags |= SLCAN_PRIV_FLAGS_ERR_RST_ON_OPEN;
+
+	return flags;
+}
+
+static int slcan_set_priv_flags(struct net_device *ndev, u32 flags)
+{
+	bool err_rst_op_open = !!(flags & SLCAN_PRIV_FLAGS_ERR_RST_ON_OPEN);
+
+	return slcan_enable_err_rst_on_open(ndev, err_rst_op_open);
+}
+
+static int slcan_get_sset_count(struct net_device *netdev, int sset)
+{
+	switch (sset) {
+	case ETH_SS_PRIV_FLAGS:
+		return ARRAY_SIZE(slcan_priv_flags_strings);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static const struct ethtool_ops slcan_ethtool_ops = {
+	.get_strings = slcan_get_strings,
+	.get_priv_flags = slcan_get_priv_flags,
+	.set_priv_flags = slcan_set_priv_flags,
+	.get_sset_count = slcan_get_sset_count,
+};
+
+void slcan_set_ethtool_ops(struct net_device *netdev)
+{
+	netdev->ethtool_ops = &slcan_ethtool_ops;
+}
diff --git a/drivers/net/can/slcan/slcan.h b/drivers/net/can/slcan/slcan.h
new file mode 100644
index 000000000000..d463c8d99e22
--- /dev/null
+++ b/drivers/net/can/slcan/slcan.h
@@ -0,0 +1,18 @@ 
+/* SPDX-License-Identifier: GPL-2.0
+ * slcan.h - serial line CAN interface driver
+ *
+ * Copyright (C) Laurence Culhane <loz@holmes.demon.co.uk>
+ * Copyright (C) Fred N. van Kempen <waltje@uwalt.nl.mugnet.org>
+ * Copyright (C) Oliver Hartkopp <socketcan@hartkopp.net>
+ * Copyright (C) 2022 Amarula Solutions, Dario Binacchi <dario.binacchi@amarulasolutions.com>
+ *
+ */
+
+#ifndef _SLCAN_H
+#define _SLCAN_H
+
+bool slcan_err_rst_on_open(struct net_device *ndev);
+int slcan_enable_err_rst_on_open(struct net_device *ndev, bool on);
+void slcan_set_ethtool_ops(struct net_device *ndev);
+
+#endif /* _SLCAN_H */