Date:   Sun, 10 Jul 2022 17:38:40 +0200
From:   Dario Binacchi <dario.binacchi@...rulasolutions.com>
To:     Marc Kleine-Budde <mkl@...gutronix.de>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        linux-can@...r.kernel.org, kernel@...gutronix.de,
        Jeroen Hofstee <jhofstee@...tronenergy.com>
Subject: Re: [PATCH net-next 08/15] can: slcan: use CAN network device driver API

Hi Marc,

On Sun, Jul 3, 2022 at 12:26 PM Marc Kleine-Budde <mkl@...gutronix.de> wrote:
>
> From: Dario Binacchi <dario.binacchi@...rulasolutions.com>
>
> 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.
>
> Using the rtnl_lock()/rtnl_unlock() functions has become a bit more
> tricky as the register_candev() function indirectly calls rtnl_lock()
> via register_netdev(). To avoid a deadlock it is therefore necessary to
> call rtnl_unlock() before calling register_candev(). The same goes for
> the unregister_candev() function.
>
> [1] commit 39549eef3587f ("can: CAN Network device driver and Netlink interface")
>
> Link: https://lore.kernel.org/all/20220628163137.413025-6-dario.binacchi@amarulasolutions.com
> Signed-off-by: Dario Binacchi <dario.binacchi@...rulasolutions.com>
> Tested-by: Jeroen Hofstee <jhofstee@...tronenergy.com>
> Signed-off-by: Marc Kleine-Budde <mkl@...gutronix.de>
> ---
>  drivers/net/can/Kconfig | 40 ++++++++++----------
>  drivers/net/can/slcan.c | 82 ++++++++++++++++++++---------------------
>  2 files changed, 60 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 4078d0775572..3048ad77edb3 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -49,26 +49,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:
> -         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 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_NETLINK
>         bool "CAN device drivers with Netlink support"
>         default y
> @@ -172,6 +152,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:
> +         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 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..bf84698f1a81 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. */
> @@ -394,6 +394,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);

Maybe better to move the spin_unlock_bh() before calling close_candev()?

Thanks and regards,
Dario

> @@ -405,20 +407,34 @@ 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 CAN_BITRATE_UNSET (0), causing
> +        * open_candev() to fail. So let's set to a fake value.
> +        */
> +       sl->can.bittiming.bitrate = CAN_BITRATE_UNKNOWN;
> +       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;
>
> +       free_candev(sl->dev);
>         slcan_devs[i] = NULL;
>  }
>
> @@ -434,24 +450,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 +512,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 +526,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;
> @@ -605,26 +598,28 @@ static int slcan_open(struct tty_struct *tty)
>
>                 set_bit(SLF_INUSE, &sl->flags);
>
> -               err = register_netdevice(sl->dev);
> -               if (err)
> +               rtnl_unlock();
> +               err = register_candev(sl->dev);
> +               if (err) {
> +                       pr_err("slcan: can't register candev\n");
>                         goto err_free_chan;
> +               }
> +       } else {
> +               rtnl_unlock();
>         }
>
> -       /* Done.  We have linked the TTY line to a channel. */
> -       rtnl_unlock();
>         tty->receive_room = 65536;      /* We don't flow control */
>
>         /* 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_free_netdev(sl->dev);
> -       /* do not call free_netdev before rtnl_unlock */
> +       slc_dealloc(sl);
>         rtnl_unlock();
> -       free_netdev(sl->dev);
>         return err;
>
>  err_exit:
> @@ -658,9 +653,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);
> +       rtnl_lock();
> +       slc_dealloc(sl);
> +       rtnl_unlock();
>  }
>
>  static void slcan_hangup(struct tty_struct *tty)
> @@ -768,14 +765,15 @@ 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");
>                 }
>
> -               unregister_netdev(dev);
> +               slc_close(dev);
> +               unregister_candev(dev);
> +               slc_dealloc(sl);
>         }
>
>         kfree(slcan_devs);
> --
> 2.35.1
>
>


-- 

Dario Binacchi

Embedded Linux Developer

dario.binacchi@...rulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

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

www.amarulasolutions.com

Powered by blists - more mailing lists