| Message ID | 20230605013212.573489-1-dlemoal@kernel.org |
|---|---|
| Headers | show |
| Series | Cleanups and improvements | expand |
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
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
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.