Re: [RESEND PATCH v2] can: peak_usb: export PCAN device ID as sysfs device attribute

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 5099 bytes --]

On 28.09.2022 20:43:19, Lukas Magel wrote:
> I was made aware by Stéphane that there has previously been discussion about
> registering a sysfs file for reading and writing the device ID [1]. IMHO, I
> believe the sysfs file approach would have one important advantage over using
> ethtool: udev rule matching.
> 
> I often work with setups that feature several CAN interfaces. In such a setup, I
> want to assign persistent interface names via udev in case device probing is not
> deterministic or the devices are plugged in to different ports. At the moment,
> the PEAK device driver and the underlying USB interface do not export any
> practically usable identifier for discriminating between different interfaces of
> the same type. The device ID solves this problem since it can be configured per
> CAN interface to uniquely identify the interface. If the device ID is exported
> as sysfs file, it is directly available for matching in udev rules via the
> ATTR{...} key. This would solve the CAN interface identification problem and
> allow easy read and write access to the device ID without Windows userspace tools.

I'm fine with a RO sysfs attribute. For writing the ethtool interface
should be used instead.

Please document the sysfs entry in
Documentation/ABI/testing/sysfs-class-net-FOO, with foo according to
/sys/class/net/<iface>/FOO, see
"Documentation/ABI/testing/sysfs-class-net-grcan" as an example.

> Regards,
> Lukas
> 
> [1]
> https://lore.kernel.org/linux-can/20211117150132.37056-1-s.grosjean@peak-system.com/
> 
> On 15.09.22 11:54, Lukas Magel wrote:
> > Peak USB devices support a configurable u8 / u32 device ID. In
> > multi-device setups, this device ID can be configured and used to
> > identify individual CAN interfaces independent of the order in which
> > they are plugged into the system. At the current time, the device ID
> > is already queried from the device and stored in the peak_usb_device
> > struct.
> >
> > This patch exports the device ID (called device_number in the struct)
> > as a sysfs attribute. The attribute name was chosen to be device_id
> > instead of device_number because the latter has been deprecated by Peak
> > in their API.
> >
> > Signed-off-by: Lukas Magel <lukas.magel@posteo.net>
> > Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > CC: Stephane Grosjean <s.grosjean@peak-system.com>
> > ---
> > V2: Update netdev_warn to output the mnemonic of the error value
> >
> > Resubmission of the patch. Also added Stéphane as maintainer in CC.
> >
> >  drivers/net/can/usb/peak_usb/pcan_usb_core.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> > index 27b0a72fd885..7af3dd0a1b35 100644
> > --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> > +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> > @@ -15,6 +15,8 @@
> >  #include <linux/netdevice.h>
> >  #include <linux/usb.h>
> >  #include <linux/ethtool.h>
> > +#include <linux/device.h>
> > +#include <linux/sysfs.h>

Please sort the include files alphabetically.

> >  
> >  #include <linux/can.h>
> >  #include <linux/can/dev.h>
> > @@ -53,6 +55,15 @@ static const struct usb_device_id peak_usb_table[] = {
> >  
> >  MODULE_DEVICE_TABLE(usb, peak_usb_table);
> >  
> > +static ssize_t device_id_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +	struct net_device *netdev = to_net_dev(dev);
> > +	struct peak_usb_device *peak_dev = netdev_priv(netdev);
> > +
> > +	return sysfs_emit(buf, "%08X\n", peak_dev->device_number);
> > +}
> > +static DEVICE_ATTR_RO(device_id);
> > +
> >  /*
> >   * dump memory
> >   */
> > @@ -887,6 +898,11 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter,
> >  	netdev_info(netdev, "attached to %s channel %u (device %u)\n",
> >  			peak_usb_adapter->name, ctrl_idx, dev->device_number);
> >  
> > +	err = device_create_file(&netdev->dev, &dev_attr_device_id);
> > +	/* Do not error out since device was configured successfully */
> > +	if (err)
> > +		netdev_warn(netdev, "unable to expose device_id via sysfs: %pe\n", ERR_PTR(err));
> > +

This is racy, please use a sysfs_groups instead, see
https://git.kernel.org/torvalds/c/3a5655a5b545e9647c3437473ee3d815fe1b9050

> >  	return 0;
> >  
> >  adap_dev_free:
> > @@ -923,6 +939,8 @@ static void peak_usb_disconnect(struct usb_interface *intf)
> >  		dev->state &= ~PCAN_USB_STATE_CONNECTED;
> >  		strlcpy(name, netdev->name, IFNAMSIZ);
> >  
> > +		device_remove_file(&netdev->dev, &dev_attr_device_id);
> > +
> >  		unregister_netdev(netdev);
> >  
> >  		kfree(dev->cmd_buf);
> 

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]