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

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

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.

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

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

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

Thanks,

Bart.