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
