| Message ID | 20221031154406.259857-1-mkl@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
On Mon, 31 Oct 2022 16:43:52 +0100 Marc Kleine-Budde wrote: > The first 7 patches are by Stephane Grosjean and Lukas Magel and > target the peak_usb driver. Support for flashing a user defined device > ID via the ethtool flash interface is added. A read only sysfs nit: ethtool eeprom set != ethtool flash > attribute for that value is added to distinguish between devices via > udev. So the user can write an arbitrary u32 value into flash which then persistently pops up in sysfs across reboots (as a custom attribute called "user_devid")? I don't know.. the whole thing strikes me as odd. Greg do you have any feelings about such.. solutions? patches 5 and 6 here: https://lore.kernel.org/all/20221031154406.259857-1-mkl@pengutronix.de/
On Mon, Oct 31, 2022 at 08:27:14PM -0700, Jakub Kicinski wrote: > On Mon, 31 Oct 2022 16:43:52 +0100 Marc Kleine-Budde wrote: > > The first 7 patches are by Stephane Grosjean and Lukas Magel and > > target the peak_usb driver. Support for flashing a user defined device > > ID via the ethtool flash interface is added. A read only sysfs > > nit: ethtool eeprom set != ethtool flash > > > attribute for that value is added to distinguish between devices via > > udev. > > So the user can write an arbitrary u32 value into flash which then > persistently pops up in sysfs across reboots (as a custom attribute > called "user_devid")? > > I don't know.. the whole thing strikes me as odd. Greg do you have any > feelings about such.. solutions? > > patches 5 and 6 here: > https://lore.kernel.org/all/20221031154406.259857-1-mkl@pengutronix.de/ Device-specific attributes should be in the device-specific directory, not burried in a class directory somewhere that is generic like this one is. Why isn't this an attribute of the usb device instead? And there's no need to reorder the .h file includes in patch 06 while you are adding a sysfs entry, that should be a separate commit, right? Also, the line: + .attrs = (struct attribute **)peak_usb_sysfs_attrs, Is odd, there should never be a need to cast anything like this if you are doing things properly. So this still needs work, sorry. thanks, greg k-h
On 31.10.2022 20:27:14, Jakub Kicinski wrote: > On Mon, 31 Oct 2022 16:43:52 +0100 Marc Kleine-Budde wrote: > > The first 7 patches are by Stephane Grosjean and Lukas Magel and > > target the peak_usb driver. Support for flashing a user defined device > > ID via the ethtool flash interface is added. A read only sysfs > > nit: ethtool eeprom set != ethtool flash Right, the driver uses the eeprom callbacks of ethtool. > > attribute for that value is added to distinguish between devices via > > udev. > > So the user can write an arbitrary u32 value into flash which then > persistently pops up in sysfs across reboots (as a custom attribute > called "user_devid")? ACK - Some devices support a u32, others only a u8. > I don't know.. the whole thing strikes me as odd. Greg do you have any > feelings about such.. solutions? > > patches 5 and 6 here: > https://lore.kernel.org/all/20221031154406.259857-1-mkl@pengutronix.de/ Marc
On 01.11.2022 06:06:13, Greg Kroah-Hartman wrote: > On Mon, Oct 31, 2022 at 08:27:14PM -0700, Jakub Kicinski wrote: > > On Mon, 31 Oct 2022 16:43:52 +0100 Marc Kleine-Budde wrote: > > > The first 7 patches are by Stephane Grosjean and Lukas Magel and > > > target the peak_usb driver. Support for flashing a user defined device > > > ID via the ethtool flash interface is added. A read only sysfs > > > > nit: ethtool eeprom set != ethtool flash > > > > > attribute for that value is added to distinguish between devices via > > > udev. > > > > So the user can write an arbitrary u32 value into flash which then > > persistently pops up in sysfs across reboots (as a custom attribute > > called "user_devid")? > > > > I don't know.. the whole thing strikes me as odd. Greg do you have any > > feelings about such.. solutions? > > > > patches 5 and 6 here: > > https://lore.kernel.org/all/20221031154406.259857-1-mkl@pengutronix.de/ > > Device-specific attributes should be in the device-specific directory, > not burried in a class directory somewhere that is generic like this one > is. > > Why isn't this an attribute of the usb device instead? What about: | /sys/devices/pci0000:00/0000:00:13.0/usb1/1-1/1-1:1.0/device_id > And there's no need to reorder the .h file includes in patch 06 while > you are adding a sysfs entry, that should be a separate commit, right? ACK > Also, the line: > > + .attrs = (struct attribute **)peak_usb_sysfs_attrs, > > Is odd, there should never be a need to cast anything like this if you > are doing things properly. After marking the struct attribute not as const, we can remove the cast: | --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c | +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c | @@ -64,14 +64,14 @@ static ssize_t user_devid_show(struct device *dev, struct device_attribute *attr | } | static DEVICE_ATTR_RO(user_devid); | | -static const struct attribute *peak_usb_sysfs_attrs[] = { | +static struct attribute *peak_usb_sysfs_attrs[] = { | &dev_attr_user_devid.attr, | NULL, | }; | | static const struct attribute_group peak_usb_sysfs_group = { | .name = "peak_usb", | - .attrs = (struct attribute **)peak_usb_sysfs_attrs, | + .attrs = peak_usb_sysfs_attrs, | }; | | /* But this code is obsolete, if we move the sysfs entry into the USB device. > So this still needs work, sorry. Marc
On Fri, Nov 04, 2022 at 02:08:57PM +0100, Marc Kleine-Budde wrote: > On 01.11.2022 06:06:13, Greg Kroah-Hartman wrote: > > Also, the line: > > > > + .attrs = (struct attribute **)peak_usb_sysfs_attrs, > > > > Is odd, there should never be a need to cast anything like this if you > > are doing things properly. > > After marking the struct attribute not as const, we can remove the cast: > > | --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c > | +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c > | @@ -64,14 +64,14 @@ static ssize_t user_devid_show(struct device *dev, struct device_attribute *attr > | } > | static DEVICE_ATTR_RO(user_devid); > | > | -static const struct attribute *peak_usb_sysfs_attrs[] = { > | +static struct attribute *peak_usb_sysfs_attrs[] = { Ah. Yeah, I would love to make this a const pointer, but people do some pretty crazy dynamic stuff with attribute groups at times, so that it will not always work. I have a long series of patches I'm working on that add more const markings to the kobject and then the driver core apis, I'll add this type of thing to the idea list as what to work on next. thanks, greg k-h
On 01.11.22 06:06, Greg Kroah-Hartman wrote: > On Mon, Oct 31, 2022 at 08:27:14PM -0700, Jakub Kicinski wrote: >> On Mon, 31 Oct 2022 16:43:52 +0100 Marc Kleine-Budde wrote: >>> The first 7 patches are by Stephane Grosjean and Lukas Magel and >>> target the peak_usb driver. Support for flashing a user defined device >>> ID via the ethtool flash interface is added. A read only sysfs >> nit: ethtool eeprom set != ethtool flash >> >>> attribute for that value is added to distinguish between devices via >>> udev. >> So the user can write an arbitrary u32 value into flash which then >> persistently pops up in sysfs across reboots (as a custom attribute >> called "user_devid")? >> >> I don't know.. the whole thing strikes me as odd. Greg do you have any >> feelings about such.. solutions? >> >> patches 5 and 6 here: >> https://lore.kernel.org/all/20221031154406.259857-1-mkl@pengutronix.de/ I now realize that I didn't do a sufficient job at describing the purpose of the device ID in the patches (which I am working on improving at the moment). In contrast to other devices (such as the ones from ETAS), at least the PCAN USB-FD devices do not export an iSerial attribute at USB level, which makes it hard to distinguish them if you are using multiple with the same product ID. The user device ID is a freely configurable identifier (basically an arbitrary u8 / u32 value like you said) that can be set individually for each CAN channel of any PEAK device and can serve as a replacement for the missing serial number. This use case is also explicitly stated in the Windows API manual for the PEAK devices (see page 11 in [1]). The patch series implements write support for the value via ethtool and exports it readonly as a sysfs attribute for udev matching. > Device-specific attributes should be in the device-specific directory, > not burried in a class directory somewhere that is generic like this one > is. > > Why isn't this an attribute of the usb device instead? Each CAN channel of a PEAK device can have its own device ID, meaning that there is a potential one-to-n mapping between USB device and device IDs. I can see how the name might appear confusing in that regard, we chose it to be consistent with the API description put out by PEAK (also see [1]). > And there's no need to reorder the .h file includes in patch 06 while > you are adding a sysfs entry, that should be a separate commit, right? I have split this into a separate commit. > Also, the line: > > + .attrs = (struct attribute **)peak_usb_sysfs_attrs, > > Is odd, there should never be a need to cast anything like this if you > are doing things properly. I have removed the const modifier from the struct as well as the cast. > So this still needs work, sorry. > > thanks, > > greg k-h > Please let me know if you require further changes to the patch series or want the attribute to be renamed. Regards, Lukas [1] See PCAN-Parameter_Documentation.pdf in https://www.peak-system.com/fileadmin/media/files/pcan-basic.zip
Hello Jakub, hello David, this is a pull request of 14 patches for net-next/master. The first 7 patches are by Stephane Grosjean and Lukas Magel and target the peak_usb driver. Support for flashing a user defined device ID via the ethtool flash interface is added. A read only sysfs attribute for that value is added to distinguish between devices via udev. The next 2 patches are by me an fix minor coding style issues in the kvaser_usb driver. The last 5 patches are by Biju Das, target the rcar_canfd driver and clean up the support for different IP cores. regards, Marc --- The following changes since commit 02a97e02c64fb3245b84835cbbed1c3a3222e2f1: Merge tag 'mlx5-updates-2022-10-24' of git://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux (2022-10-28 22:07:48 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git tags/linux-can-next-for-6.2-20221031 for you to fetch changes up to 3755b56a9b8ff8f1a6a21a979cc215c220401278: Merge patch series "R-Car CAN FD driver enhancements" (2022-10-31 16:15:25 +0100) ---------------------------------------------------------------- linux-can-next-for-6.2-20221031 ---------------------------------------------------------------- Biju Das (5): can: rcar_canfd: rcar_canfd_probe: Add struct rcar_canfd_hw_info to driver data can: rcar_canfd: Add max_channels to struct rcar_canfd_hw_info can: rcar_canfd: Add shared_global_irqs to struct rcar_canfd_hw_info can: rcar_canfd: Add postdiv to struct rcar_canfd_hw_info can: rcar_canfd: Add multi_channel_irqs to struct rcar_canfd_hw_info Lukas Magel (2): can: peak_usb: export PCAN user device ID as sysfs device attribute can: peak_usb: align user device id format in log with sysfs attribute Marc Kleine-Budde (4): Merge patch series "can: peak_usb: Introduce configurable user dev id" can: kvaser_usb: kvaser_usb_set_bittiming(): fix redundant initialization warning for err can: kvaser_usb: kvaser_usb_set_{,data}bittiming(): remove empty lines in variable declaration Merge patch series "R-Car CAN FD driver enhancements" Stephane Grosjean (5): can: peak_usb: rename device_id to a more explicit name can: peak_usb: add callback to read user value of CANFD devices can: peak_usb: allow flashing of the user device id can: peak_usb: replace unregister_netdev() with unregister_candev() can: peak_usb: add ethtool interface to user defined flashed device number Documentation/ABI/testing/sysfs-class-net-peak_usb | 15 +++ drivers/net/can/rcar/rcar_canfd.c | 85 +++++++++------ drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c | 4 +- drivers/net/can/usb/peak_usb/pcan_usb.c | 43 ++++++-- drivers/net/can/usb/peak_usb/pcan_usb_core.c | 119 +++++++++++++++++++-- drivers/net/can/usb/peak_usb/pcan_usb_core.h | 11 +- drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 64 ++++++++++- drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 26 ++++- drivers/net/can/usb/peak_usb/pcan_usb_pro.h | 1 + 9 files changed, 306 insertions(+), 62 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-class-net-peak_usb