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

[Rustam Adilov <adilov@disroot.org>]

On 2026-10-12 17:49, sashiko-bot@kernel.org wrote:
> 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?

This goes into a pre-existing issue territory, i.e not related to this patch
series at all.

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