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@xxxxxxxxxxxxxx/ > > 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 -- 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 |
Attachment:
signature.asc
Description: PGP signature
