[Spice-devel] [PATCH spice-gtk] win-usb-dev: fix device arrival event logic

[Uri Lublin]

Hi,

On 07/03/2017 06:53 PM, Yuri Benditovich wrote:
> It does not exactly removes old bugfix.

Possibly it does, but only if WinUsb (+ usbclerk) are used.

> First version of the code did check for bus.addr only and for example, 
> ignored change of vid/pid without change of bus.addr (AFAIK this is what 
> programmable devices like Cypress do).

Yes, this is the preferred method to identify a specific device in the
system, when not using WinUSB.

> Previous commit removed check for bus.addr and replaced it with check 
> for vid and pid, effectively causing the bug we are fixing now.

Said commit fixed a bug (feature?) that only occurs with WinUSb
(at least at the time it was committed). The bus.addr id
before/after installing the WinUSB driver may be different.
With WinUSB, the driver needs to be installed separately
for every device that is redirected. This sometimes caused
the bus.addr to change.
That means one can not rely on bus.addr to identify a specific device.
The WinUSB driver would only be installed once the device
is to be usbredir'ed with the guest (that was done via usbclerk).
Using vid:pid is a good identifier in such a scenario, but
of course can only support a single such vid:pid device
per client.

Since commit 66c705caca74f61ffce8637bee38b1e5e4909062,
WinUSB is not automatically installed via usbclerk.
We recommend using UsbDk.

> Now we check all of them and ignore the update event only if there is no 
> change in USB tree.
Alternatively you can simply revert the aforementioned patch.
You do not need to check vid/pid (should not harm either)

> 
> I'll mention the old commit in new commit message.
> I'll also ask the author of previous commit for comment.

I agree with Christophe that a comment mentioning
old commit and the behavior difference between WinUSB
and UsbDk would be helpful.

Regards,
     Uri.


> 
> On Mon, Jul 3, 2017 at 6:31 PM, Christophe Fergeau <cfergeau at redhat.com 
> <mailto:cfergeau at redhat.com>> wrote:
> 
>     On Mon, Jul 03, 2017 at 02:27:29PM +0300, Yuri Benditovich wrote:
>      > On Mon, Jul 3, 2017 at 10:16 AM, Christophe Fergeau
>     <cfergeau at redhat.com <mailto:cfergeau at redhat.com>>
>      > wrote:
>      >
>      > > On Mon, Jul 03, 2017 at 07:48:31AM +0300, Yuri Benditovich wrote:
>      > > > https://bugzilla.redhat.com/show_bug.cgi?id=1425961
>     <https://bugzilla.redhat.com/show_bug.cgi?id=1425961>
>      > > > If attached new device when one device with the same vid
>      > > > and pid already present, the notification is ignored and
>      > > > attached device is not redirected (if auto share set) and
>      > > > not displayed in USB devices widget
>      > >
>      > > There apparently were some issues in the past with bus/addr
>     changing
>      > > when it should not
>      > >
>     https://cgit.freedesktop.org/spice/spice-gtk/commit/?id=f9631cd6f8
>     <https://cgit.freedesktop.org/spice/spice-gtk/commit/?id=f9631cd6f8>
>      > >
>      > > Any idea whether this is no longer needed?
>      > >
>      >
>      > There is no additional information about case when the same
>     device comes
>      > with different bus.addr
>      > 1. From my point of view this should not be a problem - if new
>     device with
>      > different bus.addr comes in, the previous one with
>      > old bus.addr should disappear and be removed anywhere ; new
>     device shall be
>      > redirected automatically if required.
>      > If such flow will be identified/reported with UsbDk, we will be
>     able to
>      > investigate it and solve.
>      > 2. Whether the configuration with WinUSB is still used by
>     spice-gtk on
>      > Windows? According to instructions
>      > on https://www.spice-space.org/spice-user-manual.html
>     <https://www.spice-space.org/spice-user-manual.html> , UsbDk should be
>      > used and WinUSB is not mentioned.
> 
>     To be honest, I don't know what WinUSB is.. ;) But since this patch is
>     removing an (old) bugfix, I'd explain in the commit log why it's no
>     longer necessary to have it.
> 
>     Christophe
> 
> 
> 
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>