Re: [PATCH RESEND] phy: broadcom: ns2-usbdrd: cancel delayed work

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Race Condition and Missing Locking on Shared MMIO Registers
- [High] State Synchronization / Logic Error: Stale state reads during extcon notifications
--

--- Patch [1]: [PATCH RESEND] phy: broadcom: ns2-usbdrd: cancel delayed work ---
commit 5e11b6ff613354cecc643a7e40d800215b789869
Author: Hongyan Xu <getshell@seu.edu.cn>

phy: broadcom: ns2-usbdrd: cancel delayed work

ns2_drd_phy_probe() initializes wq_extcon. Both GPIO IRQs and the probe
tail can queue it. This patch adds a managed cleanup action to cancel
the delayed work on teardown, preventing it from running after resources
are freed.

> diff --git a/drivers/phy/broadcom/phy-bcm-ns2-usbdrd.c b/drivers/phy/broadcom/phy-bcm-ns2-usbdrd.c
> index 8473fa5745296..d0959668df793 100644
> --- a/drivers/phy/broadcom/phy-bcm-ns2-usbdrd.c
> +++ b/drivers/phy/broadcom/phy-bcm-ns2-usbdrd.c
> @@ -342,6 +349,10 @@ static int ns2_drd_phy_probe(struct platform_device *pdev)
>  
>  	INIT_DELAYED_WORK(&driver->wq_extcon, extcon_work);

[Severity: High]
This is a pre-existing issue, but does the asynchronous execution of
extcon_work() risk corrupting shared MMIO registers?

extcon_work() calls connect_change(), which does read-modify-write operations
on registers like ICFG_FSM_CTRL without locking:

drivers/phy/broadcom/phy-bcm-ns2-usbdrd.c:connect_change() {
        ...
        val = readl(driver->icfgdrd_regs + ICFG_FSM_CTRL);
        ...
        val &= ~(DRD_HOST_MODE | DRD_DEVICE_MODE);
        writel(val, driver->icfgdrd_regs + ICFG_FSM_CTRL);
        ...
}

Concurrently, PHY framework operations like ns2_drd_phy_init() and
ns2_drd_phy_poweron() invoked by consumers can modify the exact same registers
on different CPUs:

drivers/phy/broadcom/phy-bcm-ns2-usbdrd.c:ns2_drd_phy_init() {
        ...
        val = readl(driver->icfgdrd_regs + ICFG_FSM_CTRL);

        if (data->new_state == EVT_HOST) {
                val &= ~DRD_DEVICE_MODE;
                val |= DRD_HOST_MODE;
        ...
        writel(val, driver->icfgdrd_regs + ICFG_FSM_CTRL);
}

Should there be a spinlock serializing these register accesses across PHY ops
and the extcon workqueue?

[Severity: High]
This isn't a bug introduced by this patch, but does the state update order in
extcon_work() cause consumer drivers to read stale state?

The workqueue synchronously notifies consumers via extcon_set_state_sync()
before saving the new state:

drivers/phy/broadcom/phy-bcm-ns2-usbdrd.c:extcon_work() {
        ...
        extcon_set_state_sync(driver->edev, EXTCON_USB_HOST, true);
        pr_debug("Host cable connected\n");
        driver->data->new_state = EVT_HOST;
        connect_change(driver);
        ...
}

Since extcon_set_state_sync() is a blocking notifier chain, consumer drivers
execute their callbacks immediately. If a consumer reacts by calling phy_init()
or phy_power_on(), those functions will read the old, unmodified
data->new_state, which could result in incorrect hardware configurations.

Should the internal state and hardware configurations be fully updated prior to
emitting the extcon notification?

> +	ret = devm_add_action_or_reset(dev, ns2_drd_phy_cancel_work, driver);
> +	if (ret)
> +		return ret;
> +
>  	driver->id_irq = gpiod_to_irq(driver->id_gpiod);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728133418.103-1-getshell@seu.edu.cn?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy