* Re: [PATCH v5 2/3] net: add kcov handle to skb extensions
From: Johannes Berg @ 2020-11-21 19:30 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Florian Westphal, Ido Schimmel, Aleksandr Nogikh, davem, edumazet,
andreyknvl, dvyukov, elver, linux-kernel, netdev, linux-wireless,
willemdebruijn.kernel, Aleksandr Nogikh, Willem de Bruijn
In-Reply-To: <20201121103529.4b4acbff@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
On Sat, 2020-11-21 at 10:35 -0800, Jakub Kicinski wrote:
> On Sat, 21 Nov 2020 19:12:21 +0100 Johannes Berg wrote:
> > > So I'm leaning towards reverting the whole thing. You can attach
> > > kretprobes and record the information you need in BPF maps.
> >
> > I'm not going to object to reverting it (and perhaps redoing it better
> > later), but I will point out that kretprobe isn't going to work, you
> > eventually need kcov_remote_start() to be called in strategic points
> > before processing the skb after it bounced through the system.
> >
> > IOW, it's not really about serving userland, it's about enabling (and
> > later disabling) coverage collection for the bits of code it cares
> > about, mostly because collecting it for _everything_ is going to be too
> > slow and will mess up the data since for coverage guided fuzzing you
> > really need the reported coverage data to be only about the injected
> > fuzz data...
>
> All you need is make kcov_remote_start_common() be BPF-able, like
> the LSM hooks are now, right? And then BPF can return whatever handle
> it pleases.
Not sure I understand. Are you saying something should call
"kcov_remote_start_common()" with, say, the SKB, and leave it to a mass
of bpf hooks to figure out where the SKB got cloned or copied or
whatnot, track that in a map, and then ... no, wait, I don't really see
what you mean, sorry.
IIUC, fundamentally, you have this:
- at the beginning, a task is tagged with "please collect coverage
data for this handle"
- this task creates an SKB, etc, and all of the code that this task
executes is captured and the coverage data is reported
- However, the SKB traverses lots of things, gets copied, cloned, or
whatnot, and eventually leaves the annotated task, say for further
processing in softirq context or elsewhere.
Now since the whole point is to see what chaos this SKB created from
beginning (allocation) to end (free), since it was filled with fuzzed
data, you now have to figure out where to pick back up when the SKB is
processed further.
This is what the infrastructure was meant to solve. But note that the
SKB might be further cloned etc, so in order to track it you'd have to
(out-of-band) figure out all the possible places where it could
be reallocated, any time the skb pointer could change.
Then, when you know you've got interesting code on your hands, like in
mac80211 that was annotated in patch 3 here, you basically say
"oohhh, this SKB was annotated before, let's continue capturing
coverage data here"
(and turn it off again later by the corresponding kcov_remote_stop().
So the only way I could _possibly_ see how to do this would be to
* capture all possible places where the skb pointer can change
* still call something like skb_get_kcov_handle() but let it call out
to a BPF program to query a map or something to figure out if this
SKB has a handle attached to it
> Or if you don't like BPF or what to KCOV BPF itself in the future you
> can roll your own mechanism. The point is - this should be relatively
> easily doable out of line...
Seems pretty complicated to me though ...
johannes
^ permalink raw reply
* Re: [PATCH 072/141] can: peak_usb: Fix fall-through warnings for Clang
From: Joe Perches @ 2020-11-21 19:50 UTC (permalink / raw)
To: Marc Kleine-Budde, Gustavo A. R. Silva, Wolfgang Grandegger,
David S. Miller, Jakub Kicinski
Cc: linux-can, netdev, linux-kernel, linux-hardening
In-Reply-To: <bf3dbc5c-c34e-b3ef-abb6-0c88d8a90332@pengutronix.de>
On Sat, 2020-11-21 at 14:17 +0100, Marc Kleine-Budde wrote:
> On 11/20/20 7:34 PM, Gustavo A. R. Silva wrote:
> > In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning
> > by explicitly adding a break statement instead of letting the code fall
> > through to the next case.
> >
> > Link: https://github.com/KSPP/linux/issues/115
> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
[]
> > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
[]
> > @@ -299,6 +299,8 @@ static void peak_usb_write_bulk_callback(struct urb *urb)
> > if (net_ratelimit())
> > netdev_err(netdev, "Tx urb aborted (%d)\n",
> > urb->status);
> > + break;
> > +
> > case -EPROTO:
> > case -ENOENT:
> > case -ECONNRESET:
> >
>
> What about moving the default to the end if the case, which is more common anyways:
>
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
[]
> @@ -295,16 +295,16 @@ static void peak_usb_write_bulk_callback(struct urb *urb)
> netif_trans_update(netdev);
> break;
>
>
> - default:
> - if (net_ratelimit())
> - netdev_err(netdev, "Tx urb aborted (%d)\n",
> - urb->status);
> case -EPROTO:
> case -ENOENT:
> case -ECONNRESET:
> case -ESHUTDOWN:
> -
> break;
> +
> + default:
> + if (net_ratelimit())
> + netdev_err(netdev, "Tx urb aborted (%d)\n",
> + urb->status);
That's fine and is more generally used style but this
default: case should IMO also end with a break;
+ break;
> }
^ permalink raw reply
* [PATCH] compiler_attribute: remove CONFIG_ENABLE_MUST_CHECK
From: Masahiro Yamada @ 2020-11-21 19:43 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Masahiro Yamada, Jason A. Donenfeld, Shuah Khan, linux-kernel,
linux-kselftest, netdev, wireguard
Revert commit cebc04ba9aeb ("add CONFIG_ENABLE_MUST_CHECK").
A lot of warn_unused_result warnings existed in 2006, but until now
they have been fixed thanks to people doing allmodconfig tests.
Our goal is to always enable __must_check where appreciate, so this
CONFIG option is no longer needed.
I see a lot of defconfig (arch/*/configs/*_defconfig) files having:
# CONFIG_ENABLE_MUST_CHECK is not set
I did not touch them for now since it would be a big churn. If arch
maintainers want to clean them up, please go ahead.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
include/linux/compiler_types.h | 4 ----
lib/Kconfig.debug | 8 --------
tools/testing/selftests/wireguard/qemu/debug.config | 1 -
3 files changed, 13 deletions(-)
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index ac3fa37a84f9..02f6d3fbad9d 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -110,11 +110,7 @@ struct ftrace_likely_data {
unsigned long constant;
};
-#ifdef CONFIG_ENABLE_MUST_CHECK
#define __must_check __attribute__((__warn_unused_result__))
-#else
-#define __must_check
-#endif
#if defined(CC_USING_HOTPATCH)
#define notrace __attribute__((hotpatch(0, 0)))
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index c789b39ed527..cb8ef4fd0d02 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -286,14 +286,6 @@ config GDB_SCRIPTS
endif # DEBUG_INFO
-config ENABLE_MUST_CHECK
- bool "Enable __must_check logic"
- default y
- help
- Enable the __must_check logic in the kernel build. Disable this to
- suppress the "warning: ignoring return value of 'foo', declared with
- attribute warn_unused_result" messages.
-
config FRAME_WARN
int "Warn for stack frames larger than"
range 0 8192
diff --git a/tools/testing/selftests/wireguard/qemu/debug.config b/tools/testing/selftests/wireguard/qemu/debug.config
index b50c2085c1ac..fe07d97df9fa 100644
--- a/tools/testing/selftests/wireguard/qemu/debug.config
+++ b/tools/testing/selftests/wireguard/qemu/debug.config
@@ -1,5 +1,4 @@
CONFIG_LOCALVERSION="-debug"
-CONFIG_ENABLE_MUST_CHECK=y
CONFIG_FRAME_POINTER=y
CONFIG_STACK_VALIDATION=y
CONFIG_DEBUG_KERNEL=y
--
2.25.1
^ permalink raw reply related
* Re: [PATCH bpf-next v2 5/5] selftests/bpf: xsk selftests - Bi-directional Sockets - SKB, DRV
From: Weqaar Janjua @ 2020-11-21 20:14 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, netdev, Daniel Borkmann, ast, Magnus Karlsson,
Björn Töpel, Weqaar Janjua, shuah, skhan,
linux-kselftest, Anders Roxell, jonathan.lemon
In-Reply-To: <86e3a9e4-a375-1281-07bf-6b04781bb02f@fb.com>
On Fri, 20 Nov 2020 at 20:45, Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 11/20/20 5:00 AM, Weqaar Janjua wrote:
> > Adds following tests:
> >
> > 1. AF_XDP SKB mode
> > d. Bi-directional Sockets
> > Configure sockets as bi-directional tx/rx sockets, sets up fill
> > and completion rings on each socket, tx/rx in both directions.
> > Only nopoll mode is used
> >
> > 2. AF_XDP DRV/Native mode
> > d. Bi-directional Sockets
> > * Only copy mode is supported because veth does not currently support
> > zero-copy mode
> >
> > Signed-off-by: Weqaar Janjua <weqaar.a.janjua@intel.com>
> > ---
> > tools/testing/selftests/bpf/Makefile | 4 +-
> > .../bpf/test_xsk_drv_bidirectional.sh | 23 ++++
> > .../selftests/bpf/test_xsk_drv_teardown.sh | 3 -
> > .../bpf/test_xsk_skb_bidirectional.sh | 20 ++++
> > tools/testing/selftests/bpf/xdpxceiver.c | 100 +++++++++++++-----
> > tools/testing/selftests/bpf/xdpxceiver.h | 4 +
> > 6 files changed, 126 insertions(+), 28 deletions(-)
> > create mode 100755 tools/testing/selftests/bpf/test_xsk_drv_bidirectional.sh
> > create mode 100755 tools/testing/selftests/bpf/test_xsk_skb_bidirectional.sh
> >
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index 515b29d321d7..258bd72812e0 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -78,7 +78,9 @@ TEST_PROGS := test_kmod.sh \
> > test_xsk_drv_nopoll.sh \
> > test_xsk_drv_poll.sh \
> > test_xsk_skb_teardown.sh \
> > - test_xsk_drv_teardown.sh
> > + test_xsk_drv_teardown.sh \
> > + test_xsk_skb_bidirectional.sh \
> > + test_xsk_drv_bidirectional.sh
> >
> > TEST_PROGS_EXTENDED := with_addr.sh \
> > with_tunnels.sh \
> > diff --git a/tools/testing/selftests/bpf/test_xsk_drv_bidirectional.sh b/tools/testing/selftests/bpf/test_xsk_drv_bidirectional.sh
> > new file mode 100755
> > index 000000000000..d3a7e2934d83
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/test_xsk_drv_bidirectional.sh
> > @@ -0,0 +1,23 @@
> > +#!/bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright(c) 2020 Intel Corporation.
> > +
> > +# See test_xsk_prerequisites.sh for detailed information on tests
> > +
> > +. xsk_prereqs.sh
> > +. xsk_env.sh
> > +
> > +TEST_NAME="DRV BIDIRECTIONAL SOCKETS"
> > +
> > +vethXDPnative ${VETH0} ${VETH1} ${NS1}
> > +
> > +params=("-N" "-B")
> > +execxdpxceiver params
> > +
> > +retval=$?
> > +test_status $retval "${TEST_NAME}"
> > +
> > +# Must be called in the last test to execute
> > +cleanup_exit ${VETH0} ${VETH1} ${NS1}
>
> This also makes hard to run tests as users will not know this unless
> they are familiar with the details of the tests.
>
> How about you have another scripts test_xsk.sh which includes all these
> individual tests and pull the above cleanup_exit into test_xsk.sh?
> User just need to run test_xsk.sh will be able to run all tests you
> implemented here.
>
This works, test_xsk_* >> test_xsk.sh, will ship out as v3.
> > +
> > +test_exit $retval 0
> > diff --git a/tools/testing/selftests/bpf/test_xsk_drv_teardown.sh b/tools/testing/selftests/bpf/test_xsk_drv_teardown.sh
> [...]
^ permalink raw reply
* Re: [PATCH] compiler_attribute: remove CONFIG_ENABLE_MUST_CHECK
From: Miguel Ojeda @ 2020-11-21 20:45 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Jason A. Donenfeld, Shuah Khan, linux-kernel, linux-kselftest,
Network Development, wireguard, clang-built-linux,
Nick Desaulniers
In-Reply-To: <20201121194339.52290-1-masahiroy@kernel.org>
On Sat, Nov 21, 2020 at 8:44 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Our goal is to always enable __must_check where appreciate, so this
> CONFIG option is no longer needed.
This would be great. It also implies we can then move it to
`compiler_attributes.h` since it does not depend on config options
anymore.
We should also rename it to `__nodiscard`, since that is the
standardized name (coming soon to C2x and in C++ for years).
Cc'ing the Clang folks too to make them aware.
Cheers,
Miguel
^ permalink raw reply
* Re: [PATCH v5 2/3] net: add kcov handle to skb extensions
From: Jakub Kicinski @ 2020-11-21 20:55 UTC (permalink / raw)
To: Johannes Berg
Cc: Florian Westphal, Ido Schimmel, Aleksandr Nogikh, davem, edumazet,
andreyknvl, dvyukov, elver, linux-kernel, netdev, linux-wireless,
willemdebruijn.kernel, Aleksandr Nogikh, Willem de Bruijn
In-Reply-To: <106fc65f0459bc316e89beaf6bd71e823c4c01b7.camel@sipsolutions.net>
On Sat, 21 Nov 2020 20:30:44 +0100 Johannes Berg wrote:
> On Sat, 2020-11-21 at 10:35 -0800, Jakub Kicinski wrote:
> > On Sat, 21 Nov 2020 19:12:21 +0100 Johannes Berg wrote:
> > > > So I'm leaning towards reverting the whole thing. You can attach
> > > > kretprobes and record the information you need in BPF maps.
> > >
> > > I'm not going to object to reverting it (and perhaps redoing it better
> > > later), but I will point out that kretprobe isn't going to work, you
> > > eventually need kcov_remote_start() to be called in strategic points
> > > before processing the skb after it bounced through the system.
> > >
> > > IOW, it's not really about serving userland, it's about enabling (and
> > > later disabling) coverage collection for the bits of code it cares
> > > about, mostly because collecting it for _everything_ is going to be too
> > > slow and will mess up the data since for coverage guided fuzzing you
> > > really need the reported coverage data to be only about the injected
> > > fuzz data...
> >
> > All you need is make kcov_remote_start_common() be BPF-able, like
> > the LSM hooks are now, right? And then BPF can return whatever handle
> > it pleases.
>
> Not sure I understand. Are you saying something should call
> "kcov_remote_start_common()" with, say, the SKB, and leave it to a mass
> of bpf hooks to figure out where the SKB got cloned or copied or
> whatnot, track that in a map, and then ... no, wait, I don't really see
> what you mean, sorry.
>
> IIUC, fundamentally, you have this:
>
> - at the beginning, a task is tagged with "please collect coverage
> data for this handle"
Write the tag into task local storage, or map indexed by PID.
> - this task creates an SKB, etc, and all of the code that this task
> executes is captured and the coverage data is reported
kprobe the right places to record the skb -> handle mapping.
> - However, the SKB traverses lots of things, gets copied, cloned, or
> whatnot, and eventually leaves the annotated task, say for further
> processing in softirq context or elsewhere.
Which is fine.
> Now since the whole point is to see what chaos this SKB created from
> beginning (allocation) to end (free), since it was filled with fuzzed
> data, you now have to figure out where to pick back up when the SKB is
> processed further.
>
> This is what the infrastructure was meant to solve. But note that the
> SKB might be further cloned etc, so in order to track it you'd have to
> (out-of-band) figure out all the possible places where it could
> be reallocated, any time the skb pointer could change.
Ack, you have to figure out all the places anyway, the question is
whether you put probes there or calls in the source code.
Shifting the maintenance burden but also BPF is flexibility.
> Then, when you know you've got interesting code on your hands, like in
> mac80211 that was annotated in patch 3 here, you basically say
>
> "oohhh, this SKB was annotated before, let's continue capturing
> coverage data here"
>
> (and turn it off again later by the corresponding kcov_remote_stop().
Yup, the point is you can feed a raw skb pointer (and all other
possible context you may want) to a BPF prog in kcov_remote_start()
and let BPF/BTF give you the handle it recorded in its maps.
> So the only way I could _possibly_ see how to do this would be to
>
> * capture all possible places where the skb pointer can change
> * still call something like skb_get_kcov_handle() but let it call out
> to a BPF program to query a map or something to figure out if this
> SKB has a handle attached to it
>
> > Or if you don't like BPF or what to KCOV BPF itself in the future you
> > can roll your own mechanism. The point is - this should be relatively
> > easily doable out of line...
>
> Seems pretty complicated to me though ...
It is more complicated. We can go back to an skb field if this work is
expected to yield results for mac80211. Would you mind sending a patch?
^ permalink raw reply
* Re: [PATCH v5 2/3] net: add kcov handle to skb extensions
From: Johannes Berg @ 2020-11-21 20:58 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Florian Westphal, Ido Schimmel, Aleksandr Nogikh, davem, edumazet,
andreyknvl, dvyukov, elver, linux-kernel, netdev, linux-wireless,
willemdebruijn.kernel, Aleksandr Nogikh, Willem de Bruijn
In-Reply-To: <20201121125508.4d526dd0@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
On Sat, 2020-11-21 at 12:55 -0800, Jakub Kicinski wrote:
> [snip]
> Ack, you have to figure out all the places anyway, the question is
> whether you put probes there or calls in the source code.
>
> Shifting the maintenance burden but also BPF is flexibility.
Yeah, true. Though I'd argue also visibility - this stuff is pretty
simple now, if it gets into lots of lines of BPF code to track it that
is maintained "elsewhere", we won't see the bugs in it :-)
And it's kinda a thing that we as kernel developers _should_ be the ones
looking at since it's testing our code.
> Yup, the point is you can feed a raw skb pointer (and all other
> possible context you may want) to a BPF prog in kcov_remote_start()
> and let BPF/BTF give you the handle it recorded in its maps.
Yeah, it's possible. Personally, I don't think it's worth the
complexity.
> It is more complicated. We can go back to an skb field if this work is
> expected to yield results for mac80211. Would you mind sending a patch?
I can do that, but I'm not going to be able to do it now/tonight (GMT+1
here), so probably only Monday/Tuesday or so, sorry.
johannes
^ permalink raw reply
* Re: [PATCH v5 2/3] net: add kcov handle to skb extensions
From: Jakub Kicinski @ 2020-11-21 21:02 UTC (permalink / raw)
To: Johannes Berg
Cc: Florian Westphal, Ido Schimmel, Aleksandr Nogikh, davem, edumazet,
andreyknvl, dvyukov, elver, linux-kernel, netdev, linux-wireless,
willemdebruijn.kernel, Aleksandr Nogikh, Willem de Bruijn
In-Reply-To: <86c6369a937c760e374c78f5252ffc67cf67b1e1.camel@sipsolutions.net>
On Sat, 21 Nov 2020 21:58:37 +0100 Johannes Berg wrote:
> On Sat, 2020-11-21 at 12:55 -0800, Jakub Kicinski wrote:
> > It is more complicated. We can go back to an skb field if this work is
> > expected to yield results for mac80211. Would you mind sending a patch?
>
> I can do that, but I'm not going to be able to do it now/tonight (GMT+1
> here), so probably only Monday/Tuesday or so, sorry.
Oh yea, no worries, took someone a month to notice this is broken,
as long as it's fixed before the merge window it's fine ;)
^ permalink raw reply
* Re: [PATCHv3] bonding: wait for sysfs kobject destruction before freeing struct slave
From: Jakub Kicinski @ 2020-11-21 21:09 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jamie Iles, netdev, Qiushi Wu, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek
In-Reply-To: <X7fjR8ZB6BVwKS++@kroah.com>
On Fri, 20 Nov 2020 16:39:51 +0100 Greg Kroah-Hartman wrote:
> On Fri, Nov 20, 2020 at 02:28:27PM +0000, Jamie Iles wrote:
> > syzkaller found that with CONFIG_DEBUG_KOBJECT_RELEASE=y, releasing a
> > struct slave device could result in the following splat:
> > This is a potential use-after-free if the sysfs nodes are being accessed
> > whilst removing the struct slave, so wait for the object destruction to
> > complete before freeing the struct slave itself.
>
> Nice, it looks like it should have always been done this way!
>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Applied to net, thanks!
^ permalink raw reply
* Re: [PATCH] cxgb4: Fix build failure when CONFIG_TLS=m
From: Jakub Kicinski @ 2020-11-21 21:12 UTC (permalink / raw)
To: Tom Seewald; +Cc: netdev, linux-kernel, davem, ayush.sawal, rajur
In-Reply-To: <20201120192528.615-1-tseewald@gmail.com>
On Fri, 20 Nov 2020 13:25:28 -0600 Tom Seewald wrote:
> After commit 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
> whenever CONFIG_TLS=m and CONFIG_CHELSIO_T4=y, the following build
> failure occurs:
>
> ld: drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.o: in function
> `cxgb_select_queue':
> cxgb4_main.c:(.text+0x2dac): undefined reference to `tls_validate_xmit_skb'
>
> Fix this by ensuring that if TLS is set to be a module, CHELSIO_T4 will
> also be compiled as a module. As otherwise the cxgb4 driver will not be
> able to access TLS' symbols.
>
> Fixes: 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
> Signed-off-by: Tom Seewald <tseewald@gmail.com>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH v4] aquantia: Remove the build_skb path
From: Jakub Kicinski @ 2020-11-21 21:22 UTC (permalink / raw)
To: Ramsay, Lincoln
Cc: Florian Westphal, Igor Russkikh, David S. Miller,
netdev@vger.kernel.org, Dmitry Bogdanov
In-Reply-To: <CY4PR1001MB2311844FE8390F00A3363DEEE8E00@CY4PR1001MB2311.namprd10.prod.outlook.com>
On Thu, 19 Nov 2020 23:52:55 +0000 Ramsay, Lincoln wrote:
> When performing IPv6 forwarding, there is an expectation that SKBs
> will have some headroom. When forwarding a packet from the aquantia
> driver, this does not always happen, triggering a kernel warning.
>
> aq_ring.c has this code (edited slightly for brevity):
>
> if (buff->is_eop && buff->len <= AQ_CFG_RX_FRAME_MAX - AQ_SKB_ALIGN) {
> skb = build_skb(aq_buf_vaddr(&buff->rxdata), AQ_CFG_RX_FRAME_MAX);
> } else {
> skb = napi_alloc_skb(napi, AQ_CFG_RX_HDR_SIZE);
>
> There is a significant difference between the SKB produced by these
> 2 code paths. When napi_alloc_skb creates an SKB, there is a certain
> amount of headroom reserved. However, this is not done in the
> build_skb codepath.
>
> As the hardware buffer that build_skb is built around does not
> handle the presence of the SKB header, this code path is being
> removed and the napi_alloc_skb path will always be used. This code
> path does have to copy the packet header into the SKB, but it adds
> the packet data as a frag.
>
> Signed-off-by: Lincoln Ramsay <lincoln.ramsay@opengear.com>
I was going to apply as a fix to net and stable but too many small nits
here to pass. First of all please add a From: line at the beginning of
the mail which matches the signoff (or use git-send-email, it'll get it
right).
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index 4f913658eea4..425e8e5afec7 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> @@ -413,85 +413,64 @@ int aq_ring_rx_clean(struct aq_ring_s *self,
> buff->rxdata.pg_off,
> buff->len, DMA_FROM_DEVICE);
>
> - /* for single fragment packets use build_skb() */
> - if (buff->is_eop &&
> - buff->len <= AQ_CFG_RX_FRAME_MAX - AQ_SKB_ALIGN) {
> - skb = build_skb(aq_buf_vaddr(&buff->rxdata),
> + skb = napi_alloc_skb(napi, AQ_CFG_RX_HDR_SIZE);
> + if (unlikely(!skb)) {
> + u64_stats_update_begin(&self->stats.rx.syncp);
> + self->stats.rx.skb_alloc_fails++;
> + u64_stats_update_end(&self->stats.rx.syncp);
> + err = -ENOMEM;
> + goto err_exit;
> + }
> + if (is_ptp_ring)
> + buff->len -=
> + aq_ptp_extract_ts(self->aq_nic, skb,
> + aq_buf_vaddr(&buff->rxdata),
> + buff->len);
Align continuations of the lines under '(' like:
./scripts/checkpatch.pl --max-line-length=80 --strict
is asking you to.
> + hdr_len = buff->len;
> + if (hdr_len > AQ_CFG_RX_HDR_SIZE)
> + hdr_len = eth_get_headlen(skb->dev,
> + aq_buf_vaddr(&buff->rxdata),
> + AQ_CFG_RX_HDR_SIZE);
ditto
> + memcpy(__skb_put(skb, hdr_len), aq_buf_vaddr(&buff->rxdata),
> + ALIGN(hdr_len, sizeof(long)));
And here, etc.
> + if (buff->len - hdr_len > 0) {
> + skb_add_rx_frag(skb, 0, buff->rxdata.page,
> + buff->rxdata.pg_off + hdr_len,
> + buff->len - hdr_len,
> + if (!buff->is_eop) {
> + buff_ = buff;
> + i = 1U;
> + do {
> + next_ = buff_->next,
The end of this line should be a semicolon.
> + buff_ = &self->buff_ring[next_];
Thanks!
^ permalink raw reply
* Re: [PATCH v4] aquantia: Remove the build_skb path
From: Jakub Kicinski @ 2020-11-21 21:23 UTC (permalink / raw)
To: Ramsay, Lincoln
Cc: Florian Westphal, Igor Russkikh, David S. Miller,
netdev@vger.kernel.org, Dmitry Bogdanov
In-Reply-To: <20201121132204.43f9c4fb@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
On Sat, 21 Nov 2020 13:22:04 -0800 Jakub Kicinski wrote:
> On Thu, 19 Nov 2020 23:52:55 +0000 Ramsay, Lincoln wrote:
> > When performing IPv6 forwarding, there is an expectation that SKBs
> > will have some headroom. When forwarding a packet from the aquantia
> > driver, this does not always happen, triggering a kernel warning.
> >
> > aq_ring.c has this code (edited slightly for brevity):
> >
> > if (buff->is_eop && buff->len <= AQ_CFG_RX_FRAME_MAX - AQ_SKB_ALIGN) {
> > skb = build_skb(aq_buf_vaddr(&buff->rxdata), AQ_CFG_RX_FRAME_MAX);
> > } else {
> > skb = napi_alloc_skb(napi, AQ_CFG_RX_HDR_SIZE);
> >
> > There is a significant difference between the SKB produced by these
> > 2 code paths. When napi_alloc_skb creates an SKB, there is a certain
> > amount of headroom reserved. However, this is not done in the
> > build_skb codepath.
> >
> > As the hardware buffer that build_skb is built around does not
> > handle the presence of the SKB header, this code path is being
> > removed and the napi_alloc_skb path will always be used. This code
> > path does have to copy the packet header into the SKB, but it adds
> > the packet data as a frag.
> >
> > Signed-off-by: Lincoln Ramsay <lincoln.ramsay@opengear.com>
>
> I was going to apply as a fix to net and stable but too many small nits
> here to pass. First of all please add a From: line at the beginning of
> the mail which matches the signoff (or use git-send-email, it'll get it
> right).
Ah, one more thing, this is the correct fixes tag, right?
Fixes: 018423e90bee ("net: ethernet: aquantia: Add ring support code")
Please add it right before the signoff line.
^ permalink raw reply
* Re: [PATCH net-next] compat: always include linux/compat.h from net/compat.h
From: Arnd Bergmann @ 2020-11-21 21:25 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: David Miller, Networking, Christoph Hellwig, Arnd Bergmann
In-Reply-To: <20201121175224.1465831-1-kuba@kernel.org>
On Sat, Nov 21, 2020 at 6:52 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> We're about to do some reshuffling in networking headers and make
> some of the file lose the implicit includes. This results in:
>
> In file included from net/ipv4/netfilter/arp_tables.c:26:
> include/net/compat.h:57:23: error: conflicting types for ‘uintptr_t’
> #define compat_uptr_t uintptr_t
> ^~~~~~~~~
> include/asm-generic/compat.h:22:13: note: in expansion of macro ‘compat_uptr_t’
> typedef u32 compat_uptr_t;
> ^~~~~~~~~~~~~
> In file included from include/linux/limits.h:6,
> from include/linux/kernel.h:7,
> from net/ipv4/netfilter/arp_tables.c:14:
> include/linux/types.h:37:24: note: previous declaration of ‘uintptr_t’ was here
> typedef unsigned long uintptr_t;
> ^~~~~~~~~
>
> Currently net/compat.h depends on linux/compat.h being included
> first. After the upcoming changes this would break the 32bit build.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> Not sure who officially maintains this. Arnd, Christoph any objections?
Looks good to me. I would actually go one step further and completely
remove this #ifdef, if possible. In the old days, it was not possible to
include linux/compat.h on 32-bit architectures, but now this should just
work without an #ifdef.
Arnd
^ permalink raw reply
* Re: [PATCH v2 net] ptp: clockmatrix: bug fix for idtcm_strverscmp
From: Jakub Kicinski @ 2020-11-21 21:40 UTC (permalink / raw)
To: min.li.xe; +Cc: richardcochran, netdev, linux-kernel
In-Reply-To: <1605757824-3292-1-git-send-email-min.li.xe@renesas.com>
On Wed, 18 Nov 2020 22:50:24 -0500 min.li.xe@renesas.com wrote:
> From: Min Li <min.li.xe@renesas.com>
>
> Feed kstrtou8 with NULL terminated string.
>
> Changes since v1:
> -Only strcpy 15 characters to leave 1 space for '\0'
>
> Signed-off-by: Min Li <min.li.xe@renesas.com>
> -static int idtcm_strverscmp(const char *ver1, const char *ver2)
> +static int idtcm_strverscmp(const char *version1, const char *version2)
> {
> u8 num1;
> u8 num2;
> int result = 0;
> + char ver1[16];
> + char ver2[16];
> + char *cur1;
> + char *cur2;
> + char *next1;
> + char *next2;
> +
> + strncpy(ver1, version1, 15);
> + strncpy(ver2, version2, 15);
> + cur1 = ver1;
> + cur2 = ver2;
Now there is no guarantee that the buffer is null terminated.
You need to use strscpy(ver... 16) or add ver[15] = '\0';
> /* loop through each level of the version string */
> while (result == 0) {
> + next1 = strchr(cur1, '.');
> + next2 = strchr(cur2, '.');
> +
> + /* kstrtou8 could fail for dot */
> + if (next1) {
> + *next1 = '\0';
> + next1++;
> + }
> +
> + if (next2) {
> + *next2 = '\0';
> + next2++;
> + }
> +
> /* extract leading version numbers */
> - if (kstrtou8(ver1, 10, &num1) < 0)
> + if (kstrtou8(cur1, 10, &num1) < 0)
> return -1;
>
> - if (kstrtou8(ver2, 10, &num2) < 0)
> + if (kstrtou8(cur2, 10, &num2) < 0)
> return -1;
>
> /* if numbers differ, then set the result */
> - if (num1 < num2)
> + if (num1 < num2) {
> result = -1;
Why do you set the result to something instead of just returning that
value? The kstrtou8() checks above just return value directly...
If you use a return you can save yourself all the else branches.
> - else if (num1 > num2)
> + } else if (num1 > num2) {
> result = 1;
> - else {
> + } else {
> /* if numbers are the same, go to next level */
> - ver1 = strchr(ver1, '.');
> - ver2 = strchr(ver2, '.');
> - if (!ver1 && !ver2)
> + if (!next1 && !next2)
> break;
> - else if (!ver1)
> + else if (!next1) {
> result = -1;
> - else if (!ver2)
> + } else if (!next2) {
> result = 1;
> - else {
> - ver1++;
> - ver2++;
> + } else {
> + cur1 = next1;
> + cur2 = next2;
> }
> }
> }
> +
> return result;
> }
>
^ permalink raw reply
* Re: [PATCH net-next] compat: always include linux/compat.h from net/compat.h
From: Jakub Kicinski @ 2020-11-21 21:48 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: David Miller, Networking, Christoph Hellwig, Arnd Bergmann
In-Reply-To: <CAK8P3a1sZ7CUwQ-fUCS-z4CkhgSiNqzPcc_MTc2D54-8vfmV=g@mail.gmail.com>
On Sat, 21 Nov 2020 22:25:35 +0100 Arnd Bergmann wrote:
> On Sat, Nov 21, 2020 at 6:52 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > We're about to do some reshuffling in networking headers and make
> > some of the file lose the implicit includes. This results in:
> >
> > In file included from net/ipv4/netfilter/arp_tables.c:26:
> > include/net/compat.h:57:23: error: conflicting types for ‘uintptr_t’
> > #define compat_uptr_t uintptr_t
> > ^~~~~~~~~
> > include/asm-generic/compat.h:22:13: note: in expansion of macro ‘compat_uptr_t’
> > typedef u32 compat_uptr_t;
> > ^~~~~~~~~~~~~
> > In file included from include/linux/limits.h:6,
> > from include/linux/kernel.h:7,
> > from net/ipv4/netfilter/arp_tables.c:14:
> > include/linux/types.h:37:24: note: previous declaration of ‘uintptr_t’ was here
> > typedef unsigned long uintptr_t;
> > ^~~~~~~~~
> >
> > Currently net/compat.h depends on linux/compat.h being included
> > first. After the upcoming changes this would break the 32bit build.
> >
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > ---
> > Not sure who officially maintains this. Arnd, Christoph any objections?
>
> Looks good to me. I would actually go one step further and completely
> remove this #ifdef, if possible. In the old days, it was not possible to
> include linux/compat.h on 32-bit architectures, but now this should just
> work without an #ifdef.
Indeed, that appears to work, v2 coming up, thanks!
^ permalink raw reply
* [PATCH net-next v2] compat: always include linux/compat.h from net/compat.h
From: Jakub Kicinski @ 2020-11-21 21:48 UTC (permalink / raw)
To: davem; +Cc: netdev, hch, arnd, Jakub Kicinski
We're about to do reshuffling in networking headers and
eliminate some implicit includes. This results in:
In file included from ../net/ipv4/netfilter/arp_tables.c:26:
include/net/compat.h:60:40: error: unknown type name ‘compat_uptr_t’; did you mean ‘compat_ptr_ioctl’?
struct sockaddr __user **save_addr, compat_uptr_t *ptr,
^~~~~~~~~~~~~
compat_ptr_ioctl
include/net/compat.h:61:4: error: unknown type name ‘compat_size_t’; did you mean ‘compat_sigset_t’?
compat_size_t *len);
^~~~~~~~~~~~~
compat_sigset_t
Currently net/compat.h depends on linux/compat.h being included
first. After the upcoming changes this would break the 32bit build.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
--
v2:
- correct the commit message
- remove the ifdef completely (Arnd)
---
include/net/compat.h | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/include/net/compat.h b/include/net/compat.h
index 745db0d605b6..84805bdc4435 100644
--- a/include/net/compat.h
+++ b/include/net/compat.h
@@ -5,8 +5,6 @@
struct sock;
-#if defined(CONFIG_COMPAT)
-
#include <linux/compat.h>
struct compat_msghdr {
@@ -48,14 +46,6 @@ struct compat_rtentry {
unsigned short rt_irtt; /* Initial RTT */
};
-#else /* defined(CONFIG_COMPAT) */
-/*
- * To avoid compiler warnings:
- */
-#define compat_msghdr msghdr
-#define compat_mmsghdr mmsghdr
-#endif /* defined(CONFIG_COMPAT) */
-
int __get_compat_msghdr(struct msghdr *kmsg, struct compat_msghdr __user *umsg,
struct sockaddr __user **save_addr, compat_uptr_t *ptr,
compat_size_t *len);
--
2.24.1
^ permalink raw reply related
* Re: [PATCH] usbnet: ipheth: fix connectivity with iOS 14
From: Jakub Kicinski @ 2020-11-21 22:03 UTC (permalink / raw)
To: Yves-Alexis Perez
Cc: David S. Miller, Martin Habets, Luc Van Oostenryck,
Shannon Nelson, Michael S. Tsirkin, linux-usb, netdev,
linux-kernel, Matti Vuorela, stable
In-Reply-To: <20201119172439.94988-1-corsac@corsac.net>
On Thu, 19 Nov 2020 18:24:39 +0100 Yves-Alexis Perez wrote:
> Starting with iOS 14 released in September 2020, connectivity using the
> personal hotspot USB tethering function of iOS devices is broken.
>
> Communication between the host and the device (for example ICMP traffic
> or DNS resolution using the DNS service running in the device itself)
> works fine, but communication to endpoints further away doesn't work.
>
> Investigation on the matter shows that UDP and ICMP traffic from the
> tethered host is reaching the Internet at all. For TCP traffic there are
> exchanges between tethered host and server but packets are modified in
> transit leading to impossible communication.
>
> After some trials Matti Vuorela discovered that reducing the URB buffer
> size by two bytes restored the previous behavior. While a better
> solution might exist to fix the issue, since the protocol is not
> publicly documented and considering the small size of the fix, let's do
> that.
>
> Tested-by: Matti Vuorela <matti.vuorela@bitfactor.fi>
> Signed-off-by: Yves-Alexis Perez <corsac@corsac.net>
> Link: https://lore.kernel.org/linux-usb/CAAn0qaXmysJ9vx3ZEMkViv_B19ju-_ExN8Yn_uSefxpjS6g4Lw@mail.gmail.com/
> Link: https://github.com/libimobiledevice/libimobiledevice/issues/1038
Applied to net with the typo fixed, thanks!
^ permalink raw reply
* Re: [Patch stable] netfilter: clear skb->next in NF_HOOK_LIST()
From: Florian Westphal @ 2020-11-21 22:22 UTC (permalink / raw)
To: Cong Wang
Cc: netdev, Cong Wang, liuzx, Florian Westphal, Edward Cree, stable,
Greg Kroah-Hartman
In-Reply-To: <20201121034317.577081-1-xiyou.wangcong@gmail.com>
Cong Wang <xiyou.wangcong@gmail.com> wrote:
> From: Cong Wang <cong.wang@bytedance.com>
>
> NF_HOOK_LIST() uses list_del() to remove skb from the linked list,
> however, it is not sufficient as skb->next still points to other
> skb. We should just call skb_list_del_init() to clear skb->next,
> like the rest places which using skb list.
>
> This has been fixed in upstream by commit ca58fbe06c54
> ("netfilter: add and use nf_hook_slow_list()").
Thanks Cong, agree with this change, afaics its applicable to 4.19.y and 5.4.y.
^ permalink raw reply
* Re: [PATCH net-next] MAINTAINERS: Update page pool entry
From: Jakub Kicinski @ 2020-11-21 22:37 UTC (permalink / raw)
To: Jesper Dangaard Brouer; +Cc: netdev, Ilias Apalodimas
In-Reply-To: <160586497895.2808766.2902017028647296556.stgit@firesoul>
On Fri, 20 Nov 2020 10:36:19 +0100 Jesper Dangaard Brouer wrote:
> Add some file F: matches that is related to page_pool.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
> MAINTAINERS | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f827f504251b..efcdc68a03b1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13179,6 +13179,8 @@ L: netdev@vger.kernel.org
> S: Supported
> F: include/net/page_pool.h
> F: net/core/page_pool.c
> +F: include/trace/events/page_pool.h
> +F: Documentation/networking/page_pool.rst
Checkpatch says:
WARNING: Misordered MAINTAINERS entry - list file patterns in alphabetic order
#26: FILE: MAINTAINERS:13165:
F: net/core/page_pool.c
+F: include/trace/events/page_pool.h
WARNING: Misordered MAINTAINERS entry - list file patterns in alphabetic order
#27: FILE: MAINTAINERS:13166:
+F: include/trace/events/page_pool.h
+F: Documentation/networking/page_pool.rst
^ permalink raw reply
* Re: [PATCH net-next 0/5] net: hns3: misc updates for -next
From: patchwork-bot+netdevbpf @ 2020-11-21 22:40 UTC (permalink / raw)
To: tanhuazhong
Cc: davem, netdev, linux-kernel, salil.mehta, yisen.zhuang, linuxarm,
kuba
In-Reply-To: <1605863783-36995-1-git-send-email-tanhuazhong@huawei.com>
Hello:
This series was applied to netdev/net-next.git (refs/heads/master):
On Fri, 20 Nov 2020 17:16:18 +0800 you wrote:
> This series includes some misc updates for the HNS3 ethernet driver.
>
> #1 adds support for 1280 queues
> #2 adds mapping for BAR45 which is needed by RoCE client.
> #3 extend the interrupt resources.
> #4 add support to query firmware's calculated shaping parameters.
>
> [...]
Here is the summary with links:
- [net-next,1/5] net: hns3: add support for 1280 queues
https://git.kernel.org/netdev/net-next/c/9a5ef4aa5457
- [net-next,2/5] net: hns3: add support for mapping device memory
https://git.kernel.org/netdev/net-next/c/30ae7f8a6aa7
- [net-next,3/5] net: hns3: add support for pf querying new interrupt resources
https://git.kernel.org/netdev/net-next/c/3a6863e4e8ee
- [net-next,4/5] net: hns3: add support to utilize the firmware calculated shaping parameters
https://git.kernel.org/netdev/net-next/c/e364ad303fe3
- [net-next,5/5] net: hns3: adds debugfs to dump more info of shaping parameters
https://git.kernel.org/netdev/net-next/c/c331ecf1afc1
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH net-next] net: bridge: switch to net core statistics counters handling
From: Jakub Kicinski @ 2020-11-21 22:41 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Roopa Prabhu, Nikolay Aleksandrov, David Miller, bridge,
netdev@vger.kernel.org
In-Reply-To: <9bad2be2-fd84-7c6e-912f-cee433787018@gmail.com>
On Fri, 20 Nov 2020 12:22:23 +0100 Heiner Kallweit wrote:
> Use netdev->tstats instead of a member of net_bridge for storing
> a pointer to the per-cpu counters. This allows us to use core
> functionality for statistics handling.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH net] net/af_iucv: set correct sk_protocol for child sockets
From: patchwork-bot+netdevbpf @ 2020-11-21 22:50 UTC (permalink / raw)
To: Julian Wiedmann; +Cc: davem, kuba, netdev, linux-s390, hca, kgraul
In-Reply-To: <20201120100657.34407-1-jwi@linux.ibm.com>
Hello:
This patch was applied to netdev/net.git (refs/heads/master):
On Fri, 20 Nov 2020 11:06:57 +0100 you wrote:
> Child sockets erroneously inherit their parent's sk_type (ie. SOCK_*),
> instead of the PF_IUCV protocol that the parent was created with in
> iucv_sock_create().
>
> We're currently not using sk->sk_protocol ourselves, so this shouldn't
> have much impact (except eg. getting the output in skb_dump() right).
>
> [...]
Here is the summary with links:
- [net] net/af_iucv: set correct sk_protocol for child sockets
https://git.kernel.org/netdev/net/c/c5dab0941fcd
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH net] devlink: Fix reload stats structure
From: Jakub Kicinski @ 2020-11-21 22:53 UTC (permalink / raw)
To: Moshe Shemesh; +Cc: David S. Miller, Jiri Pirko, netdev, linux-kernel
In-Reply-To: <1605879637-6114-1-git-send-email-moshe@mellanox.com>
On Fri, 20 Nov 2020 15:40:37 +0200 Moshe Shemesh wrote:
> Fix reload stats structure exposed to the user. Change stats structure
> hierarchy to have the reload action as a parent of the stat entry and
> then stat entry includes value per limit. This will also help to avoid
> string concatenation on iproute2 output.
>
> Reload stats structure before this fix:
> "stats": {
> "reload": {
> "driver_reinit": 2,
> "fw_activate": 1,
> "fw_activate_no_reset": 0
> }
> }
>
> After this fix:
> "stats": {
> "reload": {
> "driver_reinit": {
> "unspecified": 2
> },
> "fw_activate": {
> "unspecified": 1,
> "no_reset": 0
> }
> }
>
> Fixes: a254c264267e ("devlink: Add reload stats")
> Signed-off-by: Moshe Shemesh <moshe@mellanox.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
At least try to fold the core networking code at 80 characters *please*.
You folded the comments at 86 chars, neither 100 nor 80.
^ permalink raw reply
* Re: [PATCH 072/141] can: peak_usb: Fix fall-through warnings for Clang
From: Marc Kleine-Budde @ 2020-11-21 23:04 UTC (permalink / raw)
To: Joe Perches, Gustavo A. R. Silva, Wolfgang Grandegger,
David S. Miller, Jakub Kicinski
Cc: linux-can, netdev, linux-kernel, linux-hardening
In-Reply-To: <de5b16cf3fdac1f783e291acc325b78368653ec5.camel@perches.com>
[-- Attachment #1.1: Type: text/plain, Size: 1580 bytes --]
On 11/21/20 8:50 PM, Joe Perches wrote:
>> What about moving the default to the end if the case, which is more common anyways:
>>
>> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> []
>> @@ -295,16 +295,16 @@ static void peak_usb_write_bulk_callback(struct urb *urb)
>> netif_trans_update(netdev);
>> break;
>>
>>
>> - default:
>> - if (net_ratelimit())
>> - netdev_err(netdev, "Tx urb aborted (%d)\n",
>> - urb->status);
>> case -EPROTO:
>> case -ENOENT:
>> case -ECONNRESET:
>> case -ESHUTDOWN:
>> -
>> break;
>> +
>> + default:
>> + if (net_ratelimit())
>> + netdev_err(netdev, "Tx urb aborted (%d)\n",
>> + urb->status);
>
> That's fine and is more generally used style but this
> default: case should IMO also end with a break;
>
> + break;
I don't mind.
process/coding-style.rst is not totally clear about the break after the default,
if this is the lase one the switch statement.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: pull-request: can-next 2020-11-20
From: Jakub Kicinski @ 2020-11-21 23:09 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: netdev, davem, linux-can, kernel
In-Reply-To: <20201120133318.3428231-1-mkl@pengutronix.de>
On Fri, 20 Nov 2020 14:32:53 +0100 Marc Kleine-Budde wrote:
> The first patch is by Yegor Yefremov and he improves the j1939 documentaton by
> adding tables for the CAN identifier and its fields.
>
> Then there are 8 patches by Oliver Hartkopp targeting the CAN driver
> infrastructure and drivers. These add support for optional DLC element to the
> Classical CAN frame structure. See patch ea7800565a12 ("can: add optional DLC
> element to Classical CAN frame structure") for details. Oliver's last patch
> adds len8_dlc support to several drivers. Stefan Mätje provides a patch to add
> len8_dlc support to the esd_usb2 driver.
>
> The next patch is by Oliver Hartkopp, too and adds support for modification of
> Classical CAN DLCs to CAN GW sockets.
>
> The next 3 patches target the nxp,flexcan DT bindings. One patch by my adds the
> missing uint32 reference to the clock-frequency property. Joakim Zhang's
> patches fix the fsl,clk-source property and add the IMX_SC_R_CAN() macro to the
> imx firmware header file, which will be used in the flexcan driver later.
>
> Another patch by Joakim Zhang prepares the flexcan driver for SCU based
> stop-mode, by giving the existing, GPR based stop-mode, a _GPR postfix.
>
> The next 5 patches are by me, target the flexcan driver, and clean up the
> .ndo_open and .ndo_stop callbacks. These patches try to fix a sporadically
> hanging flexcan_close() during simultanious ifdown, sending of CAN messages and
> probably open CAN bus. I was never aber to reproduce, but these seem to fix the
> problem at the reporting user. As these changes are rather big, I'd like to
> mainline them via net-next/master.
>
> The next patches are by Jimmy Assarsson and Christer Beskow, they add support
> for new USB devices to the existing kvaser_usb driver.
>
> The last patch is by Kaixu Xia and simplifies the return in the
> mcp251xfd_chip_softreset() function in the mcp251xfd driver.
Great, this one finally got into patchwork correctly!
Pulled, thank you!
^ 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