Re: [PATCH v7 2/6] phy: realtek: usb2: introduce read and write functions to phy_reg_desc struct

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

New issues:
- [Low] The `.read` callback in `struct phy_reg_desc` and its default implementation `rtk_usb2phy_read()` are introduced but never used.

Pre-existing issues:
- [High] Unaligned MMIO access due to incorrect pointer arithmetic on `void __iomem *` when mapping multiple PHYs.
--

--- Patch [2]: [PATCH v7 2/6] phy: realtek: usb2: introduce read and write functions to phy_reg_desc struct ---
commit 8dc103211b6a1af9aae2648b712feaff2df2fd7f
Author: Rustam Adilov <[email protected]>

phy: realtek: usb2: introduce read and write functions to phy_reg_desc struct

This commit introduces read and write function pointers to the phy_reg_desc
struct to support platforms where the vstatus register resides in a big-endian
memory region. It also provides default implementations for existing PHYs.

> diff --git a/drivers/phy/realtek/phy-rtk-usb2.c b/drivers/phy/realtek/phy-rtk-usb2.c
> index e8d0f0119ec0a..5e483c853f02f 100644
> --- a/drivers/phy/realtek/phy-rtk-usb2.c
> +++ b/drivers/phy/realtek/phy-rtk-usb2.c
> @@ -130,6 +132,16 @@ struct rtk_phy {
>  	struct dentry *debug_dir;
>  };
>  
> +static u32 rtk_usb2phy_read(void __iomem *reg)
> +{
> +	return readl(reg);
> +}
> +

[Severity: Low]
Does this driver actually use the new read callback?

The vstatus register mentioned in the commit message is write-only in this
driver, and all existing reads appear to continue using readl() directly. Is
this read function needed, or is it dead code?

> +static void rtk_usb2phy_write(u32 val, void __iomem *reg)
> +{
> +	writel(val, reg);
> +}
> +

[ ... ]

> @@ -988,7 +1000,6 @@ static int parse_phy_data(struct rtk_phy *rtk_phy)
>  		phy_parameter->phy_reg.vstatus_index = index;
>  		phy_parameter->phy_reg.desc = phy_cfg->reg_desc;
>  

[Severity: High]
This is a pre-existing issue, but does this pointer arithmetic cause unaligned
MMIO accesses when configuring multiple PHYs?

In parse_phy_data():

    phy_parameter->phy_reg.reg_gusb2phyacc0 = of_iomap(np, 1) + index;

When num_phy > 1, the loop increments index. Since of_iomap() returns a
void __iomem *, adding index directly increments the address by exactly
index bytes rather than the register stride. Could this cause an alignment
fault during device probe on systems with multi-port PHY configurations?

> -
>  		if (of_property_read_bool(np, "realtek,inverse-hstx-sync-clock"))
>  			phy_parameter->inverse_hstx_sync_clock = true;
>  		else

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

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy