[PATCH] ata: libata-sata: Simplify ata_change_queue_depth()

[PATCH] ata: libata-sata: Simplify ata_change_queue_depth()

[Damien Le Moal]

Commit 141f3d6256e5 ("ata: libata-sata: Fix device queue depth control")
added a struct ata_device argument to ata_change_queue_depth() to
address problems with changing the queue depth of ATA devices managed
through libsas. This was due to problems with ata_scsi_find_dev() which
are now fixed with commit 7f875850f20a ("ata: libata-scsi: Use correct
device no in ata_find_dev()").

Undo some of the changes of commit 141f3d6256e5: remove the added struct
ata_device aregument and use again ata_scsi_find_dev() to find the
target ATA device structure. While doing this, also make sure that
ata_scsi_find_dev() is called with ap->lock held, as it should.

libsas and libata call sites of ata_change_queue_depth() are updated to
match the modified function arguments.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-sata.c           | 19 ++++++++++---------
 drivers/scsi/libsas/sas_scsi_host.c |  3 +--
 include/linux/libata.h              |  4 ++--
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index f3e7396e3191..e3c9cb617048 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1023,7 +1023,6 @@ EXPORT_SYMBOL_GPL(dev_attr_sw_activity);
 /**
  *	ata_change_queue_depth - Set a device maximum queue depth
  *	@ap: ATA port of the target device
- *	@dev: target ATA device
  *	@sdev: SCSI device to configure queue depth for
  *	@queue_depth: new queue depth
  *
@@ -1031,24 +1030,27 @@ EXPORT_SYMBOL_GPL(dev_attr_sw_activity);
  *	and libata.
  *
  */
-int ata_change_queue_depth(struct ata_port *ap, struct ata_device *dev,
-			   struct scsi_device *sdev, int queue_depth)
+int ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
+			   int queue_depth)
 {
+	struct ata_device *dev;
 	unsigned long flags;
 
-	if (!dev || !ata_dev_enabled(dev))
-		return sdev->queue_depth;
+	spin_lock_irqsave(ap->lock, flags);
 
-	if (queue_depth < 1 || queue_depth == sdev->queue_depth)
+	dev = ata_scsi_find_dev(ap, sdev);
+	if (!dev || queue_depth < 1 || queue_depth == sdev->queue_depth) {
+		spin_unlock_irqrestore(ap->lock, flags);
 		return sdev->queue_depth;
+	}
 
 	/* NCQ enabled? */
-	spin_lock_irqsave(ap->lock, flags);
 	dev->flags &= ~ATA_DFLAG_NCQ_OFF;
 	if (queue_depth == 1 || !ata_ncq_enabled(dev)) {
 		dev->flags |= ATA_DFLAG_NCQ_OFF;
 		queue_depth = 1;
 	}
+
 	spin_unlock_irqrestore(ap->lock, flags);
 
 	/* limit and apply queue depth */
@@ -1082,8 +1084,7 @@ int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth)
 {
 	struct ata_port *ap = ata_shost_to_port(sdev->host);
 
-	return ata_change_queue_depth(ap, ata_scsi_find_dev(ap, sdev),
-				      sdev, queue_depth);
+	return ata_change_queue_depth(ap, sdev, queue_depth);
 }
 EXPORT_SYMBOL_GPL(ata_scsi_change_queue_depth);
 
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index a36fa1c128a8..94c5f14f3c16 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -872,8 +872,7 @@ int sas_change_queue_depth(struct scsi_device *sdev, int depth)
 	struct domain_device *dev = sdev_to_domain_dev(sdev);
 
 	if (dev_is_sata(dev))
-		return ata_change_queue_depth(dev->sata_dev.ap,
-					      sas_to_ata_dev(dev), sdev, depth);
+		return ata_change_queue_depth(dev->sata_dev.ap, sdev, depth);
 
 	if (!sdev->tagged_supported)
 		depth = 1;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 01f9fbb69f89..bc756f8586f3 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1144,8 +1144,8 @@ extern int ata_scsi_slave_config(struct scsi_device *sdev);
 extern void ata_scsi_slave_destroy(struct scsi_device *sdev);
 extern int ata_scsi_change_queue_depth(struct scsi_device *sdev,
 				       int queue_depth);
-extern int ata_change_queue_depth(struct ata_port *ap, struct ata_device *dev,
-				  struct scsi_device *sdev, int queue_depth);
+extern int ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
+				  int queue_depth);
 extern struct ata_device *ata_dev_pair(struct ata_device *adev);
 extern int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev);
 extern void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap);
-- 
2.40.1

Re: [PATCH] ata: libata-sata: Simplify ata_change_queue_depth()

[Jason Yan]

On 2023/6/2 6:26, Damien Le Moal wrote:
> Commit 141f3d6256e5 ("ata: libata-sata: Fix device queue depth control")
> added a struct ata_device argument to ata_change_queue_depth() to
> address problems with changing the queue depth of ATA devices managed
> through libsas. This was due to problems with ata_scsi_find_dev() which
> are now fixed with commit 7f875850f20a ("ata: libata-scsi: Use correct
> device no in ata_find_dev()").
> 
> Undo some of the changes of commit 141f3d6256e5: remove the added struct
> ata_device aregument and use again ata_scsi_find_dev() to find the
> target ATA device structure. While doing this, also make sure that
> ata_scsi_find_dev() is called with ap->lock held, as it should.
> 
> libsas and libata call sites of ata_change_queue_depth() are updated to
> match the modified function arguments.
> 
> Signed-off-by: Damien Le Moal<dlemoal@kernel.org>
> ---
>   drivers/ata/libata-sata.c           | 19 ++++++++++---------
>   drivers/scsi/libsas/sas_scsi_host.c |  3 +--
>   include/linux/libata.h              |  4 ++--
>   3 files changed, 13 insertions(+), 13 deletions(-)

Reviewed-by: Jason Yan <yanaijie@huawei.com>

Thanks,
Jason

Re: [PATCH] ata: libata-sata: Simplify ata_change_queue_depth()

[John Garry]

On 01/06/2023 23:26, Damien Le Moal wrote:
> Commit 141f3d6256e5 ("ata: libata-sata: Fix device queue depth control")
> added a struct ata_device argument to ata_change_queue_depth() to
> address problems with changing the queue depth of ATA devices managed
> through libsas. This was due to problems with ata_scsi_find_dev() which
> are now fixed with commit 7f875850f20a ("ata: libata-scsi: Use correct
> device no in ata_find_dev()").
> 
> Undo some of the changes of commit 141f3d6256e5: remove the added struct
> ata_device aregument and use again ata_scsi_find_dev() to find the
> target ATA device structure. While doing this, also make sure that
> ata_scsi_find_dev() is called with ap->lock held, as it should.
> 
> libsas and libata call sites of ata_change_queue_depth() are updated to
> match the modified function arguments.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Reviewed-by: John Garry <john.g.garry@oracle.com>

Thanks!

Just a reminder to all - I'm not asking anyone to fix it - we still have 
that funky libsas behaviour for attempting to set queue depth at 33:

https://lore.kernel.org/linux-scsi/db84e61a-1069-982a-5659-297fcffc14f4@huawei.com/

> ---
>   drivers/ata/libata-sata.c           | 19 ++++++++++---------
>   drivers/scsi/libsas/sas_scsi_host.c |  3 +--
>   include/linux/libata.h              |  4 ++--
>   3 files changed, 13 insertions(+), 13 deletions(-)
> 

...

> +			   int queue_depth)
>   {
> +	struct ata_device *dev;
>   	unsigned long flags;
>   
> -	if (!dev || !ata_dev_enabled(dev))
> -		return sdev->queue_depth;
> +	spin_lock_irqsave(ap->lock, flags);
>   
> -	if (queue_depth < 1 || queue_depth == sdev->queue_depth)
> +	dev = ata_scsi_find_dev(ap, sdev);
> +	if (!dev || queue_depth < 1 || queue_depth == sdev->queue_depth) {
> +		spin_unlock_irqrestore(ap->lock, flags);
>   		return sdev->queue_depth;
> +	}
>   

>   	/* NCQ enabled? */
> -	spin_lock_irqsave(ap->lock, flags);
>   	dev->flags &= ~ATA_DFLAG_NCQ_OFF;
>   	if (queue_depth == 1 || !ata_ncq_enabled(dev)) {

Apart from this change, should we call ata_ncq_supported() here (instead 
of ata_ncq_enabled())? ata_ncq_enabled() checks if ATA_DFLAG_NCQ_OFF is 
not set, which we have already ensured.

>   		dev->flags |= ATA_DFLAG_NCQ_OFF;
>   		queue_depth = 1;
>   	}
> +

Re: [PATCH] ata: libata-sata: Simplify ata_change_queue_depth()

[Damien Le Moal]

On 6/2/23 17:02, John Garry wrote:
> On 01/06/2023 23:26, Damien Le Moal wrote:
>> Commit 141f3d6256e5 ("ata: libata-sata: Fix device queue depth control")
>> added a struct ata_device argument to ata_change_queue_depth() to
>> address problems with changing the queue depth of ATA devices managed
>> through libsas. This was due to problems with ata_scsi_find_dev() which
>> are now fixed with commit 7f875850f20a ("ata: libata-scsi: Use correct
>> device no in ata_find_dev()").
>>
>> Undo some of the changes of commit 141f3d6256e5: remove the added struct
>> ata_device aregument and use again ata_scsi_find_dev() to find the
>> target ATA device structure. While doing this, also make sure that
>> ata_scsi_find_dev() is called with ap->lock held, as it should.
>>
>> libsas and libata call sites of ata_change_queue_depth() are updated to
>> match the modified function arguments.
>>
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> 
> Reviewed-by: John Garry <john.g.garry@oracle.com>
> 
> Thanks!
> 
> Just a reminder to all - I'm not asking anyone to fix it - we still have 
> that funky libsas behaviour for attempting to set queue depth at 33:
> 
> https://lore.kernel.org/linux-scsi/db84e61a-1069-982a-5659-297fcffc14f4@huawei.com/

I checked that again. For a libsas device, with this patch applied, we have:

# echo 1 > /sys/block/sdg/device/queue_depth
# cat /sys/block/sdg/device/queue_depth
1
# echo 33 > /sys/block/sdg/device/queue_depth; echo $?
0		<-- success !
# cat /sys/block/sdg/device/queue_depth
32		<-- qd was capped
# echo 33 > /sys/block/sdg/device/queue_depth; echo $?
1		<-- error !
# cat /sys/block/sdg/device/queue_depth
32		<-- no change

For a libata device, we have:

# echo 1 > /sys/block/sdc/device/queue_depth
# cat /sys/block/sdc/device/queue_depth
1
# echo 33 > /sys/block/sdc/device/queue_depth
# echo $?
1		<-- error !
# cat /sys/block/sdc/device/queue_depth
1		<-- no change

This is not consistent. The attempt to change from 1 to 33 with libsas should
error and not change anything.

That is because sdev->host->can_queue is larger than 32 for libsas devices as it
indicates the number of commands that the HBA can queue rather than the device
max queue depth.


>>   	/* NCQ enabled? */
>> -	spin_lock_irqsave(ap->lock, flags);
>>   	dev->flags &= ~ATA_DFLAG_NCQ_OFF;
>>   	if (queue_depth == 1 || !ata_ncq_enabled(dev)) {
> 
> Apart from this change, should we call ata_ncq_supported() here (instead 
> of ata_ncq_enabled())? ata_ncq_enabled() checks if ATA_DFLAG_NCQ_OFF is 
> not set, which we have already ensured.

Good catch. ata_ncq_enabled() calls ata_ncq_supported(), so this is not really a
bug, but it would be indeed cleaner (and less confusing) to call
ata_ncq_supported().

I am sending a patch to clean this up.

-- 
Damien Le Moal
Western Digital Research