| Message ID | 1381262298-3241-4-git-send-email-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Di, 2013-10-08 at 21:58 +0200, Hans de Goede wrote: > If we detach the kernel drivers on the first set_config, then they will > be still attached when the device gets its initial reset. Causing the drivers > to re-initialize the device after the reset, dirtying the device state. > @@ -1051,7 +1055,6 @@ static void usb_host_set_config(USBHostDevice *s, int config, USBPacket *p) > trace_usb_host_set_config(s->bus_num, s->addr, config); > > usb_host_release_interfaces(s); > - usb_host_detach_kernel(s); > rc = libusb_set_configuration(s->dh, config); > if (rc != 0) { > usb_host_libusb_error("libusb_set_configuration", rc); Sure we can safely remove the detach_kernel here? Assuming we have a device with multiple configurations, each configuration has a different set of interfaces, guest switches from one config to another. Do we correctly unbind kernel / claim interfaces then? Handling this correctly was the reason to do the kernel unbind in response to the guests set_config instead of host_open btw, although I can see the issues the late unbind introduces. cheers, Gerd
Hi, On 10/09/2013 10:55 AM, Gerd Hoffmann wrote: > On Di, 2013-10-08 at 21:58 +0200, Hans de Goede wrote: >> If we detach the kernel drivers on the first set_config, then they will >> be still attached when the device gets its initial reset. Causing the drivers >> to re-initialize the device after the reset, dirtying the device state. > >> @@ -1051,7 +1055,6 @@ static void usb_host_set_config(USBHostDevice *s, int config, USBPacket *p) >> trace_usb_host_set_config(s->bus_num, s->addr, config); >> >> usb_host_release_interfaces(s); >> - usb_host_detach_kernel(s); >> rc = libusb_set_configuration(s->dh, config); >> if (rc != 0) { >> usb_host_libusb_error("libusb_set_configuration", rc); > > Sure we can safely remove the detach_kernel here? Yes. > Assuming we have a device with multiple configurations, each > configuration has a different set of interfaces, guest switches from one > config to another. Do we correctly unbind kernel / claim interfaces > then? Yes we still have a usb_host_detach_kernel() call in the beginning of usb_host_claim_interfaces(), which gets run on the new interfaces immediately after the libusb_set_configuration call. Doing a detach_kernel after a set_config has always been necessary, since the kernel will automatically bind drivers to the new interfaces if the set_config succeeds. Doing detach_kernel before set_config is necessary if kernel drivers are attached, because the kernel will not allow a set_config if any drivers are bound. But we already do a set_config for the current config on usb_host_open() now, and if a new config gets installed we immediately detach the kernel drivers again from usb_host_claim_interfaces() so when we enter usb_host_set_config no kernel drivers, other then usbfs (the userspace access driver) will be attached, and usbfs is detached by releasing the interfaces (*). Regards, Hans *) In recent libusb versions libusb will even disallow using libusb_detach_kernel_driver to detach usbfs, so as to stop a user space program from breaking anothers userspaces program claim on the interface by completelty detaching usbfs from the interface.
Hi, > > Assuming we have a device with multiple configurations, each > > configuration has a different set of interfaces, guest switches from one > > config to another. Do we correctly unbind kernel / claim interfaces > > then? > > Yes we still have a usb_host_detach_kernel() call in the beginning > of usb_host_claim_interfaces(), which gets run on the new interfaces > immediately after the libusb_set_configuration call. Ok, good. > Doing a detach_kernel after a set_config has always been necessary, > since the kernel will automatically bind drivers to the new interfaces > if the set_config succeeds. Is there some way to avoid the kernel's autobind in the first place btw? cheers, Gerd
Hi, On 10/09/2013 03:35 PM, Gerd Hoffmann wrote: > Hi, > >>> Assuming we have a device with multiple configurations, each >>> configuration has a different set of interfaces, guest switches from one >>> config to another. Do we correctly unbind kernel / claim interfaces >>> then? >> >> Yes we still have a usb_host_detach_kernel() call in the beginning >> of usb_host_claim_interfaces(), which gets run on the new interfaces >> immediately after the libusb_set_configuration call. > > Ok, good. > >> Doing a detach_kernel after a set_config has always been necessary, >> since the kernel will automatically bind drivers to the new interfaces >> if the set_config succeeds. > > Is there some way to avoid the kernel's autobind in the first place btw? Not atm, but so far the kernel guys have been open to adding (sane) API's for things like this, and avoiding the whole re-bind after a set_config from userspace would probably be nice to have. Note that this is not triggering in 99% of all cases though, as the kernel has this bit of code in its set_config handling for usbfs: /* SET_CONFIGURATION is often abused as a "cheap" driver reset, * so avoid usb_set_configuration()'s kick to sysfs */ if (actconfig && actconfig->desc.bConfigurationValue == u) status = usb_reset_configuration(ps->dev); else status = usb_set_configuration(ps->dev, u); So it only actually does a set_config (and binds the kernel drivers to the interfaces) if the guest is asking for another config then the host os has chosen for the device. Since the guest assumes the device starts unconfigured, it does not issue a set_config(0), only a set_config(1) (usually) which the above code turns into a light weight reset. Note that my usbredirhost code avoids even the unclaim / claim dance around set_config and calling into the kernel at all in this case, it has: if (host->config && host->config->bConfigurationValue == config) { goto exit; } A downside of this is that not even the lightweight device reset is done, but the guest always starts with a full device-reset, immediately following that with a lightweight reset has little value. I think we could and should to the same in host-libusb.c. Regards, Hans
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c index 35bae55..fd320cd 100644 --- a/hw/usb/host-libusb.c +++ b/hw/usb/host-libusb.c @@ -137,6 +137,7 @@ static QTAILQ_HEAD(, USBHostDevice) hostdevs = static void usb_host_auto_check(void *unused); static void usb_host_release_interfaces(USBHostDevice *s); static void usb_host_nodev(USBHostDevice *s); +static void usb_host_detach_kernel(USBHostDevice *s); static void usb_host_attach_kernel(USBHostDevice *s); /* ------------------------------------------------------------------------ */ @@ -787,10 +788,13 @@ static int usb_host_open(USBHostDevice *s, libusb_device *dev) goto fail; } - libusb_get_device_descriptor(dev, &s->ddesc); s->dev = dev; s->bus_num = bus_num; s->addr = addr; + + usb_host_detach_kernel(s); + + libusb_get_device_descriptor(dev, &s->ddesc); usb_host_get_port(s->dev, s->port, sizeof(s->port)); usb_ep_init(udev); @@ -1051,7 +1055,6 @@ static void usb_host_set_config(USBHostDevice *s, int config, USBPacket *p) trace_usb_host_set_config(s->bus_num, s->addr, config); usb_host_release_interfaces(s); - usb_host_detach_kernel(s); rc = libusb_set_configuration(s->dh, config); if (rc != 0) { usb_host_libusb_error("libusb_set_configuration", rc);
If we detach the kernel drivers on the first set_config, then they will be still attached when the device gets its initial reset. Causing the drivers to re-initialize the device after the reset, dirtying the device state. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- hw/usb/host-libusb.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)