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

[Joe Damato <joe@dama.to>]

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@kernel.org>
> Link: https://lore.kernel.org/r/20260815032932.151F11F000E9@smtp.kernel.org/
> Link: https://lore.kernel.org/r/20260818064951.7EA231F000E9@smtp.kernel.org/
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---
> 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?


>  	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 ?

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 ?

>  }
>  
>  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...

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(....));
}

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