[RFC PATCH v3 0/9] can: slcan: extend supported features (step 2)

[RFC PATCH v3 0/9] can: slcan: extend supported features (step 2)

[Dario Binacchi]

With this series I try to finish the task, started with the series [1],
of completely removing the dependency of the slcan driver from the
userspace slcand/slcan_attach applications.

The series also contains patches that remove the legacy stuff (slcan_devs,
SLCAN_MAGIC, ...) and do some module cleanup.

The series has been created on top of the patches:

can: slcan: convert comments to network style comments
can: slcan: slcan_init() convert printk(LEVEL ...) to pr_level()
can: slcan: fix whitespace issues
can: slcan: convert comparison to NULL into !val
can: slcan: clean up if/else
can: slcan: use scnprintf() as a hardening measure
can: slcan: do not report txerr and rxerr during bus-off
can: slcan: do not sleep with a spin lock held

applied to linux-next.

[1] https://lore.kernel.org/all/20220628163137.413025-1-dario.binacchi@amarulasolutions.com/

Changes in v3:
- Update the commit message.
- Use 1 space in front of the =.
- Put the series as RFC again.
- Pick up the patch "can: slcan: use KBUILD_MODNAME and define pr_fmt to replace hardcoded names".
- Add the patch "ethtool: add support to get/set CAN bit time register"
  to the series.
- Add the patch "can: slcan: add support to set bit time register (btr)"
  to the series.
- Replace the link https://marc.info/?l=linux-can&m=165806705927851&w=2 with
  https://lore.kernel.org/all/507b5973-d673-4755-3b64-b41cb9a13b6f@hartkopp.net.
- Add the `Suggested-by' tag.

Changes in v2:
- Re-add headers that export at least one symbol used by the module.
- Update the commit description.
- Drop the old "slcan" name to use the standard canX interface naming.
- Remove comment on listen-only command.
- Update the commit subject and description.
- Add the patch "MAINTAINERS: Add myself as maintainer of the SLCAN driver"
  to the series.

Dario Binacchi (8):
  can: slcan: remove useless header inclusions
  can: slcan: remove legacy infrastructure
  can: slcan: change every `slc' occurrence in `slcan'
  can: slcan: use the generic can_change_mtu()
  can: slcan: add support for listen-only mode
  ethtool: add support to get/set CAN bit time register
  can: slcan: add support to set bit time register (btr)
  MAINTAINERS: Add maintainer for the slcan driver

Vincent Mailhol (1):
  can: slcan: use KBUILD_MODNAME and define pr_fmt to replace hardcoded
    names

 MAINTAINERS                           |   6 +
 drivers/net/can/slcan/slcan-core.c    | 515 +++++++++-----------------
 drivers/net/can/slcan/slcan-ethtool.c |  13 +
 drivers/net/can/slcan/slcan.h         |   1 +
 include/uapi/linux/ethtool.h          |   1 +
 net/ethtool/common.c                  |   1 +
 net/ethtool/ioctl.c                   |   1 +
 7 files changed, 204 insertions(+), 334 deletions(-)

-- 
2.32.0

[RFC PATCH v3 1/9] can: slcan: use KBUILD_MODNAME and define pr_fmt to replace hardcoded names

[Dario Binacchi]

From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

The driver uses the string "slcan" to populate
tty_ldisc_ops::name. KBUILD_MODNAME also evaluates to "slcan". Use
KBUILD_MODNAME to get rid on the hardcoded string names.

Similarly, the pr_info() and pr_err() hardcoded the "slcan"
prefix. Define pr_fmt so that the "slcan" prefix gets automatically
added.

CC: Dario Binacchi <dario.binacchi@amarulasolutions.com>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
---

(no changes since v1)

 drivers/net/can/slcan/slcan-core.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
index dfd1baba4130..2c9d9fc19ea9 100644
--- a/drivers/net/can/slcan/slcan-core.c
+++ b/drivers/net/can/slcan/slcan-core.c
@@ -35,6 +35,8 @@
  *
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 
@@ -863,7 +865,7 @@ static struct slcan *slc_alloc(void)
 	if (!dev)
 		return NULL;
 
-	snprintf(dev->name, sizeof(dev->name), "slcan%d", i);
+	snprintf(dev->name, sizeof(dev->name), KBUILD_MODNAME "%d", i);
 	dev->netdev_ops = &slc_netdev_ops;
 	dev->base_addr  = i;
 	slcan_set_ethtool_ops(dev);
@@ -936,7 +938,7 @@ static int slcan_open(struct tty_struct *tty)
 		rtnl_unlock();
 		err = register_candev(sl->dev);
 		if (err) {
-			pr_err("slcan: can't register candev\n");
+			pr_err("can't register candev\n");
 			goto err_free_chan;
 		}
 	} else {
@@ -1027,7 +1029,7 @@ static int slcan_ioctl(struct tty_struct *tty, unsigned int cmd,
 static struct tty_ldisc_ops slc_ldisc = {
 	.owner		= THIS_MODULE,
 	.num		= N_SLCAN,
-	.name		= "slcan",
+	.name		= KBUILD_MODNAME,
 	.open		= slcan_open,
 	.close		= slcan_close,
 	.hangup		= slcan_hangup,
@@ -1043,8 +1045,8 @@ static int __init slcan_init(void)
 	if (maxdev < 4)
 		maxdev = 4; /* Sanity */
 
-	pr_info("slcan: serial line CAN interface driver\n");
-	pr_info("slcan: %d dynamic interface channels.\n", maxdev);
+	pr_info("serial line CAN interface driver\n");
+	pr_info("%d dynamic interface channels.\n", maxdev);
 
 	slcan_devs = kcalloc(maxdev, sizeof(struct net_device *), GFP_KERNEL);
 	if (!slcan_devs)
@@ -1053,7 +1055,7 @@ static int __init slcan_init(void)
 	/* Fill in our line protocol discipline, and register it */
 	status = tty_register_ldisc(&slc_ldisc);
 	if (status)  {
-		pr_err("slcan: can't register line discipline\n");
+		pr_err("can't register line discipline\n");
 		kfree(slcan_devs);
 	}
 	return status;
-- 
2.32.0

[RFC PATCH v3 2/9] can: slcan: remove useless header inclusions

[Dario Binacchi]

Include only the necessary headers.

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

---

(no changes since v2)

Changes in v2:
- Re-add headers that export at least one symbol used by the module.

 drivers/net/can/slcan/slcan-core.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
index 2c9d9fc19ea9..ca383c43167d 100644
--- a/drivers/net/can/slcan/slcan-core.c
+++ b/drivers/net/can/slcan/slcan-core.c
@@ -48,9 +48,6 @@
 #include <linux/netdevice.h>
 #include <linux/skbuff.h>
 #include <linux/rtnetlink.h>
-#include <linux/if_arp.h>
-#include <linux/if_ether.h>
-#include <linux/sched.h>
 #include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
-- 
2.32.0

[RFC PATCH v3 3/9] can: slcan: remove legacy infrastructure

[Dario Binacchi]

Taking inspiration from the drivers/net/can/can327.c driver and at the
suggestion of its author Max Staudt, I removed legacy stuff like
`SLCAN_MAGIC' and `slcan_devs' resulting in simplification of the code
and its maintainability.

The use of slcan_devs is derived from a very old kernel, since slip.c
is about 30 years old, so today's kernel allows us to remove it.

The .hangup() ldisc function, which only called the ldisc .close(), has
been removed since the ldisc layer calls .close() in a good place
anyway.

The old slcanX name has been dropped in order to use the standard canX
interface naming. The ioctl SIOCGIFNAME can be used to query the name of
the created interface. Furthermore, there are several ways to get stable
interfaces names in user space, e.g. udev or systemd-networkd.

The `maxdev' module parameter has also been removed.

CC: Max Staudt <max@enpas.org>
Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

---

Changes in v3:
- Update the commit message.
- Use 1 space in front of the =.

Changes in v2:
- Update the commit description.
- Drop the old "slcan" name to use the standard canX interface naming.

 drivers/net/can/slcan/slcan-core.c | 318 ++++++-----------------------
 1 file changed, 63 insertions(+), 255 deletions(-)

diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
index ca383c43167d..0d309d0636df 100644
--- a/drivers/net/can/slcan/slcan-core.c
+++ b/drivers/net/can/slcan/slcan-core.c
@@ -1,11 +1,14 @@
 /*
  * slcan.c - serial line CAN interface driver (using tty line discipline)
  *
- * This file is derived from linux/drivers/net/slip/slip.c
+ * This file is derived from linux/drivers/net/slip/slip.c and got
+ * inspiration from linux/drivers/net/can/can327.c for the rework made
+ * on the line discipline code.
  *
  * slip.c Authors  : Laurence Culhane <loz@holmes.demon.co.uk>
  *                   Fred N. van Kempen <waltje@uwalt.nl.mugnet.org>
  * slcan.c Author  : Oliver Hartkopp <socketcan@hartkopp.net>
+ * can327.c Author : Max Staudt <max-linux@enpas.org>
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License as published by the
@@ -38,7 +41,6 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/module.h>
-#include <linux/moduleparam.h>
 
 #include <linux/uaccess.h>
 #include <linux/bitops.h>
@@ -48,7 +50,6 @@
 #include <linux/netdevice.h>
 #include <linux/skbuff.h>
 #include <linux/rtnetlink.h>
-#include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/workqueue.h>
@@ -63,15 +64,6 @@ MODULE_DESCRIPTION("serial line CAN interface");
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Oliver Hartkopp <socketcan@hartkopp.net>");
 
-#define SLCAN_MAGIC 0x53CA
-
-static int maxdev = 10;		/* MAX number of SLCAN channels;
-				 * This can be overridden with
-				 * insmod slcan.ko maxdev=nnn
-				 */
-module_param(maxdev, int, 0);
-MODULE_PARM_DESC(maxdev, "Maximum number of slcan interfaces");
-
 /* maximum rx buffer len: extended CAN frame with timestamp */
 #define SLC_MTU (sizeof("T1111222281122334455667788EA5F\r") + 1)
 
@@ -85,7 +77,6 @@ MODULE_PARM_DESC(maxdev, "Maximum number of slcan interfaces");
 				   SLC_STATE_BE_TXCNT_LEN)
 struct slcan {
 	struct can_priv         can;
-	int			magic;
 
 	/* Various fields. */
 	struct tty_struct	*tty;		/* ptr to TTY structure	     */
@@ -101,17 +92,14 @@ struct slcan {
 	int			xleft;          /* bytes left in XMIT queue  */
 
 	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      */
+#define SLF_ERROR		0               /* Parity, etc. error        */
+#define SLF_XCMD		1               /* 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              */
 };
 
-static struct net_device **slcan_devs;
-
 static const u32 slcan_bitrate_const[] = {
 	10000, 20000, 50000, 100000, 125000,
 	250000, 500000, 800000, 1000000
@@ -555,9 +543,8 @@ 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 ||
-	    (unlikely(!netif_running(sl->dev)) &&
-	     likely(!test_bit(SLF_XCMD, &sl->flags)))) {
+	if (unlikely(!netif_running(sl->dev)) &&
+	    likely(!test_bit(SLF_XCMD, &sl->flags))) {
 		spin_unlock_bh(&sl->lock);
 		return;
 	}
@@ -592,13 +579,9 @@ static void slcan_transmit(struct work_struct *work)
  */
 static void slcan_write_wakeup(struct tty_struct *tty)
 {
-	struct slcan *sl;
+	struct slcan *sl = (struct slcan *)tty->disc_data;
 
-	rcu_read_lock();
-	sl = rcu_dereference(tty->disc_data);
-	if (sl)
-		schedule_work(&sl->tx_work);
-	rcu_read_unlock();
+	schedule_work(&sl->tx_work);
 }
 
 /* Send a can_frame to a TTY queue. */
@@ -669,25 +652,21 @@ 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 != CAN_BITRATE_UNKNOWN) {
-			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);
+	if (sl->can.bittiming.bitrate &&
+	    sl->can.bittiming.bitrate != CAN_BITRATE_UNKNOWN) {
+		err = slcan_transmit_cmd(sl, "C\r");
+		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);
+	flush_work(&sl->tx_work);
+
 	netif_stop_queue(dev);
 	sl->rcount   = 0;
 	sl->xleft    = 0;
-	spin_unlock_bh(&sl->lock);
 	close_candev(dev);
 	sl->can.state = CAN_STATE_STOPPED;
 	if (sl->can.bittiming.bitrate == CAN_BITRATE_UNKNOWN)
@@ -703,9 +682,6 @@ static int slc_open(struct net_device *dev)
 	unsigned char cmd[SLC_MTU];
 	int err, s;
 
-	if (!sl->tty)
-		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 CAN_BITRATE_UNSET (0), causing
@@ -720,8 +696,6 @@ static int slc_open(struct net_device *dev)
 		return err;
 	}
 
-	sl->flags &= BIT(SLF_INUSE);
-
 	if (sl->can.bittiming.bitrate != CAN_BITRATE_UNKNOWN) {
 		for (s = 0; s < ARRAY_SIZE(slcan_bitrate_const); s++) {
 			if (sl->can.bittiming.bitrate == slcan_bitrate_const[s])
@@ -765,14 +739,6 @@ static int slc_open(struct net_device *dev)
 	return err;
 }
 
-static void slc_dealloc(struct slcan *sl)
-{
-	int i = sl->dev->base_addr;
-
-	free_candev(sl->dev);
-	slcan_devs[i] = NULL;
-}
-
 static int slcan_change_mtu(struct net_device *dev, int new_mtu)
 {
 	return -EINVAL;
@@ -802,7 +768,7 @@ static void slcan_receive_buf(struct tty_struct *tty,
 {
 	struct slcan *sl = (struct slcan *)tty->disc_data;
 
-	if (!sl || sl->magic != SLCAN_MAGIC || !netif_running(sl->dev))
+	if (!netif_running(sl->dev))
 		return;
 
 	/* Read the characters out of the buffer */
@@ -817,80 +783,15 @@ static void slcan_receive_buf(struct tty_struct *tty,
 	}
 }
 
-/************************************
- *  slcan_open helper routines.
- ************************************/
-
-/* Collect hanged up channels */
-static void slc_sync(void)
-{
-	int i;
-	struct net_device *dev;
-	struct slcan	  *sl;
-
-	for (i = 0; i < maxdev; i++) {
-		dev = slcan_devs[i];
-		if (!dev)
-			break;
-
-		sl = netdev_priv(dev);
-		if (sl->tty)
-			continue;
-		if (dev->flags & IFF_UP)
-			dev_close(dev);
-	}
-}
-
-/* Find a free SLCAN channel, and link in this `tty' line. */
-static struct slcan *slc_alloc(void)
-{
-	int i;
-	struct net_device *dev = NULL;
-	struct slcan       *sl;
-
-	for (i = 0; i < maxdev; i++) {
-		dev = slcan_devs[i];
-		if (!dev)
-			break;
-	}
-
-	/* Sorry, too many, all slots in use */
-	if (i >= maxdev)
-		return NULL;
-
-	dev = alloc_candev(sizeof(*sl), 1);
-	if (!dev)
-		return NULL;
-
-	snprintf(dev->name, sizeof(dev->name), KBUILD_MODNAME "%d", i);
-	dev->netdev_ops = &slc_netdev_ops;
-	dev->base_addr  = i;
-	slcan_set_ethtool_ops(dev);
-	sl = netdev_priv(dev);
-
-	/* Initialize channel control data */
-	sl->magic = SLCAN_MAGIC;
-	sl->dev	= dev;
-	sl->can.bitrate_const = slcan_bitrate_const;
-	sl->can.bitrate_const_cnt = ARRAY_SIZE(slcan_bitrate_const);
-	spin_lock_init(&sl->lock);
-	INIT_WORK(&sl->tx_work, slcan_transmit);
-	init_waitqueue_head(&sl->xcmd_wait);
-	slcan_devs[i] = dev;
-
-	return sl;
-}
-
 /* Open the high-level part of the SLCAN channel.
  * This function is called by the TTY module when the
- * SLCAN line discipline is called for.  Because we are
- * sure the tty line exists, we only have to link it to
- * a free SLCAN channel...
+ * SLCAN line discipline is called for.
  *
  * Called in process context serialized from other ldisc calls.
  */
 static int slcan_open(struct tty_struct *tty)
 {
+	struct net_device *dev;
 	struct slcan *sl;
 	int err;
 
@@ -900,72 +801,49 @@ static int slcan_open(struct tty_struct *tty)
 	if (!tty->ops->write)
 		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();
+	dev = alloc_candev(sizeof(*sl), 1);
+	if (!dev)
+		return -ENFILE;
 
-	/* Collect hanged up channels. */
-	slc_sync();
+	sl = netdev_priv(dev);
 
-	sl = tty->disc_data;
+	/* Configure TTY interface */
+	tty->receive_room = 65536; /* We don't flow control */
+	sl->rcount = 0;
+	sl->xleft = 0;
+	spin_lock_init(&sl->lock);
+	INIT_WORK(&sl->tx_work, slcan_transmit);
+	init_waitqueue_head(&sl->xcmd_wait);
 
-	err = -EEXIST;
-	/* First make sure we're not already connected. */
-	if (sl && sl->magic == SLCAN_MAGIC)
-		goto err_exit;
+	/* Configure CAN metadata */
+	sl->can.bitrate_const = slcan_bitrate_const;
+	sl->can.bitrate_const_cnt = ARRAY_SIZE(slcan_bitrate_const);
 
-	/* OK.  Find a free SLCAN channel to use. */
-	err = -ENFILE;
-	sl = slc_alloc();
-	if (!sl)
-		goto err_exit;
+	/* Configure netdev interface */
+	sl->dev	= dev;
+	dev->netdev_ops = &slc_netdev_ops;
+	slcan_set_ethtool_ops(dev);
 
+	/* Mark ldisc channel as alive */
 	sl->tty = tty;
 	tty->disc_data = sl;
 
-	if (!test_bit(SLF_INUSE, &sl->flags)) {
-		/* Perform the low-level SLCAN initialization. */
-		sl->rcount   = 0;
-		sl->xleft    = 0;
-
-		set_bit(SLF_INUSE, &sl->flags);
-
-		rtnl_unlock();
-		err = register_candev(sl->dev);
-		if (err) {
-			pr_err("can't register candev\n");
-			goto err_free_chan;
-		}
-	} else {
-		rtnl_unlock();
+	err = register_candev(dev);
+	if (err) {
+		free_candev(dev);
+		pr_err("can't register candev\n");
+		return err;
 	}
 
-	tty->receive_room = 65536;	/* We don't flow control */
-
+	netdev_info(dev, "slcan on %s.\n", tty->name);
 	/* TTY layer expects 0 on success */
 	return 0;
-
-err_free_chan:
-	rtnl_lock();
-	sl->tty = NULL;
-	tty->disc_data = NULL;
-	clear_bit(SLF_INUSE, &sl->flags);
-	slc_dealloc(sl);
-	rtnl_unlock();
-	return err;
-
-err_exit:
-	rtnl_unlock();
-
-	/* Count references from TTY module */
-	return err;
 }
 
 /* Close down a SLCAN channel.
  * This means flushing out any pending queues, and then returning. This
  * call is serialized against other ldisc functions.
+ * Once this is called, no other ldisc function of ours is entered.
  *
  * We also use this method for a hangup event.
  */
@@ -973,28 +851,20 @@ static void slcan_close(struct tty_struct *tty)
 {
 	struct slcan *sl = (struct slcan *)tty->disc_data;
 
-	/* First make sure we're connected. */
-	if (!sl || sl->magic != SLCAN_MAGIC || sl->tty != tty)
-		return;
+	/* unregister_netdev() calls .ndo_stop() so we don't have to.
+	 * Our .ndo_stop() also flushes the TTY write wakeup handler,
+	 * so we can safely set sl->tty = NULL after this.
+	 */
+	unregister_candev(sl->dev);
 
+	/* Mark channel as dead */
 	spin_lock_bh(&sl->lock);
-	rcu_assign_pointer(tty->disc_data, NULL);
+	tty->disc_data = NULL;
 	sl->tty = NULL;
 	spin_unlock_bh(&sl->lock);
 
-	synchronize_rcu();
-	flush_work(&sl->tx_work);
-
-	slc_close(sl->dev);
-	unregister_candev(sl->dev);
-	rtnl_lock();
-	slc_dealloc(sl);
-	rtnl_unlock();
-}
-
-static void slcan_hangup(struct tty_struct *tty)
-{
-	slcan_close(tty);
+	netdev_info(sl->dev, "slcan off %s.\n", tty->name);
+	free_candev(sl->dev);
 }
 
 /* Perform I/O control on an active SLCAN channel. */
@@ -1004,10 +874,6 @@ static int slcan_ioctl(struct tty_struct *tty, unsigned int cmd,
 	struct slcan *sl = (struct slcan *)tty->disc_data;
 	unsigned int tmp;
 
-	/* First make sure we're connected. */
-	if (!sl || sl->magic != SLCAN_MAGIC)
-		return -EINVAL;
-
 	switch (cmd) {
 	case SIOCGIFNAME:
 		tmp = strlen(sl->dev->name) + 1;
@@ -1029,7 +895,6 @@ static struct tty_ldisc_ops slc_ldisc = {
 	.name		= KBUILD_MODNAME,
 	.open		= slcan_open,
 	.close		= slcan_close,
-	.hangup		= slcan_hangup,
 	.ioctl		= slcan_ioctl,
 	.receive_buf	= slcan_receive_buf,
 	.write_wakeup	= slcan_write_wakeup,
@@ -1039,78 +904,21 @@ static int __init slcan_init(void)
 {
 	int status;
 
-	if (maxdev < 4)
-		maxdev = 4; /* Sanity */
-
 	pr_info("serial line CAN interface driver\n");
-	pr_info("%d dynamic interface channels.\n", maxdev);
-
-	slcan_devs = kcalloc(maxdev, sizeof(struct net_device *), GFP_KERNEL);
-	if (!slcan_devs)
-		return -ENOMEM;
 
 	/* Fill in our line protocol discipline, and register it */
 	status = tty_register_ldisc(&slc_ldisc);
-	if (status)  {
+	if (status)
 		pr_err("can't register line discipline\n");
-		kfree(slcan_devs);
-	}
+
 	return status;
 }
 
 static void __exit slcan_exit(void)
 {
-	int i;
-	struct net_device *dev;
-	struct slcan *sl;
-	unsigned long timeout = jiffies + HZ;
-	int busy = 0;
-
-	if (!slcan_devs)
-		return;
-
-	/* First of all: check for active disciplines and hangup them.
-	 */
-	do {
-		if (busy)
-			msleep_interruptible(100);
-
-		busy = 0;
-		for (i = 0; i < maxdev; i++) {
-			dev = slcan_devs[i];
-			if (!dev)
-				continue;
-			sl = netdev_priv(dev);
-			spin_lock_bh(&sl->lock);
-			if (sl->tty) {
-				busy++;
-				tty_hangup(sl->tty);
-			}
-			spin_unlock_bh(&sl->lock);
-		}
-	} while (busy && time_before(jiffies, timeout));
-
-	/* FIXME: hangup is async so we should wait when doing this second
-	 * phase
+	/* This will only be called when all channels have been closed by
+	 * userspace - tty_ldisc.c takes care of the module's refcount.
 	 */
-
-	for (i = 0; i < maxdev; i++) {
-		dev = slcan_devs[i];
-		if (!dev)
-			continue;
-
-		sl = netdev_priv(dev);
-		if (sl->tty)
-			netdev_err(dev, "tty discipline still running\n");
-
-		slc_close(dev);
-		unregister_candev(dev);
-		slc_dealloc(sl);
-	}
-
-	kfree(slcan_devs);
-	slcan_devs = NULL;
-
 	tty_unregister_ldisc(&slc_ldisc);
 }
 
-- 
2.32.0

Re: [RFC PATCH v3 3/9] can: slcan: remove legacy infrastructure

[Max Staudt]

Hi Dario,

This looks good to me now.

Thank you for cleaning up the legacy code :)


Reviewed-by: Max Staudt <max@enpas.org>

[RFC PATCH v3 4/9] can: slcan: change every `slc' occurrence in `slcan'

[Dario Binacchi]

In the driver there are parts of code where the prefix `slc' is used and
others where the prefix `slcan' is used instead. The patch replaces
every occurrence of `slc' with `slcan', except for the netdev functions
where, to avoid compilation conflicts, it was necessary to replace `slc'
with `slcan_netdev'.

The patch does not make any functional changes.

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

(no changes since v1)

 drivers/net/can/slcan/slcan-core.c | 109 +++++++++++++++--------------
 1 file changed, 56 insertions(+), 53 deletions(-)

diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
index 0d309d0636df..bd8e84ded051 100644
--- a/drivers/net/can/slcan/slcan-core.c
+++ b/drivers/net/can/slcan/slcan-core.c
@@ -65,16 +65,17 @@ MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Oliver Hartkopp <socketcan@hartkopp.net>");
 
 /* maximum rx buffer len: extended CAN frame with timestamp */
-#define SLC_MTU (sizeof("T1111222281122334455667788EA5F\r") + 1)
-
-#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
-#define SLC_STATE_FRAME_LEN       (1 + SLC_CMD_LEN + SLC_STATE_BE_RXCNT_LEN + \
-				   SLC_STATE_BE_TXCNT_LEN)
+#define SLCAN_MTU (sizeof("T1111222281122334455667788EA5F\r") + 1)
+
+#define SLCAN_CMD_LEN 1
+#define SLCAN_SFF_ID_LEN 3
+#define SLCAN_EFF_ID_LEN 8
+#define SLCAN_STATE_LEN 1
+#define SLCAN_STATE_BE_RXCNT_LEN 3
+#define SLCAN_STATE_BE_TXCNT_LEN 3
+#define SLCAN_STATE_FRAME_LEN       (1 + SLCAN_CMD_LEN + \
+				     SLCAN_STATE_BE_RXCNT_LEN + \
+				     SLCAN_STATE_BE_TXCNT_LEN)
 struct slcan {
 	struct can_priv         can;
 
@@ -85,9 +86,9 @@ struct slcan {
 	struct work_struct	tx_work;	/* Flushes transmit buffer   */
 
 	/* These are pointers to the malloc()ed frame buffers. */
-	unsigned char		rbuff[SLC_MTU];	/* receiver buffer	     */
+	unsigned char		rbuff[SLCAN_MTU];	/* receiver buffer   */
 	int			rcount;         /* received chars counter    */
-	unsigned char		xbuff[SLC_MTU];	/* transmitter buffer	     */
+	unsigned char		xbuff[SLCAN_MTU];	/* transmitter buffer*/
 	unsigned char		*xhead;         /* pointer to next XMIT byte */
 	int			xleft;          /* bytes left in XMIT queue  */
 
@@ -166,7 +167,7 @@ int slcan_enable_err_rst_on_open(struct net_device *ndev, bool on)
  *************************************************************************/
 
 /* Send one completely decapsulated can_frame to the network layer */
-static void slc_bump_frame(struct slcan *sl)
+static void slcan_bump_frame(struct slcan *sl)
 {
 	struct sk_buff *skb;
 	struct can_frame *cf;
@@ -186,10 +187,10 @@ static void slc_bump_frame(struct slcan *sl)
 		fallthrough;
 	case 't':
 		/* store dlc ASCII value and terminate SFF CAN ID string */
-		cf->len = sl->rbuff[SLC_CMD_LEN + SLC_SFF_ID_LEN];
-		sl->rbuff[SLC_CMD_LEN + SLC_SFF_ID_LEN] = 0;
+		cf->len = sl->rbuff[SLCAN_CMD_LEN + SLCAN_SFF_ID_LEN];
+		sl->rbuff[SLCAN_CMD_LEN + SLCAN_SFF_ID_LEN] = 0;
 		/* point to payload data behind the dlc */
-		cmd += SLC_CMD_LEN + SLC_SFF_ID_LEN + 1;
+		cmd += SLCAN_CMD_LEN + SLCAN_SFF_ID_LEN + 1;
 		break;
 	case 'R':
 		cf->can_id = CAN_RTR_FLAG;
@@ -197,16 +198,16 @@ static void slc_bump_frame(struct slcan *sl)
 	case 'T':
 		cf->can_id |= CAN_EFF_FLAG;
 		/* store dlc ASCII value and terminate EFF CAN ID string */
-		cf->len = sl->rbuff[SLC_CMD_LEN + SLC_EFF_ID_LEN];
-		sl->rbuff[SLC_CMD_LEN + SLC_EFF_ID_LEN] = 0;
+		cf->len = sl->rbuff[SLCAN_CMD_LEN + SLCAN_EFF_ID_LEN];
+		sl->rbuff[SLCAN_CMD_LEN + SLCAN_EFF_ID_LEN] = 0;
 		/* point to payload data behind the dlc */
-		cmd += SLC_CMD_LEN + SLC_EFF_ID_LEN + 1;
+		cmd += SLCAN_CMD_LEN + SLCAN_EFF_ID_LEN + 1;
 		break;
 	default:
 		goto decode_failed;
 	}
 
-	if (kstrtou32(sl->rbuff + SLC_CMD_LEN, 16, &tmpid))
+	if (kstrtou32(sl->rbuff + SLCAN_CMD_LEN, 16, &tmpid))
 		goto decode_failed;
 
 	cf->can_id |= tmpid;
@@ -253,7 +254,7 @@ static void slc_bump_frame(struct slcan *sl)
  * sb256256 : state bus-off: rx counter 256, tx counter 256
  * sa057033 : state active, rx counter 57, tx counter 33
  */
-static void slc_bump_state(struct slcan *sl)
+static void slcan_bump_state(struct slcan *sl)
 {
 	struct net_device *dev = sl->dev;
 	struct sk_buff *skb;
@@ -279,16 +280,16 @@ static void slc_bump_state(struct slcan *sl)
 		return;
 	}
 
-	if (state == sl->can.state || sl->rcount < SLC_STATE_FRAME_LEN)
+	if (state == sl->can.state || sl->rcount < SLCAN_STATE_FRAME_LEN)
 		return;
 
-	cmd += SLC_STATE_BE_RXCNT_LEN + SLC_CMD_LEN + 1;
-	cmd[SLC_STATE_BE_TXCNT_LEN] = 0;
+	cmd += SLCAN_STATE_BE_RXCNT_LEN + SLCAN_CMD_LEN + 1;
+	cmd[SLCAN_STATE_BE_TXCNT_LEN] = 0;
 	if (kstrtou32(cmd, 10, &txerr))
 		return;
 
 	*cmd = 0;
-	cmd -= SLC_STATE_BE_RXCNT_LEN;
+	cmd -= SLCAN_STATE_BE_RXCNT_LEN;
 	if (kstrtou32(cmd, 10, &rxerr))
 		return;
 
@@ -316,7 +317,7 @@ static void slc_bump_state(struct slcan *sl)
  * e1a : len 1, errors: ACK error
  * e3bcO: len 3, errors: Bit0 error, CRC error, Tx overrun error
  */
-static void slc_bump_err(struct slcan *sl)
+static void slcan_bump_err(struct slcan *sl)
 {
 	struct net_device *dev = sl->dev;
 	struct sk_buff *skb;
@@ -332,7 +333,7 @@ static void slc_bump_err(struct slcan *sl)
 	else
 		return;
 
-	if ((len + SLC_CMD_LEN + 1) > sl->rcount)
+	if ((len + SLCAN_CMD_LEN + 1) > sl->rcount)
 		return;
 
 	skb = alloc_can_err_skb(dev, &cf);
@@ -340,7 +341,7 @@ static void slc_bump_err(struct slcan *sl)
 	if (skb)
 		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
 
-	cmd += SLC_CMD_LEN + 1;
+	cmd += SLCAN_CMD_LEN + 1;
 	for (i = 0; i < len; i++, cmd++) {
 		switch (*cmd) {
 		case 'a':
@@ -429,7 +430,7 @@ static void slc_bump_err(struct slcan *sl)
 		netif_rx(skb);
 }
 
-static void slc_bump(struct slcan *sl)
+static void slcan_bump(struct slcan *sl)
 {
 	switch (sl->rbuff[0]) {
 	case 'r':
@@ -439,11 +440,11 @@ static void slc_bump(struct slcan *sl)
 	case 'R':
 		fallthrough;
 	case 'T':
-		return slc_bump_frame(sl);
+		return slcan_bump_frame(sl);
 	case 'e':
-		return slc_bump_err(sl);
+		return slcan_bump_err(sl);
 	case 's':
-		return slc_bump_state(sl);
+		return slcan_bump_state(sl);
 	default:
 		return;
 	}
@@ -455,12 +456,12 @@ static void slcan_unesc(struct slcan *sl, unsigned char s)
 	if ((s == '\r') || (s == '\a')) { /* CR or BEL ends the pdu */
 		if (!test_and_clear_bit(SLF_ERROR, &sl->flags) &&
 		    sl->rcount > 4)
-			slc_bump(sl);
+			slcan_bump(sl);
 
 		sl->rcount = 0;
 	} else {
 		if (!test_bit(SLF_ERROR, &sl->flags))  {
-			if (sl->rcount < SLC_MTU)  {
+			if (sl->rcount < SLCAN_MTU)  {
 				sl->rbuff[sl->rcount++] = s;
 				return;
 			}
@@ -476,7 +477,7 @@ static void slcan_unesc(struct slcan *sl, unsigned char s)
  *************************************************************************/
 
 /* Encapsulate one can_frame and stuff into a TTY queue. */
-static void slc_encaps(struct slcan *sl, struct can_frame *cf)
+static void slcan_encaps(struct slcan *sl, struct can_frame *cf)
 {
 	int actual, i;
 	unsigned char *pos;
@@ -493,11 +494,11 @@ static void slc_encaps(struct slcan *sl, struct can_frame *cf)
 	/* determine number of chars for the CAN-identifier */
 	if (cf->can_id & CAN_EFF_FLAG) {
 		id &= CAN_EFF_MASK;
-		endpos = pos + SLC_EFF_ID_LEN;
+		endpos = pos + SLCAN_EFF_ID_LEN;
 	} else {
 		*pos |= 0x20; /* convert R/T to lower case for SFF */
 		id &= CAN_SFF_MASK;
-		endpos = pos + SLC_SFF_ID_LEN;
+		endpos = pos + SLCAN_SFF_ID_LEN;
 	}
 
 	/* build 3 (SFF) or 8 (EFF) digit CAN identifier */
@@ -507,7 +508,8 @@ static void slc_encaps(struct slcan *sl, struct can_frame *cf)
 		id >>= 4;
 	}
 
-	pos += (cf->can_id & CAN_EFF_FLAG) ? SLC_EFF_ID_LEN : SLC_SFF_ID_LEN;
+	pos += (cf->can_id & CAN_EFF_FLAG) ?
+		SLCAN_EFF_ID_LEN : SLCAN_SFF_ID_LEN;
 
 	*pos++ = cf->len + '0';
 
@@ -585,7 +587,8 @@ static void slcan_write_wakeup(struct tty_struct *tty)
 }
 
 /* Send a can_frame to a TTY queue. */
-static netdev_tx_t slc_xmit(struct sk_buff *skb, struct net_device *dev)
+static netdev_tx_t slcan_netdev_xmit(struct sk_buff *skb,
+				     struct net_device *dev)
 {
 	struct slcan *sl = netdev_priv(dev);
 
@@ -604,7 +607,7 @@ static netdev_tx_t slc_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	netif_stop_queue(sl->dev);
-	slc_encaps(sl, (struct can_frame *)skb->data); /* encaps & send */
+	slcan_encaps(sl, (struct can_frame *)skb->data); /* encaps & send */
 	spin_unlock(&sl->lock);
 
 out:
@@ -647,7 +650,7 @@ static int slcan_transmit_cmd(struct slcan *sl, const unsigned char *cmd)
 }
 
 /* Netdevice UP -> DOWN routine */
-static int slc_close(struct net_device *dev)
+static int slcan_netdev_close(struct net_device *dev)
 {
 	struct slcan *sl = netdev_priv(dev);
 	int err;
@@ -676,10 +679,10 @@ static int slc_close(struct net_device *dev)
 }
 
 /* Netdevice DOWN -> UP routine */
-static int slc_open(struct net_device *dev)
+static int slcan_netdev_open(struct net_device *dev)
 {
 	struct slcan *sl = netdev_priv(dev);
-	unsigned char cmd[SLC_MTU];
+	unsigned char cmd[SLCAN_MTU];
 	int err, s;
 
 	/* The baud rate is not set with the command
@@ -739,16 +742,16 @@ static int slc_open(struct net_device *dev)
 	return err;
 }
 
-static int slcan_change_mtu(struct net_device *dev, int new_mtu)
+static int slcan_netdev_change_mtu(struct net_device *dev, int new_mtu)
 {
 	return -EINVAL;
 }
 
-static const struct net_device_ops slc_netdev_ops = {
-	.ndo_open               = slc_open,
-	.ndo_stop               = slc_close,
-	.ndo_start_xmit         = slc_xmit,
-	.ndo_change_mtu         = slcan_change_mtu,
+static const struct net_device_ops slcan_netdev_ops = {
+	.ndo_open               = slcan_netdev_open,
+	.ndo_stop               = slcan_netdev_close,
+	.ndo_start_xmit         = slcan_netdev_xmit,
+	.ndo_change_mtu         = slcan_netdev_change_mtu,
 };
 
 /******************************************
@@ -821,7 +824,7 @@ static int slcan_open(struct tty_struct *tty)
 
 	/* Configure netdev interface */
 	sl->dev	= dev;
-	dev->netdev_ops = &slc_netdev_ops;
+	dev->netdev_ops = &slcan_netdev_ops;
 	slcan_set_ethtool_ops(dev);
 
 	/* Mark ldisc channel as alive */
@@ -889,7 +892,7 @@ static int slcan_ioctl(struct tty_struct *tty, unsigned int cmd,
 	}
 }
 
-static struct tty_ldisc_ops slc_ldisc = {
+static struct tty_ldisc_ops slcan_ldisc = {
 	.owner		= THIS_MODULE,
 	.num		= N_SLCAN,
 	.name		= KBUILD_MODNAME,
@@ -907,7 +910,7 @@ static int __init slcan_init(void)
 	pr_info("serial line CAN interface driver\n");
 
 	/* Fill in our line protocol discipline, and register it */
-	status = tty_register_ldisc(&slc_ldisc);
+	status = tty_register_ldisc(&slcan_ldisc);
 	if (status)
 		pr_err("can't register line discipline\n");
 
@@ -919,7 +922,7 @@ static void __exit slcan_exit(void)
 	/* This will only be called when all channels have been closed by
 	 * userspace - tty_ldisc.c takes care of the module's refcount.
 	 */
-	tty_unregister_ldisc(&slc_ldisc);
+	tty_unregister_ldisc(&slcan_ldisc);
 }
 
 module_init(slcan_init);
-- 
2.32.0

[RFC PATCH v3 5/9] can: slcan: use the generic can_change_mtu()

[Dario Binacchi]

It is useless to define a custom function that does nothing but always
return the same error code. Better to use the generic can_change_mtu()
function.

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

(no changes since v1)

 drivers/net/can/slcan/slcan-core.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
index bd8e84ded051..d12d98426f37 100644
--- a/drivers/net/can/slcan/slcan-core.c
+++ b/drivers/net/can/slcan/slcan-core.c
@@ -742,16 +742,11 @@ static int slcan_netdev_open(struct net_device *dev)
 	return err;
 }
 
-static int slcan_netdev_change_mtu(struct net_device *dev, int new_mtu)
-{
-	return -EINVAL;
-}
-
 static const struct net_device_ops slcan_netdev_ops = {
 	.ndo_open               = slcan_netdev_open,
 	.ndo_stop               = slcan_netdev_close,
 	.ndo_start_xmit         = slcan_netdev_xmit,
-	.ndo_change_mtu         = slcan_netdev_change_mtu,
+	.ndo_change_mtu         = can_change_mtu,
 };
 
 /******************************************
-- 
2.32.0

[RFC PATCH v3 6/9] can: slcan: add support for listen-only mode

[Dario Binacchi]

For non-legacy, i.e. ip based configuration, add support for listen-only
mode. If listen-only is requested send a listen-only ("L\r") command
instead of an open ("O\r") command to the adapter.

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

---

(no changes since v2)

Changes in v2:
- Remove comment on listen-only command.
- Update the commit subject and description.

 drivers/net/can/slcan/slcan-core.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
index d12d98426f37..45e521910236 100644
--- a/drivers/net/can/slcan/slcan-core.c
+++ b/drivers/net/can/slcan/slcan-core.c
@@ -726,10 +726,20 @@ static int slcan_netdev_open(struct net_device *dev)
 			}
 		}
 
-		err = slcan_transmit_cmd(sl, "O\r");
-		if (err) {
-			netdev_err(dev, "failed to send open command 'O\\r'\n");
-			goto cmd_transmit_failed;
+		if (sl->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) {
+			err = slcan_transmit_cmd(sl, "L\r");
+			if (err) {
+				netdev_err(dev,
+					   "failed to send listen-only command 'L\\r'\n");
+				goto cmd_transmit_failed;
+			}
+		} else {
+			err = slcan_transmit_cmd(sl, "O\r");
+			if (err) {
+				netdev_err(dev,
+					   "failed to send open command 'O\\r'\n");
+				goto cmd_transmit_failed;
+			}
 		}
 	}
 
@@ -816,6 +826,7 @@ static int slcan_open(struct tty_struct *tty)
 	/* Configure CAN metadata */
 	sl->can.bitrate_const = slcan_bitrate_const;
 	sl->can.bitrate_const_cnt = ARRAY_SIZE(slcan_bitrate_const);
+	sl->can.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY;
 
 	/* Configure netdev interface */
 	sl->dev	= dev;
-- 
2.32.0

[RFC PATCH v3 7/9] ethtool: add support to get/set CAN bit time register

[Dario Binacchi]

Add ethtool support to get/set tunable values from/to the CAN bit time
register (btr).

CC: Marc Kleine-Budde <mkl@pengutronix.de>
CC: linux-can@vger.kernel.org
Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
---

(no changes since v1)

 include/uapi/linux/ethtool.h | 1 +
 net/ethtool/common.c         | 1 +
 net/ethtool/ioctl.c          | 1 +
 3 files changed, 3 insertions(+)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index e0f0ee9bc89e..a2d24f689124 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -232,6 +232,7 @@ enum tunable_id {
 	ETHTOOL_TX_COPYBREAK,
 	ETHTOOL_PFC_PREVENTION_TOUT, /* timeout in msecs */
 	ETHTOOL_TX_COPYBREAK_BUF_SIZE,
+	ETHTOOL_CAN_BTR,
 	/*
 	 * Add your fresh new tunable attribute above and remember to update
 	 * tunable_strings[] in net/ethtool/common.c
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 566adf85e658..78f23b898243 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -90,6 +90,7 @@ tunable_strings[__ETHTOOL_TUNABLE_COUNT][ETH_GSTRING_LEN] = {
 	[ETHTOOL_TX_COPYBREAK]	= "tx-copybreak",
 	[ETHTOOL_PFC_PREVENTION_TOUT] = "pfc-prevention-tout",
 	[ETHTOOL_TX_COPYBREAK_BUF_SIZE] = "tx-copybreak-buf-size",
+	[ETHTOOL_CAN_BTR] = "can-btr",
 };
 
 const char
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 326e14ee05db..17b69d6fcab4 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2403,6 +2403,7 @@ static int ethtool_get_module_eeprom(struct net_device *dev,
 static int ethtool_tunable_valid(const struct ethtool_tunable *tuna)
 {
 	switch (tuna->id) {
+	case ETHTOOL_CAN_BTR:
 	case ETHTOOL_RX_COPYBREAK:
 	case ETHTOOL_TX_COPYBREAK:
 	case ETHTOOL_TX_COPYBREAK_BUF_SIZE:
-- 
2.32.0

[RFC PATCH v3 8/9] can: slcan: add support to set bit time register (btr)

[Dario Binacchi]

It allows to set the bit time register with tunable values.
The setting can only be changed if the interface is down:

ip link set dev can0 down
ethtool --set-tunable can0 can-btr 0x31c
ip link set dev can0 up

Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
---

(no changes since v1)

 drivers/net/can/slcan/slcan-core.c    | 58 ++++++++++++++++++++-------
 drivers/net/can/slcan/slcan-ethtool.c | 13 ++++++
 drivers/net/can/slcan/slcan.h         |  1 +
 3 files changed, 58 insertions(+), 14 deletions(-)

diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
index 45e521910236..3905f21e7788 100644
--- a/drivers/net/can/slcan/slcan-core.c
+++ b/drivers/net/can/slcan/slcan-core.c
@@ -99,6 +99,7 @@ struct slcan {
 #define CF_ERR_RST		0               /* Reset errors on open      */
 	wait_queue_head_t       xcmd_wait;      /* Wait queue for commands   */
 						/* transmission              */
+	u32 btr;				/* bit timing register       */
 };
 
 static const u32 slcan_bitrate_const[] = {
@@ -128,6 +129,17 @@ int slcan_enable_err_rst_on_open(struct net_device *ndev, bool on)
 	return 0;
 }
 
+int slcan_set_btr(struct net_device *ndev, u32 btr)
+{
+	struct slcan *sl = netdev_priv(ndev);
+
+	if (netif_running(ndev))
+		return -EBUSY;
+
+	sl->btr = btr;
+	return 0;
+}
+
 /*************************************************************************
  *			SLCAN ENCAPSULATION FORMAT			 *
  *************************************************************************/
@@ -699,22 +711,40 @@ static int slcan_netdev_open(struct net_device *dev)
 		return err;
 	}
 
-	if (sl->can.bittiming.bitrate != CAN_BITRATE_UNKNOWN) {
-		for (s = 0; s < ARRAY_SIZE(slcan_bitrate_const); s++) {
-			if (sl->can.bittiming.bitrate == slcan_bitrate_const[s])
-				break;
+	if (sl->can.bittiming.bitrate != CAN_BITRATE_UNKNOWN || sl->btr) {
+		if (sl->can.bittiming.bitrate != CAN_BITRATE_UNKNOWN) {
+			for (s = 0; s < ARRAY_SIZE(slcan_bitrate_const); s++) {
+				if (sl->can.bittiming.bitrate ==
+				    slcan_bitrate_const[s])
+					break;
+			}
+
+			/* The CAN framework has already validate the bitrate
+			 * value, so we can avoid to check if `s' has been
+			 * properly set.
+			 */
+			snprintf(cmd, sizeof(cmd), "C\rS%d\r", s);
+			err = slcan_transmit_cmd(sl, cmd);
+			if (err) {
+				netdev_err(dev,
+					   "failed to send bitrate command 'C\\rS%d\\r'\n",
+					   s);
+				goto cmd_transmit_failed;
+			}
 		}
 
-		/* The CAN framework has already validate the bitrate value,
-		 * so we can avoid to check if `s' has been properly set.
-		 */
-		snprintf(cmd, sizeof(cmd), "C\rS%d\r", s);
-		err = slcan_transmit_cmd(sl, cmd);
-		if (err) {
-			netdev_err(dev,
-				   "failed to send bitrate command 'C\\rS%d\\r'\n",
-				   s);
-			goto cmd_transmit_failed;
+		if (sl->btr) {
+			u32 btr = sl->btr & GENMASK(15, 0);
+
+			snprintf(cmd, sizeof(cmd), "C\rs%04x\r", btr);
+			err = slcan_transmit_cmd(sl, cmd);
+			if (err) {
+				netdev_err(dev,
+					   "failed to send bit timing command 'C\\rs%04x\\r'\n",
+					   btr);
+				goto cmd_transmit_failed;
+			}
+
 		}
 
 		if (test_bit(CF_ERR_RST, &sl->cmd_flags)) {
diff --git a/drivers/net/can/slcan/slcan-ethtool.c b/drivers/net/can/slcan/slcan-ethtool.c
index bf0afdc4e49d..8e2e77bbffda 100644
--- a/drivers/net/can/slcan/slcan-ethtool.c
+++ b/drivers/net/can/slcan/slcan-ethtool.c
@@ -52,11 +52,24 @@ static int slcan_get_sset_count(struct net_device *netdev, int sset)
 	}
 }
 
+static int slcan_set_tunable(struct net_device *netdev,
+			     const struct ethtool_tunable *tuna,
+			     const void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_CAN_BTR:
+		return slcan_set_btr(netdev, *(u32 *)data);
+	default:
+		return -EINVAL;
+	}
+}
+
 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,
+	.set_tunable = slcan_set_tunable,
 };
 
 void slcan_set_ethtool_ops(struct net_device *netdev)
diff --git a/drivers/net/can/slcan/slcan.h b/drivers/net/can/slcan/slcan.h
index d463c8d99e22..1ac412fe8c95 100644
--- a/drivers/net/can/slcan/slcan.h
+++ b/drivers/net/can/slcan/slcan.h
@@ -13,6 +13,7 @@
 
 bool slcan_err_rst_on_open(struct net_device *ndev);
 int slcan_enable_err_rst_on_open(struct net_device *ndev, bool on);
+int slcan_set_btr(struct net_device *ndev, u32 btr);
 void slcan_set_ethtool_ops(struct net_device *ndev);
 
 #endif /* _SLCAN_H */
-- 
2.32.0

Re: [RFC PATCH v3 8/9] can: slcan: add support to set bit time register (btr)

[Marc Kleine-Budde]

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

On 26.07.2022 23:02:16, Dario Binacchi wrote:
> It allows to set the bit time register with tunable values.
> The setting can only be changed if the interface is down:
> 
> ip link set dev can0 down
> ethtool --set-tunable can0 can-btr 0x31c
> ip link set dev can0 up

As far as I understand, setting the btr is an alternative way to set the
bitrate, right? I don't like the idea of poking arbitrary values into a
hardware from user space.

Do you have a use case for this?

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 v3 8/9] can: slcan: add support to set bit time register (btr)

[Dario Binacchi]

Hello Marc,

On Wed, Jul 27, 2022 at 1:31 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 26.07.2022 23:02:16, Dario Binacchi wrote:
> > It allows to set the bit time register with tunable values.
> > The setting can only be changed if the interface is down:
> >
> > ip link set dev can0 down
> > ethtool --set-tunable can0 can-btr 0x31c
> > ip link set dev can0 up
>
> As far as I understand, setting the btr is an alternative way to set the
> bitrate, right?

I thought of a non-standard bitrate or, in addition to the bitrate, the
possibility of enabling some specific CAN controller options. Maybe Oliver
could help us come up with the right answer.

This is the the slcan source code:
https://github.com/linux-can/can-utils/blob/cad1cecf1ca19277b5f5db39f8ef6f8ae426191d/slcand.c#L331
btr case cames after speed but they don't seem to be considered alternative.

> I don't like the idea of poking arbitrary values into a
> hardware from user space.

However this is already possible through the slcand and slcan_attach
applications.
Furthermore, the driver implements the LAWICEL ASCII protocol for CAN
frame transport over serial lines,
and this is one of the supported commands.

>
> Do you have a use case for this?

I use the applications slcand and slcan_attach as a reference, I try to make the
driver independent from them for what concerns the CAN setup. And the bit time
register setting is the last dependency.

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 v3 8/9] can: slcan: add support to set bit time register (btr)

[Marc Kleine-Budde]

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

On 27.07.2022 17:55:10, Dario Binacchi wrote:
> Hello Marc,
> 
> On Wed, Jul 27, 2022 at 1:31 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >
> > On 26.07.2022 23:02:16, Dario Binacchi wrote:
> > > It allows to set the bit time register with tunable values.
> > > The setting can only be changed if the interface is down:
> > >
> > > ip link set dev can0 down
> > > ethtool --set-tunable can0 can-btr 0x31c
> > > ip link set dev can0 up
> >
> > As far as I understand, setting the btr is an alternative way to set the
> > bitrate, right?
> 
> I thought of a non-standard bitrate or, in addition to the bitrate, the
> possibility of enabling some specific CAN controller options. Maybe Oliver
> could help us come up with the right answer.
> 
> This is the the slcan source code:
> https://github.com/linux-can/can-utils/blob/cad1cecf1ca19277b5f5db39f8ef6f8ae426191d/slcand.c#L331
> btr case cames after speed but they don't seem to be considered alternative.
> 
> > I don't like the idea of poking arbitrary values into a
> > hardware from user space.
> 
> However this is already possible through the slcand and slcan_attach
> applications.
> Furthermore, the driver implements the LAWICEL ASCII protocol for CAN
> frame transport over serial lines,
> and this is one of the supported commands.
> 
> >
> > Do you have a use case for this?
> 
> I use the applications slcand and slcan_attach as a reference, I try to make the
> driver independent from them for what concerns the CAN setup. And the bit time
> register setting is the last dependency.

Ok - We avoided writing bit timing registers from user space into the
hardware for all existing drivers. If there isn't a specific use case,
let's skip this patch. If someone comes up with a use case we can think
of a proper solution.

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 v3 8/9] can: slcan: add support to set bit time register (btr)

[Dario Binacchi]

On Wed, Jul 27, 2022 at 7:21 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 27.07.2022 17:55:10, Dario Binacchi wrote:
> > Hello Marc,
> >
> > On Wed, Jul 27, 2022 at 1:31 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > >
> > > On 26.07.2022 23:02:16, Dario Binacchi wrote:
> > > > It allows to set the bit time register with tunable values.
> > > > The setting can only be changed if the interface is down:
> > > >
> > > > ip link set dev can0 down
> > > > ethtool --set-tunable can0 can-btr 0x31c
> > > > ip link set dev can0 up
> > >
> > > As far as I understand, setting the btr is an alternative way to set the
> > > bitrate, right?
> >
> > I thought of a non-standard bitrate or, in addition to the bitrate, the
> > possibility of enabling some specific CAN controller options. Maybe Oliver
> > could help us come up with the right answer.
> >
> > This is the the slcan source code:
> > https://github.com/linux-can/can-utils/blob/cad1cecf1ca19277b5f5db39f8ef6f8ae426191d/slcand.c#L331
> > btr case cames after speed but they don't seem to be considered alternative.
> >
> > > I don't like the idea of poking arbitrary values into a
> > > hardware from user space.
> >
> > However this is already possible through the slcand and slcan_attach
> > applications.
> > Furthermore, the driver implements the LAWICEL ASCII protocol for CAN
> > frame transport over serial lines,
> > and this is one of the supported commands.
> >
> > >
> > > Do you have a use case for this?
> >
> > I use the applications slcand and slcan_attach as a reference, I try to make the
> > driver independent from them for what concerns the CAN setup. And the bit time
> > register setting is the last dependency.
>
> Ok - We avoided writing bit timing registers from user space into the
> hardware for all existing drivers. If there isn't a specific use case,
> let's skip this patch. If someone comes up with a use case we can think
> of a proper solution.

Ok. So do I also remove the 7/9 "ethtool: add support to get/set CAN
bit time register"
patch ?

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 v3 8/9] can: slcan: add support to set bit time register (btr)

[Max Staudt]

On Wed, 27 Jul 2022 19:28:45 +0200
Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote:

> On Wed, Jul 27, 2022 at 7:21 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >
> > Ok - We avoided writing bit timing registers from user space into the
> > hardware for all existing drivers. If there isn't a specific use case,
> > let's skip this patch. If someone comes up with a use case we can think
> > of a proper solution.  
> 
> Ok. So do I also remove the 7/9 "ethtool: add support to get/set CAN
> bit time register"
> patch ?

If I may answer as well - IMHO, yes.

Unless we know that BTR is something other than just a different way to
express the bitrate, I'd skip it, yes. Because bitrate is already
handled by other, cross-device mechanisms.


Max

Re: [RFC PATCH v3 8/9] can: slcan: add support to set bit time register (btr)

[Dario Binacchi]

Hello Marc and Max,

On Wed, Jul 27, 2022 at 7:33 PM Max Staudt <max@enpas.org> wrote:
>
> On Wed, 27 Jul 2022 19:28:45 +0200
> Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote:
>
> > On Wed, Jul 27, 2022 at 7:21 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > >
> > > Ok - We avoided writing bit timing registers from user space into the
> > > hardware for all existing drivers. If there isn't a specific use case,
> > > let's skip this patch. If someone comes up with a use case we can think
> > > of a proper solution.
> >
> > Ok. So do I also remove the 7/9 "ethtool: add support to get/set CAN
> > bit time register"
> > patch ?
>
> If I may answer as well - IMHO, yes.
>
> Unless we know that BTR is something other than just a different way to
> express the bitrate, I'd skip it, yes. Because bitrate is already
> handled by other, cross-device mechanisms.

Thanks to both of you for the explanations.
Regards,

Dario

>
>
> Max

-- 

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 v3 8/9] can: slcan: add support to set bit time register (btr)

[Max Staudt]

On Wed, 27 Jul 2022 13:30:54 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> As far as I understand, setting the btr is an alternative way to set the
> bitrate, right? I don't like the idea of poking arbitrary values into a
> hardware from user space.

I agree with Marc here.

This is a modification across the whole stack, specific to a single
device, when there are ways around.


If I understand correctly, the CAN232 "S" command sets one of the fixed
bitrates, whereas "s" sets the two BTR registers. Now the question is,
what do BTR0/BTR1 mean, and what are they? If they are merely a divider
in a CAN controller's master clock, like in ELM327, then you could

  a) Calculate the BTR values from the bitrate userspace requests, or

  b) pre-calculate a huge table of possible bitrates and present them
     all to userspace. Sounds horrible, but that's what I did in can327,
     haha. Maybe I should have reigned them in a little, to the most
     useful values.

  c) just limit the bitrates to whatever seems most useful (like the
     "S" command's table), and let users complain if they really need
     something else. In the meantime, they are still free to slcand or
     minicom to their heart's content before attaching slcan, thanks to
     your backwards compatibility efforts.


In short, to me, this isn't a deal breaker for your patch series.


Max

Re: [RFC PATCH v3 8/9] can: slcan: add support to set bit time register (btr)

[Marc Kleine-Budde]

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

On 27.07.2022 19:28:39, Max Staudt wrote:
> On Wed, 27 Jul 2022 13:30:54 +0200
> Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> 
> > As far as I understand, setting the btr is an alternative way to set the
> > bitrate, right? I don't like the idea of poking arbitrary values into a
> > hardware from user space.
> 
> I agree with Marc here.
> 
> This is a modification across the whole stack, specific to a single
> device, when there are ways around.
> 
> If I understand correctly, the CAN232 "S" command sets one of the fixed
> bitrates, whereas "s" sets the two BTR registers. Now the question is,
> what do BTR0/BTR1 mean, and what are they? If they are merely a divider
> in a CAN controller's master clock, like in ELM327, then you could
> 
>   a) Calculate the BTR values from the bitrate userspace requests, or

Most of the other CAN drivers write the BTR values into the register of
the hardware. How are these BTR values transported into the driver?

There are 2 ways:

1) - user space configures a bitrate
   - the kernel calculates with the "struct can_bittiming_const" [1] given
     by driver and the CAN clock rate the low level timing parameters.

     [1] https://elixir.bootlin.com/linux/v5.18/source/include/uapi/linux/can/netlink.h#L47

2) - user space configures low level bit timing parameter
     (Sample point in one-tenth of a percent, Time quanta (TQ) in
      nanoseconds, Propagation segment in TQs, Phase buffer segment 1 in
      TQs, Phase buffer segment 2 in TQs, Synchronisation jump width in
      TQs)
    - the kernel calculates the Bit-rate prescaler from the given TQ and
      CAN clock rate

Both ways result in a fully calculated "struct can_bittiming" [2]. The
driver translates this into the hardware specific BTR values and writes
the into the registers.

If you know the CAN clock and the bit timing const parameters of the
slcan's BTR register you can make use of the automatic BTR calculation,
too. Maybe the framework needs some tweaking if the driver supports both
fixed CAN bit rate _and_ "struct can_bittiming_const".

[2] https://elixir.bootlin.com/linux/v5.18/source/include/uapi/linux/can/netlink.h#L31

>   b) pre-calculate a huge table of possible bitrates and present them
>      all to userspace. Sounds horrible, but that's what I did in can327,
>      haha. Maybe I should have reigned them in a little, to the most
>      useful values.

If your adapter only supports fixed values, then that's the only way to
go.

>   c) just limit the bitrates to whatever seems most useful (like the
>      "S" command's table), and let users complain if they really need
>      something else. In the meantime, they are still free to slcand or
>      minicom to their heart's content before attaching slcan, thanks to
>      your backwards compatibility efforts.

In the early stages of the non-mainline CAN framework we had tables for
BTR values for some fixed bit rates, but that turned out to be not
scaleable.

> In short, to me, this isn't a deal breaker for your patch series.

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 v3 8/9] can: slcan: add support to set bit time register (btr)

[Oliver Hartkopp]

On 27.07.22 20:24, Marc Kleine-Budde wrote:
> On 27.07.2022 19:28:39, Max Staudt wrote:
>> On Wed, 27 Jul 2022 13:30:54 +0200
>> Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>
>>> As far as I understand, setting the btr is an alternative way to set the
>>> bitrate, right? I don't like the idea of poking arbitrary values into a
>>> hardware from user space.
>>
>> I agree with Marc here.
>>
>> This is a modification across the whole stack, specific to a single
>> device, when there are ways around.
>>
>> If I understand correctly, the CAN232 "S" command sets one of the fixed
>> bitrates, whereas "s" sets the two BTR registers. Now the question is,
>> what do BTR0/BTR1 mean, and what are they? If they are merely a divider
>> in a CAN controller's master clock, like in ELM327, then you could
>>
>>    a) Calculate the BTR values from the bitrate userspace requests, or
> 
> Most of the other CAN drivers write the BTR values into the register of
> the hardware. How are these BTR values transported into the driver?
> 
> There are 2 ways:
> 
> 1) - user space configures a bitrate
>     - the kernel calculates with the "struct can_bittiming_const" [1] given
>       by driver and the CAN clock rate the low level timing parameters.
> 
>       [1] https://elixir.bootlin.com/linux/v5.18/source/include/uapi/linux/can/netlink.h#L47
> 
> 2) - user space configures low level bit timing parameter
>       (Sample point in one-tenth of a percent, Time quanta (TQ) in
>        nanoseconds, Propagation segment in TQs, Phase buffer segment 1 in
>        TQs, Phase buffer segment 2 in TQs, Synchronisation jump width in
>        TQs)
>      - the kernel calculates the Bit-rate prescaler from the given TQ and
>        CAN clock rate
> 
> Both ways result in a fully calculated "struct can_bittiming" [2]. The
> driver translates this into the hardware specific BTR values and writes
> the into the registers.
> 
> If you know the CAN clock and the bit timing const parameters of the
> slcan's BTR register you can make use of the automatic BTR calculation,
> too. Maybe the framework needs some tweaking if the driver supports both
> fixed CAN bit rate _and_ "struct can_bittiming_const".
> 
> [2] https://elixir.bootlin.com/linux/v5.18/source/include/uapi/linux/can/netlink.h#L31
> 
>>    b) pre-calculate a huge table of possible bitrates and present them
>>       all to userspace. Sounds horrible, but that's what I did in can327,
>>       haha. Maybe I should have reigned them in a little, to the most
>>       useful values.
> 
> If your adapter only supports fixed values, then that's the only way to
> go.
> 
>>    c) just limit the bitrates to whatever seems most useful (like the
>>       "S" command's table), and let users complain if they really need
>>       something else. In the meantime, they are still free to slcand or
>>       minicom to their heart's content before attaching slcan, thanks to
>>       your backwards compatibility efforts.
> 
> In the early stages of the non-mainline CAN framework we had tables for
> BTR values for some fixed bit rates, but that turned out to be not
> scaleable.

The Microchip CAN BUS Analyzer Tool is another example for fixed bitrates:

https://elixir.bootlin.com/linux/v5.18.14/source/drivers/net/can/usb/mcba_usb.c#L156

So this might be the way to go here too ...

Best regards,
Oliver

> 
>> In short, to me, this isn't a deal breaker for your patch series.
> 
> Marc
> 

Re: [RFC PATCH v3 8/9] can: slcan: add support to set bit time register (btr)

[Marc Kleine-Budde]

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

On 27.07.2022 22:12:13, Oliver Hartkopp wrote:
> 
> 
> On 27.07.22 20:24, Marc Kleine-Budde wrote:
> > On 27.07.2022 19:28:39, Max Staudt wrote:
> > > On Wed, 27 Jul 2022 13:30:54 +0200
> > > Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > > 
> > > > As far as I understand, setting the btr is an alternative way to set the
> > > > bitrate, right? I don't like the idea of poking arbitrary values into a
> > > > hardware from user space.
> > > 
> > > I agree with Marc here.
> > > 
> > > This is a modification across the whole stack, specific to a single
> > > device, when there are ways around.
> > > 
> > > If I understand correctly, the CAN232 "S" command sets one of the fixed
> > > bitrates, whereas "s" sets the two BTR registers. Now the question is,
> > > what do BTR0/BTR1 mean, and what are they? If they are merely a divider
> > > in a CAN controller's master clock, like in ELM327, then you could
> > > 
> > >    a) Calculate the BTR values from the bitrate userspace requests, or
> > 
> > Most of the other CAN drivers write the BTR values into the register of
> > the hardware. How are these BTR values transported into the driver?
> > 
> > There are 2 ways:
> > 
> > 1) - user space configures a bitrate
> >     - the kernel calculates with the "struct can_bittiming_const" [1] given
> >       by driver and the CAN clock rate the low level timing parameters.
> > 
> >       [1] https://elixir.bootlin.com/linux/v5.18/source/include/uapi/linux/can/netlink.h#L47
> > 
> > 2) - user space configures low level bit timing parameter
> >       (Sample point in one-tenth of a percent, Time quanta (TQ) in
> >        nanoseconds, Propagation segment in TQs, Phase buffer segment 1 in
> >        TQs, Phase buffer segment 2 in TQs, Synchronisation jump width in
> >        TQs)
> >      - the kernel calculates the Bit-rate prescaler from the given TQ and
> >        CAN clock rate
> > 
> > Both ways result in a fully calculated "struct can_bittiming" [2]. The
> > driver translates this into the hardware specific BTR values and writes
> > the into the registers.
> > 
> > If you know the CAN clock and the bit timing const parameters of the
> > slcan's BTR register you can make use of the automatic BTR calculation,
> > too. Maybe the framework needs some tweaking if the driver supports both
> > fixed CAN bit rate _and_ "struct can_bittiming_const".
> > 
> > [2] https://elixir.bootlin.com/linux/v5.18/source/include/uapi/linux/can/netlink.h#L31
> > 
> > >    b) pre-calculate a huge table of possible bitrates and present them
> > >       all to userspace. Sounds horrible, but that's what I did in can327,
> > >       haha. Maybe I should have reigned them in a little, to the most
> > >       useful values.
> > 
> > If your adapter only supports fixed values, then that's the only way to
> > go.
> > 
> > >    c) just limit the bitrates to whatever seems most useful (like the
> > >       "S" command's table), and let users complain if they really need
> > >       something else. In the meantime, they are still free to slcand or
> > >       minicom to their heart's content before attaching slcan, thanks to
> > >       your backwards compatibility efforts.
> > 
> > In the early stages of the non-mainline CAN framework we had tables for
> > BTR values for some fixed bit rates, but that turned out to be not
> > scaleable.
> 
> The Microchip CAN BUS Analyzer Tool is another example for fixed bitrates:
> 
> https://elixir.bootlin.com/linux/v5.18.14/source/drivers/net/can/usb/mcba_usb.c#L156
> 
> So this might be the way to go here too ...

The slcan driver already supports fixed bitrates. This discussion is
about setting the BTR registers in one way or another.

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 v3 8/9] can: slcan: add support to set bit time register (btr)

[Dario Binacchi]

Hello Marc,

On Wed, Jul 27, 2022 at 8:24 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 27.07.2022 19:28:39, Max Staudt wrote:
> > On Wed, 27 Jul 2022 13:30:54 +0200
> > Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >
> > > As far as I understand, setting the btr is an alternative way to set the
> > > bitrate, right? I don't like the idea of poking arbitrary values into a
> > > hardware from user space.
> >
> > I agree with Marc here.
> >
> > This is a modification across the whole stack, specific to a single
> > device, when there are ways around.
> >
> > If I understand correctly, the CAN232 "S" command sets one of the fixed
> > bitrates, whereas "s" sets the two BTR registers. Now the question is,
> > what do BTR0/BTR1 mean, and what are they? If they are merely a divider
> > in a CAN controller's master clock, like in ELM327, then you could
> >
> >   a) Calculate the BTR values from the bitrate userspace requests, or
>
> Most of the other CAN drivers write the BTR values into the register of
> the hardware. How are these BTR values transported into the driver?
>
> There are 2 ways:
>
> 1) - user space configures a bitrate
>    - the kernel calculates with the "struct can_bittiming_const" [1] given
>      by driver and the CAN clock rate the low level timing parameters.
>
>      [1] https://elixir.bootlin.com/linux/v5.18/source/include/uapi/linux/can/netlink.h#L47
>
> 2) - user space configures low level bit timing parameter
>      (Sample point in one-tenth of a percent, Time quanta (TQ) in
>       nanoseconds, Propagation segment in TQs, Phase buffer segment 1 in
>       TQs, Phase buffer segment 2 in TQs, Synchronisation jump width in
>       TQs)
>     - the kernel calculates the Bit-rate prescaler from the given TQ and
>       CAN clock rate
>
> Both ways result in a fully calculated "struct can_bittiming" [2]. The
> driver translates this into the hardware specific BTR values and writes
> the into the registers.
>
> If you know the CAN clock and the bit timing const parameters of the
> slcan's BTR register you can make use of the automatic BTR calculation,
> too. Maybe the framework needs some tweaking if the driver supports both
> fixed CAN bit rate _and_ "struct can_bittiming_const".

Does it make sense to use the device tree to provide the driver with those
parameters required for the automatic calculation of the BTR (clock rate,
struct can_bittiming_const, ...) that depend on the connected controller? In
this way the solution should be generic and therefore scalable. I think we
should also add some properties to map the calculated BTR value on the
physical register of the controller.

Or, use the device tree to extend the bittates supported by the controller
to the fixed ones (struct can_priv::bitrate_const)?

Thanks and regards,
Dario

>
> [2] https://elixir.bootlin.com/linux/v5.18/source/include/uapi/linux/can/netlink.h#L31
>
> >   b) pre-calculate a huge table of possible bitrates and present them
> >      all to userspace. Sounds horrible, but that's what I did in can327,
> >      haha. Maybe I should have reigned them in a little, to the most
> >      useful values.
>
> If your adapter only supports fixed values, then that's the only way to
> go.
>
> >   c) just limit the bitrates to whatever seems most useful (like the
> >      "S" command's table), and let users complain if they really need
> >      something else. In the meantime, they are still free to slcand or
> >      minicom to their heart's content before attaching slcan, thanks to
> >      your backwards compatibility efforts.
>
> In the early stages of the non-mainline CAN framework we had tables for
> BTR values for some fixed bit rates, but that turned out to be not
> scaleable.
>
> > In short, to me, this isn't a deal breaker for your patch series.
>
> 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 v3 8/9] can: slcan: add support to set bit time register (btr)

[Marc Kleine-Budde]

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

On 28.07.2022 09:36:21, Dario Binacchi wrote:
> > Most of the other CAN drivers write the BTR values into the register of
> > the hardware. How are these BTR values transported into the driver?
> >
> > There are 2 ways:
> >
> > 1) - user space configures a bitrate
> >    - the kernel calculates with the "struct can_bittiming_const" [1] given
> >      by driver and the CAN clock rate the low level timing parameters.
> >
> >      [1] https://elixir.bootlin.com/linux/v5.18/source/include/uapi/linux/can/netlink.h#L47
> >
> > 2) - user space configures low level bit timing parameter
> >      (Sample point in one-tenth of a percent, Time quanta (TQ) in
> >       nanoseconds, Propagation segment in TQs, Phase buffer segment 1 in
> >       TQs, Phase buffer segment 2 in TQs, Synchronisation jump width in
> >       TQs)
> >     - the kernel calculates the Bit-rate prescaler from the given TQ and
> >       CAN clock rate
> >
> > Both ways result in a fully calculated "struct can_bittiming" [2]. The
> > driver translates this into the hardware specific BTR values and writes
> > the into the registers.
> >
> > If you know the CAN clock and the bit timing const parameters of the
> > slcan's BTR register you can make use of the automatic BTR calculation,
> > too. Maybe the framework needs some tweaking if the driver supports both
> > fixed CAN bit rate _and_ "struct can_bittiming_const".
> 
> Does it make sense to use the device tree

The driver doesn't support DT and DT only works for static serial
interfaces.

> to provide the driver with those
> parameters required for the automatic calculation of the BTR (clock rate,
> struct can_bittiming_const, ...) that depend on the connected
> controller?

The device tree usually says it's a CAN controller compatible to X and
the following clock(s) are connected. The driver for CAN controller X
knows the bit timing const. Some USB CAN drivers query the bit timing
const from the USB device.

> In this way the solution should be generic and therefore scalable. I
> think we should also add some properties to map the calculated BTR
> value on the physical register of the controller.

The driver knows how to map the "struct can_bittiming" to the BTR
register values of the hardware.

What does the serial protocol say to the BTR values? Are these standard
SJA1000 layout with 8 MHz CAN clock or are those adapter specific?

> Or, use the device tree to extend the bittates supported by the controller
> to the fixed ones (struct can_priv::bitrate_const)?

The serial protocol defines fixed bit rates, no need to describe them in
the DT:

|           0            10 Kbit/s
|           1            20 Kbit/s
|           2            50 Kbit/s
|           3           100 Kbit/s
|           4           125 Kbit/s
|           5           250 Kbit/s
|           6           500 Kbit/s
|           7           800 Kbit/s
|           8          1000 Kbit/s

Are there more bit rates?

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 v3 8/9] can: slcan: add support to set bit time register (btr)

[Dario Binacchi]

On Thu, Jul 28, 2022 at 11:02 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 28.07.2022 09:36:21, Dario Binacchi wrote:
> > > Most of the other CAN drivers write the BTR values into the register of
> > > the hardware. How are these BTR values transported into the driver?
> > >
> > > There are 2 ways:
> > >
> > > 1) - user space configures a bitrate
> > >    - the kernel calculates with the "struct can_bittiming_const" [1] given
> > >      by driver and the CAN clock rate the low level timing parameters.
> > >
> > >      [1] https://elixir.bootlin.com/linux/v5.18/source/include/uapi/linux/can/netlink.h#L47
> > >
> > > 2) - user space configures low level bit timing parameter
> > >      (Sample point in one-tenth of a percent, Time quanta (TQ) in
> > >       nanoseconds, Propagation segment in TQs, Phase buffer segment 1 in
> > >       TQs, Phase buffer segment 2 in TQs, Synchronisation jump width in
> > >       TQs)
> > >     - the kernel calculates the Bit-rate prescaler from the given TQ and
> > >       CAN clock rate
> > >
> > > Both ways result in a fully calculated "struct can_bittiming" [2]. The
> > > driver translates this into the hardware specific BTR values and writes
> > > the into the registers.
> > >
> > > If you know the CAN clock and the bit timing const parameters of the
> > > slcan's BTR register you can make use of the automatic BTR calculation,
> > > too. Maybe the framework needs some tweaking if the driver supports both
> > > fixed CAN bit rate _and_ "struct can_bittiming_const".
> >
> > Does it make sense to use the device tree
>
> The driver doesn't support DT and DT only works for static serial
> interfaces.
>
> > to provide the driver with those
> > parameters required for the automatic calculation of the BTR (clock rate,
> > struct can_bittiming_const, ...) that depend on the connected
> > controller?
>
> The device tree usually says it's a CAN controller compatible to X and
> the following clock(s) are connected. The driver for CAN controller X
> knows the bit timing const. Some USB CAN drivers query the bit timing
> const from the USB device.
>
> > In this way the solution should be generic and therefore scalable. I
> > think we should also add some properties to map the calculated BTR
> > value on the physical register of the controller.
>
> The driver knows how to map the "struct can_bittiming" to the BTR
> register values of the hardware.
>
> What does the serial protocol say to the BTR values? Are these standard
> SJA1000 layout with 8 MHz CAN clock or are those adapter specific?

I think they are adapter specific.
This is  what the can232_ver3_Manual.pdf reports:

sxxyy[CR]         Setup with BTR0/BTR1 CAN bit-rates where xx and yy is a hex
                         value. This command is only active if the CAN
channel is closed.

xx     BTR0 value in hex
yy     BTR1 value in hex

Example:            s031C[CR]
                           Setup CAN with BTR0=0x03 & BTR1=0x1C
                           which equals to 125Kbit.

But I think the example is misleading because IMHO it depends on the
adapter's controller (0x31C -> 125Kbit).

>
> > Or, use the device tree to extend the bittates supported by the controller
> > to the fixed ones (struct can_priv::bitrate_const)?
>
> The serial protocol defines fixed bit rates, no need to describe them in
> the DT:
>
> |           0            10 Kbit/s
> |           1            20 Kbit/s
> |           2            50 Kbit/s
> |           3           100 Kbit/s
> |           4           125 Kbit/s
> |           5           250 Kbit/s
> |           6           500 Kbit/s
> |           7           800 Kbit/s
> |           8          1000 Kbit/s
>
> Are there more bit rates?

No, the manual can232_ver3_Manual.pdf does not contain any others.

What about defining a device tree node for the slcan (foo adapter):

slcan {
    compatible = "can,slcan";
                                     /* bit rate btr0btr1 */
    additional-bitrates = < 33333  0x0123
                                        80000  0x4567
                                        83333  0x89ab
                                      150000 0xcd10
                                      175000 0x2345
                                      200000 0x6789>
};

So that the can_priv::bitrate_cons array (dynamically created) will
contain the bitrates
           10000,
           20000,
           50000,
         100000,
         125000,
         250000,
         500000,
         800000,
        1000000 /* end of standards bitrates,  use S command */
           33333,  /* use s command, btr 0x0123 */
           80000,  /* use s command, btr 0x4567 */
           83333,  /* use s command, btr 0x89ab */
         150000,  /* use s command, btr 0xcd10 */
         175000, /* use s command, btr 0x2345 */
         200000  /* use s command, btr 0x6789 */
};

So if a standard bitrate is requested, the S command is used,
otherwise the s command with the associated btr.

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 v3 8/9] can: slcan: add support to set bit time register (btr)

[Marc Kleine-Budde]

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

On 28.07.2022 12:23:04, Dario Binacchi wrote:
> > > Does it make sense to use the device tree
> >
> > The driver doesn't support DT and DT only works for static serial
> > interfaces.

Have you seen my remarks about Device Tree?

> > > to provide the driver with those
> > > parameters required for the automatic calculation of the BTR (clock rate,
> > > struct can_bittiming_const, ...) that depend on the connected
> > > controller?
> >
> > The device tree usually says it's a CAN controller compatible to X and
> > the following clock(s) are connected. The driver for CAN controller X
> > knows the bit timing const. Some USB CAN drivers query the bit timing
> > const from the USB device.
> >
> > > In this way the solution should be generic and therefore scalable. I
> > > think we should also add some properties to map the calculated BTR
> > > value on the physical register of the controller.
> >
> > The driver knows how to map the "struct can_bittiming" to the BTR
> > register values of the hardware.
> >
> > What does the serial protocol say to the BTR values? Are these standard
> > SJA1000 layout with 8 MHz CAN clock or are those adapter specific?
> 
> I think they are adapter specific.

The BTR values depend on the used CAN controller, the clock rate, and on
the firmware, if it supports BTR at all.

> This is  what the can232_ver3_Manual.pdf reports:
> 
> sxxyy[CR]         Setup with BTR0/BTR1 CAN bit-rates where xx and yy is a hex
>                          value. This command is only active if the CAN
> channel is closed.
> 
> xx     BTR0 value in hex
> yy     BTR1 value in hex
> 
> Example:            s031C[CR]
>                            Setup CAN with BTR0=0x03 & BTR1=0x1C
>                            which equals to 125Kbit.
> 
> But I think the example is misleading because IMHO it depends on the
> adapter's controller (0x31C -> 125Kbit).

I think the BTR in can232_ver3_Manual.pdf is compatible to a sja1000
controller with 8 MHz ref clock. See:

| Bit timing parameters for sja1000 with 8.000000 MHz ref clock using algo 'v4.8'
|  nominal                                  real  Bitrt    nom   real  SampP
|  Bitrate TQ[ns] PrS PhS1 PhS2 SJW BRP  Bitrate  Error  SampP  SampP  Error   BTR0 BTR1
|  1000000    125   2    3    2   1   1  1000000   0.0%  75.0%  75.0%   0.0%   0x00 0x14
|   800000    125   3    4    2   1   1   800000   0.0%  80.0%  80.0%   0.0%   0x00 0x16
|   500000    125   6    7    2   1   1   500000   0.0%  87.5%  87.5%   0.0%   0x00 0x1c
|   250000    250   6    7    2   1   2   250000   0.0%  87.5%  87.5%   0.0%   0x01 0x1c
|   125000    500   6    7    2   1   4   125000   0.0%  87.5%  87.5%   0.0%   0x03 0x1c        <---
|   100000    625   6    7    2   1   5   100000   0.0%  87.5%  87.5%   0.0%   0x04 0x1c
|    50000   1250   6    7    2   1  10    50000   0.0%  87.5%  87.5%   0.0%   0x09 0x1c
|    20000   3125   6    7    2   1  25    20000   0.0%  87.5%  87.5%   0.0%   0x18 0x1c
|    10000   6250   6    7    2   1  50    10000   0.0%  87.5%  87.5%   0.0%   0x31 0x1c

> > > Or, use the device tree to extend the bittates supported by the controller
> > > to the fixed ones (struct can_priv::bitrate_const)?
> >
> > The serial protocol defines fixed bit rates, no need to describe them in
> > the DT:
> >
> > |           0            10 Kbit/s
> > |           1            20 Kbit/s
> > |           2            50 Kbit/s
> > |           3           100 Kbit/s
> > |           4           125 Kbit/s
> > |           5           250 Kbit/s
> > |           6           500 Kbit/s
> > |           7           800 Kbit/s
> > |           8          1000 Kbit/s
> >
> > Are there more bit rates?
> 
> No, the manual can232_ver3_Manual.pdf does not contain any others.
> 
> What about defining a device tree node for the slcan (foo adapter):
> 
> slcan {
>     compatible = "can,slcan";
>                                      /* bit rate btr0btr1 */
>     additional-bitrates = < 33333  0x0123
>                                         80000  0x4567
>                                         83333  0x89ab
>                                       150000 0xcd10
>                                       175000 0x2345
>                                       200000 0x6789>
> };
> 
> So that the can_priv::bitrate_cons array (dynamically created) will
> contain the bitrates
>            10000,
>            20000,
>            50000,
>          100000,
>          125000,
>          250000,
>          500000,
>          800000,
>         1000000 /* end of standards bitrates,  use S command */
>            33333,  /* use s command, btr 0x0123 */
>            80000,  /* use s command, btr 0x4567 */
>            83333,  /* use s command, btr 0x89ab */
>          150000,  /* use s command, btr 0xcd10 */
>          175000, /* use s command, btr 0x2345 */
>          200000  /* use s command, btr 0x6789 */
> };
> 
> So if a standard bitrate is requested, the S command is used,
> otherwise the s command with the associated btr.

That would be an option. For proper DT support, the driver needs to be
extended to support serdev. But DT only supports "static" devices.

But if you can calculate BTR values you know the bit timing constants
(and frequency) and then it's better to have the bit timing in the
driver so that arbitrary bit rates can be calculated by the kernel.

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 v3 8/9] can: slcan: add support to set bit time register (btr)

[Max Staudt]

On Thu, 28 Jul 2022 12:50:49 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 28.07.2022 12:23:04, Dario Binacchi wrote:
> > > > Does it make sense to use the device tree  
> > >
> > > The driver doesn't support DT and DT only works for static serial
> > > interfaces.  
> 
> Have you seen my remarks about Device Tree?

Dario, there seems to be a misunderstanding about the Device Tree.

It is used *only* for hardware that is permanently attached, present at
boot, and forever after. Not for dyamically added stuff, and definitely
not for ldiscs that have to be attached manually by the user.


The only exception to this is if you have an embedded device with an
slcan adapter permanently attached to one of its UARTs. Then you can
use the serdev ldisc adapter to attach the ldisc automatically at boot.

If you are actively developing for such a use case, please let us know,
so we know what you're after and can help you better :)


Max

Re: [RFC PATCH v3 8/9] can: slcan: add support to set bit time register (btr)

[Dario Binacchi]

Hello Marc and Max,

On Thu, Jul 28, 2022 at 12:57 PM Max Staudt <max@enpas.org> wrote:
>
> On Thu, 28 Jul 2022 12:50:49 +0200
> Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> > On 28.07.2022 12:23:04, Dario Binacchi wrote:
> > > > > Does it make sense to use the device tree
> > > >
> > > > The driver doesn't support DT and DT only works for static serial
> > > > interfaces.
> >
> > Have you seen my remarks about Device Tree?
>
> Dario, there seems to be a misunderstanding about the Device Tree.
>
> It is used *only* for hardware that is permanently attached, present at
> boot, and forever after. Not for dyamically added stuff, and definitely
> not for ldiscs that have to be attached manually by the user.
>
>
> The only exception to this is if you have an embedded device with an
> slcan adapter permanently attached to one of its UARTs. Then you can
> use the serdev ldisc adapter to attach the ldisc automatically at boot.

It is evident that I am lacking some skills (I will try to fix it :)).
I think it is
equally clear that it is not worth going down this path.

>
> If you are actively developing for such a use case, please let us know,
> so we know what you're after and can help you better :)
>

I don't have a use case, other than to try, if possible, to make the driver
autonomous from slcand / slcan_attach for the CAN bus setup.

Returning to Marc's previous analysis:
"... Some USB CAN drivers query the bit timing const from the USB device."

Can we think of taking the gs_usb driver as inspiration for getting/setting the
bit timings?

https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/gs_usb.c#L951
https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/gs_usb.c#L510

and, as done with patches:

can: slcan: extend the protocol with CAN state info
can: slcan: extend the protocol with error info

further extend the protocol to get/set the bit timing from / to the adapter ?
In the case of non-standard bit rates, the driver would try, depending on the
firmware of the adapter, to calculate and set the bit timings autonomously.

Thanks and regards,
Dario

>
> Max



-- 

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 v3 8/9] can: slcan: add support to set bit time register (btr)

[Marc Kleine-Budde]

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

On 29.07.2022 08:52:07, Dario Binacchi wrote:
> Hello Marc and Max,
> 
> On Thu, Jul 28, 2022 at 12:57 PM Max Staudt <max@enpas.org> wrote:
> >
> > On Thu, 28 Jul 2022 12:50:49 +0200
> > Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >
> > > On 28.07.2022 12:23:04, Dario Binacchi wrote:
> > > > > > Does it make sense to use the device tree
> > > > >
> > > > > The driver doesn't support DT and DT only works for static serial
> > > > > interfaces.
> > >
> > > Have you seen my remarks about Device Tree?
> >
> > Dario, there seems to be a misunderstanding about the Device Tree.
> >
> > It is used *only* for hardware that is permanently attached, present at
> > boot, and forever after. Not for dyamically added stuff, and definitely
> > not for ldiscs that have to be attached manually by the user.
> >
> >
> > The only exception to this is if you have an embedded device with an
> > slcan adapter permanently attached to one of its UARTs. Then you can
> > use the serdev ldisc adapter to attach the ldisc automatically at boot.
> 
> It is evident that I am lacking some skills (I will try to fix it :)).

We're all here to learn something!

> I think it is equally clear that it is not worth going down this path.

If you have a static attached serial devices serdev is the way to go.
But slcan has so many drawbacks compared to "real" CAN adapters that I
hope the no one uses them in such a scenario.

> > If you are actively developing for such a use case, please let us know,
> > so we know what you're after and can help you better :)
> 
> I don't have a use case, other than to try, if possible, to make the driver
> autonomous from slcand / slcan_attach for the CAN bus setup.

From my point of view your job is done!

> Returning to Marc's previous analysis:
> "... Some USB CAN drivers query the bit timing const from the USB device."
> 
> Can we think of taking the gs_usb driver as inspiration for getting/setting the
> bit timings?
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/gs_usb.c#L951
> https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/gs_usb.c#L510
> 
> and, as done with patches:
> 
> can: slcan: extend the protocol with CAN state info
> can: slcan: extend the protocol with error info

You can define a way to query bit timing constants and CAN clock rate,
but you have to get this into the "official" firmware. You have to roll
out a firmware update to all devices. What about non official firmware?

> further extend the protocol to get/set the bit timing from / to the adapter ?
> In the case of non-standard bit rates, the driver would try, depending on the
> firmware of the adapter, to calculate and set the bit timings autonomously.

If an adapter follows 100% the official firmware doc the BTR registers
are interpreted as SJA1000 with 8 MHz CAN clock.

See

| https://lore.kernel.org/all/20220728105049.43gbjuctezxzmm4j@pengutronix.de

where I compare the 125 Kbit/s BTR config of the documentation with the
bit timing calculated by the kernel algorithm.

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 v3 8/9] can: slcan: add support to set bit time register (btr)

[Dario Binacchi]

Hi Marc,

On Fri, Jul 29, 2022 at 9:33 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 29.07.2022 08:52:07, Dario Binacchi wrote:
> > Hello Marc and Max,
> >
> > On Thu, Jul 28, 2022 at 12:57 PM Max Staudt <max@enpas.org> wrote:
> > >
> > > On Thu, 28 Jul 2022 12:50:49 +0200
> > > Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > >
> > > > On 28.07.2022 12:23:04, Dario Binacchi wrote:
> > > > > > > Does it make sense to use the device tree
> > > > > >
> > > > > > The driver doesn't support DT and DT only works for static serial
> > > > > > interfaces.
> > > >
> > > > Have you seen my remarks about Device Tree?
> > >
> > > Dario, there seems to be a misunderstanding about the Device Tree.
> > >
> > > It is used *only* for hardware that is permanently attached, present at
> > > boot, and forever after. Not for dyamically added stuff, and definitely
> > > not for ldiscs that have to be attached manually by the user.
> > >
> > >
> > > The only exception to this is if you have an embedded device with an
> > > slcan adapter permanently attached to one of its UARTs. Then you can
> > > use the serdev ldisc adapter to attach the ldisc automatically at boot.
> >
> > It is evident that I am lacking some skills (I will try to fix it :)).
>
> We're all here to learn something!
>
> > I think it is equally clear that it is not worth going down this path.
>
> If you have a static attached serial devices serdev is the way to go.
> But slcan has so many drawbacks compared to "real" CAN adapters that I
> hope the no one uses them in such a scenario.
>
> > > If you are actively developing for such a use case, please let us know,
> > > so we know what you're after and can help you better :)
> >
> > I don't have a use case, other than to try, if possible, to make the driver
> > autonomous from slcand / slcan_attach for the CAN bus setup.
>
> From my point of view your job is done!

Ok.

>
> > Returning to Marc's previous analysis:
> > "... Some USB CAN drivers query the bit timing const from the USB device."
> >
> > Can we think of taking the gs_usb driver as inspiration for getting/setting the
> > bit timings?
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/gs_usb.c#L951
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/gs_usb.c#L510
> >
> > and, as done with patches:
> >
> > can: slcan: extend the protocol with CAN state info
> > can: slcan: extend the protocol with error info
>
> You can define a way to query bit timing constants and CAN clock rate,
> but you have to get this into the "official" firmware. You have to roll
> out a firmware update to all devices. What about non official firmware?
>
> > further extend the protocol to get/set the bit timing from / to the adapter ?
> > In the case of non-standard bit rates, the driver would try, depending on the
> > firmware of the adapter, to calculate and set the bit timings autonomously.
>
> If an adapter follows 100% the official firmware doc the BTR registers
> are interpreted as SJA1000 with 8 MHz CAN clock.

I checked the sources and documentation of the usb adapter I used (i.
e. Https://www.fischl.de/usbtin/):
...
sxxyyzz[CR]                 Set can bitrate registers of MCP2515. You
can set non-standard baudrates which are not supported by the "Sx"
command.
                                     xx: CNF1 as hexadecimal value (00-FF)
                                     yy: CNF2 as hexadecimal value (00-FF)
                                     zz: CNF3 as hexadecimal value
...

Different from what is reported by can232_ver3_Manual.pdf :

sxxyy[CR]         Setup with BTR0/BTR1 CAN bit-rates where xx and yy is a hex
                         value. This command is only active if the CAN

And here is the type of control carried out by the slcan_attach for
the btr parameter:
https://github.com/linux-can/can-utils/blob/master/slcan_attach.c#L144
When I would have expected a different check (i. e. if (strlen(btr) > 4).
Therefore it is possible that each adapter uses these bytes in its own
way as well as
in the content and also in the number of bytes.

Thanks and regards,
Dario

>
> See
>
> | https://lore.kernel.org/all/20220728105049.43gbjuctezxzmm4j@pengutronix.de
>
> where I compare the 125 Kbit/s BTR config of the documentation with the
> bit timing calculated by the kernel algorithm.
>
> 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 v3 8/9] can: slcan: add support to set bit time register (btr)

[Marc Kleine-Budde]

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

On 31.07.2022 17:54:01, Dario Binacchi wrote:
> > If an adapter follows 100% the official firmware doc the BTR registers
> > are interpreted as SJA1000 with 8 MHz CAN clock.
> 
> I checked the sources and documentation of the usb adapter I used (i.
> e. Https://www.fischl.de/usbtin/):
> ...
> sxxyyzz[CR]                 Set can bitrate registers of MCP2515. You
> can set non-standard baudrates which are not supported by the "Sx"
> command.

I hope the effective clock speed is documented somewhere, as you need
this information to set the registers.

>                                      xx: CNF1 as hexadecimal value (00-FF)
>                                      yy: CNF2 as hexadecimal value (00-FF)
>                                      zz: CNF3 as hexadecimal value
> ...
> 
> Different from what is reported by can232_ver3_Manual.pdf :
> 
> sxxyy[CR]         Setup with BTR0/BTR1 CAN bit-rates where xx and yy is a hex
>                          value. This command is only active if the CAN
> 
> And here is the type of control carried out by the slcan_attach for
> the btr parameter:
> https://github.com/linux-can/can-utils/blob/master/slcan_attach.c#L144
> When I would have expected a different check (i. e. if (strlen(btr) > 4).
> Therefore it is possible that each adapter uses these bytes in its own
> way as well as
> in the content and also in the number of bytes.

I expected something like that.

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

[RFC PATCH v3 9/9] MAINTAINERS: Add maintainer for the slcan driver

[Dario Binacchi]

At the suggestion of its author Oliver Hartkopp ([1]), I take over the
maintainer-ship and add myself to the authors of the driver.

[1] https://lore.kernel.org/all/507b5973-d673-4755-3b64-b41cb9a13b6f@hartkopp.net

Suggested-by: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

---

Changes in v3:
- Put the series as RFC again.
- Pick up the patch "can: slcan: use KBUILD_MODNAME and define pr_fmt to replace hardcoded names".
- Add the patch "ethtool: add support to get/set CAN bit time register"
  to the series.
- Add the patch "can: slcan: add support to set bit time register (btr)"
  to the series.
- Replace the link https://marc.info/?l=linux-can&m=165806705927851&w=2 with
  https://lore.kernel.org/all/507b5973-d673-4755-3b64-b41cb9a13b6f@hartkopp.net.
- Add the `Suggested-by' tag.

Changes in v2:
- Add the patch "MAINTAINERS: Add myself as maintainer of the SLCAN driver"
  to the series.

 MAINTAINERS                        | 6 ++++++
 drivers/net/can/slcan/slcan-core.c | 1 +
 2 files changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index fc7d75c5cdb9..74e42f78e7cb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18448,6 +18448,12 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git
 F:	include/linux/sl?b*.h
 F:	mm/sl?b*
 
+SLCAN CAN NETWORK DRIVER
+M:	Dario Binacchi <dario.binacchi@amarulasolutions.com>
+L:	linux-can@vger.kernel.org
+S:	Maintained
+F:	drivers/net/can/slcan/
+
 SLEEPABLE READ-COPY UPDATE (SRCU)
 M:	Lai Jiangshan <jiangshanlai@gmail.com>
 M:	"Paul E. McKenney" <paulmck@kernel.org>
diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
index 3905f21e7788..6a0f12cfbb4e 100644
--- a/drivers/net/can/slcan/slcan-core.c
+++ b/drivers/net/can/slcan/slcan-core.c
@@ -63,6 +63,7 @@ MODULE_ALIAS_LDISC(N_SLCAN);
 MODULE_DESCRIPTION("serial line CAN interface");
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Oliver Hartkopp <socketcan@hartkopp.net>");
+MODULE_AUTHOR("Dario Binacchi <dario.binacchi@amarulasolutions.com>");
 
 /* maximum rx buffer len: extended CAN frame with timestamp */
 #define SLCAN_MTU (sizeof("T1111222281122334455667788EA5F\r") + 1)
-- 
2.32.0

Re: [RFC PATCH v3 0/9] can: slcan: extend supported features (step 2)

[Marc Kleine-Budde]

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

On 26.07.2022 23:02:08, Dario Binacchi wrote:
> With this series I try to finish the task, started with the series [1],
> of completely removing the dependency of the slcan driver from the
> userspace slcand/slcan_attach applications.
> 
> The series also contains patches that remove the legacy stuff (slcan_devs,
> SLCAN_MAGIC, ...) and do some module cleanup.
> 
> The series has been created on top of the patches:
> 
> can: slcan: convert comments to network style comments
> can: slcan: slcan_init() convert printk(LEVEL ...) to pr_level()
> can: slcan: fix whitespace issues
> can: slcan: convert comparison to NULL into !val
> can: slcan: clean up if/else
> can: slcan: use scnprintf() as a hardening measure
> can: slcan: do not report txerr and rxerr during bus-off
> can: slcan: do not sleep with a spin lock held
> 
> applied to linux-next.
> 
> [1] https://lore.kernel.org/all/20220628163137.413025-1-dario.binacchi@amarulasolutions.com/
> 
> Changes in v3:
> - Update the commit message.
> - Use 1 space in front of the =.
> - Put the series as RFC again.

No need to change the series to RFC again :)

> - Pick up the patch "can: slcan: use KBUILD_MODNAME and define pr_fmt to replace hardcoded names".
> - Add the patch "ethtool: add support to get/set CAN bit time register"
>   to the series.
> - Add the patch "can: slcan: add support to set bit time register (btr)"
>   to the series.
> - Replace the link https://marc.info/?l=linux-can&m=165806705927851&w=2 with
>   https://lore.kernel.org/all/507b5973-d673-4755-3b64-b41cb9a13b6f@hartkopp.net.
> - Add the `Suggested-by' tag.

Please post a v4 with both BTR patches dropped and add Max Staudt's
Reviewed-by to patch 3.

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