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@xxxxxxxxxxxxxxx/
>
> 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@xxxxxxxxxx>
> > Reviewed-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
> > CC: Stephane Grosjean <s.grosjean@xxxxxxxxxxxxxxx>
> > ---
> > 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:
signature.asc
Description: PGP signature
