Bitrate and CAN status support for slcan

Bitrate and CAN status support for slcan

[Alexander Stein]

This series adds bitrate selection support. It uses the API described
in the SLCAN-API which is identical on all listed serial interfaces.
Thus slcan only supports that limited set of bitrates, as only a bitrate
index is sent over serial wire.
Patch 5 also adds support for CAN status polling. This has do be done
regulrarly from CPU side.
Tests have been done on a LAWICEL CANUSB device.

Best regards,
Alexander

[PATCH 1/5] can: slcan: Fix spinlock variant

[Alexander Stein]

slc_xmit is called within softirq context and locks sl->lock, but
slcan_write_wakeup is not softirq context, so we need to use
spin_[un]lock_bh!
Detected using kernel lock debugging mechanism.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
I found this by just enabling the kernel config options named in
Documentation/SubmitChecklist :)
> 12: Has been tested with CONFIG_PREEMPT, CONFIG_DEBUG_PREEMPT,
>    CONFIG_DEBUG_SLAB, CONFIG_DEBUG_PAGEALLOC, CONFIG_DEBUG_MUTEXES,
>    CONFIG_DEBUG_SPINLOCK, CONFIG_DEBUG_ATOMIC_SLEEP, CONFIG_PROVE_RCU
>    and CONFIG_DEBUG_OBJECTS_RCU_HEAD all simultaneously enabled.

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

diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index 3fcdae2..2577c1e 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -322,13 +322,13 @@ static void slcan_write_wakeup(struct tty_struct *tty)
 	if (!sl || sl->magic != SLCAN_MAGIC || !netif_running(sl->dev))
 		return;
 
-	spin_lock(&sl->lock);
+	spin_lock_bh(&sl->lock);
 	if (sl->xleft <= 0)  {
 		/* Now serial buffer is almost free & we can start
 		 * transmission of another packet */
 		sl->dev->stats.tx_packets++;
 		clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
-		spin_unlock(&sl->lock);
+		spin_unlock_bh(&sl->lock);
 		netif_wake_queue(sl->dev);
 		return;
 	}
@@ -336,7 +336,7 @@ static void slcan_write_wakeup(struct tty_struct *tty)
 	actual = tty->ops->write(tty, sl->xhead, sl->xleft);
 	sl->xleft -= actual;
 	sl->xhead += actual;
-	spin_unlock(&sl->lock);
+	spin_unlock_bh(&sl->lock);
 }
 
 /* Send a can_frame to a TTY queue. */
-- 
1.8.3.2

[PATCH 2/5] can: slcan: Convert to can_dev interface

[Alexander Stein]

No actual feature change despite the interface name. This is just a
preparation for additional CAN related features.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 drivers/net/can/Kconfig |  40 +++++++++----------
 drivers/net/can/slcan.c | 104 +++++++++++++++++++++---------------------------
 2 files changed, 65 insertions(+), 79 deletions(-)

diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index 9e7d95d..daea043 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -9,26 +9,6 @@ config CAN_VCAN
 	  This driver can also be built as a module.  If so, the module
 	  will be called vcan.
 
-config CAN_SLCAN
-	tristate "Serial / USB serial CAN Adaptors (slcan)"
-	depends on TTY
-	---help---
-	  CAN driver for several 'low cost' CAN interfaces that are attached
-	  via serial lines or via USB-to-serial adapters using the LAWICEL
-	  ASCII protocol. The driver implements the tty linediscipline N_SLCAN.
-
-	  As only the sending and receiving of CAN frames is implemented, this
-	  driver should work with the (serial/USB) CAN hardware from:
-	  www.canusb.com / www.can232.com / www.mictronics.de / www.canhack.de
-
-	  Userspace tools to attach the SLCAN line discipline (slcan_attach,
-	  slcand) can be found in the can-utils at the SocketCAN SVN, see
-	  http://developer.berlios.de/projects/socketcan for details.
-
-	  The slcan driver supports up to 10 CAN netdevices by default which
-	  can be changed by the 'maxdev=xx' module option. This driver can
-	  also be built as a module. If so, the module will be called slcan.
-
 config CAN_DEV
 	tristate "Platform CAN drivers with Netlink support"
 	default y
@@ -125,6 +105,26 @@ config CAN_GRCAN
 	  endian syntheses of the cores would need some modifications on
 	  the hardware level to work.
 
+config CAN_SLCAN
+	tristate "Serial / USB serial CAN Adaptors (slcan)"
+	depends on TTY
+	---help---
+	  CAN driver for several 'low cost' CAN interfaces that are attached
+	  via serial lines or via USB-to-serial adapters using the LAWICEL
+	  ASCII protocol. The driver implements the tty linediscipline N_SLCAN.
+
+	  As only the sending and receiving of CAN frames is implemented, this
+	  driver should work with the (serial/USB) CAN hardware from:
+	  www.canusb.com / www.can232.com / www.mictronics.de / www.canhack.de
+
+	  Userspace tools to attach the SLCAN line discipline (slcan_attach,
+	  slcand) can be found in the can-utils at the SocketCAN SVN, see
+	  http://developer.berlios.de/projects/socketcan for details.
+
+	  The slcan driver supports up to 10 CAN netdevices by default which
+	  can be changed by the 'maxdev=xx' module option. This driver can
+	  also be built as a module. If so, the module will be called slcan.
+
 source "drivers/net/can/mscan/Kconfig"
 
 source "drivers/net/can/sja1000/Kconfig"
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index 2577c1e..337e6e3 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -53,6 +53,7 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/can.h>
+#include <linux/can/dev.h>
 #include <linux/can/skb.h>
 
 static __initconst const char banner[] =
@@ -79,6 +80,7 @@ MODULE_PARM_DESC(maxdev, "Maximum number of slcan interfaces");
 #define SLC_EFF_ID_LEN 8
 
 struct slcan {
+	struct can_priv		can;	/* must be the first member */
 	int			magic;
 
 	/* Various fields. */
@@ -99,6 +101,7 @@ struct slcan {
 };
 
 static struct net_device **slcan_devs;
+static DEFINE_MUTEX(slcan_mutex);
 
  /************************************************************************
   *			SLCAN ENCAPSULATION FORMAT			 *
@@ -143,31 +146,33 @@ static struct net_device **slcan_devs;
 static void slc_bump(struct slcan *sl)
 {
 	struct sk_buff *skb;
-	struct can_frame cf;
+	struct can_frame *cf;
+	canid_t can_id;
+	u8 can_dlc;
 	int i, tmp;
 	u32 tmpid;
 	char *cmd = sl->rbuff;
 
-	cf.can_id = 0;
+	can_id = 0;
 
 	switch (*cmd) {
 	case 'r':
-		cf.can_id = CAN_RTR_FLAG;
+		can_id = CAN_RTR_FLAG;
 		/* fallthrough */
 	case 't':
 		/* store dlc ASCII value and terminate SFF CAN ID string */
-		cf.can_dlc = sl->rbuff[SLC_CMD_LEN + SLC_SFF_ID_LEN];
+		can_dlc = sl->rbuff[SLC_CMD_LEN + SLC_SFF_ID_LEN];
 		sl->rbuff[SLC_CMD_LEN + SLC_SFF_ID_LEN] = 0;
 		/* point to payload data behind the dlc */
 		cmd += SLC_CMD_LEN + SLC_SFF_ID_LEN + 1;
 		break;
 	case 'R':
-		cf.can_id = CAN_RTR_FLAG;
+		can_id = CAN_RTR_FLAG;
 		/* fallthrough */
 	case 'T':
-		cf.can_id |= CAN_EFF_FLAG;
+		can_id |= CAN_EFF_FLAG;
 		/* store dlc ASCII value and terminate EFF CAN ID string */
-		cf.can_dlc = sl->rbuff[SLC_CMD_LEN + SLC_EFF_ID_LEN];
+		can_dlc = sl->rbuff[SLC_CMD_LEN + SLC_EFF_ID_LEN];
 		sl->rbuff[SLC_CMD_LEN + SLC_EFF_ID_LEN] = 0;
 		/* point to payload data behind the dlc */
 		cmd += SLC_CMD_LEN + SLC_EFF_ID_LEN + 1;
@@ -179,49 +184,39 @@ static void slc_bump(struct slcan *sl)
 	if (kstrtou32(sl->rbuff + SLC_CMD_LEN, 16, &tmpid))
 		return;
 
-	cf.can_id |= tmpid;
+	can_id |= tmpid;
 
 	/* get can_dlc from sanitized ASCII value */
-	if (cf.can_dlc >= '0' && cf.can_dlc < '9')
-		cf.can_dlc -= '0';
+	if (can_dlc >= '0' && can_dlc < '9')
+		can_dlc -= '0';
 	else
 		return;
 
-	*(u64 *) (&cf.data) = 0; /* clear payload */
+	skb = alloc_can_skb(sl->dev, &cf);
+	if (!skb)
+		return;
+	cf->can_id = can_id;
+	cf->can_dlc = can_dlc;
 
 	/* RTR frames may have a dlc > 0 but they never have any data bytes */
-	if (!(cf.can_id & CAN_RTR_FLAG)) {
-		for (i = 0; i < cf.can_dlc; i++) {
+	if (!(cf->can_id & CAN_RTR_FLAG)) {
+		for (i = 0; i < cf->can_dlc; i++) {
 			tmp = hex_to_bin(*cmd++);
 			if (tmp < 0)
 				return;
-			cf.data[i] = (tmp << 4);
+			cf->data[i] = (tmp << 4);
 			tmp = hex_to_bin(*cmd++);
 			if (tmp < 0)
 				return;
-			cf.data[i] |= tmp;
+			cf->data[i] |= tmp;
 		}
 	}
-
-	skb = dev_alloc_skb(sizeof(struct can_frame) +
-			    sizeof(struct can_skb_priv));
-	if (!skb)
-		return;
-
 	skb->dev = sl->dev;
-	skb->protocol = htons(ETH_P_CAN);
-	skb->pkt_type = PACKET_BROADCAST;
-	skb->ip_summed = CHECKSUM_UNNECESSARY;
-
-	can_skb_reserve(skb);
-	can_skb_prv(skb)->ifindex = sl->dev->ifindex;
 
-	memcpy(skb_put(skb, sizeof(struct can_frame)),
-	       &cf, sizeof(struct can_frame));
 	netif_rx_ni(skb);
 
 	sl->dev->stats.rx_packets++;
-	sl->dev->stats.rx_bytes += cf.can_dlc;
+	sl->dev->stats.rx_bytes += cf->can_dlc;
 }
 
 /* parse tty input stream */
@@ -385,6 +380,10 @@ static int slc_close(struct net_device *dev)
 	netif_stop_queue(dev);
 	sl->rcount   = 0;
 	sl->xleft    = 0;
+	sl->can.state = CAN_STATE_STOPPED;
+
+	close_candev(dev);
+
 	spin_unlock_bh(&sl->lock);
 
 	return 0;
@@ -394,12 +393,19 @@ static int slc_close(struct net_device *dev)
 static int slc_open(struct net_device *dev)
 {
 	struct slcan *sl = netdev_priv(dev);
+	int err;
 
 	if (sl->tty == NULL)
 		return -ENODEV;
 
+	err = open_candev(dev);
+	if (err)
+		return err;
+
+	sl->can.state = CAN_STATE_ERROR_ACTIVE;
 	sl->flags &= (1 << SLF_INUSE);
 	netif_start_queue(dev);
+
 	return 0;
 }
 
@@ -407,7 +413,7 @@ static int slc_open(struct net_device *dev)
 static void slc_free_netdev(struct net_device *dev)
 {
 	int i = dev->base_addr;
-	free_netdev(dev);
+	free_candev(dev);
 	slcan_devs[i] = NULL;
 }
 
@@ -417,23 +423,6 @@ static const struct net_device_ops slc_netdev_ops = {
 	.ndo_start_xmit         = slc_xmit,
 };
 
-static void slc_setup(struct net_device *dev)
-{
-	dev->netdev_ops		= &slc_netdev_ops;
-	dev->destructor		= slc_free_netdev;
-
-	dev->hard_header_len	= 0;
-	dev->addr_len		= 0;
-	dev->tx_queue_len	= 10;
-
-	dev->mtu		= sizeof(struct can_frame);
-	dev->type		= ARPHRD_CAN;
-
-	/* New-style flags. */
-	dev->flags		= IFF_NOARP;
-	dev->features           = NETIF_F_HW_CSUM;
-}
-
 /******************************************
   Routines looking at TTY side.
  ******************************************/
@@ -495,7 +484,6 @@ static void slc_sync(void)
 static struct slcan *slc_alloc(dev_t line)
 {
 	int i;
-	char name[IFNAMSIZ];
 	struct net_device *dev = NULL;
 	struct slcan       *sl;
 
@@ -510,12 +498,13 @@ static struct slcan *slc_alloc(dev_t line)
 	if (i >= maxdev)
 		return NULL;
 
-	sprintf(name, "slcan%d", i);
-	dev = alloc_netdev(sizeof(*sl), name, slc_setup);
+	dev = alloc_candev(sizeof(*sl), 1);
 	if (!dev)
 		return NULL;
 
 	dev->base_addr  = i;
+	dev->netdev_ops = &slc_netdev_ops;
+	dev->destructor = slc_free_netdev;
 	sl = netdev_priv(dev);
 
 	/* Initialize channel control data */
@@ -548,11 +537,8 @@ static int slcan_open(struct tty_struct *tty)
 	if (tty->ops->write == NULL)
 		return -EOPNOTSUPP;
 
-	/* RTnetlink lock is misused here to serialize concurrent
-	   opens of slcan channels. There are better ways, but it is
-	   the simplest one.
-	 */
-	rtnl_lock();
+	/* Serialize concurrent opens of slcan channels. */
+	mutex_lock(&slcan_mutex);
 
 	/* Collect hanged up channels. */
 	slc_sync();
@@ -580,13 +566,13 @@ static int slcan_open(struct tty_struct *tty)
 
 		set_bit(SLF_INUSE, &sl->flags);
 
-		err = register_netdevice(sl->dev);
+		err = register_candev(sl->dev);
 		if (err)
 			goto err_free_chan;
 	}
 
 	/* Done.  We have linked the TTY line to a channel. */
-	rtnl_unlock();
+	mutex_unlock(&slcan_mutex);
 	tty->receive_room = 65536;	/* We don't flow control */
 
 	/* TTY layer expects 0 on success */
@@ -598,7 +584,7 @@ err_free_chan:
 	clear_bit(SLF_INUSE, &sl->flags);
 
 err_exit:
-	rtnl_unlock();
+	mutex_unlock(&slcan_mutex);
 
 	/* Count references from TTY module */
 	return err;
-- 
1.8.3.2

[PATCH 3/5] can: slcan: Send open/close command upon interface up/down

[Alexander Stein]

In case a command could not be sent, print a warning. E.g. when slcand
is killed before the interface is shutdown.
Always close serial CAN device upon CAN interface initialization.
Otherwise no bitrate setting can be done.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 drivers/net/can/slcan.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index 337e6e3..8a578bb 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -103,6 +103,40 @@ struct slcan {
 static struct net_device **slcan_devs;
 static DEFINE_MUTEX(slcan_mutex);
 
+#define CLOSE_COMMAND "C\r"
+#define OPEN_COMMAND "O\r"
+
+static int slc_open_device(struct slcan *sl) __must_hold(&sl->lock)
+{
+	int written;
+
+	if (sl->tty) {
+		written = sl->tty->ops->write(sl->tty, OPEN_COMMAND,
+					      strlen(OPEN_COMMAND));
+		if (written != strlen(OPEN_COMMAND)) {
+			netdev_warn(sl->dev, "Open command could not be sent. Serial interface still inactive!\n");
+			return -EIO;
+		}
+	}
+	sl->can.state = CAN_STATE_ERROR_ACTIVE;
+	return 0;
+}
+
+static int slc_close_device(struct slcan *sl) __must_hold(&sl->lock)
+{
+	int written;
+
+	if (sl->tty) {
+		written = sl->tty->ops->write(sl->tty, CLOSE_COMMAND,
+					      strlen(CLOSE_COMMAND));
+		if (written != strlen(CLOSE_COMMAND)) {
+			netdev_warn(sl->dev, "Close command could not be sent. Serial interface still active!\n");
+			return -EIO;
+		}
+	}
+	return 0;
+}
+
  /************************************************************************
   *			SLCAN ENCAPSULATION FORMAT			 *
   ************************************************************************/
@@ -373,10 +407,13 @@ static int slc_close(struct net_device *dev)
 	struct slcan *sl = netdev_priv(dev);
 
 	spin_lock_bh(&sl->lock);
+
 	if (sl->tty) {
 		/* TTY discipline is running. */
 		clear_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags);
 	}
+
+	slc_close_device(sl);
 	netif_stop_queue(dev);
 	sl->rcount   = 0;
 	sl->xleft    = 0;
@@ -402,10 +439,18 @@ static int slc_open(struct net_device *dev)
 	if (err)
 		return err;
 
-	sl->can.state = CAN_STATE_ERROR_ACTIVE;
+	spin_lock_bh(&sl->lock);
+
+	err = slc_open_device(sl);
+	if (err) {
+		spin_unlock_bh(&sl->lock);
+		return err;
+	}
+
 	sl->flags &= (1 << SLF_INUSE);
 	netif_start_queue(dev);
 
+	spin_unlock_bh(&sl->lock);
 	return 0;
 }
 
@@ -559,6 +604,10 @@ static int slcan_open(struct tty_struct *tty)
 	sl->tty = tty;
 	tty->disc_data = sl;
 
+	err = slc_close_device(sl);
+	if (err)
+		goto err_free_chan;
+
 	if (!test_bit(SLF_INUSE, &sl->flags)) {
 		/* Perform the low-level SLCAN initialization. */
 		sl->rcount   = 0;
-- 
1.8.3.2

[PATCH 4/5] can: slcan: Add bitrate change support

[Alexander Stein]

Only a fixed set of bitrates are supported.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 drivers/net/can/Kconfig |  5 +++--
 drivers/net/can/slcan.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index daea043..267aec2 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -113,8 +113,9 @@ config CAN_SLCAN
 	  via serial lines or via USB-to-serial adapters using the LAWICEL
 	  ASCII protocol. The driver implements the tty linediscipline N_SLCAN.
 
-	  As only the sending and receiving of CAN frames is implemented, this
-	  driver should work with the (serial/USB) CAN hardware from:
+	  Sending, receiving of CAN frames as well as bitrate setting is
+	  implemented, this driver should work with the
+	  (serial/USB) CAN hardware from:
 	  www.canusb.com / www.can232.com / www.mictronics.de / www.canhack.de
 
 	  Userspace tools to attach the SLCAN line discipline (slcan_attach,
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index 8a578bb..8d98f50 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -454,6 +454,58 @@ static int slc_open(struct net_device *dev)
 	return 0;
 }
 
+static int slc_set_bittiming(struct net_device *dev)
+{
+	struct slcan *sl = netdev_priv(dev);
+	const struct can_bittiming *bt = &sl->can.bittiming;
+	int index, written, len;
+	char buf[4];
+
+	switch (bt->bitrate) {
+	case 1000000:
+		index = 8;
+		break;
+	case 800000:
+		index = 7;
+		break;
+	case 500000:
+		index = 6;
+		break;
+	case 250000:
+		index = 5;
+		break;
+	case 125000:
+		index = 4;
+		break;
+	case 100000:
+		index = 3;
+		break;
+	case 50000:
+		index = 2;
+		break;
+	case 20000:
+		index = 1;
+		break;
+	case 10000:
+		index = 0;
+		break;
+	default:
+		return -EINVAL;
+	};
+
+	if (!sl->tty)
+		return -ENOENT;
+
+	len = snprintf(buf, sizeof(buf), "S%d\r", index);
+	written = sl->tty->ops->write(sl->tty, buf, len);
+	if (written != len) {
+		netdev_warn(sl->dev, "Setting bitrate command could not be sent!\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
 /* Hook the destructor so we can free slcan devs at the right point in time */
 static void slc_free_netdev(struct net_device *dev)
 {
@@ -613,6 +665,7 @@ static int slcan_open(struct tty_struct *tty)
 		sl->rcount   = 0;
 		sl->xleft    = 0;
 
+		sl->can.do_set_bittiming = slc_set_bittiming;
 		set_bit(SLF_INUSE, &sl->flags);
 
 		err = register_candev(sl->dev);
-- 
1.8.3.2

[PATCH 5/5] can: slcan: Add CAN status flag support

[Alexander Stein]

CAN status flags have to be polled from the serial device. So send the
request every 10ms by default.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 drivers/net/can/Kconfig |   4 +-
 drivers/net/can/slcan.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 188 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index 267aec2..9aa38943 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -113,8 +113,8 @@ config CAN_SLCAN
 	  via serial lines or via USB-to-serial adapters using the LAWICEL
 	  ASCII protocol. The driver implements the tty linediscipline N_SLCAN.
 
-	  Sending, receiving of CAN frames as well as bitrate setting is
-	  implemented, this driver should work with the
+	  Sending, receiving of CAN frames as well as bitrate setting and
+	  error handling is implemented, this driver should work with the
 	  (serial/USB) CAN hardware from:
 	  www.canusb.com / www.can232.com / www.mictronics.de / www.canhack.de
 
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index 8d98f50..35e5392 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -72,6 +72,15 @@ static int maxdev = 10;		/* MAX number of SLCAN channels;
 module_param(maxdev, int, 0);
 MODULE_PARM_DESC(maxdev, "Maximum number of slcan interfaces");
 
+/** \brief Status poll interval (in ms) in which the CAN status shall be polled
+ *
+ * Interval (in ms) in which the CAN status shall be polled.
+ * Can be overridden with insmod slcan.ko status_interval_ms=nnn
+ */
+static int status_interval_ms = 10;
+module_param(status_interval_ms, uint, 0);
+MODULE_PARM_DESC(status_interval_ms, "Status poll interval [ms]");
+
 /* maximum rx buffer len: extended CAN frame with timestamp */
 #define SLC_MTU (sizeof("T1111222281122334455667788EA5F\r")+1)
 
@@ -98,6 +107,20 @@ struct slcan {
 	unsigned long		flags;		/* Flag values/ mode etc     */
 #define SLF_INUSE		0		/* Channel in use            */
 #define SLF_ERROR		1               /* Parity, etc. error        */
+	unsigned char		can_flags;	/* CAN status flags          */
+/* #define SLC_CAN_FLAG_RFIFO_FULL BIT(0) */
+/* #define SLC_CAN_FLAG_TFIFO_FULL BIT(1) */
+#define SLC_CAN_FLAG_ERR_WARN   BIT(2)
+#define SLC_CAN_FLAG_OVERRUN    BIT(3)
+/* 4 is unused */
+#define SLC_CAN_FLAG_ERR_PASV   BIT(5)
+#define SLC_CAN_FLAG_ARB_LOST   BIT(6)
+#define SLC_CAN_FLAG_BUS_ERR    BIT(7)
+#define SLC_CAN_FLAG_VALID (SLC_CAN_FLAG_ERR_WARN | SLC_CAN_FLAG_OVERRUN \
+		| SLC_CAN_FLAG_ERR_PASV | SLC_CAN_FLAG_ARB_LOST \
+		| SLC_CAN_FLAG_BUS_ERR)
+
+	struct delayed_work work;
 };
 
 static struct net_device **slcan_devs;
@@ -105,6 +128,18 @@ static DEFINE_MUTEX(slcan_mutex);
 
 #define CLOSE_COMMAND "C\r"
 #define OPEN_COMMAND "O\r"
+#define FLAGS_COMMAND "F\r"
+
+static void slcan_queue_work(struct slcan *sl)
+{
+	unsigned long delay;
+
+	delay = msecs_to_jiffies(status_interval_ms);
+	if (delay >= HZ)
+		delay = round_jiffies_relative(delay);
+
+	queue_delayed_work(system_freezable_wq, &sl->work, delay);
+}
 
 static int slc_open_device(struct slcan *sl) __must_hold(&sl->lock)
 {
@@ -119,6 +154,7 @@ static int slc_open_device(struct slcan *sl) __must_hold(&sl->lock)
 		}
 	}
 	sl->can.state = CAN_STATE_ERROR_ACTIVE;
+	slcan_queue_work(sl);
 	return 0;
 }
 
@@ -126,6 +162,11 @@ static int slc_close_device(struct slcan *sl) __must_hold(&sl->lock)
 {
 	int written;
 
+	/* Release the lock while waiting for the delayed work to finish */
+	spin_unlock_bh(&sl->lock);
+	cancel_delayed_work_sync(&sl->work);
+	spin_lock_bh(&sl->lock);
+
 	if (sl->tty) {
 		written = sl->tty->ops->write(sl->tty, CLOSE_COMMAND,
 					      strlen(CLOSE_COMMAND));
@@ -137,6 +178,22 @@ static int slc_close_device(struct slcan *sl) __must_hold(&sl->lock)
 	return 0;
 }
 
+static void slcan_status_work(struct work_struct *work)
+{
+	struct slcan *sl = container_of(work, struct slcan, work.work);
+	int written;
+
+	spin_lock_bh(&sl->lock);
+	if (sl->tty) {
+		written = sl->tty->ops->write(sl->tty, FLAGS_COMMAND,
+					      strlen(FLAGS_COMMAND));
+		if (written != strlen(FLAGS_COMMAND))
+			netdev_warn(sl->dev, "Flags command could not be sent!\n");
+	}
+	spin_unlock_bh(&sl->lock);
+	slcan_queue_work(sl);
+}
+
  /************************************************************************
   *			SLCAN ENCAPSULATION FORMAT			 *
   ************************************************************************/
@@ -176,6 +233,94 @@ static int slc_close_device(struct slcan *sl) __must_hold(&sl->lock)
   *			STANDARD SLCAN DECAPSULATION			 *
   ************************************************************************/
 
+/* Analyse the error flags and send an error frame if approriate */
+static void slc_handle_flags(struct slcan *sl)
+{
+	struct sk_buff *skb;
+	struct can_frame *cf;
+	struct net_device_stats *stats = &sl->dev->stats;
+	enum can_state state = sl->dev->state;
+	u8 new_flags;
+
+	if (hex2bin(&new_flags, sl->rbuff + SLC_CMD_LEN, 1))
+		return;
+
+	new_flags &= SLC_CAN_FLAG_VALID;
+
+	if (!(new_flags ^ sl->can_flags))
+		return;
+
+	skb = alloc_can_err_skb(sl->dev, &cf);
+	if (!skb)
+		return;
+
+	state = CAN_STATE_ERROR_ACTIVE;
+	if (new_flags & SLC_CAN_FLAG_ARB_LOST) {
+		sl->can.can_stats.arbitration_lost++;
+		stats->tx_errors++;
+		cf->can_id |= CAN_ERR_LOSTARB;
+	}
+	if (new_flags & SLC_CAN_FLAG_OVERRUN) {
+		cf->can_id |= CAN_ERR_CRTL;
+		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
+		stats->rx_over_errors++;
+		stats->rx_errors++;
+	}
+	if (new_flags & SLC_CAN_FLAG_ERR_WARN)
+		state = CAN_STATE_ERROR_WARNING;
+	if (new_flags & SLC_CAN_FLAG_ERR_PASV)
+		state = CAN_STATE_ERROR_PASSIVE;
+	if (new_flags & SLC_CAN_FLAG_BUS_ERR)
+		state = CAN_STATE_BUS_OFF;
+
+	if (state != sl->can.state) {
+		stats->rx_errors++;
+
+		switch (state) {
+		case CAN_STATE_ERROR_ACTIVE:
+			cf->can_id |= CAN_ERR_PROT;
+			cf->data[2] = CAN_ERR_PROT_ACTIVE;
+			break;
+		case CAN_STATE_ERROR_WARNING:
+			cf->can_id |= CAN_ERR_CRTL;
+			cf->data[1] = CAN_ERR_CRTL_RX_WARNING |
+					CAN_ERR_CRTL_TX_WARNING;
+			sl->can.can_stats.error_warning++;
+			break;
+		case CAN_STATE_ERROR_PASSIVE:
+			cf->can_id |= CAN_ERR_CRTL;
+			cf->data[1] = CAN_ERR_CRTL_RX_PASSIVE |
+					CAN_ERR_CRTL_TX_PASSIVE;
+			sl->can.can_stats.error_passive++;
+			break;
+		case CAN_STATE_BUS_OFF:
+			cf->can_id |= CAN_ERR_BUSOFF;
+			can_bus_off(sl->dev);
+
+			/* Disable status fetching until
+			 * serial interface is restarted
+			 */
+			cancel_delayed_work_sync(&sl->work);
+			break;
+		case CAN_STATE_STOPPED:
+		case CAN_STATE_SLEEPING:
+		case CAN_STATE_MAX:
+			WARN_ON_ONCE(1);
+			break;
+		}
+	}
+
+	sl->can_flags = new_flags;
+	sl->can.state = state;
+
+	netif_rx_ni(skb);
+
+	stats->rx_packets++;
+	stats->rx_bytes += cf->can_dlc;
+
+	return;
+}
+
 /* Send one completely decapsulated can_frame to the network layer */
 static void slc_bump(struct slcan *sl)
 {
@@ -190,6 +335,9 @@ static void slc_bump(struct slcan *sl)
 	can_id = 0;
 
 	switch (*cmd) {
+	case 'F':
+		slc_handle_flags(sl);
+		return;
 	case 'r':
 		can_id = CAN_RTR_FLAG;
 		/* fallthrough */
@@ -258,7 +406,7 @@ 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))  {
+		    (sl->rcount > 2))  {
 			slc_bump(sl);
 		}
 		sl->rcount = 0;
@@ -454,6 +602,38 @@ static int slc_open(struct net_device *dev)
 	return 0;
 }
 
+static int slc_set_mode(struct net_device *dev, enum can_mode mode)
+{
+	struct slcan *sl = netdev_priv(dev);
+	int err;
+
+	switch (mode) {
+	case CAN_MODE_START:
+		spin_lock_bh(&sl->lock);
+
+		err = slc_close_device(sl);
+		if (err) {
+			spin_unlock_bh(&sl->lock);
+			return err;
+		}
+		err = slc_open_device(sl);
+		if (err) {
+			spin_unlock_bh(&sl->lock);
+			return err;
+		}
+
+		netif_wake_queue(dev);
+
+		spin_unlock_bh(&sl->lock);
+		break;
+
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
 static int slc_set_bittiming(struct net_device *dev)
 {
 	struct slcan *sl = netdev_priv(dev);
@@ -655,8 +835,12 @@ static int slcan_open(struct tty_struct *tty)
 
 	sl->tty = tty;
 	tty->disc_data = sl;
+	INIT_DELAYED_WORK(&sl->work, slcan_status_work);
 
+	/* slc_close_device expects the lock to be taken */
+	spin_lock_bh(&sl->lock);
 	err = slc_close_device(sl);
+	spin_unlock_bh(&sl->lock);
 	if (err)
 		goto err_free_chan;
 
@@ -666,6 +850,7 @@ static int slcan_open(struct tty_struct *tty)
 		sl->xleft    = 0;
 
 		sl->can.do_set_bittiming = slc_set_bittiming;
+		sl->can.do_set_mode = slc_set_mode;
 		set_bit(SLF_INUSE, &sl->flags);
 
 		err = register_candev(sl->dev);
-- 
1.8.3.2

Re: [PATCH 2/5] can: slcan: Convert to can_dev interface

[Marc Kleine-Budde]

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

On 04/15/2014 04:51 PM, Alexander Stein wrote:
> No actual feature change despite the interface name. This is just a
> preparation for additional CAN related features.
> 
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> ---
>  drivers/net/can/Kconfig |  40 +++++++++----------
>  drivers/net/can/slcan.c | 104 +++++++++++++++++++++---------------------------
>  2 files changed, 65 insertions(+), 79 deletions(-)
> 
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 9e7d95d..daea043 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -9,26 +9,6 @@ config CAN_VCAN
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called vcan.
>  
> -config CAN_SLCAN
> -	tristate "Serial / USB serial CAN Adaptors (slcan)"
> -	depends on TTY
> -	---help---
> -	  CAN driver for several 'low cost' CAN interfaces that are attached
> -	  via serial lines or via USB-to-serial adapters using the LAWICEL
> -	  ASCII protocol. The driver implements the tty linediscipline N_SLCAN.
> -
> -	  As only the sending and receiving of CAN frames is implemented, this
> -	  driver should work with the (serial/USB) CAN hardware from:
> -	  www.canusb.com / www.can232.com / www.mictronics.de / www.canhack.de
> -
> -	  Userspace tools to attach the SLCAN line discipline (slcan_attach,
> -	  slcand) can be found in the can-utils at the SocketCAN SVN, see
> -	  http://developer.berlios.de/projects/socketcan for details.
> -
> -	  The slcan driver supports up to 10 CAN netdevices by default which
> -	  can be changed by the 'maxdev=xx' module option. This driver can
> -	  also be built as a module. If so, the module will be called slcan.
> -
>  config CAN_DEV
>  	tristate "Platform CAN drivers with Netlink support"
>  	default y
> @@ -125,6 +105,26 @@ config CAN_GRCAN
>  	  endian syntheses of the cores would need some modifications on
>  	  the hardware level to work.
>  
> +config CAN_SLCAN
> +	tristate "Serial / USB serial CAN Adaptors (slcan)"
> +	depends on TTY
> +	---help---
> +	  CAN driver for several 'low cost' CAN interfaces that are attached
> +	  via serial lines or via USB-to-serial adapters using the LAWICEL
> +	  ASCII protocol. The driver implements the tty linediscipline N_SLCAN.
> +
> +	  As only the sending and receiving of CAN frames is implemented, this
> +	  driver should work with the (serial/USB) CAN hardware from:
> +	  www.canusb.com / www.can232.com / www.mictronics.de / www.canhack.de
> +
> +	  Userspace tools to attach the SLCAN line discipline (slcan_attach,
> +	  slcand) can be found in the can-utils at the SocketCAN SVN, see
> +	  http://developer.berlios.de/projects/socketcan for details.
> +
> +	  The slcan driver supports up to 10 CAN netdevices by default which
> +	  can be changed by the 'maxdev=xx' module option. This driver can
> +	  also be built as a module. If so, the module will be called slcan.
> +
>  source "drivers/net/can/mscan/Kconfig"
>  
>  source "drivers/net/can/sja1000/Kconfig"
> diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
> index 2577c1e..337e6e3 100644
> --- a/drivers/net/can/slcan.c
> +++ b/drivers/net/can/slcan.c
> @@ -53,6 +53,7 @@
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/can.h>
> +#include <linux/can/dev.h>
>  #include <linux/can/skb.h>
>  
>  static __initconst const char banner[] =
> @@ -79,6 +80,7 @@ MODULE_PARM_DESC(maxdev, "Maximum number of slcan interfaces");
>  #define SLC_EFF_ID_LEN 8
>  
>  struct slcan {
> +	struct can_priv		can;	/* must be the first member */
>  	int			magic;
>  
>  	/* Various fields. */
> @@ -99,6 +101,7 @@ struct slcan {
>  };
>  
>  static struct net_device **slcan_devs;
> +static DEFINE_MUTEX(slcan_mutex);
>  
>   /************************************************************************
>    *			SLCAN ENCAPSULATION FORMAT			 *
> @@ -143,31 +146,33 @@ static struct net_device **slcan_devs;
>  static void slc_bump(struct slcan *sl)
>  {
>  	struct sk_buff *skb;
> -	struct can_frame cf;
> +	struct can_frame *cf;
> +	canid_t can_id;
> +	u8 can_dlc;
>  	int i, tmp;
>  	u32 tmpid;
>  	char *cmd = sl->rbuff;
>  
> -	cf.can_id = 0;
> +	can_id = 0;
>  
>  	switch (*cmd) {
>  	case 'r':
> -		cf.can_id = CAN_RTR_FLAG;
> +		can_id = CAN_RTR_FLAG;
>  		/* fallthrough */
>  	case 't':
>  		/* store dlc ASCII value and terminate SFF CAN ID string */
> -		cf.can_dlc = sl->rbuff[SLC_CMD_LEN + SLC_SFF_ID_LEN];
> +		can_dlc = sl->rbuff[SLC_CMD_LEN + SLC_SFF_ID_LEN];
>  		sl->rbuff[SLC_CMD_LEN + SLC_SFF_ID_LEN] = 0;
>  		/* point to payload data behind the dlc */
>  		cmd += SLC_CMD_LEN + SLC_SFF_ID_LEN + 1;
>  		break;
>  	case 'R':
> -		cf.can_id = CAN_RTR_FLAG;
> +		can_id = CAN_RTR_FLAG;
>  		/* fallthrough */
>  	case 'T':
> -		cf.can_id |= CAN_EFF_FLAG;
> +		can_id |= CAN_EFF_FLAG;
>  		/* store dlc ASCII value and terminate EFF CAN ID string */
> -		cf.can_dlc = sl->rbuff[SLC_CMD_LEN + SLC_EFF_ID_LEN];
> +		can_dlc = sl->rbuff[SLC_CMD_LEN + SLC_EFF_ID_LEN];
>  		sl->rbuff[SLC_CMD_LEN + SLC_EFF_ID_LEN] = 0;
>  		/* point to payload data behind the dlc */
>  		cmd += SLC_CMD_LEN + SLC_EFF_ID_LEN + 1;
> @@ -179,49 +184,39 @@ static void slc_bump(struct slcan *sl)
>  	if (kstrtou32(sl->rbuff + SLC_CMD_LEN, 16, &tmpid))
>  		return;
>  
> -	cf.can_id |= tmpid;
> +	can_id |= tmpid;
>  
>  	/* get can_dlc from sanitized ASCII value */
> -	if (cf.can_dlc >= '0' && cf.can_dlc < '9')
> -		cf.can_dlc -= '0';
> +	if (can_dlc >= '0' && can_dlc < '9')
> +		can_dlc -= '0';
>  	else
>  		return;
>  
> -	*(u64 *) (&cf.data) = 0; /* clear payload */
> +	skb = alloc_can_skb(sl->dev, &cf);
> +	if (!skb)
> +		return;
> +	cf->can_id = can_id;
> +	cf->can_dlc = can_dlc;
>  
>  	/* RTR frames may have a dlc > 0 but they never have any data bytes */
> -	if (!(cf.can_id & CAN_RTR_FLAG)) {
> -		for (i = 0; i < cf.can_dlc; i++) {
> +	if (!(cf->can_id & CAN_RTR_FLAG)) {
> +		for (i = 0; i < cf->can_dlc; i++) {
>  			tmp = hex_to_bin(*cmd++);
>  			if (tmp < 0)
>  				return;
> -			cf.data[i] = (tmp << 4);
> +			cf->data[i] = (tmp << 4);
>  			tmp = hex_to_bin(*cmd++);
>  			if (tmp < 0)
>  				return;
> -			cf.data[i] |= tmp;
> +			cf->data[i] |= tmp;
>  		}
>  	}
> -
> -	skb = dev_alloc_skb(sizeof(struct can_frame) +
> -			    sizeof(struct can_skb_priv));
> -	if (!skb)
> -		return;
> -
>  	skb->dev = sl->dev;

I think this is not needed anymore, as alloc_can_skb() sets this already.

> -	skb->protocol = htons(ETH_P_CAN);
> -	skb->pkt_type = PACKET_BROADCAST;
> -	skb->ip_summed = CHECKSUM_UNNECESSARY;
> -
> -	can_skb_reserve(skb);
> -	can_skb_prv(skb)->ifindex = sl->dev->ifindex;
>  
> -	memcpy(skb_put(skb, sizeof(struct can_frame)),
> -	       &cf, sizeof(struct can_frame));
>  	netif_rx_ni(skb);
>  
>  	sl->dev->stats.rx_packets++;
> -	sl->dev->stats.rx_bytes += cf.can_dlc;
> +	sl->dev->stats.rx_bytes += cf->can_dlc;

Please don't touch the skb (and this cf) after netif_rx_ni

>  }
>  
>  /* parse tty input stream */
> @@ -385,6 +380,10 @@ static int slc_close(struct net_device *dev)
>  	netif_stop_queue(dev);
>  	sl->rcount   = 0;
>  	sl->xleft    = 0;
> +	sl->can.state = CAN_STATE_STOPPED;
> +
> +	close_candev(dev);
> +
>  	spin_unlock_bh(&sl->lock);
>  
>  	return 0;
> @@ -394,12 +393,19 @@ static int slc_close(struct net_device *dev)
>  static int slc_open(struct net_device *dev)
>  {
>  	struct slcan *sl = netdev_priv(dev);
> +	int err;
>  
>  	if (sl->tty == NULL)
>  		return -ENODEV;
>  
> +	err = open_candev(dev);
> +	if (err)
> +		return err;
> +
> +	sl->can.state = CAN_STATE_ERROR_ACTIVE;
>  	sl->flags &= (1 << SLF_INUSE);
>  	netif_start_queue(dev);
> +
>  	return 0;
>  }
>  
> @@ -407,7 +413,7 @@ static int slc_open(struct net_device *dev)
>  static void slc_free_netdev(struct net_device *dev)
>  {
>  	int i = dev->base_addr;
> -	free_netdev(dev);
> +	free_candev(dev);
>  	slcan_devs[i] = NULL;
>  }
>  
> @@ -417,23 +423,6 @@ static const struct net_device_ops slc_netdev_ops = {
>  	.ndo_start_xmit         = slc_xmit,
>  };
>  
> -static void slc_setup(struct net_device *dev)
> -{
> -	dev->netdev_ops		= &slc_netdev_ops;
> -	dev->destructor		= slc_free_netdev;
> -
> -	dev->hard_header_len	= 0;
> -	dev->addr_len		= 0;
> -	dev->tx_queue_len	= 10;
> -
> -	dev->mtu		= sizeof(struct can_frame);
> -	dev->type		= ARPHRD_CAN;
> -
> -	/* New-style flags. */
> -	dev->flags		= IFF_NOARP;
> -	dev->features           = NETIF_F_HW_CSUM;
> -}
> -
>  /******************************************
>    Routines looking at TTY side.
>   ******************************************/
> @@ -495,7 +484,6 @@ static void slc_sync(void)
>  static struct slcan *slc_alloc(dev_t line)
>  {
>  	int i;
> -	char name[IFNAMSIZ];
>  	struct net_device *dev = NULL;
>  	struct slcan       *sl;
>  
> @@ -510,12 +498,13 @@ static struct slcan *slc_alloc(dev_t line)
>  	if (i >= maxdev)
>  		return NULL;
>  
> -	sprintf(name, "slcan%d", i);
> -	dev = alloc_netdev(sizeof(*sl), name, slc_setup);
> +	dev = alloc_candev(sizeof(*sl), 1);
>  	if (!dev)
>  		return NULL;
>  
>  	dev->base_addr  = i;
> +	dev->netdev_ops = &slc_netdev_ops;
> +	dev->destructor = slc_free_netdev;
>  	sl = netdev_priv(dev);
>  
>  	/* Initialize channel control data */
> @@ -548,11 +537,8 @@ static int slcan_open(struct tty_struct *tty)
>  	if (tty->ops->write == NULL)
>  		return -EOPNOTSUPP;
>  
> -	/* RTnetlink lock is misused here to serialize concurrent
> -	   opens of slcan channels. There are better ways, but it is
> -	   the simplest one.
> -	 */
> -	rtnl_lock();
> +	/* Serialize concurrent opens of slcan channels. */
> +	mutex_lock(&slcan_mutex);
>  
>  	/* Collect hanged up channels. */
>  	slc_sync();
> @@ -580,13 +566,13 @@ static int slcan_open(struct tty_struct *tty)
>  
>  		set_bit(SLF_INUSE, &sl->flags);
>  
> -		err = register_netdevice(sl->dev);
> +		err = register_candev(sl->dev);
>  		if (err)
>  			goto err_free_chan;
>  	}
>  
>  	/* Done.  We have linked the TTY line to a channel. */
> -	rtnl_unlock();
> +	mutex_unlock(&slcan_mutex);
>  	tty->receive_room = 65536;	/* We don't flow control */
>  
>  	/* TTY layer expects 0 on success */
> @@ -598,7 +584,7 @@ err_free_chan:
>  	clear_bit(SLF_INUSE, &sl->flags);
>  
>  err_exit:
> -	rtnl_unlock();
> +	mutex_unlock(&slcan_mutex);
>  
>  	/* Count references from TTY module */
>  	return err;
> 

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH 4/5] can: slcan: Add bitrate change support

[Marc Kleine-Budde]

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

On 04/15/2014 04:51 PM, Alexander Stein wrote:
> Only a fixed set of bitrates are supported.
> 
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> ---
>  drivers/net/can/Kconfig |  5 +++--
>  drivers/net/can/slcan.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index daea043..267aec2 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -113,8 +113,9 @@ config CAN_SLCAN
>  	  via serial lines or via USB-to-serial adapters using the LAWICEL
>  	  ASCII protocol. The driver implements the tty linediscipline N_SLCAN.
>  
> -	  As only the sending and receiving of CAN frames is implemented, this
> -	  driver should work with the (serial/USB) CAN hardware from:
> +	  Sending, receiving of CAN frames as well as bitrate setting is
> +	  implemented, this driver should work with the
> +	  (serial/USB) CAN hardware from:
>  	  www.canusb.com / www.can232.com / www.mictronics.de / www.canhack.de
>  
>  	  Userspace tools to attach the SLCAN line discipline (slcan_attach,
> diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
> index 8a578bb..8d98f50 100644
> --- a/drivers/net/can/slcan.c
> +++ b/drivers/net/can/slcan.c
> @@ -454,6 +454,58 @@ static int slc_open(struct net_device *dev)
>  	return 0;
>  }
>  
> +static int slc_set_bittiming(struct net_device *dev)
> +{
> +	struct slcan *sl = netdev_priv(dev);
> +	const struct can_bittiming *bt = &sl->can.bittiming;
> +	int index, written, len;
> +	char buf[4];
> +
> +	switch (bt->bitrate) {
> +	case 1000000:
> +		index = 8;
> +		break;
> +	case 800000:
> +		index = 7;
> +		break;
> +	case 500000:
> +		index = 6;
> +		break;
> +	case 250000:
> +		index = 5;
> +		break;
> +	case 125000:
> +		index = 4;
> +		break;
> +	case 100000:
> +		index = 3;
> +		break;
> +	case 50000:
> +		index = 2;
> +		break;
> +	case 20000:
> +		index = 1;
> +		break;
> +	case 10000:
> +		index = 0;
> +		break;
> +	default:
> +		return -EINVAL;
> +	};
> +
> +	if (!sl->tty)
> +		return -ENOENT;
> +
> +	len = snprintf(buf, sizeof(buf), "S%d\r", index);
> +	written = sl->tty->ops->write(sl->tty, buf, len);
> +	if (written != len) {
> +		netdev_warn(sl->dev, "Setting bitrate command could not be sent!\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
>  /* Hook the destructor so we can free slcan devs at the right point in time */
>  static void slc_free_netdev(struct net_device *dev)
>  {
> @@ -613,6 +665,7 @@ static int slcan_open(struct tty_struct *tty)
>  		sl->rcount   = 0;
>  		sl->xleft    = 0;
>  
> +		sl->can.do_set_bittiming = slc_set_bittiming;

Can you move the slc_set_bittiming into the open function? Then you
don't need to assign this callback.

>  		set_bit(SLF_INUSE, &sl->flags);
>  
>  		err = register_candev(sl->dev);
> 

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH 5/5] can: slcan: Add CAN status flag support

[Marc Kleine-Budde]

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

On 04/15/2014 04:51 PM, Alexander Stein wrote:
> CAN status flags have to be polled from the serial device. So send the
> request every 10ms by default.
> 
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> ---
>  drivers/net/can/Kconfig |   4 +-
>  drivers/net/can/slcan.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 188 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 267aec2..9aa38943 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -113,8 +113,8 @@ config CAN_SLCAN
>  	  via serial lines or via USB-to-serial adapters using the LAWICEL
>  	  ASCII protocol. The driver implements the tty linediscipline N_SLCAN.
>  
> -	  Sending, receiving of CAN frames as well as bitrate setting is
> -	  implemented, this driver should work with the
> +	  Sending, receiving of CAN frames as well as bitrate setting and
> +	  error handling is implemented, this driver should work with the
>  	  (serial/USB) CAN hardware from:
>  	  www.canusb.com / www.can232.com / www.mictronics.de / www.canhack.de
>  
> diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
> index 8d98f50..35e5392 100644
> --- a/drivers/net/can/slcan.c
> +++ b/drivers/net/can/slcan.c
> @@ -72,6 +72,15 @@ static int maxdev = 10;		/* MAX number of SLCAN channels;
>  module_param(maxdev, int, 0);
>  MODULE_PARM_DESC(maxdev, "Maximum number of slcan interfaces");
>  
> +/** \brief Status poll interval (in ms) in which the CAN status shall be polled
> + *
> + * Interval (in ms) in which the CAN status shall be polled.
> + * Can be overridden with insmod slcan.ko status_interval_ms=nnn
> + */
> +static int status_interval_ms = 10;
> +module_param(status_interval_ms, uint, 0);
> +MODULE_PARM_DESC(status_interval_ms, "Status poll interval [ms]");
> +
>  /* maximum rx buffer len: extended CAN frame with timestamp */
>  #define SLC_MTU (sizeof("T1111222281122334455667788EA5F\r")+1)
>  
> @@ -98,6 +107,20 @@ struct slcan {
>  	unsigned long		flags;		/* Flag values/ mode etc     */
>  #define SLF_INUSE		0		/* Channel in use            */
>  #define SLF_ERROR		1               /* Parity, etc. error        */
> +	unsigned char		can_flags;	/* CAN status flags          */
> +/* #define SLC_CAN_FLAG_RFIFO_FULL BIT(0) */
> +/* #define SLC_CAN_FLAG_TFIFO_FULL BIT(1) */
> +#define SLC_CAN_FLAG_ERR_WARN   BIT(2)
> +#define SLC_CAN_FLAG_OVERRUN    BIT(3)
> +/* 4 is unused */
> +#define SLC_CAN_FLAG_ERR_PASV   BIT(5)
> +#define SLC_CAN_FLAG_ARB_LOST   BIT(6)
> +#define SLC_CAN_FLAG_BUS_ERR    BIT(7)
> +#define SLC_CAN_FLAG_VALID (SLC_CAN_FLAG_ERR_WARN | SLC_CAN_FLAG_OVERRUN \
> +		| SLC_CAN_FLAG_ERR_PASV | SLC_CAN_FLAG_ARB_LOST \
> +		| SLC_CAN_FLAG_BUS_ERR)
> +
> +	struct delayed_work work;
>  };
>  
>  static struct net_device **slcan_devs;
> @@ -105,6 +128,18 @@ static DEFINE_MUTEX(slcan_mutex);
>  
>  #define CLOSE_COMMAND "C\r"
>  #define OPEN_COMMAND "O\r"
> +#define FLAGS_COMMAND "F\r"
> +
> +static void slcan_queue_work(struct slcan *sl)
> +{
> +	unsigned long delay;
> +
> +	delay = msecs_to_jiffies(status_interval_ms);
> +	if (delay >= HZ)
> +		delay = round_jiffies_relative(delay);
> +
> +	queue_delayed_work(system_freezable_wq, &sl->work, delay);
> +}
>  
>  static int slc_open_device(struct slcan *sl) __must_hold(&sl->lock)
>  {
> @@ -119,6 +154,7 @@ static int slc_open_device(struct slcan *sl) __must_hold(&sl->lock)
>  		}
>  	}
>  	sl->can.state = CAN_STATE_ERROR_ACTIVE;
> +	slcan_queue_work(sl);
>  	return 0;
>  }
>  
> @@ -126,6 +162,11 @@ static int slc_close_device(struct slcan *sl) __must_hold(&sl->lock)
>  {
>  	int written;
>  
> +	/* Release the lock while waiting for the delayed work to finish */
> +	spin_unlock_bh(&sl->lock);
> +	cancel_delayed_work_sync(&sl->work);
> +	spin_lock_bh(&sl->lock);
> +
>  	if (sl->tty) {
>  		written = sl->tty->ops->write(sl->tty, CLOSE_COMMAND,
>  					      strlen(CLOSE_COMMAND));
> @@ -137,6 +178,22 @@ static int slc_close_device(struct slcan *sl) __must_hold(&sl->lock)
>  	return 0;
>  }
>  
> +static void slcan_status_work(struct work_struct *work)
> +{
> +	struct slcan *sl = container_of(work, struct slcan, work.work);
> +	int written;
> +
> +	spin_lock_bh(&sl->lock);
> +	if (sl->tty) {
> +		written = sl->tty->ops->write(sl->tty, FLAGS_COMMAND,
> +					      strlen(FLAGS_COMMAND));
> +		if (written != strlen(FLAGS_COMMAND))
> +			netdev_warn(sl->dev, "Flags command could not be sent!\n");
> +	}
> +	spin_unlock_bh(&sl->lock);
> +	slcan_queue_work(sl);
> +}
> +
>   /************************************************************************
>    *			SLCAN ENCAPSULATION FORMAT			 *
>    ************************************************************************/
> @@ -176,6 +233,94 @@ static int slc_close_device(struct slcan *sl) __must_hold(&sl->lock)
>    *			STANDARD SLCAN DECAPSULATION			 *
>    ************************************************************************/
>  
> +/* Analyse the error flags and send an error frame if approriate */
> +static void slc_handle_flags(struct slcan *sl)
> +{
> +	struct sk_buff *skb;
> +	struct can_frame *cf;
> +	struct net_device_stats *stats = &sl->dev->stats;
> +	enum can_state state = sl->dev->state;
> +	u8 new_flags;
> +
> +	if (hex2bin(&new_flags, sl->rbuff + SLC_CMD_LEN, 1))
> +		return;
> +
> +	new_flags &= SLC_CAN_FLAG_VALID;
> +
> +	if (!(new_flags ^ sl->can_flags))
> +		return;
> +
> +	skb = alloc_can_err_skb(sl->dev, &cf);
> +	if (!skb)
> +		return;

Can you please move the stats handling, so that they are updated, even
if the allocation of the skb fails.

> +
> +	state = CAN_STATE_ERROR_ACTIVE;
> +	if (new_flags & SLC_CAN_FLAG_ARB_LOST) {
> +		sl->can.can_stats.arbitration_lost++;
> +		stats->tx_errors++;
> +		cf->can_id |= CAN_ERR_LOSTARB;
> +	}
> +	if (new_flags & SLC_CAN_FLAG_OVERRUN) {
> +		cf->can_id |= CAN_ERR_CRTL;
> +		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> +		stats->rx_over_errors++;
> +		stats->rx_errors++;
> +	}
> +	if (new_flags & SLC_CAN_FLAG_ERR_WARN)
> +		state = CAN_STATE_ERROR_WARNING;
> +	if (new_flags & SLC_CAN_FLAG_ERR_PASV)
> +		state = CAN_STATE_ERROR_PASSIVE;
> +	if (new_flags & SLC_CAN_FLAG_BUS_ERR)
> +		state = CAN_STATE_BUS_OFF;
> +
> +	if (state != sl->can.state) {
> +		stats->rx_errors++;
> +
> +		switch (state) {
> +		case CAN_STATE_ERROR_ACTIVE:
> +			cf->can_id |= CAN_ERR_PROT;
> +			cf->data[2] = CAN_ERR_PROT_ACTIVE;
> +			break;
> +		case CAN_STATE_ERROR_WARNING:
> +			cf->can_id |= CAN_ERR_CRTL;
> +			cf->data[1] = CAN_ERR_CRTL_RX_WARNING |
> +					CAN_ERR_CRTL_TX_WARNING;
> +			sl->can.can_stats.error_warning++;
> +			break;
> +		case CAN_STATE_ERROR_PASSIVE:
> +			cf->can_id |= CAN_ERR_CRTL;
> +			cf->data[1] = CAN_ERR_CRTL_RX_PASSIVE |
> +					CAN_ERR_CRTL_TX_PASSIVE;
> +			sl->can.can_stats.error_passive++;
> +			break;
> +		case CAN_STATE_BUS_OFF:
> +			cf->can_id |= CAN_ERR_BUSOFF;
> +			can_bus_off(sl->dev);
> +
> +			/* Disable status fetching until
> +			 * serial interface is restarted
> +			 */
> +			cancel_delayed_work_sync(&sl->work);
> +			break;
> +		case CAN_STATE_STOPPED:
> +		case CAN_STATE_SLEEPING:
> +		case CAN_STATE_MAX:
> +			WARN_ON_ONCE(1);
> +			break;
> +		}
> +	}
> +
> +	sl->can_flags = new_flags;
> +	sl->can.state = state;
> +
> +	netif_rx_ni(skb);
> +
> +	stats->rx_packets++;
> +	stats->rx_bytes += cf->can_dlc;

Please don't touch cf after netif_rx_ni();

> +
> +	return;
> +}
> +
>  /* Send one completely decapsulated can_frame to the network layer */
>  static void slc_bump(struct slcan *sl)
>  {
> @@ -190,6 +335,9 @@ static void slc_bump(struct slcan *sl)
>  	can_id = 0;
>  
>  	switch (*cmd) {
> +	case 'F':
> +		slc_handle_flags(sl);
> +		return;
>  	case 'r':
>  		can_id = CAN_RTR_FLAG;
>  		/* fallthrough */
> @@ -258,7 +406,7 @@ 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))  {
> +		    (sl->rcount > 2))  {
>  			slc_bump(sl);
>  		}
>  		sl->rcount = 0;
> @@ -454,6 +602,38 @@ static int slc_open(struct net_device *dev)
>  	return 0;
>  }
>  
> +static int slc_set_mode(struct net_device *dev, enum can_mode mode)
> +{
> +	struct slcan *sl = netdev_priv(dev);
> +	int err;
> +
> +	switch (mode) {
> +	case CAN_MODE_START:
> +		spin_lock_bh(&sl->lock);
> +
> +		err = slc_close_device(sl);
> +		if (err) {
> +			spin_unlock_bh(&sl->lock);
> +			return err;
> +		}
> +		err = slc_open_device(sl);
> +		if (err) {
> +			spin_unlock_bh(&sl->lock);
> +			return err;
> +		}
> +
> +		netif_wake_queue(dev);
> +
> +		spin_unlock_bh(&sl->lock);
> +		break;
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
>  static int slc_set_bittiming(struct net_device *dev)
>  {
>  	struct slcan *sl = netdev_priv(dev);
> @@ -655,8 +835,12 @@ static int slcan_open(struct tty_struct *tty)
>  
>  	sl->tty = tty;
>  	tty->disc_data = sl;
> +	INIT_DELAYED_WORK(&sl->work, slcan_status_work);
>  
> +	/* slc_close_device expects the lock to be taken */
> +	spin_lock_bh(&sl->lock);
>  	err = slc_close_device(sl);
> +	spin_unlock_bh(&sl->lock);
>  	if (err)
>  		goto err_free_chan;
>  
> @@ -666,6 +850,7 @@ static int slcan_open(struct tty_struct *tty)
>  		sl->xleft    = 0;
>  
>  		sl->can.do_set_bittiming = slc_set_bittiming;
> +		sl->can.do_set_mode = slc_set_mode;
>  		set_bit(SLF_INUSE, &sl->flags);
>  
>  		err = register_candev(sl->dev);
> 

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: Bitrate and CAN status support for slcan

[Oliver Hartkopp]

Hello Alexander,

IMO your entire patch set has a major design flaw.

The general serial line discipline concept is a data encapsulation driver
which converts PDUs into a serial line data stream.

If you check other implementations of serial line disciplines from

http://lxr.free-electrons.com/source/include/uapi/linux/tty.h

you'll see that their functionality is to encapsulate datagramms and/or
support different modes for this functionality on the serial line.

There's no interaction on the serial stream triggered by open/close of the
device - which additionally breaks the current userspace, which assumes to be
responsible for the ASCII open/close commands.

And when there are working queues that talk to the serial line, these are e.g.
for line fills to prevent tty hangups.

I don't know if the first CAN frame can get lost when slcand issues the open
command and then switches to the line discipline with the ioctl() ???

Therefore the idea to create the open/close commands in slcan.c *may* prevent
a race condition at startup. Do you know whether the received ASCII characters
remain queued when they are not read by slcand which controls the tty until
the line discipline is switched to slcan.c ??

When this is really needed to prevent this kind of startup glitch the
open/close commands need to be created by newly introduced ioctl() calls, so
that slcand can check if the (new) ioctl() exists to decide if it has to send
the ASCII 'open' command before the ldisc ioctl() OR send the 'open' ioctl()
after the ldisc ioctl().

Same problem with the status polling:
It introduces a functionality which has impacts to statistics depending on the
given poll rate. Are the values consumed with answer to the 'F' command or
does the next polled status represent the same status content?

You introduced an automatism which does not look reliable to me.

What about creating a 'F' command on the serial line when you send a CAN frame
with the CAN_ERR_FLAG set down to the slcan device?
The answer of the slcan device can then be converted to a CAN frame similar to
the way you suggested in patch 5.

Finally the bitrate setting approach is broken too:
1. it breaks the serial line discipline design again
2. there's obviously no race problem which is potentially solved
3. there's no bittiming_const which is needed to fill the bitrate via ip tool

Summary:

- Can you please double check if there's a change for a race condition in the
'open' command before the ldisc ioctl() which may? lead to drop the first CAN
frame? I assume there's no such problem as every other line discipline would
have a similar problem then. And if there's no race there's no need to put
open/close into the driver.

- Remove the bitrate setting stuff
- Don't make the slcan driver depend on the CAN_DEV stuff: It's just
encapsulation which is done in slcan.c - it's no really a CAN controller driver.

Sorry but finally only the 'on demand' retrieval of the SLCAN status via
CAN_ERR_FLAG set in a sent CAN frame looks like a potentially valuable
improvement to me.

Thanks for your contribution & best regards,
Oliver

On 15.04.2014 16:51, Alexander Stein wrote:
> This series adds bitrate selection support. It uses the API described
> in the SLCAN-API which is identical on all listed serial interfaces.
> Thus slcan only supports that limited set of bitrates, as only a bitrate
> index is sent over serial wire.
> Patch 5 also adds support for CAN status polling. This has do be done
> regulrarly from CPU side.
> Tests have been done on a LAWICEL CANUSB device.

Re: [PATCH 5/5] can: slcan: Add CAN status flag support

[Oliver Hartkopp]

Hi Alexander,

On 15.04.2014 16:51, Alexander Stein wrote:
> CAN status flags have to be polled from the serial device. So send the
> request every 10ms by default.

here's my suggestion for the status retrieval from the user space by
sending a CAN frame with CAN_ERR_FLAG to the slcan device.

(I don't have this hardware with me - therefore compile tested only)

Regards,
Oliver

---

diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index f5b16e0..750b002 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -54,6 +54,7 @@
 #include <linux/kernel.h>
 #include <linux/can.h>
 #include <linux/can/skb.h>
+#include <linux/can/error.h>
 
 static __initconst const char banner[] =
 	KERN_INFO "slcan: serial line CAN interface driver\n";
@@ -78,6 +79,16 @@ MODULE_PARM_DESC(maxdev, "Maximum number of slcan interfaces");
 #define SLC_SFF_ID_LEN 3
 #define SLC_EFF_ID_LEN 8
 
+/* slcan status bits returned in 'F' response */
+#define SLC_CAN_FLAG_ERR_WARN   BIT(2)
+#define SLC_CAN_FLAG_OVERRUN    BIT(3)
+#define SLC_CAN_FLAG_ERR_PASV   BIT(5)
+#define SLC_CAN_FLAG_ARB_LOST   BIT(6)
+#define SLC_CAN_FLAG_BUS_ERR    BIT(7)
+#define SLC_CAN_FLAG_VALID (SLC_CAN_FLAG_ERR_WARN | SLC_CAN_FLAG_OVERRUN \
+			    | SLC_CAN_FLAG_ERR_PASV | SLC_CAN_FLAG_ARB_LOST \
+			    | SLC_CAN_FLAG_BUS_ERR)
+
 struct slcan {
 	int			magic;
 
@@ -135,6 +146,46 @@ static struct net_device **slcan_devs;
  *
  */
 
+void slc_fill_err_msg(struct can_frame *cf, int tmp)
+{
+	tmp &= SLC_CAN_FLAG_VALID;
+
+	cf->can_id = CAN_ERR_FLAG;
+	cf->can_dlc = CAN_ERR_DLC;
+	*(u64 *) (&cf->data) = 0; /* clear payload */
+
+	if (tmp & SLC_CAN_FLAG_ARB_LOST)
+		cf->can_id |= CAN_ERR_LOSTARB;
+
+	if (tmp & SLC_CAN_FLAG_OVERRUN) {
+		cf->can_id |= CAN_ERR_CRTL;
+		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
+	}
+
+	/* CAN state: Check most severe problems first */
+
+	if (tmp & SLC_CAN_FLAG_BUS_ERR) {
+		cf->can_id |= CAN_ERR_BUSOFF;
+		return;
+	}
+
+	if (tmp & SLC_CAN_FLAG_ERR_PASV) {
+		cf->can_id |= CAN_ERR_CRTL;
+		cf->data[1] = CAN_ERR_CRTL_RX_PASSIVE |	CAN_ERR_CRTL_TX_PASSIVE;
+		return;
+	}
+
+	if (tmp & SLC_CAN_FLAG_ERR_WARN) {
+		cf->can_id |= CAN_ERR_CRTL;
+		cf->data[1] = CAN_ERR_CRTL_RX_WARNING |	CAN_ERR_CRTL_TX_WARNING;
+		return;
+	}
+
+	/* error active - no real problem detected */
+	cf->can_id |= CAN_ERR_PROT;
+	cf->data[2] = CAN_ERR_PROT_ACTIVE;
+}
+
  /************************************************************************
   *			STANDARD SLCAN DECAPSULATION			 *
   ************************************************************************/
@@ -151,6 +202,14 @@ static void slc_bump(struct slcan *sl)
 	cf.can_id = 0;
 
 	switch (*cmd) {
+	case 'F':
+		/* retrieve adapter status information */
+		cmd += SLC_CMD_LEN;
+		tmp = hex_to_bin(*cmd);
+		if (tmp < 0)
+			return;
+		slc_fill_err_msg(&cf, tmp);
+		goto send_cf;
 	case 'r':
 		cf.can_id = CAN_RTR_FLAG;
 		/* fallthrough */
@@ -203,6 +262,7 @@ static void slc_bump(struct slcan *sl)
 		}
 	}
 
+send_cf:
 	skb = dev_alloc_skb(sizeof(struct can_frame) +
 			    sizeof(struct can_skb_priv));
 	if (!skb)
@@ -229,7 +289,7 @@ 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))  {
+		    (sl->rcount > 2))  {
 			slc_bump(sl);
 		}
 		sl->rcount = 0;
@@ -260,6 +320,11 @@ static void slc_encaps(struct slcan *sl, struct can_frame *cf)
 
 	pos = sl->xbuff;
 
+	if (cf->can_id & CAN_ERR_FLAG) {
+		*pos++ = 'F';
+		goto cmd_done;
+	}
+
 	if (cf->can_id & CAN_RTR_FLAG)
 		*pos = 'R'; /* becomes 'r' in standard frame format (SFF) */
 	else
@@ -292,6 +357,7 @@ static void slc_encaps(struct slcan *sl, struct can_frame *cf)
 			pos = hex_byte_pack_upper(pos, cf->data[i]);
 	}
 
+cmd_done:
 	*pos++ = '\r';
 
 	/* Order of next two lines is *very* important.

Re: Bitrate and CAN status support for slcan

[Alexander Stein]

Hello Oliver,

On Saturday 19 April 2014 21:24:28, Oliver Hartkopp wrote:
> IMO your entire patch set has a major design flaw.
> 
> The general serial line discipline concept is a data encapsulation driver
> which converts PDUs into a serial line data stream.
> 
> If you check other implementations of serial line disciplines from
> 
> http://lxr.free-electrons.com/source/include/uapi/linux/tty.h
> 
> you'll see that their functionality is to encapsulate datagramms and/or
> support different modes for this functionality on the serial line.
> 
> There's no interaction on the serial stream triggered by open/close of the
> device - which additionally breaks the current userspace, which assumes to 
be
> responsible for the ASCII open/close commands.

I'm aware that this change behavior is different than before, which is
intended! The current problem we have is that there is no way changing
the bitrate and get CAN status information using the interface you use with
"normal" CAN drivers, e.g. c_can. The whole patch series is intended to make
them behave more similar.

> And when there are working queues that talk to the serial line, these are 
e.g.
> for line fills to prevent tty hangups.
> 
> I don't know if the first CAN frame can get lost when slcand issues the open
> command and then switches to the line discipline with the ioctl() ???
> 
> Therefore the idea to create the open/close commands in slcan.c *may* 
prevent
> a race condition at startup. Do you know whether the received ASCII 
characters
> remain queued when they are not read by slcand which controls the tty until
> the line discipline is switched to slcan.c ??
> 
> When this is really needed to prevent this kind of startup glitch the
> open/close commands need to be created by newly introduced ioctl() calls, so
> that slcand can check if the (new) ioctl() exists to decide if it has to 
send
> the ASCII 'open' command before the ldisc ioctl() OR send the 'open' ioctl()
> after the ldisc ioctl().

The problem here is the bitrate setting using netlink. If the device is
already started because some external userspace application send the 
approriate command no bitrate change can be done, even before the CAN 
interface itself has started!
In order to let slcan change the bitrate it must be ensured where serial CAN
interface is shutdown before and start when the CAN interface is put 'UP'.

> Same problem with the status polling:
> It introduces a functionality which has impacts to statistics depending on 
the
> given poll rate. Are the values consumed with answer to the 'F' command or
> does the next polled status represent the same status content?

AFAIK the represent the current state, so polling them faster without an 
actual state change returns the same as when polling slower.

> You introduced an automatism which does not look reliable to me.
> 
> What about creating a 'F' command on the serial line when you send a CAN 
frame
> with the CAN_ERR_FLAG set down to the slcan device?
> The answer of the slcan device can then be converted to a CAN frame similar 
to
> the way you suggested in patch 5.

Mh, this could be an additional feature, e.g. when polling is disabled (value 
of 0?). But I think the polling should stay, otherwise userspace needs 
additional handling of this special CAN interface compared to others.
IMHO they all should behave as similar as possible.

> Finally the bitrate setting approach is broken too:
> 1. it breaks the serial line discipline design again

My experience to serial line disciplines is next to nothing. Maybe you can 
elaborate why this breaks its design. Or give link me to some description.

> 2. there's obviously no race problem which is potentially solved

If the slcand is killed the serial CAN interface is still up, so when 
restarted the bitrate cannot be changed using netlink. The interface must be 
shutdown first. Putting up and down is now done in slcan itself to fulfill 
this requirement.

> 3. there's no bittiming_const which is needed to fill the bitrate via ip 
tool

Actually the birate can be set using the ip tool. It's what I've done all the 
time. I checked the sources and every usage of bittiming_const seems pure 
optional. Even for copying from/to kernel over netlink.
Well, in the end I don't know bittiming_const is for, I couldn't figure out 
:-/
slcan is special here, there is ne noeed for some tseg1_min, etc. Here just 
the plain bitrate is put over serial line.

> Summary:
> 
> - Can you please double check if there's a change for a race condition in 
the
> 'open' command before the ldisc ioctl() which may? lead to drop the first 
CAN
> frame? I assume there's no such problem as every other line discipline would
> have a similar problem then. And if there's no race there's no need to put
> open/close into the driver.

open/close is put into the driver in order to be able to change bitrate from 
within that driver.

> - Remove the bitrate setting stuff

Well, this is actually one feature (flags are the others) the whole thing was 
done: Change bitrate and handle CAN errors like when using other CAN 
interfaces.

> - Don't make the slcan driver depend on the CAN_DEV stuff: It's just
> encapsulation which is done in slcan.c - it's no really a CAN controller 
driver.

Which makes it much more circumstantial to use, because it relies on different 
userspace handling.

> Sorry but finally only the 'on demand' retrieval of the SLCAN status via
> CAN_ERR_FLAG set in a sent CAN frame looks like a potentially valuable
> improvement to me.

Don't forget patch 1 ;-)
But thanks for the input anyway.

Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein

SYS TEC electronic GmbH
Am Windrad 2
08468 Heinsdorfergrund
Tel.: 03765 38600-1156
Fax: 03765 38600-4100
Email: alexander.stein@systec-electronic.com
Website: www.systec-electronic.com
 
Managing Director: Dipl.-Phys. Siegmar Schmidt
Commercial registry: Amtsgericht Chemnitz, HRB 28082

Re: Bitrate and CAN status support for slcan

[Oliver Hartkopp]

Hello Alexander,

On 22.04.2014 09:08, Alexander Stein wrote:

> On Saturday 19 April 2014 21:24:28, Oliver Hartkopp wrote:
>> IMO your entire patch set has a major design flaw.
>>
>> The general serial line discipline concept is a data encapsulation driver
>> which converts PDUs into a serial line data stream.
>>
>> If you check other implementations of serial line disciplines from
>>
>> http://lxr.free-electrons.com/source/include/uapi/linux/tty.h
>>
>> you'll see that their functionality is to encapsulate datagramms and/or
>> support different modes for this functionality on the serial line.
>>
>> There's no interaction on the serial stream triggered by open/close of the
>> device - which additionally breaks the current userspace, which assumes to 
> be
>> responsible for the ASCII open/close commands.
> 
> I'm aware that this change behavior is different than before, which is
> intended! The current problem we have is that there is no way changing
> the bitrate and get CAN status information using the interface you use with
> "normal" CAN drivers, e.g. c_can. The whole patch series is intended to make
> them behave more similar.

You can't polish crap. You just can put some whipped cream and a cherry onto
it - which your patch set obviously tries to do ;-)

The SLCAN is a simple encapsulation protocol - and we are also using it in
some setups even when I always try to bring people 'on the right path' :-)

> The problem here is the bitrate setting using netlink. If the device is
> already started because some external userspace application send the 
> approriate command no bitrate change can be done, even before the CAN 
> interface itself has started!
> In order to let slcan change the bitrate it must be ensured where serial CAN
> interface is shutdown before and start when the CAN interface is put 'UP'.

Better solve this with scripts and slcand.

Usually the bitrate of the attached CAN network is known and fixed.
After all the updates from Yegor the slcand is a handy and powerful tool for
SLCAN configuration. Adding netlink support and do fake bitrate calculation
checks, etc. makes the slcan.c even worse.

> 
>> Same problem with the status polling:
>> It introduces a functionality which has impacts to statistics depending on 
> the
>> given poll rate. Are the values consumed with answer to the 'F' command or
>> does the next polled status represent the same status content?
> 
> AFAIK the represent the current state, so polling them faster without an 
> actual state change returns the same as when polling slower.

ok.

I've seen that you compare the status to the previous one to detect changes.
IMO this works with the general status like
SLC_CAN_FLAG_ERR_WARN, SLC_CAN_FLAG_ERR_PASV and SLC_CAN_FLAG_BUS_ERR.

But it does not work with SLC_CAN_FLAG_OVERRUN and SLC_CAN_FLAG_ARB_LOST as
these bits need to be consumed by the SLCAN-adapter to be correct.

That's the problem with polling/sampling stats that are originally event
based. IMHO you try to interpret this 'F' status beyond it's entropy.

>> You introduced an automatism which does not look reliable to me.
>>
>> What about creating a 'F' command on the serial line when you send a CAN 
> frame
>> with the CAN_ERR_FLAG set down to the slcan device?
>> The answer of the slcan device can then be converted to a CAN frame similar 
> to
>> the way you suggested in patch 5.
> 
> Mh, this could be an additional feature, e.g. when polling is disabled (value 
> of 0?). But I think the polling should stay, otherwise userspace needs 
> additional handling of this special CAN interface compared to others.

It will never be like a real CAN controller. SLCAN is a simple but incomplete
protocol to attach some existing low-cost CAN adapters and provide a network
device for CAN applications. And together with slcand it does it's job -
including serial line setup and CAN bitrate setup.

Introducing all your suggested features does not make the SLCAN protocol
better nor does it bring a real benefit to the end user. If you want to have
all the CAN_DEV netlink features just buy a better CAN adapter/controller.

IMO it's an improvement to be able to query the SLCAN adapter status
especially in the case the CAN interface got stuck for some reason.

Don't know if this (polling) needs to be done in kernel space.
Probably slcand can be extended to send and evaluate CAN error messages to
restart the interface then (e.g. once every second).

>> - Don't make the slcan driver depend on the CAN_DEV stuff: It's just
>> encapsulation which is done in slcan.c - it's no really a CAN controller 
> driver.
> 
> Which makes it much more circumstantial to use, because it relies on different 
> userspace handling.

Yes. SLCAN is at least 'special' and we can not really fix this situation by
implementing some lovely configuration interface around it which rises user
expectations beyond the real provided adapter functionality users know from
'good' CAN adapters.

>> Sorry but finally only the 'on demand' retrieval of the SLCAN status via
>> CAN_ERR_FLAG set in a sent CAN frame looks like a potentially valuable
>> improvement to me.
> 
> Don't forget patch 1 ;-)

Oh, yes.

Will send my Acked-by for this patch in a minute!

Best regards,
Oliver

Re: [PATCH 1/5] can: slcan: Fix spinlock variant

[Oliver Hartkopp]

On 15.04.2014 16:51, Alexander Stein wrote:
> slc_xmit is called within softirq context and locks sl->lock, but
> slcan_write_wakeup is not softirq context, so we need to use
> spin_[un]lock_bh!
> Detected using kernel lock debugging mechanism.
> 
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>

Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

Btw. two questions:

1. Is it still valid in slc_xmit() to use spin_lock without _bh then as the
only occurrences in slcan.c?

2. Does this problem also exist in our 'twin' code in slip.c in
slip_write_wakeup() and sl_tx_timeout() then?

Best regards,
Oliver

> ---
> I found this by just enabling the kernel config options named in
> Documentation/SubmitChecklist :)
>> 12: Has been tested with CONFIG_PREEMPT, CONFIG_DEBUG_PREEMPT,
>>    CONFIG_DEBUG_SLAB, CONFIG_DEBUG_PAGEALLOC, CONFIG_DEBUG_MUTEXES,
>>    CONFIG_DEBUG_SPINLOCK, CONFIG_DEBUG_ATOMIC_SLEEP, CONFIG_PROVE_RCU
>>    and CONFIG_DEBUG_OBJECTS_RCU_HEAD all simultaneously enabled.
> 
>  drivers/net/can/slcan.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
> index 3fcdae2..2577c1e 100644
> --- a/drivers/net/can/slcan.c
> +++ b/drivers/net/can/slcan.c
> @@ -322,13 +322,13 @@ static void slcan_write_wakeup(struct tty_struct *tty)
>  	if (!sl || sl->magic != SLCAN_MAGIC || !netif_running(sl->dev))
>  		return;
>  
> -	spin_lock(&sl->lock);
> +	spin_lock_bh(&sl->lock);
>  	if (sl->xleft <= 0)  {
>  		/* Now serial buffer is almost free & we can start
>  		 * transmission of another packet */
>  		sl->dev->stats.tx_packets++;
>  		clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
> -		spin_unlock(&sl->lock);
> +		spin_unlock_bh(&sl->lock);
>  		netif_wake_queue(sl->dev);
>  		return;
>  	}
> @@ -336,7 +336,7 @@ static void slcan_write_wakeup(struct tty_struct *tty)
>  	actual = tty->ops->write(tty, sl->xhead, sl->xleft);
>  	sl->xleft -= actual;
>  	sl->xhead += actual;
> -	spin_unlock(&sl->lock);
> +	spin_unlock_bh(&sl->lock);
>  }
>  
>  /* Send a can_frame to a TTY queue. */
> 

Re: [PATCH 1/5] can: slcan: Fix spinlock variant

[Marc Kleine-Budde]

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

On 04/15/2014 04:51 PM, Alexander Stein wrote:
> slc_xmit is called within softirq context and locks sl->lock, but
> slcan_write_wakeup is not softirq context, so we need to use
> spin_[un]lock_bh!
> Detected using kernel lock debugging mechanism.
> 
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>

Applied to can/master.

Tnx,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]