Message-ID: <CAAEEuhoLM1zO3vZds58AM6X-roDkZcHzcq_GzrZ2jj_QcisKUw@mail.gmail.com>
Date: Tue, 20 Aug 2024 08:19:51 +0200
From: Rick Wertenbroek <rick.wertenbroek@...il.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: rick.wertenbroek@...g-vd.ch, dlemoal@...nel.org, 
	alberto.dassatti@...g-vd.ch, Krzysztof Wilczyński <kw@...ux.com>, 
	Kishon Vijay Abraham I <kishon@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	Niklas Cassel <cassel@...nel.org>, Frank Li <Frank.Li@....com>, 
	Lars-Peter Clausen <lars@...afoo.de>, linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] PCI: endpoint: pci-epf-test: Move DMA check into
 read/write/copy functions

On Tue, Aug 20, 2024 at 7:52 AM Manivannan Sadhasivam
<manivannan.sadhasivam@...aro.org> wrote:
>
> On Mon, Aug 19, 2024 at 02:01:10PM +0200, Rick Wertenbroek wrote:
> > The pci-epf-test PCI endpoint function /drivers/pci/endpoint/function/pci-epf_test.c
> > is meant to be used in a PCI endpoint device inside a host computer with
> > the host side driver: /drivers/misc/pci_endpoint_test.c.
> >
>
> s/inside/connected to

Thanks, I will substitute this.

>
> > The host side driver can request read/write/copy transactions from the
> > endpoint function and expects an IRQ from the endpoint function once
> > the read/write/copy transaction is finished. These can be issued with or
> > without DMA enabled. If the host side driver requests a read/write/copy
> > transaction with DMA enabled and the endpoint function does not support
> > DMA, the endpoint would only print an error message and wait for further
> > commands without sending an IRQ because pci_epf_test_raise_irq() is
> > skipped in pci_epf_test_cmd_handler(). This results in the host side
> > driver hanging indefinitely waiting for the IRQ.
> >
>
> TBH, it doesn't make sense to control the endpoint DMA from host. Host should
> just issue the transfer command, and let the endpoint use DMA or memcpy based on
> its capability.

No, because the test driver is meant to test the endpoint functions
(including the endpoint controller and its capabilities, so they test
if BARs can be read, if IRQs of each type can be sent, if transfers
work, etc.) so it is by design that it allows to ask for transfer with
or without DMA, this allows to test they both work. Also in real case
scenarios on DMA capable devices DMA will not always be used (e.g.,
small transfers where the overhead of DMA setup is bigger than just
doing memcpy_from/toio().

>
> > Move the DMA check into the pci_epf_test_read()/write()/copy() functions
> > so that they report a transfer (IO) error and that pci_epf_test_raise_irq()
> > is called when a transfer with DMA is requested, even if unsupported.
> >
> > The host side driver will no longer hang but report an error on transfer
> > (printing "NOT OKAY") thanks to the checksum because no data was moved.
> >
> > Signed-off-by: Rick Wertenbroek <rick.wertenbroek@...il.com>
> > ---
> >  drivers/pci/endpoint/functions/pci-epf-test.c | 29 +++++++++++++++----
> >  1 file changed, 23 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index 7c2ed6eae53a..ec0f79383521 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -314,6 +314,17 @@ static void pci_epf_test_print_rate(struct pci_epf_test *epf_test,
> >                (u64)ts.tv_sec, (u32)ts.tv_nsec, rate);
> >  }
> >
> > +static int pci_epf_test_check_dma(struct pci_epf_test *epf_test,
> > +                                struct pci_epf_test_reg *reg)
> > +{
> > +     if ((READ_ONCE(reg->flags) & FLAG_USE_DMA) &&
> > +         !epf_test->dma_supported) {
> > +             dev_err(&epf_test->epf->dev, "DMA transfer not supported\n");
> > +             return -EIO;
> > +     }
> > +     return 0;
> > +}
> > +
> >  static void pci_epf_test_copy(struct pci_epf_test *epf_test,
> >                             struct pci_epf_test_reg *reg)
> >  {
> > @@ -327,6 +338,10 @@ static void pci_epf_test_copy(struct pci_epf_test *epf_test,
> >       struct device *dev = &epf->dev;
> >       struct pci_epc *epc = epf->epc;
> >
> > +     ret = pci_epf_test_check_dma(epf_test, reg);
> > +     if (ret)
> > +             goto err;
> > +
> >       src_addr = pci_epc_mem_alloc_addr(epc, &src_phys_addr, reg->size);
> >       if (!src_addr) {
> >               dev_err(dev, "Failed to allocate source address\n");
> > @@ -423,6 +438,10 @@ static void pci_epf_test_read(struct pci_epf_test *epf_test,
> >       struct pci_epc *epc = epf->epc;
> >       struct device *dma_dev = epf->epc->dev.parent;
> >
> > +     ret = pci_epf_test_check_dma(epf_test, reg);
> > +     if (ret)
> > +             goto err;
> > +
> >       src_addr = pci_epc_mem_alloc_addr(epc, &phys_addr, reg->size);
> >       if (!src_addr) {
> >               dev_err(dev, "Failed to allocate address\n");
> > @@ -507,6 +526,10 @@ static void pci_epf_test_write(struct pci_epf_test *epf_test,
> >       struct pci_epc *epc = epf->epc;
> >       struct device *dma_dev = epf->epc->dev.parent;
> >
> > +     ret = pci_epf_test_check_dma(epf_test, reg);
> > +     if (ret)
> > +             goto err;
> > +
> >       dst_addr = pci_epc_mem_alloc_addr(epc, &phys_addr, reg->size);
> >       if (!dst_addr) {
> >               dev_err(dev, "Failed to allocate address\n");
> > @@ -647,12 +670,6 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
> >       WRITE_ONCE(reg->command, 0);
> >       WRITE_ONCE(reg->status, 0);
> >
> > -     if ((READ_ONCE(reg->flags) & FLAG_USE_DMA) &&
> > -         !epf_test->dma_supported) {
> > -             dev_err(dev, "Cannot transfer data using DMA\n");
> > -             goto reset_handler;
>
> Why can't you just set the status and trigger the IRQ here itself? This avoids
> duplicating the check inside each command handler.
>

You are right, that would avoid the duplicated code, I will do so for v3.

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

Thank you for your comments,
Best regards.
Rick

Powered by blists - more mailing lists