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

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

[Rick Wertenbroek]

The test for a DMA transfer was done in pci_epf_test_cmd_handler, which
if not supported would lead to the endpoint function just printing an
error message and waiting for further commands. This would leave the
host side PCI driver waiting for an interrupt because the call to
pci_epf_test_raise_irq is skipped. The host side driver
drivers/misc/pci_endpoint_test.c would hang indefinitely when sending
a transfer request with DMA if the endpoint does not support it.
This is because wait_for_completion() is used in the host side driver.

Move the DMA check into the read/write/copy functions so that they
report a transfer (IO) error so that pci_epf_test_raise_irq() is
called when a transfer with DMA is requested, even if unsupported.

The host side driver will still report an error on transfer thanks
to the checksum, because no data was moved, but will not hang anymore
waiting for an interrupt that will never arrive.

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..bd4b37f46f41 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, "Cannot transfer data using DMA\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] PCI: endpoint: pci-epf-test: Move DMA check into read/write/copy functions

[Bjorn Helgaas]

On Tue, Aug 06, 2024 at 06:27:54PM +0200, Rick Wertenbroek wrote:
> The test for a DMA transfer was done in pci_epf_test_cmd_handler, which
> if not supported would lead to the endpoint function just printing an
> error message and waiting for further commands. This would leave the

I guess it's the *test* that prints the error message?  Is this the
"Cannot transfer data using DMA" message?

> host side PCI driver waiting for an interrupt because the call to
> pci_epf_test_raise_irq is skipped. The host side driver
> drivers/misc/pci_endpoint_test.c would hang indefinitely when sending
> a transfer request with DMA if the endpoint does not support it.
> This is because wait_for_completion() is used in the host side driver.
> 
> Move the DMA check into the read/write/copy functions so that they
> report a transfer (IO) error so that pci_epf_test_raise_irq() is
> called when a transfer with DMA is requested, even if unsupported.

Add "()" after function names above, as you did for
pci_epf_test_raise_irq().

> The host side driver will still report an error on transfer thanks
> to the checksum, because no data was moved, but will not hang anymore
> waiting for an interrupt that will never arrive.

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

[Rick Wertenbroek]

On Tue, Aug 6, 2024 at 9:15 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Tue, Aug 06, 2024 at 06:27:54PM +0200, Rick Wertenbroek wrote:
> > The test for a DMA transfer was done in pci_epf_test_cmd_handler, which
> > if not supported would lead to the endpoint function just printing an
> > error message and waiting for further commands. This would leave the
>
> I guess it's the *test* that prints the error message?  Is this the
> "Cannot transfer data using DMA" message?

That is the error message, the error message is printed by the
endpoint function, on the endpoint device. On the host side, nothing
happens; the test program just hangs because the driver waits
indefinitely. With the change I proposed, the test program completes
the test and will display "NOT OKAY" as normal when a test fails.

>
> > host side PCI driver waiting for an interrupt because the call to
> > pci_epf_test_raise_irq is skipped. The host side driver
> > drivers/misc/pci_endpoint_test.c would hang indefinitely when sending
> > a transfer request with DMA if the endpoint does not support it.
> > This is because wait_for_completion() is used in the host side driver.
> >
> > Move the DMA check into the read/write/copy functions so that they
> > report a transfer (IO) error so that pci_epf_test_raise_irq() is
> > called when a transfer with DMA is requested, even if unsupported.
>
> Add "()" after function names above, as you did for
> pci_epf_test_raise_irq().

I will add this.

>
> > The host side driver will still report an error on transfer thanks
> > to the checksum, because no data was moved, but will not hang anymore
> > waiting for an interrupt that will never arrive.

Thanks.
Regards,
Rick

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

[Bjorn Helgaas]

On Fri, Aug 09, 2024 at 08:56:29AM +0200, Rick Wertenbroek wrote:
> On Tue, Aug 6, 2024 at 9:15 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Tue, Aug 06, 2024 at 06:27:54PM +0200, Rick Wertenbroek wrote:
> > > The test for a DMA transfer was done in pci_epf_test_cmd_handler, which
> > > if not supported would lead to the endpoint function just printing an
> > > error message and waiting for further commands. This would leave the
> >
> > I guess it's the *test* that prints the error message?  Is this the
> > "Cannot transfer data using DMA" message?
> 
> That is the error message, the error message is printed by the
> endpoint function, on the endpoint device. On the host side, nothing
> happens; the test program just hangs because the driver waits
> indefinitely. With the change I proposed, the test program completes
> the test and will display "NOT OKAY" as normal when a test fails.

Thanks for this; something like this would be helpful in the commit
log because it makes the two kernels involved more explicit.

Bjorn

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

[Damien Le Moal]

On 2024/08/06 9:27, Rick Wertenbroek wrote:
> The test for a DMA transfer was done in pci_epf_test_cmd_handler, which
> if not supported would lead to the endpoint function just printing an
> error message and waiting for further commands. This would leave the
> host side PCI driver waiting for an interrupt because the call to
> pci_epf_test_raise_irq is skipped. The host side driver
> drivers/misc/pci_endpoint_test.c would hang indefinitely when sending
> a transfer request with DMA if the endpoint does not support it.
> This is because wait_for_completion() is used in the host side driver.
> 
> Move the DMA check into the read/write/copy functions so that they
> report a transfer (IO) error so that pci_epf_test_raise_irq() is
> called when a transfer with DMA is requested, even if unsupported.
> 
> The host side driver will still report an error on transfer thanks
> to the checksum, because no data was moved, but will not hang anymore
> waiting for an interrupt that will never arrive.
> 
> 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..bd4b37f46f41 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, "Cannot transfer data using DMA\n");

While at it, can we change this message to be clear, e.g. "DMA not supported".
"Cannot ..." is vague and does not state the reason why it cannot be done :)

> +		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;

-- 
Damien Le Moal
Western Digital Research

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

[Rick Wertenbroek]

On Wed, Aug 7, 2024 at 6:08 PM Damien Le Moal <dlemoal@kernel.org> wrote:
>
>
> While at it, can we change this message to be clear, e.g. "DMA not supported".
> "Cannot ..." is vague and does not state the reason why it cannot be done :)
>

I can change this, it makes sense to make the message clearer, I'll
add this change to the v2.

Thanks.
Regards,
Rick