Re: [PATCH QEMU HCI-8.0 v7 08/14] usb-host: skip product-string read on CPR incoming #VSTOR-137800

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] usb-host: cpr_is_incoming() guard drops product string for any open
- [Low] usb-host: only the string read is skipped; other control I/O in open still runs
--

commit e0b84d4b8555ddc11f7b8275a12f5907c2ba320f
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

usb-host: skip product-string read on CPR incoming #VSTOR-137800

This patch skips the libusb product string descriptor read in
usb_host_open() when cpr_is_incoming() is true. It uses the synthetic
"host:bus.addr" name for product_desc instead.

> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index d90117e4292d..271456abb0a1 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c

[ ... ]

> @@ -1111,7 +1111,7 @@ static int usb_host_open(USBHostDevice *s, libusb_device *dev, int hostfd)
>      udev->speed = speed_map[libusb_speed];
>      usb_host_speed_compat(s);
>  
> -    if (s->ddesc.iProduct) {
> +    if (s->ddesc.iProduct && !cpr_is_incoming()) {

[Severity: Low]
Is this guard too broad? cpr_is_incoming() is defined as
"incoming_mode != MIG_MODE_NONE", so it is true for the whole
incoming-migration phase. It is not a per-device check.

usb_host_open() is reached from several paths:

    usb_host_realize()    (hostfd and bus/addr paths)
    usb_host_auto_check() (autoscan)

Any device opened while the flag is set takes the synthetic branch. That
includes autoscan or hotplug matches of devices that are not being handed
over.

product_desc is set once in usb_host_open() and is not refreshed. Can
those devices end up keeping the "host:bus.addr" name after the migration
completes?

[Severity: Low]
Is the product string read the only call in usb_host_open() that can block
during the handover? The neighbouring calls still run unguarded:

    usb_host_detach_kernel(s);
    ...
    usb_host_speed_compat(s);    /* libusb_get_config_descriptor() */

The fail: path also calls libusb_reset_device().

If synchronous control transfers can block on the CPR target, could the
incoming migration still hang in one of these calls? The commit message
doesn't say what causes the blocking, so it is unclear whether skipping
only the string read is enough.

>          libusb_get_string_descriptor_ascii(s->dh, s->ddesc.iProduct,
>                                             (unsigned char *)udev->product_desc,
>                                             sizeof(udev->product_desc));

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-usb-host_v3-v7-0-044f668789a1@virtuozzo.com?part=8