Re: [PATCH v2 1/1] net: fec: using page pool to manage RX buffers

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 4207 bytes --]

Hello,

while looking at the fec driver I noticed the following not explicitly
initialized variable. 

On 30.09.2022 15:44:27, Shenwei Wang wrote:
> This patch optimizes the RX buffer management by using the page
> pool. The purpose for this change is to prepare for the following
> XDP support. The current driver uses one frame per page for easy
> management.

[...]

> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index b0100fe3c9e4..33f84a30e167 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -17,6 +17,7 @@
>  #include <linux/clocksource.h>
>  #include <linux/net_tstamp.h>
>  #include <linux/pm_qos.h>
> +#include <linux/bpf.h>
>  #include <linux/ptp_clock_kernel.h>
>  #include <linux/timecounter.h>
>  #include <dt-bindings/firmware/imx/rsrc.h>
> @@ -346,8 +347,11 @@ struct bufdesc_ex {
>   * the skbuffer directly.
>   */
> 
> +#define FEC_ENET_XDP_HEADROOM	(XDP_PACKET_HEADROOM)
> +
>  #define FEC_ENET_RX_PAGES	256
> -#define FEC_ENET_RX_FRSIZE	2048
> +#define FEC_ENET_RX_FRSIZE	(PAGE_SIZE - FEC_ENET_XDP_HEADROOM \
> +		- SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))
>  #define FEC_ENET_RX_FRPPG	(PAGE_SIZE / FEC_ENET_RX_FRSIZE)
>  #define RX_RING_SIZE		(FEC_ENET_RX_FRPPG * FEC_ENET_RX_PAGES)
>  #define FEC_ENET_TX_FRSIZE	2048
> @@ -517,6 +521,12 @@ struct bufdesc_prop {
>  	unsigned char dsize_log2;
>  };
> 
> +struct fec_enet_priv_txrx_info {
> +	int	offset;
> +	struct	page *page;
> +	struct  sk_buff *skb;
> +};
> +
>  struct fec_enet_priv_tx_q {
>  	struct bufdesc_prop bd;
>  	unsigned char *tx_bounce[TX_RING_SIZE];
> @@ -532,7 +542,14 @@ struct fec_enet_priv_tx_q {
> 
>  struct fec_enet_priv_rx_q {
>  	struct bufdesc_prop bd;
> -	struct  sk_buff *rx_skbuff[RX_RING_SIZE];
> +	struct  fec_enet_priv_txrx_info rx_skb_info[RX_RING_SIZE];
> +
> +	/* page_pool */
> +	struct page_pool *page_pool;
> +	struct xdp_rxq_info xdp_rxq;
> +
> +	/* rx queue number, in the range 0-7 */
> +	u8 id;

This number is never explicitly initialized, I think it's always 0.

>  };
> 
>  struct fec_stop_mode_gpr {
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 59921218a8a4..169950e43b88 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -66,6 +66,8 @@
>  #include <linux/mfd/syscon.h>
>  #include <linux/regmap.h>
>  #include <soc/imx/cpuidle.h>
> +#include <linux/filter.h>
> +#include <linux/bpf.h>
> 
>  #include <asm/cacheflush.h>
> 
> @@ -422,6 +424,48 @@ fec_enet_clear_csum(struct sk_buff *skb, struct net_device *ndev)
>  	return 0;
>  }
> 
> +static int
> +fec_enet_create_page_pool(struct fec_enet_private *fep,
> +			  struct fec_enet_priv_rx_q *rxq, int size)
> +{
> +	struct page_pool_params pp_params = {
> +		.order = 0,
> +		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
> +		.pool_size = size,
> +		.nid = dev_to_node(&fep->pdev->dev),
> +		.dev = &fep->pdev->dev,
> +		.dma_dir = DMA_FROM_DEVICE,
> +		.offset = FEC_ENET_XDP_HEADROOM,
> +		.max_len = FEC_ENET_RX_FRSIZE,
> +	};
> +	int err;
> +
> +	rxq->page_pool = page_pool_create(&pp_params);
> +	if (IS_ERR(rxq->page_pool)) {
> +		err = PTR_ERR(rxq->page_pool);
> +		rxq->page_pool = NULL;
> +		return err;
> +	}
> +
> +	err = xdp_rxq_info_reg(&rxq->xdp_rxq, fep->netdev, rxq->id, 0);

But it's used here.

> +	if (err < 0)
> +		goto err_free_pp;
> +
> +	err = xdp_rxq_info_reg_mem_model(&rxq->xdp_rxq, MEM_TYPE_PAGE_POOL,
> +					 rxq->page_pool);
> +	if (err)
> +		goto err_unregister_rxq;
> +
> +	return 0;
> +
> +err_unregister_rxq:
> +	xdp_rxq_info_unreg(&rxq->xdp_rxq);
> +err_free_pp:
> +	page_pool_destroy(rxq->page_pool);
> +	rxq->page_pool = NULL;
> +	return err;
> +}

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]