Re: [PATCH v3 3/3] scsi: ufs: starfive: Add UFS support for StarFive JHB100 SoC

[sashiko-bot@kernel.org]

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

New issues:
- [High] Double disable and unprepare of the UFS clock (`ufs_clk`) during driver teardown.
- [High] Resource leak of the `phy_reset` line if PHY initialization fails.

Pre-existing issues:
- [High] Data race in `ufs_dwc_phy_reg_write` due to the use of an unprotected static array.
--

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

scsi: ufs: starfive: Add UFS support for StarFive JHB100 SoC

Add support for the UFS host controller on JHB100 SoC, built on
the Synopsys DWC UFS controller and using UFSHCD platform driver.
This controller requires specific configurations like
M-PHY/RMMI/UniPro

> diff --git a/drivers/ufs/host/ufs-starfive.c b/drivers/ufs/host/ufs-starfive.c
> new file mode 100644
> index 0000000000000..fab65da0507e8
> --- /dev/null
> +++ b/drivers/ufs/host/ufs-starfive.c
> @@ -0,0 +1,297 @@

[ ... ]

> +static int ufs_starfive_phy_config(struct ufs_hba *hba, struct ufs_starfive_host *host)
> +{
> +	static struct ufs_dwc_phy_pair_data phy_data[] = {
> +		{ RAWAONLANEN_DIG_MPLLA_COARSE_TUNE, 0x51},
> +		{ MPLL_SKIPCAL_COARSE_TUNE, 0x51},
> +		{ RX_AFE_ATT_IDAC(0), 0x8a},
> +		{ RX_AFE_ATT_IDAC(1), 0xc2},
> +		{ RX_AFE_CTLE_IDAC(0), 0x8e},
> +		{ RX_AFE_CTLE_IDAC(1), 0x8b},
> +		{ FAST_FLAGS(0), 0x0004 },
> +		{ FAST_FLAGS(1), 0x0004 },
> +		{ RX_ADAPT_DFE(0), 0xa00},
> +		{ RX_ADAPT_DFE(1), 0xa00},
> +	};
> +	struct ufs_dwc_phy_pair_data *data;
> +	int ret, i;
> +
> +	for (i = 0; i < ARRAY_SIZE(phy_data); i++) {
> +		data = &phy_data[i];
> +		ret = ufs_dwc_phy_reg_write(hba, data->addr, data->value);

[Severity: High]
This is a pre-existing issue, but calling ufs_dwc_phy_reg_write() here
appears to expose a data race if multiple Synopsys DWC UFS host controllers
are probed concurrently.

Looking at ufs_dwc_phy_reg_write() in drivers/ufs/host/ufshcd-dwc.c:

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 },
        ...
	phy_write_attrs[0].mib_val = (u8)addr;

Because phy_write_attrs is declared as static and modified before being
passed to ufshcd_dwc_dme_set_attrs(), concurrent invocations will clobber
each other's configuration payloads. 

Can this cause undefined device behavior or link startup failures if multiple
controllers execute PHY initialization simultaneously?

> +		if (ret)
> +			return ret;
> +	}

[ ... ]

> +static int ufs_starfive_phy_init(struct ufs_hba *hba)
> +{

[ ... ]

> +	ret = reset_control_deassert(host->phy_reset);
> +	if (ret) {
> +		dev_err(hba->dev, "Failed to reset phy\n");
> +		return ret;
> +	}
> +
> +	ret = regmap_read_poll_timeout(host->syscon,
> +				       SRAM_STATUS, val,
> +				       (val & SRAM_INIT_DONE),
> +				       MPHY_POLL_INTERVAL_US,
> +				       MPHY_POLL_TIMEOUT_US);
> +	if (ret) {
> +		dev_err(hba->dev, "wait sram init done timeout\n");
> +		return ret;
> +	}

[ ... ]

> +	host->phy_init = 1;
> +
> +	return 0;
> +}

[ ... ]

> +static int ufs_starfive_init(struct ufs_hba *hba)
> +{

[ ... ]

> +	host->ufs_clk = devm_clk_get_enabled(&pdev->dev, "main");
> +	if (IS_ERR(host->ufs_clk))
> +		return dev_err_probe(dev, PTR_ERR(host->ufs_clk),
> +				     "Failed to get ufs clock\n");

[ ... ]

> +static void ufs_starfive_exit(struct ufs_hba *hba)
> +{
> +	struct ufs_starfive_host *host = ufshcd_get_variant(hba);
> +
> +	if (host->phy_init) {
> +		reset_control_assert(host->phy_reset);
> +		host->phy_init = 0;
> +	}

[Severity: High]
Could this leak the phy_reset line in a deasserted state if initialization
fails partway through?

If ufs_starfive_phy_init() successfully deasserts host->phy_reset, but a
subsequent operation (like regmap_read_poll_timeout()) fails, it returns
early and leaves host->phy_init as 0. 

When the probe rollback path subsequently calls ufs_starfive_exit(), it sees
host->phy_init == 0 and skips asserting the reset line. 

Is it possible this leaves the hardware in an invalid state that prevents
subsequent driver probe retries from succeeding?

> +	clk_disable_unprepare(host->ufs_clk);

[Severity: High]
Will this result in an unbalanced clock disable during driver teardown?

In ufs_starfive_init(), host->ufs_clk is acquired using
devm_clk_get_enabled(). This API automatically registers a devres action that
calls clk_disable_unprepare() when the driver is unbound.

By explicitly calling clk_disable_unprepare(host->ufs_clk) here in
ufs_starfive_exit(), it seems this might disable the clock a second time when
the devres framework fires its cleanup action, triggering a kernel warning.

> +
> +	reset_control_assert(host->core_reset);
> +}

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