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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The driver attempts to access the PHY's memory-mapped registers in `probe()` before the newly introduced reset control is deasserted.

Pre-existing issues:
- [Critical] An off-by-one error in `rtk_phy_toggle` allows out-of-bounds access to the `phy_parameter` array.
- [High] The loop in `rtk_phy_init()` silently ignores initialization errors from all but the last PHY.
- [High] Pointer arithmetic on `void __iomem *` leads to unaligned I/O access and duplicated memory mappings.
- [High] Per-port PHY properties are parsed in a loop and written directly into the globally shared `rtk_phy->phy_cfg` array, overwriting settings from earlier ports.
--

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

phy: realtek: usb2: introduce reset controller struct

In RTL9607C, there is an IP Enable Controller which acts like a
reset controller. This introduces a reset_control struct to handle
deasserting the usb2 phy reset line using the reset framework.

> diff --git a/drivers/phy/realtek/phy-rtk-usb2.c b/drivers/phy/realtek/phy-rtk-usb2.c
> index a9e29d2f7e6fd..16c5fc3191dec 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: High]
This is a pre-existing issue, but does this loop silently ignore
initialization errors from all but the last PHY?

If an error occurs on any PHY other than the last one, the ret variable
is overwritten by the result of the subsequent iteration. Could this
falsely return success even if earlier ports failed to initialize?

[Severity: Critical]
This is a pre-existing issue, but is there an out-of-bounds access
in the toggle function called during initialization?

When do_rtk_phy_init() calls the toggle function for a given index, the
target function checks if index > rtk_phy->num_phy instead of
index >= rtk_phy->num_phy. Since the phy_parameter array is allocated
with rtk_phy->num_phy elements, could passing index == num_phy bypass
the bounds check and lead to memory corruption or arbitrary memory
access via I/O operations?

[ ... ]

> @@ -1059,6 +1070,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_array_get_optional_exclusive(dev);
> +	if (IS_ERR(rtk_phy->phy_rst))
> +		return dev_err_probe(dev, PTR_ERR(rtk_phy->phy_rst),
> +				     "usb2 phy resets are not working\n");
> +
>  	ret = parse_phy_data(rtk_phy);

[Severity: High]
Does parse_phy_data() attempt to access the PHY's memory-mapped registers
before the newly introduced reset control is deasserted?

It appears that the driver acquires the reset in probe() and then
immediately calls parse_phy_data(), which executes hardware read/write
operations via update_dc_driving_level() and update_hs_clk_select().

Because the reset is only deasserted later in rtk_phy_init(), could
accessing registers of an IP block held in reset trigger a synchronous
bus error or silently return zeroes, preventing proper initialization?

[Severity: High]
This is a pre-existing issue, but does pointer arithmetic on void __iomem *
lead to unaligned I/O access and duplicated memory mappings in
parse_phy_data()?

Looking at the loop in parse_phy_data():

	for (index = 0; index < rtk_phy->num_phy; index++) {
		...
		phy_parameter->phy_reg.reg_gusb2phyacc0 = of_iomap(np, 1) + index;
		...
	}

Since of_iomap() returns a void __iomem *, does adding index advance the
pointer by exactly 1 byte? Could this result in an unaligned pointer for
subsequent 32-bit readl() or writel() operations and trigger an alignment
fault?

Also, does calling of_iomap() repeatedly inside the loop create redundant,
duplicate mappings of the exact same physical resource?

[Severity: High]
This is a pre-existing issue, but are per-port PHY properties parsed in a
loop and written directly into the globally shared rtk_phy->phy_cfg array,
overwriting settings from earlier ports?

In parse_phy_data(), the driver iterates over all PHY ports and retrieves
port-specific parameters. Functions like update_dc_driving_level() modify
the globally shared array.

Will properties computed for port 1 silently overwrite those computed
for port 0? When rtk_phy_init() is later called, do_rtk_phy_init() applies
this globally shared array to all ports, which seems like it might break
per-port configurations for any SoC with num_phy > 1.

>  	if (ret)
>  		goto err;

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