Re: [PATCH v2 2/3] scsi: ufs: dwc: Rename amd-versal2 read/write PHY API and move to dwc common file

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The functions `ufs_dwc_phy_reg_write` and `ufs_dwc_phy_reg_read` use `static` local arrays (`phy_write_attrs` and `phy_read_attrs`) which are modified on every call, creating a severe data race.
--

commit b544cff60ab1cb7a813f07b65debab2ffee3a426
Author: Minda Chen <minda.chen@starfivetech.com>

scsi: ufs: dwc: Rename amd-versal2 read/write PHY API and move to dwc common file

This commit moves the read/write PHY register APIs from the AMD versal2
driver to a common file. This allows other vendors using the designware
UFS MIPI PHY to reuse the code.

> diff --git a/drivers/ufs/host/ufshcd-dwc.c b/drivers/ufs/host/ufshcd-dwc.c
> index 21b1cf912dcc6..b057a78e151c4 100644
> --- a/drivers/ufs/host/ufshcd-dwc.c
> +++ b/drivers/ufs/host/ufshcd-dwc.c
> @@ -15,6 +15,59 @@
>  #include "ufshcd-dwc.h"
>  #include "ufshci-dwc.h"
>  
> +int ufs_dwc_phy_reg_write(struct ufs_hba *hba, u32 addr, u32 val)
> +{
> +	static struct ufshcd_dme_attr_val phy_write_attrs[] = {
> +		{ UIC_ARG_MIB(CBCREGADDRLSB), 0, DME_LOCAL },
> +		{ UIC_ARG_MIB(CBCREGADDRMSB), 0, DME_LOCAL },
> +		{ UIC_ARG_MIB(CBCREGWRLSB), 0, DME_LOCAL },
> +		{ UIC_ARG_MIB(CBCREGWRMSB), 0, DME_LOCAL },
> +		{ UIC_ARG_MIB(CBCREGRDWRSEL), 1, DME_LOCAL },
> +		{ UIC_ARG_MIB(VS_MPHYCFGUPDT), 1, DME_LOCAL }
> +	};
> +
> +	phy_write_attrs[0].mib_val = (u8)addr;
> +	phy_write_attrs[1].mib_val = (u8)(addr >> 8);
> +	phy_write_attrs[2].mib_val = (u8)val;
> +	phy_write_attrs[3].mib_val = (u8)(val >> 8);

Could moving this function into common code expose a data race regression?

Since phy_write_attrs is declared static, it is allocated in global memory and
shared across all threads and UFS device instances.

If multiple host controllers probe asynchronously or undergo concurrent power
state changes, will they clobber each other's register addresses and values
here before calling ufshcd_dwc_dme_set_attrs?

Would it be safer to remove the static keyword so this structure is safely
allocated on the stack per-invocation?

[ ... ]

> +int ufs_dwc_phy_reg_read(struct ufs_hba *hba, u32 addr, u32 *val)
> +{
> +	u32 mib_val;
> +	int ret;
> +	static struct ufshcd_dme_attr_val phy_read_attrs[] = {
> +		{ UIC_ARG_MIB(CBCREGADDRLSB), 0, DME_LOCAL },
> +		{ UIC_ARG_MIB(CBCREGADDRMSB), 0, DME_LOCAL },
> +		{ UIC_ARG_MIB(CBCREGRDWRSEL), 0, DME_LOCAL },
> +		{ UIC_ARG_MIB(VS_MPHYCFGUPDT), 1, DME_LOCAL }
> +	};
> +
> +	phy_read_attrs[0].mib_val = (u8)addr;
> +	phy_read_attrs[1].mib_val = (u8)(addr >> 8);

Does this static array have the same concurrency issue as phy_write_attrs
above?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260509062759.125472-1-minda.chen@starfivetech.com?part=2