Re: [PATCH v2 2/4] phy: rockchip: inno-usb2: drop duplicated update of rport->suspended

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] phy: rockchip-inno-usb2: hotplug deadlock if rockchip_usb2phy_power_off() fails
--

commit a2dd4bb2fd5285c3bedf91d080a002423c1e6b40
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: inno-usb2: drop duplicated update of rport->suspended

rockchip_usb2phy_power_on and rockchip_usb2phy_power_off already update
rport->suspended in their success path. In case of error it's not
sensible to update rport->suspended. So drop the duplicated update.

> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> index 0ab054ee84cbe..e4d8abf935c1b 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
[ ... ]
> @@ -1004,7 +1003,6 @@ static void rockchip_usb2phy_sm_work(struct work_struct *work)
>  		if (!rport->suspended) {
>  			dev_dbg(&rport->phy->dev, "Disconnected\n");
>  			rockchip_usb2phy_power_off(rport->phy);
> -			rport->suspended = true;

[Severity: Medium]
If rockchip_usb2phy_power_off() fails, will rport->suspended remain false?

>  		}
>  
>  		/*
>  		 * activate the linestate detection to get the next device
>  		 * plug-in irq.
>  		 */
>  		property_enable(rphy->grf, &rport->port_cfg->ls_det_clr, true);
>  		property_enable(rphy->grf, &rport->port_cfg->ls_det_en, true);
>  
>  		/*
>  		 * we don't need to rearm the delayed work when the phy port
>  		 * is suspended.
>  		 */
>  		mutex_unlock(&rport->mutex);
>  		return;

[Severity: Medium]
Since the state machine exits here without re-arming the delayed work, and
rport->suspended could remain false, does this cause a permanent loss of
hotplug detection?

Looking at rockchip_usb2phy_linestate_irq(), the IRQ handler refuses to
re-arm the state machine if the port is not suspended:

static irqreturn_t rockchip_usb2phy_linestate_irq(int irq, void *data)
{
	...
	if (rport->suspended && rport->port_id == USB2PHY_PORT_HOST)
		rockchip_usb2phy_sm_work(&rport->sm_work.work);

	return IRQ_HANDLED;
}

If a USB device is disconnected and the underlying regmap_write() fails due
to local hardware or MMIO failure during rockchip_usb2phy_power_off(),
could this result in a hotplug deadlock for the USB host port?

While the code appears intended to handle this error path gracefully, it
seems to inadvertently break the state machine's re-arming condition. This
issue also appears to persist in the final commit of the series.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-phy-rockchip-inno-usb2-clock-fix-v2-0-e01bf509359e@collabora.com?part=2

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