[PATCH net-next v2 0/4] net: lan966x: Add xdp support

[PATCH net-next v2 0/4] net: lan966x: Add xdp support

[Horatiu Vultur]

Add support for xdp in lan966x driver. Currently only XDP_PASS and
XDP_DROP are supported.

The first 2 patches are just moving things around just to simplify
the code for when the xdp is added.
Patch 3 actually adds the xdp. Currently the only supported actions
are XDP_PASS and XDP_DROP. In the future this will be extended with
XDP_TX and XDP_REDIRECT.
Patch 4 changes to use page pool API, because the handling of the
pages is similar with what already lan966x driver is doing. In this
way is possible to remove some of the code.

All these changes give a small improvement on the RX side:
Before:
iperf3 -c 10.96.10.1 -R
[  5]   0.00-10.01  sec   514 MBytes   430 Mbits/sec    0         sender
[  5]   0.00-10.00  sec   509 MBytes   427 Mbits/sec              receiver

After:
iperf3 -c 10.96.10.1 -R
[  5]   0.00-10.02  sec   540 MBytes   452 Mbits/sec    0         sender
[  5]   0.00-10.01  sec   537 MBytes   450 Mbits/sec              receiver

---
v1->v2:
- rebase on net-next, once the fixes for FDMA and MTU were accepted
- drop patch 2, which changes the MTU as is not needed anymore
- allow to run xdp programs on frames bigger than 4KB

Horatiu Vultur (4):
  net: lan966x: Add define IFH_LEN_BYTES
  net: lan966x: Split function lan966x_fdma_rx_get_frame
  net: lan966x: Add basic XDP support
  net: lan96x: Use page_pool API

 .../net/ethernet/microchip/lan966x/Kconfig    |   1 +
 .../net/ethernet/microchip/lan966x/Makefile   |   3 +-
 .../ethernet/microchip/lan966x/lan966x_fdma.c | 166 ++++++++++++------
 .../ethernet/microchip/lan966x/lan966x_ifh.h  |   1 +
 .../ethernet/microchip/lan966x/lan966x_main.c |   7 +-
 .../ethernet/microchip/lan966x/lan966x_main.h |  25 +++
 .../ethernet/microchip/lan966x/lan966x_xdp.c  |  81 +++++++++
 7 files changed, 224 insertions(+), 60 deletions(-)
 create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c

-- 
2.38.0

[PATCH net-next v2 1/4] net: lan966x: Add define IFH_LEN_BYTES

[Horatiu Vultur]

The total length of IFH(inter frame header) in bytes is calculated as
IFH_LEN * sizeof(u32). Because IFH_LEN describes the length in words
and not in bytes. As the length of IFH in bytes is used quite often,
add a define for this. This is just to simplify the things.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c | 10 +++++-----
 drivers/net/ethernet/microchip/lan966x/lan966x_ifh.h  |  1 +
 drivers/net/ethernet/microchip/lan966x/lan966x_main.c |  2 +-
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
index e6948939ccc2b..6c102ee20f1d7 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
@@ -436,7 +436,7 @@ static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx)
 			       DMA_ATTR_SKIP_CPU_SYNC);
 
 	skb->dev = lan966x->ports[src_port]->dev;
-	skb_pull(skb, IFH_LEN * sizeof(u32));
+	skb_pull(skb, IFH_LEN_BYTES);
 
 	if (likely(!(skb->dev->features & NETIF_F_RXFCS)))
 		skb_trim(skb, skb->len - ETH_FCS_LEN);
@@ -592,7 +592,7 @@ int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev)
 	}
 
 	/* skb processing */
-	needed_headroom = max_t(int, IFH_LEN * sizeof(u32) - skb_headroom(skb), 0);
+	needed_headroom = max_t(int, IFH_LEN_BYTES - skb_headroom(skb), 0);
 	needed_tailroom = max_t(int, ETH_FCS_LEN - skb_tailroom(skb), 0);
 	if (needed_headroom || needed_tailroom || skb_header_cloned(skb)) {
 		err = pskb_expand_head(skb, needed_headroom, needed_tailroom,
@@ -605,8 +605,8 @@ int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev)
 	}
 
 	skb_tx_timestamp(skb);
-	skb_push(skb, IFH_LEN * sizeof(u32));
-	memcpy(skb->data, ifh, IFH_LEN * sizeof(u32));
+	skb_push(skb, IFH_LEN_BYTES);
+	memcpy(skb->data, ifh, IFH_LEN_BYTES);
 	skb_put(skb, 4);
 
 	dma_addr = dma_map_single(lan966x->dev, skb->data, skb->len,
@@ -740,7 +740,7 @@ int lan966x_fdma_change_mtu(struct lan966x *lan966x)
 	u32 val;
 
 	max_mtu = lan966x_fdma_get_max_mtu(lan966x);
-	max_mtu += IFH_LEN * sizeof(u32);
+	max_mtu += IFH_LEN_BYTES;
 	max_mtu += SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 	max_mtu += VLAN_HLEN * 2;
 
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_ifh.h b/drivers/net/ethernet/microchip/lan966x/lan966x_ifh.h
index ca3314789d18d..f3b1e0d318261 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_ifh.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_ifh.h
@@ -8,6 +8,7 @@
  */
 
 #define IFH_LEN                      7
+#define IFH_LEN_BYTES                (IFH_LEN * sizeof(u32))
 
 /* Timestamp for frame */
 #define IFH_POS_TIMESTAMP            192
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index 20ee5b28f70a5..1a27946ccaf44 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -760,7 +760,7 @@ static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
 			 NETIF_F_HW_VLAN_STAG_TX |
 			 NETIF_F_HW_TC;
 	dev->hw_features |= NETIF_F_HW_TC;
-	dev->needed_headroom = IFH_LEN * sizeof(u32);
+	dev->needed_headroom = IFH_LEN_BYTES;
 
 	eth_hw_addr_gen(dev, lan966x->base_mac, p + 1);
 
-- 
2.38.0

[PATCH net-next v2 2/4] net: lan966x: Split function lan966x_fdma_rx_get_frame

[Horatiu Vultur]

The function lan966x_fdma_rx_get_frame was unmapping the frame from
device and check also if the frame was received on a valid port. And
only after that it tried to generate the skb.
Move this check in a different function, in preparation for xdp
support. Such that xdp to be added here and the
lan966x_fdma_rx_get_frame to be used only when giving the skb to upper
layers.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../ethernet/microchip/lan966x/lan966x_fdma.c | 85 +++++++++++++------
 .../ethernet/microchip/lan966x/lan966x_main.h |  9 ++
 2 files changed, 69 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
index 6c102ee20f1d7..d37765ddd53ae 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
@@ -54,6 +54,17 @@ static void lan966x_fdma_rx_free_pages(struct lan966x_rx *rx)
 	}
 }
 
+static void lan966x_fdma_rx_free_page(struct lan966x_rx *rx)
+{
+	struct page *page;
+
+	page = rx->page[rx->dcb_index][rx->db_index];
+	if (unlikely(!page))
+		return;
+
+	__free_pages(page, rx->page_order);
+}
+
 static void lan966x_fdma_rx_add_dcb(struct lan966x_rx *rx,
 				    struct lan966x_rx_dcb *dcb,
 				    u64 nextptr)
@@ -116,6 +127,12 @@ static int lan966x_fdma_rx_alloc(struct lan966x_rx *rx)
 	return 0;
 }
 
+static void lan966x_fdma_rx_advance_dcb(struct lan966x_rx *rx)
+{
+	rx->dcb_index++;
+	rx->dcb_index &= FDMA_DCB_MAX - 1;
+}
+
 static void lan966x_fdma_rx_free(struct lan966x_rx *rx)
 {
 	struct lan966x *lan966x = rx->lan966x;
@@ -403,38 +420,53 @@ static bool lan966x_fdma_rx_more_frames(struct lan966x_rx *rx)
 	return true;
 }
 
-static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx)
+static int lan966x_fdma_rx_check_frame(struct lan966x_rx *rx, u64 *src_port)
 {
 	struct lan966x *lan966x = rx->lan966x;
-	u64 src_port, timestamp;
 	struct lan966x_db *db;
-	struct sk_buff *skb;
 	struct page *page;
 
-	/* Get the received frame and unmap it */
 	db = &rx->dcbs[rx->dcb_index].db[rx->db_index];
 	page = rx->page[rx->dcb_index][rx->db_index];
+	if (unlikely(!page))
+		return FDMA_ERROR;
 
 	dma_sync_single_for_cpu(lan966x->dev, (dma_addr_t)db->dataptr,
 				FDMA_DCB_STATUS_BLOCKL(db->status),
 				DMA_FROM_DEVICE);
 
+	dma_unmap_single_attrs(lan966x->dev, (dma_addr_t)db->dataptr,
+			       PAGE_SIZE << rx->page_order, DMA_FROM_DEVICE,
+			       DMA_ATTR_SKIP_CPU_SYNC);
+
+	lan966x_ifh_get_src_port(page_address(page), src_port);
+	if (WARN_ON(*src_port >= lan966x->num_phys_ports))
+		return FDMA_ERROR;
+
+	return FDMA_PASS;
+}
+
+static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx,
+						 u64 src_port)
+{
+	struct lan966x *lan966x = rx->lan966x;
+	struct lan966x_db *db;
+	struct sk_buff *skb;
+	struct page *page;
+	u64 timestamp;
+
+	/* Get the received frame and unmap it */
+	db = &rx->dcbs[rx->dcb_index].db[rx->db_index];
+	page = rx->page[rx->dcb_index][rx->db_index];
+
 	skb = build_skb(page_address(page), PAGE_SIZE << rx->page_order);
 	if (unlikely(!skb))
-		goto unmap_page;
+		goto free_page;
 
 	skb_put(skb, FDMA_DCB_STATUS_BLOCKL(db->status));
 
-	lan966x_ifh_get_src_port(skb->data, &src_port);
 	lan966x_ifh_get_timestamp(skb->data, &timestamp);
 
-	if (WARN_ON(src_port >= lan966x->num_phys_ports))
-		goto free_skb;
-
-	dma_unmap_single_attrs(lan966x->dev, (dma_addr_t)db->dataptr,
-			       PAGE_SIZE << rx->page_order, DMA_FROM_DEVICE,
-			       DMA_ATTR_SKIP_CPU_SYNC);
-
 	skb->dev = lan966x->ports[src_port]->dev;
 	skb_pull(skb, IFH_LEN_BYTES);
 
@@ -457,12 +489,7 @@ static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx)
 
 	return skb;
 
-free_skb:
-	kfree_skb(skb);
-unmap_page:
-	dma_unmap_single_attrs(lan966x->dev, (dma_addr_t)db->dataptr,
-			       PAGE_SIZE << rx->page_order, DMA_FROM_DEVICE,
-			       DMA_ATTR_SKIP_CPU_SYNC);
+free_page:
 	__free_pages(page, rx->page_order);
 
 	return NULL;
@@ -478,6 +505,7 @@ static int lan966x_fdma_napi_poll(struct napi_struct *napi, int weight)
 	struct sk_buff *skb;
 	struct page *page;
 	int counter = 0;
+	u64 src_port;
 	u64 nextptr;
 
 	lan966x_fdma_tx_clear_buf(lan966x, weight);
@@ -487,19 +515,26 @@ static int lan966x_fdma_napi_poll(struct napi_struct *napi, int weight)
 		if (!lan966x_fdma_rx_more_frames(rx))
 			break;
 
-		skb = lan966x_fdma_rx_get_frame(rx);
+		counter++;
 
-		rx->page[rx->dcb_index][rx->db_index] = NULL;
-		rx->dcb_index++;
-		rx->dcb_index &= FDMA_DCB_MAX - 1;
+		switch (lan966x_fdma_rx_check_frame(rx, &src_port)) {
+		case FDMA_PASS:
+			break;
+		case FDMA_ERROR:
+			lan966x_fdma_rx_free_page(rx);
+			lan966x_fdma_rx_advance_dcb(rx);
+			goto allocate_new;
+		}
 
+		skb = lan966x_fdma_rx_get_frame(rx, src_port);
+		lan966x_fdma_rx_advance_dcb(rx);
 		if (!skb)
-			break;
+			goto allocate_new;
 
 		napi_gro_receive(&lan966x->napi, skb);
-		counter++;
 	}
 
+allocate_new:
 	/* Allocate new pages and map them */
 	while (dcb_reload != rx->dcb_index) {
 		db = &rx->dcbs[dcb_reload].db[rx->db_index];
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index 4ec33999e4df6..464fb5e4a8ff6 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -100,6 +100,15 @@ enum macaccess_entry_type {
 	ENTRYTYPE_MACV6,
 };
 
+/* FDMA return action codes for checking if the frame is valid
+ * FDMA_PASS, frame is valid and can be used
+ * FDMA_ERROR, something went wrong, stop getting more frames
+ */
+enum lan966x_fdma_action {
+	FDMA_PASS = 0,
+	FDMA_ERROR,
+};
+
 struct lan966x_port;
 
 struct lan966x_db {
-- 
2.38.0

Re: [PATCH net-next v2 2/4] net: lan966x: Split function lan966x_fdma_rx_get_frame

[Alexander Lobakin]

From: Horatiu Vultur <horatiu.vultur@microchip.com>
Date: Sun, 6 Nov 2022 22:11:52 +0100

> The function lan966x_fdma_rx_get_frame was unmapping the frame from
> device and check also if the frame was received on a valid port. And
> only after that it tried to generate the skb.
> Move this check in a different function, in preparation for xdp
> support. Such that xdp to be added here and the
> lan966x_fdma_rx_get_frame to be used only when giving the skb to upper
> layers.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  .../ethernet/microchip/lan966x/lan966x_fdma.c | 85 +++++++++++++------
>  .../ethernet/microchip/lan966x/lan966x_main.h |  9 ++
>  2 files changed, 69 insertions(+), 25 deletions(-)

[...]

> -static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx)
> +static int lan966x_fdma_rx_check_frame(struct lan966x_rx *rx, u64 *src_port)
>  {
>  	struct lan966x *lan966x = rx->lan966x;
> -	u64 src_port, timestamp;
>  	struct lan966x_db *db;
> -	struct sk_buff *skb;
>  	struct page *page;
>  
> -	/* Get the received frame and unmap it */
>  	db = &rx->dcbs[rx->dcb_index].db[rx->db_index];
>  	page = rx->page[rx->dcb_index][rx->db_index];
> +	if (unlikely(!page))
> +		return FDMA_ERROR;
>  
>  	dma_sync_single_for_cpu(lan966x->dev, (dma_addr_t)db->dataptr,
>  				FDMA_DCB_STATUS_BLOCKL(db->status),
>  				DMA_FROM_DEVICE);
>  
> +	dma_unmap_single_attrs(lan966x->dev, (dma_addr_t)db->dataptr,
> +			       PAGE_SIZE << rx->page_order, DMA_FROM_DEVICE,
> +			       DMA_ATTR_SKIP_CPU_SYNC);
> +
> +	lan966x_ifh_get_src_port(page_address(page), src_port);
> +	if (WARN_ON(*src_port >= lan966x->num_phys_ports))
> +		return FDMA_ERROR;
> +
> +	return FDMA_PASS;

How about making this function return s64, which would be "src_port
or negative error", and dropping the second argument @src_port (the
example of calling it below)?

> +}
> +
> +static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx,
> +						 u64 src_port)
> +{

[...]

> -		skb = lan966x_fdma_rx_get_frame(rx);
> +		counter++;
>  
> -		rx->page[rx->dcb_index][rx->db_index] = NULL;
> -		rx->dcb_index++;
> -		rx->dcb_index &= FDMA_DCB_MAX - 1;
> +		switch (lan966x_fdma_rx_check_frame(rx, &src_port)) {
> +		case FDMA_PASS:
> +			break;
> +		case FDMA_ERROR:
> +			lan966x_fdma_rx_free_page(rx);
> +			lan966x_fdma_rx_advance_dcb(rx);
> +			goto allocate_new;
> +		}

So, here you could do (if you want to keep the current flow)::

		src_port = lan966x_fdma_rx_check_frame(rx);
		switch (src_port) {
		case 0 .. S64_MAX: // for example
			break;
		case FDMA_ERROR:   // must be < 0
			lan_966x_fdma_rx_free_page(rx);
			...
		}

But given that the error path is very unlikely and cold, I would
prefer if-else over switch case:

		src_port = lan966x_fdma_rx_check_frame(rx);
		if (unlikely(src_port < 0)) {
			lan_966x_fdma_rx_free_page(rx);
			...
			goto allocate_new;
		}

>  
> +		skb = lan966x_fdma_rx_get_frame(rx, src_port);
> +		lan966x_fdma_rx_advance_dcb(rx);
>  		if (!skb)
> -			break;
> +			goto allocate_new;
>  
>  		napi_gro_receive(&lan966x->napi, skb);
> -		counter++;
>  	}
>  
> +allocate_new:
>  	/* Allocate new pages and map them */
>  	while (dcb_reload != rx->dcb_index) {
>  		db = &rx->dcbs[dcb_reload].db[rx->db_index];
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> index 4ec33999e4df6..464fb5e4a8ff6 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> @@ -100,6 +100,15 @@ enum macaccess_entry_type {
>  	ENTRYTYPE_MACV6,
>  };
>  
> +/* FDMA return action codes for checking if the frame is valid
> + * FDMA_PASS, frame is valid and can be used
> + * FDMA_ERROR, something went wrong, stop getting more frames
> + */
> +enum lan966x_fdma_action {
> +	FDMA_PASS = 0,
> +	FDMA_ERROR,
> +};
> +
>  struct lan966x_port;
>  
>  struct lan966x_db {
> -- 
> 2.38.0

Thanks,
Olek

Re: [PATCH net-next v2 2/4] net: lan966x: Split function lan966x_fdma_rx_get_frame

[Horatiu Vultur]

The 11/07/2022 17:06, Alexander Lobakin wrote:

Hi Olek,

> 
> From: Horatiu Vultur <horatiu.vultur@microchip.com>
> Date: Sun, 6 Nov 2022 22:11:52 +0100
> 
> > The function lan966x_fdma_rx_get_frame was unmapping the frame from
> > device and check also if the frame was received on a valid port. And
> > only after that it tried to generate the skb.
> > Move this check in a different function, in preparation for xdp
> > support. Such that xdp to be added here and the
> > lan966x_fdma_rx_get_frame to be used only when giving the skb to upper
> > layers.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  .../ethernet/microchip/lan966x/lan966x_fdma.c | 85 +++++++++++++------
> >  .../ethernet/microchip/lan966x/lan966x_main.h |  9 ++
> >  2 files changed, 69 insertions(+), 25 deletions(-)
> 
> [...]
> 
> > -static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx)
> > +static int lan966x_fdma_rx_check_frame(struct lan966x_rx *rx, u64 *src_port)
> >  {
> >       struct lan966x *lan966x = rx->lan966x;
> > -     u64 src_port, timestamp;
> >       struct lan966x_db *db;
> > -     struct sk_buff *skb;
> >       struct page *page;
> >
> > -     /* Get the received frame and unmap it */
> >       db = &rx->dcbs[rx->dcb_index].db[rx->db_index];
> >       page = rx->page[rx->dcb_index][rx->db_index];
> > +     if (unlikely(!page))
> > +             return FDMA_ERROR;
> >
> >       dma_sync_single_for_cpu(lan966x->dev, (dma_addr_t)db->dataptr,
> >                               FDMA_DCB_STATUS_BLOCKL(db->status),
> >                               DMA_FROM_DEVICE);
> >
> > +     dma_unmap_single_attrs(lan966x->dev, (dma_addr_t)db->dataptr,
> > +                            PAGE_SIZE << rx->page_order, DMA_FROM_DEVICE,
> > +                            DMA_ATTR_SKIP_CPU_SYNC);
> > +
> > +     lan966x_ifh_get_src_port(page_address(page), src_port);
> > +     if (WARN_ON(*src_port >= lan966x->num_phys_ports))
> > +             return FDMA_ERROR;
> > +
> > +     return FDMA_PASS;
> 
> How about making this function return s64, which would be "src_port
> or negative error", and dropping the second argument @src_port (the
> example of calling it below)?

That was also my first thought.
But the thing is, I am also adding FDMA_DROP in the next patch of this
series(3/4). And I am planning to add also FDMA_TX and FDMA_REDIRECT in
a next patch series.
Should they(FDMA_DROP, FDMA_TX, FDMA_REDIRECT) also be some negative
numbers? And then have something like you proposed belowed:
---
src_port = lan966x_fdma_rx_check_frame(rx);
if (unlikely(src_port < 0)) {

        switch(src_port) {
        case FDMA_ERROR:
             ...
             goto allocate_new
        case FDMA_DROP:
             ...
             continue;
        case FDMA_TX:
        case FDMA_REDIRECT:
        }
}
---

> 
> > +}
> > +
> > +static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx,
> > +                                              u64 src_port)
> > +{
> 
> [...]
> 
> > -             skb = lan966x_fdma_rx_get_frame(rx);
> > +             counter++;
> >
> > -             rx->page[rx->dcb_index][rx->db_index] = NULL;
> > -             rx->dcb_index++;
> > -             rx->dcb_index &= FDMA_DCB_MAX - 1;
> > +             switch (lan966x_fdma_rx_check_frame(rx, &src_port)) {
> > +             case FDMA_PASS:
> > +                     break;
> > +             case FDMA_ERROR:
> > +                     lan966x_fdma_rx_free_page(rx);
> > +                     lan966x_fdma_rx_advance_dcb(rx);
> > +                     goto allocate_new;
> > +             }
> 
> So, here you could do (if you want to keep the current flow)::
> 
>                 src_port = lan966x_fdma_rx_check_frame(rx);
>                 switch (src_port) {
>                 case 0 .. S64_MAX: // for example
>                         break;
>                 case FDMA_ERROR:   // must be < 0
>                         lan_966x_fdma_rx_free_page(rx);
>                         ...
>                 }
> 
> But given that the error path is very unlikely and cold, I would
> prefer if-else over switch case:
> 
>                 src_port = lan966x_fdma_rx_check_frame(rx);
>                 if (unlikely(src_port < 0)) {
>                         lan_966x_fdma_rx_free_page(rx);
>                         ...
>                         goto allocate_new;
>                 }
> 
> >
> > +             skb = lan966x_fdma_rx_get_frame(rx, src_port);
> > +             lan966x_fdma_rx_advance_dcb(rx);
> >               if (!skb)
> > -                     break;
> > +                     goto allocate_new;
> >
> >               napi_gro_receive(&lan966x->napi, skb);
> > -             counter++;
> >       }
> >
> > +allocate_new:
> >       /* Allocate new pages and map them */
> >       while (dcb_reload != rx->dcb_index) {
> >               db = &rx->dcbs[dcb_reload].db[rx->db_index];
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > index 4ec33999e4df6..464fb5e4a8ff6 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > @@ -100,6 +100,15 @@ enum macaccess_entry_type {
> >       ENTRYTYPE_MACV6,
> >  };
> >
> > +/* FDMA return action codes for checking if the frame is valid
> > + * FDMA_PASS, frame is valid and can be used
> > + * FDMA_ERROR, something went wrong, stop getting more frames
> > + */
> > +enum lan966x_fdma_action {
> > +     FDMA_PASS = 0,
> > +     FDMA_ERROR,
> > +};
> > +
> >  struct lan966x_port;
> >
> >  struct lan966x_db {
> > --
> > 2.38.0
> 
> Thanks,
> Olek

-- 
/Horatiu

Re: [PATCH net-next v2 2/4] net: lan966x: Split function lan966x_fdma_rx_get_frame

[Alexander Lobakin]

From: Horatiu Vultur <horatiu.vultur@microchip.com>
Date: Mon, 7 Nov 2022 22:24:15 +0100

> The 11/07/2022 17:06, Alexander Lobakin wrote:
> 
> Hi Olek,

Hey,

> 
> > 
> > From: Horatiu Vultur <horatiu.vultur@microchip.com>
> > Date: Sun, 6 Nov 2022 22:11:52 +0100
> > 
> > > The function lan966x_fdma_rx_get_frame was unmapping the frame from
> > > device and check also if the frame was received on a valid port. And
> > > only after that it tried to generate the skb.
> > > Move this check in a different function, in preparation for xdp
> > > support. Such that xdp to be added here and the
> > > lan966x_fdma_rx_get_frame to be used only when giving the skb to upper
> > > layers.

[...]

> > > +     lan966x_ifh_get_src_port(page_address(page), src_port);
> > > +     if (WARN_ON(*src_port >= lan966x->num_phys_ports))
> > > +             return FDMA_ERROR;
> > > +
> > > +     return FDMA_PASS;
> > 
> > How about making this function return s64, which would be "src_port
> > or negative error", and dropping the second argument @src_port (the
> > example of calling it below)?
> 
> That was also my first thought.
> But the thing is, I am also adding FDMA_DROP in the next patch of this
> series(3/4). And I am planning to add also FDMA_TX and FDMA_REDIRECT in
> a next patch series.

Yeah, I was reviewing the patches one by one and found out you're
adding more return values later :S

> Should they(FDMA_DROP, FDMA_TX, FDMA_REDIRECT) also be some negative
> numbers? And then have something like you proposed belowed:
> ---
> src_port = lan966x_fdma_rx_check_frame(rx);
> if (unlikely(src_port < 0)) {
> 
>         switch(src_port) {
>         case FDMA_ERROR:
>              ...
>              goto allocate_new
>         case FDMA_DROP:
>              ...
>              continue;
>         case FDMA_TX:
>         case FDMA_REDIRECT:
>         }

It's okay to make them negative, but I wouldn't place them under
`unlikely`. It could be something like:

	src_port = lan966x_fdma_rx_check_frame(rx);
	if (unlikely(src_port == FDMA_ERROR))
		goto allocate_new;

	switch (src_port) {
	case 0 ... S64_MAX:
		// do PASS;
		break;
	case FDMA_TX:
		// do TX;
		break;
	case FDMA_REDIRECT:
	// and so on
	}

where

enum {
	FDMA_ERROR = -1, // only this one is "unlikely"
	FDMA_TX = -2,
	...
};

It's all just personal taste, so up to you :) Making
rx_check_frame() writing src_port to a pointer is fine as well.

> }
> ---
> 
> > 
> > > +}
> > > +
> > > +static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx,
> > > +                                              u64 src_port)
> > > +{

[...]

> > > --
> > > 2.38.0
> > 
> > Thanks,
> > Olek
> 
> -- 
> /Horatiu

Thanks,
Olek

[PATCH net-next v2 3/4] net: lan966x: Add basic XDP support

[Horatiu Vultur]

Introduce basic XDP support to lan966x driver. Currently the driver
supports only the actions XDP_PASS, XDP_DROP and XDP_ABORTED.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../net/ethernet/microchip/lan966x/Makefile   |  3 +-
 .../ethernet/microchip/lan966x/lan966x_fdma.c | 11 ++-
 .../ethernet/microchip/lan966x/lan966x_main.c |  5 ++
 .../ethernet/microchip/lan966x/lan966x_main.h | 13 +++
 .../ethernet/microchip/lan966x/lan966x_xdp.c  | 81 +++++++++++++++++++
 5 files changed, 111 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c

diff --git a/drivers/net/ethernet/microchip/lan966x/Makefile b/drivers/net/ethernet/microchip/lan966x/Makefile
index 962f7c5f9e7dd..251a7d561d633 100644
--- a/drivers/net/ethernet/microchip/lan966x/Makefile
+++ b/drivers/net/ethernet/microchip/lan966x/Makefile
@@ -11,4 +11,5 @@ lan966x-switch-objs  := lan966x_main.o lan966x_phylink.o lan966x_port.o \
 			lan966x_ptp.o lan966x_fdma.o lan966x_lag.o \
 			lan966x_tc.o lan966x_mqprio.o lan966x_taprio.o \
 			lan966x_tbf.o lan966x_cbs.o lan966x_ets.o \
-			lan966x_tc_matchall.o lan966x_police.o lan966x_mirror.o
+			lan966x_tc_matchall.o lan966x_police.o lan966x_mirror.o \
+			lan966x_xdp.o
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
index d37765ddd53ae..fa4198c617667 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
@@ -423,6 +423,7 @@ static bool lan966x_fdma_rx_more_frames(struct lan966x_rx *rx)
 static int lan966x_fdma_rx_check_frame(struct lan966x_rx *rx, u64 *src_port)
 {
 	struct lan966x *lan966x = rx->lan966x;
+	struct lan966x_port *port;
 	struct lan966x_db *db;
 	struct page *page;
 
@@ -443,7 +444,11 @@ static int lan966x_fdma_rx_check_frame(struct lan966x_rx *rx, u64 *src_port)
 	if (WARN_ON(*src_port >= lan966x->num_phys_ports))
 		return FDMA_ERROR;
 
-	return FDMA_PASS;
+	port = lan966x->ports[*src_port];
+	if (!lan966x_xdp_port_present(port))
+		return FDMA_PASS;
+
+	return lan966x_xdp_run(port, page, FDMA_DCB_STATUS_BLOCKL(db->status));
 }
 
 static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx,
@@ -524,6 +529,10 @@ static int lan966x_fdma_napi_poll(struct napi_struct *napi, int weight)
 			lan966x_fdma_rx_free_page(rx);
 			lan966x_fdma_rx_advance_dcb(rx);
 			goto allocate_new;
+		case FDMA_DROP:
+			lan966x_fdma_rx_free_page(rx);
+			lan966x_fdma_rx_advance_dcb(rx);
+			continue;
 		}
 
 		skb = lan966x_fdma_rx_get_frame(rx, src_port);
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index 1a27946ccaf44..42be5d0f1f015 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -468,6 +468,7 @@ static const struct net_device_ops lan966x_port_netdev_ops = {
 	.ndo_get_port_parent_id		= lan966x_port_get_parent_id,
 	.ndo_eth_ioctl			= lan966x_port_ioctl,
 	.ndo_setup_tc			= lan966x_tc_setup,
+	.ndo_bpf			= lan966x_xdp,
 };
 
 bool lan966x_netdevice_check(const struct net_device *dev)
@@ -694,6 +695,7 @@ static void lan966x_cleanup_ports(struct lan966x *lan966x)
 		if (port->dev)
 			unregister_netdev(port->dev);
 
+		lan966x_xdp_port_deinit(port);
 		if (lan966x->fdma && lan966x->fdma_ndev == port->dev)
 			lan966x_fdma_netdev_deinit(lan966x, port->dev);
 
@@ -1136,6 +1138,9 @@ static int lan966x_probe(struct platform_device *pdev)
 		lan966x->ports[p]->serdes = serdes;
 
 		lan966x_port_init(lan966x->ports[p]);
+		err = lan966x_xdp_port_init(lan966x->ports[p]);
+		if (err)
+			goto cleanup_ports;
 	}
 
 	lan966x_mdb_init(lan966x);
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index 464fb5e4a8ff6..18f727f397fd6 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -103,10 +103,12 @@ enum macaccess_entry_type {
 /* FDMA return action codes for checking if the frame is valid
  * FDMA_PASS, frame is valid and can be used
  * FDMA_ERROR, something went wrong, stop getting more frames
+ * FDMA_DROP, frame is dropped, but continue to get more frames
  */
 enum lan966x_fdma_action {
 	FDMA_PASS = 0,
 	FDMA_ERROR,
+	FDMA_DROP,
 };
 
 struct lan966x_port;
@@ -329,6 +331,9 @@ struct lan966x_port {
 	enum netdev_lag_hash hash_type;
 
 	struct lan966x_port_tc tc;
+
+	struct bpf_prog *xdp_prog;
+	struct xdp_rxq_info xdp_rxq;
 };
 
 extern const struct phylink_mac_ops lan966x_phylink_mac_ops;
@@ -536,6 +541,14 @@ void lan966x_mirror_port_stats(struct lan966x_port *port,
 			       struct flow_stats *stats,
 			       bool ingress);
 
+int lan966x_xdp_port_init(struct lan966x_port *port);
+void lan966x_xdp_port_deinit(struct lan966x_port *port);
+int lan966x_xdp(struct net_device *dev, struct netdev_bpf *xdp);
+int lan966x_xdp_run(struct lan966x_port *port,
+		    struct page *page,
+		    u32 data_len);
+bool lan966x_xdp_port_present(struct lan966x_port *port);
+
 static inline void __iomem *lan_addr(void __iomem *base[],
 				     int id, int tinst, int tcnt,
 				     int gbase, int ginst,
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
new file mode 100644
index 0000000000000..b8f30abf4f795
--- /dev/null
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/bpf.h>
+#include <linux/bpf_trace.h>
+#include <linux/filter.h>
+
+#include "lan966x_main.h"
+
+static int lan966x_xdp_setup(struct net_device *dev, struct netdev_bpf *xdp)
+{
+	struct lan966x_port *port = netdev_priv(dev);
+	struct lan966x *lan966x = port->lan966x;
+	struct bpf_prog *old_prog;
+
+	if (!lan966x->fdma) {
+		NL_SET_ERR_MSG_MOD(xdp->extack,
+				   "Allow to set xdp only when using fdma");
+		return -EOPNOTSUPP;
+	}
+
+	old_prog = xchg(&port->xdp_prog, xdp->prog);
+	if (old_prog)
+		bpf_prog_put(old_prog);
+
+	return 0;
+}
+
+int lan966x_xdp(struct net_device *dev, struct netdev_bpf *xdp)
+{
+	switch (xdp->command) {
+	case XDP_SETUP_PROG:
+		return lan966x_xdp_setup(dev, xdp);
+	default:
+		return -EINVAL;
+	}
+}
+
+int lan966x_xdp_run(struct lan966x_port *port, struct page *page, u32 data_len)
+{
+	struct bpf_prog *xdp_prog = port->xdp_prog;
+	struct lan966x *lan966x = port->lan966x;
+	struct xdp_buff xdp;
+	u32 act;
+
+	xdp_init_buff(&xdp, PAGE_SIZE << lan966x->rx.page_order,
+		      &port->xdp_rxq);
+	xdp_prepare_buff(&xdp, page_address(page), IFH_LEN_BYTES,
+			 data_len - IFH_LEN_BYTES, false);
+	act = bpf_prog_run_xdp(xdp_prog, &xdp);
+	switch (act) {
+	case XDP_PASS:
+		return FDMA_PASS;
+	default:
+		bpf_warn_invalid_xdp_action(port->dev, xdp_prog, act);
+		fallthrough;
+	case XDP_ABORTED:
+		trace_xdp_exception(port->dev, xdp_prog, act);
+		fallthrough;
+	case XDP_DROP:
+		return FDMA_DROP;
+	}
+}
+
+bool lan966x_xdp_port_present(struct lan966x_port *port)
+{
+	return !!port->xdp_prog;
+}
+
+int lan966x_xdp_port_init(struct lan966x_port *port)
+{
+	struct lan966x *lan966x = port->lan966x;
+
+	return xdp_rxq_info_reg(&port->xdp_rxq, port->dev, 0,
+				lan966x->napi.napi_id);
+}
+
+void lan966x_xdp_port_deinit(struct lan966x_port *port)
+{
+	if (xdp_rxq_info_is_reg(&port->xdp_rxq))
+		xdp_rxq_info_unreg(&port->xdp_rxq);
+}
-- 
2.38.0

Re: [PATCH net-next v2 3/4] net: lan966x: Add basic XDP support

[Alexander Lobakin]

From: Alexander Lobakin <alexander.lobakin@intel.com>

From: Horatiu Vultur <horatiu.vultur@microchip.com>
Date: Sun, 6 Nov 2022 22:11:53 +0100

> Introduce basic XDP support to lan966x driver. Currently the driver
> supports only the actions XDP_PASS, XDP_DROP and XDP_ABORTED.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  .../net/ethernet/microchip/lan966x/Makefile   |  3 +-
>  .../ethernet/microchip/lan966x/lan966x_fdma.c | 11 ++-
>  .../ethernet/microchip/lan966x/lan966x_main.c |  5 ++
>  .../ethernet/microchip/lan966x/lan966x_main.h | 13 +++
>  .../ethernet/microchip/lan966x/lan966x_xdp.c  | 81 +++++++++++++++++++
>  5 files changed, 111 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c

[...]

> +bool lan966x_xdp_port_present(struct lan966x_port *port)
> +{
> +	return !!port->xdp_prog;
> +}

Why uninline such a simple check? I realize you want to keep all XDP
stuff inside in the separate file, but doesn't this one looks too
much?

> +
> +int lan966x_xdp_port_init(struct lan966x_port *port)
> +{
> +	struct lan966x *lan966x = port->lan966x;
> +
> +	return xdp_rxq_info_reg(&port->xdp_rxq, port->dev, 0,
> +				lan966x->napi.napi_id);
> +}
> +
> +void lan966x_xdp_port_deinit(struct lan966x_port *port)
> +{
> +	if (xdp_rxq_info_is_reg(&port->xdp_rxq))
> +		xdp_rxq_info_unreg(&port->xdp_rxq);
> +}
> -- 
> 2.38.0

Thanks,
Olek

Re: [PATCH net-next v2 3/4] net: lan966x: Add basic XDP support

[Horatiu Vultur]

The 11/07/2022 17:13, Alexander Lobakin wrote:

Hi Olek,

> 
> From: Alexander Lobakin <alexander.lobakin@intel.com>
> 
> From: Horatiu Vultur <horatiu.vultur@microchip.com>
> Date: Sun, 6 Nov 2022 22:11:53 +0100
> 
> > Introduce basic XDP support to lan966x driver. Currently the driver
> > supports only the actions XDP_PASS, XDP_DROP and XDP_ABORTED.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  .../net/ethernet/microchip/lan966x/Makefile   |  3 +-
> >  .../ethernet/microchip/lan966x/lan966x_fdma.c | 11 ++-
> >  .../ethernet/microchip/lan966x/lan966x_main.c |  5 ++
> >  .../ethernet/microchip/lan966x/lan966x_main.h | 13 +++
> >  .../ethernet/microchip/lan966x/lan966x_xdp.c  | 81 +++++++++++++++++++
> >  5 files changed, 111 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
> 
> [...]
> 
> > +bool lan966x_xdp_port_present(struct lan966x_port *port)
> > +{
> > +     return !!port->xdp_prog;
> > +}
> 
> Why uninline such a simple check? I realize you want to keep all XDP
> stuff inside in the separate file, but doesn't this one looks too
> much?

I was kind of hoping that the compiler will inline it for me.
But I can add it in the header file to inline it.

> 
> > +
> > +int lan966x_xdp_port_init(struct lan966x_port *port)
> > +{
> > +     struct lan966x *lan966x = port->lan966x;
> > +
> > +     return xdp_rxq_info_reg(&port->xdp_rxq, port->dev, 0,
> > +                             lan966x->napi.napi_id);
> > +}
> > +
> > +void lan966x_xdp_port_deinit(struct lan966x_port *port)
> > +{
> > +     if (xdp_rxq_info_is_reg(&port->xdp_rxq))
> > +             xdp_rxq_info_unreg(&port->xdp_rxq);
> > +}
> > --
> > 2.38.0
> 
> Thanks,
> Olek

-- 
/Horatiu

Re: [PATCH net-next v2 3/4] net: lan966x: Add basic XDP support

[Alexander Lobakin]

From: Horatiu Vultur <horatiu.vultur@microchip.com>
Date: Mon, 7 Nov 2022 22:26:18 +0100

> The 11/07/2022 17:13, Alexander Lobakin wrote:
> 
> Hi Olek,
> 
> > 
> > From: Alexander Lobakin <alexander.lobakin@intel.com>
> > 
> > From: Horatiu Vultur <horatiu.vultur@microchip.com>
> > Date: Sun, 6 Nov 2022 22:11:53 +0100
> > 
> > > Introduce basic XDP support to lan966x driver. Currently the driver
> > > supports only the actions XDP_PASS, XDP_DROP and XDP_ABORTED.
> > >
> > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > > ---
> > >  .../net/ethernet/microchip/lan966x/Makefile   |  3 +-
> > >  .../ethernet/microchip/lan966x/lan966x_fdma.c | 11 ++-
> > >  .../ethernet/microchip/lan966x/lan966x_main.c |  5 ++
> > >  .../ethernet/microchip/lan966x/lan966x_main.h | 13 +++
> > >  .../ethernet/microchip/lan966x/lan966x_xdp.c  | 81 +++++++++++++++++++
> > >  5 files changed, 111 insertions(+), 2 deletions(-)
> > >  create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
> > 
> > [...]
> > 
> > > +bool lan966x_xdp_port_present(struct lan966x_port *port)
> > > +{
> > > +     return !!port->xdp_prog;
> > > +}
> > 
> > Why uninline such a simple check? I realize you want to keep all XDP
> > stuff inside in the separate file, but doesn't this one looks too
> > much?
> 
> I was kind of hoping that the compiler will inline it for me.
> But I can add it in the header file to inline it.

That is very unlikely for the compilers to uninline an extern
function. LTO is able to do that, but even then it's not
guaranteed. So I'd keep it in a header file as an inline.

> 
> > 
> > > +
> > > +int lan966x_xdp_port_init(struct lan966x_port *port)
> > > +{
> > > +     struct lan966x *lan966x = port->lan966x;
> > > +
> > > +     return xdp_rxq_info_reg(&port->xdp_rxq, port->dev, 0,
> > > +                             lan966x->napi.napi_id);
> > > +}
> > > +
> > > +void lan966x_xdp_port_deinit(struct lan966x_port *port)
> > > +{
> > > +     if (xdp_rxq_info_is_reg(&port->xdp_rxq))
> > > +             xdp_rxq_info_unreg(&port->xdp_rxq);
> > > +}
> > > --
> > > 2.38.0
> > 
> > Thanks,
> > Olek
> 
> -- 
> /Horatiu

Thanks,
Olek

[PATCH net-next v2 4/4] net: lan96x: Use page_pool API

[Horatiu Vultur]

Use the page_pool API for allocation, freeing and DMA handling instead
of dev_alloc_pages, __free_pages and dma_map_page.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../net/ethernet/microchip/lan966x/Kconfig    |  1 +
 .../ethernet/microchip/lan966x/lan966x_fdma.c | 72 ++++++++++---------
 .../ethernet/microchip/lan966x/lan966x_main.h |  3 +
 3 files changed, 43 insertions(+), 33 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/Kconfig b/drivers/net/ethernet/microchip/lan966x/Kconfig
index 49e1464a43139..b7ae5ce7d3f7a 100644
--- a/drivers/net/ethernet/microchip/lan966x/Kconfig
+++ b/drivers/net/ethernet/microchip/lan966x/Kconfig
@@ -7,5 +7,6 @@ config LAN966X_SWITCH
 	depends on BRIDGE || BRIDGE=n
 	select PHYLINK
 	select PACKING
+	select PAGE_POOL
 	help
 	  This driver supports the Lan966x network switch device.
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
index fa4198c617667..822c1b053e2d0 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
@@ -10,47 +10,25 @@ static int lan966x_fdma_channel_active(struct lan966x *lan966x)
 static struct page *lan966x_fdma_rx_alloc_page(struct lan966x_rx *rx,
 					       struct lan966x_db *db)
 {
-	struct lan966x *lan966x = rx->lan966x;
-	dma_addr_t dma_addr;
 	struct page *page;
 
-	page = dev_alloc_pages(rx->page_order);
+	page = page_pool_dev_alloc_pages(rx->page_pool);
 	if (unlikely(!page))
 		return NULL;
 
-	dma_addr = dma_map_page(lan966x->dev, page, 0,
-				PAGE_SIZE << rx->page_order,
-				DMA_FROM_DEVICE);
-	if (unlikely(dma_mapping_error(lan966x->dev, dma_addr)))
-		goto free_page;
-
-	db->dataptr = dma_addr;
+	db->dataptr = page_pool_get_dma_addr(page);
 
 	return page;
-
-free_page:
-	__free_pages(page, rx->page_order);
-	return NULL;
 }
 
 static void lan966x_fdma_rx_free_pages(struct lan966x_rx *rx)
 {
-	struct lan966x *lan966x = rx->lan966x;
-	struct lan966x_rx_dcb *dcb;
-	struct lan966x_db *db;
 	int i, j;
 
 	for (i = 0; i < FDMA_DCB_MAX; ++i) {
-		dcb = &rx->dcbs[i];
-
-		for (j = 0; j < FDMA_RX_DCB_MAX_DBS; ++j) {
-			db = &dcb->db[j];
-			dma_unmap_single(lan966x->dev,
-					 (dma_addr_t)db->dataptr,
-					 PAGE_SIZE << rx->page_order,
-					 DMA_FROM_DEVICE);
-			__free_pages(rx->page[i][j], rx->page_order);
-		}
+		for (j = 0; j < FDMA_RX_DCB_MAX_DBS; ++j)
+			page_pool_put_full_page(rx->page_pool,
+						rx->page[i][j], false);
 	}
 }
 
@@ -62,7 +40,7 @@ static void lan966x_fdma_rx_free_page(struct lan966x_rx *rx)
 	if (unlikely(!page))
 		return;
 
-	__free_pages(page, rx->page_order);
+	page_pool_recycle_direct(rx->page_pool, page);
 }
 
 static void lan966x_fdma_rx_add_dcb(struct lan966x_rx *rx,
@@ -84,6 +62,27 @@ static void lan966x_fdma_rx_add_dcb(struct lan966x_rx *rx,
 	rx->last_entry = dcb;
 }
 
+static int lan966x_fdma_rx_alloc_page_pool(struct lan966x_rx *rx)
+{
+	struct lan966x *lan966x = rx->lan966x;
+	struct page_pool_params pp_params = {
+		.order = rx->page_order,
+		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
+		.pool_size = FDMA_DCB_MAX,
+		.nid = NUMA_NO_NODE,
+		.dev = lan966x->dev,
+		.dma_dir = DMA_FROM_DEVICE,
+		.offset = 0,
+		.max_len = PAGE_SIZE << rx->page_order,
+	};
+
+	rx->page_pool = page_pool_create(&pp_params);
+	if (IS_ERR(rx->page_pool))
+		return PTR_ERR(rx->page_pool);
+
+	return 0;
+}
+
 static int lan966x_fdma_rx_alloc(struct lan966x_rx *rx)
 {
 	struct lan966x *lan966x = rx->lan966x;
@@ -93,6 +92,9 @@ static int lan966x_fdma_rx_alloc(struct lan966x_rx *rx)
 	int i, j;
 	int size;
 
+	if (lan966x_fdma_rx_alloc_page_pool(rx))
+		return PTR_ERR(rx->page_pool);
+
 	/* calculate how many pages are needed to allocate the dcbs */
 	size = sizeof(struct lan966x_rx_dcb) * FDMA_DCB_MAX;
 	size = ALIGN(size, PAGE_SIZE);
@@ -436,10 +438,6 @@ static int lan966x_fdma_rx_check_frame(struct lan966x_rx *rx, u64 *src_port)
 				FDMA_DCB_STATUS_BLOCKL(db->status),
 				DMA_FROM_DEVICE);
 
-	dma_unmap_single_attrs(lan966x->dev, (dma_addr_t)db->dataptr,
-			       PAGE_SIZE << rx->page_order, DMA_FROM_DEVICE,
-			       DMA_ATTR_SKIP_CPU_SYNC);
-
 	lan966x_ifh_get_src_port(page_address(page), src_port);
 	if (WARN_ON(*src_port >= lan966x->num_phys_ports))
 		return FDMA_ERROR;
@@ -468,6 +466,8 @@ static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx,
 	if (unlikely(!skb))
 		goto free_page;
 
+	skb_mark_for_recycle(skb);
+
 	skb_put(skb, FDMA_DCB_STATUS_BLOCKL(db->status));
 
 	lan966x_ifh_get_timestamp(skb->data, &timestamp);
@@ -495,7 +495,7 @@ static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx,
 	return skb;
 
 free_page:
-	__free_pages(page, rx->page_order);
+	page_pool_recycle_direct(rx->page_pool, page);
 
 	return NULL;
 }
@@ -740,6 +740,7 @@ static int lan966x_qsys_sw_status(struct lan966x *lan966x)
 
 static int lan966x_fdma_reload(struct lan966x *lan966x, int new_mtu)
 {
+	struct page_pool *page_pool;
 	dma_addr_t rx_dma;
 	void *rx_dcbs;
 	u32 size;
@@ -748,6 +749,7 @@ static int lan966x_fdma_reload(struct lan966x *lan966x, int new_mtu)
 	/* Store these for later to free them */
 	rx_dma = lan966x->rx.dma;
 	rx_dcbs = lan966x->rx.dcbs;
+	page_pool = lan966x->rx.page_pool;
 
 	napi_synchronize(&lan966x->napi);
 	napi_disable(&lan966x->napi);
@@ -765,11 +767,14 @@ static int lan966x_fdma_reload(struct lan966x *lan966x, int new_mtu)
 	size = ALIGN(size, PAGE_SIZE);
 	dma_free_coherent(lan966x->dev, size, rx_dcbs, rx_dma);
 
+	page_pool_destroy(page_pool);
+
 	lan966x_fdma_wakeup_netdev(lan966x);
 	napi_enable(&lan966x->napi);
 
 	return err;
 restore:
+	lan966x->rx.page_pool = page_pool;
 	lan966x->rx.dma = rx_dma;
 	lan966x->rx.dcbs = rx_dcbs;
 	lan966x_fdma_rx_start(&lan966x->rx);
@@ -876,5 +881,6 @@ void lan966x_fdma_deinit(struct lan966x *lan966x)
 
 	lan966x_fdma_rx_free_pages(&lan966x->rx);
 	lan966x_fdma_rx_free(&lan966x->rx);
+	page_pool_destroy(lan966x->rx.page_pool);
 	lan966x_fdma_tx_free(&lan966x->tx);
 }
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index 18f727f397fd6..97652a73017a2 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -9,6 +9,7 @@
 #include <linux/phy.h>
 #include <linux/phylink.h>
 #include <linux/ptp_clock_kernel.h>
+#include <net/page_pool.h>
 #include <net/pkt_cls.h>
 #include <net/pkt_sched.h>
 #include <net/switchdev.h>
@@ -162,6 +163,8 @@ struct lan966x_rx {
 	u8 page_order;
 
 	u8 channel_id;
+
+	struct page_pool *page_pool;
 };
 
 struct lan966x_tx_dcb_buf {
-- 
2.38.0

Re: [PATCH net-next v2 4/4] net: lan96x: Use page_pool API

[Alexander Lobakin]

From: Horatiu Vultur <horatiu.vultur@microchip.com>
Date: Sun, 6 Nov 2022 22:11:54 +0100

> Use the page_pool API for allocation, freeing and DMA handling instead
> of dev_alloc_pages, __free_pages and dma_map_page.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  .../net/ethernet/microchip/lan966x/Kconfig    |  1 +
>  .../ethernet/microchip/lan966x/lan966x_fdma.c | 72 ++++++++++---------
>  .../ethernet/microchip/lan966x/lan966x_main.h |  3 +
>  3 files changed, 43 insertions(+), 33 deletions(-)

[...]

> @@ -84,6 +62,27 @@ static void lan966x_fdma_rx_add_dcb(struct lan966x_rx *rx,
>  	rx->last_entry = dcb;
>  }
>  
> +static int lan966x_fdma_rx_alloc_page_pool(struct lan966x_rx *rx)
> +{
> +	struct lan966x *lan966x = rx->lan966x;
> +	struct page_pool_params pp_params = {
> +		.order = rx->page_order,
> +		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
> +		.pool_size = FDMA_DCB_MAX,
> +		.nid = NUMA_NO_NODE,
> +		.dev = lan966x->dev,
> +		.dma_dir = DMA_FROM_DEVICE,
> +		.offset = 0,
> +		.max_len = PAGE_SIZE << rx->page_order,

::max_len's primary purpose is to save time on DMA syncs.
First of all, you can substract
`SKB_DATA_ALIGN(sizeof(struct skb_shared_info))`, your HW never
writes to those last couple hundred bytes.
But I suggest calculating ::max_len basing on your current MTU
value. Let's say you have 16k pages and MTU of 1500, that is a huge
difference (except your DMA is always coherent, but I assume that's
not the case).

In lan966x_fdma_change_mtu() you do:

	max_mtu = lan966x_fdma_get_max_mtu(lan966x);
	max_mtu += IFH_LEN_BYTES;
	max_mtu += SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
	max_mtu += VLAN_HLEN * 2;

`lan966x_fdma_get_max_mtu(lan966x) + IFH_LEN_BYTES + VLAN_HLEN * 2`
(ie 1536 for the MTU of 1500) is your max_len value actually, given
that you don't reserve any headroom (which is unfortunate, but I
guess you're working on this already, since XDP requires
%XDP_PACKET_HEADROOM).

> +	};
> +
> +	rx->page_pool = page_pool_create(&pp_params);
> +	if (IS_ERR(rx->page_pool))
> +		return PTR_ERR(rx->page_pool);
> +
> +	return 0;

	return PTR_ERR_OR_ZERO(rx->page_pool);

> +}
> +
>  static int lan966x_fdma_rx_alloc(struct lan966x_rx *rx)
>  {
>  	struct lan966x *lan966x = rx->lan966x;

[...]

> -- 
> 2.38.0

Thanks,
Olek

Re: [PATCH net-next v2 4/4] net: lan96x: Use page_pool API

[Horatiu Vultur]

The 11/07/2022 17:40, Alexander Lobakin wrote:

Hi Olek,

> 
> From: Horatiu Vultur <horatiu.vultur@microchip.com>
> Date: Sun, 6 Nov 2022 22:11:54 +0100
> 
> > Use the page_pool API for allocation, freeing and DMA handling instead
> > of dev_alloc_pages, __free_pages and dma_map_page.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  .../net/ethernet/microchip/lan966x/Kconfig    |  1 +
> >  .../ethernet/microchip/lan966x/lan966x_fdma.c | 72 ++++++++++---------
> >  .../ethernet/microchip/lan966x/lan966x_main.h |  3 +
> >  3 files changed, 43 insertions(+), 33 deletions(-)
> 
> [...]
> 
> > @@ -84,6 +62,27 @@ static void lan966x_fdma_rx_add_dcb(struct lan966x_rx *rx,
> >       rx->last_entry = dcb;
> >  }
> >
> > +static int lan966x_fdma_rx_alloc_page_pool(struct lan966x_rx *rx)
> > +{
> > +     struct lan966x *lan966x = rx->lan966x;
> > +     struct page_pool_params pp_params = {
> > +             .order = rx->page_order,
> > +             .flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
> > +             .pool_size = FDMA_DCB_MAX,
> > +             .nid = NUMA_NO_NODE,
> > +             .dev = lan966x->dev,
> > +             .dma_dir = DMA_FROM_DEVICE,
> > +             .offset = 0,
> > +             .max_len = PAGE_SIZE << rx->page_order,
> 
> ::max_len's primary purpose is to save time on DMA syncs.
> First of all, you can substract
> `SKB_DATA_ALIGN(sizeof(struct skb_shared_info))`, your HW never
> writes to those last couple hundred bytes.
> But I suggest calculating ::max_len basing on your current MTU
> value. Let's say you have 16k pages and MTU of 1500, that is a huge
> difference (except your DMA is always coherent, but I assume that's
> not the case).
> 
> In lan966x_fdma_change_mtu() you do:
> 
>         max_mtu = lan966x_fdma_get_max_mtu(lan966x);
>         max_mtu += IFH_LEN_BYTES;
>         max_mtu += SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>         max_mtu += VLAN_HLEN * 2;
> 
> `lan966x_fdma_get_max_mtu(lan966x) + IFH_LEN_BYTES + VLAN_HLEN * 2`
> (ie 1536 for the MTU of 1500) is your max_len value actually, given
> that you don't reserve any headroom (which is unfortunate, but I
> guess you're working on this already, since XDP requires
> %XDP_PACKET_HEADROOM).

Thanks for the suggestion. I will try it.
Regarding XDP_PACKET_HEADROOM, for the XDP_DROP, I didn't see it to be
needed. Once the support for XDP_TX or XDP_REDIRECT is added, then yes I
need to reserve also the headroom.

> 
> > +     };
> > +
> > +     rx->page_pool = page_pool_create(&pp_params);
> > +     if (IS_ERR(rx->page_pool))
> > +             return PTR_ERR(rx->page_pool);
> > +
> > +     return 0;
> 
>         return PTR_ERR_OR_ZERO(rx->page_pool);

Yes, I will use this.

> 
> > +}
> > +
> >  static int lan966x_fdma_rx_alloc(struct lan966x_rx *rx)
> >  {
> >       struct lan966x *lan966x = rx->lan966x;
> 
> [...]
> 
> > --
> > 2.38.0
> 
> Thanks,
> Olek

-- 
/Horatiu

Re: [PATCH net-next v2 4/4] net: lan96x: Use page_pool API

[Alexander Lobakin]

From: Horatiu Vultur <horatiu.vultur@microchip.com>
Date: Mon, 7 Nov 2022 22:35:21 +0100

> The 11/07/2022 17:40, Alexander Lobakin wrote:
> 
> Hi Olek,
> 
> > 
> > From: Horatiu Vultur <horatiu.vultur@microchip.com>
> > Date: Sun, 6 Nov 2022 22:11:54 +0100
> > 
> > > Use the page_pool API for allocation, freeing and DMA handling instead
> > > of dev_alloc_pages, __free_pages and dma_map_page.
> > >
> > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > > ---
> > >  .../net/ethernet/microchip/lan966x/Kconfig    |  1 +
> > >  .../ethernet/microchip/lan966x/lan966x_fdma.c | 72 ++++++++++---------
> > >  .../ethernet/microchip/lan966x/lan966x_main.h |  3 +
> > >  3 files changed, 43 insertions(+), 33 deletions(-)
> > 
> > [...]
> > 
> > > @@ -84,6 +62,27 @@ static void lan966x_fdma_rx_add_dcb(struct lan966x_rx *rx,
> > >       rx->last_entry = dcb;
> > >  }
> > >
> > > +static int lan966x_fdma_rx_alloc_page_pool(struct lan966x_rx *rx)
> > > +{
> > > +     struct lan966x *lan966x = rx->lan966x;
> > > +     struct page_pool_params pp_params = {
> > > +             .order = rx->page_order,
> > > +             .flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
> > > +             .pool_size = FDMA_DCB_MAX,
> > > +             .nid = NUMA_NO_NODE,
> > > +             .dev = lan966x->dev,
> > > +             .dma_dir = DMA_FROM_DEVICE,
> > > +             .offset = 0,
> > > +             .max_len = PAGE_SIZE << rx->page_order,
> > 
> > ::max_len's primary purpose is to save time on DMA syncs.
> > First of all, you can substract
> > `SKB_DATA_ALIGN(sizeof(struct skb_shared_info))`, your HW never
> > writes to those last couple hundred bytes.
> > But I suggest calculating ::max_len basing on your current MTU
> > value. Let's say you have 16k pages and MTU of 1500, that is a huge
> > difference (except your DMA is always coherent, but I assume that's
> > not the case).
> > 
> > In lan966x_fdma_change_mtu() you do:
> > 
> >         max_mtu = lan966x_fdma_get_max_mtu(lan966x);
> >         max_mtu += IFH_LEN_BYTES;
> >         max_mtu += SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> >         max_mtu += VLAN_HLEN * 2;
> > 
> > `lan966x_fdma_get_max_mtu(lan966x) + IFH_LEN_BYTES + VLAN_HLEN * 2`
> > (ie 1536 for the MTU of 1500) is your max_len value actually, given
> > that you don't reserve any headroom (which is unfortunate, but I
> > guess you're working on this already, since XDP requires
> > %XDP_PACKET_HEADROOM).
> 
> Thanks for the suggestion. I will try it.
> Regarding XDP_PACKET_HEADROOM, for the XDP_DROP, I didn't see it to be
> needed. Once the support for XDP_TX or XDP_REDIRECT is added, then yes I
> need to reserve also the headroom.

Correct, since you're disabling metadata support in
xdp_prepare_buff(), headroom is not needed for pass and drop
actions.

It's always good to have at least %NET_SKB_PAD headroom inside an
skb, so that networking stack won't perform excessive reallocations,
and your code currently misses that -- if I understand currently,
after converting hardware-specific header to an Ethernet header you
have 28 - 14 = 14 bytes of headroom, which sometimes can be not
enough for example for forwarding cases. It's not related to XDP,
but I would do that as a prerequisite patch for Tx/redirect, since
you'll be adding headroom support anyway :)

> 
> > 
> > > +     };
> > > +
> > > +     rx->page_pool = page_pool_create(&pp_params);
> > > +     if (IS_ERR(rx->page_pool))
> > > +             return PTR_ERR(rx->page_pool);

[...]

> > > --
> > > 2.38.0
> > 
> > Thanks,
> > Olek
> 
> -- 
> /Horatiu

Thanks,
Olek

Re: [PATCH net-next v2 4/4] net: lan96x: Use page_pool API

[Horatiu Vultur]

The 11/08/2022 12:33, Alexander Lobakin wrote:
> 
> From: Horatiu Vultur <horatiu.vultur@microchip.com>
> Date: Mon, 7 Nov 2022 22:35:21 +0100
> 
> > The 11/07/2022 17:40, Alexander Lobakin wrote:
> >
> > Hi Olek,
> >
> > >
> > > From: Horatiu Vultur <horatiu.vultur@microchip.com>
> > > Date: Sun, 6 Nov 2022 22:11:54 +0100
> > >
> > > > Use the page_pool API for allocation, freeing and DMA handling instead
> > > > of dev_alloc_pages, __free_pages and dma_map_page.
> > > >
> > > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > > > ---
> > > >  .../net/ethernet/microchip/lan966x/Kconfig    |  1 +
> > > >  .../ethernet/microchip/lan966x/lan966x_fdma.c | 72 ++++++++++---------
> > > >  .../ethernet/microchip/lan966x/lan966x_main.h |  3 +
> > > >  3 files changed, 43 insertions(+), 33 deletions(-)
> > >
> > > [...]
> > >
> > > > @@ -84,6 +62,27 @@ static void lan966x_fdma_rx_add_dcb(struct lan966x_rx *rx,
> > > >       rx->last_entry = dcb;
> > > >  }
> > > >
> > > > +static int lan966x_fdma_rx_alloc_page_pool(struct lan966x_rx *rx)
> > > > +{
> > > > +     struct lan966x *lan966x = rx->lan966x;
> > > > +     struct page_pool_params pp_params = {
> > > > +             .order = rx->page_order,
> > > > +             .flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
> > > > +             .pool_size = FDMA_DCB_MAX,
> > > > +             .nid = NUMA_NO_NODE,
> > > > +             .dev = lan966x->dev,
> > > > +             .dma_dir = DMA_FROM_DEVICE,
> > > > +             .offset = 0,
> > > > +             .max_len = PAGE_SIZE << rx->page_order,
> > >
> > > ::max_len's primary purpose is to save time on DMA syncs.
> > > First of all, you can substract
> > > `SKB_DATA_ALIGN(sizeof(struct skb_shared_info))`, your HW never
> > > writes to those last couple hundred bytes.
> > > But I suggest calculating ::max_len basing on your current MTU
> > > value. Let's say you have 16k pages and MTU of 1500, that is a huge
> > > difference (except your DMA is always coherent, but I assume that's
> > > not the case).
> > >
> > > In lan966x_fdma_change_mtu() you do:
> > >
> > >         max_mtu = lan966x_fdma_get_max_mtu(lan966x);
> > >         max_mtu += IFH_LEN_BYTES;
> > >         max_mtu += SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> > >         max_mtu += VLAN_HLEN * 2;
> > >
> > > `lan966x_fdma_get_max_mtu(lan966x) + IFH_LEN_BYTES + VLAN_HLEN * 2`
> > > (ie 1536 for the MTU of 1500) is your max_len value actually, given
> > > that you don't reserve any headroom (which is unfortunate, but I
> > > guess you're working on this already, since XDP requires
> > > %XDP_PACKET_HEADROOM).
> >
> > Thanks for the suggestion. I will try it.
> > Regarding XDP_PACKET_HEADROOM, for the XDP_DROP, I didn't see it to be
> > needed. Once the support for XDP_TX or XDP_REDIRECT is added, then yes I
> > need to reserve also the headroom.
> 
> Correct, since you're disabling metadata support in
> xdp_prepare_buff(), headroom is not needed for pass and drop
> actions.
> 
> It's always good to have at least %NET_SKB_PAD headroom inside an
> skb, so that networking stack won't perform excessive reallocations,
> and your code currently misses that -- if I understand currently,
> after converting hardware-specific header to an Ethernet header you
> have 28 - 14 = 14 bytes of headroom, which sometimes can be not
> enough for example for forwarding cases. It's not related to XDP,
> but I would do that as a prerequisite patch for Tx/redirect, since
> you'll be adding headroom support anyway :)

Just a small comment here. There is no need to convert hardware-specific
header, because after that header there is the ethernet header. So I
would have 28 bytes left for headroom, but that is still less then
NET_SKB_PAD.
But I got the idea. When I will add the Tx/redirect, one of those
patches will be to make sure we have enough headroom.

> 
> >
> > >
> > > > +     };
> > > > +
> > > > +     rx->page_pool = page_pool_create(&pp_params);
> > > > +     if (IS_ERR(rx->page_pool))
> > > > +             return PTR_ERR(rx->page_pool);
> 
> [...]
> 
> > > > --
> > > > 2.38.0
> > >
> > > Thanks,
> > > Olek
> >
> > --
> > /Horatiu
> 
> Thanks,
> Olek

-- 
/Horatiu