Re: 答复: 答复: Consultation on the issue of digital headphones freezing

[Mathias Nyman <mathias.nyman@intel.com>]

On 1/14/26 12:27, 胡连勤 wrote:
> Hi  Mathias:
> 
>>> Ok, looks like we don't call xhci_sideband_notify_ep_ring_free() in all places we should.
>>>
>>> That being said, to solve this crash we need to make xhci_sideband_remove_endpoint() work
>>> in cases where device is being reset or disconnected, and endpoint is being dropped.
>>>
>>> Looks like xhci_initialize_ring_info(ep->ring) was added to xhci_sideband_remove_endpoint()
>>> in v8 of the original patch series due to a comment about leaving the endpoint in a messy state.
>>>
>>> https://lore.kernel.org/linux-usb/20231011002146.1821-1-quic_wcheng@quicinc.com/> >
>>> Calling  xhci_initialize_ring_info() does not really clean up anything, it just sets the
>>> software dequeue and enqueue pointers to the beginning of the ring. The xHC hardware
>>> dequeue pointer is still untouched, and the whole ring buffer is still uncleared.
>>>
>>> I think we can drop xhci_initialize_ring_info() call completely here.
>>> The class driver still needs to set up and reinit the endpoint properly if it wants to continue
>>> using it after it is removed from sideband usage.
>>>
>>> We do want to make sure endpoint is stopped when removing it, but take into account that endpoint
>>> might be stopped, disabled or already dropped by then.
>>>
>>> Does the code below solve your crash?
>>>
>>> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
>>> index a85f62a73313..2bd77255032b 100644
>>> --- a/drivers/usb/host/xhci-sideband.c
>>> +++ b/drivers/usb/host/xhci-sideband.c
>>> @@ -210,7 +210,6 @@ xhci_sideband_remove_endpoint(struct xhci_sideband *sb,
>>>    		return -ENODEV;
>>>
>>>    	__xhci_sideband_remove_endpoint(sb, ep);
>>> -	xhci_initialize_ring_info(ep->ring);
>>>
>>>    	return 0;
>>>    }
>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>>> index a148a1280126..4161c8c7721d 100644
>>> --- a/drivers/usb/host/xhci.c
>>> +++ b/drivers/usb/host/xhci.c
>>> @@ -2891,16 +2891,25 @@ int xhci_stop_endpoint_sync(struct xhci_hcd *xhci, struct xhci_virt_ep *ep, int
>>>    			    gfp_t gfp_flags)
>>>    {
>>>    	struct xhci_command *command;
>>> +	struct xhci_ep_ctx *ep_ctx;
>>>    	unsigned long flags;
>>> -	int ret;
>>> +	int ret = -ENODEV;
>>>
>>>    	command = xhci_alloc_command(xhci, true, gfp_flags);
>>>    	if (!command)
>>>    		return -ENOMEM;
>>>
>>>    	spin_lock_irqsave(&xhci->lock, flags);
>>> -	ret = xhci_queue_stop_endpoint(xhci, command, ep->vdev->slot_id,
>>> -				       ep->ep_index, suspend);
>>> +
>>> +	/* make sure endpoint exists and is running before stopping it */
>>> +	if (ep->ring) {
>>> +		ep_ctx = xhci_get_ep_ctx(xhci, ep->vdev->out_ctx, ep->ep_index);
>>> +		if (GET_EP_CTX_STATE(ep_ctx) == EP_STATE_RUNNING)
>>> +			ret = xhci_queue_stop_endpoint(xhci, command,
>>> +						       ep->vdev->slot_id,
>>> +						       ep->ep_index, suspend);
>>> +	}
>>> +
>>>    	if (ret < 0) {
>>>    		spin_unlock_irqrestore(&xhci->lock, flags);
>>>    		goto out
>>
>> This patch looks like good.
>> I believe this can solve the crash problem I'm currently experiencing.
>> I'll do a stress test and see.
> 
> After merging the above patch, the load test results are as follows:
> 1. The digital headset was connected to a  phone , and a total of three rounds of sleep and wake-up stress tests were conducted,
>       with 3,000 sleep and wake-up cycles in each round.
> 2. No system crashes occurred.
> 

Thanks for testing this,
I'll turn it into a proper patch, add Reported-by and Tested-by tags, and send it forward

Thanks
Mathias