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

[John Garry <john.garry@huawei.com>]

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.

Thanks,
John