Re: [PATCH net-next v2 12/18] net: macb: match skb_reserve(skb, NET_IP_ALIGN) with HW alignment

[Sean Anderson <sean.anderson@linux.dev>]

On 6/27/25 05:08, Théo Lebrun wrote:
> If HW is RSC capable, it cannot add dummy bytes at the start of IP

Receive-side coalescing? Can you add a brief description of this
feature to your commit message?

> packets. Alignment (ie number of dummy bytes) is configured using the
> RBOF field inside the NCFGR register.
> 
> On the software side, the skb_reserve(skb, NET_IP_ALIGN) call must only
> be done if those dummy bytes are added by the hardware; notice the
> skb_reserve() is done AFTER writing the address to the device.
> 
> We cannot do the skb_reserve() call BEFORE writing the address because
> the address field ignores the low 2/3 bits. Conclusion: in some cases,
> we risk not being able to respect the NET_IP_ALIGN value (which is
> picked based on unaligned CPU access performance).
> 
> Fixes: 4df95131ea80 ("net/macb: change RX path for GEM")

Do any existing MACBs support RSC? Is this a fix? 

> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/net/ethernet/cadence/macb.h      |  3 +++
>  drivers/net/ethernet/cadence/macb_main.c | 21 ++++++++++++++++++---
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index adc70b6efd52b0b11e436c2c95bb5108c40f3490..d42c81cf441ce435cad38e2dfd779b0e6a141bf3 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -523,6 +523,8 @@
>  /* Bitfields in DCFG6. */
>  #define GEM_PBUF_LSO_OFFSET			27
>  #define GEM_PBUF_LSO_SIZE			1
> +#define GEM_PBUF_RSC_OFFSET			26
> +#define GEM_PBUF_RSC_SIZE			1
>  #define GEM_PBUF_CUTTHRU_OFFSET			25
>  #define GEM_PBUF_CUTTHRU_SIZE			1
>  #define GEM_DAW64_OFFSET			23
> @@ -733,6 +735,7 @@
>  #define MACB_CAPS_MIIONRGMII			BIT(9)
>  #define MACB_CAPS_NEED_TSUCLK			BIT(10)
>  #define MACB_CAPS_QUEUE_DISABLE			BIT(11)
> +#define MACB_CAPS_RSC_CAPABLE			BIT(12)

No need to be _CAPABLE, we're already _CAPS_

>  #define MACB_CAPS_PCS				BIT(24)
>  #define MACB_CAPS_HIGH_SPEED			BIT(25)
>  #define MACB_CAPS_CLK_HW_CHG			BIT(26)
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 48b75d95861317b9925b366446c7572c7e186628..578e72c7727d4f578478ff2b3d0a6316327271b1 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1317,8 +1317,19 @@ static void gem_rx_refill(struct macb_queue *queue)
>  			dma_wmb();
>  			macb_set_addr(bp, desc, paddr);
>  
> -			/* properly align Ethernet header */
> -			skb_reserve(skb, NET_IP_ALIGN);
> +			/* Properly align Ethernet header.
> +			 *
> +			 * Hardware can add dummy bytes if asked using the RBOF
> +			 * field inside the NCFGR register. That feature isn't
> +			 * available if hardware is RSC capable.
> +			 *
> +			 * We cannot fallback to doing the 2-byte shift before
> +			 * DMA mapping because the address field does not allow
> +			 * setting the low 2/3 bits.
> +			 * It is 3 bits if HW_DMA_CAP_PTP, else 2 bits.
> +			 */
> +			if (!(bp->caps & MACB_CAPS_RSC_CAPABLE))
> +				skb_reserve(skb, NET_IP_ALIGN);
>  		} else {
>  			desc->ctrl = 0;
>  			dma_wmb();
> @@ -2787,7 +2798,9 @@ static void macb_init_hw(struct macb *bp)
>  	macb_set_hwaddr(bp);
>  
>  	config = macb_mdc_clk_div(bp);
> -	config |= MACB_BF(RBOF, NET_IP_ALIGN);	/* Make eth data aligned */
> +	/* Make eth data aligned. If RSC capable, that offset is ignored by HW. */
> +	if (!(bp->caps & MACB_CAPS_RSC_CAPABLE))
> +		config |= MACB_BF(RBOF, NET_IP_ALIGN);
>  	config |= MACB_BIT(DRFCS);		/* Discard Rx FCS */
>  	if (bp->caps & MACB_CAPS_JUMBO)
>  		config |= MACB_BIT(JFRAME);	/* Enable jumbo frames */
> @@ -4108,6 +4121,8 @@ static void macb_configure_caps(struct macb *bp,
>  		dcfg = gem_readl(bp, DCFG2);
>  		if ((dcfg & (GEM_BIT(RX_PKT_BUFF) | GEM_BIT(TX_PKT_BUFF))) == 0)
>  			bp->caps |= MACB_CAPS_FIFO_MODE;
> +		if (GEM_BFEXT(PBUF_RSC, gem_readl(bp, DCFG6)))
> +			bp->caps |= MACB_CAPS_RSC_CAPABLE;
>  		if (gem_has_ptp(bp)) {
>  			if (!GEM_BFEXT(TSU, gem_readl(bp, DCFG5)))
>  				dev_err(&bp->pdev->dev,
>