Re: [PATCH net-next v4 02/10] NTB: ntb_transport: Use little-endian shared fields

[Koichiro Den]

On Fri, Sep 18, 2026 at 05:54:34PM -0700, Joe Damato wrote:
> On Mon, Sep 14, 2026 at 05:48:30PM +0900, Koichiro Den wrote:
> > ntb_transport writes payload headers and the RX ring tail with
> > iowrite32(), but reads peer-written copies from coherent memory as native
> > integers. The values are therefore byte-swapped when read on a big-endian
> > system.
> > 
> > Mark the shared fields as __le32 and convert coherent-memory accesses
> > accordingly. Read hdr->ver and hdr->len once so their checks and later
> > uses see the same values.
> > 
> > Fixes: 74465645cdb4 ("NTB: Fix Sparse Warnings")
> > Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
> > Link: https://lore.kernel.org/r/20260815032932.151F11F000E9@xxxxxxxxxxxxxxx/
> > Link: https://lore.kernel.org/r/20260818064951.7EA231F000E9@xxxxxxxxxxxxxxx/
> > Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> > Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> > ---
> > Changes in v4:
> >   - No changes.
> > 
> >  drivers/ntb/ntb_transport.c | 47 +++++++++++++++++++++----------------
> >  1 file changed, 27 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> > index 74f4f8c1c7be..3f497a62673f 100644
> > --- a/drivers/ntb/ntb_transport.c
> > +++ b/drivers/ntb/ntb_transport.c
> 
> [...]
> 
> > @@ -514,7 +514,8 @@ static int ntb_qp_debugfs_stats_show(struct seq_file *s, void *v)
> >  	seq_printf(s, "tx_err_no_buf - %llu\n", qp->tx_err_no_buf);
> >  	seq_printf(s, "tx_mw - \t0x%p\n", qp->tx_mw);
> >  	seq_printf(s, "tx_index (H) - \t%u\n", qp->tx_index);
> > -	seq_printf(s, "RRI (T) - \t%u\n", qp->remote_rx_info->entry);
> > +	seq_printf(s, "RRI (T) - \t%u\n",
> > +		   le32_to_cpu(qp->remote_rx_info->entry));
> 
> it looks like the code READ_ONCE's this field below.... is that needed here
> too?

This is only a debugfs snapshot. But using the same helper that you suggested
below makes sense.

> 
> 
> >  	seq_printf(s, "tx_max_entry - \t%u\n", qp->tx_max_entry);
> >  	seq_printf(s, "free tx - \t%u\n", ntb_transport_tx_free_entry(qp));
> >  	seq_putc(s, '\n');
> > @@ -633,7 +634,7 @@ static int ntb_transport_setup_qp_mw(struct ntb_transport_ctx *nt,
> >  		qp->rx_alloc_entry++;
> >  	}
> >  
> > -	qp->remote_rx_info->entry = qp->rx_max_entry - 1;
> > +	qp->remote_rx_info->entry = cpu_to_le32(qp->rx_max_entry - 1);
> 
> you read_once below, do you need a write_once too ?

This initializes the local tail before normal QP operation. The READ_ONCE() in
ntb_transport_tx_free_entry() is for updates from the peer via iowrite32(), not 
this initialization.

> 
> i wonder if the read_once / write_once with endian conversion can be wrapped
> up in a helper or something to simplify? not sure, it might introduce more
> code than really necessary.
> 
> >  	/* setup the hdr offsets with 0's */
> >  	for (i = 0; i < qp->rx_max_entry; i++) {
> > @@ -919,7 +920,7 @@ static void ntb_qp_link_down_reset(struct ntb_transport_qp *qp)
> >  {
> >  	ntb_qp_link_context_reset(qp);
> >  	if (qp->remote_rx_info)
> > -		qp->remote_rx_info->entry = qp->rx_max_entry - 1;
> > +		qp->remote_rx_info->entry = cpu_to_le32(qp->rx_max_entry - 1);
> 
> same as above with the onces ?

Looks like this one deserves more care. Local readers can possibly overlap the
reset, so WRITE_ONCE() would make sense here.

Thanks for spotting this. But simply adding WRITE_ONCE() here wouldn't fix such
a race with late peer writes, though. That would need a separate lifecycle fix.

> 
> >  }
> >  
> >  static void ntb_qp_link_cleanup(struct ntb_transport_qp *qp)
> > @@ -1445,7 +1446,7 @@ static void ntb_complete_rxc(struct ntb_transport_qp *qp)
> >  		if (!(entry->flags & DESC_DONE_FLAG))
> >  			break;
> >  
> > -		entry->rx_hdr->flags = 0;
> > +		entry->rx_hdr->flags = cpu_to_le32(0);
> 
> this looked weird to me but grep says this is a common idiom, so TIL
> cpu_to_le32(0) is a thing.
> 
> [...]
> 
> > @@ -2490,7 +2495,9 @@ EXPORT_SYMBOL_GPL(ntb_transport_max_size);
> >  unsigned int ntb_transport_tx_free_entry(struct ntb_transport_qp *qp)
> >  {
> >  	unsigned int head = qp->tx_index;
> > -	unsigned int tail = qp->remote_rx_info->entry;
> > +	unsigned int tail;
> > +
> > +	tail = le32_to_cpu(READ_ONCE(qp->remote_rx_info->entry));
> 
> see above re: read_once and write_once on this field...

Here READ_ONCE() keeps the comparison and calculation on the same tail value
while the peer updates it. (Perhaps this READ_ONCE() addition should belong in a
separate patch?)

> 
> i didn't really try prototyping anything but seeing this repeated pattern
> makes me wonder if
> 
> static inline u32 ntb_remote_rx_entry(blah)
> {
>   return le32_to_cpu(READ_ONCE(....));
> }

Thanks, this seems reasonable.

Best regards,
Koichiro

> 
> would make the code clearer ... or less clear. not sure.