* pull request (net): ipsec 2015-01-26
From: Steffen Klassert @ 2015-01-26 7:48 UTC (permalink / raw)
To: David Miller; +Cc: Herbert Xu, Steffen Klassert, netdev
Just two small fixes for _decode_session6() where we
might decode to wrong header information in some rare
situations.
Please pull or let me know if there are problems.
Thanks!
The following changes since commit f2a01517f2a1040a0b156f171a7cefd748f2fd03:
openvswitch: Fix flow mask validation. (2014-12-05 21:42:16 -0800)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git master
for you to fetch changes up to f855691975bb06373a98711e4cfe2c224244b536:
xfrm6: Fix the nexthdr offset in _decode_session6. (2014-12-08 07:56:18 +0100)
----------------------------------------------------------------
Steffen Klassert (2):
xfrm6: Fix transport header offset in _decode_session6.
xfrm6: Fix the nexthdr offset in _decode_session6.
net/ipv6/xfrm6_policy.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
^ permalink raw reply
* [PATCH 2/2] xfrm6: Fix the nexthdr offset in _decode_session6.
From: Steffen Klassert @ 2015-01-26 7:48 UTC (permalink / raw)
To: David Miller; +Cc: Herbert Xu, Steffen Klassert, netdev
In-Reply-To: <1422258503-26978-1-git-send-email-steffen.klassert@secunet.com>
xfrm_decode_session() was originally designed for the
usage in the receive path where the correct nexthdr offset
is stored in IP6CB(skb)->nhoff. Over time this function
spread to code that is used in the output path (netfilter,
vti) where IP6CB(skb)->nhoff is not set. As a result, we
get a wrong nexthdr and the upper layer flow informations
are wrong. This can leed to incorrect policy lookups.
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
net/ipv6/xfrm6_policy.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
index aa48302..48bf5a0 100644
--- a/net/ipv6/xfrm6_policy.c
+++ b/net/ipv6/xfrm6_policy.c
@@ -134,8 +134,14 @@ _decode_session6(struct sk_buff *skb, struct flowi *fl, int reverse)
u16 offset = sizeof(*hdr);
struct ipv6_opt_hdr *exthdr;
const unsigned char *nh = skb_network_header(skb);
- u8 nexthdr = nh[IP6CB(skb)->nhoff];
+ u16 nhoff = IP6CB(skb)->nhoff;
int oif = 0;
+ u8 nexthdr;
+
+ if (!nhoff)
+ nhoff = offsetof(struct ipv6hdr, nexthdr);
+
+ nexthdr = nh[nhoff];
if (skb_dst(skb))
oif = skb_dst(skb)->dev->ifindex;
--
1.9.1
^ permalink raw reply related
* [PATCH 1/2] xfrm6: Fix transport header offset in _decode_session6.
From: Steffen Klassert @ 2015-01-26 7:48 UTC (permalink / raw)
To: David Miller; +Cc: Herbert Xu, Steffen Klassert, netdev
In-Reply-To: <1422258503-26978-1-git-send-email-steffen.klassert@secunet.com>
skb->transport_header might not be valid when we do a reverse
decode because the ipv6 tunnel error handlers don't update it
to the inner transport header. This leads to a wrong offset
calculation and to wrong layer 4 informations. We fix this
by using the size of the ipv6 header as the first offset.
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
net/ipv6/xfrm6_policy.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
index 5f98364..aa48302 100644
--- a/net/ipv6/xfrm6_policy.c
+++ b/net/ipv6/xfrm6_policy.c
@@ -130,8 +130,8 @@ _decode_session6(struct sk_buff *skb, struct flowi *fl, int reverse)
{
struct flowi6 *fl6 = &fl->u.ip6;
int onlyproto = 0;
- u16 offset = skb_network_header_len(skb);
const struct ipv6hdr *hdr = ipv6_hdr(skb);
+ u16 offset = sizeof(*hdr);
struct ipv6_opt_hdr *exthdr;
const unsigned char *nh = skb_network_header(skb);
u8 nexthdr = nh[IP6CB(skb)->nhoff];
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] rhashtable: fix rht_for_each_entry_safe() endless loop
From: David Miller @ 2015-01-26 8:08 UTC (permalink / raw)
To: kaber; +Cc: tgraf, netdev
In-Reply-To: <1421838733-16495-1-git-send-email-kaber@trash.net>
From: Patrick McHardy <kaber@trash.net>
Date: Wed, 21 Jan 2015 11:12:13 +0000
> "next" is not updated, causing an endless loop for buckets with more than
> one element.
>
> Signed-off-by: Patrick McHardy <kaber@trash.net>
Applied, thanks Patrick.
^ permalink raw reply
* Re: [PATCH 1/2] rhashtable: Introduce rhashtable_walk_*
From: Thomas Graf @ 2015-01-26 8:20 UTC (permalink / raw)
To: Herbert Xu; +Cc: Ying Xue, davem, kaber, paulmck, netdev, netfilter-devel
In-Reply-To: <E1YFWUl-0004gE-SH@gondolin.me.apana.org.au>
On 01/26/15 at 10:21am, Herbert Xu wrote:
> +int rhashtable_walk_start(struct rhashtable_iter *iter)
> +{
> + struct rhashtable *ht = iter->ht;
> + int err;
> +
> + err = mutex_lock_interruptible(&ht->mutex);
> + rcu_read_lock();
> +
> + if (!err)
> + mutex_unlock(&ht->mutex);
> +
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(rhashtable_walk_start);
This doesn't seem to work for Netlink dumps as it would define
a RCU read side section across user read()s.
I assume you relying on RCU to defer the initial table relinking
in the resizing to be postponed. Wouldn't it be better to hold
the mutex instead, we could sleep during the dump.
> +/**
> + * rhashtable_walk_next - Return the next object and advance the iterator
> + * @iter: Hash table iterator
> + *
> + * Note that you must call rhashtable_walk_stop when you are finished
> + * with the walk.
> + *
> + * Returns the next object or NULL when the end of the table is reached.
> + */
> +void *rhashtable_walk_next(struct rhashtable_iter *iter)
> +{
> + const struct bucket_table *tbl;
> + struct rhashtable *ht = iter->ht;
> + struct rhash_head *p = iter->p;
> +
> + tbl = rht_dereference_rcu(ht->tbl, ht);
> +
> + if (p) {
> + p = rht_dereference_bucket(p->next, tbl, iter->slot);
> + goto next;
> + }
> +
> + for (; iter->slot < tbl->size; iter->slot++) {
> + int skip = iter->skip;
> +
> + iter->lock = bucket_lock(tbl, iter->slot);
> + spin_lock_bh(iter->lock);
> +
> + rht_for_each(p, tbl, iter->slot) {
> + if (!skip)
> + break;
> + skip--;
> + }
> +
> +next:
> + if (!rht_is_a_nulls(p)) {
> + iter->skip++;
> + iter->p = p;
> + return rht_obj(ht, p);
Same here, we leave the iterator with the bucket lock held.
> + }
> + spin_unlock_bh(iter->lock);
> +
> + iter->skip = 0;
> + }
> +
> + iter->p = NULL;
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(rhashtable_walk_next);
^ permalink raw reply
* Re: [net-next PATCH v3 00/12] Flow API
From: Simon Horman @ 2015-01-26 8:26 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: John Fastabend, Thomas Graf, Jiri Pirko, Pablo Neira Ayuso,
sfeldma, netdev, davem, gerlitz.or, andy, ast
In-Reply-To: <54C397C0.3010508@mojatatu.com>
On Sat, Jan 24, 2015 at 08:01:52AM -0500, Jamal Hadi Salim wrote:
[snip]
> Lets discuss at the meeting. I am just skimming these emails (the
> conference is chewing a lot of my time so i will mostly be absent).
> Sorry if i am not responding to some things.
Is "the meeting" the Hardware Offloading BoF at Netdev 1.0?
For the benefit of others: https://www.netdev01.org/sessions/10
^ permalink raw reply
* [PATCH net-next 1/1] net: phy: unbind phy device from generic and specifical driver
From: Fugang Duan @ 2015-01-26 8:14 UTC (permalink / raw)
To: davem; +Cc: netdev, s.hauer, f.fainelli, bhutchings, stephen, b38611
The current .phy_detach() function only unbind generic phy driver, which causes
specifical driver suspend/resume function still work like Atheros AT803X PHYs.
For example:
ifconfig eth0 down
echo mem > /sys/power/status
After eth0 interface down, driver call phy_detach to unbind phy driver, and then
do suspend/resume operation, at803x_suspend()/at803x_resume() functions still get
called that call mdio bus read/write function. When eth0 interface down, MAC driver
may close all clocks and mdio bus cannot work. So the issue happens.
The patch can unbind generic and specifical driver.
Signed-off-by: Fugang Duan <B38611@freescale.com>
---
drivers/net/phy/phy_device.c | 15 ++-------------
1 files changed, 2 insertions(+), 13 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3fc91e8..8adbc5d 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -672,8 +672,6 @@ EXPORT_SYMBOL(phy_attach);
*/
void phy_detach(struct phy_device *phydev)
{
- int i;
-
if (phydev->bus->dev.driver)
module_put(phydev->bus->dev.driver->owner);
@@ -681,17 +679,8 @@ void phy_detach(struct phy_device *phydev)
phydev->attached_dev = NULL;
phy_suspend(phydev);
- /* If the device had no specific driver before (i.e. - it
- * was using the generic driver), we unbind the device
- * from the generic driver so that there's a chance a
- * real driver could be loaded
- */
- for (i = 0; i < ARRAY_SIZE(genphy_driver); i++) {
- if (phydev->dev.driver == &genphy_driver[i].driver) {
- device_release_driver(&phydev->dev);
- break;
- }
- }
+ if (phydev->dev.driver)
+ device_release_driver(&phydev->dev);
}
EXPORT_SYMBOL(phy_detach);
--
1.7.8
^ permalink raw reply related
* Re: ipv6: oops in datagram.c line 260
From: Steffen Klassert @ 2015-01-26 8:35 UTC (permalink / raw)
To: Hannes Frederic Sowa; +Cc: Chris Ruehl, netdev, davem
In-Reply-To: <1420560073.32369.60.camel@redhat.com>
On Tue, Jan 06, 2015 at 05:01:13PM +0100, Hannes Frederic Sowa wrote:
> On Mi, 2014-12-24 at 21:42 +0800, Chris Ruehl wrote:
> > [447604.244357] ipv6_pinfo is NULL
> > [447604.273733] ------------[ cut here ]------------
> > [447604.303628] WARNING: CPU: 7 PID: 0 at net/ipv6/datagram.c:262
> > ipv6_local_error+0x16b/0x1a0()
> > [[...]]
> > [last unloaded: ipmi_si]
> > [447605.087999] CPU: 7 PID: 0 Comm: swapper/7 Not tainted 3.14.27 #11
> > [447605.139687] Hardware name: Dell Inc. PowerEdge R420/0CN7CM, BIOS 2.3.3
> > 07/10/2014
> > [447605.242931] 0000000000000009 ffff8806172e3b48 ffffffff815ffd58 0000000000000000
> > [447605.349130] ffff8806172e3b80 ffffffff81043c23 ffff8800a16322e8 ffff880037daa1c0
> > [447605.459659] ffff88000b026800 0000000000000000 ffff880037daa4b8 ffff8806172e3b90
> > [447605.576385] Call Trace:
> > [447605.634243] <IRQ> [<ffffffff815ffd58>] dump_stack+0x45/0x56
> > [447605.692870] [<ffffffff81043c23>] warn_slowpath_common+0x73/0x90
> > [447605.751097] [<ffffffff81043cf5>] warn_slowpath_null+0x15/0x20
> > [447605.808000] [<ffffffff815da6db>] ipv6_local_error+0x16b/0x1a0
> > [447605.863821] [<ffffffff815e29d0>] xfrm6_local_error+0x60/0x90
> > [447605.918493] [<ffffffff8150b485>] ? skb_dequeue+0x15/0x70
> > [447605.971871] [<ffffffff815a6cc1>] xfrm_local_error+0x51/0x70
> > [447606.024218] [<ffffffff8159ca15>] xfrm4_extract_output+0x75/0xb0
> > [447606.075630] [<ffffffff815a6c5a>] xfrm_inner_extract_output+0x6a/0x80
> > [447606.126055] [<ffffffff815e27a2>] xfrm6_prepare_output+0x12/0x60
> > [447606.175310] [<ffffffff815a6ed0>] xfrm_output_resume+0x1f0/0x370
> > [447606.223406] [<ffffffff8151a486>] ? skb_checksum_help+0x76/0x190
> > [447606.270572] [<ffffffff815a709b>] xfrm_output+0x3b/0xf0
> > [447606.316454] [<ffffffff815e2ae0>] ? xfrm6_extract_output+0xe0/0xe0
> > [447606.361803] [<ffffffff815e2af7>] xfrm6_output_finish+0x17/0x20
> > [447606.406053] [<ffffffff8159cad6>] xfrm4_output+0x46/0x80
> > [447606.448694] [<ffffffff81550a80>] ip_local_out+0x20/0x30
> > [447606.489952] [<ffffffff81550dd5>] ip_queue_xmit+0x135/0x3c0
> > [447606.530017] [<ffffffff815672e1>] tcp_transmit_skb+0x461/0x8c0
> > [447606.569362] [<ffffffff8156786e>] tcp_write_xmit+0x12e/0xb20
> > [447606.607876] [<ffffffff815669ff>] ? tcp_current_mss+0x4f/0x70
> > [447606.645723] [<ffffffff8156b320>] ? tcp_write_timer_handler+0x1b0/0x1b0
> > [447606.682837] [<ffffffff81569487>] tcp_send_loss_probe+0x37/0x1f0
> > [447606.719000] [<ffffffff8156b320>] ? tcp_write_timer_handler+0x1b0/0x1b0
> > [447606.754537] [<ffffffff8156b1bb>] tcp_write_timer_handler+0x4b/0x1b0
> > [447606.789266] [<ffffffff8156b320>] ? tcp_write_timer_handler+0x1b0/0x1b0
> > [447606.823242] [<ffffffff8156b378>] tcp_write_timer+0x58/0x60
> > [447606.856047] [<ffffffff8104e848>] call_timer_fn.isra.32+0x18/0x80
> > [447606.888029] [<ffffffff8104ea1a>] run_timer_softirq+0x16a/0x200
> > [447606.920224] [<ffffffff81047efc>] __do_softirq+0xec/0x250
> > [447606.951850] [<ffffffff810482f5>] irq_exit+0xf5/0x100
> > [447606.982665] [<ffffffff8102bc6f>] smp_apic_timer_interrupt+0x3f/0x50
> > [447607.014382] [<ffffffff8160d98a>] apic_timer_interrupt+0x6a/0x70
> > [447607.046175] <EOI> [<ffffffff8104f336>] ? get_next_timer_interrupt+0x1d6/0x250
> > [447607.111311] [<ffffffff814d45a7>] ? cpuidle_enter_state+0x47/0xc0
> > [447607.145850] [<ffffffff814d45a3>] ? cpuidle_enter_state+0x43/0xc0
> > [447607.179625] [<ffffffff814d46b6>] cpuidle_idle_call+0x96/0x130
> > [447607.213531] [<ffffffff8100b909>] arch_cpu_idle+0x9/0x20
> > [447607.247052] [<ffffffff810925ba>] cpu_startup_entry+0xda/0x1d0
> > [447607.280775] [<ffffffff81029d22>] start_secondary+0x212/0x2c0
> > [447607.314555] ---[ end trace 6ff3826b6e4fdf67 ]---
> >
>
> Thanks for the report!
>
> xfrm6_output_finish unconditionally resets skb->protocol so we try to
> dispatch to the IPv6 handler, even though tcp just sends an IPv4 packet.
>
Looks like we can postpone the setting of skb->protocol to the
xfrm{4,6}_prepare_output() functions where we finally switch to
outer mode.
This has two implications:
- We reset skb->protocol only for tunnel modes, should be ok.
- This affects the xfrm_output_gso() codepath on interfamily
tunnels. skb_mac_gso_segment() dispatches to the gso_segment()
callback functions via skb->protocol. So we dispatch to
the gso_segment() function of the outer mode what looks
wrong to me. If we postpone the setting of skb->protocol
to the xfrm{4,6}_prepare_output() we dispatch to inner mode
here.
Unfortunately I was not able to reproduce the problem on our test
setup. Chris could you try if the the patch below fixes your
problem?
Subject: [PATCH RFC] xfrm: Fix local error reporting crash with interfamily
tunnels
We set the outer mode protocol too early. As a result, the
local error handler might dispatch to the wrong address family
and report the error to a wrong socket type. We fix this by
seting the outer protocol to the skb after we accessed the
inner mode for the last time, right before we do the atcual
encapsulation where we switch finally to the outer mode.
Reported-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
net/ipv4/xfrm4_output.c | 2 +-
net/ipv6/xfrm6_output.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/xfrm4_output.c b/net/ipv4/xfrm4_output.c
index d5f6bd9..dab7381 100644
--- a/net/ipv4/xfrm4_output.c
+++ b/net/ipv4/xfrm4_output.c
@@ -63,6 +63,7 @@ int xfrm4_prepare_output(struct xfrm_state *x, struct sk_buff *skb)
return err;
IPCB(skb)->flags |= IPSKB_XFRM_TUNNEL_SIZE;
+ skb->protocol = htons(ETH_P_IP);
return x->outer_mode->output2(x, skb);
}
@@ -71,7 +72,6 @@ EXPORT_SYMBOL(xfrm4_prepare_output);
int xfrm4_output_finish(struct sk_buff *skb)
{
memset(IPCB(skb), 0, sizeof(*IPCB(skb)));
- skb->protocol = htons(ETH_P_IP);
#ifdef CONFIG_NETFILTER
IPCB(skb)->flags |= IPSKB_XFRM_TRANSFORMED;
diff --git a/net/ipv6/xfrm6_output.c b/net/ipv6/xfrm6_output.c
index ca3f29b..010f8bd 100644
--- a/net/ipv6/xfrm6_output.c
+++ b/net/ipv6/xfrm6_output.c
@@ -114,6 +114,7 @@ int xfrm6_prepare_output(struct xfrm_state *x, struct sk_buff *skb)
return err;
skb->ignore_df = 1;
+ skb->protocol = htons(ETH_P_IPV6);
return x->outer_mode->output2(x, skb);
}
@@ -122,7 +123,6 @@ EXPORT_SYMBOL(xfrm6_prepare_output);
int xfrm6_output_finish(struct sk_buff *skb)
{
memset(IP6CB(skb), 0, sizeof(*IP6CB(skb)));
- skb->protocol = htons(ETH_P_IPV6);
#ifdef CONFIG_NETFILTER
IP6CB(skb)->flags |= IP6SKB_XFRM_TRANSFORMED;
--
1.9.1
^ permalink raw reply related
* Re: [PATCH net] ipv6: tcp: fix race in IPV6_2292PKTOPTIONS
From: David Miller @ 2015-01-26 8:44 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <1421840742.4832.24.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 21 Jan 2015 03:45:42 -0800
> From: Eric Dumazet <edumazet@google.com>
>
> IPv6 TCP sockets store in np->pktoptions skbs, and use skb_set_owner_r()
> to charge the skb to socket.
>
> It means that destructor must be called while socket is locked.
>
> Therefore, we cannot use skb_get() or atomic_inc(&skb->users)
> to protect ourselves : kfree_skb() might race with other users
> manipulating sk->sk_forward_alloc
>
> Fix this race by holding socket lock for the duration of
> ip6_datagram_recv_ctl()
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
> Since this bug is very old, feel free to apply on net-next ;)
Applied to net-next, thanks Eric.
^ permalink raw reply
* Re: IPsec workshop at netdev01?
From: Steffen Klassert @ 2015-01-26 9:11 UTC (permalink / raw)
To: Fan Du; +Cc: netdev, Jamal Hadi Salim, Herbert Xu, David Miller, Du, Fan
In-Reply-To: <54AF677E.9080108@gmail.com>
On Fri, Jan 09, 2015 at 01:30:38PM +0800, Fan Du wrote:
> 于 2015年01月06日 18:19, Steffen Klassert 写道:
> >
> >- We still lack a 32/64 bit compatibiltiy layer for IPsec, this issue
> > comes up from time to time. Some solutions were proposed in the past
> > but all had problems. The current behaviour is broken if someone tries
> > to configure IPsec with 32 bit tools on a 64 bit machine. Can we get
> > this right somehow or is it better to just return an error in this case?
>
> Before a clean solution show up, I think it's better to warn user in some way
> like http://patchwork.ozlabs.org/patch/323842/ did. Otherwise, many people
> who stuck there will always spend time and try to fix this issue in whatever way.
Yes, this is the first thing we should do. I'm willing to accept a patch :)
>
> >- Changing the system time can lead to unexpected SA lifetime changes. The
> > discussion on the list did not lead to a conclusion on how to fix this.
> > What is the best way to get this fixed?
>
> I rise this issue long ago before, the culprit is SA lifetime is marked by wall clock.
> In a reasonable way it should be marked as monotonic boot time(counting suspend time
> as well). Then every thing will be work correctly. I have such a patch works correctly.
> EXCEPT: SA migration, where SA lifetime comes from outside.
> I didn't look at SA migration part though, so any comments? Steffen
I have not looked into this for longer. So I can not comment on it
now, but I could be prepared for discussion on netdev01.
^ permalink raw reply
* Re: [PATCH net-next 1/7] r8152: adjust rx_bottom
From: Scott Feldman @ 2015-01-26 9:14 UTC (permalink / raw)
To: Hayes Wang
Cc: David Miller, netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
nic_swsd, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <0835B3720019904CB8F7AA43166CEEB2EE6E76-JIZ+AM9kKNzuvTFwvkocLypo8c9IxeqyAjHCUHv49ws@public.gmane.org>
On Mon, Jan 19, 2015 at 6:48 PM, Hayes Wang <hayeswang-Rasf1IRRPZFBDgjK7y7TUQ@public.gmane.org> wrote:
> David Miller [mailto:davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org]
>> Sent: Tuesday, January 20, 2015 5:14 AM
> [...]
>> >> - r8152_submit_rx(tp, agg, GFP_ATOMIC);
>> >> + if (!ret) {
>> >> + ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
>> >> + } else {
>> >> + urb->actual_length = 0;
>> >> + list_add_tail(&agg->list, next);
>> >
>> > Do you need a spin_lock_irqsave(&tp->rx_lock, flags) around this?
>>
>> Indeed, and rtl_start_rx() seems to also access agg->list without
>> proper locking.
>
> It is unnecessary because I deal with them in a local list_head. My steps are
> 1. Move the whole list from tp->rx_done to local rx_queue. (with spin lock)
> 2. dequeue/enqueue the lists in rx_queue.
> 3. Move the lists in rx_queue to tp->rx_done if it is necessary. (spin lock)
> For step 2, it wouldn't have race, because the list_head is local and no other
> function would change it. Therefore, I don't think it needs the spin lock.
Sorry guys, I think I made a mistake in my review and caused some
confusion/grief.
My mistake was getting the params to list_add_tail() backwards. It's
list_add_tail(entry, head). I saw list_add_tail(&agg->list, next) and
was fooled into thinking agg->list was the list getting appended with
the entry 'next'. It's the opposite. Duh. So locking isn't needed
because the list is indeed local.
-scott
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net-next 0/6] netns: advertise netns via netlink
From: Nicolas Dichtel @ 2015-01-26 9:20 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20150123.175142.618148236781483266.davem@davemloft.net>
Le 24/01/2015 02:51, David Miller a écrit :
> From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Date: Tue, 20 Jan 2015 15:15:41 +0100
>
>>
>> The first patch of the serie fix a bug of the previous serie (present in
>> net-next only).
>> The rest of the serie adds an attribute to advertise the peer netns for
>> rtnetlink messages where this information is needed by userland to be able to
>> interpret fully the received message.
>
> "series" has a final 's'. Well, as least you consistently misspell it
> 3 times :-)
Thanks for noticing it.
For the story, it comes from the french word 'serie' which don't end with a 's'
;-)
^ permalink raw reply
* [PATCH net-next] vlan: take link netns into account in vlan_newlink()
From: Nicolas Dichtel @ 2015-01-26 9:44 UTC (permalink / raw)
To: netdev; +Cc: davem, Nicolas Dichtel
When IFLA_LINK_NETNSID is set, the ifindex from IFLA_LINK comes from another
netns.
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
net/8021q/vlan_netlink.c | 13 ++++++++++++-
net/core/net_namespace.c | 1 +
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/net/8021q/vlan_netlink.c b/net/8021q/vlan_netlink.c
index c92b52f37d38..e7875c7f3cb4 100644
--- a/net/8021q/vlan_netlink.c
+++ b/net/8021q/vlan_netlink.c
@@ -118,6 +118,7 @@ static int vlan_newlink(struct net *src_net, struct net_device *dev,
{
struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
struct net_device *real_dev;
+ struct net *link_net;
__be16 proto;
int err;
@@ -126,7 +127,17 @@ static int vlan_newlink(struct net *src_net, struct net_device *dev,
if (!tb[IFLA_LINK])
return -EINVAL;
- real_dev = __dev_get_by_index(src_net, nla_get_u32(tb[IFLA_LINK]));
+ if (tb[IFLA_LINK_NETNSID]) {
+ int id = nla_get_s32(tb[IFLA_LINK_NETNSID]);
+
+ link_net = get_net_ns_by_id(src_net, id);
+ if (!link_net)
+ return -EINVAL;
+ } else {
+ link_net = get_net(src_net);
+ }
+ real_dev = __dev_get_by_index(link_net, nla_get_u32(tb[IFLA_LINK]));
+ put_net(link_net);
if (!real_dev)
return -ENODEV;
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index b7bde551ef76..8e24f974c665 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -219,6 +219,7 @@ struct net *get_net_ns_by_id(struct net *net, int id)
return peer;
}
+EXPORT_SYMBOL(get_net_ns_by_id);
/*
* setup_net runs the initializers for the network namespace object.
--
2.2.2
^ permalink raw reply related
* Re: [PATCH] net: Linn Ethernet Packet Sniffer driver
From: Stathis Voukelatos @ 2015-01-26 9:49 UTC (permalink / raw)
To: Daniel Borkmann, Stathis Voukelatos
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org, abrestic@chromium.org
In-Reply-To: <54C22E68.1080601@redhat.com>
On 23/01/15 11:20, Daniel Borkmann wrote:
> On 01/23/2015 11:07 AM, Stathis Voukelatos wrote:
>> This patch adds support the Ethernet Packet Sniffer H/W module
>> developed by Linn Products Ltd and found in the IMG Pistachio SoC.
>> The module allows Ethernet packets to be parsed, matched against
>> a user-defined pattern and timestamped. It sits between a 100M
>> Ethernet MAC and PHY and is completely passive with respect to
>> Ethernet frames.
>>
>> Matched packet bytes and timestamp values are returned through a
>> FIFO. Timestamps are provided to the module through an externally
>> generated Gray-encoded counter.
>>
>> The command pattern for packet matching is stored in module RAM
>> and consists of a sequence of 16-bit entries. Each entry includes
>> an 8-bit command code and and 8-bit data value. Valid command
>> codes are:
>> 0 - Don't care
>> 1 - Match: packet data must match command string byte
>> 2 - Copy: packet data will be copied to FIFO
>> 3 - Match/Stamp: if packet data matches string byte, a timestamp
>> is copied into the FIFO
>> 4 - Copy/Done: packet data will be copied into the FIFO.
>> This command terminates the command string.
>>
>> The driver consists of two modules:
>> - Core: it provides an API to user space using the Generic Netlink
>> framework. Specific backend implementations, like the
>> Ethernet Packet Sniffer, register one or more channels
>> with the Core. For each channel a Genl family is created.
>> User space can access a channel by sending Genl messages
>> to the Genl family associated with the channel. Packet
>> matching events are multicast.
>>
>> - Ethernet Packet Sniffer backend: provides the driver for the
>> Linn Ethernet Packet Sniffer H/W modules.
>>
>> The split between a core and backend modules allows software-only
>> implementations to be added for platforms where no H/W support
>> is available.
>>
>> Based on 3.19-rc5
>>
>> Signed-off-by: Stathis Voukelatos <stathis.voukelatos@linn.co.uk>
> Please have a look at packet sockets, they offer already all the
> functionality (if not more) your driver interface to the user space
> resembles, are transparent to the underlying hardware, and easily
> can cope with 100Mbit.
>
> If I understand this correctly, you are effectively introducing a
> parallel API *next* to packet sockets to user space that we have to
> maintain forever ...
>
> Thanks !
>
Hello Daniel. Thank you for your feedback.
Packet sockets could also be used for the driver interface to
user space, however I think that both approaches would require the same
amount of maintenance. We need to maintain a protocol consisting of
a set of messages or commands that user space can use to communicate
with the driver in order to configure the H/W and retrieve results.
We could use packet sockets to send those messages too, but I thought
netlink already provides a message exchange framework that we could
make use of.
^ permalink raw reply
* Re: [PATCH v6 0/7] can: kvaser_usb: Leaf bugfixes and USBCan-II support
From: Marc Kleine-Budde @ 2015-01-26 9:55 UTC (permalink / raw)
To: Ahmed S. Darwish, Olivier Sobrie, Oliver Hartkopp,
Wolfgang Grandegger, Andri Yngvason
Cc: Linux-CAN, netdev, LKML
In-Reply-To: <20150126051755.GA3014@Darwish.PC>
[-- Attachment #1: Type: text/plain, Size: 1060 bytes --]
On 01/26/2015 06:17 AM, Ahmed S. Darwish wrote:
> Hi!
>
> This is an updated patch series for the Kvaser CAN/USB devices:
>
> 1- Extra patches are now added to the series. Most importantly
> patch #1 which fixes a critical `sleep in atomic context' bug
> in the current upstream driver. Patch #2 fixes a corruption in
> the kernel logs, also affecting current upstream driver. Patch
> #4 was originally USBCan-II only, but since it's a generic fix,
> it's now retrofitted to the Leaf-only upstream driver first.
>
> 2- The series has been re-organized so that patches #1 -> #4
> inclusive can go to linux-can/origin, while the rest can move
> to -next.
Applied 1-4 to can/master, I'm adding stable on Cc before sending a
pull-request to David.
Thanks,
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: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v6 0/7] can: kvaser_usb: Leaf bugfixes and USBCan-II support
From: Marc Kleine-Budde @ 2015-01-26 10:07 UTC (permalink / raw)
To: Ahmed S. Darwish, Olivier Sobrie, Oliver Hartkopp,
Wolfgang Grandegger, Andri Yngvason
Cc: Linux-CAN, netdev, LKML
In-Reply-To: <20150126051755.GA3014@Darwish.PC>
[-- Attachment #1: Type: text/plain, Size: 1645 bytes --]
On 01/26/2015 06:17 AM, Ahmed S. Darwish wrote:
> Hi!
>
> This is an updated patch series for the Kvaser CAN/USB devices:
>
> 1- Extra patches are now added to the series. Most importantly
> patch #1 which fixes a critical `sleep in atomic context' bug
> in the current upstream driver. Patch #2 fixes a corruption in
> the kernel logs, also affecting current upstream driver. Patch
> #4 was originally USBCan-II only, but since it's a generic fix,
> it's now retrofitted to the Leaf-only upstream driver first.
>
> 2- The series has been re-organized so that patches #1 -> #4
> inclusive can go to linux-can/origin, while the rest can move
> to -next.
>
> 3- There are some important updates regarding the USBCan-II
> error counters, and how really erratic their heaviour is. All
> the new details are covered at the bottom of this URL:
>
> http://article.gmane.org/gmane.linux.can/7481
>
> 4- Attached below is the new candump traces. Now
> `back-to-error-active' states appear _if_ the hardware was
> kind enough and decreased the error counters appropriately.
> The earlier code did not recognize the error counters going
> down, thus the `back-to-error-active' transitions did not
> appear.
For patches 5-7 I'm waiting for an Acked by Andri.
...and we have to wait until Patches 1-4 will show up on net-next/master.
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: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] net: Linn Ethernet Packet Sniffer driver
From: Daniel Borkmann @ 2015-01-26 10:10 UTC (permalink / raw)
To: Stathis Voukelatos
Cc: Stathis Voukelatos, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
abrestic@chromium.org
In-Reply-To: <54C60DB8.1060900@linn.co.uk>
Hi Stathis,
On 01/26/2015 10:49 AM, Stathis Voukelatos wrote:
> On 23/01/15 11:20, Daniel Borkmann wrote:
>> On 01/23/2015 11:07 AM, Stathis Voukelatos wrote:
>>> This patch adds support the Ethernet Packet Sniffer H/W module
>>> developed by Linn Products Ltd and found in the IMG Pistachio SoC.
>>> The module allows Ethernet packets to be parsed, matched against
>>> a user-defined pattern and timestamped. It sits between a 100M
>>> Ethernet MAC and PHY and is completely passive with respect to
>>> Ethernet frames.
>>>
>>> Matched packet bytes and timestamp values are returned through a
>>> FIFO. Timestamps are provided to the module through an externally
>>> generated Gray-encoded counter.
>>>
>>> The command pattern for packet matching is stored in module RAM
>>> and consists of a sequence of 16-bit entries. Each entry includes
>>> an 8-bit command code and and 8-bit data value. Valid command
>>> codes are:
>>> 0 - Don't care
>>> 1 - Match: packet data must match command string byte
>>> 2 - Copy: packet data will be copied to FIFO
>>> 3 - Match/Stamp: if packet data matches string byte, a timestamp
>>> is copied into the FIFO
>>> 4 - Copy/Done: packet data will be copied into the FIFO.
>>> This command terminates the command string.
>>>
>>> The driver consists of two modules:
>>> - Core: it provides an API to user space using the Generic Netlink
>>> framework. Specific backend implementations, like the
>>> Ethernet Packet Sniffer, register one or more channels
>>> with the Core. For each channel a Genl family is created.
>>> User space can access a channel by sending Genl messages
>>> to the Genl family associated with the channel. Packet
>>> matching events are multicast.
>>>
>>> - Ethernet Packet Sniffer backend: provides the driver for the
>>> Linn Ethernet Packet Sniffer H/W modules.
>>>
>>> The split between a core and backend modules allows software-only
>>> implementations to be added for platforms where no H/W support
>>> is available.
>>>
>>> Based on 3.19-rc5
>>>
>>> Signed-off-by: Stathis Voukelatos <stathis.voukelatos@linn.co.uk>
>> Please have a look at packet sockets, they offer already all the
>> functionality (if not more) your driver interface to the user space
>> resembles, are transparent to the underlying hardware, and easily
>> can cope with 100Mbit.
>>
>> If I understand this correctly, you are effectively introducing a
>> parallel API *next* to packet sockets to user space that we have to
>> maintain forever ...
>>
>> Thanks !
>
> Hello Daniel. Thank you for your feedback.
> Packet sockets could also be used for the driver interface to
> user space, however I think that both approaches would require the same
> amount of maintenance. We need to maintain a protocol consisting of
> a set of messages or commands that user space can use to communicate
> with the driver in order to configure the H/W and retrieve results.
> We could use packet sockets to send those messages too, but I thought
> netlink already provides a message exchange framework that we could
> make use of.
When using packet sockets and your driver as a backend feeding them,
users can see that there's an extra capturing/monitoring netdev present,
all libpcap-based tools such as tcpdump et al would work out of the box
w/o adapting any code, and as an admin you can also see what users/tools
are making of use of the device through packet sockets. I couldn't parse
the exact motivation from the commit message of why avoiding all this is
better?
Thanks,
Daniel
^ permalink raw reply
* RE: [PATCH 1/2] rhashtable: Introduce rhashtable_walk_*
From: David Laight @ 2015-01-26 10:09 UTC (permalink / raw)
To: 'Herbert Xu', Thomas Graf, Ying Xue, davem@davemloft.net,
kaber@trash.net, paulmck@linux.vnet.ibm.com,
netdev@vger.kernel.org, netfilter-devel@vger.kernel.org
In-Reply-To: <E1YFWUl-0004gE-SH@gondolin.me.apana.org.au>
From: Herbert Xu
> Some existing rhashtable users get too intimate with it by walking
> the buckets directly. This prevents us from easily changing the
> internals of rhashtable.
>
> This patch adds the helpers rhashtable_walk_init/next/end which
> will replace these custom walkers.
>
> They are meant to be usable for both procfs seq_file walks as well
> as walking by a netlink dump. The iterator structure should fit
> inside a netlink dump cb structure, with at least one element to
> spare.
...
> +/**
> + * rhashtable_walk_start - Start a hash table walk
> + * @iter: Hash table iterator
> + *
> + * Start a hash table walk.
> + *
> + * Returns zero if successful. Returns -EINTR if we couldn't
> + * obtain the resize lock.
> + */
> +int rhashtable_walk_start(struct rhashtable_iter *iter)
> +{
> + struct rhashtable *ht = iter->ht;
> + int err;
> +
> + err = mutex_lock_interruptible(&ht->mutex);
> + rcu_read_lock();
> +
> + if (!err)
> + mutex_unlock(&ht->mutex);
> +
> + return err;
> +}
That doesn't look right to me.
Surely you shouldn't be calling rcu_read_lock() when the mutex
request is interrupted.
So maybe:
err = mutex_lock_interruptible(&ht->mutex);
if (err)
return err;
rcu_read_lock();
return 0;
}
David
^ permalink raw reply
* Re: [PATCH] net: Linn Ethernet Packet Sniffer driver
From: Stathis Voukelatos @ 2015-01-26 10:16 UTC (permalink / raw)
To: Mark Rutland, Stathis Voukelatos
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org, abrestic@chromium.org
In-Reply-To: <20150123105114.GB23493@leverpostej>
On 23/01/15 10:51, Mark Rutland wrote:
>
> diff --git a/Documentation/devicetree/bindings/net/linn-ether-packet-sniffer.txt b/Documentation/devicetree/bindings/net/linn-ether-packet-sniffer.txt
> new file mode 100644
> index 0000000..6b6e105
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/linn-ether-packet-sniffer.txt
> @@ -0,0 +1,27 @@
> +* Linn Products Ethernet Packet Sniffer
> +
> +Required properties:
> +- compatible : must be "linn,eth-sniffer"
> +- reg : physical addresses and sizes of registers. Must contain 3 entries:
> + first entry: registers memory space
> + second entry: TX command memory
> + third entry: RX command memory
> Just to check: are those memories are part of the packet sniffer device,
> or are carveouts from other memory?
Yes, the 3 memory areas are part of the packet sniffer module.
>> +- fifo-block-words : number of words in one data FIFO entry
>> +
>> +Example:
>> +
>> +sniffer@1814a000 {
>> + compatible = "linn,eth-sniffer";
>> + reg = <0x1814a000 0x100>, <0x1814a400 0x400>, <0x1814a800 0x400>;
>> + reg-names = "regs", "tx-ram", "rx-ram";
>> + interrupts = <GIC_SHARED 58 IRQ_TYPE_LEVEL_HIGH>;
>> + interrupt-names = "eth-sniffer-irq";
>> + clocks = <&system_clk>;
>> + clock-names = "sys";
>> + fifo-block-words = <4>;
>> + };
> Surely the relationship between the sniffer, MAC, and PHY should be
> described, so we know which interface the sniffer is related to?
>
The packet sniffer sits between the MAC and the PHY and monitors TX or RX
packets, or both. Will add a description in the binding doc.
Your other suggestions too, will be incorporated in the next version of
the patch set.
Thank you,
Stathis
^ permalink raw reply
* Re: [PATCH v5 2/5] can: kvaser_usb: Consolidate and unify state change handling
From: Andri Yngvason @ 2015-01-26 10:21 UTC (permalink / raw)
To: Ahmed S. Darwish
Cc: Wolfgang Grandegger, Olivier Sobrie, Oliver Hartkopp,
Marc Kleine-Budde, Linux-CAN, netdev, LKML
In-Reply-To: <20150125024300.GA6028@Darwish.PC>
Quoting Ahmed S. Darwish (2015-01-25 02:43:00)
> On Fri, Jan 23, 2015 at 10:32:13AM +0000, Andri Yngvason wrote:
> > Quoting Ahmed S. Darwish (2015-01-23 06:07:34)
> > > On Wed, Jan 21, 2015 at 05:13:45PM +0100, Wolfgang Grandegger wrote:
> > > > On Wed, 21 Jan 2015 10:36:47 -0500, "Ahmed S. Darwish"
> > > > <darwish.07@gmail.com> wrote:
> > > > > On Wed, Jan 21, 2015 at 03:00:15PM +0000, Andri Yngvason wrote:
> > > > >> Quoting Ahmed S. Darwish (2015-01-21 14:43:23)
> > > > >> > Hi!
> > > > >
> > > > > ...
> > > > >
> > > > >> > <-- Unplug the cable -->
> > > > >> >
> > > > >> > (000.009106) can0 20000080 [8] 00 00 00 00 00 00 08 00
> > > > >> > ERRORFRAME
> > > > >> > bus-error
> > > > >> > error-counter-tx-rx{{8}{0}}
> > > > >> > (000.001872) can0 20000080 [8] 00 00 00 00 00 00 10 00
> > > >
> > > > For a bus-errors I would also expcect some more information in the
> > > > data[2..3] fields. But these are always zero.
> > > >
> > >
> > > M16C error factors made it possible to report things like
> > > CAN_ERR_PROT_FORM/STUFF/BIT0/BIT1/TX in data[2], and
> > > CAN_ERR_PROT_LOC_ACK/CRC_DEL in data[3].
> > >
> > > Unfortunately such error factors are only reported in Leaf, but
> > > not in USBCan-II due to the wire format change in the error event:
> > >
> > > struct leaf_msg_error_event {
> > > u8 tid;
> > > u8 flags;
> > > __le16 time[3];
> > > u8 channel;
> > > u8 padding;
> > > u8 tx_errors_count;
> > > u8 rx_errors_count;
> > > u8 status;
> > > u8 error_factor;
> > > } __packed;
> > >
> > > struct usbcan_msg_error_event {
> > > u8 tid;
> > > u8 padding;
> > > u8 tx_errors_count_ch0;
> > > u8 rx_errors_count_ch0;
> > > u8 tx_errors_count_ch1;
> > > u8 rx_errors_count_ch1;
> > > u8 status_ch0;
> > > u8 status_ch1;
> > > __le16 time;
> > > } __packed;
> > >
> > > I speculate that the wire format was changed due to controller
> > > bugs in the USBCan-II, which was slightly mentioned in their
> > > data sheets here:
> > >
> > > http://www.kvaser.com/canlib-webhelp/page_hardware_specific_can_controllers.html
> > >
> > > So it seems there's really no way for filling such bus error
> > > info given the very limited amount of data exported :-(
> > >
> > We experienced similar problems with FlexCAN.
>
> Hmm, I'll have a look there then...
>
> Although my initial instincts imply that the FlexCAN driver has
> access to the raw CAN registers, something I'm unable to do here.
> But maybe there's some black magic I'm missing :-)
>
Yes, it's memory mapped.
>
> [...]
>
> > >
> > > I've dumped _every_ message I receive from the firmware while
> > > disconnecting the CAN bus, waiting a while, and connecting it again.
> > > I really received _nothing_ from the firmware when the CAN bus was
> > > reconnected and the data packets were flowing again. Not even a
> > > single CHIP_STATE_EVENT, even after waiting for a long time.
> > >
> > > So it's basically:
> > > ...
> > > ERR EVENT, txerr=128, rxerr=0
> > > ERR EVENT, txerr=128, rxerr=0
> > > ERR EVENT, txerr=128, rxerr=0
> > > ...
> > >
> > > then complete silence, except the data frames. I've even tried with
> > > different versions of the firmware, but the same behaviour persisted.
> > >
> > > > > So, What can the driver do given the above?
> > > >
> > > > Little if the notification does not come.
> > > >
> > >
> > > We can poll the state by sending CMD_GET_CHIP_STATE to the firmware,
> > > and it will hopefully reply with a CHIP_STATE_EVENT response
> > > containing the new txerr and rxerr values that we can use for
> > > reverse state transitions.
> > >
> > > But do we _really_ want to go through the path? I feel that it will
> > > open some cans of worms w.r.t. concurrent access to both the netdev
> > > and USB stacks from a single driver.
> > >
> > Honestly, I don't know.
> > >
> > > A possible solution can be setting up a kernel thread that queries
> > > for a CHIP_STATE_EVENT every second?
> > >
> > Have you considered polling in kvaser_usb_tx_acknowledge? You could do something
> > like:
> > if(unlikely(dev->can.state != CAN_STATE_ERROR_ACTIVE))
> > {
> > request_state();
> > }
> >
>
> OK, I have four important updates on this issue:
>
> a) My initial testing was done on high-speed channel, at a bitrate
> of 50K. After setting the bus to a more reasonable bitrate 500K
> or 1M, I was _consistently_ able to receive CHIP_STATE_EVENTs
> when plugging the CAN connector again after an unplug.
>
> b) The error counters on this device do not get reset on plugging
> after an unplug. I've setup a kernel thread [2] that queries
> the chip state event every second, and the error counters stays
> the same all the time. [1]
>
> c) There's a single case when the erro counters do indeed get
> reversed, and it happens only when introducing some noise in
> the bus after the re-plug. In that case, the new error events
> get raised with new error counters starting from 0/1 again.
>
> d) I've discovered a bug that forbids the CAN state from
> returning to ERROR_ACTIVE in case of the error counters
> numbers getting decreased. But independent from that bug, the
> verbose debugging messages clearly imply that we only get the
> error counters decreased in the case mentioned at `c)' above.
>
> So from [1] and [2], it's now clear that the device do not reset
> these counters back in the re-plug case. I'll give a check to
> flexcan as advised, but unfortunately I don't really think there's
> much I can do about this.
>
> [1]
>
> [ 877.207082] CAN_ERROR_: channel=0, txerr=88, rxerr=0
> [ 877.207090] CAN_ERROR_: channel=0, txerr=136, rxerr=0
> [ 877.207094] CAN_ERROR_: channel=0, txerr=144, rxerr=0
> [ 877.207098] CAN_ERROR_: channel=0, txerr=152, rxerr=0
> [ 877.207100] CAN_ERROR_: channel=0, txerr=160, rxerr=0
> [ 877.207102] CAN_ERROR_: channel=0, txerr=168, rxerr=0
> [ 877.208075] CAN_ERROR_: channel=0, txerr=200, rxerr=0
>
> (( The above error event, staying the same at txerr=200 keeps
> flooding the bus until the CAN cable is re-plugged ))
>
> [ 878.225116] CHIP_STATE: channel=0, txerr=200, rxerr=0
> [ 878.225143] CHIP_STATE: channel=1, txerr=0, rxerr=0
> [ 879.265167] CHIP_STATE: channel=0, txerr=200, rxerr=0
> [ 879.267152] CHIP_STATE: channel=1, txerr=0, rxerr=0
> [ 879.265167] CHIP_STATE: channel=0, txerr=200, rxerr=0
> [ 879.267152] CHIP_STATE: channel=1, txerr=0, rxerr=0
>
> (( The same counters get repeated every second ))
>
> [2] State was polled using:
>
> static int kvaser_usb_poll_chip_state(void *vpriv) {
> struct kvaser_usb_net_priv *priv = vpriv;
>
> while (!kthread_should_stop()) {
> kvaser_usb_simple_msg_async(priv, CMD_GET_CHIP_STATE);
> ssleep(1);
> }
>
> return 0;
> }
>
> > I don't think that anything beyond that would be worth pursuing.
> >
>
> I agree, but given the new input, it seems that our problem
> extends to the error counters themselves not getting decreased
> on re-plug. So, even polling will not solve the issue: we'll
> get the same txerr/rxerr values again and again :-(
>
Well, that's a damn shame. Not much you can do about that.
Best regards,
Andri
^ permalink raw reply
* Re: [PATCH v6 6/7] can: kvaser_usb: Consolidate and unify state change handling
From: Andri Yngvason @ 2015-01-26 10:26 UTC (permalink / raw)
To: Ahmed S. Darwish, Olivier Sobrie, Oliver Hartkopp,
Wolfgang Grandegger, Marc Kleine-Budde
Cc: Linux-CAN, netdev, LKML
In-Reply-To: <20150126052915.GG3014@Darwish.PC>
Quoting Ahmed S. Darwish (2015-01-26 05:29:15)
> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
>
> Replace most of the can interface's state and error counters
> handling with the new can-dev can_change_state() mechanism.
>
> Suggested-by: Andri Yngvason <andri.yngvason@marel.com>
> Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> ---
> drivers/net/can/usb/kvaser_usb.c | 113 ++++++++++++++++---------------------
> 1 files changed, 49 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
> index f57ce55..ddc2954 100644
> --- a/drivers/net/can/usb/kvaser_usb.c
> +++ b/drivers/net/can/usb/kvaser_usb.c
> @@ -620,39 +620,44 @@ static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
> }
>
> static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv *priv,
> - const struct kvaser_usb_error_summary *es)
> + const struct kvaser_usb_error_summary *es,
> + struct can_frame *cf)
> {
> struct net_device_stats *stats;
> - enum can_state new_state;
> -
> - stats = &priv->netdev->stats;
> - new_state = priv->can.state;
> + enum can_state cur_state, new_state, tx_state, rx_state;
>
> netdev_dbg(priv->netdev, "Error status: 0x%02x\n", es->status);
>
> - if (es->status & (M16C_STATE_BUS_OFF | M16C_STATE_BUS_RESET)) {
> - priv->can.can_stats.bus_off++;
> + stats = &priv->netdev->stats;
> + new_state = cur_state = priv->can.state;
> +
> + if (es->status & (M16C_STATE_BUS_OFF | M16C_STATE_BUS_RESET))
> new_state = CAN_STATE_BUS_OFF;
> - } else if (es->status & M16C_STATE_BUS_PASSIVE) {
> - if (priv->can.state != CAN_STATE_ERROR_PASSIVE)
> - priv->can.can_stats.error_passive++;
> + else if (es->status & M16C_STATE_BUS_PASSIVE)
> new_state = CAN_STATE_ERROR_PASSIVE;
> - } else if (es->status & M16C_STATE_BUS_ERROR) {
> - if ((priv->can.state < CAN_STATE_ERROR_WARNING) &&
> - ((es->txerr >= 96) || (es->rxerr >= 96))) {
> - priv->can.can_stats.error_warning++;
> + else if (es->status & M16C_STATE_BUS_ERROR) {
> + if ((es->txerr >= 256) || (es->rxerr >= 256))
> + new_state = CAN_STATE_BUS_OFF;
> + else if ((es->txerr >= 128) || (es->rxerr >= 128))
> + new_state = CAN_STATE_ERROR_PASSIVE;
> + else if ((es->txerr >= 96) || (es->rxerr >= 96))
> new_state = CAN_STATE_ERROR_WARNING;
> - } else if ((priv->can.state > CAN_STATE_ERROR_ACTIVE) &&
> - ((es->txerr < 96) && (es->rxerr < 96))) {
> + else if (cur_state > CAN_STATE_ERROR_ACTIVE)
> new_state = CAN_STATE_ERROR_ACTIVE;
> - }
> }
>
> if (!es->status)
> new_state = CAN_STATE_ERROR_ACTIVE;
>
> + if (new_state != cur_state) {
> + tx_state = (es->txerr >= es->rxerr) ? new_state : 0;
> + rx_state = (es->txerr <= es->rxerr) ? new_state : 0;
> +
> + can_change_state(priv->netdev, cf, tx_state, rx_state);
> + }
> +
> if (priv->can.restart_ms &&
> - (priv->can.state >= CAN_STATE_BUS_OFF) &&
> + (cur_state >= CAN_STATE_BUS_OFF) &&
> (new_state < CAN_STATE_BUS_OFF)) {
> priv->can.can_stats.restarts++;
> }
> @@ -664,18 +669,17 @@ static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv *pri
>
> priv->bec.txerr = es->txerr;
> priv->bec.rxerr = es->rxerr;
> - priv->can.state = new_state;
> }
>
> static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> const struct kvaser_msg *msg)
> {
> - struct can_frame *cf;
> + struct can_frame *cf, tmp_cf = { .can_id = CAN_ERR_FLAG, .can_dlc = CAN_ERR_DLC };
> struct sk_buff *skb;
> struct net_device_stats *stats;
> struct kvaser_usb_net_priv *priv;
> struct kvaser_usb_error_summary es = { };
> - enum can_state old_state;
> + enum can_state old_state, new_state;
>
> switch (msg->id) {
> case CMD_CAN_ERROR_EVENT:
> @@ -715,59 +719,40 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> stats = &priv->netdev->stats;
>
> /* Update all of the can interface's state and error counters before
> - * trying any skb allocation that can actually fail with -ENOMEM.
> + * trying any memory allocation that can actually fail with -ENOMEM.
> + *
> + * We send a temporary stack-allocated error can frame to
> + * can_change_state() for the very same reason.
> + *
> + * TODO: Split can_change_state() responsibility between updating the
> + * can interface's state and counters, and the setting up of can error
> + * frame ID and data to userspace. Remove stack allocation afterwards.
> */
> old_state = priv->can.state;
> - kvaser_usb_rx_error_update_can_state(priv, &es);
> + kvaser_usb_rx_error_update_can_state(priv, &es, &tmp_cf);
> + new_state = priv->can.state;
>
> skb = alloc_can_err_skb(priv->netdev, &cf);
> if (!skb) {
> stats->rx_dropped++;
> return;
> }
> -
> - if (es.status & (M16C_STATE_BUS_OFF | M16C_STATE_BUS_RESET)) {
> - cf->can_id |= CAN_ERR_BUSOFF;
> -
> - if (!priv->can.restart_ms)
> - kvaser_usb_simple_msg_async(priv, CMD_STOP_CHIP);
> - netif_carrier_off(priv->netdev);
> - } else if (es.status & M16C_STATE_BUS_PASSIVE) {
> - if (old_state != CAN_STATE_ERROR_PASSIVE) {
> - cf->can_id |= CAN_ERR_CRTL;
> -
> - if (es.txerr || es.rxerr)
> - cf->data[1] = (es.txerr > es.rxerr)
> - ? CAN_ERR_CRTL_TX_PASSIVE
> - : CAN_ERR_CRTL_RX_PASSIVE;
> - else
> - cf->data[1] = CAN_ERR_CRTL_TX_PASSIVE |
> - CAN_ERR_CRTL_RX_PASSIVE;
> - }
> - } else if (es.status & M16C_STATE_BUS_ERROR) {
> - if ((old_state < CAN_STATE_ERROR_WARNING) &&
> - ((es.txerr >= 96) || (es.rxerr >= 96))) {
> - cf->can_id |= CAN_ERR_CRTL;
> - cf->data[1] = (es.txerr > es.rxerr)
> - ? CAN_ERR_CRTL_TX_WARNING
> - : CAN_ERR_CRTL_RX_WARNING;
> - } else if ((old_state > CAN_STATE_ERROR_ACTIVE) &&
> - ((es.txerr < 96) && (es.rxerr < 96))) {
> - cf->can_id |= CAN_ERR_PROT;
> - cf->data[2] = CAN_ERR_PROT_ACTIVE;
> + memcpy(cf, &tmp_cf, sizeof(*cf));
> +
> + if (new_state != old_state) {
> + if (es.status &
> + (M16C_STATE_BUS_OFF | M16C_STATE_BUS_RESET)) {
> + if (!priv->can.restart_ms)
> + kvaser_usb_simple_msg_async(priv, CMD_STOP_CHIP);
> + netif_carrier_off(priv->netdev);
> }
> - }
> -
> - if (!es.status) {
> - cf->can_id |= CAN_ERR_PROT;
> - cf->data[2] = CAN_ERR_PROT_ACTIVE;
> - }
>
> - if (priv->can.restart_ms &&
> - (old_state >= CAN_STATE_BUS_OFF) &&
> - (priv->can.state < CAN_STATE_BUS_OFF)) {
> - cf->can_id |= CAN_ERR_RESTARTED;
> - netif_carrier_on(priv->netdev);
> + if (priv->can.restart_ms &&
> + (old_state >= CAN_STATE_BUS_OFF) &&
> + (new_state < CAN_STATE_BUS_OFF)) {
> + cf->can_id |= CAN_ERR_RESTARTED;
> + netif_carrier_on(priv->netdev);
> + }
> }
>
> if (es.error_factor) {
> --
> 1.7.7.6
ACK, for real this time. ;)
--
Andri
^ permalink raw reply
* Re: [PATCH v6 5/7] can: kvaser_usb: Update interface state before exiting on OOM
From: Andri Yngvason @ 2015-01-26 10:28 UTC (permalink / raw)
To: Ahmed S. Darwish, Olivier Sobrie, Oliver Hartkopp,
Wolfgang Grandegger, Marc Kleine-Budde
Cc: Linux-CAN, netdev, LKML
In-Reply-To: <20150126052719.GF3014@Darwish.PC>
Quoting Ahmed S. Darwish (2015-01-26 05:27:19)
> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
>
> Update all of the can interface's state and error counters before
> trying any skb allocation that can actually fail with -ENOMEM.
>
> Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> ---
> drivers/net/can/usb/kvaser_usb.c | 181 ++++++++++++++++++++++----------------
> 1 files changed, 105 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
> index 7af379c..f57ce55 100644
> --- a/drivers/net/can/usb/kvaser_usb.c
> +++ b/drivers/net/can/usb/kvaser_usb.c
> @@ -273,6 +273,10 @@ struct kvaser_msg {
> } u;
> } __packed;
>
> +struct kvaser_usb_error_summary {
> + u8 channel, status, txerr, rxerr, error_factor;
> +};
> +
> struct kvaser_usb_tx_urb_context {
> struct kvaser_usb_net_priv *priv;
> u32 echo_index;
> @@ -615,6 +619,54 @@ static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
> priv->tx_contexts[i].echo_index = MAX_TX_URBS;
> }
>
> +static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv *priv,
> + const struct kvaser_usb_error_summary *es)
> +{
> + struct net_device_stats *stats;
> + enum can_state new_state;
> +
> + stats = &priv->netdev->stats;
> + new_state = priv->can.state;
> +
> + netdev_dbg(priv->netdev, "Error status: 0x%02x\n", es->status);
> +
> + if (es->status & (M16C_STATE_BUS_OFF | M16C_STATE_BUS_RESET)) {
> + priv->can.can_stats.bus_off++;
> + new_state = CAN_STATE_BUS_OFF;
> + } else if (es->status & M16C_STATE_BUS_PASSIVE) {
> + if (priv->can.state != CAN_STATE_ERROR_PASSIVE)
> + priv->can.can_stats.error_passive++;
> + new_state = CAN_STATE_ERROR_PASSIVE;
> + } else if (es->status & M16C_STATE_BUS_ERROR) {
> + if ((priv->can.state < CAN_STATE_ERROR_WARNING) &&
> + ((es->txerr >= 96) || (es->rxerr >= 96))) {
> + priv->can.can_stats.error_warning++;
> + new_state = CAN_STATE_ERROR_WARNING;
> + } else if ((priv->can.state > CAN_STATE_ERROR_ACTIVE) &&
> + ((es->txerr < 96) && (es->rxerr < 96))) {
> + new_state = CAN_STATE_ERROR_ACTIVE;
> + }
> + }
> +
> + if (!es->status)
> + new_state = CAN_STATE_ERROR_ACTIVE;
> +
> + if (priv->can.restart_ms &&
> + (priv->can.state >= CAN_STATE_BUS_OFF) &&
> + (new_state < CAN_STATE_BUS_OFF)) {
> + priv->can.can_stats.restarts++;
> + }
> +
> + if (es->error_factor) {
> + priv->can.can_stats.bus_error++;
> + stats->rx_errors++;
> + }
> +
> + priv->bec.txerr = es->txerr;
> + priv->bec.rxerr = es->rxerr;
> + priv->can.state = new_state;
> +}
> +
> static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> const struct kvaser_msg *msg)
> {
> @@ -622,30 +674,30 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> struct sk_buff *skb;
> struct net_device_stats *stats;
> struct kvaser_usb_net_priv *priv;
> - unsigned int new_state;
> - u8 channel, status, txerr, rxerr, error_factor;
> + struct kvaser_usb_error_summary es = { };
> + enum can_state old_state;
>
> switch (msg->id) {
> case CMD_CAN_ERROR_EVENT:
> - channel = msg->u.error_event.channel;
> - status = msg->u.error_event.status;
> - txerr = msg->u.error_event.tx_errors_count;
> - rxerr = msg->u.error_event.rx_errors_count;
> - error_factor = msg->u.error_event.error_factor;
> + es.channel = msg->u.error_event.channel;
> + es.status = msg->u.error_event.status;
> + es.txerr = msg->u.error_event.tx_errors_count;
> + es.rxerr = msg->u.error_event.rx_errors_count;
> + es.error_factor = msg->u.error_event.error_factor;
> break;
> case CMD_LOG_MESSAGE:
> - channel = msg->u.log_message.channel;
> - status = msg->u.log_message.data[0];
> - txerr = msg->u.log_message.data[2];
> - rxerr = msg->u.log_message.data[3];
> - error_factor = msg->u.log_message.data[1];
> + es.channel = msg->u.log_message.channel;
> + es.status = msg->u.log_message.data[0];
> + es.txerr = msg->u.log_message.data[2];
> + es.rxerr = msg->u.log_message.data[3];
> + es.error_factor = msg->u.log_message.data[1];
> break;
> case CMD_CHIP_STATE_EVENT:
> - channel = msg->u.chip_state_event.channel;
> - status = msg->u.chip_state_event.status;
> - txerr = msg->u.chip_state_event.tx_errors_count;
> - rxerr = msg->u.chip_state_event.rx_errors_count;
> - error_factor = 0;
> + es.channel = msg->u.chip_state_event.channel;
> + es.status = msg->u.chip_state_event.status;
> + es.txerr = msg->u.chip_state_event.tx_errors_count;
> + es.rxerr = msg->u.chip_state_event.rx_errors_count;
> + es.error_factor = 0;
> break;
> default:
> dev_err(dev->udev->dev.parent, "Invalid msg id (%d)\n",
> @@ -653,116 +705,93 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> return;
> }
>
> - if (channel >= dev->nchannels) {
> + if (es.channel >= dev->nchannels) {
> dev_err(dev->udev->dev.parent,
> - "Invalid channel number (%d)\n", channel);
> + "Invalid channel number (%d)\n", es.channel);
> return;
> }
>
> - priv = dev->nets[channel];
> + priv = dev->nets[es.channel];
> stats = &priv->netdev->stats;
>
> + /* Update all of the can interface's state and error counters before
> + * trying any skb allocation that can actually fail with -ENOMEM.
> + */
> + old_state = priv->can.state;
> + kvaser_usb_rx_error_update_can_state(priv, &es);
> +
> skb = alloc_can_err_skb(priv->netdev, &cf);
> if (!skb) {
> stats->rx_dropped++;
> return;
> }
>
> - new_state = priv->can.state;
> -
> - netdev_dbg(priv->netdev, "Error status: 0x%02x\n", status);
> -
> - if (status & (M16C_STATE_BUS_OFF | M16C_STATE_BUS_RESET)) {
> + if (es.status & (M16C_STATE_BUS_OFF | M16C_STATE_BUS_RESET)) {
> cf->can_id |= CAN_ERR_BUSOFF;
>
> - priv->can.can_stats.bus_off++;
> if (!priv->can.restart_ms)
> kvaser_usb_simple_msg_async(priv, CMD_STOP_CHIP);
> -
> netif_carrier_off(priv->netdev);
> -
> - new_state = CAN_STATE_BUS_OFF;
> - } else if (status & M16C_STATE_BUS_PASSIVE) {
> - if (priv->can.state != CAN_STATE_ERROR_PASSIVE) {
> + } else if (es.status & M16C_STATE_BUS_PASSIVE) {
> + if (old_state != CAN_STATE_ERROR_PASSIVE) {
> cf->can_id |= CAN_ERR_CRTL;
>
> - if (txerr || rxerr)
> - cf->data[1] = (txerr > rxerr)
> + if (es.txerr || es.rxerr)
> + cf->data[1] = (es.txerr > es.rxerr)
> ? CAN_ERR_CRTL_TX_PASSIVE
> : CAN_ERR_CRTL_RX_PASSIVE;
> else
> cf->data[1] = CAN_ERR_CRTL_TX_PASSIVE |
> CAN_ERR_CRTL_RX_PASSIVE;
> -
> - priv->can.can_stats.error_passive++;
> }
> -
> - new_state = CAN_STATE_ERROR_PASSIVE;
> - } else if (status & M16C_STATE_BUS_ERROR) {
> - if ((priv->can.state < CAN_STATE_ERROR_WARNING) &&
> - ((txerr >= 96) || (rxerr >= 96))) {
> + } else if (es.status & M16C_STATE_BUS_ERROR) {
> + if ((old_state < CAN_STATE_ERROR_WARNING) &&
> + ((es.txerr >= 96) || (es.rxerr >= 96))) {
> cf->can_id |= CAN_ERR_CRTL;
> - cf->data[1] = (txerr > rxerr)
> + cf->data[1] = (es.txerr > es.rxerr)
> ? CAN_ERR_CRTL_TX_WARNING
> : CAN_ERR_CRTL_RX_WARNING;
> -
> - priv->can.can_stats.error_warning++;
> - new_state = CAN_STATE_ERROR_WARNING;
> - } else if ((priv->can.state > CAN_STATE_ERROR_ACTIVE) &&
> - ((txerr < 96) && (rxerr < 96))) {
> + } else if ((old_state > CAN_STATE_ERROR_ACTIVE) &&
> + ((es.txerr < 96) && (es.rxerr < 96))) {
> cf->can_id |= CAN_ERR_PROT;
> cf->data[2] = CAN_ERR_PROT_ACTIVE;
> -
> - new_state = CAN_STATE_ERROR_ACTIVE;
> }
> }
>
> - if (!status) {
> + if (!es.status) {
> cf->can_id |= CAN_ERR_PROT;
> cf->data[2] = CAN_ERR_PROT_ACTIVE;
> -
> - new_state = CAN_STATE_ERROR_ACTIVE;
> }
>
> if (priv->can.restart_ms &&
> - (priv->can.state >= CAN_STATE_BUS_OFF) &&
> - (new_state < CAN_STATE_BUS_OFF)) {
> + (old_state >= CAN_STATE_BUS_OFF) &&
> + (priv->can.state < CAN_STATE_BUS_OFF)) {
> cf->can_id |= CAN_ERR_RESTARTED;
> netif_carrier_on(priv->netdev);
> -
> - priv->can.can_stats.restarts++;
> }
>
> - if (error_factor) {
> - priv->can.can_stats.bus_error++;
> - stats->rx_errors++;
> -
> + if (es.error_factor) {
> cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
>
> - if (error_factor & M16C_EF_ACKE)
> + if (es.error_factor & M16C_EF_ACKE)
> cf->data[3] |= (CAN_ERR_PROT_LOC_ACK);
> - if (error_factor & M16C_EF_CRCE)
> + if (es.error_factor & M16C_EF_CRCE)
> cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
> CAN_ERR_PROT_LOC_CRC_DEL);
> - if (error_factor & M16C_EF_FORME)
> + if (es.error_factor & M16C_EF_FORME)
> cf->data[2] |= CAN_ERR_PROT_FORM;
> - if (error_factor & M16C_EF_STFE)
> + if (es.error_factor & M16C_EF_STFE)
> cf->data[2] |= CAN_ERR_PROT_STUFF;
> - if (error_factor & M16C_EF_BITE0)
> + if (es.error_factor & M16C_EF_BITE0)
> cf->data[2] |= CAN_ERR_PROT_BIT0;
> - if (error_factor & M16C_EF_BITE1)
> + if (es.error_factor & M16C_EF_BITE1)
> cf->data[2] |= CAN_ERR_PROT_BIT1;
> - if (error_factor & M16C_EF_TRE)
> + if (es.error_factor & M16C_EF_TRE)
> cf->data[2] |= CAN_ERR_PROT_TX;
> }
>
> - cf->data[6] = txerr;
> - cf->data[7] = rxerr;
> -
> - priv->bec.txerr = txerr;
> - priv->bec.rxerr = rxerr;
> -
> - priv->can.state = new_state;
> + cf->data[6] = es.txerr;
> + cf->data[7] = es.rxerr;
>
> stats->rx_packets++;
> stats->rx_bytes += cf->can_dlc;
> @@ -786,6 +815,9 @@ static void kvaser_usb_rx_can_err(const struct kvaser_usb_net_priv *priv,
> }
>
> if (msg->u.rx_can.flag & MSG_FLAG_OVERRUN) {
> + stats->rx_over_errors++;
> + stats->rx_errors++;
> +
> skb = alloc_can_err_skb(priv->netdev, &cf);
> if (!skb) {
> stats->rx_dropped++;
> @@ -795,9 +827,6 @@ static void kvaser_usb_rx_can_err(const struct kvaser_usb_net_priv *priv,
> cf->can_id |= CAN_ERR_CRTL;
> cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
>
> - stats->rx_over_errors++;
> - stats->rx_errors++;
> -
> stats->rx_packets++;
> stats->rx_bytes += cf->can_dlc;
> netif_rx(skb);
> --
> 1.7.7.6
Looks good to me.
--
Andri
^ permalink raw reply
* Re: [PATCH v6 7/7] can: kvaser_usb: Add support for the USBcan-II family
From: Andri Yngvason @ 2015-01-26 10:34 UTC (permalink / raw)
To: Ahmed S. Darwish, Olivier Sobrie, Oliver Hartkopp,
Wolfgang Grandegger, Marc Kleine-Budde
Cc: Linux-CAN, netdev, LKML
In-Reply-To: <20150126053310.GH3014@Darwish.PC>
Quoting Ahmed S. Darwish (2015-01-26 05:33:10)
> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
>
> CAN to USB interfaces sold by the Swedish manufacturer Kvaser are
> divided into two major families: 'Leaf', and 'USBcanII'. From an
> Operating System perspective, the firmware of both families behave
> in a not too drastically different fashion.
>
> This patch adds support for the USBcanII family of devices to the
> current Kvaser Leaf-only driver.
>
> CAN frames sending, receiving, and error handling paths has been
> tested using the dual-channel "Kvaser USBcan II HS/LS" dongle. It
> should also work nicely with other products in the same category.
>
> List of new devices supported by this driver update:
>
> - Kvaser USBcan II HS/HS
> - Kvaser USBcan II HS/LS
> - Kvaser USBcan Rugged ("USBcan Rev B")
> - Kvaser Memorator HS/HS
> - Kvaser Memorator HS/LS
> - Scania VCI2 (if you have the Kvaser logo on top)
>
> Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> ---
> drivers/net/can/usb/Kconfig | 8 +-
> drivers/net/can/usb/kvaser_usb.c | 590 ++++++++++++++++++++++++++++++--------
> 2 files changed, 474 insertions(+), 124 deletions(-)
>
> ** V6 Changelog:
> - Revert to the error-active state if the error counters were
> decreased by hardware
> - Rebase over a new set of upstream Leaf-driver bugfixes
>
> ** V5 Changelog:
> - Rebase on the new CAN error state changes added for the Leaf driver
> - Add minor changes (remove unused commands, constify poniters, etc.)
>
> ** V4 Changelog:
> - Use type-safe C methods instead of cpp macros
> - Remove defensive checks against non-existing families
> - Re-order methods to remove forward declarations
> - Smaller stuff spotted by earlier review (function prefexes, etc.)
>
> ** V3 Changelog:
> - Fix padding for the usbcan_msg_tx_acknowledge command
> - Remove kvaser_usb->max_channels and the MAX_NET_DEVICES macro
> - Rename commands to CMD_LEAF_xxx and CMD_USBCAN_xxx
> - Apply checkpatch.pl suggestions ('net/' comments, multi-line strings, etc.)
>
> ** V2 Changelog:
> - Update Kconfig entries
> - Use actual number of CAN channels (instead of max) where appropriate
> - Rebase over a new set of UsbcanII-independent driver fixes
>
> diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
> index a77db919..f6f5500 100644
> --- a/drivers/net/can/usb/Kconfig
> +++ b/drivers/net/can/usb/Kconfig
> @@ -25,7 +25,7 @@ config CAN_KVASER_USB
> tristate "Kvaser CAN/USB interface"
> ---help---
> This driver adds support for Kvaser CAN/USB devices like Kvaser
> - Leaf Light.
> + Leaf Light and Kvaser USBcan II.
>
> The driver provides support for the following devices:
> - Kvaser Leaf Light
> @@ -46,6 +46,12 @@ config CAN_KVASER_USB
> - Kvaser USBcan R
> - Kvaser Leaf Light v2
> - Kvaser Mini PCI Express HS
> + - Kvaser USBcan II HS/HS
> + - Kvaser USBcan II HS/LS
> + - Kvaser USBcan Rugged ("USBcan Rev B")
> + - Kvaser Memorator HS/HS
> + - Kvaser Memorator HS/LS
> + - Scania VCI2 (if you have the Kvaser logo on top)
>
> If unsure, say N.
>
> diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
> index ddc2954..17d28d7 100644
> --- a/drivers/net/can/usb/kvaser_usb.c
> +++ b/drivers/net/can/usb/kvaser_usb.c
> @@ -6,10 +6,12 @@
> * Parts of this driver are based on the following:
> * - Kvaser linux leaf driver (version 4.78)
> * - CAN driver for esd CAN-USB/2
> + * - Kvaser linux usbcanII driver (version 5.3)
> *
> * Copyright (C) 2002-2006 KVASER AB, Sweden. All rights reserved.
> * Copyright (C) 2010 Matthias Fuchs <matthias.fuchs@esd.eu>, esd gmbh
> * Copyright (C) 2012 Olivier Sobrie <olivier@sobrie.be>
> + * Copyright (C) 2015 Valeo A.S.
> */
>
> #include <linux/completion.h>
> @@ -30,8 +32,9 @@
> #define RX_BUFFER_SIZE 3072
> #define CAN_USB_CLOCK 8000000
> #define MAX_NET_DEVICES 3
> +#define MAX_USBCAN_NET_DEVICES 2
>
> -/* Kvaser USB devices */
> +/* Kvaser Leaf USB devices */
> #define KVASER_VENDOR_ID 0x0bfd
> #define USB_LEAF_DEVEL_PRODUCT_ID 10
> #define USB_LEAF_LITE_PRODUCT_ID 11
> @@ -56,6 +59,24 @@
> #define USB_LEAF_LITE_V2_PRODUCT_ID 288
> #define USB_MINI_PCIE_HS_PRODUCT_ID 289
>
> +static inline bool kvaser_is_leaf(const struct usb_device_id *id)
> +{
> + return id->idProduct >= USB_LEAF_DEVEL_PRODUCT_ID &&
> + id->idProduct <= USB_MINI_PCIE_HS_PRODUCT_ID;
> +}
> +
> +/* Kvaser USBCan-II devices */
> +#define USB_USBCAN_REVB_PRODUCT_ID 2
> +#define USB_VCI2_PRODUCT_ID 3
> +#define USB_USBCAN2_PRODUCT_ID 4
> +#define USB_MEMORATOR_PRODUCT_ID 5
> +
> +static inline bool kvaser_is_usbcan(const struct usb_device_id *id)
> +{
> + return id->idProduct >= USB_USBCAN_REVB_PRODUCT_ID &&
> + id->idProduct <= USB_MEMORATOR_PRODUCT_ID;
> +}
> +
> /* USB devices features */
> #define KVASER_HAS_SILENT_MODE BIT(0)
> #define KVASER_HAS_TXRX_ERRORS BIT(1)
> @@ -73,7 +94,7 @@
> #define MSG_FLAG_TX_ACK BIT(6)
> #define MSG_FLAG_TX_REQUEST BIT(7)
>
> -/* Can states */
> +/* Can states (M16C CxSTRH register) */
> #define M16C_STATE_BUS_RESET BIT(0)
> #define M16C_STATE_BUS_ERROR BIT(4)
> #define M16C_STATE_BUS_PASSIVE BIT(5)
> @@ -98,7 +119,11 @@
> #define CMD_START_CHIP_REPLY 27
> #define CMD_STOP_CHIP 28
> #define CMD_STOP_CHIP_REPLY 29
> -#define CMD_GET_CARD_INFO2 32
> +
> +#define CMD_LEAF_GET_CARD_INFO2 32
> +#define CMD_USBCAN_RESET_CLOCK 32
> +#define CMD_USBCAN_CLOCK_OVERFLOW_EVENT 33
> +
> #define CMD_GET_CARD_INFO 34
> #define CMD_GET_CARD_INFO_REPLY 35
> #define CMD_GET_SOFTWARE_INFO 38
> @@ -108,8 +133,9 @@
> #define CMD_RESET_ERROR_COUNTER 49
> #define CMD_TX_ACKNOWLEDGE 50
> #define CMD_CAN_ERROR_EVENT 51
> -#define CMD_USB_THROTTLE 77
> -#define CMD_LOG_MESSAGE 106
> +
> +#define CMD_LEAF_USB_THROTTLE 77
> +#define CMD_LEAF_LOG_MESSAGE 106
>
> /* error factors */
> #define M16C_EF_ACKE BIT(0)
> @@ -121,6 +147,14 @@
> #define M16C_EF_RCVE BIT(6)
> #define M16C_EF_TRE BIT(7)
>
> +/* Only Leaf-based devices can report M16C error factors,
> + * thus define our own error status flags for USBCANII
> + */
> +#define USBCAN_ERROR_STATE_NONE 0
> +#define USBCAN_ERROR_STATE_TX_ERROR BIT(0)
> +#define USBCAN_ERROR_STATE_RX_ERROR BIT(1)
> +#define USBCAN_ERROR_STATE_BUSERROR BIT(2)
> +
> /* bittiming parameters */
> #define KVASER_USB_TSEG1_MIN 1
> #define KVASER_USB_TSEG1_MAX 16
> @@ -137,9 +171,18 @@
> #define KVASER_CTRL_MODE_SELFRECEPTION 3
> #define KVASER_CTRL_MODE_OFF 4
>
> -/* log message */
> +/* Extended CAN identifier flag */
> #define KVASER_EXTENDED_FRAME BIT(31)
>
> +/* Kvaser USB CAN dongles are divided into two major families:
> + * - Leaf: Based on Renesas M32C, running firmware labeled as 'filo'
> + * - UsbcanII: Based on Renesas M16C, running firmware labeled as 'helios'
> + */
> +enum kvaser_usb_family {
> + KVASER_LEAF,
> + KVASER_USBCAN,
> +};
> +
> struct kvaser_msg_simple {
> u8 tid;
> u8 channel;
> @@ -148,30 +191,55 @@ struct kvaser_msg_simple {
> struct kvaser_msg_cardinfo {
> u8 tid;
> u8 nchannels;
> - __le32 serial_number;
> - __le32 padding;
> + union {
> + struct {
> + __le32 serial_number;
> + __le32 padding;
> + } __packed leaf0;
> + struct {
> + __le32 serial_number_low;
> + __le32 serial_number_high;
> + } __packed usbcan0;
> + } __packed;
> __le32 clock_resolution;
> __le32 mfgdate;
> u8 ean[8];
> u8 hw_revision;
> - u8 usb_hs_mode;
> - __le16 padding2;
> + union {
> + struct {
> + u8 usb_hs_mode;
> + } __packed leaf1;
> + struct {
> + u8 padding;
> + } __packed usbcan1;
> + } __packed;
> + __le16 padding;
> } __packed;
>
> struct kvaser_msg_cardinfo2 {
> u8 tid;
> - u8 channel;
> + u8 reserved;
> u8 pcb_id[24];
> __le32 oem_unlock_code;
> } __packed;
>
> -struct kvaser_msg_softinfo {
> +struct leaf_msg_softinfo {
> u8 tid;
> - u8 channel;
> + u8 padding0;
> __le32 sw_options;
> __le32 fw_version;
> __le16 max_outstanding_tx;
> - __le16 padding[9];
> + __le16 padding1[9];
> +} __packed;
> +
> +struct usbcan_msg_softinfo {
> + u8 tid;
> + u8 fw_name[5];
> + __le16 max_outstanding_tx;
> + u8 padding[6];
> + __le32 fw_version;
> + __le16 checksum;
> + __le16 sw_options;
> } __packed;
>
> struct kvaser_msg_busparams {
> @@ -188,36 +256,86 @@ struct kvaser_msg_tx_can {
> u8 channel;
> u8 tid;
> u8 msg[14];
> - u8 padding;
> - u8 flags;
> + union {
> + struct {
> + u8 padding;
> + u8 flags;
> + } __packed leaf;
> + struct {
> + u8 flags;
> + u8 padding;
> + } __packed usbcan;
> + } __packed;
> +} __packed;
> +
> +struct kvaser_msg_rx_can_header {
> + u8 channel;
> + u8 flag;
> } __packed;
>
> -struct kvaser_msg_rx_can {
> +struct leaf_msg_rx_can {
> u8 channel;
> u8 flag;
> +
> __le16 time[3];
> u8 msg[14];
> } __packed;
>
> -struct kvaser_msg_chip_state_event {
> +struct usbcan_msg_rx_can {
> + u8 channel;
> + u8 flag;
> +
> + u8 msg[14];
> + __le16 time;
> +} __packed;
> +
> +struct leaf_msg_chip_state_event {
> u8 tid;
> u8 channel;
> +
> __le16 time[3];
> u8 tx_errors_count;
> u8 rx_errors_count;
> +
> + u8 status;
> + u8 padding[3];
> +} __packed;
> +
> +struct usbcan_msg_chip_state_event {
> + u8 tid;
> + u8 channel;
> +
> + u8 tx_errors_count;
> + u8 rx_errors_count;
> + __le16 time;
> +
> u8 status;
> u8 padding[3];
> } __packed;
>
> -struct kvaser_msg_tx_acknowledge {
> +struct kvaser_msg_tx_acknowledge_header {
> u8 channel;
> u8 tid;
> +} __packed;
> +
> +struct leaf_msg_tx_acknowledge {
> + u8 channel;
> + u8 tid;
> +
> __le16 time[3];
> u8 flags;
> u8 time_offset;
> } __packed;
>
> -struct kvaser_msg_error_event {
> +struct usbcan_msg_tx_acknowledge {
> + u8 channel;
> + u8 tid;
> +
> + __le16 time;
> + __le16 padding;
> +} __packed;
> +
> +struct leaf_msg_error_event {
> u8 tid;
> u8 flags;
> __le16 time[3];
> @@ -229,6 +347,18 @@ struct kvaser_msg_error_event {
> u8 error_factor;
> } __packed;
>
> +struct usbcan_msg_error_event {
> + u8 tid;
> + u8 padding;
> + u8 tx_errors_count_ch0;
> + u8 rx_errors_count_ch0;
> + u8 tx_errors_count_ch1;
> + u8 rx_errors_count_ch1;
> + u8 status_ch0;
> + u8 status_ch1;
> + __le16 time;
> +} __packed;
> +
> struct kvaser_msg_ctrl_mode {
> u8 tid;
> u8 channel;
> @@ -243,7 +373,7 @@ struct kvaser_msg_flush_queue {
> u8 padding[3];
> } __packed;
>
> -struct kvaser_msg_log_message {
> +struct leaf_msg_log_message {
> u8 channel;
> u8 flags;
> __le16 time[3];
> @@ -260,21 +390,55 @@ struct kvaser_msg {
> struct kvaser_msg_simple simple;
> struct kvaser_msg_cardinfo cardinfo;
> struct kvaser_msg_cardinfo2 cardinfo2;
> - struct kvaser_msg_softinfo softinfo;
> struct kvaser_msg_busparams busparams;
> +
> + struct kvaser_msg_rx_can_header rx_can_header;
> + struct kvaser_msg_tx_acknowledge_header tx_acknowledge_header;
> +
> + union {
> + struct leaf_msg_softinfo softinfo;
> + struct leaf_msg_rx_can rx_can;
> + struct leaf_msg_chip_state_event chip_state_event;
> + struct leaf_msg_tx_acknowledge tx_acknowledge;
> + struct leaf_msg_error_event error_event;
> + struct leaf_msg_log_message log_message;
> + } __packed leaf;
> +
> + union {
> + struct usbcan_msg_softinfo softinfo;
> + struct usbcan_msg_rx_can rx_can;
> + struct usbcan_msg_chip_state_event chip_state_event;
> + struct usbcan_msg_tx_acknowledge tx_acknowledge;
> + struct usbcan_msg_error_event error_event;
> + } __packed usbcan;
> +
> struct kvaser_msg_tx_can tx_can;
> - struct kvaser_msg_rx_can rx_can;
> - struct kvaser_msg_chip_state_event chip_state_event;
> - struct kvaser_msg_tx_acknowledge tx_acknowledge;
> - struct kvaser_msg_error_event error_event;
> struct kvaser_msg_ctrl_mode ctrl_mode;
> struct kvaser_msg_flush_queue flush_queue;
> - struct kvaser_msg_log_message log_message;
> } u;
> } __packed;
>
> +/* Summary of a kvaser error event, for a unified Leaf/Usbcan error
> + * handling. Some discrepancies between the two families exist:
> + *
> + * - USBCAN firmware does not report M16C "error factors"
> + * - USBCAN controllers has difficulties reporting if the raised error
> + * event is for ch0 or ch1. They leave such arbitration to the OS
> + * driver by letting it compare error counters with previous values
> + * and decide the error event's channel. Thus for USBCAN, the channel
> + * field is only advisory.
> + */
> struct kvaser_usb_error_summary {
> - u8 channel, status, txerr, rxerr, error_factor;
> + u8 channel, status, txerr, rxerr;
> + union {
> + struct {
> + u8 error_factor;
> + } leaf;
> + struct {
> + u8 other_ch_status;
> + u8 error_state;
> + } usbcan;
> + };
> };
>
> struct kvaser_usb_tx_urb_context {
> @@ -292,6 +456,7 @@ struct kvaser_usb {
>
> u32 fw_version;
> unsigned int nchannels;
> + enum kvaser_usb_family family;
>
> bool rxinitdone;
> void *rxbuf[MAX_RX_URBS];
> @@ -315,6 +480,7 @@ struct kvaser_usb_net_priv {
> };
>
> static const struct usb_device_id kvaser_usb_table[] = {
> + /* Leaf family IDs */
> { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_DEVEL_PRODUCT_ID) },
> { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_PRODUCT_ID) },
> { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_PRODUCT_ID),
> @@ -364,6 +530,17 @@ static const struct usb_device_id kvaser_usb_table[] = {
> .driver_info = KVASER_HAS_TXRX_ERRORS },
> { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_V2_PRODUCT_ID) },
> { USB_DEVICE(KVASER_VENDOR_ID, USB_MINI_PCIE_HS_PRODUCT_ID) },
> +
> + /* USBCANII family IDs */
> + { USB_DEVICE(KVASER_VENDOR_ID, USB_USBCAN2_PRODUCT_ID),
> + .driver_info = KVASER_HAS_TXRX_ERRORS },
> + { USB_DEVICE(KVASER_VENDOR_ID, USB_USBCAN_REVB_PRODUCT_ID),
> + .driver_info = KVASER_HAS_TXRX_ERRORS },
> + { USB_DEVICE(KVASER_VENDOR_ID, USB_MEMORATOR_PRODUCT_ID),
> + .driver_info = KVASER_HAS_TXRX_ERRORS },
> + { USB_DEVICE(KVASER_VENDOR_ID, USB_VCI2_PRODUCT_ID),
> + .driver_info = KVASER_HAS_TXRX_ERRORS },
> +
> { }
> };
> MODULE_DEVICE_TABLE(usb, kvaser_usb_table);
> @@ -467,7 +644,14 @@ static int kvaser_usb_get_software_info(struct kvaser_usb *dev)
> if (err)
> return err;
>
> - dev->fw_version = le32_to_cpu(msg.u.softinfo.fw_version);
> + switch (dev->family) {
> + case KVASER_LEAF:
> + dev->fw_version = le32_to_cpu(msg.u.leaf.softinfo.fw_version);
> + break;
> + case KVASER_USBCAN:
> + dev->fw_version = le32_to_cpu(msg.u.usbcan.softinfo.fw_version);
> + break;
> + }
>
> return 0;
> }
> @@ -486,7 +670,9 @@ static int kvaser_usb_get_card_info(struct kvaser_usb *dev)
> return err;
>
> dev->nchannels = msg.u.cardinfo.nchannels;
> - if (dev->nchannels > MAX_NET_DEVICES)
> + if ((dev->nchannels > MAX_NET_DEVICES) ||
> + (dev->family == KVASER_USBCAN &&
> + dev->nchannels > MAX_USBCAN_NET_DEVICES))
> return -EINVAL;
>
> return 0;
> @@ -500,8 +686,10 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
> struct kvaser_usb_net_priv *priv;
> struct sk_buff *skb;
> struct can_frame *cf;
> - u8 channel = msg->u.tx_acknowledge.channel;
> - u8 tid = msg->u.tx_acknowledge.tid;
> + u8 channel, tid;
> +
> + channel = msg->u.tx_acknowledge_header.channel;
> + tid = msg->u.tx_acknowledge_header.tid;
>
> if (channel >= dev->nchannels) {
> dev_err(dev->udev->dev.parent,
> @@ -623,12 +811,12 @@ static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv *pri
> const struct kvaser_usb_error_summary *es,
> struct can_frame *cf)
> {
> - struct net_device_stats *stats;
> + struct kvaser_usb *dev = priv->dev;
> + struct net_device_stats *stats = &priv->netdev->stats;
> enum can_state cur_state, new_state, tx_state, rx_state;
>
> netdev_dbg(priv->netdev, "Error status: 0x%02x\n", es->status);
>
> - stats = &priv->netdev->stats;
> new_state = cur_state = priv->can.state;
>
> if (es->status & (M16C_STATE_BUS_OFF | M16C_STATE_BUS_RESET))
> @@ -662,9 +850,22 @@ static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv *pri
> priv->can.can_stats.restarts++;
> }
>
> - if (es->error_factor) {
> - priv->can.can_stats.bus_error++;
> - stats->rx_errors++;
> + switch (dev->family) {
> + case KVASER_LEAF:
> + if (es->leaf.error_factor) {
> + priv->can.can_stats.bus_error++;
> + stats->rx_errors++;
> + }
> + break;
> + case KVASER_USBCAN:
> + if (es->usbcan.error_state & USBCAN_ERROR_STATE_TX_ERROR)
> + stats->tx_errors++;
> + if (es->usbcan.error_state & USBCAN_ERROR_STATE_RX_ERROR)
> + stats->rx_errors++;
> + if (es->usbcan.error_state & USBCAN_ERROR_STATE_BUSERROR) {
> + priv->can.can_stats.bus_error++;
> + }
> + break;
> }
>
> priv->bec.txerr = es->txerr;
> @@ -672,50 +873,21 @@ static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv *pri
> }
>
> static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> - const struct kvaser_msg *msg)
> + const struct kvaser_usb_error_summary *es)
> {
> struct can_frame *cf, tmp_cf = { .can_id = CAN_ERR_FLAG, .can_dlc = CAN_ERR_DLC };
> struct sk_buff *skb;
> struct net_device_stats *stats;
> struct kvaser_usb_net_priv *priv;
> - struct kvaser_usb_error_summary es = { };
> enum can_state old_state, new_state;
>
> - switch (msg->id) {
> - case CMD_CAN_ERROR_EVENT:
> - es.channel = msg->u.error_event.channel;
> - es.status = msg->u.error_event.status;
> - es.txerr = msg->u.error_event.tx_errors_count;
> - es.rxerr = msg->u.error_event.rx_errors_count;
> - es.error_factor = msg->u.error_event.error_factor;
> - break;
> - case CMD_LOG_MESSAGE:
> - es.channel = msg->u.log_message.channel;
> - es.status = msg->u.log_message.data[0];
> - es.txerr = msg->u.log_message.data[2];
> - es.rxerr = msg->u.log_message.data[3];
> - es.error_factor = msg->u.log_message.data[1];
> - break;
> - case CMD_CHIP_STATE_EVENT:
> - es.channel = msg->u.chip_state_event.channel;
> - es.status = msg->u.chip_state_event.status;
> - es.txerr = msg->u.chip_state_event.tx_errors_count;
> - es.rxerr = msg->u.chip_state_event.rx_errors_count;
> - es.error_factor = 0;
> - break;
> - default:
> - dev_err(dev->udev->dev.parent, "Invalid msg id (%d)\n",
> - msg->id);
> - return;
> - }
> -
> - if (es.channel >= dev->nchannels) {
> + if (es->channel >= dev->nchannels) {
> dev_err(dev->udev->dev.parent,
> - "Invalid channel number (%d)\n", es.channel);
> + "Invalid channel number (%d)\n", es->channel);
> return;
> }
>
> - priv = dev->nets[es.channel];
> + priv = dev->nets[es->channel];
> stats = &priv->netdev->stats;
>
> /* Update all of the can interface's state and error counters before
> @@ -729,7 +901,7 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> * frame ID and data to userspace. Remove stack allocation afterwards.
> */
> old_state = priv->can.state;
> - kvaser_usb_rx_error_update_can_state(priv, &es, &tmp_cf);
> + kvaser_usb_rx_error_update_can_state(priv, es, &tmp_cf);
> new_state = priv->can.state;
>
> skb = alloc_can_err_skb(priv->netdev, &cf);
> @@ -740,7 +912,7 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> memcpy(cf, &tmp_cf, sizeof(*cf));
>
> if (new_state != old_state) {
> - if (es.status &
> + if (es->status &
> (M16C_STATE_BUS_OFF | M16C_STATE_BUS_RESET)) {
> if (!priv->can.restart_ms)
> kvaser_usb_simple_msg_async(priv, CMD_STOP_CHIP);
> @@ -755,34 +927,161 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> }
> }
>
> - if (es.error_factor) {
> - cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
> -
> - if (es.error_factor & M16C_EF_ACKE)
> - cf->data[3] |= (CAN_ERR_PROT_LOC_ACK);
> - if (es.error_factor & M16C_EF_CRCE)
> - cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
> - CAN_ERR_PROT_LOC_CRC_DEL);
> - if (es.error_factor & M16C_EF_FORME)
> - cf->data[2] |= CAN_ERR_PROT_FORM;
> - if (es.error_factor & M16C_EF_STFE)
> - cf->data[2] |= CAN_ERR_PROT_STUFF;
> - if (es.error_factor & M16C_EF_BITE0)
> - cf->data[2] |= CAN_ERR_PROT_BIT0;
> - if (es.error_factor & M16C_EF_BITE1)
> - cf->data[2] |= CAN_ERR_PROT_BIT1;
> - if (es.error_factor & M16C_EF_TRE)
> - cf->data[2] |= CAN_ERR_PROT_TX;
> + switch (dev->family) {
> + case KVASER_LEAF:
> + if (es->leaf.error_factor) {
> + cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
> +
> + if (es->leaf.error_factor & M16C_EF_ACKE)
> + cf->data[3] |= (CAN_ERR_PROT_LOC_ACK);
> + if (es->leaf.error_factor & M16C_EF_CRCE)
> + cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
> + CAN_ERR_PROT_LOC_CRC_DEL);
> + if (es->leaf.error_factor & M16C_EF_FORME)
> + cf->data[2] |= CAN_ERR_PROT_FORM;
> + if (es->leaf.error_factor & M16C_EF_STFE)
> + cf->data[2] |= CAN_ERR_PROT_STUFF;
> + if (es->leaf.error_factor & M16C_EF_BITE0)
> + cf->data[2] |= CAN_ERR_PROT_BIT0;
> + if (es->leaf.error_factor & M16C_EF_BITE1)
> + cf->data[2] |= CAN_ERR_PROT_BIT1;
> + if (es->leaf.error_factor & M16C_EF_TRE)
> + cf->data[2] |= CAN_ERR_PROT_TX;
> + }
> + break;
> + case KVASER_USBCAN:
> + if (es->usbcan.error_state & USBCAN_ERROR_STATE_BUSERROR) {
> + cf->can_id |= CAN_ERR_BUSERROR;
> + }
> + break;
> }
>
> - cf->data[6] = es.txerr;
> - cf->data[7] = es.rxerr;
> + cf->data[6] = es->txerr;
> + cf->data[7] = es->rxerr;
>
> stats->rx_packets++;
> stats->rx_bytes += cf->can_dlc;
> netif_rx(skb);
> }
>
> +/* For USBCAN, report error to userspace iff the channels's errors counter
> + * has changed, or we're the only channel seeing a bus error state.
> + */
> +static void kvaser_usbcan_conditionally_rx_error(const struct kvaser_usb *dev,
> + struct kvaser_usb_error_summary *es)
> +{
> + struct kvaser_usb_net_priv *priv;
> + int channel;
> + bool report_error;
> +
> + channel = es->channel;
> + if (channel >= dev->nchannels) {
> + dev_err(dev->udev->dev.parent,
> + "Invalid channel number (%d)\n", channel);
> + return;
> + }
> +
> + priv = dev->nets[channel];
> + report_error = false;
> +
> + if (es->txerr != priv->bec.txerr) {
> + es->usbcan.error_state |= USBCAN_ERROR_STATE_TX_ERROR;
> + report_error = true;
> + }
> + if (es->rxerr != priv->bec.rxerr) {
> + es->usbcan.error_state |= USBCAN_ERROR_STATE_RX_ERROR;
> + report_error = true;
> + }
> + if ((es->status & M16C_STATE_BUS_ERROR) &&
> + !(es->usbcan.other_ch_status & M16C_STATE_BUS_ERROR)) {
> + es->usbcan.error_state |= USBCAN_ERROR_STATE_BUSERROR;
> + report_error = true;
> + }
> +
> + if (report_error)
> + kvaser_usb_rx_error(dev, es);
> +}
> +
> +static void kvaser_usbcan_rx_error(const struct kvaser_usb *dev,
> + const struct kvaser_msg *msg)
> +{
> + struct kvaser_usb_error_summary es = { };
> +
> + switch (msg->id) {
> + /* Sometimes errors are sent as unsolicited chip state events */
> + case CMD_CHIP_STATE_EVENT:
> + es.channel = msg->u.usbcan.chip_state_event.channel;
> + es.status = msg->u.usbcan.chip_state_event.status;
> + es.txerr = msg->u.usbcan.chip_state_event.tx_errors_count;
> + es.rxerr = msg->u.usbcan.chip_state_event.rx_errors_count;
> + kvaser_usbcan_conditionally_rx_error(dev, &es);
> + break;
> +
> + case CMD_CAN_ERROR_EVENT:
> + es.channel = 0;
> + es.status = msg->u.usbcan.error_event.status_ch0;
> + es.txerr = msg->u.usbcan.error_event.tx_errors_count_ch0;
> + es.rxerr = msg->u.usbcan.error_event.rx_errors_count_ch0;
> + es.usbcan.other_ch_status =
> + msg->u.usbcan.error_event.status_ch1;
> + kvaser_usbcan_conditionally_rx_error(dev, &es);
> +
> + /* The USBCAN firmware supports up to 2 channels.
> + * Now that ch0 was checked, check if ch1 has any errors.
> + */
> + if (dev->nchannels == MAX_USBCAN_NET_DEVICES) {
> + es.channel = 1;
> + es.status = msg->u.usbcan.error_event.status_ch1;
> + es.txerr = msg->u.usbcan.error_event.tx_errors_count_ch1;
> + es.rxerr = msg->u.usbcan.error_event.rx_errors_count_ch1;
> + es.usbcan.other_ch_status =
> + msg->u.usbcan.error_event.status_ch0;
> + kvaser_usbcan_conditionally_rx_error(dev, &es);
> + }
> + break;
> +
> + default:
> + dev_err(dev->udev->dev.parent, "Invalid msg id (%d)\n",
> + msg->id);
> + }
> +}
> +
> +static void kvaser_leaf_rx_error(const struct kvaser_usb *dev,
> + const struct kvaser_msg *msg)
> +{
> + struct kvaser_usb_error_summary es = { };
> +
> + switch (msg->id) {
> + case CMD_CAN_ERROR_EVENT:
> + es.channel = msg->u.leaf.error_event.channel;
> + es.status = msg->u.leaf.error_event.status;
> + es.txerr = msg->u.leaf.error_event.tx_errors_count;
> + es.rxerr = msg->u.leaf.error_event.rx_errors_count;
> + es.leaf.error_factor = msg->u.leaf.error_event.error_factor;
> + break;
> + case CMD_LEAF_LOG_MESSAGE:
> + es.channel = msg->u.leaf.log_message.channel;
> + es.status = msg->u.leaf.log_message.data[0];
> + es.txerr = msg->u.leaf.log_message.data[2];
> + es.rxerr = msg->u.leaf.log_message.data[3];
> + es.leaf.error_factor = msg->u.leaf.log_message.data[1];
> + break;
> + case CMD_CHIP_STATE_EVENT:
> + es.channel = msg->u.leaf.chip_state_event.channel;
> + es.status = msg->u.leaf.chip_state_event.status;
> + es.txerr = msg->u.leaf.chip_state_event.tx_errors_count;
> + es.rxerr = msg->u.leaf.chip_state_event.rx_errors_count;
> + es.leaf.error_factor = 0;
> + break;
> + default:
> + dev_err(dev->udev->dev.parent, "Invalid msg id (%d)\n",
> + msg->id);
> + return;
> + }
> +
> + kvaser_usb_rx_error(dev, &es);
> +}
> +
> static void kvaser_usb_rx_can_err(const struct kvaser_usb_net_priv *priv,
> const struct kvaser_msg *msg)
> {
> @@ -790,16 +1089,16 @@ static void kvaser_usb_rx_can_err(const struct kvaser_usb_net_priv *priv,
> struct sk_buff *skb;
> struct net_device_stats *stats = &priv->netdev->stats;
>
> - if (msg->u.rx_can.flag & (MSG_FLAG_ERROR_FRAME |
> + if (msg->u.rx_can_header.flag & (MSG_FLAG_ERROR_FRAME |
> MSG_FLAG_NERR)) {
> netdev_err(priv->netdev, "Unknow error (flags: 0x%02x)\n",
> - msg->u.rx_can.flag);
> + msg->u.rx_can_header.flag);
>
> stats->rx_errors++;
> return;
> }
>
> - if (msg->u.rx_can.flag & MSG_FLAG_OVERRUN) {
> + if (msg->u.rx_can_header.flag & MSG_FLAG_OVERRUN) {
> stats->rx_over_errors++;
> stats->rx_errors++;
>
> @@ -825,7 +1124,8 @@ static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
> struct can_frame *cf;
> struct sk_buff *skb;
> struct net_device_stats *stats;
> - u8 channel = msg->u.rx_can.channel;
> + u8 channel = msg->u.rx_can_header.channel;
> + const u8 *rx_msg = NULL; /* GCC */
>
> if (channel >= dev->nchannels) {
> dev_err(dev->udev->dev.parent,
> @@ -836,60 +1136,68 @@ static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
> priv = dev->nets[channel];
> stats = &priv->netdev->stats;
>
> - if ((msg->u.rx_can.flag & MSG_FLAG_ERROR_FRAME) &&
> - (msg->id == CMD_LOG_MESSAGE)) {
> - kvaser_usb_rx_error(dev, msg);
> + if ((msg->u.rx_can_header.flag & MSG_FLAG_ERROR_FRAME) &&
> + (dev->family == KVASER_LEAF && msg->id == CMD_LEAF_LOG_MESSAGE)) {
> + kvaser_leaf_rx_error(dev, msg);
> return;
> - } else if (msg->u.rx_can.flag & (MSG_FLAG_ERROR_FRAME |
> - MSG_FLAG_NERR |
> - MSG_FLAG_OVERRUN)) {
> + } else if (msg->u.rx_can_header.flag & (MSG_FLAG_ERROR_FRAME |
> + MSG_FLAG_NERR |
> + MSG_FLAG_OVERRUN)) {
> kvaser_usb_rx_can_err(priv, msg);
> return;
> - } else if (msg->u.rx_can.flag & ~MSG_FLAG_REMOTE_FRAME) {
> + } else if (msg->u.rx_can_header.flag & ~MSG_FLAG_REMOTE_FRAME) {
> netdev_warn(priv->netdev,
> "Unhandled frame (flags: 0x%02x)",
> - msg->u.rx_can.flag);
> + msg->u.rx_can_header.flag);
> return;
> }
>
> + switch (dev->family) {
> + case KVASER_LEAF:
> + rx_msg = msg->u.leaf.rx_can.msg;
> + break;
> + case KVASER_USBCAN:
> + rx_msg = msg->u.usbcan.rx_can.msg;
> + break;
> + }
> +
> skb = alloc_can_skb(priv->netdev, &cf);
> if (!skb) {
> stats->tx_dropped++;
> return;
> }
>
> - if (msg->id == CMD_LOG_MESSAGE) {
> - cf->can_id = le32_to_cpu(msg->u.log_message.id);
> + if (dev->family == KVASER_LEAF && msg->id == CMD_LEAF_LOG_MESSAGE) {
> + cf->can_id = le32_to_cpu(msg->u.leaf.log_message.id);
> if (cf->can_id & KVASER_EXTENDED_FRAME)
> cf->can_id &= CAN_EFF_MASK | CAN_EFF_FLAG;
> else
> cf->can_id &= CAN_SFF_MASK;
>
> - cf->can_dlc = get_can_dlc(msg->u.log_message.dlc);
> + cf->can_dlc = get_can_dlc(msg->u.leaf.log_message.dlc);
>
> - if (msg->u.log_message.flags & MSG_FLAG_REMOTE_FRAME)
> + if (msg->u.leaf.log_message.flags & MSG_FLAG_REMOTE_FRAME)
> cf->can_id |= CAN_RTR_FLAG;
> else
> - memcpy(cf->data, &msg->u.log_message.data,
> + memcpy(cf->data, &msg->u.leaf.log_message.data,
> cf->can_dlc);
> } else {
> - cf->can_id = ((msg->u.rx_can.msg[0] & 0x1f) << 6) |
> - (msg->u.rx_can.msg[1] & 0x3f);
> + cf->can_id = ((rx_msg[0] & 0x1f) << 6) | (rx_msg[1] & 0x3f);
>
> if (msg->id == CMD_RX_EXT_MESSAGE) {
> cf->can_id <<= 18;
> - cf->can_id |= ((msg->u.rx_can.msg[2] & 0x0f) << 14) |
> - ((msg->u.rx_can.msg[3] & 0xff) << 6) |
> - (msg->u.rx_can.msg[4] & 0x3f);
> + cf->can_id |= ((rx_msg[2] & 0x0f) << 14) |
> + ((rx_msg[3] & 0xff) << 6) |
> + (rx_msg[4] & 0x3f);
> cf->can_id |= CAN_EFF_FLAG;
> }
>
> - cf->can_dlc = get_can_dlc(msg->u.rx_can.msg[5]);
> + cf->can_dlc = get_can_dlc(rx_msg[5]);
>
> - if (msg->u.rx_can.flag & MSG_FLAG_REMOTE_FRAME)
> + if (msg->u.rx_can_header.flag & MSG_FLAG_REMOTE_FRAME)
> cf->can_id |= CAN_RTR_FLAG;
> else
> - memcpy(cf->data, &msg->u.rx_can.msg[6],
> + memcpy(cf->data, &rx_msg[6],
> cf->can_dlc);
> }
>
> @@ -952,21 +1260,35 @@ static void kvaser_usb_handle_message(const struct kvaser_usb *dev,
>
> case CMD_RX_STD_MESSAGE:
> case CMD_RX_EXT_MESSAGE:
> - case CMD_LOG_MESSAGE:
> + kvaser_usb_rx_can_msg(dev, msg);
> + break;
> +
> + case CMD_LEAF_LOG_MESSAGE:
> + if (dev->family != KVASER_LEAF)
> + goto warn;
> kvaser_usb_rx_can_msg(dev, msg);
> break;
>
> case CMD_CHIP_STATE_EVENT:
> case CMD_CAN_ERROR_EVENT:
> - kvaser_usb_rx_error(dev, msg);
> + if (dev->family == KVASER_LEAF)
> + kvaser_leaf_rx_error(dev, msg);
> + else
> + kvaser_usbcan_rx_error(dev, msg);
> break;
>
> case CMD_TX_ACKNOWLEDGE:
> kvaser_usb_tx_acknowledge(dev, msg);
> break;
>
> + /* Ignored messages */
> + case CMD_USBCAN_CLOCK_OVERFLOW_EVENT:
> + if (dev->family != KVASER_USBCAN)
> + goto warn;
> + break;
> +
> default:
> - dev_warn(dev->udev->dev.parent,
> +warn: dev_warn(dev->udev->dev.parent,
> "Unhandled message (%d)\n", msg->id);
> break;
> }
> @@ -1186,7 +1508,7 @@ static void kvaser_usb_unlink_all_urbs(struct kvaser_usb *dev)
> dev->rxbuf[i],
> dev->rxbuf_dma[i]);
>
> - for (i = 0; i < MAX_NET_DEVICES; i++) {
> + for (i = 0; i < dev->nchannels; i++) {
> struct kvaser_usb_net_priv *priv = dev->nets[i];
>
> if (priv)
> @@ -1294,6 +1616,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
> struct kvaser_msg *msg;
> int i, err;
> int ret = NETDEV_TX_OK;
> + u8 *msg_tx_can_flags = NULL; /* GCC */
>
> if (can_dropped_invalid_skb(netdev, skb))
> return NETDEV_TX_OK;
> @@ -1315,9 +1638,19 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
>
> msg = buf;
> msg->len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_tx_can);
> - msg->u.tx_can.flags = 0;
> msg->u.tx_can.channel = priv->channel;
>
> + switch (dev->family) {
> + case KVASER_LEAF:
> + msg_tx_can_flags = &msg->u.tx_can.leaf.flags;
> + break;
> + case KVASER_USBCAN:
> + msg_tx_can_flags = &msg->u.tx_can.usbcan.flags;
> + break;
> + }
> +
> + *msg_tx_can_flags = 0;
> +
> if (cf->can_id & CAN_EFF_FLAG) {
> msg->id = CMD_TX_EXT_MESSAGE;
> msg->u.tx_can.msg[0] = (cf->can_id >> 24) & 0x1f;
> @@ -1335,7 +1668,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
> memcpy(&msg->u.tx_can.msg[6], cf->data, cf->can_dlc);
>
> if (cf->can_id & CAN_RTR_FLAG)
> - msg->u.tx_can.flags |= MSG_FLAG_REMOTE_FRAME;
> + *msg_tx_can_flags |= MSG_FLAG_REMOTE_FRAME;
>
> for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++) {
> if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
> @@ -1604,6 +1937,17 @@ static int kvaser_usb_probe(struct usb_interface *intf,
> if (!dev)
> return -ENOMEM;
>
> + if (kvaser_is_leaf(id)) {
> + dev->family = KVASER_LEAF;
> + } else if (kvaser_is_usbcan(id)) {
> + dev->family = KVASER_USBCAN;
> + } else {
> + dev_err(&intf->dev,
> + "Product ID (%d) does not belong to any known Kvaser USB family",
> + id->idProduct);
> + return -ENODEV;
> + }
> +
> err = kvaser_usb_get_endpoints(intf, &dev->bulk_in, &dev->bulk_out);
> if (err) {
> dev_err(&intf->dev, "Cannot get usb endpoint(s)");
> --
> 1.7.7.6
There's a lot of changes here but as far as error state is concerned, I see
nothing out of the ordinary.
--
Andri
^ permalink raw reply
* Re: [PATCH v6 5/7] can: kvaser_usb: Update interface state before exiting on OOM
From: Marc Kleine-Budde @ 2015-01-26 10:50 UTC (permalink / raw)
To: Andri Yngvason, Ahmed S. Darwish, Olivier Sobrie, Oliver Hartkopp,
Wolfgang Grandegger
Cc: Linux-CAN, netdev, LKML
In-Reply-To: <20150126102843.17894.84372@shannon>
[-- Attachment #1: Type: text/plain, Size: 910 bytes --]
On 01/26/2015 11:28 AM, Andri Yngvason wrote:
> Quoting Ahmed S. Darwish (2015-01-26 05:27:19)
>> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
>>
>> Update all of the can interface's state and error counters before
>> trying any skb allocation that can actually fail with -ENOMEM.
>>
>> Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
>> ---
>> drivers/net/can/usb/kvaser_usb.c | 181 ++++++++++++++++++++++----------------
>> 1 files changed, 105 insertions(+), 76 deletions(-)
[..]
> Looks good to me.
Can I add your Acked-by to 5-7?
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: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v6 5/7] can: kvaser_usb: Update interface state before exiting on OOM
From: Andri Yngvason @ 2015-01-26 10:52 UTC (permalink / raw)
To: Marc Kleine-Budde, Ahmed S. Darwish, Olivier Sobrie,
Oliver Hartkopp, Wolfgang Grandegger
Cc: Linux-CAN, netdev, LKML
In-Reply-To: <54C61BE4.8070105@pengutronix.de>
Quoting Marc Kleine-Budde (2015-01-26 10:50:12)
> On 01/26/2015 11:28 AM, Andri Yngvason wrote:
> > Quoting Ahmed S. Darwish (2015-01-26 05:27:19)
> >> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> >>
> >> Update all of the can interface's state and error counters before
> >> trying any skb allocation that can actually fail with -ENOMEM.
> >>
> >> Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
> >> Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> >> ---
> >> drivers/net/can/usb/kvaser_usb.c | 181 ++++++++++++++++++++++----------------
> >> 1 files changed, 105 insertions(+), 76 deletions(-)
> [..]
>
> > Looks good to me.
>
> Can I add your Acked-by to 5-7?
>
Yes.
--
Andri
^ 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