Re: [PATCH v2 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 5 potential issue(s) to consider:
- [High] Missing PM runtime cleanup in the driver's remove callback.
- [Medium] Unconditional assignment of `dev_pm_ops` structure without `pm_ptr()` wrapper.
- [Medium] Hardware reset lines are deasserted during probe but never asserted on removal.
- [Low] Inappropriate delay function used for microsecond delay.
- [Low] Shared DWC library directly appended to multiple host drivers' Kbuild targets.
--

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

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

This commit adds support for the UFS host controller on JHB100 SoC, built on
the Synopsys DWC UFS controller and using the UFSHCD platform driver.

> diff --git a/drivers/ufs/host/Makefile b/drivers/ufs/host/Makefile
> index 65d8bb23ab7b3..adfee2ae3b486 100644
> --- a/drivers/ufs/host/Makefile
> +++ b/drivers/ufs/host/Makefile
> @@ -14,3 +14,4 @@ obj-$(CONFIG_SCSI_UFS_ROCKCHIP) += ufs-rockchip.o
>  obj-$(CONFIG_SCSI_UFS_SPRD) += ufs-sprd.o
>  obj-$(CONFIG_SCSI_UFS_TI_J721E) += ti-j721e-ufs.o
>  obj-$(CONFIG_SCSI_UFS_AMD_VERSAL2) += ufs-amd-versal2.o ufshcd-dwc.o
> +obj-$(CONFIG_SCSI_UFS_STARFIVE) += ufs-starfive.o ufshcd-dwc.o

This isn't a bug, but is it safe to append ufshcd-dwc.o directly to multiple
independent Kconfig targets?

If one driver is configured as =y and another as =m, could this cause
unexpected linking behavior for the exported symbols in ufshcd-dwc.c?
Would a dedicated Kconfig symbol to manage the shared DWC library be safer?

> diff --git a/drivers/ufs/host/ufs-starfive.c b/drivers/ufs/host/ufs-starfive.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/ufs/host/ufs-starfive.c
[ ... ]
> +static int ufs_starfive_init(struct ufs_hba *hba)
> +{
[ ... ]
> +	regmap_update_bits(host->syscon, UFS_REFCLK,
> +			   REFCLK_OEN | RESET_OEN, 0);
> +	usleep_range(2, 3);

This isn't a bug, but should udelay(2) be used here instead? For delays
shorter than 10us, the context switching and timer setup overhead for
usleep_range() usually heavily outweighs the tiny interval.

> +	regmap_update_bits(host->syscon, UFS_REFCLK, RESET_I, RESET_I);
> +
> +	ret = reset_control_deassert(host->core_reset);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to reset core clock");
[ ... ]
> +static void ufs_starfive_remove(struct platform_device *pdev)
> +{
> +	struct ufs_hba *hba =  platform_get_drvdata(pdev);
> +
> +	pm_runtime_get_sync(&(pdev)->dev);
> +	ufshcd_remove(hba);
> +}

Does this code leak the PM runtime usage counter?

The driver calls pm_runtime_get_sync() to wake the device before calling
ufshcd_remove(), but fails to subsequently call pm_runtime_put_noidle() or
pm_runtime_disable(). Could this permanently leak the usage counter and leave
PM runtime active after the device is unbound?

Additionally, should the hardware reset lines (core_reset and phy_reset)
be asserted here during removal?

While devm_reset_control_get_exclusive() frees the handles on unbind, it
does not automatically assert the hardware reset lines, which might leave the
hardware permanently active after the driver is unloaded.

[ ... ]
> +static struct platform_driver ufs_starfive_driver = {
> +	.probe		= ufs_starfive_probe,
> +	.remove		= ufs_starfive_remove,
> +	.driver		= {
> +		.name	= "ufs-starfive",
> +		.pm	= &ufs_starfive_pm_ops,

Should ufs_starfive_pm_ops be wrapped with the pm_ptr() macro?

Assigning it unconditionally forces the dev_pm_ops structure and its referenced
callbacks to be compiled into the binary as dead code when CONFIG_PM is
disabled.

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