Re: [PATCH v2 06/11] scsi: ufs: core: Add support to retrain TX Equalization via debugfs

[Can Guo <can.guo@oss.qualcomm.com>]

On 3/5/2026 10:11 PM, Bart Van Assche wrote:
> On 3/4/26 7:53 AM, Can Guo wrote:
>> +    ret = kstrtoint_from_user(buf, count, 0, &val);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (val != 1)
>> +        return -EINVAL;
>
> Why does "1" have to be written into the "retrain_tx_eq" attribute to
> trigger retraining? Nobody will know that "1" has to be written into
> this attribute without reading the code. I propose to accept strings
> for this attribute, e.g. "retrain" to trigger retraining. I expect that
> this will make shell scripts that write into this attribute easier to
> read.
Thanks for the suggestion, point taken. Will use string "retrain" as
input for the trigger.
>
>> +int ufshcd_retrain_tx_eq(struct ufs_hba *hba, u32 gear)
>> +{
>> +    struct ufs_pa_layer_attr new_pwr_info, final_params = { 0 };
>> +    int ret;
>
> The recommended style for zero-initializing data structures is "{}"
> instead of "{ 0 }". The initializer "{}" doesn't trigger any compiler
> warnings if the first member of a data structure is a pointer. A
> compiler warning will be triggered when using "{ 0 }" and the first
> member of a data structure is a pointer.
Thanks for letting me know.
>
>> +    ret = ufshcd_pause_command_processing(hba, 1 * USEC_PER_SEC);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ufshcd_hold(hba);
>
> The ufshcd_hold() call probably should come before the
> ufshcd_pause_command_processing() call to reduce latency.
OK.
>
>> +int ufshcd_pause_command_processing(struct ufs_hba *hba, u64 
>> timeout_us)
>> +{
>> +    int ret = 0;
>> +
>> +    mutex_lock(&hba->host->scan_mutex);
>> +    blk_mq_quiesce_tagset(&hba->host->tag_set);
>> +    down_write(&hba->clk_scaling_lock);
>> +
>> +    if (ufshcd_wait_for_pending_cmds(hba, 1 * USEC_PER_SEC)) {
>> +        ret = -EBUSY;
>> +        up_write(&hba->clk_scaling_lock);
>> +        blk_mq_unquiesce_tagset(&hba->host->tag_set);
>> +        mutex_unlock(&hba->host->scan_mutex);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +void ufshcd_resume_command_processing(struct ufs_hba *hba)
>> +{
>> +    up_write(&hba->clk_scaling_lock);
>> +    blk_mq_unquiesce_tagset(&hba->host->tag_set);
>> +    mutex_unlock(&hba->host->scan_mutex);
>> +}
>
> Because of the "one change per patch" rule, introduction of these two
> helper functions should go into a separate patch.
Sure.

Thanks,
Can Guo.
>
> Thanks,
>
> Bart.