Re: [PATCH QEMU HCI-8.0 v6 07/12] 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 in usb_host_open() skips product string for all opens
- [Low] usb-host: CPR product-string skip is an uncommented workaround for an unexplained hang
--

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

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

This patch skips the synchronous libusb product string read in
usb_host_open() when the process is the CPR target. In that case 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 85509dc2d83e..020a685aa419 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c

[ ... ]

> @@ -1106,7 +1106,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 broader than the CPR hostfd hand-over it is meant to
cover?

cpr_is_incoming() returns incoming_mode != MIG_MODE_NONE, which is a
process-wide flag that stays set for the whole incoming migration. It
is not a per-device indicator.

Any usb_host_open() call in that window therefore takes the synthetic
name branch. That includes the autoscan path:

usb_host_auto_check()
    usb_host_open(s, devs[i], 0)

It also includes the explicit bus/addr realize path. Neither of these
is the hostfd hand-over described in the commit message.

Since product_desc is only written in usb_host_open(), would those
devices keep the "host:bus.addr" name after migration completes?

Could the check be narrowed to the device that was actually handed over
by CPR, for example by keying off hostfd?

[Severity: Low]
This isn't a functional problem, but could a comment be added here
explaining why the product string read is skipped on the CPR target?

The commit message says only that the transfer "can block". The cause
is not recorded in the code, and other cpr_is_incoming() checks in this
file carry comments.

It is also unclear whether the other libusb calls that still run in
usb_host_open() before usb_device_attach() can block in the same way:

libusb_wrap_sys_device()
usb_host_detach_kernel()
usb_host_ep_update()
usb_host_speed_compat()

>          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-v6-0-8688ec9576d1@virtuozzo.com?part=7