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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Incorrect bitwise logic and inverted polling condition in `ufs_spacemit_hce_enable_notify` cause a guaranteed infinite loop and kernel hang.
- [High] Missing cancellation of `fsm_dump_work` upon driver removal leads to a Use-After-Free vulnerability.
- [High] Asynchronous hardware access in `ufs_spacemit_fsm_dump_work` lacks PM runtime and reset synchronization.
- [Medium] The driver uses massive `mdelay()` calls in sleepable contexts, unnecessarily stalling the CPU.
- [Low] `ufs_spacemit_dump_host_regs()` uses a large GFP_ATOMIC allocation for a single log message that will be truncated.
- [Low] `ufs_spacemit_link_startup_post_change()` fetches a hardware state that is discarded, and its return value is ignored by the caller.
- [Low] `ufs_spacemit_wait_mphy_pll_lock()` open-codes a 10ms polling loop using `udelay(1)`.
--

commit cd15fb1a128c7213cfc272df0441215d19da3d01
Author: Yixun Lan <dlan@kernel.org>

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

This commit introduces the UFS host controller driver for SpacemiT K3 SoC.
It adds PHY initialization, quirks, and power management features for standard
JEDEC UFS 2.2 operations.

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

[ ... ]

> +static void ufs_spacemit_dump_host_regs(struct ufs_hba *hba)
> +{
[ ... ]
> +	buf = kzalloc(VENDOR_DUMP_BUF_SIZE, GFP_ATOMIC);
> +	if (!buf)
> +		return;
[ ... ]
> +	dev_warn(hba->dev, "%s", buf);

[Severity: Low]
Does this 2048-byte allocation need to be GFP_ATOMIC, and will the dev_warn()
print truncate the output? The printk subsystem limits lines to LOG_LINE_MAX
(usually 1024 bytes), which might silently drop the later M-PHY registers in
the buffer.

[ ... ]

> +static int ufs_spacemit_wait_mphy_pll_lock(struct ufs_hba *hba)
> +{
> +	int timeout = MPHY_PLL_LOCK_TIMEOUT_US;
> +	u32 val;
> +
> +	while (timeout-- > 0) {
> +		val = ufshcd_readl(hba, UFS_PHY_MNG_BASE + UFS_MPHY_PU_CTRL);
> +		if (val & MPHY_PLL_LOCK_BIT)
> +			return 0;
> +
> +		udelay(1);
> +	}

[Severity: Low]
Could this open-coded polling loop be replaced with readl_poll_timeout()?

[ ... ]

> +static int ufs_spacemit_link_startup_post_change(struct ufs_hba *hba)
> +{
> +	u32 tx_lanes;
[ ... ]
> +	return ufs_spacemit_get_connected_tx_lanes(hba, &tx_lanes);
> +}
> +
> +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;

[Severity: Low]
Are we discarding the tx_lanes state here? The function
ufs_spacemit_link_startup_post_change() populates the variable but never uses
it, and its return value is ignored by ufs_spacemit_link_startup_notify().

[ ... ]

> +static int ufs_spacemit_apply_dev_quirks(struct ufs_hba *hba)
> +{
[ ... ]
> +	/*LCC_DISABLE*/
> +	mdelay(50);
> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(TX_LCC_ENABLE, UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0)), 0);

[Severity: Medium]
Is it safe to block the CPU completely with mdelay() here? Since
ufs_spacemit_apply_dev_quirks() is called from a sleepable context during
initialization and reset, could this use msleep(50) instead?

[ ... ]

> +static void ufs_spacemit_fsm_dump_work(struct work_struct *work)
> +{
> +	struct ufs_spacemit_host *host = container_of(work, struct ufs_spacemit_host,
> +							  fsm_dump_work);
> +	struct ufs_hba *hba = host->hba;
> +
> +	/* Safe to call blocking functions in workqueue context */
> +	if (ufshcd_is_link_active(hba))
> +		ufs_spacemit_dump_fsm_state(hba);
> +}

[Severity: High]
Does this lockless link state check properly serialize against concurrent
host resets? If the error handler triggers a hardware reset concurrently,
this could access unclocked or resetting hardware since it doesn't acquire
hba->host_sem.

[ ... ]

> +static int ufs_spacemit_init(struct ufs_hba *hba)
> +{
> +	int err = 0;
> +	struct device *dev = hba->dev;
> +	struct ufs_spacemit_host *host;
> +
> +	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
[ ... ]
> +	/* Initialize workqueue for deferred FSM state dump */
> +	INIT_WORK(&host->fsm_dump_work, ufs_spacemit_fsm_dump_work);

[ ... ]

> +static void ufs_spacemit_remove(struct platform_device *pdev)
> +{
> +	ufshcd_pltfrm_remove(pdev);
> +}

[Severity: High]
Will this leak or cause a use-after-free if the driver is unbound while
work is queued? The host structure is managed by devres and will be freed
after ufs_spacemit_remove() completes, but there is no cancel_work_sync()
for host->fsm_dump_work to ensure pending items finish executing before
the struct is destroyed.

[ ... ]

> +static int ufs_spacemit_hce_enable_notify(struct ufs_hba *hba,
> +					  enum ufs_notify_change_status status)
> +{
> +	struct ufs_spacemit_host *host = ufshcd_get_variant(hba);
> +	u32 enable_val, val;
> +
> +	if (status == PRE_CHANGE) {
> +		enable_val = CONTROLLER_ENABLE;
> +
> +		if (hba->caps & UFSHCD_CAP_CRYPTO)
> +			enable_val = CRYPTO_GENERAL_ENABLE | CONTROLLER_ENABLE;
> +
> +		if (!host->first_hce_done) {
> +			host->first_hce_done = true;
> +			dev_dbg(hba->dev, "First HCE enable\n");
> +		} else {
> +			val = ufshcd_readl(hba, REG_CONTROLLER_ENABLE);
> +			if (val == enable_val) {
> +				ufshcd_writel(hba, enable_val & (1 << CONTROLLER_ENABLE),
> +					      REG_CONTROLLER_ENABLE);
> +
> +				while (ufshcd_readl(hba, REG_CONTROLLER_ENABLE) ==
> +				       (enable_val & (1 << CONTROLLER_ENABLE)))
> +					;
> +			}
> +		}
> +	}

[Severity: Critical]
Will this while loop hang indefinitely? 

CONTROLLER_ENABLE is 1, so (1 << CONTROLLER_ENABLE) evaluates to 2.
The expression enable_val & 2 evaluates to 0 (since enable_val is 1 without
crypto).

Writing 0 will disable the controller. The while loop then checks if reading
the register returns 0, which immediately evaluates to true and creates an
infinite loop resulting in a hard kernel lockup.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-08-k3-ufs-support-v1-0-1a64a3ab128f@kernel.org?part=2