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
