[PATCH] ufs: core: Disable queue depth tracking support

[PATCH] ufs: core: Disable queue depth tracking support

[Bart Van Assche]

Disable queue depth tracking support because:
 - UFS devices shouldn't need queue tracking support. If the host sends
   less than or equal to bQueueDepth commands to the UFS device, no TASK
   SET FULL condition should be reported. If a TASK SET FULL condition
   is reported anyway, the host->host_blocked variable will be set and
   the SCSI command will be requeued. Submission of the SCSI command
   will be retried after another SCSI command has completed.
 - The queue depth tracking code is not thread-safe. With MCQ enabled,
   scsi_track_queue_full() may be called from multiple CPU cores
   concurrently. This is not safe because individual
   scsi_track_queue_full() calls are not serialized.
 - scsi_track_queue_full() is called from interrupt context but not all
   code called by scsi_track_queue_full() is IRQ-safe. Here is an
   example of a call chain that may cause interrupts to be enabled,
   something that is not allowed from interrupt context:

   ufshcd_mcq_poll_cqe_lock()
     ufshcd_compl_one_cqe()
       scsi_done()
         blk_mq_complete_request()
           scsi_complete()
             scsi_decide_disposition()
               scsi_handle_queue_full()
                 scsi_track_queue_full()
                   scsi_change_queue_depth()
                     blk_set_queue_depth()
                       rq_qos_queue_depth_changed()
                         ioc_rqos_queue_depth_changed()
                           spin_lock_irq()
                           spin_unlock_irq() <- enables interrupts

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |  4 ++++
 drivers/ufs/core/ufshcd.c           | 16 ----------------
 2 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index acbd787de265..19a6400d1ec4 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1044,6 +1044,10 @@ static void srp_remove_target(struct srp_target_port *target)
 	WARN_ON_ONCE(target->state != SRP_TARGET_REMOVED);
 
 	srp_del_scsi_host_attr(target->scsi_host);
+	/*
+	 * The srp_remove_host() call decrements the rport reference count.
+	 * Keep the rport as long as its needed.
+	 */
 	srp_rport_get(target->rport);
 	srp_remove_host(target->scsi_host);
 	scsi_remove_host(target->scsi_host);
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 67745d2796d0..59212a3a46de 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -5602,20 +5602,6 @@ static int ufshcd_sdev_init(struct scsi_device *sdev)
 	return 0;
 }
 
-/**
- * ufshcd_change_queue_depth - change queue depth
- * @sdev: pointer to SCSI device
- * @depth: required depth to set
- *
- * Change queue depth and make sure the max. limits are not crossed.
- *
- * Return: new queue depth.
- */
-static int ufshcd_change_queue_depth(struct scsi_device *sdev, int depth)
-{
-	return scsi_change_queue_depth(sdev, min(depth, sdev->host->can_queue));
-}
-
 /**
  * ufshcd_sdev_configure - adjust SCSI device configurations
  * @sdev: pointer to SCSI device
@@ -9662,7 +9648,6 @@ static const struct scsi_host_template ufshcd_driver_template = {
 	.sdev_init		= ufshcd_sdev_init,
 	.sdev_configure		= ufshcd_sdev_configure,
 	.sdev_destroy		= ufshcd_sdev_destroy,
-	.change_queue_depth	= ufshcd_change_queue_depth,
 	.eh_abort_handler	= ufshcd_abort,
 	.eh_device_reset_handler = ufshcd_eh_device_reset_handler,
 	.eh_host_reset_handler   = ufshcd_eh_host_reset_handler,
@@ -9673,7 +9658,6 @@ static const struct scsi_host_template ufshcd_driver_template = {
 	.max_sectors		= SZ_1M / SECTOR_SIZE,
 	.max_host_blocked	= 1,
 	.host_tagset		= true,
-	.track_queue_depth	= 1,
 	.skip_settle_delay	= 1,
 	.sdev_groups		= ufshcd_driver_groups,
 };

Re: [PATCH] ufs: core: Disable queue depth tracking support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Silently disabling userspace's ability to modify the UFS device queue depth via sysfs.
- [Medium] Unrelated and undocumented modification to the InfiniBand SRP target driver (`ib_srp.c`).

Pre-existing issues:
- [High] Unresolved concurrency and IRQ-handling bug in SCSI core (`scsi_track_queue_full`) and Block layer (`ioc_rqos_queue_depth_changed`).
--

--- Patch [1]: [PATCH] ufs: core: Disable queue depth tracking support ---
commit f181b5b33f08308dbca6a708c0dbc602ecd352b0
Author: Bart Van Assche <bvanassche@acm.org>

ufs: core: Disable queue depth tracking support

Disable queue depth tracking support because UFS devices shouldn't need
queue tracking support. The queue depth tracking code is not thread-safe,
and scsi_track_queue_full() is called from interrupt context but not all
code called by it is IRQ-safe.

>      - scsi_track_queue_full() is called from interrupt context but not all
>        code called by scsi_track_queue_full() is IRQ-safe. Here is an
>        example of a call chain that may cause interrupts to be enabled,
>        something that is not allowed from interrupt context:
>     
>        ufshcd_mcq_poll_cqe_lock()
>          ufshcd_compl_one_cqe()
>            scsi_done()
>              blk_mq_complete_request()
>                scsi_complete()
>                  scsi_decide_disposition()
>                    scsi_handle_queue_full()
>                      scsi_track_queue_full()
>                        scsi_change_queue_depth()
>                          blk_set_queue_depth()
>                            rq_qos_queue_depth_changed()
>                              ioc_rqos_queue_depth_changed()
>                                spin_lock_irq()
>                                spin_unlock_irq() <- enables interrupts

[Severity: High]
This is a pre-existing issue, but by only disabling queue depth tracking
locally in the UFS driver, are dozens of other SCSI drivers that set
.track_queue_depth = 1 still exposed to this IRQ-safety bug?

Since ioc_rqos_queue_depth_changed() unconditionally uses spin_unlock_irq(),
and scsi_track_queue_full() has no locking for sdev->last_queue_full_count,
should this concurrency and IRQ unmasking issue be addressed in the SCSI core
and block layers directly?

> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 6b429ef63f8f3..d671a3d3359ba 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -1044,6 +1044,10 @@ static void srp_remove_target(struct srp_target_port *target)
>  	WARN_ON_ONCE(target->state != SRP_TARGET_REMOVED);
>  
>  	srp_del_scsi_host_attr(target->scsi_host);
> +	/*
> +	 * The srp_remove_host() call decrements the rport reference count.
> +	 * Keep the rport as long as its needed.
> +	 */

[Severity: Medium]
Was this modification to the InfiniBand SRP target driver included by
accident?

The commit message only mentions disabling queue depth tracking for UFS, but
this introduces an undocumented change to srp_remove_target().

>  	srp_rport_get(target->rport);
>  	srp_remove_host(target->scsi_host);
>  	scsi_remove_host(target->scsi_host);

[ ... ]

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index d3044a3089b53..bdb926a9f2d75 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c

[ ... ]

> @@ -9580,7 +9566,6 @@ static const struct scsi_host_template ufshcd_driver_template = {
>  	.sdev_init		= ufshcd_sdev_init,
>  	.sdev_configure		= ufshcd_sdev_configure,
>  	.sdev_destroy		= ufshcd_sdev_destroy,
> -	.change_queue_depth	= ufshcd_change_queue_depth,

[Severity: High]
Does removing .change_queue_depth from the template completely disable
userspace's ability to modify the queue depth via sysfs?

Looking at sdev_store_queue_depth(), it returns -EINVAL if this callback is
missing:

drivers/scsi/scsi_sysfs.c:sdev_store_queue_depth() {
    ...
	if (!sht->change_queue_depth)
		return -EINVAL;
    ...
}

Could .change_queue_depth be set to scsi_change_queue_depth instead to
preserve the manual sysfs tuning interface?

>  	.eh_abort_handler	= ufshcd_abort,
>  	.eh_device_reset_handler = ufshcd_eh_device_reset_handler,
>  	.eh_host_reset_handler   = ufshcd_eh_host_reset_handler,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814214951.80673-1-bvanassche@acm.org?part=1