Re: [PATCH 1/3] ata: libata-sata: Improve ata_change_queue_depth()

[Damien Le Moal <dlemoal@kernel.org>]

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.

-- 
Damien Le Moal
Western Digital Research