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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] phy: realtek: data race between debugfs and PHY ops corrupts hardware state
--

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

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

This commit introduces a phy_reg_desc struct to the realtek usb2 driver. It
stores per-SoC differences such as register offsets and busy bits. This
avoids duplication in driver data for RTL9607C and RTD SoCs.

> 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);
>  
> -	ret = utmi_wait_register(reg_gusb2phyacc0, PHY_VSTS_BUSY, 0);
> +	ret = utmi_wait_register(reg_gusb2phyacc0, reg_desc->vstatus_busy, 0);
>  	if (ret)
>  		return ret;
>  
> -	/* VCtrl = low nibble of addr, set PHY_NEW_REG_REQ */
> -	val = PHY_NEW_REG_REQ | (GET_LOW_NIBBLE(addr) << PHY_VCTRL_SHIFT);
> +	/* VCtrl = low nibble of addr, set reg_desc->new_reg_req */
> +	val = reg_desc->new_reg_req | (GET_LOW_NIBBLE(addr) << PHY_VCTRL_SHIFT);
>  
>  	writel(val, reg_gusb2phyacc0);

[Severity: Medium]
Can this multi-step state write be interrupted and corrupted by concurrent
debugfs accesses?

A user with root privileges reading the debugfs file parameter via
rtk_usb2_parameter_show() in drivers/phy/realtek/phy-rtk-usb2.c repeatedly
calls rtk_phy_set_page() and rtk_phy_read() without acquiring any
driver-specific lock:

    /* Set page 0 */
    phy_data_page = phy_cfg->page0;
    rtk_phy_set_page(phy_reg, 0);

If the kernel concurrently executes a PHY state change (such as
rtk_phy_connect() or rtk_phy_init()), could their register accesses collide
and corrupt the PHY's internal register access state machine?

Additionally, since struct rtk_phy lacks a reference to struct phy, it
appears the debugfs callback cannot acquire phy->mutex. Is there another way
to synchronize these accesses?

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy