| Message ID | 20230605013212.573489-2-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| 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 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.
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.
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); }
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(-)