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: 3996 bytes --]

On 30.09.2022 14:06:49, Marc Kleine-Budde wrote:
> 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,

I mean with FOO the name of the driver, as in the target of the link
"/sys/class/net/<iface>/device/driver", which is peak_usb in this case.

> see "Documentation/ABI/testing/sysfs-class-net-grcan" as an example.

> 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>
>  
>  #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);

I just noticed that the driver prints the device ID, but in decimal:

| ➜ (pts/0) frogger@riot:~ (master) journalctl -k|grep peak
| Sep 30 14:23:24 riot kernel: peak_usb 1-1.4:1.0: PEAK-System PCAN-USB FD v1 fw v3.2.0 (1 channels)
| Sep 30 14:23:24 riot kernel: peak_usb 1-1.4:1.0 can1: attached to PCAN-USB FD channel 0 (device 1144201745)
| Sep 30 14:23:24 riot kernel: usbcore: registered new interface driver peak_usb
| ➜ (pts/0) frogger@riot:~ (master) cat /sys/class/net/can1/device_id 
| 44332211

While the sysfs attribute is in hex. I have overwritten my original
device ID with 0x44332211 while testing Stéphane's patch. I'm wondering
if the serial number printed on the device's housing corresponds to the
default device ID. Is the printed device ID in hex or dec?

Mine is IPEH-004022-019814:
- it has leading zeros
- interpreted as hex, it doesn't fit into 32 bits
- interpreted as dec and converted into hex: 0xefbb26e6
  it would fit in 32 bits

If the default device ID corresponds to the printed-on number, do we
want to see/use this number in the sysfs, too?

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 --]