Re: [PATCH v36 05/31] usb: host: xhci: Notify xHCI sideband on transfer ring free

[Jung Daehwan <dh10.jung@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 6697 bytes --]

On Fri, Mar 28, 2025 at 03:45:00PM +0800, Puma Hsu wrote:
> On Wed, Mar 19, 2025 at 8:52 AM Wesley Cheng <quic_wcheng@quicinc.com> wrote:
> >
> > In the case of handling a USB bus reset, the xhci_discover_or_reset_device
> > can run without first notifying the xHCI sideband client driver to stop or
> > prevent the use of the transfer ring.  It was seen that when a bus reset
> > situation happened, the USB offload driver was attempting to fetch the xHCI
> > transfer ring information, which was already freed.
> >
> > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> 
> Tested-by: Puma Hsu <pumahsu@google.com>
> 

Tested-by: Daehwan Jung <dh10.jung@samsung.com>

> > ---
> >  drivers/usb/host/xhci-sideband.c  | 29 ++++++++++++++++++++++++++++-
> >  drivers/usb/host/xhci.c           |  3 +++
> >  include/linux/usb/xhci-sideband.h | 30 +++++++++++++++++++++++++++++-
> >  3 files changed, 60 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
> > index 742bbc6c2d9b..d49f9886dd84 100644
> > --- a/drivers/usb/host/xhci-sideband.c
> > +++ b/drivers/usb/host/xhci-sideband.c
> > @@ -88,6 +88,30 @@ __xhci_sideband_remove_endpoint(struct xhci_sideband *sb, struct xhci_virt_ep *e
> >
> >  /* sideband api functions */
> >
> > +/**
> > + * xhci_sideband_notify_ep_ring_free - notify client of xfer ring free
> > + * @sb: sideband instance for this usb device
> > + * @ep_index: usb endpoint index
> > + *
> > + * Notifies the xHCI sideband client driver of a xHCI transfer ring free
> > + * routine.  This will allow for the client to ensure that all transfers
> > + * are completed.
> > + *
> > + * The callback should be synchronous, as the ring free happens after.
> > + */
> > +void xhci_sideband_notify_ep_ring_free(struct xhci_sideband *sb,
> > +                                      unsigned int ep_index)
> > +{
> > +       struct xhci_sideband_event evt;
> > +
> > +       evt.type = XHCI_SIDEBAND_XFER_RING_FREE;
> > +       evt.evt_data = &ep_index;
> > +
> > +       if (sb->notify_client)
> > +               sb->notify_client(sb->intf, &evt);
> > +}
> > +EXPORT_SYMBOL_GPL(xhci_sideband_notify_ep_ring_free);
> > +
> >  /**
> >   * xhci_sideband_add_endpoint - add endpoint to sideband access list
> >   * @sb: sideband instance for this usb device
> > @@ -342,7 +366,9 @@ EXPORT_SYMBOL_GPL(xhci_sideband_interrupter_id);
> >   * Return: pointer to a new xhci_sideband instance if successful. NULL otherwise.
> >   */
> >  struct xhci_sideband *
> > -xhci_sideband_register(struct usb_interface *intf, enum xhci_sideband_type type)
> > +xhci_sideband_register(struct usb_interface *intf, enum xhci_sideband_type type,
> > +                      int (*notify_client)(struct usb_interface *intf,
> > +                                   struct xhci_sideband_event *evt))
> >  {
> >         struct usb_device *udev = interface_to_usbdev(intf);
> >         struct usb_hcd *hcd = bus_to_hcd(udev->bus);
> > @@ -381,6 +407,7 @@ xhci_sideband_register(struct usb_interface *intf, enum xhci_sideband_type type)
> >         sb->vdev = vdev;
> >         sb->intf = intf;
> >         sb->type = type;
> > +       sb->notify_client = notify_client;
> >         vdev->sideband = sb;
> >
> >         spin_unlock_irq(&xhci->lock);
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index 61950a350432..91e2d6eac8b7 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/string_choices.h>
> >  #include <linux/dmi.h>
> >  #include <linux/dma-mapping.h>
> > +#include <linux/usb/xhci-sideband.h>
> >
> >  #include "xhci.h"
> >  #include "xhci-trace.h"
> > @@ -3919,6 +3920,8 @@ static int xhci_discover_or_reset_device(struct usb_hcd *hcd,
> >                 }
> >
> >                 if (ep->ring) {
> > +                       if (ep->sideband)
> > +                               xhci_sideband_notify_ep_ring_free(ep->sideband, i);
> >                         xhci_debugfs_remove_endpoint(xhci, virt_dev, i);
> >                         xhci_free_endpoint_ring(xhci, virt_dev, i);
> >                 }
> > diff --git a/include/linux/usb/xhci-sideband.h b/include/linux/usb/xhci-sideband.h
> > index f8722afb8a2d..45288c392f6e 100644
> > --- a/include/linux/usb/xhci-sideband.h
> > +++ b/include/linux/usb/xhci-sideband.h
> > @@ -21,6 +21,20 @@ enum xhci_sideband_type {
> >         XHCI_SIDEBAND_VENDOR,
> >  };
> >
> > +enum xhci_sideband_notify_type {
> > +       XHCI_SIDEBAND_XFER_RING_FREE,
> > +};
> > +
> > +/**
> > + * struct xhci_sideband_event - sideband event
> > + * @type: notifier type
> > + * @evt_data: event data
> > + */
> > +struct xhci_sideband_event {
> > +       enum xhci_sideband_notify_type type;
> > +       void *evt_data;
> > +};
> > +
> >  /**
> >   * struct xhci_sideband - representation of a sideband accessed usb device.
> >   * @xhci: The xhci host controller the usb device is connected to
> > @@ -30,6 +44,7 @@ enum xhci_sideband_type {
> >   * @type: xHCI sideband type
> >   * @mutex: mutex for sideband operations
> >   * @intf: USB sideband client interface
> > + * @notify_client: callback for xHCI sideband sequences
> >   *
> >   * FIXME usb device accessed via sideband Keeping track of sideband accessed usb devices.
> >   */
> > @@ -44,10 +59,14 @@ struct xhci_sideband {
> >         struct mutex                    mutex;
> >
> >         struct usb_interface            *intf;
> > +       int (*notify_client)(struct usb_interface *intf,
> > +                            struct xhci_sideband_event *evt);
> >  };
> >
> >  struct xhci_sideband *
> > -xhci_sideband_register(struct usb_interface *intf, enum xhci_sideband_type type);
> > +xhci_sideband_register(struct usb_interface *intf, enum xhci_sideband_type type,
> > +                      int (*notify_client)(struct usb_interface *intf,
> > +                                   struct xhci_sideband_event *evt));
> >  void
> >  xhci_sideband_unregister(struct xhci_sideband *sb);
> >  int
> > @@ -71,4 +90,13 @@ void
> >  xhci_sideband_remove_interrupter(struct xhci_sideband *sb);
> >  int
> >  xhci_sideband_interrupter_id(struct xhci_sideband *sb);
> > +
> > +#if IS_ENABLED(CONFIG_USB_XHCI_SIDEBAND)
> > +void xhci_sideband_notify_ep_ring_free(struct xhci_sideband *sb,
> > +                                      unsigned int ep_index);
> > +#else
> > +static inline void xhci_sideband_notify_ep_ring_free(struct xhci_sideband *sb,
> > +                                                    unsigned int ep_index)
> > +{ }
> > +#endif /* IS_ENABLED(CONFIG_USB_XHCI_SIDEBAND) */
> >  #endif /* __LINUX_XHCI_SIDEBAND_H */
> >
> 
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]