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@xxxxxxxxxxxxxx/ 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
