Re: dwc3 spin_lock_irq flags

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

Alan Stern wrote:
> On Tue, Aug 10, 2021 at 10:10:00PM +0000, Thinh Nguyen wrote:
>> Hi,
>>
>> Typically when we use spin_lock_irqsave and spin_unlock_irqrestore,
>> we save the irq state in the "flags" variable and pass it down to any
>> function that may need to do spin_unlock_irqrestore and update the flags
>> again.
>>
>> I don't see that we're doing it for dwc3 when we give back the requests:
>>
>> void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req,
>> 		int status)
>> {
>> 	struct dwc3			*dwc = dep->dwc;
>>
>> 	dwc3_gadget_del_and_unmap_request(dep, req, status);
>> 	req->status = DWC3_REQUEST_STATUS_COMPLETED;
>>
>> 	spin_unlock(&dwc->lock);
>> 	usb_gadget_giveback_request(&dep->endpoint, &req->request);
>> 	spin_lock(&dwc->lock);
>> }
>>
>> Then we would use the stale "flags" to do spin_unlock_irqrestore() at a later
>> time. Maybe someone can help shed some light on what issue this would cause
>> (if any). From our hardware testing, there's no obvious failure or performance
>> impact that we see.
> 
> There are no issues with this code pattern; it is perfectl valid.  Its 
> only effect is that sometimes the request's completion handler will be 
> called with interrupts disabled when in theory, it could have been 
> called with interrupts enabled.  This won't cause any problem because 
> completion handlers are _always_ called with interrupts disabled; this 
> is mentioned in the kerneldoc for the @complete member of struct 
> usb_request.
> 
> When the spin_unlock_irqrestore() call runs later on, its "flags" 
> argument won't be stale.  It will accurately reflect whether interrupts 
> were enabled when the original spin_lock_irqsave() ran.  By themselves, 
> spin_unlock() and spin_lock() don't change the enable state for 
> interrupts.
> 
> Alan Stern
> 

It's clear now. Thanks for the explanation Alan.

BR,
Thinh