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

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

[Hongyan Xu]

ns2_drd_phy_probe() initializes wq_extcon. Both GPIO IRQs and the probe
tail can queue it. There is no remove hook, so the delayed work can run
after driver-managed resources are being released.

Add a managed cleanup action before requesting IRQs. Devres releases the
IRQs first, then cancels the delayed work while extcon, GPIO and MMIO
state is still valid.

This issue was found by a static analysis tool.

Signed-off-by: Hongyan Xu <getshell@seu.edu.cn>
---
Resend with recipients from the current MAINTAINERS file. No code changes.

 drivers/phy/broadcom/phy-bcm-ns2-usbdrd.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/phy/broadcom/phy-bcm-ns2-usbdrd.c b/drivers/phy/broadcom/phy-bcm-ns2-usbdrd.c
index 36ad02c33..54929edb1 100644
--- a/drivers/phy/broadcom/phy-bcm-ns2-usbdrd.c
+++ b/drivers/phy/broadcom/phy-bcm-ns2-usbdrd.c
@@ -264,6 +264,13 @@ static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static void ns2_drd_phy_cancel_work(void *data)
+{
+	struct ns2_phy_driver *driver = data;
+
+	cancel_delayed_work_sync(&driver->wq_extcon);
+}
+
 static const struct phy_ops ops = {
 	.init		= ns2_drd_phy_init,
 	.power_on	= ns2_drd_phy_poweron,
@@ -342,6 +349,10 @@ static int ns2_drd_phy_probe(struct platform_device *pdev)
 
 	INIT_DELAYED_WORK(&driver->wq_extcon, extcon_work);
 
+	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);
 	if (driver->id_irq < 0) {
 		dev_err(dev, "failed to get ID IRQ\n");
-- 
2.50.1.windows.1

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

[Hongyan Xu]

ns2_drd_phy_probe() initializes wq_extcon. Both GPIO IRQs and the probe
tail can queue it. There is no remove hook, so the delayed work can run
after driver-managed resources are being released.

Add a managed cleanup action before requesting IRQs. Devres releases the
IRQs first, then cancels the delayed work while extcon, GPIO and MMIO
state is still valid.

This issue was found by a static analysis tool.

Signed-off-by: Hongyan Xu <getshell@seu.edu.cn>
---
Resend with recipients from the current MAINTAINERS file. No code changes.

 drivers/phy/broadcom/phy-bcm-ns2-usbdrd.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/phy/broadcom/phy-bcm-ns2-usbdrd.c b/drivers/phy/broadcom/phy-bcm-ns2-usbdrd.c
index 36ad02c33..54929edb1 100644
--- a/drivers/phy/broadcom/phy-bcm-ns2-usbdrd.c
+++ b/drivers/phy/broadcom/phy-bcm-ns2-usbdrd.c
@@ -264,6 +264,13 @@ static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static void ns2_drd_phy_cancel_work(void *data)
+{
+	struct ns2_phy_driver *driver = data;
+
+	cancel_delayed_work_sync(&driver->wq_extcon);
+}
+
 static const struct phy_ops ops = {
 	.init		= ns2_drd_phy_init,
 	.power_on	= ns2_drd_phy_poweron,
@@ -342,6 +349,10 @@ static int ns2_drd_phy_probe(struct platform_device *pdev)
 
 	INIT_DELAYED_WORK(&driver->wq_extcon, extcon_work);
 
+	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);
 	if (driver->id_irq < 0) {
 		dev_err(dev, "failed to get ID IRQ\n");
-- 
2.50.1.windows.1

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

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

[sashiko-bot]

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