* Re: Pull request: sfc-next 2013-08-25
From: Ben Hutchings @ 2013-08-27 16:26 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <20130827.121826.532177208564127527.davem@davemloft.net>
On Tue, 2013-08-27 at 12:18 -0400, David Miller wrote:
> From: Ben Hutchings <bhutchings@solarflare.com>
> Date: Sun, 25 Aug 2013 23:56:03 +0100
>
> > The following changes since commit f073dde03b3e8d11050d82f52caaf75fd924e069:
> >
> > sfc: Make efx_mcdi_init() call efx_mcdi_handle_assertion() (2013-08-21 19:43:09 +0100)
> >
> > are available in the git repository at:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/bwh/sfc-next.git for-davem
> >
> > for you to fetch changes up to f76fe120d81c96fa2a17ae41f0647c963dbb43cd:
> >
> > sfc: Update and improve kernel-doc for efx_mcdi_state & efx_mcdi_iface (2013-08-21 20:20:41 +0100)
> >
> > 1. Refactoring and cleanup in preparation for new hardware support.
> > 2. Some bug fixes for firmware completion handling. (They're not known
> > to cause real problems, otherwise I'd be submitting these for net and
> > stable.)
> > 3. Update to the firmware protocol (MCDI) definitions.
>
> Pulled, but I want to point out an existing issue which is that using
> an atomic_t for mcdi->state is bogus. You're only using atomic_set()
> and atomic_read() on the value, and that provides no "atomicity" per-se.
Thanks. I don't know what you looked for, but there are actually two
atomic_cmpxchg() calls in mcdi.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: [PATCH] ipv6:examine the IP source address legitimacy
From: David Miller @ 2013-08-27 16:32 UTC (permalink / raw)
To: duanj.fnst; +Cc: netdev
In-Reply-To: <521C90A0.60708@cn.fujitsu.com>
From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
Date: Tue, 27 Aug 2013 19:42:24 +0800
> rt = (struct rt6_info *) dst;
> - if (rt == net->ipv6.ip6_null_entry) {
> + if (rt == net->ipv6.ip6_null_entry || !ipv6_addr_equal(&rt->rt6i_gateway, &iph->saddr)) {
This line is significantly longer than 80 columns, please do not
create such long lines.
^ permalink raw reply
* Re: Pull request: sfc-next 2013-08-25
From: David Miller @ 2013-08-27 16:37 UTC (permalink / raw)
To: bhutchings; +Cc: netdev, linux-net-drivers
In-Reply-To: <1377620768.13272.3.camel@bwh-desktop.uk.level5networks.com>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Tue, 27 Aug 2013 17:26:08 +0100
> On Tue, 2013-08-27 at 12:18 -0400, David Miller wrote:
>> From: Ben Hutchings <bhutchings@solarflare.com>
>> Date: Sun, 25 Aug 2013 23:56:03 +0100
>>
>> > The following changes since commit f073dde03b3e8d11050d82f52caaf75fd924e069:
>> >
>> > sfc: Make efx_mcdi_init() call efx_mcdi_handle_assertion() (2013-08-21 19:43:09 +0100)
>> >
>> > are available in the git repository at:
>> >
>> > git://git.kernel.org/pub/scm/linux/kernel/git/bwh/sfc-next.git for-davem
>> >
>> > for you to fetch changes up to f76fe120d81c96fa2a17ae41f0647c963dbb43cd:
>> >
>> > sfc: Update and improve kernel-doc for efx_mcdi_state & efx_mcdi_iface (2013-08-21 20:20:41 +0100)
>> >
>> > 1. Refactoring and cleanup in preparation for new hardware support.
>> > 2. Some bug fixes for firmware completion handling. (They're not known
>> > to cause real problems, otherwise I'd be submitting these for net and
>> > stable.)
>> > 3. Update to the firmware protocol (MCDI) definitions.
>>
>> Pulled, but I want to point out an existing issue which is that using
>> an atomic_t for mcdi->state is bogus. You're only using atomic_set()
>> and atomic_read() on the value, and that provides no "atomicity" per-se.
>
> Thanks. I don't know what you looked for, but there are actually two
> atomic_cmpxchg() calls in mcdi.c.
I still think atomic_t is not appropriate, you can use plain xchg() as we
do all over the IPV4 stack, such as in the TCP metrics cache and the
routing code.
Really, if I don't see an atomic_*_and_test() or similar kind of
operation being used, I really don't want to see an atomic_t type in
use.
^ permalink raw reply
* Re: [PATCH 0/2] net/cadence/macb: add support for dt phy definition
From: boris brezillon @ 2013-08-27 16:38 UTC (permalink / raw)
To: David Miller
Cc: nicolas.ferre, rob.herring, pawel.moll, mark.rutland, swarren,
ian.campbell, linux, f.fainelli, netdev, linux-kernel,
linux-arm-kernel, devicetree
In-Reply-To: <20130827.122056.419758783951274884.davem@davemloft.net>
On 27/08/2013 18:20, David Miller wrote:
> From: boris brezillon <b.brezillon@overkiz.com>
> Date: Tue, 27 Aug 2013 09:42:34 +0200
>
>> Could you apply, the 3rd version of this series instead ?
> There can never be an "instead" or reverting patches I've said I've
> applied already.
>
> If you want changes, you have to submit follow-on fixes.
Hello David,
I sent the incremental patches based on your net-next branch:
https://lkml.org/lkml/2013/8/27/257
https://lkml.org/lkml/2013/8/27/259
Thanks.
Best Regards,
Boris
^ permalink raw reply
* Re: [PATCH 0/2] net/cadence/macb: add support for dt phy definition
From: David Miller @ 2013-08-27 16:41 UTC (permalink / raw)
To: b.brezillon
Cc: nicolas.ferre, rob.herring, pawel.moll, mark.rutland, swarren,
ian.campbell, linux, f.fainelli, netdev, linux-kernel,
linux-arm-kernel, devicetree
In-Reply-To: <521CD61C.8010103@overkiz.com>
From: boris brezillon <b.brezillon@overkiz.com>
Date: Tue, 27 Aug 2013 18:38:52 +0200
> On 27/08/2013 18:20, David Miller wrote:
>> From: boris brezillon <b.brezillon@overkiz.com>
>> Date: Tue, 27 Aug 2013 09:42:34 +0200
>>
>>> Could you apply, the 3rd version of this series instead ?
>> There can never be an "instead" or reverting patches I've said I've
>> applied already.
>>
>> If you want changes, you have to submit follow-on fixes.
>
> Hello David,
>
> I sent the incremental patches based on your net-next branch:
> https://lkml.org/lkml/2013/8/27/257
> https://lkml.org/lkml/2013/8/27/259
The place to check to see if your patch is queued up is here:
http://patchwork.ozlabs.org/project/netdev/list/
If it's there, it's not necessary to notify me of anything.
^ permalink raw reply
* Re: Pull request: sfc-next 2013-08-25
From: Ben Hutchings @ 2013-08-27 16:52 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <20130827.123718.732194018797657395.davem@davemloft.net>
On Tue, 2013-08-27 at 12:37 -0400, David Miller wrote:
> From: Ben Hutchings <bhutchings@solarflare.com>
> Date: Tue, 27 Aug 2013 17:26:08 +0100
>
> > On Tue, 2013-08-27 at 12:18 -0400, David Miller wrote:
> >> From: Ben Hutchings <bhutchings@solarflare.com>
> >> Date: Sun, 25 Aug 2013 23:56:03 +0100
> >>
> >> > The following changes since commit f073dde03b3e8d11050d82f52caaf75fd924e069:
> >> >
> >> > sfc: Make efx_mcdi_init() call efx_mcdi_handle_assertion() (2013-08-21 19:43:09 +0100)
> >> >
> >> > are available in the git repository at:
> >> >
> >> > git://git.kernel.org/pub/scm/linux/kernel/git/bwh/sfc-next.git for-davem
> >> >
> >> > for you to fetch changes up to f76fe120d81c96fa2a17ae41f0647c963dbb43cd:
> >> >
> >> > sfc: Update and improve kernel-doc for efx_mcdi_state & efx_mcdi_iface (2013-08-21 20:20:41 +0100)
> >> >
> >> > 1. Refactoring and cleanup in preparation for new hardware support.
> >> > 2. Some bug fixes for firmware completion handling. (They're not known
> >> > to cause real problems, otherwise I'd be submitting these for net and
> >> > stable.)
> >> > 3. Update to the firmware protocol (MCDI) definitions.
> >>
> >> Pulled, but I want to point out an existing issue which is that using
> >> an atomic_t for mcdi->state is bogus. You're only using atomic_set()
> >> and atomic_read() on the value, and that provides no "atomicity" per-se.
> >
> > Thanks. I don't know what you looked for, but there are actually two
> > atomic_cmpxchg() calls in mcdi.c.
>
> I still think atomic_t is not appropriate, you can use plain xchg() as we
> do all over the IPV4 stack, such as in the TCP metrics cache and the
> routing code.
>
> Really, if I don't see an atomic_*_and_test() or similar kind of
> operation being used, I really don't want to see an atomic_t type in
> use.
We definitely need some variant on cmpxchg() rather than xchg(), but
you're right that we don't need to use atomic_t for that.
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
* question about drivers/net/ethernet/mellanox/mlx4/en_tx.c
From: Julia Lawall @ 2013-08-27 17:07 UTC (permalink / raw)
To: amirv, netdev
Hello,
I was wondering why in drivers/net/ethernet/mellanox/mlx4/en_tx.c there
are a number of uses of dma_sync_single_for_cpu without corresponding uses
of dma_sync_single_for_device (for example in mlx4_en_rx_skb)? In most
drivers they come together.
thanks,
julia
^ permalink raw reply
* Re: [net-next RFC 3/7] i40evf: core ethtool functionality
From: Ben Hutchings @ 2013-08-27 17:27 UTC (permalink / raw)
To: Jeff Kirsher; +Cc: davem, Mitch A Williams, netdev, gospo, sassmann
In-Reply-To: <1377233609-11627-4-git-send-email-jeffrey.t.kirsher@intel.com>
On Thu, 2013-08-22 at 21:53 -0700, Jeff Kirsher wrote:
[...]
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c
[...]
> +/**
> + * i40evf_get_settings - Get Link Speed and Duplex settings
> + * @netdev: network interface device structure
> + * @ecmd: ethtool command
> + *
> + * Reports speed/duplex settings. Because this is a VF, we don't know what
> + * kind of link we really have, so we fake it.
> + **/
> +static int i40evf_get_settings(struct net_device *netdev,
> + struct ethtool_cmd *ecmd)
> +{
> + ecmd->supported = SUPPORTED_10000baseT_Full;
This is silly.
> + ecmd->autoneg = AUTONEG_DISABLE;
> + ecmd->transceiver = XCVR_DUMMY1;
> + ecmd->port = -1;
PORT_NONE, I think.
> + return 0;
> +}
> +
> +/**
> + * i40evf_get_sset_count - Get length of string set
> + * @netdev: network interface device structure
> + * @sset: id of string set
> + *
> + * Reports size (in bytes) of string set.
Not in bytes.
> This driver only supports
> + * strings for statistics.
> + **/
[...]
> +/**
> + * i40evf_get_strings - Get string set
> + * @netdev: network interface device structure
> + * @sset: id of string set
> + * @data: buffer for string data
> + *
> + * Strings are concatenated into the data buffer, separated by nulls.
This looks like a description of a Windows-style double-null-terminated
string list, not an ethtool string set.
> + **/
> +static void i40evf_get_strings(struct net_device *netdev, u32 sset, u8 *data)
> +{
> + struct i40evf_adapter *adapter = netdev_priv(netdev);
> + u8 *p = data;
> + int i;
> +
> + if (sset == ETH_SS_STATS) {
> + for (i = 0; i < I40EVF_GLOBAL_STATS_LEN; i++) {
> + memcpy(p, i40evf_gstrings_stats[i].stat_string,
> + ETH_GSTRING_LEN);
> + p += ETH_GSTRING_LEN;
> + }
> + for (i = 0; i < adapter->vsi_res->num_queue_pairs; i++) {
> + snprintf(p, ETH_GSTRING_LEN, "%s.tx_queue_%u.packets",
> + netdev->name, i);
> + p += ETH_GSTRING_LEN;
> + snprintf(p, ETH_GSTRING_LEN, "%s.tx_queue_%u.bytes",
> + netdev->name, i);
> + p += ETH_GSTRING_LEN;
> + }
> + for (i = 0; i < adapter->vsi_res->num_queue_pairs; i++) {
> + snprintf(p, ETH_GSTRING_LEN, "%s.rx_queue_%u.packets",
> + netdev->name, i);
> + p += ETH_GSTRING_LEN;
> + snprintf(p, ETH_GSTRING_LEN, "%s.rx_queue_%u.bytes",
> + netdev->name, i);
> + p += ETH_GSTRING_LEN;
It is completely redundant to put the device name into statistic names,
and is liable to cause the names to be truncated. There was also a
discussion about how to name per-queue statistics a while back, which
seemed to end in agreement:
http://thread.gmane.org/gmane.linux.network/273014/focus=12205
So I think the name formats should be "tx-%u.packets" etc.
[...]
> +/**
> + * i40evf_get_drvinto - Get driver info
> + * @netdev: network interface device structure
> + * @drvinfo: ethool driver info structure
> + *
> + * Returns information about the driver and device for display to the user.
> + **/
> +static void i40evf_get_drvinfo(struct net_device *netdev,
> + struct ethtool_drvinfo *drvinfo)
> +{
> + struct i40evf_adapter *adapter = netdev_priv(netdev);
> +
> + strncpy(drvinfo->driver, i40evf_driver_name, 32);
> + strncpy(drvinfo->version, i40evf_driver_version, 32);
> +
> + strncpy(drvinfo->fw_version, "N/A", 4);
If there is no firmware version then don't set fw_version at all.
> + strncpy(drvinfo->bus_info, pci_name(adapter->pdev), 32);
Use strlcpy() not strncpy() for all of the other strings.
> + drvinfo->n_stats = I40EVF_STATS_LEN;
> + drvinfo->regdump_len = 0;
Don't set these at all, as they are initialised by the ethtool core
based on other driver operations.
[...]
> +static void i40evf_get_ringparam(struct net_device *netdev,
> + struct ethtool_ringparam *ring)
> +{
> + struct i40evf_adapter *adapter = netdev_priv(netdev);
> + struct i40e_ring *tx_ring = &adapter->tx_rings[0];
> + struct i40e_ring *rx_ring = &adapter->rx_rings[0];
> +
> + ring->rx_max_pending = I40EVF_MAX_RXD;
> + ring->tx_max_pending = I40EVF_MAX_TXD;
> + ring->rx_mini_max_pending = 0;
> + ring->rx_jumbo_max_pending = 0;
> + ring->rx_pending = rx_ring->count;
> + ring->tx_pending = tx_ring->count;
> + ring->rx_mini_pending = 0;
> + ring->rx_jumbo_pending = 0;
> +}
No need to set the unsupported fields to 0.
[...]
> +static int i40evf_get_coalesce(struct net_device *netdev,
> + struct ethtool_coalesce *ec)
> +{
> + struct i40evf_adapter *adapter = netdev_priv(netdev);
> + struct i40e_vsi *vsi = &adapter->vsi;
> +
> + ec->tx_max_coalesced_frames_irq = vsi->work_limit;
> + ec->rx_max_coalesced_frames_irq = vsi->work_limit;
Use the fields without the '_irq' suffix. The '_irq' suffixed fields
are there for some NICs that change coalescing behaviour depending on
whether the previous IRQ is still being handled.
[...]
> +static int i40evf_set_coalesce(struct net_device *netdev,
> + struct ethtool_coalesce *ec)
> +{
> + struct i40evf_adapter *adapter = netdev_priv(netdev);
> + struct i40e_hw *hw = &adapter->hw;
> + struct i40e_vsi *vsi = &adapter->vsi;
> + struct i40e_q_vector *q_vector;
> + int i;
> +
> + if (ec->tx_max_coalesced_frames_irq || ec->rx_max_coalesced_frames_irq)
> + vsi->work_limit = ec->tx_max_coalesced_frames_irq;
[...]
Again, use the fields without the '_irq' suffix.
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 RFC 6/7] i40evf: init code and hardware support
From: Ben Hutchings @ 2013-08-27 17:42 UTC (permalink / raw)
To: Jeff Kirsher; +Cc: davem, Mitch A Williams, netdev, gospo, sassmann
In-Reply-To: <1377233609-11627-7-git-send-email-jeffrey.t.kirsher@intel.com>
On Thu, 2013-08-22 at 21:53 -0700, Jeff Kirsher wrote:
[...]
> Intel does wish to maintain exclusive Copyright to these files, and will
> work with the community to implement requested changes or transfer
> Copyright for larger patches. If you do see an issue with this code
> please email us and we will address any concerns in a timely manner.
[...]
I wonder why this is thought to be necessary. I've never heard of
copyright assignment conditions being accepted in the Linux kernel.
Some Linux drivers are dual-licenced under a permissive licence and GPL,
and all contributions to them presumably also have to be dual-licenced.
It seems like that would allow you to retain the ability to reuse the
core hardware code on other operating systems and under proprietary
licences, even with copyrightable contributions from others.
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: [PATCH 0/2] Rename nsproxy.pid_ns and fix a related security bug
From: David Miller @ 2013-08-27 17:53 UTC (permalink / raw)
To: luto; +Cc: ebiederm, security, linux-kernel, netdev
In-Reply-To: <cover.1377196394.git.luto@amacapital.net>
From: Andy Lutomirski <luto@amacapital.net>
Date: Thu, 22 Aug 2013 11:39:14 -0700
> Eric fell for my bogus claim that nsproxy->pid_ns was the current'
> process's pid ns. This isn't true.
>
> Let's fix the bug and rename pid_ns so that no one gets this wrong again.
>
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH net] be2net: Check for POST state in suspend-resume sequence
From: David Miller @ 2013-08-27 17:54 UTC (permalink / raw)
To: sarveshwar.bandi; +Cc: netdev
In-Reply-To: <771abba1-2664-4807-9a45-56ca9ede3c74@CMEXHTCAS2.ad.emulex.com>
From: <sarveshwar.bandi@emulex.com>
Date: Fri, 23 Aug 2013 14:59:33 +0530
> From: Sarveshwar Bandi <sarveshwar.bandi@emulex.com>
>
> In suspend-resume sequence, the OS could attempt to initialize the controller
> before it is ready, check for POST state before going ahead.
>
> Signed-off-by: Sarveshwar Bandi <sarveshwar.bandi@emulex.com>
Applied, thanks.
^ permalink raw reply
* [PATCH] ipv4: sendto/hdrincl: don't use destination address found in header
From: Chris Clark @ 2013-08-27 18:02 UTC (permalink / raw)
To: davem; +Cc: netdev
ipv4: raw_sendmsg: don't use header's destination address
A sendto() regression was bisected and found to start with commit
f8126f1d5136be1 (ipv4: Adjust semantics of rt->rt_gateway.)
The problem is that it tries to ARP-lookup the constructed packet's
destination address rather than the explicitly provided address.
Fix this using FLOWI_FLAG_KNOWN_NH so that given nexthop is used.
cf. commit 2ad5b9e4bd314fc685086b99e90e5de3bc59e26b
Reported-by: Chris Clark <chris.clark@alcatel-lucent.com>
Bisected-by: Chris Clark <chris.clark@alcatel-lucent.com>
Tested-by: Chris Clark <chris.clark@alcatel-lucent.com>
Suggested-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Chris Clark <chris.clark@alcatel-lucent.com>
---
net/ipv4/raw.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c
index dd44e0a..61e60d6 100644
--- a/net/ipv4/raw.c
+++ b/net/ipv4/raw.c
@@ -571,7 +571,8 @@ static int raw_sendmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
flowi4_init_output(&fl4, ipc.oif, sk->sk_mark, tos,
RT_SCOPE_UNIVERSE,
inet->hdrincl ? IPPROTO_RAW : sk->sk_protocol,
- inet_sk_flowi_flags(sk) | FLOWI_FLAG_CAN_SLEEP,
+ inet_sk_flowi_flags(sk) | FLOWI_FLAG_CAN_SLEEP |
+ (inet->hdrincl ? FLOWI_FLAG_KNOWN_NH : 0),
daddr, saddr, 0, 0);
if (!inet->hdrincl) {
^ permalink raw reply related
* Re: [PATCH net-next v1 5/9] bonding: convert bond_has_this_ip() to use upper devices
From: Veaceslav Falico @ 2013-08-27 18:10 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, Jay Vosburgh, Andy Gospodarek
In-Reply-To: <20130827112529.GA4732@minipsycho.brq.redhat.com>
On Tue, Aug 27, 2013 at 01:25:29PM +0200, Jiri Pirko wrote:
>Tue, Aug 27, 2013 at 01:16:48PM CEST, vfalico@redhat.com wrote:
>>On Mon, Aug 26, 2013 at 10:53:38PM +0200, Jiri Pirko wrote:
>>>Mon, Aug 26, 2013 at 10:32:38PM CEST, vfalico@redhat.com wrote:
>>...snip...
>>>>+ rcu_read_lock();
>>>>+ netdev_for_each_upper_dev(bond->dev, upper, iter) {
>>>>+ if (ip == bond_confirm_addr(upper, 0, ip)) {
>>>>+ ret = true;
>>>>+ break;
>>>>+ }
>>>
>>>You need the same recursion __vlan_find_dev_deep() is doing. If you do
>>>not do that, you will miss anything over the first upper level.
>>
>>Good point, and it's true for other uses also - bond_arp_send_all(), for
>>example, will also miss anything that's higher than the first upper level.
>>
>>I can't think of a use case scenario when we would need only the first
>>upper level - so maybe we should either make netdev_for_each_upper_dev()
>>recursive by default (I don't know how it can be done easily, tbh, without
>>modifying the existing code), or add something like:
>>
>>diff --git a/net/core/dev.c b/net/core/dev.c
>>index 566e99a..4a4468f 100644
>>--- a/net/core/dev.c
>>+++ b/net/core/dev.c
>>@@ -4387,6 +4387,31 @@ static void __append_search_uppers(struct list_head *search_list,
>> }
>> }
>>+struct net_device *netdev_upper_recursive_do_rcu(struct net_device *dev,
>>+ struct net_device *orig_dev,
>>+ bool (*f)(struct net_device *,
>>+ struct net_device *))
>>+{
>>+ struct netdev_upper *upper;
>>+ struct net_device *ret = NULL;
>>+
>>+ list_for_each_entry_rcu(upper, &dev->upper_dev_list, list) {
>>+ if (f(orig_dev, upper->dev)) {
>>+ ret = upper->dev;
>>+ break;
>>+ }
>>+
>>+ if (!list_empty(&upper->dev->upper_dev_list)) {
>>+ ret = netdev_upper_recursive_do_rcu(upper->dev,
>>+ orig_dev, f);
>>+ if (ret)
>>+ break;
>>+ }
>>+ }
>>+
>>+ return ret;
>>+}
>>+
>> static bool __netdev_search_upper_dev(struct net_device *dev,
>> struct net_device *upper_dev)
>> {
>>
>>How do you think?
>
>I do not like this. How about to put all levels to upper_dev list and
>mark those who are not direct (not level1) ? Then we can use single list
>for all purposes.
I've looked at the code a bit more and I really don't see a way to do
non-recursive, RCUed way to traverse the whole list of upper devices.
I see three ways to handle this situation:
1) The one that I've posted, recursive search and calling a provided
function (the function should also get as a parameter a user-provided void
*pointer). It's, indeed, a bit hacky, however will work perfectly.
2) Implementing the search (recursive) in bonding (or any further device)
itself. Less intrusive, however it'll be code duplication actually for
point 1).
3) Adding lower_dev_list, populating it accordingly, and also adding an int
distance to the netdev_upper (or, with this approach, rather netdev_adjacent
or something like that), which will help to implement your idea - a device
will have lower/upper_dev_list populated with all lower/upper devices and
their distance (i.e. distance == 1 means that it's first level of
lower/upper device). With this approach, we might also afterwards get rid
of slave lists from 'grouping' devices like bonding/team/bridge/etc. and,
thus, the locking.
Now I'd rather go with 1), but if you don't like it - I can go with 2).
And, if 3) sounds ok, I can implement it also and try to get rid of bonding
slave list (hopefully).
Do you have any other ideas/thoughts?
^ permalink raw reply
* Re: [PATCH] tipc: set sk_err correctly when connection fails
From: Paul Gortmaker @ 2013-08-27 18:12 UTC (permalink / raw)
To: Erik Hugne; +Cc: netdev, jon.maloy, ying.xue, tipc-discussion, nhan.tt.vo
In-Reply-To: <20130827151841.GE1939@eerihug-hybrid.rnd.ki.sw.ericsson.se>
On 13-08-27 11:18 AM, Erik Hugne wrote:
> On Tue, Aug 27, 2013 at 09:20:23AM -0400, Paul Gortmaker wrote:
>> What was the high level user visible symptom in this case?
>> Stalled connections or ... ?
>
> Should the connect fail, if the publication/server is unavailable or some other
> error. The connect() call returns the error code directly (as a positive value).
Please send a v2 with the end-user visible symptom clearly described;
as this information is what people use in order to triage whether
commits belong in stable, or net vs. net-next etc. For example:
Should the connect fail, say if the publication/server is unavailable or
some other error, then the code returns a positive return value. Since
most code only checks for a negative return on connect(), it tries to
continue, but will ultimately fail on the 1st sendto() as the strace
snippet below shows.
I've said "most code" since I simply don't know what it was that you were
tracing below. It would help if we knew if this part of a common application
or similar.
Thanks,
Paul.
--
>
> [...]
> socket(0x1e /* PF_??? */, SOCK_SEQPACKET, 0) = 3
> setsockopt(3, 0x10f /* SOL_?? */, 129, [0], 4) = 0
> setsockopt(3, 0x10f /* SOL_?? */, 127, [0], 4) = 0
> connect(3, {sa_family=0x1e /* AF_??? */, sa_data="\2\1\322\4\0\0\322\4\0\0\0\0\0\0"}, 16) = 111
> sendto(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 66000, 0, NULL, 0) = -1 EPIPE (Broken pipe)
>
> In the strace above, error checking was done as:
> if (connect(fd,.....) < 0)
> perror("connect");
>
> //E
>
^ permalink raw reply
* RE: [net-next RFC 3/7] i40evf: core ethtool functionality
From: Williams, Mitch A @ 2013-08-27 18:23 UTC (permalink / raw)
To: Ben Hutchings, Kirsher, Jeffrey T
Cc: davem@davemloft.net, netdev@vger.kernel.org, gospo@redhat.com,
sassmann@redhat.com
In-Reply-To: <1377624472.13272.43.camel@bwh-desktop.uk.level5networks.com>
> -----Original Message-----
> From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> Sent: Tuesday, August 27, 2013 10:28 AM
> To: Kirsher, Jeffrey T
> Cc: davem@davemloft.net; Williams, Mitch A; netdev@vger.kernel.org;
> gospo@redhat.com; sassmann@redhat.com
> Subject: Re: [net-next RFC 3/7] i40evf: core ethtool functionality
>
> On Thu, 2013-08-22 at 21:53 -0700, Jeff Kirsher wrote:
> [...]
> > --- /dev/null
> > +++ b/drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c
> [...]
> > +/**
> > + * i40evf_get_settings - Get Link Speed and Duplex settings
> > + * @netdev: network interface device structure
> > + * @ecmd: ethtool command
> > + *
> > + * Reports speed/duplex settings. Because this is a VF, we don't know
> what
> > + * kind of link we really have, so we fake it.
> > + **/
> > +static int i40evf_get_settings(struct net_device *netdev,
> > + struct ethtool_cmd *ecmd)
> > +{
> > + ecmd->supported = SUPPORTED_10000baseT_Full;
>
> This is silly.
>
> > + ecmd->autoneg = AUTONEG_DISABLE;
> > + ecmd->transceiver = XCVR_DUMMY1;
> > + ecmd->port = -1;
>
> PORT_NONE, I think.
>
> > + return 0;
> > +}
> > +
> > +/**
> > + * i40evf_get_sset_count - Get length of string set
> > + * @netdev: network interface device structure
> > + * @sset: id of string set
> > + *
> > + * Reports size (in bytes) of string set.
>
> Not in bytes.
>
> > This driver only supports
> > + * strings for statistics.
> > + **/
> [...]
> > +/**
> > + * i40evf_get_strings - Get string set
> > + * @netdev: network interface device structure
> > + * @sset: id of string set
> > + * @data: buffer for string data
> > + *
> > + * Strings are concatenated into the data buffer, separated by nulls.
>
> This looks like a description of a Windows-style double-null-terminated
> string list, not an ethtool string set.
>
> > + **/
> > +static void i40evf_get_strings(struct net_device *netdev, u32 sset, u8
> *data)
> > +{
> > + struct i40evf_adapter *adapter = netdev_priv(netdev);
> > + u8 *p = data;
> > + int i;
> > +
> > + if (sset == ETH_SS_STATS) {
> > + for (i = 0; i < I40EVF_GLOBAL_STATS_LEN; i++) {
> > + memcpy(p, i40evf_gstrings_stats[i].stat_string,
> > + ETH_GSTRING_LEN);
> > + p += ETH_GSTRING_LEN;
> > + }
> > + for (i = 0; i < adapter->vsi_res->num_queue_pairs; i++) {
> > + snprintf(p, ETH_GSTRING_LEN, "%s.tx_queue_%u.packets",
> > + netdev->name, i);
> > + p += ETH_GSTRING_LEN;
> > + snprintf(p, ETH_GSTRING_LEN, "%s.tx_queue_%u.bytes",
> > + netdev->name, i);
> > + p += ETH_GSTRING_LEN;
> > + }
> > + for (i = 0; i < adapter->vsi_res->num_queue_pairs; i++) {
> > + snprintf(p, ETH_GSTRING_LEN, "%s.rx_queue_%u.packets",
> > + netdev->name, i);
> > + p += ETH_GSTRING_LEN;
> > + snprintf(p, ETH_GSTRING_LEN, "%s.rx_queue_%u.bytes",
> > + netdev->name, i);
> > + p += ETH_GSTRING_LEN;
>
> It is completely redundant to put the device name into statistic names,
> and is liable to cause the names to be truncated. There was also a
> discussion about how to name per-queue statistics a while back, which
> seemed to end in agreement:
>
> http://thread.gmane.org/gmane.linux.network/273014/focus=12205
>
> So I think the name formats should be "tx-%u.packets" etc.
>
> [...]
> > +/**
> > + * i40evf_get_drvinto - Get driver info
> > + * @netdev: network interface device structure
> > + * @drvinfo: ethool driver info structure
> > + *
> > + * Returns information about the driver and device for display to the
> user.
> > + **/
> > +static void i40evf_get_drvinfo(struct net_device *netdev,
> > + struct ethtool_drvinfo *drvinfo)
> > +{
> > + struct i40evf_adapter *adapter = netdev_priv(netdev);
> > +
> > + strncpy(drvinfo->driver, i40evf_driver_name, 32);
> > + strncpy(drvinfo->version, i40evf_driver_version, 32);
> > +
> > + strncpy(drvinfo->fw_version, "N/A", 4);
>
> If there is no firmware version then don't set fw_version at all.
>
> > + strncpy(drvinfo->bus_info, pci_name(adapter->pdev), 32);
>
> Use strlcpy() not strncpy() for all of the other strings.
>
> > + drvinfo->n_stats = I40EVF_STATS_LEN;
> > + drvinfo->regdump_len = 0;
>
> Don't set these at all, as they are initialised by the ethtool core
> based on other driver operations.
>
> [...]
> > +static void i40evf_get_ringparam(struct net_device *netdev,
> > + struct ethtool_ringparam *ring)
> > +{
> > + struct i40evf_adapter *adapter = netdev_priv(netdev);
> > + struct i40e_ring *tx_ring = &adapter->tx_rings[0];
> > + struct i40e_ring *rx_ring = &adapter->rx_rings[0];
> > +
> > + ring->rx_max_pending = I40EVF_MAX_RXD;
> > + ring->tx_max_pending = I40EVF_MAX_TXD;
> > + ring->rx_mini_max_pending = 0;
> > + ring->rx_jumbo_max_pending = 0;
> > + ring->rx_pending = rx_ring->count;
> > + ring->tx_pending = tx_ring->count;
> > + ring->rx_mini_pending = 0;
> > + ring->rx_jumbo_pending = 0;
> > +}
>
> No need to set the unsupported fields to 0.
>
> [...]
> > +static int i40evf_get_coalesce(struct net_device *netdev,
> > + struct ethtool_coalesce *ec)
> > +{
> > + struct i40evf_adapter *adapter = netdev_priv(netdev);
> > + struct i40e_vsi *vsi = &adapter->vsi;
> > +
> > + ec->tx_max_coalesced_frames_irq = vsi->work_limit;
> > + ec->rx_max_coalesced_frames_irq = vsi->work_limit;
>
> Use the fields without the '_irq' suffix. The '_irq' suffixed fields
> are there for some NICs that change coalescing behaviour depending on
> whether the previous IRQ is still being handled.
>
> [...]
> > +static int i40evf_set_coalesce(struct net_device *netdev,
> > + struct ethtool_coalesce *ec)
> > +{
> > + struct i40evf_adapter *adapter = netdev_priv(netdev);
> > + struct i40e_hw *hw = &adapter->hw;
> > + struct i40e_vsi *vsi = &adapter->vsi;
> > + struct i40e_q_vector *q_vector;
> > + int i;
> > +
> > + if (ec->tx_max_coalesced_frames_irq || ec->rx_max_coalesced_frames_irq)
> > + vsi->work_limit = ec->tx_max_coalesced_frames_irq;
> [...]
>
> Again, use the fields without the '_irq' suffix.
>
> Ben.
>
Thanks for the review, Ben. I'll fix all of these before we send this out again.
-Mitch
> --
> 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 RFC net-next] ipv6: avoid high order memory allocations for /proc/net/ipv6_route
From: Hannes Frederic Sowa @ 2013-08-27 18:24 UTC (permalink / raw)
To: netdev, yoshfuji; +Cc: greearb, kaber, eric.dumazet, gaofeng, kuznet, davem
In-Reply-To: <20130822031602.GA19269@order.stressinduktion.org>
Hello Ben and Patrick!
I have a question regarding this patch:
commit 2bec5a369ee79576a3eea2c23863325089785a2c
Author: Patrick McHardy <kaber@trash.net>
Date: Mon Feb 8 05:19:03 2010 +0000
ipv6: fib: fix crash when changing large fib while dumping it
When the fib size exceeds what can be dumped in a single skb, the
dump is suspended and resumed once the last skb has been received
by userspace. When the fib is changed while the dump is suspended,
the walker might contain stale pointers, causing a crash when the
dump is resumed.
Below is a patch to switch /proc/net/ipv6_route to an iterating interface
as well, thus avoiding large order allocations in the procfs code.
Looking at the netlink dump code I think I might also need the
serialnumber check and restart logic but I could not reproduce any NULL-ptr
deref with my code or with your patch reverted. Maybe you remember some
special cases how to reproduce this? There was already some doubt the
serialnumber check would be needed in this thread:
https://lkml.org/lkml/2012/6/12/386
I also tried to check the walker logic and have not found any spots where it
should produce such a BUG.
It would also help to know the exact kernel version on which this bug was
produced.
Thanks a lot!
On Thu, Aug 22, 2013 at 05:16:02AM +0200, Hannes Frederic Sowa wrote:
> On Wed, Aug 21, 2013 at 07:50:24PM +0200, Hannes Frederic Sowa wrote:
> > I created this patch in a hurry while trying to get the rt6i_flags of
> > the routing entries on a system with lots of routes (I later cleaned it
> > up a bit).
> >
> > The current approach for dumping routes over netlink is to redump a tree
> > as soon as the serialnumber on the nodes changes. Is this behavior more
> > preferable as to just ignore the routes for that moment? I am in favour
> > of not restarting the dump on /proc/net/ipv6_route.
>
> Ok, I hit the same panics as Patrick once did
> while some more stress testing this change
> (http://permalink.gmane.org/gmane.linux.network/151391).
>
> As I read this up, this was also the reason the serial checks where
> introduced. It also tries to skip visited notes, so please void my
> question above.
>
> The reason is a disconnected fib6_node in FWS_U state trying to walk
> up. I'll investigate if we can fix this up in the walker post-cleanup on
> fib6_repair_tree/fib6_del_route (then we could also remove the serial
> number checks on inet6_dump_fib) or I will make the appropriate serial
> changes to this patch, too.
This was actually an error in my debugging code. I could not trigger it
afterwards. It was fully my fault.
Current patch looks like this:
[PATCH RFC] ipv6: avoid high order memory allocations for /proc/net/ipv6_route
Dumping routes on a system with lots rt6_infos in the fibs causes up to
11-order allocations in seq_file (which fail). While we could switch
there to vmalloc we could just implement the streaming interface for
/proc/net/ipv6_route. This patch switches /proc/net/ipv6_route from
single_open_net to seq_open_net.
loff_t *pos tracks dst entries.
Also kill never used struct rt6_proc_arg and now unused function
fib6_clean_all_ro.
Cc: Ben Greear <greearb@candelatech.com>
Cc: Patrick McHardy <kaber@trash.net>
Cc: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
include/net/ip6_fib.h | 7 +-
net/ipv6/ip6_fib.c | 191 +++++++++++++++++++++++++++++++++++++++++++++-----
net/ipv6/route.c | 46 +-----------
3 files changed, 176 insertions(+), 68 deletions(-)
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 48ec25a..9bd5de8 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -281,10 +281,6 @@ struct fib6_node *fib6_locate(struct fib6_node *root,
const struct in6_addr *daddr, int dst_len,
const struct in6_addr *saddr, int src_len);
-extern void fib6_clean_all_ro(struct net *net,
- int (*func)(struct rt6_info *, void *arg),
- int prune, void *arg);
-
extern void fib6_clean_all(struct net *net,
int (*func)(struct rt6_info *, void *arg),
int prune, void *arg);
@@ -306,6 +302,9 @@ extern void fib6_gc_cleanup(void);
extern int fib6_init(void);
+extern int ipv6_route_open(struct inode *inode,
+ struct file *file);
+
#ifdef CONFIG_IPV6_MULTIPLE_TABLES
extern int fib6_rules_init(void);
extern void fib6_rules_cleanup(void);
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 73db48e..04192af 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -1529,25 +1529,6 @@ static void fib6_clean_tree(struct net *net, struct fib6_node *root,
fib6_walk(&c.w);
}
-void fib6_clean_all_ro(struct net *net, int (*func)(struct rt6_info *, void *arg),
- int prune, void *arg)
-{
- struct fib6_table *table;
- struct hlist_head *head;
- unsigned int h;
-
- rcu_read_lock();
- for (h = 0; h < FIB6_TABLE_HASHSZ; h++) {
- head = &net->ipv6.fib_table_hash[h];
- hlist_for_each_entry_rcu(table, head, tb6_hlist) {
- read_lock_bh(&table->tb6_lock);
- fib6_clean_tree(net, &table->tb6_root,
- func, prune, arg);
- read_unlock_bh(&table->tb6_lock);
- }
- }
- rcu_read_unlock();
-}
void fib6_clean_all(struct net *net, int (*func)(struct rt6_info *, void *arg),
int prune, void *arg)
{
@@ -1782,3 +1763,175 @@ void fib6_gc_cleanup(void)
unregister_pernet_subsys(&fib6_net_ops);
kmem_cache_destroy(fib6_node_kmem);
}
+
+#ifdef CONFIG_PROC_FS
+
+struct ipv6_route_iter {
+ struct seq_net_private p;
+ struct fib6_walker_t w;
+ loff_t skip;
+ struct fib6_table *tbl;
+};
+
+static int ipv6_route_seq_show(struct seq_file *seq, void *v)
+{
+ struct rt6_info *rt = v;
+ struct ipv6_route_iter *iter = seq->private;
+
+ seq_printf(seq, "%pi6 %02x ", &rt->rt6i_dst.addr, rt->rt6i_dst.plen);
+
+#ifdef CONFIG_IPV6_SUBTREES
+ seq_printf(seq, "%pi6 %02x ", &rt->rt6i_src.addr, rt->rt6i_src.plen);
+#else
+ seq_puts(seq, "00000000000000000000000000000000 00 ");
+#endif
+ if (rt->rt6i_flags & RTF_GATEWAY)
+ seq_printf(seq, "%pi6", &rt->rt6i_gateway);
+ else
+ seq_puts(seq, "00000000000000000000000000000000");
+
+ seq_printf(seq, " %08x %08x %08x %08x %8s\n",
+ rt->rt6i_metric, atomic_read(&rt->dst.__refcnt),
+ rt->dst.__use, rt->rt6i_flags,
+ rt->dst.dev ? rt->dst.dev->name : "");
+ iter->w.leaf = NULL;
+ return 0;
+}
+
+static int ipv6_route_yield(struct fib6_walker_t *w)
+{
+ struct ipv6_route_iter *iter = w->args;
+
+ if (!iter->skip)
+ return 1;
+
+ do {
+ iter->w.leaf = iter->w.leaf->dst.rt6_next;
+ iter->skip--;
+ if (!iter->skip && iter->w.leaf)
+ return 1;
+ } while (iter->w.leaf);
+
+ return 0;
+}
+
+static void ipv6_route_seq_setup_walk(struct ipv6_route_iter *iter)
+{
+ memset(&iter->w, 0, sizeof(iter->w));
+ iter->w.func = ipv6_route_yield;
+ iter->w.root = &iter->tbl->tb6_root;
+ iter->w.state = FWS_INIT;
+ iter->w.node = iter->w.root;
+ iter->w.args = iter;
+ INIT_LIST_HEAD(&iter->w.lh);
+ fib6_walker_link(&iter->w);
+}
+
+static struct fib6_table *ipv6_route_seq_next_table(struct fib6_table *tbl,
+ struct net *net)
+{
+ unsigned int h;
+ struct hlist_node *node;
+
+ if (tbl) {
+ h = (tbl->tb6_id & (FIB6_TABLE_HASHSZ - 1)) + 1;
+ node = rcu_dereference_bh(hlist_next_rcu(&tbl->tb6_hlist));
+ } else {
+ h = 0;
+ node = NULL;
+ }
+
+ while (!node && h < FIB6_TABLE_HASHSZ) {
+ node = rcu_dereference_bh(
+ hlist_first_rcu(&net->ipv6.fib_table_hash[h++]));
+ }
+ return hlist_entry_safe(node, struct fib6_table, tb6_hlist);
+}
+
+static void *ipv6_route_seq_next(struct seq_file *seq, void *v, loff_t *pos)
+{
+ int r;
+ struct rt6_info *n;
+ struct net *net = seq_file_net(seq);
+ struct ipv6_route_iter *iter = seq->private;
+
+ if (!v)
+ goto iter_table;
+
+ n = ((struct rt6_info *)v)->dst.rt6_next;
+ if (n) {
+ ++*pos;
+ return n;
+ }
+
+iter_table:
+ read_lock(&iter->tbl->tb6_lock);
+ r = fib6_walk_continue(&iter->w);
+ read_unlock(&iter->tbl->tb6_lock);
+ if (r > 0) {
+ if (v)
+ ++*pos;
+ return iter->w.leaf;
+ } else if (r < 0) {
+ fib6_walker_unlink(&iter->w);
+ return NULL;
+ }
+ fib6_walker_unlink(&iter->w);
+
+ iter->tbl = ipv6_route_seq_next_table(iter->tbl, net);
+ if (!iter->tbl)
+ return NULL;
+
+ ipv6_route_seq_setup_walk(iter);
+ goto iter_table;
+}
+
+static void *ipv6_route_seq_start(struct seq_file *seq, loff_t *pos)
+ __acquires(RCU_BH)
+{
+ struct net *net = seq_file_net(seq);
+ struct ipv6_route_iter *iter = seq->private;
+
+ rcu_read_lock_bh();
+ iter->tbl = ipv6_route_seq_next_table(NULL, net);
+ iter->skip = *pos;
+
+ if (iter->tbl) {
+ ipv6_route_seq_setup_walk(iter);
+ return ipv6_route_seq_next(seq, NULL, pos);
+ } else {
+ return NULL;
+ }
+}
+
+static bool ipv6_route_iter_active(struct ipv6_route_iter *iter)
+{
+ struct fib6_walker_t *w = &iter->w;
+ return w->node && !(w->state == FWS_U && w->node == w->root);
+}
+
+static void ipv6_route_seq_stop(struct seq_file *seq, void *v)
+ __releases(RCU_BH)
+{
+ struct ipv6_route_iter *iter = seq->private;
+
+ if (ipv6_route_iter_active(iter))
+ fib6_walker_unlink(&iter->w);
+
+ rcu_read_unlock_bh();
+}
+
+static const struct seq_operations ipv6_route_seq_ops = {
+ .start = ipv6_route_seq_start,
+ .next = ipv6_route_seq_next,
+ .stop = ipv6_route_seq_stop,
+ .show = ipv6_route_seq_show
+};
+
+int ipv6_route_open(struct inode *inode, struct file *file)
+{
+ return seq_open_net(inode, file, &ipv6_route_seq_ops,
+ sizeof(struct ipv6_route_iter));
+}
+
+#endif /* CONFIG_PROC_FS */
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index e22c4db..42cb227 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2729,56 +2729,12 @@ static int ip6_route_dev_notify(struct notifier_block *this,
#ifdef CONFIG_PROC_FS
-struct rt6_proc_arg
-{
- char *buffer;
- int offset;
- int length;
- int skip;
- int len;
-};
-
-static int rt6_info_route(struct rt6_info *rt, void *p_arg)
-{
- struct seq_file *m = p_arg;
-
- seq_printf(m, "%pi6 %02x ", &rt->rt6i_dst.addr, rt->rt6i_dst.plen);
-
-#ifdef CONFIG_IPV6_SUBTREES
- seq_printf(m, "%pi6 %02x ", &rt->rt6i_src.addr, rt->rt6i_src.plen);
-#else
- seq_puts(m, "00000000000000000000000000000000 00 ");
-#endif
- if (rt->rt6i_flags & RTF_GATEWAY) {
- seq_printf(m, "%pi6", &rt->rt6i_gateway);
- } else {
- seq_puts(m, "00000000000000000000000000000000");
- }
- seq_printf(m, " %08x %08x %08x %08x %8s\n",
- rt->rt6i_metric, atomic_read(&rt->dst.__refcnt),
- rt->dst.__use, rt->rt6i_flags,
- rt->dst.dev ? rt->dst.dev->name : "");
- return 0;
-}
-
-static int ipv6_route_show(struct seq_file *m, void *v)
-{
- struct net *net = (struct net *)m->private;
- fib6_clean_all_ro(net, rt6_info_route, 0, m);
- return 0;
-}
-
-static int ipv6_route_open(struct inode *inode, struct file *file)
-{
- return single_open_net(inode, file, ipv6_route_show);
-}
-
static const struct file_operations ipv6_route_proc_fops = {
.owner = THIS_MODULE,
.open = ipv6_route_open,
.read = seq_read,
.llseek = seq_lseek,
- .release = single_release_net,
+ .release = seq_release_net,
};
static int rt6_stats_seq_show(struct seq_file *seq, void *v)
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH 06/11] net: calxedaxgmac: fix race with tx queue stop/wake
From: Ben Hutchings @ 2013-08-27 18:31 UTC (permalink / raw)
To: Rob Herring
Cc: netdev, linux-kernel, Andreas Herrmann, Lennert Buytenhek,
Rob Herring
In-Reply-To: <1377557126-10716-6-git-send-email-robherring2@gmail.com>
On Mon, 2013-08-26 at 17:45 -0500, Rob Herring wrote:
> From: Rob Herring <rob.herring@calxeda.com>
>
> Since the xgmac transmit start and completion work locklessly, it is
> possible for xgmac_xmit to stop the tx queue after the xgmac_tx_complete
> has run resulting in the tx queue never being woken up. Fix this by
> ensuring that ring buffer index updates are visible and serialize the
> queue wake with netif_tx_lock.
>
> The implementation used here was copied from
> drivers/net/ethernet/broadcom/tg3.c.
>
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> ---
> drivers/net/ethernet/calxeda/xgmac.c | 25 +++++++++++++++++++------
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/calxeda/xgmac.c b/drivers/net/ethernet/calxeda/xgmac.c
> index f630855..cd5872c 100644
> --- a/drivers/net/ethernet/calxeda/xgmac.c
> +++ b/drivers/net/ethernet/calxeda/xgmac.c
> @@ -410,6 +410,9 @@ struct xgmac_priv {
> #define dma_ring_space(h, t, s) CIRC_SPACE(h, t, s)
> #define dma_ring_cnt(h, t, s) CIRC_CNT(h, t, s)
>
> +#define tx_dma_ring_space(p) \
> + dma_ring_space((p)->tx_head, (p)->tx_tail, DMA_TX_RING_SZ)
> +
> /* XGMAC Descriptor Access Helpers */
> static inline void desc_set_buf_len(struct xgmac_dma_desc *p, u32 buf_sz)
> {
> @@ -886,9 +889,14 @@ static void xgmac_tx_complete(struct xgmac_priv *priv)
> priv->tx_tail = dma_ring_incr(entry, DMA_TX_RING_SZ);
> }
>
> - if (dma_ring_space(priv->tx_head, priv->tx_tail, DMA_TX_RING_SZ) >
> - MAX_SKB_FRAGS)
> - netif_wake_queue(priv->dev);
> + /* Ensure tx_tail is visible to xgmac_xmit */
> + smp_mb();
> + if (unlikely(netif_queue_stopped(priv->dev))) {
> + netif_tx_lock(priv->dev);
> + if (tx_dma_ring_space(priv) > MAX_SKB_FRAGS)
> + netif_wake_queue(priv->dev);
> + netif_tx_unlock(priv->dev);
> + }
> }
You don't need to take the TX lock for this. The memory barriers
provide sufficient synchronisation.
> static void xgmac_tx_timeout_work(struct work_struct *work)
> @@ -1125,10 +1133,15 @@ static netdev_tx_t xgmac_xmit(struct sk_buff *skb, struct net_device *dev)
>
> priv->tx_head = dma_ring_incr(entry, DMA_TX_RING_SZ);
>
> - if (dma_ring_space(priv->tx_head, priv->tx_tail, DMA_TX_RING_SZ) <
> - MAX_SKB_FRAGS)
> + /* Ensure tx_head update is visible to tx completion */
> + smp_mb();
> + if (unlikely(tx_dma_ring_space(priv) < MAX_SKB_FRAGS)) {
> netif_stop_queue(dev);
> -
> + /* Ensure netif_stop_queue is visible to tx completion */
> + smp_mb();
> + if (tx_dma_ring_space(priv) > MAX_SKB_FRAGS)
> + netif_wake_queue(dev);
You should use netif_start_queue() rather than netif_wake_queue(), since
you know the TX scheduler is already active.
Ben.
> + }
> return NETDEV_TX_OK;
> }
>
--
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 net-next 01/10] net: etherdevice: add address inherit helper
From: David Miller @ 2013-08-27 18:35 UTC (permalink / raw)
To: bjorn; +Cc: stephen, netdev
In-Reply-To: <87ppt4yrbs.fsf@nemi.mork.no>
From: Bjørn Mork <bjorn@mork.no>
Date: Sat, 24 Aug 2013 00:28:55 +0200
> Stephen Hemminger <stephen@networkplumber.org> writes:
>> On Fri, 23 Aug 2013 11:35:04 +0200
>> Bjørn Mork <bjorn@mork.no> wrote:
>>
>>> /**
>>> + * eth_hw_addr_inherit - Copy dev_addr from another net_device
>>> + * @dst: pointer to net_device to copy dev_addr to
>>> + * @src: pointer to net_device to copy dev_addr from
>>> + *
>>> + * Copy the Ethernet address from one net_device to another along with
>>> + * the addr_assign_type.
>>> + */
>>> +static inline int eth_hw_addr_inherit(struct net_device *dst,
>>> + struct net_device *src)
>>> +{
>>> + if (dst->addr_len != src->addr_len)
>>> + return -EINVAL;
>>> +
>>> + dst->addr_assign_type = src->addr_assign_type;
>>> + memcpy(dst->dev_addr, src->dev_addr, dst->addr_len);
>>> + return 0;
>>> +}
>>> +
>>
>> Since all the other code in this file assumes addr_len == ETH_ALEN
>> why does this code need to handle variable addresses. Trivial but
>> then the memcpy is fixed size and can be optimized.
>
> Didn't know that. I'll make that change in the next version. Not that
> optimization matters much here, but consistency is always good.
You can bug check that addr_len ETH_ALEN if you want.
I realize that some drivers don't initialize it by this call point but
that is arguably a bug.
^ permalink raw reply
* Re: [net-next RFC 6/7] i40evf: init code and hardware support
From: David Miller @ 2013-08-27 18:44 UTC (permalink / raw)
To: john.ronciak
Cc: bhutchings, jeffrey.t.kirsher, mitch.a.williams, netdev, gospo,
sassmann
In-Reply-To: <CALWPMBHKfzvLpi9J+tqeFtmOzPANXw5ozdiXhSku_sQtVGg5yg@mail.gmail.com>
From: John Ronciak <john.ronciak@gmail.com>
Date: Tue, 27 Aug 2013 11:39:03 -0700
>> I wonder why this is thought to be necessary. I've never heard of
>> copyright assignment conditions being accepted in the Linux kernel.
>
>> Some Linux drivers are dual-licenced under a permissive licence and GPL,
>> and all contributions to them presumably also have to be dual-licenced.
>> It seems like that would allow you to retain the ability to reuse the
>> core hardware code on other operating systems and under proprietary
>> licences, even with copyrightable contributions from others.
>
> The problem here is that it's a legal issue. The Intel lawyers (and I've
> heard form other companies on this as well) say that just because we
> release code under a dual license does not mean that any changes (patches)
> that come back are also under a dual license. That cannot be assumed. So
> all changes would require us to ask that the patches in question are being
> supplied under the same dual license. This can get unmanageable fairly
> quickly. There is also long term tracking of those requests/responses. So
> using a dual license isn't the same thing unfortunately.
I think it's pretty anti-social to not accept changes made outside of
your group.
I don't want Intel ethernet drivers to go the way of ACPI where nobody
outside of you guys can make changes to the code.
I guarentee I will want to make changes to your drivers if an interface
is changed and I have to touch your code to keep the tree building when
a function signature changes.
To say that I am upset about this would be an understatement, it blocks
normal smooth development of networking infrastructure.
^ permalink raw reply
* Re: [patch] net/fec: "u32" is more explicit than "unsigned long"
From: David Miller @ 2013-08-27 18:51 UTC (permalink / raw)
To: bhutchings
Cc: dan.carpenter, grant.likely, rob.herring, fabio.estevam, Frank.Li,
jim_baxter, B38611, netdev, devicetree, kernel-janitors
In-Reply-To: <1377261869.3364.3.camel@bwh-desktop.uk.level5networks.com>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Fri, 23 Aug 2013 13:44:29 +0100
> On Fri, 2013-08-23 at 12:49 +0300, Dan Carpenter wrote:
>> tmpaddr[] is a six byte array. We want to set the first four bytes on
>> the first line and the remaining two on the next line. The code assumes
>> that "unsigned long" is 32 bits and obviously that's not true on 64 bit
>> arches. It's better to just use u32 instead.
>>
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>> ---
>> This is a static checker thing and I can't compile this file.
>>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
>> index fdf9307..422b125 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -1100,9 +1100,9 @@ static void fec_get_mac(struct net_device *ndev)
>> * 4) FEC mac registers set by bootloader
>> */
>> if (!is_valid_ether_addr(iap)) {
>> - *((unsigned long *) &tmpaddr[0]) =
>> + *((u32 *) &tmpaddr[0]) =
>> be32_to_cpu(readl(fep->hwp + FEC_ADDR_LOW));
>> - *((unsigned short *) &tmpaddr[4]) =
>> + *((u16 *) &tmpaddr[4]) =
>> be16_to_cpu(readl(fep->hwp + FEC_ADDR_HIGH) >> 16);
>> iap = &tmpaddr[0];
>> }
>
> This code also seems to have CPU vs big-endian byte order the wrong way
> round. readl() returns bytes in native order whereas we always store
> MAC addresses in network (big-endian) order. So I think it should be
> doing:
>
> *((__be32 *) &tmpaddr[0]) =
> cpu_to_be32(readl(fep->hwp + FEC_ADDR_LOW));
> *((__be16 *) &tmpaddr[4]) =
> cpu_to_be16(readl(fep->hwp + FEC_ADDR_HIGH) >> 16);
Dan please resubmit with Ben's suggested changes, thanks.
^ permalink raw reply
* Re: [PATCH net-next v2 1/2] ipv4: IP_TOS and IP_TTL can be specified as ancillary data
From: David Miller @ 2013-08-27 18:56 UTC (permalink / raw)
To: ffusco; +Cc: netdev
In-Reply-To: <aaa7e0af2fe535ea5216e438b5f65db88dcef8b2.1377257704.git.ffusco@redhat.com>
From: Francesco Fusco <ffusco@redhat.com>
Date: Fri, 23 Aug 2013 14:19:32 +0200
> - changed the icmp_cookie ttl field from __s16 to __u8.
> A value of 0 means that the TTL has not been specified
Sorry, I have to ask you to change the ttl field type back to __s16
and use "-1" to mean not-specified.
Zero is a valid TTL setting and it means to not allow the
packet to leave this host.
Please make this change and resubmit, thanks.
^ permalink raw reply
* Re: [patch] net/fec: "u32" is more explicit than "unsigned long"
From: Dan Carpenter @ 2013-08-27 18:59 UTC (permalink / raw)
To: David Miller
Cc: bhutchings, grant.likely, rob.herring, fabio.estevam, Frank.Li,
jim_baxter, B38611, netdev, devicetree, kernel-janitors
In-Reply-To: <20130827.145144.478588680902912563.davem@davemloft.net>
On Tue, Aug 27, 2013 at 02:51:44PM -0400, David Miller wrote:
>
> Dan please resubmit with Ben's suggested changes, thanks.
Yes. Of course. Sorry for that, I'll resend tomorrow.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH v2 1/4] jme: lower NAPI weight
From: David Miller @ 2013-08-27 19:00 UTC (permalink / raw)
To: mschmidt; +Cc: sergei.shtylyov, netdev, edumazet, cooldavid
In-Reply-To: <52176665.2030507@redhat.com>
From: Michal Schmidt <mschmidt@redhat.com>
Date: Fri, 23 Aug 2013 15:40:53 +0200
> Since commit 82dc3c63 ("net: introduce NAPI_POLL_WEIGHT")
> netif_napi_add() produces an error message if a NAPI poll weight
> greater than 64 is requested.
>
> jme requests a quarter of the rx ring size as the NAPI weight.
> jme's rx ring size is 1 << 9 = 512.
>
> Use the standard NAPI weight.
>
> v2: proper reference to the related commit
>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
Applied.
^ permalink raw reply
* Re: [PATCH v2 2/4] netxen: lower NAPI weight
From: David Miller @ 2013-08-27 19:00 UTC (permalink / raw)
To: mschmidt
Cc: sergei.shtylyov, netdev, edumazet, manish.chopra, sony.chacko,
rajesh.borundia
In-Reply-To: <52176675.3060202@redhat.com>
From: Michal Schmidt <mschmidt@redhat.com>
Date: Fri, 23 Aug 2013 15:41:09 +0200
> Since commit 82dc3c63 ("net: introduce NAPI_POLL_WEIGHT")
> netif_napi_add() produces an error message if a NAPI poll weight
> greater than 64 is requested.
>
> Use the standard NAPI weight.
>
> v2: proper reference to the related commit
>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
Applied.
^ permalink raw reply
* Re: [PATCH v2 3/4] ps3_gelic: lower NAPI weight
From: David Miller @ 2013-08-27 19:00 UTC (permalink / raw)
To: mschmidt; +Cc: geoff, netdev, edumazet, sergei.shtylyov
In-Reply-To: <5217667F.5010207@redhat.com>
From: Michal Schmidt <mschmidt@redhat.com>
Date: Fri, 23 Aug 2013 15:41:19 +0200
> Since commit 82dc3c63 ("net: introduce NAPI_POLL_WEIGHT")
> netif_napi_add() produces an error message if a NAPI poll weight
> greater than 64 is requested.
>
> GELIC_NET_NAPI_WEIGHT is defined to GELIC_NET_RX_DESCRIPTORS,
> which is 128.
>
> Use the standard NAPI weight.
>
> v2: proper reference to the related commit
>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> Acked-by: Geoff Levand <geoff@infradead.org>
Applied.
^ 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