| Message ID | 20220607094752.1029295-7-dario.binacchi@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On 07.06.2022 11:47:45, Dario Binacchi wrote: > This is a preparation patch for the upcoming support to change the > bitrate via ip tool, reset the adapter error states via the ethtool API > and, more generally, send commands to the adapter. > > Since some commands (e. g. setting the bitrate) will be sent before > calling the open_candev(), the netif_running() will return false and so > a new flag bit (i. e. SLF_XCMD) for serial transmission has to be added. > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> I think this patch can be dropped, let me explain: You don't have to implement the do_set_bittiming callback. It's perfectly OK to set the bitrate during the ndo_open callback after open_candev(). regards, Marc
Hi Marc, On Thu, Jun 9, 2022 at 9:16 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > On 07.06.2022 11:47:45, Dario Binacchi wrote: > > This is a preparation patch for the upcoming support to change the > > bitrate via ip tool, reset the adapter error states via the ethtool API > > and, more generally, send commands to the adapter. > > > > Since some commands (e. g. setting the bitrate) will be sent before > > calling the open_candev(), the netif_running() will return false and so > > a new flag bit (i. e. SLF_XCMD) for serial transmission has to be added. > > > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> > > I think this patch can be dropped, let me explain: > > You don't have to implement the do_set_bittiming callback. It's > perfectly OK to set the bitrate during the ndo_open callback after > open_candev(). I developed what you suggested (i. e. remove the SLF_XCMD bit and set the bitrate, as well as send the "F\r" and "O\r" commands, after calling the open_candev(), but now I can't send the close command ("C\r") during the ndo_stop() since netif_running() returns false. The CAN framework clears netif_running() before calling the ndo_stop(). IMHO the SLF_XCMD bit then becomes necessary to transmit the commands to the adapter from the ndo_open() and ndo_stop() routines. Any suggestions ? Thanks and regards, Dario > > 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 |
On 11.06.2022 23:43:47, Dario Binacchi wrote: > Hi Marc, > > On Thu, Jun 9, 2022 at 9:16 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > > > On 07.06.2022 11:47:45, Dario Binacchi wrote: > > > This is a preparation patch for the upcoming support to change the > > > bitrate via ip tool, reset the adapter error states via the ethtool API > > > and, more generally, send commands to the adapter. > > > > > > Since some commands (e. g. setting the bitrate) will be sent before > > > calling the open_candev(), the netif_running() will return false and so > > > a new flag bit (i. e. SLF_XCMD) for serial transmission has to be added. > > > > > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> > > > > I think this patch can be dropped, let me explain: > > > > You don't have to implement the do_set_bittiming callback. It's > > perfectly OK to set the bitrate during the ndo_open callback after > > open_candev(). > > I developed what you suggested (i. e. remove the SLF_XCMD bit and set the > bitrate, as well as send the "F\r" and "O\r" commands, after calling > the open_candev(), Note: Max Staudt noticed that you need a do_set_bittiming() callback if you have only fixed bitrates. I've create a patch that you can have fixed bit rate only an no do_set_bittiming() callback. | https://lore.kernel.org/all/20220611144248.3924903-1-mkl@pengutronix.de/ Or use the can-next/master branch as your base. > but now I can't send the close command ("C\r") during the ndo_stop() since > netif_running() returns false. The CAN framework clears netif_running() before > calling the ndo_stop(). IMHO the SLF_XCMD bit then becomes necessary to > transmit the commands to the adapter from the ndo_open() and > ndo_stop() routines. > Any suggestions ? I see. Keep the setting of the bit rate in the ndo_open(), add the SLF_XCMD so that you can send messages in ndo_close(). Marc
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c index 4df0455e11a2..dbd4ebdfa024 100644 --- a/drivers/net/can/slcan.c +++ b/drivers/net/can/slcan.c @@ -97,6 +97,9 @@ struct slcan { 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 */ + wait_queue_head_t xcmd_wait; /* Wait queue for commands */ + /* transmission */ }; static struct net_device **slcan_devs; @@ -310,12 +313,22 @@ 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 || !netif_running(sl->dev)) { + if (!sl->tty || sl->magic != SLCAN_MAGIC || + (unlikely(!netif_running(sl->dev)) && + likely(!test_bit(SLF_XCMD, &sl->flags)))) { spin_unlock_bh(&sl->lock); return; } if (sl->xleft <= 0) { + if (unlikely(test_bit(SLF_XCMD, &sl->flags))) { + clear_bit(SLF_XCMD, &sl->flags); + clear_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags); + spin_unlock_bh(&sl->lock); + wake_up(&sl->xcmd_wait); + return; + } + /* Now serial buffer is almost free & we can start * transmission of another packet */ sl->dev->stats.tx_packets++; @@ -379,6 +392,36 @@ static netdev_tx_t slc_xmit(struct sk_buff *skb, struct net_device *dev) * Routines looking at netdevice side. ******************************************/ +static int slcan_transmit_cmd(struct slcan *sl, const unsigned char *cmd) +{ + int ret, actual, n; + + spin_lock(&sl->lock); + if (sl->tty == NULL) { + spin_unlock(&sl->lock); + return -ENODEV; + } + + n = snprintf(sl->xbuff, sizeof(sl->xbuff), "%s", cmd); + set_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags); + actual = sl->tty->ops->write(sl->tty, sl->xbuff, n); + sl->xleft = n - actual; + sl->xhead = sl->xbuff + actual; + set_bit(SLF_XCMD, &sl->flags); + spin_unlock(&sl->lock); + ret = wait_event_interruptible_timeout(sl->xcmd_wait, + !test_bit(SLF_XCMD, &sl->flags), + HZ); + clear_bit(SLF_XCMD, &sl->flags); + if (ret == -ERESTARTSYS) + return ret; + + if (ret == 0) + return -ETIMEDOUT; + + return 0; +} + /* Netdevice UP -> DOWN routine */ static int slc_close(struct net_device *dev) { @@ -542,6 +585,7 @@ static struct slcan *slc_alloc(void) sl->dev = dev; spin_lock_init(&sl->lock); INIT_WORK(&sl->tx_work, slcan_transmit); + init_waitqueue_head(&sl->xcmd_wait); slcan_devs[i] = dev; return sl;
This is a preparation patch for the upcoming support to change the bitrate via ip tool, reset the adapter error states via the ethtool API and, more generally, send commands to the adapter. Since some commands (e. g. setting the bitrate) will be sent before calling the open_candev(), the netif_running() will return false and so a new flag bit (i. e. SLF_XCMD) for serial transmission has to be added. Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> --- drivers/net/can/slcan.c | 46 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-)