Re: [PATCH v2 2/2] ata: libata-sata: Fix device queue depth control

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 9/28/22 16:53, John Garry wrote:
> On 28/09/2022 08:00, Damien Le Moal wrote:
>>> So we don't return. However the following subsequent test does evaluate
>>> true in ata_change_queue_depth():
>>>
>>> if (sdev->queue_depth == queue_depth)
>>> 	return -EINVAL;
>>>
>>> And we error.
>> I dug further into this. For AHCI, I still get an error when trying to set
>> 33. No capping and defaulting to 32. The reason is I believe that
>> sdev_store_queue_depth() has the check:
>>
>> 	if (depth < 1 || depth > sdev->host->can_queue)
>>                  return -EINVAL;
>>
>> as you mentioned. So all good.
>>
>> So changing that last "if" in ata_change_queue_depth() to
>>
>> 	if (sdev->queue_depth == queue_depth)
>> 		return sdev->queue_depth;
>>
>> has no effect. The error remains.
>>
>> Now, for a libsas SATA drive, if I add the above change, I do indeed get a
>> cap to 32 and the QD changes, no error. That is bothering me as that is
>> really inconsistent. Instead of suppressing the error, shouldn't we unify
>> AHCI and libsas behavior and error if the user is attempting to set a
>> value larger than what the*device*  supports (the host can_queue was
>> checked already). In a nutshell, the difference comes form
>> sdev->host->can_queue being equal to the device max qd for AHCI but not
>> necessarily for libsas.
> 
> Yes, I think consistent behaviour would be good. I suppose we just need 
> the same check to reject QD of > 32 in ata_change_queue_depth() (and not 
> just cap to 32 there).
> 
> Having said all that, scsi_device_max_queue_depth() does introduce some 
> capping. But let's just consider SATA behaviour now.
> 
>>
>> I am tempted to leave things as is for now (not changin gthe current weird
>> behavior) and cleaning that up during the next round. Thoughts ?
>>
> 
> It's up to you. Obviously we are making an improvement in this series, 
> but if we are going to backport then it's better to backport something 
> fully working first time.

OK. Since the current behavior has been in place for a long time, no
urgency to change anything now I think.
I will push the current 2 patches for 6.0-fixes and cook a full cleanup &
improvement for 6.1.

> 
> Thanks,
> John

-- 
Damien Le Moal
Western Digital Research