[Spice-devel,spice-gtk,0/9] Windows: identify USB devices by vid:pid instead of bus.address

Submitted by Uri Lublin on March 27, 2013, 3:33 p.m.

Details

Message ID 51531135.6090302@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Uri Lublin March 27, 2013, 3:33 p.m.
On 03/25/2013 12:17 PM, Hans de Goede wrote:
> Hi,

Hi Hans,

Thanks for reviewing.

>
> On 03/25/2013 11:01 AM, Uri Lublin wrote:
>> rhbz#842816
>>
>> It seems that sometimes a USB device's bus.address is changing after
>> installation of WinUSB driver for that device.
>> So instead use vid:pid which are consistent across driver installation.
>>
>> The switch to using vid:pid is done in patch 8/9.
>>
>> Some code reuses variables named "bus" and "address" to hold vid and pid
>> values. If people think this should be changed to new variables names 
>> (ifdefed)
>> and/or new function instead of 
>> spice_usb_device_manager_get_udev_bus_n_address
>> let me know.
>>
>> Using two devices with the same vid:pid on the same Windows client is 
>> already
>> not supported, since Windows installs USB drivers for specific devices
>> based on their vid:pid
>
> Hmm, I'm not very happy with the approach you've chosen here. Just 
> because
> usbclerck / our driver swapping magic currently does not support 
> having devices
> with identical vid:pid is not a good reason to add the same problem to 
> other
> parts of the code. The contrary is true really, this is something 
> which we
> should try to fix, not make worse.

I'm only trying to fix the Windows bus.addr inconsistency problem.
I don't think that makes things worse, but better.
I agree that this patch-set inserts the existing driver limitation
(of using two devices with the same vid:pid) into spice-gtk,
for Windows (and keeps Linux as is).
This limitation already exists for Windows clients, and
we can remove it from spice-gtk in the future if needed.

>
> Have you tried using libusb_get_port_path? I know that is sensitive to
> device swap races on the same port, so how about using a combination of
> libusb_get_port_path + vid & pid to identify a device?

I did not try using libusb_get_port_path.
It may be a better solution:
  - It works the same for both Linux and Windows.
  - It fixes the Windows problem (all should be consistent through 
driver install)
  - It is a more robust solution for Linux (is it really ?).
    + Wouldn't device_address change upon device_swap ?
It's also requires a bit more memory.

We'd need to use bus_number as part of device identification.
There may be two different devices on two different buses that have
the same port_path (at least that's the case on Linux).

This seems like a feature (bug?) of libusbx:
I changed libusbx's examples/listdevs.c to print port_number and port_path
and plugged the same device on my machine's ports (diff attached):

$ ./listdevs | grep 0457
0457:0151 (bus 1, device 26 port=1) (port_path[2]={1 1 }
$ ./listdevs | grep 0457
0457:0151 (bus 2, device 7 port=1) (port_path[2]={1 1 }

So the identifier must be vid:pid, bus_number, port_path.

Thanks,
     Uri.

Patch hide | download patch | download mbox

diff --git a/examples/listdevs.c b/examples/listdevs.c
index e3d7ef0..f02c795 100644
--- a/examples/listdevs.c
+++ b/examples/listdevs.c
@@ -24,6 +24,9 @@ 
 static void print_devs(libusb_device **devs)
 {
 	libusb_device *dev;
+	uint8_t port_path[10];
+	uint8_t port_path_len = 10;
+	int npaths, j;
 	int i = 0;
 
 	while ((dev = devs[i++]) != NULL) {
@@ -34,9 +37,18 @@  static void print_devs(libusb_device **devs)
 			return;
 		}
 
-		printf("%04x:%04x (bus %d, device %d)\n",
+		npaths = libusb_get_port_path(NULL, dev, port_path, port_path_len);
+		printf("%04x:%04x (bus %d, device %d port=%d) (port_path[%d]={",
 			desc.idVendor, desc.idProduct,
-			libusb_get_bus_number(dev), libusb_get_device_address(dev));
+			libusb_get_bus_number(dev), libusb_get_device_address(dev),
+			libusb_get_port_number(dev),
+			npaths);
+
+		for (j=0; j<npaths; j++) {
+			printf("%u ", port_path[j]);
+		}
+
+		printf("}\n");
 	}
 }
 

Comments

Hi,

On 03/27/2013 04:33 PM, Uri Lublin wrote:
> On 03/25/2013 12:17 PM, Hans de Goede wrote:
>> Hi,
>
> Hi Hans,
>
> Thanks for reviewing.
>
>>
>> On 03/25/2013 11:01 AM, Uri Lublin wrote:
>>> rhbz#842816
>>>
>>> It seems that sometimes a USB device's bus.address is changing after
>>> installation of WinUSB driver for that device.
>>> So instead use vid:pid which are consistent across driver installation.
>>>
>>> The switch to using vid:pid is done in patch 8/9.
>>>
>>> Some code reuses variables named "bus" and "address" to hold vid and pid
>>> values. If people think this should be changed to new variables names (ifdefed)
>>> and/or new function instead of spice_usb_device_manager_get_udev_bus_n_address
>>> let me know.
>>>
>>> Using two devices with the same vid:pid on the same Windows client is already
>>> not supported, since Windows installs USB drivers for specific devices
>>> based on their vid:pid
>>
>> Hmm, I'm not very happy with the approach you've chosen here. Just because
>> usbclerck / our driver swapping magic currently does not support having devices
>> with identical vid:pid is not a good reason to add the same problem to other
>> parts of the code. The contrary is true really, this is something which we
>> should try to fix, not make worse.
>
> I'm only trying to fix the Windows bus.addr inconsistency problem.
> I don't think that makes things worse, but better.
> I agree that this patch-set inserts the existing driver limitation
> (of using two devices with the same vid:pid) into spice-gtk,
> for Windows (and keeps Linux as is).
> This limitation already exists for Windows clients, and
> we can remove it from spice-gtk in the future if needed.

Ok, so lets split the discussion in 2:

1) What do we need today

Today we need a way to work around the bus.addr pair not being stable
for a single device under Windows, since today we already do NOT
support using multiple devices with the same vid:pid, using vid:pid
(iow this patchset) would be an acceptable solution, but only today!

2) Where do we want to be tomorrow

"Tomorrow" we will hopefully be using filter drivers, and hopefully
then we don't need the whole driver install dance (even for new devices
which were never plugged in before?), and then we can remove a whole
bunch of windows specific "murk" from the spice-gtk code for usb
handling.

So if we can agree on this being a temporary thing, then I can agree
with the vid:pid approach *for now*, but this really really needs to
be replaced / fixed in the near future.

So with this clear, and assuming you will agree, let me actually really
review this patch-set :)


Some remarks on alternative approaches below, but lets just go with
vid:pid for now.

>
>>
>> Have you tried using libusb_get_port_path? I know that is sensitive to
>> device swap races on the same port, so how about using a combination of
>> libusb_get_port_path + vid & pid to identify a device?
>
> I did not try using libusb_get_port_path.
> It may be a better solution:
>   - It works the same for both Linux and Windows.
>   - It fixes the Windows problem (all should be consistent through driver install)
It may seem to fix the Windows problem, but if a device gets removed, and
then replaced by another one while the driver install is happening, then
just using the port-path is not enough, we could combine it with vid:pid,
but then 2 identical devices (from a vid:pid) pov get swapped, we would
miss that, so this is still not ideal.

>   - It is a more robust solution for Linux (is it really ?).
>     + Wouldn't device_address change upon device_swap ?

bus.addr is 100% reliable on Linux, replacing it with bus + port-path
introduces the issues we're having on Windows without any gains (other then
unification of the code.

> It's also requires a bit more memory.
>
> We'd need to use bus_number as part of device identification.
> There may be two different devices on two different buses that have
> the same port_path (at least that's the case on Linux).

Ah, good one, so you need bus_number + port_path to uniquely identify
a port, note this is identifying a port, not what is plugged in hence
the need to also check vid:pid.

Regards,

Hans
On 03/27/2013 06:13 PM, Hans de Goede wrote:
> Hi,
>
> On 03/27/2013 04:33 PM, Uri Lublin wrote:
>> On 03/25/2013 12:17 PM, Hans de Goede wrote:
>>> Hi,
>>
>> Hi Hans,
>>
>> Thanks for reviewing.
>>
>>>
>>> On 03/25/2013 11:01 AM, Uri Lublin wrote:
>>>> rhbz#842816
>>>>
>>>> It seems that sometimes a USB device's bus.address is changing after
>>>> installation of WinUSB driver for that device.
>>>> So instead use vid:pid which are consistent across driver 
>>>> installation.
>>>>
>>>> The switch to using vid:pid is done in patch 8/9.
>>>>
>>>> Some code reuses variables named "bus" and "address" to hold vid 
>>>> and pid
>>>> values. If people think this should be changed to new variables 
>>>> names (ifdefed)
>>>> and/or new function instead of 
>>>> spice_usb_device_manager_get_udev_bus_n_address
>>>> let me know.
>>>>
>>>> Using two devices with the same vid:pid on the same Windows client 
>>>> is already
>>>> not supported, since Windows installs USB drivers for specific devices
>>>> based on their vid:pid
>>>
>>> Hmm, I'm not very happy with the approach you've chosen here. Just 
>>> because
>>> usbclerck / our driver swapping magic currently does not support 
>>> having devices
>>> with identical vid:pid is not a good reason to add the same problem 
>>> to other
>>> parts of the code. The contrary is true really, this is something 
>>> which we
>>> should try to fix, not make worse.
>>
>> I'm only trying to fix the Windows bus.addr inconsistency problem.
>> I don't think that makes things worse, but better.
>> I agree that this patch-set inserts the existing driver limitation
>> (of using two devices with the same vid:pid) into spice-gtk,
>> for Windows (and keeps Linux as is).
>> This limitation already exists for Windows clients, and
>> we can remove it from spice-gtk in the future if needed.
>
> Ok, so lets split the discussion in 2:
>
> 1) What do we need today
>
> Today we need a way to work around the bus.addr pair not being stable
> for a single device under Windows, since today we already do NOT
> support using multiple devices with the same vid:pid, using vid:pid
> (iow this patchset) would be an acceptable solution, but only today!

great.

>
> 2) Where do we want to be tomorrow
>
> "Tomorrow" we will hopefully be using filter drivers, and hopefully
> then we don't need the whole driver install dance (even for new devices
> which were never plugged in before?), and then we can remove a whole
> bunch of windows specific "murk" from the spice-gtk code for usb
> handling.

Yes, we want to use a filter driver which needs to be installed only once,
and works for all/more USB devices. We wanted filter driver from
the start, but it was not in good enough a condition.

Once we can use a filter driver, we can remove all the code
that asks usbclerk to install the driver for specific devices
(and of course we will not need usbclerk at all anymore)

>
> So if we can agree on this being a temporary thing, then I can agree
> with the vid:pid approach *for now*, but this really really needs to
> be replaced / fixed in the near future.

I agree. Once the filter driver is good enough we'll switch to using it.
When using a filter driver we can identify devices with bus.addr
on Windows too.

>
> So with this clear, and assuming you will agree, let me actually really
> review this patch-set :)

Thanks.

>
>
> Some remarks on alternative approaches below, but lets just go with
> vid:pid for now.
>
<snipped>
> bus.addr is 100% reliable on Linux, replacing it with bus + port-path
> introduces the issues we're having on Windows without any gains (other 
> then
> unification of the code.

Hopefully with the filter driver, bus.addr is going to be reliable for 
Windows too.

Thanks,
     Uri.