[RFC PATCH 00/13] can: slcan: extend supported features

[RFC PATCH 00/13] can: slcan: extend supported features

[Dario Binacchi]

This series originated as a result of CAN communication tests for an
application using the USBtin adapter (https://www.fischl.de/usbtin/).
The tests showed some errors but for the driver everything was ok.
Also, being the first time I used the slcan driver, I was amazed that
it was not possible to configure the bitrate via the ip tool.
For these two reasons, I started looking at the driver code and realized
that it didn't use the CAN network device driver interface.

Starting from these assumptions, I tried to:
- Use the CAN network device driver interface.
- Set the bitrate via the ip tool.
- Send the open/close command to the adapter from the driver.
- Add ethtool support to reset the adapter errors.
- Extend the protocol to forward the adapter CAN communication
  errors and the CAN state changes to the netdev upper layers.

Except for the protocol extension patches (i. e. forward the adapter CAN
communication errors and the CAN state changes to the netdev upper
layers), the whole series has been tested. Testing the extension
protocol patches requires updating the adapter firmware. Before modifying
the firmware I think it makes sense to know if these extensions can be
considered useful.

Before applying the series I used these commands:

slcan_attach -f -s6 -o /dev/ttyACM0
slcand ttyACM0 can0
ip link set can0 up

After applying the series I am using these commands:

slcan_attach /dev/ttyACM0
slcand ttyACM0 can0
ip link set dev can0 down
ip link set can0 type can bitrate 500000
ethtool --set-priv-flags can0 err-rst-on-open on
ip link set dev can0 up

Now there is a clearer separation between serial line and CAN,
but above all, it is possible to use the ip and ethtool commands
as it happens for any CAN device driver. The changes are backward
compatible, you can continue to use the slcand and slcan_attach
command options.



Dario Binacchi (13):
  can: slcan: use the BIT() helper
  can: slcan: use netdev helpers to print out messages
  can: slcan: use the alloc_can_skb() helper
  can: slcan: use CAN network device driver API
  can: slcan: simplify the device de-allocation
  can: slcan: allow to send commands to the adapter
  can: slcan: set bitrate by CAN device driver API
  can: slcan: send the open command to the adapter
  can: slcan: send the close command to the adapter
  can: slcan: move driver into separate sub directory
  can: slcan: add ethtool support to reset adapter errors
  can: slcan: extend the protocol with error info
  can: slcan: extend the protocol with CAN state info

 drivers/net/can/Makefile                      |   2 +-
 drivers/net/can/slcan/Makefile                |   7 +
 .../net/can/{slcan.c => slcan/slcan-core.c}   | 464 +++++++++++++++---
 drivers/net/can/slcan/slcan-ethtool.c         |  65 +++
 drivers/net/can/slcan/slcan.h                 |  18 +
 5 files changed, 480 insertions(+), 76 deletions(-)
 create mode 100644 drivers/net/can/slcan/Makefile
 rename drivers/net/can/{slcan.c => slcan/slcan-core.c} (67%)
 create mode 100644 drivers/net/can/slcan/slcan-ethtool.c
 create mode 100644 drivers/net/can/slcan/slcan.h

-- 
2.32.0

[RFC PATCH 01/13] can: slcan: use the BIT() helper

[Dario Binacchi]

Use the BIT() helper instead of an explicit shift.

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

 drivers/net/can/slcan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index 64a3aee8a7da..b37d35c2a23a 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -413,7 +413,7 @@ static int slc_open(struct net_device *dev)
 	if (sl->tty == NULL)
 		return -ENODEV;
 
-	sl->flags &= (1 << SLF_INUSE);
+	sl->flags &= BIT(SLF_INUSE);
 	netif_start_queue(dev);
 	return 0;
 }
-- 
2.32.0

[RFC PATCH 02/13] can: slcan: use netdev helpers to print out messages

[Dario Binacchi]

Replace printk() calls with corresponding netdev helpers.

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

 drivers/net/can/slcan.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index b37d35c2a23a..6162a9c21672 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -365,7 +365,7 @@ static netdev_tx_t slc_xmit(struct sk_buff *skb, struct net_device *dev)
 	spin_lock(&sl->lock);
 	if (!netif_running(dev))  {
 		spin_unlock(&sl->lock);
-		printk(KERN_WARNING "%s: xmit: iface is down\n", dev->name);
+		netdev_warn(dev, "xmit: iface is down\n");
 		goto out;
 	}
 	if (sl->tty == NULL) {
@@ -776,8 +776,7 @@ static void __exit slcan_exit(void)
 
 		sl = netdev_priv(dev);
 		if (sl->tty) {
-			printk(KERN_ERR "%s: tty discipline still running\n",
-			       dev->name);
+			netdev_err(dev, "tty discipline still running\n");
 		}
 
 		unregister_netdev(dev);
-- 
2.32.0

[RFC PATCH 03/13] can: slcan: use the alloc_can_skb() helper

[Dario Binacchi]

It is used successfully by most (if not all) CAN device drivers. It
allows to remove replicated code.

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

 drivers/net/can/slcan.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index 6162a9c21672..964b02f321ab 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -54,6 +54,7 @@
 #include <linux/kernel.h>
 #include <linux/workqueue.h>
 #include <linux/can.h>
+#include <linux/can/dev.h>
 #include <linux/can/skb.h>
 #include <linux/can/can-ml.h>
 
@@ -143,7 +144,7 @@ static struct net_device **slcan_devs;
 static void slc_bump(struct slcan *sl)
 {
 	struct sk_buff *skb;
-	struct can_frame cf;
+	struct can_frame cf, *scf;
 	int i, tmp;
 	u32 tmpid;
 	char *cmd = sl->rbuff;
@@ -201,21 +202,11 @@ static void slc_bump(struct slcan *sl)
 		}
 	}
 
-	skb = dev_alloc_skb(sizeof(struct can_frame) +
-			    sizeof(struct can_skb_priv));
-	if (!skb)
+	skb = alloc_can_skb(sl->dev, &scf);
+	if (unlikely(!skb))
 		return;
 
-	skb->dev = sl->dev;
-	skb->protocol = htons(ETH_P_CAN);
-	skb->pkt_type = PACKET_BROADCAST;
-	skb->ip_summed = CHECKSUM_UNNECESSARY;
-
-	can_skb_reserve(skb);
-	can_skb_prv(skb)->ifindex = sl->dev->ifindex;
-	can_skb_prv(skb)->skbcnt = 0;
-
-	skb_put_data(skb, &cf, sizeof(struct can_frame));
+	memcpy(scf, &cf, sizeof(*scf));
 
 	sl->dev->stats.rx_packets++;
 	if (!(cf.can_id & CAN_RTR_FLAG))
-- 
2.32.0

Re: [RFC PATCH 03/13] can: slcan: use the alloc_can_skb() helper

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 609 bytes --]

On 07.06.2022 11:47:42, Dario Binacchi wrote:
> It is used successfully by most (if not all) CAN device drivers. It
> allows to remove replicated code.

While you're at it, you can change the function to put the data into the
allocated skb directly instead of first filling the "cf" on the stack
and then doing a memcpy();

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 04/13] can: slcan: use CAN network device driver API

[Dario Binacchi]

As suggested by commit [1], now the driver uses the functions and the
data structures provided by the CAN network device driver interface.

There is no way to set bitrate for SLCAN based devices via ip tool, so
you'll have to do this by slcand/slcan_attach invocation through the
-sX parameter:

- slcan_attach -f -s6 -o /dev/ttyACM0
- slcand -f -s8 -o /dev/ttyUSB0

where -s6 in will set adapter's bitrate to 500 Kbit/s and -s8 to
1Mbit/s.
See the table below for further CAN bitrates:
- s0 ->   10 Kbit/s
- s1 ->   20 Kbit/s
- s2 ->   50 Kbit/s
- s3 ->  100 Kbit/s
- s4 ->  125 Kbit/s
- s5 ->  250 Kbit/s
- s6 ->  500 Kbit/s
- s7 ->  800 Kbit/s
- s8 -> 1000 Kbit/s

In doing so, the struct can_priv::bittiming.bitrate of the driver is not
set and since the open_candev() checks that the bitrate has been set, it
must be a non-zero value, the bitrate is set to a fake value (-1) before
it is called.

[1] 39549eef3587f ("can: CAN Network device driver and Netlink interface")
Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
---

 drivers/net/can/slcan.c | 112 ++++++++++++++++++++--------------------
 1 file changed, 57 insertions(+), 55 deletions(-)

diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index 964b02f321ab..956b47bd40a7 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -56,7 +56,6 @@
 #include <linux/can.h>
 #include <linux/can/dev.h>
 #include <linux/can/skb.h>
-#include <linux/can/can-ml.h>
 
 MODULE_ALIAS_LDISC(N_SLCAN);
 MODULE_DESCRIPTION("serial line CAN interface");
@@ -79,6 +78,7 @@ MODULE_PARM_DESC(maxdev, "Maximum number of slcan interfaces");
 #define SLC_EFF_ID_LEN 8
 
 struct slcan {
+	struct can_priv         can;
 	int			magic;
 
 	/* Various fields. */
@@ -100,6 +100,7 @@ struct slcan {
 };
 
 static struct net_device **slcan_devs;
+static DEFINE_SPINLOCK(slcan_lock);
 
  /************************************************************************
   *			SLCAN ENCAPSULATION FORMAT			 *
@@ -369,7 +370,7 @@ static netdev_tx_t slc_xmit(struct sk_buff *skb, struct net_device *dev)
 	spin_unlock(&sl->lock);
 
 out:
-	kfree_skb(skb);
+	can_put_echo_skb(skb, dev, 0, 0);
 	return NETDEV_TX_OK;
 }
 
@@ -389,6 +390,8 @@ static int slc_close(struct net_device *dev)
 		clear_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags);
 	}
 	netif_stop_queue(dev);
+	close_candev(dev);
+	sl->can.state = CAN_STATE_STOPPED;
 	sl->rcount   = 0;
 	sl->xleft    = 0;
 	spin_unlock_bh(&sl->lock);
@@ -400,21 +403,36 @@ static int slc_close(struct net_device *dev)
 static int slc_open(struct net_device *dev)
 {
 	struct slcan *sl = netdev_priv(dev);
+	int err;
 
 	if (sl->tty == NULL)
 		return -ENODEV;
 
+	/* The baud rate is not set with the command
+	 * `ip link set <iface> type can bitrate <baud>' and therefore
+	 * can.bittiming.bitrate is 0, causing open_candev() to fail.
+	 * So let's set to a fake value.
+	 */
+	sl->can.bittiming.bitrate = -1;
+	err = open_candev(dev);
+	if (err) {
+		netdev_err(dev, "failed to open can device\n");
+		return err;
+	}
+
+	sl->can.state = CAN_STATE_ERROR_ACTIVE;
 	sl->flags &= BIT(SLF_INUSE);
 	netif_start_queue(dev);
 	return 0;
 }
 
-/* Hook the destructor so we can free slcan devs at the right point in time */
-static void slc_free_netdev(struct net_device *dev)
+static void slc_dealloc(struct slcan *sl)
 {
-	int i = dev->base_addr;
+	int i = sl->dev->base_addr;
 
-	slcan_devs[i] = NULL;
+	free_candev(sl->dev);
+	if (slcan_devs)
+		slcan_devs[i] = NULL;
 }
 
 static int slcan_change_mtu(struct net_device *dev, int new_mtu)
@@ -429,24 +447,6 @@ static const struct net_device_ops slc_netdev_ops = {
 	.ndo_change_mtu         = slcan_change_mtu,
 };
 
-static void slc_setup(struct net_device *dev)
-{
-	dev->netdev_ops		= &slc_netdev_ops;
-	dev->needs_free_netdev	= true;
-	dev->priv_destructor	= slc_free_netdev;
-
-	dev->hard_header_len	= 0;
-	dev->addr_len		= 0;
-	dev->tx_queue_len	= 10;
-
-	dev->mtu		= CAN_MTU;
-	dev->type		= ARPHRD_CAN;
-
-	/* New-style flags. */
-	dev->flags		= IFF_NOARP;
-	dev->features           = NETIF_F_HW_CSUM;
-}
-
 /******************************************
   Routines looking at TTY side.
  ******************************************/
@@ -509,11 +509,8 @@ static void slc_sync(void)
 static struct slcan *slc_alloc(void)
 {
 	int i;
-	char name[IFNAMSIZ];
 	struct net_device *dev = NULL;
-	struct can_ml_priv *can_ml;
 	struct slcan       *sl;
-	int size;
 
 	for (i = 0; i < maxdev; i++) {
 		dev = slcan_devs[i];
@@ -526,16 +523,14 @@ static struct slcan *slc_alloc(void)
 	if (i >= maxdev)
 		return NULL;
 
-	sprintf(name, "slcan%d", i);
-	size = ALIGN(sizeof(*sl), NETDEV_ALIGN) + sizeof(struct can_ml_priv);
-	dev = alloc_netdev(size, name, NET_NAME_UNKNOWN, slc_setup);
+	dev = alloc_candev(sizeof(*sl), 1);
 	if (!dev)
 		return NULL;
 
+	snprintf(dev->name, sizeof(dev->name), "slcan%d", i);
+	dev->netdev_ops = &slc_netdev_ops;
 	dev->base_addr  = i;
 	sl = netdev_priv(dev);
-	can_ml = (void *)sl + ALIGN(sizeof(*sl), NETDEV_ALIGN);
-	can_set_ml_priv(dev, can_ml);
 
 	/* Initialize channel control data */
 	sl->magic = SLCAN_MAGIC;
@@ -568,11 +563,7 @@ static int slcan_open(struct tty_struct *tty)
 	if (tty->ops->write == NULL)
 		return -EOPNOTSUPP;
 
-	/* RTnetlink lock is misused here to serialize concurrent
-	   opens of slcan channels. There are better ways, but it is
-	   the simplest one.
-	 */
-	rtnl_lock();
+	spin_lock(&slcan_lock);
 
 	/* Collect hanged up channels. */
 	slc_sync();
@@ -600,13 +591,15 @@ static int slcan_open(struct tty_struct *tty)
 
 		set_bit(SLF_INUSE, &sl->flags);
 
-		err = register_netdevice(sl->dev);
-		if (err)
+		err = register_candev(sl->dev);
+		if (err) {
+			pr_err("slcan: can't register candev\n");
 			goto err_free_chan;
+		}
 	}
 
 	/* Done.  We have linked the TTY line to a channel. */
-	rtnl_unlock();
+	spin_unlock(&slcan_lock);
 	tty->receive_room = 65536;	/* We don't flow control */
 
 	/* TTY layer expects 0 on success */
@@ -616,14 +609,10 @@ static int slcan_open(struct tty_struct *tty)
 	sl->tty = NULL;
 	tty->disc_data = NULL;
 	clear_bit(SLF_INUSE, &sl->flags);
-	slc_free_netdev(sl->dev);
-	/* do not call free_netdev before rtnl_unlock */
-	rtnl_unlock();
-	free_netdev(sl->dev);
-	return err;
+	slc_dealloc(sl);
 
 err_exit:
-	rtnl_unlock();
+	spin_unlock(&slcan_lock);
 
 	/* Count references from TTY module */
 	return err;
@@ -653,9 +642,11 @@ static void slcan_close(struct tty_struct *tty)
 	synchronize_rcu();
 	flush_work(&sl->tx_work);
 
-	/* Flush network side */
-	unregister_netdev(sl->dev);
-	/* This will complete via sl_free_netdev */
+	slc_close(sl->dev);
+	unregister_candev(sl->dev);
+	spin_lock(&slcan_lock);
+	slc_dealloc(sl);
+	spin_unlock(&slcan_lock);
 }
 
 static void slcan_hangup(struct tty_struct *tty)
@@ -763,18 +754,29 @@ static void __exit slcan_exit(void)
 		dev = slcan_devs[i];
 		if (!dev)
 			continue;
-		slcan_devs[i] = NULL;
 
-		sl = netdev_priv(dev);
-		if (sl->tty) {
-			netdev_err(dev, "tty discipline still running\n");
-		}
+		spin_lock(&slcan_lock);
+		dev = slcan_devs[i];
+		if (dev) {
+			slcan_devs[i] = NULL;
+			spin_unlock(&slcan_lock);
+			sl = netdev_priv(dev);
+			if (sl->tty) {
+				netdev_err(dev,
+					   "tty discipline still running\n");
+			}
 
-		unregister_netdev(dev);
+			slc_close(dev);
+			unregister_candev(dev);
+		} else {
+			spin_unlock(&slcan_lock);
+		}
 	}
 
+	spin_lock(&slcan_lock);
 	kfree(slcan_devs);
 	slcan_devs = NULL;
+	spin_unlock(&slcan_lock);
 
 	tty_unregister_ldisc(&slc_ldisc);
 }
-- 
2.32.0

Re: [RFC PATCH 04/13] can: slcan: use CAN network device driver API

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1447 bytes --]

On 07.06.2022 11:47:43, Dario Binacchi wrote:
> As suggested by commit [1], now the driver uses the functions and the
> data structures provided by the CAN network device driver interface.
> 
> There is no way to set bitrate for SLCAN based devices via ip tool, so
  ^^^^^^^^^^^^^^^
Currently the driver doesn't implement a way

> you'll have to do this by slcand/slcan_attach invocation through the
> -sX parameter:
> 
> - slcan_attach -f -s6 -o /dev/ttyACM0
> - slcand -f -s8 -o /dev/ttyUSB0
> 
> where -s6 in will set adapter's bitrate to 500 Kbit/s and -s8 to
> 1Mbit/s.
> See the table below for further CAN bitrates:
> - s0 ->   10 Kbit/s
> - s1 ->   20 Kbit/s
> - s2 ->   50 Kbit/s
> - s3 ->  100 Kbit/s
> - s4 ->  125 Kbit/s
> - s5 ->  250 Kbit/s
> - s6 ->  500 Kbit/s
> - s7 ->  800 Kbit/s
> - s8 -> 1000 Kbit/s
> 
> In doing so, the struct can_priv::bittiming.bitrate of the driver is not
> set and since the open_candev() checks that the bitrate has been set, it
> must be a non-zero value, the bitrate is set to a fake value (-1) before
> it is called.

What does

| ip --details -s -s link show

show as the bit rate?

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 04/13] can: slcan: use CAN network device driver API

[Dario Binacchi]

Hi Marc,

On Tue, Jun 7, 2022 at 1:13 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 07.06.2022 11:47:43, Dario Binacchi wrote:
> > As suggested by commit [1], now the driver uses the functions and the
> > data structures provided by the CAN network device driver interface.
> >
> > There is no way to set bitrate for SLCAN based devices via ip tool, so
>   ^^^^^^^^^^^^^^^
> Currently the driver doesn't implement a way

Ok, I'll do it.

>
> > you'll have to do this by slcand/slcan_attach invocation through the
> > -sX parameter:
> >
> > - slcan_attach -f -s6 -o /dev/ttyACM0
> > - slcand -f -s8 -o /dev/ttyUSB0
> >
> > where -s6 in will set adapter's bitrate to 500 Kbit/s and -s8 to
> > 1Mbit/s.
> > See the table below for further CAN bitrates:
> > - s0 ->   10 Kbit/s
> > - s1 ->   20 Kbit/s
> > - s2 ->   50 Kbit/s
> > - s3 ->  100 Kbit/s
> > - s4 ->  125 Kbit/s
> > - s5 ->  250 Kbit/s
> > - s6 ->  500 Kbit/s
> > - s7 ->  800 Kbit/s
> > - s8 -> 1000 Kbit/s
> >
> > In doing so, the struct can_priv::bittiming.bitrate of the driver is not
> > set and since the open_candev() checks that the bitrate has been set, it
> > must be a non-zero value, the bitrate is set to a fake value (-1) before
> > it is called.
>
> What does
>
> | ip --details -s -s link show
>
> show as the bit rate?

# ip --details -s -s link show dev can0
 can0: <NOARP,UP,LOWER_UP> mtu 16 qdisc pfifo_fast state UP mode
DEFAULT group default qlen 10
    link/can  promiscuity 0 minmtu 0 maxmtu 0
    can state ERROR-ACTIVE restart-ms 0
  bitrate 500000 sample-point 0.875
  tq 41 prop-seg 20 phase-seg1 21 phase-seg2 6 sjw 1
  slcan: tseg1 2..256 tseg2 1..128 sjw 1..128 brp 1..256 brp-inc 1
  clock 24000000
  re-started bus-errors arbit-lost error-warn error-pass bus-off
  0          0          0          0          0          0
numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535
    RX: bytes  packets  errors  dropped overrun mcast
    292        75       0       0       0       0
    RX errors: length   crc     frame   fifo    missed
               0        0       0       0       0
    TX: bytes  packets  errors  dropped carrier collsns
    0          0        0       0       0       0
    TX errors: aborted  fifo   window heartbeat transns
               0        0       0       0       1

And after applying your suggestions about using the CAN framework
support for setting the fixed bit rates (you'll
find it in V2), this is the output instead:

# ip --details -s -s link show dev can0
5: can0: <NOARP,UP,LOWER_UP> mtu 16 qdisc pfifo_fast state UP mode
DEFAULT group default qlen 10
    link/can  promiscuity 0 minmtu 0 maxmtu 0
    can state ERROR-ACTIVE restart-ms 0
  bitrate 500000
     [   10000,    20000,    50000,   100000,   125000,   250000,
        500000,   800000,  1000000 ]
  clock 0
  re-started bus-errors arbit-lost error-warn error-pass bus-off
  0          0          0          0          0          0
numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535
    RX: bytes  packets  errors  dropped overrun mcast
    37307      4789     0       0       0       0
    RX errors: length   crc     frame   fifo    missed
               0        0       0       0       0
    TX: bytes  packets  errors  dropped carrier collsns
    7276       988      0       0       0       0
    TX errors: aborted  fifo   window heartbeat transns
               0        0       0       0       1

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 |



-- 

Dario Binacchi

Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@amarulasolutions.com

www.amarulasolutions.com

Re: [RFC PATCH 04/13] can: slcan: use CAN network device driver API

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3258 bytes --]

On 08.06.2022 18:42:09, Dario Binacchi wrote:
> > > In doing so, the struct can_priv::bittiming.bitrate of the driver is not
> > > set and since the open_candev() checks that the bitrate has been set, it
> > > must be a non-zero value, the bitrate is set to a fake value (-1) before
> > > it is called.
> >
> > What does
> >
> > | ip --details -s -s link show
> >
> > show as the bit rate?
> 
> # ip --details -s -s link show dev can0

This is the bitrate configured with "ip"?

>  can0: <NOARP,UP,LOWER_UP> mtu 16 qdisc pfifo_fast state UP mode
> DEFAULT group default qlen 10
>     link/can  promiscuity 0 minmtu 0 maxmtu 0
>     can state ERROR-ACTIVE restart-ms 0
>   bitrate 500000 sample-point 0.875
>   tq 41 prop-seg 20 phase-seg1 21 phase-seg2 6 sjw 1
>   slcan: tseg1 2..256 tseg2 1..128 sjw 1..128 brp 1..256 brp-inc 1
>   clock 24000000
>   re-started bus-errors arbit-lost error-warn error-pass bus-off
>   0          0          0          0          0          0
> numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535
>     RX: bytes  packets  errors  dropped overrun mcast
>     292        75       0       0       0       0
>     RX errors: length   crc     frame   fifo    missed
>                0        0       0       0       0
>     TX: bytes  packets  errors  dropped carrier collsns
>     0          0        0       0       0       0
>     TX errors: aborted  fifo   window heartbeat transns
>                0        0       0       0       1
> 
> And after applying your suggestions about using the CAN framework
> support for setting the fixed bit rates (you'll
> find it in V2), this is the output instead:

This looks good.

> # ip --details -s -s link show dev can0
> 5: can0: <NOARP,UP,LOWER_UP> mtu 16 qdisc pfifo_fast state UP mode
> DEFAULT group default qlen 10
>     link/can  promiscuity 0 minmtu 0 maxmtu 0
>     can state ERROR-ACTIVE restart-ms 0
>   bitrate 500000
>      [   10000,    20000,    50000,   100000,   125000,   250000,
>         500000,   800000,  1000000 ]
>   clock 0
>   re-started bus-errors arbit-lost error-warn error-pass bus-off
>   0          0          0          0          0          0
> numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535
>     RX: bytes  packets  errors  dropped overrun mcast
>     37307      4789     0       0       0       0
>     RX errors: length   crc     frame   fifo    missed
>                0        0       0       0       0
>     TX: bytes  packets  errors  dropped carrier collsns
>     7276       988      0       0       0       0
>     TX errors: aborted  fifo   window heartbeat transns
>                0        0       0       0       1

Can you configure the bitrate with slcand and show the output of "ip
--details -s -s link show dev can0". I fear it will show 4294967295 as
the bitrate, which I don't like.

A hack would be to replace the -1 by 0 in the CAN netlink code.

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 04/13] can: slcan: use CAN network device driver API

[Dario Binacchi]

On Thu, Jun 9, 2022 at 9:07 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 08.06.2022 18:42:09, Dario Binacchi wrote:
> > > > In doing so, the struct can_priv::bittiming.bitrate of the driver is not
> > > > set and since the open_candev() checks that the bitrate has been set, it
> > > > must be a non-zero value, the bitrate is set to a fake value (-1) before
> > > > it is called.
> > >
> > > What does
> > >
> > > | ip --details -s -s link show
> > >
> > > show as the bit rate?
> >
> > # ip --details -s -s link show dev can0
>
> This is the bitrate configured with "ip"?
>
> >  can0: <NOARP,UP,LOWER_UP> mtu 16 qdisc pfifo_fast state UP mode
> > DEFAULT group default qlen 10
> >     link/can  promiscuity 0 minmtu 0 maxmtu 0
> >     can state ERROR-ACTIVE restart-ms 0
> >   bitrate 500000 sample-point 0.875
> >   tq 41 prop-seg 20 phase-seg1 21 phase-seg2 6 sjw 1
> >   slcan: tseg1 2..256 tseg2 1..128 sjw 1..128 brp 1..256 brp-inc 1
> >   clock 24000000
> >   re-started bus-errors arbit-lost error-warn error-pass bus-off
> >   0          0          0          0          0          0
> > numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535
> >     RX: bytes  packets  errors  dropped overrun mcast
> >     292        75       0       0       0       0
> >     RX errors: length   crc     frame   fifo    missed
> >                0        0       0       0       0
> >     TX: bytes  packets  errors  dropped carrier collsns
> >     0          0        0       0       0       0
> >     TX errors: aborted  fifo   window heartbeat transns
> >                0        0       0       0       1
> >
> > And after applying your suggestions about using the CAN framework
> > support for setting the fixed bit rates (you'll
> > find it in V2), this is the output instead:
>
> This looks good.
>
> > # ip --details -s -s link show dev can0
> > 5: can0: <NOARP,UP,LOWER_UP> mtu 16 qdisc pfifo_fast state UP mode
> > DEFAULT group default qlen 10
> >     link/can  promiscuity 0 minmtu 0 maxmtu 0
> >     can state ERROR-ACTIVE restart-ms 0
> >   bitrate 500000
> >      [   10000,    20000,    50000,   100000,   125000,   250000,
> >         500000,   800000,  1000000 ]
> >   clock 0
> >   re-started bus-errors arbit-lost error-warn error-pass bus-off
> >   0          0          0          0          0          0
> > numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535
> >     RX: bytes  packets  errors  dropped overrun mcast
> >     37307      4789     0       0       0       0
> >     RX errors: length   crc     frame   fifo    missed
> >                0        0       0       0       0
> >     TX: bytes  packets  errors  dropped carrier collsns
> >     7276       988      0       0       0       0
> >     TX errors: aborted  fifo   window heartbeat transns
> >                0        0       0       0       1
>
> Can you configure the bitrate with slcand and show the output of "ip
> --details -s -s link show dev can0". I fear it will show 4294967295 as
> the bitrate, which I don't like.
>

Yes, you are right.

> A hack would be to replace the -1 by 0 in the CAN netlink code.

You will find it in V3.

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 |



-- 

Dario Binacchi

Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@amarulasolutions.com

www.amarulasolutions.com

[RFC PATCH 05/13] can: slcan: simplify the device de-allocation

[Dario Binacchi]

Since slcan_devs array contains the addresses of the created devices, I
think it is more natural to use its address to remove it from the list.
It is not necessary to store the index of the array that points to the
device in the driver's private data.

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

 drivers/net/can/slcan.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index 956b47bd40a7..4df0455e11a2 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -428,11 +428,17 @@ static int slc_open(struct net_device *dev)
 
 static void slc_dealloc(struct slcan *sl)
 {
-	int i = sl->dev->base_addr;
+	unsigned int i;
 
-	free_candev(sl->dev);
-	if (slcan_devs)
-		slcan_devs[i] = NULL;
+	for (i = 0; i < maxdev; i++) {
+		if (sl->dev == slcan_devs[i]) {
+			free_candev(sl->dev);
+			slcan_devs[i] = NULL;
+			return;
+		}
+	}
+
+	pr_err("slcan: can't free %s resources\n",  sl->dev->name);
 }
 
 static int slcan_change_mtu(struct net_device *dev, int new_mtu)
@@ -529,7 +535,6 @@ static struct slcan *slc_alloc(void)
 
 	snprintf(dev->name, sizeof(dev->name), "slcan%d", i);
 	dev->netdev_ops = &slc_netdev_ops;
-	dev->base_addr  = i;
 	sl = netdev_priv(dev);
 
 	/* Initialize channel control data */
-- 
2.32.0

Re: [RFC PATCH 05/13] can: slcan: simplify the device de-allocation

[Oliver Hartkopp]

On 07.06.22 11:47, Dario Binacchi wrote:
> Since slcan_devs array contains the addresses of the created devices, I
> think it is more natural to use its address to remove it from the list.
> It is not necessary to store the index of the array that points to the
> device in the driver's private data.

IMO this patch should not be part of the slcan enhancement series.

I can see the "miss-use" of dev->base_addr but when we change this code 
we should also take care of a similar handling in drivers/net/slip/slip.c

Therefore a change like this should be done in slcan.c and slip.c 
simultaneously with a single patch.

Best regards,
Oliver

> 
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> ---
> 
>   drivers/net/can/slcan.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
> index 956b47bd40a7..4df0455e11a2 100644
> --- a/drivers/net/can/slcan.c
> +++ b/drivers/net/can/slcan.c
> @@ -428,11 +428,17 @@ static int slc_open(struct net_device *dev)
>   
>   static void slc_dealloc(struct slcan *sl)
>   {
> -	int i = sl->dev->base_addr;
> +	unsigned int i;
>   
> -	free_candev(sl->dev);
> -	if (slcan_devs)
> -		slcan_devs[i] = NULL;
> +	for (i = 0; i < maxdev; i++) {
> +		if (sl->dev == slcan_devs[i]) {
> +			free_candev(sl->dev);
> +			slcan_devs[i] = NULL;
> +			return;
> +		}
> +	}
> +
> +	pr_err("slcan: can't free %s resources\n",  sl->dev->name);
>   }
>   
>   static int slcan_change_mtu(struct net_device *dev, int new_mtu)
> @@ -529,7 +535,6 @@ static struct slcan *slc_alloc(void)
>   
>   	snprintf(dev->name, sizeof(dev->name), "slcan%d", i);
>   	dev->netdev_ops = &slc_netdev_ops;
> -	dev->base_addr  = i;
>   	sl = netdev_priv(dev);
>   
>   	/* Initialize channel control data */
> 

[RFC PATCH 06/13] can: slcan: allow to send commands to the adapter

[Dario Binacchi]

This is a preparation patch for the upcoming support to change the
bitrate via ip tool, reset the adapter error states via the ethtool API
and, more generally, send commands to the adapter.

Since some commands (e. g. setting the bitrate) will be sent before
calling the open_candev(), the netif_running() will return false and so
a new flag bit (i. e. SLF_XCMD) for serial transmission has to be added.

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

 drivers/net/can/slcan.c | 46 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index 4df0455e11a2..dbd4ebdfa024 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -97,6 +97,9 @@ struct slcan {
 	unsigned long		flags;		/* Flag values/ mode etc     */
 #define SLF_INUSE		0		/* Channel in use            */
 #define SLF_ERROR		1               /* Parity, etc. error        */
+#define SLF_XCMD		2               /* Command transmission      */
+	wait_queue_head_t       xcmd_wait;      /* Wait queue for commands   */
+						/* transmission              */
 };
 
 static struct net_device **slcan_devs;
@@ -310,12 +313,22 @@ static void slcan_transmit(struct work_struct *work)
 
 	spin_lock_bh(&sl->lock);
 	/* First make sure we're connected. */
-	if (!sl->tty || sl->magic != SLCAN_MAGIC || !netif_running(sl->dev)) {
+	if (!sl->tty || sl->magic != SLCAN_MAGIC ||
+	    (unlikely(!netif_running(sl->dev)) &&
+	     likely(!test_bit(SLF_XCMD, &sl->flags)))) {
 		spin_unlock_bh(&sl->lock);
 		return;
 	}
 
 	if (sl->xleft <= 0)  {
+		if (unlikely(test_bit(SLF_XCMD, &sl->flags))) {
+			clear_bit(SLF_XCMD, &sl->flags);
+			clear_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags);
+			spin_unlock_bh(&sl->lock);
+			wake_up(&sl->xcmd_wait);
+			return;
+		}
+
 		/* Now serial buffer is almost free & we can start
 		 * transmission of another packet */
 		sl->dev->stats.tx_packets++;
@@ -379,6 +392,36 @@ static netdev_tx_t slc_xmit(struct sk_buff *skb, struct net_device *dev)
  *   Routines looking at netdevice side.
  ******************************************/
 
+static int slcan_transmit_cmd(struct slcan *sl, const unsigned char *cmd)
+{
+	int ret, actual, n;
+
+	spin_lock(&sl->lock);
+	if (sl->tty == NULL) {
+		spin_unlock(&sl->lock);
+		return -ENODEV;
+	}
+
+	n = snprintf(sl->xbuff, sizeof(sl->xbuff), "%s", cmd);
+	set_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags);
+	actual = sl->tty->ops->write(sl->tty, sl->xbuff, n);
+	sl->xleft = n - actual;
+	sl->xhead = sl->xbuff + actual;
+	set_bit(SLF_XCMD, &sl->flags);
+	spin_unlock(&sl->lock);
+	ret = wait_event_interruptible_timeout(sl->xcmd_wait,
+					       !test_bit(SLF_XCMD, &sl->flags),
+					       HZ);
+	clear_bit(SLF_XCMD, &sl->flags);
+	if (ret == -ERESTARTSYS)
+		return ret;
+
+	if (ret == 0)
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
 /* Netdevice UP -> DOWN routine */
 static int slc_close(struct net_device *dev)
 {
@@ -542,6 +585,7 @@ static struct slcan *slc_alloc(void)
 	sl->dev	= dev;
 	spin_lock_init(&sl->lock);
 	INIT_WORK(&sl->tx_work, slcan_transmit);
+	init_waitqueue_head(&sl->xcmd_wait);
 	slcan_devs[i] = dev;
 
 	return sl;
-- 
2.32.0

Re: [RFC PATCH 06/13] can: slcan: allow to send commands to the adapter

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]

On 07.06.2022 11:47:45, Dario Binacchi wrote:
> This is a preparation patch for the upcoming support to change the
> bitrate via ip tool, reset the adapter error states via the ethtool API
> and, more generally, send commands to the adapter.
> 
> Since some commands (e. g. setting the bitrate) will be sent before
> calling the open_candev(), the netif_running() will return false and so
> a new flag bit (i. e. SLF_XCMD) for serial transmission has to be added.
> 
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

I think this patch can be dropped, let me explain:

You don't have to implement the do_set_bittiming callback. It's
perfectly OK to set the bitrate during the ndo_open callback after
open_candev().

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 06/13] can: slcan: allow to send commands to the adapter

[Dario Binacchi]

Hi Marc,

On Thu, Jun 9, 2022 at 9:16 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 07.06.2022 11:47:45, Dario Binacchi wrote:
> > This is a preparation patch for the upcoming support to change the
> > bitrate via ip tool, reset the adapter error states via the ethtool API
> > and, more generally, send commands to the adapter.
> >
> > Since some commands (e. g. setting the bitrate) will be sent before
> > calling the open_candev(), the netif_running() will return false and so
> > a new flag bit (i. e. SLF_XCMD) for serial transmission has to be added.
> >
> > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>
> I think this patch can be dropped, let me explain:
>
> You don't have to implement the do_set_bittiming callback. It's
> perfectly OK to set the bitrate during the ndo_open callback after
> open_candev().

I developed what you suggested (i. e. remove the SLF_XCMD bit and set the
bitrate, as well as send the "F\r" and "O\r" commands, after calling
the open_candev(),
but now I can't send the close command ("C\r") during the ndo_stop() since
netif_running() returns false. The CAN framework clears netif_running() before
calling the ndo_stop(). IMHO the SLF_XCMD bit then becomes necessary to
transmit the commands to the adapter from the ndo_open() and
ndo_stop() routines.
Any suggestions ?

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 |



-- 

Dario Binacchi

Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@amarulasolutions.com

www.amarulasolutions.com

Re: [RFC PATCH 06/13] can: slcan: allow to send commands to the adapter

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2216 bytes --]

On 11.06.2022 23:43:47, Dario Binacchi wrote:
> Hi Marc,
> 
> On Thu, Jun 9, 2022 at 9:16 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >
> > On 07.06.2022 11:47:45, Dario Binacchi wrote:
> > > This is a preparation patch for the upcoming support to change the
> > > bitrate via ip tool, reset the adapter error states via the ethtool API
> > > and, more generally, send commands to the adapter.
> > >
> > > Since some commands (e. g. setting the bitrate) will be sent before
> > > calling the open_candev(), the netif_running() will return false and so
> > > a new flag bit (i. e. SLF_XCMD) for serial transmission has to be added.
> > >
> > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> >
> > I think this patch can be dropped, let me explain:
> >
> > You don't have to implement the do_set_bittiming callback. It's
> > perfectly OK to set the bitrate during the ndo_open callback after
> > open_candev().
> 
> I developed what you suggested (i. e. remove the SLF_XCMD bit and set the
> bitrate, as well as send the "F\r" and "O\r" commands, after calling
> the open_candev(),

Note:
Max Staudt noticed that you need a do_set_bittiming() callback if you
have only fixed bitrates. I've create a patch that you can have fixed
bit rate only an no do_set_bittiming() callback.

| https://lore.kernel.org/all/20220611144248.3924903-1-mkl@pengutronix.de/

Or use the can-next/master branch as your base.

> but now I can't send the close command ("C\r") during the ndo_stop() since
> netif_running() returns false. The CAN framework clears netif_running() before
> calling the ndo_stop(). IMHO the SLF_XCMD bit then becomes necessary to
> transmit the commands to the adapter from the ndo_open() and
> ndo_stop() routines.
> Any suggestions ?

I see. Keep the setting of the bit rate in the ndo_open(), add the
SLF_XCMD so that you can send messages in ndo_close().

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 07/13] can: slcan: set bitrate by CAN device driver API

[Dario Binacchi]

It allows to set the bitrate via ip tool, as it happens for the other
CAN device drivers. It still remains possible to set the bitrate via
slcand or slcan_attach utilities. In case the ip tool is used, the
driver will send the serial command to the adapter.

The struct can_bittiming_const and struct can_priv::clock.freq has been
set with empirical values ​​that allow you to get a correct bit timing, so
that the slc_do_set_bittiming() can be called.

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

---
DTS properties could be used to set the can.clock.freq and the
can.bittiming_const variables. This way the parameters could be changed
based on the type of the adapter.

 drivers/net/can/slcan.c | 54 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index dbd4ebdfa024..f1bf32b70c4d 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -105,6 +105,18 @@ struct slcan {
 static struct net_device **slcan_devs;
 static DEFINE_SPINLOCK(slcan_lock);
 
+static const struct can_bittiming_const slcan_bittiming_const = {
+	.name = KBUILD_MODNAME,
+	.tseg1_min = 2,
+	.tseg1_max = 256,
+	.tseg2_min = 1,
+	.tseg2_max = 128,
+	.sjw_max = 128,
+	.brp_min = 1,
+	.brp_max = 256,
+	.brp_inc = 1,
+};
+
  /************************************************************************
   *			SLCAN ENCAPSULATION FORMAT			 *
   ************************************************************************/
@@ -435,6 +447,7 @@ static int slc_close(struct net_device *dev)
 	netif_stop_queue(dev);
 	close_candev(dev);
 	sl->can.state = CAN_STATE_STOPPED;
+	sl->can.bittiming.bitrate = 0;
 	sl->rcount   = 0;
 	sl->xleft    = 0;
 	spin_unlock_bh(&sl->lock);
@@ -456,7 +469,9 @@ static int slc_open(struct net_device *dev)
 	 * can.bittiming.bitrate is 0, causing open_candev() to fail.
 	 * So let's set to a fake value.
 	 */
-	sl->can.bittiming.bitrate = -1;
+	if (sl->can.bittiming.bitrate == 0)
+		sl->can.bittiming.bitrate = -1UL;
+
 	err = open_candev(dev);
 	if (err) {
 		netdev_err(dev, "failed to open can device\n");
@@ -554,6 +569,40 @@ static void slc_sync(void)
 	}
 }
 
+static int slc_do_set_bittiming(struct net_device *dev)
+{
+	struct slcan *sl = netdev_priv(dev);
+	unsigned char cmd[SLC_MTU];
+	int i, s = -1, err;
+	unsigned int bitrates[] = {
+		10000, 20000, 50000, 100000,
+		125000, 250000, 500000, 800000,
+		1000000,
+	};
+
+	for (i = 0; i < ARRAY_SIZE(bitrates); i++) {
+		if (sl->can.bittiming.bitrate == bitrates[i]) {
+			s = i;
+			break;
+		}
+	}
+
+	if (s < 0) {
+		netdev_err(dev, "invalid bitrate\n");
+		return -EINVAL;
+	}
+
+	snprintf(cmd, sizeof(cmd), "C\rS%d\r", s);
+	err = slcan_transmit_cmd(sl, cmd);
+	if (err) {
+		sl->can.bittiming.bitrate = 0;
+		netdev_err(sl->dev,
+			   "failed to send bitrate command 'C\\rS%d\\r'\n", s);
+	}
+
+	return err;
+}
+
 /* Find a free SLCAN channel, and link in this `tty' line. */
 static struct slcan *slc_alloc(void)
 {
@@ -583,6 +632,9 @@ static struct slcan *slc_alloc(void)
 	/* Initialize channel control data */
 	sl->magic = SLCAN_MAGIC;
 	sl->dev	= dev;
+	sl->can.clock.freq = 24 * 1000 * 1000;
+	sl->can.bittiming_const = &slcan_bittiming_const;
+	sl->can.do_set_bittiming = slc_do_set_bittiming;
 	spin_lock_init(&sl->lock);
 	INIT_WORK(&sl->tx_work, slcan_transmit);
 	init_waitqueue_head(&sl->xcmd_wait);
-- 
2.32.0

Re: [RFC PATCH 07/13] can: slcan: set bitrate by CAN device driver API

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 970 bytes --]

On 07.06.2022 11:47:46, Dario Binacchi wrote:
> It allows to set the bitrate via ip tool, as it happens for the other
> CAN device drivers. It still remains possible to set the bitrate via
> slcand or slcan_attach utilities. In case the ip tool is used, the
> driver will send the serial command to the adapter.
> 
> The struct can_bittiming_const and struct can_priv::clock.freq has been
> set with empirical values ​​that allow you to get a correct bit timing, so
> that the slc_do_set_bittiming() can be called.

The CAN framework supports setting of fixed bit rates. Look for
can327_bitrate_const in

| https://lore.kernel.org/all/20220602213544.68273-1-max@enpas.org/

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 08/13] can: slcan: send the open command to the adapter

[Dario Binacchi]

In case the bitrate has been set via ip tool, it sends the open command
("O\r") to the adapter.

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

 drivers/net/can/slcan.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index f1bf32b70c4d..f18097c62222 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -469,8 +469,15 @@ static int slc_open(struct net_device *dev)
 	 * can.bittiming.bitrate is 0, causing open_candev() to fail.
 	 * So let's set to a fake value.
 	 */
-	if (sl->can.bittiming.bitrate == 0)
+	if (sl->can.bittiming.bitrate == 0) {
 		sl->can.bittiming.bitrate = -1UL;
+	} else {
+		err = slcan_transmit_cmd(sl, "O\r");
+		if (err) {
+			netdev_err(dev, "failed to send open command 'O\\r'\n");
+			return err;
+		}
+	}
 
 	err = open_candev(dev);
 	if (err) {
-- 
2.32.0

Re: [RFC PATCH 08/13] can: slcan: send the open command to the adapter

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 535 bytes --]

On 07.06.2022 11:47:47, Dario Binacchi wrote:
> In case the bitrate has been set via ip tool, it sends the open command
                                                ^^^^^^^^

...this patch changes the driver to send...

> ("O\r") to the adapter.

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 09/13] can: slcan: send the close command to the adapter

[Dario Binacchi]

In case the bitrate has been set via ip tool, it sends the close command
("C\r") to the adapter.

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

 drivers/net/can/slcan.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index f18097c62222..d63d270d21da 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -438,9 +438,20 @@ static int slcan_transmit_cmd(struct slcan *sl, const unsigned char *cmd)
 static int slc_close(struct net_device *dev)
 {
 	struct slcan *sl = netdev_priv(dev);
+	int err;
 
 	spin_lock_bh(&sl->lock);
 	if (sl->tty) {
+		if (sl->can.bittiming.bitrate &&
+		    sl->can.bittiming.bitrate != -1) {
+			spin_unlock_bh(&sl->lock);
+			err = slcan_transmit_cmd(sl, "C\r");
+			spin_lock_bh(&sl->lock);
+			if (err)
+				netdev_warn(dev,
+					    "failed to send close command 'C\\r'\n");
+		}
+
 		/* TTY discipline is running. */
 		clear_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags);
 	}
-- 
2.32.0

[RFC PATCH 10/13] can: slcan: move driver into separate sub directory

[Dario Binacchi]

This patch moves the slcan driver into a separate directory, a later
patch will add more files.

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

 drivers/net/can/Makefile                        | 2 +-
 drivers/net/can/slcan/Makefile                  | 6 ++++++
 drivers/net/can/{slcan.c => slcan/slcan-core.c} | 0
 3 files changed, 7 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/can/slcan/Makefile
 rename drivers/net/can/{slcan.c => slcan/slcan-core.c} (100%)

diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
index 0af85983634c..210354df273c 100644
--- a/drivers/net/can/Makefile
+++ b/drivers/net/can/Makefile
@@ -5,7 +5,7 @@
 
 obj-$(CONFIG_CAN_VCAN)		+= vcan.o
 obj-$(CONFIG_CAN_VXCAN)		+= vxcan.o
-obj-$(CONFIG_CAN_SLCAN)		+= slcan.o
+obj-$(CONFIG_CAN_SLCAN)		+= slcan/
 
 obj-y				+= dev/
 obj-y				+= rcar/
diff --git a/drivers/net/can/slcan/Makefile b/drivers/net/can/slcan/Makefile
new file mode 100644
index 000000000000..2e84f7bf7617
--- /dev/null
+++ b/drivers/net/can/slcan/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_CAN_SLCAN) += slcan.o
+
+slcan-objs :=
+slcan-objs += slcan-core.o
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan/slcan-core.c
similarity index 100%
rename from drivers/net/can/slcan.c
rename to drivers/net/can/slcan/slcan-core.c
-- 
2.32.0

[RFC PATCH 11/13] can: slcan: add ethtool support to reset adapter errors

[Dario Binacchi]

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

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 */
-- 
2.32.0

Re: [RFC PATCH 11/13] can: slcan: add ethtool support to reset adapter errors

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1030 bytes --]

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

-- 
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 11/13] can: slcan: add ethtool support to reset adapter errors

[Dario Binacchi]

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 |



-- 

Dario Binacchi

Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@amarulasolutions.com

www.amarulasolutions.com

Re: [RFC PATCH 11/13] can: slcan: add ethtool support to reset adapter errors

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]

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

-- 
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 11/13] can: slcan: add ethtool support to reset adapter errors

[Dario Binacchi]

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 |



-- 

Dario Binacchi

Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@amarulasolutions.com

www.amarulasolutions.com

Re: [RFC PATCH 11/13] can: slcan: add ethtool support to reset adapter errors

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2492 bytes --]

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

-- 
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 11/13] can: slcan: add ethtool support to reset adapter errors

[Dario Binacchi]

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 |



-- 

Dario Binacchi

Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@amarulasolutions.com

www.amarulasolutions.com

Re: [RFC PATCH 11/13] can: slcan: add ethtool support to reset adapter errors

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1286 bytes --]

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

-- 
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 12/13] can: slcan: extend the protocol with error info

[Dario Binacchi]

It extends the protocol to receive the adapter CAN communication errors
and forward them to the netdev upper levels.

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

 drivers/net/can/slcan/slcan-core.c | 104 ++++++++++++++++++++++++++++-
 1 file changed, 103 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
index b813a59534a3..02e7c14de45c 100644
--- a/drivers/net/can/slcan/slcan-core.c
+++ b/drivers/net/can/slcan/slcan-core.c
@@ -182,8 +182,92 @@ int slcan_enable_err_rst_on_open(struct net_device *ndev, bool on)
   *			STANDARD SLCAN DECAPSULATION			 *
   ************************************************************************/
 
+static void slc_bump_err(struct slcan *sl)
+{
+	struct net_device *dev = sl->dev;
+	struct sk_buff *skb;
+	struct can_frame *cf;
+	char *cmd = sl->rbuff;
+	bool rx_errors = false, tx_errors = false;
+	int i, len;
+
+	if (*cmd != 'e')
+		return;
+
+	cmd += SLC_CMD_LEN;
+	/* get len from sanitized ASCII value */
+	len = *cmd++;
+	if (len >= '0' && len < '9')
+		len -= '0';
+	else
+		return;
+
+	skb = alloc_can_err_skb(dev, &cf);
+	if (unlikely(!skb))
+		return;
+
+	cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+	for (i = 0; i < len; i++, cmd++) {
+		switch (*cmd) {
+		case 'a':
+			netdev_dbg(dev, "ACK error\n");
+			cf->can_id |= CAN_ERR_ACK;
+			cf->data[3] = CAN_ERR_PROT_LOC_ACK;
+			tx_errors = true;
+			break;
+		case 'b':
+			netdev_dbg(dev, "Bit0 error\n");
+			cf->data[2] |= CAN_ERR_PROT_BIT0;
+			tx_errors = true;
+			break;
+		case 'B':
+			netdev_dbg(dev, "Bit1 error\n");
+			cf->data[2] |= CAN_ERR_PROT_BIT1;
+			tx_errors = true;
+			break;
+		case 'c':
+			netdev_dbg(dev, "CRC error\n");
+			cf->data[2] |= CAN_ERR_PROT_BIT;
+			cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;
+			rx_errors = true;
+			break;
+		case 'f':
+			netdev_dbg(dev, "Form Error\n");
+			cf->data[2] |= CAN_ERR_PROT_FORM;
+			rx_errors = true;
+			break;
+		case 'o':
+			netdev_dbg(dev, "Rx overrun error\n");
+			cf->can_id |= CAN_ERR_CRTL;
+			cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
+			dev->stats.rx_over_errors++;
+			dev->stats.rx_errors++;
+			break;
+		case 'O':
+			netdev_dbg(dev, "Tx overrun error\n");
+			cf->can_id |= CAN_ERR_CRTL;
+			cf->data[1] = CAN_ERR_CRTL_TX_OVERFLOW;
+			dev->stats.tx_errors++;
+			break;
+		case 's':
+			netdev_dbg(dev, "Stuff error\n");
+			cf->data[2] |= CAN_ERR_PROT_STUFF;
+			rx_errors = true;
+			break;
+		}
+	}
+
+	if (rx_errors)
+		dev->stats.rx_errors++;
+
+	if (tx_errors)
+		dev->stats.tx_errors++;
+
+	netif_rx(skb);
+}
+
 /* Send one completely decapsulated can_frame to the network layer */
-static void slc_bump(struct slcan *sl)
+static void slc_bump_frame(struct slcan *sl)
 {
 	struct sk_buff *skb;
 	struct can_frame cf, *scf;
@@ -257,6 +341,24 @@ static void slc_bump(struct slcan *sl)
 	netif_rx(skb);
 }
 
+static void slc_bump(struct slcan *sl)
+{
+	switch (sl->rbuff[0]) {
+	case 'r':
+		fallthrough;
+	case 't':
+		fallthrough;
+	case 'R':
+		fallthrough;
+	case 'T':
+		return slc_bump_frame(sl);
+	case 'e':
+		return slc_bump_err(sl);
+	default:
+		return;
+	}
+}
+
 /* parse tty input stream */
 static void slcan_unesc(struct slcan *sl, unsigned char s)
 {
-- 
2.32.0

Re: [RFC PATCH 12/13] can: slcan: extend the protocol with error info

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3403 bytes --]

On 07.06.2022 11:47:51, Dario Binacchi wrote:
> It extends the protocol to receive the adapter CAN communication errors
> and forward them to the netdev upper levels.
> 
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> ---
> 
>  drivers/net/can/slcan/slcan-core.c | 104 ++++++++++++++++++++++++++++-
>  1 file changed, 103 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
> index b813a59534a3..02e7c14de45c 100644
> --- a/drivers/net/can/slcan/slcan-core.c
> +++ b/drivers/net/can/slcan/slcan-core.c
> @@ -182,8 +182,92 @@ int slcan_enable_err_rst_on_open(struct net_device *ndev, bool on)
>    *			STANDARD SLCAN DECAPSULATION			 *
>    ************************************************************************/
>  
> +static void slc_bump_err(struct slcan *sl)
> +{
> +	struct net_device *dev = sl->dev;
> +	struct sk_buff *skb;
> +	struct can_frame *cf;
> +	char *cmd = sl->rbuff;
> +	bool rx_errors = false, tx_errors = false;
> +	int i, len;
> +
> +	if (*cmd != 'e')
> +		return;
> +
> +	cmd += SLC_CMD_LEN;
> +	/* get len from sanitized ASCII value */

What happens is a malicious device sends a wrong len value, that's
longer than the RX'ed data?

> +	len = *cmd++;
> +	if (len >= '0' && len < '9')
> +		len -= '0';
> +	else
> +		return;
> +
> +	skb = alloc_can_err_skb(dev, &cf);

Please continue error handling, even if no skb can be allocated.

> +	if (unlikely(!skb))
> +		return;
> +
> +	cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +	for (i = 0; i < len; i++, cmd++) {
> +		switch (*cmd) {
> +		case 'a':
> +			netdev_dbg(dev, "ACK error\n");
> +			cf->can_id |= CAN_ERR_ACK;
> +			cf->data[3] = CAN_ERR_PROT_LOC_ACK;
> +			tx_errors = true;
> +			break;
> +		case 'b':
> +			netdev_dbg(dev, "Bit0 error\n");
> +			cf->data[2] |= CAN_ERR_PROT_BIT0;
> +			tx_errors = true;
> +			break;
> +		case 'B':
> +			netdev_dbg(dev, "Bit1 error\n");
> +			cf->data[2] |= CAN_ERR_PROT_BIT1;
> +			tx_errors = true;
> +			break;
> +		case 'c':
> +			netdev_dbg(dev, "CRC error\n");
> +			cf->data[2] |= CAN_ERR_PROT_BIT;
> +			cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;
> +			rx_errors = true;
> +			break;
> +		case 'f':
> +			netdev_dbg(dev, "Form Error\n");
> +			cf->data[2] |= CAN_ERR_PROT_FORM;
> +			rx_errors = true;
> +			break;
> +		case 'o':
> +			netdev_dbg(dev, "Rx overrun error\n");
> +			cf->can_id |= CAN_ERR_CRTL;
> +			cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> +			dev->stats.rx_over_errors++;
> +			dev->stats.rx_errors++;
> +			break;
> +		case 'O':
> +			netdev_dbg(dev, "Tx overrun error\n");
> +			cf->can_id |= CAN_ERR_CRTL;
> +			cf->data[1] = CAN_ERR_CRTL_TX_OVERFLOW;
> +			dev->stats.tx_errors++;
> +			break;
> +		case 's':
> +			netdev_dbg(dev, "Stuff error\n");
> +			cf->data[2] |= CAN_ERR_PROT_STUFF;
> +			rx_errors = true;
> +			break;
> +		}
> +	}
> +
> +	if (rx_errors)
> +		dev->stats.rx_errors++;
> +
> +	if (tx_errors)
> +		dev->stats.tx_errors++;
> +
> +	netif_rx(skb);
> +}
> +

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 13/13] can: slcan: extend the protocol with CAN state info

[Dario Binacchi]

It extends the protocol to receive the adapter CAN state changes
(warning, busoff, etc.) and forward them to the netdev upper levels.

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

---

 drivers/net/can/slcan/slcan-core.c | 65 ++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
index 02e7c14de45c..ab4c08a7dc81 100644
--- a/drivers/net/can/slcan/slcan-core.c
+++ b/drivers/net/can/slcan/slcan-core.c
@@ -78,6 +78,9 @@ MODULE_PARM_DESC(maxdev, "Maximum number of slcan interfaces");
 #define SLC_CMD_LEN 1
 #define SLC_SFF_ID_LEN 3
 #define SLC_EFF_ID_LEN 8
+#define SLC_STATE_LEN 1
+#define SLC_STATE_BE_RXCNT_LEN 3
+#define SLC_STATE_BE_TXCNT_LEN 3
 
 struct slcan {
 	struct can_priv         can;
@@ -182,6 +185,66 @@ int slcan_enable_err_rst_on_open(struct net_device *ndev, bool on)
   *			STANDARD SLCAN DECAPSULATION			 *
   ************************************************************************/
 
+static void slc_bump_state(struct slcan *sl)
+{
+	struct net_device *dev = sl->dev;
+	struct sk_buff *skb;
+	struct can_frame *cf;
+	char *cmd = sl->rbuff;
+	u32 rxerr, txerr;
+	enum can_state state, rx_state, tx_state;
+
+	if (*cmd != 's')
+		return;
+
+	cmd += SLC_CMD_LEN;
+	switch (*cmd) {
+	case 'a':
+		state = CAN_STATE_ERROR_ACTIVE;
+		break;
+	case 'w':
+		state = CAN_STATE_ERROR_WARNING;
+		break;
+	case 'p':
+		state = CAN_STATE_ERROR_PASSIVE;
+		break;
+	case 'f':
+		state = CAN_STATE_BUS_OFF;
+		break;
+	default:
+		return;
+	}
+
+	if (state == sl->can.state)
+		return;
+
+	cmd += SLC_STATE_BE_RXCNT_LEN + 1;
+	cmd[SLC_STATE_BE_TXCNT_LEN] = 0;
+	if (kstrtou32(cmd, 10, &txerr))
+		return;
+
+	*cmd = 0;
+	cmd -= SLC_STATE_BE_RXCNT_LEN;
+	if (kstrtou32(cmd, 10, &rxerr))
+		return;
+
+	skb = alloc_can_err_skb(dev, &cf);
+	if (unlikely(!skb))
+		return;
+
+	cf->data[6] = txerr;
+	cf->data[7] = rxerr;
+
+	tx_state = txerr >= rxerr ? state : 0;
+	rx_state = txerr <= rxerr ? state : 0;
+	can_change_state(dev, cf, tx_state, rx_state);
+
+	if (state == CAN_STATE_BUS_OFF)
+		can_bus_off(dev);
+
+	netif_rx(skb);
+}
+
 static void slc_bump_err(struct slcan *sl)
 {
 	struct net_device *dev = sl->dev;
@@ -354,6 +417,8 @@ static void slc_bump(struct slcan *sl)
 		return slc_bump_frame(sl);
 	case 'e':
 		return slc_bump_err(sl);
+	case 's':
+		return slc_bump_state(sl);
 	default:
 		return;
 	}
-- 
2.32.0

Re: [RFC PATCH 13/13] can: slcan: extend the protocol with CAN state info

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3140 bytes --]

On 07.06.2022 11:47:52, Dario Binacchi wrote:
> It extends the protocol to receive the adapter CAN state changes
> (warning, busoff, etc.) and forward them to the netdev upper levels.
> 
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> 
> ---
> 
>  drivers/net/can/slcan/slcan-core.c | 65 ++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
> index 02e7c14de45c..ab4c08a7dc81 100644
> --- a/drivers/net/can/slcan/slcan-core.c
> +++ b/drivers/net/can/slcan/slcan-core.c
> @@ -78,6 +78,9 @@ MODULE_PARM_DESC(maxdev, "Maximum number of slcan interfaces");
>  #define SLC_CMD_LEN 1
>  #define SLC_SFF_ID_LEN 3
>  #define SLC_EFF_ID_LEN 8
> +#define SLC_STATE_LEN 1
> +#define SLC_STATE_BE_RXCNT_LEN 3
> +#define SLC_STATE_BE_TXCNT_LEN 3
>  
>  struct slcan {
>  	struct can_priv         can;
> @@ -182,6 +185,66 @@ int slcan_enable_err_rst_on_open(struct net_device *ndev, bool on)
>    *			STANDARD SLCAN DECAPSULATION			 *
>    ************************************************************************/
>  
> +static void slc_bump_state(struct slcan *sl)
> +{
> +	struct net_device *dev = sl->dev;
> +	struct sk_buff *skb;
> +	struct can_frame *cf;
> +	char *cmd = sl->rbuff;
> +	u32 rxerr, txerr;
> +	enum can_state state, rx_state, tx_state;
> +
> +	if (*cmd != 's')
> +		return;
> +
> +	cmd += SLC_CMD_LEN;
> +	switch (*cmd) {
> +	case 'a':
> +		state = CAN_STATE_ERROR_ACTIVE;
> +		break;
> +	case 'w':
> +		state = CAN_STATE_ERROR_WARNING;
> +		break;
> +	case 'p':
> +		state = CAN_STATE_ERROR_PASSIVE;
> +		break;
> +	case 'f':
> +		state = CAN_STATE_BUS_OFF;
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	if (state == sl->can.state)
> +		return;
> +
> +	cmd += SLC_STATE_BE_RXCNT_LEN + 1;
> +	cmd[SLC_STATE_BE_TXCNT_LEN] = 0;
> +	if (kstrtou32(cmd, 10, &txerr))
> +		return;
> +
> +	*cmd = 0;
> +	cmd -= SLC_STATE_BE_RXCNT_LEN;
> +	if (kstrtou32(cmd, 10, &rxerr))
> +		return;
> +
> +	skb = alloc_can_err_skb(dev, &cf);
> +	if (unlikely(!skb))
> +		return;

Please continue error handling, even if no skb can be allocated.

> +
> +	cf->data[6] = txerr;
> +	cf->data[7] = rxerr;
> +
> +	tx_state = txerr >= rxerr ? state : 0;
> +	rx_state = txerr <= rxerr ? state : 0;
> +	can_change_state(dev, cf, tx_state, rx_state);
> +
> +	if (state == CAN_STATE_BUS_OFF)
> +		can_bus_off(dev);
> +
> +	netif_rx(skb);
> +}
> +
>  static void slc_bump_err(struct slcan *sl)
>  {
>  	struct net_device *dev = sl->dev;
> @@ -354,6 +417,8 @@ static void slc_bump(struct slcan *sl)
>  		return slc_bump_frame(sl);
>  	case 'e':
>  		return slc_bump_err(sl);
> +	case 's':
> +		return slc_bump_state(sl);
>  	default:
>  		return;
>  	}

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 00/13] can: slcan: extend supported features

[Vincent MAILHOL]

On Tue. 7 juin 2022 at 18:47, Dario Binacchi
<dario.binacchi@amarulasolutions.com> wrote:
> This series originated as a result of CAN communication tests for an
> application using the USBtin adapter (https://www.fischl.de/usbtin/).
> The tests showed some errors but for the driver everything was ok.
> Also, being the first time I used the slcan driver, I was amazed that
> it was not possible to configure the bitrate via the ip tool.
> For these two reasons, I started looking at the driver code and realized
> that it didn't use the CAN network device driver interface.

That's funny! Yesterday, I sent this comment:
https://lore.kernel.org/linux-can/CAMZ6RqKZwC_OKcgH+WPacY6kbNbj4xR2Gdg2NQtm5Ka5Hfw79A@mail.gmail.com/

And today, you send a full series to remove all the dust from the
slcan driver. Do I have some kind of mystical power to summon people
on the mailing list?

> Starting from these assumptions, I tried to:
> - Use the CAN network device driver interface.

In order to use the CAN network device driver, a.k.a. can-dev module,
drivers/net/can/Kbuild has to be adjusted: move slcan inside CAN_DEV
scope.

@Mark: because I will have to send a new version for my can-dev/Kbuild
cleanup, maybe I can take that change and add it to my series?

> - Set the bitrate via the ip tool.
> - Send the open/close command to the adapter from the driver.
> - Add ethtool support to reset the adapter errors.
> - Extend the protocol to forward the adapter CAN communication
>   errors and the CAN state changes to the netdev upper layers.
>
> Except for the protocol extension patches (i. e. forward the adapter CAN
> communication errors and the CAN state changes to the netdev upper
> layers), the whole series has been tested. Testing the extension
> protocol patches requires updating the adapter firmware. Before modifying
> the firmware I think it makes sense to know if these extensions can be
> considered useful.
>
> Before applying the series I used these commands:
>
> slcan_attach -f -s6 -o /dev/ttyACM0
> slcand ttyACM0 can0
> ip link set can0 up
>
> After applying the series I am using these commands:
>
> slcan_attach /dev/ttyACM0
> slcand ttyACM0 can0
> ip link set dev can0 down
> ip link set can0 type can bitrate 500000
> ethtool --set-priv-flags can0 err-rst-on-open on
> ip link set dev can0 up
>
> Now there is a clearer separation between serial line and CAN,
> but above all, it is possible to use the ip and ethtool commands
> as it happens for any CAN device driver. The changes are backward
> compatible, you can continue to use the slcand and slcan_attach
> command options.
>
>
>
> Dario Binacchi (13):
>   can: slcan: use the BIT() helper
>   can: slcan: use netdev helpers to print out messages
>   can: slcan: use the alloc_can_skb() helper
>   can: slcan: use CAN network device driver API
>   can: slcan: simplify the device de-allocation
>   can: slcan: allow to send commands to the adapter
>   can: slcan: set bitrate by CAN device driver API
>   can: slcan: send the open command to the adapter
>   can: slcan: send the close command to the adapter
>   can: slcan: move driver into separate sub directory
>   can: slcan: add ethtool support to reset adapter errors
>   can: slcan: extend the protocol with error info
>   can: slcan: extend the protocol with CAN state info
>
>  drivers/net/can/Makefile                      |   2 +-
>  drivers/net/can/slcan/Makefile                |   7 +
>  .../net/can/{slcan.c => slcan/slcan-core.c}   | 464 +++++++++++++++---
>  drivers/net/can/slcan/slcan-ethtool.c         |  65 +++
>  drivers/net/can/slcan/slcan.h                 |  18 +
>  5 files changed, 480 insertions(+), 76 deletions(-)
>  create mode 100644 drivers/net/can/slcan/Makefile
>  rename drivers/net/can/{slcan.c => slcan/slcan-core.c} (67%)
>  create mode 100644 drivers/net/can/slcan/slcan-ethtool.c
>  create mode 100644 drivers/net/can/slcan/slcan.h

Re: [RFC PATCH 00/13] can: slcan: extend supported features

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1878 bytes --]

On 07.06.2022 19:27:05, Vincent MAILHOL wrote:
> On Tue. 7 juin 2022 at 18:47, Dario Binacchi
> <dario.binacchi@amarulasolutions.com> wrote:
> > This series originated as a result of CAN communication tests for an
> > application using the USBtin adapter (https://www.fischl.de/usbtin/).
> > The tests showed some errors but for the driver everything was ok.
> > Also, being the first time I used the slcan driver, I was amazed that
> > it was not possible to configure the bitrate via the ip tool.
> > For these two reasons, I started looking at the driver code and realized
> > that it didn't use the CAN network device driver interface.
> 
> That's funny! Yesterday, I sent this comment:
> https://lore.kernel.org/linux-can/CAMZ6RqKZwC_OKcgH+WPacY6kbNbj4xR2Gdg2NQtm5Ka5Hfw79A@mail.gmail.com/
> 
> And today, you send a full series to remove all the dust from the
> slcan driver. Do I have some kind of mystical power to summon people
> on the mailing list?

That would be very useful and awesome super power, I'm a bit jealous. :D

> > Starting from these assumptions, I tried to:
> > - Use the CAN network device driver interface.
> 
> In order to use the CAN network device driver, a.k.a. can-dev module,
> drivers/net/can/Kbuild has to be adjusted: move slcan inside CAN_DEV
> scope.
> 
> @Mark: because I will have to send a new version for my can-dev/Kbuild
> cleanup, maybe I can take that change and add it to my series?

Let's get the your Kconfig/Makefile changes into can-next/master first.
Then Dario can then base this series on that branch.

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 00/13] can: slcan: extend supported features

[Vincent MAILHOL]

On Tue. 7 Jun 2022 at 19:39, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 07.06.2022 19:27:05, Vincent MAILHOL wrote:
> > On Tue. 7 juin 2022 at 18:47, Dario Binacchi
> > <dario.binacchi@amarulasolutions.com> wrote:
> > > This series originated as a result of CAN communication tests for an
> > > application using the USBtin adapter (https://www.fischl.de/usbtin/).
> > > The tests showed some errors but for the driver everything was ok.
> > > Also, being the first time I used the slcan driver, I was amazed that
> > > it was not possible to configure the bitrate via the ip tool.
> > > For these two reasons, I started looking at the driver code and realized
> > > that it didn't use the CAN network device driver interface.
> >
> > That's funny! Yesterday, I sent this comment:
> > https://lore.kernel.org/linux-can/CAMZ6RqKZwC_OKcgH+WPacY6kbNbj4xR2Gdg2NQtm5Ka5Hfw79A@mail.gmail.com/
> >
> > And today, you send a full series to remove all the dust from the
> > slcan driver. Do I have some kind of mystical power to summon people
> > on the mailing list?
>
> That would be very useful and awesome super power, I'm a bit jealous. :D
>
> > > Starting from these assumptions, I tried to:
> > > - Use the CAN network device driver interface.
> >
> > In order to use the CAN network device driver, a.k.a. can-dev module,
> > drivers/net/can/Kbuild has to be adjusted: move slcan inside CAN_DEV
> > scope.
> >
> > @Mark: because I will have to send a new version for my can-dev/Kbuild
> > cleanup, maybe I can take that change and add it to my series?
>
> Let's get the your Kconfig/Makefile changes into can-next/master first.
> Then Dario can then base this series on that branch.

ACK. I'll keep my series as-is.

Re: [RFC PATCH 00/13] can: slcan: extend supported features

[Vincent MAILHOL]

+CC: Max Staudt <max@enpas.org>

On Tue. 7 Jun. 2022 at 18:47, Dario Binacchi
<dario.binacchi@amarulasolutions.com> wrote:
> This series originated as a result of CAN communication tests for an
> application using the USBtin adapter (https://www.fischl.de/usbtin/).
> The tests showed some errors but for the driver everything was ok.
> Also, being the first time I used the slcan driver, I was amazed that
> it was not possible to configure the bitrate via the ip tool.
> For these two reasons, I started looking at the driver code and realized
> that it didn't use the CAN network device driver interface.
>
> Starting from these assumptions, I tried to:
> - Use the CAN network device driver interface.
> - Set the bitrate via the ip tool.
> - Send the open/close command to the adapter from the driver.
> - Add ethtool support to reset the adapter errors.
> - Extend the protocol to forward the adapter CAN communication
>   errors and the CAN state changes to the netdev upper layers.
>
> Except for the protocol extension patches (i. e. forward the adapter CAN
> communication errors and the CAN state changes to the netdev upper
> layers), the whole series has been tested. Testing the extension
> protocol patches requires updating the adapter firmware. Before modifying
> the firmware I think it makes sense to know if these extensions can be
> considered useful.
>
> Before applying the series I used these commands:
>
> slcan_attach -f -s6 -o /dev/ttyACM0
> slcand ttyACM0 can0
> ip link set can0 up
>
> After applying the series I am using these commands:
>
> slcan_attach /dev/ttyACM0
> slcand ttyACM0 can0
> ip link set dev can0 down
> ip link set can0 type can bitrate 500000
> ethtool --set-priv-flags can0 err-rst-on-open on
> ip link set dev can0 up

In his CAN327 driver, Max manages to bring the can0 device without the
need of dedicated user space daemon by using line discipline
(ldattach):
https://lore.kernel.org/linux-can/20220602213544.68273-1-max@enpas.org/

Isn't the same feasible with slcan so that we completely remove the
dependency toward slcand?
Max what do you think of this?

> Now there is a clearer separation between serial line and CAN,
> but above all, it is possible to use the ip and ethtool commands
> as it happens for any CAN device driver. The changes are backward
> compatible, you can continue to use the slcand and slcan_attach
> command options.

Yours sincerely,
Vincent Mailhol

Re: [RFC PATCH 00/13] can: slcan: extend supported features

[Max Staudt]

On Tue, 7 Jun 2022 21:19:54 +0900
Vincent MAILHOL <mailhol.vincent@wanadoo.fr> wrote:

> In his CAN327 driver, Max manages to bring the can0 device without the
> need of dedicated user space daemon by using line discipline
> (ldattach):
> https://lore.kernel.org/linux-can/20220602213544.68273-1-max@enpas.org/
> 
> Isn't the same feasible with slcan so that we completely remove the
> dependency toward slcand?
> Max what do you think of this?

I think it is a good idea to move this into the kernel driver. I don't
have a slcan device, but a quick peek at its protocol suggests that
it can be done much easier and cleaner than in can327.


Fun fact: The use of a userspace "ignition" tool is a possible use case
baked into can327's design ;)

There is only one use case I can see for it though: Probing the
ELM327's baud rate, and/or setting it before attaching the ldisc. This
is the single thing that the kernel driver cannot do, since it is
attached to an already running TTY, with a fixed speed. Everything else
is configurable via "ip link".

slcand could be used to auto-probe and set the baud rate as well.

Then again, these stripped-down tools could likely be implemented as
shell scripts calling stty and ldattach...


Max

Re: [RFC PATCH 00/13] can: slcan: extend supported features

[Max Staudt]

Dario, thank you so much for working on slcan!


To speed up the slcan cleanup, may I suggest looking at can327?

It started as a modification of slcan, and over the past few months,
it has gone through several review rounds in upstreaming. In fact, a
*ton* of things pointed out during reviews would apply 1:1 to slcan.

What's more, there's legacy stuff that's no longer needed. No
SLCAN_MAGIC, no slcan_devs, ... it's all gone in can327. May I suggest
you have a look at it and bring slcan's boilerplate in line with it?

It's certainly not perfect (7 patch series and counting, and that's
just the public ones), but I'm sure that looking at the two drivers
side-by-side could serve as a good starting point, to avoid
re-reviewing the same things all over again.


The current out-of-tree version can be found here (the repo name is
still the old one, elmcan), where I occasionally push changes before
bundling them up into an upstreaming patch. If a specific line seems
strange to you, "git blame" on this repo is likely to dig up a helpful
commit message, explaining the choice:

  https://github.com/norly/elmcan
  https://git.enpas.org/?p=elmcan.git


Max

Re: [RFC PATCH 00/13] can: slcan: extend supported features

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1295 bytes --]

On 08.06.2022 02:15:37, Max Staudt wrote:
> To speed up the slcan cleanup, may I suggest looking at can327?
> 
> It started as a modification of slcan, and over the past few months,
> it has gone through several review rounds in upstreaming. In fact, a
> *ton* of things pointed out during reviews would apply 1:1 to slcan.
> 
> What's more, there's legacy stuff that's no longer needed. No
> SLCAN_MAGIC, no slcan_devs, ... it's all gone in can327. May I suggest
> you have a look at it and bring slcan's boilerplate in line with it?

+1

Most of Dario's series looks good. I suggest that we mainline this
first. If there's interest and energy the slcan driver can be reworked
to re-use the more modern concepts of the can327 driver.

> It's certainly not perfect (7 patch series and counting, and that's
> just the public ones), but I'm sure that looking at the two drivers
> side-by-side could serve as a good starting point, to avoid
> re-reviewing the same things all over again.

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 00/13] can: slcan: extend supported features

[Max Staudt]

On Wed, 8 Jun 2022 09:19:47 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 08.06.2022 02:15:37, Max Staudt wrote:
> > To speed up the slcan cleanup, may I suggest looking at can327?
> > 
> > It started as a modification of slcan, and over the past few months,
> > it has gone through several review rounds in upstreaming. In fact, a
> > *ton* of things pointed out during reviews would apply 1:1 to slcan.
> > 
> > What's more, there's legacy stuff that's no longer needed. No
> > SLCAN_MAGIC, no slcan_devs, ... it's all gone in can327. May I
> > suggest you have a look at it and bring slcan's boilerplate in line
> > with it?  
> 
> +1
> 
> Most of Dario's series looks good. I suggest that we mainline this
> first. If there's interest and energy the slcan driver can be reworked
> to re-use the more modern concepts of the can327 driver.

Agreed. It does look good, and I'm glad to see slcan get some
love.

Thanks Dario!


Max