Re: [PATCH] misc: pci_endpoint_test: Use memcpy_toio()/memcpy_fromio() for BAR tests

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Tue, Mar 19, 2024 at 05:40:33PM +0100, Niklas Cassel wrote:
> Hello Arnd,
> 
> On Mon, Mar 18, 2024 at 09:02:21PM +0100, Arnd Bergmann wrote:
> > On Mon, Mar 18, 2024, at 20:30, Niklas Cassel wrote:
> > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> > > index 705029ad8eb5..cb6c9ccf3a5f 100644
> > > --- a/drivers/misc/pci_endpoint_test.c
> > > +++ b/drivers/misc/pci_endpoint_test.c
> > > @@ -272,31 +272,59 @@ static const u32 bar_test_pattern[] = {
> > >  	0xA5A5A5A5,
> > >  };
> > > 
> > > +static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
> > > +					enum pci_barno barno, int offset,
> > > +					void *write_buf, void *read_buf,
> > > +					int size)
> > > +{
> > > +	memset(write_buf, bar_test_pattern[barno], size);
> > > +	memcpy_toio(test->bar[barno] + offset, write_buf, size);
> > > +
> > > +	/* Make sure that reads are performed after writes. */
> > > +	mb();
> > > +	memcpy_fromio(read_buf, test->bar[barno] + offset, size);
> > 
> > Did you see actual bugs without the barrier? On normal PCI
> > semantics, a read will always force a write to be flushed first.
> 
> I'm aware that a Read Request must not pass a Posted Request under
> normal PCI transaction ordering rules.
> (As defined in PCIe 6.0, Table 2-42 Ordering Rules Summary)
> 
> I was more worried about the compiler or CPU reordering things:
> https://github.com/torvalds/linux/blob/master/Documentation/memory-barriers.txt#L1876-L1878
> 
> I did try the patch without the barrier, and did not see any issues.
> 

There shouldn't be an issue without the barrier.

> 
> I did also see this comment:
> https://github.com/torvalds/linux/blob/master/Documentation/memory-barriers.txt#L2785-L2790
> 
> Do you think that we need to perform any flushing after the memset(),
> to ensure that the data written using memcpy_toio() is actually what
> we expect it to me?
> 

The documentation recommends cache flushing only if the normal memory write and
MMIO access are dependent. But here you are just accessing the MMIO. So no
explicit ordering or cache flushing is required.

- Mani

-- 
மணிவண்ணன் சதாசிவம்