| Message ID | 20220607094752.1029295-1-dario.binacchi@amarulasolutions.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
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
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