This series originated as a result of CAN communication tests for an
application using the USBtin adapter (https://www.fischl.de/usbtin/).
The tests showed some errors but for the driver everything was ok.
Also, being the first time I used the slcan driver, I was amazed that
it was not possible to configure the bitrate via the ip tool.
For these two reasons, I started looking at the driver code and realized
that it didn't use the CAN network device driver interface.
Starting from these assumptions, I tried to:
- Use the CAN network device driver interface.
- Set the bitrate via the ip tool.
- Send the open/close command to the adapter from the driver.
- Add ethtool support to reset the adapter errors.
- Extend the protocol to forward the adapter CAN communication
errors and the CAN state changes to the netdev upper layers.
Except for the protocol extension patches (i. e. forward the adapter CAN
communication errors and the CAN state changes to the netdev upper
layers), the whole series has been tested under QEMU with Linux 4.19.208
using the USBtin adapter.
Testing the extension protocol patches requires updating the adapter
firmware. Before modifying the firmware I think it makes sense to know if
these extensions can be considered useful.
Before applying the series I used these commands:
slcan_attach -f -s6 -o /dev/ttyACM0
slcand ttyACM0 can0
ip link set can0 up
After applying the series I am using these commands:
slcan_attach /dev/ttyACM0
slcand ttyACM0 can0
ip link set dev can0 down
ip link set can0 type can bitrate 500000
ethtool --set-priv-flags can0 err-rst-on-open on
ip link set dev can0 up
Now there is a clearer separation between serial line and CAN,
but above all, it is possible to use the ip and ethtool commands
as it happens for any CAN device driver. The changes are backward
compatible, you can continue to use the slcand and slcan_attach
command options.
Changes in v3:
- Increment the error counter in case of decoding failure.
- Replace (-1) with (-1U) in the commit description.
- Update the commit description.
- Remove the slc_do_set_bittiming().
- Set the bitrate in the ndo_open().
- Replace -1UL with -1U in setting a fake value for the bitrate.
- Drop the patch "can: slcan: simplify the device de-allocation".
- Add the patch "can: netlink: dump bitrate 0 if can_priv::bittiming.bitrate is -1U".
Changes in v2:
- Put the data into the allocated skb directly instead of first
filling the "cf" on the stack and then doing a memcpy().
- Move CAN_SLCAN Kconfig option inside CAN_DEV scope.
- Improve the commit message.
- Use the CAN framework support for setting fixed bit rates.
- Improve the commit message.
- Improve the commit message.
- Protect decoding against the case the len value is longer than the
received data.
- Continue error handling even if no skb can be allocated.
- Continue error handling even if no skb can be allocated.
Dario Binacchi (13):
can: slcan: use the BIT() helper
can: slcan: use netdev helpers to print out messages
can: slcan: use the alloc_can_skb() helper
can: slcan: use CAN network device driver API
can: netlink: dump bitrate 0 if can_priv::bittiming.bitrate is -1U
can: slcan: allow to send commands to the adapter
can: slcan: set bitrate by CAN device driver API
can: slcan: send the open command to the adapter
can: slcan: send the close command to the adapter
can: slcan: move driver into separate sub directory
can: slcan: add ethtool support to reset adapter errors
can: slcan: extend the protocol with error info
can: slcan: extend the protocol with CAN state info
drivers/net/can/Kconfig | 40 +-
drivers/net/can/Makefile | 2 +-
drivers/net/can/dev/netlink.c | 12 +-
drivers/net/can/slcan/Makefile | 7 +
.../net/can/{slcan.c => slcan/slcan-core.c} | 518 ++++++++++++++----
drivers/net/can/slcan/slcan-ethtool.c | 65 +++
drivers/net/can/slcan/slcan.h | 18 +
7 files changed, 541 insertions(+), 121 deletions(-)
create mode 100644 drivers/net/can/slcan/Makefile
rename drivers/net/can/{slcan.c => slcan/slcan-core.c} (65%)
create mode 100644 drivers/net/can/slcan/slcan-ethtool.c
create mode 100644 drivers/net/can/slcan/slcan.h
--
2.32.0
It extends the protocol to receive the adapter CAN state changes
(warning, busoff, etc.) and forward them to the netdev upper levels.
Signed-off-by: Dario Binacchi <[email protected]>
---
Changes in v3:
- Drop the patch "can: slcan: simplify the device de-allocation".
- Add the patch "can: netlink: dump bitrate 0 if can_priv::bittiming.bitrate is -1U".
Changes in v2:
- Continue error handling even if no skb can be allocated.
drivers/net/can/slcan/slcan-core.c | 66 ++++++++++++++++++++++++++++++
1 file changed, 66 insertions(+)
diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
index 48077edb9497..5ba1c141f942 100644
--- a/drivers/net/can/slcan/slcan-core.c
+++ b/drivers/net/can/slcan/slcan-core.c
@@ -78,6 +78,9 @@ MODULE_PARM_DESC(maxdev, "Maximum number of slcan interfaces");
#define SLC_CMD_LEN 1
#define SLC_SFF_ID_LEN 3
#define SLC_EFF_ID_LEN 8
+#define SLC_STATE_LEN 1
+#define SLC_STATE_BE_RXCNT_LEN 3
+#define SLC_STATE_BE_TXCNT_LEN 3
struct slcan {
struct can_priv can;
@@ -175,6 +178,67 @@ int slcan_enable_err_rst_on_open(struct net_device *ndev, bool on)
* STANDARD SLCAN DECAPSULATION *
************************************************************************/
+static void slc_bump_state(struct slcan *sl)
+{
+ struct net_device *dev = sl->dev;
+ struct sk_buff *skb;
+ struct can_frame *cf;
+ char *cmd = sl->rbuff;
+ u32 rxerr, txerr;
+ enum can_state state, rx_state, tx_state;
+
+ if (*cmd != 's')
+ return;
+
+ cmd += SLC_CMD_LEN;
+ switch (*cmd) {
+ case 'a':
+ state = CAN_STATE_ERROR_ACTIVE;
+ break;
+ case 'w':
+ state = CAN_STATE_ERROR_WARNING;
+ break;
+ case 'p':
+ state = CAN_STATE_ERROR_PASSIVE;
+ break;
+ case 'f':
+ state = CAN_STATE_BUS_OFF;
+ break;
+ default:
+ return;
+ }
+
+ if (state == sl->can.state)
+ return;
+
+ cmd += SLC_STATE_BE_RXCNT_LEN + 1;
+ cmd[SLC_STATE_BE_TXCNT_LEN] = 0;
+ if (kstrtou32(cmd, 10, &txerr))
+ return;
+
+ *cmd = 0;
+ cmd -= SLC_STATE_BE_RXCNT_LEN;
+ if (kstrtou32(cmd, 10, &rxerr))
+ return;
+
+ skb = alloc_can_err_skb(dev, &cf);
+
+ if (skb) {
+ cf->data[6] = txerr;
+ cf->data[7] = rxerr;
+ }
+
+ tx_state = txerr >= rxerr ? state : 0;
+ rx_state = txerr <= rxerr ? state : 0;
+ can_change_state(dev, skb ? cf : NULL, tx_state, rx_state);
+
+ if (state == CAN_STATE_BUS_OFF)
+ can_bus_off(dev);
+
+ if (skb)
+ netif_rx(skb);
+}
+
static void slc_bump_err(struct slcan *sl)
{
struct net_device *dev = sl->dev;
@@ -378,6 +442,8 @@ static void slc_bump(struct slcan *sl)
return slc_bump_frame(sl);
case 'e':
return slc_bump_err(sl);
+ case 's':
+ return slc_bump_state(sl);
default:
return;
}
--
2.32.0
On 12.06.2022 23:39:27, Dario Binacchi wrote:
> It extends the protocol to receive the adapter CAN state changes
> (warning, busoff, etc.) and forward them to the netdev upper levels.
>
> Signed-off-by: Dario Binacchi <[email protected]>
>
> ---
>
> Changes in v3:
> - Drop the patch "can: slcan: simplify the device de-allocation".
> - Add the patch "can: netlink: dump bitrate 0 if can_priv::bittiming.bitrate is -1U".
>
> Changes in v2:
> - Continue error handling even if no skb can be allocated.
>
> drivers/net/can/slcan/slcan-core.c | 66 ++++++++++++++++++++++++++++++
> 1 file changed, 66 insertions(+)
>
> diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
> index 48077edb9497..5ba1c141f942 100644
> --- a/drivers/net/can/slcan/slcan-core.c
> +++ b/drivers/net/can/slcan/slcan-core.c
> @@ -78,6 +78,9 @@ MODULE_PARM_DESC(maxdev, "Maximum number of slcan interfaces");
> #define SLC_CMD_LEN 1
> #define SLC_SFF_ID_LEN 3
> #define SLC_EFF_ID_LEN 8
> +#define SLC_STATE_LEN 1
> +#define SLC_STATE_BE_RXCNT_LEN 3
> +#define SLC_STATE_BE_TXCNT_LEN 3
>
> struct slcan {
> struct can_priv can;
> @@ -175,6 +178,67 @@ int slcan_enable_err_rst_on_open(struct net_device *ndev, bool on)
> * STANDARD SLCAN DECAPSULATION *
> ************************************************************************/
>
> +static void slc_bump_state(struct slcan *sl)
> +{
> + struct net_device *dev = sl->dev;
> + struct sk_buff *skb;
> + struct can_frame *cf;
> + char *cmd = sl->rbuff;
> + u32 rxerr, txerr;
> + enum can_state state, rx_state, tx_state;
> +
> + if (*cmd != 's')
> + return;
Checked by the caller?
> +
> + cmd += SLC_CMD_LEN;
> + switch (*cmd) {
> + case 'a':
> + state = CAN_STATE_ERROR_ACTIVE;
> + break;
> + case 'w':
> + state = CAN_STATE_ERROR_WARNING;
> + break;
> + case 'p':
> + state = CAN_STATE_ERROR_PASSIVE;
> + break;
> + case 'f':
> + state = CAN_STATE_BUS_OFF;
> + break;
> + default:
> + return;
> + }
> +
> + if (state == sl->can.state)
> + return;
> +
> + cmd += SLC_STATE_BE_RXCNT_LEN + 1;
Have you checked that you have received that much data?
> + cmd[SLC_STATE_BE_TXCNT_LEN] = 0;
> + if (kstrtou32(cmd, 10, &txerr))
> + return;
> +
> + *cmd = 0;
> + cmd -= SLC_STATE_BE_RXCNT_LEN;
> + if (kstrtou32(cmd, 10, &rxerr))
> + return;
Why do you parse TX first and then RX?
> +
> + skb = alloc_can_err_skb(dev, &cf);
> +
> + if (skb) {
> + cf->data[6] = txerr;
> + cf->data[7] = rxerr;
> + }
> +
> + tx_state = txerr >= rxerr ? state : 0;
> + rx_state = txerr <= rxerr ? state : 0;
> + can_change_state(dev, skb ? cf : NULL, tx_state, rx_state);
alloc_can_err_skb() set cf to NULL if no skb can be allocated.
> +
> + if (state == CAN_STATE_BUS_OFF)
> + can_bus_off(dev);
> +
> + if (skb)
> + netif_rx(skb);
> +}
> +
> static void slc_bump_err(struct slcan *sl)
> {
> struct net_device *dev = sl->dev;
> @@ -378,6 +442,8 @@ static void slc_bump(struct slcan *sl)
> return slc_bump_frame(sl);
> case 'e':
> return slc_bump_err(sl);
> + case 's':
> + return slc_bump_state(sl);
> default:
> return;
> }
> --
> 2.32.0
>
>
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 Mon, Jun 13, 2022 at 9:37 AM Marc Kleine-Budde <[email protected]> wrote:
>
> On 12.06.2022 23:39:27, Dario Binacchi wrote:
> > It extends the protocol to receive the adapter CAN state changes
> > (warning, busoff, etc.) and forward them to the netdev upper levels.
> >
> > Signed-off-by: Dario Binacchi <[email protected]>
> >
> > ---
> >
> > Changes in v3:
> > - Drop the patch "can: slcan: simplify the device de-allocation".
> > - Add the patch "can: netlink: dump bitrate 0 if can_priv::bittiming.bitrate is -1U".
> >
> > Changes in v2:
> > - Continue error handling even if no skb can be allocated.
> >
> > drivers/net/can/slcan/slcan-core.c | 66 ++++++++++++++++++++++++++++++
> > 1 file changed, 66 insertions(+)
> >
> > diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
> > index 48077edb9497..5ba1c141f942 100644
> > --- a/drivers/net/can/slcan/slcan-core.c
> > +++ b/drivers/net/can/slcan/slcan-core.c
> > @@ -78,6 +78,9 @@ MODULE_PARM_DESC(maxdev, "Maximum number of slcan interfaces");
> > #define SLC_CMD_LEN 1
> > #define SLC_SFF_ID_LEN 3
> > #define SLC_EFF_ID_LEN 8
> > +#define SLC_STATE_LEN 1
> > +#define SLC_STATE_BE_RXCNT_LEN 3
> > +#define SLC_STATE_BE_TXCNT_LEN 3
> >
> > struct slcan {
> > struct can_priv can;
> > @@ -175,6 +178,67 @@ int slcan_enable_err_rst_on_open(struct net_device *ndev, bool on)
> > * STANDARD SLCAN DECAPSULATION *
> > ************************************************************************/
> >
> > +static void slc_bump_state(struct slcan *sl)
> > +{
> > + struct net_device *dev = sl->dev;
> > + struct sk_buff *skb;
> > + struct can_frame *cf;
> > + char *cmd = sl->rbuff;
> > + u32 rxerr, txerr;
> > + enum can_state state, rx_state, tx_state;
> > +
> > + if (*cmd != 's')
> > + return;
>
> Checked by the caller?
>
> > +
> > + cmd += SLC_CMD_LEN;
> > + switch (*cmd) {
> > + case 'a':
> > + state = CAN_STATE_ERROR_ACTIVE;
> > + break;
> > + case 'w':
> > + state = CAN_STATE_ERROR_WARNING;
> > + break;
> > + case 'p':
> > + state = CAN_STATE_ERROR_PASSIVE;
> > + break;
> > + case 'f':
> > + state = CAN_STATE_BUS_OFF;
> > + break;
> > + default:
> > + return;
> > + }
> > +
> > + if (state == sl->can.state)
> > + return;
> > +
> > + cmd += SLC_STATE_BE_RXCNT_LEN + 1;
>
> Have you checked that you have received that much data?
>
> > + cmd[SLC_STATE_BE_TXCNT_LEN] = 0;
> > + if (kstrtou32(cmd, 10, &txerr))
> > + return;
> > +
> > + *cmd = 0;
> > + cmd -= SLC_STATE_BE_RXCNT_LEN;
> > + if (kstrtou32(cmd, 10, &rxerr))
> > + return;
>
> Why do you parse TX first and then RX?
Since adding the end-of-string character to the counter to be decoded
invalidates the next one.
If I had started from the rx counter, I would have found the
transmission counter always at 0.
Thanks and regards,
Dario
>
> > +
> > + skb = alloc_can_err_skb(dev, &cf);
> > +
> > + if (skb) {
> > + cf->data[6] = txerr;
> > + cf->data[7] = rxerr;
> > + }
> > +
> > + tx_state = txerr >= rxerr ? state : 0;
> > + rx_state = txerr <= rxerr ? state : 0;
> > + can_change_state(dev, skb ? cf : NULL, tx_state, rx_state);
>
> alloc_can_err_skb() set cf to NULL if no skb can be allocated.
>
> > +
> > + if (state == CAN_STATE_BUS_OFF)
> > + can_bus_off(dev);
> > +
> > + if (skb)
> > + netif_rx(skb);
> > +}
> > +
> > static void slc_bump_err(struct slcan *sl)
> > {
> > struct net_device *dev = sl->dev;
> > @@ -378,6 +442,8 @@ static void slc_bump(struct slcan *sl)
> > return slc_bump_frame(sl);
> > case 'e':
> > return slc_bump_err(sl);
> > + case 's':
> > + return slc_bump_state(sl);
> > default:
> > return;
> > }
> > --
> > 2.32.0
> >
> >
>
> 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 |
--
Dario Binacchi
Embedded Linux Developer
[email protected]
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
[email protected]
http://www.amarulasolutions.com
On 14.06.2022 08:29:57, Dario Binacchi wrote:
> > > + cmd[SLC_STATE_BE_TXCNT_LEN] = 0;
> > > + if (kstrtou32(cmd, 10, &txerr))
> > > + return;
> > > +
> > > + *cmd = 0;
> > > + cmd -= SLC_STATE_BE_RXCNT_LEN;
> > > + if (kstrtou32(cmd, 10, &rxerr))
> > > + return;
> >
> > Why do you parse TX first and then RX?
>
> Since adding the end-of-string character to the counter to be decoded
> invalidates the next one.
> If I had started from the rx counter, I would have found the
> transmission counter always at 0.
Thanks for the explanation.
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 |