On Mon, Oct 17, 2022 at 07:12:36PM +0300, Mathias Nyman wrote:
> On 14.10.2022 23.29, Marek Marczykowski-Górecki wrote:
> > On Fri, Oct 14, 2022 at 07:02:13PM +0300, Mathias Nyman wrote:
> > > This whole software bandwidth issue should only be visible in Intel
> > > Panther Point PCH xHC (Ivy bridge)
> >
> > It is indeed Ivy Bridge platform.
> >
> > > Endpoints should be deleted from bw_table list, and xhci_virt_devices
> > > should be freed already before xhci_mem_cleanup() is called if all goes well.
> > >
> > > Normally endpoints are deleted from bw_table list during usb_disconnect()
> > >
> > > usb_disconnect()
> > > ...
> > > usb_hcd_alloc_bandwidth(dev, NULL, NULL, NULL);
> > > hcd->driver->drop_endpoint() // flags endpoint to be dropped
> > > hcd->driver->check_bandwidth()
> > > ->xhci_check_bandwidth()
> > > xhci_configure_endpoint()
> > > xhci_reserve_bandwidth() // only for Panther Point
> > > xhci_drop_ep_from_interval_table()
> > >
> > > But to avoid queuing new commands to a host in XHCI_STATE_DYING or
> > > XHCI_STATE_REMOVING state we return early, not calling xhci_reserve_bandwidth().
> >
> > Indeed when I remove that early return in xhci_check_bandwidth(), the
> > crash is gone. What's the proper solution?
> >
>
> We could probably just delete the endpoint from the bw list when freeing the device and
> endpoints. Currently we just print that "endpoint x not removed from BW list!" message
>
> does the below help?
Yes, this helps!
xhci_drop_ep_from_interval_table() does few more things, but I assume
this all doesn't matter at the xhci_free_virt_device() time, right?
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 9e56aa28efcd..2adc0c2b470c 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -894,10 +894,12 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, int slot_id)
> * We can't drop them anyway, because the udev might have gone
> * away by this point, and we can't tell what speed it was.
> */
> - if (!list_empty(&dev->eps[i].bw_endpoint_list))
> + if (!list_empty(&dev->eps[i].bw_endpoint_list)) {
> + list_del_init(&dev->eps[i].bw_endpoint_list);
> xhci_warn(xhci, "Slot %u endpoint %u "
> "not removed from BW list!\n",
> slot_id, i);
> + }
> }
> /* If this is a hub, free the TT(s) from the TT list */
> xhci_free_tt_info(xhci, dev, slot_id);
>
> Thanks
> -Mathias
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
Attachment:
signature.asc
Description: PGP signature
