[PATCH] misc: pci_endpoint_test: Add WARN_ON() to detect broken EPC drivers

[PATCH] misc: pci_endpoint_test: Add WARN_ON() to detect broken EPC drivers

[Niklas Cassel]

Getting two IRQs when we are only expecting a single IRQ is a very
serious error, and suggests that something is very wrong in the EPC
driver.

Scream loudly so that EPC driver developers can see the problem,
which might otherwise be very hard to detect.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/misc/pci_endpoint_test.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 3635741c3e7a..a212e53654c6 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -178,6 +178,19 @@ static irqreturn_t pci_endpoint_test_irqhandler(int irq, void *dev_id)
 	if (reg & STATUS_IRQ_RAISED) {
 		test->last_irq = irq;
 		complete(&test->irq_raised);
+		/*
+		 * The endpoint test driver performs all testing sequentially.
+		 * This means that test->irq_raised.done should never exceed 1.
+		 * If it does, then we received two IRQs in a row, without a
+		 * successful wait_for_completion_timeout() call in between.
+		 *
+		 * While complete() increases test->irq_raised.done by one,
+		 * wait_for_completion_timeout() reduces test->irq_raised.done
+		 * by one on success.
+		 *
+		 * Please debug your EPC driver if you see this warning.
+		 */
+		WARN_ON(test->irq_raised.done > 1);
 	}
 
 	return IRQ_HANDLED;
-- 
2.55.0

Re: [PATCH] misc: pci_endpoint_test: Add WARN_ON() to detect broken EPC drivers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] The newly added `WARN_ON` can be trivially triggered by shared interrupts or untrusted hardware, leading to a Denial of Service (host panic) if `panic_on_warn` is enabled.
- [Medium] Direct, lockless read of the internal `done` field in `struct completion` causes a data race, violates API encapsulation, and relies on a flawed TOCTOU heuristic.

Pre-existing issues:
- [High] The shared IRQ handler unconditionally returns `IRQ_HANDLED` even when the interrupt was not generated by this device.
- [High] Use-after-free of `struct pci_endpoint_test` when the PCI device is unbound while a file descriptor is still open.
--

--- Patch [1]: [PATCH] misc: pci_endpoint_test: Add WARN_ON() to detect broken EPC drivers ---
commit e4f6d6484e68c6a478412d97f6734cf8590a6f97
Author: Niklas Cassel <cassel@kernel.org>

misc: pci_endpoint_test: Add WARN_ON() to detect broken EPC drivers

This commit adds a WARN_ON check to the IRQ handler in the PCI endpoint
test driver. It aims to detect if the driver receives multiple interrupts
sequentially without a successful wait in between.

> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 3635741c3e7a7..a212e53654c68 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -178,6 +178,19 @@ static irqreturn_t pci_endpoint_test_irqhandler(int irq, void *dev_id)
>  	if (reg & STATUS_IRQ_RAISED) {
>  		test->last_irq = irq;
>  		complete(&test->irq_raised);
> +		/*
> +		 * The endpoint test driver performs all testing sequentially.
> +		 * This means that test->irq_raised.done should never exceed 1.
> +		 * If it does, then we received two IRQs in a row, without a
> +		 * successful wait_for_completion_timeout() call in between.
> +		 *
> +		 * While complete() increases test->irq_raised.done by one,
> +		 * wait_for_completion_timeout() reduces test->irq_raised.done
> +		 * by one on success.
> +		 *
> +		 * Please debug your EPC driver if you see this warning.
> +		 */
> +		WARN_ON(test->irq_raised.done > 1);

[Severity: Medium]
Does this direct, lockless read of the completion structure's internal state
introduce a data race?

If wait_for_completion_timeout() executes concurrently on another CPU between
the complete() call and this check, it could decrement done back to 0. Would
this window result in missing the duplicate IRQ this is attempting to detect?

[Severity: High]
Can this warning be trivially triggered by shared interrupts or untrusted
hardware?

If the driver requests its IRQ with IRQF_SHARED, an interrupt from another
device could cause this handler to execute. If it reads a stale
STATUS_IRQ_RAISED bit, could repeated shared interrupts trigger this WARN_ON
and potentially panic the host if panic_on_warn is enabled?

>  	}
>  
>  	return IRQ_HANDLED;

[Severity: High]
This isn't a bug introduced by this patch, but does this function
unconditionally return IRQ_HANDLED even when the interrupt wasn't generated
by this device?

If the status register does not have STATUS_IRQ_RAISED set, bypassing the
internal block, returning IRQ_HANDLED instead of IRQ_NONE could mask
unhandled interrupts from other devices sharing the same IRQ line.

[ ... ]

[Severity: High]
This is a pre-existing issue, but does using devm_kzalloc() for the state
structure expose a use-after-free regression during device unbind?

If the PCI device is unbound, devm_kzalloc() frees the struct
pci_endpoint_test upon return from pci_endpoint_test_remove(). However, if
an application still holds the device node open, could a subsequent
ioctl() access the freed pointer when resolving the private data?

drivers/misc/pci_endpoint_test.c:pci_endpoint_test_ioctl() {
	...
	struct pci_endpoint_test *test = to_endpoint_test(file->private_data);
	struct pci_dev *pdev = test->pdev;
	
	mutex_lock(&test->mutex);
	...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729202821.1153443-2-cassel@kernel.org?part=1

Re: [PATCH] misc: pci_endpoint_test: Add WARN_ON() to detect broken EPC drivers

[Niklas Cassel]

On Wed, Jul 29, 2026 at 08:38:49PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> 
> New issues:
> - [High] The newly added `WARN_ON` can be trivially triggered by shared interrupts or untrusted hardware, leading to a Denial of Service (host panic) if `panic_on_warn` is enabled.

Don't think this something we should care about.
This is a test driver, and if someone plugs in a PCIe endpoint on the PCIe bus
that runs the PCIe endpoint framework, and the pci-epf-test driver, they
obviously know what they are doing, and want good/important warnings.

> - [Medium] Direct, lockless read of the internal `done` field in `struct completion` causes a data race, violates API encapsulation, and relies on a flawed TOCTOU heuristic.

Will user proper API in V2.


Kind regards,
Niklas

Re: [PATCH] misc: pci_endpoint_test: Add WARN_ON() to detect broken EPC drivers

[Niklas Cassel]

On Thu, Jul 30, 2026 at 02:02:11PM +0200, Niklas Cassel wrote:
> On Wed, Jul 29, 2026 at 08:38:49PM +0000, sashiko-bot@kernel.org wrote:
> > - [Medium] Direct, lockless read of the internal `done` field in `struct completion` causes a data race, violates API encapsulation, and relies on a flawed TOCTOU heuristic.
> 
> Will user proper API in V2.

Seems like the proper API is to use completion_done().

But we cannot use this API, as it tries to take the
completion spin lock... so it does not work.

Will just drop this patch for now.


Kind regards,
Niklas

DWC eDMA weirdness

[Niklas Cassel]

On Wed, Jul 29, 2026 at 10:28:22PM +0200, Niklas Cassel wrote:
> Getting two IRQs when we are only expecting a single IRQ is a very
> serious error, and suggests that something is very wrong in the EPC
> driver.
> 
> Scream loudly so that EPC driver developers can see the problem,
> which might otherwise be very hard to detect.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/misc/pci_endpoint_test.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 3635741c3e7a..a212e53654c6 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -178,6 +178,19 @@ static irqreturn_t pci_endpoint_test_irqhandler(int irq, void *dev_id)
>  	if (reg & STATUS_IRQ_RAISED) {
>  		test->last_irq = irq;
>  		complete(&test->irq_raised);
> +		/*
> +		 * The endpoint test driver performs all testing sequentially.
> +		 * This means that test->irq_raised.done should never exceed 1.
> +		 * If it does, then we received two IRQs in a row, without a
> +		 * successful wait_for_completion_timeout() call in between.
> +		 *
> +		 * While complete() increases test->irq_raised.done by one,
> +		 * wait_for_completion_timeout() reduces test->irq_raised.done
> +		 * by one on success.
> +		 *
> +		 * Please debug your EPC driver if you see this warning.
> +		 */
> +		WARN_ON(test->irq_raised.done > 1);
>  	}
>  
>  	return IRQ_HANDLED;
> -- 
> 2.55.0
> 

Hello Koichiro,

I created this patch in response to a problem that I saw on v7.2-rc5.

When running the pci_endpoint selftest:
# /pcitest
(snip)
#  RUN           pcie_ep_doorbell.DOORBELL_TEST ...
[  215.256772] pci-endpoint-test 0000:01:00.0: Failed to trigger doorbell in endpoint
#            OK  pcie_ep_doorbell.DOORBELL_TEST
ok 23 pcie_ep_doorbell.DOORBELL_TEST
# FAILED: 19 / 23 tests passed.
# 2 skipped test(s) detected. Consider enabling relevant config options to improve coverage.
# Totals: pass:17 fail:4 xfail:0 xpass:0 skip:2 error:0


I saw the weird error:
"Failed to trigger doorbell in endpoint"

Ignore that the test case leaves things to be desired, since it passes even
though we see this warning.


The problem seems to be that the embedded doorbell IRQ handler is executed twice,
in response to a single writel() which rings the doorbell.

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 4802d4f80f78..7f4360579ffb 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -714,6 +714,7 @@ static irqreturn_t pci_epf_test_doorbell_handler(int irq, void *data)
 
        status |= STATUS_DOORBELL_SUCCESS;
        reg->status = cpu_to_le32(status);
+       trace_printk("calling pci_epf_test_raise_irq()\n");
        pci_epf_test_raise_irq(epf_test, reg);
 
        return IRQ_HANDLED;


As you can see:
 irq/58-pci-ep-t-221     [006] .....    58.601020: pci_epf_test_doorbell_handler: calling pci_epf_test_raise_irq()
 irq/58-pci-ep-t-221     [006] .....    58.601421: pci_epf_test_doorbell_handler: calling pci_epf_test_raise_irq()

It is called twice for a single doorbell ring.

Note that you might need to run the whole selftest test a few times to trigger
the WARN_ON(), even if you can see the trace printks every time.
(Using printks instead of trace printks seems to affect the timing too much.)


My first thought was that we should add reinit_completion() calls after each
wait_for_completion_timeout() in pci_endpoint_test_doorbell(), but after
reading up on complete() and wait_for_completion_timeout(), this should
actually not be needed, and if we did, it would simply hide the read problem,
which appears to be that the embedded doorbell IRQ handler on the EP side is
called twice for a single writel() that rings the doorbell.


Kind regards,
Niklas

Re: DWC eDMA weirdness

[Manivannan Sadhasivam]

On Wed, Jul 29, 2026 at 10:49:56PM +0200, Niklas Cassel wrote:
> On Wed, Jul 29, 2026 at 10:28:22PM +0200, Niklas Cassel wrote:
> > Getting two IRQs when we are only expecting a single IRQ is a very
> > serious error, and suggests that something is very wrong in the EPC
> > driver.
> > 
> > Scream loudly so that EPC driver developers can see the problem,
> > which might otherwise be very hard to detect.
> > 
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > ---
> >  drivers/misc/pci_endpoint_test.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> > index 3635741c3e7a..a212e53654c6 100644
> > --- a/drivers/misc/pci_endpoint_test.c
> > +++ b/drivers/misc/pci_endpoint_test.c
> > @@ -178,6 +178,19 @@ static irqreturn_t pci_endpoint_test_irqhandler(int irq, void *dev_id)
> >  	if (reg & STATUS_IRQ_RAISED) {
> >  		test->last_irq = irq;
> >  		complete(&test->irq_raised);
> > +		/*
> > +		 * The endpoint test driver performs all testing sequentially.
> > +		 * This means that test->irq_raised.done should never exceed 1.
> > +		 * If it does, then we received two IRQs in a row, without a
> > +		 * successful wait_for_completion_timeout() call in between.
> > +		 *
> > +		 * While complete() increases test->irq_raised.done by one,
> > +		 * wait_for_completion_timeout() reduces test->irq_raised.done
> > +		 * by one on success.
> > +		 *
> > +		 * Please debug your EPC driver if you see this warning.
> > +		 */
> > +		WARN_ON(test->irq_raised.done > 1);
> >  	}
> >  
> >  	return IRQ_HANDLED;
> > -- 
> > 2.55.0
> > 
> 
> Hello Koichiro,
> 
> I created this patch in response to a problem that I saw on v7.2-rc5.
> 
> When running the pci_endpoint selftest:
> # /pcitest
> (snip)
> #  RUN           pcie_ep_doorbell.DOORBELL_TEST ...
> [  215.256772] pci-endpoint-test 0000:01:00.0: Failed to trigger doorbell in endpoint
> #            OK  pcie_ep_doorbell.DOORBELL_TEST
> ok 23 pcie_ep_doorbell.DOORBELL_TEST
> # FAILED: 19 / 23 tests passed.
> # 2 skipped test(s) detected. Consider enabling relevant config options to improve coverage.
> # Totals: pass:17 fail:4 xfail:0 xpass:0 skip:2 error:0
> 
> 
> I saw the weird error:
> "Failed to trigger doorbell in endpoint"
> 
> Ignore that the test case leaves things to be desired, since it passes even
> though we see this warning.
> 

How come this is possible? Doorbell timeout occured, but still Doorbell got
triggered in EP?

> 
> The problem seems to be that the embedded doorbell IRQ handler is executed twice,
> in response to a single writel() which rings the doorbell.
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 4802d4f80f78..7f4360579ffb 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -714,6 +714,7 @@ static irqreturn_t pci_epf_test_doorbell_handler(int irq, void *data)
>  
>         status |= STATUS_DOORBELL_SUCCESS;
>         reg->status = cpu_to_le32(status);
> +       trace_printk("calling pci_epf_test_raise_irq()\n");
>         pci_epf_test_raise_irq(epf_test, reg);
>  
>         return IRQ_HANDLED;
> 
> 
> As you can see:
>  irq/58-pci-ep-t-221     [006] .....    58.601020: pci_epf_test_doorbell_handler: calling pci_epf_test_raise_irq()
>  irq/58-pci-ep-t-221     [006] .....    58.601421: pci_epf_test_doorbell_handler: calling pci_epf_test_raise_irq()
> 
> It is called twice for a single doorbell ring.
> 

Which doorbell is used here? Platform MSI or Embedded one?

- Mani

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

Re: DWC eDMA weirdness

[Niklas Cassel]

On Thu, Jul 30, 2026 at 06:32:08AM +0200, Manivannan Sadhasivam wrote:
> > 
> > Ignore that the test case leaves things to be desired, since it passes even
> > though we see this warning.
> > 
> 
> How come this is possible? Doorbell timeout occured, but still Doorbell got
> triggered in EP?

The doorbell is triggered. It is just that the if statement:

if (!left || (status & STATUS_DOORBELL_ENABLE_FAIL)) {

is really bad...

A better way would be to check

if (!left || !(status & STATUS_DOORBELL_ENABLE_SUCCESS)) {


The problem seems to be that we re-read status, and the second time we
read the status, the STATUS_DOORBELL_SUCCESS bit is set.

I can send two patches for this.


> 
> > 
> > The problem seems to be that the embedded doorbell IRQ handler is executed twice,
> > in response to a single writel() which rings the doorbell.
> > 
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index 4802d4f80f78..7f4360579ffb 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -714,6 +714,7 @@ static irqreturn_t pci_epf_test_doorbell_handler(int irq, void *data)
> >  
> >         status |= STATUS_DOORBELL_SUCCESS;
> >         reg->status = cpu_to_le32(status);
> > +       trace_printk("calling pci_epf_test_raise_irq()\n");
> >         pci_epf_test_raise_irq(epf_test, reg);
> >  
> >         return IRQ_HANDLED;
> > 
> > 
> > As you can see:
> >  irq/58-pci-ep-t-221     [006] .....    58.601020: pci_epf_test_doorbell_handler: calling pci_epf_test_raise_irq()
> >  irq/58-pci-ep-t-221     [006] .....    58.601421: pci_epf_test_doorbell_handler: calling pci_epf_test_raise_irq()
> > 
> > It is called twice for a single doorbell ring.
> > 
> 
> Which doorbell is used here? Platform MSI or Embedded one?

Embedded.

I now see what is going on:

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 3635741c3e7a..b554f7c38ecc 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -176,8 +176,10 @@ static irqreturn_t pci_endpoint_test_irqhandler(int irq, void *dev_id)

        reg = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
        if (reg & STATUS_IRQ_RAISED) {
+               unsigned int old = READ_ONCE(test->irq_raised.done);
                test->last_irq = irq;
                complete(&test->irq_raised);
+               trace_printk("old: %u new: %u\n", old, READ_ONCE(test->irq_raised.done));
        }

        return IRQ_HANDLED;
@@ -1076,6 +1078,8 @@ static int pci_endpoint_test_doorbell(struct pci_endpoint_test *test)
        u32 addr;
        int left;

+       trace_printk("enter\n");
+
        if (!(test->ep_caps & CAP_DYNAMIC_INBOUND_MAPPING))
                return -EOPNOTSUPP;

@@ -1093,11 +1097,14 @@ static int pci_endpoint_test_doorbell(struct pci_endpoint_test *test)
        left = wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000));

        status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
+       trace_printk("after ENABLE DOORBELL wait, status: %u left: %d\n", status, left);
        if (!left || (status & STATUS_DOORBELL_ENABLE_FAIL)) {
                dev_err(dev, "Failed to enable doorbell\n");
                return -EINVAL;
        }

+       msleep(500);
+
        data = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_DATA);
        addr = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_OFFSET);

@@ -1113,12 +1120,13 @@ static int pci_endpoint_test_doorbell(struct pci_endpoint_test *test)
                return -ERANGE;
        }

+       trace_printk("ringing doorbell\n");
        writel(data, test->bar[bar] + addr);

        left = wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000));

        status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
-
+       trace_printk("after RING DOORBELL wait, status: %u left: %d\n", status, left);
        if (!left || !(status & STATUS_DOORBELL_SUCCESS))
                dev_err(dev, "Failed to trigger doorbell in endpoint\n");



As you can see, even adding a msleep(500) between the
ENABLE_DOORBELL call

and the RING DOORBELL writel(), we still get two IRQs.

         pcitest-307     [002] .....    65.050035: pci_endpoint_test_ioctl: enter
          <idle>-0       [007] d.h1.    65.057615: pci_endpoint_test_irqhandler: old: 0 new: 1
          <idle>-0       [007] d.h1.    65.057626: pci_endpoint_test_irqhandler: old: 1 new: 2
         pcitest-307     [006] .....    65.057734: pci_endpoint_test_ioctl: after ENABLE DOORBELL wait, status: 1088 left: 248
         pcitest-307     [006] .....    65.585419: pci_endpoint_test_ioctl: ringing doorbell
         pcitest-307     [006] .....    65.585423: pci_endpoint_test_ioctl: after RING DOORBELL wait, status: 0 left: 250
          <idle>-0       [007] d.h1.    65.585682: pci_endpoint_test_irqhandler: old: 0 new: 1
          <idle>-0       [007] d.h1.    65.591753: pci_endpoint_test_irqhandler: old: 0 new: 1


As you can see, status is 1088, so bit 10 is set (STATUS_DOORBELL_ENABLE_SUCCESS).

And since this is before even writing the doorbell, what seems to happen is that
we get an IRQ when calling request_irq().

So it looks like we have uncleared hardware status flags, or possibly incorrect
edge/level trigger configurations.


Kind regards,
Niklas

Re: DWC eDMA weirdness

[Koichiro Den]

On Wed, Jul 29, 2026 at 10:49:56PM +0200, Niklas Cassel wrote:
> On Wed, Jul 29, 2026 at 10:28:22PM +0200, Niklas Cassel wrote:
> > Getting two IRQs when we are only expecting a single IRQ is a very
> > serious error, and suggests that something is very wrong in the EPC
> > driver.
> > 
> > Scream loudly so that EPC driver developers can see the problem,
> > which might otherwise be very hard to detect.
> > 
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > ---
> >  drivers/misc/pci_endpoint_test.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> > index 3635741c3e7a..a212e53654c6 100644
> > --- a/drivers/misc/pci_endpoint_test.c
> > +++ b/drivers/misc/pci_endpoint_test.c
> > @@ -178,6 +178,19 @@ static irqreturn_t pci_endpoint_test_irqhandler(int irq, void *dev_id)
> >  	if (reg & STATUS_IRQ_RAISED) {
> >  		test->last_irq = irq;
> >  		complete(&test->irq_raised);
> > +		/*
> > +		 * The endpoint test driver performs all testing sequentially.
> > +		 * This means that test->irq_raised.done should never exceed 1.
> > +		 * If it does, then we received two IRQs in a row, without a
> > +		 * successful wait_for_completion_timeout() call in between.
> > +		 *
> > +		 * While complete() increases test->irq_raised.done by one,
> > +		 * wait_for_completion_timeout() reduces test->irq_raised.done
> > +		 * by one on success.
> > +		 *
> > +		 * Please debug your EPC driver if you see this warning.
> > +		 */
> > +		WARN_ON(test->irq_raised.done > 1);
> >  	}
> >  
> >  	return IRQ_HANDLED;
> > -- 
> > 2.55.0
> > 
> 
> Hello Koichiro,
> 
> I created this patch in response to a problem that I saw on v7.2-rc5.
> 
> When running the pci_endpoint selftest:
> # /pcitest
> (snip)
> #  RUN           pcie_ep_doorbell.DOORBELL_TEST ...
> [  215.256772] pci-endpoint-test 0000:01:00.0: Failed to trigger doorbell in endpoint
> #            OK  pcie_ep_doorbell.DOORBELL_TEST
> ok 23 pcie_ep_doorbell.DOORBELL_TEST
> # FAILED: 19 / 23 tests passed.
> # 2 skipped test(s) detected. Consider enabling relevant config options to improve coverage.
> # Totals: pass:17 fail:4 xfail:0 xpass:0 skip:2 error:0
> 
> 
> I saw the weird error:
> "Failed to trigger doorbell in endpoint"
> 
> Ignore that the test case leaves things to be desired, since it passes even
> though we see this warning.
> 
> 
> The problem seems to be that the embedded doorbell IRQ handler is executed twice,
> in response to a single writel() which rings the doorbell.
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 4802d4f80f78..7f4360579ffb 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -714,6 +714,7 @@ static irqreturn_t pci_epf_test_doorbell_handler(int irq, void *data)
>  
>         status |= STATUS_DOORBELL_SUCCESS;
>         reg->status = cpu_to_le32(status);
> +       trace_printk("calling pci_epf_test_raise_irq()\n");
>         pci_epf_test_raise_irq(epf_test, reg);
>  
>         return IRQ_HANDLED;
> 
> 
> As you can see:
>  irq/58-pci-ep-t-221     [006] .....    58.601020: pci_epf_test_doorbell_handler: calling pci_epf_test_raise_irq()
>  irq/58-pci-ep-t-221     [006] .....    58.601421: pci_epf_test_doorbell_handler: calling pci_epf_test_raise_irq()
> 
> It is called twice for a single doorbell ring.
> 
> Note that you might need to run the whole selftest test a few times to trigger
> the WARN_ON(), even if you can see the trace printks every time.
> (Using printks instead of trace printks seems to affect the timing too much.)
> 
> 
> My first thought was that we should add reinit_completion() calls after each
> wait_for_completion_timeout() in pci_endpoint_test_doorbell(), but after
> reading up on complete() and wait_for_completion_timeout(), this should
> actually not be needed, and if we did, it would simply hide the read problem,
> which appears to be that the embedded doorbell IRQ handler on the EP side is
> called twice for a single writel() that rings the doorbell.

Hi Niklas,

I was struggling to reproduce this, but I think I figured out what triggers it.
It happens when the 'dma' variant runs before DOORBELL_TEST, e.g. when running
the full test suite with no arguments, or by running '-v dma' first.

I think the following patch fixes it:

diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
index 89a4c498a17b..ec097c68cf5f 100644
--- a/drivers/dma/dw-edma/dw-edma-core.c
+++ b/drivers/dma/dw-edma/dw-edma-core.c
@@ -755,6 +755,7 @@ static int dw_edma_emul_irq_alloc(struct dw_edma *dw)
                return virq;

        irq_set_chip_and_handler(virq, &dw_edma_emul_irqchip, handle_level_irq);
+       irq_set_status_flags(virq, IRQ_LEVEL);
        irq_set_chip_data(virq, dw);
        irq_set_noprobe(virq);

Could you test this?

Best regards,
Koichiro

> 
> 
> Kind regards,
> Niklas

Re: DWC eDMA weirdness

[Niklas Cassel]

On Thu, Jul 30, 2026 at 05:39:46PM +0900, Koichiro Den wrote:
> 
> I was struggling to reproduce this, but I think I figured out what triggers it.
> It happens when the 'dma' variant runs before DOORBELL_TEST, e.g. when running
> the full test suite with no arguments, or by running '-v dma' first.
> 
> I think the following patch fixes it:
> 
> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 89a4c498a17b..ec097c68cf5f 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -755,6 +755,7 @@ static int dw_edma_emul_irq_alloc(struct dw_edma *dw)
>                 return virq;
> 
>         irq_set_chip_and_handler(virq, &dw_edma_emul_irqchip, handle_level_irq);
> +       irq_set_status_flags(virq, IRQ_LEVEL);
>         irq_set_chip_data(virq, dw);
>         irq_set_noprobe(virq);
> 
> Could you test this?

That appears to fix my problem.

We have had a ton of DWC problems related to IRQs.
IIRC, most DWC IRQs, e.g. the embedded MSI controller on the PCIe controller
itself, works using level triggered IRQs.

However, e.g. MSIs themselves are by definition edge triggered, so there is
a difference of how you mark the IRQ handler in the DWC driver, and how you
configure the IRQ domain that you configure to the MSI/IRQ kernel code:
https://patchew.org/linux/20250205151635.v2.1.Id60295bee6aacf44aa3664e702012cb4710529c3@changeid/


Since this is for an IRQ within the PCIe controller, I think this is correct.



We've also seen problems where we've been clearing the level IRQ too
late, see e.g.
ca1658921b63 ("PCI: designware: Fix missing MSI IRQs")

which caused us to miss IRQs when two MSIs came very close after one another.



Kind regards,
Niklas

Re: DWC eDMA weirdness

[Koichiro Den]

On Thu, Jul 30, 2026 at 12:43:20PM +0200, Niklas Cassel wrote:
> On Thu, Jul 30, 2026 at 05:39:46PM +0900, Koichiro Den wrote:
> > 
> > I was struggling to reproduce this, but I think I figured out what triggers it.
> > It happens when the 'dma' variant runs before DOORBELL_TEST, e.g. when running
> > the full test suite with no arguments, or by running '-v dma' first.
> > 
> > I think the following patch fixes it:
> > 
> > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> > index 89a4c498a17b..ec097c68cf5f 100644
> > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> > @@ -755,6 +755,7 @@ static int dw_edma_emul_irq_alloc(struct dw_edma *dw)
> >                 return virq;
> > 
> >         irq_set_chip_and_handler(virq, &dw_edma_emul_irqchip, handle_level_irq);
> > +       irq_set_status_flags(virq, IRQ_LEVEL);
> >         irq_set_chip_data(virq, dw);
> >         irq_set_noprobe(virq);
> > 
> > Could you test this?
> 
> That appears to fix my problem.

Thanks for confirming. I'll send a fix for this. Sorry for the trouble.

Best regards,
Koichiro

> 
> We have had a ton of DWC problems related to IRQs.
> IIRC, most DWC IRQs, e.g. the embedded MSI controller on the PCIe controller
> itself, works using level triggered IRQs.
> 
> However, e.g. MSIs themselves are by definition edge triggered, so there is
> a difference of how you mark the IRQ handler in the DWC driver, and how you
> configure the IRQ domain that you configure to the MSI/IRQ kernel code:
> https://patchew.org/linux/20250205151635.v2.1.Id60295bee6aacf44aa3664e702012cb4710529c3@changeid/
> 
> 
> Since this is for an IRQ within the PCIe controller, I think this is correct.
> 
> 
> 
> We've also seen problems where we've been clearing the level IRQ too
> late, see e.g.
> ca1658921b63 ("PCI: designware: Fix missing MSI IRQs")
> 
> which caused us to miss IRQs when two MSIs came very close after one another.
> 
> 
> 
> Kind regards,
> Niklas

Re: DWC eDMA weirdness

[Niklas Cassel]

On 30 July 2026 17:17:48 CEST, Koichiro Den <den@valinux.co.jp> wrote:
>On Thu, Jul 30, 2026 at 12:43:20PM +0200, Niklas Cassel wrote:
>> On Thu, Jul 30, 2026 at 05:39:46PM +0900, Koichiro Den wrote:
>> > 
>> > I was struggling to reproduce this, but I think I figured out what triggers it.
>> > It happens when the 'dma' variant runs before DOORBELL_TEST, e.g. when running
>> > the full test suite with no arguments, or by running '-v dma' first.
>> > 
>> > I think the following patch fixes it:
>> > 
>> > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
>> > index 89a4c498a17b..ec097c68cf5f 100644
>> > --- a/drivers/dma/dw-edma/dw-edma-core.c
>> > +++ b/drivers/dma/dw-edma/dw-edma-core.c
>> > @@ -755,6 +755,7 @@ static int dw_edma_emul_irq_alloc(struct dw_edma *dw)
>> >                 return virq;
>> > 
>> >         irq_set_chip_and_handler(virq, &dw_edma_emul_irqchip, handle_level_irq);
>> > +       irq_set_status_flags(virq, IRQ_LEVEL);
>> >         irq_set_chip_data(virq, dw);
>> >         irq_set_noprobe(virq);
>> > 
>> > Could you test this?
>> 
>> That appears to fix my problem.
>
>Thanks for confirming. I'll send a fix for this. Sorry for the trouble.
>

Hello Koichiro,


I am just happy that we found this :)

Bad things can happen when IRQs are incorrectly marked as egde instead of level, and vice versa.

Hopefully your existing work on improving the eDMA driver will be more stable as a result.


Kind regards,
Niklas