[PATCH] usb-host: enable autoscan for bus+addr to survive host suspend/resume

[PATCH] usb-host: enable autoscan for bus+addr to survive host suspend/resume

[Yuri Nesterov]

Currently, there is a special case for usb-host devices added using the
hostbus= and hostaddr= properties to avoid adding them to the hotplug
watchlist, since the address changes every time the device is plugged
in. However, on Linux, when the host system goes into suspend and then
resumes, those devices stop working in both the guest and the host.

Enabling autoscan and adding those devices to the watchlist allows them
to keep working in the guest after host suspend/resume.

Signed-off-by: Yuri Nesterov <yuri.nesterov@gmail.com>
---
 hw/usb/host-libusb.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index c3d642c9d3..32c0251471 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1227,7 +1227,7 @@ static void usb_host_realize(USBDevice *udev, Error **errp)
         !s->match.vendor_id &&
         !s->match.product_id &&
         !s->match.port) {
-        s->needs_autoscan = false;
+        s->needs_autoscan = true;
         ldev = usb_host_find_ref(s->match.bus_num,
                                  s->match.addr);
         if (!ldev) {
@@ -1244,6 +1244,9 @@ static void usb_host_realize(USBDevice *udev, Error **errp)
         }
     } else {
         s->needs_autoscan = true;
+    }
+
+    if (s->needs_autoscan) {
         QTAILQ_INSERT_TAIL(&hostdevs, s, next);
         usb_host_auto_check(NULL);
     }
-- 
2.43.0

Re: [PATCH] usb-host: enable autoscan for bus+addr to survive host suspend/resume

[Daniel P. Berrangé]

On Wed, Apr 16, 2025 at 07:19:29PM +0300, Yuri Nesterov wrote:
> Currently, there is a special case for usb-host devices added using the
> hostbus= and hostaddr= properties to avoid adding them to the hotplug
> watchlist, since the address changes every time the device is plugged
> in. However, on Linux, when the host system goes into suspend and then
> resumes, those devices stop working in both the guest and the host.
> 
> Enabling autoscan and adding those devices to the watchlist allows them
> to keep working in the guest after host suspend/resume.

So IIUC what you're saying is that on suspend the host device
is removed by the kernel, and on resume, the USB device is
recreated. So QEMU's open file handle for the USB device is
invalid after resume.

If the /dev/bus/usb/NNN/NNN file goes away and then gets
re-created by the kernel though, we can't assume QEMU is
going to be able to re-open the new /dev/bus/usb device
file though.

When QEMU runs under libvirt, QEMU won't have any access
to the /dev/bus/usb device node unless libvirt has set the
right permissions and (where appropriate) also set the
SELinux label.

The current autoscan logic seems rather crude. AFAIK every 2
seconds it will re-scan every host USB device to see if any
has gone away and close it, and/or re-open if re-appeared.

If we enable this autoscan logic, then under libvirt, AFAICS,
QEMU is going to fail to re-open the device, and a counter in
the autoscan logic means that after 3 attempts to re-open QEMU
will stop attempting it at all...but strangely it appears QEMU
will keep the timer running, so every 2 seconds it will iterate
over every USB device, but never try to open any of them.

Regardless of your current patch this autoscan logic feels like
it needs improvement. We shouldn't need to busy-poll to see that
a USB device goes away, it should be possible to rely on event
notifications in some way.

Even then we'd still need a way to prevent this immediate auto
re-opening when under libvirt. Libvirt would have to detect the
reappearance of the device and perform relabelling and permissions
changes, before QEMU is able to be told to try to re-open.
Potentially this implies a new QMP command to tell QEMU to try
to re-open, unless perhaps QEMU can be triggered off an inotify
event for the permissions/label change of the device node.

> 
> Signed-off-by: Yuri Nesterov <yuri.nesterov@gmail.com>
> ---
>  hw/usb/host-libusb.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index c3d642c9d3..32c0251471 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c
> @@ -1227,7 +1227,7 @@ static void usb_host_realize(USBDevice *udev, Error **errp)
>          !s->match.vendor_id &&
>          !s->match.product_id &&
>          !s->match.port) {
> -        s->needs_autoscan = false;
> +        s->needs_autoscan = true;
>          ldev = usb_host_find_ref(s->match.bus_num,
>                                   s->match.addr);
>          if (!ldev) {
> @@ -1244,6 +1244,9 @@ static void usb_host_realize(USBDevice *udev, Error **errp)
>          }
>      } else {
>          s->needs_autoscan = true;
> +    }
> +
> +    if (s->needs_autoscan) {
>          QTAILQ_INSERT_TAIL(&hostdevs, s, next);
>          usb_host_auto_check(NULL);
>      }
> -- 
> 2.43.0
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] usb-host: enable autoscan for bus+addr to survive host suspend/resume

[Yuri Nesterov]

On Wed, Apr 16, 2025 at 8:07 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Apr 16, 2025 at 07:19:29PM +0300, Yuri Nesterov wrote:
> > Currently, there is a special case for usb-host devices added using the
> > hostbus= and hostaddr= properties to avoid adding them to the hotplug
> > watchlist, since the address changes every time the device is plugged
> > in. However, on Linux, when the host system goes into suspend and then
> > resumes, those devices stop working in both the guest and the host.
> >
> > Enabling autoscan and adding those devices to the watchlist allows them
> > to keep working in the guest after host suspend/resume.
>
> So IIUC what you're saying is that on suspend the host device
> is removed by the kernel, and on resume, the USB device is
> recreated. So QEMU's open file handle for the USB device is
> invalid after resume.
>
> If the /dev/bus/usb/NNN/NNN file goes away and then gets
> re-created by the kernel though, we can't assume QEMU is
> going to be able to re-open the new /dev/bus/usb device
> file though.

I'm not sure if the file actually goes away. It looks like the internal
state of the device changes and QEMU receives a "no device"
response in usb_host_req_complete_data. However, the file
remains in place. At least I don't see any changes in udevadm
monitor or inotifywait aside from attribute modifications.

After resuming from suspend, the device doesn't work on either
host or guest. Probably the device stays with a detached kernel
driver since QEMU can't reattach it after receiving the "no device"
error. Adding such devices to the hotplug watchlist causes QEMU
to reopen them the same way it does for devices specified by
vendorid and productid or hostport.

The reason bus+addr devices aren't currently added to that list is well
explained in commit e058fa2dd599ccc780d334558be9c1d155222b80.
A special case was made because the device address changes every
time it's replugged. However, it turns out that it doesn't change after
a suspend/resume cycle so adding them to the list allows them to
keep working after resume.

I haven't had a chance to test it with libvirt though.

> When QEMU runs under libvirt, QEMU won't have any access
> to the /dev/bus/usb device node unless libvirt has set the
> right permissions and (where appropriate) also set the
> SELinux label.
>
> The current autoscan logic seems rather crude. AFAIK every 2
> seconds it will re-scan every host USB device to see if any
> has gone away and close it, and/or re-open if re-appeared.
>
> If we enable this autoscan logic, then under libvirt, AFAICS,
> QEMU is going to fail to re-open the device, and a counter in
> the autoscan logic means that after 3 attempts to re-open QEMU
> will stop attempting it at all...but strangely it appears QEMU
> will keep the timer running, so every 2 seconds it will iterate
> over every USB device, but never try to open any of them.
>
> Regardless of your current patch this autoscan logic feels like
> it needs improvement. We shouldn't need to busy-poll to see that
> a USB device goes away, it should be possible to rely on event
> notifications in some way.
>
> Even then we'd still need a way to prevent this immediate auto
> re-opening when under libvirt. Libvirt would have to detect the
> reappearance of the device and perform relabelling and permissions
> changes, before QEMU is able to be told to try to re-open.
> Potentially this implies a new QMP command to tell QEMU to try
> to re-open, unless perhaps QEMU can be triggered off an inotify
> event for the permissions/label change of the device node.
>
> >
> > Signed-off-by: Yuri Nesterov <yuri.nesterov@gmail.com>
> > ---
> >  hw/usb/host-libusb.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> > index c3d642c9d3..32c0251471 100644
> > --- a/hw/usb/host-libusb.c
> > +++ b/hw/usb/host-libusb.c
> > @@ -1227,7 +1227,7 @@ static void usb_host_realize(USBDevice *udev, Error **errp)
> >          !s->match.vendor_id &&
> >          !s->match.product_id &&
> >          !s->match.port) {
> > -        s->needs_autoscan = false;
> > +        s->needs_autoscan = true;
> >          ldev = usb_host_find_ref(s->match.bus_num,
> >                                   s->match.addr);
> >          if (!ldev) {
> > @@ -1244,6 +1244,9 @@ static void usb_host_realize(USBDevice *udev, Error **errp)
> >          }
> >      } else {
> >          s->needs_autoscan = true;
> > +    }
> > +
> > +    if (s->needs_autoscan) {
> >          QTAILQ_INSERT_TAIL(&hostdevs, s, next);
> >          usb_host_auto_check(NULL);
> >      }
> > --
> > 2.43.0
> >
> >
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>

Re: [PATCH] usb-host: enable autoscan for bus+addr to survive host suspend/resume

[Daniel P. Berrangé]

On Wed, Apr 16, 2025 at 10:27:54PM +0300, Yuri Nesterov wrote:
> On Wed, Apr 16, 2025 at 8:07 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Wed, Apr 16, 2025 at 07:19:29PM +0300, Yuri Nesterov wrote:
> > > Currently, there is a special case for usb-host devices added using the
> > > hostbus= and hostaddr= properties to avoid adding them to the hotplug
> > > watchlist, since the address changes every time the device is plugged
> > > in. However, on Linux, when the host system goes into suspend and then
> > > resumes, those devices stop working in both the guest and the host.
> > >
> > > Enabling autoscan and adding those devices to the watchlist allows them
> > > to keep working in the guest after host suspend/resume.
> >
> > So IIUC what you're saying is that on suspend the host device
> > is removed by the kernel, and on resume, the USB device is
> > recreated. So QEMU's open file handle for the USB device is
> > invalid after resume.
> >
> > If the /dev/bus/usb/NNN/NNN file goes away and then gets
> > re-created by the kernel though, we can't assume QEMU is
> > going to be able to re-open the new /dev/bus/usb device
> > file though.
> 
> I'm not sure if the file actually goes away. It looks like the internal
> state of the device changes and QEMU receives a "no device"
> response in usb_host_req_complete_data. However, the file
> remains in place. At least I don't see any changes in udevadm
> monitor or inotifywait aside from attribute modifications.
> 
> After resuming from suspend, the device doesn't work on either
> host or guest. Probably the device stays with a detached kernel
> driver since QEMU can't reattach it after receiving the "no device"
> error. Adding such devices to the hotplug watchlist causes QEMU
> to reopen them the same way it does for devices specified by
> vendorid and productid or hostport.

This is a bit odd, as AFAICT from reading the code, the
usb_host_auto_check wll only trigger close + re-open of
the device, if there is a period of time in which the
/dev/bus/usb device node does not exist, but you're
saying it remains existing across suspend/resume.

> 
> The reason bus+addr devices aren't currently added to that list is well
> explained in commit e058fa2dd599ccc780d334558be9c1d155222b80.
> A special case was made because the device address changes every
> time it's replugged. However, it turns out that it doesn't change after
> a suspend/resume cycle so adding them to the list allows them to
> keep working after resume.
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] usb-host: enable autoscan for bus+addr to survive host suspend/resume

[Yuri Nesterov]

On Thu, Apr 17, 2025 at 12:41 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Apr 16, 2025 at 10:27:54PM +0300, Yuri Nesterov wrote:
> > On Wed, Apr 16, 2025 at 8:07 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > >
> > > On Wed, Apr 16, 2025 at 07:19:29PM +0300, Yuri Nesterov wrote:
> > > > Currently, there is a special case for usb-host devices added using the
> > > > hostbus= and hostaddr= properties to avoid adding them to the hotplug
> > > > watchlist, since the address changes every time the device is plugged
> > > > in. However, on Linux, when the host system goes into suspend and then
> > > > resumes, those devices stop working in both the guest and the host.
> > > >
> > > > Enabling autoscan and adding those devices to the watchlist allows them
> > > > to keep working in the guest after host suspend/resume.
> > >
> > > So IIUC what you're saying is that on suspend the host device
> > > is removed by the kernel, and on resume, the USB device is
> > > recreated. So QEMU's open file handle for the USB device is
> > > invalid after resume.
> > >
> > > If the /dev/bus/usb/NNN/NNN file goes away and then gets
> > > re-created by the kernel though, we can't assume QEMU is
> > > going to be able to re-open the new /dev/bus/usb device
> > > file though.
> >
> > I'm not sure if the file actually goes away. It looks like the internal
> > state of the device changes and QEMU receives a "no device"
> > response in usb_host_req_complete_data. However, the file
> > remains in place. At least I don't see any changes in udevadm
> > monitor or inotifywait aside from attribute modifications.
> >
> > After resuming from suspend, the device doesn't work on either
> > host or guest. Probably the device stays with a detached kernel
> > driver since QEMU can't reattach it after receiving the "no device"
> > error. Adding such devices to the hotplug watchlist causes QEMU
> > to reopen them the same way it does for devices specified by
> > vendorid and productid or hostport.
>
> This is a bit odd, as AFAICT from reading the code, the
> usb_host_auto_check wll only trigger close + re-open of
> the device, if there is a period of time in which the
> /dev/bus/usb device node does not exist, but you're
> saying it remains existing across suspend/resume.

I guess it depends on how it's configured, both in hardware
and software. I'm testing on a regular ThinkPad with Ubuntu
and default settings. It's probably possible to configure USB
to turn off power during suspend. In that case, it might
reappear after resume with a new address. However, with
the default settings, it seems to keep the same device node
and address. I assume the driver just switches it to
low-power mode and then back to normal.

In usb_host_auto_check, it reopens the device when the
handle s->dh is NULL. This happens after
usb_host_req_complete_data receives the "no device" status
and closes the device. Maybe one of the kernel functions
returns -ENODEV at some point, but after that, the device
continues to work fine.