diff mbox series

[net-next,13/13] net: fec: fec_enet_rx_queue(): factor out VLAN handling into separate function fec_enet_rx_vlan()

Message ID 20241016-fec-cleanups-v1-13-de783bd15e6a@pengutronix.de (mailing list archive)
State New
Headers show
Series net: fec: cleanups, update quirk, update IRQ naming | expand

Commit Message

Marc Kleine-Budde Oct. 16, 2024, 9:52 p.m. UTC 
In order to clean up of the VLAN handling, factor out the VLAN
handling into separate function fec_enet_rx_vlan().

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec_main.c | 32 ++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

Comments

Frank Li Oct. 17, 2024, 3:07 a.m. UTC | #1 
On Wed, Oct 16, 2024 at 11:52:01PM +0200, Marc Kleine-Budde wrote:
> In order to clean up of the VLAN handling, factor out the VLAN
> handling into separate function fec_enet_rx_vlan().
>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/net/ethernet/freescale/fec_main.c | 32 ++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index d9415c7c16cea3fc3d91e198c21af9fe9e21747e..e14000ba85586b9cd73151e62924c3b4597bb580 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1672,6 +1672,22 @@ fec_enet_run_xdp(struct fec_enet_private *fep, struct bpf_prog *prog,
>  	return ret;
>  }
>
> +static void fec_enet_rx_vlan(struct net_device *ndev, struct sk_buff *skb)
> +{
> +	struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
> +
> +	if (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) {
> +		/* Push and remove the vlan tag */
> +		u16 vlan_tag = ntohs(vlan_header->h_vlan_TCI);
> +
> +		memmove(skb->data + VLAN_HLEN, skb->data, ETH_ALEN * 2);
> +		skb_pull(skb, VLAN_HLEN);
> +		__vlan_hwaccel_put_tag(skb,
> +				       htons(ETH_P_8021Q),
> +				       vlan_tag);
> +	}
> +}
> +
>  /* 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,
> @@ -1812,19 +1828,9 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
>  			ebdp = (struct bufdesc_ex *)bdp;
>
>  		/* If this is a VLAN packet remove the VLAN Tag */
> -		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
> -		    fep->bufdesc_ex &&
> -		    (ebdp->cbd_esc & cpu_to_fec32(BD_ENET_RX_VLAN))) {
> -			/* Push and remove the vlan tag */
> -			struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
> -			u16 vlan_tag = ntohs(vlan_header->h_vlan_TCI);
> -
> -			memmove(skb->data + VLAN_HLEN, skb->data, ETH_ALEN * 2);
> -			skb_pull(skb, VLAN_HLEN);
> -			__vlan_hwaccel_put_tag(skb,
> -					       htons(ETH_P_8021Q),
> -					       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);
>
>
> --
> 2.45.2
>
>
Wei Fang Oct. 17, 2024, 3:33 a.m. UTC | #2 
> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2024年10月17日 5:52
> To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>;
> Clark Wang <xiaoning.wang@nxp.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard
> Cochran <richardcochran@gmail.com>
> Cc: imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de; Marc Kleine-Budde <mkl@pengutronix.de>
> Subject: [PATCH net-next 13/13] net: fec: fec_enet_rx_queue(): factor out
> VLAN handling into separate function fec_enet_rx_vlan()
> 
> In order to clean up of the VLAN handling, factor out the VLAN
> handling into separate function fec_enet_rx_vlan().
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 32
> ++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index
> d9415c7c16cea3fc3d91e198c21af9fe9e21747e..e14000ba85586b9cd73151e
> 62924c3b4597bb580 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1672,6 +1672,22 @@ fec_enet_run_xdp(struct fec_enet_private *fep,
> struct bpf_prog *prog,
>  	return ret;
>  }
> 
> +static void fec_enet_rx_vlan(struct net_device *ndev, struct sk_buff *skb)
> +{
> +	struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);

Why not move vlan_header into the if statement?

> +
> +	if (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) {
> +		/* Push and remove the vlan tag */
> +		u16 vlan_tag = ntohs(vlan_header->h_vlan_TCI);
> +
> +		memmove(skb->data + VLAN_HLEN, skb->data, ETH_ALEN * 2);
> +		skb_pull(skb, VLAN_HLEN);
> +		__vlan_hwaccel_put_tag(skb,
> +				       htons(ETH_P_8021Q),
> +				       vlan_tag);
> +	}
> +}
> +
>  /* 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,
> @@ -1812,19 +1828,9 @@ fec_enet_rx_queue(struct net_device *ndev, u16
> queue_id, int budget)
>  			ebdp = (struct bufdesc_ex *)bdp;
> 
>  		/* If this is a VLAN packet remove the VLAN Tag */
> -		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
> -		    fep->bufdesc_ex &&
> -		    (ebdp->cbd_esc & cpu_to_fec32(BD_ENET_RX_VLAN))) {
> -			/* Push and remove the vlan tag */
> -			struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
> -			u16 vlan_tag = ntohs(vlan_header->h_vlan_TCI);
> -
> -			memmove(skb->data + VLAN_HLEN, skb->data, ETH_ALEN * 2);
> -			skb_pull(skb, VLAN_HLEN);
> -			__vlan_hwaccel_put_tag(skb,
> -					       htons(ETH_P_8021Q),
> -					       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);
> 
> 
> --
> 2.45.2
>
Marc Kleine-Budde Oct. 17, 2024, 7:09 a.m. UTC | #3 
On 17.10.2024 03:33:09, Wei Fang wrote:
> > -----Original Message-----
> > From: Marc Kleine-Budde <mkl@pengutronix.de>
> > Sent: 2024年10月17日 5:52
> > To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>;
> > Clark Wang <xiaoning.wang@nxp.com>; David S. Miller
> > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard
> > Cochran <richardcochran@gmail.com>
> > Cc: imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> > kernel@pengutronix.de; Marc Kleine-Budde <mkl@pengutronix.de>
> > Subject: [PATCH net-next 13/13] net: fec: fec_enet_rx_queue(): factor out
> > VLAN handling into separate function fec_enet_rx_vlan()
> > 
> > In order to clean up of the VLAN handling, factor out the VLAN
> > handling into separate function fec_enet_rx_vlan().
> > 
> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > ---
> >  drivers/net/ethernet/freescale/fec_main.c | 32
> > ++++++++++++++++++-------------
> >  1 file changed, 19 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/fec_main.c
> > b/drivers/net/ethernet/freescale/fec_main.c
> > index
> > d9415c7c16cea3fc3d91e198c21af9fe9e21747e..e14000ba85586b9cd73151e
> > 62924c3b4597bb580 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -1672,6 +1672,22 @@ fec_enet_run_xdp(struct fec_enet_private *fep,
> > struct bpf_prog *prog,
> >  	return ret;
> >  }
> > 
> > +static void fec_enet_rx_vlan(struct net_device *ndev, struct sk_buff *skb)
> > +{
> > +	struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
> 
> Why not move vlan_header into the if statement?

I've an upcoming patch that adds NETIF_F_HW_VLAN_STAG_RX (a.k.a.
801.2ad, S-VLAN) handling that changes this function.

One hunk looks like this, it uses the vlan_header outside of the if:

@@ -1675,15 +1678,19 @@ fec_enet_run_xdp(struct fec_enet_private *fep, struct bpf_prog *prog,
 static void fec_enet_rx_vlan(struct net_device *ndev, struct sk_buff *skb)
 {
         struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
+        __be16 vlan_proto = vlan_header->h_vlan_proto;
 
-        if (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) {
+        if ((vlan_proto == htons(ETH_P_8021Q) &&
+             ndev->features & NETIF_F_HW_VLAN_CTAG_RX) ||
+            (vlan_proto == htons(ETH_P_8021AD) &&
+             ndev->features & NETIF_F_HW_VLAN_STAG_RX)) {
                 /* Push and remove the vlan tag */
                 u16 vlan_tag = ntohs(vlan_header->h_vlan_TCI);

regards,
Marc
Wei Fang Oct. 17, 2024, 7:48 a.m. UTC | #4 
> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2024年10月17日 15:09
> To: Wei Fang <wei.fang@nxp.com>
> Cc: Shenwei Wang <shenwei.wang@nxp.com>; Clark Wang
> <xiaoning.wang@nxp.com>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; Richard Cochran <richardcochran@gmail.com>;
> imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de
> Subject: Re: RE: [PATCH net-next 13/13] net: fec: fec_enet_rx_queue(): factor
> out VLAN handling into separate function fec_enet_rx_vlan()
> 
> On 17.10.2024 03:33:09, Wei Fang wrote:
> > > -----Original Message-----
> > > From: Marc Kleine-Budde <mkl@pengutronix.de>
> > > Sent: 2024年10月17日 5:52
> > > To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang
> <shenwei.wang@nxp.com>;
> > > Clark Wang <xiaoning.wang@nxp.com>; David S. Miller
> > > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> > > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard
> > > Cochran <richardcochran@gmail.com>
> > > Cc: imx@lists.linux.dev; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org;
> > > kernel@pengutronix.de; Marc Kleine-Budde <mkl@pengutronix.de>
> > > Subject: [PATCH net-next 13/13] net: fec: fec_enet_rx_queue(): factor out
> > > VLAN handling into separate function fec_enet_rx_vlan()
> > >
> > > In order to clean up of the VLAN handling, factor out the VLAN
> > > handling into separate function fec_enet_rx_vlan().
> > >
> > > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > > ---
> > >  drivers/net/ethernet/freescale/fec_main.c | 32
> > > ++++++++++++++++++-------------
> > >  1 file changed, 19 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/freescale/fec_main.c
> > > b/drivers/net/ethernet/freescale/fec_main.c
> > > index
> > >
> d9415c7c16cea3fc3d91e198c21af9fe9e21747e..e14000ba85586b9cd73151e
> > > 62924c3b4597bb580 100644
> > > --- a/drivers/net/ethernet/freescale/fec_main.c
> > > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > > @@ -1672,6 +1672,22 @@ fec_enet_run_xdp(struct fec_enet_private
> *fep,
> > > struct bpf_prog *prog,
> > >  	return ret;
> > >  }
> > >
> > > +static void fec_enet_rx_vlan(struct net_device *ndev, struct sk_buff *skb)
> > > +{
> > > +	struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
> >
> > Why not move vlan_header into the if statement?
> 
> I've an upcoming patch that adds NETIF_F_HW_VLAN_STAG_RX (a.k.a.
> 801.2ad, S-VLAN) handling that changes this function.
> 
> One hunk looks like this, it uses the vlan_header outside of the if:
> 

You can move valn_header out of the 'if' when it happens.

> @@ -1675,15 +1678,19 @@ fec_enet_run_xdp(struct fec_enet_private *fep,
> struct bpf_prog *prog,
>  static void fec_enet_rx_vlan(struct net_device *ndev, struct sk_buff *skb)
>  {
>          struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
> +        __be16 vlan_proto = vlan_header->h_vlan_proto;
> 
> -        if (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) {
> +        if ((vlan_proto == htons(ETH_P_8021Q) &&
> +             ndev->features & NETIF_F_HW_VLAN_CTAG_RX) ||
> +            (vlan_proto == htons(ETH_P_8021AD) &&
> +             ndev->features & NETIF_F_HW_VLAN_STAG_RX)) {
>                  /* Push and remove the vlan tag */
>                  u16 vlan_tag = ntohs(vlan_header->h_vlan_TCI);
> 
> 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   |
Marc Kleine-Budde Oct. 18, 2024, 7:21 a.m. UTC | #5 
On 17.10.2024 07:48:34, Wei Fang wrote:
> > > Why not move vlan_header into the if statement?
> > 
> > I've an upcoming patch that adds NETIF_F_HW_VLAN_STAG_RX (a.k.a.
> > 801.2ad, S-VLAN) handling that changes this function.
> > 
> > One hunk looks like this, it uses the vlan_header outside of the if:
> > 
> 
> You can move valn_header out of the 'if' when it happens.

Ok.

regards,
Marc
diff mbox series

Patch

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index d9415c7c16cea3fc3d91e198c21af9fe9e21747e..e14000ba85586b9cd73151e62924c3b4597bb580 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1672,6 +1672,22 @@  fec_enet_run_xdp(struct fec_enet_private *fep, struct bpf_prog *prog,
 	return ret;
 }
 
+static void fec_enet_rx_vlan(struct net_device *ndev, struct sk_buff *skb)
+{
+	struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
+
+	if (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) {
+		/* Push and remove the vlan tag */
+		u16 vlan_tag = ntohs(vlan_header->h_vlan_TCI);
+
+		memmove(skb->data + VLAN_HLEN, skb->data, ETH_ALEN * 2);
+		skb_pull(skb, VLAN_HLEN);
+		__vlan_hwaccel_put_tag(skb,
+				       htons(ETH_P_8021Q),
+				       vlan_tag);
+	}
+}
+
 /* 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,
@@ -1812,19 +1828,9 @@  fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
 			ebdp = (struct bufdesc_ex *)bdp;
 
 		/* If this is a VLAN packet remove the VLAN Tag */
-		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
-		    fep->bufdesc_ex &&
-		    (ebdp->cbd_esc & cpu_to_fec32(BD_ENET_RX_VLAN))) {
-			/* Push and remove the vlan tag */
-			struct vlan_ethhdr *vlan_header = skb_vlan_eth_hdr(skb);
-			u16 vlan_tag = ntohs(vlan_header->h_vlan_TCI);
-
-			memmove(skb->data + VLAN_HLEN, skb->data, ETH_ALEN * 2);
-			skb_pull(skb, VLAN_HLEN);
-			__vlan_hwaccel_put_tag(skb,
-					       htons(ETH_P_8021Q),
-					       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);