* Re: brcmfmac: print name of connect status event
From: Kalle Valo @ 2016-11-17 6:45 UTC (permalink / raw)
To: Rafał Miłecki
Cc: Arend van Spriel, Franky Lin, Hante Meuleman,
Pieter-Paul Giesberts, Franky Lin, linux-wireless,
brcm80211-dev-list.pdl, netdev, linux-kernel,
Rafał Miłecki
In-Reply-To: <20161014074559.22962-1-zajec5@gmail.com>
Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> This simplifies debugging. Format %s (%u) comes from similar debugging
> message in brcmf_fweh_event_worker.
>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
Patch applied to wireless-drivers-next.git, thanks.
e1c122d55f9e brcmfmac: print name of connect status event
--
https://patchwork.kernel.org/patch/9376147/
Documentation about submitting wireless patches and checking status
from patchwork:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: p54: memset(0) whole array
From: Kalle Valo @ 2016-11-17 6:45 UTC (permalink / raw)
To: Jiri Slaby
Cc: chunkeey-gM/Ye1E23mwN+BqQ9rBEUg,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Jiri Slaby,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161014092309.24113-1-jslaby-AlSwsSmVLrQ@public.gmane.org>
Jiri Slaby <jslaby-AlSwsSmVLrQ@public.gmane.org> wrote:
> gcc 7 complains:
> drivers/net/wireless/intersil/p54/fwio.c: In function 'p54_scan':
> drivers/net/wireless/intersil/p54/fwio.c:491:4: warning: 'memset' used with length equal to number of elements without multiplication by element size [-Wmemset-elt-size]
>
> Fix that by passing the correct size to memset.
>
> Signed-off-by: Jiri Slaby <jslaby-AlSwsSmVLrQ@public.gmane.org>
> Cc: Christian Lamparter <chunkeey-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> Cc: Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Acked-by: Christian Lamparter <chunkeey-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
Patch applied to wireless-drivers-next.git, thanks.
6f1758178820 p54: memset(0) whole array
--
https://patchwork.kernel.org/patch/9376333/
Documentation about submitting wireless patches and checking status
from patchwork:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: wireless: fix bogus maybe-uninitialized warning
From: Kalle Valo @ 2016-11-17 6:46 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Kalle Valo, Stanislav Yakovlev, Jouni Malinen, Johannes Berg,
David S. Miller, linux-wireless, netdev, Arnd Bergmann,
linux-kernel
In-Reply-To: <20161024153918.2810634-2-arnd@arndb.de>
Arnd Bergmann <arnd@arndb.de> wrote:
> The hostap_80211_rx() function is supposed to set up the mac addresses
> for four possible cases, based on two bits of input data. For
> some reason, gcc decides that it's possible that none of the these
> four cases apply and the addresses remain uninitialized:
>
> drivers/net/wireless/intersil/hostap/hostap_80211_rx.c: In function ‘hostap_80211_rx’:
> arch/x86/include/asm/string_32.h:77:14: warning: ‘src’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> drivers/net/wireless/intel/ipw2x00/libipw_rx.c: In function ‘libipw_rx’:
> arch/x86/include/asm/string_32.h:77:14: error: ‘dst’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> arch/x86/include/asm/string_32.h:78:22: error: ‘*((void *)&dst+4)’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> This warning is clearly nonsense, but changing the last case into
> 'default' makes it obvious to the compiler too, which avoids the
> warning and probably leads to better object code too.
>
> The same code is duplicated several times in the kernel, so this
> patch uses the same workaround for all copies. The exact configuration
> was hit only very rarely in randconfig builds and I only saw it
> in three drivers, but I assume that all of them are potentially
> affected, and it's better to keep the code consistent.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Patch applied to wireless-drivers-next.git, thanks.
10f3366b4d89 wireless: fix bogus maybe-uninitialized warning
--
https://patchwork.kernel.org/patch/9392385/
Documentation about submitting wireless patches and checking status
from patchwork:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [v2] cw1200: fix bogus maybe-uninitialized warning
From: Kalle Valo @ 2016-11-17 6:48 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Solomon Peachy, David Laight, Arnd Bergmann, Johannes Berg,
linux-wireless, netdev, linux-kernel
In-Reply-To: <20161025202121.1070879-1-arnd@arndb.de>
Arnd Bergmann <arnd@arndb.de> wrote:
> On x86, the cw1200 driver produces a rather silly warning about the
> possible use of the 'ret' variable without an initialization
> presumably after being confused by the architecture specific definition
> of WARN_ON:
>
> drivers/net/wireless/st/cw1200/wsm.c: In function ‘wsm_handle_rx’:
> drivers/net/wireless/st/cw1200/wsm.c:1457:9: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> We have already checked that 'count' is larger than 0 here, so
> we know that 'ret' is initialized. Changing the 'for' loop
> into do/while also makes this clear to the compiler.
>
> Suggested-by: David Laight <David.Laight@ACULAB.COM>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Patch applied to wireless-drivers-next.git, thanks.
7fc1503c906f cw1200: fix bogus maybe-uninitialized warning
--
https://patchwork.kernel.org/patch/9395413/
Documentation about submitting wireless patches and checking status
from patchwork:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH net-next v2 3/3] net: marvell: Allow drivers to be built with COMPILE_TEST
From: kbuild test robot @ 2016-11-17 7:10 UTC (permalink / raw)
To: Florian Fainelli
Cc: kbuild-all, netdev, davem, mw, arnd, gregory.clement, Shaohui.Xie,
Florian Fainelli
In-Reply-To: <20161116003541.18415-4-f.fainelli@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 902 bytes --]
Hi Florian,
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Florian-Fainelli/net-fsl-Allow-most-drivers-to-be-built-with-COMPILE_TEST/20161116-094841
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=mips
All errors (new ones prefixed by >>):
drivers/built-in.o: In function `mvneta_bm_pool_create':
>> mvneta_bm.c:(.text+0x772f0c): undefined reference to `mvebu_mbus_get_dram_win_info'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 44138 bytes --]
^ permalink raw reply
* Re: [Patch net] net: check dead netns for peernet2id_alloc()
From: Andrei Vagin @ 2016-11-17 7:17 UTC (permalink / raw)
To: Cong Wang; +Cc: Linux Kernel Network Developers, Nicolas Dichtel
In-Reply-To: <1479320822-17483-1-git-send-email-xiyou.wangcong@gmail.com>
On Wed, Nov 16, 2016 at 10:27 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> Andrei reports we still allocate netns ID from idr after we destroy
> it in cleanup_net().
>
> cleanup_net():
> ...
> idr_destroy(&net->netns_ids);
> ...
> list_for_each_entry_reverse(ops, &pernet_list, list)
> ops_exit_list(ops, &net_exit_list);
> -> rollback_registered_many()
> -> rtmsg_ifinfo_build_skb()
> -> rtnl_fill_ifinfo()
> -> peernet2id_alloc()
>
> After that point we should not even access net->netns_ids, we
> should check the death of the current netns as early as we can in
> peernet2id_alloc().
>
> For net-next we can consider to avoid sending rtmsg totally,
> it is a good optimization for netns teardown path.
It works for me and looks good. Thanks.
Acked-by: Andrei Vagin <avagin@openvz.org>
>
> Fixes: 0c7aecd4bde4 ("netns: add rtnl cmd to add and get peer netns ids")
> Reported-by: Andrei Vagin <avagin@gmail.com>
> Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
> net/core/net_namespace.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index f61c0e0..7001da9 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -219,6 +219,8 @@ int peernet2id_alloc(struct net *net, struct net *peer)
> bool alloc;
> int id;
>
> + if (atomic_read(&net->count) == 0)
> + return NETNSA_NSID_NOT_ASSIGNED;
> spin_lock_irqsave(&net->nsid_lock, flags);
> alloc = atomic_read(&peer->count) == 0 ? false : true;
> id = __peernet2id_alloc(net, peer, &alloc);
> --
> 2.1.0
>
^ permalink raw reply
* Re: Virtio_net support vxlan encapsulation package TSO offload discuss
From: Jason Wang @ 2016-11-17 7:27 UTC (permalink / raw)
To: Zhangming (James, Euler), netdev@vger.kernel.org
Cc: Michael S. Tsirkin, Vlad Yasevic, Amnon Ilan
In-Reply-To: <DBCD2614ECF3FF4087A2C27CA80E34DD54027936@SZXEMA501-MBX.china.huawei.com>
On 2016年11月17日 09:31, Zhangming (James, Euler) wrote:
> On 2016年11月15日 11:28, Jason Wang wrote:
>> On 2016年11月10日 14:19, Zhangming (James, Euler) wrote:
>>> On 2016年11月09日 15:14, Jason Wang wrote:
>>>> On 2016年11月08日 19:58, Zhangming (James, Euler) wrote:
>>>>> On 2016年11月08日 19:17, Jason Wang wrote:
>>>>>
>>>>>> On 2016年11月08日 19:13, Jason Wang wrote:
>>>>>>> Cc Michael
>>>>>>>
>>>>>>> On 2016年11月08日 16:34, Zhangming (James, Euler) wrote:
>>>>>>>> In container scenario, OVS is installed in the Virtual machine,
>>>>>>>> and all the containers connected to the OVS will communicated
>>>>>>>> through VXLAN encapsulation.
>>>>>>>>
>>>>>>>> By now, virtio_net does not support TSO offload for VXLAN
>>>>>>>> encapsulated TSO package. In this condition, the performance is
>>>>>>>> not good, sender is bottleneck
>>>>>>>>
>>>>>>>> I googled this scenario, but I didn’t find any information. Will
>>>>>>>> virtio_net support VXLAN encapsulation package TSO offload later?
>>>>>>>>
>>>>>>> Yes and for both sender and receiver.
>>>>>>>
>>>>>>>> My idea is virtio_net open encapsulated TSO offload, and
>>>>>>>> transport encapsulation info to TUN, TUN will parse the info and
>>>>>>>> build skb with encapsulation info.
>>>>>>>>
>>>>>>>> OVS or kernel on the host should be modified to support this.
>>>>>>>> Using this method, the TCP performance aremore than 2x as before.
>>>>>>>>
>>>>>>>> Any advice and suggestions for this idea or new idea will be
>>>>>>>> greatly appreciated!
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>>
>>>>>>>> James zhang
>>>>>>>>
>>>>>>> Sounds very good. And we may also need features bits
>>>>>>> (VIRTIO_NET_F_GUEST|HOST_GSO_X) for this.
>>>>>>>
>>>>>>> This is in fact one of items in networking todo list. (See
>>>>>>> http://www.linux-kvm.org/page/NetworkingTodo). While at it, we'd
>>>>>>> better support not only VXLAN but also other tunnels.
>>>>>> Cc Vlad who is working on extending virtio-net headers.
>>>>>>
>>>>>>> We can start with the spec work, or if you've already had some
>>>>>>> bits you can post them as RFC for early review.
>>>>>>>
>>>>>>> Thanks
>>>>> Below is my demo code
>>>>> Virtio_net.c
>>>>> static int virtnet_probe(struct virtio_device *vdev), add belows codes:
>>>>> if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF) || // avoid gso segment, it should be negotiation later, because in the demo I reuse num_buffers.
>>>>> virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>>>>> dev->hw_enc_features |= NETIF_F_TSO;
>>>>> dev->hw_enc_features |= NETIF_F_ALL_CSUM;
>>>>> dev->hw_enc_features |= NETIF_F_GSO_UDP_TUNNEL;
>>>>> dev->hw_enc_features |= NETIF_F_GSO_UDP_TUNNEL_CSUM;
>>>>> dev->hw_enc_features |=
>>>>> NETIF_F_GSO_TUNNEL_REMCSUM;
>>>>>
>>>>> dev->features |= NETIF_F_GSO_UDP_TUNNEL;
>>>>> dev->features |= NETIF_F_GSO_UDP_TUNNEL_CSUM;
>>>>> dev->features |= NETIF_F_GSO_TUNNEL_REMCSUM;
>>>>> }
>>>>>
>>>>> static int xmit_skb(struct send_queue *sq, struct sk_buff *skb), add
>>>>> below to pieces of codes
>>>>>
>>>>> if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL)
>>>>> hdr->hdr.gso_type |= VIRTIO_NET_HDR_GSO_TUNNEL;
>>>>> if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL_CSUM)
>>>>> hdr->hdr.gso_type |= VIRTIO_NET_HDR_GSO_TUNNEL_CSUM;
>>>>> if (skb_shinfo(skb)->gso_type & SKB_GSO_TUNNEL_REMCSUM)
>>>>> hdr->hdr.gso_type |=
>>>>> VIRTIO_NET_HDR_GSO_TUNNEL_REMCSUM;
>>>>>
>>>>> if (skb->encapsulation && skb_is_gso(skb)) {
>>>>> inner_mac_len = skb_inner_network_header(skb) - skb_inner_mac_header(skb);
>>>>> tnl_len = skb_inner_mac_header(skb) - skb_mac_header(skb);
>>>>> if ( !(inner_mac_len >> DATA_LEN_SHIFT) && !(tnl_len >> DATA_LEN_SHIFT) ) {
>>>>> hdr->hdr.flags |= VIRTIO_NET_HDR_F_ENCAPSULATION;
>>>>> hdr->num_buffers = (__virtio16)((inner_mac_len << DATA_LEN_SHIFT) | tnl_len); //we reuse num_buffers for simple , we should add extend member for later.
>>>>> } else
>>>>> hdr->num_buffers = 0;
>>>>> }
>>>>>
>>>>> Tun.c
>>>>> if (memcpy_fromiovecend((void *)&hdr, iv, offset, tun->vnet_hdr_sz)) //read header with negotiation length
>>>>> return -EFAULT;
>>>>>
>>>>> if (hdr.gso_type & VIRTIO_NET_HDR_GSO_TUNNEL) //set tunnel gso info
>>>>> skb_shinfo(skb)->gso_type |= SKB_GSO_UDP_TUNNEL;
>>>>> if (hdr.gso_type & VIRTIO_NET_HDR_GSO_TUNNEL_CSUM)
>>>>> skb_shinfo(skb)->gso_type |= SKB_GSO_UDP_TUNNEL_CSUM;
>>>>> if (hdr.gso_type & VIRTIO_NET_HDR_GSO_TUNNEL_REMCSUM)
>>>>> skb_shinfo(skb)->gso_type |=
>>>>> SKB_GSO_TUNNEL_REMCSUM;
>>>>>
>>>>> if (hdr.flags & VIRTIO_NET_HDR_F_ENCAPSULATION) { //read tunnel info from header and set to built skb.
>>>>> tnl_len = tun16_to_cpu(tun, hdr.num_buffers) & TUN_TNL_LEN_MASK;
>>>>> payload_mac_len = tun16_to_cpu(tun, hdr.num_buffers) >> TUN_DATA_LEN_SHIFT;
>>>>> mac_len = skb_network_header(skb) - skb_mac_header(skb);
>>>>> skb_set_inner_mac_header(skb, tnl_len - mac_len);
>>>>> skb_set_inner_network_header(skb, tnl_len + payload_mac_len - mac_len);
>>>>> skb->encapsulation = 1;
>>>>> }
>>>>>
>>>>>
>>>> Something like this, and you probably need do something more:
>>>>
>>>> - use net-next.git to generate the patch (for the latest code)
>>>> - add feature negotiation
>>>> - tun/macvtap/qemu patches for this, you can start with tun/macvtap
>>>> patches
>>>> - support for all other SKB_GSO_* types which is not supported
>>>> - use a new field instead of num_buffers
>>>> - a virtio spec patch to describe the support for encapsulation
>>>> offload
>>>>
>>>> Thanks
>>> Thank you for your advice, I will start it right now.
>>>
>>> Thanks
>> Cool, one more question: while at it, I think you may want to add support for dpdk too?
>>
>> Thanks
> Do you mean that the patch should be compatible with virtio pmd, or give virtio pmd patch?
>
> Thanks
I mean it's better to prepare patches for both virtio pmd and dpdk.
Thanks
^ permalink raw reply
* Re: Netperf UDP issue with connected sockets
From: Jesper Dangaard Brouer @ 2016-11-17 8:16 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Rick Jones, "netdev
In-Reply-To: <1479342849.8455.233.camel@edumazet-glaptop3.roam.corp.google.com>
On Wed, 16 Nov 2016 16:34:09 -0800
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2016-11-16 at 23:40 +0100, Jesper Dangaard Brouer wrote:
>
> > Using -R 1 does not seem to help remove __ip_select_ident()
> >
> > Samples: 56K of event 'cycles', Event count (approx.): 78628132661
> > Overhead Command Shared Object Symbol
> > + 9.11% netperf [kernel.vmlinux] [k] __ip_select_ident
> > + 6.98% netperf [kernel.vmlinux] [k] _raw_spin_lock
> > + 6.21% swapper [mlx5_core] [k] mlx5e_poll_tx_cq
> > + 5.03% netperf [kernel.vmlinux] [k] copy_user_enhanced_fast_string
> > + 4.69% netperf [kernel.vmlinux] [k] __ip_make_skb
> > + 4.63% netperf [kernel.vmlinux] [k] skb_set_owner_w
> > + 4.15% swapper [kernel.vmlinux] [k] __slab_free
> > + 3.80% netperf [mlx5_core] [k] mlx5e_sq_xmit
> > + 2.00% swapper [kernel.vmlinux] [k] sock_wfree
> > + 1.94% netperf netperf [.] send_data
> > + 1.92% netperf netperf [.] send_omni_inner
>
> Check "ss -nu" ?
>
> You will see if sockets are connected (present in ss output or not)
Tested different versions of netperf, commands used below signature:
netperf-2.6.0: connected "broken"
netperf-2.7.0: connected works
SVN-r709 : connected works
I noticed there is a Send-Q, and the perf-top2 is _raw_spin_lock, which
looks like it comes from __dev_queue_xmit(), but we know from
experience that this stall is actually caused by writing the
tailptr/doorbell in the HW. Thus, this could benefit a lot from
bulk/xmit_more into the qdisc layer.
> UDP being connected does not prevent __ip_select_ident() being used.
>
> if ((iph->frag_off & htons(IP_DF)) && !skb->ignore_df) {
>
> So you need IP_DF being set, and skb->ignore_df being 0
Thanks for explaining that! :-)
http://lxr.free-electrons.com/source/include/net/ip.h?v=4.8#L332
http://lxr.free-electrons.com/source/net/ipv4/ip_output.c?v=4.8#L449
Netperf UDP_STREAM default send 64K packets that get fragmented...
which actually is very unfortunate because people end-up testing a code
path in the kernel they didn't expect. That is why I use the
option "-- -m 1472".
> time to try IP_MTU_DISCOVER ;)
To Rick, maybe you can find a good solution or option with Eric's hint,
to send appropriate sized UDP packets with Don't Fragment (DF).
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
Testing with ss -nua
$ /usr/local/stow/netperf-2.6.0-demo/bin/netperf -H 198.18.50.1 -t UDP_STREAM -l 3 -- -m 1472 -n -N > /dev/null & sleep 1; ss -una
State Recv-Q Send-Q Local Address:Port Peer Address:Port
UNCONN 0 11520 *:54589 *:*
$ /usr/local/stow/netperf-2.7.0-demo/bin/netperf -H 198.18.50.1 -t UDP_STREAM -l 3 -- -m 1472 -n -N > /dev/null & sleep 1; ss -una
State Recv-Q Send-Q Local Address:Port Peer Address:Port
ESTAB 0 18432 198.18.50.3:46803 198.18.50.1:51851
$ ~/tools/netperf2-svn/src/netperf -H 198.18.50.1 -t UDP_STREAM -l 3 -- -m 1472 -n -N > /dev/null & sleep 1; ss -una
State Recv-Q Send-Q Local Address:Port Peer Address:Port
ESTAB 0 43776 198.18.50.3:42965 198.18.50.1:51948
^ permalink raw reply
* Re: [PATCH net v2] net: nsid cannot be allocated for a dead netns
From: Nicolas Dichtel @ 2016-11-17 8:39 UTC (permalink / raw)
To: David Miller, xiyou.wangcong; +Cc: avagin, netdev
In-Reply-To: <20161116.231722.2198167385156796830.davem@davemloft.net>
Le 17/11/2016 à 05:17, David Miller a écrit :
[snip]
> Indeed, this looks cleaner, Nicolas?
Yes, I agree. Cong already sent a v3 of my patch, let's apply it.
^ permalink raw reply
* Re: [Patch net] net: check dead netns for peernet2id_alloc()
From: Nicolas Dichtel @ 2016-11-17 8:39 UTC (permalink / raw)
To: Cong Wang, netdev; +Cc: avagin
In-Reply-To: <1479320822-17483-1-git-send-email-xiyou.wangcong@gmail.com>
Le 16/11/2016 à 19:27, Cong Wang a écrit :
> Andrei reports we still allocate netns ID from idr after we destroy
> it in cleanup_net().
>
> cleanup_net():
> ...
> idr_destroy(&net->netns_ids);
> ...
> list_for_each_entry_reverse(ops, &pernet_list, list)
> ops_exit_list(ops, &net_exit_list);
> -> rollback_registered_many()
> -> rtmsg_ifinfo_build_skb()
> -> rtnl_fill_ifinfo()
> -> peernet2id_alloc()
>
> After that point we should not even access net->netns_ids, we
> should check the death of the current netns as early as we can in
> peernet2id_alloc().
>
> For net-next we can consider to avoid sending rtmsg totally,
> it is a good optimization for netns teardown path.
>
> Fixes: 0c7aecd4bde4 ("netns: add rtnl cmd to add and get peer netns ids")
> Reported-by: Andrei Vagin <avagin@gmail.com>
> Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
^ permalink raw reply
* Re: [PATCH net v2] net: nsid cannot be allocated for a dead netns
From: Nicolas Dichtel @ 2016-11-17 8:41 UTC (permalink / raw)
To: Cong Wang, David Miller; +Cc: Andrey Wagin, Linux Kernel Network Developers
In-Reply-To: <CAM_iQpUGayFoDSQL2w6bOVKG2q=dSDyf1jQ5LvrYuy7V_y_auA@mail.gmail.com>
Le 17/11/2016 à 07:32, Cong Wang a écrit :
[snip]
> since Nicolas' patch doesn't even compile...
It's always surprising how agressive you are, really :(
Can you show me the compilation error of this patch (we are talking about the v2
patch here)?
^ permalink raw reply
* Re: [PATCH ipsec-next 6/8] xfrm: policy: only use rcu in xfrm_sk_policy_lookup
From: Nicolas Dichtel @ 2016-11-17 8:55 UTC (permalink / raw)
To: Florian Westphal; +Cc: netdev, Steffen Klassert
In-Reply-To: <20161116173028.GI30581@breakpoint.cc>
Le 16/11/2016 à 18:30, Florian Westphal a écrit :
[snip]
> This should fix it (if we succeed grabbing the refcount, then if (err && !xfrm_pol_hold_rcu
> evaluates to false and we hit last else branch and set pol to ERR_PTR(0)...
Haha, right :) Thank you for the quick fix.
The patch solves my problem. You can add my 'tested-by' tag in the formal
submission.
Tested-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
^ permalink raw reply
* Re: [PATCH net-next v2 3/3] net: marvell: Allow drivers to be built with COMPILE_TEST
From: Arnd Bergmann @ 2016-11-17 9:10 UTC (permalink / raw)
To: kbuild test robot
Cc: Florian Fainelli, kbuild-all, netdev, davem, mw, gregory.clement,
Shaohui.Xie
In-Reply-To: <201611171025.rUxQZT0L%fengguang.wu@intel.com>
On Thursday, November 17, 2016 10:30:10 AM CET kbuild test robot wrote:
>
> url: https://github.com/0day-ci/linux/commits/Florian-Fainelli/net-fsl-Allow-most-drivers-to-be-built-with-COMPILE_TEST/20161116-094841
> config: m32r-allmodconfig (attached as .config)
> compiler: m32r-linux-gcc (GCC) 6.2.0
> reproduce:
> wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=m32r
>
> All errors (new ones prefixed by >>):
>
> ERROR: "bad_dma_ops" [sound/core/snd-pcm.ko] undefined!
> ERROR: "dma_common_mmap" [sound/core/snd-pcm.ko] undefined!
> >> ERROR: "bad_dma_ops" [drivers/net/ethernet/marvell/mvpp2.ko] undefined!
> ERROR: "mvebu_mbus_get_dram_win_info" [drivers/net/ethernet/marvell/mvneta_bm.ko] undefined!
> >> ERROR: "bad_dma_ops" [drivers/net/ethernet/marvell/mvneta_bm.ko] undefined!
> >> ERROR: "bad_dma_ops" [drivers/net/ethernet/marvell/mvneta.ko] undefined!
> >> ERROR: "bad_dma_ops" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> ERROR: "bad_dma_ops" [drivers/net/ethernet/freescale/gianfar_driver.ko] undefined!
>
>
This can be avoided by adding 'depends on HAS_DMA'. We should probably just
fix m32r instead to not require that and allow building all drivers even when
they can't work in the end.
Arnd
^ permalink raw reply
* Re: [PATCH net-next v2 3/3] net: marvell: Allow drivers to be built with COMPILE_TEST
From: Arnd Bergmann @ 2016-11-17 9:12 UTC (permalink / raw)
To: kbuild test robot
Cc: Florian Fainelli, kbuild-all, netdev, davem, mw, gregory.clement,
Shaohui.Xie
In-Reply-To: <201611171559.suMIPVBq%fengguang.wu@intel.com>
On Thursday, November 17, 2016 3:10:10 PM CET kbuild test robot wrote:
> url: https://github.com/0day-ci/linux/commits/Florian-Fainelli/net-fsl-Allow-most-drivers-to-be-built-with-COMPILE_TEST/20161116-094841
> config: mips-allyesconfig (attached as .config)
> compiler: mips-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
> wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=mips
>
> All errors (new ones prefixed by >>):
>
> drivers/built-in.o: In function `mvneta_bm_pool_create':
> >> mvneta_bm.c:(.text+0x772f0c): undefined reference to `mvebu_mbus_get_dram_win_info'
We should fix this, I'd suggest either adding an inline wrapper for
mvebu_mbus_get_dram_win_info() when CONFIG_MVEBU_MBUS is not set,
or allowing CONFIG_MVEBU_MBUS to be enabled for COMPILE_TEST as well.
Arnd
^ permalink raw reply
* Re: [PATCH 1/1] ixgbe: write flush vfta registers
From: zhuyj @ 2016-11-17 9:33 UTC (permalink / raw)
To: Lino Sanfilippo
Cc: e1000-devel@lists.sourceforge.net, netdev, intel-wired-lan
In-Reply-To: <trinity-ec3a2e6f-7310-41aa-8743-61bc62ccf663-1479305144399@3capp-gmx-bs59>
Sure. From the following.
"
VLAN Filter. Each bit ‘i’ in register ‘n’ affects packets with VLAN
tags equal to 32*n+i.
128 VLAN Filter registers compose a table of 4096 bits that cover all
possible VLAN
tags.
Each bit when set, enables packets with the associated VLAN tags to
pass. Each bit
when cleared, blocks packets with this VLAN tag.
"
Your suggestions seems reasonable. Please wait. I will make tests to
vefiry your suggestions.
I will keep you update.
On Wed, Nov 16, 2016 at 10:05 PM, Lino Sanfilippo <LinoSanfilippo@gmx.de> wrote:
>
>
> Hi,
>
>>
>> Sometimes vfta registers can not be written successfully in dcb mode.
>> This is very occassional. When the ixgbe nic runs for a very long time,
>> sometimes this bug occurs. But after IXGBE_WRITE_FLUSH is executed,
>> this bug never occurs.
>>
>> Signed-off-by: Zhu Yanjun <zyjzyj2000@gmail.com>
>> ---
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index bd93d82..1221cfb 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -4138,8 +4138,10 @@ static void ixgbe_vlan_promisc_enable(struct ixgbe_adapter *adapter)
>> }
>>
>> /* Set all bits in the VLAN filter table array */
>> - for (i = hw->mac.vft_size; i--;)
>> + for (i = hw->mac.vft_size; i--;) {
>> IXGBE_WRITE_REG(hw, IXGBE_VFTA(i), ~0U);
>> + IXGBE_WRITE_FLUSH(hw);
>> + }
>
> Should it not be sufficient to do the flush only once, at the end of the function?
>
> Regards,
> Lino
------------------------------------------------------------------------------
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* Re: [PATCH net-next v2 3/4] bpf, mlx5: drop priv->xdp_prog reference on netdev cleanup
From: Saeed Mahameed @ 2016-11-17 9:34 UTC (permalink / raw)
To: Daniel Borkmann
Cc: David S. Miller, Alexei Starovoitov, Brenden Blanco, zhiyisun,
Rana Shahout, Saeed Mahameed, Linux Netdev List
In-Reply-To: <582C817A.4030902@iogearbox.net>
On Wed, Nov 16, 2016 at 5:55 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 11/16/2016 04:45 PM, Daniel Borkmann wrote:
>>
>> On 11/16/2016 01:51 PM, Saeed Mahameed wrote:
>>>
>>> On Wed, Nov 16, 2016 at 2:04 AM, Daniel Borkmann <daniel@iogearbox.net>
>>> wrote:
>>>>
>>>> mlx5e_xdp_set() is currently the only place where we drop reference on
>>>> the
>>>> prog sitting in priv->xdp_prog when it's exchanged by a new one. We also
>>>> need to make sure that we eventually release that reference, for
>>>> example,
>>>> in case the netdev is dismantled.
>>>>
>>>> Fixes: 86994156c736 ("net/mlx5e: XDP fast RX drop bpf programs support")
>>>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>>>> ---
>>>> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>>>> b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>>>> index cf26672..60fe54c 100644
>>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>>>> @@ -3715,6 +3715,9 @@ static void mlx5e_nic_cleanup(struct mlx5e_priv
>>>> *priv)
>>>>
>>>> if (MLX5_CAP_GEN(mdev, vport_group_manager))
>>>> mlx5_eswitch_unregister_vport_rep(esw, 0);
>>>> +
>>>> + if (priv->xdp_prog)
>>>> + bpf_prog_put(priv->xdp_prog);
>>>> }
>>>
>>>
>>> I thought that on unregister_netdev ndo_xdp_set will be called with
>>> NULL prog to cleanup. like any other resources (Vlans/mac_lists/
>>> etc..), why xdp should be different ?
>>> Anyway if this is the case, I am ok with this fix, you can even send
>>> it to net (it looks like a serious leak).
>>
>>
>> The only interaction with ndo_xdp() right now is dev_change_xdp_fd()
>> and the currently a bit terse dump via rtnl_xdp_fill(). The latter
>> only tells whether something is actually attached and will have more
>> info in near future, but doesn't alter anything.
>>
>> dev_change_xdp_fd() is only triggered from user side via netlink when
>> IFLA_XDP container attr is around, so no automatic cleanup here. This
>> means as per documentation in enum xdp_netdev_command, that the driver
>> has full ownership, thus needs to bpf_prog_put().
>
>
> Note that without patch 2/4, just sending this one to net doesn't really
> solve anything, since there the mlx5e_xdp_set() still has the incorrect
> bpf_prog_add(prog, 1) around. So it's the whole series if so. I had it
> originally targeted at net, but Alexei suggested net-next; I don't really
> mind either way, so I agreed to go for net-next.
Ok, Thank you Daniel
^ permalink raw reply
* Re: [PATCH net 1/7] net: ethernet: ti: cpsw: fix bad register access in probe error path
From: Johan Hovold @ 2016-11-17 9:41 UTC (permalink / raw)
To: Grygorii Strashko
Cc: Johan Hovold, Mugunthan V N, linux-omap, netdev, linux-kernel
In-Reply-To: <664c1f10-dd31-8566-852f-ee5ca0b5c46e@ti.com>
On Wed, Nov 16, 2016 at 02:33:18PM -0600, Grygorii Strashko wrote:
> On 11/16/2016 08:35 AM, Johan Hovold wrote:
> > Make sure to resume the platform device to enable clocks before
> > accessing the CPSW registers in the probe error path (e.g. for deferred
> > probe).
> >
> > Unhandled fault: external abort on non-linefetch (0x1008) at 0xd0872d08
> > ...
> > [<c04fabcc>] (cpsw_ale_control_set) from [<c04fb8b4>] (cpsw_ale_destroy+0x2c/0x44)
> > [<c04fb8b4>] (cpsw_ale_destroy) from [<c04fea58>] (cpsw_probe+0xbd0/0x10c4)
> > [<c04fea58>] (cpsw_probe) from [<c047b2a0>] (platform_drv_probe+0x5c/0xc0)
> >
> > Note that in the unlikely event of a runtime-resume failure, we'll leak
> > the ale struct.
> >
> > Fixes: df828598a755 ("netdev: driver: ethernet: Add TI CPSW driver")
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> > drivers/net/ethernet/ti/cpsw.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> > index c6cff3d2ff05..5bc5e6189661 100644
> > --- a/drivers/net/ethernet/ti/cpsw.c
> > +++ b/drivers/net/ethernet/ti/cpsw.c
> > @@ -2818,7 +2818,12 @@ static int cpsw_probe(struct platform_device *pdev)
> > return 0;
> >
> > clean_ale_ret:
> > - cpsw_ale_destroy(cpsw->ale);
> > + if (pm_runtime_get_sync(&pdev->dev) < 0) {
> > + pm_runtime_put_noidle(&pdev->dev);
> > + } else {
> > + cpsw_ale_destroy(cpsw->ale);
> > + pm_runtime_put_sync(&pdev->dev);
> > + }
> > clean_dma_ret:
> > cpdma_ctlr_destroy(cpsw->dma);
> > clean_runtime_disable_ret:
> >
>
> I think, wouldn't it be logically more simple to just keep CPSW PM
> runtime enabled during probe?
Indeed it would. I'll do that in a v2.
Thanks,
Johan
^ permalink raw reply
* Re: [PATCH net-next v2 2/4] bpf, mlx5: fix various refcount issues in mlx5e_xdp_set
From: Saeed Mahameed @ 2016-11-17 9:48 UTC (permalink / raw)
To: Daniel Borkmann
Cc: David S. Miller, Alexei Starovoitov, Brenden Blanco, Zhiyi Sun,
Rana Shahout, Saeed Mahameed, Linux Netdev List
In-Reply-To: <582C7A8D.1020005@iogearbox.net>
On Wed, Nov 16, 2016 at 5:26 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 11/16/2016 03:30 PM, Saeed Mahameed wrote:
>>
>> On Wed, Nov 16, 2016 at 3:54 PM, Daniel Borkmann <daniel@iogearbox.net>
>> wrote:
>>>
>>> On 11/16/2016 01:25 PM, Saeed Mahameed wrote:
>>>>
>>>> On Wed, Nov 16, 2016 at 2:04 AM, Daniel Borkmann <daniel@iogearbox.net>
>>>> wrote:
>>>>>
>>>>>
>>>>> There are multiple issues in mlx5e_xdp_set():
>>>>>
>>>>> 1) The batched bpf_prog_add() is currently not checked for errors! When
>>>>> doing so, it should be done at an earlier point in time to makes
>>>>> sure
>>>>> that we cannot fail anymore at the time we want to set the program
>>>>> for
>>>>> each channel. This only means that we have to undo the
>>>>> bpf_prog_add()
>>>>> in case we return due to required reset.
>>>>>
>>>>> 2) When swapping the priv->xdp_prog, then no extra reference count must
>>>>> be
>>>>> taken since we got that from call path via dev_change_xdp_fd()
>>>>> already.
>>>>> Otherwise, we'd never be able to free the program. Also,
>>>>> bpf_prog_add()
>>>>> without checking the return code could fail.
>>>>>
>>>>> Fixes: 86994156c736 ("net/mlx5e: XDP fast RX drop bpf programs
>>>>> support")
>>>>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>>>>> ---
>>>>> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 25
>>>>> ++++++++++++++++++-----
>>>>> 1 file changed, 20 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>>>>> b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>>>>> index ab0c336..cf26672 100644
>>>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>>>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>>>>> @@ -3142,6 +3142,17 @@ static int mlx5e_xdp_set(struct net_device
>>>>> *netdev, struct bpf_prog *prog)
>>>>> goto unlock;
>>>>> }
>>>>>
>>>>> + if (prog) {
>>>>> + /* num_channels is invariant here, so we can take the
>>>>> + * batched reference right upfront.
>>>>> + */
>>>>> + prog = bpf_prog_add(prog, priv->params.num_channels);
>>>>> + if (IS_ERR(prog)) {
>>>>> + err = PTR_ERR(prog);
>>>>> + goto unlock;
>>>>> + }
>>>>> + }
>>>>> +
>>>>
>>>>
>>>> With this approach you will end up taking a ref count twice per RQ! on
>>>> the first time xdp_set is called i.e (old_prog = NULL, prog != NULL).
>>>> One ref will be taken per RQ/Channel from the above code, and since
>>>> reset will be TRUE mlx5e_open_locked will be called and another ref
>>>> count will be taken on mlx5e_create_rq.
>>>>
>>>> The issue here is that we have two places for ref count accounting,
>>>> xdp_set and mlx5e_create_rq. Having ref-count updates in
>>>> mlx5e_create_rq is essential for num_channels configuration changes
>>>> (mlx5e_set_ringparam).
>>>>
>>>> Our previous approach made sure that only one path will do the ref
>>>> counting (mlx5e_open_locked vs. mlx5e_xdp_set batch ref update when
>>>> reset == false).
>>>
>>>
>>> That is correct, for a short time bpf_prog_add() was charged also when
>>> we reset. When we reset, we will then jump to unlock_put and safely undo
>>> this since we took ref from mlx5e_create_rq() already in that case, and
>>> return successfully. That was intentional, so that eventually we end up
>>> just taking a /single/ ref per RQ when we exit mlx5e_xdp_set(), but more
>>> below ...
>>>
>>> [...]
>>>>
>>>>
>>>> 2. Keep the current approach and make sure to not update the ref count
>>>> twice, you can batch update only if (!reset && was_open) otherwise you
>>>> can rely on mlx5e_open_locked to take the num_channels ref count for
>>>> you.
>>>>
>>>> Personally I prefer option 2, since we will keep the current logic
>>>> which will allow configuration updates such as (mlx5e_set_ringparam)
>>>> to not worry about ref counting since it will be done in the reset
>>>> flow.
>>>
>>>
>>> ... agree on keeping current approach. I actually like the idea, so we'd
>>> end up with this simpler version for the batched ref then.
>>
>>
>> Great :).
>>
>> So let's do the batched update only if we are not going to reset (we
>> already know that in advance), instead of the current patch where you
>> batch update unconditionally and then
>> unlock_put in case reset was performed (which is just redundant and
>> confusing).
>>
>> Please note that if open_locked fails you need to goto unlock_put.
>
>
> Sorry, I'm not quite sure I follow you here; are you /now/ commenting on
> the original patch or on the already updated diff I did from my very last
> email, that is, http://patchwork.ozlabs.org/patch/695564/ ?
>
I am commenting on this patch "V2 2/4".
this patch will take batched ref count unconditionally and release the
extra refs "unlock_put" if reset was performed.
I am asking to take the batched ref only if we are not going to reset
in advance to save the extra "unlock_put"
>>> Note, your "bpf_prog_add(prog, 1) // one ref for the device." is not
>>> needed
>>> since this we already got that one through dev_change_xdp_fd() as
>>> mentioned.
>>
>>
>> If it is not needed then why we need bpf_prog_put on mlx5e_nic_cleanup
>> in your next patch? this doesn't look symmetric (right) !
>> you shouldn't release a reference you didn't take.
>> Overall with this series the driver can take num_channels refs and
>> will release num_channels refs on mlx5e_close. we shouldn't release
>> one extra ref on NIC cleanup.
>
>
> I already explained this in the commit description; when dev_change_xdp_fd()
> is called and sees a valid fd, it does bpf_prog_get_type(), which calls the
> __bpf_prog_get() taking a ref on the program (bpf_prog_inc()). That is then
> passed down to ops->ndo_xdp(). Only in case of error from the ->ndo_xdp()
> callback, we bpf_prog_put() this reference from dev_change_xdp_fd() side.
>
> For drivers that implement against this ndo, it means that you need N-1 refs
> in addition. Have a look at the other two in-tree users, which do it
> correctly,
> that is mlx4_xdp_set() and nfp_net_xdp_setup().
>
> It's documented here (include/linux/netdevice.h) with ndo_xdp referring to
> it:
>
> /* These structures hold the attributes of xdp state that are being passed
> * to the netdevice through the xdp op.
> */
> enum xdp_netdev_command {
> /* Set or clear a bpf program used in the earliest stages of packet
> * rx. The prog will have been loaded as BPF_PROG_TYPE_XDP. The
> callee
> * is responsible for calling bpf_prog_put on any old progs that are
> * stored. In case of error, the callee need not release the new
> prog
> * reference, but on success it takes ownership and must
> bpf_prog_put
> * when it is no longer used.
> */
> XDP_SETUP_PROG,
> [...]
> };
>
> I think reason why this is rather confusing would be that initially, it was
> just a single prog for all queues, but it was requested along the way to
> move
> prog pointer down to queues instead and let them have their own ref, so that
> some time in future individual progs from a subset of the queues can be
> exchanged.
>
> Since the way xdp in mlx5 is implemented, you currently have the
> priv->xdp_prog
> as the control prog. That's okay right now, but requires to drop the last
> ref
> on priv->xdp_prog via bpf_prog_put() when netdev is dismantled and
> priv->xdp_prog
> still present.
Got it, but i would rather make the stack cleanup those refs once it
receives an unregester_netdev event instead of counting on netdev to
do it, as i said before, like any other resource added to the netdev
by the stack (vlan/mac/etc..).
but this is a general discussion of xdp API, it won't block this series :).
Saeed.
^ permalink raw reply
* Re: [PATCH] net: dsa: fix fixed-link-phy device leaks
From: Johan Hovold @ 2016-11-17 9:52 UTC (permalink / raw)
To: Florian Fainelli
Cc: Johan Hovold, Andrew Lunn, Vivien Didelot, David S. Miller,
netdev, linux-kernel
In-Reply-To: <8387696c-813a-4c88-5f10-7d2e5fdae6b8@gmail.com>
On Wed, Nov 16, 2016 at 10:14:10AM -0800, Florian Fainelli wrote:
> On 11/16/2016 09:11 AM, Johan Hovold wrote:
> > On Wed, Nov 16, 2016 at 09:06:26AM -0800, Florian Fainelli wrote:
> >> On 11/16/2016 06:47 AM, Johan Hovold wrote:
> >>> Make sure to drop the reference taken by of_phy_find_device() when
> >>> registering and deregistering the fixed-link PHY-device.
> >>>
> >>> Note that we need to put both references held at deregistration.
> >>>
> >>> Fixes: 39b0c705195e ("net: dsa: Allow configuration of CPU & DSA port
> >>> speeds/duplex")
> >>> Signed-off-by: Johan Hovold <johan@kernel.org>
> >>> ---
> >>>
> >>> Hi,
> >>>
> >>> This is one has been compile tested only, but fixes a couple of leaks
> >>> similar to one that was found in the cpsw driver for which I just posted
> >>> a patch.
> >>>
> >>> It turns out all drivers but DSA fail to deregister the fixed-link PHYs
> >>> registered by of_phy_register_fixed_link(). Due to the way this
> >>> interface was designed, deregistering such a PHY is a bit cumbersome and
> >>> looks like it would benefit from a common helper.
> >>>
> >>> However, perhaps the interface should instead be changed so that the PHY
> >>> device is returned so that drivers do not need to use
> >>> of_phy_find_device() when they need to access properties of the fixed
> >>> link (e.g. as in dsu_cpu_dsa_setup below).
> >>>
> >>> Thoughts?
> >>>
> >>> Thanks,
> >>> Johan
> >>>
> >>>
> >>> net/dsa/dsa.c | 8 +++++++-
> >>> 1 file changed, 7 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> >>> index a6902c1e2f28..798a6a776a5f 100644
> >>> --- a/net/dsa/dsa.c
> >>> +++ b/net/dsa/dsa.c
> >>> @@ -233,6 +233,8 @@ int dsa_cpu_dsa_setup(struct dsa_switch *ds, struct device *dev,
> >>> genphy_read_status(phydev);
> >>> if (ds->ops->adjust_link)
> >>> ds->ops->adjust_link(ds, port, phydev);
> >>> +
> >>> + phy_device_free(phydev); /* of_phy_find_device */
> >>> }
> >>>
> >>> return 0;
> >>> @@ -509,8 +511,12 @@ void dsa_cpu_dsa_destroy(struct device_node *port_dn)
> >>> if (of_phy_is_fixed_link(port_dn)) {
> >>> phydev = of_phy_find_device(port_dn);
> >>> if (phydev) {
> >>> - phy_device_free(phydev);
> >>> fixed_phy_unregister(phydev);
> >>> + /* Put references taken by of_phy_find_device() and
> >>> + * of_phy_register_fixed_link().
> >>> + */
> >>> + phy_device_free(phydev);
> >>> + phy_device_free(phydev);
> >>
> >> Double free, this looks bogus here. Actually would not this mean a
> >> triple free since you already free in dsa_cpu_dsa_setup() which is
> >> paired with dsa_cpu_dsa_destroy()?
> >
> > The naming of phy_device_free() is unfortunate when it's really a put():
> >
> > void phy_device_free(struct phy_device *phydev)
> > {
> > put_device(&phydev->mdio.dev);
> > }
>
> Indeed, should have looked a little harder.
>
> >
> > which may need to be called multiple times, specifically after a call to
> > of_phy_find_device() which takes another reference.
> >
> > With this patch the refcounts are properly balanced.
>
> The intent of your patch is good, but it still feels like having to
> double imbalance the refcount is symptomatic of a larger issue here, it
> does not seem like having several refcounts are necessary, so we may
> really want to rework the API.
I'll cook something up for -next, but how about using
put_device(&phydev->mdio.dev)
for the reference taken by of_phy_find_device() (as some driver
already do) to fix the immediate leaks?
Then deregistration would look like:
phydev = of_phy_find_device(port_dn);
if (phydev) {
- phy_device_free(phydev);
fixed_phy_unregister(phydev);
+ put_device(&phydev->mdio.dev)
+ phy_device_free(phydev);
Thanks,
Johan
^ permalink raw reply
* Re: Cannot set IPv6 address
From: Saeed Mahameed @ 2016-11-17 9:55 UTC (permalink / raw)
To: Eric Dumazet; +Cc: david.lebrun, Linux Netdev List, Doron Tsur, Majd Dibbiny
In-Reply-To: <1479310198.8455.199.camel@edumazet-glaptop3.roam.corp.google.com>
On Wed, Nov 16, 2016 at 5:29 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2016-11-16 at 17:22 +0200, Saeed Mahameed wrote:
>> Hi David,
>>
>> The following commit introduced a new issue when setting IPv6 address
>> via the following command:
>>
>> /sbin/ip -6 addr add 2001:0db8:0:f112::1/64 dev enp2s2
>> RTNETLINK answers: Operation not supported
>>
>> Offending commit:
>>
>> commit 6c8702c60b88651072460f3f4026c7dfe2521d12
>> Author: David Lebrun <david.lebrun@uclouvain.be>
>> Date: Tue Nov 8 14:57:41 2016 +0100
>>
>> ipv6: sr: add support for SRH encapsulation and injection with lwtunnels
>>
>> This patch creates a new type of interfaceless lightweight tunnel (SEG6),
>> enabling the encapsulation and injection of SRH within locally emitted
>> packets and forwarded packets.
>>
>> >From a configuration viewpoint, a seg6 tunnel would be configured
>> as follows:
>>
>> ip -6 ro ad fc00::1/128 encap seg6 mode encap segs
>> fc42::1,fc42::2,fc42::3 dev eth0
>>
>> Any packet whose destination address is fc00::1 would thus be encapsulated
>> within an outer IPv6 header containing the SRH with three
>> segments, and would
>> actually be routed to the first segment of the list. If `mode inline' was
>> specified instead of `mode encap', then the SRH would be directly inserted
>> after the IPv6 header without outer encapsulation.
>>
>> The inline mode is only available if CONFIG_IPV6_SEG6_INLINE is
>> enabled. This
>> feature was made configurable because direct header insertion may break
>> several mechanisms such as PMTUD or IPSec AH.
>>
>> Signed-off-by: David Lebrun <david.lebrun@uclouvain.be>
>> Signed-off-by: David S. Miller <davem@davemloft.net>
>>
>>
>> Can you check ? Are we missing something here ?
>
> Sure, patch is under review. Please look at netdev archives and/or
> ozlabs
>
> https://patchwork.ozlabs.org/patch/695060/
>
>
Thank you Eric.
^ permalink raw reply
* Re: Cannot set IPv6 address
From: Saeed Mahameed @ 2016-11-17 10:01 UTC (permalink / raw)
To: David Lebrun; +Cc: Linux Netdev List, Doron Tsur, Majd Dibbiny
In-Reply-To: <582C7C92.2000306@uclouvain.be>
On Wed, Nov 16, 2016 at 5:34 PM, David Lebrun <david.lebrun@uclouvain.be> wrote:
> On 11/16/2016 04:22 PM, Saeed Mahameed wrote:
>> Hi David,
>>
>> The following commit introduced a new issue when setting IPv6 address
>> via the following command:
>>
>> /sbin/ip -6 addr add 2001:0db8:0:f112::1/64 dev enp2s2
>> RTNETLINK answers: Operation not supported
>>
>> Offending commit:
>>
>> commit 6c8702c60b88651072460f3f4026c7dfe2521d12
>
> Saeed,
>
> Do you have LWTUNNEL enabled ? This commit introduced a bug causing IPv6
Disabled.
> initialization to fail if LWTUNNEL is disabled. The patch has been
> submitted to the list and is pending approval from DaveM.
>
> If you see something like
>
> NET: Registered protocol family 10
> IPv6: Attempt to unregister permanent protocol 6
> IPv6: Attempt to unregister permanent protocol 136
> IPv6: Attempt to unregister permanent protocol 17
> NET: Unregistered protocol family 10
>
> in you dmesg logs then it would confirm my theory.
>
> Short fix: enable CONFIG_LWTUNNEL or apply patch in attachment
>
> David
Looks like the fix was applied [1],
Thanks for the quick response.
[1] https://patchwork.ozlabs.org/patch/695060/.
^ permalink raw reply
* Re: [PATCH 01/15] net: phy: Add phy_ethtool_nway_reset
From: Krzysztof Hałasa @ 2016-11-17 10:06 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev
In-Reply-To: <20161115180644.3941-2-f.fainelli@gmail.com>
Hi,
Florian Fainelli <f.fainelli@gmail.com> writes:
> This function just calls into genphy_restart_aneg() to perform an
> autonegotation restart.
>
> +int phy_ethtool_nway_reset(struct net_device *ndev)
> +{
> + struct phy_device *phydev = ndev->phydev;
> +
> + if (!phydev)
> + return -ENODEV;
> +
> + return genphy_restart_aneg(phydev);
> +}
and then you're using this function as .nway_reset of all netdev drivers
in question.
genphy_restart_aneg() deals directly with MII BMCR register:
int genphy_restart_aneg(struct phy_device *phydev)
{
int ctl = phy_read(phydev, MII_BMCR);
if (ctl < 0)
return ctl;
ctl |= BMCR_ANENABLE | BMCR_ANRESTART;
/* Don't isolate the PHY if we're negotiating */
ctl &= ~BMCR_ISOLATE;
return phy_write(phydev, MII_BMCR, ctl);
}
Will the above work if the PHY is something special, e.g. a "fixed"
connection, or some kind of fiber trx etc? Normally, the PHY driver
could provide its own config_aneg().
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
^ permalink raw reply
* RE: [PATCH net-next v3 4/7] vxlan: improve vxlan route lookup checks.
From: David Laight @ 2016-11-17 10:17 UTC (permalink / raw)
To: 'Jiri Benc', Pravin B Shelar; +Cc: netdev@vger.kernel.org
In-Reply-To: <20161115153934.363e176d@griffin>
From: Of Jiri Benc
> Sent: 15 November 2016 14:40
> On Sun, 13 Nov 2016 20:43:55 -0800, Pravin B Shelar wrote:
> > @@ -1929,8 +1951,8 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
> > union vxlan_addr *src;
> > struct vxlan_metadata _md;
> > struct vxlan_metadata *md = &_md;
> > - struct dst_entry *ndst = NULL;
> > __be16 src_port = 0, dst_port;
> > + struct dst_entry *ndst = NULL;
> > __be32 vni, label;
> > __be16 df = 0;
> > __u8 tos, ttl;
>
> This looks kind of arbitrary. You might want to remove this hunk or
> merge it to patch 3.
Worse than arbitrary, it adds 4 bytes of pad on 64bit systems.
David
^ permalink raw reply
* Re: [PATCH net 1/3] net: phy: realtek: add eee advertisement disable options
From: Jerome Brunet @ 2016-11-17 10:20 UTC (permalink / raw)
To: Anand Moon
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, devicetree, Florian Fainelli,
Alexandre TORGUE, Neil Armstrong, Martin Blumenstingl,
Kevin Hilman, Linux Kernel, Andre Roth,
linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Carlo Caione,
Giuseppe Cavallaro, linux-arm-kernel
In-Reply-To: <CANAwSgTjG8G0U+A1p7hOKND9rjY4BzZfPgyWWHAEryYkHj_UOw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, 2016-11-16 at 22:36 +0530, Anand Moon wrote:
> Hi Jerome.
>
> On 15 November 2016 at 19:59, Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> wrote:
> >
> > On some platforms, energy efficient ethernet with rtl8211 devices
> > is
> > causing issue, like throughput drop or broken link.
> >
> > This was reported on the OdroidC2 (DWMAC + RTL8211F). While the
> > issue root
> > cause is not fully understood yet, disabling EEE advertisement
> > prevent auto
> > negotiation from enabling EEE.
> >
> > This patch provides options to disable 1000T and 100TX EEE
> > advertisement
> > individually for the realtek phys supporting this feature.
> >
> > Reported-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23myhRSP0FMvGiw@public.gmane.org
> > m>
> > Cc: Giuseppe Cavallaro <peppe.cavallaro-qxv4g6HH51o@public.gmane.org>
> > Cc: Alexandre TORGUE <alexandre.torgue-qxv4g6HH51o@public.gmane.org>
> > Signed-off-by: Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Neil Armstrong <narmstrong-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> > Tested-by: Andre Roth <neolynx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> > drivers/net/phy/realtek.c | 65
> > ++++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 64 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > index aadd6e9f54ad..77235fd5faaf 100644
> > --- a/drivers/net/phy/realtek.c
> > +++ b/drivers/net/phy/realtek.c
> > @@ -15,6 +15,12 @@
> > */
> > #include <linux/phy.h>
> > #include <linux/module.h>
> > +#include <linux/of.h>
> > +
> > +struct rtl8211x_phy_priv {
> > + bool eee_1000t_disable;
> > + bool eee_100tx_disable;
> > +};
> >
> > #define RTL821x_PHYSR 0x11
> > #define RTL821x_PHYSR_DUPLEX 0x2000
> > @@ -93,12 +99,44 @@ static int rtl8211f_config_intr(struct
> > phy_device *phydev)
> > return err;
> > }
> >
> > +static void rtl8211x_clear_eee_adv(struct phy_device *phydev)
> > +{
> > + struct rtl8211x_phy_priv *priv = phydev->priv;
> > + u16 val;
> > +
> > + if (priv->eee_1000t_disable || priv->eee_100tx_disable) {
> > + val = phy_read_mmd_indirect(phydev,
> > MDIO_AN_EEE_ADV,
> > + MDIO_MMD_AN);
> > +
> > + if (priv->eee_1000t_disable)
> > + val &= ~MDIO_AN_EEE_ADV_1000T;
> > + if (priv->eee_100tx_disable)
> > + val &= ~MDIO_AN_EEE_ADV_100TX;
> > +
> > + phy_write_mmd_indirect(phydev, MDIO_AN_EEE_ADV,
> > + MDIO_MMD_AN, val);
> > + }
> > +}
> > +
> > +static int rtl8211x_config_init(struct phy_device *phydev)
> > +{
> > + int ret;
> > +
> > + ret = genphy_config_init(phydev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + rtl8211x_clear_eee_adv(phydev);
> > +
> > + return 0;
> > +}
> > +
> > static int rtl8211f_config_init(struct phy_device *phydev)
> > {
> > int ret;
> > u16 reg;
> >
> > - ret = genphy_config_init(phydev);
> > + ret = rtl8211x_config_init(phydev);
> > if (ret < 0)
> > return ret;
> >
> > @@ -115,6 +153,26 @@ static int rtl8211f_config_init(struct
> > phy_device *phydev)
> > return 0;
> > }
> >
> > +static int rtl8211x_phy_probe(struct phy_device *phydev)
> > +{
> > + struct device *dev = &phydev->mdio.dev;
> > + struct device_node *of_node = dev->of_node;
> > + struct rtl8211x_phy_priv *priv;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->eee_1000t_disable =
> > + of_property_read_bool(of_node, "realtek,disable-
> > eee-1000t");
> > + priv->eee_100tx_disable =
> > + of_property_read_bool(of_node, "realtek,disable-
> > eee-100tx");
> > +
> > + phydev->priv = priv;
> > +
> > + return 0;
> > +}
> > +
> > static struct phy_driver realtek_drvs[] = {
> > {
> > .phy_id = 0x00008201,
> > @@ -140,7 +198,9 @@ static struct phy_driver realtek_drvs[] = {
> > .phy_id_mask = 0x001fffff,
> > .features = PHY_GBIT_FEATURES,
> > .flags = PHY_HAS_INTERRUPT,
> > + .probe = &rtl8211x_phy_probe,
> > .config_aneg = genphy_config_aneg,
> > + .config_init = &rtl8211x_config_init,
> > .read_status = genphy_read_status,
> > .ack_interrupt = rtl821x_ack_interrupt,
> > .config_intr = rtl8211e_config_intr,
> > @@ -152,7 +212,9 @@ static struct phy_driver realtek_drvs[] = {
> > .phy_id_mask = 0x001fffff,
> > .features = PHY_GBIT_FEATURES,
> > .flags = PHY_HAS_INTERRUPT,
> > + .probe = &rtl8211x_phy_probe,
> > .config_aneg = &genphy_config_aneg,
> > + .config_init = &rtl8211x_config_init,
> > .read_status = &genphy_read_status,
> > .ack_interrupt = &rtl821x_ack_interrupt,
> > .config_intr = &rtl8211e_config_intr,
> > @@ -164,6 +226,7 @@ static struct phy_driver realtek_drvs[] = {
> > .phy_id_mask = 0x001fffff,
> > .features = PHY_GBIT_FEATURES,
> > .flags = PHY_HAS_INTERRUPT,
> > + .probe = &rtl8211x_phy_probe,
> > .config_aneg = &genphy_config_aneg,
> > .config_init = &rtl8211f_config_init,
> > .read_status = &genphy_read_status,
> > --
> > 2.7.4
> >
>
> How about adding callback functionality for .soft_reset to handle
> BMCR
> where we update the Auto-Negotiation for the phy,
> as per the datasheet of the rtl8211f.
I'm not sure I understand how this would help with our issue (and EEE).
Am I missing something or is it something unrelated that you would like
to see happening on this driver ?
>
> -Best Regard
> Anand Moon
>
> >
> >
> > _______________________________________________
> > linux-amlogic mailing list
> > linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > http://lists.infradead.org/mailman/listinfo/linux-amlogic
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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: mlx4 BUG_ON in probe path
From: Yishai Hadas @ 2016-11-17 10:22 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Yishai Hadas, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Johannes Thumshirn,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161116182527.GC26600-1RhO1Y9PlrlHTL0Zs8A6p5iNqAH0jzoTYJqu5kTmcBRl57MIdRCFDg@public.gmane.org>
On 11/16/2016 8:25 PM, Bjorn Helgaas wrote:
> Hi Yishai,
>
> Johannes has been working on an mlx4 initialization problem on an
> IBM x3850 X6. The underlying problem is a PCI core issue -- we're
> setting RCB in the Mellanox device, which means it thinks it can
> generate 128-byte Completions, even though the Root Port above it
> can't handle them. That issue is
> https://bugzilla.kernel.org/show_bug.cgi?id=187781
>
> The machine crashed when this happened, apparently not because of any
> error reported via AER, but because mlx4 contains a BUG_ON, probably
> the one in mlx4_enter_error_state().
>
> That one happens if pci_channel_offline() returns false. Is this
> telling us about a problem in PCI error handling, or is it just a case
> where mlx4 isn't as smart as it could be?
Yes, we expect at that step a problem/bug in the PCI layer that should
be fixed (e.g. reporting online but really is offline, etc.), can you
please evaluate and confirm that ?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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
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