Re: [PATCH v2] scsi: ufs: core: Quiesce SCSI devices instead of host tagset during error handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Switching from blk_mq_unquiesce_tagset() to shost_for_each_device() permanently freezes queues of devices being removed, deadlocking the removal path.
--

commit b89bd79e56222f4a0cb23f7f3bd5ee7ab397b8c4
Author: Stanley Jhu <stanleyjhu@google.com>

scsi: ufs: core: Quiesce SCSI devices instead of host tagset during error handling

This commit changes UFS error handling to quiesce only logical units instead
of the entire host tagset. By iterating over active devices, it allows internal
commands to be dispatched on the pseudo SCSI device during error recovery,
preventing a circular wait deadlock.

Link: https://lore.kernel.org/all/6f78c4bd-a70b-402d-abfd-599091b67674@acm.org/

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 234e18b5078f2..b5c63e88af140 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
[ ... ]
> @@ -6805,6 +6805,23 @@ static void ufshcd_clk_scaling_suspend(struct ufs_hba *hba, bool suspend)
[ ... ]
> +static void ufshcd_unquiesce_all_sdevs(struct ufs_hba *hba)
> +{
> +	struct scsi_device *sdev;
> +
> +	shost_for_each_device(sdev, hba->host)
> +		blk_mq_unquiesce_queue(sdev->request_queue);

[Severity: High]
Will this loop skip devices that are in the process of being removed, leaving
their request queues permanently quiesced?

Looking at __scsi_iterate_devices(), which is called by shost_for_each_device(),
it uses scsi_device_get() to acquire a reference to the next device.

If a device is in the SDEV_CANCEL state due to a concurrent removal event
(such as sysfs or hot-unplug), scsi_device_get() returns a negative error.
This causes __scsi_iterate_devices() to skip the device and continue searching:

drivers/scsi/scsi.c:__scsi_iterate_devices() {
    ...
    if (!scsi_device_is_pseudo_dev(next) && !scsi_device_get(next))
        break;
    ...
}

If a device removal occurs concurrently with an error event,
ufshcd_err_handling_prepare() might successfully quiesce the device's queue
before or during its transition to SDEV_CANCEL.

Later, when ufshcd_err_handling_unprepare() calls ufshcd_unquiesce_all_sdevs(),
the removed device is skipped, and its queue is never unquiesced.

The device removal thread will eventually call device_del() -> sd_remove() ->
sd_shutdown() -> sd_sync_cache(). This issues a SYNCHRONIZE CACHE command via
blk_execute_rq().

Since the queue is permanently quiesced, the request is never dispatched and
the block layer timeout timer never starts.

Could this cause the removal thread to deadlock indefinitely in blk_wait_io()
waiting for the completion of the SYNCHRONIZE CACHE command?

> +}
> +
>  static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
>  {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912094107.2098303-1-stanleyjhu@google.com?part=1