Re: [PATCH v3 2/2] usb: musb: gadget: don't flush a waiting RX packet when queueing a request

["Elvira Khabirova" <lineprinter0@gmail.com>]

On Sat Nov 7, 2026 at 7:11 PM CET, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [Medium] usb: musb: gadget: premature and unlocked rx_kick queueing in
>   musb_resume()
> --
>
> commit c9ae36b9b6933c41b159c85ef258637f80341755
> Author: Elvira Khabirova <lineprinter0@gmail.com>
>
> usb: musb: gadget: don't flush a waiting RX packet when queueing a request
>
> This commit replaces the RX FIFO flush in musb_ep_restart() with a
> system_bh_wq work item (rx_kick) to process waiting RX packets when a
> request is queued or after an in-progress giveback finishes.
>
>> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
>> index 73ac25f53698f..38469727acc6f 100644
>> --- a/drivers/usb/musb/musb_core.c
>> +++ b/drivers/usb/musb/musb_core.c
> [ ... ]
>> @@ -2866,6 +2868,7 @@ static int musb_resume(struct device *dev)
>>  	if ((devctl & mask) != (musb->context.devctl & mask))
>>  		musb->port1_status = 0;
>>  
>> +	musb_gadget_enable_rx_kicks(musb);
>>  	musb_enable_interrupts(musb);
>>  	musb_platform_enable(musb);
>>  
>> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
>> index b500e0d8b0bd3..08bb605aa0b31 100644
>> --- a/drivers/usb/musb/musb_gadget.c
>> +++ b/drivers/usb/musb/musb_gadget.c
> [ ... ]
>> +static void musb_g_enable_rx_kick(struct musb *musb, struct musb_ep *musb_ep)
>> +{
>> +	if (!musb_ep->rx_kick.func)
>> +		return;
>> +
>> +	enable_work(&musb_ep->rx_kick);
>> +	if (musb_ep->desc && !musb_ep->is_in) {
>> +		pm_runtime_get_noresume(musb->controller);
>> +		if (!queue_work(system_bh_wq, &musb_ep->rx_kick))
>> +			pm_runtime_put_noidle(musb->controller);
>> +	}
>> +}
>> +
>> +void musb_gadget_enable_rx_kicks(struct musb *musb)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < musb->nr_endpoints; i++) {
>> +		musb_g_enable_rx_kick(musb, &musb->endpoints[i].ep_in);
>> +		musb_g_enable_rx_kick(musb, &musb->endpoints[i].ep_out);
>> +	}
>> +}
>
> [Severity: Medium]
> Can calling musb_gadget_enable_rx_kicks() unlocked before
> musb_enable_interrupts() and musb_platform_enable() in musb_resume()
> wedge an OUT endpoint or race on endpoint and interrupt state?
>
> During system suspend, musb_suspend() calls pm_runtime_get_sync() (leaving
> musb->is_runtime_suspended at 0) and musb_disable_interrupts(), which
> zeros musb->intrtxe, musb->intrrxe, MUSB_INTRTXE, and MUSB_INTRRXE.  On
> resume, musb_restore_context() writes those zeroed masks back to hardware
> before musb_gadget_enable_rx_kicks() queues rx_kick on system_bh_wq:
>
> CPU0                                  CPU1 (system_bh_wq)
> musb_resume()
>   musb_restore_context()
>     /* MUSB_INTRRXE is 0 */
>   musb_gadget_enable_rx_kicks()
>     queue_work(..., &musb_ep->rx_kick)
>                                       musb_g_rx_kick()
>                                         musb_g_rx()
>                                           rxstate()
>                                             /* clears RXPKTRDY */
>                                       /* next host packet arrives while
>                                        * MUSB_INTRRXE == 0 or platform
>                                        * interrupts are still disabled
>                                        * <-- IRQ lost, endpoint wedged */
>   musb_enable_interrupts()
>   musb_platform_enable()
>
> In addition, musb_g_enable_rx_kick() inspects musb_ep->desc and
> musb_ep->is_in without holding musb->lock, and if musb_g_rx_kick()
> completes a request via musb_g_giveback() before musb_enable_interrupts()
> runs, the gadget completion callback (or a thread it wakes) can call
> usb_ep_enable() or usb_ep_disable() and modify musb->intrrxe,
> musb->intrtxe, and musb_ep->desc concurrently with the unlocked loop in
> musb_gadget_enable_rx_kicks() and musb_enable_interrupts().
>
> Calling musb_gadget_enable_rx_kicks() under musb->lock (or splitting
> enable_work() from queuing rx_kick under musb->lock) after
> musb_enable_interrupts() and musb_platform_enable() would avoid this.

Makes sense. Will be back with a v4 shortly.