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@xxxxxxxxxx>
>
> I have some nitpicks below. Regardless of those:
> Reviewed-by: John Garry <john.g.garry@xxxxxxxxxx>
>
> 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.
--
Damien Le Moal
Western Digital Research
