Re: [PATCH v4 06/12] scsi: ufs: core: Add helpers to pause and resume command processing

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] AB-BA deadlock between block layer tagset quiesce and clk_scaling_lock during concurrent device management commands.
- [High] Missing ufshcd_hold() allows concurrent UIC commands and Hibern8 transitions from the clock gating worker.
- [High] Incomplete quiescence allows BSG and TMF commands to disrupt hardware state during paused operations.
--

--- Patch [6]: [PATCH v4 06/12] scsi: ufs: core: Add helpers to pause and resume command processing ---
commit fb401ec0f2366dbaa8f0c8881fcde7ff615bb4d2
Author: Can Guo <[email protected]>

scsi: ufs: core: Add helpers to pause and resume command processing

In preparation for supporting TX Equalization refreshing, introduce helper
functions to safely pause and resume command processing.
ufshcd_pause_command_processing() ensures the host is in a quiescent state,
and ufshcd_resume_command_processing() restores it.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index b2050281f3f89..71bda9dd60729 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
[ ... ]
> @@ -1364,6 +1364,48 @@ static int ufshcd_wait_for_pending_cmds(struct ufs_hba *hba,
>  	return ret;
>  }
>  
> +/**
> + * ufshcd_pause_command_processing - Pause command processing
> + * @hba: per-adapter instance
> + * @timeout_us: timeout in microseconds to wait for pending commands to finish
> + *
> + * This function stops new command submissions and waits for existing commands
> + * to complete.
> + *
> + * Return: 0 on success, %-EBUSY if commands did not finish within @timeout_us.
> + * On failure, all acquired locks are released and the tagset is unquiesced.
> + */
> +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);

[Severity: High]
Does quiescing only the SCSI host tagset leave the independent Task Management
(hba->tmf_queue) and BSG (hba->bsg_queue) queues active?

A userspace application could concurrently issue operations via the BSG device
node, bypassing the scan_mutex and clk_scaling_lock, and write directly to
hardware doorbell registers (REG_UTP_TASK_REQ_DOOR_BELL and REG_UIC_COMMAND)
while the controller is undergoing maintenance.

> +	down_write(&hba->clk_scaling_lock);

[Severity: High]
Does quiescing the tagset before waiting on clk_scaling_lock create an AB-BA
deadlock with concurrent device management commands?

If a concurrent device management operation calls ufshcd_dev_man_lock() and
acquires down_read(&hba->clk_scaling_lock), it then submits a request to the
pseudo_sdev via blk_execute_rq(). 

Because the pseudo_sdev shares the SCSI host tagset which was just quiesced
above, the request is queued but never dispatched. Since it is never
dispatched, the block layer timeout timer never starts. The management
operation waits infinitely in wait_for_completion() holding the
clk_scaling_lock, while this pause function waits infinitely here for the
same lock.

> +
> +	if (ufshcd_wait_for_pending_cmds(hba, timeout_us)) {
> +		ret = -EBUSY;
> +		up_write(&hba->clk_scaling_lock);
> +		blk_mq_unquiesce_tagset(&hba->host->tag_set);
> +		mutex_unlock(&hba->host->scan_mutex);
> +	}
> +
> +	return ret;

[Severity: High]
If this function successfully returns without bumping the PM usage counter
(e.g., via ufshcd_hold()), couldn't the asynchronous ufshcd_gate_work fire?

Once pending SCSI commands complete, the queue becomes idle and active_reqs
drops to zero. This allows the clock gating worker to put the link into
Hibern8 mode and disable clocks, which could disrupt the caller of this pause
function if it attempts low-level PHY operations assuming the link is fully
active.

> +}
> +
> +/**
> + * ufshcd_resume_command_processing - Resume command processing
> + * @hba: per-adapter instance
> + *
> + * This function resumes command submissions.
> + */
> +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);

[Severity: High]
If ufshcd_hold() is added to ufshcd_pause_command_processing(), would a
matching ufshcd_release() be needed here to restore normal clock gating?

> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6