* Re: [iproute PATCH v3 0/6] Covscan: Misc fixes
From: Stephen Hemminger @ 2017-08-24 22:30 UTC (permalink / raw)
To: Phil Sutter; +Cc: netdev
In-Reply-To: <20170824094131.2963-1-phil@nwl.cc>
On Thu, 24 Aug 2017 11:41:25 +0200
Phil Sutter <phil@nwl.cc> wrote:
> This series collects patches from v1 addressing miscellaneous issues
> detected by covscan.
>
> Changes since v2:
> - Dropped patch 1 since v2 discussion is still inconclusive.
> - Replaced patch 2 by a more appropriate one given feedback from v2.
>
> Phil Sutter (6):
> ss: Make struct tcpstat fields 'timer' and 'timeout' unsigned
> ss: Make sure scanned index value to unix_state_map is sane
> netem/maketable: Check return value of fscanf()
> lib/bpf: Check return value of write()
> lib/fs: Fix and simplify make_path()
> lib/libnetlink: Don't pass NULL parameter to memcpy()
>
> lib/bpf.c | 3 ++-
> lib/fs.c | 20 +++++---------------
> lib/libnetlink.c | 6 ++++--
> misc/ss.c | 11 +++++------
> netem/maketable.c | 4 ++--
> 5 files changed, 18 insertions(+), 26 deletions(-)
>
Applied this group
^ permalink raw reply
* Re: [iproute PATCH v3 6/6] lib/libnetlink: Don't pass NULL parameter to memcpy()
From: Stephen Hemminger @ 2017-08-24 22:29 UTC (permalink / raw)
To: Phil Sutter; +Cc: netdev
In-Reply-To: <20170824094131.2963-7-phil@nwl.cc>
On Thu, 24 Aug 2017 11:41:31 +0200
Phil Sutter <phil@nwl.cc> wrote:
> Both addattr_l() and rta_addattr_l() may be called with NULL data
> pointer and 0 alen parameters. Avoid calling memcpy() in that case.
>
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
> lib/libnetlink.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lib/libnetlink.c b/lib/libnetlink.c
> index 874e660be7eb4..fbe719ee10449 100644
> --- a/lib/libnetlink.c
> +++ b/lib/libnetlink.c
> @@ -871,7 +871,8 @@ int addattr_l(struct nlmsghdr *n, int maxlen, int type, const void *data,
> rta = NLMSG_TAIL(n);
> rta->rta_type = type;
> rta->rta_len = len;
> - memcpy(RTA_DATA(rta), data, alen);
> + if (alen)
> + memcpy(RTA_DATA(rta), data, alen);
> n->nlmsg_len = NLMSG_ALIGN(n->nlmsg_len) + RTA_ALIGN(len);
> return 0;
> }
> @@ -958,7 +959,8 @@ int rta_addattr_l(struct rtattr *rta, int maxlen, int type,
> subrta = (struct rtattr *)(((char *)rta) + RTA_ALIGN(rta->rta_len));
> subrta->rta_type = type;
> subrta->rta_len = len;
> - memcpy(RTA_DATA(subrta), data, alen);
> + if (alen)
> + memcpy(RTA_DATA(subrta), data, alen);
> rta->rta_len = NLMSG_ALIGN(rta->rta_len) + RTA_ALIGN(len);
> return 0;
> }
Ok, applied. You never know when GCC language experts might decide
to exploit undefined behavior.
^ permalink raw reply
* [PATCH net] net: systemport: Be drop monitor friendly
From: Florian Fainelli @ 2017-08-24 22:20 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, Florian Fainelli
Utilize dev_consume_skb_any(cb->skb) in bcm_sysport_free_cb() which is
used when a TX packet is completed, as well as when the RX ring is
cleaned on shutdown. None of these two cases are packet drops, so be
drop monitor friendly.
Suggested-by: Eric Dumazet <edumazet@gmail.com>
Fixes: 80105befdb4b ("net: systemport: add Broadcom SYSTEMPORT Ethernet MAC driver")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/broadcom/bcmsysport.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index dc3052751bc1..e6add99cc31c 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -597,7 +597,7 @@ static int bcm_sysport_set_coalesce(struct net_device *dev,
static void bcm_sysport_free_cb(struct bcm_sysport_cb *cb)
{
- dev_kfree_skb_any(cb->skb);
+ dev_consume_skb_any(cb->skb);
cb->skb = NULL;
dma_unmap_addr_set(cb, dma_addr, 0);
}
--
2.9.3
^ permalink raw reply related
* Re: [iproute PATCH v4 0/6] Covscan: Fixes for string termination
From: Stephen Hemminger @ 2017-08-24 22:16 UTC (permalink / raw)
To: Phil Sutter; +Cc: netdev
In-Reply-To: <20170824095150.5469-1-phil@nwl.cc>
On Thu, 24 Aug 2017 11:51:44 +0200
Phil Sutter <phil@nwl.cc> wrote:
> This series collects patches from v1 dealing with code potentially
> leaving string buffers unterminated. This does not include situations
> where it happens for parsed interface names since an overall solution
> was attempted for that and it's state is still unclear due to lack of
> feedback from upstream.
>
> Changes since v3:
> - Dropped patch 2 since upstream discussion in v3 is not conclusive yet.
>
> Phil Sutter (6):
> ipntable: Avoid memory allocation for filter.name
> lib/fs: Fix format string in find_fs_mount()
> lib/inet_proto: Review inet_proto_{a2n,n2a}()
> lnstat_util: Simplify alloc_and_open() a bit
> tc/m_xt: Fix for potential string buffer overflows
> lib/ll_map: Choose size of new cache items at run-time
>
> ip/ipntable.c | 6 +++---
> lib/fs.c | 2 +-
> lib/inet_proto.c | 24 +++++++++++++-----------
> lib/ll_map.c | 4 ++--
> misc/lnstat_util.c | 7 ++-----
> tc/m_xt.c | 7 ++++---
> 6 files changed, 25 insertions(+), 25 deletions(-)
>
Applied.
^ permalink raw reply
* Re: [PATCH net-next 09/13] net: mvpp2: dynamic reconfiguration of the PHY mode
From: Russell King - ARM Linux @ 2017-08-24 22:16 UTC (permalink / raw)
To: Andrew Lunn
Cc: Antoine Tenart, davem, kishon, jason, sebastian.hesselbarth,
gregory.clement, thomas.petazzoni, nadavh, linux-kernel, mw,
stefanc, miquel.raynal, netdev
In-Reply-To: <20170824174519.GC18700@lunn.ch>
On Thu, Aug 24, 2017 at 07:45:19PM +0200, Andrew Lunn wrote:
> > The 88x3310 driver in the kernel knows about these combinations and
> > sets the phy interface parameter correctly depending on whether the
> > PHY has configured itself for copper at whatever speed or SFP+.
>
> So when the PHY decides to swap from copper to fibre etc, is the
> phylib state machine kept up to date. Does it see a down, followed by
> an up?
I'd have to re-check to make sure, but I believe it does, because the
negotiation is held off on the "other" media until the currently active
link has gone down.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply
* Re: [PATCH v4] net: sunrpc: svcsock: fix NULL-pointer exception
From: J. Bruce Fields @ 2017-08-24 22:13 UTC (permalink / raw)
To: Jeff Layton
Cc: Vadim Lomovtsev, trond.myklebust-7I+n7zu2hftEKMMhf/gKZA,
anna.schumaker-HgOvQuBEEgTQT0dZR+AlfA,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
pabeni-H+wXaHxf7aLQT0dZR+AlfA
In-Reply-To: <1503484422.5294.0.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Wed, Aug 23, 2017 at 06:33:42AM -0400, Jeff Layton wrote:
> I think this one looks fine. Bruce, do you mind picking this one up? It
> might also be reasonable for stable...
Yep, thanks to you both, I'll plan to send a pull request tonight or
tomorrow.
--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [Patch net-next] net_sched: remove tc class reference counting
From: Cong Wang @ 2017-08-24 22:09 UTC (permalink / raw)
To: David Miller; +Cc: Linux Kernel Network Developers, Jamal Hadi Salim
In-Reply-To: <20170824.123220.2250866539237700218.davem@davemloft.net>
On Thu, Aug 24, 2017 at 12:32 PM, David Miller <davem@davemloft.net> wrote:
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Tue, 22 Aug 2017 21:38:10 -0700
>
>> For TC classes, their ->get() and ->put() are always paired, and the
>> reference counting is completely useless, because:
>>
>> 1) For class modification and dumping paths, we already hold RTNL lock,
>> so all of these ->get(),->change(),->put() are atomic.
>>
>> 2) For filter bindiing/unbinding, we use other reference counter than
>> this one, and they should have RTNL lock too.
>>
>> 3) For ->qlen_notify(), it is special because it is called on ->enqueue()
>> path, but we already hold qdisc tree lock there, and we hold this
>> tree lock when graft or delete the class too, so it should not be gone
>> or changed until we release the tree lock.
>>
>> Therefore, this patch removes ->get() and ->put(), but:
>>
>> 1) Adds a new ->find() to find the pointer to a class by classid, no
>> refcnt.
>>
>> 2) Move the original class destroy upon the last refcnt into ->delete(),
>> right after releasing tree lock. This is fine because the class is
>> already removed from hash when holding the lock.
>>
>> For those who also use ->put() as ->unbind(), just rename them to reflect
>> this change.
>>
>> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>
> This one looks good to me.
>
> Jamal, please give it a quick look over and review.
>
Hold on, I spot a problem near ->delete(), need a fix like
I did for tc actions...
The reason why I didn't catch it in my test is actually we
don't notify delete event for classes when deleting the
whole tree.
Anyway, v2 is coming.
^ permalink raw reply
* [PATCH] cfg80211: Use tabs for identation
From: Himanshu Jha @ 2017-08-24 22:02 UTC (permalink / raw)
To: johannes; +Cc: davem, linux-wireless, netdev, linux-kernel, Himanshu Jha
Removed whitespaces and added necessary tabs conforming to coding style.
Issue found using checkpatch.pl
Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com>
---
net/wireless/chan.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/wireless/chan.c b/net/wireless/chan.c
index b8aa5a7..13c95e5 100644
--- a/net/wireless/chan.c
+++ b/net/wireless/chan.c
@@ -984,9 +984,9 @@ int cfg80211_set_monitor_channel(struct cfg80211_registered_device *rdev,
void
cfg80211_get_chan_state(struct wireless_dev *wdev,
- struct ieee80211_channel **chan,
- enum cfg80211_chan_mode *chanmode,
- u8 *radar_detect)
+ struct ieee80211_channel **chan,
+ enum cfg80211_chan_mode *chanmode,
+ u8 *radar_detect)
{
int ret;
--
2.7.4
^ permalink raw reply related
* Re: [iproute PATCH v4 0/4] Covscan: Fix potential NULL pointer dereferences
From: Stephen Hemminger @ 2017-08-24 21:51 UTC (permalink / raw)
To: Phil Sutter; +Cc: netdev
In-Reply-To: <20170824094634.4093-1-phil@nwl.cc>
On Thu, 24 Aug 2017 11:46:30 +0200
Phil Sutter <phil@nwl.cc> wrote:
> This series collects patches from v1 which eliminate possible cases of
> NULL pointer dereferences.
>
> Changes since v3:
> - Dropped upstream rejected patch 2.
>
> Phil Sutter (4):
> ifstat, nstat: Check fdopen() return value
> tc/q_netem: Don't dereference possibly NULL pointer
> tc/tc_filter: Make sure filter name is not empty
> tipc/bearer: Prevent NULL pointer dereference
>
> misc/ifstat.c | 16 +++++++++++-----
> misc/nstat.c | 16 +++++++++++-----
> tc/q_netem.c | 3 ++-
> tc/tc_filter.c | 3 +++
> tipc/bearer.c | 2 +-
> 5 files changed, 28 insertions(+), 12 deletions(-)
>
Applied. Thanks
^ permalink raw reply
* Re: [PATCH net] virtio_net: be drop monitor friend
From: Michael S. Tsirkin @ 2017-08-24 21:42 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Jason Wang
In-Reply-To: <1503590569.2499.81.camel@edumazet-glaptop3.roam.corp.google.com>
On Thu, Aug 24, 2017 at 09:02:49AM -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> This change is needed to not fool drop monitor.
> (perf record ... -e skb:kfree_skb )
>
> Packets were properly sent and are consumed after TX completion.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/net/virtio_net.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 98f17b05c68b..b06169ea60dc 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1058,7 +1058,7 @@ static void free_old_xmit_skbs(struct send_queue *sq)
> bytes += skb->len;
> packets++;
>
> - dev_kfree_skb_any(skb);
> + dev_consume_skb_any(skb);
> }
>
> /* Avoid overhead when no packets have been processed
>
^ permalink raw reply
* [PATCH] strparser: initialize all callbacks
From: Eric Biggers @ 2017-08-24 21:38 UTC (permalink / raw)
To: netdev
Cc: David S . Miller, linux-kernel, Eric Biggers, Dmitry Vyukov,
Tom Herbert
From: Eric Biggers <ebiggers@google.com>
commit bbb03029a899 ("strparser: Generalize strparser") added more
function pointers to 'struct strp_callbacks'; however, kcm_attach() was
not updated to initialize them. This could cause the ->lock() and/or
->unlock() function pointers to be set to garbage values, causing a
crash in strp_work().
Fix the bug by moving the callback structs into static memory, so
unspecified members are zeroed. Also constify them while we're at it.
This bug was found by syzkaller, which encountered the following splat:
IP: 0x55
PGD 3b1ca067
P4D 3b1ca067
PUD 3b12f067
PMD 0
Oops: 0010 [#1] SMP KASAN
Dumping ftrace buffer:
(ftrace buffer empty)
Modules linked in:
CPU: 2 PID: 1194 Comm: kworker/u8:1 Not tainted 4.13.0-rc4-next-20170811 #2
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Workqueue: kstrp strp_work
task: ffff88006bb0e480 task.stack: ffff88006bb10000
RIP: 0010:0x55
RSP: 0018:ffff88006bb17540 EFLAGS: 00010246
RAX: dffffc0000000000 RBX: ffff88006ce4bd60 RCX: 0000000000000000
RDX: 1ffff1000d9c97bd RSI: 0000000000000000 RDI: ffff88006ce4bc48
RBP: ffff88006bb17558 R08: ffffffff81467ab2 R09: 0000000000000000
R10: ffff88006bb17438 R11: ffff88006bb17940 R12: ffff88006ce4bc48
R13: ffff88003c683018 R14: ffff88006bb17980 R15: ffff88003c683000
FS: 0000000000000000(0000) GS:ffff88006de00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000055 CR3: 000000003c145000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
process_one_work+0xbf3/0x1bc0 kernel/workqueue.c:2098
worker_thread+0x223/0x1860 kernel/workqueue.c:2233
kthread+0x35e/0x430 kernel/kthread.c:231
ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:431
Code: Bad RIP value.
RIP: 0x55 RSP: ffff88006bb17540
CR2: 0000000000000055
---[ end trace f0e4920047069cee ]---
Here is a C reproducer (requires CONFIG_BPF_SYSCALL=y and
CONFIG_AF_KCM=y):
#include <linux/bpf.h>
#include <linux/kcm.h>
#include <linux/types.h>
#include <stdint.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#include <sys/syscall.h>
#include <unistd.h>
static const struct bpf_insn bpf_insns[3] = {
{ .code = 0xb7 }, /* BPF_MOV64_IMM(0, 0) */
{ .code = 0x95 }, /* BPF_EXIT_INSN() */
};
static const union bpf_attr bpf_attr = {
.prog_type = 1,
.insn_cnt = 2,
.insns = (uintptr_t)&bpf_insns,
.license = (uintptr_t)"",
};
int main(void)
{
int bpf_fd = syscall(__NR_bpf, BPF_PROG_LOAD,
&bpf_attr, sizeof(bpf_attr));
int inet_fd = socket(AF_INET, SOCK_STREAM, 0);
int kcm_fd = socket(AF_KCM, SOCK_DGRAM, 0);
ioctl(kcm_fd, SIOCKCMATTACH,
&(struct kcm_attach) { .fd = inet_fd, .bpf_fd = bpf_fd });
}
Fixes: bbb03029a899 ("strparser: Generalize strparser")
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Tom Herbert <tom@quantonium.net>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
Documentation/networking/strparser.txt | 2 +-
include/net/strparser.h | 2 +-
kernel/bpf/sockmap.c | 10 +++++-----
net/kcm/kcmsock.c | 11 +++++------
net/strparser/strparser.c | 2 +-
5 files changed, 13 insertions(+), 14 deletions(-)
diff --git a/Documentation/networking/strparser.txt b/Documentation/networking/strparser.txt
index fe01302471ae..13081b3decef 100644
--- a/Documentation/networking/strparser.txt
+++ b/Documentation/networking/strparser.txt
@@ -35,7 +35,7 @@ Functions
=========
strp_init(struct strparser *strp, struct sock *sk,
- struct strp_callbacks *cb)
+ const struct strp_callbacks *cb)
Called to initialize a stream parser. strp is a struct of type
strparser that is allocated by the upper layer. sk is the TCP
diff --git a/include/net/strparser.h b/include/net/strparser.h
index 4fe966a0ad92..7dc131d62ad5 100644
--- a/include/net/strparser.h
+++ b/include/net/strparser.h
@@ -138,7 +138,7 @@ void strp_done(struct strparser *strp);
void strp_stop(struct strparser *strp);
void strp_check_rcv(struct strparser *strp);
int strp_init(struct strparser *strp, struct sock *sk,
- struct strp_callbacks *cb);
+ const struct strp_callbacks *cb);
void strp_data_ready(struct strparser *strp);
int strp_process(struct strparser *strp, struct sk_buff *orig_skb,
unsigned int orig_offset, size_t orig_len,
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 78b2bb9370ac..617c239590c2 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -368,12 +368,12 @@ static int smap_read_sock_done(struct strparser *strp, int err)
static int smap_init_sock(struct smap_psock *psock,
struct sock *sk)
{
- struct strp_callbacks cb;
+ static const struct strp_callbacks cb = {
+ .rcv_msg = smap_read_sock_strparser,
+ .parse_msg = smap_parse_func_strparser,
+ .read_sock_done = smap_read_sock_done,
+ };
- memset(&cb, 0, sizeof(cb));
- cb.rcv_msg = smap_read_sock_strparser;
- cb.parse_msg = smap_parse_func_strparser;
- cb.read_sock_done = smap_read_sock_done;
return strp_init(&psock->strp, sk, &cb);
}
diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
index 88ce73288247..48e993b2dbcf 100644
--- a/net/kcm/kcmsock.c
+++ b/net/kcm/kcmsock.c
@@ -1376,7 +1376,11 @@ static int kcm_attach(struct socket *sock, struct socket *csock,
struct kcm_psock *psock = NULL, *tpsock;
struct list_head *head;
int index = 0;
- struct strp_callbacks cb;
+ static const struct strp_callbacks cb = {
+ .rcv_msg = kcm_rcv_strparser,
+ .parse_msg = kcm_parse_func_strparser,
+ .read_sock_done = kcm_read_sock_done,
+ };
int err;
csk = csock->sk;
@@ -1391,11 +1395,6 @@ static int kcm_attach(struct socket *sock, struct socket *csock,
psock->sk = csk;
psock->bpf_prog = prog;
- cb.rcv_msg = kcm_rcv_strparser;
- cb.abort_parser = NULL;
- cb.parse_msg = kcm_parse_func_strparser;
- cb.read_sock_done = kcm_read_sock_done;
-
err = strp_init(&psock->strp, csk, &cb);
if (err) {
kmem_cache_free(kcm_psockp, psock);
diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
index 434aa6637a52..d4ea46a5f233 100644
--- a/net/strparser/strparser.c
+++ b/net/strparser/strparser.c
@@ -472,7 +472,7 @@ static void strp_sock_unlock(struct strparser *strp)
}
int strp_init(struct strparser *strp, struct sock *sk,
- struct strp_callbacks *cb)
+ const struct strp_callbacks *cb)
{
if (!cb || !cb->rcv_msg || !cb->parse_msg)
--
2.14.1.342.g6490525c54-goog
^ permalink raw reply related
* Re: [PATCH][next] net: hinic: fix comparison of a uint16_t type with -1
From: David Miller @ 2017-08-24 21:32 UTC (permalink / raw)
To: colin.king; +Cc: aviad.krawczyk, netdev, kernel-janitors, linux-kernel
In-Reply-To: <20170823153936.22857-1-colin.king@canonical.com>
From: Colin King <colin.king@canonical.com>
Date: Wed, 23 Aug 2017 16:39:36 +0100
> From: Colin Ian King <colin.king@canonical.com>
>
> The comparison of hw_ioctxt.rx_buf_sz_idx == -1 is always false because
> rx_buf_sz_idx is a uint16_t. Fix this by explicitly casting -1 to uint16_t.
>
> Detected by CoverityScan, CID#1454559 ("Operands don't affect result")
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c b/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c
> index 09dec6de8dd5..71e26070fb7f 100644
> --- a/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c
> +++ b/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c
> @@ -352,7 +352,7 @@ static int set_hw_ioctxt(struct hinic_hwdev *hwdev, unsigned int rq_depth,
> }
> }
>
> - if (hw_ioctxt.rx_buf_sz_idx == -1)
> + if (hw_ioctxt.rx_buf_sz_idx == (uint16_t)-1)
> return -EINVAL;
>
> hw_ioctxt.sq_depth = ilog2(sq_depth);
This is really silly.
The code in question is trying to convert a size (HINIC_RX_BUF_SZ)
into a table index, using a loop.
It should just compute this at compile time and do away with this
overly confusing code.
Even a switch statement would be much better and the compiler
would optimize the whole away into a single assignment in
the generated code.
^ permalink raw reply
* [PATCH] staging: rtlwifi: Improve debugging by using debugfs
From: Larry Finger @ 2017-08-24 21:28 UTC (permalink / raw)
To: gregkh
Cc: netdev, devel, Larry Finger, Ping-Ke Shih, Yan-Hsuan Chuang,
Birming Chiu, Shaofu, Steven Ting
The changes in this commit are also being sent to the main rtlwifi
drivers in wireless-next; however, these changes will also be useful for
any debugging of r8822be before it gets moved into the main tree.
Use debugfs to dump register and btcoex status, and also write registers
and h2c.
We create topdir in /sys/kernel/debug/rtlwifi/, and use the MAC address
as subdirectory with several entries to dump mac_reg, bb_reg, rf_reg etc.
An example is
/sys/kernel/debug/rtlwifi/00-11-22-33-44-55-66/mac_0
This change permits examination of device registers in a dynamic manner,
a feature not available with the current debug mechanism.
We use seq_file to replace RT_TRACE to dump status, then we can use 'cat'
to access btcoex's status through debugfs.
(i.e. /sys/kernel/debug/rtlwifi/00-11-22-33-44-55-66/btcoex)
Other related changes are
1. implement btc_disp_dbg_msg() to access btcoex's common status.
2. remove obsolete field bt_exist
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Yan-Hsuan Chuang <yhchuang@realtek.com>
Cc: Birming Chiu <birming@realtek.com>
Cc: Shaofu <shaofu@realtek.com>
Cc: Steven Ting <steventing@realtek.com>
---
drivers/staging/rtlwifi/debug.c | 226 ++++++++++++++++++++++++----------------
1 file changed, 135 insertions(+), 91 deletions(-)
diff --git a/drivers/staging/rtlwifi/debug.c b/drivers/staging/rtlwifi/debug.c
index b9fd47aeaa9b..7446d71c41d1 100644
--- a/drivers/staging/rtlwifi/debug.c
+++ b/drivers/staging/rtlwifi/debug.c
@@ -80,9 +80,11 @@ void _rtl_dbg_print_data(struct rtl_priv *rtlpriv, u64 comp, int level,
}
}
-struct rtl_debgufs_priv {
+struct rtl_debugfs_priv {
struct rtl_priv *rtlpriv;
- int (*cb)(struct seq_file *m, void *v);
+ int (*cb_read)(struct seq_file *m, void *v);
+ ssize_t (*cb_write)(struct file *filp, const char __user *buffer,
+ size_t count, loff_t *loff);
u32 cb_data;
};
@@ -90,9 +92,9 @@ static struct dentry *debugfs_topdir;
static int rtl_debug_get_common(struct seq_file *m, void *v)
{
- struct rtl_debgufs_priv *debugfs_priv = m->private;
+ struct rtl_debugfs_priv *debugfs_priv = m->private;
- return debugfs_priv->cb(m, v);
+ return debugfs_priv->cb_read(m, v);
}
static int dl_debug_open_common(struct inode *inode, struct file *file)
@@ -109,7 +111,7 @@ static const struct file_operations file_ops_common = {
static int rtl_debug_get_mac_page(struct seq_file *m, void *v)
{
- struct rtl_debgufs_priv *debugfs_priv = m->private;
+ struct rtl_debugfs_priv *debugfs_priv = m->private;
struct rtl_priv *rtlpriv = debugfs_priv->rtlpriv;
u32 page = debugfs_priv->cb_data;
int i, n;
@@ -126,8 +128,8 @@ static int rtl_debug_get_mac_page(struct seq_file *m, void *v)
}
#define RTL_DEBUG_IMPL_MAC_SERIES(page, addr) \
-struct rtl_debgufs_priv rtl_debug_priv_mac_ ##page = { \
- .cb = rtl_debug_get_mac_page, \
+struct rtl_debugfs_priv rtl_debug_priv_mac_ ##page = { \
+ .cb_read = rtl_debug_get_mac_page, \
.cb_data = addr, \
}
@@ -150,7 +152,7 @@ RTL_DEBUG_IMPL_MAC_SERIES(17, 0x1700);
static int rtl_debug_get_bb_page(struct seq_file *m, void *v)
{
- struct rtl_debgufs_priv *debugfs_priv = m->private;
+ struct rtl_debugfs_priv *debugfs_priv = m->private;
struct rtl_priv *rtlpriv = debugfs_priv->rtlpriv;
struct ieee80211_hw *hw = rtlpriv->hw;
u32 page = debugfs_priv->cb_data;
@@ -168,8 +170,8 @@ static int rtl_debug_get_bb_page(struct seq_file *m, void *v)
}
#define RTL_DEBUG_IMPL_BB_SERIES(page, addr) \
-struct rtl_debgufs_priv rtl_debug_priv_bb_ ##page = { \
- .cb = rtl_debug_get_bb_page, \
+struct rtl_debugfs_priv rtl_debug_priv_bb_ ##page = { \
+ .cb_read = rtl_debug_get_bb_page, \
.cb_data = addr, \
}
@@ -192,7 +194,7 @@ RTL_DEBUG_IMPL_BB_SERIES(1f, 0x1f00);
static int rtl_debug_get_reg_rf(struct seq_file *m, void *v)
{
- struct rtl_debgufs_priv *debugfs_priv = m->private;
+ struct rtl_debugfs_priv *debugfs_priv = m->private;
struct rtl_priv *rtlpriv = debugfs_priv->rtlpriv;
struct ieee80211_hw *hw = rtlpriv->hw;
enum radio_path rfpath = debugfs_priv->cb_data;
@@ -215,8 +217,8 @@ static int rtl_debug_get_reg_rf(struct seq_file *m, void *v)
}
#define RTL_DEBUG_IMPL_RF_SERIES(page, addr) \
-struct rtl_debgufs_priv rtl_debug_priv_rf_ ##page = { \
- .cb = rtl_debug_get_reg_rf, \
+struct rtl_debugfs_priv rtl_debug_priv_rf_ ##page = { \
+ .cb_read = rtl_debug_get_reg_rf, \
.cb_data = addr, \
}
@@ -225,7 +227,7 @@ RTL_DEBUG_IMPL_RF_SERIES(b, RF90_PATH_B);
static int rtl_debug_get_cam_register(struct seq_file *m, void *v)
{
- struct rtl_debgufs_priv *debugfs_priv = m->private;
+ struct rtl_debugfs_priv *debugfs_priv = m->private;
struct rtl_priv *rtlpriv = debugfs_priv->rtlpriv;
int start = debugfs_priv->cb_data;
u32 target_cmd = 0;
@@ -270,8 +272,8 @@ static int rtl_debug_get_cam_register(struct seq_file *m, void *v)
}
#define RTL_DEBUG_IMPL_CAM_SERIES(page, addr) \
-struct rtl_debgufs_priv rtl_debug_priv_cam_ ##page = { \
- .cb = rtl_debug_get_cam_register, \
+struct rtl_debugfs_priv rtl_debug_priv_cam_ ##page = { \
+ .cb_read = rtl_debug_get_cam_register, \
.cb_data = addr, \
}
@@ -281,7 +283,7 @@ RTL_DEBUG_IMPL_CAM_SERIES(3, 22);
static int rtl_debug_get_btcoex(struct seq_file *m, void *v)
{
- struct rtl_debgufs_priv *debugfs_priv = m->private;
+ struct rtl_debugfs_priv *debugfs_priv = m->private;
struct rtl_priv *rtlpriv = debugfs_priv->rtlpriv;
if (rtlpriv->cfg->ops->get_btc_status())
@@ -293,8 +295,8 @@ static int rtl_debug_get_btcoex(struct seq_file *m, void *v)
return 0;
}
-static struct rtl_debgufs_priv rtl_debug_priv_btcoex = {
- .cb = rtl_debug_get_btcoex,
+static struct rtl_debugfs_priv rtl_debug_priv_btcoex = {
+ .cb_read = rtl_debug_get_btcoex,
.cb_data = 0,
};
@@ -302,18 +304,15 @@ static ssize_t rtl_debugfs_set_write_reg(struct file *filp,
const char __user *buffer,
size_t count, loff_t *loff)
{
- struct rtl_debgufs_priv *debugfs_priv = filp->private_data;
+ struct rtl_debugfs_priv *debugfs_priv = filp->private_data;
struct rtl_priv *rtlpriv = debugfs_priv->rtlpriv;
- struct ieee80211_hw *hw = rtlpriv->hw;
char tmp[32 + 1];
int tmp_len;
u32 addr, val, len;
int num;
- if (count < 3) {
- /*printk("argument size is less than 3\n");*/
+ if (count < 3)
return -EFAULT;
- }
tmp_len = (count > sizeof(tmp) - 1 ? sizeof(tmp) - 1 : count);
@@ -322,57 +321,11 @@ static ssize_t rtl_debugfs_set_write_reg(struct file *filp,
tmp[tmp_len] = '\0';
- /* write H2C */
- if (!strncmp(tmp, "h2c", 3)) {
- u8 h2c_len, h2c_data_packed[8];
- int h2c_data[8]; /* idx 0: cmd */
- int i;
-
- h2c_len = sscanf(tmp + 3, "%X %X %X %X %X %X %X %X",
- &h2c_data[0], &h2c_data[1],
- &h2c_data[2], &h2c_data[3],
- &h2c_data[4], &h2c_data[5],
- &h2c_data[6], &h2c_data[7]);
-
- if (h2c_len <= 0)
- return count;
-
- for (i = 0; i < h2c_len; i++)
- h2c_data_packed[i] = (u8)h2c_data[i];
-
- rtlpriv->cfg->ops->fill_h2c_cmd(hw, h2c_data_packed[0],
- h2c_len - 1,
- &h2c_data_packed[1]);
-
- return count;
- }
-
- /* write RF register */
- if (!strncmp(tmp, "rf", 2)) {
- int path;
- u32 addr, bitmask, data;
-
- num = sscanf(tmp + 2, "%X %X %X %X",
- &path, &addr, &bitmask, &data);
-
- if (num != 4) {
- RT_TRACE(rtlpriv, COMP_ERR, DBG_DMESG,
- "Format is <path> <addr> <mask> <data>\n");
- return count;
- }
-
- rtl_set_rfreg(hw, path, addr, bitmask, data);
-
- return count;
- }
-
/* write BB/MAC register */
- num = sscanf(tmp, "%x %x %x", &addr, &val, &len);
+ num = sscanf(tmp, "%x %x %x", &addr, &val, &len);
- if (num != 3) {
- /*printk("invalid write_reg parameter!\n");*/
+ if (num != 3)
return count;
- }
switch (len) {
case 1:
@@ -392,27 +345,115 @@ static ssize_t rtl_debugfs_set_write_reg(struct file *filp,
return count;
}
-static int rtl_debugfs_open(struct inode *inode, struct file *filp)
+static struct rtl_debugfs_priv rtl_debug_priv_write_reg = {
+ .cb_write = rtl_debugfs_set_write_reg,
+};
+
+static ssize_t rtl_debugfs_set_write_h2c(struct file *filp,
+ const char __user *buffer,
+ size_t count, loff_t *loff)
{
- filp->private_data = inode->i_private;
+ struct rtl_debugfs_priv *debugfs_priv = filp->private_data;
+ struct rtl_priv *rtlpriv = debugfs_priv->rtlpriv;
+ struct ieee80211_hw *hw = rtlpriv->hw;
+ char tmp[32 + 1];
+ int tmp_len;
+ u8 h2c_len, h2c_data_packed[8];
+ int h2c_data[8]; /* idx 0: cmd */
+ int i;
- return 0;
+ if (count < 3)
+ return -EFAULT;
+
+ tmp_len = (count > sizeof(tmp) - 1 ? sizeof(tmp) - 1 : count);
+
+ if (!buffer || copy_from_user(tmp, buffer, tmp_len))
+ return count;
+
+ tmp[tmp_len] = '\0';
+
+ h2c_len = sscanf(tmp, "%X %X %X %X %X %X %X %X",
+ &h2c_data[0], &h2c_data[1],
+ &h2c_data[2], &h2c_data[3],
+ &h2c_data[4], &h2c_data[5],
+ &h2c_data[6], &h2c_data[7]);
+
+ if (h2c_len <= 0)
+ return count;
+
+ for (i = 0; i < h2c_len; i++)
+ h2c_data_packed[i] = (u8)h2c_data[i];
+
+ rtlpriv->cfg->ops->fill_h2c_cmd(hw, h2c_data_packed[0],
+ h2c_len - 1,
+ &h2c_data_packed[1]);
+
+ return count;
+}
+
+static struct rtl_debugfs_priv rtl_debug_priv_write_h2c = {
+ .cb_write = rtl_debugfs_set_write_h2c,
+};
+
+static ssize_t rtl_debugfs_set_write_rfreg(struct file *filp,
+ const char __user *buffer,
+ size_t count, loff_t *loff)
+{
+ struct rtl_debugfs_priv *debugfs_priv = filp->private_data;
+ struct rtl_priv *rtlpriv = debugfs_priv->rtlpriv;
+ struct ieee80211_hw *hw = rtlpriv->hw;
+ char tmp[32 + 1];
+ int tmp_len;
+ int num;
+ int path;
+ u32 addr, bitmask, data;
+
+ if (count < 3)
+ return -EFAULT;
+
+ tmp_len = (count > sizeof(tmp) - 1 ? sizeof(tmp) - 1 : count);
+
+ if (!buffer || copy_from_user(tmp, buffer, tmp_len))
+ return count;
+
+ tmp[tmp_len] = '\0';
+
+ num = sscanf(tmp, "%X %X %X %X",
+ &path, &addr, &bitmask, &data);
+
+ if (num != 4) {
+ RT_TRACE(rtlpriv, COMP_ERR, DBG_DMESG,
+ "Format is <path> <addr> <mask> <data>\n");
+ return count;
+ }
+
+ rtl_set_rfreg(hw, path, addr, bitmask, data);
+
+ return count;
}
+static struct rtl_debugfs_priv rtl_debug_priv_write_rfreg = {
+ .cb_write = rtl_debugfs_set_write_rfreg,
+};
+
static int rtl_debugfs_close(struct inode *inode, struct file *filp)
{
return 0;
}
-static struct rtl_debgufs_priv rtl_debug_priv_write_reg = {
- .cb = NULL,
- .cb_data = 0,
-};
+static ssize_t rtl_debugfs_common_write(struct file *filp,
+ const char __user *buffer,
+ size_t count, loff_t *loff)
+{
+ struct rtl_debugfs_priv *debugfs_priv = filp->private_data;
+
+ return debugfs_priv->cb_write(filp, buffer, count, loff);
+}
-static const struct file_operations file_ops_write_reg = {
+static const struct file_operations file_ops_common_write = {
.owner = THIS_MODULE,
- .write = rtl_debugfs_set_write_reg,
- .open = rtl_debugfs_open,
+ .write = rtl_debugfs_common_write,
+ .open = simple_open,
.release = rtl_debugfs_close,
};
@@ -420,7 +461,7 @@ static ssize_t rtl_debugfs_phydm_cmd(struct file *filp,
const char __user *buffer,
size_t count, loff_t *loff)
{
- struct rtl_debgufs_priv *debugfs_priv = filp->private_data;
+ struct rtl_debugfs_priv *debugfs_priv = filp->private_data;
struct rtl_priv *rtlpriv = debugfs_priv->rtlpriv;
char tmp[64];
@@ -444,7 +485,7 @@ static ssize_t rtl_debugfs_phydm_cmd(struct file *filp,
static int rtl_debug_get_phydm_cmd(struct seq_file *m, void *v)
{
- struct rtl_debgufs_priv *debugfs_priv = m->private;
+ struct rtl_debugfs_priv *debugfs_priv = m->private;
struct rtl_priv *rtlpriv = debugfs_priv->rtlpriv;
if (rtlpriv->dbg.msg_buf)
@@ -456,7 +497,7 @@ static int rtl_debug_get_phydm_cmd(struct seq_file *m, void *v)
static int rtl_debugfs_open_rw(struct inode *inode, struct file *filp)
{
if (filp->f_mode & FMODE_READ)
- single_open(filp, rtl_debug_get_phydm_cmd, inode->i_private);
+ single_open(filp, rtl_debug_get_common, inode->i_private);
else
filp->private_data = inode->i_private;
@@ -471,18 +512,19 @@ static int rtl_debugfs_close_rw(struct inode *inode, struct file *filp)
return 0;
}
-static struct rtl_debgufs_priv rtl_debug_priv_phydm_cmd = {
- .cb = NULL,
+static struct rtl_debugfs_priv rtl_debug_priv_phydm_cmd = {
+ .cb_read = rtl_debug_get_phydm_cmd,
+ .cb_write = rtl_debugfs_phydm_cmd,
.cb_data = 0,
};
-static const struct file_operations file_ops_phydm_cmd = {
+static const struct file_operations file_ops_common_rw = {
.owner = THIS_MODULE,
.open = rtl_debugfs_open_rw,
.release = rtl_debugfs_close_rw,
.read = seq_read,
.llseek = seq_lseek,
- .write = rtl_debugfs_phydm_cmd,
+ .write = rtl_debugfs_common_write,
};
#define RTL_DEBUGFS_ADD_CORE(name, mode, fopname) \
@@ -499,9 +541,9 @@ static const struct file_operations file_ops_phydm_cmd = {
#define RTL_DEBUGFS_ADD(name) \
RTL_DEBUGFS_ADD_CORE(name, S_IFREG | 0444, common)
#define RTL_DEBUGFS_ADD_W(name) \
- RTL_DEBUGFS_ADD_CORE(name, S_IFREG | 0222, write_reg)
+ RTL_DEBUGFS_ADD_CORE(name, S_IFREG | 0222, common_write)
#define RTL_DEBUGFS_ADD_RW(name) \
- RTL_DEBUGFS_ADD_CORE(name, S_IFREG | 0666, phydm_cmd)
+ RTL_DEBUGFS_ADD_CORE(name, S_IFREG | 0666, common_rw)
void rtl_debug_add_one(struct ieee80211_hw *hw)
{
@@ -565,6 +607,8 @@ void rtl_debug_add_one(struct ieee80211_hw *hw)
RTL_DEBUGFS_ADD(btcoex);
RTL_DEBUGFS_ADD_W(write_reg);
+ RTL_DEBUGFS_ADD_W(write_h2c);
+ RTL_DEBUGFS_ADD_W(write_rfreg);
RTL_DEBUGFS_ADD_RW(phydm_cmd);
}
--
2.12.3
^ permalink raw reply related
* Re: [PATCH net-next 1/3] gre: refactor the gre_fb_xmit
From: David Miller @ 2017-08-24 21:16 UTC (permalink / raw)
To: u9012063; +Cc: netdev
In-Reply-To: <1503500157-3717-1-git-send-email-u9012063@gmail.com>
Please repost this series with a proper "[PATCH net-next 0/3] ..." header
posting.
Thank you.
^ permalink raw reply
* Re: [PATCH net-next 2/5] ipv6: sr: add support for encapsulation of L2 frames
From: David Miller @ 2017-08-24 21:15 UTC (permalink / raw)
To: david.lebrun; +Cc: netdev
In-Reply-To: <20170823153203.11951-3-david.lebrun@uclouvain.be>
From: David Lebrun <david.lebrun@uclouvain.be>
Date: Wed, 23 Aug 2017 17:32:00 +0200
> + case SEG6_IPTUN_MODE_L2ENCAP:
> + head = sizeof(struct ipv6hdr) + 14;
> + break;
This 14 is ETH_HLEN.
>
> + if (pskb_expand_head(skb, skb->mac_len, 0, GFP_ATOMIC) < 0)
> + return -ENOMEM;
But you seem to support arbitrary L2 MAC header sizes.
Please sort this out.
^ permalink raw reply
* Re: [PATCH] tipc: Fix tipc_sk_reinit handling of -EAGAIN
From: David Miller @ 2017-08-24 21:02 UTC (permalink / raw)
To: rpeterso; +Cc: herbert, psutter, netdev, linux-kernel
In-Reply-To: <25145358.1155254.1503499382947.JavaMail.zimbra@redhat.com>
From: Bob Peterson <rpeterso@redhat.com>
Date: Wed, 23 Aug 2017 10:43:02 -0400 (EDT)
> In 9dbbfb0ab6680c6a85609041011484e6658e7d3c function tipc_sk_reinit
> had additional logic added to loop in the event that function
> rhashtable_walk_next() returned -EAGAIN. No worries.
>
> However, if rhashtable_walk_start returns -EAGAIN, it does "continue",
> and therefore skips the call to rhashtable_walk_stop(). That has
> the effect of calling rcu_read_lock() without its paired call to
> rcu_read_unlock(). Since rcu_read_lock() may be nested, the problem
> may not be apparent for a while, especially since resize events may
> be rare. But the comments to rhashtable_walk_start() state:
>
> * ...Note that we take the RCU lock in all
> * cases including when we return an error. So you must always call
> * rhashtable_walk_stop to clean up.
>
> This patch replaces the continue with a goto and label to ensure a
> matching call to rhashtable_walk_stop().
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* Re: [PATCH] [net-next] qlge: avoid memcpy buffer overflow
From: David Miller @ 2017-08-24 21:01 UTC (permalink / raw)
To: arnd
Cc: harish.patil, manish.chopra, Dept-GELinuxNICDev, stable, keescook,
gustavo, netdev, linux-kernel
In-Reply-To: <20170823135958.1379527-1-arnd@arndb.de>
From: Arnd Bergmann <arnd@arndb.de>
Date: Wed, 23 Aug 2017 15:59:49 +0200
> gcc-8.0.0 (snapshot) points out that we copy a variable-length string
> into a fixed length field using memcpy() with the destination length,
> and that ends up copying whatever follows the string:
>
> inlined from 'ql_core_dump' at drivers/net/ethernet/qlogic/qlge/qlge_dbg.c:1106:2:
> drivers/net/ethernet/qlogic/qlge/qlge_dbg.c:708:2: error: 'memcpy' reading 15 bytes from a region of size 14 [-Werror=stringop-overflow=]
> memcpy(seg_hdr->description, desc, (sizeof(seg_hdr->description)) - 1);
>
> Changing it to use strncpy() will instead zero-pad the destination,
> which seems to be the right thing to do here.
>
> The bug is probably harmless, but it seems like a good idea to address
> it in stable kernels as well, if only for the purpose of building with
> gcc-8 without warnings.
>
> Cc: stable@vger.kernel.org
Please don't use explicit stable CC:'ing for networking changes.
> Fixes: a61f80261306 ("qlge: Add ethtool register dump function.")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Applied to 'net' and queued up for -stable, thanks.
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Michael S. Tsirkin @ 2017-08-24 20:50 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Koichiro Den, Jason Wang, virtualization, Network Development
In-Reply-To: <CAF=yD-+9Ah8pC9i2w3Ad3WnhQit7Yo479pMrToty7priL6BFLw@mail.gmail.com>
On Thu, Aug 24, 2017 at 04:20:39PM -0400, Willem de Bruijn wrote:
> >> Traffic shaping can introduce msec timescale latencies.
> >>
> >> The delay may actually be a useful signal. If the guest does not
> >> orphan skbs early, TSQ will throttle the socket causing host
> >> queue build up.
> >>
> >> But, if completions are queued in-order, unrelated flows may be
> >> throttled as well. Allowing out of order completions would resolve
> >> this HoL blocking.
> >
> > We can allow out of order, no guests that follow virtio spec
> > will break. But this won't help in all cases
> > - a single slow flow can occupy the whole ring, you will not
> > be able to make any new buffers available for the fast flow
> > - what host considers a single flow can be multiple flows for guest
> >
> > There are many other examples.
>
> These examples are due to exhaustion of the fixed ubuf_info pool,
> right?
No - the ring size itself.
> We could use dynamic allocation or a resizable pool if these
> issues are serious enough.
We need some kind of limit on how many requests a guest can queue in the
host, or it's an obvious DoS attack vector. We used the ring size for
that.
> >> > Neither
> >> > do I see why would using tx interrupts within guest be a work around -
> >> > AFAIK windows driver uses tx interrupts.
> >>
> >> It does not address completion latency itself. What I meant was
> >> that in an interrupt-driver model, additional starvation issues,
> >> such as the potential deadlock raised at the start of this thread,
> >> or the timer delay observed before packets were orphaned in
> >> virtio-net in commit b0c39dbdc204, are mitigated.
> >>
> >> Specifically, it breaks the potential deadlock where sockets are
> >> blocked waiting for completions (to free up budget in sndbuf, tsq, ..),
> >> yet completion handling is blocked waiting for a new packet to
> >> trigger free_old_xmit_skbs from start_xmit.
> >
> > This talk of potential deadlock confuses me - I think you mean we would
> > deadlock if we did not orphan skbs in !use_napi - is that right? If you
> > mean that you can drop skb orphan and this won't lead to a deadlock if
> > free skbs upon a tx interrupt, I agree, for sure.
>
> Yes, that is what I meant.
>
> >> >> That is the only thing keeping us from removing the HoL blocking in vhost-net zerocopy.
> >> >
> >> > We don't enable network watchdog on virtio but we could and maybe
> >> > should.
> >>
> >> Can you elaborate?
> >
> > The issue is that holding onto buffers for very long times makes guests
> > think they are stuck. This is funamentally because from guest point of
> > view this is a NIC, so it is supposed to transmit things out in
> > a timely manner. If host backs the virtual NIC by something that is not
> > a NIC, with traffic shaping etc introducing unbounded latencies,
> > guest will be confused.
>
> That assumes that guests are fragile in this regard. A linux guest
> does not make such assumptions.
Yes it does. Examples above:
> > - a single slow flow can occupy the whole ring, you will not
> > be able to make any new buffers available for the fast flow
> > - what host considers a single flow can be multiple flows for guest
it's easier to see if you enable the watchdog timer for virtio. Linux
supports that.
> There are NICs with hardware
> rate limiting, so I'm not sure how much of a leap host os rate
> limiting is.
I don't know what happens if these NICs hold onto packets for seconds.
--
MST
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Willem de Bruijn @ 2017-08-24 20:20 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Koichiro Den, Jason Wang, virtualization, Network Development
In-Reply-To: <20170824160748-mutt-send-email-mst@kernel.org>
>> Traffic shaping can introduce msec timescale latencies.
>>
>> The delay may actually be a useful signal. If the guest does not
>> orphan skbs early, TSQ will throttle the socket causing host
>> queue build up.
>>
>> But, if completions are queued in-order, unrelated flows may be
>> throttled as well. Allowing out of order completions would resolve
>> this HoL blocking.
>
> We can allow out of order, no guests that follow virtio spec
> will break. But this won't help in all cases
> - a single slow flow can occupy the whole ring, you will not
> be able to make any new buffers available for the fast flow
> - what host considers a single flow can be multiple flows for guest
>
> There are many other examples.
These examples are due to exhaustion of the fixed ubuf_info pool,
right? We could use dynamic allocation or a resizable pool if these
issues are serious enough.
>> > Neither
>> > do I see why would using tx interrupts within guest be a work around -
>> > AFAIK windows driver uses tx interrupts.
>>
>> It does not address completion latency itself. What I meant was
>> that in an interrupt-driver model, additional starvation issues,
>> such as the potential deadlock raised at the start of this thread,
>> or the timer delay observed before packets were orphaned in
>> virtio-net in commit b0c39dbdc204, are mitigated.
>>
>> Specifically, it breaks the potential deadlock where sockets are
>> blocked waiting for completions (to free up budget in sndbuf, tsq, ..),
>> yet completion handling is blocked waiting for a new packet to
>> trigger free_old_xmit_skbs from start_xmit.
>
> This talk of potential deadlock confuses me - I think you mean we would
> deadlock if we did not orphan skbs in !use_napi - is that right? If you
> mean that you can drop skb orphan and this won't lead to a deadlock if
> free skbs upon a tx interrupt, I agree, for sure.
Yes, that is what I meant.
>> >> That is the only thing keeping us from removing the HoL blocking in vhost-net zerocopy.
>> >
>> > We don't enable network watchdog on virtio but we could and maybe
>> > should.
>>
>> Can you elaborate?
>
> The issue is that holding onto buffers for very long times makes guests
> think they are stuck. This is funamentally because from guest point of
> view this is a NIC, so it is supposed to transmit things out in
> a timely manner. If host backs the virtual NIC by something that is not
> a NIC, with traffic shaping etc introducing unbounded latencies,
> guest will be confused.
That assumes that guests are fragile in this regard. A linux guest
does not make such assumptions. There are NICs with hardware
rate limiting, so I'm not sure how much of a leap host os rate
limiting is.
^ permalink raw reply
* Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac
From: Florian Fainelli @ 2017-08-24 19:59 UTC (permalink / raw)
To: Corentin Labbe
Cc: Maxime Ripard, Chen-Yu Tsai, Andrew Lunn, Rob Herring,
Mark Rutland, Russell King, Giuseppe Cavallaro, Alexandre Torgue,
devicetree, linux-arm-kernel, linux-kernel, netdev
In-Reply-To: <20170824082124.GA14302@Red>
On 08/24/2017 01:21 AM, Corentin Labbe wrote:
> On Wed, Aug 23, 2017 at 09:31:53AM -0700, Florian Fainelli wrote:
>> On 08/23/2017 12:49 AM, Maxime Ripard wrote:
>>> Hi Florian,
>>>
>>> On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote:
>>>>>>> So I think what you are saying is either impossible or engineering-wise
>>>>>>> a very stupid design, like using an external MAC with a discrete PHY
>>>>>>> connected to the internal MAC's MDIO bus, while using the internal MAC
>>>>>>> with the internal PHY.
>>>>>>>
>>>>>>> Now can we please decide on something? We're a week and a half from
>>>>>>> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
>>>>>>> nodes (internal-mdio & external-mdio).
>>>>>>
>>>>>> I really don't see a need for a mdio-mux in the first place, just have
>>>>>> one MDIO controller (current state) sub-node which describes the
>>>>>> built-in STMMAC MDIO controller and declare the internal PHY as a child
>>>>>> node (along with 'phy-is-integrated'). If a different configuration is
>>>>>> used, then just put the external PHY as a child node there.
>>>>>>
>>>>>> If fixed-link is required, the mdio node becomes unused anyway.
>>>>>>
>>>>>> Works for everyone?
>>>>>
>>>>> If we put an external PHY with reg=1 as a child of internal MDIO,
>>>>> il will be merged with internal PHY node and get
>>>>> phy-is-integrated.
>>>>
>>>> Then have the .dtsi file contain just the mdio node, but no internal or
>>>> external PHY and push all the internal and external PHY node definition
>>>> (in its entirety) to the per-board DTS file, does not that work?
>>>
>>> If possible, I'd really like to have the internal PHY in the
>>> DTSI. It's always there in hardware anyway, and duplicating the PHY,
>>> with its clock, reset line, and whatever info we might need in the
>>> future in each and every board DTS that uses it will be very error
>>> prone and we will have the usual bunch of issues that come up with
>>> duplication.
>>
>> OK, then what if you put the internal PHY in the DTSI, mark it with a
>> status = "disabled" property, and have the per-board DTS put a status =
>> "okay" property along with a "phy-is-integrated" boolean property? Would
>> that work?
>
> No, I tested and for example with sun8i-h3-orangepi-plus.dts, the external PHY (ethernet-phy@1) is still merged.
Is not there is a mistake in the unit address not matching the "reg"
property, or am I not looking at the right tree?
&mdio {
ext_rgmii_phy: ethernet-phy@1 {
compatible = "ethernet-phy-ieee802.3-c22";
reg = <0>;
};
};
If the PHY is really at MDIO address 0, then it should be
ethernet-phy@0, and not ethernet-phy@1, and then no problem with the
merging?
> So that adding a 'status = "disabled"' does not bring anything.
>
>>
>> What I really don't think is necessary is:
>>
>> - duplicating the "mdio" controller node for external vs. internal PHY,
>> because this is not accurate, there is just one MDIO controller, but
>> there may be different kinds of MDIO/PHY devices attached
>
> For me, if we want to represent the reality, we need two MDIO:
> - since two PHY at the same address could co-exists
> - since they are isolated so not on the same MDIO bus
Is that really true? It might be, but from experience with e.g:
bcmgenet, the integrated PHY and the external PHYs are on the same MDIO
bus, which is convenient, except when you have an address conflict.
>
>> - having the STMMAC driver MDIO probing code having to deal with a
>> "mdio" sub-node or an "internal-mdio" sub-node because this is confusing
>> and requiring more driver-level changes that are error prone
>
> My patch for stmmac is really small, only the name of my variable ("need_mdio_mux_ids")
> have to be changed to something like "register_parent_mdio"
>
>
> So I agree with Maxime, we need to avoid merging PHY nodes, and we can avoid it only by having two separate MDIO nodes.
> Furthermore, with only one MDIO, we will face with lots of small patch for adding phy-is-integrated, with two we do not need to change any board DT, all is simply clean.
> Really having two MDIO seems cleaner.
The only valid thing that you have provided so far is this merging
problem. Anything else ranging from "we will face with lots of small
patch for adding phy-is-integrated" to "Really having two MDIO seems
cleaner." are hard to receive as technical arguments for correctness.
What happens if someone connects an external PHY at the same MDIO
address than the internal PHY, which one do you get responses from? If
you shutdown the internal PHY and it stops responding, then this
probably becomes deterministic, but it still supports the fact there is
just one MDIO bus controller per MAC.
Anyway, do whatever works best for you I guess.
--
Florian
--
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: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac
From: Corentin Labbe @ 2017-08-24 19:44 UTC (permalink / raw)
To: Florian Fainelli, maxime.ripard, andrew, wens
Cc: Rob Herring, Mark Rutland, Russell King, Giuseppe Cavallaro,
Alexandre Torgue, devicetree, linux-arm-kernel, linux-kernel,
netdev
In-Reply-To: <20170824082124.GA14302@Red>
On Thu, Aug 24, 2017 at 10:21:24AM +0200, Corentin Labbe wrote:
> On Wed, Aug 23, 2017 at 09:31:53AM -0700, Florian Fainelli wrote:
> > On 08/23/2017 12:49 AM, Maxime Ripard wrote:
> > > Hi Florian,
> > >
> > > On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote:
> > >>>>> So I think what you are saying is either impossible or engineering-wise
> > >>>>> a very stupid design, like using an external MAC with a discrete PHY
> > >>>>> connected to the internal MAC's MDIO bus, while using the internal MAC
> > >>>>> with the internal PHY.
> > >>>>>
> > >>>>> Now can we please decide on something? We're a week and a half from
> > >>>>> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
> > >>>>> nodes (internal-mdio & external-mdio).
> > >>>>
> > >>>> I really don't see a need for a mdio-mux in the first place, just have
> > >>>> one MDIO controller (current state) sub-node which describes the
> > >>>> built-in STMMAC MDIO controller and declare the internal PHY as a child
> > >>>> node (along with 'phy-is-integrated'). If a different configuration is
> > >>>> used, then just put the external PHY as a child node there.
> > >>>>
> > >>>> If fixed-link is required, the mdio node becomes unused anyway.
> > >>>>
> > >>>> Works for everyone?
> > >>>
> > >>> If we put an external PHY with reg=1 as a child of internal MDIO,
> > >>> il will be merged with internal PHY node and get
> > >>> phy-is-integrated.
> > >>
> > >> Then have the .dtsi file contain just the mdio node, but no internal or
> > >> external PHY and push all the internal and external PHY node definition
> > >> (in its entirety) to the per-board DTS file, does not that work?
> > >
> > > If possible, I'd really like to have the internal PHY in the
> > > DTSI. It's always there in hardware anyway, and duplicating the PHY,
> > > with its clock, reset line, and whatever info we might need in the
> > > future in each and every board DTS that uses it will be very error
> > > prone and we will have the usual bunch of issues that come up with
> > > duplication.
> >
> > OK, then what if you put the internal PHY in the DTSI, mark it with a
> > status = "disabled" property, and have the per-board DTS put a status =
> > "okay" property along with a "phy-is-integrated" boolean property? Would
> > that work?
>
> No, I tested and for example with sun8i-h3-orangepi-plus.dts, the external PHY (ethernet-phy@1) is still merged.
> So that adding a 'status = "disabled"' does not bring anything.
>
> >
> > What I really don't think is necessary is:
> >
> > - duplicating the "mdio" controller node for external vs. internal PHY,
> > because this is not accurate, there is just one MDIO controller, but
> > there may be different kinds of MDIO/PHY devices attached
>
> For me, if we want to represent the reality, we need two MDIO:
> - since two PHY at the same address could co-exists
> - since they are isolated so not on the same MDIO bus
>
> > - having the STMMAC driver MDIO probing code having to deal with a
> > "mdio" sub-node or an "internal-mdio" sub-node because this is confusing
> > and requiring more driver-level changes that are error prone
>
> My patch for stmmac is really small, only the name of my variable ("need_mdio_mux_ids")
> have to be changed to something like "register_parent_mdio"
>
>
> So I agree with Maxime, we need to avoid merging PHY nodes, and we can avoid it only by having two separate MDIO nodes.
> Furthermore, with only one MDIO, we will face with lots of small patch for adding phy-is-integrated, with two we do not need to change any board DT, all is simply clean.
> Really having two MDIO seems cleaner.
>
Hello
I have speaked with Neil Amstrong, and he said that they get the same problem on amlogic.
They use a mdio-mux-mmioreg, (see eth-phy-mux in arch/arm64/boot/dts/amlogic/meson-gxl.dtsi)
So tomorow, i will send a patch series which do the same with the exception that we need a mdio-mux-syscon (which is easy/simple to do).
Since their setup use stmmac, it means that we will need 0 changes on stmmac.
Regards
^ permalink raw reply
* Re: [PATCH net] virtio_net: be drop monitor friend
From: Eric Dumazet @ 2017-08-24 19:34 UTC (permalink / raw)
To: David Miller; +Cc: netdev, mst, jasowang
In-Reply-To: <20170824.115053.1345828961956619603.davem@davemloft.net>
On Thu, 2017-08-24 at 11:50 -0700, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Thu, 24 Aug 2017 09:02:49 -0700
>
> > From: Eric Dumazet <edumazet@google.com>
> >
> > This change is needed to not fool drop monitor.
> > (perf record ... -e skb:kfree_skb )
> >
> > Packets were properly sent and are consumed after TX completion.
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
>
> I'm pretty sure you meant "friendly" in the Subject line so I fixed
> it up to say that :-)
>
> Applied, thanks.
Ah, great, thanks for fixing this ;)
^ permalink raw reply
* Re: [PATCH] net/mlx5e: make mlx5e_profile const
From: David Miller @ 2017-08-24 19:33 UTC (permalink / raw)
To: bhumirks
Cc: julia.lawall, saeedm, matanb, leonro, netdev, linux-rdma,
linux-kernel
In-Reply-To: <1503492721-3187-1-git-send-email-bhumirks@gmail.com>
From: Bhumika Goyal <bhumirks@gmail.com>
Date: Wed, 23 Aug 2017 18:22:01 +0530
> Make this const as it is only passed as an argument to the function
> mlx5e_create_netdev and the corresponding argument is of type const.
>
> Signed-off-by: Bhumika Goyal <bhumirks@gmail.com>
Applied to net-next.
^ permalink raw reply
* Re: [PATCH] net/mlx4_core: make mlx4_profile const
From: David Miller @ 2017-08-24 19:33 UTC (permalink / raw)
To: bhumirks; +Cc: julia.lawall, tariqt, netdev, linux-rdma, linux-kernel
In-Reply-To: <1503492459-3110-1-git-send-email-bhumirks@gmail.com>
From: Bhumika Goyal <bhumirks@gmail.com>
Date: Wed, 23 Aug 2017 18:17:39 +0530
> Make these const as they are only used in a copy operation.
>
> Signed-off-by: Bhumika Goyal <bhumirks@gmail.com>
Applied to net-next.
^ permalink raw reply
* Re: [Patch net-next] net_sched: remove tc class reference counting
From: David Miller @ 2017-08-24 19:32 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev, jhs
In-Reply-To: <20170823043810.17602-1-xiyou.wangcong@gmail.com>
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue, 22 Aug 2017 21:38:10 -0700
> For TC classes, their ->get() and ->put() are always paired, and the
> reference counting is completely useless, because:
>
> 1) For class modification and dumping paths, we already hold RTNL lock,
> so all of these ->get(),->change(),->put() are atomic.
>
> 2) For filter bindiing/unbinding, we use other reference counter than
> this one, and they should have RTNL lock too.
>
> 3) For ->qlen_notify(), it is special because it is called on ->enqueue()
> path, but we already hold qdisc tree lock there, and we hold this
> tree lock when graft or delete the class too, so it should not be gone
> or changed until we release the tree lock.
>
> Therefore, this patch removes ->get() and ->put(), but:
>
> 1) Adds a new ->find() to find the pointer to a class by classid, no
> refcnt.
>
> 2) Move the original class destroy upon the last refcnt into ->delete(),
> right after releasing tree lock. This is fine because the class is
> already removed from hash when holding the lock.
>
> For those who also use ->put() as ->unbind(), just rename them to reflect
> this change.
>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
This one looks good to me.
Jamal, please give it a quick look over and review.
Thanks!
^ 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