Re: [PATCH v7 1/6] phy: realtek: usb2: introduce phy_reg_desc struct to the driver

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

Pre-existing issues:
- [High] Repeated `of_iomap()` calls inside a loop redundantly map hardware registers without corresponding `iounmap()` calls, causing permanent memory leaks.
- [High] Incorrect pointer arithmetic on `void __iomem *` creates unaligned I/O pointers for subsequent PHYs, leading to unaligned 32-bit accesses.
- [High] The return values of `of_iomap()` are not checked for NULL, leading to potential NULL pointer dereferences during MMIO operations.
- [High] `rtk_phy_write()` overwrites the entire shared `VStatusOut2` register, clobbering the configuration of other PHY ports.
- [High] The driver accumulates per-port PHY calibration values into a single shared `phy_cfg` structure, causing all ports to be initialized with corrupted data.
--

--- Patch [1]: [PATCH v7 1/6] phy: realtek: usb2: introduce phy_reg_desc struct to the driver ---
commit 9a2c62ecf6aef3b339ebeb8b5606b980417dd955
Author: Rustam Adilov <[email protected]>

phy: realtek: usb2: introduce phy_reg_desc struct to the driver

This commit creates a per-SoC phy_reg_desc struct to hold register offsets
and bits that vary across different SoCs. It adds a default rtd_phy_reg_desc
and updates the driver data to point towards it.

> diff --git a/drivers/phy/realtek/phy-rtk-usb2.c b/drivers/phy/realtek/phy-rtk-usb2.c
> index 248550ef98cab..e8d0f0119ec0a 100644
> --- a/drivers/phy/realtek/phy-rtk-usb2.c
> +++ b/drivers/phy/realtek/phy-rtk-usb2.c
[ ... ]
> @@ -190,29 +199,31 @@ static int rtk_phy_write(struct phy_reg *phy_reg, char addr, char data)
>  	unsigned int val;
>  	void __iomem *reg_wrap_vstatus = phy_reg->reg_wrap_vstatus;
>  	void __iomem *reg_gusb2phyacc0 = phy_reg->reg_gusb2phyacc0;
> +	const struct phy_reg_desc *reg_desc = phy_reg->desc;
>  	int shift_bits = phy_reg->vstatus_index * 8;
>  	int ret = 0;
>  
>  	/* write data to VStatusOut2 (data output to phy) */
> -	writel((u32)data << shift_bits, reg_wrap_vstatus);
> +	writel((u32)data << shift_bits,
> +	       reg_wrap_vstatus + reg_desc->vstatus_offset);

[Severity: High]
This is a pre-existing issue, but does this unconditional 32-bit writel()
clobber the shared register's other byte lanes without a read-modify-write?

In multi-port setups, all ports map the exact same physical reg_wrap_vstatus
register. When configuring port 1 (where shift_bits = 8), writing
(u32)data << shift_bits will correctly position port 1's byte, but write
zeroes to all other bytes, entirely erasing the configuration just applied to
port 0.

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for
reg_wrap_vstatus to be NULL here?

The pointer is acquired via of_iomap() in parse_phy_data(), which doesn't
appear to be checked for NULL before being dereferenced in this writel().

[ ... ]
> @@ -974,6 +986,8 @@ static int parse_phy_data(struct rtk_phy *rtk_phy)
>  		phy_parameter->phy_reg.reg_wrap_vstatus = of_iomap(np, 0);
>  		phy_parameter->phy_reg.reg_gusb2phyacc0 = of_iomap(np, 1) + index;

[Severity: High]
This is a pre-existing issue, but do these redundant of_iomap() calls create
a memory leak?

They occur inside the loop iterating over num_phy, repeatedly mapping the
same physical region without storing all mapped pointers or unmapping them.

[Severity: High]
This isn't a bug introduced by this patch, but does the pointer arithmetic on
of_iomap(np, 1) + index create unaligned I/O pointers for subsequent PHYs?

The return type of of_iomap() is void __iomem *. By GCC extension, adding
index advances the pointer by index bytes. For num_phy > 1, index becomes 1
or higher, creating a misaligned pointer. Subsequent calls to readl() or
writel() on this pointer will perform 32-bit accesses misaligned by 1 byte.

[Severity: High]
This isn't a bug introduced by this patch, but does the driver accumulate
per-port PHY calibration values into a single shared phy_cfg structure?

In parse_phy_data(), it iterates over all ports and calls
update_dc_driving_level(), which modifies phy_cfg->page0[4].data:

drivers/phy/realtek/phy-rtk-usb2.c:update_dc_driving_level() {
    ...
    phy_cfg->page0[4].data = __update_dc_driving_page0_0xe4(phy_cfg,
                                                phy_parameter,
                                                phy_cfg->page0[4].data);
    ...
}

Because phy_cfg is shared globally for the entire driver instance, it seems
the calibration math meant for port 1 is added cumulatively on top of the
results of port 0. Later, do_rtk_phy_init() pushes this heavily corrupted
array to all ports.

>  		phy_parameter->phy_reg.vstatus_index = index;
> +		phy_parameter->phy_reg.desc = phy_cfg->reg_desc;
> +
>  
>  		if (of_property_read_bool(np, "realtek,inverse-hstx-sync-clock"))

[ ... ]
> @@ -1060,6 +1074,12 @@ static void rtk_usb2phy_remove(struct platform_device *pdev)
>  	remove_debug_files(rtk_phy);
>  }

[Severity: High]
This is a pre-existing issue, but does rtk_usb2phy_remove() fail to call
iounmap() to clean up the mappings created by of_iomap() during probe?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1