[PATCH] ata: libata-sata: allow re-enabling NCQ with the same queue_depth

[PATCH] ata: libata-sata: allow re-enabling NCQ with the same queue_depth

[Igor Pylypiv]

When ata_eh_speed_down() disables NCQ due to excessive errors, it sets
ATA_DFLAG_NCQ_OFF without changing sdev->queue_depth. Userspace can
re-enable NCQ by writing a queue depth > 1 to the sysfs queue_depth
attribute, which invokes ata_change_queue_depth() to clear
ATA_DFLAG_NCQ_OFF.

However, ata_change_queue_depth() returns early if the requested
queue_depth is equal to sdev->queue_depth before clearing
ATA_DFLAG_NCQ_OFF. As a result, writing a different queue depth > 1
re-enables NCQ, whereas writing the current sdev->queue_depth value
leaves ATA_DFLAG_NCQ_OFF set.

Since ata_change_queue_depth() already checks whether queue_depth equals
sdev->queue_depth before calling scsi_change_queue_depth(), remove the
early check so that writing the current queue_depth > 1 clears
ATA_DFLAG_NCQ_OFF and re-enables NCQ.

Fixes: 45623d33bfc0 ("ata: libata-sata: Improve ata_change_queue_depth()")
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
Commit 45623d33bfc0 ("ata: libata-sata: Improve ata_change_queue_depth()")
didn't add the first queue depth check, but it changed the return value
of the second check from -EINVAL to sdev->queue_depth so it seemed like
a better candidate for the Fixes tag.

 drivers/ata/libata-sata.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 32fd0e804654..d917242ac136 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1287,7 +1287,7 @@ int ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
 	spin_lock_irqsave(ap->lock, flags);
 
 	dev = ata_scsi_find_dev(ap, sdev);
-	if (!dev || queue_depth < 1 || queue_depth == sdev->queue_depth) {
+	if (!dev || queue_depth < 1) {
 		spin_unlock_irqrestore(ap->lock, flags);
 		return sdev->queue_depth;
 	}
-- 
2.56.0.317.g454f8302e1-goog

Re: [PATCH] ata: libata-sata: allow re-enabling NCQ with the same queue_depth

[Niklas Cassel]

On Thu, Oct 29, 2026 at 11:44:45AM -0700, Igor Pylypiv wrote:
> When ata_eh_speed_down() disables NCQ due to excessive errors, it sets
> ATA_DFLAG_NCQ_OFF without changing sdev->queue_depth. Userspace can
> re-enable NCQ by writing a queue depth > 1 to the sysfs queue_depth
> attribute, which invokes ata_change_queue_depth() to clear
> ATA_DFLAG_NCQ_OFF.
> 
> However, ata_change_queue_depth() returns early if the requested
> queue_depth is equal to sdev->queue_depth before clearing
> ATA_DFLAG_NCQ_OFF. As a result, writing a different queue depth > 1
> re-enables NCQ, whereas writing the current sdev->queue_depth value
> leaves ATA_DFLAG_NCQ_OFF set.
> 
> Since ata_change_queue_depth() already checks whether queue_depth equals
> sdev->queue_depth before calling scsi_change_queue_depth(), remove the
> early check so that writing the current queue_depth > 1 clears
> ATA_DFLAG_NCQ_OFF and re-enables NCQ.
> 
> Fixes: 45623d33bfc0 ("ata: libata-sata: Improve ata_change_queue_depth()")
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
> Commit 45623d33bfc0 ("ata: libata-sata: Improve ata_change_queue_depth()")
> didn't add the first queue depth check, but it changed the return value
> of the second check from -EINVAL to sdev->queue_depth so it seemed like
> a better candidate for the Fixes tag.
> 
>  drivers/ata/libata-sata.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 32fd0e804654..d917242ac136 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -1287,7 +1287,7 @@ int ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
>  	spin_lock_irqsave(ap->lock, flags);
>  
>  	dev = ata_scsi_find_dev(ap, sdev);
> -	if (!dev || queue_depth < 1 || queue_depth == sdev->queue_depth) {
> +	if (!dev || queue_depth < 1) {
>  		spin_unlock_irqrestore(ap->lock, flags);
>  		return sdev->queue_depth;
>  	}
> -- 
> 2.56.0.317.g454f8302e1-goog
> 

You could probably even change you commit message to say:
before 45623d33bfc0 ("ata: libata-sata: Improve ata_change_queue_depth()"),
ata_change_queue_depth() cleared ATA_DFLAG_NCQ_OFF unconditionally:
-       dev->flags &= ~ATA_DFLAG_NCQ_OFF;

So the subject of the patch could theoretically be:
ata: libata-sata: re-allow re-enabling NCQ with the same queue_depth

Perhaps that is too many re-..

Anyway, re-gardless:
Reviewed-by: Niklas Cassel <cassel@kernel.org>