| Message ID | 20221104171604.24052-1-mailhol.vincent@wanadoo.fr |
|---|---|
| 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 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
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.
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);
The goal of this series is to report the firmware version of ETAS ES58x devices through ethtool. The easiest way to do so is by using usb_cache_string so that we do not have to manage errors. First patch exports usb_cache_string(). The second patch then does a small cleanup in the existing function and replace existing code with usb_cache_string(). The third and final patch reports the firmware version of the device to the userland through ethtool. * Changelog * v1 -> v2: * was a single patch. It is now a series of three patches. * add a first new patch to export usb_cache_string(). * add a second new patch to apply usb_cache_string() to existing code. * add missing check on product info string to prevent a buffer overflow. * add comma on the last entry of struct es58x_parameters. Vincent Mailhol (3): USB: core: export usb_cache_string() can: etas_es58x: use usb_cache_string() to retrieve the product info string can: etas_es58x: report the firmware version through ethtool drivers/net/can/usb/etas_es58x/es581_4.c | 5 +- drivers/net/can/usb/etas_es58x/es58x_core.c | 75 ++++++++++++--------- drivers/net/can/usb/etas_es58x/es58x_core.h | 8 ++- drivers/net/can/usb/etas_es58x/es58x_fd.c | 5 +- drivers/usb/core/message.c | 1 + drivers/usb/core/usb.h | 1 - include/linux/usb.h | 1 + 7 files changed, 60 insertions(+), 36 deletions(-)