Re: [PATCH v3 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:

Pre-existing issues:
- [High] Data race in `ufs_dwc_phy_reg_write` and `ufs_dwc_phy_reg_read` due to shared mutable `static` arrays
--

commit 0483c36fe85ab166d063a9b9c880720ee52b5d3b
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 renames the AMD Versal2 UFS read/write PHY register APIs and moves
them to the common ufshcd-dwc.c file. This allows other designware UFS PHY
vendors to reuse the PHY register read/write functions.

> 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[] = {

[Severity: High]
This isn't a bug introduced by this patch, but moving it from the specific
versal2 driver to the generic ufshcd-dwc.c library makes it affect all DWC
UFS controllers.

Since the phy_write_attrs array in ufs_dwc_phy_reg_write() is declared static
but is mutated on every function call below, could this cause a data race?

If multiple UFS host controllers in a system execute PHY configuration or
power state changes concurrently (e.g., during asynchronous device probing),
they could overwrite each other's attribute values in this globally shared
array. This might result in writing incorrect values to the UFS PHY
registers, potentially leading to link instability or storage failures.

> +		{ 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);
> +
> +	return ufshcd_dwc_dme_set_attrs(hba, phy_write_attrs, ARRAY_SIZE(phy_write_attrs));
> +}
> +EXPORT_SYMBOL(ufs_dwc_phy_reg_write);
> +
> +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[] = {

[Severity: High]
This is a pre-existing issue, but similar to the above, the phy_read_attrs
array in ufs_dwc_phy_reg_read() is mutated on every function call.

Could multiple controllers executing concurrently overwrite each other's
values here, leading to incorrect UFS PHY register reads?

> +		{ 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);

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