[PATCH] usb: also announce bcdDevice

[PATCH] usb: also announce bcdDevice

[Paul Bolle]

Currently announce_device() does print the idVendor and idProduct values
but does not print the bcdDevice value. USB devices are accurately
identified by all three values. See, for instance, the USB storage
quirks which will only apply for a certain (range of) bcdDevice
value(s). So it seems useful to also print bcdDevice when announcing USB
devices.

Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
0) This is something I ran into while trying to track down the log
errors generated by each of the USB sticks I happen to have lying
around. Of course, there are other ways to track down the bcdDevice
value of a specific device ("lusb -v -d $vendorid:$productid | grep
bcdDevice" or "usb-devices | grep $vendorid.*$productid" come to mind).
But since idVendor and idProduct are already printed by this debugging
aid I think adding bcdDevice makes sense too. But is this all worth the
small bit of additional noise?

1) The patch generates a checkpatch warning: "quoted string split across
lines". But since it is hard to grep for this entire string without
knowing the printk "conversion specifications" in the string beforehand,
I think the warning can be ignored here. Note that I actually found this
string by git grepping for just "New USB device found".

 drivers/usb/core/Kconfig |    4 ++--
 drivers/usb/core/hub.c   |    6 ++++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig
index 18d02e3..85f795b 100644
--- a/drivers/usb/core/Kconfig
+++ b/drivers/usb/core/Kconfig
@@ -14,8 +14,8 @@ config USB_ANNOUNCE_NEW_DEVICES
 	depends on USB
 	default N
 	help
-	  Say Y here if you want the USB core to always announce the
-	  idVendor, idProduct, Manufacturer, Product, and SerialNumber
+	  Say Y here if you want the USB core to always announce the idVendor,
+	  idProduct, bcdDevice, Manufacturer, Product, and SerialNumber
 	  strings for every new USB device to the syslog.  This option is
 	  usually used by distro vendors to help with debugging and to
 	  let users know what specific device was added to the machine
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index ec6c97d..6e1bfaea 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1726,9 +1726,11 @@ static void show_string(struct usb_device *udev, char *id, char *string)
 
 static void announce_device(struct usb_device *udev)
 {
-	dev_info(&udev->dev, "New USB device found, idVendor=%04x, idProduct=%04x\n",
+	dev_info(&udev->dev, "New USB device found, idVendor=%04x, "
+		"idProduct=%04x, bcdDevice=%04x\n",
 		le16_to_cpu(udev->descriptor.idVendor),
-		le16_to_cpu(udev->descriptor.idProduct));
+		le16_to_cpu(udev->descriptor.idProduct),
+		le16_to_cpu(udev->descriptor.bcdDevice));
 	dev_info(&udev->dev,
 		"New USB device strings: Mfr=%d, Product=%d, SerialNumber=%d\n",
 		udev->descriptor.iManufacturer,
-- 
1.7.7.6

Re: [PATCH] usb: also announce bcdDevice

[Martin Mokrejs]

Paul Bolle wrote:
> Currently announce_device() does print the idVendor and idProduct values
> but does not print the bcdDevice value. USB devices are accurately
> identified by all three values. See, for instance, the USB storage
> quirks which will only apply for a certain (range of) bcdDevice
> value(s). So it seems useful to also print bcdDevice when announcing USB
> devices.

Could it also report negotiated speed? full-speed, high-speed, super-speed?
Thanks,
Martin

> 
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> ---
> 0) This is something I ran into while trying to track down the log
> errors generated by each of the USB sticks I happen to have lying
> around. Of course, there are other ways to track down the bcdDevice
> value of a specific device ("lusb -v -d $vendorid:$productid | grep
> bcdDevice" or "usb-devices | grep $vendorid.*$productid" come to mind).
> But since idVendor and idProduct are already printed by this debugging
> aid I think adding bcdDevice makes sense too. But is this all worth the
> small bit of additional noise?
> 
> 1) The patch generates a checkpatch warning: "quoted string split across
> lines". But since it is hard to grep for this entire string without
> knowing the printk "conversion specifications" in the string beforehand,
> I think the warning can be ignored here. Note that I actually found this
> string by git grepping for just "New USB device found".
> 
>  drivers/usb/core/Kconfig |    4 ++--
>  drivers/usb/core/hub.c   |    6 ++++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig
> index 18d02e3..85f795b 100644
> --- a/drivers/usb/core/Kconfig
> +++ b/drivers/usb/core/Kconfig
> @@ -14,8 +14,8 @@ config USB_ANNOUNCE_NEW_DEVICES
>  	depends on USB
>  	default N
>  	help
> -	  Say Y here if you want the USB core to always announce the
> -	  idVendor, idProduct, Manufacturer, Product, and SerialNumber
> +	  Say Y here if you want the USB core to always announce the idVendor,
> +	  idProduct, bcdDevice, Manufacturer, Product, and SerialNumber
>  	  strings for every new USB device to the syslog.  This option is
>  	  usually used by distro vendors to help with debugging and to
>  	  let users know what specific device was added to the machine
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index ec6c97d..6e1bfaea 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -1726,9 +1726,11 @@ static void show_string(struct usb_device *udev, char *id, char *string)
>  
>  static void announce_device(struct usb_device *udev)
>  {
> -	dev_info(&udev->dev, "New USB device found, idVendor=%04x, idProduct=%04x\n",
> +	dev_info(&udev->dev, "New USB device found, idVendor=%04x, "
> +		"idProduct=%04x, bcdDevice=%04x\n",
>  		le16_to_cpu(udev->descriptor.idVendor),
> -		le16_to_cpu(udev->descriptor.idProduct));
> +		le16_to_cpu(udev->descriptor.idProduct),
> +		le16_to_cpu(udev->descriptor.bcdDevice));
>  	dev_info(&udev->dev,
>  		"New USB device strings: Mfr=%d, Product=%d, SerialNumber=%d\n",
>  		udev->descriptor.iManufacturer,

Re: [PATCH] usb: also announce bcdDevice

[Greg Kroah-Hartman]

On Sat, May 05, 2012 at 02:14:43PM +0200, Martin Mokrejs wrote:
> Paul Bolle wrote:
> > Currently announce_device() does print the idVendor and idProduct values
> > but does not print the bcdDevice value. USB devices are accurately
> > identified by all three values. See, for instance, the USB storage
> > quirks which will only apply for a certain (range of) bcdDevice
> > value(s). So it seems useful to also print bcdDevice when announcing USB
> > devices.
> 
> Could it also report negotiated speed? full-speed, high-speed, super-speed?

All of this, including the bcdDevice, can be found in sysfs.  So I don't
want to take this patch, otherwise we would be just adding more and more
to the kernel log.

If you programatically want to find this out, use libudev or listen to
the dbus messages for new devices, don't watch the kernel log messages.

thanks,

greg k-h

Re: [PATCH] usb: also announce bcdDevice

[Martin Mokrejs]

Greg Kroah-Hartman wrote:
> On Sat, May 05, 2012 at 02:14:43PM +0200, Martin Mokrejs wrote:
>> Paul Bolle wrote:
>>> Currently announce_device() does print the idVendor and idProduct values
>>> but does not print the bcdDevice value. USB devices are accurately
>>> identified by all three values. See, for instance, the USB storage
>>> quirks which will only apply for a certain (range of) bcdDevice
>>> value(s). So it seems useful to also print bcdDevice when announcing USB
>>> devices.
>>
>> Could it also report negotiated speed? full-speed, high-speed, super-speed?
> 
> All of this, including the bcdDevice, can be found in sysfs.  So I don't
> want to take this patch, otherwise we would be just adding more and more
> to the kernel log.
> 
> If you programatically want to find this out, use libudev or listen to
> the dbus messages for new devices, don't watch the kernel log messages.

Hmm, but when you go through your syslog/dmesg lines especially in case of
USB devices which you swap in and out quite often, it is too late to lookup
some file elsewhere which relates to *current* device. The information
is lost already if you changed device meanwhile. You just realize USB disk
disconnected but you have not way find out except from the log files what
speed did it use. In case of USB3 devices capable of lower speeds it is
quite important. Some just claim USB3 capabilities (on the package box)
but in real, always end up at high-speed only.

For me testing several USB disks, USB to SATA bridges, host controllers,
kernel command-lines ... it is way to much work. Having the USB debug
enabled, especially XHCI_HCD_DEBUG gives on the other had too much output
for daily use but as this is all stuff prone to fail somewhere and at some
point I keep it enabled.

I don't think it clutters syslog nor that it adds significant extra size
to the output. It would save people from enabling debug just because of this.
And no, average linux user never never lookup sysfs nor use libudev, really.
Still, the link speed is of interest to everybody fiddling with the stuff
and wondering why the damn thing is so slow or why did it disconnect. It gives
an answer or at least a hint.

And if it is not enough to identify a device based on idVendor and idProduct
but one needs also bcdDevice, why not to print it?


Thanks,
Martin

Re: [PATCH] usb: also announce bcdDevice

[Alan Stern]

On Sat, 5 May 2012, Martin Mokrejs wrote:

> Greg Kroah-Hartman wrote:
> > On Sat, May 05, 2012 at 02:14:43PM +0200, Martin Mokrejs wrote:
> >> Paul Bolle wrote:
> >>> Currently announce_device() does print the idVendor and idProduct values
> >>> but does not print the bcdDevice value. USB devices are accurately
> >>> identified by all three values. See, for instance, the USB storage
> >>> quirks which will only apply for a certain (range of) bcdDevice
> >>> value(s). So it seems useful to also print bcdDevice when announcing USB
> >>> devices.
> >>
> >> Could it also report negotiated speed? full-speed, high-speed, super-speed?
> > 
> > All of this, including the bcdDevice, can be found in sysfs.  So I don't
> > want to take this patch, otherwise we would be just adding more and more
> > to the kernel log.
> > 
> > If you programatically want to find this out, use libudev or listen to
> > the dbus messages for new devices, don't watch the kernel log messages.
> 
> Hmm, but when you go through your syslog/dmesg lines especially in case of
> USB devices which you swap in and out quite often, it is too late to lookup
> some file elsewhere which relates to *current* device. The information
> is lost already if you changed device meanwhile. You just realize USB disk
> disconnected but you have not way find out except from the log files what
> speed did it use. In case of USB3 devices capable of lower speeds it is
> quite important. Some just claim USB3 capabilities (on the package box)
> but in real, always end up at high-speed only.
> 
> For me testing several USB disks, USB to SATA bridges, host controllers,
> kernel command-lines ... it is way to much work. Having the USB debug
> enabled, especially XHCI_HCD_DEBUG gives on the other had too much output
> for daily use but as this is all stuff prone to fail somewhere and at some
> point I keep it enabled.
> 
> I don't think it clutters syslog nor that it adds significant extra size
> to the output. It would save people from enabling debug just because of this.
> And no, average linux user never never lookup sysfs nor use libudev, really.
> Still, the link speed is of interest to everybody fiddling with the stuff
> and wondering why the damn thing is so slow or why did it disconnect. It gives
> an answer or at least a hint.

But the kernel _already_ logs a message listing new USB devices along 
with their speed.  In fact, it does this even if the ANNOUNCE config 
option isn't set.

Alan Stern

Re: [PATCH] usb: also announce bcdDevice

[Greg Kroah-Hartman]

On Sat, May 05, 2012 at 05:09:06PM +0200, Martin Mokrejs wrote:
> Greg Kroah-Hartman wrote:
> > On Sat, May 05, 2012 at 02:14:43PM +0200, Martin Mokrejs wrote:
> >> Paul Bolle wrote:
> >>> Currently announce_device() does print the idVendor and idProduct values
> >>> but does not print the bcdDevice value. USB devices are accurately
> >>> identified by all three values. See, for instance, the USB storage
> >>> quirks which will only apply for a certain (range of) bcdDevice
> >>> value(s). So it seems useful to also print bcdDevice when announcing USB
> >>> devices.
> >>
> >> Could it also report negotiated speed? full-speed, high-speed, super-speed?
> > 
> > All of this, including the bcdDevice, can be found in sysfs.  So I don't
> > want to take this patch, otherwise we would be just adding more and more
> > to the kernel log.
> > 
> > If you programatically want to find this out, use libudev or listen to
> > the dbus messages for new devices, don't watch the kernel log messages.
> 
> Hmm, but when you go through your syslog/dmesg lines especially in case of
> USB devices which you swap in and out quite often, it is too late to lookup
> some file elsewhere which relates to *current* device. The information
> is lost already if you changed device meanwhile. You just realize USB disk
> disconnected but you have not way find out except from the log files what
> speed did it use. In case of USB3 devices capable of lower speeds it is
> quite important. Some just claim USB3 capabilities (on the package box)
> but in real, always end up at high-speed only.

Then use a tool that tracks this type of thing (hint, it's not syslog).
That is what the patches that recently got sent to lkml are trying to
help accomplish.  It doesn't entail sending all of the device
information to the kernel log for every device, that's just noise.

> For me testing several USB disks, USB to SATA bridges, host controllers,
> kernel command-lines ... it is way to much work. Having the USB debug
> enabled, especially XHCI_HCD_DEBUG gives on the other had too much output
> for daily use but as this is all stuff prone to fail somewhere and at some
> point I keep it enabled.
> 
> I don't think it clutters syslog nor that it adds significant extra size
> to the output. It would save people from enabling debug just because of this.
> And no, average linux user never never lookup sysfs nor use libudev, really.
> Still, the link speed is of interest to everybody fiddling with the stuff
> and wondering why the damn thing is so slow or why did it disconnect. It gives
> an answer or at least a hint.
> 
> And if it is not enough to identify a device based on idVendor and idProduct
> but one needs also bcdDevice, why not to print it?

Why not print all other usb information to the syslog?  i.e. where do
you stop?  Right now, we are stopping at idVendor and idProduct, sorry.

greg k-h