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

  ES58x devices report below information in their usb product info
string:

  * the firmware version
  * the bootloader version
  * the hardware revision

Report the firmware version through ethtool_drvinfo::fw_version.
Because struct ethtool_drvinfo has no fields to report the boatloader
version nor the hardware revision, continue to print these in the
kernel log (c.f. es58x_get_product_info()).

While doing so, bump up copyright year of each modified files.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/usb/etas_es58x/es581_4.c    |  5 ++-
 drivers/net/can/usb/etas_es58x/es58x_core.c | 42 ++++++++++++++++++++-
 drivers/net/can/usb/etas_es58x/es58x_core.h |  5 ++-
 drivers/net/can/usb/etas_es58x/es58x_fd.c   |  5 ++-
 4 files changed, 51 insertions(+), 6 deletions(-)
  

On Sat, Nov 05, 2022 at 02:16:04AM +0900, Vincent Mailhol wrote:
> ES58x devices report below information in their usb product info
> string:
> 
>   * the firmware version
>   * the bootloader version
>   * the hardware revision
> 
> Report the firmware version through ethtool_drvinfo::fw_version.
> Because struct ethtool_drvinfo has no fields to report the boatloader
> version nor the hardware revision, continue to print these in the
> kernel log (c.f. es58x_get_product_info()).
> 
> While doing so, bump up copyright year of each modified files.

Why not just stick to the normal USB interface here and not try to tie
it into ethtool?  These values are all availble today in sysfs or in
libusb, right?

What workflow wants this added to ethtool?

thanks,

greg k-h
  

On Sat. 5 Nov. 2022 at 17:36, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, Nov 05, 2022 at 02:16:04AM +0900, Vincent Mailhol wrote:
> > ES58x devices report below information in their usb product info
> > string:
> >
> >   * the firmware version
> >   * the bootloader version
> >   * the hardware revision
> >
> > Report the firmware version through ethtool_drvinfo::fw_version.
> > Because struct ethtool_drvinfo has no fields to report the boatloader
> > version nor the hardware revision, continue to print these in the
> > kernel log (c.f. es58x_get_product_info()).
> >
> > While doing so, bump up copyright year of each modified files.
>
> Why not just stick to the normal USB interface here and not try to tie
> it into ethtool?  These values are all availble today in sysfs or in
> libusb, right?

The simple answer is ignorance. I am more familiar with ethtool than
libusb and I just did not think to explore that second option.
Thanks for the review, comments taken. I will study sysfs and libusb
and will rework that.

> What workflow wants this added to ethtool?

No workflow. My work is not bound to any company and this driver
maintenance and anything else I am doing on the mailing list at this
time is pure hobby.

Yours sincerely,
Vincent Mailhol
  

On Sat. 5 Nov. 2022 at 18:27, Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
> On Sat. 5 Nov. 2022 at 17:36, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Sat, Nov 05, 2022 at 02:16:04AM +0900, Vincent Mailhol wrote:
> > > ES58x devices report below information in their usb product info
> > > string:
> > >
> > >   * the firmware version
> > >   * the bootloader version
> > >   * the hardware revision
> > >
> > > Report the firmware version through ethtool_drvinfo::fw_version.
> > > Because struct ethtool_drvinfo has no fields to report the boatloader
> > > version nor the hardware revision, continue to print these in the
> > > kernel log (c.f. es58x_get_product_info()).
> > >
> > > While doing so, bump up copyright year of each modified files.
> >
> > Why not just stick to the normal USB interface here and not try to tie
> > it into ethtool?  These values are all availble today in sysfs or in
> > libusb, right?
>
> The simple answer is ignorance. I am more familiar with ethtool than
> libusb and I just did not think to explore that second option.
> Thanks for the review, comments taken. I will study sysfs and libusb
> and will rework that.

I double checked following options:
  * CONFIG_USB_ANNOUNCE_NEW_DEVICES
  * lsusb -v from usbutils
  * sysfs

None of those will return the firmware version. The only strings I am
getting are: the Product name, the Manufacturer and the SerialNumber.

I guess you were expecting some default behavior from the device, but
unfortunately, this is not the case.
On this device, the firmware version is stored at some arbitrary
descriptor index (if you ask me: 6). Unless you query that magic
number, the information will not pot up.

So as far as I can see, this does not duplicate existing mechanisms.
With this patch, the firmware version becomes available using:
  $ ethtool -i canX

> > What workflow wants this added to ethtool?
>
> No workflow. My work is not bound to any company and this driver
> maintenance and anything else I am doing on the mailing list at this
> time is pure hobby.
>
> Yours sincerely,
> Vincent Mailhol
  

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@wanadoo.fr> wrote:
> > On Sat. 5 Nov. 2022 at 17:36, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > On Sat, Nov 05, 2022 at 02:16:04AM +0900, Vincent Mailhol wrote:
> > > > ES58x devices report below information in their usb product info
> > > > string:
> > > >
> > > >   * the firmware version
> > > >   * the bootloader version
> > > >   * the hardware revision
> > > >
> > > > Report the firmware version through ethtool_drvinfo::fw_version.
> > > > Because struct ethtool_drvinfo has no fields to report the boatloader
> > > > version nor the hardware revision, continue to print these in the
> > > > kernel log (c.f. es58x_get_product_info()).
> > > >
> > > > While doing so, bump up copyright year of each modified files.
> > >
> > > Why not just stick to the normal USB interface here and not try to tie
> > > it into ethtool?  These values are all availble today in sysfs or in
> > > libusb, right?
> >
> > The simple answer is ignorance. I am more familiar with ethtool than
> > libusb and I just did not think to explore that second option.
> > Thanks for the review, comments taken. I will study sysfs and libusb
> > and will rework that.
> 
> I double checked following options:
>   * CONFIG_USB_ANNOUNCE_NEW_DEVICES
>   * lsusb -v from usbutils
>   * sysfs
> 
> None of those will return the firmware version. The only strings I am
> getting are: the Product name, the Manufacturer and the SerialNumber.

Those are the default strings that a device can have, so it's good that
the core tries to get them.

Anything other than those are "custom" strings and you can use libusb
for that.  For some reason I thought sysfs also had custom strings, but
as they are so rare I don't know if anyone has tried that.

> I guess you were expecting some default behavior from the device, but
> unfortunately, this is not the case.
> On this device, the firmware version is stored at some arbitrary
> descriptor index (if you ask me: 6). Unless you query that magic
> number, the information will not pot up.
> 
> So as far as I can see, this does not duplicate existing mechanisms.
> With this patch, the firmware version becomes available using:
>   $ ethtool -i canX

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.

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

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.

Sound reasonable?

greg k-h
  

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@wanadoo.fr> wrote:
> > > On Sat. 5 Nov. 2022 at 17:36, Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> 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.

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

Alan Stern

> Sound reasonable?
> 
> greg k-h
  

On Sun. 6 Nov. 2022 at 09:48, Alan Stern <stern@rowland.harvard.edu> 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@wanadoo.fr> wrote:
> > > > On Sat. 5 Nov. 2022 at 17:36, Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> 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
  

On Sat, Nov 05, 2022 at 08:45:10PM -0400, Alan Stern 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@wanadoo.fr> wrote:
> > > > On Sat. 5 Nov. 2022 at 17:36, Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> 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.

Yeah, my fault, nevermind about that, sorry.

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

We could have a directory of strings/ with the individual descriptors in
there as files with the name being the string id.

But that might take a long time to populate, as it can take a few tries
to get the string from a device, and to do that 256 times might be
noticable at device insertion time.

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

Agreed.

Ok, for this specific instance, adding the "we know this string id
should be here" as a device-specific sysfs file seems to be the easiest
way forward.

Vincent, want to try that instead?

thanks,

greg k-h
  

On Sun. 6 Nov. 2022 at 20:25, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, Nov 05, 2022 at 08:45:10PM -0400, Alan Stern 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@wanadoo.fr> wrote:
> > > > > On Sat. 5 Nov. 2022 at 17:36, Greg Kroah-Hartman
> > > > > <gregkh@linuxfoundation.org> 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.
>
> Yeah, my fault, nevermind about that, sorry.
>
> > > 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.
>
> We could have a directory of strings/ with the individual descriptors in
> there as files with the name being the string id.
>
> But that might take a long time to populate, as it can take a few tries
> to get the string from a device, and to do that 256 times might be
> noticable at device insertion time.
>
> > 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 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.
>
> Agreed.
>
> Ok, for this specific instance, adding the "we know this string id
> should be here" as a device-specific sysfs file seems to be the easiest
> way forward.
>
> Vincent, want to try that instead?

OK for me. Will do that and remove the kernel log spam and replace it
by a sysfs entry.

I have two questions:

1/ Can I still export and use usb_cache_string()? In other terms, does
the first patch of this series still apply? This looks like the most
convenient function to retrieve that custom string to me.

2/ Is it a no-go to also populate ethtool_drvinfo::fw_version? Some
users might look for the value in sysfs, while some might use ethtool.
If the info is not available in ethtool, people might simply assume it
is not available at all. So, I would like to provide both.


Yours sincerely,
Vincent Mailhol
  

On Sun, Nov 06, 2022 at 09:47:05PM +0900, Vincent MAILHOL wrote:
> On Sun. 6 Nov. 2022 at 20:25, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Sat, Nov 05, 2022 at 08:45:10PM -0400, Alan Stern 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@wanadoo.fr> wrote:
> > > > > > On Sat. 5 Nov. 2022 at 17:36, Greg Kroah-Hartman
> > > > > > <gregkh@linuxfoundation.org> 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.
> >
> > Yeah, my fault, nevermind about that, sorry.
> >
> > > > 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.
> >
> > We could have a directory of strings/ with the individual descriptors in
> > there as files with the name being the string id.
> >
> > But that might take a long time to populate, as it can take a few tries
> > to get the string from a device, and to do that 256 times might be
> > noticable at device insertion time.
> >
> > > 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 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.
> >
> > Agreed.
> >
> > Ok, for this specific instance, adding the "we know this string id
> > should be here" as a device-specific sysfs file seems to be the easiest
> > way forward.
> >
> > Vincent, want to try that instead?
> 
> OK for me. Will do that and remove the kernel log spam and replace it
> by a sysfs entry.
> 
> I have two questions:
> 
> 1/ Can I still export and use usb_cache_string()? In other terms, does
> the first patch of this series still apply? This looks like the most
> convenient function to retrieve that custom string to me.

Everyone seems to just use the usb_string() function, will that not work
for you?

> 2/ Is it a no-go to also populate ethtool_drvinfo::fw_version? Some
> users might look for the value in sysfs, while some might use ethtool.
> If the info is not available in ethtool, people might simply assume it
> is not available at all. So, I would like to provide both.

That's up to the network developers/maintainers.  I don't know if that's
a required or common api for network devices to have.

thanks,

greg k-h
  

Le dim. 6 nov. 2022 à 23:22, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> a écrit :
>
> On Sun, Nov 06, 2022 at 09:47:05PM +0900, Vincent MAILHOL wrote:
> > On Sun. 6 Nov. 2022 at 20:25, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > On Sat, Nov 05, 2022 at 08:45:10PM -0400, Alan Stern 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@wanadoo.fr> wrote:
> > > > > > > On Sat. 5 Nov. 2022 at 17:36, Greg Kroah-Hartman
> > > > > > > <gregkh@linuxfoundation.org> 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.
> > >
> > > Yeah, my fault, nevermind about that, sorry.
> > >
> > > > > 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.
> > >
> > > We could have a directory of strings/ with the individual descriptors in
> > > there as files with the name being the string id.
> > >
> > > But that might take a long time to populate, as it can take a few tries
> > > to get the string from a device, and to do that 256 times might be
> > > noticable at device insertion time.
> > >
> > > > 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 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.
> > >
> > > Agreed.
> > >
> > > Ok, for this specific instance, adding the "we know this string id
> > > should be here" as a device-specific sysfs file seems to be the easiest
> > > way forward.
> > >
> > > Vincent, want to try that instead?
> >
> > OK for me. Will do that and remove the kernel log spam and replace it
> > by a sysfs entry.
> >
> > I have two questions:
> >
> > 1/ Can I still export and use usb_cache_string()? In other terms, does
> > the first patch of this series still apply? This looks like the most
> > convenient function to retrieve that custom string to me.
>
> Everyone seems to just use the usb_string() function, will that not work
> for you?

It is just that I would have to write two or three lines of code less.
But if you prefer I can use usb_string(), no problem on that.

> > 2/ Is it a no-go to also populate ethtool_drvinfo::fw_version? Some
> > users might look for the value in sysfs, while some might use ethtool.
> > If the info is not available in ethtool, people might simply assume it
> > is not available at all. So, I would like to provide both.
>
> That's up to the network developers/maintainers.  I don't know if that's
> a required or common api for network devices to have.

My question was more to know if you had any objection if I did so.
From the documentation, there is no indication that this is required.
But I don't like to leave a field empty when I have the information to
fill it.


Yours sincerely,
Vincent Mailhol
  

On Sun, Nov 06, 2022 at 09:47:05PM +0900, Vincent MAILHOL wrote:
> I have two questions:
> 
> 1/ Can I still export and use usb_cache_string()? In other terms, does
> the first patch of this series still apply? This looks like the most
> convenient function to retrieve that custom string to me.

FWIW, that's okay with me.

> 2/ Is it a no-go to also populate ethtool_drvinfo::fw_version? Some
> users might look for the value in sysfs, while some might use ethtool.
> If the info is not available in ethtool, people might simply assume it
> is not available at all. So, I would like to provide both.

I don't see any reason not to do this.

Alan Stern
  

On Sun, Nov 06, 2022 at 11:44:52PM +0900, Vincent MAILHOL wrote:
> Le dim. 6 nov. 2022 à 23:22, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> a écrit :
> >
> > On Sun, Nov 06, 2022 at 09:47:05PM +0900, Vincent MAILHOL wrote:
> > > On Sun. 6 Nov. 2022 at 20:25, Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > > On Sat, Nov 05, 2022 at 08:45:10PM -0400, Alan Stern 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@wanadoo.fr> wrote:
> > > > > > > > On Sat. 5 Nov. 2022 at 17:36, Greg Kroah-Hartman
> > > > > > > > <gregkh@linuxfoundation.org> 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.
> > > >
> > > > Yeah, my fault, nevermind about that, sorry.
> > > >
> > > > > > 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.
> > > >
> > > > We could have a directory of strings/ with the individual descriptors in
> > > > there as files with the name being the string id.
> > > >
> > > > But that might take a long time to populate, as it can take a few tries
> > > > to get the string from a device, and to do that 256 times might be
> > > > noticable at device insertion time.
> > > >
> > > > > 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 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.
> > > >
> > > > Agreed.
> > > >
> > > > Ok, for this specific instance, adding the "we know this string id
> > > > should be here" as a device-specific sysfs file seems to be the easiest
> > > > way forward.
> > > >
> > > > Vincent, want to try that instead?
> > >
> > > OK for me. Will do that and remove the kernel log spam and replace it
> > > by a sysfs entry.
> > >
> > > I have two questions:
> > >
> > > 1/ Can I still export and use usb_cache_string()? In other terms, does
> > > the first patch of this series still apply? This looks like the most
> > > convenient function to retrieve that custom string to me.
> >
> > Everyone seems to just use the usb_string() function, will that not work
> > for you?
> 
> It is just that I would have to write two or three lines of code less.

Odd, should it be used instead where others are calling usb_string()?

> But if you prefer I can use usb_string(), no problem on that.

Try it both ways.  If it's easier with usb_cache_string(), then we can
export it.  It's just odd that it hasn't been exported yet.

> > > 2/ Is it a no-go to also populate ethtool_drvinfo::fw_version? Some
> > > users might look for the value in sysfs, while some might use ethtool.
> > > If the info is not available in ethtool, people might simply assume it
> > > is not available at all. So, I would like to provide both.
> >
> > That's up to the network developers/maintainers.  I don't know if that's
> > a required or common api for network devices to have.
> 
> My question was more to know if you had any objection if I did so.
> From the documentation, there is no indication that this is required.
> But I don't like to leave a field empty when I have the information to
> fill it.

No objection from me.

thanks,

greg k-h
  

On Mon. 7 nov. 2022 at 01:18, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sun, Nov 06, 2022 at 11:44:52PM +0900, Vincent MAILHOL wrote:
> > On Sun. 6 Nov. 2022 à 23:22, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > > On Sun, Nov 06, 2022 at 09:47:05PM +0900, Vincent MAILHOL wrote:
> > > > On Sun. 6 Nov. 2022 at 20:25, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > > > 1/ Can I still export and use usb_cache_string()? In other terms, does
> > > > the first patch of this series still apply? This looks like the most
> > > > convenient function to retrieve that custom string to me.
> > >
> > > Everyone seems to just use the usb_string() function, will that not work
> > > for you?
> >
> > It is just that I would have to write two or three lines of code less.
>
> Odd, should it be used instead where others are calling usb_string()?
>
> > But if you prefer I can use usb_string(), no problem on that.
>
> Try it both ways.  If it's easier with usb_cache_string(), then we can
> export it.  It's just odd that it hasn't been exported yet.

I tried both. Not counting the line breaks, the empty lines and the
comments, the usb_string() version needs 6 more lines. Not a huge
difference but the usb_cache_string() remains easier (at least in my
eyes).

For reference, here is the diff before and after using usb_cache_string():

diff --git a/drivers/net/can/usb/etas_es58x/es58x_sysfs.c
b/drivers/net/can/usb/etas_es58x/es58x_sysfs.c
index 4ff0332f6f50..c1d220d0d35f 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_sysfs.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_sysfs.c
@@ -178,17 +178,10 @@ void es58x_create_file(struct device *dev)
 {
        struct es58x_device *es58x_dev = dev_get_drvdata(dev);
        char *prod_info;
-       int ret;

-       prod_info = kmalloc(ES58X_PROD_INFO_SIZE, GFP_KERNEL);
-       if (!prod_info)
-               return;
-
-        ret = usb_string(es58x_dev->udev, ES58X_PROD_INFO_IDX,
-                        prod_info, ES58X_PROD_INFO_SIZE);
-        if (ret < 0) {
+       prod_info = usb_cache_string(es58x_dev->udev, ES58X_PROD_INFO_IDX);
+       if (!prod_info) {
                dev_warn(dev, "could not retrieve the product info string\n");
-               kfree(prod_info);
                return;
        }

diff --git a/drivers/net/can/usb/etas_es58x/es58x_sysfs.h
b/drivers/net/can/usb/etas_es58x/es58x_sysfs.h
index 62347ffa0214..a204aa5344a8 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_sysfs.h
+++ b/drivers/net/can/usb/etas_es58x/es58x_sysfs.h
@@ -14,13 +14,6 @@
 /* USB descriptor index containing the product information string. */
 #define ES58X_PROD_INFO_IDX 6

-/* Maximum size for the USB information custom string. USB strings are
- * at most 127 characters and es58x devices only use ASCII (i.e. one
- * byte). Also, empirical observations show a maximum length of 83
- * bytes for the product information.
- */
-#define ES58X_PROD_INFO_SIZE (127 + 1)
-
 void es58x_create_file(struct device *dev);
 void es58x_remove_file(struct device *dev);