Re: [PATCH net-next 0/14] pull-request: can-next 2022-10-31

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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