Skip to content

usb: musb: gadget: don't flush a waiting RX packet

Elvira Khabirova linux-usb lkml sashiko-reviews 4 versions – In review

Get series with b4
b4 am [email protected]

Versions newest first · linked by author and title

  1. v4 Latest2 patches 1 replyIn review
  2. v32 patches 1 replySuperseded
  3. v21 patch 1 replySuperseded
Earlier versions · 1
VersionPostedPatchesRepliesStatus
v11 patch0 repliesSuperseded

Changes in v4 from the cover letter

- Move musb_gadget_enable_rx_kicks() in musb_resume() to be after musb_enable_interrupts() and under musb->lock (addresses sashiko's "premature and unlocked rx_kick queueing in musb_resume()")

Patches in v4 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/2usb: musb: gadget: count givebacks in busydrivers/usb/musb/musb_gadget.c drivers/usb/musb/musb_gadget.h+6 -61 reply
2/2usb: musb: gadget: don't flush a waiting RX packet when queueing a requestdrivers/usb/musb/musb_core.c drivers/usb/musb/musb_gadget.c drivers/usb/musb/musb_gadget.h+127 -30 replies

Reviews trailers per patch

No Reviewed-by, Acked-by or Tested-by trailers for v4 yet.

Read the v4 thread

Cover letter [PATCH v4 0/2] usb: musb: gadget: don't flush a waiting RX packet

Since commit 3fc137386c46 ("usb: musb: Fix hardware lockup on first Rx endpoint request"), musb_ep_restart() flushes the RX FIFO instead of calling rxstate().

When using some functions with musb, for example, f_mass_storage or f_acm, this code path will be used mid-communication, and it will cause data loss.

Patch 2 fixes this in musb_ep_restart() and schedules a bh work item instead of flushing and losing the data in the RX FIFO.

Patch 1 modifies the semantics of musb_ep.busy from a flag to a counter. If usb_ep_dequeue() runs at the same time as another musb_g_giveback(), busy could end up with a false 1; this would stall an endpoint. I added this patch in the series as prep for patch 2, because patch 2 would otherwise add one more case where such a race is possible -- a musb_g_giveback() of a work item with a musb_g_giveback() of nuke().

v3 -> v4:
- Move musb_gadget_enable_rx_kicks() in musb_resume() to be after musb_enable_interrupts() and under musb->lock (addresses sashiko's "premature and unlocked rx_kick queueing in musb_resume()")

v2 -> v3:
- Added patch 1: busy is a counter now (addresses sashiko's "overlapping musb_g_giveback() calls wedge ep->busy" and the nuke() part of "race and ordering bug in musb_gadget_disable() for rx_kick")
- Schedule a kick on every OUT ep when resuming to avoid stalls when the USB session is preserved across suspend/resume (addresses sashiko's "lost rx_kick across system suspend and resume")

v1 -> v2:
- musb_g_rx_kick() does not re-queue itself when busy is set. Instead, musb_g_giveback() re-queues it when rx_kick_deferred. This new flag is set in three cases:
1) busy is set inside musb_g_rx_kick()
2) musb_gadget_queue() adds an RX request to an empty request queue and busy is set
3) busy is set in the beginning of musb_g_rx() (addresses sashiko's "RX endpoint lockup after giveback in musb_g_rx_kick()", "softirq busy-loop and livelock in musb_g_rx_kick()", and "race and ordering bug in musb_gadget_disable() for rx_kick")
- musb_g_rx_kick checks musb->is_runtime_suspended now and exits early if the controller is suspended, and the work items are disabled during system suspend (addresses sashiko's
"unushed rx_kick work in musb_suspend() accesses disabled MMIO")
- This time tested also on a Pine A64+ (Allwinner A64), which has both MUSB and 4 cores.

v3: https://lore.kernel.org/linux-usb/[email protected]/
v2: https://lore.kernel.org/linux-usb/[email protected]/
v1: https://lore.kernel.org/linux-usb/[email protected]/

Elvira Khabirova (2):
usb: musb: gadget: count givebacks in busy
usb: musb: gadget: don't flush a waiting RX packet when queueing a
request

 drivers/usb/musb/musb_core.c   |   3 +
 drivers/usb/musb/musb_gadget.c | 128 ++++++++++++++++++++++++++++++---
 drivers/usb/musb/musb_gadget.h |  11 ++-
 3 files changed, 133 insertions(+), 9 deletions(-)

base-commit: c339f4010dea8b0d754f84ace70cd1b089878ef8

Signature · 1 line
2.53.0