| Message ID | 20220716170007.2020037-3-dario.binacchi@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Dario, This looks good, thank you for continuing to look after slcan! A few comments below. On Sat, 16 Jul 2022 19:00:04 +0200 Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote: [...] > @@ -68,7 +62,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 */ > @@ -84,17 +77,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 */ I assume xcmd_wait() came in as part of the previous patch series? [...] > /* Send a can_frame to a TTY queue. */ > @@ -652,25 +637,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; I suggest moving these two assignments to slc_open() - see below. [...] > @@ -883,72 +786,50 @@ 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; I suggest moving the zeroing to slc_open() - i.e. to the netdev open function. As a bonus, you can then remove the same two assignments from slc_close() (see above). They are only used when netif_running(), with appropiate guards already in place as far as I can see. > + 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; > + strscpy(dev->name, "slcan%d", sizeof(dev->name)); The third parameter looks... unintentional :) What do the maintainers think of dropping the old "slcan" name, and just allowing this to be a normal canX device? These patches do bring it closer to that, after all. In this case, this name string magic could be dropped altogether. [...] This looks good to me overall. Thanks Dario! Max
On 17.07.22 23:38, Max Staudt wrote: > Hi Dario, > > This looks good, thank you for continuing to look after slcan! > > A few comments below. > > > > On Sat, 16 Jul 2022 19:00:04 +0200 > Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote: > > [...] > > >> @@ -68,7 +62,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 */ >> @@ -84,17 +77,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 */ > > I assume xcmd_wait() came in as part of the previous patch series? > > > [...] > > >> /* Send a can_frame to a TTY queue. */ >> @@ -652,25 +637,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; > > I suggest moving these two assignments to slc_open() - see below. > > > [...] > > >> @@ -883,72 +786,50 @@ 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; > > I suggest moving the zeroing to slc_open() - i.e. to the netdev open > function. As a bonus, you can then remove the same two assignments from > slc_close() (see above). They are only used when netif_running(), with > appropiate guards already in place as far as I can see. > > >> + 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; >> + strscpy(dev->name, "slcan%d", sizeof(dev->name)); > > The third parameter looks... unintentional :) > > What do the maintainers think of dropping the old "slcan" name, and > just allowing this to be a normal canX device? These patches do bring > it closer to that, after all. In this case, this name string magic > could be dropped altogether. > I'm fine with it in general. But we have to take into account that there might be existing setups that still might use the slcan_attach or slcand mechanic which will likely break after the kernel update. But in the end the slcan0 shows up everywhere - even in log files, etc. So we really should name it canX. When people really get in trouble with it, they can rename the network interface name with the 'ip' tool ... Best regards, Oliver > > [...] > > > > This looks good to me overall. > > Thanks Dario! > > > Max
On 18.07.2022 08:57:21, Oliver Hartkopp wrote: > > What do the maintainers think of dropping the old "slcan" name, and > > just allowing this to be a normal canX device? These patches do bring > > it closer to that, after all. In this case, this name string magic > > could be dropped altogether. > > > > I'm fine with it in general. But we have to take into account that there > might be existing setups that still might use the slcan_attach or slcand > mechanic which will likely break after the kernel update. > > But in the end the slcan0 shows up everywhere - even in log files, etc. > > So we really should name it canX. When people really get in trouble with it, > they can rename the network interface name with the 'ip' tool ... Don't break user space! If you don't like slcanX use udev to give it a proper name. regards, Marc
On 7/18/22 12:15, Marc Kleine-Budde wrote: > On 18.07.2022 08:57:21, Oliver Hartkopp wrote: >>> What do the maintainers think of dropping the old "slcan" name, and >>> just allowing this to be a normal canX device? These patches do bring >>> it closer to that, after all. In this case, this name string magic >>> could be dropped altogether. >>> >> >> I'm fine with it in general. But we have to take into account that there >> might be existing setups that still might use the slcan_attach or slcand >> mechanic which will likely break after the kernel update. >> >> But in the end the slcan0 shows up everywhere - even in log files, etc. >> >> So we really should name it canX. When people really get in trouble with it, >> they can rename the network interface name with the 'ip' tool ... > > Don't break user space! If you don't like slcanX use udev to give it a > proper name. Ok. Fine with me too. IMO it does not break user space when slcan gets the common naming schema for CAN interface names. We had the same thing with 'eth0' which is now named enblablabla or 'wlan0' now named wlp2s0. But I have no strong opinion on that naming ... Best regards, Oliver
On Mon, 18 Jul 2022 12:23:05 +0200 Oliver Hartkopp <socketcan@hartkopp.net> wrote: > IMO it does not break user space when slcan gets the common naming > schema for CAN interface names. For what it's worth, slcan provides a specific ioctl to resolve from (tty fd) to (netdev name). As far as I can see, any sane program should use this API to find whatever device it has created, irrespective of the netdev name. @ Marc: slcand and slcan_attach in can-utils do not depend on the slcanX name, as far as I can see. They are the main interfaces to slcan, and apart from that I can only think of scripts doing netlink stuff and scanning for slcanX. Does this change your opinion, or is breaking such scripts already a step too far? I'm thinking that in other circumstances, I've had cases where devices were numbered differently just because they enumerated faster or slower at different boots, or with other kernel versions, or because of media inserted, etc. - though renumbering is of course one step less than changing the prefix. > We had the same thing with 'eth0' which is now named enblablabla or > 'wlan0' now named wlp2s0. Actually, the kernel still calls them ethX and wlanX. It's systemd+udev that renames them to the "unique" names afterwards. It's an extra step that happens in userspace. udev is immensely useful, but also great stuff for race conditions :) Max
On 19.07.2022 03:03:26, Max Staudt wrote: > On Mon, 18 Jul 2022 12:23:05 +0200 > Oliver Hartkopp <socketcan@hartkopp.net> wrote: > > > IMO it does not break user space when slcan gets the common naming > > schema for CAN interface names. > > For what it's worth, slcan provides a specific ioctl to resolve from > (tty fd) to (netdev name). As far as I can see, any sane program should > use this API to find whatever device it has created, irrespective of > the netdev name. > > > @ Marc: slcand and slcan_attach in can-utils do not depend on the > slcanX name, as far as I can see. They are the main interfaces to > slcan, and apart from that I can only think of scripts doing netlink > stuff and scanning for slcanX. Does this change your opinion, or is > breaking such scripts already a step too far? > > I'm thinking that in other circumstances, I've had cases where devices > were numbered differently just because they enumerated faster or slower > at different boots, or with other kernel versions, or because of media > inserted, etc. - though renumbering is of course one step less than > changing the prefix. Convinced! Please change the patch to use standard canX interface naming. Can you add a pointer to the ioctl() to get the network interface name to the tty fd in the patch description. > > We had the same thing with 'eth0' which is now named enblablabla or > > 'wlan0' now named wlp2s0. > > Actually, the kernel still calls them ethX and wlanX. It's systemd+udev > that renames them to the "unique" names afterwards. It's an extra step > that happens in userspace. > > udev is immensely useful, but also great stuff for race conditions :) Marc
Hi Max, First of all thank you for your review, it took me a while to get back to you because I wanted to do some analysis and tests regarding the code you suggested I change and also last week was very busy. On Sun, Jul 17, 2022 at 11:38 PM Max Staudt <max@enpas.org> wrote: > > Hi Dario, > > This looks good, thank you for continuing to look after slcan! > > A few comments below. > > > > On Sat, 16 Jul 2022 19:00:04 +0200 > Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote: > > [...] > > > > @@ -68,7 +62,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 */ > > @@ -84,17 +77,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 */ > > I assume xcmd_wait() came in as part of the previous patch series? > Yes, correct. > > [...] > > > > /* Send a can_frame to a TTY queue. */ > > @@ -652,25 +637,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; > > I suggest moving these two assignments to slc_open() - see below. > > > [...] > > > > @@ -883,72 +786,50 @@ 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; > > I suggest moving the zeroing to slc_open() - i.e. to the netdev open > function. As a bonus, you can then remove the same two assignments from > slc_close() (see above). They are only used when netif_running(), with > appropiate guards already in place as far as I can see. I think it is better to keep the code as it is, since at the entry of the netdev open function, netif_running already returns true (it is set to true by the calling function) and therefore it would be less safe to reset the rcount and xleft fields. Thanks and regards, Dario > > > > + 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; > > + strscpy(dev->name, "slcan%d", sizeof(dev->name)); > > The third parameter looks... unintentional :) > > What do the maintainers think of dropping the old "slcan" name, and > just allowing this to be a normal canX device? These patches do bring > it closer to that, after all. In this case, this name string magic > could be dropped altogether. > > > [...] > > > > This looks good to me overall. > > Thanks Dario! > > > Max
On Mon, 25 Jul 2022 08:40:24 +0200 Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote: > > > @@ -883,72 +786,50 @@ 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; > > > > I suggest moving the zeroing to slc_open() - i.e. to the netdev open > > function. As a bonus, you can then remove the same two assignments from > > slc_close() (see above). They are only used when netif_running(), with > > appropiate guards already in place as far as I can see. > > I think it is better to keep the code as it is, since at the entry of > the netdev > open function, netif_running already returns true (it is set to true by the > calling function) and therefore it would be less safe to reset the > rcount and xleft > fields. Wow, great catch! I wonder why __LINK_STATE_START is set before ->ndo_open() is called...? Since the drivers are similar, I've checked can327. It is unaffected, because the counters are additionally guarded by a spinlock. Same in slcan, where netdev_close() takes the spinlock to reset the counters. So you *could* move them to netdev_open() *if* they are always guarded by the slcan lock. Or, leave it as it is, as it seems to be correct. Your choice :) Thank you! Max
Hi Max, On Mon, Jul 25, 2022 at 3:09 PM Max Staudt <max@enpas.org> wrote: > > On Mon, 25 Jul 2022 08:40:24 +0200 > Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote: > > > > > @@ -883,72 +786,50 @@ 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; > > > > > > I suggest moving the zeroing to slc_open() - i.e. to the netdev open > > > function. As a bonus, you can then remove the same two assignments from > > > slc_close() (see above). They are only used when netif_running(), with > > > appropiate guards already in place as far as I can see. > > > > I think it is better to keep the code as it is, since at the entry of > > the netdev > > open function, netif_running already returns true (it is set to true by the > > calling function) and therefore it would be less safe to reset the > > rcount and xleft > > fields. > > Wow, great catch! > > I wonder why __LINK_STATE_START is set before ->ndo_open() is called...? > > > Since the drivers are similar, I've checked can327. It is unaffected, > because the counters are additionally guarded by a spinlock. Same in > slcan, where netdev_close() takes the spinlock to reset the counters. > > So you *could* move them to netdev_open() *if* they are always guarded > by the slcan lock. > > Or, leave it as it is, as it seems to be correct. Your choice :) If possible I prefer not to use spin_lock. So I prefer to keep the code as is. Thanks and regards, Dario > > > Thank you! > > Max
diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c index 3394d059fc29..92cab093453d 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 @@ -46,15 +49,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) @@ -68,7 +62,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 */ @@ -84,17 +77,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 @@ -538,9 +528,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; } @@ -575,13 +564,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. */ @@ -652,25 +637,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) @@ -686,9 +667,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 @@ -703,8 +681,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]) @@ -748,14 +724,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; @@ -785,7 +753,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 */ @@ -800,80 +768,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), "slcan%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; @@ -883,72 +786,50 @@ 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; + strscpy(dev->name, "slcan%d", sizeof(dev->name)); + 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("slcan: can't register candev\n"); - goto err_free_chan; - } - } else { - rtnl_unlock(); + err = register_candev(dev); + if (err) { + free_candev(dev); + pr_err("slcan: 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. */ @@ -956,28 +837,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. */ @@ -987,10 +860,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; @@ -1012,7 +881,6 @@ static struct tty_ldisc_ops slc_ldisc = { .name = "slcan", .open = slcan_open, .close = slcan_close, - .hangup = slcan_hangup, .ioctl = slcan_ioctl, .receive_buf = slcan_receive_buf, .write_wakeup = slcan_write_wakeup, @@ -1022,78 +890,21 @@ static int __init slcan_init(void) { int status; - if (maxdev < 4) - maxdev = 4; /* Sanity */ - pr_info("slcan: serial line CAN interface driver\n"); - pr_info("slcan: %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("slcan: 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); }
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 `maxdev' module parameter has also been removed. CC: Max Staudt <max@enpas.org> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> --- drivers/net/can/slcan/slcan-core.c | 317 ++++++----------------------- 1 file changed, 64 insertions(+), 253 deletions(-)