Re: [PATCH v2 3/3] can: etas_es58x: report the firmware version through ethtool

[Vincent MAILHOL]

On Sun. 6 Nov. 2022 at 09:48, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Sat, Nov 05, 2022 at 06:38:35PM +0100, Greg Kroah-Hartman wrote:
> > On Sun, Nov 06, 2022 at 02:21:11AM +0900, Vincent MAILHOL wrote:
> > > On Sat. 5 Nov. 2022 at 18:27, Vincent MAILHOL
> > > <mailhol.vincent@xxxxxxxxxx> wrote:
> > > > On Sat. 5 Nov. 2022 at 17:36, Greg Kroah-Hartman
> > > > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > It's late right now, and I can't remember the whole USB spec, but I
> > think the device provides a list of the string ids that are valid for
> > it.  If so, we can add that to sysfs for any USB device out there, no
> > matter the string descriptor number.
>
> No, there is no such list.
>
> > If not, maybe we can just iterate the 255 values and populate sysfs
> > files if they are present?  I'll dig up the USB spec tomorrow...
>
> Yes, we could do that.  But the filename would have to be the string
> id, which is not meaningful.  We wouldn't be able to have labels like
> "product-info" unless somehow a driver could provide the label.

My shot on this would be like this:

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 549590e9c644..d0a4fc3ffe07 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -77,6 +77,19 @@ struct ieee1394_device_id {
  * Use the flag values to control which fields are compared.
  */

+/**
+ * struct custom_string - information of custom string and their indexes
+ * @idx: Index of the custom string descriptor.
+ * @label: Mnemotechnic, will be used as a filename for the sysfs entry.
+ *
+ * USB devices might expose some information in some customs strings. Drivers
+ * can use this structure to inform the USB core of where to find these.
+ */
+struct custom_string {
+       __u8 idx;
+       const char *label;
+};
+
 /**
  * struct usb_device_id - identifies USB devices for probing and hotplugging
  * @match_flags: Bit mask controlling which of the other fields are used to
@@ -110,6 +123,9 @@ struct ieee1394_device_id {
  * @driver_info: Holds information used by the driver.  Usually it holds
  *         a pointer to a descriptor understood by the driver, or perhaps
  *         device flags.
+ * @customs_strings_table: devices using customs strings can use this table to
+ *         inform the USB core of how to retrieve them. If used, must
contained an
+ *         empty terminating entry.
  *
  * In most cases, drivers will create a table of device IDs by using
  * USB_DEVICE(), or similar macros designed for that purpose.
@@ -150,6 +166,7 @@ struct usb_device_id {
            /* not matched against */
            kernel_ulong_t  driver_info
                        __attribute__((aligned(sizeof(kernel_ulong_t))));
+           const struct custom_string *custom_strings_table;
 };

 /* Some useful macros to use to create struct usb_device_id */


Then the driver would declare its custom stings like this:

  static const struct custom_string es58x_custom_strings_table[] = {
          { .idx = 6, .label = "product_info" },
          { /* Terminating entry */ }
  };


Finally, the USB core can iterate through it and populate the sysfs
entries using the provided label.


> Also, there's the matter of language.  Devices can have string
> descriptors in multiple languages; which one should we show in sysfs?
> All of them?  Right now we use just the default language for the strings
> that we put in sysfs.

I do not have the knowledge to comment on the multiple languages
issue. FYI, the device which I maintain does not have multiple
languages.

> > I say do this at the USB core level, that way it works for any USB
> > device, and you don't have a device-specific sysfs file and custom
> > userspace code just for this.
>
> This is unavoidable to some extent.  Without device-specific information
> or userspace code, there is no way to know which string descriptor
> contains the data you want.

ACK. I also do not want any userspace code for that. Users should not
need to know a magic number to retrieve the thing.

> Alan Stern
>
> > Sound reasonable?
> >
> > greg k-h