On Wed, 26 Nov 2025 at 11:20, Yang, Liang1 <[email protected]> wrote:
> From: Peter Maydell <[email protected]>
> > + warn_report("usb-host: ignoring invalid configuration "
> > + "for device %s (bus=%03d, addr=%03d)",
> > + udev->product_desc ? udev->product_desc : "unknown",
> > + s->bus_num, s->addr);
> > + return;
>
> This is something we already get wrong in a lot of the error-exit paths of
> this function, but we need to free the conf descriptor with
> libusb_free_config_descriptor().
> (If you wanted to include a patch to fix those existing leaks that would be
> great.)
>
> YL: I see, let me check and see if can create a patch to fix those leaks.
>
> We already ignored a bNumInterfaces==0 config (because the rest of the
> function would just do nothing), so the only change here is to warn about it.
>
> (1) Have you seen the bNumInterfaces==0 situation in real life?
>
> YL: No, I have not seen (or noticed) this in real testing, but I think it's
> good to have especially during very fast USB hot-unplug/hot-replug.
If you haven't seen it, why worry about it? We won't crash if it
happens, we'll just ignore the device.
> (2) This is unrelated to the other half of the patch, so it should be its own
> patch (i.e. one which just says "warn about this kind of invalid device
> rather than silently ignoring it").
>
> YL: This is for preventing the potential assert, but we can separate it into
> another patch or just remove it if it is really not required.
>
> (3) All the other cases in this function of "the info about the endpoint
> looked weird" we report via the tracepoint trace_usb_host_parse_error(). I
> don't have a strong opinion about warn_report vs a tracepoint here, but I do
> think we should be consistent.
>
> YL: Understand, I think it depends. For normal QEMU USB event tracing or
> debugging, it's better to use the tracepoint functions. But in this case, it
> is reporting an unexpected condition or behavior.
All of these error cases are the same thing, though -- the device (via
libusb) is reporting something to us that seems to be invalid.
> > + * invalid altsetting index during fast hotplug/unplug.
> > + * Instead of aborting, log a warning and skip the
> > interface.
> > + */
> > + warn_report("usb-host: ignoring invalid altsetting=%d
> > (max=%d) "
> > + "for interface=%d on %s (bus=%03d, addr=%03d)",
> > + alt,
> > + conf->interface[i].num_altsetting ?
> > conf->interface[i].num_altsetting - 1 : -1,
> > + i,
> > + udev->product_desc ? udev->product_desc : "unknown",
> > + s->bus_num, s->addr);
> > + continue;
>
> For other errors with the intf info we "return", i.e.
> skip the whole endpoint, rather than just continuing.
> Should we do the same thing here ?
> (Conceptually the same thing -- just "return" leaks the config descriptor, as
> noted above.)
>
> YL: In this case, only one interface has an inconsistent altsetting index
> (caused by hot unplug/replug) while the others remain valid, so I use
> "continue" for skipping only the invalid interface avoids losing valid
> endpoint information. Returning early would drop all remaining interfaces,
> which seems undesirable. Thanks.
But this USB device is just (temporarily) broken, isn't it? Doesn't it all come
back with the right information later?
If it's reporting garbage to us then ignoring it entirely until it's
consistently sensible seems safer to me.
thanks
-- PMM
