On 6/5/23 18:46, Hannes Reinecke wrote:
> 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@xxxxxxxxxx>
>>>> ---
>>>> 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.
Yes, agree.
There is also some funky logic going on with all this:
ATA_DFLAG_PIO meaning "PIO supported" should always be set for NCQ drives,
because NCQ drives necessarily can do DMA and so they can do PIO as well...
So patterns like:
dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ |
ATA_DFLAG_NCQ_OFF)) == ATA_DFLAG_NCQ
To test if NCQ is supported does not really make any sense... Example:
ata_scsi_security_inout_xlat() has:
bool dma = !(qc->dev->flags & ATA_DFLAG_PIO);
Sure... That works, but that is not technically super correct has a DMA capable
drive can do PIO as well.
I think we are dealing with ata history here as everything was "PIO" at the
beginning of ata times, so that flag did not really needed to exist and was
added later to differentiate with NCQ case.
Splitting the gigantic enum in include/linux/libata.h into manageable pieces
(e.g. "enum ata_device_flags { ... };") with some renaming and better kdocs
would help clarifies things. Will try to work on that.
--
Damien Le Moal
Western Digital Research
