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

[sashiko-bot@kernel.org]

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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107173601.63095-1-lineprinter0@gmail.com?part=2