* RE: [PATCH] vlan_dev: VLAN 0 should be treated as "no vlan tag" (802.1p packet)
From: Vladislav Zolotarov @ 2010-07-08 12:54 UTC (permalink / raw)
To: Pedro Garcia, netdev@vger.kernel.org
Cc: Patrick McHardy, Ben Hutchings, Eric Dumazet
In-Reply-To: <311b59aee7d648c6124a84b5ca06ac60@dondevamos.com>
Margaret,
In order to keep exploring this we need the following data:
1. What is the scenario exactly? What is vMotion? Which kind of operation does it do? Which kind of traffic does it pass?
2. What is the nature of the failure- driver hang? PSOD? Loss of traffic?
3. We need a grcdump after the failure.
Thanks,
vlad
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> Behalf Of Pedro Garcia
> Sent: Wednesday, June 16, 2010 11:49 AM
> To: netdev@vger.kernel.org
> Cc: Patrick McHardy; Ben Hutchings; Eric Dumazet
> Subject: Re: [PATCH] vlan_dev: VLAN 0 should be treated as "no vlan tag"
> (802.1p packet)
>
> On Mon, 14 Jun 2010 21:12:52 +0200, Eric Dumazet <eric.dumazet@gmail.com>
> wrote:
> > Le lundi 14 juin 2010 à 19:11 +0200, Patrick McHardy a écrit :
> >> Ben Hutchings wrote:
> >> > On Mon, 2010-06-14 at 18:49 +0200, Pedro Garcia wrote:
> >> >
> >> >> On Sun, 13 Jun 2010 22:56:30 +0100, Ben Hutchings
> >> >> <bhutchings@solarflare.com> wrote:
> >> >>
> >> >>> I have no particular opinion on this change, but you need to read and
> >> >>> follow Documentation/SubmittingPatches.
> >> >>>
> >> >>> Ben.
> >> >>>
> >> >> Sorry, first kernel patch, and I did not know about it. I resubmit
> >> >> with
> >> >> the correct style / format:
> >> >>
> >> > [...]
> >> >
> >> > Sorry, no you haven't.
> >> >
> >> > - Networking changes go through David Miller's net-next-2.6 tree so you
> >> > need to use that as the baseline, not 2.6.26
> >> > - Patches should be applicable with -p1, not -p0 (so if you use diff,
> >> > you should run it from one directory level up)
> >> > - The patch was word-wrapped
> >>
> >> Additionally:
> >>
> >> - please use the proper comment style, meaning each line begins
> >> with a '*'
> >>
> >> - the pr_debug statements may be useful for debugging, but are
> >> a bit excessive for the final version
> >>
> >> - + /* 2010-06-13: Pedro Garcia
> >>
> >> We have changelogs for this, simply explaining what the code
> >> does is enough.
> >>
> >> - Please CC the maintainer (which is me)
> >> --
> >
> > Pedro, we have two kind of vlan setups :
> >
> > accelerated and non accelerated ones.
> >
> > Your patch address non accelated ones only, since you only touch
> > vlan_skb_recv()
> >
> > Accelerated vlan can follow these paths :
> >
> > 1) NAPI devices
> >
> > vlan_gro_receive() -> vlan_gro_common()
> >
> > 2) non NAPI devices
> >
> > __vlan_hwaccel_rx()
> >
> > So you might also patch __vlan_hwaccel_rx() and vlan_gro_common()
> >
> > Please merge following bits to your patch submission :
> >
> > http://kerneltrap.org/mailarchive/linux-netdev/2010/5/23/6277868
> >
> >
> > Good luck for your first patch !
>
> Here it is again. I added the modifications in
> http://kerneltrap.org/mailarchive/linux-netdev/2010/5/23/6277868 for HW
> accelerated incoming packets (it did not apply cleanly on the last version of
> the kernel, so I applied manually). Now, if the VLAN 0 is not explicitly
> created by the user, VLAN 0 packets will be treated as no VLAN (802.1p
> packets), instead of dropping them.
>
> The patch is now for two files: vlan_core (accel) and vlan_dev (non accel)
>
> I can not test on HW accelerated devices, so if someone can check it I will
> appreciate (even though in the thread above it looked like yes). For non
> accel I tessted in 2.6.26. Now the patch is for
> net-next-2.6, and it compiles OK, but I a have to setup a test environment to
> check it is still OK (should, but better to test).
>
> Signed-off-by: Pedro Garcia <pedro.netdev@dondevamos.com>
> --
> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> index 50f58f5..daaca31 100644
> --- a/net/8021q/vlan_core.c
> +++ b/net/8021q/vlan_core.c
> @@ -8,6 +8,9 @@
> int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
> u16 vlan_tci, int polling)
> {
> + struct net_device *vlan_dev;
> + u16 vlan_id;
> +
> if (netpoll_rx(skb))
> return NET_RX_DROP;
>
> @@ -16,10 +19,14 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct
> vlan_group *grp,
>
> skb->skb_iif = skb->dev->ifindex;
> __vlan_hwaccel_put_tag(skb, vlan_tci);
> - skb->dev = vlan_group_get_device(grp, vlan_tci & VLAN_VID_MASK);
> + vlan_id = vlan_tci & VLAN_VID_MASK;
> + vlan_dev = vlan_group_get_device(grp, vlan_id);
>
> - if (!skb->dev)
> - goto drop;
> + if (vlan_dev)
> + skb->dev = vlan_dev;
> + else
> + if (vlan_id)
> + goto drop;
>
> return (polling ? netif_receive_skb(skb) : netif_rx(skb));
>
> @@ -82,16 +89,22 @@ vlan_gro_common(struct napi_struct *napi, struct
> vlan_group *grp,
> unsigned int vlan_tci, struct sk_buff *skb)
> {
> struct sk_buff *p;
> + struct net_device *vlan_dev;
> + u16 vlan_id;
>
> if (skb_bond_should_drop(skb, ACCESS_ONCE(skb->dev->master)))
> skb->deliver_no_wcard = 1;
>
> skb->skb_iif = skb->dev->ifindex;
> __vlan_hwaccel_put_tag(skb, vlan_tci);
> - skb->dev = vlan_group_get_device(grp, vlan_tci & VLAN_VID_MASK);
> -
> - if (!skb->dev)
> - goto drop;
> + vlan_id = vlan_tci & VLAN_VID_MASK;
> + vlan_dev = vlan_group_get_device(grp, vlan_id);
> +
> + if (vlan_dev)
> + skb->dev = vlan_dev;
> + else
> + if (vlan_id)
> + goto drop;
>
> for (p = napi->gro_list; p; p = p->next) {
> NAPI_GRO_CB(p)->same_flow =
> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index 5298426..65512c3 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -142,6 +142,7 @@ int vlan_skb_recv(struct sk_buff *skb, struct net_device
> *dev,
> {
> struct vlan_hdr *vhdr;
> struct vlan_rx_stats *rx_stats;
> + struct net_device *vlan_dev;
> u16 vlan_id;
> u16 vlan_tci;
>
> @@ -157,8 +158,18 @@ int vlan_skb_recv(struct sk_buff *skb, struct net_device
> *dev,
> vlan_id = vlan_tci & VLAN_VID_MASK;
>
> rcu_read_lock();
> - skb->dev = __find_vlan_dev(dev, vlan_id);
> - if (!skb->dev) {
> + vlan_dev = __find_vlan_dev(dev, vlan_id);
> +
> + /* If the VLAN device is defined, we use it.
> + * If not, and the VID is 0, it is a 802.1p packet (not
> + * really a VLAN), so we will just netif_rx it later to the
> + * original interface, but with the skb->proto set to the
> + * wrapped proto: we do nothing here.
> + */
> +
> + if (vlan_dev) {
> + skb->dev = vlan_dev;
> + } else if (vlan_id) {
> pr_debug("%s: ERROR: No net_device for VID: %u on dev: %s\n",
> __func__, vlan_id, dev->name);
> goto err_unlock;
>
>
> --
> 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
^ permalink raw reply
* RE: [PATCH] vlan_dev: VLAN 0 should be treated as "no vlan tag" (802.1p packet)
From: Vladislav Zolotarov @ 2010-07-08 12:58 UTC (permalink / raw)
To: Vladislav Zolotarov, Pedro Garcia, netdev@vger.kernel.org
Cc: Patrick McHardy, Ben Hutchings, Eric Dumazet
In-Reply-To: <8628FE4E7912BF47A96AE7DD7BAC0AADDDE647018F@SJEXCHCCR02.corp.ad.broadcom.com>
Opps...
Sorry. Wrong thread. :)
Pls., discard!
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> Behalf Of Vladislav Zolotarov
> Sent: Thursday, July 08, 2010 3:55 PM
> To: Pedro Garcia; netdev@vger.kernel.org
> Cc: Patrick McHardy; Ben Hutchings; Eric Dumazet
> Subject: RE: [PATCH] vlan_dev: VLAN 0 should be treated as "no vlan tag"
> (802.1p packet)
>
> Margaret,
> In order to keep exploring this we need the following data:
> 1. What is the scenario exactly? What is vMotion? Which kind of operation
> does it do? Which kind of traffic does it pass?
> 2. What is the nature of the failure- driver hang? PSOD? Loss of traffic?
> 3. We need a grcdump after the failure.
>
> Thanks,
> vlad
>
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> > Behalf Of Pedro Garcia
> > Sent: Wednesday, June 16, 2010 11:49 AM
> > To: netdev@vger.kernel.org
> > Cc: Patrick McHardy; Ben Hutchings; Eric Dumazet
> > Subject: Re: [PATCH] vlan_dev: VLAN 0 should be treated as "no vlan tag"
> > (802.1p packet)
> >
> > On Mon, 14 Jun 2010 21:12:52 +0200, Eric Dumazet <eric.dumazet@gmail.com>
> > wrote:
> > > Le lundi 14 juin 2010 à 19:11 +0200, Patrick McHardy a écrit :
> > >> Ben Hutchings wrote:
> > >> > On Mon, 2010-06-14 at 18:49 +0200, Pedro Garcia wrote:
> > >> >
> > >> >> On Sun, 13 Jun 2010 22:56:30 +0100, Ben Hutchings
> > >> >> <bhutchings@solarflare.com> wrote:
> > >> >>
> > >> >>> I have no particular opinion on this change, but you need to read
> and
> > >> >>> follow Documentation/SubmittingPatches.
> > >> >>>
> > >> >>> Ben.
> > >> >>>
> > >> >> Sorry, first kernel patch, and I did not know about it. I resubmit
> > >> >> with
> > >> >> the correct style / format:
> > >> >>
> > >> > [...]
> > >> >
> > >> > Sorry, no you haven't.
> > >> >
> > >> > - Networking changes go through David Miller's net-next-2.6 tree so
> you
> > >> > need to use that as the baseline, not 2.6.26
> > >> > - Patches should be applicable with -p1, not -p0 (so if you use diff,
> > >> > you should run it from one directory level up)
> > >> > - The patch was word-wrapped
> > >>
> > >> Additionally:
> > >>
> > >> - please use the proper comment style, meaning each line begins
> > >> with a '*'
> > >>
> > >> - the pr_debug statements may be useful for debugging, but are
> > >> a bit excessive for the final version
> > >>
> > >> - + /* 2010-06-13: Pedro Garcia
> > >>
> > >> We have changelogs for this, simply explaining what the code
> > >> does is enough.
> > >>
> > >> - Please CC the maintainer (which is me)
> > >> --
> > >
> > > Pedro, we have two kind of vlan setups :
> > >
> > > accelerated and non accelerated ones.
> > >
> > > Your patch address non accelated ones only, since you only touch
> > > vlan_skb_recv()
> > >
> > > Accelerated vlan can follow these paths :
> > >
> > > 1) NAPI devices
> > >
> > > vlan_gro_receive() -> vlan_gro_common()
> > >
> > > 2) non NAPI devices
> > >
> > > __vlan_hwaccel_rx()
> > >
> > > So you might also patch __vlan_hwaccel_rx() and vlan_gro_common()
> > >
> > > Please merge following bits to your patch submission :
> > >
> > > http://kerneltrap.org/mailarchive/linux-netdev/2010/5/23/6277868
> > >
> > >
> > > Good luck for your first patch !
> >
> > Here it is again. I added the modifications in
> > http://kerneltrap.org/mailarchive/linux-netdev/2010/5/23/6277868 for HW
> > accelerated incoming packets (it did not apply cleanly on the last version
> of
> > the kernel, so I applied manually). Now, if the VLAN 0 is not explicitly
> > created by the user, VLAN 0 packets will be treated as no VLAN (802.1p
> > packets), instead of dropping them.
> >
> > The patch is now for two files: vlan_core (accel) and vlan_dev (non accel)
> >
> > I can not test on HW accelerated devices, so if someone can check it I will
> > appreciate (even though in the thread above it looked like yes). For non
> > accel I tessted in 2.6.26. Now the patch is for
> > net-next-2.6, and it compiles OK, but I a have to setup a test environment
> to
> > check it is still OK (should, but better to test).
> >
> > Signed-off-by: Pedro Garcia <pedro.netdev@dondevamos.com>
> > --
> > diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> > index 50f58f5..daaca31 100644
> > --- a/net/8021q/vlan_core.c
> > +++ b/net/8021q/vlan_core.c
> > @@ -8,6 +8,9 @@
> > int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
> > u16 vlan_tci, int polling)
> > {
> > + struct net_device *vlan_dev;
> > + u16 vlan_id;
> > +
> > if (netpoll_rx(skb))
> > return NET_RX_DROP;
> >
> > @@ -16,10 +19,14 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct
> > vlan_group *grp,
> >
> > skb->skb_iif = skb->dev->ifindex;
> > __vlan_hwaccel_put_tag(skb, vlan_tci);
> > - skb->dev = vlan_group_get_device(grp, vlan_tci & VLAN_VID_MASK);
> > + vlan_id = vlan_tci & VLAN_VID_MASK;
> > + vlan_dev = vlan_group_get_device(grp, vlan_id);
> >
> > - if (!skb->dev)
> > - goto drop;
> > + if (vlan_dev)
> > + skb->dev = vlan_dev;
> > + else
> > + if (vlan_id)
> > + goto drop;
> >
> > return (polling ? netif_receive_skb(skb) : netif_rx(skb));
> >
> > @@ -82,16 +89,22 @@ vlan_gro_common(struct napi_struct *napi, struct
> > vlan_group *grp,
> > unsigned int vlan_tci, struct sk_buff *skb)
> > {
> > struct sk_buff *p;
> > + struct net_device *vlan_dev;
> > + u16 vlan_id;
> >
> > if (skb_bond_should_drop(skb, ACCESS_ONCE(skb->dev->master)))
> > skb->deliver_no_wcard = 1;
> >
> > skb->skb_iif = skb->dev->ifindex;
> > __vlan_hwaccel_put_tag(skb, vlan_tci);
> > - skb->dev = vlan_group_get_device(grp, vlan_tci & VLAN_VID_MASK);
> > -
> > - if (!skb->dev)
> > - goto drop;
> > + vlan_id = vlan_tci & VLAN_VID_MASK;
> > + vlan_dev = vlan_group_get_device(grp, vlan_id);
> > +
> > + if (vlan_dev)
> > + skb->dev = vlan_dev;
> > + else
> > + if (vlan_id)
> > + goto drop;
> >
> > for (p = napi->gro_list; p; p = p->next) {
> > NAPI_GRO_CB(p)->same_flow =
> > diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> > index 5298426..65512c3 100644
> > --- a/net/8021q/vlan_dev.c
> > +++ b/net/8021q/vlan_dev.c
> > @@ -142,6 +142,7 @@ int vlan_skb_recv(struct sk_buff *skb, struct
> net_device
> > *dev,
> > {
> > struct vlan_hdr *vhdr;
> > struct vlan_rx_stats *rx_stats;
> > + struct net_device *vlan_dev;
> > u16 vlan_id;
> > u16 vlan_tci;
> >
> > @@ -157,8 +158,18 @@ int vlan_skb_recv(struct sk_buff *skb, struct
> net_device
> > *dev,
> > vlan_id = vlan_tci & VLAN_VID_MASK;
> >
> > rcu_read_lock();
> > - skb->dev = __find_vlan_dev(dev, vlan_id);
> > - if (!skb->dev) {
> > + vlan_dev = __find_vlan_dev(dev, vlan_id);
> > +
> > + /* If the VLAN device is defined, we use it.
> > + * If not, and the VID is 0, it is a 802.1p packet (not
> > + * really a VLAN), so we will just netif_rx it later to the
> > + * original interface, but with the skb->proto set to the
> > + * wrapped proto: we do nothing here.
> > + */
> > +
> > + if (vlan_dev) {
> > + skb->dev = vlan_dev;
> > + } else if (vlan_id) {
> > pr_debug("%s: ERROR: No net_device for VID: %u on dev:
> %s\n",
> > __func__, vlan_id, dev->name);
> > goto err_unlock;
> >
> >
> > --
> > 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
>
> N�����r��y���b�X��ǧv�^�){.n�+���z�^�)���w*
> jg���\x1e�����ݢj/���z�ޖ��2�ޙ���&�)ߡ�a��\x7f��\x1e�G���h�\x0f�j:+v���w�٥
^ permalink raw reply
* Re: [PATCH 3/4] ll_temac: free everything on error path
From: Denis Kirjanov @ 2010-07-08 13:16 UTC (permalink / raw)
To: Kulikov Vasiliy
Cc: Kernel Janitors, David S. Miller, John Linn, Grant Likely,
Jiri Pirko, Brian Hill, netdev
In-Reply-To: <1278591012-3213-1-git-send-email-segooon@gmail.com>
On Thu, Jul 8, 2010 at 4:10 PM, Kulikov Vasiliy <segooon@gmail.com> wrote:
> temac_dma_bd_init() must free all allocated resources: memory, dma, skbs.
>
> Signed-off-by: Kulikov Vasiliy <segooon@gmail.com>
> ---
> drivers/net/ll_temac_main.c | 30 +++++++++++++++++++++++++-----
> 1 files changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ll_temac_main.c b/drivers/net/ll_temac_main.c
> index a2da3d7..38d658a 100644
> --- a/drivers/net/ll_temac_main.c
> +++ b/drivers/net/ll_temac_main.c
> @@ -200,6 +200,7 @@ static int temac_dma_bd_init(struct net_device *ndev)
> struct temac_local *lp = netdev_priv(ndev);
> struct sk_buff *skb;
> int i;
> + int tx_bd_v_size, rx_bd_v_size;
>
> lp->rx_skb = kzalloc(sizeof(*lp->rx_skb) * RX_BD_NUM, GFP_KERNEL);
> if (!lp->rx_skb) {
> @@ -209,21 +210,23 @@ static int temac_dma_bd_init(struct net_device *ndev)
> }
> /* allocate the tx and rx ring buffer descriptors. */
> /* returns a virtual addres and a physical address. */
> + tx_bd_v_size = sizeof(*lp->tx_bd_v) * TX_BD_NUM;
> lp->tx_bd_v = dma_alloc_coherent(ndev->dev.parent,
> - sizeof(*lp->tx_bd_v) * TX_BD_NUM,
> + tx_bd_v_size,
> &lp->tx_bd_p, GFP_KERNEL);
> if (!lp->tx_bd_v) {
> dev_err(&ndev->dev,
> "unable to allocate DMA TX buffer descriptors");
> - goto out;
> + goto err_rx_skb;
> }
> + rx_bd_v_size = sizeof(*lp->rx_bd_v) * RX_BD_NUM;
> lp->rx_bd_v = dma_alloc_coherent(ndev->dev.parent,
> - sizeof(*lp->rx_bd_v) * RX_BD_NUM,
> + rx_bd_v_size,
> &lp->rx_bd_p, GFP_KERNEL);
> if (!lp->rx_bd_v) {
> dev_err(&ndev->dev,
> "unable to allocate DMA RX buffer descriptors");
> - goto out;
> + goto err_tx_bd;
> }
>
> memset(lp->tx_bd_v, 0, sizeof(*lp->tx_bd_v) * TX_BD_NUM);
> @@ -242,7 +245,7 @@ static int temac_dma_bd_init(struct net_device *ndev)
>
> if (skb == 0) {
> dev_err(&ndev->dev, "alloc_skb error %d\n", i);
> - goto out;
> + goto err_dma_single;
> }
> lp->rx_skb[i] = skb;
> /* returns physical address of skb->data */
> @@ -274,6 +277,23 @@ static int temac_dma_bd_init(struct net_device *ndev)
>
> return 0;
>
> +err_dma_single:
> + for (i = 0; i < RX_BD_NUM; i++) {
> + if (lp->rx_skb[i] == NULL)
> + break;
> +
> + kfree_skb(lp->rx_skb[i]);
> + dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys,
> + XTE_MAX_JUMBO_FRAME_SIZE, DMA_FROM_DEVICE);
> + }
> +
> + dma_free_coherent(ndev->dev.parent, rx_bd_v_size,
> + lp->rx_bd_v, lp->rx_bd_p);
> +err_tx_bd:
> + dma_free_coherent(ndev->dev.parent, tx_bd_v_size,
> + lp->tx_bd_v, lp->tx_bd_p);
> +err_rx_skb:
> + kfree(lp->rx_skb);
> out:
> return -ENOMEM;
> }
> --
> 1.7.0.4
This is not enough. DMA resources also must be released on exit.
> --
> 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
>
--
Regards,
Denis
^ permalink raw reply
* Re: [PATCH] igbvf: avoid name clash between PF and VF
From: Arnd Bergmann @ 2010-07-08 13:41 UTC (permalink / raw)
To: Stefan Assmann
Cc: netdev, e1000-devel, Duyck, Alexander H, gregory.v.rose,
jeffrey.t.kirsher, Andy Gospodarek
In-Reply-To: <4C2B0614.9040004@redhat.com>
On Wednesday 30 June 2010, Stefan Assmann wrote:
> diff --git a/drivers/net/igbvf/netdev.c b/drivers/net/igbvf/netdev.c
> index 5e2b2a8..2fb665b 100644
> --- a/drivers/net/igbvf/netdev.c
> +++ b/drivers/net/igbvf/netdev.c
> @@ -2787,7 +2787,7 @@ static int __devinit igbvf_probe(struct pci_dev *pdev,
> netif_carrier_off(netdev);
> netif_stop_queue(netdev);
>
> - strcpy(netdev->name, "eth%d");
> + strcpy(netdev->name, "veth%d");
> err = register_netdev(netdev);
> if (err)
> goto err_hw_init;
Note that 'veth' is the name used for a virtual ethernet pair by
drivers/net/veth.c. If a variant of your patch gets applied, it would
probably be useful to use a different naming scheme to avoid confusion
with the veth driver.
Arnd
^ permalink raw reply
* Re: [PATCH 3/4] ll_temac: free everything on error path
From: Kulikov Vasiliy @ 2010-07-08 13:46 UTC (permalink / raw)
To: dkirjanov
Cc: Kernel Janitors, David S. Miller, John Linn, Grant Likely,
Jiri Pirko, Brian Hill, netdev
In-Reply-To: <AANLkTilm1b61ikLi28We3aLrF3Ld4c05oFZlh3K1pORe@mail.gmail.com>
On Thu, Jul 08, 2010 at 17:16 +0400, Denis Kirjanov wrote:
> On Thu, Jul 8, 2010 at 4:10 PM, Kulikov Vasiliy <segooon@gmail.com> wrote:
> > temac_dma_bd_init() must free all allocated resources: memory, dma, skbs.
> >
> > Signed-off-by: Kulikov Vasiliy <segooon@gmail.com>
> > ---
> > drivers/net/ll_temac_main.c | 30 +++++++++++++++++++++++++-----
> > 1 files changed, 25 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ll_temac_main.c b/drivers/net/ll_temac_main.c
> > index a2da3d7..38d658a 100644
> > --- a/drivers/net/ll_temac_main.c
> > +++ b/drivers/net/ll_temac_main.c
> > @@ -200,6 +200,7 @@ static int temac_dma_bd_init(struct net_device *ndev)
> > struct temac_local *lp = netdev_priv(ndev);
> > struct sk_buff *skb;
> > int i;
> > + int tx_bd_v_size, rx_bd_v_size;
> >
> > lp->rx_skb = kzalloc(sizeof(*lp->rx_skb) * RX_BD_NUM, GFP_KERNEL);
> > if (!lp->rx_skb) {
> > @@ -209,21 +210,23 @@ static int temac_dma_bd_init(struct net_device *ndev)
> > }
> > /* allocate the tx and rx ring buffer descriptors. */
> > /* returns a virtual addres and a physical address. */
> > + tx_bd_v_size = sizeof(*lp->tx_bd_v) * TX_BD_NUM;
> > lp->tx_bd_v = dma_alloc_coherent(ndev->dev.parent,
^^^^^^^^^^^
1st
> > - sizeof(*lp->tx_bd_v) * TX_BD_NUM,
> > + tx_bd_v_size,
> > &lp->tx_bd_p, GFP_KERNEL);
> > if (!lp->tx_bd_v) {
> > dev_err(&ndev->dev,
> > "unable to allocate DMA TX buffer descriptors");
> > - goto out;
> > + goto err_rx_skb;
> > }
> > + rx_bd_v_size = sizeof(*lp->rx_bd_v) * RX_BD_NUM;
> > lp->rx_bd_v = dma_alloc_coherent(ndev->dev.parent,
^^^^^^^^^^^
2nd
> > - sizeof(*lp->rx_bd_v) * RX_BD_NUM,
> > + rx_bd_v_size,
> > &lp->rx_bd_p, GFP_KERNEL);
> > if (!lp->rx_bd_v) {
> > dev_err(&ndev->dev,
> > "unable to allocate DMA RX buffer descriptors");
> > - goto out;
> > + goto err_tx_bd;
> > }
> >
> > memset(lp->tx_bd_v, 0, sizeof(*lp->tx_bd_v) * TX_BD_NUM);
> > @@ -242,7 +245,7 @@ static int temac_dma_bd_init(struct net_device *ndev)
> >
> > if (skb == 0) {
> > dev_err(&ndev->dev, "alloc_skb error %d\n", i);
> > - goto out;
> > + goto err_dma_single;
> > }
> > lp->rx_skb[i] = skb;
> > /* returns physical address of skb->data */
> > @@ -274,6 +277,23 @@ static int temac_dma_bd_init(struct net_device *ndev)
> >
> > return 0;
> >
> > +err_dma_single:
> > + for (i = 0; i < RX_BD_NUM; i++) {
> > + if (lp->rx_skb[i] == NULL)
> > + break;
> > +
> > + kfree_skb(lp->rx_skb[i]);
> > + dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys,
> > + XTE_MAX_JUMBO_FRAME_SIZE, DMA_FROM_DEVICE);
> > + }
> > +
> > + dma_free_coherent(ndev->dev.parent, rx_bd_v_size,
> > + lp->rx_bd_v, lp->rx_bd_p);
^^^^^^^^^^^
2nd
> > +err_tx_bd:
> > + dma_free_coherent(ndev->dev.parent, tx_bd_v_size,
> > + lp->tx_bd_v, lp->tx_bd_p);
^^^^^^^^^^^
1st
> > +err_rx_skb:
> > + kfree(lp->rx_skb);
> > out:
> > return -ENOMEM;
> > }
> > --
> > 1.7.0.4
> This is not enough. DMA resources also must be released on exit.
Could you point to it? I see only two variables with allocated DMA (see above).
^ permalink raw reply
* RE: [PATCH] vlan_dev: VLAN 0 should be treated as "no vlan tag" (802.1p packet)
From: Vladislav Zolotarov @ 2010-07-08 13:51 UTC (permalink / raw)
To: Pedro Garcia, netdev@vger.kernel.org
Cc: Patrick McHardy, Ben Hutchings, Eric Dumazet
In-Reply-To: <311b59aee7d648c6124a84b5ca06ac60@dondevamos.com>
I've checked your patch on bnx2x HW acceleration capable device in both HW accelerated and none-accelerated modes and it looks good.
Thanks,
vlad
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> Behalf Of Pedro Garcia
> Sent: Wednesday, June 16, 2010 11:49 AM
> To: netdev@vger.kernel.org
> Cc: Patrick McHardy; Ben Hutchings; Eric Dumazet
> Subject: Re: [PATCH] vlan_dev: VLAN 0 should be treated as "no vlan tag"
> (802.1p packet)
>
> On Mon, 14 Jun 2010 21:12:52 +0200, Eric Dumazet <eric.dumazet@gmail.com>
> wrote:
> > Le lundi 14 juin 2010 à 19:11 +0200, Patrick McHardy a écrit :
> >> Ben Hutchings wrote:
> >> > On Mon, 2010-06-14 at 18:49 +0200, Pedro Garcia wrote:
> >> >
> >> >> On Sun, 13 Jun 2010 22:56:30 +0100, Ben Hutchings
> >> >> <bhutchings@solarflare.com> wrote:
> >> >>
> >> >>> I have no particular opinion on this change, but you need to read and
> >> >>> follow Documentation/SubmittingPatches.
> >> >>>
> >> >>> Ben.
> >> >>>
> >> >> Sorry, first kernel patch, and I did not know about it. I resubmit
> >> >> with
> >> >> the correct style / format:
> >> >>
> >> > [...]
> >> >
> >> > Sorry, no you haven't.
> >> >
> >> > - Networking changes go through David Miller's net-next-2.6 tree so you
> >> > need to use that as the baseline, not 2.6.26
> >> > - Patches should be applicable with -p1, not -p0 (so if you use diff,
> >> > you should run it from one directory level up)
> >> > - The patch was word-wrapped
> >>
> >> Additionally:
> >>
> >> - please use the proper comment style, meaning each line begins
> >> with a '*'
> >>
> >> - the pr_debug statements may be useful for debugging, but are
> >> a bit excessive for the final version
> >>
> >> - + /* 2010-06-13: Pedro Garcia
> >>
> >> We have changelogs for this, simply explaining what the code
> >> does is enough.
> >>
> >> - Please CC the maintainer (which is me)
> >> --
> >
> > Pedro, we have two kind of vlan setups :
> >
> > accelerated and non accelerated ones.
> >
> > Your patch address non accelated ones only, since you only touch
> > vlan_skb_recv()
> >
> > Accelerated vlan can follow these paths :
> >
> > 1) NAPI devices
> >
> > vlan_gro_receive() -> vlan_gro_common()
> >
> > 2) non NAPI devices
> >
> > __vlan_hwaccel_rx()
> >
> > So you might also patch __vlan_hwaccel_rx() and vlan_gro_common()
> >
> > Please merge following bits to your patch submission :
> >
> > http://kerneltrap.org/mailarchive/linux-netdev/2010/5/23/6277868
> >
> >
> > Good luck for your first patch !
>
> Here it is again. I added the modifications in
> http://kerneltrap.org/mailarchive/linux-netdev/2010/5/23/6277868 for HW
> accelerated incoming packets (it did not apply cleanly on the last version of
> the kernel, so I applied manually). Now, if the VLAN 0 is not explicitly
> created by the user, VLAN 0 packets will be treated as no VLAN (802.1p
> packets), instead of dropping them.
>
> The patch is now for two files: vlan_core (accel) and vlan_dev (non accel)
>
> I can not test on HW accelerated devices, so if someone can check it I will
> appreciate (even though in the thread above it looked like yes). For non
> accel I tessted in 2.6.26. Now the patch is for
> net-next-2.6, and it compiles OK, but I a have to setup a test environment to
> check it is still OK (should, but better to test).
>
> Signed-off-by: Pedro Garcia <pedro.netdev@dondevamos.com>
> --
> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> index 50f58f5..daaca31 100644
> --- a/net/8021q/vlan_core.c
> +++ b/net/8021q/vlan_core.c
> @@ -8,6 +8,9 @@
> int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
> u16 vlan_tci, int polling)
> {
> + struct net_device *vlan_dev;
> + u16 vlan_id;
> +
> if (netpoll_rx(skb))
> return NET_RX_DROP;
>
> @@ -16,10 +19,14 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct
> vlan_group *grp,
>
> skb->skb_iif = skb->dev->ifindex;
> __vlan_hwaccel_put_tag(skb, vlan_tci);
> - skb->dev = vlan_group_get_device(grp, vlan_tci & VLAN_VID_MASK);
> + vlan_id = vlan_tci & VLAN_VID_MASK;
> + vlan_dev = vlan_group_get_device(grp, vlan_id);
>
> - if (!skb->dev)
> - goto drop;
> + if (vlan_dev)
> + skb->dev = vlan_dev;
> + else
> + if (vlan_id)
> + goto drop;
>
> return (polling ? netif_receive_skb(skb) : netif_rx(skb));
>
> @@ -82,16 +89,22 @@ vlan_gro_common(struct napi_struct *napi, struct
> vlan_group *grp,
> unsigned int vlan_tci, struct sk_buff *skb)
> {
> struct sk_buff *p;
> + struct net_device *vlan_dev;
> + u16 vlan_id;
>
> if (skb_bond_should_drop(skb, ACCESS_ONCE(skb->dev->master)))
> skb->deliver_no_wcard = 1;
>
> skb->skb_iif = skb->dev->ifindex;
> __vlan_hwaccel_put_tag(skb, vlan_tci);
> - skb->dev = vlan_group_get_device(grp, vlan_tci & VLAN_VID_MASK);
> -
> - if (!skb->dev)
> - goto drop;
> + vlan_id = vlan_tci & VLAN_VID_MASK;
> + vlan_dev = vlan_group_get_device(grp, vlan_id);
> +
> + if (vlan_dev)
> + skb->dev = vlan_dev;
> + else
> + if (vlan_id)
> + goto drop;
>
> for (p = napi->gro_list; p; p = p->next) {
> NAPI_GRO_CB(p)->same_flow =
> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index 5298426..65512c3 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -142,6 +142,7 @@ int vlan_skb_recv(struct sk_buff *skb, struct net_device
> *dev,
> {
> struct vlan_hdr *vhdr;
> struct vlan_rx_stats *rx_stats;
> + struct net_device *vlan_dev;
> u16 vlan_id;
> u16 vlan_tci;
>
> @@ -157,8 +158,18 @@ int vlan_skb_recv(struct sk_buff *skb, struct net_device
> *dev,
> vlan_id = vlan_tci & VLAN_VID_MASK;
>
> rcu_read_lock();
> - skb->dev = __find_vlan_dev(dev, vlan_id);
> - if (!skb->dev) {
> + vlan_dev = __find_vlan_dev(dev, vlan_id);
> +
> + /* If the VLAN device is defined, we use it.
> + * If not, and the VID is 0, it is a 802.1p packet (not
> + * really a VLAN), so we will just netif_rx it later to the
> + * original interface, but with the skb->proto set to the
> + * wrapped proto: we do nothing here.
> + */
> +
> + if (vlan_dev) {
> + skb->dev = vlan_dev;
> + } else if (vlan_id) {
> pr_debug("%s: ERROR: No net_device for VID: %u on dev: %s\n",
> __func__, vlan_id, dev->name);
> goto err_unlock;
>
>
> --
> 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
^ permalink raw reply
* [PATCH -net-2.6] ll_temac: fix DMA resources leak
From: Denis Kirjanov @ 2010-07-08 13:54 UTC (permalink / raw)
To: davem; +Cc: john.linn, brian.hill, netdev
Fix DMA resources leak.
Signed-off-by: Denis Kirjanov <dkirjanov@kernel.org>
Signed-off-by: Kulikov Vasiliy <segooon@gmail.com>
---
diff --git a/drivers/net/ll_temac_main.c b/drivers/net/ll_temac_main.c
index fa303c8..afec660 100644
--- a/drivers/net/ll_temac_main.c
+++ b/drivers/net/ll_temac_main.c
@@ -193,6 +193,28 @@ static int temac_dcr_setup(struct temac_local *lp, struct of_device *op,
#endif
/**
+ * * temac_dma_bd_release - Release buffer descriptor rings
+ */
+static void temac_dma_bd_release(struct net_device *ndev)
+{
+ struct temac_local *lp = netdev_priv(ndev);
+ int i;
+
+ for (i = 0; i < RX_BD_NUM; i++) {
+ if (lp->rx_skb[i]) {
+ dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys,
+ XTE_MAX_JUMBO_FRAME_SIZE, DMA_FROM_DEVICE);
+ dev_kfree_skb(lp->rx_skb[i]);
+ }
+ }
+ dma_free_coherent(ndev->dev.parent, sizeof(*lp->rx_bd_v) * RX_BD_NUM,
+ lp->rx_bd_v, lp->rx_bd_p);
+ dma_free_coherent(ndev->dev.parent, sizeof(*lp->tx_bd_v) * TX_BD_NUM,
+ lp->tx_bd_v, lp->tx_bd_p);
+ kfree(lp->rx_skb);
+}
+
+/**
* temac_dma_bd_init - Setup buffer descriptor rings
*/
static int temac_dma_bd_init(struct net_device *ndev)
@@ -275,6 +297,7 @@ static int temac_dma_bd_init(struct net_device *ndev)
return 0;
out:
+ temac_dma_bd_release(ndev);
return -ENOMEM;
}
@@ -858,6 +881,8 @@ static int temac_stop(struct net_device *ndev)
phy_disconnect(lp->phy_dev);
lp->phy_dev = NULL;
+ temac_dma_bd_release(ndev);
+
return 0;
}
^ permalink raw reply related
* Re: [PATCH 3/4] ll_temac: free everything on error path
From: Denis Kirjanov @ 2010-07-08 13:56 UTC (permalink / raw)
To: Kulikov Vasiliy
Cc: Kernel Janitors, David S. Miller, John Linn, Grant Likely,
Jiri Pirko, Brian Hill, netdev
In-Reply-To: <20100708134651.GA27196@shinshilla>
On Thu, Jul 8, 2010 at 5:46 PM, Kulikov Vasiliy <segooon@gmail.com> wrote:
> On Thu, Jul 08, 2010 at 17:16 +0400, Denis Kirjanov wrote:
>> On Thu, Jul 8, 2010 at 4:10 PM, Kulikov Vasiliy <segooon@gmail.com> wrote:
>> > temac_dma_bd_init() must free all allocated resources: memory, dma, skbs.
>> >
>> > Signed-off-by: Kulikov Vasiliy <segooon@gmail.com>
>> > ---
>> > drivers/net/ll_temac_main.c | 30 +++++++++++++++++++++++++-----
>> > 1 files changed, 25 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/net/ll_temac_main.c b/drivers/net/ll_temac_main.c
>> > index a2da3d7..38d658a 100644
>> > --- a/drivers/net/ll_temac_main.c
>> > +++ b/drivers/net/ll_temac_main.c
>> > @@ -200,6 +200,7 @@ static int temac_dma_bd_init(struct net_device *ndev)
>> > struct temac_local *lp = netdev_priv(ndev);
>> > struct sk_buff *skb;
>> > int i;
>> > + int tx_bd_v_size, rx_bd_v_size;
>> >
>> > lp->rx_skb = kzalloc(sizeof(*lp->rx_skb) * RX_BD_NUM, GFP_KERNEL);
>> > if (!lp->rx_skb) {
>> > @@ -209,21 +210,23 @@ static int temac_dma_bd_init(struct net_device *ndev)
>> > }
>> > /* allocate the tx and rx ring buffer descriptors. */
>> > /* returns a virtual addres and a physical address. */
>> > + tx_bd_v_size = sizeof(*lp->tx_bd_v) * TX_BD_NUM;
>> > lp->tx_bd_v = dma_alloc_coherent(ndev->dev.parent,
> ^^^^^^^^^^^
> 1st
>> > - sizeof(*lp->tx_bd_v) * TX_BD_NUM,
>> > + tx_bd_v_size,
>> > &lp->tx_bd_p, GFP_KERNEL);
>> > if (!lp->tx_bd_v) {
>> > dev_err(&ndev->dev,
>> > "unable to allocate DMA TX buffer descriptors");
>> > - goto out;
>> > + goto err_rx_skb;
>> > }
>> > + rx_bd_v_size = sizeof(*lp->rx_bd_v) * RX_BD_NUM;
>> > lp->rx_bd_v = dma_alloc_coherent(ndev->dev.parent,
> ^^^^^^^^^^^
> 2nd
>> > - sizeof(*lp->rx_bd_v) * RX_BD_NUM,
>> > + rx_bd_v_size,
>> > &lp->rx_bd_p, GFP_KERNEL);
>> > if (!lp->rx_bd_v) {
>> > dev_err(&ndev->dev,
>> > "unable to allocate DMA RX buffer descriptors");
>> > - goto out;
>> > + goto err_tx_bd;
>> > }
>> >
>> > memset(lp->tx_bd_v, 0, sizeof(*lp->tx_bd_v) * TX_BD_NUM);
>> > @@ -242,7 +245,7 @@ static int temac_dma_bd_init(struct net_device *ndev)
>> >
>> > if (skb == 0) {
>> > dev_err(&ndev->dev, "alloc_skb error %d\n", i);
>> > - goto out;
>> > + goto err_dma_single;
>> > }
>> > lp->rx_skb[i] = skb;
>> > /* returns physical address of skb->data */
>> > @@ -274,6 +277,23 @@ static int temac_dma_bd_init(struct net_device *ndev)
>> >
>> > return 0;
>> >
>> > +err_dma_single:
>> > + for (i = 0; i < RX_BD_NUM; i++) {
>> > + if (lp->rx_skb[i] == NULL)
>> > + break;
>> > +
>> > + kfree_skb(lp->rx_skb[i]);
>> > + dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys,
>> > + XTE_MAX_JUMBO_FRAME_SIZE, DMA_FROM_DEVICE);
>> > + }
>> > +
>> > + dma_free_coherent(ndev->dev.parent, rx_bd_v_size,
>> > + lp->rx_bd_v, lp->rx_bd_p);
> ^^^^^^^^^^^
> 2nd
>> > +err_tx_bd:
>> > + dma_free_coherent(ndev->dev.parent, tx_bd_v_size,
>> > + lp->tx_bd_v, lp->tx_bd_p);
> ^^^^^^^^^^^
> 1st
>> > +err_rx_skb:
>> > + kfree(lp->rx_skb);
>> > out:
>> > return -ENOMEM;
>> > }
>> > --
>> > 1.7.0.4
>> This is not enough. DMA resources also must be released on exit.
> Could you point to it? I see only two variables with allocated DMA (see above).
> --
I just sent a patch that fixes this.
> 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
>
--
Regards,
Denis
^ permalink raw reply
* Re: [PATCH net-next-2.6] bnx2: 64 bit stats on all arches
From: Michael Chan @ 2010-07-08 14:00 UTC (permalink / raw)
To: 'Eric Dumazet', David Miller; +Cc: netdev, Matthew Carlson
In-Reply-To: <1278583542.2651.19.camel@edumazet-laptop>
Eric Dumazet wrote:
> Now core network is able to handle 64 bit netdevice stats on 32 bit
> arches, we can provide them for bnx2, since hardware maintains some 64
> bit counters.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> drivers/net/bnx2.c | 22 +++++++---------------
> 1 file changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
> index 22fa1e9..2975bf9 100644
> --- a/drivers/net/bnx2.c
> +++ b/drivers/net/bnx2.c
> @@ -6589,36 +6589,28 @@ bnx2_save_stats(struct bnx2 *bp)
> temp_stats[i] += hw_stats[i];
> }
>
> -#define GET_64BIT_NET_STATS64(ctr) \
> - (unsigned long) ((unsigned long) (ctr##_hi) << 32) + \
> - (unsigned long) (ctr##_lo)
> +#define GET_64BIT_NET_STATS64(ctr) \
> + (((u64) (ctr##_hi) << 32) + (u64) (ctr##_lo))
>
> #define GET_64BIT_NET_STATS32(ctr) \
> (ctr##_lo)
It seems that GET_64BIT_NET_STATS32 is no longer needed.
Other than that,
Acked-by: Michael Chan <mchan@broadcom.com>
Thanks.
>
> -#if (BITS_PER_LONG == 64)
> #define GET_64BIT_NET_STATS(ctr) \
> GET_64BIT_NET_STATS64(bp->stats_blk->ctr) + \
> GET_64BIT_NET_STATS64(bp->temp_stats_blk->ctr)
> -#else
> -#define GET_64BIT_NET_STATS(ctr) \
> - GET_64BIT_NET_STATS32(bp->stats_blk->ctr) + \
> - GET_64BIT_NET_STATS32(bp->temp_stats_blk->ctr)
> -#endif
>
> #define GET_32BIT_NET_STATS(ctr) \
> (unsigned long) (bp->stats_blk->ctr + \
> bp->temp_stats_blk->ctr)
>
> -static struct net_device_stats *
> -bnx2_get_stats(struct net_device *dev)
> +static struct rtnl_link_stats64 *
> +bnx2_get_stats64(struct net_device *dev, struct
> rtnl_link_stats64 *net_stats)
> {
> struct bnx2 *bp = netdev_priv(dev);
> - struct net_device_stats *net_stats = &dev->stats;
>
> - if (bp->stats_blk == NULL) {
> + if (bp->stats_blk == NULL)
> return net_stats;
> - }
> +
> net_stats->rx_packets =
> GET_64BIT_NET_STATS(stat_IfHCInUcastPkts) +
> GET_64BIT_NET_STATS(stat_IfHCInMulticastPkts) +
> @@ -8289,7 +8281,7 @@ static const struct net_device_ops
> bnx2_netdev_ops = {
> .ndo_open = bnx2_open,
> .ndo_start_xmit = bnx2_start_xmit,
> .ndo_stop = bnx2_close,
> - .ndo_get_stats = bnx2_get_stats,
> + .ndo_get_stats64 = bnx2_get_stats64,
> .ndo_set_rx_mode = bnx2_set_rx_mode,
> .ndo_do_ioctl = bnx2_ioctl,
> .ndo_validate_addr = eth_validate_addr,
>
>
>
>
^ permalink raw reply
* Re: [PATCH net-next-2.6] bnx2: 64 bit stats on all arches
From: Eric Dumazet @ 2010-07-08 14:08 UTC (permalink / raw)
To: Michael Chan; +Cc: David Miller, netdev, Matthew Carlson
In-Reply-To: <C27F8246C663564A84BB7AB3439772421B842BD250@IRVEXCHCCR01.corp.ad.broadcom.com>
Le jeudi 08 juillet 2010 à 07:00 -0700, Michael Chan a écrit :
> It seems that GET_64BIT_NET_STATS32 is no longer needed.
> Other than that,
>
> Acked-by: Michael Chan <mchan@broadcom.com>
>
> Thanks.
Indeed, thanks for the tip !
Here is the updated version.
[PATCH net-next-2.6] bnx2: 64 bit stats on all arches
Now core network is able to handle 64 bit netdevice stats on 32 bit
arches, we can provide them for bnx2, since hardware maintains some 64
bit counters.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Acked-by: Michael Chan <mchan@broadcom.com>
---
drivers/net/bnx2.c | 25 +++++++------------------
1 file changed, 7 insertions(+), 18 deletions(-)
diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 22fa1e9..a203f39 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -6589,36 +6589,25 @@ bnx2_save_stats(struct bnx2 *bp)
temp_stats[i] += hw_stats[i];
}
-#define GET_64BIT_NET_STATS64(ctr) \
- (unsigned long) ((unsigned long) (ctr##_hi) << 32) + \
- (unsigned long) (ctr##_lo)
+#define GET_64BIT_NET_STATS64(ctr) \
+ (((u64) (ctr##_hi) << 32) + (u64) (ctr##_lo))
-#define GET_64BIT_NET_STATS32(ctr) \
- (ctr##_lo)
-
-#if (BITS_PER_LONG == 64)
#define GET_64BIT_NET_STATS(ctr) \
GET_64BIT_NET_STATS64(bp->stats_blk->ctr) + \
GET_64BIT_NET_STATS64(bp->temp_stats_blk->ctr)
-#else
-#define GET_64BIT_NET_STATS(ctr) \
- GET_64BIT_NET_STATS32(bp->stats_blk->ctr) + \
- GET_64BIT_NET_STATS32(bp->temp_stats_blk->ctr)
-#endif
#define GET_32BIT_NET_STATS(ctr) \
(unsigned long) (bp->stats_blk->ctr + \
bp->temp_stats_blk->ctr)
-static struct net_device_stats *
-bnx2_get_stats(struct net_device *dev)
+static struct rtnl_link_stats64 *
+bnx2_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *net_stats)
{
struct bnx2 *bp = netdev_priv(dev);
- struct net_device_stats *net_stats = &dev->stats;
- if (bp->stats_blk == NULL) {
+ if (bp->stats_blk == NULL)
return net_stats;
- }
+
net_stats->rx_packets =
GET_64BIT_NET_STATS(stat_IfHCInUcastPkts) +
GET_64BIT_NET_STATS(stat_IfHCInMulticastPkts) +
@@ -8289,7 +8278,7 @@ static const struct net_device_ops bnx2_netdev_ops = {
.ndo_open = bnx2_open,
.ndo_start_xmit = bnx2_start_xmit,
.ndo_stop = bnx2_close,
- .ndo_get_stats = bnx2_get_stats,
+ .ndo_get_stats64 = bnx2_get_stats64,
.ndo_set_rx_mode = bnx2_set_rx_mode,
.ndo_do_ioctl = bnx2_ioctl,
.ndo_validate_addr = eth_validate_addr,
^ permalink raw reply related
* Re: [PATCH 3/4] ll_temac: free everything on error path
From: Kulikov Vasiliy @ 2010-07-08 14:24 UTC (permalink / raw)
To: dkirjanov
Cc: Kernel Janitors, David S. Miller, John Linn, Grant Likely,
Jiri Pirko, Brian Hill, netdev
In-Reply-To: <AANLkTilMaITOMsn_9MmZ7-zHwpvTd2ZYjj4UUWOloYsn@mail.gmail.com>
> I just sent a patch that fixes this.
Hey, you forgot to CC me/kernel-janitors in your patch.
Anyway,
> /**
>+ * * temac_dma_bd_release - Release buffer descriptor rings
>+ */
>+static void temac_dma_bd_release(struct net_device *ndev)
>+{
>+ struct temac_local *lp = netdev_priv(ndev);
>+ int i;
>+
>+ for (i = 0; i < RX_BD_NUM; i++) {
>+ if (lp->rx_skb[i]) {
^^^^^^^^^^
This can be NULL if kzalloc() failed.
>+ dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys,
>+ XTE_MAX_JUMBO_FRAME_SIZE, DMA_FROM_DEVICE);
>+ dev_kfree_skb(lp->rx_skb[i]);
>+ }
>+ }
>+ dma_free_coherent(ndev->dev.parent, sizeof(*lp->rx_bd_v) * RX_BD_NUM,
>+ lp->rx_bd_v, lp->rx_bd_p);
>+ dma_free_coherent(ndev->dev.parent, sizeof(*lp->tx_bd_v) * TX_BD_NUM,
>+ lp->tx_bd_v, lp->tx_bd_p);
These two DMA's can be unallocated if dma_alloc_coherent() failed.
I wanted to introduce many labels to divide these error cases because of it.
>+ kfree(lp->rx_skb);
>+}
^ permalink raw reply
* Re: [PATCH 2/4] ll_temac: fix memory leak
From: Grant Likely @ 2010-07-08 15:24 UTC (permalink / raw)
To: Kulikov Vasiliy
Cc: Kernel Janitors, David S. Miller, John Linn, Jiri Pirko,
Brian Hill, netdev
In-Reply-To: <1278591006-3156-1-git-send-email-segooon@gmail.com>
On Thu, Jul 8, 2010 at 6:10 AM, Kulikov Vasiliy <segooon@gmail.com> wrote:
> If of_iomap() or irq_of_parse_and_map() fail then np must be freed.
>
> Signed-off-by: Kulikov Vasiliy <segooon@gmail.com>
Looks correct to me.
g.
> ---
> drivers/net/ll_temac_main.c | 5 ++++-
> 1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/ll_temac_main.c b/drivers/net/ll_temac_main.c
> index 5bca20b..a2da3d7 100644
> --- a/drivers/net/ll_temac_main.c
> +++ b/drivers/net/ll_temac_main.c
> @@ -999,19 +999,22 @@ temac_of_probe(struct of_device *op, const struct of_device_id *match)
> dev_dbg(&op->dev, "MEM base: %p\n", lp->sdma_regs);
> } else {
> dev_err(&op->dev, "unable to map DMA registers\n");
> + of_node_put(np);
> goto err_iounmap;
> }
> }
>
> lp->rx_irq = irq_of_parse_and_map(np, 0);
> lp->tx_irq = irq_of_parse_and_map(np, 1);
> +
> + of_node_put(np); /* Finished with the DMA node; drop the reference */
> +
> if ((lp->rx_irq == NO_IRQ) || (lp->tx_irq == NO_IRQ)) {
> dev_err(&op->dev, "could not determine irqs\n");
> rc = -ENOMEM;
> goto err_iounmap_2;
> }
>
> - of_node_put(np); /* Finished with the DMA node; drop the reference */
>
> /* Retrieve the MAC address */
> addr = of_get_property(op->dev.of_node, "local-mac-address", &size);
> --
> 1.7.0.4
>
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH v2] vlan: allow TSO setting on vlan interfaces
From: Ben Hutchings @ 2010-07-08 15:43 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Patrick McHardy
In-Reply-To: <1278581971.2651.10.camel@edumazet-laptop>
On Thu, 2010-07-08 at 11:39 +0200, Eric Dumazet wrote:
> When we need to shape traffic with low speeds, we need to disable tso on
> network interface :
>
> ethtool -K eth0.2240 tso off
>
> It seems vlan interfaces miss the set_tso() ethtool method.
> Propagating tso changes from lower device is not always wanted, some
> vlans want TSO on, others want TSO off.
>
> Before enabling TSO, we must check real device supports it.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> net/8021q/vlan_dev.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index c6456cb..870bc53 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -838,12 +838,25 @@ static struct rtnl_link_stats64 *vlan_dev_get_stats64(struct net_device *dev)
> return stats;
> }
>
> +static int vlan_ethtool_set_tso(struct net_device *dev, u32 data)
> +{
> + if (data) {
> + struct net_device *real_dev = vlan_dev_info(dev)->real_dev;
> + unsigned long rdev_vfeatures = real_dev->features & real_dev->vlan_features;
> +
> + dev->features |= (NETIF_F_TSO & rdev_vfeatures);
> + } else
> + dev->features &= ~NETIF_F_TSO;
> + return 0;
> +}
[...]
This should not silently ignore attempts to enable TSO. I think it
should be something like:
static int vlan_ethtool_set_tso(struct net_device *dev, u32 data)
{
if (data) {
struct net_device *real_dev = vlan_dev_info(dev)->real_dev;
/* Underlying device must support TSO for VLAN-tagged packets
* and must have TSO enabled now. */
if (!(real_dev->vlan_features & NETIF_F_TSO))
return -EOPNOTSUPP;
if (!(real_dev->features & NETIF_F_TSO))
return -EINVAL;
dev->features |= NETIF_F_TSO;
} else {
dev->features &= ~NETIF_F_TSO;
}
return 0;
}
I think the vlan driver should also have a netdev notifier to handle
feature changes on the underlying device.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH v2] vlan: allow TSO setting on vlan interfaces
From: Ben Hutchings @ 2010-07-08 15:49 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Patrick McHardy
In-Reply-To: <1278603804.16013.24.camel@achroite.uk.solarflarecom.com>
On Thu, 2010-07-08 at 16:43 +0100, Ben Hutchings wrote:
> On Thu, 2010-07-08 at 11:39 +0200, Eric Dumazet wrote:
> > When we need to shape traffic with low speeds, we need to disable tso on
> > network interface :
> >
> > ethtool -K eth0.2240 tso off
> >
> > It seems vlan interfaces miss the set_tso() ethtool method.
> > Propagating tso changes from lower device is not always wanted, some
> > vlans want TSO on, others want TSO off.
[...]
> I think the vlan driver should also have a netdev notifier to handle
> feature changes on the underlying device.
To clarify, I think offload features should be disabled on a vlan device
if they are later disabled on the underlying device. Propagating
changes to enable features, as you say, might not be wanted.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH v2] vlan: allow TSO setting on vlan interfaces
From: Patrick McHardy @ 2010-07-08 15:55 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Eric Dumazet, David Miller, netdev
In-Reply-To: <1278604198.16013.35.camel@achroite.uk.solarflarecom.com>
Ben Hutchings wrote:
> On Thu, 2010-07-08 at 16:43 +0100, Ben Hutchings wrote:
>
>> On Thu, 2010-07-08 at 11:39 +0200, Eric Dumazet wrote:
>>
>>> When we need to shape traffic with low speeds, we need to disable tso on
>>> network interface :
>>>
>>> ethtool -K eth0.2240 tso off
>>>
>>> It seems vlan interfaces miss the set_tso() ethtool method.
>>> Propagating tso changes from lower device is not always wanted, some
>>> vlans want TSO on, others want TSO off.
>>>
> [...]
>
>> I think the vlan driver should also have a netdev notifier to handle
>> feature changes on the underlying device.
>>
>
> To clarify, I think offload features should be disabled on a vlan device
> if they are later disabled on the underlying device. Propagating
> changes to enable features, as you say, might not be wanted.
I agree. The VLAN driver should also propagate feature changes
upwards itself.
^ permalink raw reply
* Re: [PATCH v2] vlan: allow TSO setting on vlan interfaces
From: Eric Dumazet @ 2010-07-08 16:00 UTC (permalink / raw)
To: Ben Hutchings; +Cc: David Miller, netdev, Patrick McHardy
In-Reply-To: <1278604198.16013.35.camel@achroite.uk.solarflarecom.com>
Le jeudi 08 juillet 2010 à 16:49 +0100, Ben Hutchings a écrit :
> On Thu, 2010-07-08 at 16:43 +0100, Ben Hutchings wrote:
> > On Thu, 2010-07-08 at 11:39 +0200, Eric Dumazet wrote:
> > > When we need to shape traffic with low speeds, we need to disable tso on
> > > network interface :
> > >
> > > ethtool -K eth0.2240 tso off
> > >
> > > It seems vlan interfaces miss the set_tso() ethtool method.
> > > Propagating tso changes from lower device is not always wanted, some
> > > vlans want TSO on, others want TSO off.
> [...]
> > I think the vlan driver should also have a netdev notifier to handle
> > feature changes on the underlying device.
>
> To clarify, I think offload features should be disabled on a vlan device
> if they are later disabled on the underlying device. Propagating
> changes to enable features, as you say, might not be wanted.
OK, but isnt it already done ?
Check vlan_transfer_features() in net/8021q/vlan.c
# ethtool -k eth3|grep tcp-segmentation
tcp-segmentation-offload: on
# ethtool -k eth3.103|grep tcp-segmentation
tcp-segmentation-offload: on
# ethtool -K eth3 tso off
# ethtool -k eth3|grep tcp-segmentation
tcp-segmentation-offload: off
# ethtool -k eth3.103|grep tcp-segmentation
tcp-segmentation-offload: off
# ethtool -K eth3 tso on
# ethtool -k eth3.103|grep tcp-segmentation
tcp-segmentation-offload: on
We should not change it to avoid ORing TSO, it might break some setups.
^ permalink raw reply
* Re: [PATCH v2] vlan: allow TSO setting on vlan interfaces
From: Eric Dumazet @ 2010-07-08 16:03 UTC (permalink / raw)
To: Ben Hutchings; +Cc: David Miller, netdev, Patrick McHardy
In-Reply-To: <1278603804.16013.24.camel@achroite.uk.solarflarecom.com>
Le jeudi 08 juillet 2010 à 16:43 +0100, Ben Hutchings a écrit :
> On Thu, 2010-07-08 at 11:39 +0200, Eric Dumazet wrote:
> > When we need to shape traffic with low speeds, we need to disable tso on
> > network interface :
> >
> > ethtool -K eth0.2240 tso off
> >
> > It seems vlan interfaces miss the set_tso() ethtool method.
> > Propagating tso changes from lower device is not always wanted, some
> > vlans want TSO on, others want TSO off.
> >
> > Before enabling TSO, we must check real device supports it.
> >
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> > ---
> > net/8021q/vlan_dev.c | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> > index c6456cb..870bc53 100644
> > --- a/net/8021q/vlan_dev.c
> > +++ b/net/8021q/vlan_dev.c
> > @@ -838,12 +838,25 @@ static struct rtnl_link_stats64 *vlan_dev_get_stats64(struct net_device *dev)
> > return stats;
> > }
> >
> > +static int vlan_ethtool_set_tso(struct net_device *dev, u32 data)
> > +{
> > + if (data) {
> > + struct net_device *real_dev = vlan_dev_info(dev)->real_dev;
> > + unsigned long rdev_vfeatures = real_dev->features & real_dev->vlan_features;
> > +
> > + dev->features |= (NETIF_F_TSO & rdev_vfeatures);
> > + } else
> > + dev->features &= ~NETIF_F_TSO;
> > + return 0;
> > +}
> [...]
>
> This should not silently ignore attempts to enable TSO. I think it
> should be something like:
>
> static int vlan_ethtool_set_tso(struct net_device *dev, u32 data)
> {
> if (data) {
> struct net_device *real_dev = vlan_dev_info(dev)->real_dev;
>
> /* Underlying device must support TSO for VLAN-tagged packets
> * and must have TSO enabled now. */
> if (!(real_dev->vlan_features & NETIF_F_TSO))
> return -EOPNOTSUPP;
> if (!(real_dev->features & NETIF_F_TSO))
> return -EINVAL;
>
> dev->features |= NETIF_F_TSO;
> } else {
> dev->features &= ~NETIF_F_TSO;
> }
>
> return 0;
> }
I agree.
What about other TSO flags like NETIF_F_TSO6 & NETIF_F_TSO_ECN ?
Not sure if we should manipulate them in set_tso()
^ permalink raw reply
* Re: [PATCH v2] vlan: allow TSO setting on vlan interfaces
From: Eric Dumazet @ 2010-07-08 16:04 UTC (permalink / raw)
To: Patrick McHardy; +Cc: Ben Hutchings, David Miller, netdev
In-Reply-To: <4C35F4E4.4070606@trash.net>
Le jeudi 08 juillet 2010 à 17:55 +0200, Patrick McHardy a écrit :
> I agree. The VLAN driver should also propagate feature changes
> upwards itself.
I noticed bonding is also at fault in this area.
bond0.xxx are not noticed of TSO changes on bond0
^ permalink raw reply
* Re: [PATCH v2] vlan: allow TSO setting on vlan interfaces
From: Ben Hutchings @ 2010-07-08 16:05 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Patrick McHardy
In-Reply-To: <1278604830.2651.32.camel@edumazet-laptop>
On Thu, 2010-07-08 at 18:00 +0200, Eric Dumazet wrote:
> Le jeudi 08 juillet 2010 à 16:49 +0100, Ben Hutchings a écrit :
> > On Thu, 2010-07-08 at 16:43 +0100, Ben Hutchings wrote:
> > > On Thu, 2010-07-08 at 11:39 +0200, Eric Dumazet wrote:
> > > > When we need to shape traffic with low speeds, we need to disable tso on
> > > > network interface :
> > > >
> > > > ethtool -K eth0.2240 tso off
> > > >
> > > > It seems vlan interfaces miss the set_tso() ethtool method.
> > > > Propagating tso changes from lower device is not always wanted, some
> > > > vlans want TSO on, others want TSO off.
> > [...]
> > > I think the vlan driver should also have a netdev notifier to handle
> > > feature changes on the underlying device.
> >
> > To clarify, I think offload features should be disabled on a vlan device
> > if they are later disabled on the underlying device. Propagating
> > changes to enable features, as you say, might not be wanted.
>
> OK, but isnt it already done ?
[...]
Yes, sorry for the noise.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH v2] vlan: allow TSO setting on vlan interfaces
From: Eric Dumazet @ 2010-07-08 16:12 UTC (permalink / raw)
To: Ben Hutchings; +Cc: David Miller, netdev, Patrick McHardy
In-Reply-To: <1278605109.16013.52.camel@achroite.uk.solarflarecom.com>
Le jeudi 08 juillet 2010 à 17:05 +0100, Ben Hutchings a écrit :
>
> Yes, sorry for the noise.
No problem, it was a good point to recheck anyway.
^ permalink raw reply
* [PATCH net-next-2.6] tg3: allow TSO on vlan devices
From: Eric Dumazet @ 2010-07-08 16:14 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Matt Carlson, Michael Chan
Similar to commit 72dccb01e8632aa (bnx2: Update vlan_features)
In order to enable TSO on vlan devices, tg3 needs to update
dev->vlan_features.
Tested on HP NC326m (aka BCM5715S (rev a3))
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
drivers/net/tg3.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 289cdc5..68329a9 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -12800,6 +12800,13 @@ done:
static struct pci_dev * __devinit tg3_find_peer(struct tg3 *);
+static void inline vlan_features_add(struct net_device *dev, unsigned long flags)
+{
+#if TG3_VLAN_TAG_USED
+ dev->vlan_features |= flags;
+#endif
+}
+
static int __devinit tg3_get_invariants(struct tg3 *tp)
{
static struct pci_device_id write_reorder_chipsets[] = {
@@ -13032,11 +13039,13 @@ static int __devinit tg3_get_invariants(struct tg3 *tp)
if (tp->pci_chip_rev_id == CHIPREV_ID_5700_B0)
tp->tg3_flags |= TG3_FLAG_BROKEN_CHECKSUMS;
else {
+ unsigned long features = NETIF_F_IP_CSUM | NETIF_F_SG | NETIF_F_GRO;
+
tp->tg3_flags |= TG3_FLAG_RX_CHECKSUMS;
- tp->dev->features |= NETIF_F_IP_CSUM | NETIF_F_SG;
if (tp->tg3_flags3 & TG3_FLG3_5755_PLUS)
- tp->dev->features |= NETIF_F_IPV6_CSUM;
- tp->dev->features |= NETIF_F_GRO;
+ features |= NETIF_F_IPV6_CSUM;
+ tp->dev->features |= features;
+ vlan_features_add(tp->dev, features);
}
/* Determine TSO capabilities */
@@ -14525,20 +14534,25 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
* is off by default, but can be enabled using ethtool.
*/
if ((tp->tg3_flags2 & TG3_FLG2_HW_TSO) &&
- (dev->features & NETIF_F_IP_CSUM))
+ (dev->features & NETIF_F_IP_CSUM)) {
dev->features |= NETIF_F_TSO;
-
+ vlan_features_add(dev, NETIF_F_TSO);
+ }
if ((tp->tg3_flags2 & TG3_FLG2_HW_TSO_2) ||
(tp->tg3_flags2 & TG3_FLG2_HW_TSO_3)) {
- if (dev->features & NETIF_F_IPV6_CSUM)
+ if (dev->features & NETIF_F_IPV6_CSUM) {
dev->features |= NETIF_F_TSO6;
+ vlan_features_add(dev, NETIF_F_TSO6);
+ }
if ((tp->tg3_flags2 & TG3_FLG2_HW_TSO_3) ||
GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5761 ||
(GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5784 &&
GET_CHIP_REV(tp->pci_chip_rev_id) != CHIPREV_5784_AX) ||
GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5785 ||
- GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_57780)
+ GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_57780) {
dev->features |= NETIF_F_TSO_ECN;
+ vlan_features_add(dev, NETIF_F_TSO_ECN);
+ }
}
if (tp->pci_chip_rev_id == CHIPREV_ID_5705_A1 &&
^ permalink raw reply related
* [PATCH] sysfs: Don't allow the creation of symlinks we can't remove
From: Eric W. Biederman @ 2010-07-08 16:31 UTC (permalink / raw)
To: Greg KH
Cc: Andrew Morton, Rafael J. Wysocki, Maciej W. Rozycki, Kay Sievers,
Johannes Berg, Greg KH, netdev
In-Reply-To: <20100622035631.GA3755@suse.de>
Recently my tagged sysfs support revealed a flaw in the device core
that a few rare drivers are running into such that we don't always put
network devices in a class subdirectory named net/.
Since we are not creating the class directory the network devices wind
up in a non-tagged directory, but the symlinks to the network devices
from /sys/class/net are in a tagged directory. All of which works
until we go to remove or rename the symlink. When we remove or rename
a symlink we look in the namespace of the target of the symlink.
Since the target of the symlink is in a non-tagged sysfs directory we
don't have a namespace to look in, and we fail to remove the symlink.
Detect this problem up front and simply don't create symlinks we won't
be able to remove later. This prevents symlink leakage and fails in
a much clearer and more understandable way.
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
fs/sysfs/symlink.c | 23 ++++++++++++++++++-----
1 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index f71246b..44bca5f 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -28,6 +28,7 @@ static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,
struct sysfs_dirent *target_sd = NULL;
struct sysfs_dirent *sd = NULL;
struct sysfs_addrm_cxt acxt;
+ enum kobj_ns_type ns_type;
int error;
BUG_ON(!name);
@@ -58,16 +59,28 @@ static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,
if (!sd)
goto out_put;
- if (sysfs_ns_type(parent_sd))
+ ns_type = sysfs_ns_type(parent_sd);
+ if (ns_type)
sd->s_ns = target->ktype->namespace(target);
sd->s_symlink.target_sd = target_sd;
target_sd = NULL; /* reference is now owned by the symlink */
sysfs_addrm_start(&acxt, parent_sd);
- if (warn)
- error = sysfs_add_one(&acxt, sd);
- else
- error = __sysfs_add_one(&acxt, sd);
+ /* Symlinks must be between directories with the same ns_type */
+ if (ns_type == sysfs_ns_type(sd->s_symlink.target_sd->s_parent)) {
+ if (warn)
+ error = sysfs_add_one(&acxt, sd);
+ else
+ error = __sysfs_add_one(&acxt, sd);
+ } else {
+ error = -EINVAL;
+ WARN(1, KERN_WARNING
+ "sysfs: symlink across ns_types %s/%s -> %s/%s\n",
+ parent_sd->s_name,
+ sd->s_name,
+ sd->s_symlink.target_sd->s_parent->s_name,
+ sd->s_symlink.target_sd->s_name);
+ }
sysfs_addrm_finish(&acxt);
if (error)
--
1.6.5.2.143.g8cc62
^ permalink raw reply related
* [PATCH v3] vlan: allow TSO setting on vlan interfaces
From: Eric Dumazet @ 2010-07-08 16:37 UTC (permalink / raw)
To: Ben Hutchings; +Cc: David Miller, netdev, Patrick McHardy
In-Reply-To: <1278604985.2651.35.camel@edumazet-laptop>
When we need to shape traffic using low speeds, we need to
disable tso on network interface :
ethtool -K eth0.2240 tso off
It seems vlan interfaces miss the set_tso() ethtool method.
Before enabling TSO, we must check real device supports
TSO for VLAN-tagged packets and enables TSO.
Note that a TSO change on real device propagates TSO setting
on all vlans, even if admin selected a different TSO setting.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/8021q/vlan_dev.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 7865a4c..a1b8171 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -836,12 +836,32 @@ static struct rtnl_link_stats64 *vlan_dev_get_stats64(struct net_device *dev, st
return stats;
}
+static int vlan_ethtool_set_tso(struct net_device *dev, u32 data)
+{
+ if (data) {
+ struct net_device *real_dev = vlan_dev_info(dev)->real_dev;
+
+ /* Underlying device must support TSO for VLAN-tagged packets
+ * and must have TSO enabled now.
+ */
+ if (!(real_dev->vlan_features & NETIF_F_TSO))
+ return -EOPNOTSUPP;
+ if (!(real_dev->features & NETIF_F_TSO))
+ return -EINVAL;
+ dev->features |= NETIF_F_TSO;
+ } else {
+ dev->features &= ~NETIF_F_TSO;
+ }
+ return 0;
+}
+
static const struct ethtool_ops vlan_ethtool_ops = {
.get_settings = vlan_ethtool_get_settings,
.get_drvinfo = vlan_ethtool_get_drvinfo,
.get_link = ethtool_op_get_link,
.get_rx_csum = vlan_ethtool_get_rx_csum,
.get_flags = vlan_ethtool_get_flags,
+ .set_tso = vlan_ethtool_set_tso,
};
static const struct net_device_ops vlan_netdev_ops = {
^ permalink raw reply related
* [RFC][PATCH] mac80211_hwsim: No parent is better than an illegimate one.
From: Eric W. Biederman @ 2010-07-08 16:37 UTC (permalink / raw)
To: Johannes Berg
Cc: Andrew Morton, Rafael J. Wysocki, Maciej W. Rozycki, Kay Sievers,
Johannes Berg, Greg KH, Greg KH <gregkh@suse.de> netdev
In-Reply-To: <m1630q7x5v.fsf_-_@fess.ebiederm.org>
Don't call SET_IEEE80211_DEV. This weakens the connections between
the phy files in sysfs slightly but otherwise it makes the driver work
in the face of tagged sysfs support.
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
Johannes does this change look usable?
drivers/net/wireless/mac80211_hwsim.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 6f8cb3e..b387222 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -1282,7 +1282,6 @@ static int __init init_mac80211_hwsim(void)
}
data->dev->driver = &mac80211_hwsim_driver;
- SET_IEEE80211_DEV(hw, data->dev);
addr[3] = i >> 8;
addr[4] = i;
memcpy(data->addresses[0].addr, addr, ETH_ALEN);
--
1.6.5.2.143.g8cc62
^ permalink raw reply related
* Re: [Bugme-new] [Bug 16257] New: sysfs changes break hwsim and bnep drivers
From: Eric W. Biederman @ 2010-07-08 16:55 UTC (permalink / raw)
To: Greg KH
Cc: Andrew Morton, Rafael J. Wysocki, Maciej W. Rozycki, Kay Sievers,
Johannes Berg, Greg KH, netdev
In-Reply-To: <20100622035631.GA3755@suse.de>
Greg KH <gregkh@suse.de> writes:
> Does this only show up if you enable network namespaces? Or is it a
> problem with the "normal" kernel configuration of no network namespaces?
It is a problem with the normal kernel configuration. In that case
we have a single network namespace.
> The only thing that changed here was your network namespace work, what
> caused this problem to show up? Was it bisectable down to a single
> patch?
The problem is that when we remove symlinks we look at the target of
the symlink to see which tag to remove it in it's parent directory.
Because the target of the symlink was not in a class directory because
of that crazy class on class stacking exception in get_device_parent
we fail to find the symlink when we attempt to remove it.
My one line patch to get_device_parent really does fix this. I have
just sent you a patch to prevent the creation of these crazy symlinks
in sysfs, which at least clearly isolates this to a handful of drivers.
> Classes on top of classes should never have originally worked, I guess
> we just let them slide by accident, and we can go and fix them up as
> they are found. But for now, for .35, it would be good to find the root
> cause of the problem here. It might be as simple as putting a
> CONFIG_BROKEN on network namespaces until we get this working, right?
Nope, it isn't as simple as disabling network namespaces.
For the mac80211_hwsim we can just remove the device parent, and all will
be well for the moment.
For the bnep code we could also not set the parent but I don't know if
that would have undesirable complications for power management or not.
I don't have a bluetooth attached network device so I don't even know how
to test that code.
My preferred fix is the one liner I set you to get_device_parent. It
is no worse than the magic device_is_not_partition checks then we
already have in sysfs. Especially when it is a bug that
get_device_parent does that early return in the first place. To fix
this we will have to introduce the missing subdirectory one way or
another.
With SYSFS_DEPRECATED enabled I don't believe this problem can actually
happen, as all network devices are placed immediately under
/sys/class/net/ in sysfs.
Eric
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox