* Re: [PATCH 3/6] rtlwifi: btcoexist: Add routines for RTL8812AE with single antenna
From: Larry Finger @ 2015-01-23 20:24 UTC (permalink / raw)
To: Kalle Valo
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA, Troy Tan,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <87r3uljm7o.fsf-HodKDYzPHsUD5k0oWYwrnHL1okKdlPRT@public.gmane.org>
On 01/23/2015 02:06 PM, Kalle Valo wrote:
> Larry Finger <Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org> writes:
>
>> From: Troy Tan <troy_tan-kXabqFNEczNtrwSWzY7KCg@public.gmane.org>
>>
>> The RTL8812AE needs different BT coexistence routines than does the
>> RTL8821AE. This patch adds the necessary routines for devices with a
>> single antenna.
>>
>> Signed-off-by: Troy Tan <troy_tan-kXabqFNEczNtrwSWzY7KCg@public.gmane.org>
>> Signed-off-by: Larry Finger <Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
>> ---
>> .../wireless/rtlwifi/btcoexist/halbtc8812a1ant.c | 2073 ++++++++++++++++++++
>> .../wireless/rtlwifi/btcoexist/halbtc8812a1ant.h | 152 ++
>> 2 files changed, 2225 insertions(+)
>> create mode 100644 drivers/net/wireless/rtlwifi/btcoexist/halbtc8812a1ant.c
>> create mode 100644 drivers/net/wireless/rtlwifi/btcoexist/halbtc8812a1ant.h
>>
>> diff --git a/drivers/net/wireless/rtlwifi/btcoexist/halbtc8812a1ant.c b/drivers/net/wireless/rtlwifi/btcoexist/halbtc8812a1ant.c
>> new file mode 100644
>> index 0000000..11ae66b
>> --- /dev/null
>> +++ b/drivers/net/wireless/rtlwifi/btcoexist/halbtc8812a1ant.c
>> @@ -0,0 +1,2073 @@
>> +/* */
>> +/* Description: */
>> +/* */
>> +/* This file is for 8812a1ant Co-exist mechanism */
>> +/* */
>> +/* History */
>> +/* 2012/11/15 Cosa first check in. */
>> +/* */
>> +/* */
>> +
>> +/* */
>> +/* include files */
>> +/* */
>
> One problem I see is that there's no license on either of the files. It
> would be much better if the files had that.
I will add that and send a V2 after I wait a while for any other comments.
Thanks,
Larry
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] atheros/atlx: Simplify bit manipulations
From: Francois Romieu @ 2015-01-23 20:26 UTC (permalink / raw)
To: Rasmus Villemoes; +Cc: Jay Cliburn, Chris Snook, netdev, linux-kernel
In-Reply-To: <1422011212-30095-1-git-send-email-linux@rasmusvillemoes.dk>
Rasmus Villemoes <linux@rasmusvillemoes.dk> :
[...]
> diff --git a/drivers/net/ethernet/atheros/atlx/atl2.c b/drivers/net/ethernet/atheros/atlx/atl2.c
> index 84a09e8ddd9c..46d1b959daa8 100644
> --- a/drivers/net/ethernet/atheros/atlx/atl2.c
> +++ b/drivers/net/ethernet/atheros/atlx/atl2.c
> @@ -1278,14 +1278,10 @@ static void atl2_setup_pcicmd(struct pci_dev *pdev)
>
> pci_read_config_word(pdev, PCI_COMMAND, &cmd);
>
> - if (cmd & PCI_COMMAND_INTX_DISABLE)
> - cmd &= ~PCI_COMMAND_INTX_DISABLE;
> - if (cmd & PCI_COMMAND_IO)
> - cmd &= ~PCI_COMMAND_IO;
> - if (0 == (cmd & PCI_COMMAND_MEMORY))
> - cmd |= PCI_COMMAND_MEMORY;
> - if (0 == (cmd & PCI_COMMAND_MASTER))
> - cmd |= PCI_COMMAND_MASTER;
> + cmd &= ~PCI_COMMAND_INTX_DISABLE;
> + cmd &= ~PCI_COMMAND_IO;
> + cmd |= PCI_COMMAND_MEMORY;
> + cmd |= PCI_COMMAND_MASTER;
> pci_write_config_word(pdev, PCI_COMMAND, cmd);
Mostly open-coded pci_set_master, pci_enable_device_mem and pci_intx.
I'd suggest to ignore the PCI_COMMAND_IO bit at all then use the standard
pci helpers.
--
Ueimor
^ permalink raw reply
* Re: [PATCH net-next 0/3] openvswitch: Add STT support.
From: Tom Herbert @ 2015-01-23 20:57 UTC (permalink / raw)
To: Jesse Gross; +Cc: Pravin B Shelar, David Miller, Linux Netdev List
In-Reply-To: <CAEP_g=-Yn849TmrN5xFLJVxuux0fBuYGFSM7K-psg=pdxgPR9w@mail.gmail.com>
On Fri, Jan 23, 2015 at 12:20 PM, Jesse Gross <jesse@nicira.com> wrote:
> On Fri, Jan 23, 2015 at 10:25 AM, Tom Herbert <therbert@google.com> wrote:
>> On Fri, Jan 23, 2015 at 9:38 AM, Jesse Gross <jesse@nicira.com> wrote:
>>> On Fri, Jan 23, 2015 at 8:58 AM, Tom Herbert <therbert@google.com> wrote:
>>>> On Tue, Jan 20, 2015 at 12:25 PM, Pravin B Shelar <pshelar@nicira.com> wrote:
>>>>> Following patch series adds support for Stateless Transport
>>>>> Tunneling protocol.
>>>>> STT uses TCP segmentation offload available in most of NIC. On
>>>>> packet xmit STT driver appends STT header along with TCP header
>>>>> to the packet. For GSO packet GSO parameters are set according
>>>>> to tunnel configuration and packet is handed over to networking
>>>>> stack. This allows use of segmentation offload available in NICs
>>>>>
>>>>> Netperf unidirectional test gives ~9.4 Gbits/s performance on 10Gbit
>>>>> NIC with 1500 byte MTU with two TCP streams.
>>>>>
>>>> The reason you're able to get 9.4 Gbit/s with an L2 encapsulation
>>>> using STT is that it has less protocol overhead per packet when doing
>>>> segmentation compared to VXLAN (without segmentation STT packets will
>>>> have more overhead than VXLAN).
>>>>
>>>> A VXLAN packet with TCP/IP has headers
>>>> IP|UDP|VXLAN|Ethernet|IP|TCP+options. Assuming TCP is stuffed with
>>>> options, this is 20+8+8+16+20+40=112 bytes, or 7.4% MTU. Each STT
>>>> segment created in GSO, other than the first, has just IP|TCP headers
>>>> which is 20+20=40 bytes or 2.6% MTU. So this explains throughput
>>>> differences between VXLAN and STT.
>>>
>>> Tom, what performance do you see with a single stream of VXLAN running
>>> on net-next with default configuration? The difference in numbers
>>> being posted here is greater than a few percent caused by protocol
>>> overheard.
>>
>> Please look at the data I posted with the VXLAN RCO patches.
>
> The data you posted uses 200 streams, so I assume that you are using
> multiple CPUs. It's not surprising that you would be able to consume a
> 10G link in that case. STT can do this with a single stream and less
> than 1 core (or alternately handle higher throughput). Claiming that
> since both can hit 10G they are same is not accurate.
>
> Discussing performance like this seems a little silly given that the
> code is available. Pravin posted some numbers that he got, if you want
> to dispute them then why don't you just try running it?
Because you haven't provided network interface like I already
requested twice, and I really don't have time or motivation to do
development on your patches or figure out how to do this with OVS. If
you want me to test your patches re-spin them with a network interface
included.
^ permalink raw reply
* udp_diag: Fix socket skipping within chain
From: Herbert Xu @ 2015-01-23 21:02 UTC (permalink / raw)
To: Pavel Emelyanov, netdev
While working on rhashtable walking I noticed that the UDP diag
dumping code is buggy. In particular, the socket skipping within
a chain never happens, even though we record the number of sockets
that should be skipped.
As this code was supposedly copied from TCP, this patch does what
TCP does and resets num before we walk a chain.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/net/ipv4/udp_diag.c b/net/ipv4/udp_diag.c
index 7927db0..4a000f1 100644
--- a/net/ipv4/udp_diag.c
+++ b/net/ipv4/udp_diag.c
@@ -99,11 +99,13 @@ static void udp_dump(struct udp_table *table, struct sk_buff *skb, struct netlin
s_slot = cb->args[0];
num = s_num = cb->args[1];
- for (slot = s_slot; slot <= table->mask; num = s_num = 0, slot++) {
+ for (slot = s_slot; slot <= table->mask; s_num = 0, slot++) {
struct sock *sk;
struct hlist_nulls_node *node;
struct udp_hslot *hslot = &table->hash[slot];
+ num = 0;
+
if (hlist_nulls_empty(&hslot->head))
continue;
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply related
* [PATCH] net: dsa/mv88e6xxx: add reg read and write debug
From: Vivien Didelot @ 2015-01-23 21:10 UTC (permalink / raw)
To: netdev; +Cc: Vivien Didelot, David S . Miller, linux-kernel, kernel
This commit adds debug messages for the generic mv88e6xxx read and write
routines. The output is similar to this:
mdio-gpio mdio-gpio.0: <- addr: 0x1b reg: 0x05 val: 0x4000
mdio-gpio mdio-gpio.0: -> addr: 0x1b reg: 0x07 val: 0x3113
mdio-gpio mdio-gpio.0: -> addr: 0x1b reg: 0x08 val: 0x0330
mdio-gpio mdio-gpio.0: -> addr: 0x1b reg: 0x09 val: 0x0000
This is convenient to dynamically debug operations through debugfs with:
echo file mv88e6xxx.c +p > <debugfs>/dynamic_debug/control
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
drivers/net/dsa/mv88e6xxx.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index cd6807c..3e7e31a 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -85,6 +85,12 @@ int mv88e6xxx_reg_read(struct dsa_switch *ds, int addr, int reg)
ret = __mv88e6xxx_reg_read(bus, ds->pd->sw_addr, addr, reg);
mutex_unlock(&ps->smi_mutex);
+ if (ret < 0)
+ return ret;
+
+ dev_dbg(ds->master_dev, "<- addr: 0x%.2x reg: 0x%.2x val: 0x%.4x\n",
+ addr, reg, ret);
+
return ret;
}
@@ -128,6 +134,9 @@ int mv88e6xxx_reg_write(struct dsa_switch *ds, int addr, int reg, u16 val)
if (bus == NULL)
return -EINVAL;
+ dev_dbg(ds->master_dev, "-> addr: 0x%.2x reg: 0x%.2x val: 0x%.4x\n",
+ addr, reg, val);
+
mutex_lock(&ps->smi_mutex);
ret = __mv88e6xxx_reg_write(bus, ds->pd->sw_addr, addr, reg, val);
mutex_unlock(&ps->smi_mutex);
--
2.2.2
^ permalink raw reply related
* Re: [PATCH net-next 0/3] openvswitch: Add STT support.
From: Pravin Shelar @ 2015-01-23 21:11 UTC (permalink / raw)
To: Tom Herbert; +Cc: Jesse Gross, David Miller, Linux Netdev List
In-Reply-To: <CA+mtBx8bK16McNgBXKN8Vu6_ifEdyZL7FPzOmXvxLP5AqKx1Hw@mail.gmail.com>
On Fri, Jan 23, 2015 at 12:57 PM, Tom Herbert <therbert@google.com> wrote:
> On Fri, Jan 23, 2015 at 12:20 PM, Jesse Gross <jesse@nicira.com> wrote:
>> On Fri, Jan 23, 2015 at 10:25 AM, Tom Herbert <therbert@google.com> wrote:
>>> On Fri, Jan 23, 2015 at 9:38 AM, Jesse Gross <jesse@nicira.com> wrote:
>>>> On Fri, Jan 23, 2015 at 8:58 AM, Tom Herbert <therbert@google.com> wrote:
>>>>> On Tue, Jan 20, 2015 at 12:25 PM, Pravin B Shelar <pshelar@nicira.com> wrote:
>>>>>> Following patch series adds support for Stateless Transport
>>>>>> Tunneling protocol.
>>>>>> STT uses TCP segmentation offload available in most of NIC. On
>>>>>> packet xmit STT driver appends STT header along with TCP header
>>>>>> to the packet. For GSO packet GSO parameters are set according
>>>>>> to tunnel configuration and packet is handed over to networking
>>>>>> stack. This allows use of segmentation offload available in NICs
>>>>>>
>>>>>> Netperf unidirectional test gives ~9.4 Gbits/s performance on 10Gbit
>>>>>> NIC with 1500 byte MTU with two TCP streams.
>>>>>>
>>>>> The reason you're able to get 9.4 Gbit/s with an L2 encapsulation
>>>>> using STT is that it has less protocol overhead per packet when doing
>>>>> segmentation compared to VXLAN (without segmentation STT packets will
>>>>> have more overhead than VXLAN).
>>>>>
>>>>> A VXLAN packet with TCP/IP has headers
>>>>> IP|UDP|VXLAN|Ethernet|IP|TCP+options. Assuming TCP is stuffed with
>>>>> options, this is 20+8+8+16+20+40=112 bytes, or 7.4% MTU. Each STT
>>>>> segment created in GSO, other than the first, has just IP|TCP headers
>>>>> which is 20+20=40 bytes or 2.6% MTU. So this explains throughput
>>>>> differences between VXLAN and STT.
>>>>
>>>> Tom, what performance do you see with a single stream of VXLAN running
>>>> on net-next with default configuration? The difference in numbers
>>>> being posted here is greater than a few percent caused by protocol
>>>> overheard.
>>>
>>> Please look at the data I posted with the VXLAN RCO patches.
>>
>> The data you posted uses 200 streams, so I assume that you are using
>> multiple CPUs. It's not surprising that you would be able to consume a
>> 10G link in that case. STT can do this with a single stream and less
>> than 1 core (or alternately handle higher throughput). Claiming that
>> since both can hit 10G they are same is not accurate.
>>
>> Discussing performance like this seems a little silly given that the
>> code is available. Pravin posted some numbers that he got, if you want
>> to dispute them then why don't you just try running it?
>
> Because you haven't provided network interface like I already
> requested twice, and I really don't have time or motivation to do
> development on your patches or figure out how to do this with OVS. If
> you want me to test your patches re-spin them with a network interface
> included.
I am woking in it. I will have patches by next week.
^ permalink raw reply
* Re: [RFC PATCH] net: ipv6: Make address flushing on ifdown optional
From: Brian Haley @ 2015-01-23 21:17 UTC (permalink / raw)
To: David Ahern, netdev; +Cc: hannes
In-Reply-To: <54C2919A.2050707@gmail.com>
On 01/23/2015 01:23 PM, David Ahern wrote:
>>> Add a new sysctl to make this behavior optional. Setting defaults to flush
>>> addresses to maintain backwards compatibility. When reset flushing is bypassed:
>>>
>>> [root@f20 ~]# echo 0 > /proc/sys/net/ipv6/conf/eth1/flush_addr_on_down
>>> [root@f20 ~]# ip -6 addr add dev eth1 2000:11:1:1::1/64
>>> [root@f20 ~]# ip addr show dev eth1
>>> 3: eth1: <BROADCAST,MULTICAST> mtu 1500 qdisc pfifo_fast state DOWN group
>>> default qlen 1000
>>> link/ether 02:04:11:22:33:01 brd ff:ff:ff:ff:ff:ff
>>> inet6 2000:11:1:1::1/64 scope global tentative
>>> valid_lft forever preferred_lft forever
>>> [root@f20 ~]# ip link set dev eth1 up
>>> [root@f20 ~]# ip link set dev eth1 down
>>> [root@f20 ~]# ip addr show dev eth1
>>> 3: eth1: <BROADCAST,MULTICAST> mtu 1500 qdisc pfifo_fast state DOWN group
>>> default qlen 1000
>>> link/ether 02:04:11:22:33:01 brd ff:ff:ff:ff:ff:ff
>>> inet6 2000:11:1:1::1/64 scope global
>>> valid_lft forever preferred_lft forever
>>> inet6 fe80::4:11ff:fe22:3301/64 scope link
>>> valid_lft forever preferred_lft forever
>>
>> I think this was brought up in a previous thread on this, but don't you have to
>> do DAD on these addresses once the interface comes back up? Some other system
>> could have come along, done DAD, succeeded, and is now using it. Or does the
>> use of this flag assume the user is Ok without doing DAD, and will deal with the
>> fallout?
>
> You have the same problem today, don't you? Current code allows an IPv6 address
> to be configured on interface in the down state. The intent of this sysctl is to
> allow that address to stay on an up-down cycle.
Yes, looks like ndisc_send_skb() never returns any lower-level error back up to
the caller, so it's assumed the Neighbour Advertisement is always sent.
Although the address will be marked "tentative" until IFF_UP is set.
> I don't have a strong IPv6 background so the first email thread and this RFC
> patch are both asking first and foremost if there is any harm in this behavior.
> None has been raised - so far. To maintain backwards compatibility this is a new
> option which when reset allows the addresses to be retained (not flushed).
Seems as though you're in an RFC grey area then. Personally, I'd do DAD, even
though the possibility of a collision is always very small. But that's just my
opinion.
-Brian
^ permalink raw reply
* net: raw socket accessing invalid memory
From: Sasha Levin @ 2015-01-23 22:18 UTC (permalink / raw)
To: netdev@vger.kernel.org
Cc: David S. Miller, James Morris, yoshfuji, Patrick McHardy, LKML,
Dave Jones, Andrey Ryabinin
Hi all,
While fuzzing with trinity inside a KVM tools guest running the latest -next
kernel and the KASan patchset, I've stumbled on the following spew:
[ 2560.693067] BUG: AddressSanitizer: out of bounds on stack in memcpy_fromiovec+0x24d/0x260 at addr ffff880200697dd0
[ 2560.693067] Read of size 8 by task trinity-c9/25362
[ 2560.693067] page:ffffea000801a5c0 count:0 mapcount:0 mapping: (null) index:0x0
[ 2560.693067] flags: 0x1afffff80000000()
[ 2560.693067] page dumped because: kasan: bad access detected
[ 2560.693067] CPU: 9 PID: 25362 Comm: trinity-c9 Not tainted 3.19.0-rc5-next-20150121-sasha-00064-g3c37e35-dirty #1810
[ 2560.693067] 0000000000000000 0000000000000000 ffff880200697790 ffff8802006976d8
[ 2560.693067] ffffffff92e9e8b7 1ffffd40010034bf ffffea000801a5c0 ffff880200697778
[ 2560.693067] ffffffff81b4a7b2 ffffed00629442ba dffffc0000000000 ffffed00629442b8
[ 2560.693067] Call Trace:
[ 2560.693067] dump_stack (lib/dump_stack.c:52)
[ 2560.693067] kasan_report_error (mm/kasan/report.c:136 mm/kasan/report.c:194)
[ 2560.693067] __asan_report_load8_noabort (mm/kasan/report.c:236)
[ 2560.693067] memcpy_fromiovec (lib/iovec.c:14)
[ 2560.693067] raw_sendmsg (net/ipv4/raw.c:444 net/ipv4/raw.c:606)
[ 2560.693067] inet_sendmsg (net/ipv4/af_inet.c:734)
[ 2560.693067] ? inet_sendmsg (include/net/sock.h:875 net/ipv4/af_inet.c:726)
[ 2560.693067] do_sock_sendmsg (net/socket.c:645 (discriminator 4))
[ 2560.771124] SYSC_sendto (net/socket.c:1782)
[ 2560.794593] SyS_sendto (net/socket.c:1748)
[ 2560.794593] tracesys_phase2 (arch/x86/kernel/entry_64.S:530)
[ 2560.794593] Memory state around the buggy address:
[ 2560.794593] ffff880200697c80: 00 f4 f4 f3 f3 f3 f3 00 00 00 00 00 00 00 00 00
[ 2560.794593] ffff880200697d00: 00 00 00 f1 f1 f1 f1 04 f4 f4 f4 f2 f2 f2 f2 04
[ 2560.794593] >ffff880200697d80: f4 f4 f4 f2 f2 f2 f2 00 00 f4 f4 f2 f2 f2 f2 00
[ 2560.794593] ^
[ 2560.794593] ffff880200697e00: 00 00 00 00 00 00 00 00 00 f4 f4 f2 f2 f2 f2 00
[ 2560.794593] ffff880200697e80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f3
Thanks,
Sasha
^ permalink raw reply
* [PATCH] ping: Fix race in free in receive path
From: subashab @ 2015-01-23 22:26 UTC (permalink / raw)
To: netdev; +Cc: edumazet
An exception is seen in ICMP ping receive path where the skb
destructor sock_rfree() tries to access a freed socket. This happens
because ping_rcv() releases socket reference with sock_put() and this
internally frees up the socket. Later icmp_rcv() will try to free the
skb and as part of this, skb destructor is called and which leads
to a kernel panic as the socket is freed already in ping_rcv().
-->|exception
-007|sk_mem_uncharge
-007|sock_rfree
-008|skb_release_head_state
-009|skb_release_all
-009|__kfree_skb
-010|kfree_skb
-011|icmp_rcv
-012|ip_local_deliver_finish
Fix this incorrect free by cloning this skb and processing this cloned
skb instead.
This patch was suggested by Eric Dumazet
Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
Cc: Eric Dumazet <edumazet@google.com>
---
net/ipv4/ping.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index c0d82f7..2a3720f 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -966,8 +966,11 @@ bool ping_rcv(struct sk_buff *skb)
sk = ping_lookup(net, skb, ntohs(icmph->un.echo.id));
if (sk != NULL) {
+ struct sk_buff *skb2 = skb_clone(skb, GFP_ATOMIC);
+
pr_debug("rcv on socket %p\n", sk);
- ping_queue_rcv_skb(sk, skb_get(skb));
+ if (skb2)
+ ping_queue_rcv_skb(sk, skb2);
sock_put(sk);
return true;
}
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related
* Re: ipvlan: Initial check-in of the IPVLAN driver.
From: Mahesh Bandewar @ 2015-01-23 22:40 UTC (permalink / raw)
To: David Miller; +Cc: dan.carpenter, linux-netdev
In-Reply-To: <20150119.162059.2087370902498436539.davem@davemloft.net>
On Mon, Jan 19, 2015 at 1:20 PM, David Miller <davem@davemloft.net> wrote:
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Date: Mon, 19 Jan 2015 17:40:11 +0300
>
>> The patch 2ad7bf363841: "ipvlan: Initial check-in of the IPVLAN
>> driver." from Nov 23, 2014, leads to the following static checker
>> warning:
>>
>> drivers/net/ipvlan/ipvlan_core.c:380 ipvlan_process_v6_outbound()
>> warn: 'dst' isn't an ERR_PTR
>>
>> drivers/net/ipvlan/ipvlan_core.c
>> 378
>> 379 dst = ip6_route_output(dev_net(dev), NULL, &fl6);
>> 380 if (IS_ERR(dst))
>> 381 goto err;
>>
>> The ip6_route_output() function is not documented but it always returns
>> a valid pointer. I believe you are supposed to check something like:
>>
>> if (dst->error) {
>> ret = dst->error;
>> goto error;
>> }
>>
>> 382
>> 383 skb_dst_drop(skb);
>> 384 skb_dst_set(skb, dst);
>
> This is correct.
Thanks Dan for pointing it out. I'll send a patch correcting that code.
^ permalink raw reply
* Re: [PATCH net-next v3 2/5] swdevice: add new api to set and del bridge port attributes
From: roopa @ 2015-01-23 22:45 UTC (permalink / raw)
To: Jiri Pirko
Cc: sfeldma, jhs, bcrl, tgraf, john.fastabend, stephen, vyasevic,
ronen.arad, netdev, davem, shm, gospo
In-Reply-To: <20150123160636.GM2065@nanopsycho.orion>
On 1/23/15, 8:06 AM, Jiri Pirko wrote:
> Fri, Jan 23, 2015 at 04:58:57PM CET, roopa@cumulusnetworks.com wrote:
>> On 1/23/15, 2:41 AM, Jiri Pirko wrote:
>>
>> <snip..>
>>> +
>>> +/**
>>> + * netdev_switch_port_bridge_dellink - Notify switch device port of bridge
>>> + * attribute delete
>>> + *
>>> + * @dev: port device
>>> + * @nlh: netlink msg with bridge port attributes
>>> + *
>>> + * Notify switch device port of bridge port attribute delete
>>> + */
>>> +int netdev_switch_port_bridge_dellink(struct net_device *dev,
>>> + struct nlmsghdr *nlh, u16 flags)
>>> +{
>>> + const struct net_device_ops *ops = dev->netdev_ops;
>>> + struct net_device *lower_dev;
>>> + struct list_head *iter;
>>> + int ret = 0, err = 0;
>>> +
>>> + if (!(dev->features & NETIF_F_HW_NETFUNC_OFFLOAD))
>>> + return err;
>>> +
>>> + if (ops->ndo_bridge_dellink) {
>>> + WARN_ON(!ops->ndo_switch_parent_id_get);
>>> + return ops->ndo_bridge_dellink(dev, nlh, flags);
>>> + }
>>> +
>>> + netdev_for_each_lower_dev(dev, lower_dev, iter) {
>>> + err = netdev_switch_port_bridge_dellink(lower_dev, nlh, flags);
>>> + if (err)
>>> + ret = err;
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +EXPORT_SYMBOL(netdev_switch_port_bridge_dellink);
>>> --
>>> 1.7.10.4
>>>
>>> Is there any other place, other than bridge code, this functions are
>>> suppored to be called from?
>> No other place today. Its usually the master that implements
>> ndo_bridge_setlink/dellink.
>>
>>> If not, which I consider likely, it would
>>> make more sense to me to:
>>>
>>> - move netdev_for_each_lower_dev iterations directly to bridge code
>>> - let the masters (bond, team, ..) implement ndo_bridge_*link and do
>>> the traversing there (can be in a form of pre-prepared default
>>> ndo callback (ndo_dflt_netdev_switch_port_bridge_*link)
>> But, i am still not understanding why i would modify bond, team and other
>> slaves
> Well, that is the usual way to propagate ndo calls. People are used to
> this. It is visible right away in bonding/other code that is propagated
> some ndo call to slaves. With your code, that is somehow hidden and only
> dependent on NETIF_F_HW_NETFUNC_OFFLOAD flag.
>
> Note that there are only couple of "master drivers" (for this, most likely
> only bond and team modifications are needed).
>
ndo_bridge_setlink today is only implemented by drivers that implement
bridging function.
So, having the bond and team driver implement it...seems odd.
But if you insist, i am going to do just that.
^ permalink raw reply
* Re: [PATCH net-next 1/5] bonding: keep bond interface carrier off until at least one active member
From: Jonathan Toppins @ 2015-01-23 23:04 UTC (permalink / raw)
To: Jay Vosburgh
Cc: netdev, Scott Feldman, Andy Gospodarek, Veaceslav Falico,
Nikolay Aleksandrov
In-Reply-To: <5368.1421824444@famine>
On 1/21/15 2:14 AM, Jay Vosburgh wrote:
> Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote:
>
>> On 1/19/15 4:16 PM, Jay Vosburgh wrote:
>>> Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote:
>>>
>>>> From: Scott Feldman <sfeldma@cumulusnetworks.com>
>>>>
>>>> Bonding driver parameter min_links is now used to signal upper-level
>>>> protocols of bond status. The way it works is if the total number of
>>>> active members in slaves drops below min_links, the bond link carrier
>>>> will go down, signaling upper levels that bond is inactive. When active
>>>> members returns to >= min_links, bond link carrier will go up (RUNNING),
>>>> and protocols can resume. When bond is carrier down, member ports are
>>>> in stp fwd state blocked (rather than normal disabled state), so
>>>> low-level ctrl protocols (LACP) can still get in and be processed by
>>>> bonding driver.
>>>
>>> Presuming that "stp" is Spanning Tree, is the last sentence
>>> above actually describing the behavior of a bridge port when a bond is
>>> the member of the bridge? I'm not sure I understand what "member ports"
>>> refers to (bridge ports or bonding slaves).
>>
>> Ack, maybe replacing the last sentence with something like:
>> When bond is carrier down, the slave ports are only forwarding
>> low-level control protocols (e.g. LACP PDU) and discarding all other
>> packets.
>
> Ah, are you actually referring to the fact that slaves that are
> up will still deliver packets to listeners that bind directly to the
> slave or hook in through a rx_handler? This is, in part, the
> "RX_HANDLER_EXACT" business in bond_handle_frame and
> __netif_receive_skb_core.
>
> The decision for that has nothing to do with the protocol; I
> seem to recall that FCoE (or maybe it's iSCSI) does its regular traffic
> reception this way (although via dev_add_pack, not an rx_handler) so it
> can run traffic regardless of the bonding master's state.
I see, it seems you are basically saying; the slaves are up but when the
logical bond interface is carrier down there was no code changed in
bond_handle_frame() to actually drop frames other than LACPDUs. So
basically having this statement makes no sense until there is code to
actually drop those additional frames.
>
>>>> @@ -2381,10 +2386,15 @@ int bond_3ad_set_carrier(struct bonding *bond)
>>>> ret = 0;
>>>> goto out;
>>>> }
>>>> +
>>>> + bond_for_each_slave_rcu(bond, slave, iter)
>>>> + if (SLAVE_AD_INFO(slave)->aggregator.is_active)
>>>> + active_slaves++;
>>>> +
>>>> active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator));
>>>> - if (active) {
>>>> + if (active && __agg_has_partner(active)) {
>>>
>>> Why "__agg_has_partner"? Since the "else" of this clause is:
>>>
>>> } else if (netif_carrier_ok(bond->dev)) {
>>> netif_carrier_off(bond->dev);
>>> }
>>>
>>> I'm wondering if this will do the right thing for the case that
>>> there are no LACP partners at all (e.g., the switch ports do not have
>>> LACP enabled), in which case the active aggregator should be a single
>>> "individual" port as a fallback, but will not have a partner.
>>>
>>> -J
>>>
>>
>> I see your point. The initial thinking was the logical bond carrier should
>> not be brought up until the bond has a partner and is ready to pass
>> traffic, otherwise we start blackholing frames. Looking over the code it
>> seems the aggregator.is_individual flag is only set to true when a slave
>> is in half-duplex, this seems odd?
>
> The agg.is_individual flag and an "individual" aggregator are
> subtly different things.
>
> The is_individual flag is part of the implementation of the
> standard requirement that half duplex ports are not allowed to enable
> LACP (and thus cannot aggregate, and end up as "Individual" in
> standard-ese). The standard capitalizes "Individual" when it describes
> the cannot-aggregate property of a port (note that half duplex is only
> one reason of many for a port being Individual).
>
> An "individual" aggregator (my usage of 802.1AX 5.3.6 (b)) is an
> aggregator containing exactly one port that is Individual. A port can
> end up as Individual (for purposes of this discussion) either through
> the is_individual business, or because the bonding port does run LACP,
> but the link partner does not, and thus no LACPDUs are ever received.
>
> For either of the above cases (is_individual or no-LACP-parter),
> then the active aggregator will be an "individual" aggregator, but will
> not have a parter (__agg_has_partner() will be false). The standard has
> a bunch of verbiage about this in 802.1AX 5.3.5 - 5.3.9.
>
>> My initial thinking to alleviate the concern is something like the
>> following:
>>
>> if (active && !SLAVE_AD_INFO(slave)->aggregator.is_individual &&
>> __agg_has_partner(active)) {
>> /* set carrier based on min_links */
>> } else if (active && SLAVE_AD_INFO(slave)->aggregator.is_individual) {
>> /* set bond carrier state according to carrier state of slave */
>> } else if (netif_carrier_ok(bond->dev)) {
>> netif_carrier_off(bond->dev);
>> }
>
> I'm not sure you need to care about is_individual or
> __agg_has_partner at all. If either of those conditions is true for the
> active aggregator, it will contain exactly one port, and so if min_links
> is 2, you'll have carrier off, and if min_links is 1 or less you'll have
> carrier on.
>
> If I'm reading the patch right, the real point (which isn't
> really described very well in the change log) is that you're changing
> the carrier decision to count only active ports in the active
> aggregator, not the total number of ports as is currently done.
>
> I'm not sure why this change is needed:
>
> @@ -2381,10 +2386,15 @@ int bond_3ad_set_carrier(struct bonding *bond)
> ret = 0;
> goto out;
> }
> +
> + bond_for_each_slave_rcu(bond, slave, iter)
> + if (SLAVE_AD_INFO(slave)->aggregator.is_active)
> + active_slaves++;
> +
> active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator));
> - if (active) {
> + if (active && __agg_has_partner(active)) {
> /* are enough slaves available to consider link up? */
> - if (active->num_of_ports < bond->params.min_links) {
> + if (active_slaves < bond->params.min_links) {
> if (netif_carrier_ok(bond->dev)) {
> netif_carrier_off(bond->dev);
> goto out;
>
> because a port (slave) that loses carrier or whose link partner
> becomes unresponsive to LACPDUs will be removed from the aggregator. As
> I recall, there are no "inactive" ports in an aggregator; all of them
> have to match in terms of capabilities.
>
> In other words, I'm unsure of when the count of is_active ports
> will not match active->num_of_ports.
>
> Also, the other parts of the patch add some extra updates to the
> carrier state when a port is enabled or disabled, e.g.,
>
> @@ -189,6 +189,7 @@ static inline int __agg_has_partner(struct aggregator *agg)
> static inline void __disable_port(struct port *port)
> {
> bond_set_slave_inactive_flags(port->slave, BOND_SLAVE_NOTIFY_LATER);
> + bond_3ad_set_carrier(port->slave->bond);
> }
>
> Again, I'm not sure why this is necessary, as the cases that
> disable or enable a port will eventually call bond_3ad_set_carrier. For
> example, ad_agg_selection_logic will, when changing active aggregator,
> individually disable all ports of the old active and then may
> individually enable ports of the new active if necessary, and then
> finally call bond_3ad_set_carrier.
>
> In what situations is the patch's behavior an improvement (i.e.,
> is there a situation I'm missing that doesn't do it right)?
I think the addition of bond_3ad_set_carrier() to both __enable_port()
and __disable_port() were optimizations so the bond carrier transition
would happen faster, though I am not certain.
>
> The last portion of the patch:
>
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -1181,6 +1181,7 @@ static int bond_option_min_links_set(struct bonding *bond,
> netdev_info(bond->dev, "Setting min links value to %llu\n",
> newval->value);
> bond->params.min_links = newval->value;
> + bond_set_carrier(bond);
>
> return 0;
> }
>
> does seem to fix a legitimate bug, in that when min_links is
> changed, it does not take effect in real time.
>
>> Maybe I am missing something and there is a simpler option.
>>
>> Thinking about how to validate this, it seems having a bond with two
>> slaves and both slaves in half-duplex will force an aggregator that is
>> individual to be selected.
>>
>> Thoughts?
>
> That's one way, yes. You'll also get an "individual" aggregator
> if none of the link partners enable LACP.
>
It seems it might be better to drop the changes to __enable/disable_port
and bond_3ad_set_carrier from this patch until more testing can be done
from me, especially if you agree the other changes in this series are of
benefit.
^ permalink raw reply
* Re: [PATCH] ping: Fix race in free in receive path
From: Eric Dumazet @ 2015-01-23 23:07 UTC (permalink / raw)
To: subashab; +Cc: netdev, edumazet
In-Reply-To: <45fab13fce077924f957cba84ba20ba4.squirrel@www.codeaurora.org>
On Fri, 2015-01-23 at 22:26 +0000, subashab@codeaurora.org wrote:
> An exception is seen in ICMP ping receive path where the skb
> destructor sock_rfree() tries to access a freed socket. This happens
> because ping_rcv() releases socket reference with sock_put() and this
> internally frees up the socket. Later icmp_rcv() will try to free the
> skb and as part of this, skb destructor is called and which leads
> to a kernel panic as the socket is freed already in ping_rcv().
...
> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
> Cc: Eric Dumazet <edumazet@google.com>
> ---
Thanks !
Signed-off-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* Re: [PATCH net-next v3 1/5] netdev: introduce new NETIF_F_HW_NETFUNC_OFFLOAD feature flag for switch device offloads
From: roopa @ 2015-01-23 23:09 UTC (permalink / raw)
To: Jiri Pirko
Cc: Thomas Graf, sfeldma, jhs, bcrl, john.fastabend, stephen,
vyasevic, ronen.arad, netdev, davem, shm, gospo
In-Reply-To: <20150123190518.GP2065@nanopsycho.orion>
On 1/23/15, 11:05 AM, Jiri Pirko wrote:
> Fri, Jan 23, 2015 at 06:56:54PM CET, tgraf@suug.ch wrote:
>> On 01/23/15 at 07:48am, roopa wrote:
>>> On 1/23/15, 1:44 AM, Jiri Pirko wrote:
>>>> How about rather "HW_DATAPATH_OFFLOAD"? Feels more accurate. By the
>>>> name, I still cannot understand what NETFUNC should mean.
>>> It was supposed to mean 'network function offload'. sure, will consider
>>> HW_DATAPATH_OFFLOAD
>>> or if anybody has other suggestions.
>> I'm still fascinated that the description says 'for all switch asic
>> offloads', 'switch drivers', 'switch ports' but we try very hard to
>> come up with a different name ;-)
> +1
> But we'll see in future. I bet that "switch" will stick.
because....am sure anything else will get rejected ;).
...The API/patches is currently targeting switch ASIC devices but the
idea behind having the flag
not be called 'switch' was to cover all the devices being discussed
today (previous threads on this indicate that).
Its fair to say that I only care about switch asics today. If that's the
popular vote i am going with that.
renaming it in the next series...
^ permalink raw reply
* Re: [PATCH net-next v3 2/5] swdevice: add new api to set and del bridge port attributes
From: roopa @ 2015-01-23 23:10 UTC (permalink / raw)
To: Jiri Pirko
Cc: sfeldma, jhs, bcrl, tgraf, john.fastabend, stephen, vyasevic,
ronen.arad, netdev, davem, shm, gospo
In-Reply-To: <54C2CF1A.1080905@cumulusnetworks.com>
On 1/23/15, 2:45 PM, roopa wrote:
> On 1/23/15, 8:06 AM, Jiri Pirko wrote:
>> Fri, Jan 23, 2015 at 04:58:57PM CET, roopa@cumulusnetworks.com wrote:
>>> On 1/23/15, 2:41 AM, Jiri Pirko wrote:
>>>
>>> <snip..>
>>>> +
>>>> +/**
>>>> + * netdev_switch_port_bridge_dellink - Notify switch device
>>>> port of bridge
>>>> + * attribute delete
>>>> + *
>>>> + * @dev: port device
>>>> + * @nlh: netlink msg with bridge port attributes
>>>> + *
>>>> + * Notify switch device port of bridge port attribute delete
>>>> + */
>>>> +int netdev_switch_port_bridge_dellink(struct net_device *dev,
>>>> + struct nlmsghdr *nlh, u16 flags)
>>>> +{
>>>> + const struct net_device_ops *ops = dev->netdev_ops;
>>>> + struct net_device *lower_dev;
>>>> + struct list_head *iter;
>>>> + int ret = 0, err = 0;
>>>> +
>>>> + if (!(dev->features & NETIF_F_HW_NETFUNC_OFFLOAD))
>>>> + return err;
>>>> +
>>>> + if (ops->ndo_bridge_dellink) {
>>>> + WARN_ON(!ops->ndo_switch_parent_id_get);
>>>> + return ops->ndo_bridge_dellink(dev, nlh, flags);
>>>> + }
>>>> +
>>>> + netdev_for_each_lower_dev(dev, lower_dev, iter) {
>>>> + err = netdev_switch_port_bridge_dellink(lower_dev, nlh,
>>>> flags);
>>>> + if (err)
>>>> + ret = err;
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +EXPORT_SYMBOL(netdev_switch_port_bridge_dellink);
>>>> --
>>>> 1.7.10.4
>>>>
>>>> Is there any other place, other than bridge code, this functions are
>>>> suppored to be called from?
>>> No other place today. Its usually the master that implements
>>> ndo_bridge_setlink/dellink.
>>>
>>>> If not, which I consider likely, it would
>>>> make more sense to me to:
>>>>
>>>> - move netdev_for_each_lower_dev iterations directly to bridge code
>>>> - let the masters (bond, team, ..) implement ndo_bridge_*link and do
>>>> the traversing there (can be in a form of pre-prepared default
>>>> ndo callback (ndo_dflt_netdev_switch_port_bridge_*link)
>>> But, i am still not understanding why i would modify bond, team and
>>> other
>>> slaves
>> Well, that is the usual way to propagate ndo calls. People are used to
>> this. It is visible right away in bonding/other code that is propagated
>> some ndo call to slaves. With your code, that is somehow hidden and only
>> dependent on NETIF_F_HW_NETFUNC_OFFLOAD flag.
>>
>> Note that there are only couple of "master drivers" (for this, most
>> likely
>> only bond and team modifications are needed).
> ndo_bridge_setlink today is only implemented by drivers that implement
> bridging function.
> So, having the bond and team driver implement it...seems odd.
>
> But if you insist, i am going to do just that.
A side note, I dont see any reason for ndo_bridge_setlink to be renamed
to ndo_setlink. Because it seems to take the whole netlink msg anyways.
It can be used to offload other link attributes besides bridging (vxlan
and so on).
Any thoughts on that ?.
^ permalink raw reply
* Re: [net-next PATCH v3 00/12] Flow API
From: Thomas Graf @ 2015-01-23 23:16 UTC (permalink / raw)
To: John Fastabend
Cc: Jiri Pirko, Jamal Hadi Salim, Pablo Neira Ayuso, simon.horman,
sfeldma, netdev, davem, gerlitz.or, andy, ast
In-Reply-To: <54C2A805.7030401@gmail.com>
[Skipping the tc model part for now. Need some time to digest
all of that]
On 01/23/15 at 11:59am, John Fastabend wrote:
> On 01/23/2015 09:46 AM, Thomas Graf wrote:
> >.... if we can get rid of the rtnl lock in the flow mod path ;-)
>
> Well isn't it the qdisc lock here? And its not needed anymore for
> filters/actions only qdisc's use it because they are not lock-safe
> yet. Its been on my backlog to start replacing the skb lists with
> lock-free rings but I haven't got anywhere on this yet.
>
> Although a hardware doesn't really need a queuing discipline its
> done in hardware so you could drop the qdisc lock in this case.
I'm not even in the data path yet with that comment. I'm worried
with the locking in the control path as talking rtnetlink implies
taking rtnl for each flow modification.
Agreed that we wouldn't depend on the qdisc lock for offloaded flows.
^ permalink raw reply
* [PATCH] net: dsa/mv88e6352: make mv88e6352_wait generic
From: Vivien Didelot @ 2015-01-23 23:34 UTC (permalink / raw)
To: netdev; +Cc: Vivien Didelot, Guenter Roeck, linux-kernel, kernel
Some busy bits are available in the global register 1, such as the ATU
Busy bit. We may want to use this function to wait for them to change,
so add a new parameter to mv88e6352_wait() instead of hard-coding
REG_GLOBAL2.
In the meantime, since the REG_READ() macro already checks for error,
remove the redundant check for ret < 0.
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
drivers/net/dsa/mv88e6352.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c
index 258d9ef..e13adc7 100644
--- a/drivers/net/dsa/mv88e6352.c
+++ b/drivers/net/dsa/mv88e6352.c
@@ -22,17 +22,14 @@
#include <net/dsa.h>
#include "mv88e6xxx.h"
-static int mv88e6352_wait(struct dsa_switch *ds, int reg, u16 mask)
+static int mv88e6352_wait(struct dsa_switch *ds, int reg, int offset, u16 mask)
{
unsigned long timeout = jiffies + HZ / 10;
while (time_before(jiffies, timeout)) {
int ret;
- ret = REG_READ(REG_GLOBAL2, reg);
- if (ret < 0)
- return ret;
-
+ ret = REG_READ(reg, offset);
if (!(ret & mask))
return 0;
@@ -43,17 +40,17 @@ static int mv88e6352_wait(struct dsa_switch *ds, int reg, u16 mask)
static inline int mv88e6352_phy_wait(struct dsa_switch *ds)
{
- return mv88e6352_wait(ds, 0x18, 0x8000);
+ return mv88e6352_wait(ds, REG_GLOBAL2, 0x18, 0x8000);
}
static inline int mv88e6352_eeprom_load_wait(struct dsa_switch *ds)
{
- return mv88e6352_wait(ds, 0x14, 0x0800);
+ return mv88e6352_wait(ds, REG_GLOBAL2, 0x14, 0x0800);
}
static inline int mv88e6352_eeprom_busy_wait(struct dsa_switch *ds)
{
- return mv88e6352_wait(ds, 0x14, 0x8000);
+ return mv88e6352_wait(ds, REG_GLOBAL2, 0x14, 0x8000);
}
static int __mv88e6352_phy_read(struct dsa_switch *ds, int addr, int regnum)
--
2.2.2
^ permalink raw reply related
* Re: [PATCH] ethernet: fm10k: Actually drop 4 bits
From: Vick, Matthew @ 2015-01-24 0:18 UTC (permalink / raw)
To: Rasmus Villemoes, Alexander Duyck, Kirsher, Jeffrey T
Cc: e1000-devel@lists.sourceforge.net, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <1421967198-16667-1-git-send-email-linux@rasmusvillemoes.dk>
On 1/22/15, 2:53 PM, "Rasmus Villemoes" <linux@rasmusvillemoes.dk> wrote:
>The comment explains the intention, but vid has type u16. Before the
>inner shift, it is promoted to int, which has plenty of space for all
>vid's bits, so nothing is dropped. Use a simple mask instead.
>
>Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>---
> drivers/net/ethernet/intel/fm10k/fm10k_pf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pf.c
>b/drivers/net/ethernet/intel/fm10k/fm10k_pf.c
>index 275423d4f777..b1c57d0166a9 100644
>--- a/drivers/net/ethernet/intel/fm10k/fm10k_pf.c
>+++ b/drivers/net/ethernet/intel/fm10k/fm10k_pf.c
>@@ -335,7 +335,7 @@ static s32 fm10k_update_xc_addr_pf(struct fm10k_hw
>*hw, u16 glort,
> return FM10K_ERR_PARAM;
>
> /* drop upper 4 bits of VLAN ID */
>- vid = (vid << 4) >> 4;
>+ vid &= 0x0fff;
>
> /* record fields */
> mac_update.mac_lower = cpu_to_le32(((u32)mac[2] << 24) |
Good catch! I noticed this too and was getting a patch together to address
this.
The difference is that I was planning on not silently accepting an invalid
VLAN ID to begin with and returning FM10K_ERR_PARAM if the VLAN was
invalid, which I think is a better approach for this situation. If it's
alright with you, I'll generate the patch shortly and credit you via your
Reported-by.
Cheers,
Matthew
^ permalink raw reply
* Re: [PATCH] ethernet: fm10k: Actually drop 4 bits
From: Rasmus Villemoes @ 2015-01-24 0:50 UTC (permalink / raw)
To: Vick, Matthew
Cc: Alexander Duyck, Kirsher, Jeffrey T,
e1000-devel@lists.sourceforge.net, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <D0E81E0C.682EB%matthew.vick@intel.com>
On Sat, Jan 24 2015, "Vick, Matthew" <matthew.vick@intel.com> wrote:
> Good catch! I noticed this too and was getting a patch together to address
> this.
>
> The difference is that I was planning on not silently accepting an invalid
> VLAN ID to begin with and returning FM10K_ERR_PARAM if the VLAN was
> invalid, which I think is a better approach for this situation. If it's
> alright with you, I'll generate the patch shortly and credit you via your
> Reported-by.
Sure, do what you think is best.
Rasmus
^ permalink raw reply
* Re: [PATCH net-next] act_connmark: Add missing dependency on NF_CONNTRACK_MARK
From: David Miller @ 2015-01-24 1:46 UTC (permalink / raw)
To: tgraf; +Cc: netdev, nbd, jhs
In-Reply-To: <eb5e12e8fec691cb08e663f2f8ed183ac92e5a1f.1421757793.git.tgraf@suug.ch>
From: Thomas Graf <tgraf@suug.ch>
Date: Tue, 20 Jan 2015 13:44:25 +0100
> Depending on NETFILTER is not sufficient to ensure the presence of the
> 'mark' field in nf_conn, also needs to depend on NF_CONNTRACK_MARK.
>
> Fixes: 22a5dc ("net: sched: Introduce connmark action")
> Cc: Felix Fietkau <nbd@openwrt.org>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
Applied, thanks Thomas.
^ permalink raw reply
* [PATCH] net: llc: use correct size for sysctl timeout entries
From: Sasha Levin @ 2015-01-24 1:47 UTC (permalink / raw)
To: linux-kernel
Cc: Sasha Levin, Arnaldo Carvalho de Melo, David S. Miller,
open list:NETWORKING [GENERAL]
The timeout entries are sizeof(int) rather than sizeof(long), which
means that when they were getting read we'd also leak kernel memory
to userspace along with the timeout values.
Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
net/llc/sysctl_net_llc.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/llc/sysctl_net_llc.c b/net/llc/sysctl_net_llc.c
index 612a5dd..799bafc 100644
--- a/net/llc/sysctl_net_llc.c
+++ b/net/llc/sysctl_net_llc.c
@@ -18,28 +18,28 @@ static struct ctl_table llc2_timeout_table[] = {
{
.procname = "ack",
.data = &sysctl_llc2_ack_timeout,
- .maxlen = sizeof(long),
+ .maxlen = sizeof(sysctl_llc2_ack_timeout),
.mode = 0644,
.proc_handler = proc_dointvec_jiffies,
},
{
.procname = "busy",
.data = &sysctl_llc2_busy_timeout,
- .maxlen = sizeof(long),
+ .maxlen = sizeof(sysctl_llc2_busy_timeout),
.mode = 0644,
.proc_handler = proc_dointvec_jiffies,
},
{
.procname = "p",
.data = &sysctl_llc2_p_timeout,
- .maxlen = sizeof(long),
+ .maxlen = sizeof(sysctl_llc2_p_timeout),
.mode = 0644,
.proc_handler = proc_dointvec_jiffies,
},
{
.procname = "rej",
.data = &sysctl_llc2_rej_timeout,
- .maxlen = sizeof(long),
+ .maxlen = sizeof(sysctl_llc2_rej_timeout),
.mode = 0644,
.proc_handler = proc_dointvec_jiffies,
},
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH] net: dsa/mv88e6352: make mv88e6352_wait generic
From: Guenter Roeck @ 2015-01-24 1:47 UTC (permalink / raw)
To: Vivien Didelot, netdev; +Cc: linux-kernel, kernel
In-Reply-To: <1422056081-30088-1-git-send-email-vivien.didelot@savoirfairelinux.com>
On 01/23/2015 03:34 PM, Vivien Didelot wrote:
> Some busy bits are available in the global register 1, such as the ATU
> Busy bit. We may want to use this function to wait for them to change,
> so add a new parameter to mv88e6352_wait() instead of hard-coding
> REG_GLOBAL2.
>
> In the meantime, since the REG_READ() macro already checks for error,
> remove the redundant check for ret < 0.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Acked-by: Guenter Roeck <linux@roeck-us.net>
Guenter
^ permalink raw reply
* Re: [PATCH net-next 0/6] netns: advertise netns via netlink
From: David Miller @ 2015-01-24 1:51 UTC (permalink / raw)
To: nicolas.dichtel; +Cc: netdev
In-Reply-To: <1421763347-4354-1-git-send-email-nicolas.dichtel@6wind.com>
From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Tue, 20 Jan 2015 15:15:41 +0100
>
> The first patch of the serie fix a bug of the previous serie (present in
> net-next only).
> The rest of the serie adds an attribute to advertise the peer netns for
> rtnetlink messages where this information is needed by userland to be able to
> interpret fully the received message.
"series" has a final 's'. Well, as least you consistently misspell it
3 times :-)
"Series" applied, thanks!
^ permalink raw reply
* Re: [PATCHv2 net-next] xen-netback: always fully coalesce guest Rx packets
From: David Miller @ 2015-01-24 1:57 UTC (permalink / raw)
To: david.vrabel; +Cc: netdev, xen-devel, ian.campbell, wei.liu2
In-Reply-To: <1421765392-30303-1-git-send-email-david.vrabel@citrix.com>
From: David Vrabel <david.vrabel@citrix.com>
Date: Tue, 20 Jan 2015 14:49:52 +0000
> Always fully coalesce guest Rx packets into the minimum number of ring
> slots. Reducing the number of slots per packet has significant
> performance benefits when receiving off-host traffic.
>
> Results from XenServer's performance benchmarks:
>
> Baseline Full coalesce
> Interhost VM receive 7.2 Gb/s 11 Gb/s
> Interhost aggregate 24 Gb/s 24 Gb/s
> Intrahost single stream 14 Gb/s 14 Gb/s
> Intrahost aggregate 34 Gb/s 34 Gb/s
>
> However, this can increase the number of grant ops per packet which
> decreases performance of backend (dom0) to VM traffic (by ~10%)
> /unless/ grant copy has been optimized for adjacent ops with the same
> source or destination (see "grant-table: defer releasing pages
> acquired in a grant copy"[1] expected in Xen 4.6).
>
> [1] http://lists.xen.org/archives/html/xen-devel/2015-01/msg01118.html
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> Changes in v2:
> - Updated commit message with better results.
Applied, thanks David.
^ permalink raw reply
* Re: [PATCH net-next 1/5] bonding: keep bond interface carrier off until at least one active member
From: Jay Vosburgh @ 2015-01-24 3:15 UTC (permalink / raw)
To: Jonathan Toppins
Cc: netdev, Scott Feldman, Andy Gospodarek, Veaceslav Falico,
Nikolay Aleksandrov
In-Reply-To: <54C2D39B.7070104@cumulusnetworks.com>
Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote:
>On 1/21/15 2:14 AM, Jay Vosburgh wrote:
>> Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote:
>>
>>> On 1/19/15 4:16 PM, Jay Vosburgh wrote:
>>>> Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote:
>>>>
>>>>> From: Scott Feldman <sfeldma@cumulusnetworks.com>
>>>>>
>>>>> Bonding driver parameter min_links is now used to signal upper-level
>>>>> protocols of bond status. The way it works is if the total number of
>>>>> active members in slaves drops below min_links, the bond link carrier
>>>>> will go down, signaling upper levels that bond is inactive. When active
>>>>> members returns to >= min_links, bond link carrier will go up (RUNNING),
>>>>> and protocols can resume. When bond is carrier down, member ports are
>>>>> in stp fwd state blocked (rather than normal disabled state), so
>>>>> low-level ctrl protocols (LACP) can still get in and be processed by
>>>>> bonding driver.
>>>>
>>>> Presuming that "stp" is Spanning Tree, is the last sentence
>>>> above actually describing the behavior of a bridge port when a bond is
>>>> the member of the bridge? I'm not sure I understand what "member ports"
>>>> refers to (bridge ports or bonding slaves).
>>>
>>> Ack, maybe replacing the last sentence with something like:
>>> When bond is carrier down, the slave ports are only forwarding
>>> low-level control protocols (e.g. LACP PDU) and discarding all other
>>> packets.
>>
>> Ah, are you actually referring to the fact that slaves that are
>> up will still deliver packets to listeners that bind directly to the
>> slave or hook in through a rx_handler? This is, in part, the
>> "RX_HANDLER_EXACT" business in bond_handle_frame and
>> __netif_receive_skb_core.
>>
>> The decision for that has nothing to do with the protocol; I
>> seem to recall that FCoE (or maybe it's iSCSI) does its regular traffic
>> reception this way (although via dev_add_pack, not an rx_handler) so it
>> can run traffic regardless of the bonding master's state.
>
>I see, it seems you are basically saying; the slaves are up but when the
>logical bond interface is carrier down there was no code changed in
>bond_handle_frame() to actually drop frames other than LACPDUs. So
>basically having this statement makes no sense until there is code to
>actually drop those additional frames.
What I'm saying is that the fact that bond_handle_frame() does
not outright drop anything is a feature, not an oversight. It's done
this way on purpose so that the slave device can be utilized separately
from its participation in the bond. Off the top of my head, as I recall
this is used by LLDP, FCoE, and probably other "converged" Ethernet
facilities. I believe some network monitoring tools use this property
as well, but I don't recall the details.
Frames received on inactive slaves (which these active slaves
under a carrier-off bond are not; more on that in a bit) are marked as
such by the RX_HANDLER_EXACT return, and do not propagate up the stack
in the usual way, but are delivered only to packet listeners that bind
directly to the slave (typically via a dev_add_pack handler, which is
how LACPDUs were received prior to the rx_handler logic being
implemented).
The bonding inactive slave receive logic used to work the other
way around; anything not explicitly needed by the bond itself would be
dropped in this particular case. The "deliver to exact match" logic was
added later.
Now, the possible hole here that I think you're alluding to is
that if the bond sets itself carrier down due to a min_links violation,
the slaves in the active aggregator are not inactive, and there's
nothing in the receive path to prevent incoming packets from being
processed normally if a receiving slave is still up.
A quick test suggests that this is indeed the case; if I set up
a 802.3ad bond with min_links=3 and two slaves, incoming ICMP ECHOs to
the bond's IP appear to be received by icmp_echo, which calls
icmp_reply, which apparently fails. I'm basing this conclusion on the
IcmpInErrors, IcmpInEchoReps, IcmpOutErrors, IcmpMsgInType8 and
IcmpMsgOutType0 stat counters all incrementing more or less in lock
step. I haven't traced the code to see where it fails.
I'm not sure exactly what ought to be done in that case; one
thought (which I have not tested) is bond_should_deliver_exact_match()
always returns true if the bonding master is carrier down.
Transmit appears to not function (at the bond level), so that
part doesn't appear to be an issue.
>>>>> @@ -2381,10 +2386,15 @@ int bond_3ad_set_carrier(struct bonding *bond)
>>>>> ret = 0;
>>>>> goto out;
>>>>> }
>>>>> +
>>>>> + bond_for_each_slave_rcu(bond, slave, iter)
>>>>> + if (SLAVE_AD_INFO(slave)->aggregator.is_active)
>>>>> + active_slaves++;
>>>>> +
>>>>> active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator));
>>>>> - if (active) {
>>>>> + if (active && __agg_has_partner(active)) {
>>>>
>>>> Why "__agg_has_partner"? Since the "else" of this clause is:
>>>>
>>>> } else if (netif_carrier_ok(bond->dev)) {
>>>> netif_carrier_off(bond->dev);
>>>> }
>>>>
>>>> I'm wondering if this will do the right thing for the case that
>>>> there are no LACP partners at all (e.g., the switch ports do not have
>>>> LACP enabled), in which case the active aggregator should be a single
>>>> "individual" port as a fallback, but will not have a partner.
>>>>
>>>> -J
>>>>
>>>
>>> I see your point. The initial thinking was the logical bond carrier should
>>> not be brought up until the bond has a partner and is ready to pass
>>> traffic, otherwise we start blackholing frames. Looking over the code it
>>> seems the aggregator.is_individual flag is only set to true when a slave
>>> is in half-duplex, this seems odd?
>>
>> The agg.is_individual flag and an "individual" aggregator are
>> subtly different things.
>>
>> The is_individual flag is part of the implementation of the
>> standard requirement that half duplex ports are not allowed to enable
>> LACP (and thus cannot aggregate, and end up as "Individual" in
>> standard-ese). The standard capitalizes "Individual" when it describes
>> the cannot-aggregate property of a port (note that half duplex is only
>> one reason of many for a port being Individual).
>>
>> An "individual" aggregator (my usage of 802.1AX 5.3.6 (b)) is an
>> aggregator containing exactly one port that is Individual. A port can
>> end up as Individual (for purposes of this discussion) either through
>> the is_individual business, or because the bonding port does run LACP,
>> but the link partner does not, and thus no LACPDUs are ever received.
>>
>> For either of the above cases (is_individual or no-LACP-parter),
>> then the active aggregator will be an "individual" aggregator, but will
>> not have a parter (__agg_has_partner() will be false). The standard has
>> a bunch of verbiage about this in 802.1AX 5.3.5 - 5.3.9.
>>
>>> My initial thinking to alleviate the concern is something like the
>>> following:
>>>
>>> if (active && !SLAVE_AD_INFO(slave)->aggregator.is_individual &&
>>> __agg_has_partner(active)) {
>>> /* set carrier based on min_links */
>>> } else if (active && SLAVE_AD_INFO(slave)->aggregator.is_individual) {
>>> /* set bond carrier state according to carrier state of slave */
>>> } else if (netif_carrier_ok(bond->dev)) {
>>> netif_carrier_off(bond->dev);
>>> }
>>
>> I'm not sure you need to care about is_individual or
>> __agg_has_partner at all. If either of those conditions is true for the
>> active aggregator, it will contain exactly one port, and so if min_links
>> is 2, you'll have carrier off, and if min_links is 1 or less you'll have
>> carrier on.
>>
>> If I'm reading the patch right, the real point (which isn't
>> really described very well in the change log) is that you're changing
>> the carrier decision to count only active ports in the active
>> aggregator, not the total number of ports as is currently done.
>>
>> I'm not sure why this change is needed:
>>
>> @@ -2381,10 +2386,15 @@ int bond_3ad_set_carrier(struct bonding *bond)
>> ret = 0;
>> goto out;
>> }
>> +
>> + bond_for_each_slave_rcu(bond, slave, iter)
>> + if (SLAVE_AD_INFO(slave)->aggregator.is_active)
>> + active_slaves++;
>> +
>> active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator));
>> - if (active) {
>> + if (active && __agg_has_partner(active)) {
>> /* are enough slaves available to consider link up? */
>> - if (active->num_of_ports < bond->params.min_links) {
>> + if (active_slaves < bond->params.min_links) {
>> if (netif_carrier_ok(bond->dev)) {
>> netif_carrier_off(bond->dev);
>> goto out;
>>
>> because a port (slave) that loses carrier or whose link partner
>> becomes unresponsive to LACPDUs will be removed from the aggregator. As
>> I recall, there are no "inactive" ports in an aggregator; all of them
>> have to match in terms of capabilities.
>>
>> In other words, I'm unsure of when the count of is_active ports
>> will not match active->num_of_ports.
>>
>> Also, the other parts of the patch add some extra updates to the
>> carrier state when a port is enabled or disabled, e.g.,
>>
>> @@ -189,6 +189,7 @@ static inline int __agg_has_partner(struct aggregator *agg)
>> static inline void __disable_port(struct port *port)
>> {
>> bond_set_slave_inactive_flags(port->slave, BOND_SLAVE_NOTIFY_LATER);
>> + bond_3ad_set_carrier(port->slave->bond);
>> }
>>
>> Again, I'm not sure why this is necessary, as the cases that
>> disable or enable a port will eventually call bond_3ad_set_carrier. For
>> example, ad_agg_selection_logic will, when changing active aggregator,
>> individually disable all ports of the old active and then may
>> individually enable ports of the new active if necessary, and then
>> finally call bond_3ad_set_carrier.
>>
>> In what situations is the patch's behavior an improvement (i.e.,
>> is there a situation I'm missing that doesn't do it right)?
>
>I think the addition of bond_3ad_set_carrier() to both __enable_port() and
>__disable_port() were optimizations so the bond carrier transition would
>happen faster, though I am not certain.
I get the impression that these patches have been around for a
while internally; it would be good to validate that there is an actual
performance change in the current mainline, since there does not seem to
be any functionality change.
>>
>> The last portion of the patch:
>>
>> --- a/drivers/net/bonding/bond_options.c
>> +++ b/drivers/net/bonding/bond_options.c
>> @@ -1181,6 +1181,7 @@ static int bond_option_min_links_set(struct bonding *bond,
>> netdev_info(bond->dev, "Setting min links value to %llu\n",
>> newval->value);
>> bond->params.min_links = newval->value;
>> + bond_set_carrier(bond);
>>
>> return 0;
>> }
>>
>> does seem to fix a legitimate bug, in that when min_links is
>> changed, it does not take effect in real time.
>>
>>> Maybe I am missing something and there is a simpler option.
>>>
>>> Thinking about how to validate this, it seems having a bond with two
>>> slaves and both slaves in half-duplex will force an aggregator that is
>>> individual to be selected.
>>>
>>> Thoughts?
>>
>> That's one way, yes. You'll also get an "individual" aggregator
>> if none of the link partners enable LACP.
>>
>
>It seems it might be better to drop the changes to __enable/disable_port
>and bond_3ad_set_carrier from this patch until more testing can be done
>from me, especially if you agree the other changes in this series are of
>benefit.
I haven't looked at patch 3 in detail yet; patches 2, 4 and 5
appear to be ok.
For this patch, the patch fragment immediately above (min_links
take effect immediately) looks good. I think the rest of it still needs
some evaluation.
-J
---
-Jay Vosburgh, jay.vosburgh@canonical.com
^ 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