[PATCH 0/4] PCI: endpoint: Doorbell-related fixes

[PATCH 0/4] PCI: endpoint: Doorbell-related fixes

[Koichiro Den]

Hi,

This is a small fix-only series related to the previous (v6)
doorbell-related series:
https://lore.kernel.org/linux-pci/20260209125316.2132589-1-den@valinux.co.jp/

These patches address a few independent fixes in pci-epf-vntb,
pci-epf-test and pci-ep-msi:

  1/4 fixes IRQ unwind in MSI doorbell setup (pci-epf-vntb)
  2/4 adds a bounds check for doorbell BAR offset (pci-epf-test)
  3/4 avoids free_irq() if doorbell IRQ was not successfully requested
      (pci-epf-test)
  4/4 fixes error unwind and prevent double allocation in
      pci_epf_alloc_doorbell() (pci-ep-msi)

These fixes were originally intended to be included in the next revision
of the main series. However, doing so would have grown the v7 series to
around 15 patches, so I am posting them separately to keep the feature
series manageable.

Kind regards,
Koichiro


Koichiro Den (4):
  PCI: endpoint: pci-epf-vntb: Fix MSI doorbell IRQ unwind
  PCI: endpoint: pci-epf-test: Sanity-check doorbell offset within BAR
  PCI: endpoint: pci-epf-test: Don't free doorbell IRQ unless requested
  PCI: endpoint: pci-ep-msi: Fix error unwind and prevent double alloc

 drivers/pci/endpoint/functions/pci-epf-test.c | 12 +++++++++++-
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 11 ++++++-----
 drivers/pci/endpoint/pci-ep-msi.c             |  5 +++++
 3 files changed, 22 insertions(+), 6 deletions(-)

-- 
2.51.0

[PATCH 1/4] PCI: endpoint: pci-epf-vntb: Fix MSI doorbell IRQ unwind

[Koichiro Den]

epf_ntb_db_bar_init_msi_doorbell() requests ntb->db_count doorbell IRQs
and then performs additional MSI doorbell setup that may still fail.
The error path unwinds the requested IRQs, but it uses a loop variable
that is reused later in the function. When a later step fails, the
unwind can run with an unexpected index value and leave some IRQs
requested.

Track the number of successfully requested IRQs separately and use that
counter for the unwind so all previously requested IRQs are freed on
failure.

Fixes: dc693d606644 ("PCI: endpoint: pci-epf-vntb: Add MSI doorbell support")
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index 20a400e83439..20efa27325f1 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -523,6 +523,7 @@ static int epf_ntb_db_bar_init_msi_doorbell(struct epf_ntb *ntb,
 					    enum pci_barno barno)
 {
 	struct pci_epf *epf = ntb->epf;
+	unsigned int req;
 	dma_addr_t low, high;
 	struct msi_msg *msg;
 	size_t sz;
@@ -533,14 +534,14 @@ static int epf_ntb_db_bar_init_msi_doorbell(struct epf_ntb *ntb,
 	if (ret)
 		return ret;
 
-	for (i = 0; i < ntb->db_count; i++) {
-		ret = request_irq(epf->db_msg[i].virq, epf_ntb_doorbell_handler,
+	for (req = 0; req < ntb->db_count; req++) {
+		ret = request_irq(epf->db_msg[req].virq, epf_ntb_doorbell_handler,
 				  0, "pci_epf_vntb_db", ntb);
 
 		if (ret) {
 			dev_err(&epf->dev,
 				"Failed to request doorbell IRQ: %d\n",
-				epf->db_msg[i].virq);
+				epf->db_msg[req].virq);
 			goto err_free_irq;
 		}
 	}
@@ -598,8 +599,8 @@ static int epf_ntb_db_bar_init_msi_doorbell(struct epf_ntb *ntb,
 	return 0;
 
 err_free_irq:
-	for (i--; i >= 0; i--)
-		free_irq(epf->db_msg[i].virq, ntb);
+	while (req)
+		free_irq(epf->db_msg[--req].virq, ntb);
 
 	pci_epf_free_doorbell(ntb->epf);
 	return ret;
-- 
2.51.0

Re: [PATCH 1/4] PCI: endpoint: pci-epf-vntb: Fix MSI doorbell IRQ unwind

[Niklas Cassel]

On Mon, Feb 16, 2026 at 12:09:11AM +0900, Koichiro Den wrote:
> epf_ntb_db_bar_init_msi_doorbell() requests ntb->db_count doorbell IRQs
> and then performs additional MSI doorbell setup that may still fail.
> The error path unwinds the requested IRQs, but it uses a loop variable
> that is reused later in the function. When a later step fails, the
> unwind can run with an unexpected index value and leave some IRQs
> requested.
> 
> Track the number of successfully requested IRQs separately and use that
> counter for the unwind so all previously requested IRQs are freed on
> failure.
> 
> Fixes: dc693d606644 ("PCI: endpoint: pci-epf-vntb: Add MSI doorbell support")
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---
>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index 20a400e83439..20efa27325f1 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> @@ -523,6 +523,7 @@ static int epf_ntb_db_bar_init_msi_doorbell(struct epf_ntb *ntb,
>  					    enum pci_barno barno)
>  {
>  	struct pci_epf *epf = ntb->epf;
> +	unsigned int req;
>  	dma_addr_t low, high;
>  	struct msi_msg *msg;
>  	size_t sz;
> @@ -533,14 +534,14 @@ static int epf_ntb_db_bar_init_msi_doorbell(struct epf_ntb *ntb,
>  	if (ret)
>  		return ret;
>  
> -	for (i = 0; i < ntb->db_count; i++) {
> -		ret = request_irq(epf->db_msg[i].virq, epf_ntb_doorbell_handler,
> +	for (req = 0; req < ntb->db_count; req++) {
> +		ret = request_irq(epf->db_msg[req].virq, epf_ntb_doorbell_handler,
>  				  0, "pci_epf_vntb_db", ntb);
>  
>  		if (ret) {
>  			dev_err(&epf->dev,
>  				"Failed to request doorbell IRQ: %d\n",
> -				epf->db_msg[i].virq);
> +				epf->db_msg[req].virq);
>  			goto err_free_irq;
>  		}
>  	}
> @@ -598,8 +599,8 @@ static int epf_ntb_db_bar_init_msi_doorbell(struct epf_ntb *ntb,
>  	return 0;
>  
>  err_free_irq:
> -	for (i--; i >= 0; i--)
> -		free_irq(epf->db_msg[i].virq, ntb);
> +	while (req)
> +		free_irq(epf->db_msg[--req].virq, ntb);

Please keep the for-loop.
Or if you want to change it, do so in a separate patch.


I understand that you need a separate variable for this,
since "i" is (re-)used in other places in the function,
but changing the for loop to a while is distracting from
the actual fix.


Kind regards,
Niklas

Re: [PATCH 1/4] PCI: endpoint: pci-epf-vntb: Fix MSI doorbell IRQ unwind

[Koichiro Den]

On Mon, Feb 16, 2026 at 12:56:18PM +0100, Niklas Cassel wrote:
> On Mon, Feb 16, 2026 at 12:09:11AM +0900, Koichiro Den wrote:
> > epf_ntb_db_bar_init_msi_doorbell() requests ntb->db_count doorbell IRQs
> > and then performs additional MSI doorbell setup that may still fail.
> > The error path unwinds the requested IRQs, but it uses a loop variable
> > that is reused later in the function. When a later step fails, the
> > unwind can run with an unexpected index value and leave some IRQs
> > requested.
> > 
> > Track the number of successfully requested IRQs separately and use that
> > counter for the unwind so all previously requested IRQs are freed on
> > failure.
> > 
> > Fixes: dc693d606644 ("PCI: endpoint: pci-epf-vntb: Add MSI doorbell support")
> > Signed-off-by: Koichiro Den <den@valinux.co.jp>
> > ---
> >  drivers/pci/endpoint/functions/pci-epf-vntb.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > index 20a400e83439..20efa27325f1 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > @@ -523,6 +523,7 @@ static int epf_ntb_db_bar_init_msi_doorbell(struct epf_ntb *ntb,
> >  					    enum pci_barno barno)
> >  {
> >  	struct pci_epf *epf = ntb->epf;
> > +	unsigned int req;
> >  	dma_addr_t low, high;
> >  	struct msi_msg *msg;
> >  	size_t sz;
> > @@ -533,14 +534,14 @@ static int epf_ntb_db_bar_init_msi_doorbell(struct epf_ntb *ntb,
> >  	if (ret)
> >  		return ret;
> >  
> > -	for (i = 0; i < ntb->db_count; i++) {
> > -		ret = request_irq(epf->db_msg[i].virq, epf_ntb_doorbell_handler,
> > +	for (req = 0; req < ntb->db_count; req++) {
> > +		ret = request_irq(epf->db_msg[req].virq, epf_ntb_doorbell_handler,
> >  				  0, "pci_epf_vntb_db", ntb);
> >  
> >  		if (ret) {
> >  			dev_err(&epf->dev,
> >  				"Failed to request doorbell IRQ: %d\n",
> > -				epf->db_msg[i].virq);
> > +				epf->db_msg[req].virq);
> >  			goto err_free_irq;
> >  		}
> >  	}
> > @@ -598,8 +599,8 @@ static int epf_ntb_db_bar_init_msi_doorbell(struct epf_ntb *ntb,
> >  	return 0;
> >  
> >  err_free_irq:
> > -	for (i--; i >= 0; i--)
> > -		free_irq(epf->db_msg[i].virq, ntb);
> > +	while (req)
> > +		free_irq(epf->db_msg[--req].virq, ntb);
> 
> Please keep the for-loop.
> Or if you want to change it, do so in a separate patch.
> 
> 
> I understand that you need a separate variable for this,
> since "i" is (re-)used in other places in the function,
> but changing the for loop to a while is distracting from
> the actual fix.

In that case, would you prefer the new "req" to be an int rather than unsigned
int, so the for-loop can remain safely unchanged?

I don't have a strong opinion on this. The reuse of "i" is the real problem part
and the main point of this fix.

Best regards,
Koichiro

> 
> 
> Kind regards,
> Niklas

Re: [PATCH 1/4] PCI: endpoint: pci-epf-vntb: Fix MSI doorbell IRQ unwind

[Niklas Cassel]

On Mon, Feb 16, 2026 at 11:02:58PM +0900, Koichiro Den wrote:
> On Mon, Feb 16, 2026 at 12:56:18PM +0100, Niklas Cassel wrote:
> > On Mon, Feb 16, 2026 at 12:09:11AM +0900, Koichiro Den wrote:
> > > epf_ntb_db_bar_init_msi_doorbell() requests ntb->db_count doorbell IRQs
> > > and then performs additional MSI doorbell setup that may still fail.
> > > The error path unwinds the requested IRQs, but it uses a loop variable
> > > that is reused later in the function. When a later step fails, the
> > > unwind can run with an unexpected index value and leave some IRQs
> > > requested.
> > > 
> > > Track the number of successfully requested IRQs separately and use that
> > > counter for the unwind so all previously requested IRQs are freed on
> > > failure.
> > > 
> > > Fixes: dc693d606644 ("PCI: endpoint: pci-epf-vntb: Add MSI doorbell support")
> > > Signed-off-by: Koichiro Den <den@valinux.co.jp>
> > > ---
> > >  drivers/pci/endpoint/functions/pci-epf-vntb.c | 11 ++++++-----
> > >  1 file changed, 6 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > index 20a400e83439..20efa27325f1 100644
> > > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > @@ -523,6 +523,7 @@ static int epf_ntb_db_bar_init_msi_doorbell(struct epf_ntb *ntb,
> > >  					    enum pci_barno barno)
> > >  {
> > >  	struct pci_epf *epf = ntb->epf;
> > > +	unsigned int req;
> > >  	dma_addr_t low, high;
> > >  	struct msi_msg *msg;
> > >  	size_t sz;
> > > @@ -533,14 +534,14 @@ static int epf_ntb_db_bar_init_msi_doorbell(struct epf_ntb *ntb,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	for (i = 0; i < ntb->db_count; i++) {
> > > -		ret = request_irq(epf->db_msg[i].virq, epf_ntb_doorbell_handler,
> > > +	for (req = 0; req < ntb->db_count; req++) {
> > > +		ret = request_irq(epf->db_msg[req].virq, epf_ntb_doorbell_handler,
> > >  				  0, "pci_epf_vntb_db", ntb);
> > >  
> > >  		if (ret) {
> > >  			dev_err(&epf->dev,
> > >  				"Failed to request doorbell IRQ: %d\n",
> > > -				epf->db_msg[i].virq);
> > > +				epf->db_msg[req].virq);
> > >  			goto err_free_irq;
> > >  		}
> > >  	}
> > > @@ -598,8 +599,8 @@ static int epf_ntb_db_bar_init_msi_doorbell(struct epf_ntb *ntb,
> > >  	return 0;
> > >  
> > >  err_free_irq:
> > > -	for (i--; i >= 0; i--)
> > > -		free_irq(epf->db_msg[i].virq, ntb);
> > > +	while (req)
> > > +		free_irq(epf->db_msg[--req].virq, ntb);
> > 
> > Please keep the for-loop.
> > Or if you want to change it, do so in a separate patch.
> > 
> > 
> > I understand that you need a separate variable for this,
> > since "i" is (re-)used in other places in the function,
> > but changing the for loop to a while is distracting from
> > the actual fix.
> 
> In that case, would you prefer the new "req" to be an int rather than unsigned
> int, so the for-loop can remain safely unchanged?

I suppose the same type as currently used for "i".


Kind regards,
Niklas

[PATCH 2/4] PCI: endpoint: pci-epf-test: Sanity-check doorbell offset within BAR

[Koichiro Den]

pci-epf-test advertises the doorbell target to the RC as a BAR number
and an offset. The RC rings the doorbell with a single DWORD MMIO write
to BAR + offset.

For MSI/MSI-X-based doorbells, the message address is required to be
DWORD-aligned, so the computed offset should not straddle a BAR boundary
in normal operation.

However, with support for doorbells based on mechanisms other than
MSI/MSI-X (via pci_epf_alloc_doorbell()), the returned message address
may not necessarily be DWORD-aligned. In such a case, offset plus the
32-bit write width could cross the end of the BAR aperture. The offset
returned by pci_epf_align_inbound_addr() is guaranteed to be within the
BAR size, but this alone does not ensure that a 32-bit write starting at
that offset stays within the BAR.

Add a bounds check to ensure that the 32-bit doorbell write always stays
within the BAR aperture. While this should not trigger for
spec-compliant MSI/MSI-X addresses, it provides a defensive guard
against unexpected offsets from future doorbell implementations.

Suggested-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 0cb7af0919dc..148a34e51f6b 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -761,6 +761,9 @@ static void pci_epf_test_enable_doorbell(struct pci_epf_test *epf_test,
 	if (ret)
 		goto err_doorbell_cleanup;
 
+	if (size_add(offset, sizeof(u32)) > epf->bar[bar].size)
+		goto err_doorbell_cleanup;
+
 	reg->doorbell_offset = cpu_to_le32(offset);
 
 	epf_test->db_bar.barno = bar;
-- 
2.51.0

Re: [PATCH 2/4] PCI: endpoint: pci-epf-test: Sanity-check doorbell offset within BAR

[Niklas Cassel]

On Mon, Feb 16, 2026 at 12:09:12AM +0900, Koichiro Den wrote:
> pci-epf-test advertises the doorbell target to the RC as a BAR number
> and an offset. The RC rings the doorbell with a single DWORD MMIO write
> to BAR + offset.
> 
> For MSI/MSI-X-based doorbells, the message address is required to be
> DWORD-aligned, so the computed offset should not straddle a BAR boundary
> in normal operation.
> 
> However, with support for doorbells based on mechanisms other than
> MSI/MSI-X (via pci_epf_alloc_doorbell()), the returned message address
> may not necessarily be DWORD-aligned. In such a case, offset plus the
> 32-bit write width could cross the end of the BAR aperture. The offset
> returned by pci_epf_align_inbound_addr() is guaranteed to be within the
> BAR size, but this alone does not ensure that a 32-bit write starting at
> that offset stays within the BAR.
> 
> Add a bounds check to ensure that the 32-bit doorbell write always stays
> within the BAR aperture. While this should not trigger for
> spec-compliant MSI/MSI-X addresses, it provides a defensive guard
> against unexpected offsets from future doorbell implementations.

I think everything you write is true,
and I know that I suggested this...

But for the MMIO address, will it ever not be 32-bit aligned?

Even in the eDMA case, the eDMA registers are 32-bit aligned.

Did I perhaps have a brain fart and overthink this?


I guess theoretically, some future pci_epf_alloc_doorbell() implementation
could return something that is not 32-bit aligned...

But if we really want to add a safety check for that... perhaps a 32-bit
alignment check would be better suited to have in pci_epf_alloc_doorbell() ?


Perhaps this check is better added in pci_epf_alloc_doorbell() or
pci_epf_alloc_doorbell_embedded(), in the series that adds support for
embedded doorbells ?


Kind regards,
Niklas

Re: [PATCH 2/4] PCI: endpoint: pci-epf-test: Sanity-check doorbell offset within BAR

[Koichiro Den]

On Mon, Feb 16, 2026 at 02:14:14PM +0100, Niklas Cassel wrote:
> On Mon, Feb 16, 2026 at 12:09:12AM +0900, Koichiro Den wrote:
> > pci-epf-test advertises the doorbell target to the RC as a BAR number
> > and an offset. The RC rings the doorbell with a single DWORD MMIO write
> > to BAR + offset.
> > 
> > For MSI/MSI-X-based doorbells, the message address is required to be
> > DWORD-aligned, so the computed offset should not straddle a BAR boundary
> > in normal operation.
> > 
> > However, with support for doorbells based on mechanisms other than
> > MSI/MSI-X (via pci_epf_alloc_doorbell()), the returned message address
> > may not necessarily be DWORD-aligned. In such a case, offset plus the
> > 32-bit write width could cross the end of the BAR aperture. The offset
> > returned by pci_epf_align_inbound_addr() is guaranteed to be within the
> > BAR size, but this alone does not ensure that a 32-bit write starting at
> > that offset stays within the BAR.
> > 
> > Add a bounds check to ensure that the 32-bit doorbell write always stays
> > within the BAR aperture. While this should not trigger for
> > spec-compliant MSI/MSI-X addresses, it provides a defensive guard
> > against unexpected offsets from future doorbell implementations.
> 
> I think everything you write is true,
> and I know that I suggested this...
> 
> But for the MMIO address, will it ever not be 32-bit aligned?

Yes I see your point.

> 
> Even in the eDMA case, the eDMA registers are 32-bit aligned.
> 
> Did I perhaps have a brain fart and overthink this?
> 
> 
> I guess theoretically, some future pci_epf_alloc_doorbell() implementation
> could return something that is not 32-bit aligned...

Yes, that's my impression as well. This looks more like a theoritical safeguard
in case pci_epf_align_inbound_addr() or nearby code paths gets accidentally
broken by future changes..

> 
> But if we really want to add a safety check for that... perhaps a 32-bit
> alignment check would be better suited to have in pci_epf_alloc_doorbell() ?
> 
> 
> Perhaps this check is better added in pci_epf_alloc_doorbell() or
> pci_epf_alloc_doorbell_embedded(), in the series that adds support for
> embedded doorbells ?

Hm, ok let's drop this patch at least from this series for now. I'll reconsider
it as part of the feature series instead.

Thanks,
Koichiro

> 
> 
> Kind regards,
> Niklas

[PATCH 3/4] PCI: endpoint: pci-epf-test: Don't free doorbell IRQ unless requested

[Koichiro Den]

pci_epf_test_enable_doorbell() allocates a doorbell and then installs
the interrupt handler with request_threaded_irq(). On failures before
the IRQ is successfully requested (e.g. no free BAR,
request_threaded_irq() failure), the error path jumps to
err_doorbell_cleanup and calls pci_epf_test_doorbell_cleanup().

pci_epf_test_doorbell_cleanup() unconditionally calls free_irq() for the
doorbell virq, which can trigger "Trying to free already-free IRQ"
warnings when the IRQ was never requested or when request_threaded_irq()
failed.

Track whether the doorbell IRQ has been successfully requested and only
call free_irq() when it has.

Fixes: eff0c286aa91 ("PCI: endpoint: pci-epf-test: Add doorbell test support")
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 148a34e51f6b..defe1e2ea427 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -86,6 +86,7 @@ struct pci_epf_test {
 	bool			dma_private;
 	const struct pci_epc_features *epc_features;
 	struct pci_epf_bar	db_bar;
+	bool			db_irq_requested;
 	size_t			bar_size[PCI_STD_NUM_BARS];
 };
 
@@ -715,7 +716,10 @@ static void pci_epf_test_doorbell_cleanup(struct pci_epf_test *epf_test)
 	struct pci_epf_test_reg *reg = epf_test->reg[epf_test->test_reg_bar];
 	struct pci_epf *epf = epf_test->epf;
 
-	free_irq(epf->db_msg[0].virq, epf_test);
+	if (epf_test->db_irq_requested && epf->db_msg) {
+		free_irq(epf->db_msg[0].virq, epf_test);
+		epf_test->db_irq_requested = false;
+	}
 	reg->doorbell_bar = cpu_to_le32(NO_BAR);
 
 	pci_epf_free_doorbell(epf);
@@ -732,6 +736,8 @@ static void pci_epf_test_enable_doorbell(struct pci_epf_test *epf_test,
 	size_t offset;
 	int ret;
 
+	epf_test->db_irq_requested = false;
+
 	ret = pci_epf_alloc_doorbell(epf, 1);
 	if (ret)
 		goto set_status_err;
@@ -751,6 +757,7 @@ static void pci_epf_test_enable_doorbell(struct pci_epf_test *epf_test,
 		goto err_doorbell_cleanup;
 	}
 
+	epf_test->db_irq_requested = true;
 	reg->doorbell_data = cpu_to_le32(msg->data);
 	reg->doorbell_bar = cpu_to_le32(bar);
 
-- 
2.51.0

Re: [PATCH 3/4] PCI: endpoint: pci-epf-test: Don't free doorbell IRQ unless requested

[Niklas Cassel]

On Mon, Feb 16, 2026 at 12:09:13AM +0900, Koichiro Den wrote:
> pci_epf_test_enable_doorbell() allocates a doorbell and then installs
> the interrupt handler with request_threaded_irq(). On failures before
> the IRQ is successfully requested (e.g. no free BAR,
> request_threaded_irq() failure), the error path jumps to
> err_doorbell_cleanup and calls pci_epf_test_doorbell_cleanup().
> 
> pci_epf_test_doorbell_cleanup() unconditionally calls free_irq() for the
> doorbell virq, which can trigger "Trying to free already-free IRQ"
> warnings when the IRQ was never requested or when request_threaded_irq()
> failed.
> 
> Track whether the doorbell IRQ has been successfully requested and only
> call free_irq() when it has.
> 
> Fixes: eff0c286aa91 ("PCI: endpoint: pci-epf-test: Add doorbell test support")
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 148a34e51f6b..defe1e2ea427 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -86,6 +86,7 @@ struct pci_epf_test {
>  	bool			dma_private;
>  	const struct pci_epc_features *epc_features;
>  	struct pci_epf_bar	db_bar;
> +	bool			db_irq_requested;

It would be nice if we could avoid this, it looks a bit odd.


>  	size_t			bar_size[PCI_STD_NUM_BARS];
>  };
>  
> @@ -715,7 +716,10 @@ static void pci_epf_test_doorbell_cleanup(struct pci_epf_test *epf_test)
>  	struct pci_epf_test_reg *reg = epf_test->reg[epf_test->test_reg_bar];
>  	struct pci_epf *epf = epf_test->epf;
>  
> -	free_irq(epf->db_msg[0].virq, epf_test);
> +	if (epf_test->db_irq_requested && epf->db_msg) {
> +		free_irq(epf->db_msg[0].virq, epf_test);
> +		epf_test->db_irq_requested = false;
> +	}
>  	reg->doorbell_bar = cpu_to_le32(NO_BAR);
>  
>  	pci_epf_free_doorbell(epf);
> @@ -732,6 +736,8 @@ static void pci_epf_test_enable_doorbell(struct pci_epf_test *epf_test,
>  	size_t offset;
>  	int ret;
>  
> +	epf_test->db_irq_requested = false;
> +
>  	ret = pci_epf_alloc_doorbell(epf, 1);
>  	if (ret)
>  		goto set_status_err;
> @@ -751,6 +757,7 @@ static void pci_epf_test_enable_doorbell(struct pci_epf_test *epf_test,
>  		goto err_doorbell_cleanup;
>  	}
>  
> +	epf_test->db_irq_requested = true;
>  	reg->doorbell_data = cpu_to_le32(msg->data);
>  	reg->doorbell_bar = cpu_to_le32(bar);
>  

Can't we do something like:

-For all goto's after request_threaded_irq() success case:
jump to a label that also cleans up the IRQ.

For failures before or at request_threaded_irq(), jump to
a label that does not call free_irq().



pci_epf_test_disable_doorbell() should probably return error
if (!epf_test->db_bar.size)

(before pci_epf_test_disable_doorbell() calls free_irq())

pci_epf_test_disable_doorbell() should probably also memset
epf_test->db_bar.


Kind regards,
Niklas

Re: [PATCH 3/4] PCI: endpoint: pci-epf-test: Don't free doorbell IRQ unless requested

[Koichiro Den]

On Mon, Feb 16, 2026 at 12:35:56PM +0100, Niklas Cassel wrote:
> On Mon, Feb 16, 2026 at 12:09:13AM +0900, Koichiro Den wrote:
> > pci_epf_test_enable_doorbell() allocates a doorbell and then installs
> > the interrupt handler with request_threaded_irq(). On failures before
> > the IRQ is successfully requested (e.g. no free BAR,
> > request_threaded_irq() failure), the error path jumps to
> > err_doorbell_cleanup and calls pci_epf_test_doorbell_cleanup().
> > 
> > pci_epf_test_doorbell_cleanup() unconditionally calls free_irq() for the
> > doorbell virq, which can trigger "Trying to free already-free IRQ"
> > warnings when the IRQ was never requested or when request_threaded_irq()
> > failed.
> > 
> > Track whether the doorbell IRQ has been successfully requested and only
> > call free_irq() when it has.
> > 
> > Fixes: eff0c286aa91 ("PCI: endpoint: pci-epf-test: Add doorbell test support")
> > Signed-off-by: Koichiro Den <den@valinux.co.jp>
> > ---
> >  drivers/pci/endpoint/functions/pci-epf-test.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index 148a34e51f6b..defe1e2ea427 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -86,6 +86,7 @@ struct pci_epf_test {
> >  	bool			dma_private;
> >  	const struct pci_epc_features *epc_features;
> >  	struct pci_epf_bar	db_bar;
> > +	bool			db_irq_requested;
> 
> It would be nice if we could avoid this, it looks a bit odd.
> 
> 
> >  	size_t			bar_size[PCI_STD_NUM_BARS];
> >  };
> >  
> > @@ -715,7 +716,10 @@ static void pci_epf_test_doorbell_cleanup(struct pci_epf_test *epf_test)
> >  	struct pci_epf_test_reg *reg = epf_test->reg[epf_test->test_reg_bar];
> >  	struct pci_epf *epf = epf_test->epf;
> >  
> > -	free_irq(epf->db_msg[0].virq, epf_test);
> > +	if (epf_test->db_irq_requested && epf->db_msg) {
> > +		free_irq(epf->db_msg[0].virq, epf_test);
> > +		epf_test->db_irq_requested = false;
> > +	}
> >  	reg->doorbell_bar = cpu_to_le32(NO_BAR);
> >  
> >  	pci_epf_free_doorbell(epf);
> > @@ -732,6 +736,8 @@ static void pci_epf_test_enable_doorbell(struct pci_epf_test *epf_test,
> >  	size_t offset;
> >  	int ret;
> >  
> > +	epf_test->db_irq_requested = false;
> > +
> >  	ret = pci_epf_alloc_doorbell(epf, 1);
> >  	if (ret)
> >  		goto set_status_err;
> > @@ -751,6 +757,7 @@ static void pci_epf_test_enable_doorbell(struct pci_epf_test *epf_test,
> >  		goto err_doorbell_cleanup;
> >  	}
> >  
> > +	epf_test->db_irq_requested = true;
> >  	reg->doorbell_data = cpu_to_le32(msg->data);
> >  	reg->doorbell_bar = cpu_to_le32(bar);
> >  
> 
> Can't we do something like:
> 
> -For all goto's after request_threaded_irq() success case:
> jump to a label that also cleans up the IRQ.
> 
> For failures before or at request_threaded_irq(), jump to
> a label that does not call free_irq().

I thought this would be a minimal change to avoid the problematic case, but I
agree it's not very clean (the "db_irq_requested" flag looks a bit odd).

So I'll split pci_epf_test_doorbell_cleanup() into two helper functions to match
your suggested structure.

> 
> 
> 
> pci_epf_test_disable_doorbell() should probably return error
> if (!epf_test->db_bar.size)
> 
> (before pci_epf_test_disable_doorbell() calls free_irq())
> 
> pci_epf_test_disable_doorbell() should probably also memset
> epf_test->db_bar.

and I'll take care of these suggestions as well.

Thanks for the review,
Koichiro

> 
> 
> Kind regards,
> Niklas

Re: [PATCH 3/4] PCI: endpoint: pci-epf-test: Don't free doorbell IRQ unless requested

[Koichiro Den]

On Mon, Feb 16, 2026 at 11:30:44PM +0900, Koichiro Den wrote:
> On Mon, Feb 16, 2026 at 12:35:56PM +0100, Niklas Cassel wrote:
> > On Mon, Feb 16, 2026 at 12:09:13AM +0900, Koichiro Den wrote:
> > > pci_epf_test_enable_doorbell() allocates a doorbell and then installs
> > > the interrupt handler with request_threaded_irq(). On failures before
> > > the IRQ is successfully requested (e.g. no free BAR,
> > > request_threaded_irq() failure), the error path jumps to
> > > err_doorbell_cleanup and calls pci_epf_test_doorbell_cleanup().
> > > 
> > > pci_epf_test_doorbell_cleanup() unconditionally calls free_irq() for the
> > > doorbell virq, which can trigger "Trying to free already-free IRQ"
> > > warnings when the IRQ was never requested or when request_threaded_irq()
> > > failed.
> > > 
> > > Track whether the doorbell IRQ has been successfully requested and only
> > > call free_irq() when it has.
> > > 
> > > Fixes: eff0c286aa91 ("PCI: endpoint: pci-epf-test: Add doorbell test support")
> > > Signed-off-by: Koichiro Den <den@valinux.co.jp>
> > > ---
> > >  drivers/pci/endpoint/functions/pci-epf-test.c | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > index 148a34e51f6b..defe1e2ea427 100644
> > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > @@ -86,6 +86,7 @@ struct pci_epf_test {
> > >  	bool			dma_private;
> > >  	const struct pci_epc_features *epc_features;
> > >  	struct pci_epf_bar	db_bar;
> > > +	bool			db_irq_requested;
> > 
> > It would be nice if we could avoid this, it looks a bit odd.
> > 
> > 
> > >  	size_t			bar_size[PCI_STD_NUM_BARS];
> > >  };
> > >  
> > > @@ -715,7 +716,10 @@ static void pci_epf_test_doorbell_cleanup(struct pci_epf_test *epf_test)
> > >  	struct pci_epf_test_reg *reg = epf_test->reg[epf_test->test_reg_bar];
> > >  	struct pci_epf *epf = epf_test->epf;
> > >  
> > > -	free_irq(epf->db_msg[0].virq, epf_test);
> > > +	if (epf_test->db_irq_requested && epf->db_msg) {
> > > +		free_irq(epf->db_msg[0].virq, epf_test);
> > > +		epf_test->db_irq_requested = false;
> > > +	}
> > >  	reg->doorbell_bar = cpu_to_le32(NO_BAR);
> > >  
> > >  	pci_epf_free_doorbell(epf);
> > > @@ -732,6 +736,8 @@ static void pci_epf_test_enable_doorbell(struct pci_epf_test *epf_test,
> > >  	size_t offset;
> > >  	int ret;
> > >  
> > > +	epf_test->db_irq_requested = false;
> > > +
> > >  	ret = pci_epf_alloc_doorbell(epf, 1);
> > >  	if (ret)
> > >  		goto set_status_err;
> > > @@ -751,6 +757,7 @@ static void pci_epf_test_enable_doorbell(struct pci_epf_test *epf_test,
> > >  		goto err_doorbell_cleanup;
> > >  	}
> > >  
> > > +	epf_test->db_irq_requested = true;
> > >  	reg->doorbell_data = cpu_to_le32(msg->data);
> > >  	reg->doorbell_bar = cpu_to_le32(bar);
> > >  
> > 
> > Can't we do something like:
> > 
> > -For all goto's after request_threaded_irq() success case:
> > jump to a label that also cleans up the IRQ.
> > 
> > For failures before or at request_threaded_irq(), jump to
> > a label that does not call free_irq().
> 
> I thought this would be a minimal change to avoid the problematic case, but I
> agree it's not very clean (the "db_irq_requested" flag looks a bit odd).
> 
> So I'll split pci_epf_test_doorbell_cleanup() into two helper functions to match
> your suggested structure.
> 
> > 
> > 
> > 
> > pci_epf_test_disable_doorbell() should probably return error
> > if (!epf_test->db_bar.size)
> > 
> > (before pci_epf_test_disable_doorbell() calls free_irq())
> > 
> > pci_epf_test_disable_doorbell() should probably also memset
> > epf_test->db_bar.
> 
> and I'll take care of these suggestions as well.

I revisited the latest doorbell code with those latter suggestions in mind.
Looking at the current flow, it seems we may not actually need these additional
safeguards.

- any error path inside pci_epf_test_enable_doorbell() resets reg->doorbell_bar
  to NO_BAR.
- pci_epf_test_disable_doorbell() performs its cleanup only if reg->doorbell_bar
  is not NO_BAR. When it runs, it always resets reg->doorbell_bar to NO_BAR, so
  it cannot effectively run twice.

Given that the guarding logic is already centralized around reg->doorbell_bar,
explicitly clearing epf_test->db_bar does not seem strictly necessary.

It would be more of a refactoring cleanup than a functional fix.

Koichiro

> 
> Thanks for the review,
> Koichiro
> 
> > 
> > 
> > Kind regards,
> > Niklas

[PATCH 4/4] PCI: endpoint: pci-ep-msi: Fix error unwind and prevent double alloc

[Koichiro Den]

pci_epf_alloc_doorbell() stores the allocated doorbell message array in
epf->db_msg/epf->num_db before requesting MSI vectors. If MSI allocation
fails, the array is freed but the EPF state may still point to freed
memory.

Clear epf->db_msg and epf->num_db on the MSI allocation failure path so
that later cleanup cannot double-free the array and callers can retry
allocation.

Also return -EBUSY when doorbells have already been allocated to prevent
leaking or overwriting an existing allocation.

Fixes: 1c3b002c6bf6 ("PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller")
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/pci/endpoint/pci-ep-msi.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/endpoint/pci-ep-msi.c b/drivers/pci/endpoint/pci-ep-msi.c
index 1b58357b905f..ad8a81d6ad77 100644
--- a/drivers/pci/endpoint/pci-ep-msi.c
+++ b/drivers/pci/endpoint/pci-ep-msi.c
@@ -50,6 +50,9 @@ int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 num_db)
 		return -EINVAL;
 	}
 
+	if (epf->db_msg)
+		return -EBUSY;
+
 	domain = of_msi_map_get_device_domain(epc->dev.parent, 0,
 					      DOMAIN_BUS_PLATFORM_MSI);
 	if (!domain) {
@@ -79,6 +82,8 @@ int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 num_db)
 	if (ret) {
 		dev_err(dev, "Failed to allocate MSI\n");
 		kfree(msg);
+		epf->db_msg = NULL;
+		epf->num_db = 0;
 		return ret;
 	}
 
-- 
2.51.0

Re: [PATCH 4/4] PCI: endpoint: pci-ep-msi: Fix error unwind and prevent double alloc

[Niklas Cassel]

On Mon, Feb 16, 2026 at 12:09:14AM +0900, Koichiro Den wrote:
> pci_epf_alloc_doorbell() stores the allocated doorbell message array in
> epf->db_msg/epf->num_db before requesting MSI vectors. If MSI allocation
> fails, the array is freed but the EPF state may still point to freed
> memory.
> 
> Clear epf->db_msg and epf->num_db on the MSI allocation failure path so
> that later cleanup cannot double-free the array and callers can retry
> allocation.
> 
> Also return -EBUSY when doorbells have already been allocated to prevent
> leaking or overwriting an existing allocation.
> 
> Fixes: 1c3b002c6bf6 ("PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller")
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---

Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH 0/4] PCI: endpoint: Doorbell-related fixes

[Niklas Cassel]

On Mon, Feb 16, 2026 at 12:09:10AM +0900, Koichiro Den wrote:
> Hi,
> 
> This is a small fix-only series related to the previous (v6)
> doorbell-related series:
> https://lore.kernel.org/linux-pci/20260209125316.2132589-1-den@valinux.co.jp/
> 
> These patches address a few independent fixes in pci-epf-vntb,
> pci-epf-test and pci-ep-msi:
> 
>   1/4 fixes IRQ unwind in MSI doorbell setup (pci-epf-vntb)
>   2/4 adds a bounds check for doorbell BAR offset (pci-epf-test)
>   3/4 avoids free_irq() if doorbell IRQ was not successfully requested
>       (pci-epf-test)
>   4/4 fixes error unwind and prevent double allocation in
>       pci_epf_alloc_doorbell() (pci-ep-msi)
> 
> These fixes were originally intended to be included in the next revision
> of the main series. However, doing so would have grown the v7 series to
> around 15 patches, so I am posting them separately to keep the feature
> series manageable.

I think it is a good idea to split out the doorbell fixes to its own series.

However, when splitting things out, it is getting a bit hard to track the
most "up to date" thing to look at.

At least for me, it would be nice if you could create a patchwork account
and then go in to:
https://patchwork.kernel.org/project/linux-pci/list/?submitter=216987

And mark your older series (that now has a newer version) as "Superseded".

You've been doing a lot of nice work lately, but it seems like the PCI
maintainers patchwork queue/backlog is quite large right now (7 long pages
in patchwork).


I think the chances are higher that your work will get picked up if you mark
your old series as "Superseeded", because it keeps the PCI maintainers queue/
backlog smaller. (So less chance that something will be overlooked/missed.)

(I do this myself too, because it seems to make things more likely to get
picked up.)


Kind regards,
Niklas

Re: [PATCH 0/4] PCI: endpoint: Doorbell-related fixes

[Koichiro Den]

On Mon, Feb 16, 2026 at 12:51:23PM +0100, Niklas Cassel wrote:
> On Mon, Feb 16, 2026 at 12:09:10AM +0900, Koichiro Den wrote:
> > Hi,
> > 
> > This is a small fix-only series related to the previous (v6)
> > doorbell-related series:
> > https://lore.kernel.org/linux-pci/20260209125316.2132589-1-den@valinux.co.jp/
> > 
> > These patches address a few independent fixes in pci-epf-vntb,
> > pci-epf-test and pci-ep-msi:
> > 
> >   1/4 fixes IRQ unwind in MSI doorbell setup (pci-epf-vntb)
> >   2/4 adds a bounds check for doorbell BAR offset (pci-epf-test)
> >   3/4 avoids free_irq() if doorbell IRQ was not successfully requested
> >       (pci-epf-test)
> >   4/4 fixes error unwind and prevent double allocation in
> >       pci_epf_alloc_doorbell() (pci-ep-msi)
> > 
> > These fixes were originally intended to be included in the next revision
> > of the main series. However, doing so would have grown the v7 series to
> > around 15 patches, so I am posting them separately to keep the feature
> > series manageable.
> 
> I think it is a good idea to split out the doorbell fixes to its own series.
> 
> However, when splitting things out, it is getting a bit hard to track the
> most "up to date" thing to look at.
> 
> At least for me, it would be nice if you could create a patchwork account
> and then go in to:
> https://patchwork.kernel.org/project/linux-pci/list/?submitter=216987
> 
> And mark your older series (that now has a newer version) as "Superseded".
> 
> You've been doing a lot of nice work lately, but it seems like the PCI
> maintainers patchwork queue/backlog is quite large right now (7 long pages
> in patchwork).
> 
> 
> I think the chances are higher that your work will get picked up if you mark
> your old series as "Superseeded", because it keeps the PCI maintainers queue/
> backlog smaller. (So less chance that something will be overlooked/missed.)
> 
> (I do this myself too, because it seems to make things more likely to get
> picked up.)

Thanks a lot for the tip. I've just gone through my patches in Patchwork and
marked the older series as Superseded. It looks like someone had already cleaned
up most of them. Thank you to whoever did that.

Best regards,
Koichiro

> 
> 
> Kind regards,
> Niklas