* [PATCH v2 1/3] ata: libata-sata: Improve ata_change_queue_depth()
2023-06-05 10:52 [PATCH v2 0/3] Cleanups and improvements Damien Le Moal
@ 2023-06-05 10:52 ` Damien Le Moal
2023-06-05 10:59 ` Johannes Thumshirn
2023-06-05 10:52 ` [PATCH v2 2/3] ata: libata-eh: Use ata_ncq_enabled() in ata_eh_speed_down() Damien Le Moal
2023-06-05 10:52 ` [PATCH v2 3/3] ata: libata-scsi: Use ata_ncq_supported in ata_scsi_dev_config() Damien Le Moal
2 siblings, 1 reply; 10+ messages in thread
From: Damien Le Moal @ 2023-06-05 10:52 UTC (permalink / raw)
To: linux-ide, linux-scsi, Martin K . Petersen; +Cc: John Garry, Jason Yan
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>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: John Garry <john.g.garry@oracle.com>
---
drivers/ata/libata-sata.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index e3c9cb617048..6c07025011ed 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,32 @@ 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)) {
+ /*
+ * Make sure that the queue depth requested does not exceed the device
+ * capabilities.
+ */
+ 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;
+ }
+
+ /*
+ * If NCQ is not supported by the device or if the target queue depth
+ * is 1 (to disable drive side command queueing), turn off NCQ.
+ */
+ 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 2/3] ata: libata-eh: Use ata_ncq_enabled() in ata_eh_speed_down()
2023-06-05 10:52 [PATCH v2 0/3] Cleanups and improvements Damien Le Moal
2023-06-05 10:52 ` [PATCH v2 1/3] ata: libata-sata: Improve ata_change_queue_depth() Damien Le Moal
@ 2023-06-05 10:52 ` Damien Le Moal
2023-06-05 11:00 ` Johannes Thumshirn
2023-06-05 12:33 ` John Garry
2023-06-05 10:52 ` [PATCH v2 3/3] ata: libata-scsi: Use ata_ncq_supported in ata_scsi_dev_config() Damien Le Moal
2 siblings, 2 replies; 10+ messages in thread
From: Damien Le Moal @ 2023-06-05 10:52 UTC (permalink / raw)
To: linux-ide, linux-scsi, Martin K . Petersen; +Cc: John Garry, Jason Yan
In ata_eh_speed_down(), instead of hard-coding the test on the device
flags to detect if NCQ is supported and enabled, use ata_ncq_enabled().
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
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 c7336a0a884d..b80e68000dd3 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 3/3] ata: libata-scsi: Use ata_ncq_supported in ata_scsi_dev_config()
2023-06-05 10:52 [PATCH v2 0/3] Cleanups and improvements Damien Le Moal
2023-06-05 10:52 ` [PATCH v2 1/3] ata: libata-sata: Improve ata_change_queue_depth() Damien Le Moal
2023-06-05 10:52 ` [PATCH v2 2/3] ata: libata-eh: Use ata_ncq_enabled() in ata_eh_speed_down() Damien Le Moal
@ 2023-06-05 10:52 ` Damien Le Moal
2023-06-05 11:00 ` Johannes Thumshirn
2023-06-05 12:32 ` John Garry
2 siblings, 2 replies; 10+ messages in thread
From: Damien Le Moal @ 2023-06-05 10:52 UTC (permalink / raw)
To: linux-ide, linux-scsi, Martin K . Petersen; +Cc: John Garry, Jason Yan
In ata_scsi_dev_config(), instead of hard-coding 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>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
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 7bb12deab70c..9e79998e3958 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
^ permalink raw reply related [flat|nested] 10+ messages in thread