As suggested by commit [1], now the driver uses the functions and the
data structures provided by the CAN network device driver interface.
Currently the driver doesn't implement a way to set bitrate for SLCAN
based devices via ip tool, so you'll have to do this by slcand or
slcan_attach invocation through the -sX parameter:
- slcan_attach -f -s6 -o /dev/ttyACM0
- slcand -f -s8 -o /dev/ttyUSB0
where -s6 in will set adapter's bitrate to 500 Kbit/s and -s8 to
1Mbit/s.
See the table below for further CAN bitrates:
- s0 -> 10 Kbit/s
- s1 -> 20 Kbit/s
- s2 -> 50 Kbit/s
- s3 -> 100 Kbit/s
- s4 -> 125 Kbit/s
- s5 -> 250 Kbit/s
- s6 -> 500 Kbit/s
- s7 -> 800 Kbit/s
- s8 -> 1000 Kbit/s
In doing so, the struct can_priv::bittiming.bitrate of the driver is not
set and since the open_candev() checks that the bitrate has been set, it
must be a non-zero value, the bitrate is set to a fake value (-1U)
before it is called.
[1] 39549eef3587f ("can: CAN Network device driver and Netlink interface")
Signed-off-by: Dario Binacchi <[email protected]>
---
Changes in v3:
- Replace (-1) with (-1U) in the commit description.
Changes in v2:
- Move CAN_SLCAN Kconfig option inside CAN_DEV scope.
- Improve the commit message.
drivers/net/can/Kconfig | 40 +++++++-------
drivers/net/can/slcan.c | 112 ++++++++++++++++++++--------------------
2 files changed, 77 insertions(+), 75 deletions(-)
diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index b2dcc1e5a388..45997d39621c 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -28,26 +28,6 @@ config CAN_VXCAN
This driver can also be built as a module. If so, the module
will be called vxcan.
-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:
- http://www.canusb.com / http://www.can232.com / http://www.mictronics.de / http://www.canhack.de
-
- Userspace tools to attach the SLCAN line discipline (slcan_attach,
- slcand) can be found in the can-utils at the linux-can project, see
- https://github.com/linux-can/can-utils 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
@@ -118,6 +98,26 @@ config CAN_KVASER_PCIEFD
Kvaser Mini PCI Express HS v2
Kvaser Mini PCI Express 2xHS v2
+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:
+ http://www.canusb.com / http://www.can232.com / http://www.mictronics.de / http://www.canhack.de
+
+ Userspace tools to attach the SLCAN line discipline (slcan_attach,
+ slcand) can be found in the can-utils at the linux-can project, see
+ https://github.com/linux-can/can-utils 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_SUN4I
tristate "Allwinner A10 CAN controller"
depends on MACH_SUN4I || MACH_SUN7I || COMPILE_TEST
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index c39580b142e0..a70f930b7c3a 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -56,7 +56,6 @@
#include <linux/can.h>
#include <linux/can/dev.h>
#include <linux/can/skb.h>
-#include <linux/can/can-ml.h>
MODULE_ALIAS_LDISC(N_SLCAN);
MODULE_DESCRIPTION("serial line CAN interface");
@@ -79,6 +78,7 @@ MODULE_PARM_DESC(maxdev, "Maximum number of slcan interfaces");
#define SLC_EFF_ID_LEN 8
struct slcan {
+ struct can_priv can;
int magic;
/* Various fields. */
@@ -100,6 +100,7 @@ struct slcan {
};
static struct net_device **slcan_devs;
+static DEFINE_SPINLOCK(slcan_lock);
/************************************************************************
* SLCAN ENCAPSULATION FORMAT *
@@ -374,7 +375,7 @@ static netdev_tx_t slc_xmit(struct sk_buff *skb, struct net_device *dev)
spin_unlock(&sl->lock);
out:
- kfree_skb(skb);
+ can_put_echo_skb(skb, dev, 0, 0);
return NETDEV_TX_OK;
}
@@ -394,6 +395,8 @@ static int slc_close(struct net_device *dev)
clear_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags);
}
netif_stop_queue(dev);
+ close_candev(dev);
+ sl->can.state = CAN_STATE_STOPPED;
sl->rcount = 0;
sl->xleft = 0;
spin_unlock_bh(&sl->lock);
@@ -405,21 +408,36 @@ static int slc_close(struct net_device *dev)
static int slc_open(struct net_device *dev)
{
struct slcan *sl = netdev_priv(dev);
+ int err;
if (sl->tty == NULL)
return -ENODEV;
+ /* The baud rate is not set with the command
+ * `ip link set <iface> type can bitrate <baud>' and therefore
+ * can.bittiming.bitrate is 0, causing open_candev() to fail.
+ * So let's set to a fake value.
+ */
+ sl->can.bittiming.bitrate = -1;
+ err = open_candev(dev);
+ if (err) {
+ netdev_err(dev, "failed to open can device\n");
+ return err;
+ }
+
+ sl->can.state = CAN_STATE_ERROR_ACTIVE;
sl->flags &= BIT(SLF_INUSE);
netif_start_queue(dev);
return 0;
}
-/* Hook the destructor so we can free slcan devs at the right point in time */
-static void slc_free_netdev(struct net_device *dev)
+static void slc_dealloc(struct slcan *sl)
{
- int i = dev->base_addr;
+ int i = sl->dev->base_addr;
- slcan_devs[i] = NULL;
+ free_candev(sl->dev);
+ if (slcan_devs)
+ slcan_devs[i] = NULL;
}
static int slcan_change_mtu(struct net_device *dev, int new_mtu)
@@ -434,24 +452,6 @@ static const struct net_device_ops slc_netdev_ops = {
.ndo_change_mtu = slcan_change_mtu,
};
-static void slc_setup(struct net_device *dev)
-{
- dev->netdev_ops = &slc_netdev_ops;
- dev->needs_free_netdev = true;
- dev->priv_destructor = slc_free_netdev;
-
- dev->hard_header_len = 0;
- dev->addr_len = 0;
- dev->tx_queue_len = 10;
-
- dev->mtu = CAN_MTU;
- dev->type = ARPHRD_CAN;
-
- /* New-style flags. */
- dev->flags = IFF_NOARP;
- dev->features = NETIF_F_HW_CSUM;
-}
-
/******************************************
Routines looking at TTY side.
******************************************/
@@ -514,11 +514,8 @@ static void slc_sync(void)
static struct slcan *slc_alloc(void)
{
int i;
- char name[IFNAMSIZ];
struct net_device *dev = NULL;
- struct can_ml_priv *can_ml;
struct slcan *sl;
- int size;
for (i = 0; i < maxdev; i++) {
dev = slcan_devs[i];
@@ -531,16 +528,14 @@ static struct slcan *slc_alloc(void)
if (i >= maxdev)
return NULL;
- sprintf(name, "slcan%d", i);
- size = ALIGN(sizeof(*sl), NETDEV_ALIGN) + sizeof(struct can_ml_priv);
- dev = alloc_netdev(size, name, NET_NAME_UNKNOWN, slc_setup);
+ dev = alloc_candev(sizeof(*sl), 1);
if (!dev)
return NULL;
+ snprintf(dev->name, sizeof(dev->name), "slcan%d", i);
+ dev->netdev_ops = &slc_netdev_ops;
dev->base_addr = i;
sl = netdev_priv(dev);
- can_ml = (void *)sl + ALIGN(sizeof(*sl), NETDEV_ALIGN);
- can_set_ml_priv(dev, can_ml);
/* Initialize channel control data */
sl->magic = SLCAN_MAGIC;
@@ -573,11 +568,7 @@ static int slcan_open(struct tty_struct *tty)
if (tty->ops->write == NULL)
return -EOPNOTSUPP;
- /* RTnetlink lock is misused here to serialize concurrent
- opens of slcan channels. There are better ways, but it is
- the simplest one.
- */
- rtnl_lock();
+ spin_lock(&slcan_lock);
/* Collect hanged up channels. */
slc_sync();
@@ -605,13 +596,15 @@ static int slcan_open(struct tty_struct *tty)
set_bit(SLF_INUSE, &sl->flags);
- err = register_netdevice(sl->dev);
- if (err)
+ err = register_candev(sl->dev);
+ if (err) {
+ pr_err("slcan: can't register candev\n");
goto err_free_chan;
+ }
}
/* Done. We have linked the TTY line to a channel. */
- rtnl_unlock();
+ spin_unlock(&slcan_lock);
tty->receive_room = 65536; /* We don't flow control */
/* TTY layer expects 0 on success */
@@ -621,14 +614,10 @@ static int slcan_open(struct tty_struct *tty)
sl->tty = NULL;
tty->disc_data = NULL;
clear_bit(SLF_INUSE, &sl->flags);
- slc_free_netdev(sl->dev);
- /* do not call free_netdev before rtnl_unlock */
- rtnl_unlock();
- free_netdev(sl->dev);
- return err;
+ slc_dealloc(sl);
err_exit:
- rtnl_unlock();
+ spin_unlock(&slcan_lock);
/* Count references from TTY module */
return err;
@@ -658,9 +647,11 @@ static void slcan_close(struct tty_struct *tty)
synchronize_rcu();
flush_work(&sl->tx_work);
- /* Flush network side */
- unregister_netdev(sl->dev);
- /* This will complete via sl_free_netdev */
+ slc_close(sl->dev);
+ unregister_candev(sl->dev);
+ spin_lock(&slcan_lock);
+ slc_dealloc(sl);
+ spin_unlock(&slcan_lock);
}
static void slcan_hangup(struct tty_struct *tty)
@@ -768,18 +759,29 @@ static void __exit slcan_exit(void)
dev = slcan_devs[i];
if (!dev)
continue;
- slcan_devs[i] = NULL;
- sl = netdev_priv(dev);
- if (sl->tty) {
- netdev_err(dev, "tty discipline still running\n");
- }
+ spin_lock(&slcan_lock);
+ dev = slcan_devs[i];
+ if (dev) {
+ slcan_devs[i] = NULL;
+ spin_unlock(&slcan_lock);
+ sl = netdev_priv(dev);
+ if (sl->tty) {
+ netdev_err(dev,
+ "tty discipline still running\n");
+ }
- unregister_netdev(dev);
+ slc_close(dev);
+ unregister_candev(dev);
+ } else {
+ spin_unlock(&slcan_lock);
+ }
}
+ spin_lock(&slcan_lock);
kfree(slcan_devs);
slcan_devs = NULL;
+ spin_unlock(&slcan_lock);
tty_unregister_ldisc(&slc_ldisc);
}
--
2.32.0
On 12.06.2022 23:39:18, Dario Binacchi wrote:
> As suggested by commit [1], now the driver uses the functions and the
> data structures provided by the CAN network device driver interface.
>
> Currently the driver doesn't implement a way to set bitrate for SLCAN
> based devices via ip tool, so you'll have to do this by slcand or
> slcan_attach invocation through the -sX parameter:
>
> - slcan_attach -f -s6 -o /dev/ttyACM0
> - slcand -f -s8 -o /dev/ttyUSB0
>
> where -s6 in will set adapter's bitrate to 500 Kbit/s and -s8 to
> 1Mbit/s.
> See the table below for further CAN bitrates:
> - s0 -> 10 Kbit/s
> - s1 -> 20 Kbit/s
> - s2 -> 50 Kbit/s
> - s3 -> 100 Kbit/s
> - s4 -> 125 Kbit/s
> - s5 -> 250 Kbit/s
> - s6 -> 500 Kbit/s
> - s7 -> 800 Kbit/s
> - s8 -> 1000 Kbit/s
>
> In doing so, the struct can_priv::bittiming.bitrate of the driver is not
> set and since the open_candev() checks that the bitrate has been set, it
> must be a non-zero value, the bitrate is set to a fake value (-1U)
> before it is called.
The patch description doesn't mention the change of the locking from
rtnl to a spin_lock. Please test your code with a kernel that has
CONFIG_PROVE_LOCKING enabled.
Please move patch
| [PATCH v3 05/13] can: netlink: dump bitrate 0 if can_priv::bittiming.bitrate is -1U
in front of this one.
> @@ -374,7 +375,7 @@ static netdev_tx_t slc_xmit(struct sk_buff *skb, struct net_device *dev)
> spin_unlock(&sl->lock);
>
> out:
> - kfree_skb(skb);
> + can_put_echo_skb(skb, dev, 0, 0);
Where's the corresponding can_get_echo_skb()?
If you convert the driver to can_put_echo_skb()/can_get_echo_skb(), you
must set "netdev->flags |= IFF_ECHO;". And you should do the put()
before triggering the send, the corresponding get() needs to be added
where you have the TX completion from the hardware. If you don't get a
response the best you can do is moving it after the triggering of the
send.
If you want to convert the driver to can_put/get_echo_skb(), please make
it a separate patch.
> return NETDEV_TX_OK;
> }
>
> @@ -394,6 +395,8 @@ static int slc_close(struct net_device *dev)
> clear_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags);
> }
> netif_stop_queue(dev);
> + close_candev(dev);
> + sl->can.state = CAN_STATE_STOPPED;
> sl->rcount = 0;
> sl->xleft = 0;
> spin_unlock_bh(&sl->lock);
> @@ -405,21 +408,36 @@ static int slc_close(struct net_device *dev)
> static int slc_open(struct net_device *dev)
> {
> struct slcan *sl = netdev_priv(dev);
> + int err;
>
> if (sl->tty == NULL)
> return -ENODEV;
>
> + /* The baud rate is not set with the command
> + * `ip link set <iface> type can bitrate <baud>' and therefore
> + * can.bittiming.bitrate is 0, causing open_candev() to fail.
> + * So let's set to a fake value.
> + */
> + sl->can.bittiming.bitrate = -1;
> + err = open_candev(dev);
> + if (err) {
> + netdev_err(dev, "failed to open can device\n");
> + return err;
> + }
> +
> + sl->can.state = CAN_STATE_ERROR_ACTIVE;
> sl->flags &= BIT(SLF_INUSE);
> netif_start_queue(dev);
> return 0;
> }
>
> -/* Hook the destructor so we can free slcan devs at the right point in time */
> -static void slc_free_netdev(struct net_device *dev)
> +static void slc_dealloc(struct slcan *sl)
> {
> - int i = dev->base_addr;
> + int i = sl->dev->base_addr;
>
> - slcan_devs[i] = NULL;
> + free_candev(sl->dev);
> + if (slcan_devs)
Why have you added this check?
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 |