Re: [PATCH QEMU HCI-8.0 v8 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-incoming workaround skips only one blocking call, with no root cause
- [Low] usb-host: cpr_is_incoming() is a global check, so unrelated opens lose product name
--

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

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

This patch skips the libusb_get_string_descriptor_ascii() call in
usb_host_open() when cpr_is_incoming() is true. In that case product_desc
falls back to the synthetic "host:bus.addr" name. The stated reason is that
the synchronous control transfer can block during CPR handover and hang the
incoming migration.

> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index 9cc7d72aa63a..6c00ce528b1e 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c
> @@ -1123,7 +1123,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()) {
>          libusb_get_string_descriptor_ascii(s->dh, s->ddesc.iProduct,
>                                             (unsigned char *)udev->product_desc,
>                                             sizeof(udev->product_desc));

[Severity: Low]
Is skipping only this one transfer enough to avoid the hang? The commit
message says the transfer "can block there, hanging the incoming
migration", but no root cause is given.

If the cause is the device handover state, other calls in usb_host_open()
during CPR incoming could block in the same way:

usb_host_open()
    libusb_wrap_sys_device() / libusb_open()
    usb_host_detach_kernel()
    libusb_get_device_descriptor()
    usb_host_speed_compat()
        libusb_get_config_descriptor()
    ioctl(hostfd, USBDEVFS_GET_SPEED, NULL)
    usb_device_attach()

Could the root cause be described, and a comment added at this guard
explaining why only the product string read is skipped?

Also, nothing appears to re-read the real product string after CPR
completes. Does product_desc then stay as "host:bus.addr" where the source
showed the real name?

[Severity: Low]
This isn't a functional bug, but cpr_is_incoming() is a process-global check
(incoming_mode != MIG_MODE_NONE), not a per-device indicator that the device
was inherited via CPR.

incoming_mode is cleared only by cpr_set_incoming_mode(MIG_MODE_NONE) in
migration_incoming_state_destroy(). So the window is wider than the device
handover.

Can this also affect other opens during that window? For example,
usb_host_open() reached from usb_host_auto_check() or from the realize
paths would skip the product read for a device that was not handed over by
CPR. Such a device would get the synthetic "host:bus.addr" product_desc,
which is visible in info usb, in log messages and in traces.

The commit message does not mention this wider effect. Would a per-device
condition be more appropriate?

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