[PATCH v2] PCI: endpoint: pci-epf-test: Move DMA check into read/write/copy functions

[PATCH v2] PCI: endpoint: pci-epf-test: Move DMA check into read/write/copy functions

[Rick Wertenbroek]

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.

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.

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@gmail.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;
-	}
-
 	if (reg->irq_type > IRQ_TYPE_MSIX) {
 		dev_err(dev, "Failed to detect IRQ type\n");
 		goto reset_handler;
-- 
2.25.1

Re: [PATCH v2] PCI: endpoint: pci-epf-test: Move DMA check into read/write/copy functions

[Damien Le Moal]

On 8/19/24 21:01, 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.
> 
> 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.
> 
> 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@gmail.com>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] PCI: endpoint: pci-epf-test: Move DMA check into read/write/copy functions

[Manivannan Sadhasivam]

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

> 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.

> 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@gmail.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.

- Mani

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

Re: [PATCH v2] PCI: endpoint: pci-epf-test: Move DMA check into read/write/copy functions

[Rick Wertenbroek]

On Tue, Aug 20, 2024 at 7:52 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.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@gmail.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