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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <a26d7160-d303-4257-a63a-1e94b61f5331@oss.qualcomm.com>
Date: Sat, 18 Oct 2025 12:19:26 +0530
From: Palash Kambar <palash.kambar@....qualcomm.com>
To: Bart Van Assche <bvanassche@....org>, 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/14/2025 9:40 PM, Bart Van Assche wrote:
> 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? 


Thanks for catching this, will take care of this in next patch set.

>
>> @@ -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. 
>
Sure, will take care of this in next patch set.


>> @@ -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. 

Sure, will take care of this in next patch set.


>
>> 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. 

Sure will update this as per the new change.


Regards,

Palash K