lkml.org 
[lkml]   [2022]   [Jul]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [RFC PATCH 2/5] can: slcan: remove legacy infrastructure
    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



    --

    Dario Binacchi

    Embedded Linux Developer

    dario.binacchi@amarulasolutions.com

    __________________________________


    Amarula Solutions SRL

    Via Le Canevare 30, 31100 Treviso, Veneto, IT

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

    www.amarulasolutions.com

    \
     
     \ /
      Last update: 2022-07-25 08:41    [W:3.665 / U:0.068 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site