| Message ID | 20240806162756.607002-1-rick.wertenbroek@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: endpoint: pci-epf-test: Move DMA check into read/write/copy functions | expand |
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.
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;
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
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
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
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;
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(-)