On 9/27/22 16:05, John Garry wrote:
> On 27/09/2022 00:05, Damien Le Moal wrote:
>>>> ata_scsi_find_dev() returns NULL, turning
>>>> __ata_change_queue_depth() into a nop, which prevents the user from
>>>> setting the maximum queue depth of ATA devices used with libsas based
>>>> HBAs.
>>>>
>>>> Fix this by renaming __ata_change_queue_depth() to
>>>> ata_change_queue_depth() and adding a pointer to the ata_device
>>>> structure of the target device as argument. This pointer is provided by
>>>> ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of
>>>> a libata managed device and by sas_change_queue_depth() using
>>>> sas_to_ata_dev() in the case of a libsas managed ata device.
>>> This seems ok. But could you alternatively use ata_for_each_dev() and
>>> match by ata_device.sdev pointer? That pointer is set quite late in the
>>> probe, though, so maybe it would not work.
>> Not sure I understand why we should search for the ata device again using
>> ata_for_each_dev() when sas_to_ata_dev() gives us directly what we need
>> for the libsas controlled device... Can you clarify ?
>>
>
> Sure, we can use sas_to_ata_dev() to get the ata_device.
>
> I am just suggesting my way such that we can have a consistent method to
> get the ata_device between all libata users and we don't need to change
> the ata_change_queue_depth() interface. It would be something like:
>
> struct ata_device *ata_scsi_find_dev(struct ata_port *ap, const struct
> scsi_device *scsidev)
> {
> struct ata_link *link;
> struct ata_device *dev;
>
> ata_for_each_link(link, ap, EDGE) {
> ata_for_each_dev(dev, link, ENABLED) {
> if (scsidev == dev->sdev)
> return dev;
> }
> }
> // todo: check pmp
> return NULL;
> }
I see. Need to think about this one... This may also unify the pmp case.
Are you OK with the patch as is though ? We can improve with something
like the above on top later. Really need to fix that qd setting as it is
causing problems for testing devices with/without ncq commands.
>
> Thanks,
> John
--
Damien Le Moal
Western Digital Research
