Re: [PATCH v2 net-next 04/14] net: fec: add fec_build_skb() to build a skb

[Frank Li <Frank.li@nxp.com>]

On Fri, Jan 16, 2026 at 03:40:17PM +0800, Wei Fang wrote:
> Extract the helper fec_build_skb() from fec_enet_rx_queue(), so that the
> code for building a skb is centralized in fec_build_skb(), which makes
> the code of fec_enet_rx_queue() more concise and readable.
>
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/net/ethernet/freescale/fec_main.c | 106 ++++++++++++----------
>  1 file changed, 60 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 68410cb3ef0a..7e8ac9d2a5ff 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1781,6 +1781,59 @@ static int fec_rx_error_check(struct net_device *ndev, u16 status)
>  	return 0;
>  }
>
> +static struct sk_buff *fec_build_skb(struct fec_enet_private *fep,
> +				     struct fec_enet_priv_rx_q *rxq,
> +				     struct bufdesc *bdp,
> +				     struct page *page, u32 len)
> +{
> +	struct net_device *ndev = fep->netdev;
> +	struct bufdesc_ex *ebdp;
> +	struct sk_buff *skb;
> +
> +	skb = build_skb(page_address(page),
> +			PAGE_SIZE << fep->pagepool_order);
> +	if (unlikely(!skb)) {
> +		page_pool_recycle_direct(rxq->page_pool, page);
> +		ndev->stats.rx_dropped++;
> +		if (net_ratelimit())
> +			netdev_err(ndev, "build_skb failed\n");
> +
> +		return NULL;
> +	}
> +
> +	skb_reserve(skb, FEC_ENET_XDP_HEADROOM + fep->rx_shift);
> +	skb_put(skb, len);
> +	skb_mark_for_recycle(skb);
> +
> +	/* Get offloads from the enhanced buffer descriptor */
> +	if (fep->bufdesc_ex) {
> +		ebdp = (struct bufdesc_ex *)bdp;
> +
> +		/* If this is a VLAN packet remove the VLAN Tag */
> +		if (ebdp->cbd_esc & cpu_to_fec32(BD_ENET_RX_VLAN))
> +			fec_enet_rx_vlan(ndev, skb);
> +
> +		/* Get receive timestamp from the skb */
> +		if (fep->hwts_rx_en)
> +			fec_enet_hwtstamp(fep, fec32_to_cpu(ebdp->ts),
> +					  skb_hwtstamps(skb));
> +
> +		if (fep->csum_flags & FLAG_RX_CSUM_ENABLED) {
> +			if (!(ebdp->cbd_esc &
> +			      cpu_to_fec32(FLAG_RX_CSUM_ERROR)))
> +				/* don't check it */
> +				skb->ip_summed = CHECKSUM_UNNECESSARY;
> +			else
> +				skb_checksum_none_assert(skb);
> +		}
> +	}
> +
> +	skb->protocol = eth_type_trans(skb, ndev);
> +	skb_record_rx_queue(skb, rxq->bd.qid);
> +
> +	return skb;
> +}
> +
>  /* During a receive, the bd_rx.cur points to the current incoming buffer.
>   * When we update through the ring, if the next incoming buffer has
>   * not been given to the system, we just set the empty indicator,
> @@ -1796,7 +1849,6 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
>  	struct  sk_buff *skb;
>  	ushort	pkt_len;
>  	int	pkt_received = 0;
> -	struct	bufdesc_ex *ebdp = NULL;
>  	int	index = 0;
>  	bool	need_swap = fep->quirks & FEC_QUIRK_SWAP_FRAME;
>  	u32 data_start = FEC_ENET_XDP_HEADROOM + fep->rx_shift;
> @@ -1866,24 +1918,6 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
>  				goto rx_processing_done;
>  		}
>
> -		/* The packet length includes FCS, but we don't want to
> -		 * include that when passing upstream as it messes up
> -		 * bridging applications.
> -		 */
> -		skb = build_skb(page_address(page),
> -				PAGE_SIZE << fep->pagepool_order);
> -		if (unlikely(!skb)) {
> -			page_pool_recycle_direct(rxq->page_pool, page);
> -			ndev->stats.rx_dropped++;
> -
> -			netdev_err_once(ndev, "build_skb failed!\n");
> -			goto rx_processing_done;
> -		}
> -
> -		skb_reserve(skb, data_start);
> -		skb_put(skb, pkt_len - sub_len);
> -		skb_mark_for_recycle(skb);
> -
>  		if (unlikely(need_swap)) {
>  			u8 *data;
>
> @@ -1891,34 +1925,14 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
>  			swap_buffer(data, pkt_len);
>  		}
>
> -		/* Extract the enhanced buffer descriptor */
> -		ebdp = NULL;
> -		if (fep->bufdesc_ex)
> -			ebdp = (struct bufdesc_ex *)bdp;
> -
> -		/* If this is a VLAN packet remove the VLAN Tag */
> -		if (fep->bufdesc_ex &&
> -		    (ebdp->cbd_esc & cpu_to_fec32(BD_ENET_RX_VLAN)))
> -			fec_enet_rx_vlan(ndev, skb);
> -
> -		skb->protocol = eth_type_trans(skb, ndev);
> -
> -		/* Get receive timestamp from the skb */
> -		if (fep->hwts_rx_en && fep->bufdesc_ex)
> -			fec_enet_hwtstamp(fep, fec32_to_cpu(ebdp->ts),
> -					  skb_hwtstamps(skb));
> -
> -		if (fep->bufdesc_ex &&
> -		    (fep->csum_flags & FLAG_RX_CSUM_ENABLED)) {
> -			if (!(ebdp->cbd_esc & cpu_to_fec32(FLAG_RX_CSUM_ERROR))) {
> -				/* don't check it */
> -				skb->ip_summed = CHECKSUM_UNNECESSARY;
> -			} else {
> -				skb_checksum_none_assert(skb);
> -			}
> -		}
> +		/* The packet length includes FCS, but we don't want to
> +		 * include that when passing upstream as it messes up
> +		 * bridging applications.
> +		 */
> +		skb = fec_build_skb(fep, rxq, bdp, page, pkt_len - sub_len);
> +		if (!skb)
> +			goto rx_processing_done;
>
> -		skb_record_rx_queue(skb, queue_id);
>  		napi_gro_receive(&fep->napi, skb);
>
>  rx_processing_done:
> --
> 2.34.1
>