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

From: Marc Kleine-Budde
Date: Wed Aug 07 2024 - 06:27:13 EST


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: signature.asc
Description: PGP signature