usb: dwc3: gadget: don't error on dequeue of a completed request - #22
Conversation
|
Hi @munzzyy, thanks for your patch! We’d rather not include known non-upstreamable changes into the repository, because the goal is for the Flipper to be fully usable on unpatched kernels from kernel.org Would appreciate if you could reshape your solution in a way that addresses the maintainer’s prior feedback. |
Dequeuing a request that has already been given back logs an error and returns -EINVAL: dwc3 23000000.usb: request 00000000ad92f1c4 was not queued to ep0out f_fs hits this on every teardown. functionfs_unbind() dequeues ep0req unconditionally before freeing it, which commit ce405d5 ("usb: gadget: f_fs: Ensure ep0req is dequeued before free_request") made deliberate to close a use-after-free. By then the control transfer has long completed, so dwc3_gadget_ep_dequeue() finds the request on none of cancelled_list, pending_list or started_list and falls through to the error path. Nothing is actually wrong. The request is not queued, which is what the caller asked for, and both callers ignore the return value and free the request straight after. The only effect is an error line in every gadget teardown, which buries real USB errors. dwc3 already tracks enough to tell the two cases apart. dwc3_gadget_ep_alloc_request() sets DWC3_REQUEST_STATUS_UNKNOWN, both __dwc3_gadget_ep_queue() and __dwc3_gadget_ep0_queue() set DWC3_REQUEST_STATUS_QUEUED, and dwc3_gadget_giveback() sets DWC3_REQUEST_STATUS_COMPLETED. A request that reaches the end of dequeue with status COMPLETED was queued to this endpoint and has finished. Anything else was never queued here, or the driver lost track of it. Keep the error for those, and return success for a completed request. This is narrower than the cdnsp fix for the same caller, commit 34f08eb ("usb: cdnsp: Fixes issue with dequeuing not queued requests"), which returns 0 whenever usb_request::status is not -EINPROGRESS. That also swallows a request that was never queued, since status is zero out of allocation. Going by dwc3's own request status keeps that case an error, which is what was asked for when a separate ep0 dequeue was proposed in 2022. Link: https://lore.kernel.org/linux-usb/20221117054917.30104-1-quic_ugoswami@quicinc.com/ Signed-off-by: Cole Munz <Munzzyy1@proton.me>
a80e093 to
cf3748b
Compare
munzzyy
changed the title
|
Understood, and agreed on not carrying it. Reshaped and force-pushed. Rather than the cdnsp-style no-op, it now decides on dwc3's own request status, which separates "queued here and completed" from "never queued here". The first returns success, the second keeps the Details in the updated description. arm64 build, sparse and checkpatch --strict are all clean. I'll send it to linux-usb whenever you're happy with the shape, with the ftrace from #20 as the reproducer Thinh asked for back then. |
2 participants
Closes #20.
Reshaped to option 2 from the issue rather than the fork-local carry, so it can go to linux-usb as-is.
The 2022 thread ended on Thinh's point that
functionfs_unbind()only runs after a disconnect, and that soft-disconnect already stalls and gives back any pending control transfer. That's correct, and it's what makes this safe to fix in dequeue: by the time f_fs dequeuesep0reqthe request has already completed, so finding it on none of the three lists is the expected state rather than a fault.The patch keys off dwc3's own per-request status instead of
usb_request::status:dwc3_gadget_ep_alloc_request()setsDWC3_REQUEST_STATUS_UNKNOWN__dwc3_gadget_ep_queue()and__dwc3_gadget_ep0_queue()setDWC3_REQUEST_STATUS_QUEUEDdwc3_gadget_giveback()setsDWC3_REQUEST_STATUS_COMPLETEDReaching the end of dequeue with
COMPLETEDtherefore means the request was queued to this endpoint and has finished. Anything else means it was never queued here, or the driver lost track of it, and those keep thedev_errand the-EINVAL.That is the difference from the cdnsp fix, 34f08eb, which returns 0 whenever
usb_request::status != -EINPROGRESS. That field is zero straight out of allocation, so cdnsp also quietly accepts a request that was never queued anywhere. Thinh's ask on the 2022 patch was to validate the request and return an error when it isn't a valid one to dequeue, and keying off the driver's own status keeps that property.Verified here:
gadget.obuilds clean for arm64, sparse (C=1) reports nothing,checkpatch --strictis 0/0/0. The observable effect, the log line gone at gadget teardown, still needs your hardware.Happy to send it to linux-usb once you're good with the shape.