Re: [PATCH v7 5/6] phy: realtek: usb2: add support for RTL9607C USB2 PHY

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The rtl9607_phy_cfg structure omits the .check_efuse flag, causing eFuse parameters to be ignored.
- [Medium] The page0 register configuration stores address 0xf3 at the wrong index, corrupting debugfs output.

Pre-existing issues:
- [High] Unchecked return value from of_iomap() leads to a NULL pointer dereference.
- [High] Pointer arithmetic on void __iomem * causes unaligned device register accesses.
- [High] Signed char integer promotion corrupts register writes on architectures with signed chars (e.g., MIPS).
- [High] Silent failure when do_rtk_phy_init() fails for multi-port SoCs.
--

--- Patch [5]: [PATCH v7 5/6] phy: realtek: usb2: add support for RTL9607C USB2 PHY ---
commit 6a5b4e2235314d4730ccd147be12148b19afc984
Author: Rustam Adilov <adilov@disroot.org>

    phy: realtek: usb2: add support for RTL9607C USB2 PHY
    
    Add support for the usb2 phy of RTL9607C series based SoCs.
    Add the macros and phy config struct for rtl9607.

> diff --git a/drivers/phy/realtek/phy-rtk-usb2.c b/drivers/phy/realtek/phy-rtk-usb2.c
> index 2ddad7be73534..990ceca37ef8a 100644
> --- a/drivers/phy/realtek/phy-rtk-usb2.c
> +++ b/drivers/phy/realtek/phy-rtk-usb2.c

[ ... ]

> @@ -1342,6 +1384,29 @@ static const struct phy_cfg rtd1315e_phy_cfg = {
>  	.reg_desc = &rtd_phy_reg_desc,
>  };
>  
> +static const struct phy_cfg rtl9607_phy_cfg = {
> +	.page0_size = MAX_USB_PHY_PAGE0_DATA_SIZE,
> +	.page0 = { [0] = {0xe0, 0x95},

[Severity: High]
This is a pre-existing issue, but does writing 0x95 here corrupt the register
on architectures like MIPS?

rtk_phy_write() takes data as a signed char:

drivers/phy/realtek/phy-rtk-usb2.c:rtk_phy_write() {
...
	reg_desc->write((u32)data << shift_bits,
...
}

On architectures where char is signed by default, a value of 0x95 is
interpreted as a negative number (-107). When cast to u32, it undergoes sign
extension to 0xFFFFFF95, and after shifting, it can clobber the upper bits
of the shared reg_wrap_vstatus register.

> +		   [4] = {0xe4, 0x6a},
> +		  [12] = {0xf3, 0x31}, },

[Severity: Medium]
Will this map to the correct debugfs output?

Address 0xf3 mathematically maps to array index 11 via the strict mapping
formula in page_addr_to_array_index(). Because it is placed at index 12
here, when rtk_usb2_parameter_show() computes the target read address
dynamically, it will read from 0xf4 and display it alongside the expected
0xf3 data.

> +	.page1_size = MAX_USB_PHY_PAGE1_DATA_SIZE,
> +	.page1 = { [0] = {0xe0, 0x26}, },
> +	.page2_size = MAX_USB_PHY_PAGE2_DATA_SIZE,
> +	.page2 = { [7] = {0xe7, 0x33}, },
> +	.num_phy = 1,
> +	.check_efuse_version = CHECK_EFUSE_V2,

[Severity: Medium]
Is the .check_efuse flag missing here?

Without .check_efuse = true, the eFuse parameters like check_efuse_version
and efuse_dc_driving_rate specified in this structure appear to be ignored
by get_phy_data_by_efuse(), leaving the hardware uncalibrated.

> +	.efuse_dc_driving_rate = EFUS_USB_DC_CAL_RATE,
> +	.dc_driving_mask = 0x1f,
> +	.efuse_dc_disconnect_rate = EFUS_USB_DC_DIS_RATE,
> +	.dc_disconnect_mask = 0xf,
> +	.usb_dc_disconnect_at_page0 = true,
> +	.do_toggle = true,
> +	.driving_updated_for_dev_dis = 0x8,
> +	.is_double_sensitivity_mode = true,
> +	.reg_desc = &rtl9607_phy_reg_desc,
> +	.force_host_disconnect = true,
> +};
> +

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but looking at the existing
parse_phy_data() function:

drivers/phy/realtek/phy-rtk-usb2.c:parse_phy_data() {
...
	phy_parameter->phy_reg.reg_wrap_vstatus = of_iomap(np, 0);
...
}

Does this code dereference a NULL pointer during device probe if the device
tree lacks the required reg properties?

The return values from of_iomap() are assigned and subsequently dereferenced
without any validation.

[Severity: High]
This is a pre-existing issue, but does adding index to the of_iomap() result
cause unaligned device register accesses?

Looking at the existing parse_phy_data() function:

drivers/phy/realtek/phy-rtk-usb2.c:parse_phy_data() {
...
	phy_parameter->phy_reg.reg_gusb2phyacc0 = of_iomap(np, 1) + index;
...
}

Since of_iomap() returns void __iomem *, GCC treats the pointer arithmetic
as 1-byte elements. For multi-port PHYs where index > 0, this adds exactly
1 byte to the base address, which will trigger alignment exceptions during
subsequent 32-bit writel() calls on architectures like ARM64.

[Severity: High]
This is a pre-existing issue, but does the initialization loop mask errors
for multi-port SoCs?

Looking at the existing rtk_phy_init() function:

drivers/phy/realtek/phy-rtk-usb2.c:rtk_phy_init() {
...
	for (i = 0; i < rtk_phy->num_phy; i++)
		ret = do_rtk_phy_init(rtk_phy, i);
...
}

If do_rtk_phy_init() fails for any port except the last one, ret is
overwritten by the result of the subsequent PHY's initialization, silently
ignoring earlier failures while incorrectly reporting probe success.

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