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

[PATCH] 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>
---
 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] 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>
---
 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] phy: broadcom: ns2-usbdrd: cancel delayed work

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing locking between asynchronous extcon workqueue and synchronous PHY operations, combined with incorrect state synchronization ordering.
--

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

phy: broadcom: ns2-usbdrd: cancel delayed work

This patch adds a managed cleanup action to cancel the delayed work
wq_extcon before requesting IRQs. This ensures the work does not run
after driver-managed resources have been released.

> 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);
>  
> +	ret = devm_add_action_or_reset(dev, ns2_drd_phy_cancel_work, driver);
> +	if (ret)
> +		return ret;
> +

[Severity: High]
This is a pre-existing issue, but I have a question about how the
extcon_work delayed work interacts with synchronous PHY operations.

In extcon_work(), we call extcon_set_state_sync() before updating
driver->data->new_state and calling connect_change():

	if (!id && vbus) { /* Host connected */
		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);

Because extcon_set_state_sync() invokes consumer notifiers synchronously,
could a consumer immediately call PHY APIs like ns2_drd_phy_init() or
ns2_drd_phy_poweron() in response?

If so, those PHY APIs would read driver->data->new_state before it has
been updated by the workqueue.

Furthermore, since there appears to be no internal locking, wouldn't
concurrent calls to ns2_drd_phy_init() and connect_change() race when
performing read-modify-write operations on the PHY registers?

>  	driver->id_irq = gpiod_to_irq(driver->id_gpiod);
>  	if (driver->id_irq < 0) {
>  		dev_err(dev, "failed to get ID IRQ\n");

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

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