Re: [PATCH v2 03/10] ufs: host: Add common Hibern8 TX FSM polling helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The polling loop in `ufshcd_dme_check_tx_hibern8()` incorrectly conflates a DME communication timeout with the TX lane not yet being in the HIBERN8 state, causing redundant retries on hard failures.
--

commit 36540d76db2dafb48398ab3a254711f31858e35f
Author: Larisa Grigore <larisa.grigore@oss.nxp.com>

ufs: host: Add common Hibern8 TX FSM polling helper

This commit factors out the common logic used to poll TX_FSM_STATE until
the TX lanes enter Hibern8 into a single helper, ufshcd_dme_check_tx_hibern8().
It updates the HiSilicon and Qualcomm drivers to use this new helper and
normalizes the error path to return -ETIMEDOUT.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 2ba244cf40ac7..e5facc0a59a79 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -4448,6 +4448,84 @@ int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
>  }
>  EXPORT_SYMBOL_GPL(ufshcd_dme_get_attr);
>  
> +/**
> + * ufshcd_poll_tx_hibern8_lanes - Check TX_FSM_STATE of all TX lanes once
> + * @hba: host controller instance
> + * @num_lanes: number of TX lanes to check
> + *
> + * Read TX_FSM_STATE for every lane and verify it reached Hibern8.
> + *
> + * Return: 0 if all lanes are in Hibern8, -ETIMEDOUT if any lane is not
> + * (yet) in Hibern8, or a negative errno if the attribute read fails.
> + */
> +static int ufshcd_poll_tx_hibern8_lanes(struct ufs_hba *hba,
> +					unsigned int num_lanes)
> +{
> +	u32 tx_fsm_val = 0;
> +	unsigned int i;
> +	int err;
> +
> +	for (i = 0; i < num_lanes; i++) {
> +		err = ufshcd_dme_get(hba,
> +				UIC_ARG_MIB_SEL(TX_FSM_STATE,
> +					UIC_ARG_MPHY_TX_GEN_SEL_INDEX(i)),
> +				&tx_fsm_val);
> +		if (err) {
> +			dev_err(hba->dev,
> +				"%s: unable to get TX_FSM_STATE for lane %u, err %d\n",
> +				__func__, i, err);
> +			return err;
> +		}
> +
> +		if (tx_fsm_val != TX_STATE_HIBERN8)
> +			return -ETIMEDOUT;

[Severity: Medium]
Since ufshcd_dme_get() returns -ETIMEDOUT on a hard UIC command timeout, is it
safe to overload -ETIMEDOUT here to mean "state not reached"? Could this cause
the caller to conflate a simple retry condition with a hard hardware
communication failure?

> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * ufshcd_dme_check_tx_hibern8 - Check if all TX lanes entered Hibern8 state
> + * @hba: host controller instance
> + * @num_lanes: number of TX lanes to check
> + * @timeout_ms: timeout in milliseconds for all lanes
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int ufshcd_dme_check_tx_hibern8(struct ufs_hba *hba, unsigned int num_lanes,
> +				unsigned int timeout_ms)
> +{
> +	unsigned long timeout;
> +	int err;
> +
> +	if (!num_lanes)
> +		return -EINVAL;
> +
> +	timeout = jiffies + msecs_to_jiffies(timeout_ms);
> +
> +	do {
> +		err = ufshcd_poll_tx_hibern8_lanes(hba, num_lanes);
> +		if (err != -ETIMEDOUT)
> +			return err;
> +
> +		/* sleep for max. 200us */
> +		usleep_range(100, 200);
> +	} while (time_before(jiffies, timeout));

[Severity: Medium]
If ufshcd_dme_get() encounters a hard transport communication failure and
returns -ETIMEDOUT, will this loop blindly continue retrying instead of
aborting immediately?

It seems this could significantly multiply the timeout duration on an error
path (e.g., waiting multiple 500ms UIC timeouts) and spam the error log on
each iteration.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914125000.30812-1-larisa.grigore@oss.nxp.com?part=3