[PATCH 0/3] Cleanups and improvements

[PATCH 0/3] Cleanups and improvements

[Damien Le Moal]

Improve ata_change_queue_depth() to have its behavior consistent for
libsas managed devices with libata managed devices. Also add a couple of
simple cleanups to use defined helper functions instead of open coding
device flags checks.

Damien Le Moal (3):
  ata: libata-sata: Improve ata_change_queue_depth()
  ata: libata-eh: Use ata_ncq_enabled() in ata_eh_speed_down()
  ata: libata-scsi: Use ata_ncq_supported in ata_scsi_dev_config()

 drivers/ata/libata-eh.c   |  4 +---
 drivers/ata/libata-sata.c | 25 +++++++++++++++----------
 drivers/ata/libata-scsi.c |  2 +-
 3 files changed, 17 insertions(+), 14 deletions(-)

-- 
2.40.1

[PATCH 1/3] ata: libata-sata: Improve ata_change_queue_depth()

[Damien Le Moal]

ata_change_queue_depth() implements different behaviors for ATA devices
managed by libsas than for those managed by libata directly.
Specifically, if a user attempts to set a device queue depth to a value
larger than 32 (ATA_MAX_QUEUE), the queue depth is capped to the maximum
and set to 32 for libsas managed devices whereas for libata managed
devices, the queue depth is unchanged and an error returned to the user.
This is due to the fact that for libsas devices, sdev->host->can_queue
may indicate the host (HBA) maximum number of commands that can be
queued rather than the device maximum queue depth.

Change ata_change_queue_depth() to provide a consistent behavior for all
devices by changing the queue depth capping code to a check that the
user provided value does not exceed the device maximum queue depth.
This check is moved before the code clearing or setting the
ATA_DFLAG_NCQ_OFF flag to ensure that this flag is not modified when an
invlaid queue depth is provided.

While at it, two other small improvements are added:
1) Use ata_ncq_supported() instead of ata_ncq_enabled() and clear the
   ATA_DFLAG_NCQ_OFF flag only and only if needed.
2) If the user provided queue depth is equal to the current queue depth,
   do not return an error as that is useless.

Overall, the behavior of ata_change_queue_depth() for libata managed
devices is unchanged. The behavior with libsas managed devices becomes
consistent with libata managed devices.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-sata.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index e3c9cb617048..56a1cd57a107 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1035,6 +1035,7 @@ int ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
 {
 	struct ata_device *dev;
 	unsigned long flags;
+	int max_queue_depth;
 
 	spin_lock_irqsave(ap->lock, flags);
 
@@ -1044,22 +1045,26 @@ int ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
 		return sdev->queue_depth;
 	}
 
-	/* NCQ enabled? */
-	dev->flags &= ~ATA_DFLAG_NCQ_OFF;
-	if (queue_depth == 1 || !ata_ncq_enabled(dev)) {
+	/* limit queue depth */
+	max_queue_depth = min(ATA_MAX_QUEUE, sdev->host->can_queue);
+	max_queue_depth = min(max_queue_depth, ata_id_queue_depth(dev->id));
+	if (queue_depth > max_queue_depth) {
+		spin_unlock_irqrestore(ap->lock, flags);
+		return -EINVAL;
+	}
+
+	/* NCQ supported ? */
+	if (queue_depth == 1 || !ata_ncq_supported(dev)) {
 		dev->flags |= ATA_DFLAG_NCQ_OFF;
 		queue_depth = 1;
+	} else {
+		dev->flags &= ~ATA_DFLAG_NCQ_OFF;
 	}
 
 	spin_unlock_irqrestore(ap->lock, flags);
 
-	/* limit and apply queue depth */
-	queue_depth = min(queue_depth, sdev->host->can_queue);
-	queue_depth = min(queue_depth, ata_id_queue_depth(dev->id));
-	queue_depth = min(queue_depth, ATA_MAX_QUEUE);
-
-	if (sdev->queue_depth == queue_depth)
-		return -EINVAL;
+	if (queue_depth == sdev->queue_depth)
+		return sdev->queue_depth;
 
 	return scsi_change_queue_depth(sdev, queue_depth);
 }
-- 
2.40.1

[PATCH 2/3] ata: libata-eh: Use ata_ncq_enabled() in ata_eh_speed_down()

[Damien Le Moal]

Instead of hardconfign the device flag tests to detect if NCQ is
supported and enabled in ata_eh_speed_down(), use ata_ncq_enabled().

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-eh.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index a6c901811802..c0993d755e8d 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1817,9 +1817,7 @@ static unsigned int ata_eh_speed_down(struct ata_device *dev,
 	verdict = ata_eh_speed_down_verdict(dev);
 
 	/* turn off NCQ? */
-	if ((verdict & ATA_EH_SPDN_NCQ_OFF) &&
-	    (dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ |
-			   ATA_DFLAG_NCQ_OFF)) == ATA_DFLAG_NCQ) {
+	if ((verdict & ATA_EH_SPDN_NCQ_OFF) && ata_ncq_enabled(dev)) {
 		dev->flags |= ATA_DFLAG_NCQ_OFF;
 		ata_dev_warn(dev, "NCQ disabled due to excessive errors\n");
 		goto done;
-- 
2.40.1

[PATCH 3/3] ata: libata-scsi: Use ata_ncq_supported in ata_scsi_dev_config()

[Damien Le Moal]

In ata_scsi_dev_config(), instead of hardconing the test to check if
an ATA device supports NCQ by looking at the ATA_DFLAG_NCQ flag, use
ata_ncq_supported().

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 8ce90284eb34..22e2e9ab6b60 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1122,7 +1122,7 @@ 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)
+	if (ata_ncq_supported(dev))
 		depth = min(sdev->host->can_queue, ata_id_queue_depth(dev->id));
 	depth = min(ATA_MAX_QUEUE, depth);
 	scsi_change_queue_depth(sdev, depth);
-- 
2.40.1

Re: [PATCH 1/3] ata: libata-sata: Improve ata_change_queue_depth()

[Hannes Reinecke]

On 6/5/23 03:32, Damien Le Moal wrote:
> ata_change_queue_depth() implements different behaviors for ATA devices
> managed by libsas than for those managed by libata directly.
> Specifically, if a user attempts to set a device queue depth to a value
> larger than 32 (ATA_MAX_QUEUE), the queue depth is capped to the maximum
> and set to 32 for libsas managed devices whereas for libata managed
> devices, the queue depth is unchanged and an error returned to the user.
> This is due to the fact that for libsas devices, sdev->host->can_queue
> may indicate the host (HBA) maximum number of commands that can be
> queued rather than the device maximum queue depth.
> 
> Change ata_change_queue_depth() to provide a consistent behavior for all
> devices by changing the queue depth capping code to a check that the
> user provided value does not exceed the device maximum queue depth.
> This check is moved before the code clearing or setting the
> ATA_DFLAG_NCQ_OFF flag to ensure that this flag is not modified when an
> invlaid queue depth is provided.
> 
> While at it, two other small improvements are added:
> 1) Use ata_ncq_supported() instead of ata_ncq_enabled() and clear the
>     ATA_DFLAG_NCQ_OFF flag only and only if needed.
> 2) If the user provided queue depth is equal to the current queue depth,
>     do not return an error as that is useless.
> 
> Overall, the behavior of ata_change_queue_depth() for libata managed
> devices is unchanged. The behavior with libsas managed devices becomes
> consistent with libata managed devices.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-sata.c | 25 +++++++++++++++----------
>   1 file changed, 15 insertions(+), 10 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

Re: [PATCH 2/3] ata: libata-eh: Use ata_ncq_enabled() in ata_eh_speed_down()

[Hannes Reinecke]

On 6/5/23 03:32, Damien Le Moal wrote:
> Instead of hardconfign the device flag tests to detect if NCQ is
> supported and enabled in ata_eh_speed_down(), use ata_ncq_enabled().
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-eh.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

Re: [PATCH 3/3] ata: libata-scsi: Use ata_ncq_supported in ata_scsi_dev_config()

[Hannes Reinecke]

On 6/5/23 03:32, Damien Le Moal wrote:
> In ata_scsi_dev_config(), instead of hardconing the test to check if
> an ATA device supports NCQ by looking at the ATA_DFLAG_NCQ flag, use
> ata_ncq_supported().
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-scsi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 8ce90284eb34..22e2e9ab6b60 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1122,7 +1122,7 @@ 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)
> +	if (ata_ncq_supported(dev))
>   		depth = min(sdev->host->can_queue, ata_id_queue_depth(dev->id));
>   	depth = min(ATA_MAX_QUEUE, depth);
>   	scsi_change_queue_depth(sdev, depth);

Argh. ATA NCQ flags. We have ATA_DFLAG_NCQ, ATA_DFLAG_PIO, 
ATA_DFLAG_NCQ_OFF (and maybe even more which I forgot about).
Can we please move them into some more descriptive, ie which flags
are for the drive capabilities (ie _can_ the drive do NCQ) and
the current current drive status (ie _does_ the drive do NCQ)?
As it stands it's quite confusing.

But probably not a problem with this patch, so:

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

Re: [PATCH 3/3] ata: libata-scsi: Use ata_ncq_supported in ata_scsi_dev_config()

[Damien Le Moal]

On 6/5/23 17:04, Hannes Reinecke wrote:
> On 6/5/23 03:32, Damien Le Moal wrote:
>> In ata_scsi_dev_config(), instead of hardconing the test to check if
>> an ATA device supports NCQ by looking at the ATA_DFLAG_NCQ flag, use
>> ata_ncq_supported().
>>
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> ---
>>   drivers/ata/libata-scsi.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 8ce90284eb34..22e2e9ab6b60 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -1122,7 +1122,7 @@ 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)
>> +	if (ata_ncq_supported(dev))
>>   		depth = min(sdev->host->can_queue, ata_id_queue_depth(dev->id));
>>   	depth = min(ATA_MAX_QUEUE, depth);
>>   	scsi_change_queue_depth(sdev, depth);
> 
> Argh. ATA NCQ flags. We have ATA_DFLAG_NCQ, ATA_DFLAG_PIO, 
> ATA_DFLAG_NCQ_OFF (and maybe even more which I forgot about).
> Can we please move them into some more descriptive, ie which flags
> are for the drive capabilities (ie _can_ the drive do NCQ) and
> the current current drive status (ie _does_ the drive do NCQ)?
> As it stands it's quite confusing.

In include/linux/libata.h, we have:

ATA_DFLAG_NCQ		= (1 << 3), /* device supports NCQ */
ATA_DFLAG_PIO		= (1 << 13), /* device limited to PIO mode */
ATA_DFLAG_NCQ_OFF	= (1 << 14), /* device limited to non-NCQ mode */

So there are some description. Not enough ?

> 
> But probably not a problem with this patch, so:
> 
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> 
> Cheers,
> 
> Hannes

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/3] ata: libata-eh: Use ata_ncq_enabled() in ata_eh_speed_down()

[Sergei Shtylyov]

On 6/5/23 4:32 AM, Damien Le Moal wrote:

> Instead of hardconfign the device flag tests to detect if NCQ is

   Hardcoding?

> supported and enabled in ata_eh_speed_down(), use ata_ncq_enabled().
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
[...]

MBR, Sergey

Re: [PATCH 3/3] ata: libata-scsi: Use ata_ncq_supported in ata_scsi_dev_config()

[Hannes Reinecke]

On 6/5/23 11:37, Damien Le Moal wrote:
> On 6/5/23 17:04, Hannes Reinecke wrote:
>> On 6/5/23 03:32, Damien Le Moal wrote:
>>> In ata_scsi_dev_config(), instead of hardconing the test to check if
>>> an ATA device supports NCQ by looking at the ATA_DFLAG_NCQ flag, use
>>> ata_ncq_supported().
>>>
>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>>> ---
>>>    drivers/ata/libata-scsi.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>>> index 8ce90284eb34..22e2e9ab6b60 100644
>>> --- a/drivers/ata/libata-scsi.c
>>> +++ b/drivers/ata/libata-scsi.c
>>> @@ -1122,7 +1122,7 @@ 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)
>>> +	if (ata_ncq_supported(dev))
>>>    		depth = min(sdev->host->can_queue, ata_id_queue_depth(dev->id));
>>>    	depth = min(ATA_MAX_QUEUE, depth);
>>>    	scsi_change_queue_depth(sdev, depth);
>>
>> Argh. ATA NCQ flags. We have ATA_DFLAG_NCQ, ATA_DFLAG_PIO,
>> ATA_DFLAG_NCQ_OFF (and maybe even more which I forgot about).
>> Can we please move them into some more descriptive, ie which flags
>> are for the drive capabilities (ie _can_ the drive do NCQ) and
>> the current current drive status (ie _does_ the drive do NCQ)?
>> As it stands it's quite confusing.
> 
> In include/linux/libata.h, we have:
> 
> ATA_DFLAG_NCQ		= (1 << 3), /* device supports NCQ */
> ATA_DFLAG_PIO		= (1 << 13), /* device limited to PIO mode */
> ATA_DFLAG_NCQ_OFF	= (1 << 14), /* device limited to non-NCQ mode */
> 
> So there are some description. Not enough ?
> 
Well. Guess my point is that ATA_DFLAG_PIO is a device status (which 
might change during runtime), whereas ATA_DFLAG_NCQ is a device setting
(either it does or does not support NCQ).
And ATA_DFLAG_NCQ_OFF is again a device status (device supports NCQ, but
it's disabled for whatever reason).
I'd rather have a more descriptive naming like
ATA_DFLAG_NCQ_SUPPORTED
to clearly indicate that this flag is not about what the driver 
_currently_ is using (as opposed to ATA_DFLAG_PIO), but rather a static
device configuration which won't change whatever I do.

Cheers,

Hannes

Re: [PATCH 3/3] ata: libata-scsi: Use ata_ncq_supported in ata_scsi_dev_config()

[Sergei Shtylyov]

On 6/5/23 4:32 AM, Damien Le Moal wrote:

> In ata_scsi_dev_config(), instead of hardconing the test to check if

   Again, hardcoding?

> an ATA device supports NCQ by looking at the ATA_DFLAG_NCQ flag, use
> ata_ncq_supported().
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
[...]

MBR, Sergey

Re: [PATCH 2/3] ata: libata-eh: Use ata_ncq_enabled() in ata_eh_speed_down()

[Damien Le Moal]

On 6/5/23 18:45, Sergei Shtylyov wrote:
> On 6/5/23 4:32 AM, Damien Le Moal wrote:
> 
>> Instead of hardconfign the device flag tests to detect if NCQ is
> 
>    Hardcoding?

Yes. Will fix. Thanks.

> 
>> supported and enabled in ata_eh_speed_down(), use ata_ncq_enabled().
>>
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> [...]
> 
> MBR, Sergey

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/3] ata: libata-scsi: Use ata_ncq_supported in ata_scsi_dev_config()

[Damien Le Moal]

On 6/5/23 18:47, Sergei Shtylyov wrote:
> On 6/5/23 4:32 AM, Damien Le Moal wrote:
> 
>> In ata_scsi_dev_config(), instead of hardconing the test to check if
> 
>    Again, hardcoding?

Yes... Square fingers today :)

> 
>> an ATA device supports NCQ by looking at the ATA_DFLAG_NCQ flag, use
>> ata_ncq_supported().
>>
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> [...]
> 
> MBR, Sergey

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/3] ata: libata-scsi: Use ata_ncq_supported in ata_scsi_dev_config()

[Damien Le Moal]

On 6/5/23 18:46, Hannes Reinecke wrote:
> On 6/5/23 11:37, Damien Le Moal wrote:
>> On 6/5/23 17:04, Hannes Reinecke wrote:
>>> On 6/5/23 03:32, Damien Le Moal wrote:
>>>> In ata_scsi_dev_config(), instead of hardconing the test to check if
>>>> an ATA device supports NCQ by looking at the ATA_DFLAG_NCQ flag, use
>>>> ata_ncq_supported().
>>>>
>>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>>>> ---
>>>>    drivers/ata/libata-scsi.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>>>> index 8ce90284eb34..22e2e9ab6b60 100644
>>>> --- a/drivers/ata/libata-scsi.c
>>>> +++ b/drivers/ata/libata-scsi.c
>>>> @@ -1122,7 +1122,7 @@ 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)
>>>> +	if (ata_ncq_supported(dev))
>>>>    		depth = min(sdev->host->can_queue, ata_id_queue_depth(dev->id));
>>>>    	depth = min(ATA_MAX_QUEUE, depth);
>>>>    	scsi_change_queue_depth(sdev, depth);
>>>
>>> Argh. ATA NCQ flags. We have ATA_DFLAG_NCQ, ATA_DFLAG_PIO,
>>> ATA_DFLAG_NCQ_OFF (and maybe even more which I forgot about).
>>> Can we please move them into some more descriptive, ie which flags
>>> are for the drive capabilities (ie _can_ the drive do NCQ) and
>>> the current current drive status (ie _does_ the drive do NCQ)?
>>> As it stands it's quite confusing.
>>
>> In include/linux/libata.h, we have:
>>
>> ATA_DFLAG_NCQ		= (1 << 3), /* device supports NCQ */
>> ATA_DFLAG_PIO		= (1 << 13), /* device limited to PIO mode */
>> ATA_DFLAG_NCQ_OFF	= (1 << 14), /* device limited to non-NCQ mode */
>>
>> So there are some description. Not enough ?
>>
> Well. Guess my point is that ATA_DFLAG_PIO is a device status (which 
> might change during runtime), whereas ATA_DFLAG_NCQ is a device setting
> (either it does or does not support NCQ).
> And ATA_DFLAG_NCQ_OFF is again a device status (device supports NCQ, but
> it's disabled for whatever reason).
> I'd rather have a more descriptive naming like
> ATA_DFLAG_NCQ_SUPPORTED
> to clearly indicate that this flag is not about what the driver 
> _currently_ is using (as opposed to ATA_DFLAG_PIO), but rather a static
> device configuration which won't change whatever I do.

Yes, agree.

There is also some funky logic going on with all this:
ATA_DFLAG_PIO meaning "PIO supported" should always be set for NCQ drives,
because NCQ drives necessarily can do DMA and so they can do PIO as well...
So patterns like:

dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ |
		ATA_DFLAG_NCQ_OFF)) == ATA_DFLAG_NCQ

To test if NCQ is supported does not really make any sense... Example:
ata_scsi_security_inout_xlat() has:

bool dma = !(qc->dev->flags & ATA_DFLAG_PIO);

Sure... That works, but that is not technically super correct has a DMA capable
drive can do PIO as well.

I think we are dealing with ata history here as everything was "PIO" at the
beginning of ata times, so that flag did not really needed to exist and was
added later to differentiate with NCQ case.

Splitting the gigantic enum in include/linux/libata.h into manageable pieces
(e.g. "enum ata_device_flags { ... };") with some renaming and better kdocs
would help clarifies things. Will try to work on that.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/3] ata: libata-sata: Improve ata_change_queue_depth()

[John Garry]

On 05/06/2023 02:32, Damien Le Moal wrote:
> ata_change_queue_depth() implements different behaviors for ATA devices
> managed by libsas than for those managed by libata directly.
> Specifically, if a user attempts to set a device queue depth to a value
> larger than 32 (ATA_MAX_QUEUE), the queue depth is capped to the maximum
> and set to 32 for libsas managed devices whereas for libata managed
> devices, the queue depth is unchanged and an error returned to the user.
> This is due to the fact that for libsas devices, sdev->host->can_queue
> may indicate the host (HBA) maximum number of commands that can be
> queued rather than the device maximum queue depth.
> 
> Change ata_change_queue_depth() to provide a consistent behavior for all
> devices by changing the queue depth capping code to a check that the
> user provided value does not exceed the device maximum queue depth.
> This check is moved before the code clearing or setting the
> ATA_DFLAG_NCQ_OFF flag to ensure that this flag is not modified when an
> invlaid queue depth is provided.
> 
> While at it, two other small improvements are added:
> 1) Use ata_ncq_supported() instead of ata_ncq_enabled() and clear the
>     ATA_DFLAG_NCQ_OFF flag only and only if needed.
> 2) If the user provided queue depth is equal to the current queue depth,
>     do not return an error as that is useless.
> 
> Overall, the behavior of ata_change_queue_depth() for libata managed
> devices is unchanged. The behavior with libsas managed devices becomes
> consistent with libata managed devices.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

I have some nitpicks below. Regardless of those:
Reviewed-by: John Garry <john.g.garry@oracle.com>

Thanks!!

> ---
>   drivers/ata/libata-sata.c | 25 +++++++++++++++----------
>   1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index e3c9cb617048..56a1cd57a107 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -1035,6 +1035,7 @@ int ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
>   {
>   	struct ata_device *dev;
>   	unsigned long flags;
> +	int max_queue_depth;
>   
>   	spin_lock_irqsave(ap->lock, flags);
>   
> @@ -1044,22 +1045,26 @@ int ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
>   		return sdev->queue_depth;
>   	}
>   
> -	/* NCQ enabled? */
> -	dev->flags &= ~ATA_DFLAG_NCQ_OFF;
> -	if (queue_depth == 1 || !ata_ncq_enabled(dev)) {
> +	/* limit queue depth */
> +	max_queue_depth = min(ATA_MAX_QUEUE, sdev->host->can_queue);
> +	max_queue_depth = min(max_queue_depth, ata_id_queue_depth(dev->id));
> +	if (queue_depth > max_queue_depth) {
> +		spin_unlock_irqrestore(ap->lock, flags);
> +		return -EINVAL;
> +	}
> +
> +	/* NCQ supported ? */

nit: I find this comment so vague that it is ambiguous. The previous 
code had it. What exactly are we trying to say?

> +	if (queue_depth == 1 || !ata_ncq_supported(dev)) {
>   		dev->flags |= ATA_DFLAG_NCQ_OFF;

super nit: I don't like checking a value and then setting it to the same 
pass if the check passes, so ...

>   		queue_depth = 1;
> +	} else {
> +		dev->flags &= ~ATA_DFLAG_NCQ_OFF;
>   	}
>   

.. we could have instead:

if (queue_depth == 1)
	dev->flags |= ATA_DFLAG_NCQ_OFF;
else if (!ata_ncq_supported(dev)) {
	dev->flags |= ATA_DFLAG_NCQ_OFF;
	queue_depth = 1;
} else
	dev->flags &= ~ATA_DFLAG_NCQ_OFF;
	
Maybe too long-winded.

Re: [PATCH 1/3] ata: libata-sata: Improve ata_change_queue_depth()

[Damien Le Moal]

On 6/5/23 18:58, John Garry wrote:
> On 05/06/2023 02:32, Damien Le Moal wrote:
>> ata_change_queue_depth() implements different behaviors for ATA devices
>> managed by libsas than for those managed by libata directly.
>> Specifically, if a user attempts to set a device queue depth to a value
>> larger than 32 (ATA_MAX_QUEUE), the queue depth is capped to the maximum
>> and set to 32 for libsas managed devices whereas for libata managed
>> devices, the queue depth is unchanged and an error returned to the user.
>> This is due to the fact that for libsas devices, sdev->host->can_queue
>> may indicate the host (HBA) maximum number of commands that can be
>> queued rather than the device maximum queue depth.
>>
>> Change ata_change_queue_depth() to provide a consistent behavior for all
>> devices by changing the queue depth capping code to a check that the
>> user provided value does not exceed the device maximum queue depth.
>> This check is moved before the code clearing or setting the
>> ATA_DFLAG_NCQ_OFF flag to ensure that this flag is not modified when an
>> invlaid queue depth is provided.
>>
>> While at it, two other small improvements are added:
>> 1) Use ata_ncq_supported() instead of ata_ncq_enabled() and clear the
>>     ATA_DFLAG_NCQ_OFF flag only and only if needed.
>> 2) If the user provided queue depth is equal to the current queue depth,
>>     do not return an error as that is useless.
>>
>> Overall, the behavior of ata_change_queue_depth() for libata managed
>> devices is unchanged. The behavior with libsas managed devices becomes
>> consistent with libata managed devices.
>>
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> 
> I have some nitpicks below. Regardless of those:
> Reviewed-by: John Garry <john.g.garry@oracle.com>
> 
> Thanks!!
> 
>> ---
>>   drivers/ata/libata-sata.c | 25 +++++++++++++++----------
>>   1 file changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
>> index e3c9cb617048..56a1cd57a107 100644
>> --- a/drivers/ata/libata-sata.c
>> +++ b/drivers/ata/libata-sata.c
>> @@ -1035,6 +1035,7 @@ int ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
>>   {
>>   	struct ata_device *dev;
>>   	unsigned long flags;
>> +	int max_queue_depth;
>>   
>>   	spin_lock_irqsave(ap->lock, flags);
>>   
>> @@ -1044,22 +1045,26 @@ int ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
>>   		return sdev->queue_depth;
>>   	}
>>   
>> -	/* NCQ enabled? */
>> -	dev->flags &= ~ATA_DFLAG_NCQ_OFF;
>> -	if (queue_depth == 1 || !ata_ncq_enabled(dev)) {
>> +	/* limit queue depth */
>> +	max_queue_depth = min(ATA_MAX_QUEUE, sdev->host->can_queue);
>> +	max_queue_depth = min(max_queue_depth, ata_id_queue_depth(dev->id));
>> +	if (queue_depth > max_queue_depth) {
>> +		spin_unlock_irqrestore(ap->lock, flags);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* NCQ supported ? */
> 
> nit: I find this comment so vague that it is ambiguous. The previous 
> code had it. What exactly are we trying to say?

I will detail this.

> 
>> +	if (queue_depth == 1 || !ata_ncq_supported(dev)) {
>>   		dev->flags |= ATA_DFLAG_NCQ_OFF;
> 
> super nit: I don't like checking a value and then setting it to the same 
> pass if the check passes, so ...
> 
>>   		queue_depth = 1;
>> +	} else {
>> +		dev->flags &= ~ATA_DFLAG_NCQ_OFF;
>>   	}
>>   
> 
> .. we could have instead:
> 
> if (queue_depth == 1)
> 	dev->flags |= ATA_DFLAG_NCQ_OFF;
> else if (!ata_ncq_supported(dev)) {
> 	dev->flags |= ATA_DFLAG_NCQ_OFF;
> 	queue_depth = 1;
> } else
> 	dev->flags &= ~ATA_DFLAG_NCQ_OFF;
> 	
> Maybe too long-winded.

Yes, that makes the code self-explanatory but is indeed a bit verbose. I will
improve the comment instead.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/3] ata: libata-eh: Use ata_ncq_enabled() in ata_eh_speed_down()

[John Garry]

On 05/06/2023 02:32, Damien Le Moal wrote:
> Instead of hardconfign the device flag tests to detect if NCQ is
> supported and enabled in ata_eh_speed_down(), use ata_ncq_enabled().
> 
> Signed-off-by: Damien Le Moal<dlemoal@kernel.org>

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