[PATCH v2 0/2] Fixes for ATA device queue depth control

[PATCH v2 0/2] Fixes for ATA device queue depth control

[Damien Le Moal]

A couple of patches to fix maximum queue depth initialization and
control for ATA devices used through libsas.

Changes from v1:
* Addressed Sergey comments: fixed typos in patch 2 commit message and
  removed a temporary pr_info() debug call.

Damien Le Moal (2):
  ata: libata-scsi: Fix initialization of device queue depth
  ata: libata-sata: Fix device queue depth control

 drivers/ata/libata-sata.c           | 24 ++++++++++++------------
 drivers/ata/libata-scsi.c           | 10 ++++------
 drivers/scsi/libsas/sas_scsi_host.c |  3 ++-
 include/linux/libata.h              |  4 ++--
 4 files changed, 20 insertions(+), 21 deletions(-)

-- 
2.37.3

[PATCH v2 1/2] ata: libata-scsi: Fix initialization of device queue depth

[Damien Le Moal]

For SATA devices supporting NCQ, drivers using libsas first initialize a
scsi device queue depth based on the controller and device capabilities,
leading to the scsi device queue_depth field being 32 (ATA maximum queue
depth) for most setup. However, if libata was loaded using the
force=[ID]]noncq argument, the default queue depth should be set to 1 to
reflect the fact that queuable commands will never be used. This is
consistent with manually setting a device queue depth to 1 through sysfs
as that disables NCQ use for the device.

Fix ata_scsi_dev_config() to honor the noncq parameter by sertting the
device queue depth to 1 for devices that do not have the ATA_DFLAG_NCQ
flag set.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/ata/libata-scsi.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 29e2f55c6faa..ff9602a0e54e 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1055,6 +1055,7 @@ EXPORT_SYMBOL_GPL(ata_scsi_dma_need_drain);
 int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev)
 {
 	struct request_queue *q = sdev->request_queue;
+	int depth = 1;
 
 	if (!ata_id_has_unload(dev->id))
 		dev->flags |= ATA_DFLAG_NO_UNLOAD;
@@ -1100,13 +1101,10 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev)
 	if (dev->flags & ATA_DFLAG_AN)
 		set_bit(SDEV_EVT_MEDIA_CHANGE, sdev->supported_events);
 
-	if (dev->flags & ATA_DFLAG_NCQ) {
-		int depth;
-
+	if (dev->flags & ATA_DFLAG_NCQ)
 		depth = min(sdev->host->can_queue, ata_id_queue_depth(dev->id));
-		depth = min(ATA_MAX_QUEUE, depth);
-		scsi_change_queue_depth(sdev, depth);
-	}
+	depth = min(ATA_MAX_QUEUE, depth);
+	scsi_change_queue_depth(sdev, depth);
 
 	if (dev->flags & ATA_DFLAG_TRUSTED)
 		sdev->security_supported = 1;
-- 
2.37.3

[PATCH v2 2/2] ata: libata-sata: Fix device queue depth control

[Damien Le Moal]

The function __ata_change_queue_depth() uses the helper
ata_scsi_find_dev() to get the ata_device structure of a scsi device and
set that device maximum queue depth. However, when the ata device is
managed by libsas, ata_scsi_find_dev() returns NULL, turning
__ata_change_queue_depth() into a nop, which prevents the user from
setting the maximum queue depth of ATA devices used with libsas based
HBAs.

Fix this by renaming __ata_change_queue_depth() to
ata_change_queue_depth() and adding a pointer to the ata_device
structure of the target device as argument. This pointer is provided by
ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of
a libata managed device and by sas_change_queue_depth() using
sas_to_ata_dev() in the case of a libsas managed ata device.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/ata/libata-sata.c           | 24 ++++++++++++------------
 drivers/scsi/libsas/sas_scsi_host.c |  3 ++-
 include/linux/libata.h              |  4 ++--
 3 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 7a5fe41aa5ae..13b9d0fdd42c 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1018,26 +1018,25 @@ DEVICE_ATTR(sw_activity, S_IWUSR | S_IRUGO, ata_scsi_activity_show,
 EXPORT_SYMBOL_GPL(dev_attr_sw_activity);
 
 /**
- *	__ata_change_queue_depth - helper for ata_scsi_change_queue_depth
- *	@ap: ATA port to which the device change the queue depth
+ *	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
  *
- *	libsas and libata have different approaches for associating a sdev to
- *	its ata_port.
+ *	Helper to set a device maximum queue depth, usable with both libsas
+ *	and libata.
  *
  */
-int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
-			     int queue_depth)
+int ata_change_queue_depth(struct ata_port *ap, struct ata_device *dev,
+			   struct scsi_device *sdev, int queue_depth)
 {
-	struct ata_device *dev;
 	unsigned long flags;
 
-	if (queue_depth < 1 || queue_depth == sdev->queue_depth)
+	if (!dev || !ata_dev_enabled(dev))
 		return sdev->queue_depth;
 
-	dev = ata_scsi_find_dev(ap, sdev);
-	if (!dev || !ata_dev_enabled(dev))
+	if (queue_depth < 1 || queue_depth == sdev->queue_depth)
 		return sdev->queue_depth;
 
 	/* NCQ enabled? */
@@ -1059,7 +1058,7 @@ int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
 
 	return scsi_change_queue_depth(sdev, queue_depth);
 }
-EXPORT_SYMBOL_GPL(__ata_change_queue_depth);
+EXPORT_SYMBOL_GPL(ata_change_queue_depth);
 
 /**
  *	ata_scsi_change_queue_depth - SCSI callback for queue depth config
@@ -1080,7 +1079,8 @@ 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, sdev, queue_depth);
+	return ata_change_queue_depth(ap, ata_scsi_find_dev(ap, sdev),
+				      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 9c82e5dc4fcc..a36fa1c128a8 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -872,7 +872,8 @@ 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, sdev, depth);
+		return ata_change_queue_depth(dev->sata_dev.ap,
+					      sas_to_ata_dev(dev), sdev, depth);
 
 	if (!sdev->tagged_supported)
 		depth = 1;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 698032e5ef2d..20765d1c5f80 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1136,8 +1136,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 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 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.37.3

Re: [PATCH v2 2/2] ata: libata-sata: Fix device queue depth control

[John Garry]

On 26/09/2022 00:08, Damien Le Moal wrote:
> The function __ata_change_queue_depth() uses the helper
> ata_scsi_find_dev() to get the ata_device structure of a scsi device and
> set that device maximum queue depth. However, when the ata device is
> managed by libsas,

Yeah, this current code is utterly broken. Just a nit is that it would 
be good to mention that the sdev id is assigned by scsi_transport_sas 
(and not libata) for when using libsas.

In my series "Allocate SCSI device earlier for ata port probe" I 
mentioned this problem of different id assignment scheme. It would be 
nice to try to commonize this.

> ata_scsi_find_dev() returns NULL, turning
> __ata_change_queue_depth() into a nop, which prevents the user from
> setting the maximum queue depth of ATA devices used with libsas based
> HBAs.
> 
> Fix this by renaming __ata_change_queue_depth() to
> ata_change_queue_depth() and adding a pointer to the ata_device
> structure of the target device as argument. This pointer is provided by
> ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of
> a libata managed device and by sas_change_queue_depth() using
> sas_to_ata_dev() in the case of a libsas managed ata device.

This seems ok. But could you alternatively use ata_for_each_dev() and 
match by ata_device.sdev pointer? That pointer is set quite late in the 
probe, though, so maybe it would not work.

As an aside, one other thing I noticed is that ata_device.private_data 
is unused. Maybe we should delete it - apparently it was added just for 
symmetry of libata structures.

Thanks,
John

> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>   drivers/ata/libata-sata.c           | 24 ++++++++++++------------
>   drivers/scsi/libsas/sas_scsi_host.c |  3 ++-
>   include/linux/libata.h              |  4 ++--
>   3 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 7a5fe41aa5ae..13b9d0fdd42c 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -1018,26 +1018,25 @@ DEVICE_ATTR(sw_activity, S_IWUSR | S_IRUGO, ata_scsi_activity_show,
>   EXPORT_SYMBOL_GPL(dev_attr_sw_activity);
>   
>   /**
> - *	__ata_change_queue_depth - helper for ata_scsi_change_queue_depth
> - *	@ap: ATA port to which the device change the queue depth
> + *	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
>    *
> - *	libsas and libata have different approaches for associating a sdev to
> - *	its ata_port.
> + *	Helper to set a device maximum queue depth, usable with both libsas
> + *	and libata.
>    *
>    */
> -int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
> -			     int queue_depth)
> +int ata_change_queue_depth(struct ata_port *ap, struct ata_device *dev,
> +			   struct scsi_device *sdev, int queue_depth)
>   {
> -	struct ata_device *dev;
>   	unsigned long flags;
>   
> -	if (queue_depth < 1 || queue_depth == sdev->queue_depth)
> +	if (!dev || !ata_dev_enabled(dev))
>   		return sdev->queue_depth;
>   
> -	dev = ata_scsi_find_dev(ap, sdev);
> -	if (!dev || !ata_dev_enabled(dev))
> +	if (queue_depth < 1 || queue_depth == sdev->queue_depth)
>   		return sdev->queue_depth;
>   
>   	/* NCQ enabled? */
> @@ -1059,7 +1058,7 @@ int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
>   
>   	return scsi_change_queue_depth(sdev, queue_depth);
>   }
> -EXPORT_SYMBOL_GPL(__ata_change_queue_depth);
> +EXPORT_SYMBOL_GPL(ata_change_queue_depth);
>   
>   /**
>    *	ata_scsi_change_queue_depth - SCSI callback for queue depth config
> @@ -1080,7 +1079,8 @@ 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, sdev, queue_depth);
> +	return ata_change_queue_depth(ap, ata_scsi_find_dev(ap, sdev),
> +				      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 9c82e5dc4fcc..a36fa1c128a8 100644
> --- a/drivers/scsi/libsas/sas_scsi_host.c
> +++ b/drivers/scsi/libsas/sas_scsi_host.c
> @@ -872,7 +872,8 @@ 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, sdev, depth);
> +		return ata_change_queue_depth(dev->sata_dev.ap,
> +					      sas_to_ata_dev(dev), sdev, depth);
>   
>   	if (!sdev->tagged_supported)
>   		depth = 1;
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 698032e5ef2d..20765d1c5f80 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1136,8 +1136,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 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 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);

Re: [PATCH v2 2/2] ata: libata-sata: Fix device queue depth control

[Damien Le Moal]

On 9/26/22 20:31, John Garry wrote:
> On 26/09/2022 00:08, Damien Le Moal wrote:
>> The function __ata_change_queue_depth() uses the helper
>> ata_scsi_find_dev() to get the ata_device structure of a scsi device and
>> set that device maximum queue depth. However, when the ata device is
>> managed by libsas,
> 
> Yeah, this current code is utterly broken. Just a nit is that it would 
> be good to mention that the sdev id is assigned by scsi_transport_sas 
> (and not libata) for when using libsas.
> 
> In my series "Allocate SCSI device earlier for ata port probe" I 
> mentioned this problem of different id assignment scheme. It would be 
> nice to try to commonize this.
> 
>> ata_scsi_find_dev() returns NULL, turning
>> __ata_change_queue_depth() into a nop, which prevents the user from
>> setting the maximum queue depth of ATA devices used with libsas based
>> HBAs.
>>
>> Fix this by renaming __ata_change_queue_depth() to
>> ata_change_queue_depth() and adding a pointer to the ata_device
>> structure of the target device as argument. This pointer is provided by
>> ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of
>> a libata managed device and by sas_change_queue_depth() using
>> sas_to_ata_dev() in the case of a libsas managed ata device.
> 
> This seems ok. But could you alternatively use ata_for_each_dev() and 
> match by ata_device.sdev pointer? That pointer is set quite late in the 
> probe, though, so maybe it would not work.

Not sure I understand why we should search for the ata device again using
ata_for_each_dev() when sas_to_ata_dev() gives us directly what we need
for the libsas controlled device... Can you clarify ?

> 
> As an aside, one other thing I noticed is that ata_device.private_data 
> is unused. Maybe we should delete it - apparently it was added just for 
> symmetry of libata structures.
> 
> Thanks,
> John
> 
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> ---
>>   drivers/ata/libata-sata.c           | 24 ++++++++++++------------
>>   drivers/scsi/libsas/sas_scsi_host.c |  3 ++-
>>   include/linux/libata.h              |  4 ++--
>>   3 files changed, 16 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
>> index 7a5fe41aa5ae..13b9d0fdd42c 100644
>> --- a/drivers/ata/libata-sata.c
>> +++ b/drivers/ata/libata-sata.c
>> @@ -1018,26 +1018,25 @@ DEVICE_ATTR(sw_activity, S_IWUSR | S_IRUGO, ata_scsi_activity_show,
>>   EXPORT_SYMBOL_GPL(dev_attr_sw_activity);
>>   
>>   /**
>> - *	__ata_change_queue_depth - helper for ata_scsi_change_queue_depth
>> - *	@ap: ATA port to which the device change the queue depth
>> + *	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
>>    *
>> - *	libsas and libata have different approaches for associating a sdev to
>> - *	its ata_port.
>> + *	Helper to set a device maximum queue depth, usable with both libsas
>> + *	and libata.
>>    *
>>    */
>> -int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
>> -			     int queue_depth)
>> +int ata_change_queue_depth(struct ata_port *ap, struct ata_device *dev,
>> +			   struct scsi_device *sdev, int queue_depth)
>>   {
>> -	struct ata_device *dev;
>>   	unsigned long flags;
>>   
>> -	if (queue_depth < 1 || queue_depth == sdev->queue_depth)
>> +	if (!dev || !ata_dev_enabled(dev))
>>   		return sdev->queue_depth;
>>   
>> -	dev = ata_scsi_find_dev(ap, sdev);
>> -	if (!dev || !ata_dev_enabled(dev))
>> +	if (queue_depth < 1 || queue_depth == sdev->queue_depth)
>>   		return sdev->queue_depth;
>>   
>>   	/* NCQ enabled? */
>> @@ -1059,7 +1058,7 @@ int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
>>   
>>   	return scsi_change_queue_depth(sdev, queue_depth);
>>   }
>> -EXPORT_SYMBOL_GPL(__ata_change_queue_depth);
>> +EXPORT_SYMBOL_GPL(ata_change_queue_depth);
>>   
>>   /**
>>    *	ata_scsi_change_queue_depth - SCSI callback for queue depth config
>> @@ -1080,7 +1079,8 @@ 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, sdev, queue_depth);
>> +	return ata_change_queue_depth(ap, ata_scsi_find_dev(ap, sdev),
>> +				      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 9c82e5dc4fcc..a36fa1c128a8 100644
>> --- a/drivers/scsi/libsas/sas_scsi_host.c
>> +++ b/drivers/scsi/libsas/sas_scsi_host.c
>> @@ -872,7 +872,8 @@ 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, sdev, depth);
>> +		return ata_change_queue_depth(dev->sata_dev.ap,
>> +					      sas_to_ata_dev(dev), sdev, depth);
>>   
>>   	if (!sdev->tagged_supported)
>>   		depth = 1;
>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>> index 698032e5ef2d..20765d1c5f80 100644
>> --- a/include/linux/libata.h
>> +++ b/include/linux/libata.h
>> @@ -1136,8 +1136,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 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 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);
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 2/2] ata: libata-sata: Fix device queue depth control

[John Garry]

On 27/09/2022 00:05, Damien Le Moal wrote:
>>> ata_scsi_find_dev() returns NULL, turning
>>> __ata_change_queue_depth() into a nop, which prevents the user from
>>> setting the maximum queue depth of ATA devices used with libsas based
>>> HBAs.
>>>
>>> Fix this by renaming __ata_change_queue_depth() to
>>> ata_change_queue_depth() and adding a pointer to the ata_device
>>> structure of the target device as argument. This pointer is provided by
>>> ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of
>>> a libata managed device and by sas_change_queue_depth() using
>>> sas_to_ata_dev() in the case of a libsas managed ata device.
>> This seems ok. But could you alternatively use ata_for_each_dev() and
>> match by ata_device.sdev pointer? That pointer is set quite late in the
>> probe, though, so maybe it would not work.
> Not sure I understand why we should search for the ata device again using
> ata_for_each_dev() when sas_to_ata_dev() gives us directly what we need
> for the libsas controlled device... Can you clarify ?
> 

Sure, we can use sas_to_ata_dev() to get the ata_device.

I am just suggesting my way such that we can have a consistent method to 
get the ata_device between all libata users and we don't need to change 
the ata_change_queue_depth() interface. It would be something like:

struct ata_device *ata_scsi_find_dev(struct ata_port *ap, const struct 
scsi_device *scsidev)
{
	struct ata_link *link;
	struct ata_device *dev;

	ata_for_each_link(link, ap, EDGE) {
		ata_for_each_dev(dev, link, ENABLED) {
			if (scsidev == dev->sdev)
				return dev;
		}
	}
	// todo: check pmp
	return NULL;
}

Thanks,
John

Re: [PATCH v2 2/2] ata: libata-sata: Fix device queue depth control

[Damien Le Moal]

On 9/27/22 16:05, John Garry wrote:
> On 27/09/2022 00:05, Damien Le Moal wrote:
>>>> ata_scsi_find_dev() returns NULL, turning
>>>> __ata_change_queue_depth() into a nop, which prevents the user from
>>>> setting the maximum queue depth of ATA devices used with libsas based
>>>> HBAs.
>>>>
>>>> Fix this by renaming __ata_change_queue_depth() to
>>>> ata_change_queue_depth() and adding a pointer to the ata_device
>>>> structure of the target device as argument. This pointer is provided by
>>>> ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of
>>>> a libata managed device and by sas_change_queue_depth() using
>>>> sas_to_ata_dev() in the case of a libsas managed ata device.
>>> This seems ok. But could you alternatively use ata_for_each_dev() and
>>> match by ata_device.sdev pointer? That pointer is set quite late in the
>>> probe, though, so maybe it would not work.
>> Not sure I understand why we should search for the ata device again using
>> ata_for_each_dev() when sas_to_ata_dev() gives us directly what we need
>> for the libsas controlled device... Can you clarify ?
>>
> 
> Sure, we can use sas_to_ata_dev() to get the ata_device.
> 
> I am just suggesting my way such that we can have a consistent method to 
> get the ata_device between all libata users and we don't need to change 
> the ata_change_queue_depth() interface. It would be something like:
> 
> struct ata_device *ata_scsi_find_dev(struct ata_port *ap, const struct 
> scsi_device *scsidev)
> {
> 	struct ata_link *link;
> 	struct ata_device *dev;
> 
> 	ata_for_each_link(link, ap, EDGE) {
> 		ata_for_each_dev(dev, link, ENABLED) {
> 			if (scsidev == dev->sdev)
> 				return dev;
> 		}
> 	}
> 	// todo: check pmp
> 	return NULL;
> }

I see. Need to think about this one... This may also unify the pmp case.
Are you OK with the patch as is though ? We can improve with something
like the above on top later. Really need to fix that qd setting as it is
causing problems for testing devices with/without ncq commands.

> 
> Thanks,
> John

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 2/2] ata: libata-sata: Fix device queue depth control

[John Garry]

On 27/09/2022 10:28, Damien Le Moal wrote:
>> Sure, we can use sas_to_ata_dev() to get the ata_device.
>>
>> I am just suggesting my way such that we can have a consistent method to
>> get the ata_device between all libata users and we don't need to change
>> the ata_change_queue_depth() interface. It would be something like:
>>
>> struct ata_device *ata_scsi_find_dev(struct ata_port *ap, const struct
>> scsi_device *scsidev)
>> {
>> 	struct ata_link *link;
>> 	struct ata_device *dev;
>>
>> 	ata_for_each_link(link, ap, EDGE) {
>> 		ata_for_each_dev(dev, link, ENABLED) {
>> 			if (scsidev == dev->sdev)
>> 				return dev;
>> 		}
>> 	}
>> 	// todo: check pmp
>> 	return NULL;
>> }
> I see. Need to think about this one... This may also unify the pmp case.
> Are you OK with the patch as is though ? 

I'm ok with your patchset, but let me test it and get back to you later 
today.

We can improve with something
> like the above on top later. Really need to fix that qd setting as it is
> causing problems for testing devices with/without ncq commands.

Out of curiosity, are you considering your patchset for 6.0?

> 

Thanks,
John

Re: [PATCH v2 2/2] ata: libata-sata: Fix device queue depth control

[Damien Le Moal]

On 9/27/22 18:47, John Garry wrote:
> On 27/09/2022 10:28, Damien Le Moal wrote:
>>> Sure, we can use sas_to_ata_dev() to get the ata_device.
>>>
>>> I am just suggesting my way such that we can have a consistent method to
>>> get the ata_device between all libata users and we don't need to change
>>> the ata_change_queue_depth() interface. It would be something like:
>>>
>>> struct ata_device *ata_scsi_find_dev(struct ata_port *ap, const struct
>>> scsi_device *scsidev)
>>> {
>>> 	struct ata_link *link;
>>> 	struct ata_device *dev;
>>>
>>> 	ata_for_each_link(link, ap, EDGE) {
>>> 		ata_for_each_dev(dev, link, ENABLED) {
>>> 			if (scsidev == dev->sdev)
>>> 				return dev;
>>> 		}
>>> 	}
>>> 	// todo: check pmp
>>> 	return NULL;
>>> }
>> I see. Need to think about this one... This may also unify the pmp case.
>> Are you OK with the patch as is though ? 
> 
> I'm ok with your patchset, but let me test it and get back to you later 
> today.
> 
> We can improve with something
>> like the above on top later. Really need to fix that qd setting as it is
>> causing problems for testing devices with/without ncq commands.
> 
> Out of curiosity, are you considering your patchset for 6.0?

Yes. But I can send it for 6.1 with cc: stable too.

> 
>>
> 
> Thanks,
> John

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 2/2] ata: libata-sata: Fix device queue depth control

[John Garry]

On 26/09/2022 00:08, Damien Le Moal wrote:
> The function __ata_change_queue_depth() uses the helper
> ata_scsi_find_dev() to get the ata_device structure of a scsi device and
> set that device maximum queue depth. However, when the ata device is
> managed by libsas, ata_scsi_find_dev() returns NULL, turning
> __ata_change_queue_depth() into a nop, which prevents the user from
> setting the maximum queue depth of ATA devices used with libsas based
> HBAs.
> 
> Fix this by renaming __ata_change_queue_depth() to
> ata_change_queue_depth() and adding a pointer to the ata_device
> structure of the target device as argument. This pointer is provided by
> ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of
> a libata managed device and by sas_change_queue_depth() using
> sas_to_ata_dev() in the case of a libsas managed ata device.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Tested-by: John Garry <john.garry@huawei.com>

However - a big however - I will note that this following behaviour is 
strange for a SATA device for libsas:

root@(none)$ echo 33 > 0:0:2:0/device/queue_depth
root@(none)$ echo 33 > 0:0:2:0/device/queue_depth
sh: echo: write error: Invalid argument
root@(none)$

I also note that setting a value out of range is just rejected for a SAS 
device, and not capped to the max range (like it is for SATA).

AHCI rejects out of range values it as it exceeds the shost can_queue in 
sdev_store_queue_depth().

Thanks,
John

> ---
>   drivers/ata/libata-sata.c           | 24 ++++++++++++------------
>   drivers/scsi/libsas/sas_scsi_host.c |  3 ++-
>   include/linux/libata.h              |  4 ++--
>   3 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 7a5fe41aa5ae..13b9d0fdd42c 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -1018,26 +1018,25 @@ DEVICE_ATTR(sw_activity, S_IWUSR | S_IRUGO, ata_scsi_activity_show,
>   EXPORT_SYMBOL_GPL(dev_attr_sw_activity);
>   
>   /**
> - *	__ata_change_queue_depth - helper for ata_scsi_change_queue_depth
> - *	@ap: ATA port to which the device change the queue depth
> + *	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
>    *
> - *	libsas and libata have different approaches for associating a sdev to
> - *	its ata_port.
> + *	Helper to set a device maximum queue depth, usable with both libsas
> + *	and libata.
>    *
>    */
> -int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
> -			     int queue_depth)
> +int ata_change_queue_depth(struct ata_port *ap, struct ata_device *dev,
> +			   struct scsi_device *sdev, int queue_depth)
>   {
> -	struct ata_device *dev;
>   	unsigned long flags;
>   
> -	if (queue_depth < 1 || queue_depth == sdev->queue_depth)
> +	if (!dev || !ata_dev_enabled(dev))
>   		return sdev->queue_depth;
>   
> -	dev = ata_scsi_find_dev(ap, sdev);
> -	if (!dev || !ata_dev_enabled(dev))
> +	if (queue_depth < 1 || queue_depth == sdev->queue_depth)
>   		return sdev->queue_depth;
>   
>   	/* NCQ enabled? */
> @@ -1059,7 +1058,7 @@ int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
>   
>   	return scsi_change_queue_depth(sdev, queue_depth);
>   }
> -EXPORT_SYMBOL_GPL(__ata_change_queue_depth);
> +EXPORT_SYMBOL_GPL(ata_change_queue_depth);
>   
>   /**
>    *	ata_scsi_change_queue_depth - SCSI callback for queue depth config
> @@ -1080,7 +1079,8 @@ 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, sdev, queue_depth);
> +	return ata_change_queue_depth(ap, ata_scsi_find_dev(ap, sdev),
> +				      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 9c82e5dc4fcc..a36fa1c128a8 100644
> --- a/drivers/scsi/libsas/sas_scsi_host.c
> +++ b/drivers/scsi/libsas/sas_scsi_host.c
> @@ -872,7 +872,8 @@ 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, sdev, depth);
> +		return ata_change_queue_depth(dev->sata_dev.ap,
> +					      sas_to_ata_dev(dev), sdev, depth);
>   
>   	if (!sdev->tagged_supported)
>   		depth = 1;
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 698032e5ef2d..20765d1c5f80 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1136,8 +1136,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 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 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);

Re: [PATCH v2 2/2] ata: libata-sata: Fix device queue depth control

[Damien Le Moal]

On 9/27/22 20:51, John Garry wrote:
> On 26/09/2022 00:08, Damien Le Moal wrote:
>> The function __ata_change_queue_depth() uses the helper
>> ata_scsi_find_dev() to get the ata_device structure of a scsi device and
>> set that device maximum queue depth. However, when the ata device is
>> managed by libsas, ata_scsi_find_dev() returns NULL, turning
>> __ata_change_queue_depth() into a nop, which prevents the user from
>> setting the maximum queue depth of ATA devices used with libsas based
>> HBAs.
>>
>> Fix this by renaming __ata_change_queue_depth() to
>> ata_change_queue_depth() and adding a pointer to the ata_device
>> structure of the target device as argument. This pointer is provided by
>> ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of
>> a libata managed device and by sas_change_queue_depth() using
>> sas_to_ata_dev() in the case of a libsas managed ata device.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> 
> Tested-by: John Garry <john.garry@huawei.com>
> 
> However - a big however - I will note that this following behaviour is 
> strange for a SATA device for libsas:
> 
> root@(none)$ echo 33 > 0:0:2:0/device/queue_depth
> root@(none)$ echo 33 > 0:0:2:0/device/queue_depth
> sh: echo: write error: Invalid argument
> root@(none)$

Weird. I am not getting any error (pm80xx driver). The qd gets capped at
32 as expected. Is it something that changes per libsas driver ?

> I also note that setting a value out of range is just rejected for a SAS 
> device, and not capped to the max range (like it is for SATA).

Not sure where that come from. A quick look does not reveal anything
obvious. Need to dig into that one.
> 
> AHCI rejects out of range values it as it exceeds the shost can_queue in 
> sdev_store_queue_depth().

Indeed:

echo 1 > /sys/block/sdk/device/queue_depth
echo 33 > /sys/block/sdk/device/queue_depth
cat /sys/block/sdk/device/queue_depth
1

But for the libsas SATA device:

echo 1 > /sys/block/sdd/device/queue_depth
cat /sys/block/sdd/device/queue_depth
1
echo 33 > /sys/block/sdd/device/queue_depth
cat /sys/block/sdd/device/queue_depth
32

As one would expect...

Need to dig into that one.

> 
> Thanks,
> John
> 
>> ---
>>   drivers/ata/libata-sata.c           | 24 ++++++++++++------------
>>   drivers/scsi/libsas/sas_scsi_host.c |  3 ++-
>>   include/linux/libata.h              |  4 ++--
>>   3 files changed, 16 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
>> index 7a5fe41aa5ae..13b9d0fdd42c 100644
>> --- a/drivers/ata/libata-sata.c
>> +++ b/drivers/ata/libata-sata.c
>> @@ -1018,26 +1018,25 @@ DEVICE_ATTR(sw_activity, S_IWUSR | S_IRUGO, ata_scsi_activity_show,
>>   EXPORT_SYMBOL_GPL(dev_attr_sw_activity);
>>   
>>   /**
>> - *	__ata_change_queue_depth - helper for ata_scsi_change_queue_depth
>> - *	@ap: ATA port to which the device change the queue depth
>> + *	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
>>    *
>> - *	libsas and libata have different approaches for associating a sdev to
>> - *	its ata_port.
>> + *	Helper to set a device maximum queue depth, usable with both libsas
>> + *	and libata.
>>    *
>>    */
>> -int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
>> -			     int queue_depth)
>> +int ata_change_queue_depth(struct ata_port *ap, struct ata_device *dev,
>> +			   struct scsi_device *sdev, int queue_depth)
>>   {
>> -	struct ata_device *dev;
>>   	unsigned long flags;
>>   
>> -	if (queue_depth < 1 || queue_depth == sdev->queue_depth)
>> +	if (!dev || !ata_dev_enabled(dev))
>>   		return sdev->queue_depth;
>>   
>> -	dev = ata_scsi_find_dev(ap, sdev);
>> -	if (!dev || !ata_dev_enabled(dev))
>> +	if (queue_depth < 1 || queue_depth == sdev->queue_depth)
>>   		return sdev->queue_depth;
>>   
>>   	/* NCQ enabled? */
>> @@ -1059,7 +1058,7 @@ int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
>>   
>>   	return scsi_change_queue_depth(sdev, queue_depth);
>>   }
>> -EXPORT_SYMBOL_GPL(__ata_change_queue_depth);
>> +EXPORT_SYMBOL_GPL(ata_change_queue_depth);
>>   
>>   /**
>>    *	ata_scsi_change_queue_depth - SCSI callback for queue depth config
>> @@ -1080,7 +1079,8 @@ 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, sdev, queue_depth);
>> +	return ata_change_queue_depth(ap, ata_scsi_find_dev(ap, sdev),
>> +				      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 9c82e5dc4fcc..a36fa1c128a8 100644
>> --- a/drivers/scsi/libsas/sas_scsi_host.c
>> +++ b/drivers/scsi/libsas/sas_scsi_host.c
>> @@ -872,7 +872,8 @@ 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, sdev, depth);
>> +		return ata_change_queue_depth(dev->sata_dev.ap,
>> +					      sas_to_ata_dev(dev), sdev, depth);
>>   
>>   	if (!sdev->tagged_supported)
>>   		depth = 1;
>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>> index 698032e5ef2d..20765d1c5f80 100644
>> --- a/include/linux/libata.h
>> +++ b/include/linux/libata.h
>> @@ -1136,8 +1136,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 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 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);
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 2/2] ata: libata-sata: Fix device queue depth control

[John Garry]

On 27/09/2022 16:03, Damien Le Moal wrote:
> On 9/27/22 20:51, John Garry wrote:
>> On 26/09/2022 00:08, Damien Le Moal wrote:
>>> The function __ata_change_queue_depth() uses the helper
>>> ata_scsi_find_dev() to get the ata_device structure of a scsi device and
>>> set that device maximum queue depth. However, when the ata device is
>>> managed by libsas, ata_scsi_find_dev() returns NULL, turning
>>> __ata_change_queue_depth() into a nop, which prevents the user from
>>> setting the maximum queue depth of ATA devices used with libsas based
>>> HBAs.
>>>
>>> Fix this by renaming __ata_change_queue_depth() to
>>> ata_change_queue_depth() and adding a pointer to the ata_device
>>> structure of the target device as argument. This pointer is provided by
>>> ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of
>>> a libata managed device and by sas_change_queue_depth() using
>>> sas_to_ata_dev() in the case of a libsas managed ata device.
>>>
>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>
>> Tested-by: John Garry <john.garry@huawei.com>
>>
>> However - a big however - I will note that this following behaviour is
>> strange for a SATA device for libsas:
>>
>> root@(none)$ echo 33 > 0:0:2:0/device/queue_depth
>> root@(none)$ echo 33 > 0:0:2:0/device/queue_depth
>> sh: echo: write error: Invalid argument
>> root@(none)$
> 
> Weird. I am not getting any error (pm80xx driver). The qd gets capped at
> 32 as expected. Is it something that changes per libsas driver ?

That is with my pm8001 card.

What happens here is that the first store of 33 gets through to 
ata_change_queue_depth() as it does not exceed the SAS shost can_queue, 
which is >> 32, and then we cap this to 32 and store it in 
sdev->queue_depth. And then the 2nd store of 33 also gets through, but 
this following expression not evaluate true in ata_change_queue_depth():

queue_depth < 1 || queue_depth == sdev->queue_depth

So we don't return. However the following subsequent test does evaluate 
true in ata_change_queue_depth():

if (sdev->queue_depth == queue_depth)
	return -EINVAL;

And we error.

> 
>> I also note that setting a value out of range is just rejected for a SAS
>> device, and not capped to the max range (like it is for SATA).
> 
> Not sure where that come from. A quick look does not reveal anything
> obvious. Need to dig into that one.
>>
>> AHCI rejects out of range values it as it exceeds the shost can_queue in
>> sdev_store_queue_depth().
> 
> Indeed:
> 
> echo 1 > /sys/block/sdk/device/queue_depth
> echo 33 > /sys/block/sdk/device/queue_depth

Hmmmm... why no error message? is the printk silenced?

> cat /sys/block/sdk/device/queue_depth
> 1
> 
> But for the libsas SATA device:
> 
> echo 1 > /sys/block/sdd/device/queue_depth
> cat /sys/block/sdd/device/queue_depth
> 1
> echo 33 > /sys/block/sdd/device/queue_depth
> cat /sys/block/sdd/device/queue_depth
> 32
> 
> As one would expect... > Need to dig into that one.
> 

thanks,
John

Re: [PATCH v2 2/2] ata: libata-sata: Fix device queue depth control

[Damien Le Moal]

On 9/28/22 01:09, John Garry wrote:
> On 27/09/2022 16:03, Damien Le Moal wrote:
>> On 9/27/22 20:51, John Garry wrote:
>>> On 26/09/2022 00:08, Damien Le Moal wrote:
>>>> The function __ata_change_queue_depth() uses the helper
>>>> ata_scsi_find_dev() to get the ata_device structure of a scsi device and
>>>> set that device maximum queue depth. However, when the ata device is
>>>> managed by libsas, ata_scsi_find_dev() returns NULL, turning
>>>> __ata_change_queue_depth() into a nop, which prevents the user from
>>>> setting the maximum queue depth of ATA devices used with libsas based
>>>> HBAs.
>>>>
>>>> Fix this by renaming __ata_change_queue_depth() to
>>>> ata_change_queue_depth() and adding a pointer to the ata_device
>>>> structure of the target device as argument. This pointer is provided by
>>>> ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of
>>>> a libata managed device and by sas_change_queue_depth() using
>>>> sas_to_ata_dev() in the case of a libsas managed ata device.
>>>>
>>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>>
>>> Tested-by: John Garry <john.garry@huawei.com>
>>>
>>> However - a big however - I will note that this following behaviour is
>>> strange for a SATA device for libsas:
>>>
>>> root@(none)$ echo 33 > 0:0:2:0/device/queue_depth
>>> root@(none)$ echo 33 > 0:0:2:0/device/queue_depth
>>> sh: echo: write error: Invalid argument
>>> root@(none)$
>>
>> Weird. I am not getting any error (pm80xx driver). The qd gets capped at
>> 32 as expected. Is it something that changes per libsas driver ?
> 
> That is with my pm8001 card.
> 
> What happens here is that the first store of 33 gets through to 
> ata_change_queue_depth() as it does not exceed the SAS shost can_queue, 
> which is >> 32, and then we cap this to 32 and store it in 
> sdev->queue_depth. And then the 2nd store of 33 also gets through, but 
> this following expression not evaluate true in ata_change_queue_depth():
> 
> queue_depth < 1 || queue_depth == sdev->queue_depth
> 
> So we don't return. However the following subsequent test does evaluate 
> true in ata_change_queue_depth():
> 
> if (sdev->queue_depth == queue_depth)
> 	return -EINVAL;
> 
> And we error.

Gotcha. This needs to be changed to:

if (sdev->queue_depth == queue_depth)
	return queue_depth;

And the non-sensical error will go away.

Will send another patch for that.

> 
>>
>>> I also note that setting a value out of range is just rejected for a SAS
>>> device, and not capped to the max range (like it is for SATA).
>>
>> Not sure where that come from. A quick look does not reveal anything
>> obvious. Need to dig into that one.
>>>
>>> AHCI rejects out of range values it as it exceeds the shost can_queue in
>>> sdev_store_queue_depth().
>>
>> Indeed:
>>
>> echo 1 > /sys/block/sdk/device/queue_depth
>> echo 33 > /sys/block/sdk/device/queue_depth
> 
> Hmmmm... why no error message? is the printk silenced?
> 
>> cat /sys/block/sdk/device/queue_depth
>> 1
>>
>> But for the libsas SATA device:
>>
>> echo 1 > /sys/block/sdd/device/queue_depth
>> cat /sys/block/sdd/device/queue_depth
>> 1
>> echo 33 > /sys/block/sdd/device/queue_depth
>> cat /sys/block/sdd/device/queue_depth
>> 32
>>
>> As one would expect... > Need to dig into that one.
>>
> 
> thanks,
> John
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 2/2] ata: libata-sata: Fix device queue depth control

[Damien Le Moal]

On 9/28/22 01:09, John Garry wrote:
> On 27/09/2022 16:03, Damien Le Moal wrote:
>> On 9/27/22 20:51, John Garry wrote:
>>> On 26/09/2022 00:08, Damien Le Moal wrote:
>>>> The function __ata_change_queue_depth() uses the helper
>>>> ata_scsi_find_dev() to get the ata_device structure of a scsi device and
>>>> set that device maximum queue depth. However, when the ata device is
>>>> managed by libsas, ata_scsi_find_dev() returns NULL, turning
>>>> __ata_change_queue_depth() into a nop, which prevents the user from
>>>> setting the maximum queue depth of ATA devices used with libsas based
>>>> HBAs.
>>>>
>>>> Fix this by renaming __ata_change_queue_depth() to
>>>> ata_change_queue_depth() and adding a pointer to the ata_device
>>>> structure of the target device as argument. This pointer is provided by
>>>> ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of
>>>> a libata managed device and by sas_change_queue_depth() using
>>>> sas_to_ata_dev() in the case of a libsas managed ata device.
>>>>
>>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>>
>>> Tested-by: John Garry <john.garry@huawei.com>
>>>
>>> However - a big however - I will note that this following behaviour is
>>> strange for a SATA device for libsas:
>>>
>>> root@(none)$ echo 33 > 0:0:2:0/device/queue_depth
>>> root@(none)$ echo 33 > 0:0:2:0/device/queue_depth
>>> sh: echo: write error: Invalid argument
>>> root@(none)$
>>
>> Weird. I am not getting any error (pm80xx driver). The qd gets capped at
>> 32 as expected. Is it something that changes per libsas driver ?
> 
> That is with my pm8001 card.
> 
> What happens here is that the first store of 33 gets through to 
> ata_change_queue_depth() as it does not exceed the SAS shost can_queue, 
> which is >> 32, and then we cap this to 32 and store it in 
> sdev->queue_depth. And then the 2nd store of 33 also gets through, but 
> this following expression not evaluate true in ata_change_queue_depth():
> 
> queue_depth < 1 || queue_depth == sdev->queue_depth
> 
> So we don't return. However the following subsequent test does evaluate 
> true in ata_change_queue_depth():
> 
> if (sdev->queue_depth == queue_depth)
> 	return -EINVAL;
> 
> And we error.

I dug further into this. For AHCI, I still get an error when trying to set
33. No capping and defaulting to 32. The reason is I believe that
sdev_store_queue_depth() has the check:

	if (depth < 1 || depth > sdev->host->can_queue)
                return -EINVAL;

as you mentioned. So all good.

So changing that last "if" in ata_change_queue_depth() to

	if (sdev->queue_depth == queue_depth)
		return sdev->queue_depth;

has no effect. The error remains.

Now, for a libsas SATA drive, if I add the above change, I do indeed get a
cap to 32 and the QD changes, no error. That is bothering me as that is
really inconsistent. Instead of suppressing the error, shouldn't we unify
AHCI and libsas behavior and error if the user is attempting to set a
value larger than what the *device* supports (the host can_queue was
checked already). In a nutshell, the difference comes form
sdev->host->can_queue being equal to the device max qd for AHCI but not
necessarily for libsas.

I am tempted to leave things as is for now (not changin gthe current weird
behavior) and cleaning that up during the next round. Thoughts ?

> 
>>
>>> I also note that setting a value out of range is just rejected for a SAS
>>> device, and not capped to the max range (like it is for SATA).
>>
>> Not sure where that come from. A quick look does not reveal anything
>> obvious. Need to dig into that one.
>>>
>>> AHCI rejects out of range values it as it exceeds the shost can_queue in
>>> sdev_store_queue_depth().
>>
>> Indeed:
>>
>> echo 1 > /sys/block/sdk/device/queue_depth
>> echo 33 > /sys/block/sdk/device/queue_depth
> 
> Hmmmm... why no error message? is the printk silenced?
> 
>> cat /sys/block/sdk/device/queue_depth
>> 1
>>
>> But for the libsas SATA device:
>>
>> echo 1 > /sys/block/sdd/device/queue_depth
>> cat /sys/block/sdd/device/queue_depth
>> 1
>> echo 33 > /sys/block/sdd/device/queue_depth
>> cat /sys/block/sdd/device/queue_depth
>> 32
>>
>> As one would expect... > Need to dig into that one.
>>
> 
> thanks,
> John
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 2/2] ata: libata-sata: Fix device queue depth control

[John Garry]

On 28/09/2022 08:00, Damien Le Moal wrote:
>> So we don't return. However the following subsequent test does evaluate
>> true in ata_change_queue_depth():
>>
>> if (sdev->queue_depth == queue_depth)
>> 	return -EINVAL;
>>
>> And we error.
> I dug further into this. For AHCI, I still get an error when trying to set
> 33. No capping and defaulting to 32. The reason is I believe that
> sdev_store_queue_depth() has the check:
> 
> 	if (depth < 1 || depth > sdev->host->can_queue)
>                  return -EINVAL;
> 
> as you mentioned. So all good.
> 
> So changing that last "if" in ata_change_queue_depth() to
> 
> 	if (sdev->queue_depth == queue_depth)
> 		return sdev->queue_depth;
> 
> has no effect. The error remains.
> 
> Now, for a libsas SATA drive, if I add the above change, I do indeed get a
> cap to 32 and the QD changes, no error. That is bothering me as that is
> really inconsistent. Instead of suppressing the error, shouldn't we unify
> AHCI and libsas behavior and error if the user is attempting to set a
> value larger than what the*device*  supports (the host can_queue was
> checked already). In a nutshell, the difference comes form
> sdev->host->can_queue being equal to the device max qd for AHCI but not
> necessarily for libsas.

Yes, I think consistent behaviour would be good. I suppose we just need 
the same check to reject QD of > 32 in ata_change_queue_depth() (and not 
just cap to 32 there).

Having said all that, scsi_device_max_queue_depth() does introduce some 
capping. But let's just consider SATA behaviour now.

> 
> I am tempted to leave things as is for now (not changin gthe current weird
> behavior) and cleaning that up during the next round. Thoughts ?
> 

It's up to you. Obviously we are making an improvement in this series, 
but if we are going to backport then it's better to backport something 
fully working first time.

Thanks,
John

Re: [PATCH v2 2/2] ata: libata-sata: Fix device queue depth control

[Damien Le Moal]

On 9/28/22 16:53, John Garry wrote:
> On 28/09/2022 08:00, Damien Le Moal wrote:
>>> So we don't return. However the following subsequent test does evaluate
>>> true in ata_change_queue_depth():
>>>
>>> if (sdev->queue_depth == queue_depth)
>>> 	return -EINVAL;
>>>
>>> And we error.
>> I dug further into this. For AHCI, I still get an error when trying to set
>> 33. No capping and defaulting to 32. The reason is I believe that
>> sdev_store_queue_depth() has the check:
>>
>> 	if (depth < 1 || depth > sdev->host->can_queue)
>>                  return -EINVAL;
>>
>> as you mentioned. So all good.
>>
>> So changing that last "if" in ata_change_queue_depth() to
>>
>> 	if (sdev->queue_depth == queue_depth)
>> 		return sdev->queue_depth;
>>
>> has no effect. The error remains.
>>
>> Now, for a libsas SATA drive, if I add the above change, I do indeed get a
>> cap to 32 and the QD changes, no error. That is bothering me as that is
>> really inconsistent. Instead of suppressing the error, shouldn't we unify
>> AHCI and libsas behavior and error if the user is attempting to set a
>> value larger than what the*device*  supports (the host can_queue was
>> checked already). In a nutshell, the difference comes form
>> sdev->host->can_queue being equal to the device max qd for AHCI but not
>> necessarily for libsas.
> 
> Yes, I think consistent behaviour would be good. I suppose we just need 
> the same check to reject QD of > 32 in ata_change_queue_depth() (and not 
> just cap to 32 there).
> 
> Having said all that, scsi_device_max_queue_depth() does introduce some 
> capping. But let's just consider SATA behaviour now.
> 
>>
>> I am tempted to leave things as is for now (not changin gthe current weird
>> behavior) and cleaning that up during the next round. Thoughts ?
>>
> 
> It's up to you. Obviously we are making an improvement in this series, 
> but if we are going to backport then it's better to backport something 
> fully working first time.

OK. Since the current behavior has been in place for a long time, no
urgency to change anything now I think.
I will push the current 2 patches for 6.0-fixes and cook a full cleanup &
improvement for 6.1.

> 
> Thanks,
> John

-- 
Damien Le Moal
Western Digital Research