diff mbox series

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

Message ID 20221106211154.3225784-3-horatiu.vultur@microchip.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: lan966x: Add xdp support | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 1 maintainers not CCed: UNGLinuxDriver@microchip.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 162 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Horatiu Vultur Nov. 6, 2022, 9:11 p.m. UTC 
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(-)

Comments

Alexander Lobakin Nov. 7, 2022, 4:06 p.m. UTC | #1 
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
Horatiu Vultur Nov. 7, 2022, 9:24 p.m. UTC | #2 
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
Alexander Lobakin Nov. 8, 2022, 11:21 a.m. UTC | #3 
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
diff mbox series

Patch

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 {