| Message ID | 20220725065419.3005015-1-dario.binacchi@amarulasolutions.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
On 25.07.2022 08:54:13, Dario Binacchi wrote: > With this series I try to finish the task, started with the series [1], > of completely removing the dependency of the slcan driver from the > userspace slcand/slcan_attach applications. > > The series, however, still lacks a patch for sending the bitrate setting > command to the adapter: > > slcan_attach -b <btr> <dev> > > Without at least this patch the task cannot be considered truly completed. > > The idea I got is that this can only happen through the ethtool API. > Among the various operations made available by this interface I would > have used the set_regs (but only the get_regs has been developed), or, > the set_eeprom, even if the setting would not be stored in an eeprom. > IMHO it would take a set_regs operation with a `struct ethtool_wregs' > parameter similar to `struct ethtool_eeprom' without the magic field: This doesn't feel right. > struct ethtool_wregs { > __u32 cmd; > __u32 offset; > __u32 len; > __u8 data[0]; > }; > > But I am not the expert and if there was an alternative solution already > usable, it would be welcome. Have a look at the get/set_tunable() callback: | https://elixir.bootlin.com/linux/latest/source/include/linux/ethtool.h#L575 You probably have to add a new tunable. Here you'll find the people and commits that changed the tunable: | https://github.com/torvalds/linux/blame/master/include/uapi/linux/ethtool.h#L229 It's usually worth including them in an RFC patch series where you add a new tunable and make use of it in the slcan driver. regards, Marc
Hello Marc, On Mon, Jul 25, 2022 at 3:25 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > On 25.07.2022 08:54:13, Dario Binacchi wrote: > > With this series I try to finish the task, started with the series [1], > > of completely removing the dependency of the slcan driver from the > > userspace slcand/slcan_attach applications. > > > > The series, however, still lacks a patch for sending the bitrate setting > > command to the adapter: > > > > slcan_attach -b <btr> <dev> > > > > Without at least this patch the task cannot be considered truly completed. > > > > The idea I got is that this can only happen through the ethtool API. > > Among the various operations made available by this interface I would > > have used the set_regs (but only the get_regs has been developed), or, > > the set_eeprom, even if the setting would not be stored in an eeprom. > > IMHO it would take a set_regs operation with a `struct ethtool_wregs' > > parameter similar to `struct ethtool_eeprom' without the magic field: > > This doesn't feel right. > > > struct ethtool_wregs { > > __u32 cmd; > > __u32 offset; > > __u32 len; > > __u8 data[0]; > > }; > > > > But I am not the expert and if there was an alternative solution already > > usable, it would be welcome. > > Have a look at the get/set_tunable() callback: > > | https://elixir.bootlin.com/linux/latest/source/include/linux/ethtool.h#L575 > > You probably have to add a new tunable. Here you'll find the people and > commits that changed the tunable: > > | https://github.com/torvalds/linux/blame/master/include/uapi/linux/ethtool.h#L229 > > It's usually worth including them in an RFC patch series where you add a > new tunable and make use of it in the slcan driver. Thank you very much for the suggestions. 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 |
With this series I try to finish the task, started with the series [1], of completely removing the dependency of the slcan driver from the userspace slcand/slcan_attach applications. The series, however, still lacks a patch for sending the bitrate setting command to the adapter: slcan_attach -b <btr> <dev> Without at least this patch the task cannot be considered truly completed. The idea I got is that this can only happen through the ethtool API. Among the various operations made available by this interface I would have used the set_regs (but only the get_regs has been developed), or, the set_eeprom, even if the setting would not be stored in an eeprom. IMHO it would take a set_regs operation with a `struct ethtool_wregs' parameter similar to `struct ethtool_eeprom' without the magic field: struct ethtool_wregs { __u32 cmd; __u32 offset; __u32 len; __u8 data[0]; }; But I am not the expert and if there was an alternative solution already usable, it would be welcome. The series also contains patches that remove the legacy stuff (slcan_devs, SLCAN_MAGIC, ...) and do some module cleanup. The series has been created on top of the patches: can: slcan: convert comments to network style comments can: slcan: slcan_init() convert printk(LEVEL ...) to pr_level() can: slcan: fix whitespace issues can: slcan: convert comparison to NULL into !val can: slcan: clean up if/else can: slcan: use scnprintf() as a hardening measure can: slcan: do not report txerr and rxerr during bus-off can: slcan: do not sleep with a spin lock held applied to linux-next. [1] https://lore.kernel.org/all/20220628163137.413025-1-dario.binacchi@amarulasolutions.com/ Changes in v2: - Re-add headers that export at least one symbol used by the module. - Update the commit description. - Drop the old "slcan" name to use the standard canX interface naming. - Remove comment on listen-only command. - Update the commit subject and description. - Add the patch "MAINTAINERS: Add myself as maintainer of the SLCAN driver" to the series. Dario Binacchi (6): can: slcan: remove useless header inclusions can: slcan: remove legacy infrastructure can: slcan: change every `slc' occurrence in `slcan' can: slcan: use the generic can_change_mtu() can: slcan: add support for listen-only mode MAINTAINERS: Add maintainer for the slcan driver MAINTAINERS | 6 + drivers/net/can/slcan/slcan-core.c | 451 +++++++++-------------------- 2 files changed, 139 insertions(+), 318 deletions(-)