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

[Damien Le Moal <dlemoal@kernel.org>]

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 ?

> 
> But probably not a problem with this patch, so:
> 
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> 
> Cheers,
> 
> Hannes

-- 
Damien Le Moal
Western Digital Research