Re: [PATCH V1 1/2] ufs: core:Add vendor-specific callbacks and update setup_xfer_req interface

[Bart Van Assche <bvanassche@acm.org>]

On 10/13/25 11:04 PM, palash.kambar@oss.qualcomm.com wrote:
> On QCOM UFSHC V6 in MCQ mode, a race condition exists where simultaneous
> data and hibernate commands can cause data commands to be dropped when
> the Auto-Hibernate Idle Timer (AHIT) is near expiration.
> 
> To mitigate this, AHIT is disabled before updating the SQ tail pointer,
> and re-enabled only when no active commands remain. This prevents
> conflicting command sequences from reaching the UniPro layer during
> critical timing windows.
> 
> To support this:
> - Introduce a new vendor operation `compl_command` to allow vendors to
>    handle command completion in a customized manner.
> - Update the argument list for the existing `setup_xfer_req` vendor
>    operation to align with the updated UFS core interface.
> - Modify the Exynos-specific `setup_xfer_req` implementation to match
>    the new interface and support the AHIT handling logic.

Yikes. Please disable AHIT entirely or disable/enable AHIT from inside
the runtime power management callbacks rather than inventing a new
mechanism for tracking whether any commands are outstanding.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 568a449e7331..fd771d6c315e 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2383,11 +2383,11 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag,
>   		memcpy(dest, src, utrd_size);
>   		ufshcd_inc_sq_tail(hwq);
>   		spin_unlock(&hwq->sq_lock);
> +		hba->vops->setup_xfer_req(hba, lrbp);

What will happen if hba->vops->setup_xfer_req == NULL? Will the above
code trigger a kernel crash?

> @@ -5637,6 +5637,7 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
>   	}
>   	cmd = lrbp->cmd;
>   	if (cmd) {
> +		hba->vops->compl_command(hba, lrbp);
>   		if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
>   			ufshcd_update_monitor(hba, lrbp);
>   		ufshcd_add_command_trace(hba, task_tag, UFS_CMD_COMP);

Yikes. New unconditional indirect function calls in the hot path are not
acceptable because these have a negative performance impact.

> @@ -5645,6 +5646,7 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
>   		/* Do not touch lrbp after scsi done */
>   		scsi_done(cmd);
>   	} else {
> +		hba->vops->compl_command(hba, lrbp);
>   		if (cqe) {
>   			ocs = le32_to_cpu(cqe->status) & MASK_OCS;
>   			lrbp->utr_descriptor_ptr->header.ocs = ocs;

Same comment here.

> diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
> index 70d195179eba..d87276f45e01 100644
> --- a/drivers/ufs/host/ufs-exynos.c
> +++ b/drivers/ufs/host/ufs-exynos.c
> @@ -910,11 +910,15 @@ static int exynos_ufs_post_pwr_mode(struct ufs_hba *hba,
>   }
>   
>   static void exynos_ufs_specify_nexus_t_xfer_req(struct ufs_hba *hba,
> -						int tag, bool is_scsi_cmd)
> +						struct ufshcd_lrb *lrbp)
>   {
>   	struct exynos_ufs *ufs = ufshcd_get_variant(hba);
>   	u32 type;
> +	int tag;
> +	bool is_scsi_cmd;
>   
> +	tag = lrbp->task_tag;
> +	is_scsi_cmd = !!lrbp->cmd;
>   	type =  hci_readl(ufs, HCI_UTRL_NEXUS_TYPE);
>   
>   	if (is_scsi_cmd)

I'm about to remove lrbp->cmd so please don't introduce any new users of
this structure member.

Bart.