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

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

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