Re: [PATCH v3 09/10] NTB: epf: Fix doorbell bitmask handling in db_read/db_clear

[Koichiro Den]

On Fri, May 01, 2026 at 09:59:42PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Mar 23, 2026 at 12:15:43PM +0900, Koichiro Den wrote:
> > The EPF driver currently stores the incoming doorbell as a vector number
> > (irq_no + 1) in db_val and db_clear() clears all bits unconditionally.
> > This breaks db_read()/db_clear() semantics when multiple doorbells are
> > used.
> > 
> > Store doorbells as a bitmask (BIT_ULL(vector)) and make
> > db_clear(db_bits) clear only the specified bits. Use atomic64 operations
> > as db_val is updated from interrupt context.
> > 
> > Fixes: 812ce2f8d14e ("NTB: Add support for EPF PCI Non-Transparent Bridge")
> > Reviewed-by: Frank Li <Frank.Li@xxxxxxx>
> > Suggested-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> > Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> 
> On this patch too, but some pre-existing issues as well:
> 
> https://sashiko.dev/#/patchset/20260323031544.2598111-1-den%40valinux.co.jp?part=9

Hi Mani,

I also revisited Sashiko's feedback against patch 9.

My current understanding is still that most of the underlying IRQ handling
concerns are pre-existing. However, I realized that patch 9 does make one of
them more relevant because this series starts giving the reported doorbell
vector real per-vector semantics, so I think this should be fixed in this
series.

For the individual Sashiko reports against patch 9:

1) irq_no derivation from the Linux IRQ number

   The reverse calculation itself is not introduced by patch 9. The existing
   code already derives irq_no from the Linux IRQ number and uses it as the
   doorbell event number. Patch 9 preserves that assumption.

   Still, I agree the assumption is fragile. Linux IRQ numbers are not the
   device vector numbers, and the code should not rely on arithmetic on Linux
   IRQ numbers to recover the device vector. Since I will respin in any case,
   I'll resolve this as well in v4.

   The IRQ series in [2] addresses the request_irq() unwind problem and avoids
   calling pci_irq_vector() from hardirq context. It is already acked, but is
   not visible in mainline yet. I will therefore make [2] an explicit
   prerequisite for v4 and call that out in the cover letter.

   So, on top of [2], v4 will replace the remaining irq_base-style vector
   derivation with a per-vector IRQ context passed as request_irq()'s dev_id, so
   the ISR receives the device vector index directly.

2) unvalidated irq_no

   This is also pre-existing in the sense that the current code already passes
   the derived irq_no to ntb_db_event() without validation.

   However, patch 9 additionally uses the derived value in BIT_ULL(), and this
   series implements db_vector_count/mask(), so an invalid vector could lead to
   both an invalid bit shift and wrong downstream per-vector doorbell handling.

   So v4 will validate the doorbell vector before updating db_val and reporting
   the doorbell event.

3) request_irq() unwind / probe failure cleanup

   This is the ntb_hw_epf IRQ lifetime issue I mentioned in [1], and what I was
   trying to address in [2]. I still think this is orthogonal to the doorbell
   bitmask semantics, so I'll keep [2] as a prerequisite rather than fold that
   cleanup into this series.

To summarize, I'm planning to include the following in v4:
  - list [2] as an explicit prerequisite and call it out in the cover letter,
  - stop deriving the device vector from Linux IRQ-number arithmetic by
    using per-vector IRQ context for ntb_epf_vec_isr(), and
  - validate the doorbell vector before BIT_ULL() and ntb_db_event().

Please let me know if you would prefer a different split.

[1] https://lore.kernel.org/linux-pci/7jkibbdz3m3vuq2kq2xxnkt2rjdt5v7itgowc7qormtwgnf6tv@krl5brfjul5y/
[2] https://lore.kernel.org/ntb/20260304083028.1391068-1-den@xxxxxxxxxxxxx/

Best regards,
Koichiro

> 
> - Mani
> 
> > ---
> >  drivers/ntb/hw/epf/ntb_hw_epf.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/ntb/hw/epf/ntb_hw_epf.c b/drivers/ntb/hw/epf/ntb_hw_epf.c
> > index 67cdc5d729d5..741d30821390 100644
> > --- a/drivers/ntb/hw/epf/ntb_hw_epf.c
> > +++ b/drivers/ntb/hw/epf/ntb_hw_epf.c
> > @@ -6,6 +6,7 @@
> >   * Author: Kishon Vijay Abraham I <kishon@xxxxxx>
> >   */
> >  
> > +#include <linux/atomic.h>
> >  #include <linux/delay.h>
> >  #include <linux/module.h>
> >  #include <linux/pci.h>
> > @@ -108,7 +109,7 @@ struct ntb_epf_dev {
> >  	unsigned int self_spad;
> >  	unsigned int peer_spad;
> >  
> > -	int db_val;
> > +	atomic64_t db_val;
> >  	u64 db_valid_mask;
> >  };
> >  
> > @@ -337,15 +338,16 @@ static irqreturn_t ntb_epf_vec_isr(int irq, void *dev)
> >  	int irq_no;
> >  
> >  	irq_no = irq - pci_irq_vector(ndev->ntb.pdev, 0);
> > -	ndev->db_val = irq_no + 1;
> >  
> >  	if (irq_no == EPF_IRQ_LINK) {
> >  		ntb_link_event(&ndev->ntb);
> >  	} else if (irq_no == EPF_IRQ_RESERVED_DB) {
> >  		dev_warn_ratelimited(ndev->dev,
> >  				     "Unexpected irq_no 1 received. Treat it as DB#0.\n");
> > +		atomic64_or(BIT_ULL(0), &ndev->db_val);
> >  		ntb_db_event(&ndev->ntb, 0);
> >  	} else {
> > +		atomic64_or(BIT_ULL(irq_no - EPF_IRQ_DB_START), &ndev->db_val);
> >  		ntb_db_event(&ndev->ntb, irq_no - EPF_IRQ_DB_START);
> >  	}
> >  
> > @@ -530,7 +532,7 @@ static u64 ntb_epf_db_read(struct ntb_dev *ntb)
> >  {
> >  	struct ntb_epf_dev *ndev = ntb_ndev(ntb);
> >  
> > -	return ndev->db_val;
> > +	return atomic64_read(&ndev->db_val);
> >  }
> >  
> >  static int ntb_epf_db_clear_mask(struct ntb_dev *ntb, u64 db_bits)
> > @@ -542,7 +544,7 @@ static int ntb_epf_db_clear(struct ntb_dev *ntb, u64 db_bits)
> >  {
> >  	struct ntb_epf_dev *ndev = ntb_ndev(ntb);
> >  
> > -	ndev->db_val = 0;
> > +	atomic64_and(~db_bits, &ndev->db_val);
> >  
> >  	return 0;
> >  }
> > -- 
> > 2.51.0
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்