Re: [PATCH v6 2/6] phy: realtek: usb2: introduce read and write functions to driver data

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The return values of `of_iomap()` are not checked, which can lead to a NULL pointer dereference.
- [High] MMIO regions mapped with `of_iomap()` are never unmapped, and are redundantly mapped in a loop.
- [High] Incorrect pointer arithmetic on a `void __iomem *` leads to unaligned memory accesses for multi-PHY configurations.
--

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

phy: realtek: usb2: introduce read and write functions to driver data

This commit introduces read and write function pointers to the driver data
to handle RTL9607C, which has a little endian USB host controller but a big
endian vstatus register. It also provides default accessors for existing SoCs.

> diff --git a/drivers/phy/realtek/phy-rtk-usb2.c b/drivers/phy/realtek/phy-rtk-usb2.c
> index bd9ee895b9f90..a9e29d2f7e6fd 100644
> --- a/drivers/phy/realtek/phy-rtk-usb2.c
> +++ b/drivers/phy/realtek/phy-rtk-usb2.c

[ ... ]

> @@ -985,6 +1001,8 @@ static int parse_phy_data(struct rtk_phy *rtk_phy)
>  		phy_parameter->phy_reg.vstatus_offset = phy_cfg->vstatus_offset;
>  		phy_parameter->phy_reg.vstatus_busy = phy_cfg->vstatus_busy;
>  		phy_parameter->phy_reg.new_reg_req = phy_cfg->new_reg_req;

[Severity: High]
This isn't a bug introduced by this patch, but looking at the earlier lines in
this loop:

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

Could we add a check to verify if of_iomap() returns NULL? Dereferencing a
NULL pointer during subsequent I/O operations will cause a kernel panic
during device probe.

[Severity: High]
This is a pre-existing issue, but does this code cause a resource leak?

The of_iomap() function is called inside a for loop that iterates
rtk_phy->num_phy times. For configurations with multiple PHYs (like
rtd1395_phy_cfg_2port), this maps the exact same device tree resources
multiple times.

There appear to be no corresponding iounmap() calls in the driver's error
handling paths or the remove function, which could lead to a permanent
vmalloc space resource leak upon driver unbind or probe failure.

[Severity: High]
This is a pre-existing issue, but could this pointer arithmetic lead to
unaligned memory accesses?

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

Since of_iomap() returns a void __iomem *, pointer arithmetic is performed
in single bytes. For multi-PHY configurations where index is 1, this
increments the base address by exactly 1 byte, resulting in an unaligned
32-bit offset that could cause an alignment fault during writel().

> +		phy_parameter->phy_reg.read = phy_cfg->read;
> +		phy_parameter->phy_reg.write = phy_cfg->write;
>  
>  		if (of_property_read_bool(np, "realtek,inverse-hstx-sync-clock"))
>  			phy_parameter->inverse_hstx_sync_clock = true;

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