Hello Mani,
On Fri, Mar 22, 2024 at 03:50:58PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Mar 20, 2024 at 10:01:05AM +0100, Niklas Cassel wrote:
> > The current code uses writel()/readl(), which has an implicit memory
> > barrier for every single readl()/writel().
> >
> > Additionally, reading 4 bytes at a time over the PCI bus is not really
> > optimal, considering that this code is running in an ioctl handler.
> >
> > Use memcpy_toio()/memcpy_fromio() for BAR tests.
> >
> > Before patch with a 4MB BAR:
> > $ time /usr/bin/pcitest -b 1
> > BAR1: OKAY
> > real 0m 1.56s
> >
> > After patch with a 4MB BAR:
> > $ time /usr/bin/pcitest -b 1
> > BAR1: OKAY
> > real 0m 0.54s
> >
> > Signed-off-by: Niklas Cassel <cassel@xxxxxxxxxx>
> > ---
> > Changes since v2:
> > -Actually free the allocated memory... (thank you Kuppuswamy)
> >
> > drivers/misc/pci_endpoint_test.c | 68 ++++++++++++++++++++++++++------
> > 1 file changed, 55 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> > index 705029ad8eb5..1d361589fb61 100644
> > --- a/drivers/misc/pci_endpoint_test.c
> > +++ b/drivers/misc/pci_endpoint_test.c
> > @@ -272,33 +272,75 @@ 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);
> > +
> > + memcpy_fromio(read_buf, test->bar[barno] + offset, size);
> > +
> > + return memcmp(write_buf, read_buf, size);
> > +}
> > +
> > static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
> > enum pci_barno barno)
> > {
> > - int j;
> > - u32 val;
> > - int size;
> > + int j, bar_size, buf_size, iters, remain;
> > + void *write_buf;
> > + void *read_buf;
> > struct pci_dev *pdev = test->pdev;
> > + bool ret;
> >
> > if (!test->bar[barno])
> > return false;
> >
> > - size = pci_resource_len(pdev, barno);
> > + bar_size = pci_resource_len(pdev, barno);
> >
> > if (barno == test->test_reg_bar)
> > - size = 0x4;
> > + bar_size = 0x4;
> >
> > - for (j = 0; j < size; j += 4)
> > - pci_endpoint_test_bar_writel(test, barno, j,
> > - bar_test_pattern[barno]);
> > + buf_size = min(SZ_1M, bar_size);
> >
> > - for (j = 0; j < size; j += 4) {
> > - val = pci_endpoint_test_bar_readl(test, barno, j);
> > - if (val != bar_test_pattern[barno])
> > - return false;
> > + write_buf = kmalloc(buf_size, GFP_KERNEL);
> > + if (!write_buf)
> > + return false;
> > +
> > + read_buf = kmalloc(buf_size, GFP_KERNEL);
> > + if (!read_buf) {
> > + ret = false;
> > + goto err;
>
> This frees read_buf also. Please fix that and also rename the labels to:
>
> err_free_write_buf
> err_free_read_buf
This was intentional since kfree() handles NULL perfectly fine.
(I was thinking that it would just add extra lines for no good reason.)
Do you think that there is any point in having two labels in this case?
Kind regards,
Niklas
>
> - Mani
>
> > }
> >
> > - return true;
> > + iters = bar_size / buf_size;
> > + for (j = 0; j < iters; j++) {
> > + if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * j,
> > + write_buf, read_buf,
> > + buf_size)) {
> > + ret = false;
> > + goto err;
> > + }
> > + }
> > +
> > + remain = bar_size % buf_size;
> > + if (remain) {
> > + if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * iters,
> > + write_buf, read_buf,
> > + remain)) {
> > + ret = false;
> > + goto err;
> > + }
> > + }
> > +
> > + ret = true;
> > +
> > +err:
> > + kfree(write_buf);
> > + kfree(read_buf);
> > +
> > + return ret;
> > }
> >
> > static bool pci_endpoint_test_intx_irq(struct pci_endpoint_test *test)
> > --
> > 2.44.0
> >
>
> --
> மணிவண்ணன் சதாசிவம்
