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@xxxxxxxxxxxxxxxxxxxx> 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.
