[PATCH v6 1/6] phy: realtek: usb2: introduce vstatus/new_reg_req variables to driver data

[sashiko-bot at kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the driver fails to check the return value of `of_iomap()`, which can lead to a NULL pointer dereference.
- [High] This problem wasn't introduced by this patch, but incorrect pointer arithmetic on a `void __iomem *` pointer results in unaligned I/O memory accesses for multi-port setups.
- [High] This problem wasn't introduced by this patch, but I/O memory mappings created by `of_iomap()` are never unmapped, leading to resource leaks.
- [High] This problem wasn't introduced by this patch, but multi-port PHY configurations concurrently modify shared hardware registers without synchronization or a read-modify-write cycle.
--

commit 734507efb75a6be151dcde389bfc7f0e1df95132
Author: Rustam Adilov <adilov at disroot.org>

phy: realtek: usb2: introduce vstatus/new_reg_req variables to driver data

This commit introduces vstatus_offset, vstatus_busy, and new_reg_req variables 
to the driver data to handle different register offsets and busy bit 
definitions in the RTL9607C SoC. These variables are added to the existing 
PHY configuration structures and assigned default values.

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

[ ... ]

> @@ -194,25 +200,26 @@ static int rtk_phy_write(struct phy_reg *phy_reg, char addr, char data)
>  	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 + phy_reg->vstatus_offset);

[Severity: High]
This is a pre-existing issue, but does writing directly to this register
without a read-modify-write cycle overwrite the configuration for other
ports?

In multi-port setups, all PHY instances appear to share this same base
address, and a direct writel() could clear the bits belonging to other PHYs.

Is there also a need for synchronization to prevent concurrent writes across
different ports?

>  
> -	ret = utmi_wait_register(reg_gusb2phyacc0, PHY_VSTS_BUSY, 0);
> +	ret = utmi_wait_register(reg_gusb2phyacc0, phy_reg->vstatus_busy, 0);
>  	if (ret)
>  		return ret;

[ ... ]

> @@ -974,6 +982,9 @@ 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 isn't a bug introduced by this patch, but does the driver need to
check the return value of of_iomap() here?

If the mapping fails, this would leave the pointers as NULL and cause a
dereference during I/O operations.

[Severity: High]
This is a pre-existing issue, but does adding index directly to the
void __iomem * returned by of_iomap() result in exactly a 1-byte offset
when index > 0?

If so, this could cause an unaligned memory access fault on ARM
architectures. Should the index be scaled appropriately?

[Severity: High]
This is also a pre-existing issue, but does the driver leak these I/O
mappings?

The driver maps the memory here using of_iomap(), but there does not
appear to be a matching iounmap() in the driver cleanup paths. Would
using devm_of_iomap() prevent this leak?

>  		phy_parameter->phy_reg.vstatus_index = index;
> +		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;
>  
>  		if (of_property_read_bool(np, "realtek,inverse-hstx-sync-clock"))

[ ... ]

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