linux-kernel - Re: [PATCH V1 1/2] ufs: core:Add vendor-specific callbacks and update setup_xfer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d027689e-9c45-4584-ac35-411b74b551a9@acm.org>
Date: Tue, 14 Oct 2025 09:10:22 -0700
From: Bart Van Assche <bvanassche@....org>
To: palash.kambar@....qualcomm.com, mani@...nel.org, alim.akhtar@...sung.com,
 avri.altman@....com, peter.griffin@...aro.org, krzk@...nel.org,
 peter.wang@...iatek.com, beanhuo@...ron.com, quic_nguyenb@...cinc.com,
 adrian.hunter@...el.com, ebiggers@...nel.org, neil.armstrong@...aro.org,
 James.Bottomley@...senPartnership.com, martin.petersen@...cle.com
Cc: linux-arm-msm@...r.kernel.org, linux-scsi@...r.kernel.org,
 linux-kernel@...r.kernel.org, quic_cang@...cinc.com,
 quic_nitirawa@...cinc.com
Subject: Re: [PATCH V1 1/2] ufs: core:Add vendor-specific callbacks and update
 setup_xfer_req interface

On 10/13/25 11:04 PM, palash.kambar@....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.