Re: [PATCH v9 4/6] phy: realtek: usb2: introduce reset controller struct

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] phy: realtek: register access before reset deassertion causes hang/timeout
- [Medium] phy: missing reset_control_assert in rtk_phy_exit and error paths
--

commit ae1074cfd8fee3a5506c0c9db06f187c80793713
Author: Rustam Adilov <adilov@disroot.org>

phy: realtek: usb2: introduce reset controller struct

In RTL9607C, there is so called "IP Enable Controller" which resemble
reset controller with reset lines and is used for various things like
USB, PCIE, GMAC and such.

> phy: realtek: usb2: introduce reset controller struct
> 
> In RTL9607C, there is so called "IP Enable Controller" which resemble
> reset controller with reset lines and is used for various things like
> USB, PCIE, GMAC and such.
> 
> Introduce the reset_control struct to this driver to handle deasserting
> usb2 phy reset line.
> 
> Make use of the function devm_reset_control_get_optional_exclusive()
> function to get the reset controller and since existing RTD SoCs don't
> specify the resets we can have a cleaner code.
> 
> Since the vendor usb driver developed by Realtek doesn't assert the reset
> line (or in their case clear the register bit), we can reasonably assume
> reset_control_assert is not needed here.

[Severity: Medium]
Since this is an exclusive reset control, does it need to be asserted on
teardown even if the vendor driver doesn't do it?

If the reset line is never asserted during module removal or driver unbind
(such as in an rtk_phy_exit() function), could this lead to power leakage
or unexpected initial states during subsequent driver reloads?

> diff --git a/drivers/phy/realtek/phy-rtk-usb2.c b/drivers/phy/realtek/phy-rtk-usb2.c
> index 5e483c853f02f..2ddad7be73534 100644
> --- a/drivers/phy/realtek/phy-rtk-usb2.c
> +++ b/drivers/phy/realtek/phy-rtk-usb2.c
[ ... ]
> @@ -676,6 +678,15 @@ static int rtk_phy_init(struct phy *phy)
>  	if (!rtk_phy)
>  		return -EINVAL;
>  
> +	if (rtk_phy->phy_rst) {
> +		ret = reset_control_deassert(rtk_phy->phy_rst);
> +
> +		if (ret)
> +			return ret;
> +
> +		usleep_range(5000, 6000);
> +	}
> +
>  	for (i = 0; i < rtk_phy->num_phy; i++)
>  		ret = do_rtk_phy_init(rtk_phy, i);

[Severity: Medium]
If do_rtk_phy_init() fails, the reset line remains deasserted on the error
path. Should the reset line be asserted before returning the error?

[ ... ]
> @@ -1055,6 +1066,11 @@ static int rtk_usb2phy_probe(struct platform_device *pdev)
>  
>  	rtk_phy->num_phy = phy_cfg->num_phy;
>  
> +	rtk_phy->phy_rst = devm_reset_control_get_optional_exclusive(dev, NULL);
> +	if (IS_ERR(rtk_phy->phy_rst))
> +		return dev_err_probe(dev, PTR_ERR(rtk_phy->phy_rst),
> +				     "usb2 phy reset is not working\n");
> +
>  	ret = parse_phy_data(rtk_phy);

[Severity: High]
Can calling parse_phy_data() here before deasserting the reset control cause
a hardware hang or timeout?

Inside parse_phy_data(), if the SoC configuration lacks a pre-populated
default value for certain registers, functions like update_hs_clk_select()
and update_dc_driving_level() will attempt to read from the PHY registers:

drivers/phy/realtek/phy-rtk-usb2.c:update_hs_clk_select() {
	if (phy_parameter->inverse_hstx_sync_clock) {
		if (!phy_cfg->page0[6].addr) {
			rtk_phy_set_page(phy_reg, 0);
			phy_cfg->page0[6].addr = PAGE0_0XE6;
			phy_cfg->page0[6].data = rtk_phy_read(phy_reg, PAGE0_0XE6);
		}
	}
}

If the realtek,inverse-hstx-sync-clock DT property is set, it will read
from the unclocked or reset IP block, which could lock up the system bus
on many SoCs.

Would it be safer to defer these register reads until after the reset is
deasserted in rtk_phy_init()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016180314.31760-1-adilov@disroot.org?part=4

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