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@xxxxxxxxxx>
---
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
