RE: PCIe EPF

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>]

Hi Prabhakar-san,

> From: Lad, Prabhakar, Sent: Wednesday, April 1, 2020 5:00 PM
<snip>
> > > root@hihope-rzg2m:~# pcitest -r -s 2176
> > <snip>
> > > [  528.556991] pci-endpoint-test 0000:01:00.0: pci_endpoint_test_read
> > > ffff0004b61f9000 7e951000 910c690d=910c690d
> >
> > I'd like to know how to print these crc values. Your report on the case 1
> > mentioned on pci-epf-test.c like below though.
> >
> > > +       dev_err(dev, "%s %llx %llx csum: %x = %x\n", __func__, reg->dst_addr,
> > > +               phys_addr, reg->checksum, crc32_le(~0, dst_addr, reg->size));
> >
> > Also, as I mentioned on previous email, this is possible to cause timing issue.
> > So, I'd like to where you added the hexdump on pci_endpoint_test.c.
> > Perhaps, copy and pasting your whole debug diff is useful to understand about it.
> >
> 
> Following is the complete diff:

Thanks!

> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 3c49514b4813..e7bf58a1fee8 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -561,6 +561,23 @@ static bool pci_endpoint_test_write(struct
> pci_endpoint_test *test,
>      return ret;
>  }
> 
> +static void print_buffer(struct device *dev, void *buff_addr, size_t size)
> +{
> +    size_t i;
> +    u64 *addr = buff_addr;
> +
> +    for(i = 0; i < size; i += sizeof(addr))
> +        dev_err(dev, "buf[%zu] : %llx\n", i, *addr);
> +
> +    for(i = 0; i < size; i +=1) {
> +        if (0x910c690d == crc32_le(~0, buff_addr, i))
> +            dev_err(dev, "%x\n",
> +                crc32_le(~0, buff_addr, i));
> +    }
> +
> +    dev_err(dev, "\n\n\n\n");
> +}
> +
>  static bool pci_endpoint_test_read(struct pci_endpoint_test *test,
>                     unsigned long arg)
>  {
> @@ -608,7 +625,7 @@ static bool pci_endpoint_test_read(struct
> pci_endpoint_test *test,
>      }

I'd like to allocate temporary buffer here to copy data later...
	void *tmp;
	...
	tmp = kzalloc(size + alignment, GFP_KERNEL);

>      orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
> -                    DMA_FROM_DEVICE);
> +                    DMA_BIDIRECTIONAL);
>      if (dma_mapping_error(dev, orig_phys_addr)) {
>          dev_err(dev, "failed to map source buffer address\n");
>          ret = false;
> @@ -640,12 +657,17 @@ static bool pci_endpoint_test_read(struct
> pci_endpoint_test *test,
>      wait_for_completion(&test->irq_raised);
> 
>      dma_unmap_single(dev, orig_phys_addr, size + alignment,
> -             DMA_FROM_DEVICE);
> +             DMA_BIDIRECTIONAL);

And then, I'd like to copy addr buffer to the tmp here.
	memcpy(tmp, addr, size);

>      crc32 = crc32_le(~0, addr, size);

And addr is replaced with tmp;

>      if (crc32 == pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM))
>          ret = true;
> 
> +    print_buffer(dev, addr, size);

This addr is also replaced with tmp;

> +    dev_err(dev, "%s %px %llx %x=%x\n", __func__, orig_addr,
> +        orig_phys_addr, crc32,
> +        pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM));

The crc32 value was calculated before print the buffer.
This means that timing is difference between crc32_le(addr) and print_buffer(addr).
So, I'd like to copy the addr buffer to tmp and use the tmp for crc32_le() and print_buffer().

> +
>  err_phys_addr:
>      kfree(orig_addr);
>  err:
> 
> Note: I have added DMA_BIDIRECTIONAL that is because I am also
> printing the buffer on ep
> and calulating the crc
> 
> > > READ (   2176 bytes):           OKAY
> > > root@hihope-rzg2m:~# pcitest -r -s 2176
> > <snip>
> > > [  533.457921] pci-endpoint-test 0000:01:00.0: pci_endpoint_test_read
> > > ffff0004b61f9000 7e959800  ce535039=910c690d
> > > READ (   2176 bytes):           NOT OKAY
> > >
> > > Note: for failure case the crc is always ce535039
> >
> > The value of ce535039 is from pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM)?
> > If so, it's strange because all 0xdf values with 2176 bytes should be 910c690d by crc32_le().
> >
> The value ce535039 is the one which is calculated from the buffer and
> value 910c690d is the one
> which is passed from the BAR memory which is correct.

I got it. If my guess is correct, using the tmp buffer above can print
the buffer with wrong value(s).


By the way, your PCIe host environment (RZ/G2N) output the following log?

[    0.000000] software IO TLB: mapped [mem 0x7bfff000-0x7ffff000] (64MB)

If so, I guess this issue is related to this software IO TLB behavior.
IIUC, if we use coherent buffer or GPF_DMA buffer, the kernel will not
use software IO TLB for these buffers.

Best regards,
Yoshihiro Shimoda

> Cheers,
> --Prabhakar