dwc3 spin_lock_irq flags

dwc3 spin_lock_irq flags

[Thinh Nguyen]

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.

Thanks,
Thinh

Re: dwc3 spin_lock_irq flags

[Alan Stern]

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

Re: dwc3 spin_lock_irq flags

[Thinh Nguyen]

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