* Re: [patch] net/ethernet: ks8851_mll unregister_netdev() before freeing
From: Raffaele Recalcati @ 2012-06-06 17:03 UTC (permalink / raw)
To: Dan Carpenter; +Cc: David S. Miller, netdev, kernel-janitors
In-Reply-To: <20120606063129.GA26829@elgon.mountain>
On 09:31 Wed 06 Jun , Dan Carpenter wrote:
> We added another error condition here, but if we were to hit it then
> we need to unregister_netdev() before doing the free_netdev().
> Otherwise we would hit the BUG_ON() in free_netdev():
>
> BUG_ON(dev->reg_state != NETREG_UNREGISTERED);
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Static checker stuff. I don't have this hardware.
>
> diff --git a/drivers/net/ethernet/micrel/ks8851_mll.c b/drivers/net/ethernet/micrel/ks8851_mll.c
> index 70bd329..875dd5e 100644
> --- a/drivers/net/ethernet/micrel/ks8851_mll.c
> +++ b/drivers/net/ethernet/micrel/ks8851_mll.c
> @@ -1606,7 +1606,7 @@ static int __devinit ks8851_probe(struct platform_device *pdev)
> if (!pdata) {
> netdev_err(netdev, "No platform data\n");
> err = -ENODEV;
> - goto err_register;
> + goto err_pdata;
> }
> memcpy(ks->mac_addr, pdata->mac_addr, 6);
> if (!is_valid_ether_addr(ks->mac_addr)) {
> @@ -1626,6 +1626,8 @@ static int __devinit ks8851_probe(struct platform_device *pdev)
> (id >> 8) & 0xff, (id >> 4) & 0xf, (id >> 1) & 0x7);
> return 0;
>
> +err_pdata:
> + unregister_netdev(netdev);
> err_register:
> err_get_irq:
> iounmap(ks->hw_addr_cmd);
Tested-by: Raffaele Recalcati <raffaele.recalcati@bticino.it>
[ 2.274993] ks8851_mll ks8851_mll: (unregistered net_device): message enable is 0
[ 2.282897] ks8851_mll ks8851_mll: (unregistered net_device): the selftest passes
[ 2.302246] ks8851_mll ks8851_mll: eth0: No platform data
^ permalink raw reply
* Re: [PATCH] virtio-net: fix a race on 32bit arches
From: Eric Dumazet @ 2012-06-06 17:13 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jason Wang, netdev, rusty, linux-kernel, virtualization,
Stephen Hemminger
In-Reply-To: <20120606161715.GA17575@redhat.com>
On Wed, 2012-06-06 at 19:17 +0300, Michael S. Tsirkin wrote:
> But why do you say at most 1 packet?
>
> Consider get_stats doing:
> u64_stats_update_begin(&stats->syncp);
> stats->tx_bytes += skb->len;
>
> on 64 bit at this point
> tx_packets might get incremented any number of times, no?
>
> stats->tx_packets++;
> u64_stats_update_end(&stats->syncp);
>
> now tx_bytes and tx_packets are out of sync by more than 1.
You lost me there.
No idea of what you are thinking about.
There is no atomicity guarantee in SNMP counters. (Ie fetching tx_bytes
and tx_packets in a transaction is not mandatory in any RFC)
As long as there is no cumulative error, its OK.
^ permalink raw reply
* [PATCH net-next] fec: Add support for Coldfire M5441x enet-mac.
From: Steven King @ 2012-06-06 17:06 UTC (permalink / raw)
To: netdev; +Cc: uClinux development list, Greg Ungerer
Add support for the Freescale Coldfire M5441x; as these parts have an
enet-mac, add a quirk to distinguish them from the other Coldfire parts so we
can use the existing enet-mac support.
Signed-off-by: Steven king <sfking@fdwdc.com>
---
drivers/net/ethernet/freescale/Kconfig | 5 ++---
drivers/net/ethernet/freescale/fec.c | 6 +++++-
drivers/net/ethernet/freescale/fec.h | 3 ++-
3 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/freescale/Kconfig b/drivers/net/ethernet/freescale/Kconfig
index 3574e14..f9aa244 100644
--- a/drivers/net/ethernet/freescale/Kconfig
+++ b/drivers/net/ethernet/freescale/Kconfig
@@ -7,7 +7,7 @@ config NET_VENDOR_FREESCALE
default y
depends on FSL_SOC || QUICC_ENGINE || CPM1 || CPM2 || PPC_MPC512x || \
M523x || M527x || M5272 || M528x || M520x || M532x || \
- ARCH_MXC || ARCH_MXS || (PPC_MPC52xx && PPC_BESTCOMM)
+ M5441x || ARCH_MXC || ARCH_MXS || (PPC_MPC52xx && PPC_BESTCOMM)
---help---
If you have a network (Ethernet) card belonging to this class, say Y
and read the Ethernet-HOWTO, available from
@@ -22,8 +22,7 @@ if NET_VENDOR_FREESCALE
config FEC
tristate "FEC ethernet controller (of ColdFire and some i.MX CPUs)"
- depends on (M523x || M527x || M5272 || M528x || M520x || M532x || \
- ARCH_MXC || SOC_IMX28)
+ depends on (M523x || M527x || M5272 || M528x || M520x || M532x || M5441x || ARCH_MXC || SOC_IMX28)
default ARCH_MXC || SOC_IMX28 if ARM
select PHYLIB
---help---
diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index ff7f4c5..9567667 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -94,6 +94,9 @@ static struct platform_device_id fec_devtype[] = {
.name = "imx6q-fec",
.driver_data = FEC_QUIRK_ENET_MAC | FEC_QUIRK_HAS_GBIT,
}, {
+ .name = "enet-fec",
+ .driver_data = FEC_QUIRK_ENET_MAC,
+ }, {
/* sentinel */
}
};
@@ -187,7 +190,8 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
* account when setting it.
*/
#if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \
- defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARM)
+ defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARM) || \
+ defined(CONFIG_M5441x)
#define OPT_FRAME_SIZE (PKT_MAXBUF_SIZE << 16)
#else
#define OPT_FRAME_SIZE 0
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 8408c62..298cfb7 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -15,7 +15,8 @@
#if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \
defined(CONFIG_M520x) || defined(CONFIG_M532x) || \
- defined(CONFIG_ARCH_MXC) || defined(CONFIG_SOC_IMX28)
+ defined(CONFIG_ARCH_MXC) || defined(CONFIG_SOC_IMX28) || \
+ defined(CONFIG_M5441x)
/*
* Just figures, Motorola would have to change the offsets for
* registers in the same peripheral device on different models
^ permalink raw reply related
* Re: Strange latency spikes/TX network stalls on Sun Fire X4150(x86) and e1000e
From: Eric Dumazet @ 2012-06-06 17:05 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: Hiroaki SHIMODA, Tom Herbert, Denys Fedoryshchenko, netdev,
e1000-devel, jeffrey.t.kirsher, davem
In-Reply-To: <20120606092635.00003b61@unknown>
On Wed, 2012-06-06 at 09:26 -0700, Jesse Brandeburg wrote:
> On Wed, 6 Jun 2012 Hiroaki SHIMODA <shimoda.hiroaki@gmail.com> wrote:
> > Sorry for long delay. I'll post.
> > (I have no idea how to fix this problem as keeping TXDCTL.WTHRESH to 5)
>
> I don't like changing WTHRESH wholesale because making the global change
> to WTHRESH on e1000e just to fix this one bug (likely specific to a
> particular chip/hardware) will adversely effect performance on many
> models supported by e1000e not demonstrating any problem. We could
> possibly check something in for just ESB2LAN (S5000 chipset).
>
> Other people (tom herbert) with this same chipset have been unable to
> reproduce this issue right?
Thats because Tom Herbert (and me on my T420s laptop)
dont have FLAG2_DMA_BURST in adapter->flags2
So it depends on some e1000e, not all...
You cant hold a TX completion indefinitely, this breaks BQL but also
other stuff.
^ permalink raw reply
* Re: Possible deadlock in ipv6?
From: Eric Dumazet @ 2012-06-06 17:02 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: netdev@vger.kernel.org
In-Reply-To: <D0983DA9-2545-4007-8A69-31DDCB9B7CCF@parallels.com>
On Wed, 2012-06-06 at 20:01 +0400, Vladimir Davydov wrote:
> On Jun 6, 2012, at 7:58 PM, Eric Dumazet wrote:
>
> > On Wed, 2012-06-06 at 17:53 +0200, Eric Dumazet wrote:
> >> On Wed, 2012-06-06 at 18:49 +0400, Vladimir Davydov wrote:
> >>> I'm not familiar with the linux net subsystem, so I would appreciate if
> >>> someone could clarify if the following call chain is possible:
> >>>
> >>> addrconf_ifdown() calls neigh_ifdown(nd_tbl) which locks nd_tbl.lock for
> >>> writing and calls
> >>>
> >>> pneigh_ifdown
> >>> pndisc_destructor
> >>> ipv6_dev_mc_dec
> >>> __ipv6_dev_mc_dec
> >>> igmp6_group_dropped
> >>> igmp6_leave_group
> >>> igmp6_send
> >>> icmp6_dst_alloc
> >>> ip6_neigh_lookup
> >>> neigh_create
> >>>
> >>> and neigh_create() locks nd_tbl.lock for writing again resulting in a
> >>> deadlock.
> >>
> >> It seems a deadlock is possible indeed, good catch !
> >>
> >>
> >
> > And it seems this neigh_down() can be removed, its called later
> > (after dev->ip6_ptr is cleared)
> >
>
> BTW, commit d1ed113f1669390da9898da3beddcc058d938587 did exactly the same, but it was reverted along with a bundle of other commits by 73a8bd74e2618990dbb218c3d82f53e60acd9af0.
Yes, but the revert was a 'revert a serie', while this particular patch
seems fine, especially if fixing a deadlock ;)
^ permalink raw reply
* Re: [PATCH] virtio-net: fix a race on 32bit arches
From: Michael S. Tsirkin @ 2012-06-06 16:57 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, linux-kernel, virtualization, Stephen Hemminger
In-Reply-To: <1338995944.26966.6.camel@edumazet-glaptop>
On Wed, Jun 06, 2012 at 05:19:04PM +0200, Eric Dumazet wrote:
> On Wed, 2012-06-06 at 17:49 +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 06, 2012 at 03:10:10PM +0200, Eric Dumazet wrote:
> > > On Wed, 2012-06-06 at 14:13 +0300, Michael S. Tsirkin wrote:
> > >
> > > > We currently do all stats either on napi callback or from
> > > > start_xmit callback.
> > > > This makes them safe, yes?
> > >
> > > Hmm, then _bh() variant is needed in virtnet_stats(), as explained in
> > > include/linux/u64_stats_sync.h section 6)
> > >
> > > * 6) If counter might be written by an interrupt, readers should block interrupts.
> > > * (On UP, there is no seqcount_t protection, a reader allowing interrupts could
> > > * read partial values)
> > >
> > > Yes, its tricky...
> >
> > Sounds good, but I have a question: this realies on counters
> > being atomic on 64 bit.
> > Would not it be better to always use a seqlock even on 64 bit?
> > This way counters would actually be correct and in sync.
> > As it is if we want e.g. average packet size,
> > we can not rely e.g. on it being bytes/packets.
>
> When this stuff was discussed, we chose to have a nop on 64bits.
>
> Your point has little to do with 64bit stats, it was already like that
> with 'long int' counters.
>
> Consider average driver doing :
>
> dev->stats.rx_bytes += skb->len;
> dev->stats.rx_packets++;
>
> A concurrent reader can read an updated rx_bytes and a 'previous'
> rx_packets one.
>
> 'fixing' this requires a lot of work and memory barriers (in all
> drivers), for a very litle gain (at most one packet error)
>
> u64_stats_sync was really meant to be 0-cost on 64bit arches.
>
So for virtio since all counters get incremented from bh we can
ensure they are read atomically, simply but reading them
from the correct CPU with bh disabled.
And then we don't need u64_stats_sync at all.
--
MST
^ permalink raw reply
* Re: [PATCH net-next] x86 bpf_jit: support BPF_S_ANC_ALU_XOR_X instruction
From: David Miller @ 2012-06-06 16:44 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, jpirko, mgherzan, matt
In-Reply-To: <1338881190.2760.1992.camel@edumazet-glaptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 05 Jun 2012 09:26:30 +0200
> From: Eric Dumazet <edumazet@google.com>
>
> commit ffe06c17afbb (filter: add XOR operation) added generic support
> for XOR operation.
>
> This patch implements the XOR instruction in x86 jit.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Jiri Pirko <jpirko@redhat.com>
Applied, thanks Eric. I'll take care of Sparc.
ARM and POWERPC folks, this is just a reminder for you guys
to do this too. Adding support for XOR to your JITs ought to
be quite trivial.
Thanks.
^ permalink raw reply
* Re: [PATCH v9] tilegx network driver: initial support
From: David Miller @ 2012-06-06 16:41 UTC (permalink / raw)
To: cmetcalf; +Cc: bhutchings, arnd, linux-kernel, netdev
In-Reply-To: <201206042023.q54KNEZp003834@farm-0002.internal.tilera.com>
From: Chris Metcalf <cmetcalf@tilera.com>
Date: Mon, 4 Jun 2012 16:12:03 -0400
> This change adds support for the tilegx network driver based on the
> GXIO IORPC support in the tilegx software stack, using the on-chip
> mPIPE packet processing engine.
>
> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
> ---
> This change fixes some bugs that were discovered during additional
> testing of the TSO refactoring. In addition, I added a comment
> explaining why we provide TSO support as essentially driver-side GSO.
>
> The previous v8 version of the patch (from 10 days ago) received no
> feedback; if anyone would care to provide feedback on this version of
> the driver, it would be much appreciated. Thanks!
Someone other than me please review this driver.
^ permalink raw reply
* [PATCH] Net: mv643xx_eth: Fix compile error for architectures without clk.
From: Andrew Lunn @ 2012-06-06 16:40 UTC (permalink / raw)
To: jwboyer; +Cc: buytenh, olof, netdev, ben, Andrew Lunn
Commit 452503ebc (ARM: Orion: Eth: Add clk/clkdev support.) broke
the building of the driver on architectures which don't have clk
support. In particular PPC32 Pegasos which uses this driver.
Add #ifdef around the clk API usage.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
drivers/net/ethernet/marvell/mv643xx_eth.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index 04d901d..f0f06b2 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -436,7 +436,9 @@ struct mv643xx_eth_private {
/*
* Hardware-specific parameters.
*/
+#if defined(CONFIG_HAVE_CLK)
struct clk *clk;
+#endif
unsigned int t_clk;
};
@@ -2895,17 +2897,17 @@ static int mv643xx_eth_probe(struct platform_device *pdev)
mp->dev = dev;
/*
- * Get the clk rate, if there is one, otherwise use the default.
+ * Start with a default rate, and if there is a clock, allow
+ * it to override the default.
*/
+ mp->t_clk = 133000000;
+#if defined(CONFIG_HAVE_CLK)
mp->clk = clk_get(&pdev->dev, (pdev->id ? "1" : "0"));
if (!IS_ERR(mp->clk)) {
clk_prepare_enable(mp->clk);
mp->t_clk = clk_get_rate(mp->clk);
- } else {
- mp->t_clk = 133000000;
- printk(KERN_WARNING "Unable to get clock");
}
-
+#endif
set_params(mp, pd);
netif_set_real_num_tx_queues(dev, mp->txq_count);
netif_set_real_num_rx_queues(dev, mp->rxq_count);
@@ -2995,10 +2997,13 @@ static int mv643xx_eth_remove(struct platform_device *pdev)
phy_detach(mp->phy);
cancel_work_sync(&mp->tx_timeout_task);
+#if defined(CONFIG_HAVE_CLK)
if (!IS_ERR(mp->clk)) {
clk_disable_unprepare(mp->clk);
clk_put(mp->clk);
}
+#endif
+
free_netdev(mp->dev);
platform_set_drvdata(pdev, NULL);
--
1.7.10
^ permalink raw reply related
* RE: [net-next PATCH v2 1/3] Added kernel support in EEE Ethtool commands
From: Yuval Mintz @ 2012-06-06 16:40 UTC (permalink / raw)
To: Ben Hutchings
Cc: davem@davemloft.net, netdev@vger.kernel.org, Eilon Greenstein,
peppe.cavallaro@st.com
In-Reply-To: <1338996011.2836.2.camel@bwh-desktop.uk.solarflarecom.com>
> > + * @supported: Link speeds for which there is eee support.
> > + * @advertised: Link speeds the interface advertises (AN) as eee capable.
> > + * @lp_advertised: Link speeds the link partner advertised as eee capable.
>
> And these are bitmasks of SUPPORTED_* & ADVERTISED_* flags, right?
Right.
> Maybe 'link modes' not 'link speeds'?
Not that it matters greatly, but there are SUPPORTED & ADVERTISED flags for
things other than link speeds, such as connection type and flow control,
so using exactly the same semantic in description might confuse someone.
Thank,
Yuval
^ permalink raw reply
* Re: [PATCH (net.git) 0/3 (v3)] stmmac fixes for net.git
From: David Miller @ 2012-06-06 16:35 UTC (permalink / raw)
To: peppe.cavallaro; +Cc: netdev
In-Reply-To: <1338873777-5374-1-git-send-email-peppe.cavallaro@st.com>
From: Giuseppe CAVALLARO <peppe.cavallaro@st.com>
Date: Tue, 5 Jun 2012 07:22:54 +0200
> These patches fix a problem in the driver when built as dynamic
> module and fix the driver's documentation.
>
> v2: this patchset has the same patches I had sent before but
> I removed a patch that did a cleanup (now moved for net-next).
>
> v3: removed wrong reviewed-by from this patch:
> "stmmac: fix driver Kconfig when built as module"
>
> Giuseppe Cavallaro (3):
> stmmac: fix driver's doc when run kernel-doc script
> stmmac: update driver's doc
> stmmac: fix driver Kconfig when built as module
All applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next 0/3] Remove casts to same type
From: David Miller @ 2012-06-06 16:33 UTC (permalink / raw)
To: joe; +Cc: netdev
In-Reply-To: <cover.1338849364.git.joe@perches.com>
From: Joe Perches <joe@perches.com>
Date: Mon, 4 Jun 2012 15:44:15 -0700
> Adding casts of objects to the same type is unnecessary
> and confusing for a human reader.
>
> Remove them via coccinelle script.
>
> No additional sparse warnings are emitted.
>
> Joe Perches (3):
> ethernet: Remove casts to same type
> wireless: Remove casts to same type
> drivers: net: Remove casts to same type
All applied, thanks Joe.
^ permalink raw reply
* RE: [PATCH 1/2] Ethtool: Add EEE support
From: Yuval Mintz @ 2012-06-06 16:27 UTC (permalink / raw)
To: Ben Hutchings
Cc: netdev@vger.kernel.org, Eilon Greenstein, peppe.cavallaro@st.com
In-Reply-To: <1338997734.2836.17.camel@bwh-desktop.uk.solarflarecom.com>
Hi Ben,
I stand corrected; I'll supply a new version shortly.
I've got 2 questions though:
> For now, use int for booleans. At some point I would like to see a
> thorough cleanup of ethtool to use bool where appropriate - but that's
> independent of this
I've noticed that the 'do_generic_set' function assumes all fields are ints.
Is this a convention we should stick to (using __u32 in the ethtool structs)?
I'm asking because I'm "wasting" fields in the ethtool_eee struct as I use
__u32 for boolean fields, simply because what seems to be the conventional
method won't work with smaller fields (corrupts the following fields).
The seconds question - is there a dependency between your acceptance of
this patch series and Dave's acceptance of the kernel's ethtool modification?
I'm asking because changes in the ethtool header there should be applied in
this patch series as well (in ethtool-copy.h).
Thanks,
Yuval
^ permalink raw reply
* Re: Strange latency spikes/TX network stalls on Sun Fire X4150(x86) and e1000e
From: Jesse Brandeburg @ 2012-06-06 16:26 UTC (permalink / raw)
To: Hiroaki SHIMODA
Cc: Eric Dumazet, Tom Herbert, Denys Fedoryshchenko, netdev,
e1000-devel, jeffrey.t.kirsher, davem
In-Reply-To: <20120606174303.0bfc9868.shimoda.hiroaki@gmail.com>
On Wed, 6 Jun 2012 Hiroaki SHIMODA <shimoda.hiroaki@gmail.com> wrote:
> Sorry for long delay. I'll post.
> (I have no idea how to fix this problem as keeping TXDCTL.WTHRESH to 5)
I don't like changing WTHRESH wholesale because making the global change
to WTHRESH on e1000e just to fix this one bug (likely specific to a
particular chip/hardware) will adversely effect performance on many
models supported by e1000e not demonstrating any problem. We could
possibly check something in for just ESB2LAN (S5000 chipset).
Other people (tom herbert) with this same chipset have been unable to
reproduce this issue right?
^ permalink raw reply
* Re: [PATCH] virtio-net: fix a race on 32bit arches
From: Michael S. Tsirkin @ 2012-06-06 16:17 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, linux-kernel, virtualization, Stephen Hemminger
In-Reply-To: <1338995944.26966.6.camel@edumazet-glaptop>
On Wed, Jun 06, 2012 at 05:19:04PM +0200, Eric Dumazet wrote:
> On Wed, 2012-06-06 at 17:49 +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 06, 2012 at 03:10:10PM +0200, Eric Dumazet wrote:
> > > On Wed, 2012-06-06 at 14:13 +0300, Michael S. Tsirkin wrote:
> > >
> > > > We currently do all stats either on napi callback or from
> > > > start_xmit callback.
> > > > This makes them safe, yes?
> > >
> > > Hmm, then _bh() variant is needed in virtnet_stats(), as explained in
> > > include/linux/u64_stats_sync.h section 6)
> > >
> > > * 6) If counter might be written by an interrupt, readers should block interrupts.
> > > * (On UP, there is no seqcount_t protection, a reader allowing interrupts could
> > > * read partial values)
> > >
> > > Yes, its tricky...
> >
> > Sounds good, but I have a question: this realies on counters
> > being atomic on 64 bit.
> > Would not it be better to always use a seqlock even on 64 bit?
> > This way counters would actually be correct and in sync.
> > As it is if we want e.g. average packet size,
> > we can not rely e.g. on it being bytes/packets.
>
> When this stuff was discussed, we chose to have a nop on 64bits.
>
> Your point has little to do with 64bit stats, it was already like that
> with 'long int' counters.
Yes, of course.
> Consider average driver doing :
>
> dev->stats.rx_bytes += skb->len;
> dev->stats.rx_packets++;
>
> A concurrent reader can read an updated rx_bytes and a 'previous'
> rx_packets one.
>
> 'fixing' this requires a lot of work and memory barriers (in all
> drivers), for a very litle gain (at most one packet error)
> u64_stats_sync was really meant to be 0-cost on 64bit arches.
>
>
I understand, and not arguing about that.
But why do you say at most 1 packet?
Consider get_stats doing:
u64_stats_update_begin(&stats->syncp);
stats->tx_bytes += skb->len;
on 64 bit at this point
tx_packets might get incremented any number of times, no?
stats->tx_packets++;
u64_stats_update_end(&stats->syncp);
now tx_bytes and tx_packets are out of sync by more than 1.
^ permalink raw reply
* Re: [PATCH net-next 1/7] be2net: don't call vid_config() when there's no vlan config
From: David Miller @ 2012-06-06 16:09 UTC (permalink / raw)
To: sathya.perla; +Cc: netdev
In-Reply-To: <9e61e8da-5c37-448c-8c07-71fa8b81fb5c@exht1.ad.emulex.com>
Series applied, thank you.
^ permalink raw reply
* Re: Possible deadlock in ipv6?
From: Vladimir Davydov @ 2012-06-06 16:01 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev@vger.kernel.org
In-Reply-To: <1338998314.26966.12.camel@edumazet-glaptop>
On Jun 6, 2012, at 7:58 PM, Eric Dumazet wrote:
> On Wed, 2012-06-06 at 17:53 +0200, Eric Dumazet wrote:
>> On Wed, 2012-06-06 at 18:49 +0400, Vladimir Davydov wrote:
>>> I'm not familiar with the linux net subsystem, so I would appreciate if
>>> someone could clarify if the following call chain is possible:
>>>
>>> addrconf_ifdown() calls neigh_ifdown(nd_tbl) which locks nd_tbl.lock for
>>> writing and calls
>>>
>>> pneigh_ifdown
>>> pndisc_destructor
>>> ipv6_dev_mc_dec
>>> __ipv6_dev_mc_dec
>>> igmp6_group_dropped
>>> igmp6_leave_group
>>> igmp6_send
>>> icmp6_dst_alloc
>>> ip6_neigh_lookup
>>> neigh_create
>>>
>>> and neigh_create() locks nd_tbl.lock for writing again resulting in a
>>> deadlock.
>>
>> It seems a deadlock is possible indeed, good catch !
>>
>>
>
> And it seems this neigh_down() can be removed, its called later
> (after dev->ip6_ptr is cleared)
>
BTW, commit d1ed113f1669390da9898da3beddcc058d938587 did exactly the same, but it was reverted along with a bundle of other commits by 73a8bd74e2618990dbb218c3d82f53e60acd9af0.
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 8f6411c..62c4c00 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -2750,7 +2750,6 @@ static int addrconf_ifdown(struct net_device *dev, int how)
> ASSERT_RTNL();
>
> rt6_ifdown(net, dev);
> - neigh_ifdown(&nd_tbl, dev);
>
> idev = __in6_dev_get(dev);
> if (idev == NULL)
>
>
^ permalink raw reply
* Re: [net 0/3][pull request] Intel Wired LAN Driver Updates
From: David Miller @ 2012-06-06 16:00 UTC (permalink / raw)
To: jeffrey.t.kirsher; +Cc: netdev, gospo, sassmann
In-Reply-To: <1338955735-8967-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Tue, 5 Jun 2012 21:08:52 -0700
> This series contains 2 fixes for ixgbe and a fix for e1000e.
>
> The following are changes since commit dc5cd894cace7bda4a743487a9f87d59a3f0a095:
> net/hyperv: Use wait_event on outstanding sends during device removal
> and are available in the git repository at:
> git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/net master
Pulled, thanks Jeff.
^ permalink raw reply
* Re: Possible deadlock in ipv6?
From: Eric Dumazet @ 2012-06-06 15:58 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: netdev
In-Reply-To: <1338998019.26966.10.camel@edumazet-glaptop>
On Wed, 2012-06-06 at 17:53 +0200, Eric Dumazet wrote:
> On Wed, 2012-06-06 at 18:49 +0400, Vladimir Davydov wrote:
> > I'm not familiar with the linux net subsystem, so I would appreciate if
> > someone could clarify if the following call chain is possible:
> >
> > addrconf_ifdown() calls neigh_ifdown(nd_tbl) which locks nd_tbl.lock for
> > writing and calls
> >
> > pneigh_ifdown
> > pndisc_destructor
> > ipv6_dev_mc_dec
> > __ipv6_dev_mc_dec
> > igmp6_group_dropped
> > igmp6_leave_group
> > igmp6_send
> > icmp6_dst_alloc
> > ip6_neigh_lookup
> > neigh_create
> >
> > and neigh_create() locks nd_tbl.lock for writing again resulting in a
> > deadlock.
>
> It seems a deadlock is possible indeed, good catch !
>
>
And it seems this neigh_down() can be removed, its called later
(after dev->ip6_ptr is cleared)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 8f6411c..62c4c00 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2750,7 +2750,6 @@ static int addrconf_ifdown(struct net_device *dev, int how)
ASSERT_RTNL();
rt6_ifdown(net, dev);
- neigh_ifdown(&nd_tbl, dev);
idev = __in6_dev_get(dev);
if (idev == NULL)
^ permalink raw reply related
* Re: [PATCH 2/2] Ethtool: add EEE to ethtool's documentation
From: Ben Hutchings @ 2012-06-06 15:56 UTC (permalink / raw)
To: Yuval Mintz; +Cc: netdev, eilong, peppe.cavallaro
In-Reply-To: <1338878470-24784-3-git-send-email-yuvalmin@broadcom.com>
On Tue, 2012-06-05 at 09:41 +0300, Yuval Mintz wrote:
> under Synopsis:
> ethtool --get-eee devname
> ethtool --set-eee devname [eee on|off] [tx-lpi on|off] [tx-timer N] [advertise N]
>
> under Options:
> --get-eee
> Queries the specified network device for its support in Efficient Energy Ethernet (ac-
> cording to the IEEE 802.3az specifications)
> --set-eee
> Sets the device EEE behaviour.
> eee on|off
> Enables/Disables the device support in EEE.
> tx-lpi on|off
> Determines whether the device should assert its tx lpi.
> advertise N
> Sets the speeds for which the device would advertise EEE capabliities. Values are as
> for --change advertise
> tx-timer N
> Sets the amount of time the device should stay in idle mode prior to asserting its tx
> lpi (in microseconds). This has meaning only when tx lpi is on.
Please just fold this change into the patch that adds the options. Also
there is no need to repeat the content in the commit message.
> Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
> Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
> ---
> ethtool.8.in | 32 ++++++++++++++++++++++++++++++++
> 1 files changed, 32 insertions(+), 0 deletions(-)
>
> diff --git a/ethtool.8.in b/ethtool.8.in
> index 523b737..b906d8e 100644
> --- a/ethtool.8.in
> +++ b/ethtool.8.in
> @@ -335,6 +335,16 @@ ethtool \- query or control network driver and hardware settings
> .I devname flag
> .A1 on off
> .RB ...
> +.HP
> +.B ethtool \-\-get\-eee
> +.I devname
> +.HP
> +.B ethtool \-\-set\-eee
> +.I devname
> +.B2 eee on off
> +.B2 tx-lpi on off
> +.BN tx-timer
> +.BN advertise
> .
> .\" Adjust lines (i.e. full justification) and hyphenate.
> .ad
> @@ -817,6 +827,28 @@ Sets the device's private flags as specified.
> .I flag
> .A1 on off
> Sets the state of the named private flag.
> +.TP
> +.B \-\-get\-eee
> +Queries the specified network device for its support in Efficient Energy
> +Ethernet (according to the IEEE 802.3az specifications)
'of', not 'in'
> +.TP
> +.B \-\-set\-eee
> +Sets the device EEE behaviour.
> +.TP
> +.A2 eee on off
> +Enables/Disables the device support in EEE.
Lower-case 'd' for 'disables'.
'of', not 'in'
> +.TP
> +.A2 tx-lpi on off
> +Determines whether the device should assert its tx lpi.
'TX LPI' should be in upper-case.
> +.TP
> +.BI advertise \ N
> +Sets the speeds for which the device would advertise EEE capabliities.
'would', not 'should'
'capabilities', not 'capabliities'
> +Values are as for
> +.B \-\-change advertise
> +.TP
> +.BI tx-timer \ N
> +Sets the amount of time the device should stay in idle mode prior to asserting
> +its tx lpi (in microseconds). This has meaning only when tx lpi is on.
Same here.
Ben.
> .SH BUGS
> Not supported (in part or whole) on all network drivers.
> .SH AUTHOR
--
Ben Hutchings, Staff Engineer, Solarflare
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: Possible deadlock in ipv6?
From: Eric Dumazet @ 2012-06-06 15:53 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: netdev
In-Reply-To: <4FCF6DF4.2090304@parallels.com>
On Wed, 2012-06-06 at 18:49 +0400, Vladimir Davydov wrote:
> I'm not familiar with the linux net subsystem, so I would appreciate if
> someone could clarify if the following call chain is possible:
>
> addrconf_ifdown() calls neigh_ifdown(nd_tbl) which locks nd_tbl.lock for
> writing and calls
>
> pneigh_ifdown
> pndisc_destructor
> ipv6_dev_mc_dec
> __ipv6_dev_mc_dec
> igmp6_group_dropped
> igmp6_leave_group
> igmp6_send
> icmp6_dst_alloc
> ip6_neigh_lookup
> neigh_create
>
> and neigh_create() locks nd_tbl.lock for writing again resulting in a
> deadlock.
It seems a deadlock is possible indeed, good catch !
^ permalink raw reply
* Re: [PATCH 1/2] Ethtool: Add EEE support
From: Ben Hutchings @ 2012-06-06 15:48 UTC (permalink / raw)
To: Yuval Mintz; +Cc: netdev, eilong, peppe.cavallaro
In-Reply-To: <1338878470-24784-2-git-send-email-yuvalmin@broadcom.com>
On Tue, 2012-06-05 at 09:41 +0300, Yuval Mintz wrote:
> This patch adds 2 new ethtool commands which can be
> used to manipulate network interfaces' support in
> EEE.
>
> Output of 'get' has the following form:
>
> EEE Settings for p2p1:
> EEE status: enabled - active
> Tx LPI: 1000 (u)
> Supported EEE link modes: 10000baseT/Full
> Advertised EEE link modes: 10000baseT/Full
> Link partner advertised EEE link modes: 10000baseT/Full
>
> Thanks goes to Giuseppe Cavallaro for his original patch.
[...]
> diff --git a/ethtool.c b/ethtool.c
> index f18f611..063e72b 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -359,7 +359,8 @@ static int do_version(struct cmd_context *ctx)
> return 0;
> }
>
> -static void dump_link_caps(const char *prefix, const char *an_prefix, u32 mask);
> +static void dump_link_caps(const char *prefix, const char *an_prefix, u32 mask,
> + u8 all);
For now, use int for booleans. At some point I would like to see a
thorough cleanup of ethtool to use bool where appropriate - but that's
independent of this.
[...]
> /* Print link capability flags (supported, advertised or lp_advertised).
> * Assumes that the corresponding SUPPORTED and ADVERTISED flags are equal.
> */
> static void
> -dump_link_caps(const char *prefix, const char *an_prefix, u32 mask)
> +dump_link_caps(const char *prefix, const char *an_prefix, u32 mask, u8 all)
> {
> int indent;
> int did1;
> @@ -456,24 +457,26 @@ dump_link_caps(const char *prefix, const char *an_prefix, u32 mask)
> fprintf(stdout, "Not reported");
> fprintf(stdout, "\n");
>
> - fprintf(stdout, " %s pause frame use: ", prefix);
> - if (mask & ADVERTISED_Pause) {
> - fprintf(stdout, "Symmetric");
> - if (mask & ADVERTISED_Asym_Pause)
> - fprintf(stdout, " Receive-only");
> - fprintf(stdout, "\n");
> - } else {
> - if (mask & ADVERTISED_Asym_Pause)
> - fprintf(stdout, "Transmit-only\n");
> + if (all) {
It might be clearer to invert this flag and name it something like
'link_mode_only'.
> + fprintf(stdout, " %s pause frame use: ", prefix);
> + if (mask & ADVERTISED_Pause) {
> + fprintf(stdout, "Symmetric");
> + if (mask & ADVERTISED_Asym_Pause)
> + fprintf(stdout, " Receive-only");
> + fprintf(stdout, "\n");
> + } else {
> + if (mask & ADVERTISED_Asym_Pause)
> + fprintf(stdout, "Transmit-only\n");
> + else
> + fprintf(stdout, "No\n");
> + }
> +
> + fprintf(stdout, " %s auto-negotiation: ", an_prefix);
> + if (mask & ADVERTISED_Autoneg)
> + fprintf(stdout, "Yes\n");
> else
> fprintf(stdout, "No\n");
> }
> -
> - fprintf(stdout, " %s auto-negotiation: ", an_prefix);
> - if (mask & ADVERTISED_Autoneg)
> - fprintf(stdout, "Yes\n");
> - else
> - fprintf(stdout, "No\n");
> }
>
> static int dump_ecmd(struct ethtool_cmd *ep)
[...]
> @@ -1116,6 +1120,36 @@ static int dump_rxfhash(int fhash, u64 val)
> return 0;
> }
>
> +static int dump_eeecmd(struct ethtool_eee *ep)
Is there any reason for this not to return void?
> +{
> +
> + fprintf(stdout, " EEE status: ");
> + if (!ep->supported) {
> + fprintf(stdout, "not supported\n");
> + return 0;
> + } else if (!ep->eee_enabled) {
> + fprintf(stdout, "disabled\n");
> + } else {
> + fprintf(stdout, "enabled - ");
> + if (ep->eee_active)
> + fprintf(stdout, "active\n");
> + else
> + fprintf(stdout, "inactive\n");
> + }
> +
> + fprintf(stdout, " Tx LPI:");
> + if (ep->tx_lpi_enabled)
> + fprintf(stdout, " %d (u)\n", ep->tx_lpi_timer);
"us" not "(u)"
> + else
> + fprintf(stdout, " disabled\n");
> +
> + dump_link_caps("Supported EEE", "", ep->supported, 0);
> + dump_link_caps("Advertised EEE", "", ep->advertised, 0);
> + dump_link_caps("Link partner advertised EEE", "", ep->lp_advertised, 0);
> +
> + return 0;
> +}
> +
[...]
> +static int do_seee(struct cmd_context *ctx)
> +{
> + int adv_c = -1, lpi_c = -1, lpi_time_c = -1, eee_c = -1;
> + int change = -1, change2 = -1;
> + struct ethtool_eee eeecmd;
> + struct cmdline_info cmdline_eee[] = {
> + { "advertise", CMDL_U32, &adv_c, &eeecmd.advertised },
> + { "tx-lpi", CMDL_BOOL, &lpi_c, &eeecmd.tx_lpi_enabled },
> + { "tx-timer", CMDL_U32, &lpi_time_c, &eeecmd.tx_lpi_timer},
> + { "eee", CMDL_BOOL, &eee_c, &eeecmd.eee_enabled},
> + };
> +
> + if (ctx->argc == 0)
> + exit_bad_args();
> +
> + parse_generic_cmdline(ctx, &change, cmdline_eee,
> + ARRAY_SIZE(cmdline_eee));
> +
> + eeecmd.cmd = ETHTOOL_GEEE;
> + if (send_ioctl(ctx, &eeecmd)) {
> + perror("Cannot get EEE settings");
> + return 1;
> + }
> +
> + do_generic_set(cmdline_eee, ARRAY_SIZE(cmdline_eee), &change2);
> +
> + if (change2) {
> +
> + eeecmd.cmd = ETHTOOL_SEEE;
> + if (send_ioctl(ctx, &eeecmd)) {
> + perror("Cannot set EEE settings");
> + return 1;
> + }
> + }
> +
> + return 1;
return 0, I think!
> +}
> +
> int send_ioctl(struct cmd_context *ctx, void *cmd)
> {
> #ifndef TEST_ETHTOOL
> @@ -3423,6 +3516,12 @@ static const struct option {
> " [ hex on|off ]\n"
> " [ offset N ]\n"
> " [ length N ]\n" },
> + { "--get-eee", 1, do_geee, "Get EEE settings"},
> + { "--set-eee", 1, do_seee, "Set EEE settings",
> + " [ eee on|off ]\n"
> + " [ advertise %x ]\n"
> + " [ tx-lpi on|off ]\n"
> + " [ tx-timer %x ]\n"},
The tx-timer value would normally be specified in decimal, so put "%d"
here.
> { "-h|--help", 0, show_usage, "Show this help" },
> { "--version", 0, do_version, "Show version number" },
> {}
You also need to add some test cases for the command line parsing in
test-cmdline.c.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
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: [net-next PATCH v2 1/3] Added kernel support in EEE Ethtool commands
From: Ben Hutchings @ 2012-06-06 15:20 UTC (permalink / raw)
To: Yuval Mintz; +Cc: davem, netdev, eilong, peppe.cavallaro
In-Reply-To: <1338973098-16439-2-git-send-email-yuvalmin@broadcom.com>
On Wed, 2012-06-06 at 11:58 +0300, Yuval Mintz wrote:
> This patch extends the kernel's ethtool interface by adding support
> for 2 new EEE commands - get_eee and set_eee.
>
> Thanks goes to Giuseppe Cavallaro for his original patch adding this support.
>
> Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
> Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
> ---
> include/linux/ethtool.h | 32 ++++++++++++++++++++++++++++++++
> net/core/ethtool.c | 40 ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 72 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index e17fa71..6250e1f 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -137,6 +137,32 @@ struct ethtool_eeprom {
> };
>
> /**
> + * struct ethtool_eee - Energy Efficient Ethernet information
> + * @cmd: ETHTOOL_{G,S}EEE
> + * @supported: Link speeds for which there is eee support.
> + * @advertised: Link speeds the interface advertises (AN) as eee capable.
> + * @lp_advertised: Link speeds the link partner advertised as eee capable.
And these are bitmasks of SUPPORTED_* & ADVERTISED_* flags, right?
Maybe 'link modes' not 'link speeds'?
Otherwise, this all looks good to me (with limited knowledge of EEE).
Ben.
> + * @eee_active: Result of the eee auto negotiation.
> + * @eee_enabled: EEE configured mode (enabled/disabled).
> + * @tx_lpi_enabled: Whether the interface should assert its tx lpi, given
> + * that eee was negotiated.
> + * @tx_lpi_timer: Time in microseconds the interface delays prior to asserting
> + * its tx lpi (after reaching 'idle' state). Effective only when eee
> + * was negotiated and tx_lpi_enabled was set.
> + */
> +struct ethtool_eee {
[...]
--
Ben Hutchings, Staff Engineer, Solarflare
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] virtio-net: fix a race on 32bit arches
From: Eric Dumazet @ 2012-06-06 15:19 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, linux-kernel, virtualization, Stephen Hemminger
In-Reply-To: <20120606144941.GA17092@redhat.com>
On Wed, 2012-06-06 at 17:49 +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 06, 2012 at 03:10:10PM +0200, Eric Dumazet wrote:
> > On Wed, 2012-06-06 at 14:13 +0300, Michael S. Tsirkin wrote:
> >
> > > We currently do all stats either on napi callback or from
> > > start_xmit callback.
> > > This makes them safe, yes?
> >
> > Hmm, then _bh() variant is needed in virtnet_stats(), as explained in
> > include/linux/u64_stats_sync.h section 6)
> >
> > * 6) If counter might be written by an interrupt, readers should block interrupts.
> > * (On UP, there is no seqcount_t protection, a reader allowing interrupts could
> > * read partial values)
> >
> > Yes, its tricky...
>
> Sounds good, but I have a question: this realies on counters
> being atomic on 64 bit.
> Would not it be better to always use a seqlock even on 64 bit?
> This way counters would actually be correct and in sync.
> As it is if we want e.g. average packet size,
> we can not rely e.g. on it being bytes/packets.
When this stuff was discussed, we chose to have a nop on 64bits.
Your point has little to do with 64bit stats, it was already like that
with 'long int' counters.
Consider average driver doing :
dev->stats.rx_bytes += skb->len;
dev->stats.rx_packets++;
A concurrent reader can read an updated rx_bytes and a 'previous'
rx_packets one.
'fixing' this requires a lot of work and memory barriers (in all
drivers), for a very litle gain (at most one packet error)
u64_stats_sync was really meant to be 0-cost on 64bit arches.
^ permalink raw reply
* Re: [PATCH] virtio-net: fix a race on 32bit arches
From: Stephen Hemminger @ 2012-06-06 15:14 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Eric Dumazet, Jason Wang, netdev, rusty, linux-kernel,
virtualization
In-Reply-To: <20120606144941.GA17092@redhat.com>
On Wed, 6 Jun 2012 17:49:42 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> Sounds good, but I have a question: this realies on counters
> being atomic on 64 bit.
> Would not it be better to always use a seqlock even on 64 bit?
> This way counters would actually be correct and in sync.
> As it is if we want e.g. average packet size,
> we can not rely e.g. on it being bytes/packets.
This has not been a requirement on real physical devices; therefore
the added overhead is not really justified.
Many network cards use counters in hardware to count packets/bytes
and there is no expectation of atomic access there.
^ 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