[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20220613070105.wfvjfyyoozyhlqgw@pengutronix.de>
Date: Mon, 13 Jun 2022 09:01:05 +0200
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Dario Binacchi <dario.binacchi@...rulasolutions.com>
Cc: linux-kernel@...r.kernel.org, michael@...rulasolutions.com,
Amarula patchwork <linux-amarula@...rulasolutions.com>,
Oliver Hartkopp <socketcan@...tkopp.net>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Wolfgang Grandegger <wg@...ndegger.com>,
linux-can@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v3 04/13] can: slcan: use CAN network device driver API
On 12.06.2022 23:39:18, Dario Binacchi wrote:
> 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.
The patch description doesn't mention the change of the locking from
rtnl to a spin_lock. Please test your code with a kernel that has
CONFIG_PROVE_LOCKING enabled.
Please move patch
| [PATCH v3 05/13] can: netlink: dump bitrate 0 if can_priv::bittiming.bitrate is -1U
in front of this one.
> @@ -374,7 +375,7 @@ static netdev_tx_t slc_xmit(struct sk_buff *skb, struct net_device *dev)
> spin_unlock(&sl->lock);
>
> out:
> - kfree_skb(skb);
> + can_put_echo_skb(skb, dev, 0, 0);
Where's the corresponding can_get_echo_skb()?
If you convert the driver to can_put_echo_skb()/can_get_echo_skb(), you
must set "netdev->flags |= IFF_ECHO;". And you should do the put()
before triggering the send, the corresponding get() needs to be added
where you have the TX completion from the hardware. If you don't get a
response the best you can do is moving it after the triggering of the
send.
If you want to convert the driver to can_put/get_echo_skb(), please make
it a separate patch.
> return NETDEV_TX_OK;
> }
>
> @@ -394,6 +395,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);
> @@ -405,21 +408,36 @@ 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 0, causing open_candev() to fail.
> + * So let's set to a fake value.
> + */
> + sl->can.bittiming.bitrate = -1;
> + 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;
>
> - slcan_devs[i] = NULL;
> + free_candev(sl->dev);
> + if (slcan_devs)
Why have you added this check?
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 |
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists