Re: [PATCH v3 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 wedge on rockchip_usb2phy_power_off() error
--

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

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

This commit removes the unconditional assignments to rport->suspended in
rockchip_usb2phy_sm_work() after calling rockchip_usb2phy_power_on() and
rockchip_usb2phy_power_off(), relying on those functions to update
rport->suspended on success.

> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> index 0b61f152eb3f3..51afbcc40c4eb 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> @@ -994,7 +994,6 @@ static void rockchip_usb2phy_sm_work(struct work_struct *work)
>  		if (rport->suspended) {
>  			dev_dbg(&rport->phy->dev, "Connected\n");
>  			rockchip_usb2phy_power_on(rport->phy);
> -			rport->suspended = false;
>  		} else {
>  			/* D+ line pull-up, D- line pull-down */
>  			dev_dbg(&rport->phy->dev, "FS/LS online\n");
> @@ -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]
As reported by Sashiko on v2 [1], if rockchip_usb2phy_power_off() fails in
rockchip_usb2phy_sm_work() under PHY_STATE_DISCONNECT (when
property_enable() on phy_sus returns an error), rport->suspended now remains
false and rphy->clk480m stays enabled.

However, the PHY_STATE_DISCONNECT path does not check the return value of
rockchip_usb2phy_power_off() and still proceeds as if the port were
suspended, enabling ls_det_clr and ls_det_en and returning early without
rescheduling rport->sm_work:

	case PHY_STATE_DISCONNECT:
		if (!rport->suspended) {
			dev_dbg(&rport->phy->dev, "Disconnected\n");
			rockchip_usb2phy_power_off(rport->phy);
		}

		property_enable(rphy->grf, &rport->port_cfg->ls_det_clr, true);
		property_enable(rphy->grf, &rport->port_cfg->ls_det_en, true);

		mutex_unlock(&rport->mutex);
		return;

When the next linestate interrupt fires, rockchip_usb2phy_linestate_irq()
disables ls_det_en and only restarts rockchip_usb2phy_sm_work() when
rport->suspended is true:

	property_enable(rphy->grf, &rport->port_cfg->ls_det_en, false);
	property_enable(rphy->grf, &rport->port_cfg->ls_det_clr, true);

	mutex_unlock(&rport->mutex);

	if (rport->suspended && rport->port_id == USB2PHY_PORT_HOST)
		rockchip_usb2phy_sm_work(&rport->sm_work.work);

Because rport->suspended is false, would this leave ls_det_en disabled and
rport->sm_work stopped, permanently wedging host port hotplug detection and
leaking the rphy->clk480m enable count? Should rockchip_usb2phy_sm_work()
jump to next_schedule when rockchip_usb2phy_power_off() returns an error?

[1] https://lore.kernel.org/r/20261001-phy-rockchip-inno-usb2-clock-fix-v2-2-e01bf509359e@collabora.com

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

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