Re: [PATCH V2,net-next, 1/2] net: mana: Add support for coalesced RX packets on CQE

[Jakub Kicinski <kuba@kernel.org>]

On Tue,  6 Jan 2026 12:46:46 -0800 Haiyang Zhang wrote:
> From: Haiyang Zhang <haiyangz@microsoft.com>
> 
> Our NIC can have up to 4 RX packets on 1 CQE. To support this feature,
> check and process the type CQE_RX_COALESCED_4. The default setting is
> disabled, to avoid possible regression on latency.
> 
> And add ethtool handler to switch this feature. To turn it on, run:
>   ethtool -C <nic> rx-frames 4
> To turn it off:
>   ethtool -C <nic> rx-frames 1

Exposing just rx frame count, and only two values is quite unusual.
Please explain in more detail the coalescing logic of the device.

> @@ -2079,14 +2081,10 @@ static void mana_process_rx_cqe(struct mana_rxq *rxq, struct mana_cq *cq,
>  		return;
>  	}
>  
> -	pktlen = oob->ppi[0].pkt_len;
> -
> -	if (pktlen == 0) {
> -		/* data packets should never have packetlength of zero */
> -		netdev_err(ndev, "RX pkt len=0, rq=%u, cq=%u, rxobj=0x%llx\n",
> -			   rxq->gdma_id, cq->gdma_id, rxq->rxobj);
> +nextpkt:
> +	pktlen = oob->ppi[i].pkt_len;
> +	if (pktlen == 0)
>  		return;
> -	}
>  
>  	curr = rxq->buf_index;
>  	rxbuf_oob = &rxq->rx_oobs[curr];
> @@ -2097,12 +2095,15 @@ static void mana_process_rx_cqe(struct mana_rxq *rxq, struct mana_cq *cq,
>  	/* Unsuccessful refill will have old_buf == NULL.
>  	 * In this case, mana_rx_skb() will drop the packet.
>  	 */
> -	mana_rx_skb(old_buf, old_fp, oob, rxq);
> +	mana_rx_skb(old_buf, old_fp, oob, rxq, i);
>  
>  drop:
>  	mana_move_wq_tail(rxq->gdma_rq, rxbuf_oob->wqe_inf.wqe_size_in_bu);
>  
>  	mana_post_pkt_rxq(rxq);
> +
> +	if (coalesced && (++i < MANA_RXCOMP_OOB_NUM_PPI))
> +		goto nextpkt;

Please code this up as a loop. Using gotos for control flow other than
to jump to error handling epilogues is a poor coding practice (see the
kernel coding style).

> +static int mana_set_coalesce(struct net_device *ndev,
> +			     struct ethtool_coalesce *ec,
> +			     struct kernel_ethtool_coalesce *kernel_coal,
> +			     struct netlink_ext_ack *extack)
> +{
> +	struct mana_port_context *apc = netdev_priv(ndev);
> +	u8 saved_cqe_coalescing_enable;
> +	int err;
> +
> +	if (ec->rx_max_coalesced_frames != 1 &&
> +	    ec->rx_max_coalesced_frames != MANA_RXCOMP_OOB_NUM_PPI) {
> +		NL_SET_ERR_MSG_FMT(extack,
> +				   "rx-frames must be 1 or %u, got %u",
> +				   MANA_RXCOMP_OOB_NUM_PPI,
> +				   ec->rx_max_coalesced_frames);
> +		return -EINVAL;
> +	}
> +
> +	saved_cqe_coalescing_enable = apc->cqe_coalescing_enable;
> +	apc->cqe_coalescing_enable =
> +		ec->rx_max_coalesced_frames == MANA_RXCOMP_OOB_NUM_PPI;
> +
> +	if (!apc->port_is_up)
> +		return 0;
> +
> +	err = mana_config_rss(apc, TRI_STATE_TRUE, false, false);
> +

unnecessary empty line

> +	if (err) {
> +		netdev_err(ndev, "Set rx-frames to %u failed:%d\n",
> +			   ec->rx_max_coalesced_frames, err);
> +		NL_SET_ERR_MSG_FMT(extack, "Set rx-frames to %u failed",
> +				   ec->rx_max_coalesced_frames);

These messages are both pointless. If HW communication has failed
presumably there will already be an error in the logs. The extack
gives the user no information they wouldn't already have.

> +		apc->cqe_coalescing_enable = saved_cqe_coalescing_enable;
> +	}
> +
> +	return err;
> +}