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
