[RFC,00/13] can: slcan: extend supported features

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



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: slcan: simplify the device de-allocation
  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/Makefile                      |   2 +-
 drivers/net/can/slcan/Makefile                |   7 +
 .../net/can/{slcan.c => slcan/slcan-core.c}   | 464 +++++++++++++++---
 drivers/net/can/slcan/slcan-ethtool.c         |  65 +++
 drivers/net/can/slcan/slcan.h                 |  18 +
 5 files changed, 480 insertions(+), 76 deletions(-)
 create mode 100644 drivers/net/can/slcan/Makefile
 rename drivers/net/can/{slcan.c => slcan/slcan-core.c} (67%)
 create mode 100644 drivers/net/can/slcan/slcan-ethtool.c
 create mode 100644 drivers/net/can/slcan/slcan.h
  

On Tue. 7 juin 2022 at 18:47, Dario Binacchi
<dario.binacchi@amarulasolutions.com> wrote:
> 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.

That's funny! Yesterday, I sent this comment:
https://lore.kernel.org/linux-can/CAMZ6RqKZwC_OKcgH+WPacY6kbNbj4xR2Gdg2NQtm5Ka5Hfw79A@mail.gmail.com/

And today, you send a full series to remove all the dust from the
slcan driver. Do I have some kind of mystical power to summon people
on the mailing list?

> Starting from these assumptions, I tried to:
> - Use the CAN network device driver interface.

In order to use the CAN network device driver, a.k.a. can-dev module,
drivers/net/can/Kbuild has to be adjusted: move slcan inside CAN_DEV
scope.

@Mark: because I will have to send a new version for my can-dev/Kbuild
cleanup, maybe I can take that change and add it to my series?

> - 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. 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.
>
>
>
> 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: slcan: simplify the device de-allocation
>   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/Makefile                      |   2 +-
>  drivers/net/can/slcan/Makefile                |   7 +
>  .../net/can/{slcan.c => slcan/slcan-core.c}   | 464 +++++++++++++++---
>  drivers/net/can/slcan/slcan-ethtool.c         |  65 +++
>  drivers/net/can/slcan/slcan.h                 |  18 +
>  5 files changed, 480 insertions(+), 76 deletions(-)
>  create mode 100644 drivers/net/can/slcan/Makefile
>  rename drivers/net/can/{slcan.c => slcan/slcan-core.c} (67%)
>  create mode 100644 drivers/net/can/slcan/slcan-ethtool.c
>  create mode 100644 drivers/net/can/slcan/slcan.h
  

On 07.06.2022 19:27:05, Vincent MAILHOL wrote:
> On Tue. 7 juin 2022 at 18:47, Dario Binacchi
> <dario.binacchi@amarulasolutions.com> wrote:
> > 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.
> 
> That's funny! Yesterday, I sent this comment:
> https://lore.kernel.org/linux-can/CAMZ6RqKZwC_OKcgH+WPacY6kbNbj4xR2Gdg2NQtm5Ka5Hfw79A@mail.gmail.com/
> 
> And today, you send a full series to remove all the dust from the
> slcan driver. Do I have some kind of mystical power to summon people
> on the mailing list?

That would be very useful and awesome super power, I'm a bit jealous. :D

> > Starting from these assumptions, I tried to:
> > - Use the CAN network device driver interface.
> 
> In order to use the CAN network device driver, a.k.a. can-dev module,
> drivers/net/can/Kbuild has to be adjusted: move slcan inside CAN_DEV
> scope.
> 
> @Mark: because I will have to send a new version for my can-dev/Kbuild
> cleanup, maybe I can take that change and add it to my series?

Let's get the your Kconfig/Makefile changes into can-next/master first.
Then Dario can then base this series on that branch.

regards,
Marc
  

+CC: Max Staudt <max@enpas.org>

On Tue. 7 Jun. 2022 at 18:47, Dario Binacchi
<dario.binacchi@amarulasolutions.com> wrote:
> 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. 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

In his CAN327 driver, Max manages to bring the can0 device without the
need of dedicated user space daemon by using line discipline
(ldattach):
https://lore.kernel.org/linux-can/20220602213544.68273-1-max@enpas.org/

Isn't the same feasible with slcan so that we completely remove the
dependency toward slcand?
Max what do you think of this?

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

Yours sincerely,
Vincent Mailhol
  

On Tue. 7 Jun 2022 at 19:39, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 07.06.2022 19:27:05, Vincent MAILHOL wrote:
> > On Tue. 7 juin 2022 at 18:47, Dario Binacchi
> > <dario.binacchi@amarulasolutions.com> wrote:
> > > 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.
> >
> > That's funny! Yesterday, I sent this comment:
> > https://lore.kernel.org/linux-can/CAMZ6RqKZwC_OKcgH+WPacY6kbNbj4xR2Gdg2NQtm5Ka5Hfw79A@mail.gmail.com/
> >
> > And today, you send a full series to remove all the dust from the
> > slcan driver. Do I have some kind of mystical power to summon people
> > on the mailing list?
>
> That would be very useful and awesome super power, I'm a bit jealous. :D
>
> > > Starting from these assumptions, I tried to:
> > > - Use the CAN network device driver interface.
> >
> > In order to use the CAN network device driver, a.k.a. can-dev module,
> > drivers/net/can/Kbuild has to be adjusted: move slcan inside CAN_DEV
> > scope.
> >
> > @Mark: because I will have to send a new version for my can-dev/Kbuild
> > cleanup, maybe I can take that change and add it to my series?
>
> Let's get the your Kconfig/Makefile changes into can-next/master first.
> Then Dario can then base this series on that branch.

ACK. I'll keep my series as-is.
  

On Tue, 7 Jun 2022 21:19:54 +0900
Vincent MAILHOL <mailhol.vincent@wanadoo.fr> wrote:

> In his CAN327 driver, Max manages to bring the can0 device without the
> need of dedicated user space daemon by using line discipline
> (ldattach):
> https://lore.kernel.org/linux-can/20220602213544.68273-1-max@enpas.org/
> 
> Isn't the same feasible with slcan so that we completely remove the
> dependency toward slcand?
> Max what do you think of this?

I think it is a good idea to move this into the kernel driver. I don't
have a slcan device, but a quick peek at its protocol suggests that
it can be done much easier and cleaner than in can327.


Fun fact: The use of a userspace "ignition" tool is a possible use case
baked into can327's design ;)

There is only one use case I can see for it though: Probing the
ELM327's baud rate, and/or setting it before attaching the ldisc. This
is the single thing that the kernel driver cannot do, since it is
attached to an already running TTY, with a fixed speed. Everything else
is configurable via "ip link".

slcand could be used to auto-probe and set the baud rate as well.

Then again, these stripped-down tools could likely be implemented as
shell scripts calling stty and ldattach...


Max
  

Dario, thank you so much for working on slcan!


To speed up the slcan cleanup, may I suggest looking at can327?

It started as a modification of slcan, and over the past few months,
it has gone through several review rounds in upstreaming. In fact, a
*ton* of things pointed out during reviews would apply 1:1 to slcan.

What's more, there's legacy stuff that's no longer needed. No
SLCAN_MAGIC, no slcan_devs, ... it's all gone in can327. May I suggest
you have a look at it and bring slcan's boilerplate in line with it?

It's certainly not perfect (7 patch series and counting, and that's
just the public ones), but I'm sure that looking at the two drivers
side-by-side could serve as a good starting point, to avoid
re-reviewing the same things all over again.


The current out-of-tree version can be found here (the repo name is
still the old one, elmcan), where I occasionally push changes before
bundling them up into an upstreaming patch. If a specific line seems
strange to you, "git blame" on this repo is likely to dig up a helpful
commit message, explaining the choice:

  https://github.com/norly/elmcan
  https://git.enpas.org/?p=elmcan.git


Max
  

On 08.06.2022 02:15:37, Max Staudt wrote:
> To speed up the slcan cleanup, may I suggest looking at can327?
> 
> It started as a modification of slcan, and over the past few months,
> it has gone through several review rounds in upstreaming. In fact, a
> *ton* of things pointed out during reviews would apply 1:1 to slcan.
> 
> What's more, there's legacy stuff that's no longer needed. No
> SLCAN_MAGIC, no slcan_devs, ... it's all gone in can327. May I suggest
> you have a look at it and bring slcan's boilerplate in line with it?

+1

Most of Dario's series looks good. I suggest that we mainline this
first. If there's interest and energy the slcan driver can be reworked
to re-use the more modern concepts of the can327 driver.

> It's certainly not perfect (7 patch series and counting, and that's
> just the public ones), but I'm sure that looking at the two drivers
> side-by-side could serve as a good starting point, to avoid
> re-reviewing the same things all over again.

regards,
Marc
  

On Wed, 8 Jun 2022 09:19:47 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 08.06.2022 02:15:37, Max Staudt wrote:
> > To speed up the slcan cleanup, may I suggest looking at can327?
> > 
> > It started as a modification of slcan, and over the past few months,
> > it has gone through several review rounds in upstreaming. In fact, a
> > *ton* of things pointed out during reviews would apply 1:1 to slcan.
> > 
> > What's more, there's legacy stuff that's no longer needed. No
> > SLCAN_MAGIC, no slcan_devs, ... it's all gone in can327. May I
> > suggest you have a look at it and bring slcan's boilerplate in line
> > with it?  
> 
> +1
> 
> Most of Dario's series looks good. I suggest that we mainline this
> first. If there's interest and energy the slcan driver can be reworked
> to re-use the more modern concepts of the can327 driver.

Agreed. It does look good, and I'm glad to see slcan get some
love.

Thanks Dario!


Max