[PATCH] net: fec: fix rxvlan feature

[PATCH] net: fec: fix rxvlan feature

[Michael Grzeschik]

The patch 1b7bde6d659d30f171259cc2dfba8e5dab34e735
"net: fec: implement rx_copybreak to improve rx performance"
changed the code path for the vlan check in fec_enet_rx_queue:

 @@ -1417,62 +1486,48 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
 		/* If this is a VLAN packet remove the VLAN Tag */
 		vlan_packet_rcvd = false;
 		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
 			fep->bufdesc_ex && (ebdp->cbd_esc & BD_ENET_RX_VLAN)) {
 			/* Push and remove the vlan tag */
                         struct vlan_hdr *vlan_header =
                                         (struct vlan_hdr *) (data + ETH_HLEN);
                         vlan_tag = ntohs(vlan_header->h_vlan_TCI);
 -                       pkt_len -= VLAN_HLEN;

                         vlan_packet_rcvd = true;
 +
 +                       skb_copy_to_linear_data_offset(skb, VLAN_HLEN,
 +                                                      data, (2 * ETH_ALEN));
 +                       skb_pull(skb, VLAN_HLEN);
                 }

With the call of skb_copy_to_linear_data_offset the code here is doing more
than previously and is breaking the rxvlan feature. This patch removes this
call to fix it.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec_main.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 9bb6220..77dd539 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1479,8 +1479,6 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
 
 			vlan_packet_rcvd = true;
 
-			skb_copy_to_linear_data_offset(skb, VLAN_HLEN,
-						       data, (2 * ETH_ALEN));
 			skb_pull(skb, VLAN_HLEN);
 		}
 
-- 
2.1.4

Re: [PATCH] net: fec: fix rxvlan feature

[Michael Grzeschik]

On Mon, Mar 09, 2015 at 05:55:53PM +0100, Michael Grzeschik wrote:
> The patch 1b7bde6d659d30f171259cc2dfba8e5dab34e735
> "net: fec: implement rx_copybreak to improve rx performance"
> changed the code path for the vlan check in fec_enet_rx_queue:
> 
>  @@ -1417,62 +1486,48 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
>  		/* If this is a VLAN packet remove the VLAN Tag */
>  		vlan_packet_rcvd = false;
>  		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
>  			fep->bufdesc_ex && (ebdp->cbd_esc & BD_ENET_RX_VLAN)) {
>  			/* Push and remove the vlan tag */
>                          struct vlan_hdr *vlan_header =
>                                          (struct vlan_hdr *) (data + ETH_HLEN);
>                          vlan_tag = ntohs(vlan_header->h_vlan_TCI);
>  -                       pkt_len -= VLAN_HLEN;
> 
>                          vlan_packet_rcvd = true;
>  +
>  +                       skb_copy_to_linear_data_offset(skb, VLAN_HLEN,
>  +                                                      data, (2 * ETH_ALEN));
>  +                       skb_pull(skb, VLAN_HLEN);
>                  }
> 
> With the call of skb_copy_to_linear_data_offset the code here is doing more
> than previously and is breaking the rxvlan feature. This patch removes this
> call to fix it.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 9bb6220..77dd539 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1479,8 +1479,6 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
>  
>  			vlan_packet_rcvd = true;
>  
> -			skb_copy_to_linear_data_offset(skb, VLAN_HLEN,
> -						       data, (2 * ETH_ALEN));

I was reviewing the surrounding code of the mentioned code. AFAIK I am
pretty sure that this had ment to be another call: (to vs. from)

+			skb_copy_from_linear_data_offset(skb, VLAN_HLEN,
+						       data, (2 * ETH_ALEN));

I will test it tomorrow morning, as I currently have no hardware for
today.

Regards,
Michael

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [PATCH] net: fec: fix rxvlan feature

[fugang.duan]

From: Michael Grzeschik <m.grzeschik@pengutronix.de> Sent: Tuesday, March 10, 2015 12:56 AM
> To: Duan Fugang-B38611
> Cc: davem@davemloft.net; Li Frank-B20596; netdev@vger.kernel.org;
> kernel@pengutronix.de
> Subject: [PATCH] net: fec: fix rxvlan feature
> 
> The patch 1b7bde6d659d30f171259cc2dfba8e5dab34e735
> "net: fec: implement rx_copybreak to improve rx performance"
> changed the code path for the vlan check in fec_enet_rx_queue:
> 
>  @@ -1417,62 +1486,48 @@ fec_enet_rx_queue(struct net_device *ndev, int
> budget, u16 queue_id)
>  		/* If this is a VLAN packet remove the VLAN Tag */
>  		vlan_packet_rcvd = false;
>  		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
>  			fep->bufdesc_ex && (ebdp->cbd_esc & BD_ENET_RX_VLAN)) {
>  			/* Push and remove the vlan tag */
>                          struct vlan_hdr *vlan_header =
>                                          (struct vlan_hdr *) (data +
> ETH_HLEN);
>                          vlan_tag = ntohs(vlan_header->h_vlan_TCI);
>  -                       pkt_len -= VLAN_HLEN;
> 
>                          vlan_packet_rcvd = true;  +
>  +                       skb_copy_to_linear_data_offset(skb, VLAN_HLEN,
>  +                                                      data, (2 *
> ETH_ALEN));
>  +                       skb_pull(skb, VLAN_HLEN);
>                  }
> 
> With the call of skb_copy_to_linear_data_offset the code here is doing
> more than previously and is breaking the rxvlan feature. This patch
> removes this call to fix it.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 9bb6220..77dd539 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1479,8 +1479,6 @@ fec_enet_rx_queue(struct net_device *ndev, int
> budget, u16 queue_id)
> 
>  			vlan_packet_rcvd = true;
> 
> -			skb_copy_to_linear_data_offset(skb, VLAN_HLEN,
> -						       data, (2 * ETH_ALEN));
>  			skb_pull(skb, VLAN_HLEN);
>  		}
> 
> --
> 2.1.4

The change doesn't work for enet VLAN.

I remember the current enet VLAN can work for some cases like run PTP VALN case.
I don't know why it cannot work for your test.

Regards,
Andy

Re: [PATCH] net: fec: fix rxvlan feature

[David Miller]

From: Michael Grzeschik <m.grzeschik@pengutronix.de>
Date: Mon,  9 Mar 2015 17:55:53 +0100

> The patch 1b7bde6d659d30f171259cc2dfba8e5dab34e735
> "net: fec: implement rx_copybreak to improve rx performance"
> changed the code path for the vlan check in fec_enet_rx_queue:
> 
>  @@ -1417,62 +1486,48 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
>  		/* If this is a VLAN packet remove the VLAN Tag */
>  		vlan_packet_rcvd = false;
>  		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
>  			fep->bufdesc_ex && (ebdp->cbd_esc & BD_ENET_RX_VLAN)) {
>  			/* Push and remove the vlan tag */
>                          struct vlan_hdr *vlan_header =
>                                          (struct vlan_hdr *) (data + ETH_HLEN);
>                          vlan_tag = ntohs(vlan_header->h_vlan_TCI);
>  -                       pkt_len -= VLAN_HLEN;
> 
>                          vlan_packet_rcvd = true;
>  +
>  +                       skb_copy_to_linear_data_offset(skb, VLAN_HLEN,
>  +                                                      data, (2 * ETH_ALEN));
>  +                       skb_pull(skb, VLAN_HLEN);
>                  }
> 
> With the call of skb_copy_to_linear_data_offset the code here is doing more
> than previously and is breaking the rxvlan feature. This patch removes this
> call to fix it.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

But don't we want to copy the proper header there in the linear case?

I'd rather hear from the original author why they put that copy
there before it just gets blindly removed.

And you'll need to update your commit message with more explanation
once that discussion occurs.

RE: [PATCH] net: fec: fix rxvlan feature

[fugang.duan]

From: David Miller <davem@davemloft.net> Sent: Tuesday, March 10, 2015 10:43 AM
> To: m.grzeschik@pengutronix.de
> Cc: Duan Fugang-B38611; Li Frank-B20596; netdev@vger.kernel.org;
> kernel@pengutronix.de
> Subject: Re: [PATCH] net: fec: fix rxvlan feature
> 
> From: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Date: Mon,  9 Mar 2015 17:55:53 +0100
> 
> > The patch 1b7bde6d659d30f171259cc2dfba8e5dab34e735
> > "net: fec: implement rx_copybreak to improve rx performance"
> > changed the code path for the vlan check in fec_enet_rx_queue:
> >
> >  @@ -1417,62 +1486,48 @@ fec_enet_rx_queue(struct net_device *ndev, int
> budget, u16 queue_id)
> >  		/* If this is a VLAN packet remove the VLAN Tag */
> >  		vlan_packet_rcvd = false;
> >  		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
> >  			fep->bufdesc_ex && (ebdp->cbd_esc & BD_ENET_RX_VLAN)) {
> >  			/* Push and remove the vlan tag */
> >                          struct vlan_hdr *vlan_header =
> >                                          (struct vlan_hdr *) (data +
> ETH_HLEN);
> >                          vlan_tag = ntohs(vlan_header->h_vlan_TCI);
> >  -                       pkt_len -= VLAN_HLEN;
> >
> >                          vlan_packet_rcvd = true;  +
> >  +                       skb_copy_to_linear_data_offset(skb, VLAN_HLEN,
> >  +                                                      data, (2 *
> ETH_ALEN));
> >  +                       skb_pull(skb, VLAN_HLEN);
> >                  }
> >
> > With the call of skb_copy_to_linear_data_offset the code here is doing
> > more than previously and is breaking the rxvlan feature. This patch
> > removes this call to fix it.
> >
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> 
> But don't we want to copy the proper header there in the linear case?
> 
> I'd rather hear from the original author why they put that copy there
> before it just gets blindly removed.
> 
> And you'll need to update your commit message with more explanation once
> that discussion occurs.

There have no function change comparing with previous commit version.
If enable hw VLAN support (sw simulate hw), software remove VLAN tag. 

Below code is previous code base, it also removes the VLAN tag.
-                       /* Extract the frame data without the VLAN header. */
-                       skb_copy_to_linear_data(skb, data, (2 * ETH_ALEN));
-                       if (vlan_packet_rcvd)
-                               payload_offset = (2 * ETH_ALEN) + VLAN_HLEN;
-                       skb_copy_to_linear_data_offset(skb, (2 * ETH_ALEN),
-                                                      data + payload_offset,
-                                                      pkt_len - 4 - (2 * ETH_ALEN));


Does there have un-correct points ? I did run some VLAN test pass.

Regards,
Andy

Re: [PATCH] net: fec: fix rxvlan feature

[Michael Grzeschik]

On Tue, Mar 10, 2015 at 03:29:35AM +0000, fugang.duan@freescale.com wrote:
> From: David Miller <davem@davemloft.net> Sent: Tuesday, March 10, 2015 10:43 AM
> > To: m.grzeschik@pengutronix.de
> > Cc: Duan Fugang-B38611; Li Frank-B20596; netdev@vger.kernel.org;
> > kernel@pengutronix.de
> > Subject: Re: [PATCH] net: fec: fix rxvlan feature
> > 
> > From: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > Date: Mon,  9 Mar 2015 17:55:53 +0100
> > 
> > > The patch 1b7bde6d659d30f171259cc2dfba8e5dab34e735
> > > "net: fec: implement rx_copybreak to improve rx performance"
> > > changed the code path for the vlan check in fec_enet_rx_queue:
> > >
> > >  @@ -1417,62 +1486,48 @@ fec_enet_rx_queue(struct net_device *ndev, int
> > budget, u16 queue_id)
> > >  		/* If this is a VLAN packet remove the VLAN Tag */
> > >  		vlan_packet_rcvd = false;
> > >  		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
> > >  			fep->bufdesc_ex && (ebdp->cbd_esc & BD_ENET_RX_VLAN)) {
> > >  			/* Push and remove the vlan tag */
> > >                          struct vlan_hdr *vlan_header =
> > >                                          (struct vlan_hdr *) (data +
> > ETH_HLEN);
> > >                          vlan_tag = ntohs(vlan_header->h_vlan_TCI);
> > >  -                       pkt_len -= VLAN_HLEN;
> > >
> > >                          vlan_packet_rcvd = true;  +
> > >  +                       skb_copy_to_linear_data_offset(skb, VLAN_HLEN,
> > >  +                                                      data, (2 *
> > ETH_ALEN));
> > >  +                       skb_pull(skb, VLAN_HLEN);
> > >                  }
> > >
> > > With the call of skb_copy_to_linear_data_offset the code here is doing
> > > more than previously and is breaking the rxvlan feature. This patch
> > > removes this call to fix it.
> > >
> > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > 
> > But don't we want to copy the proper header there in the linear case?
> > 
> > I'd rather hear from the original author why they put that copy there
> > before it just gets blindly removed.
> > 
> > And you'll need to update your commit message with more explanation once
> > that discussion occurs.
> 
> There have no function change comparing with previous commit version.
> If enable hw VLAN support (sw simulate hw), software remove VLAN tag. 
> 
> Below code is previous code base, it also removes the VLAN tag.
> -                       /* Extract the frame data without the VLAN header. */
> -                       skb_copy_to_linear_data(skb, data, (2 * ETH_ALEN));
> -                       if (vlan_packet_rcvd)
> -                               payload_offset = (2 * ETH_ALEN) + VLAN_HLEN;
> -                       skb_copy_to_linear_data_offset(skb, (2 * ETH_ALEN),
> -                                                      data + payload_offset,
> -                                                      pkt_len - 4 - (2 * ETH_ALEN));
> 
> 
> Does there have un-correct points ? I did run some VLAN test pass.

Yes, in the previous version the skb_copy_... functions were working
on two different memory pointers. The code was calling netdev_alloc_skb
to create the new memory.

Currently the code is calling this:

data = skb->data;

which leads the skb_copy_... operations, which are no more then memcpy,
to work on the same memory. This can not be valid.

Michael

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [PATCH] net: fec: fix rxvlan feature

[fugang.duan]

From: Michael Grzeschik <mgr@pengutronix.de> Sent: Tuesday, March 10, 2015 5:39 PM
> To: Duan Fugang-B38611
> Cc: David Miller; Li Frank-B20596; netdev@vger.kernel.org;
> kernel@pengutronix.de
> Subject: Re: [PATCH] net: fec: fix rxvlan feature
> 
> 
> On Tue, Mar 10, 2015 at 03:29:35AM +0000, fugang.duan@freescale.com wrote:
> > From: David Miller <davem@davemloft.net> Sent: Tuesday, March 10, 2015
> > 10:43 AM
> > > To: m.grzeschik@pengutronix.de
> > > Cc: Duan Fugang-B38611; Li Frank-B20596; netdev@vger.kernel.org;
> > > kernel@pengutronix.de
> > > Subject: Re: [PATCH] net: fec: fix rxvlan feature
> > >
> > > From: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > > Date: Mon,  9 Mar 2015 17:55:53 +0100
> > >
> > > > The patch 1b7bde6d659d30f171259cc2dfba8e5dab34e735
> > > > "net: fec: implement rx_copybreak to improve rx performance"
> > > > changed the code path for the vlan check in fec_enet_rx_queue:
> > > >
> > > >  @@ -1417,62 +1486,48 @@ fec_enet_rx_queue(struct net_device
> > > > *ndev, int
> > > budget, u16 queue_id)
> > > >  		/* If this is a VLAN packet remove the VLAN Tag */
> > > >  		vlan_packet_rcvd = false;
> > > >  		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
> > > >  			fep->bufdesc_ex && (ebdp->cbd_esc &
> BD_ENET_RX_VLAN)) {
> > > >  			/* Push and remove the vlan tag */
> > > >                          struct vlan_hdr *vlan_header =
> > > >                                          (struct vlan_hdr *) (data
> > > > +
> > > ETH_HLEN);
> > > >                          vlan_tag = ntohs(vlan_header->h_vlan_TCI);
> > > >  -                       pkt_len -= VLAN_HLEN;
> > > >
> > > >                          vlan_packet_rcvd = true;  +
> > > >  +                       skb_copy_to_linear_data_offset(skb,
> VLAN_HLEN,
> > > >  +                                                      data, (2 *
> > > ETH_ALEN));
> > > >  +                       skb_pull(skb, VLAN_HLEN);
> > > >                  }
> > > >
> > > > With the call of skb_copy_to_linear_data_offset the code here is
> > > > doing more than previously and is breaking the rxvlan feature.
> > > > This patch removes this call to fix it.
> > > >
> > > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > >
> > > But don't we want to copy the proper header there in the linear case?
> > >
> > > I'd rather hear from the original author why they put that copy
> > > there before it just gets blindly removed.
> > >
> > > And you'll need to update your commit message with more explanation
> > > once that discussion occurs.
> >
> > There have no function change comparing with previous commit version.
> > If enable hw VLAN support (sw simulate hw), software remove VLAN tag.
> >
> > Below code is previous code base, it also removes the VLAN tag.
> > -                       /* Extract the frame data without the VLAN
> header. */
> > -                       skb_copy_to_linear_data(skb, data, (2 *
> ETH_ALEN));
> > -                       if (vlan_packet_rcvd)
> > -                               payload_offset = (2 * ETH_ALEN) +
> VLAN_HLEN;
> > -                       skb_copy_to_linear_data_offset(skb, (2 *
> ETH_ALEN),
> > -                                                      data +
> payload_offset,
> > -                                                      pkt_len - 4 - (2
> * ETH_ALEN));
> >
> >
> > Does there have un-correct points ? I did run some VLAN test pass.
> 
> Yes, in the previous version the skb_copy_... functions were working on
> two different memory pointers. The code was calling netdev_alloc_skb to
> create the new memory.
> 
> Currently the code is calling this:
> 
> data = skb->data;
> 
> which leads the skb_copy_... operations, which are no more then memcpy,
> to work on the same memory. This can not be valid.
> 
You are right. I will generate one patch for the issue and then pls help for the test with your case.

Regards,
Andy