| Message ID | 20221104171604.24052-4-mailhol.vincent@wanadoo.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | can: etas_es58x: report firmware version | expand |
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);
diff --git a/drivers/net/can/usb/etas_es58x/es581_4.c b/drivers/net/can/usb/etas_es58x/es581_4.c index 1bcdcece5ec7..29c03c8b3f07 100644 --- a/drivers/net/can/usb/etas_es58x/es581_4.c +++ b/drivers/net/can/usb/etas_es58x/es581_4.c @@ -6,7 +6,7 @@ * * Copyright (c) 2019 Robert Bosch Engineering and Business Solutions. All rights reserved. * Copyright (c) 2020 ETAS K.K.. All rights reserved. - * Copyright (c) 2020, 2021 Vincent Mailhol <mailhol.vincent@wanadoo.fr> + * Copyright (c) 2020-2022 Vincent Mailhol <mailhol.vincent@wanadoo.fr> */ #include <linux/kernel.h> @@ -492,7 +492,8 @@ const struct es58x_parameters es581_4_param = { .tx_bulk_max = ES581_4_TX_BULK_MAX, .urb_cmd_header_len = ES581_4_URB_CMD_HEADER_LEN, .rx_urb_max = ES58X_RX_URBS_MAX, - .tx_urb_max = ES58X_TX_URBS_MAX + .tx_urb_max = ES58X_TX_URBS_MAX, + .prod_info_delim = ',', }; const struct es58x_operators es581_4_ops = { diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c index 1a17aadfc1dc..72a60f5f92c8 100644 --- a/drivers/net/can/usb/etas_es58x/es58x_core.c +++ b/drivers/net/can/usb/etas_es58x/es58x_core.c @@ -7,7 +7,7 @@ * * Copyright (c) 2019 Robert Bosch Engineering and Business Solutions. All rights reserved. * Copyright (c) 2020 ETAS K.K.. All rights reserved. - * Copyright (c) 2020, 2021 Vincent Mailhol <mailhol.vincent@wanadoo.fr> + * Copyright (c) 2020-2022 Vincent Mailhol <mailhol.vincent@wanadoo.fr> */ #include <linux/ethtool.h> @@ -1978,7 +1978,47 @@ static const struct net_device_ops es58x_netdev_ops = { .ndo_eth_ioctl = can_eth_ioctl_hwts, }; +/** + * es58x_get_drvinfo() - Get the driver name and firmware version. + * @netdev: CAN network device. + * @drvinfo: Driver information. + * + * Populate @drvinfo with the driver name and the firmware version. + */ +static void es58x_get_drvinfo(struct net_device *netdev, + struct ethtool_drvinfo *drvinfo) +{ + struct es58x_device *es58x_dev = es58x_priv(netdev)->es58x_dev; + char *prod_info, *start, *end; + + strscpy(drvinfo->driver, KBUILD_MODNAME, sizeof(drvinfo->driver)); + + prod_info = usb_cache_string(es58x_dev->udev, ES58X_PROD_INFO_IDX); + if (!prod_info) + return; + + /* The firmware prefix is either "FW_V" or "FW:" */ + start = strstr(prod_info, "FW"); + if (!start) + goto free_prod_info; + /* Go to first digit */ + while (!isdigit(*start)) { + start++; + if (!*start) + goto free_prod_info; + } + end = strchr(start, es58x_dev->param->prod_info_delim); + if (!end || end - start >= sizeof(drvinfo->fw_version)) + goto free_prod_info; + + strncpy(drvinfo->fw_version, start, end - start); + + free_prod_info: + kfree(prod_info); +} + static const struct ethtool_ops es58x_ethtool_ops = { + .get_drvinfo = es58x_get_drvinfo, .get_ts_info = can_ethtool_op_get_ts_info_hwts, }; diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.h b/drivers/net/can/usb/etas_es58x/es58x_core.h index 9a5a616df783..9c2cdb57f34a 100644 --- a/drivers/net/can/usb/etas_es58x/es58x_core.h +++ b/drivers/net/can/usb/etas_es58x/es58x_core.h @@ -6,7 +6,7 @@ * * Copyright (c) 2019 Robert Bosch Engineering and Business Solutions. All rights reserved. * Copyright (c) 2020 ETAS K.K.. All rights reserved. - * Copyright (c) 2020, 2021 Vincent Mailhol <mailhol.vincent@wanadoo.fr> + * Copyright (c) 2020-2022 Vincent Mailhol <mailhol.vincent@wanadoo.fr> */ #ifndef __ES58X_COMMON_H__ @@ -309,6 +309,8 @@ struct es58x_priv { * @urb_cmd_header_len: Length of the URB command header. * @rx_urb_max: Number of RX URB to be allocated during device probe. * @tx_urb_max: Number of TX URB to be allocated during device probe. + * @prod_info_delim: delimiter of the different fields in the USB + * product information string. */ struct es58x_parameters { const struct can_bittiming_const *bittiming_const; @@ -327,6 +329,7 @@ struct es58x_parameters { u8 urb_cmd_header_len; u8 rx_urb_max; u8 tx_urb_max; + char prod_info_delim; }; /** diff --git a/drivers/net/can/usb/etas_es58x/es58x_fd.c b/drivers/net/can/usb/etas_es58x/es58x_fd.c index c97ffa71fd75..aa7a4866f870 100644 --- a/drivers/net/can/usb/etas_es58x/es58x_fd.c +++ b/drivers/net/can/usb/etas_es58x/es58x_fd.c @@ -8,7 +8,7 @@ * * Copyright (c) 2019 Robert Bosch Engineering and Business Solutions. All rights reserved. * Copyright (c) 2020 ETAS K.K.. All rights reserved. - * Copyright (c) 2020, 2021 Vincent Mailhol <mailhol.vincent@wanadoo.fr> + * Copyright (c) 2020-2022 Vincent Mailhol <mailhol.vincent@wanadoo.fr> */ #include <linux/kernel.h> @@ -550,7 +550,8 @@ const struct es58x_parameters es58x_fd_param = { .tx_bulk_max = ES58X_FD_TX_BULK_MAX, .urb_cmd_header_len = ES58X_FD_URB_CMD_HEADER_LEN, .rx_urb_max = ES58X_RX_URBS_MAX, - .tx_urb_max = ES58X_TX_URBS_MAX + .tx_urb_max = ES58X_TX_URBS_MAX, + .prod_info_delim = '-', }; const struct es58x_operators es58x_fd_ops = {
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(-)