Re: [PATCH 07/12] pci: epf-test: Add debug and error messages

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

On 2/15/23 22:49, Arnd Bergmann wrote:
> On Wed, Feb 15, 2023, at 14:24, Greg Kroah-Hartman wrote:
>> On Wed, Feb 15, 2023 at 09:18:48PM +0900, Damien Le Moal wrote:
>>> On 2/15/23 21:01, Greg Kroah-Hartman wrote:
>>>>>>> @@ -330,6 +330,10 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test, bool use_dma)
>>>>>>>  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
>>>>>>>  	volatile struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
>>>>>>
>>>>>> note, volatile is almost always wrong, please fix that up.
>>>>>
>>>>> OK. Will think of something else.
>>>>
>>>> If this is io memory, use the proper accessors to access it.  If it is
>>>> not io memory, then why is it marked volatile at all?
>>>
>>> This is a PCI bar memory. So I can simply copy the structure locally with
>>> memcpy_fromio() and memcpy_toio().
>>
>> Great, please do so instead of trying to access it directly like this,
>> which will break on some platforms.
> 
> I think the reverse is true here: looking at where the pointer comes
> from, 'reg' is actually the result of dma_alloc_coherent() in the
> memory of the local (endpoint) machine, though it appears as a BAR on
> the remote (host) side and gets mapped with ioremap() there.
> 
> This means that the host must use readl/write/memcpy_fromio/memcpy_toio
> to access the buffer, matching the __iomem token there, while the
> endpoint side not use those. On some machines, readl/write take
> arguments that are not compatible with normal pointers, and will
> do something completely different there.
> 
> A volatile access is not the worst option here, though this conflicts
> with the '__packed' annotation in the structure definition that
> may require bytewise access on architectures without unaligned
> access.
> 
> I would drop the __packed in the definition, possibly annotating
> only the 64-bit src_addr and dst_addr members as __packed to ensure
> the layout is unchanged but the structure as a whole is 32-bit
> aligned, and then use READ_ONCE()/WRITE_ONCE() to atomically
> access each member in the coherent buffer.

I guess that would work too. But given that there are accesses to individual
members all over the place, I think it would be easier to get a local copy of
the reg structure in pci_epf_test_cmd_handler() and pass a pointer of that local
copy to the pci_epf_test_xxx() functions. The only READ_ONCE() needed would be
to test the command field on entry to pci_epf_test_cmd_handler() to be sure that
we have a valid command.

The host side always sets the reg command field last, which I think kind of
assumes an ordered update on the EP side (all other fields set before the
command field). That does seem a bit fragile to me as my understanding is that
PCI does not necessarily guarantees ordering of IO TLPs. But I may be wrong here.

> If ordering between the accesses is required, you can add
> dma_rmb() and dma_wmb() barriers.

Which I guess is the one thing we need after testing the reg command field in
pci_epf_test_cmd_handler() and before making the local copy, to avoid problems
with ordering of the reg fields writes from the host.

Will use that in v2.


-- 
Damien Le Moal
Western Digital Research