Re: [PATCH] ata: libata-transport: fix {dma|pio|xfer}_mode sysfs files

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

On 2022/06/07 5:38, Sergey Shtylyov wrote:
> Hello!
> 
> On 6/6/22 5:42 AM, Damien Le Moal wrote:
> 
>>> The {dma|pio|xfer}_mode sysfs files are incorrectly handled by the
>>> ata_bitfield_name_match() macro which leads to reading such kind of
>>> nonsense from them:
>>>
>>> $ cat /sys/class/ata_device/dev3.0/pio_mode
>>> XFER_UDMA_7, XFER_UDMA_6, XFER_UDMA_5, XFER_UDMA_4, XFER_MW_DMA_4,
>>> XFER_PIO_6, XFER_PIO_5, XFER_PIO_4, XFER_PIO_3, XFER_PIO_2, XFER_PIO_1,
>>> XFER_PIO_0
>>>
>>> Using the correct ata_bitfield_name_search() macro fixes that:
>>>
>>> $ cat /sys/class/ata_device/dev3.0/pio_mode
>>> XFER_PIO_4
>>
>> Looks good, but Documentation/ABI/testing/sysfs-ata says:
> 
>    Completely forgot that the sysfs files are documented as ABIs... :-(
>    Hm, shouldn't that file be added to the libata's entry in MAINTAINERS?
> 
>> pio_mode:       (RO) Transfer modes supported by the device when
>>                 in PIO mode. Mostly used by PATA device.
>>
>> xfer_mode:      (RO) Current transfer mode
>>
>> dma_mode:       (RO) Transfer modes supported by the device when
>>                 in DMA mode. Mostly used by PATA device.
>>
>> which seems incorrect/badly worded for pio_mode and dma_mode. Since these
>> 2 sysfs attributes do not actually device the pio mask (list of supported
> 
>    Device?

advertise :)

> 
>> pio modes) but the pio mode that will be used for that device, we should
>> reword, no ?
> 
>    Yes, of course. :-)
> 
>> What about:
>>
>> pio_mode:       (RO) Transfer mode used by the device when
>>                 in PIO mode. Mostly used by PATA device.
>>
>> xfer_mode:      (RO) Current transfer mode
>>
>> dma_mode:       (RO) Transfer mode used by the device when
>>                 in DMA mode. Mostly used by PATA device.
> 
>    Sounds quite tautological... :-)
>    What about:
> 
> {dma|pio}_mode: (RO) {DMA|PIO} transfer mode used by the device.
>                 Mostly used by PATA devices.
> 
>    I think this should be done in the same patch. Or would you prefer 2 patches?

Let's do 2 patches. Not sure if you can find a fixes tag for the doc update
though. But we should not aggregate the 2 attributes as you did. These doc files
have a defined format and may not be happy with that merged syntax.

> 
> [...]
> 
> MBR, Sergey


-- 
Damien Le Moal
Western Digital Research