Re: [PATCH net v2 5/5] ionic: fix completion descriptor access with 2x desc size

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ionic: fix completion descriptor access with 2x desc size

This commit fixes completion descriptor access when the IONIC_Q_F_2X_CQ_DESC
flag is set. It introduces pointer arithmetic to correctly calculate the
offset of the 32-byte completion descriptor slot.

> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
[ ... ]
> @@ -729,7 +725,12 @@ static bool __ionic_rx_service(struct ionic_cq *cq, struct bpf_prog *xdp_prog)
>  	struct ionic_queue *q = cq->bound_q;
>  	struct ionic_rxq_comp *comp;
>  
> -	comp = &((struct ionic_rxq_comp *)cq->base)[cq->tail_idx];
> +	if (likely(cq->desc_size == sizeof(*comp)))
> +		comp = &((struct ionic_rxq_comp *)cq->base)[cq->tail_idx];
> +	else
> +		comp = cq->base +
> +		       cq->desc_size * cq->tail_idx +
> +		       cq->desc_size - sizeof(*comp);
>  
>  	if (!color_match(comp->pkt_type_color, cq->done_color))
>  		return false;

This is a pre-existing issue, but since the descriptor access is being
modified here, should there be a dma_rmb() after the color_match() check?

Without a read memory barrier, weakly-ordered CPU architectures might
speculatively read other fields of the descriptor from memory before checking
the color bit. This could result in processing stale data from the previous
ring cycle.

[ ... ]
> @@ -1244,7 +1240,12 @@ static bool ionic_tx_service(struct ionic_cq *cq,
>  	unsigned int pkts = 0;
>  	u16 index;
>  
> -	comp = &((struct ionic_txq_comp *)cq->base)[cq->tail_idx];
> +	if (likely(cq->desc_size == sizeof(*comp)))
> +		comp = &((struct ionic_txq_comp *)cq->base)[cq->tail_idx];
> +	else
> +		comp = cq->base +
> +		       cq->desc_size * cq->tail_idx +
> +		       cq->desc_size - sizeof(*comp);
>  
>  	if (!color_match(comp->color, cq->done_color))
>  		return false;

Does this color_match() check also need a dma_rmb() immediately following it
to prevent stale descriptor reads on the transmit side?