* Re: usb/net/rt2x00: warning in rt2800_eeprom_word_index
From: Dmitry Vyukov @ 2017-10-19 8:25 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: Andrey Konovalov, Helmut Schaa, Kalle Valo, linux-wireless,
netdev, LKML, Kostya Serebryany, syzkaller
In-Reply-To: <CACT4Y+YB7PTCFCjeqwJeGpSxxAh_U6F6UsDKaN4XAx6uL8+pZw@mail.gmail.com>
On Mon, Oct 16, 2017 at 2:19 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Mon, Oct 16, 2017 at 11:40 AM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
>> Hi Dmitry
>>
>> On Sat, Oct 14, 2017 at 04:38:03PM +0200, Dmitry Vyukov wrote:
>>> On Thu, Oct 12, 2017 at 9:25 AM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
>>> > Hi
>>> >
>>> > On Mon, Oct 09, 2017 at 07:50:53PM +0200, Andrey Konovalov wrote:
>>> >> I've got the following report while fuzzing the kernel with syzkaller.
>>> >>
>>> >> On commit 8a5776a5f49812d29fe4b2d0a2d71675c3facf3f (4.14-rc4).
>>> >>
>>> >> I'm not sure whether this is a bug in the driver, or just a way to
>>> >> report misbehaving device. In the latter case this shouldn't be a
>>> >> WARN() call, since WARN() means bug in the kernel.
>>> >
>>> > This is about wrong EEPROM, which reported 3 tx streams on
>>> > non 3 antenna device. I think WARN() is justified and thanks
>>> > to the call trace I was actually able to to understand what
>>> > happened.
>>> >
>>> > In general I do not think WARN() only means a kernel bug, it
>>> > can be F/W or H/W bug too.
>>>
>>> Hi Stanislaw,
>>>
>>> Printing messages is fine. Printing stacks is fine. Just please make
>>> them distinguishable from kernel bugs and don't kill the whole
>>> possibility of automated Linux kernel testing. That's an important
>>> capability.
>>
>> We do not distinguish between bugs and other problems when WARN() is
>> used in (wireless) drivers, what I think is correct, taking comment from
>> include/asm-generic/bug.h :
>>
>> /*
>> * WARN(), WARN_ON(), WARN_ON_ONCE, and so on can be used to report
>> * significant issues that need prompt attention if they should ever
>> * appear at runtime. Use the versions with printk format strings
>> * to provide better diagnostics.
>> */
>>
>> Historically we have BUG() to mark the bugs, but usage if it is not
>> recommended as it can kill the system, so for anything that can
>> be recovered in runtime - WARN() is recommended.
>>
>> Perhaps we can introduce another helper like PROBLEM() for marking
>> situations when something is wrong, but it is not a bug. However I'm
>> not even sure at what extent it can be used, since for many cases
>> if not the most, driver author can not tell apriori if the problem
>> is a bug in the driver or HW/FW misbehaviour (or maybe particular
>> issue can happen because of both).
>
> I will write a separate email to LKML.
Sent a mail titled "Distinguishing kernel bugs from invalid inputs" to
LKML. Here is a copy:
https://groups.google.com/forum/#!topic/syzkaller/dGh7qtbu14Q
^ permalink raw reply
* Re: [PATCH net-next v2 1/2] ipv6: start fib6 gc on RTF_CACHE dst creation
From: Sergei Shtylyov @ 2017-10-19 8:45 UTC (permalink / raw)
To: Paolo Abeni, netdev
Cc: David S. Miller, Wei Wang, Martin KaFai Lau, Eric Dumazet,
Hannes Frederic Sowa
In-Reply-To: <22a7dd612b11e30164ad489fb1446b9f6e2de9e6.1508358132.git.pabeni@redhat.com>
Hello!
On 10/18/2017 11:23 PM, Paolo Abeni wrote:
> After the commit Fixes: 2b760fcf5cfb ("ipv6: hook up exception
Fixes: belongs in the tag section. :-)
> table to store dst cache"), the fib6 gc is not started after
> the creation of a RTF_CACHE via a redirect or pmtu update, since
> fib6_add() isn't invoked anymore for such dsts.
>
> We need the fib6 gc to run periodically to clean the RTF_CACHE,
> or the dst will stay there forever.
>
> Fix it by explicitly calling fib6_force_start_gc() on successful
> exception creation. gc_args->more accounting will ensure that
> the gc timer will run for whatever time needed to properly
> clean the table.
>
> Fixes: 2b760fcf5cfb ("ipv6: hook up exception table to store dst cache")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> Acked-by: Wei Wang <weiwan@google.com>
> Acked-by: Martin KaFai Lau <kafai@fb.com>
[...]
MBR, Sergei
^ permalink raw reply
* Re: [PATCH] net: l2tp: mark expected switch fall-through
From: Guillaume Nault @ 2017-10-19 8:51 UTC (permalink / raw)
To: Gustavo A. R. Silva; +Cc: David S. Miller, netdev, linux-kernel
In-Reply-To: <20171017224253.GA5877@embeddedor.com>
On Tue, Oct 17, 2017 at 05:42:53PM -0500, Gustavo A. R. Silva wrote:
> This code was tested by compilation only (GCC 7.2.0 was used).
>
No worry, this case really has to fall through.
> diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
> index 7135f46..f517942 100644
> --- a/net/l2tp/l2tp_netlink.c
> +++ b/net/l2tp/l2tp_netlink.c
> @@ -406,7 +406,7 @@ static int l2tp_nl_tunnel_send(struct sk_buff *skb, u32 portid, u32 seq, int fla
> if (nla_put_u16(skb, L2TP_ATTR_UDP_SPORT, ntohs(inet->inet_sport)) ||
> nla_put_u16(skb, L2TP_ATTR_UDP_DPORT, ntohs(inet->inet_dport)))
> goto nla_put_failure;
> - /* NOBREAK */
> + /* fall through */
> case L2TP_ENCAPTYPE_IP:
>
Nit: extra whitespace before '*/' (just couldn't restrain myself from
noticing).
Acked-by: Guillaume Nault <g.nault@alphalink.fr>
^ permalink raw reply
* Re: [PATCH] wireless: iwlegacy: Convert timers to use timer_setup()
From: Stanislaw Gruszka @ 2017-10-19 8:54 UTC (permalink / raw)
To: Kees Cook; +Cc: Kalle Valo, linux-wireless, netdev, linux-kernel
In-Reply-To: <20171016233744.GA101655@beast>
On Mon, Oct 16, 2017 at 04:37:44PM -0700, Kees Cook wrote:
> In preparation for unconditionally passing the struct timer_list pointer to
> all timer callbacks, switch to using the new timer_setup() and from_timer()
> to pass the timer pointer explicitly.
>
> Cc: Stanislaw Gruszka <sgruszka@redhat.com>
> Cc: Kalle Valo <kvalo@codeaurora.org>
> Cc: linux-wireless@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Stanislaw Gruszka <sgruszka@redhat.com>
^ permalink raw reply
* Re: [PATCH iproute2] ip maddr: fix filtering by device
From: Phil Sutter @ 2017-10-19 9:06 UTC (permalink / raw)
To: Michal Kubecek; +Cc: Stephen Hemminger, netdev, Petr Vorel
In-Reply-To: <20171019082108.40D7AA0EDF@unicorn.suse.cz>
On Thu, Oct 19, 2017 at 10:21:08AM +0200, Michal Kubecek wrote:
> Commit 530903dd9003 ("ip: fix igmp parsing when iface is long") uses
> variable len to keep trailing colon from interface name comparison. This
> variable is local to loop body but we set it in one pass and use it in
> following one(s) so that we are actually using (pseudo)random length for
> comparison. This became apparent since commit b48a1161f5f9 ("ipmaddr: Avoid
> accessing uninitialized data") always initializes len to zero so that the
> name comparison is always true. As a result, "ip maddr show dev eth0" shows
> IPv4 multicast addresses for all interfaces.
>
> Instead of keeping the length, let's simply replace the trailing colon with
> a null byte. The bonus is that we get correct interface name in ma.name.
>
> Fixes: 530903dd9003 ("ip: fix igmp parsing when iface is long")
> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
Acked-by: Phil Sutter <phil@nwl.cc>
^ permalink raw reply
* Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates
From: Marc Kleine-Budde @ 2017-10-19 9:13 UTC (permalink / raw)
To: Sekhar Nori, Franklin S Cooper Jr, Mario Hüttel,
Yang, Wenyou, wg, socketcan, quentin.schulz, edumazet, linux-can,
netdev, linux-kernel
Cc: Wenyou Yang, Dong Aisheng, Quadros, Roger
In-Reply-To: <93c6d531-c38b-6f88-510f-59eb3e733b78@ti.com>
[-- Attachment #1.1: Type: text/plain, Size: 3942 bytes --]
On 10/19/2017 07:07 AM, Sekhar Nori wrote:
>>>> Sounds reasonable. What's the status of this series?
>>>
>>> I have had some offline discussions with Franklin on this, and I am not
>>> fully convinced that DT is the way to go here (although I don't have the
>>> agreement with Franklin there).
>>
>> Probably the fundamental area where we disagree is what "default" SSP
>> value should be used. Based on a short (< 1 ft) point to point test
>> using a SSP of 50% worked fine. However, I'm not convinced that this
>> default value of 50% will work in a more "traditional" CAN bus at higher
>> speeds. Nor am I convinced that a SSP of 50% will work on every MCAN
>> board in even the simplest test cases.
>>
>> I believe that this default SSP should be a DT property that allows any
>> board to determine what default value works best in general.
>
> With that, I think, we are taking DT from describing board/hardware
> characteristics to providing default values that software should use.
If the default value is board specific and cannot be calculated in
general or from other values present in the DT, then it's from my point
of view describing the hardware.
> In any case, if Marc and/or Wolfgang are okay with it, binding
> documentation for such a property should be sent to DT maintainers for
> review.
>
>>>
>>> There are two components in configuring the secondary sample point. It
>>> is the transceiver loopback delay and an offset (example half of the bit
>>> time in data phase).
>>>
>>> While the transceiver loopback delay is pretty board dependent (and thus
>>> amenable to DT encoding), I am not quite sure the offset can be
>>> configured in DT because its not really board dependent.
>>>
>>> Unfortunately, offset calculation does not seem to be an exact science.
>>> There are recommendations ranging from using 50% of bit time to making
>>> it same as the sample point configured. This means users who need to
>>> change the SSP due to offset variations need to change their DT even
>>> without anything changing on their board.
>>>
>>> Since we have a netlink socket interface to configure sample point, I
>>> wonder if that should be extended to configure SSP too (or at least the
>>> offset part of SSP)?
>>
>> Sekhar is right that ideally the user should be able to set the SSP at
>> runtime. However, my issue is that based on my experience CAN users
>> expect the driver to just work the majority of times. For unique use
>> cases where the driver calculated values don't work then the user should
>> be able to override it. This should only be done for a very small
>> percentage of CAN users. Unless you allow DT to provide a default SSP
>> many users of MCAN may find that the default SSP doesn't work and must
>> always use runtime overrides to get anything to work. I don't think that
>> is a good user experience which is why I don't like the idea.
>
> Fair enough. But not quite sure if CAN users expect CAN-FD to "just
> work" without doing any bittiming related setup.
From my point of view I'd rather buy a board from a HW vendor where
CAN-FD works, rather than a board where I have to tweak the bit-timing
for a simple CAN-FD test setup.
As far as I see for the m_can driver it's a single tuple: "bitrate > 2.5
MBit/s -> 50%". Do we need an array of tuples in general?
If good default values are transceiver and board specific, they can go
into the DT. We need a generic (this means driver agnostic) binding for
this. If this table needs to be tweaked for special purpose, then we can
add a netlink interface for this as well.
Comments?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH iproute2] ip maddr: fix filtering by device
From: Petr Vorel @ 2017-10-19 9:14 UTC (permalink / raw)
To: Phil Sutter, Michal Kubecek, Stephen Hemminger, netdev
In-Reply-To: <20171019090658.GA21378@orbyte.nwl.cc>
> On Thu, Oct 19, 2017 at 10:21:08AM +0200, Michal Kubecek wrote:
> > Commit 530903dd9003 ("ip: fix igmp parsing when iface is long") uses
> > variable len to keep trailing colon from interface name comparison. This
> > variable is local to loop body but we set it in one pass and use it in
> > following one(s) so that we are actually using (pseudo)random length for
> > comparison. This became apparent since commit b48a1161f5f9 ("ipmaddr: Avoid
> > accessing uninitialized data") always initializes len to zero so that the
> > name comparison is always true. As a result, "ip maddr show dev eth0" shows
> > IPv4 multicast addresses for all interfaces.
> > Instead of keeping the length, let's simply replace the trailing colon with
> > a null byte. The bonus is that we get correct interface name in ma.name.
> > Fixes: 530903dd9003 ("ip: fix igmp parsing when iface is long")
> > Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
> Acked-by: Phil Sutter <phil@nwl.cc>
Acked-by: Petr Vorel <pvorel@suse.cz>
Thanks for fixing, Michal.
Petr
^ permalink raw reply
* Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates
From: Marc Kleine-Budde @ 2017-10-19 9:21 UTC (permalink / raw)
To: Ramesh Shanmugasundaram, Sekhar Nori, Franklin S Cooper Jr,
Mario Hüttel, Yang, Wenyou, wg@grandegger.com,
socketcan@hartkopp.net, quentin.schulz@free-electrons.com,
edumazet@google.com, linux-can@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: Wenyou Yang, Dong Aisheng, Quadros, Roger
In-Reply-To: <KL1PR0601MB20386197BA2733930329257BC3420@KL1PR0601MB2038.apcprd06.prod.outlook.com>
[-- Attachment #1.1: Type: text/plain, Size: 2149 bytes --]
On 10/19/2017 10:04 AM, Ramesh Shanmugasundaram wrote:
>>> Sounds reasonable. What's the status of this series?
>>
>> I have had some offline discussions with Franklin on this, and I am not
>> fully convinced that DT is the way to go here (although I don't have the
>> agreement with Franklin there).
>>
>> There are two components in configuring the secondary sample point. It is
>> the transceiver loopback delay and an offset (example half of the bit time
>> in data phase).
>>
>> While the transceiver loopback delay is pretty board dependent (and thus
>> amenable to DT encoding), I am not quite sure the offset can be configured
>> in DT because its not really board dependent.
>>
>> Unfortunately, offset calculation does not seem to be an exact science.
>> There are recommendations ranging from using 50% of bit time to making it
>> same as the sample point configured. This means users who need to change
>> the SSP due to offset variations need to change their DT even without
>> anything changing on their board.
>>
>> Since we have a netlink socket interface to configure sample point, I
>> wonder if that should be extended to configure SSP too (or at least the
>> offset part of SSP)?
>
> +1
> > I also wonder how a default TDCO setting in DT would help.
For a given board with a given transceiver and layout and proper values
in the supplied DT the higher bitrates would work out of the box.
> Is the driver going to hard code the Tq settings as well for the
> most commonly used bitrate?
What's Tq in this context. Time Quanta?
> A link up start-up script (using netlink) would help setting a
> default TDCO along with other params if this is the main concern.
There are various ways to setup these values. Plain old start-up scripts
are one of them, systemd-networkd or network manager are others.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH net-next v6 0/5] bpf: security: New file mode and LSM hooks for eBPF object permission control
From: Daniel Borkmann @ 2017-10-19 9:48 UTC (permalink / raw)
To: James Morris, David Miller
Cc: chenbofeng.kernel, netdev, Selinux, linux-security-module, jeffv,
alexei.starovoitov, lorenzo, sds, paul, fengc
In-Reply-To: <alpine.LFD.2.20.1710191248100.1108@t440.gateway.2wire.net>
On 10/19/2017 03:49 AM, James Morris wrote:
> On Wed, 18 Oct 2017, David Miller wrote:
>
>> Series applied.
>
> I hadn't gotten to reviewing this patchset yet.
>
> Please wait for more acks/reviews from LSM folk for things touching
> security/, next time.
It didn't get applied yet due to a build error, v7 is pending here
if you want to take a look (the series is already on the lists with
multiple feedback rounds since beginning of Oct):
http://patchwork.ozlabs.org/project/netdev/list/?series=8967
Thanks,
Daniel
^ permalink raw reply
* Re: Fwd: Netlink XFRM socket subsystem NULL pointer dereference
From: Herbert Xu @ 2017-10-19 9:57 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Steffen Klassert, Timo Teras, netdev
In-Reply-To: <20171019092625.GA12863@gondor.apana.org.au>
On Thu, Oct 19, 2017 at 05:26:25PM +0800, Herbert Xu wrote:
>
> So it's an netlink API issue. It is possible for cb->done to be
> called without cb->dump ever being called. And xfrm_user doesn't
> deal with that. Let me survey the others to see whether we should
> fix this in netlink, xfrm, or both.
OK it looks like an XFRM bug pure and simple. Funnily enough
the xfrm sa dump code which got changed by the same commit that
added this bug isn't buggy.
---8<---
Subject: ipsec: Fix aborted xfrm policy dump crash
An independent security researcher, Mohamed Ghannam, has reported
this vulnerability to Beyond Security's SecuriTeam Secure Disclosure
program.
The xfrm_dump_policy_done function expects xfrm_dump_policy to
have been called at least once or it will crash. This can be
triggered if a dump fails because the target socket's receive
buffer is full.
This patch fixes it by using the cb->start mechanism to ensure that
the initialisation is always done regardless of the buffer situation.
Fixes: 4c563f7669c1 ("[XFRM]: Speed up xfrm_policy and xfrm_state...")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 2bfbd91..98a6d65 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1692,32 +1692,34 @@ static int dump_one_policy(struct xfrm_policy *xp, int dir, int count, void *ptr
static int xfrm_dump_policy_done(struct netlink_callback *cb)
{
- struct xfrm_policy_walk *walk = (struct xfrm_policy_walk *) &cb->args[1];
+ struct xfrm_policy_walk *walk = (struct xfrm_policy_walk *)cb->args;
struct net *net = sock_net(cb->skb->sk);
xfrm_policy_walk_done(walk, net);
return 0;
}
+static int xfrm_dump_policy_start(struct netlink_callback *cb)
+{
+ struct xfrm_policy_walk *walk = (struct xfrm_policy_walk *)cb->args;
+
+ BUILD_BUG_ON(sizeof(*walk) > sizeof(cb->args));
+
+ xfrm_policy_walk_init(walk, XFRM_POLICY_TYPE_ANY);
+ return 0;
+}
+
static int xfrm_dump_policy(struct sk_buff *skb, struct netlink_callback *cb)
{
struct net *net = sock_net(skb->sk);
- struct xfrm_policy_walk *walk = (struct xfrm_policy_walk *) &cb->args[1];
+ struct xfrm_policy_walk *walk = (struct xfrm_policy_walk *)cb->args;
struct xfrm_dump_info info;
- BUILD_BUG_ON(sizeof(struct xfrm_policy_walk) >
- sizeof(cb->args) - sizeof(cb->args[0]));
-
info.in_skb = cb->skb;
info.out_skb = skb;
info.nlmsg_seq = cb->nlh->nlmsg_seq;
info.nlmsg_flags = NLM_F_MULTI;
- if (!cb->args[0]) {
- cb->args[0] = 1;
- xfrm_policy_walk_init(walk, XFRM_POLICY_TYPE_ANY);
- }
-
(void) xfrm_policy_walk(net, walk, dump_one_policy, &info);
return skb->len;
@@ -2473,6 +2475,7 @@ static int xfrm_send_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
static const struct xfrm_link {
int (*doit)(struct sk_buff *, struct nlmsghdr *, struct nlattr **);
+ int (*start)(struct netlink_callback *);
int (*dump)(struct sk_buff *, struct netlink_callback *);
int (*done)(struct netlink_callback *);
const struct nla_policy *nla_pol;
@@ -2486,6 +2489,7 @@ static int xfrm_send_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
[XFRM_MSG_NEWPOLICY - XFRM_MSG_BASE] = { .doit = xfrm_add_policy },
[XFRM_MSG_DELPOLICY - XFRM_MSG_BASE] = { .doit = xfrm_get_policy },
[XFRM_MSG_GETPOLICY - XFRM_MSG_BASE] = { .doit = xfrm_get_policy,
+ .start = xfrm_dump_policy_start,
.dump = xfrm_dump_policy,
.done = xfrm_dump_policy_done },
[XFRM_MSG_ALLOCSPI - XFRM_MSG_BASE] = { .doit = xfrm_alloc_userspi },
@@ -2538,6 +2542,7 @@ static int xfrm_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
{
struct netlink_dump_control c = {
+ .start = link->start,
.dump = link->dump,
.done = link->done,
};
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply related
* Re: [net-next V8 PATCH 3/5] bpf: cpumap xdp_buff to skb conversion and allocation
From: Jesper Dangaard Brouer @ 2017-10-19 10:10 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, jakub.kicinski, pavel.odintsov, Jason Wang, mchan,
John Fastabend, peter.waskiewicz.jr, ast, Daniel Borkmann,
Alexei Starovoitov, Andy Gospodarek, brouer
In-Reply-To: <20171018165207-mutt-send-email-mst@kernel.org>
On Wed, 18 Oct 2017 17:12:09 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Oct 16, 2017 at 12:19:39PM +0200, Jesper Dangaard Brouer wrote:
> > @@ -191,15 +280,45 @@ static int cpu_map_kthread_run(void *data)
> > * kthread_stop signal until queue is empty.
> > */
> > while (!kthread_should_stop() || !__ptr_ring_empty(rcpu->queue)) {
> > + unsigned int processed = 0, drops = 0;
> > struct xdp_pkt *xdp_pkt;
> >
> > - schedule();
> > - /* Do work */
> > - while ((xdp_pkt = ptr_ring_consume(rcpu->queue))) {
> > - /* For now just "refcnt-free" */
> > - page_frag_free(xdp_pkt);
> > + /* Release CPU reschedule checks */
> > + if (__ptr_ring_empty(rcpu->queue)) {
>
>
> I suspect this is racy: if ring becomes non empty here and
> you wake the task, next line will put it to sleep.
> I think you want to reverse the order:
>
> __set_current_state(TASK_INTERRUPTIBLE);
>
> then check __ptr_ring_empty.
I'll look into this.
The window will be minimal, as __cpu_map_flush() after the last packets
enqueue will call wake_up_process(rcpu->kthread). But I guess there is
still small race possible. Worst case, a packet could be stuck in the
queue until a new packet arrive. Thanks for spotting this.
> I note using the __ version means you can not resize the ring.
> Hope you do not need to.
Resize is not supported. If user change the queue size, a new ptr_ring
and kthread is created, and logic assured the old ptr_ring and kthread
flush packets appropriately (this is tested with the --stress-mode).
> > + __set_current_state(TASK_INTERRUPTIBLE);
> > + schedule();
> > + } else {
> > + cond_resched();
> > + }
> > + __set_current_state(TASK_RUNNING);
> > +
> > + /* Process packets in rcpu->queue */
> > + local_bh_disable();
> > + /*
> > + * The bpf_cpu_map_entry is single consumer, with this
> > + * kthread CPU pinned. Lockless access to ptr_ring
> > + * consume side valid as no-resize allowed of queue.
> > + */
> > + while ((xdp_pkt = __ptr_ring_consume(rcpu->queue))) {
> > + struct sk_buff *skb;
> > + int ret;
> > +
> > + skb = cpu_map_build_skb(rcpu, xdp_pkt);
> > + if (!skb) {
> > + page_frag_free(xdp_pkt);
> > + continue;
> > + }
> > +
> > + /* Inject into network stack */
> > + ret = netif_receive_skb_core(skb);
> > + if (ret == NET_RX_DROP)
> > + drops++;
> > +
> > + /* Limit BH-disable period */
> > + if (++processed == 8)
> > + break;
> > }
> > - __set_current_state(TASK_INTERRUPTIBLE);
> > + local_bh_enable(); /* resched point, may call do_softirq() */
> > }
> > __set_current_state(TASK_RUNNING);
> >
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* RE: [PATCH 5/7] devlink: Adding pre-boot permanent config parameters
From: Yuval Mintz @ 2017-10-19 10:30 UTC (permalink / raw)
To: Steve Lin, netdev@vger.kernel.org
Cc: Jiri Pirko, davem@davemloft.net, michael.chan@broadcom.com,
linville@tuxdriver.com, gospo@broadcom.com
In-Reply-To: <1508273069-40461-6-git-send-email-steven.lin1@broadcom.com>
> DEVLINK_ATTR_PERM_CFG_MBA_LINK_SPEED: Configured link speed
> while executing MBA host software (PXI/iSCSI); use enum
> devlink_mba_link_speed.
#4 introduces:
> DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D0: Configure default
> pre-OS link speed in full power (D0) state; use enum
> devlink_pre_os_link_speed.
> DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D3: Configure default
> pre-OS link speed in sleep (D3) state; use enum
> devlink_pre_os_link_speed.
Why would user need an additional value for the MBA speed?
^ permalink raw reply
* Re: [PATCH net-next v2 2/2] net: ethernet: socionext: add AVE ethernet driver
From: Kunihiko Hayashi @ 2017-10-19 10:35 UTC (permalink / raw)
To: Masahiro Yamada
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, Andrew Lunn, Florian Fainelli,
Rob Herring, Mark Rutland, linux-arm-kernel,
Linux Kernel Mailing List, devicetree-u79uwXL29TY76Z2rM5mHXA,
Masami Hiramatsu, Jassi Brar
In-Reply-To: <CAK7LNARuoK-M9iKyP7JxRgH7naA+teikqZMOHm3f2wykEieqLw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Thu, 19 Oct 2017 09:29:03 +0900 <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:
> 2017-10-18 19:23 GMT+09:00 Kunihiko Hayashi <hayashi.kunihiko-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>:
> > On Mon, 16 Oct 2017 00:08:21 +0900 <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:
>
> >> priv->rst = devm_reset_control_get_optional_shared(dev, NULL);
> >> if (IS_ERR(priv->rst))
> >> return PTR_ERR(priv->rst);
> >
> > The clk and reset are optional in the driver.
> > Referring to your suggested method, I'll fix the part of clk and reset.
> >
>
>
> Why is clk optional?
Indeed. My check point was wrong.
This clk property should be treated as "required".
In v2, the clock enabling code moved to ndo_init().
Although probing the driver succeeds without clk,
enabling the driver fails clearly.
---
Best Regards,
Kunihiko Hayashi
--
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 3/7] devlink: Adding high level dev perm config params
From: Yuval Mintz @ 2017-10-19 10:36 UTC (permalink / raw)
To: Steve Lin, netdev@vger.kernel.org
Cc: Jiri Pirko, davem@davemloft.net, michael.chan@broadcom.com,
linville@tuxdriver.com, gospo@broadcom.com
In-Reply-To: <1508273069-40461-4-git-send-email-steven.lin1@broadcom.com>
> DEVLINK_ATTR_PERM_CFG_MULTIFUNC_MODE: Configure multi-function
> mode; use devlink_multifunc_mode.
...
> +enum devlink_multifunc_mode {
> + DEVLINK_MULTIFUNC_MODE_ALLOWED, /* Ext switch
> activates MF */
> + DEVLINK_MULTIFUNC_MODE_FORCE_SINGFUNC,
> + DEVLINK_MULTIFUNC_MODE_NPAR10, /* NPAR 1.0
> */
> + DEVLINK_MULTIFUNC_MODE_NPAR15, /* NPAR 1.5
> */
> + DEVLINK_MULTIFUNC_MODE_NPAR20, /* NPAR 2.0
> */
> +};
... And when someone would invent a new partitioning scheme, what then?
You'd extend the 'generic' modes?
It's not very generic.
> DEVLINK_ATTR_PERM_CFG_SECURE_NIC_ENABLED: 1 to enable Secure NIC
> functionality, 0 to disable.
What does it mean? Is there some spec explaining it?
^ permalink raw reply
* RE: [PATCH 2/7] devlink: Adding NPAR permanent config parameters
From: Yuval Mintz @ 2017-10-19 10:39 UTC (permalink / raw)
To: Steve Lin, netdev@vger.kernel.org
Cc: Jiri Pirko, davem@davemloft.net, michael.chan@broadcom.com,
linville@tuxdriver.com, gospo@broadcom.com
In-Reply-To: <1508273069-40461-3-git-send-email-steven.lin1@broadcom.com>
> DEVLINK_ATTR_PERM_CFG_NPAR_BW_RESERVATION_VALID: 1 to use
> BW_RESERVATION setting, 0 to ignore.
>
...
> DEVLINK_ATTR_PERM_CFG_NPAR_BW_LIMIT_VALID: 1 to use BW_LIMIT
> setting, 0 to ignore.
While it probably ties to different fields in your NVM layout why would the user
require specific attributes for these? Why not have values in the actual
attributes indicating of this status?
^ permalink raw reply
* Re: [net-next 00/15][pull request] 40GbE Intel Wired LAN Driver Updates 2017-10-17
From: David Miller @ 2017-10-19 10:50 UTC (permalink / raw)
To: jeffrey.t.kirsher; +Cc: netdev, nhorman, sassmann, jogreene
In-Reply-To: <20171017182119.71989-1-jeffrey.t.kirsher@intel.com>
From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Tue, 17 Oct 2017 11:21:04 -0700
> This series contains updates to i40e and ethtool.
>
> Alan provides most of the changes in this series which are mainly fixes
> and cleanups. Renamed the ethtool "cmd" variable to "ks", since the new
> ethtool API passes us ksettings structs instead of command structs.
> Cleaned up an ifdef that was not accomplishing anything. Added function
> header comments to provide better documentation. Fixed two issues in
> i40e_get_link_ksettings(), by calling
> ethtool_link_ksettings_zero_link_mode() to ensure the advertising and
> link masks are cleared before we start setting bits. Cleaned up and fixed
> code comments which were incorrect. Separated the setting of autoneg in
> i40e_phy_types_to_ethtool() into its own conditional to clarify what PHYs
> support and advertise autoneg, and makes it easier to add new PHY types in
> the future. Added ethtool functionality to intersect two link masks
> together to find the common ground between them. Overhauled i40e to
> ensure that the new ethtool API macros are being used, instead of the
> old ones. Fixed the usage of unsigned 64-bit division which is not
> supported on all architectures.
>
> Sudheer adds support for 25G Active Optical Cables (AOC) and Active Copper
> Cables (ACC) PHY types.
Pulled, thanks Jeff.
^ permalink raw reply
* Re: [PATCH] lib/dynamic_queue_limits.c: relax BUG_ON to WARN_ON in dql_complete()
From: David Miller @ 2017-10-19 10:57 UTC (permalink / raw)
To: ard.biesheuvel; +Cc: linux-kernel, netdev
In-Reply-To: <20171018154515.16751-1-ard.biesheuvel@linaro.org>
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date: Wed, 18 Oct 2017 16:45:15 +0100
> Even though calling dql_completed() with a count that exceeds the
> queued count is a serious error, it still does not justify bringing
> down the entire kernel with a BUG_ON(). So relax it to a WARN_ON()
> instead.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
This is bogus.
Unless you are going to do all of the work necessary to fix
the out-of-bounds condition here, you cannot safely continue
into the rest of this function.
Things are going to explode in many places if you don't, at
a minimum, fix the 'count' value to be in range.
But like others I don't like this, the driver needs to be fixed
urgently if this condition triggers.
Sorry I'm not applying this.
^ permalink raw reply
* Re: [PATCH net-next v6 0/5] bpf: security: New file mode and LSM hooks for eBPF object permission control
From: David Miller @ 2017-10-19 11:00 UTC (permalink / raw)
To: james.l.morris
Cc: chenbofeng.kernel, netdev, Selinux, linux-security-module, jeffv,
alexei.starovoitov, lorenzo, daniel, sds, paul, fengc
In-Reply-To: <alpine.LFD.2.20.1710191248100.1108@t440.gateway.2wire.net>
From: James Morris <james.l.morris@oracle.com>
Date: Thu, 19 Oct 2017 12:49:59 +1100 (AEDT)
> On Wed, 18 Oct 2017, David Miller wrote:
>
>> Series applied.
>
> I hadn't gotten to reviewing this patchset yet.
>
> Please wait for more acks/reviews from LSM folk for things touching
> security/, next time.
Well it got reverted and respun in order to deal with build failures.
So you have some time.
However...
Please review the changes more expediantly. I don't like paches
rotting in my backlog for more than a day or two, weekends not
withstanding. So if I don't see anything after two work week days,
I'll use my best judgment and apply if I can't find a major flaw in
the change(s).
^ permalink raw reply
* Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates
From: Sekhar Nori @ 2017-10-19 11:09 UTC (permalink / raw)
To: Marc Kleine-Budde, Franklin S Cooper Jr, Mario Hüttel,
Yang, Wenyou, wg, socketcan, quentin.schulz, edumazet, linux-can,
netdev, linux-kernel
Cc: Wenyou Yang, Dong Aisheng, Quadros, Roger
In-Reply-To: <545ac690-10c8-b206-76dd-dbbe602eb6af@pengutronix.de>
On Thursday 19 October 2017 02:43 PM, Marc Kleine-Budde wrote:
> On 10/19/2017 07:07 AM, Sekhar Nori wrote:
>>>>> Sounds reasonable. What's the status of this series?
>>>>
>>>> I have had some offline discussions with Franklin on this, and I am not
>>>> fully convinced that DT is the way to go here (although I don't have the
>>>> agreement with Franklin there).
>>>
>>> Probably the fundamental area where we disagree is what "default" SSP
>>> value should be used. Based on a short (< 1 ft) point to point test
>>> using a SSP of 50% worked fine. However, I'm not convinced that this
>>> default value of 50% will work in a more "traditional" CAN bus at higher
>>> speeds. Nor am I convinced that a SSP of 50% will work on every MCAN
>>> board in even the simplest test cases.
>>>
>>> I believe that this default SSP should be a DT property that allows any
>>> board to determine what default value works best in general.
>>
>> With that, I think, we are taking DT from describing board/hardware
>> characteristics to providing default values that software should use.
>
> If the default value is board specific and cannot be calculated in
> general or from other values present in the DT, then it's from my point
> of view describing the hardware.
>
>> In any case, if Marc and/or Wolfgang are okay with it, binding
>> documentation for such a property should be sent to DT maintainers for
>> review.
>>
>>>>
>>>> There are two components in configuring the secondary sample point. It
>>>> is the transceiver loopback delay and an offset (example half of the bit
>>>> time in data phase).
>>>>
>>>> While the transceiver loopback delay is pretty board dependent (and thus
>>>> amenable to DT encoding), I am not quite sure the offset can be
>>>> configured in DT because its not really board dependent.
>>>>
>>>> Unfortunately, offset calculation does not seem to be an exact science.
>>>> There are recommendations ranging from using 50% of bit time to making
>>>> it same as the sample point configured. This means users who need to
>>>> change the SSP due to offset variations need to change their DT even
>>>> without anything changing on their board.
>>>>
>>>> Since we have a netlink socket interface to configure sample point, I
>>>> wonder if that should be extended to configure SSP too (or at least the
>>>> offset part of SSP)?
>>>
>>> Sekhar is right that ideally the user should be able to set the SSP at
>>> runtime. However, my issue is that based on my experience CAN users
>>> expect the driver to just work the majority of times. For unique use
>>> cases where the driver calculated values don't work then the user should
>>> be able to override it. This should only be done for a very small
>>> percentage of CAN users. Unless you allow DT to provide a default SSP
>>> many users of MCAN may find that the default SSP doesn't work and must
>>> always use runtime overrides to get anything to work. I don't think that
>>> is a good user experience which is why I don't like the idea.
>>
>> Fair enough. But not quite sure if CAN users expect CAN-FD to "just
>> work" without doing any bittiming related setup.
>
> From my point of view I'd rather buy a board from a HW vendor where
> CAN-FD works, rather than a board where I have to tweak the bit-timing
> for a simple CAN-FD test setup.
>
> As far as I see for the m_can driver it's a single tuple: "bitrate > 2.5
> MBit/s -> 50%". Do we need an array of tuples in general?
>
> If good default values are transceiver and board specific, they can go
> into the DT. We need a generic (this means driver agnostic) binding for
> this. If this table needs to be tweaked for special purpose, then we can
> add a netlink interface for this as well.
>
> Comments?
I dont know how a good default (other than 50% as the starting point)
can be arrived at without doing any actual measurements on the actual
network. Since we do know that the value has to be tweaked, agree that
netlink interface has to be provided.
I wonder whether even if a DT binding for default is provided, everyone
will end up setting it to 50% (since there is no way for them to predict
any better). In effect, I am suggesting using a hardcoded value of 50%
instead of introducing a binding without a clear need for it.
Note that I am only talking about there "offset" part of SSP here. The
transceiver loopback delay is calculated automatically by Bosch's MCAN
IP. But if there are other IPs which don't do that, then yes, that
should be a DT property IMO.
Thanks,
Sekhar
^ permalink raw reply
* Re: [RFC] ip: introduce IFA_F_DHCP flag
From: David Miller @ 2017-10-19 11:13 UTC (permalink / raw)
To: girish.moodalbail; +Cc: netdev
In-Reply-To: <1508350606-21218-1-git-send-email-girish.moodalbail@oracle.com>
From: Girish Moodalbail <girish.moodalbail@oracle.com>
Date: Wed, 18 Oct 2017 11:16:46 -0700
> This flag identifies that the address was obtained through DHCP.
>
> Today there is no easy way to find out whether an address on an
> interface is DHCP controlled or is static. Either you will need to
> grep for 'dhclient' process (or something else in case one is using a
> different DHCP client) or if you are using NetworkManager (or some
> such), then you will need to query through their interface to find out
> if an address is DHCP or not.
>
> This flag will be set by the DHCP clients in the userspace when it
> brings up the DHCP address on an interface. For example: ISC DHCP
> client (aka dhclient) today brings up the address on an interface by
> running ip-address(8) command (in dhclient-script). This command can
> be extended to include 'dhcp' keyword in its 'add' or 'replace'
> subcommand. Once this flag is set, the show subcommand can display the
> keyword 'dhcp' against the address to indicate that the address was
> obtained through DHCP.
>
> This flag can also be set and obtained programmatically using
> AF_NETLINK. Besides providing observability, this flag will be useful
> for applications that need to prevent/allow certain settings on
> addresses based on whether they are DHCP or not.
>
> Signed-off-by: Girish Moodalbail <girish.moodalbail@oracle.com>
Well, two things.
if nobody has asked for this for 25+ years, I have serious doubts that
there is something fundamental you cannot do without this piece of
information.
Second, a lack of this flag being set won't mean "is static", it will
mean "might be STATIC, might be DHCP" and frankly that is completely
useless. It is this way because you cannot have every dhcp agent on
every Linux system changed immediately.
I think it will give a false sense of accuracy, even on tightly
controlled sets of userspace components.
I'm not applying this, sorry.
^ permalink raw reply
* Re: [PATCH] lib/dynamic_queue_limits.c: relax BUG_ON to WARN_ON in dql_complete()
From: Ard Biesheuvel @ 2017-10-19 11:14 UTC (permalink / raw)
To: David Miller; +Cc: linux-kernel@vger.kernel.org, <netdev@vger.kernel.org>
In-Reply-To: <20171019.115756.1156159427863267036.davem@davemloft.net>
On 19 October 2017 at 11:57, David Miller <davem@davemloft.net> wrote:
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Date: Wed, 18 Oct 2017 16:45:15 +0100
>
>> Even though calling dql_completed() with a count that exceeds the
>> queued count is a serious error, it still does not justify bringing
>> down the entire kernel with a BUG_ON(). So relax it to a WARN_ON()
>> instead.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> This is bogus.
>
> Unless you are going to do all of the work necessary to fix
> the out-of-bounds condition here, you cannot safely continue
> into the rest of this function.
>
> Things are going to explode in many places if you don't, at
> a minimum, fix the 'count' value to be in range.
>
> But like others I don't like this, the driver needs to be fixed
> urgently if this condition triggers.
>
> Sorry I'm not applying this.
Fair enough.
^ permalink raw reply
* Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates
From: Oliver Hartkopp @ 2017-10-19 11:14 UTC (permalink / raw)
To: Marc Kleine-Budde, Sekhar Nori, Franklin S Cooper Jr,
Mario Hüttel, Yang, Wenyou, wg, quentin.schulz, edumazet,
linux-can, netdev, linux-kernel
Cc: Wenyou Yang, Dong Aisheng, Quadros, Roger
In-Reply-To: <545ac690-10c8-b206-76dd-dbbe602eb6af@pengutronix.de>
On 10/19/2017 11:13 AM, Marc Kleine-Budde wrote:
> On 10/19/2017 07:07 AM, Sekhar Nori wrote:
>>>> Since we have a netlink socket interface to configure sample point, I
>>>> wonder if that should be extended to configure SSP too (or at least the
>>>> offset part of SSP)?
+1 too
>>>
>>> Sekhar is right that ideally the user should be able to set the SSP at
>>> runtime. However, my issue is that based on my experience CAN users
>>> expect the driver to just work the majority of times. For unique use
>>> cases where the driver calculated values don't work then the user should
>>> be able to override it. This should only be done for a very small
>>> percentage of CAN users. Unless you allow DT to provide a default SSP
>>> many users of MCAN may find that the default SSP doesn't work and must
>>> always use runtime overrides to get anything to work. I don't think that
>>> is a good user experience which is why I don't like the idea.
>>
>> Fair enough. But not quite sure if CAN users expect CAN-FD to "just
>> work" without doing any bittiming related setup.
>
> From my point of view I'd rather buy a board from a HW vendor where
> CAN-FD works, rather than a board where I have to tweak the bit-timing
> for a simple CAN-FD test setup.
>
> As far as I see for the m_can driver it's a single tuple: "bitrate > 2.5
> MBit/s -> 50%". Do we need an array of tuples in general?
>
> If good default values are transceiver and board specific, they can go
> into the DT. We need a generic (this means driver agnostic) binding for
> this. If this table needs to be tweaked for special purpose, then we can
> add a netlink interface for this as well. >
> Comments?
By now we calculate reasonable default values (e.g. for SP and SJW), you
can override by setting alternative values via netlink configuration.
I would tend to stay on this approach and not hide these things in DTs -
just because of someone wants to initialize his specific interface 'easier'.
One tool, one place is IMHO still the best option.
Regards,
Oliver
^ permalink raw reply
* [PATCH 01/11] can: flexcan: fix state transition regression
From: Marc Kleine-Budde @ 2017-10-19 11:19 UTC (permalink / raw)
To: netdev
Cc: davem, linux-can, kernel, ZHU Yi (ST-FIR/ENG1-Zhu), Zhu Yi,
Mark Jonas, linux-stable, Marc Kleine-Budde
In-Reply-To: <20171019111945.13580-1-mkl@pengutronix.de>
From: "ZHU Yi (ST-FIR/ENG1-Zhu)" <Yi.Zhu5@cn.bosch.com>
Update state upon any interrupt to report correct state transitions in
case the flexcan core enabled the broken error state quirk fix.
Signed-off-by: Zhu Yi <yi.zhu5@cn.bosch.com>
Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
Acked-by: Wolfgang Grandegger <wg@grandegger.com>
Cc: linux-stable <stable@vger.kernel.org> # >= v4.11
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/flexcan.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 13f0f219d8aa..df4bfb83024c 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -765,8 +765,9 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
flexcan_write(reg_esr & FLEXCAN_ESR_ALL_INT, ®s->esr);
}
- /* state change interrupt */
- if (reg_esr & FLEXCAN_ESR_ERR_STATE)
+ /* state change interrupt or broken error state quirk fix is enabled */
+ if ((reg_esr & FLEXCAN_ESR_ERR_STATE) ||
+ (priv->devtype_data->quirks & FLEXCAN_QUIRK_BROKEN_ERR_STATE))
flexcan_irq_state(dev, reg_esr);
/* bus error IRQ - handle if bus error reporting is activated */
--
2.14.2
^ permalink raw reply related
* [PATCH 02/11] can: flexcan: rename legacy error state quirk
From: Marc Kleine-Budde @ 2017-10-19 11:19 UTC (permalink / raw)
To: netdev
Cc: davem, linux-can, kernel, ZHU Yi (ST-FIR/ENG1-Zhu), Zhu Yi,
Mark Jonas, linux-stable, Marc Kleine-Budde
In-Reply-To: <20171019111945.13580-1-mkl@pengutronix.de>
From: "ZHU Yi (ST-FIR/ENG1-Zhu)" <Yi.Zhu5@cn.bosch.com>
Rename FLEXCAN_QUIRK_BROKEN_ERR_STATE to FLEXCAN_QUIRK_BROKEN_WERR_STATE
for better description of the missing [TR]WRN_INT quirk.
Signed-off-by: Zhu Yi <yi.zhu5@cn.bosch.com>
Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
Acked-by: Wolfgang Grandegger <wg@grandegger.com>
Cc: linux-stable <stable@vger.kernel.org> # >= v4.11
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/flexcan.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index df4bfb83024c..e163c55e737b 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -193,7 +193,7 @@
*
* Some SOCs do not have the RX_WARN & TX_WARN interrupt line connected.
*/
-#define FLEXCAN_QUIRK_BROKEN_ERR_STATE BIT(1) /* [TR]WRN_INT not connected */
+#define FLEXCAN_QUIRK_BROKEN_WERR_STATE BIT(1) /* [TR]WRN_INT not connected */
#define FLEXCAN_QUIRK_DISABLE_RXFG BIT(2) /* Disable RX FIFO Global mask */
#define FLEXCAN_QUIRK_ENABLE_EACEN_RRS BIT(3) /* Enable EACEN and RRS bit in ctrl2 */
#define FLEXCAN_QUIRK_DISABLE_MECR BIT(4) /* Disable Memory error detection */
@@ -281,7 +281,7 @@ struct flexcan_priv {
};
static const struct flexcan_devtype_data fsl_p1010_devtype_data = {
- .quirks = FLEXCAN_QUIRK_BROKEN_ERR_STATE,
+ .quirks = FLEXCAN_QUIRK_BROKEN_WERR_STATE,
};
static const struct flexcan_devtype_data fsl_imx28_devtype_data;
@@ -767,7 +767,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
/* state change interrupt or broken error state quirk fix is enabled */
if ((reg_esr & FLEXCAN_ESR_ERR_STATE) ||
- (priv->devtype_data->quirks & FLEXCAN_QUIRK_BROKEN_ERR_STATE))
+ (priv->devtype_data->quirks & FLEXCAN_QUIRK_BROKEN_WERR_STATE))
flexcan_irq_state(dev, reg_esr);
/* bus error IRQ - handle if bus error reporting is activated */
@@ -888,7 +888,7 @@ static int flexcan_chip_start(struct net_device *dev)
* on most Flexcan cores, too. Otherwise we don't get
* any error warning or passive interrupts.
*/
- if (priv->devtype_data->quirks & FLEXCAN_QUIRK_BROKEN_ERR_STATE ||
+ if (priv->devtype_data->quirks & FLEXCAN_QUIRK_BROKEN_WERR_STATE ||
priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
reg_ctrl |= FLEXCAN_CTRL_ERR_MSK;
else
--
2.14.2
^ permalink raw reply related
* [PATCH 03/11] can: flexcan: implement error passive state quirk
From: Marc Kleine-Budde @ 2017-10-19 11:19 UTC (permalink / raw)
To: netdev
Cc: davem, linux-can, kernel, ZHU Yi (ST-FIR/ENG1-Zhu), Zhu Yi,
Mark Jonas, linux-stable, Marc Kleine-Budde
In-Reply-To: <20171019111945.13580-1-mkl@pengutronix.de>
From: "ZHU Yi (ST-FIR/ENG1-Zhu)" <Yi.Zhu5@cn.bosch.com>
Add FLEXCAN_QUIRK_BROKEN_PERR_STATE for better description of the
missing error passive interrupt quirk.
Error interrupt flooding may happen if the broken error state quirk fix
is enabled. For example, in case there is singled out node on the bus
and the node sends a frame, then error interrupt flooding happens and
will not stop because the node cannot go to bus off. The flooding will
stop after another node connected to the bus again.
If high bitrate configured on the low end system, then the flooding
may causes performance issue, hence, this patch mitigates this by:
1. disable error interrupt upon error passive state transition
2. re-enable error interrupt upon error warning state transition
3. disable/enable error interrupt upon error active state transition
depends on FLEXCAN_QUIRK_BROKEN_WERR_STATE
In this way, the driver is still able to report correct state
transitions without additional latency. When there are bus problems,
flooding of error interrupts is limited to the number of frames required
to change state from error warning to error passive if the core has
[TR]WRN_INT connected (FLEXCAN_QUIRK_BROKEN_WERR_STATE is not enabled),
otherwise, the flooding is limited to the number of frames required to
change state from error active to error passive.
Signed-off-by: Zhu Yi <yi.zhu5@cn.bosch.com>
Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
Acked-by: Wolfgang Grandegger <wg@grandegger.com>
Cc: linux-stable <stable@vger.kernel.org> # >= v4.11
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/flexcan.c | 75 +++++++++++++++++++++++++++++++++++++++++------
1 file changed, 66 insertions(+), 9 deletions(-)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index e163c55e737b..c83a09fa4166 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -182,14 +182,14 @@
/* FLEXCAN hardware feature flags
*
* Below is some version info we got:
- * SOC Version IP-Version Glitch- [TR]WRN_INT Memory err RTR re-
- * Filter? connected? detection ception in MB
- * MX25 FlexCAN2 03.00.00.00 no no no no
- * MX28 FlexCAN2 03.00.04.00 yes yes no no
- * MX35 FlexCAN2 03.00.00.00 no no no no
- * MX53 FlexCAN2 03.00.00.00 yes no no no
- * MX6s FlexCAN3 10.00.12.00 yes yes no yes
- * VF610 FlexCAN3 ? no yes yes yes?
+ * SOC Version IP-Version Glitch- [TR]WRN_INT IRQ Err Memory err RTR re-
+ * Filter? connected? Passive detection ception in MB
+ * MX25 FlexCAN2 03.00.00.00 no no ? no no
+ * MX28 FlexCAN2 03.00.04.00 yes yes no no no
+ * MX35 FlexCAN2 03.00.00.00 no no ? no no
+ * MX53 FlexCAN2 03.00.00.00 yes no no no no
+ * MX6s FlexCAN3 10.00.12.00 yes yes no no yes
+ * VF610 FlexCAN3 ? no yes ? yes yes?
*
* Some SOCs do not have the RX_WARN & TX_WARN interrupt line connected.
*/
@@ -198,6 +198,7 @@
#define FLEXCAN_QUIRK_ENABLE_EACEN_RRS BIT(3) /* Enable EACEN and RRS bit in ctrl2 */
#define FLEXCAN_QUIRK_DISABLE_MECR BIT(4) /* Disable Memory error detection */
#define FLEXCAN_QUIRK_USE_OFF_TIMESTAMP BIT(5) /* Use timestamp based offloading */
+#define FLEXCAN_QUIRK_BROKEN_PERR_STATE BIT(6) /* No interrupt for error passive */
/* Structure of the message buffer */
struct flexcan_mb {
@@ -335,6 +336,22 @@ static inline void flexcan_write(u32 val, void __iomem *addr)
}
#endif
+static inline void flexcan_error_irq_enable(const struct flexcan_priv *priv)
+{
+ struct flexcan_regs __iomem *regs = priv->regs;
+ u32 reg_ctrl = (priv->reg_ctrl_default | FLEXCAN_CTRL_ERR_MSK);
+
+ flexcan_write(reg_ctrl, ®s->ctrl);
+}
+
+static inline void flexcan_error_irq_disable(const struct flexcan_priv *priv)
+{
+ struct flexcan_regs __iomem *regs = priv->regs;
+ u32 reg_ctrl = (priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_MSK);
+
+ flexcan_write(reg_ctrl, ®s->ctrl);
+}
+
static inline int flexcan_transceiver_enable(const struct flexcan_priv *priv)
{
if (!priv->reg_xceiver)
@@ -713,6 +730,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
struct flexcan_regs __iomem *regs = priv->regs;
irqreturn_t handled = IRQ_NONE;
u32 reg_iflag1, reg_esr;
+ enum can_state last_state = priv->can.state;
reg_iflag1 = flexcan_read(®s->iflag1);
@@ -767,7 +785,8 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
/* state change interrupt or broken error state quirk fix is enabled */
if ((reg_esr & FLEXCAN_ESR_ERR_STATE) ||
- (priv->devtype_data->quirks & FLEXCAN_QUIRK_BROKEN_WERR_STATE))
+ (priv->devtype_data->quirks & (FLEXCAN_QUIRK_BROKEN_WERR_STATE |
+ FLEXCAN_QUIRK_BROKEN_PERR_STATE)))
flexcan_irq_state(dev, reg_esr);
/* bus error IRQ - handle if bus error reporting is activated */
@@ -775,6 +794,44 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
(priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING))
flexcan_irq_bus_err(dev, reg_esr);
+ /* availability of error interrupt among state transitions in case
+ * bus error reporting is de-activated and
+ * FLEXCAN_QUIRK_BROKEN_PERR_STATE is enabled:
+ * +--------------------------------------------------------------+
+ * | +----------------------------------------------+ [stopped / |
+ * | | | sleeping] -+
+ * +-+-> active <-> warning <-> passive -> bus off -+
+ * ___________^^^^^^^^^^^^_______________________________
+ * disabled(1) enabled disabled
+ *
+ * (1): enabled if FLEXCAN_QUIRK_BROKEN_WERR_STATE is enabled
+ */
+ if ((last_state != priv->can.state) &&
+ (priv->devtype_data->quirks & FLEXCAN_QUIRK_BROKEN_PERR_STATE) &&
+ !(priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)) {
+ switch (priv->can.state) {
+ case CAN_STATE_ERROR_ACTIVE:
+ if (priv->devtype_data->quirks &
+ FLEXCAN_QUIRK_BROKEN_WERR_STATE)
+ flexcan_error_irq_enable(priv);
+ else
+ flexcan_error_irq_disable(priv);
+ break;
+
+ case CAN_STATE_ERROR_WARNING:
+ flexcan_error_irq_enable(priv);
+ break;
+
+ case CAN_STATE_ERROR_PASSIVE:
+ case CAN_STATE_BUS_OFF:
+ flexcan_error_irq_disable(priv);
+ break;
+
+ default:
+ break;
+ }
+ }
+
return handled;
}
--
2.14.2
^ permalink raw reply related
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