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
