Re: [PATCH 3/3] ata: libata-scsi: Use ata_ncq_supported in ata_scsi_dev_config()

[Hannes Reinecke <hare@suse.de>]

On 6/5/23 11:37, Damien Le Moal wrote:
> On 6/5/23 17:04, Hannes Reinecke wrote:
>> On 6/5/23 03:32, Damien Le Moal wrote:
>>> In ata_scsi_dev_config(), instead of hardconing the test to check if
>>> an ATA device supports NCQ by looking at the ATA_DFLAG_NCQ flag, use
>>> ata_ncq_supported().
>>>
>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>>> ---
>>>    drivers/ata/libata-scsi.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>>> index 8ce90284eb34..22e2e9ab6b60 100644
>>> --- a/drivers/ata/libata-scsi.c
>>> +++ b/drivers/ata/libata-scsi.c
>>> @@ -1122,7 +1122,7 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev)
>>>    	if (dev->flags & ATA_DFLAG_AN)
>>>    		set_bit(SDEV_EVT_MEDIA_CHANGE, sdev->supported_events);
>>>    
>>> -	if (dev->flags & ATA_DFLAG_NCQ)
>>> +	if (ata_ncq_supported(dev))
>>>    		depth = min(sdev->host->can_queue, ata_id_queue_depth(dev->id));
>>>    	depth = min(ATA_MAX_QUEUE, depth);
>>>    	scsi_change_queue_depth(sdev, depth);
>>
>> Argh. ATA NCQ flags. We have ATA_DFLAG_NCQ, ATA_DFLAG_PIO,
>> ATA_DFLAG_NCQ_OFF (and maybe even more which I forgot about).
>> Can we please move them into some more descriptive, ie which flags
>> are for the drive capabilities (ie _can_ the drive do NCQ) and
>> the current current drive status (ie _does_ the drive do NCQ)?
>> As it stands it's quite confusing.
> 
> In include/linux/libata.h, we have:
> 
> ATA_DFLAG_NCQ		= (1 << 3), /* device supports NCQ */
> ATA_DFLAG_PIO		= (1 << 13), /* device limited to PIO mode */
> ATA_DFLAG_NCQ_OFF	= (1 << 14), /* device limited to non-NCQ mode */
> 
> So there are some description. Not enough ?
> 
Well. Guess my point is that ATA_DFLAG_PIO is a device status (which 
might change during runtime), whereas ATA_DFLAG_NCQ is a device setting
(either it does or does not support NCQ).
And ATA_DFLAG_NCQ_OFF is again a device status (device supports NCQ, but
it's disabled for whatever reason).
I'd rather have a more descriptive naming like
ATA_DFLAG_NCQ_SUPPORTED
to clearly indicate that this flag is not about what the driver 
_currently_ is using (as opposed to ATA_DFLAG_PIO), but rather a static
device configuration which won't change whatever I do.

Cheers,

Hannes