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

[Can Guo]

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.