| Message ID | 1425920153-13319-1-git-send-email-m.grzeschik@pengutronix.de |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); }
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(-)