Re: [PATCH 2/2] usb: dwc3: imx8mp: set needs_full_reinit as true

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: dwc3: imx8mp: bitfield race on needs_full_reinit in
  dwc3_imx8mp_probe()
--

commit f35ce9daa116e0df2a34cb6ec0a405a630719820
Author: Xu Yang <xu.yang_2@nxp.com>

usb: dwc3: imx8mp: set needs_full_reinit as true

This commit sets dwc3->needs_full_reinit in dwc3_imx8mp_probe() so that the
USB3 controller is re-initialized after system suspend on i.MX8MP.

> diff --git a/drivers/usb/dwc3/dwc3-imx8mp.c b/drivers/usb/dwc3/dwc3-imx8mp.c
> index ee92a5efaa6e8..78cb79fefba2c 100644
> --- a/drivers/usb/dwc3/dwc3-imx8mp.c
> +++ b/drivers/usb/dwc3/dwc3-imx8mp.c
> @@ -267,6 +267,7 @@ static int dwc3_imx8mp_probe(struct platform_device *pdev)
>  	}
>  
>  	dwc3->glue_ops = &dwc3_imx_glue_ops;
> +	dwc3->needs_full_reinit = true;

[Severity: Medium]
Can setting dwc3->needs_full_reinit without holding dwc3->lock clobber
adjacent bitfields in struct dwc3?

needs_full_reinit is a 1-bit bitfield sharing a storage word with runtime
gadget flags:

drivers/usb/dwc3/core.h:struct dwc3 {
    ...
	unsigned		connected:1;
	unsigned		softconnect:1;
    ...
	unsigned		pending_events:1;
	unsigned		do_fifo_resize:1;
	unsigned		pullups_connected:1;
	unsigned		setup_packet_pending:1;
	unsigned		three_stage_setup:1;
    ...
	unsigned		needs_full_reinit:1;
};

Earlier in dwc3_imx8mp_probe(), of_platform_populate() synchronously runs
dwc3_probe() -> dwc3_core_probe() -> dwc3_core_init_mode().  In peripheral
mode, usb_add_gadget() registers the UDC, allowing a gadget driver to bind
and start the controller before of_platform_populate() returns:

CPU0                              CPU1
dwc3_imx8mp_probe()
  of_platform_populate()
    ...
      usb_add_gadget()
                                  gadget_bind_driver()
                                    ...
                                      dwc3_gadget_pullup()
                                        spin_lock_irqsave(&dwc->lock)
  /* RMW loads bitfield word */
                                        dwc3_gadget_soft_connect()
                                          dwc->pullups_connected = true;
                                        spin_unlock_irqrestore(&dwc->lock)
  dwc3->needs_full_reinit = true;
  /* RMW store overwrites pullups_connected */

Overwriting pullups_connected or adjacent EP0 and event flags can corrupt
EP0 state or cause dwc3_gadget_soft_disconnect() to see pullups_connected
as zero and skip halting the controller before suspend.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106111943.2868733-1-xu.yang_2@oss.nxp.com?part=2