usbutils-016: displaying [unknown] has triggered a regression

[OpusElectronics]

In versions up to 015, lsusb would display the descriptor strings for the product and vendor of a device if the corresponding ID was not found in usb.ids, which I found very handy.

Due to the following commit: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usbutils.git/commit/?id=8a80f70a8afb2f6831b150a69cdf8f566deefe66

lsusb now displays [unknown] instead.

For instance, for one of my USB devices, with usbutils-015: Bus 003 Device 007: ID 1e7d:2de6 ROCCAT ROCCAT Burst Core

With usbutils-016: Bus 003 Device 007: ID 1e7d:2de6 ROCCAT [unknown]

I consider that a regression (found this feature pretty useful), although this may have been intentional, but I doubt it, as the commit seems to assume that an empty string was shown before, whereas the caller, when one of the functions such as get_product_string() returned 0, would pull the string from the descriptors. Now the functions do not return 0 in this case anymore.

[gregkh]

Ugh, you are so right, that's not intentional at all. Let me try to work on this next week unless someone beats me to it with a fix.

Sorry about this.

[abelbeck]

@OpusElectronics : We are seeing the same with 016. Our project does not install a usb.ids file for storage size considerations.

A 0 return value of get_vendor_string and get_product_string is checked in lsusb.c and a runtime sysfs value is used if not found in the usb.ids file.

https://github.com/gregkh/usbutils/blob/5148e86d09f9f1b1c44587d014e5bcee3bb8c26e/lsusb.c#L3618

Fix: (revert get_vendor_string and get_product_string changes)

--- usbutils-016/names.c.orig   2023-10-29 10:54:35.410937611 -0500
+++ usbutils-016/names.c    2023-10-29 10:55:49.123797360 -0500
@@ -192,7 +192,7 @@
                 return 0;
         *buf = 0;
         if (!(cp = names_vendor(vid)))
-       return snprintf(buf, size, "[unknown]");
+       return 0;
         return snprintf(buf, size, "%s", cp);
 }

@@ -204,7 +204,7 @@
                 return 0;
         *buf = 0;
         if (!(cp = names_product(vid, pid)))
-       return snprintf(buf, size, "[unknown]");
+       return 0;
         return snprintf(buf, size, "%s", cp);
 }

[gregkh]

Should be fixed in the tree now, can people test to verify it?

thanks for all of the reports.

[abelbeck]

@gregkh : Thanks for your timely attention

Though the read_sysfs_prop() read() could return -1 which would be considered TRUE (before negating) in your new if().

Possibly something like: (revert db6b20ab8477005658ba62ba48fe7c60b7144cc6 lsusb.c changes, then add)

    if (have_vendor && have_product)
        return;

    if (get_sysfs_name(sysfs_name, sizeof(sysfs_name), dev) >= 0) {
        if (!have_vendor)
            read_sysfs_prop(vendor, vendor_len, sysfs_name,
                    "manufacturer");
        if (!have_product)
            read_sysfs_prop(product, product_len, sysfs_name,
                    "product");
    }

+   if (!(*vendor))
+       strncpy(vendor, "[unknown]", vendor_len);
+   if (!(*product))
+       strncpy(product, "[unknown]", product_len);

This also handles the case when get_sysfs_name() fails.

[gregkh]

Good point, I've cleaned this up a bit more now in commit 2fc9cfc5a4cd ("names: simplify get_vendor_product_with_fallback() a bit")

[gregkh]

Meta-comment, this whole mess of "sometimes we parse sysfs, sometimes we parse the hwdb, sometimes we get info from lsusb" is a mess, and it's driving me to potentially fix this up much better in the future so we don't have this going forward. Give me a few weeks...

[abelbeck]

Good point, I've cleaned this up a bit more now in commit 2fc9cfc ("names: simplify get_vendor_product_with_fallback() a bit")

@gregkh : I don't think 2fc9cfc5a4cd5bc66aeca951c4f6b58f2b6671ef does what you want, as the get_vendor_string(), get_product_string() and read_sysfs_prop() functions all initially make the buffer a NULL string (as expected).

I think testing for a NULL string after all the sources have been tried, and only then set to "[unknown]".

[gregkh]

{sigh} You are right, my change does not work. I'll give it a day to calm down and then will implement your change :)

[OpusElectronics]

Thanks! Release 017 works well. =)