Re: [PATCH v2 2/3] scsi: ufs: spacemit: k3: Add UFS Host Controller driver

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The host driver asserts the physical device reset pin during Hibern8 (runtime PM suspend), wiping the UFS device state.
- [High] Failure error codes from essential DME configurations and M-PHY initialization are silently ignored.
- [High] Missing Power Management (PM) operations in the platform driver.
- [Medium] Bitwise OR is used on negative integer error codes from `ufshcd_dme_get`.
--

--- Patch [2]: [PATCH v2 2/3] scsi: ufs: spacemit: k3: Add UFS Host Controller driver ---
commit 7cc317e95f9d6ed1949a4f8f4ae915ca4f1bb1f3
Author: Yixun Lan <[email protected]>

scsi: ufs: spacemit: k3: Add UFS Host Controller driver

This commit adds a new UFS Host Controller driver for the SpacemiT K3 SoC.
The controller supports features compatible with JEDEC UFS 2.2, MIPI UniPro
v1.61 and M-PHY v3.0 standards.

> diff --git a/drivers/ufs/host/ufs-spacemit.c b/drivers/ufs/host/ufs-spacemit.c
> new file mode 100644
> index 0000000000000..30e5092071af8
> --- /dev/null
> +++ b/drivers/ufs/host/ufs-spacemit.c

[ ... ]

> +static int ufs_spacemit_check_hibern8(struct ufs_hba *hba)
> +{
> +	u32 tx_fsm_val_0 = 0;
> +	u32 tx_fsm_val_1 = 0;
> +	int retries = DIV_ROUND_UP(HBRN8_POLL_TOUT_MS * 1000, 100);
> +	int err = 0;
> +
> +	do {
> +		err = ufshcd_dme_get(hba,
> +				     UIC_ARG_MIB_SEL(MPHY_TX_FSM_STATE,
> +						     UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0)),
> +				     &tx_fsm_val_0);
> +		err |= ufshcd_dme_get(hba,
> +				      UIC_ARG_MIB_SEL(MPHY_TX_FSM_STATE,
> +						      UIC_ARG_MPHY_TX_GEN_SEL_INDEX(1)),
> +				      &tx_fsm_val_1);

[Severity: Medium]
Does using the bitwise OR operator here corrupt the error codes?

ufshcd_dme_get() returns standard negative error codes like -EINVAL. Performing
a bitwise OR on two negative two's complement integers produces a mathematically
meaningless negative value, which will destroy the original error values.

[ ... ]

> +static int ufs_spacemit_link_startup_notify(struct ufs_hba *hba,
> +					    enum ufs_notify_change_status status)
> +{
> +	switch (status) {
> +	case PRE_CHANGE:
> +		ufs_spacemit_link_startup_pre_change(hba);
> +		break;
> +	case POST_CHANGE:
> +		ufs_spacemit_link_startup_post_change(hba);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Is it safe to ignore the return values from the link startup phases?

If M-PHY initialization fails (e.g., PLL lock timeout) or DME configurations
fail during ufs_spacemit_link_startup_pre_change() or post_change(), the
failure is masked from the core because this function always returns 0.

Could this cause obscure link failures or timeouts later when the hardware
is in an uninitialized or inconsistent state?

[ ... ]

> +static void ufs_spacemit_post_hibern8(struct ufs_hba *hba, enum uic_cmd_dme cmd)
> +{
> +	static const struct ufshcd_dme_attr_val setup_attrs[] = {
> +		{ UIC_ARG_MIB_SEL(0xf1, UIC_ARG_MPHY_RX_GEN_SEL_INDEX(0)), 0x84 },
> +		{ UIC_ARG_MIB_SEL(0xf1, UIC_ARG_MPHY_RX_GEN_SEL_INDEX(1)), 0x84 },
> +		{ UIC_ARG_MIB_SEL(0xf1, UIC_ARG_MPHY_RX_GEN_SEL_INDEX(0)), 0x80 },
> +		{ UIC_ARG_MIB_SEL(0xf1, UIC_ARG_MPHY_RX_GEN_SEL_INDEX(1)), 0x80 },
> +		{ UIC_ARG_MIB(0xdd), 0x57 },
> +		{ UIC_ARG_MIB(0xdd), 0xd7 },
> +		{ UIC_ARG_MIB(0xe8), 0x57 },
> +		{ UIC_ARG_MIB(0xe8), 0xd7 },
> +	};
> +
> +	if (cmd == UIC_CMD_DME_HIBER_ENTER) {
> +		ufs_spacemit_check_hibern8(hba);
> +
> +		ufs_spacemit_dme_set(hba, setup_attrs, ARRAY_SIZE(setup_attrs));
> +
> +		/* Power down M-PHY */
> +		ufshcd_writel(hba, 0x0, UFS_PHY_MNG_BASE + UFS_MPHY_PU_CTRL);
> +
> +		/* Keep reference clock enabled, assert device reset */
> +		ufshcd_writel(hba, MPHY_DEVICE_RESET_ASSERT,
> +			      UFS_PHY_MNG_BASE + UFS_DEVICE_IO_CTRL);

[Severity: High]
Does asserting the hardware reset pin during Hibern8 wipe the device state?

Looking at ufs_spacemit_device_reset(), UFS_DEVICE_IO_CTRL controls the physical
UFS device hardware reset pin (RST_n). Asserting a hard reset during Hibern8
(such as during runtime suspend) wipes the device's internal state and LUN
contexts.

Will the device fail to respond when the core attempts to exit Hibern8
without a full LinkStartup sequence?

[ ... ]

> +static struct platform_driver ufs_spacemit_pltform = {
> +	.probe	= ufs_spacemit_probe,
> +	.remove	= ufs_spacemit_remove,
> +	.driver	= {
> +		.name	= "ufshcd-spacemit",
> +		.of_match_table = of_match_ptr(ufs_spacemit_of_match),
> +	},
> +};
> +module_platform_driver(ufs_spacemit_pltform);

[Severity: High]
Are we missing the power management operations for this platform driver?

Without assigning the .pm field (e.g., to &ufshcd_pltfrm_pm_ops), the UFS host
controller will not participate in system suspend/resume or runtime PM, which
could lead to failure to transition power states or battery drain.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2