答复: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[胡连勤]

Hi Michal,

> > The snd-usb-audio driver sets supports_autosuspend = 1
> > (card.c:1347), so the device can autosuspend when no audio stream
> > is active.
> >
> > During suspend, usb_audio_suspend() calls
> > platform_ops->suspend_cb → qc_usb_audio_offload_suspend(),
> > which sends a QMI disconnect indication to the ADSP and waits for
> > dev->in_use to be cleared.
> >
> > However, qc_usb_audio_offload_suspend() does not call
> > xhci_sideband_remove_endpoint() or xhci_sideband_unregister().
> > The sideband registration stays active during suspend — the
> > endpoint pointers to xHCI ring buffers remain in place.
> >
> > This is the root design issue: the sideband client assumes the
> > xHCI ring buffers remain valid across suspend/resume cycles. But
> > on reset-resume, usb_reset_and_verify_device() → hub_port_init()
> > → xhci_discover_or_reset_device() frees and reallocates those
> > rings, invalidating the sideband's pointers.
> >
> > Your suggestion is correct — the proper fix is for the audio
> > offload driver to remove sideband endpoints during suspend and
> > re-add them during resume, rather than leaving them registered
> > across suspend/resume cycles.
> 
> What's the state of the audio streaming interface during supend?
> Is it being switched to altsetting 0, with side effect of removing
> the isoc endpoint and then re-adding it after resume?
> 
No, it is not. During suspend, snd_usb_endpoint_suspend() only
sets need_prepare = true and need_setup = true flags
(endpoint.c:982-989). The actual altsetting switch (via
endpoint_set_interface(chip, ep, false)) is deferred to the next
snd_usb_endpoint_prepare() call (endpoint.c:1492), which runs from
the PCM prepare callback, not during suspend.

So the isoc endpoints stay enabled, ring buffers remain allocated,
and sideband pointers stay valid throughout suspend.

> If yes, that seems like a problem already, because the ring may be
> reallocated to a different address. Is this handled?
> 
> If not, then disabling sideband without disabling the endpoint may
> conflict with pending "large page support" patches, which (IIRC)
> assume that sideband is added *before* enabling the endpoint. So
> either we solve this without disabling sideband on suspend or there
> must be rule that sideband can be added/removed at any time.
> 
You're right — this is a valid concern. Given that the endpoints
are not disabled during suspend, removing sideband without
disabling the endpoint could violate the ordering assumption of
the large page support patches.

Given this constraint, Mathias's bandaid (skipping
xhci_disable_and_free_slot() when virt_dev->sideband is set)
is the safer approach. It avoids touching sideband at all during
suspend/resume, while preventing the use-after-free on the
disconnect path.

Regards,
Lianqin