* Re: BUG in xennet_make_frags with paged skb data
From: David Vrabel @ 2014-11-10 14:41 UTC (permalink / raw)
To: netdev, David S. Miller, Eric Dumazet, Konrad Rzeszutek Wilk,
Boris Ostrovsky, Stefan Bader, Jay Vosburgh, linux-kernel,
xen-devel
In-Reply-To: <20141110143517.GA74005@ubuntu-hedt>
On 10/11/14 14:35, Seth Forshee wrote:
> On Fri, Nov 07, 2014 at 10:44:15AM +0000, David Vrabel wrote:
>> On 06/11/14 21:49, Seth Forshee wrote:
>>> We've had several reports of hitting the following BUG_ON in
>>> xennet_make_frags with 3.2 and 3.13 kernels (I'm currently awaiting
>>> results of testing with 3.17):
>>>
>>> /* Grant backend access to each skb fragment page. */
>>> for (i = 0; i < frags; i++) {
>>> skb_frag_t *frag = skb_shinfo(skb)->frags + i;
>>> struct page *page = skb_frag_page(frag);
>>>
>>> len = skb_frag_size(frag);
>>> offset = frag->page_offset;
>>>
>>> /* Data must not cross a page boundary. */
>>> BUG_ON(len + offset > PAGE_SIZE<<compound_order(page));
>>>
>>> When this happens the page in question is a "middle" page in a compound
>>> page (i.e. it's a tail page but not the last tail page), and the data is
>>> fully contained within the compound page. The data does however cross
>>> the hardware page boundary, and since compound_order evaluates to 0 for
>>> tail pages the check fails.
>>>
>>> In going over this I've been unable to determine whether the BUG_ON in
>>> xennet_make_frags is incorrect or the paged skb data is wrong. I can't
>>> find that it's documented anywhere, and the networking code itself is a
>>> bit ambiguous when it comes to compound pages. On the one hand
>>> __skb_fill_page_desc specifically handles adding tail pages as paged
>>> data, but on the other hand skb_copy_bits kmaps frag->page.p which could
>>> fail with data that extends into another page.
>>
>> netfront will safely handle this case so you can remove this BUG_ON()
>> (and the one later on). But it would be better to find out were these
>> funny-looking skbs are coming from and (if necessary) fixing the bug there.
>
> There still seems to be disagreement about whether the "funny" skb is
> valid though - you imply it isn't, but Eric says it is. I've been trying
> to track down where these skbs originate, and so far I've determined
> that they come from a socket spliced to a pipe spliced to a socket. It
> looks like the particular page/offset/len tuple originates at least as
> far back as the first socket, as the tuple is simply copied from an skb
> into the pipe and from the pipe into the final skb.
Apologies for the lack of clarity. I meant either: a) fix the producer
if these skbs are invalid; or b) remove the BUG_ON()s. Since Eric says
these are actually valid skbs, please do option (b).
i.e., remove both BUG_ON()s.
David
^ permalink raw reply
* Re: [RFC PATCH net-next] net: Convert LIMIT_NETDEBUG to net_dbg_ratelimited
From: Joe Perches @ 2014-11-10 15:26 UTC (permalink / raw)
To: nicolas.dichtel; +Cc: David Miller, netdev, linux-kernel, Remi Denis-Courmont
In-Reply-To: <5460CB43.4050008@6wind.com>
On Mon, 2014-11-10 at 15:27 +0100, Nicolas Dichtel wrote:
> Le 09/11/2014 20:17, Joe Perches a écrit :
> > Use the more common dynamic_debug capable net_dbg_ratelimited
> > and remove the LIMIT_NETDEBUG macro.
> >
> > This may have some negative impact on messages that were
> > emitted at KERN_INFO that are not not enabled at all unless
> > DEBUG is defined or dynamic_debug is enabled. Even so,
> > these messages are now _not_ emitted by default.
> >
> > This eliminates the use of the net_msg_warn sysctl
> > "/proc/sys/net/core/warnings".
[]
> > diff --git a/include/net/sock.h b/include/net/sock.h
[]
> > @@ -2276,13 +2276,7 @@ bool sk_ns_capable(const struct sock *sk,
> > bool sk_capable(const struct sock *sk, int cap);
> > bool sk_net_capable(const struct sock *sk, int cap);
> >
> > -/*
> > - * Enable debug/info messages
> > - */
> > -extern int net_msg_warn;
> > -#define LIMIT_NETDEBUG(fmt, args...) \
> > - do { if (net_msg_warn && net_ratelimit()) printk(fmt,##args); } while(0)
> > -
> > +extern int net_msg_warn; /* Unused, but still a sysctl */
> Why not removing this variable from this header and from net/core/utils.c?
> Just declaring a static variable in net/core/sysctl_net_core.c should be enough.
> Am I missing something?
No. It's reasonable to remove its EXPORT_SYMBOL use too.
First let's see if there are any objections to the removal.
^ permalink raw reply
* Re: [patch net-next v2 07/10] bridge: call netdev_sw_port_stp_update when bridge port STP status changes
From: Roopa Prabhu @ 2014-11-10 15:59 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Jiri Pirko, netdev, davem, nhorman, andy, tgraf, dborkman,
ogerlitz, jesse, pshelar, azhou, ben, stephen, jeffrey.t.kirsher,
vyasevic, xiyou.wangcong, john.r.fastabend, edumazet, sfeldma,
f.fainelli, linville, jasowang, ebiederm, nicolas.dichtel,
ryazanov.s.a, buytenh, aviadr, nbd, alexei.starovoitov,
Neil.Jerram, ronye, simon.horman, alexander.h.duyck, john.ronciak,
mleitner, shrijeet, gospo, bcrl
In-Reply-To: <5460B989.8030404@mojatatu.com>
On 11/10/14, 5:11 AM, Jamal Hadi Salim wrote:
> On 11/09/14 05:51, Jiri Pirko wrote:
>> From: Scott Feldman <sfeldma@gmail.com>
>>
>> To notify switch driver of change in STP state of bridge port, add new
>> .ndo op and provide swdev wrapper func to call ndo op. Use it in bridge
>> code then.
>>
>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>> include/linux/netdevice.h | 6 ++++++
>> include/net/switchdev.h | 6 ++++++
>> net/bridge/br_netlink.c | 2 ++
>> net/bridge/br_stp.c | 4 ++++
>> net/bridge/br_stp_if.c | 3 +++
>> net/bridge/br_stp_timer.c | 2 ++
>> net/switchdev/switchdev.c | 19 +++++++++++++++++++
>> 7 files changed, 42 insertions(+)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 116a19d..35f21a95 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -1033,6 +1033,10 @@ typedef u16 (*select_queue_fallback_t)(struct
>> net_device *dev,
>> * const unsigned char *addr,
>> * u16 vid);
>> * Called to delete a fdb from switch device port.
>> + *
>> + * int (*ndo_sw_port_stp_update)(struct net_device *dev, u8 state);
>> + * Called to notify switch device port of bridge port STP
>> + * state change.
>
> You are unconditionally calling
> netdev_sw_port_stp_update(p->dev, p->state);
> Again issue is policy. Could you make this work the same
> way the fdb_add e.g user intent of whether i want to turn
> a port in hardware and/or software to disabled/learning/etc
> is reflected?
>
> btw: does _sw_ stand for switch? why not _hw_ ?
> Could we have one ndo for all flags instead of individual ones.
I agree. There is the bridge port state and a bunch of bridge port
flags. A generic ndo will be good.
>
> I know the current user space code uses u8 as a bitflag; but
> maybe we can introduce a new u32 flag bitmask that has all the
> flags set for backward compat? I can count about a total of 10.
^ permalink raw reply
* Re: [patch net-next v2 10/10] rocker: implement L2 bridge offloading
From: Roopa Prabhu @ 2014-11-10 16:12 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Scott Feldman, Jiri Pirko, Netdev, David S. Miller, nhorman,
Andy Gospodarek, Thomas Graf, dborkman, ogerlitz, jesse, pshelar,
azhou, ben, stephen, Kirsher, Jeffrey T, vyasevic, xiyou.wangcong,
Fastabend, John R, edumazet, Florian Fainelli, John Linville,
jasowang, ebiederm, nicolas.dichtel, ryazanov.s.a, buytenh,
aviadr, nbd, alexei.starovoitov, Neil.Jerram, ronye, simon.horman
In-Reply-To: <5460AF22.2040701@mojatatu.com>
On 11/10/14, 4:27 AM, Jamal Hadi Salim wrote:
> On 11/10/14 03:46, Scott Feldman wrote:
>
>>
>> IFLA_BRPORT_LEARNING is u8 attr and we're only using lower bit to turn
>> learning on/off. Maybe we can use another bit to indicate learning to
>> be done in sw or hw. I don't think adding another bit would break
>> existing iproute2.
>>
>> LEARNING_ENABLED (1 << 0)
>> LEARNING_HW (1 << 1)
>>
>> Would this work?
>>
>
> Yes to making it a bit. But:
> This is not *learning*. You are doing a *sync*.
> Those are two different things.
>
> Learning on/off exists today. It signals to the L2 whether you
> should learn or not.
> I like the way fdb_add/del work with a flag which says
> it is the software and/or offloaded version. Please keep that
> semantic.
> What you are doing above is letting the hardware learn then
> syncing to software. You need a different flag there. something
> like:
>
> SYNC_HW_FDB (1<<1)
>
And in any case, It seems like this policy should be per bridge or per
switch chip...or per fdb..
entry (like the original fdb_add/del) and not a "port" flag.. ?
^ permalink raw reply
* Re: [PATCH] brcmfmac: unlink URB when request timed out
From: Mathy Vanhoef @ 2014-11-10 16:08 UTC (permalink / raw)
To: Arend van Spriel, brudley, frankyl, meuleman, linville, pieterpg,
linux-wireless, brcm80211-dev-list, netdev, Oliver Neukum
In-Reply-To: <54609F09.6070807@broadcom.com>
On 11/10/2014 06:18 AM, Arend van Spriel wrote:
> On 09-11-14 19:10, Mathy Vanhoef wrote:
>> From: Mathy Vanhoef <vanhoefm@gmail.com>
>>
>> Unlink the submitted URB in brcmf_usb_dl_cmd if the request timed out. This
>> assures the URB is never submitted twice, preventing a driver crash.
>
> Hi Mathy,
>
> What driver crash are you referring to? The log only shows the WARNING
> ending in a USB disconnect but no actual crash. Does your patch get the
> driver running properly or does it only avoid the warning.
Hi Arend,
It shows a warning, after which the device doesn't work (but the computer is
still usable). But I've noticed that when *unplugging* the USB cable the OS may
freeze. This doesn't always happen though, sometimes unplugging works OK. The
patch both avoids the warning, and gets the device/driver running properly
(unplugging also works OK).
>
> With that said, it seems there is some need for improvement, but I also
> notice you are running this on a virtual machine so could that affect
> the timeout to kick in before completion. Could you try to increase the
> timeout. Still when a timeout occurs this needs to be handled properly.
> Could you also try the following patch?
I did a few additional tests:
1. When increasing IOCTL_RESP_TIMEOUT to 20000 (ten times the normal value) the
timeout and warning still occur. Device/driver doesn't work.
2. When increasing BRCMF_USB_RESET_GETVER_SPINWAIT to 1000 (ten timers the
normal value) everything works. Device/driver works.
3. Quick test using backports-3.18-rc1-1 (aka unpatched driver) on a non-
virtualized Linux install: In that case everything worked fine. So the bug
may only be triggered in a virtualized environment / VMWare.
4. When applying your patch, the driver stops early during initialization of
the device. I included a WARN_ONCE before returning EINVAL and got the
output below.
Kind regards,
Mathy
---
[ 220.955647] usb 1-1: new high-speed USB device number 3 using ehci-pci
[ 221.487797] usb 1-1: New USB device found, idVendor=043e, idProduct=3004
[ 221.487802] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[ 221.487804] usb 1-1: Product: Remote Download Wireless Adapter
[ 221.487806] usb 1-1: Manufacturer: Broadcom
[ 221.487808] usb 1-1: SerialNumber: 000000000001
[ 221.490472] brcmfmac: brcmf_usb_probe Enter 0x043e:0x3004
[ 221.490476] brcmfmac: brcmf_usb_probe Broadcom high speed USB WLAN interface detected
[ 221.490477] brcmfmac: brcmf_usb_probe_cb Enter
[ 221.490480] brcmfmac: brcmf_usb_attach Enter
[ 221.490494] brcmfmac: brcmf_usb_dlneeded Enter
[ 221.490495] ------------[ cut here ]------------
[ 221.490503] WARNING: CPU: 0 PID: 100 at drivers/net/wireless/brcm80211/brcmfmac/usb.c:716 brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac]()
[ 221.490505] EINVAL devinfo=c0044000 ctl_rub=ef898380 completed=0
[ 221.490506] Modules linked in: brcmfmac brcmutil vmw_pvscsi pcnet32 mptspi mptscsih mptbase
[ 221.490514] CPU: 0 PID: 100 Comm: kworker/0:1 Not tainted 3.18.0-rc3-wl+ #2
[ 221.490515] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/31/2013
[ 221.490528] Workqueue: usb_hub_wq hub_event
[ 221.490530] 00000000 00000000 eecffb58 c1711f4a eecffb98 eecffb88 c103edaf f11cbc58
[ 221.490534] eecffbb4 00000064 f11cbc84 000002cc f11c1595 f11c1595 c0044000 ffffffea
[ 221.490537] ef726000 eecffba0 c103ee4e 00000009 eecffb98 f11cbc58 eecffbb4 eecffbd0
[ 221.490541] Call Trace:
[ 221.490550] [<c1711f4a>] dump_stack+0x41/0x52
[ 221.490558] [<c103edaf>] warn_slowpath_common+0x7f/0xa0
[ 221.490563] [<f11c1595>] ? brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac]
[ 221.490567] [<f11c1595>] ? brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac]
[ 221.490570] [<c103ee4e>] warn_slowpath_fmt+0x2e/0x30
[ 221.490575] [<f11c1595>] brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac]
[ 221.490580] [<f11c2cd8>] brcmf_usb_probe+0x3c8/0x640 [brcmfmac]
[ 221.490583] [<c1717d53>] ? mutex_lock+0x13/0x32
[ 221.490586] [<c1493ae3>] usb_probe_interface+0xa3/0x180
[ 221.490590] [<c13f5690>] ? __driver_attach+0x90/0x90
[ 221.490592] [<c13f546e>] driver_probe_device+0x5e/0x1f0
[ 221.490595] [<c13f5690>] ? __driver_attach+0x90/0x90
[ 221.490597] [<c13f56c9>] __device_attach+0x39/0x50
[ 221.490600] [<c13f3d84>] bus_for_each_drv+0x34/0x70
[ 221.490602] [<c13f53db>] device_attach+0x7b/0x90
[ 221.490604] [<c13f5690>] ? __driver_attach+0x90/0x90
[ 221.490607] [<c13f4b8f>] bus_probe_device+0x6f/0x90
[ 221.490609] [<c13f3256>] device_add+0x426/0x520
[ 221.490611] [<c1491503>] ? usb_control_msg+0xb3/0xd0
[ 221.490614] [<c1717d53>] ? mutex_lock+0x13/0x32
[ 221.490627] [<c14922f8>] usb_set_configuration+0x3f8/0x700
[ 221.490630] [<c13f5690>] ? __driver_attach+0x90/0x90
[ 221.490633] [<c149ac7b>] generic_probe+0x2b/0x90
[ 221.490637] [<c1188bc0>] ? sysfs_create_link+0x20/0x40
[ 221.490639] [<c1492bec>] usb_probe_device+0xc/0x10
[ 221.490641] [<c13f546e>] driver_probe_device+0x5e/0x1f0
[ 221.490644] [<c13f5690>] ? __driver_attach+0x90/0x90
[ 221.490646] [<c13f56c9>] __device_attach+0x39/0x50
[ 221.490649] [<c13f3d84>] bus_for_each_drv+0x34/0x70
[ 221.490651] [<c13f53db>] device_attach+0x7b/0x90
[ 221.490653] [<c13f5690>] ? __driver_attach+0x90/0x90
[ 221.490656] [<c13f4b8f>] bus_probe_device+0x6f/0x90
[ 221.490658] [<c13f3256>] device_add+0x426/0x520
[ 221.490661] [<c148aa2e>] ? usb_new_device+0x16e/0x3a0
[ 221.490663] [<c148aad7>] usb_new_device+0x217/0x3a0
[ 221.490666] [<c148bff7>] hub_event+0xa17/0xda0
[ 221.490668] [<c1716918>] ? __schedule+0x2f8/0x710
[ 221.490672] [<c105127c>] ? pwq_dec_nr_in_flight+0x3c/0x90
[ 221.490674] [<c10513ee>] process_one_work+0x11e/0x360
[ 221.490677] [<c1051750>] worker_thread+0xf0/0x3c0
[ 221.490680] [<c106e14a>] ? __wake_up_locked+0x1a/0x20
[ 221.490682] [<c1051660>] ? process_scheduled_works+0x30/0x30
[ 221.490685] [<c1055b56>] kthread+0x96/0xb0
[ 221.490687] [<c1050000>] ? put_unbound_pool+0x110/0x170
[ 221.490691] [<c1719c81>] ret_from_kernel_thread+0x21/0x30
[ 221.490693] [<c1055ac0>] ? kthread_worker_fn+0x110/0x110
[ 221.490695] ---[ end trace 9befd914693f3083 ]---
[ 221.490697] brcmfmac: brcmf_usb_dlneeded chip 57005 rev 0xf11cfcec
[ 221.490699] brcmfmac: brcmf_fw_get_firmwares enter: dev=1-1
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
index 5265aa7..15b1aa7 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
@@ -709,8 +709,13 @@ static int brcmf_usb_dl_cmd(struct brcmf_usbdev_info *devinfo, u8 cmd,
char *tmpbuf;
u16 size;
- if ((!devinfo) || (devinfo->ctl_urb == NULL))
+ if (!devinfo || !devinfo->ctl_urb || !devinfo->ctl_completed) {
+ WARN_ONCE(1, "EINVAL devinfo=%p ctl_rub=%p completed=%d\n",
+ devinfo,
+ devinfo ? devinfo->ctl_urb : NULL,
+ devinfo ? devinfo->ctl_completed : -1);
return -EINVAL;
+ }
tmpbuf = kmalloc(buflen, GFP_ATOMIC);
if (!tmpbuf)
>
> Regards,
> Arend
> ---
> drivers/net/wireless/brcm80211/brcmfmac/usb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
> b/drivers/net/wireles
> index dc13591..786c40b 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
> @@ -640,7 +640,7 @@ static int brcmf_usb_dl_cmd(struct brcmf_usbdev_info
> *devinf
> char *tmpbuf;
> u16 size;
>
> - if ((!devinfo) || (devinfo->ctl_urb == NULL))
> + if (!devinfo || !devinfo->ctl_urb || !devinfo->ctl_completed)
> return -EINVAL;
>
> tmpbuf = kmalloc(buflen, GFP_ATOMIC);
>
>> Signed-off-by: Mathy Vanhoef <vanhoefm@gmail.com>
>> ---
>> Currently brcmfmac may crash when a USB device is attached (tested with a LG
>> TWFM-B003D). In particular it fails on the second call to brcmf_usb_dl_cmd in
>> the while loop of brcmf_usb_resetcfg. The problem is that an URB is being
>> submitted twice:
>>
>> [ 169.861800] brcmfmac: brcmf_usb_dl_writeimage Enter, fw f14db000, len 348160
>> [ 171.787791] brcmfmac: brcmf_usb_dl_writeimage Exit, err=0
>> [ 171.787797] brcmfmac: brcmf_usb_dlstart Exit, err=0
>> [ 171.787799] brcmfmac: brcmf_usb_dlrun Enter
>> [ 171.791794] brcmfmac: brcmf_usb_resetcfg Enter
>> [ 173.988072] ------------[ cut here ]------------
>> [ 173.988083] WARNING: CPU: 0 PID: 369 at drivers/usb/core/urb.c:339 usb_submit_urb+0x4e6/0x500()
>> [ 173.988085] URB eaf45f00 submitted while active
>> [ 173.988086] Modules linked in: brcmfmac brcmutil vmw_pvscsi pcnet32 mptspi mptscsih mptbase
>> [ 173.988100] CPU: 0 PID: 369 Comm: kworker/0:2 Not tainted 3.18.0-rc3-wl #1
>> [ 173.988102] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/31/2013
>> [ 173.988106] Workqueue: events request_firmware_work_func
>> [ 173.988108] 00000000 00000000 ee747db8 c1711f4a ee747df8 ee747de8 c103edaf c18d1e10
>> [ 173.988112] ee747e14 00000171 c18a8b29 00000153 c1490556 c1490556 eaf45f00 eafdc660
>> [ 173.988115] f14b8fa0 ee747e00 c103ee4e 00000009 ee747df8 c18d1e10 ee747e14 ee747e50
>> [ 173.988119] Call Trace:
>> [ 173.988129] [<c1711f4a>] dump_stack+0x41/0x52
>> [ 173.988136] [<c103edaf>] warn_slowpath_common+0x7f/0xa0
>> [ 173.988139] [<c1490556>] ? usb_submit_urb+0x4e6/0x500
>> [ 173.988141] [<c1490556>] ? usb_submit_urb+0x4e6/0x500
>> [ 173.988147] [<f14b8fa0>] ? brcmf_usb_ioctl_resp_wake+0x40/0x40 [brcmfmac]
>> [ 173.988150] [<c103ee4e>] warn_slowpath_fmt+0x2e/0x30
>> [ 173.988152] [<c1490556>] usb_submit_urb+0x4e6/0x500
>> [ 173.988156] [<c1123de1>] ? __kmalloc+0x21/0x140
>> [ 173.988161] [<f14b91c3>] ? brcmf_usb_dl_cmd+0x33/0x120 [brcmfmac]
>> [ 173.988166] [<f14b9243>] brcmf_usb_dl_cmd+0xb3/0x120 [brcmfmac]
>> [ 173.988170] [<f14ba6c4>] brcmf_usb_probe_phase2+0x4e4/0x640 [brcmfmac]
>> [ 173.988176] [<f14b4900>] brcmf_fw_request_code_done+0xd0/0xf0 [brcmfmac]
>> [ 173.988178] [<c1400876>] request_firmware_work_func+0x26/0x50
>> [ 173.988182] [<c10513ee>] process_one_work+0x11e/0x360
>> [ 173.988184] [<c1051750>] worker_thread+0xf0/0x3c0
>> [ 173.988205] [<c106e14a>] ? __wake_up_locked+0x1a/0x20
>> [ 173.988208] [<c1051660>] ? process_scheduled_works+0x30/0x30
>> [ 173.988211] [<c1055b56>] kthread+0x96/0xb0
>> [ 173.988214] [<c1719c81>] ret_from_kernel_thread+0x21/0x30
>> [ 173.988217] [<c1055ac0>] ? kthread_worker_fn+0x110/0x110
>> [ 173.988219] ---[ end trace 0c88bf46801de083 ]---
>> [ 173.988221] brcmf_usb_dl_cmd: usb_submit_urb failed -16
>> [ 173.988396] brcmfmac: brcmf_usb_probe_phase2 failed: dev=1-1, err=-19
>> [ 173.989503] brcmfmac: brcmf_usb_disconnect Enter
>>
>> This patch fixes the brcmf_usb_dl_cmd function to prevent an URB from being
>> submitted twice. Tested using a LG TWFM-B003D, which now works properly.
>>
>>
>> drivers/net/wireless/brcm80211/brcmfmac/usb.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
>> index 5265aa7..1bc7858 100644
>> --- a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
>> @@ -738,10 +738,12 @@ static int brcmf_usb_dl_cmd(struct brcmf_usbdev_info *devinfo, u8 cmd,
>> goto finalize;
>> }
>>
>> - if (!brcmf_usb_ioctl_resp_wait(devinfo))
>> + if (!brcmf_usb_ioctl_resp_wait(devinfo)) {
>> + usb_unlink_urb(devinfo->ctl_urb);
>> ret = -ETIMEDOUT;
>> - else
>> + } else {
>> memcpy(buffer, tmpbuf, buflen);
>> + }
>>
>> finalize:
>> kfree(tmpbuf);
>>
>
^ permalink raw reply related
* Re: [PATCH 1/4] inet: Add skb_copy_datagram_iter
From: David Miller @ 2014-11-10 16:18 UTC (permalink / raw)
To: viro; +Cc: herbert, netdev, linux-kernel, bcrl, mst
In-Reply-To: <20141110090955.GH7996@ZenIV.linux.org.uk>
From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Mon, 10 Nov 2014 09:09:55 +0000
> BTW, what's the usual regression suite used for net/* stuff?
There's a regression suite? :-)
^ permalink raw reply
* Re: [patch net-next v2 01/10] net: rename netdev_phys_port_id to more generic name
From: David Miller @ 2014-11-10 16:28 UTC (permalink / raw)
To: jhs
Cc: jiri, netdev, nhorman, andy, tgraf, dborkman, ogerlitz, jesse,
pshelar, azhou, ben, stephen, jeffrey.t.kirsher, vyasevic,
xiyou.wangcong, john.r.fastabend, edumazet, sfeldma, f.fainelli,
roopa, linville, jasowang, ebiederm, nicolas.dichtel,
ryazanov.s.a, buytenh, aviadr, nbd, alexei.starovoitov,
Neil.Jerram, ronye, simon.horman, alexander.h.duyck, john.ronciak,
mleitner, shrijeet, gospo, bcrl
In-Reply-To: <5460AA45.6000007@mojatatu.com>
From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Mon, 10 Nov 2014 07:06:29 -0500
> It is a _user space visible rename_, how about:
>
> #define MAX_PHYS_ITEM_ID_LEN 32
> #define MAX_PHYS_PORT_ID_LEN MAX_PHYS_ITEM_ID_LEN
>
> I did miss the fact that the size didnt change.
The user cannot see this macro Jamal, please really read this
code, instead of, once again, jumping to conclusions.
^ permalink raw reply
* Re: [patch net-next v2 01/10] net: rename netdev_phys_port_id to more generic name
From: David Miller @ 2014-11-10 16:28 UTC (permalink / raw)
To: jhs
Cc: jiri, netdev, nhorman, andy, tgraf, dborkman, ogerlitz, jesse,
pshelar, azhou, ben, stephen, jeffrey.t.kirsher, vyasevic,
xiyou.wangcong, john.r.fastabend, edumazet, sfeldma, f.fainelli,
roopa, linville, jasowang, ebiederm, nicolas.dichtel,
ryazanov.s.a, buytenh, aviadr, nbd, alexei.starovoitov,
Neil.Jerram, ronye, simon.horman, alexander.h.duyck, john.ronciak,
mleitner, shrijeet, gospo, bcrl
In-Reply-To: <5460ACC0.3020805@mojatatu.com>
From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Mon, 10 Nov 2014 07:17:04 -0500
> On 11/10/14 02:43, Jiri Pirko wrote:
>> Mon, Nov 10, 2014 at 04:35:12AM CET, jhs@mojatatu.com wrote:
>
>> I don't see a reason why this would break kabi:
>>
>> -#define MAX_PHYS_PORT_ID_LEN 32
>> +#define MAX_PHYS_ITEM_ID_LEN 32
>>
>
> refer to my response to Dave. Just define MAX_PHYS_PORT_ID_LEN to
> MAX_PHYS_ITEM_ID_LEN so people dont have to change their code
> because a name change.
Again, nobody has to change anything.
This macro is not visible outside of the kernel.
Jamal, this is really driving me crazy, this is a non-issue.
^ permalink raw reply
* Re: [Xen-devel] BUG in xennet_make_frags with paged skb data
From: Zoltan Kiss @ 2014-11-10 16:39 UTC (permalink / raw)
To: David Vrabel, netdev, David S. Miller, Eric Dumazet,
Konrad Rzeszutek Wilk, Boris Ostrovsky, Stefan Bader,
Jay Vosburgh, linux-kernel, xen-devel
In-Reply-To: <5460CEA5.3070201@citrix.com>
On 10/11/14 14:41, David Vrabel wrote:
> On 10/11/14 14:35, Seth Forshee wrote:
>> On Fri, Nov 07, 2014 at 10:44:15AM +0000, David Vrabel wrote:
>>> On 06/11/14 21:49, Seth Forshee wrote:
>>>> We've had several reports of hitting the following BUG_ON in
>>>> xennet_make_frags with 3.2 and 3.13 kernels (I'm currently awaiting
>>>> results of testing with 3.17):
>>>>
>>>> /* Grant backend access to each skb fragment page. */
>>>> for (i = 0; i < frags; i++) {
>>>> skb_frag_t *frag = skb_shinfo(skb)->frags + i;
>>>> struct page *page = skb_frag_page(frag);
>>>>
>>>> len = skb_frag_size(frag);
>>>> offset = frag->page_offset;
>>>>
>>>> /* Data must not cross a page boundary. */
>>>> BUG_ON(len + offset > PAGE_SIZE<<compound_order(page));
>>>>
>>>> When this happens the page in question is a "middle" page in a compound
>>>> page (i.e. it's a tail page but not the last tail page), and the data is
>>>> fully contained within the compound page. The data does however cross
>>>> the hardware page boundary, and since compound_order evaluates to 0 for
>>>> tail pages the check fails.
>>>>
>>>> In going over this I've been unable to determine whether the BUG_ON in
>>>> xennet_make_frags is incorrect or the paged skb data is wrong. I can't
>>>> find that it's documented anywhere, and the networking code itself is a
>>>> bit ambiguous when it comes to compound pages. On the one hand
>>>> __skb_fill_page_desc specifically handles adding tail pages as paged
>>>> data, but on the other hand skb_copy_bits kmaps frag->page.p which could
>>>> fail with data that extends into another page.
>>>
>>> netfront will safely handle this case so you can remove this BUG_ON()
>>> (and the one later on). But it would be better to find out were these
>>> funny-looking skbs are coming from and (if necessary) fixing the bug there.
>>
>> There still seems to be disagreement about whether the "funny" skb is
>> valid though - you imply it isn't, but Eric says it is. I've been trying
>> to track down where these skbs originate, and so far I've determined
>> that they come from a socket spliced to a pipe spliced to a socket. It
>> looks like the particular page/offset/len tuple originates at least as
>> far back as the first socket, as the tuple is simply copied from an skb
>> into the pipe and from the pipe into the final skb.
>
> Apologies for the lack of clarity. I meant either: a) fix the producer
> if these skbs are invalid; or b) remove the BUG_ON()s. Since Eric says
> these are actually valid skbs, please do option (b).
>
> i.e., remove both BUG_ON()s.
The BUG_ON suggested by Stefan would be still reasonable:
BUG_ON(((page-compound_head(page))*PAGE_SIZE)+offset+len >
PAGE_SIZE<<compound_order(compound_head(page)));
>
> David
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
^ permalink raw reply
* Re: [Xen-devel] BUG in xennet_make_frags with paged skb data
From: David Vrabel @ 2014-11-10 16:42 UTC (permalink / raw)
To: Zoltan Kiss, netdev, David S. Miller, Eric Dumazet,
Konrad Rzeszutek Wilk, Boris Ostrovsky, Stefan Bader,
Jay Vosburgh, linux-kernel, xen-devel
In-Reply-To: <5460EA45.2080202@linaro.org>
On 10/11/14 16:39, Zoltan Kiss wrote:
>
> The BUG_ON suggested by Stefan would be still reasonable:
>
> BUG_ON(((page-compound_head(page))*PAGE_SIZE)+offset+len >
> PAGE_SIZE<<compound_order(compound_head(page)));
Well, it wouldn't trigger but I don't think it is useful any more.
David
^ permalink raw reply
* Re: [patch net-next v2 00/10] introduce rocker switch driver with hardware accelerated datapath api - phase 1: bridge fdb offload
From: Thomas Graf @ 2014-11-10 16:48 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, nhorman, andy, dborkman, ogerlitz, jesse, pshelar,
azhou, ben, stephen, jeffrey.t.kirsher, vyasevic, xiyou.wangcong,
john.r.fastabend, edumazet, jhs, sfeldma, f.fainelli, roopa,
linville, jasowang, ebiederm, nicolas.dichtel, ryazanov.s.a,
buytenh, aviadr, nbd, alexei.starovoitov, Neil.Jerram, ronye,
simon.horman, alexander.h.duyck, john.ronciak, mleitner, shrijeet,
gospo, bcrl
In-Reply-To: <1415530280-9190-1-git-send-email-jiri@resnulli.us>
On 11/09/14 at 11:51am, Jiri Pirko wrote:
> Hi all.
>
> This patchset is just the first phase of switch and switch-ish device
> support api in kernel. Note that the api will extend (our complete work
> can be pulled from https://github.com/jpirko/net-next-rocker).
Despite my comment on ndo_fdb_add() which I believe can be reconsidered
later. I like this pach series a lot. Thanks for putting in so much work
on this topic.
Reviewed-by: Thomas Graf <tgraf@suug.ch>
^ permalink raw reply
* [PATCH net] net: sctp: fix NULL pointer dereference in af->from_addr_param on malformed packet
From: Daniel Borkmann @ 2014-11-10 16:54 UTC (permalink / raw)
To: davem; +Cc: linux-sctp, netdev, Vlad Yasevich
An SCTP server doing ASCONF will panic on malformed INIT ping-of-death
in the form of:
------------ INIT[PARAM: SET_PRIMARY_IP] ------------>
While the INIT chunk parameter verification dissects through many things
in order to detect malformed input, it misses to actually check parameters
inside of parameters. E.g. RFC5061, section 4.2.4 proposes a 'set primary
IP address' parameter in ASCONF, which has as a subparameter an address
parameter.
So an attacker may send a parameter type other than SCTP_PARAM_IPV4_ADDRESS
or SCTP_PARAM_IPV6_ADDRESS, param_type2af() will subsequently return 0
and thus sctp_get_af_specific() returns NULL, too, which we then happily
dereference unconditionally through af->from_addr_param().
The trace for the log:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000078
IP: [<ffffffffa01e9c62>] sctp_process_init+0x492/0x990 [sctp]
PGD 0
Oops: 0000 [#1] SMP
[...]
Pid: 0, comm: swapper Not tainted 2.6.32-504.el6.x86_64 #1 Bochs Bochs
RIP: 0010:[<ffffffffa01e9c62>] [<ffffffffa01e9c62>] sctp_process_init+0x492/0x990 [sctp]
[...]
Call Trace:
<IRQ>
[<ffffffffa01f2add>] ? sctp_bind_addr_copy+0x5d/0xe0 [sctp]
[<ffffffffa01e1fcb>] sctp_sf_do_5_1B_init+0x21b/0x340 [sctp]
[<ffffffffa01e3751>] sctp_do_sm+0x71/0x1210 [sctp]
[<ffffffffa01e5c09>] ? sctp_endpoint_lookup_assoc+0xc9/0xf0 [sctp]
[<ffffffffa01e61f6>] sctp_endpoint_bh_rcv+0x116/0x230 [sctp]
[<ffffffffa01ee986>] sctp_inq_push+0x56/0x80 [sctp]
[<ffffffffa01fcc42>] sctp_rcv+0x982/0xa10 [sctp]
[<ffffffffa01d5123>] ? ipt_local_in_hook+0x23/0x28 [iptable_filter]
[<ffffffff8148bdc9>] ? nf_iterate+0x69/0xb0
[<ffffffff81496d10>] ? ip_local_deliver_finish+0x0/0x2d0
[<ffffffff8148bf86>] ? nf_hook_slow+0x76/0x120
[<ffffffff81496d10>] ? ip_local_deliver_finish+0x0/0x2d0
[...]
A minimal way to address this is to check for NULL as we do on all
other such occasions where we know sctp_get_af_specific() could
possibly return with NULL.
Fixes: d6de3097592b ("[SCTP]: Add the handling of "Set Primary IP Address" parameter to INIT")
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Cc: Vlad Yasevich <vyasevich@gmail.com>
---
net/sctp/sm_make_chunk.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index ab734be..9f32741 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -2609,6 +2609,9 @@ do_addr_param:
addr_param = param.v + sizeof(sctp_addip_param_t);
af = sctp_get_af_specific(param_type2af(param.p->type));
+ if (af == NULL)
+ break;
+
af->from_addr_param(&addr, addr_param,
htons(asoc->peer.port), 0);
--
1.7.11.7
^ permalink raw reply related
* Re: [PATCH nf] netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse
From: Pablo Neira Ayuso @ 2014-11-10 16:54 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: programme110, netfilter-devel, Florian Westphal, netdev
In-Reply-To: <20141106133648.2534.1403.stgit@dragon>
On Thu, Nov 06, 2014 at 02:36:48PM +0100, Jesper Dangaard Brouer wrote:
> From: bill bonaparte <programme110@gmail.com>
>
> After removal of the central spinlock nf_conntrack_lock, in
> commit 93bb0ceb75be2 ("netfilter: conntrack: remove central
> spinlock nf_conntrack_lock"), it is possible to race against
> get_next_corpse().
>
> The race is against the get_next_corpse() cleanup on
> the "unconfirmed" list (a per-cpu list with seperate locking),
> which set the DYING bit.
>
> Fix this race, in __nf_conntrack_confirm(), by removing the CT
> from unconfirmed list before checking the DYING bit. In case
> race occured, re-add the CT to the dying list.
This seems correct to me, some side comments.
> Fixes: 93bb0ceb75be2 ("netfilter: conntrack: remove central spinlock nf_conntrack_lock")
> Reported-by: bill bonaparte <programme110@gmail.com>
> Signed-off-by: bill bonaparte <programme110@gmail.com>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
>
> net/netfilter/nf_conntrack_core.c | 7 ++++---
> 1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index 5016a69..1072650 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -611,12 +611,15 @@ __nf_conntrack_confirm(struct sk_buff *skb)
> */
> NF_CT_ASSERT(!nf_ct_is_confirmed(ct));
> pr_debug("Confirming conntrack %p\n", ct);
> - /* We have to check the DYING flag inside the lock to prevent
> +
> + /* We have to check the DYING flag after unlink to prevent
> a race against nf_ct_get_next_corpse() possibly called from
> user context, else we insert an already 'dead' hash, blocking
> further use of that particular connection -JM */
While at this, I think it would be good to fix comment style to:
/* We have ...
* ...
*/
I can fix this here, no need to resend, just let me know.
> + nf_ct_del_from_dying_or_unconfirmed_list(ct);
>
> if (unlikely(nf_ct_is_dying(ct))) {
> + nf_ct_add_to_dying_list(ct);
> nf_conntrack_double_unlock(hash, reply_hash);
> local_bh_enable();
> return NF_ACCEPT;
Not directly related to your patch, but I don't find a good reason why
we're accepting this packet.
If the conntrack from the unconfirmed list is dying, then the object
will be released by when the packet leaves the stack to its
destination. With stateful filtering depending in place, the follow up
packet in the reply direction will likely be considered invalid (if
tcp tracking is on). Fortunately for us, the origin will likely
retransmit the syn again, so the ct will be setup accordingly.
So, why should we allow this to go through?
This return verdict was introduced in: fc35077 ("netfilter:
nf_conntrack: fix a race in __nf_conntrack_confirm against
nf_ct_get_next_corpse()") btw.
^ permalink raw reply
* [PATCH net] net: sctp: fix memory leak in auth key management
From: Daniel Borkmann @ 2014-11-10 17:00 UTC (permalink / raw)
To: davem; +Cc: linux-sctp, netdev, Vlad Yasevich
A very minimal and simple user space application allocating an SCTP
socket, setting SCTP_AUTH_KEY setsockopt(2) on it and then closing
the socket again will leak the memory containing the authentication
key from user space:
unreferenced object 0xffff8800837047c0 (size 16):
comm "a.out", pid 2789, jiffies 4296954322 (age 192.258s)
hex dump (first 16 bytes):
01 00 00 00 04 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffff816d7e8e>] kmemleak_alloc+0x4e/0xb0
[<ffffffff811c88d8>] __kmalloc+0xe8/0x270
[<ffffffffa0870c23>] sctp_auth_create_key+0x23/0x50 [sctp]
[<ffffffffa08718b1>] sctp_auth_set_key+0xa1/0x140 [sctp]
[<ffffffffa086b383>] sctp_setsockopt+0xd03/0x1180 [sctp]
[<ffffffff815bfd94>] sock_common_setsockopt+0x14/0x20
[<ffffffff815beb61>] SyS_setsockopt+0x71/0xd0
[<ffffffff816e58a9>] system_call_fastpath+0x12/0x17
[<ffffffffffffffff>] 0xffffffffffffffff
This is bad because of two things, we can bring down a machine from
user space when auth_enable=1, but also we would leave security sensitive
keying material in memory without clearing it after use. The issue is
that sctp_auth_create_key() already sets the refcount to 1, but after
allocation sctp_auth_set_key() does an additional refcount on it, and
thus leaving it around when we free the socket.
Fixes: 65b07e5d0d0 ("[SCTP]: API updates to suport SCTP-AUTH extensions.")
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Cc: Vlad Yasevich <vyasevich@gmail.com>
---
net/sctp/auth.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/net/sctp/auth.c b/net/sctp/auth.c
index 0e85291..fb7976a 100644
--- a/net/sctp/auth.c
+++ b/net/sctp/auth.c
@@ -862,8 +862,6 @@ int sctp_auth_set_key(struct sctp_endpoint *ep,
list_add(&cur_key->key_list, sh_keys);
cur_key->key = key;
- sctp_auth_key_hold(key);
-
return 0;
nomem:
if (!replace)
--
1.7.11.7
^ permalink raw reply related
* Re: [Xen-devel] BUG in xennet_make_frags with paged skb data
From: Eric Dumazet @ 2014-11-10 17:02 UTC (permalink / raw)
To: David Vrabel
Cc: Zoltan Kiss, netdev, David S. Miller, Konrad Rzeszutek Wilk,
Boris Ostrovsky, Stefan Bader, Jay Vosburgh, linux-kernel,
xen-devel
In-Reply-To: <5460EAF1.5020903@citrix.com>
On Mon, 2014-11-10 at 16:42 +0000, David Vrabel wrote:
> On 10/11/14 16:39, Zoltan Kiss wrote:
> >
> > The BUG_ON suggested by Stefan would be still reasonable:
> >
> > BUG_ON(((page-compound_head(page))*PAGE_SIZE)+offset+len >
> > PAGE_SIZE<<compound_order(compound_head(page)));
>
> Well, it wouldn't trigger but I don't think it is useful any more.
Right.
This BUG_ON() does not make sense (its current implementation is
assuming a very precise layout anyway)
If we really wanted some debugging, we would need something more generic
and done in core networking stack, not in a particular driver.
^ permalink raw reply
* Re: BUG in xennet_make_frags with paged skb data
From: Frediano Ziglio @ 2014-11-10 17:20 UTC (permalink / raw)
To: David Vrabel
Cc: Zoltan Kiss, Eric Dumazet, netdev, linux-kernel, Stefan Bader,
xen-devel, Jay Vosburgh, Boris Ostrovsky, David S. Miller
In-Reply-To: <5460EAF1.5020903@citrix.com>
[-- Attachment #1.1: Type: text/plain, Size: 860 bytes --]
2014-11-10 16:42 GMT+00:00 David Vrabel <david.vrabel@citrix.com>:
> On 10/11/14 16:39, Zoltan Kiss wrote:
> >
> > The BUG_ON suggested by Stefan would be still reasonable:
> >
> > BUG_ON(((page-compound_head(page))*PAGE_SIZE)+offset+len >
> > PAGE_SIZE<<compound_order(compound_head(page)));
>
> Well, it wouldn't trigger but I don't think it is useful any more.
>
> David
>
>
Looks like this structure was designed to just contains physical single
pages and was extended to support compound pages however there are still
some functions that assume the usage of single pages. Just for instance the
offset/size of fragments are computed from PAGE_SIZE. On x86 (4KB pages) if
your compound page is bigger than 64KB you cannot fully use for a fragment
as offset/size are 16 bits. Some functions in skbuff.c use kmap which is
limited to physical page.
Frediano
[-- Attachment #1.2: Type: text/html, Size: 1336 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply
* Re: [patch net-next v2 06/10] bridge: introduce fdb offloading via switchdev
From: Andy Gospodarek @ 2014-11-10 17:30 UTC (permalink / raw)
To: Thomas Graf
Cc: Jiri Pirko, Jamal Hadi Salim, netdev, davem, nhorman, andy,
dborkman, ogerlitz, jesse, pshelar, azhou, ben, stephen,
jeffrey.t.kirsher, vyasevic, xiyou.wangcong, john.r.fastabend,
edumazet, sfeldma, f.fainelli, roopa, linville, jasowang,
ebiederm, nicolas.dichtel, ryazanov.s.a, buytenh, aviadr, nbd,
alexei.starovoitov, Neil.Jerram, ronye, simon.horman,
alexander.h.duyck, john.ronciak, mleitner, shrijeet, b
In-Reply-To: <20141110135100.GA19157@casper.infradead.org>
On Mon, Nov 10, 2014 at 01:51:00PM +0000, Thomas Graf wrote:
> On 11/10/14 at 09:15am, Jiri Pirko wrote:
> > There are few problems in re-using this. It is netlink based so for calling
> > it from bridge code, we would have to construct netlink message. But
> > that could be probably changed.
> > As you can see from the list of parameters, this is no longer about fdb (addr,
> > vlanid) but this has been extended to something else. See vxlan code for
> > what this is used for. I believe that fdb_add/del should be renamed to
> > something else, perhaps l2neigh_add/del or something like that.
> > The other problem is that fdb_add/del is currently used by various
> > drivers for different purpose (adding macs to unicast list).
>
> Can you elaborate a bit on the intended semantic differences between
> the existing ndo_fdb_add() and ndo_sw_port_fdb_add()? I'm not sure we
> need the sw_ prefix for this specific ndo.
>
> I completely agree that relying on Netlink is wrong because we'll have
> in-kernel users of the API but I believe that existing ndo_fdb_add()
> implementations in i40e, ixgbe, qlcnic and macvlan could use the new
> API you propose.
I also think the same API could be used quite easily on the current
drivers that use it.
I was looking at this earlier today and there are only 5 drivers
(outside the bridge code) that support ndo_fdb_add. The 3 hardware
drivers and vxlan driver seem like they could use this new API. The
macvlan code appears to simply set the uc and mc lists, which seems like
it could be done other ways -- confirmation from John Fastabend, Roopa,
and mst would be good.
> How about we rename the existing ndo_fdb_add() to ndo_neigh_add() as
> you propose and convert vxlan over to it and have all others which don't
> even depend on the Netlink attributes being passed in (i40e, ixgbe,
> qlcnic, macvlan) use ndo_fdb_add() which would have the behaviour of your
> proposed ndo_sw_port_fdb_add()?
I would much rather see something like Thomas proposes here. I know you
would like to see these patches get included (I'm anxious to see better
in-kernel offload support too!), but separate, possibly unnecessary
APIs like this can get painful for driver maintainers (upstream and
distro maintainers).
^ permalink raw reply
* Re: [patch net-next v2 10/10] rocker: implement L2 bridge offloading
From: Scott Feldman @ 2014-11-10 17:36 UTC (permalink / raw)
To: Roopa Prabhu
Cc: Jamal Hadi Salim, Jiri Pirko, Netdev, David S. Miller, nhorman,
Andy Gospodarek, Thomas Graf, dborkman, ogerlitz, jesse, pshelar,
azhou, ben, stephen, Kirsher, Jeffrey T, vyasevic, Cong Wang,
Fastabend, John R, Eric Dumazet, Florian Fainelli, John Linville,
jasowang, ebiederm, nicolas.dichtel, ryazanov.s.a, buytenh,
aviadr, nbd, Alexei Starovoitov, Neil
In-Reply-To: <5460E3D5.3000104@cumulusnetworks.com>
On Mon, Nov 10, 2014 at 6:12 AM, Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
> On 11/10/14, 4:27 AM, Jamal Hadi Salim wrote:
>>
>> On 11/10/14 03:46, Scott Feldman wrote:
>>
>>>
>>> IFLA_BRPORT_LEARNING is u8 attr and we're only using lower bit to turn
>>> learning on/off. Maybe we can use another bit to indicate learning to
>>> be done in sw or hw. I don't think adding another bit would break
>>> existing iproute2.
>>>
>>> LEARNING_ENABLED (1 << 0)
>>> LEARNING_HW (1 << 1)
>>>
>>> Would this work?
>>>
>>
>> Yes to making it a bit. But:
>> This is not *learning*. You are doing a *sync*.
>> Those are two different things.
>>
>> Learning on/off exists today. It signals to the L2 whether you
>> should learn or not.
>> I like the way fdb_add/del work with a flag which says
>> it is the software and/or offloaded version. Please keep that
>> semantic.
>> What you are doing above is letting the hardware learn then
>> syncing to software. You need a different flag there. something
>> like:
>>
>> SYNC_HW_FDB (1<<1)
>>
> And in any case, It seems like this policy should be per bridge or per
> switch chip...or per fdb..
> entry (like the original fdb_add/del) and not a "port" flag.. ?
Per-port gives more flexibility, and it looks like we can extend
existing IFLA_BRPORT_LEARNING without much trouble.
I didn't follow the fdb_add/del comment? Isn't an fdb entry
port-specific by nature? fdb entry = {port, mac, vlan} tuple.
^ permalink raw reply
* Re: [patch net-next v2 10/10] rocker: implement L2 bridge offloading
From: Scott Feldman @ 2014-11-10 17:22 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Jiri Pirko, Netdev, David S. Miller, nhorman, Andy Gospodarek,
Thomas Graf, dborkman, ogerlitz, jesse, pshelar, azhou, ben,
stephen, Kirsher, Jeffrey T, vyasevic, Cong Wang,
Fastabend, John R, Eric Dumazet, Florian Fainelli, Roopa Prabhu,
John Linville, jasowang, ebiederm, nicolas.dichtel, ryazanov.s.a,
buytenh, aviadr, nbd, Alexei Starovoitov, Neil.Je
In-Reply-To: <5460AF22.2040701@mojatatu.com>
On Mon, Nov 10, 2014 at 2:27 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 11/10/14 03:46, Scott Feldman wrote:
>
>>
>> IFLA_BRPORT_LEARNING is u8 attr and we're only using lower bit to turn
>> learning on/off. Maybe we can use another bit to indicate learning to
>> be done in sw or hw. I don't think adding another bit would break
>> existing iproute2.
>>
>> LEARNING_ENABLED (1 << 0)
>> LEARNING_HW (1 << 1)
>>
>> Would this work?
>>
>
> Yes to making it a bit. But:
> This is not *learning*. You are doing a *sync*.
> Those are two different things.
>
> Learning on/off exists today. It signals to the L2 whether you
> should learn or not.
> I like the way fdb_add/del work with a flag which says
> it is the software and/or offloaded version. Please keep that
> semantic.
> What you are doing above is letting the hardware learn then
> syncing to software. You need a different flag there. something
> like:
>
> SYNC_HW_FDB (1<<1)
Agreed, that's more accurate. Thanks for the refinement.
>
> cheers,
> jamal
^ permalink raw reply
* Re: linux-next: manual merge of the tiny tree with the net-next tree
From: Josh Triplett @ 2014-11-10 17:56 UTC (permalink / raw)
To: Stephen Rothwell
Cc: David Miller, netdev, linux-next, linux-kernel,
Hannes Frederic Sowa, Catalina Mocanu
In-Reply-To: <20141110142511.03354f6a@canb.auug.org.au>
On Mon, Nov 10, 2014 at 02:25:11PM +1100, Stephen Rothwell wrote:
> Hi Josh,
>
> Today's linux-next merge of the tiny tree got a conflict in
> lib/Makefile between commit e5a2c8999576 ("fast_hash: avoid indirect
> function calls") from the net-next tree and commit 4ecea0db79ef ("lib:
> rhashtable: Make rhashtable.c optional") from the tiny tree.
>
> I fixed it up (see below) and can carry the fix as necessary (no action
> is required).
This resolution looks correct to me.
- Josh Triplett
> diff --cc lib/Makefile
> index 04e53dd16070,47b8305288e2..000000000000
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@@ -22,11 -22,14 +22,14 @@@ lib-$(CONFIG_SMP) += cpumask.
> lib-y += kobject.o klist.o
> obj-y += lockref.o
>
> - obj-y += bcd.o div64.o sort.o parser.o halfmd4.o debug_locks.o random32.o \
> + obj-y += bcd.o div64.o sort.o parser.o debug_locks.o random32.o \
> bust_spinlocks.o hexdump.o kasprintf.o bitmap.o scatterlist.o \
> - gcd.o lcm.o list_sort.o uuid.o flex_array.o iovec.o clz_ctz.o \
> + gcd.o lcm.o list_sort.o uuid.o iovec.o clz_ctz.o \
> bsearch.o find_last_bit.o find_next_bit.o llist.o memweight.o kfifo.o \
> - percpu-refcount.o percpu_ida.o rhashtable.o
> - percpu-refcount.o percpu_ida.o hash.o
> ++ percpu-refcount.o percpu_ida.o
> + obj-$(CONFIG_FLEX_ARRAY) += flex_array.o
> + obj-$(CONFIG_HALFMD4) += halfmd4.o
> + obj-$(CONFIG_RHASHTABLE) += rhashtable.o
> obj-y += string_helpers.o
> obj-$(CONFIG_TEST_STRING_HELPERS) += test-string_helpers.o
> obj-y += kstrtox.o
^ permalink raw reply
* Re: [patch net-next v2 03/10] rtnl: expose physical switch id for particular device
From: Roopa Prabhu @ 2014-11-10 17:58 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, nhorman, andy, tgraf, dborkman, ogerlitz, jesse,
pshelar, azhou, ben, stephen, jeffrey.t.kirsher, vyasevic,
xiyou.wangcong, john.r.fastabend, edumazet, jhs, sfeldma,
f.fainelli, linville, jasowang, ebiederm, nicolas.dichtel,
ryazanov.s.a, buytenh, aviadr, nbd, alexei.starovoitov,
Neil.Jerram, ronye, simon.horman, alexander.h.duyck, john.ronciak,
mleitner, shrijeet, gospo, bcrl
In-Reply-To: <1415530280-9190-4-git-send-email-jiri@resnulli.us>
On 11/9/14, 2:51 AM, Jiri Pirko wrote:
> The netdevice represents a port in a switch, it will expose
> IFLA_PHYS_SWITCH_ID value via rtnl. Two netdevices with the same value
> belong to one physical switch.
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
> include/uapi/linux/if_link.h | 1 +
> net/core/rtnetlink.c | 26 +++++++++++++++++++++++++-
> 2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index 7072d83..4163753 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -145,6 +145,7 @@ enum {
> IFLA_CARRIER,
> IFLA_PHYS_PORT_ID,
> IFLA_CARRIER_CHANGES,
> + IFLA_PHYS_SWITCH_ID,
Jiri, since we have not really converged on the switchdev class or
having a separate switchdev instance,
am thinking it is better if we dont expose any such switch_id to
userspace yet until absolutely needed. Do you need it today ?
There is no real in kernel hw switch driver that will use it today. And
quite likely this will need to change when we introduce real hw switch
drivers.
> __IFLA_MAX
> };
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 1087c6d..f839354 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -43,6 +43,7 @@
>
> #include <linux/inet.h>
> #include <linux/netdevice.h>
> +#include <net/switchdev.h>
> #include <net/ip.h>
> #include <net/protocol.h>
> #include <net/arp.h>
> @@ -868,7 +869,8 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
> + rtnl_port_size(dev, ext_filter_mask) /* IFLA_VF_PORTS + IFLA_PORT_SELF */
> + rtnl_link_get_size(dev) /* IFLA_LINKINFO */
> + rtnl_link_get_af_size(dev) /* IFLA_AF_SPEC */
> - + nla_total_size(MAX_PHYS_ITEM_ID_LEN); /* IFLA_PHYS_PORT_ID */
> + + nla_total_size(MAX_PHYS_ITEM_ID_LEN) /* IFLA_PHYS_PORT_ID */
> + + nla_total_size(MAX_PHYS_ITEM_ID_LEN); /* IFLA_PHYS_SWITCH_ID */
> }
>
> static int rtnl_vf_ports_fill(struct sk_buff *skb, struct net_device *dev)
> @@ -967,6 +969,24 @@ static int rtnl_phys_port_id_fill(struct sk_buff *skb, struct net_device *dev)
> return 0;
> }
>
> +static int rtnl_phys_switch_id_fill(struct sk_buff *skb, struct net_device *dev)
> +{
> + int err;
> + struct netdev_phys_item_id psid;
> +
> + err = netdev_sw_parent_id_get(dev, &psid);
> + if (err) {
> + if (err == -EOPNOTSUPP)
> + return 0;
> + return err;
> + }
> +
> + if (nla_put(skb, IFLA_PHYS_SWITCH_ID, psid.id_len, psid.id))
> + return -EMSGSIZE;
> +
> + return 0;
> +}
> +
> static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
> int type, u32 pid, u32 seq, u32 change,
> unsigned int flags, u32 ext_filter_mask)
> @@ -1039,6 +1059,9 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
> if (rtnl_phys_port_id_fill(skb, dev))
> goto nla_put_failure;
>
> + if (rtnl_phys_switch_id_fill(skb, dev))
> + goto nla_put_failure;
> +
> attr = nla_reserve(skb, IFLA_STATS,
> sizeof(struct rtnl_link_stats));
> if (attr == NULL)
> @@ -1198,6 +1221,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
> [IFLA_NUM_RX_QUEUES] = { .type = NLA_U32 },
> [IFLA_PHYS_PORT_ID] = { .type = NLA_BINARY, .len = MAX_PHYS_ITEM_ID_LEN },
> [IFLA_CARRIER_CHANGES] = { .type = NLA_U32 }, /* ignored */
> + [IFLA_PHYS_SWITCH_ID] = { .type = NLA_BINARY, .len = MAX_PHYS_ITEM_ID_LEN },
> };
>
> static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
^ permalink raw reply
* Re: [PATCH v2 net-next 2/2] mlx4: use napi_complete_done()
From: Eric Dumazet @ 2014-11-10 18:01 UTC (permalink / raw)
To: David Miller; +Cc: netdev, amirv, ogerlitz, willemb
In-Reply-To: <20141107.170053.1003349690694025765.davem@redhat.com>
On Fri, 2014-11-07 at 17:00 -0500, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Thu, 06 Nov 2014 21:10:11 -0800
...
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
>
> Applied.
I managed to add a bug in this trivial patch.
I'll send a fix asap.
^ permalink raw reply
* Re: [PATCH] brcmfmac: unlink URB when request timed out
From: Arend van Spriel @ 2014-11-10 18:03 UTC (permalink / raw)
To: Mathy Vanhoef, brudley, frankyl, meuleman, linville, pieterpg,
linux-wireless, brcm80211-dev-list, netdev, Oliver Neukum
In-Reply-To: <5460E2FA.40201@gmail.com>
On 10-11-14 17:08, Mathy Vanhoef wrote:
> On 11/10/2014 06:18 AM, Arend van Spriel wrote:
>> On 09-11-14 19:10, Mathy Vanhoef wrote:
>>> From: Mathy Vanhoef <vanhoefm@gmail.com>
>>>
>>> Unlink the submitted URB in brcmf_usb_dl_cmd if the request timed out. This
>>> assures the URB is never submitted twice, preventing a driver crash.
>>
>> Hi Mathy,
>>
>> What driver crash are you referring to? The log only shows the WARNING
>> ending in a USB disconnect but no actual crash. Does your patch get the
>> driver running properly or does it only avoid the warning.
>
> Hi Arend,
>
> It shows a warning, after which the device doesn't work (but the computer is
> still usable). But I've noticed that when *unplugging* the USB cable the OS may
> freeze. This doesn't always happen though, sometimes unplugging works OK. The
> patch both avoids the warning, and gets the device/driver running properly
> (unplugging also works OK).
>
>>
>> With that said, it seems there is some need for improvement, but I also
>> notice you are running this on a virtual machine so could that affect
>> the timeout to kick in before completion. Could you try to increase the
>> timeout. Still when a timeout occurs this needs to be handled properly.
>> Could you also try the following patch?
>
> I did a few additional tests:
Thanks for being thorough here.
> 1. When increasing IOCTL_RESP_TIMEOUT to 20000 (ten times the normal value) the
> timeout and warning still occur. Device/driver doesn't work.
> 2. When increasing BRCMF_USB_RESET_GETVER_SPINWAIT to 1000 (ten timers the
> normal value) everything works. Device/driver works.
Just to clarify, but did you have IOCTL_RESP_TIMEOUT back on normal
value here?
> 3. Quick test using backports-3.18-rc1-1 (aka unpatched driver) on a non-
> virtualized Linux install: In that case everything worked fine. So the bug
> may only be triggered in a virtualized environment / VMWare.
My colleague has been running brcmfmac with USB devices under VirtualBox
and I know he needed to fix something to get it working properly. Just
could not ask him yet as he (hopefully) enjoyed an extended weekend.
> 4. When applying your patch, the driver stops early during initialization of
> the device. I included a WARN_ONCE before returning EINVAL and got the
> output below.
I expected as much. It only avoids the resubmit of the ctl_urb. Given
your description of the unplug freezes I suspect some issue in the
VMware usb virtualization.
Regards,
Arend
> Kind regards,
> Mathy
> ---
> [ 220.955647] usb 1-1: new high-speed USB device number 3 using ehci-pci
> [ 221.487797] usb 1-1: New USB device found, idVendor=043e, idProduct=3004
> [ 221.487802] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
> [ 221.487804] usb 1-1: Product: Remote Download Wireless Adapter
> [ 221.487806] usb 1-1: Manufacturer: Broadcom
> [ 221.487808] usb 1-1: SerialNumber: 000000000001
> [ 221.490472] brcmfmac: brcmf_usb_probe Enter 0x043e:0x3004
> [ 221.490476] brcmfmac: brcmf_usb_probe Broadcom high speed USB WLAN interface detected
> [ 221.490477] brcmfmac: brcmf_usb_probe_cb Enter
> [ 221.490480] brcmfmac: brcmf_usb_attach Enter
> [ 221.490494] brcmfmac: brcmf_usb_dlneeded Enter
> [ 221.490495] ------------[ cut here ]------------
> [ 221.490503] WARNING: CPU: 0 PID: 100 at drivers/net/wireless/brcm80211/brcmfmac/usb.c:716 brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac]()
> [ 221.490505] EINVAL devinfo=c0044000 ctl_rub=ef898380 completed=0
> [ 221.490506] Modules linked in: brcmfmac brcmutil vmw_pvscsi pcnet32 mptspi mptscsih mptbase
> [ 221.490514] CPU: 0 PID: 100 Comm: kworker/0:1 Not tainted 3.18.0-rc3-wl+ #2
> [ 221.490515] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/31/2013
> [ 221.490528] Workqueue: usb_hub_wq hub_event
> [ 221.490530] 00000000 00000000 eecffb58 c1711f4a eecffb98 eecffb88 c103edaf f11cbc58
> [ 221.490534] eecffbb4 00000064 f11cbc84 000002cc f11c1595 f11c1595 c0044000 ffffffea
> [ 221.490537] ef726000 eecffba0 c103ee4e 00000009 eecffb98 f11cbc58 eecffbb4 eecffbd0
> [ 221.490541] Call Trace:
> [ 221.490550] [<c1711f4a>] dump_stack+0x41/0x52
> [ 221.490558] [<c103edaf>] warn_slowpath_common+0x7f/0xa0
> [ 221.490563] [<f11c1595>] ? brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac]
> [ 221.490567] [<f11c1595>] ? brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac]
> [ 221.490570] [<c103ee4e>] warn_slowpath_fmt+0x2e/0x30
> [ 221.490575] [<f11c1595>] brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac]
> [ 221.490580] [<f11c2cd8>] brcmf_usb_probe+0x3c8/0x640 [brcmfmac]
> [ 221.490583] [<c1717d53>] ? mutex_lock+0x13/0x32
> [ 221.490586] [<c1493ae3>] usb_probe_interface+0xa3/0x180
> [ 221.490590] [<c13f5690>] ? __driver_attach+0x90/0x90
> [ 221.490592] [<c13f546e>] driver_probe_device+0x5e/0x1f0
> [ 221.490595] [<c13f5690>] ? __driver_attach+0x90/0x90
> [ 221.490597] [<c13f56c9>] __device_attach+0x39/0x50
> [ 221.490600] [<c13f3d84>] bus_for_each_drv+0x34/0x70
> [ 221.490602] [<c13f53db>] device_attach+0x7b/0x90
> [ 221.490604] [<c13f5690>] ? __driver_attach+0x90/0x90
> [ 221.490607] [<c13f4b8f>] bus_probe_device+0x6f/0x90
> [ 221.490609] [<c13f3256>] device_add+0x426/0x520
> [ 221.490611] [<c1491503>] ? usb_control_msg+0xb3/0xd0
> [ 221.490614] [<c1717d53>] ? mutex_lock+0x13/0x32
> [ 221.490627] [<c14922f8>] usb_set_configuration+0x3f8/0x700
> [ 221.490630] [<c13f5690>] ? __driver_attach+0x90/0x90
> [ 221.490633] [<c149ac7b>] generic_probe+0x2b/0x90
> [ 221.490637] [<c1188bc0>] ? sysfs_create_link+0x20/0x40
> [ 221.490639] [<c1492bec>] usb_probe_device+0xc/0x10
> [ 221.490641] [<c13f546e>] driver_probe_device+0x5e/0x1f0
> [ 221.490644] [<c13f5690>] ? __driver_attach+0x90/0x90
> [ 221.490646] [<c13f56c9>] __device_attach+0x39/0x50
> [ 221.490649] [<c13f3d84>] bus_for_each_drv+0x34/0x70
> [ 221.490651] [<c13f53db>] device_attach+0x7b/0x90
> [ 221.490653] [<c13f5690>] ? __driver_attach+0x90/0x90
> [ 221.490656] [<c13f4b8f>] bus_probe_device+0x6f/0x90
> [ 221.490658] [<c13f3256>] device_add+0x426/0x520
> [ 221.490661] [<c148aa2e>] ? usb_new_device+0x16e/0x3a0
> [ 221.490663] [<c148aad7>] usb_new_device+0x217/0x3a0
> [ 221.490666] [<c148bff7>] hub_event+0xa17/0xda0
> [ 221.490668] [<c1716918>] ? __schedule+0x2f8/0x710
> [ 221.490672] [<c105127c>] ? pwq_dec_nr_in_flight+0x3c/0x90
> [ 221.490674] [<c10513ee>] process_one_work+0x11e/0x360
> [ 221.490677] [<c1051750>] worker_thread+0xf0/0x3c0
> [ 221.490680] [<c106e14a>] ? __wake_up_locked+0x1a/0x20
> [ 221.490682] [<c1051660>] ? process_scheduled_works+0x30/0x30
> [ 221.490685] [<c1055b56>] kthread+0x96/0xb0
> [ 221.490687] [<c1050000>] ? put_unbound_pool+0x110/0x170
> [ 221.490691] [<c1719c81>] ret_from_kernel_thread+0x21/0x30
> [ 221.490693] [<c1055ac0>] ? kthread_worker_fn+0x110/0x110
> [ 221.490695] ---[ end trace 9befd914693f3083 ]---
> [ 221.490697] brcmfmac: brcmf_usb_dlneeded chip 57005 rev 0xf11cfcec
> [ 221.490699] brcmfmac: brcmf_fw_get_firmwares enter: dev=1-1
>
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
> index 5265aa7..15b1aa7 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
> @@ -709,8 +709,13 @@ static int brcmf_usb_dl_cmd(struct brcmf_usbdev_info *devinfo, u8 cmd,
> char *tmpbuf;
> u16 size;
>
> - if ((!devinfo) || (devinfo->ctl_urb == NULL))
> + if (!devinfo || !devinfo->ctl_urb || !devinfo->ctl_completed) {
> + WARN_ONCE(1, "EINVAL devinfo=%p ctl_rub=%p completed=%d\n",
> + devinfo,
> + devinfo ? devinfo->ctl_urb : NULL,
> + devinfo ? devinfo->ctl_completed : -1);
> return -EINVAL;
> + }
>
> tmpbuf = kmalloc(buflen, GFP_ATOMIC);
> if (!tmpbuf)
>
>
>>
>> Regards,
>> Arend
>> ---
>> drivers/net/wireless/brcm80211/brcmfmac/usb.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
>> b/drivers/net/wireles
>> index dc13591..786c40b 100644
>> --- a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
>> @@ -640,7 +640,7 @@ static int brcmf_usb_dl_cmd(struct brcmf_usbdev_info
>> *devinf
>> char *tmpbuf;
>> u16 size;
>>
>> - if ((!devinfo) || (devinfo->ctl_urb == NULL))
>> + if (!devinfo || !devinfo->ctl_urb || !devinfo->ctl_completed)
>> return -EINVAL;
>>
>> tmpbuf = kmalloc(buflen, GFP_ATOMIC);
>>
>>> Signed-off-by: Mathy Vanhoef <vanhoefm@gmail.com>
>>> ---
>>> Currently brcmfmac may crash when a USB device is attached (tested with a LG
>>> TWFM-B003D). In particular it fails on the second call to brcmf_usb_dl_cmd in
>>> the while loop of brcmf_usb_resetcfg. The problem is that an URB is being
>>> submitted twice:
>>>
>>> [ 169.861800] brcmfmac: brcmf_usb_dl_writeimage Enter, fw f14db000, len 348160
>>> [ 171.787791] brcmfmac: brcmf_usb_dl_writeimage Exit, err=0
>>> [ 171.787797] brcmfmac: brcmf_usb_dlstart Exit, err=0
>>> [ 171.787799] brcmfmac: brcmf_usb_dlrun Enter
>>> [ 171.791794] brcmfmac: brcmf_usb_resetcfg Enter
>>> [ 173.988072] ------------[ cut here ]------------
>>> [ 173.988083] WARNING: CPU: 0 PID: 369 at drivers/usb/core/urb.c:339 usb_submit_urb+0x4e6/0x500()
>>> [ 173.988085] URB eaf45f00 submitted while active
>>> [ 173.988086] Modules linked in: brcmfmac brcmutil vmw_pvscsi pcnet32 mptspi mptscsih mptbase
>>> [ 173.988100] CPU: 0 PID: 369 Comm: kworker/0:2 Not tainted 3.18.0-rc3-wl #1
>>> [ 173.988102] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/31/2013
>>> [ 173.988106] Workqueue: events request_firmware_work_func
>>> [ 173.988108] 00000000 00000000 ee747db8 c1711f4a ee747df8 ee747de8 c103edaf c18d1e10
>>> [ 173.988112] ee747e14 00000171 c18a8b29 00000153 c1490556 c1490556 eaf45f00 eafdc660
>>> [ 173.988115] f14b8fa0 ee747e00 c103ee4e 00000009 ee747df8 c18d1e10 ee747e14 ee747e50
>>> [ 173.988119] Call Trace:
>>> [ 173.988129] [<c1711f4a>] dump_stack+0x41/0x52
>>> [ 173.988136] [<c103edaf>] warn_slowpath_common+0x7f/0xa0
>>> [ 173.988139] [<c1490556>] ? usb_submit_urb+0x4e6/0x500
>>> [ 173.988141] [<c1490556>] ? usb_submit_urb+0x4e6/0x500
>>> [ 173.988147] [<f14b8fa0>] ? brcmf_usb_ioctl_resp_wake+0x40/0x40 [brcmfmac]
>>> [ 173.988150] [<c103ee4e>] warn_slowpath_fmt+0x2e/0x30
>>> [ 173.988152] [<c1490556>] usb_submit_urb+0x4e6/0x500
>>> [ 173.988156] [<c1123de1>] ? __kmalloc+0x21/0x140
>>> [ 173.988161] [<f14b91c3>] ? brcmf_usb_dl_cmd+0x33/0x120 [brcmfmac]
>>> [ 173.988166] [<f14b9243>] brcmf_usb_dl_cmd+0xb3/0x120 [brcmfmac]
>>> [ 173.988170] [<f14ba6c4>] brcmf_usb_probe_phase2+0x4e4/0x640 [brcmfmac]
>>> [ 173.988176] [<f14b4900>] brcmf_fw_request_code_done+0xd0/0xf0 [brcmfmac]
>>> [ 173.988178] [<c1400876>] request_firmware_work_func+0x26/0x50
>>> [ 173.988182] [<c10513ee>] process_one_work+0x11e/0x360
>>> [ 173.988184] [<c1051750>] worker_thread+0xf0/0x3c0
>>> [ 173.988205] [<c106e14a>] ? __wake_up_locked+0x1a/0x20
>>> [ 173.988208] [<c1051660>] ? process_scheduled_works+0x30/0x30
>>> [ 173.988211] [<c1055b56>] kthread+0x96/0xb0
>>> [ 173.988214] [<c1719c81>] ret_from_kernel_thread+0x21/0x30
>>> [ 173.988217] [<c1055ac0>] ? kthread_worker_fn+0x110/0x110
>>> [ 173.988219] ---[ end trace 0c88bf46801de083 ]---
>>> [ 173.988221] brcmf_usb_dl_cmd: usb_submit_urb failed -16
>>> [ 173.988396] brcmfmac: brcmf_usb_probe_phase2 failed: dev=1-1, err=-19
>>> [ 173.989503] brcmfmac: brcmf_usb_disconnect Enter
>>>
>>> This patch fixes the brcmf_usb_dl_cmd function to prevent an URB from being
>>> submitted twice. Tested using a LG TWFM-B003D, which now works properly.
>>>
>>>
>>> drivers/net/wireless/brcm80211/brcmfmac/usb.c | 6 ++++--
>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
>>> index 5265aa7..1bc7858 100644
>>> --- a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
>>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
>>> @@ -738,10 +738,12 @@ static int brcmf_usb_dl_cmd(struct brcmf_usbdev_info *devinfo, u8 cmd,
>>> goto finalize;
>>> }
>>>
>>> - if (!brcmf_usb_ioctl_resp_wait(devinfo))
>>> + if (!brcmf_usb_ioctl_resp_wait(devinfo)) {
>>> + usb_unlink_urb(devinfo->ctl_urb);
>>> ret = -ETIMEDOUT;
>>> - else
>>> + } else {
>>> memcpy(buffer, tmpbuf, buflen);
>>> + }
>>>
>>> finalize:
>>> kfree(tmpbuf);
>>>
>>
^ permalink raw reply
* Re: [PATCHv3 net-next 0/3] RDMA/cxgb4,cxgb4vf,cxgb4i,csiostor: Cleanup macros
From: David Miller @ 2014-11-10 17:57 UTC (permalink / raw)
To: hariprasad
Cc: netdev, linux-rdma, linux-scsi, roland, JBottomley, hch, swise,
leedom, anish, praveenm, nirranjan, kumaras
In-Reply-To: <1415333125-10635-1-git-send-email-hariprasad@chelsio.com>
From: Hariprasad Shenai <hariprasad@chelsio.com>
Date: Fri, 7 Nov 2014 09:35:22 +0530
> This series moves the debugfs code to a new file debugfs.c and cleans up
> macros/register defines.
>
> Various patches have ended up changing the style of the symbolic macros/register
> defines and some of them used the macros/register defines that matches the
> output of the script from the hardware team.
>
> As a result, the current kernel.org files are a mix of different macro styles.
> Since this macro/register defines is used by five different drivers, a
> few patch series have ended up adding duplicate macro/register define entries
> with different styles. This makes these register define/macro files a complete
> mess and we want to make them clean and consistent.
>
> Will post few more series so that we can cover all the macros so that they all
> follow the same style to be consistent.
>
> The patches series is created against 'net-next' tree.
> And includes patches on cxgb4, cxgb4vf, iw_cxgb4, csiostor and cxgb4i driver.
>
> We have included all the maintainers of respective drivers. Kindly review the
> change and let us know in case of any review comments.
...
> V3: Use suffix instead of prefix for macros/register defines
> V2: Changes the description and cover-letter content to answer David Miller's
> question
Series applied, thanks.
^ permalink raw reply
* Re: linux-next: Tree for Nov 10 (net/ipv4/ip_tunnel.c)
From: Randy Dunlap @ 2014-11-10 18:15 UTC (permalink / raw)
To: Stephen Rothwell, linux-next; +Cc: linux-kernel, netdev@vger.kernel.org
In-Reply-To: <20141110205945.51af32a6@canb.auug.org.au>
On 11/10/14 01:59, Stephen Rothwell wrote:
> Hi all,
>
> Changes since 20141106:
>
on x86_64:
when CONFIG_NET_IP_TUNNEL=y and CONFIG_NET_FOU=m:
net/built-in.o: In function `ip_tunnel_encap':
(.text+0xb04d8): undefined reference to `gue_build_header'
net/built-in.o: In function `ip_tunnel_encap':
(.text+0xb04ea): undefined reference to `fou_build_header'
--
~Randy
^ 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