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

[sashiko-bot@kernel.org]

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