Netdev List
 help / color / mirror / Atom feed
* Re: Corrupted SKB
From: Michael Ma @ 2017-04-19  4:46 UTC (permalink / raw)
  To: Cong Wang; +Cc: Linux Kernel Network Developers, jin.oyj
In-Reply-To: <CAM_iQpVOi0FH_quusHHpvREdvpqq6=RjVOQvcjAWGbh1X0_5tA@mail.gmail.com>

2017-04-18 16:12 GMT-07:00 Cong Wang <xiyou.wangcong@gmail.com>:
> On Mon, Apr 17, 2017 at 5:39 PM, Michael Ma <make0818@gmail.com> wrote:
>> Hi -
>>
>> We've implemented a "glue" qdisc similar to mqprio which can associate
>> one qdisc to multiple txqs as the root qdisc. Reference count of the
>> child qdiscs have been adjusted properly in this case so that it
>> represents the number of txqs it has been attached to. However when
>> sending packets we saw the skb from dequeue_skb() corrupted with the
>> following call stack:
>>
>>     [exception RIP: netif_skb_features+51]
>>     RIP: ffffffff815292b3  RSP: ffff8817f6987940  RFLAGS: 00010246
>>
>>  #9 [ffff8817f6987968] validate_xmit_skb at ffffffff815294aa
>> #10 [ffff8817f69879a0] validate_xmit_skb at ffffffff8152a0d9
>> #11 [ffff8817f69879b0] __qdisc_run at ffffffff8154a193
>> #12 [ffff8817f6987a00] dev_queue_xmit at ffffffff81529e03
>>
>> It looks like the skb has already been released since its dev pointer
>> field is invalid.
>>
>> Any clue on how this can be investigated further? My current thought
>> is to add some instrumentation to the place where skb is released and
>> analyze whether there is any race condition happening there. However
>
> Either dropwatch or perf could do the work to instrument kfree_skb().

Thanks - will try it out.
>
>> by looking through the existing code I think the case where one root
>> qdisc is associated with multiple txqs already exists (when mqprio is
>> not used) so not sure why it won't work when we group txqs and assign
>> each group a root qdisc. Any insight on this issue would be much
>> appreciated!
>
> How do you implement ->attach()? How does it work with netdev_pick_tx()?

attach() essentially grafts the default qdisc(pfifo) to each "txq
group" represented by a TC class. For netdev_pick_txq() we use classid
of the socket to select a class based on a "class id base" and the
class to txq mapping defined together with this glue qdisc - it's
pretty much the same as mqprio with the difference of mapping one
class to multiple txqs and selecting the txq through a hash.

^ permalink raw reply

* (unknown), 
From: kelley @ 2017-04-19  4:29 UTC (permalink / raw)
  To: netdev

[-- Attachment #1: EMAIL_42114478079_netdev.zip --]
[-- Type: application/zip, Size: 1237 bytes --]

^ permalink raw reply

* Re: [PATCH net-next 1/1] net sched actions: add time filter for action dumping
From: Jakub Kicinski @ 2017-04-19  3:55 UTC (permalink / raw)
  To: Jamal Hadi Salim; +Cc: davem, netdev, xiyou.wangcong, eric.dumazet, jiri
In-Reply-To: <930f107b-ac5f-3fab-445b-9050437c1909@mojatatu.com>

On Tue, 18 Apr 2017 21:37:12 -0400, Jamal Hadi Salim wrote:
> On 17-04-18 06:12 PM, Jakub Kicinski wrote:
> 
> > Noob question - would it be an option to establish some form of
> > streaming statistic updates out of the kernel?  
> 
> It would be more efficient, certainly. Current polling approach is not
> as bad though (you send one extra kernel message) and is more reliable
> (events generated by the kernel are not reliably delivered to user
> space).

I see.

> > The notion of user
> > space trying to keep track of the time here seems a bit shaky.  
> 
> You mean the patch i sent or suggestion you are making would be
> more shaky?
> I see both being fine from that perspective - you dont need 100%
> accuracy. Just something that is within reason of a small delta
> of time.

I'm just referring to the theoretical possibility that if the dumping
process gets preempted for long enough you may loose samples.  Just
because the dumping process cannot control when kernel executes this
line:

	jiffy_wanted = jiffies - jiffy_msecs;

It could in theory be few seconds after the request was made.  Perhaps
using timestamps from a proper time sources instead of the notion of
"last X seconds" would solve that?

^ permalink raw reply

* Re: [PATCH net-next 1/2 v2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Eric Dumazet @ 2017-04-19  3:17 UTC (permalink / raw)
  To: Jamal Hadi Salim; +Cc: davem, netdev, jiri, xiyou.wangcong
In-Reply-To: <b00c5e30-47e9-0e71-7980-b57740ac09be@mojatatu.com>

On Tue, 2017-04-18 at 22:32 -0400, Jamal Hadi Salim wrote:
> On 17-04-18 09:49 PM, Eric Dumazet wrote:
> > On Tue, 2017-04-18 at 21:14 -0400, Jamal Hadi Salim wrote:
> >> From: Jamal Hadi Salim <jhs@mojatatu.com>
> >
> > So there is no more limit ? How user is supposed to size the buffer for
> > recvmsg() ?
> >
> 
> That part doesnt change.  Ok, I believe more clarity is needed:->
> 
> Current code allows only 32 actions to be dumped at a time.
> This code:
> 
> So lets i have 128 actions. I can fit them into 10K.
> I do recvmsg(..., 32K). This will make about 32K space for me to fill
> up the 128 actions. But the code only allows me to send 32
> TCA_ACT_MAX_PRIO which takes about 2-3K.
> So I will do 4 runs from kernel->user to get 128 actions ;->
> User space in tc is trained to expect no more than
> TCA_ACT_MAX_PRIO in every batch.
> 
> With this change I ask the kernel to fit as many actions as
> possible in the 32K (all these 128 will fit in one batch).
> Then it has to tell user space how many are in that batch
> using TCAA_ACT_COUNT attribute.
> 
> Make sense?

What if we have 1024 actions, and user provides a 4KB buffer ?

Normally multiple recvmsg() calls would be needed, but I do not see how
the nla_put_u32(skb, TCAA_ACT_COUNT, cb->args[1]) can always succeed.

^ permalink raw reply

* Re: [PATCH net-next 1/2 v2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jamal Hadi Salim @ 2017-04-19  2:32 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: davem, netdev, jiri, xiyou.wangcong
In-Reply-To: <1492566545.10587.159.camel@edumazet-glaptop3.roam.corp.google.com>

On 17-04-18 09:49 PM, Eric Dumazet wrote:
> On Tue, 2017-04-18 at 21:14 -0400, Jamal Hadi Salim wrote:
>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>
> So there is no more limit ? How user is supposed to size the buffer for
> recvmsg() ?
>

That part doesnt change.  Ok, I believe more clarity is needed:->

Current code allows only 32 actions to be dumped at a time.
This code:

So lets i have 128 actions. I can fit them into 10K.
I do recvmsg(..., 32K). This will make about 32K space for me to fill
up the 128 actions. But the code only allows me to send 32
TCA_ACT_MAX_PRIO which takes about 2-3K.
So I will do 4 runs from kernel->user to get 128 actions ;->
User space in tc is trained to expect no more than
TCA_ACT_MAX_PRIO in every batch.

With this change I ask the kernel to fit as many actions as
possible in the 32K (all these 128 will fit in one batch).
Then it has to tell user space how many are in that batch
using TCAA_ACT_COUNT attribute.

Make sense?

cheers,
jamal

^ permalink raw reply

* Re: [PATCH RFC] sparc64: eBPF JIT
From: David Miller @ 2017-04-19  2:27 UTC (permalink / raw)
  To: alexei.starovoitov; +Cc: sparclinux, netdev, ast, daniel
In-Reply-To: <20170418225707.GA65449@ast-mbp.thefacebook.com>

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date: Tue, 18 Apr 2017 15:57:09 -0700

> On Tue, Apr 18, 2017 at 02:37:32PM -0400, David Miller wrote:
>> From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
>> Date: Mon, 17 Apr 2017 22:44:47 -0700
>> 
>> > The way llvm generates stack access is:
>> > rX = r10
>> > rX += imm
>> > and that's the only thing verifier recognizes as valid ptr_to_stack.
>> > Like rX -= imm will not be recognized as proper stack offset,
>> > since llvm never does it.
>> 
>> That simplifies things significantly for me.
>> 
>> I only allow moves from the frame pointer to another register,
>> and when I see that I rewrite it to "add FP, STACK_BIAS, DST_REG"
> 
> Sounds good to me. Alternative idea: can the above
> 'add FP, STACK_BIAS, one_of_local_regs' be done once in prologue
> and that register used as substitue for R10 ?
> (assuming non-leaf function)
> I completely forgot by now how 2047 magic works.

Well, I'm trying to understand what that would buy us.

If I'm always going to see that kind of sequence:

	rX = r10
	rx += offset

then I always have that "r10" move available to deal with the bias.
It comes for free.

Right now the current version of the sparc64 JIT will emit:

	add	%fp, 2047, bpf2sparc[rX]
	add	bpf2sparc[rX], offset, bpf2sparc[rX]

for that BPF sequence.

Anyways, thanks to your and Daniel's feedback I'm very confident
with the code now.  All that's really left is tail calls, and I'll
set up a BPF compilation and test environment for that.

^ permalink raw reply

* Re: FEC on i.MX 7 transmit queue timeout
From: Andy Duan @ 2017-04-19  2:24 UTC (permalink / raw)
  To: Stefan Agner, fugang.duan@freescale.com, festevam@gmail.com
  Cc: netdev@vger.kernel.org
In-Reply-To: <c6b66956c48f981a952048da6b2ddd54@agner.ch>


On 2017年04月19日 03:46, Stefan Agner wrote:
> Hi,
>
> I noticed last week on upstream (v4.11-rc6) on a Colibri iMX7 board that
> after a while (~10 minutes) the detdev wachdog prints a stacktrace and
> the driver then continuously dumps the TX ring. I then did a quick test
> with 4.10, and realized it actually suffers the same issue, so it seems
> not to be a regression. I use a rootfs mounted over NFS...
>
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 0 at net/sched/sch_generic.c:316
> dev_watchdog+0x240/0x244
> NETDEV WATCHDOG: eth0 (fec): transmit queue 2 timed out
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted
> 4.11.0-rc7-00030-g2c4e6bd0c4f0-dirty #330
> Hardware name: Freescale i.MX7 Dual (Device Tree)
> [<c02293f0>] (unwind_backtrace) from [<c0225820>] (show_stack+0x10/0x14)
> [<c0225820>] (show_stack) from [<c050db6c>] (dump_stack+0x90/0xa0)
> [<c050db6c>] (dump_stack) from [<c023ae68>] (__warn+0xac/0x11c)
> [<c023ae68>] (__warn) from [<c023af10>] (warn_slowpath_fmt+0x38/0x48)
> [<c023af10>] (warn_slowpath_fmt) from [<c088bb8c>]
> (dev_watchdog+0x240/0x244)
> [<c088bb8c>] (dev_watchdog) from [<c0294798>]
> (run_timer_softirq+0x24c/0x708)
> [<c0294798>] (run_timer_softirq) from [<c023f584>]
> (__do_softirq+0x12c/0x2a8)
> [<c023f584>] (__do_softirq) from [<c023f8c4>] (irq_exit+0xdc/0x13c)
> [<c023f8c4>] (irq_exit) from [<c02818ac>]
> (__handle_domain_irq+0xa4/0xf8)
> [<c02818ac>] (__handle_domain_irq) from [<c0201624>]
> (gic_handle_irq+0x34/0xa4)
> [<c0201624>] (gic_handle_irq) from [<c0226338>] (__irq_svc+0x58/0x8c)
> Exception stack(0xc1201f30 to 0xc1201f78)
> 1f20:                                     c0233320 00000000 00000000
> 01400000
> 1f40: c1203d80 ffffe000 00000000 00000000 c107bf10 c0e055b5 c1203d34
> 00000001
> 1f60: c07d2324 c1201f80 c0222ac8 c0222acc 60000013 ffffffff
> [<c0226338>] (__irq_svc) from [<c0222acc>] (arch_cpu_idle+0x38/0x3c)
> [<c0222acc>] (arch_cpu_idle) from [<c0275f24>] (do_idle+0xa8/0x250)
> [<c0275f24>] (do_idle) from [<c02760e4>] (cpu_startup_entry+0x18/0x1c)
> [<c02760e4>] (cpu_startup_entry) from [<c1000aa0>]
> (start_kernel+0x3fc/0x45c)
> ---[ end trace 5b0c6dc3466a7918 ]---
> fec 30be0000.ethernet eth0: TX ring dump
> Nr     SC     addr       len  SKB
>    0    0x1c00 0x00000000  590   (null)
>    1    0x1c00 0x00000000  590   (null)
>    2    0x1c00 0x00000000   42   (null)
>    3  H 0x1c00 0x00000000   42   (null)
>    4 S  0x0000 0x00000000    0   (null)
>    5    0x0000 0x00000000    0   (null)
>    6    0x0000 0x00000000    0   (null)
>    7    0x0000 0x00000000    0   (null)
>    8    0x0000 0x00000000    0   (null)
>    9    0x0000 0x00000000    0   (null)
>   10    0x0000 0x00000000    0   (null)
>   11    0x0000 0x00000000    0   (null)
>   12    0x0000 0x00000000    0   (null)
>   13    0x0000 0x00000000    0   (null)
>   14    0x0000 0x00000000    0   (null)
>   15    0x0000 0x00000000    0   (null)
>   16    0x0000 0x00000000    0   (null)
>   17    0x0000 0x00000000    0   (null)
>   18    0x0000 0x00000000    0   (null)
> ...
>
>
> A second TX ring dump from 4.10:
> fec 30be0000.ethernet eth0: TX ring dump
> Nr     SC     addr       len  SKB
>    0    0x1c00 0x00000000   42   (null)
>    1    0x1c00 0x00000000   42   (null)
>    2    0x1c00 0x00000000   90   (null)
>    3    0x1c00 0x00000000   90   (null)
>    4    0x1c00 0x00000000   90   (null)
>    5    0x1c00 0x00000000  218   (null)
>    6    0x1c00 0x00000000  218   (null)
>    7    0x1c00 0x00000000  218   (null)
>    8    0x1c00 0x00000000   90   (null)
>    9    0x1c00 0x00000000  206   (null)
>   10    0x1c00 0x00000000  216   (null)
>   11    0x1c00 0x00000000  216   (null)
>   12    0x1c00 0x00000000  216   (null)
>   13    0x1c00 0x00000000  311   (null)
>   14    0x1c00 0x00000000  178   (null)
>   15    0x1c00 0x00000000  311   (null)
>   16    0x1c00 0x00000000  206   (null)
>   17  H 0x1c00 0x00000000  311   (null)
>   18 S  0x0000 0x00000000    0   (null)
>   19    0x0000 0x00000000    0   (null)
The dump show tx ring is fine.

>
> The ring dump prints continously, but I can access console every now and
> then. I noticed that the second interrupt seems static (66441, TX
> interrupt?):
>   58:         18     GIC-0 150 Level     30be0000.ethernet
>   59:      66441     GIC-0 151 Level     30be0000.ethernet
>   60:      70477     GIC-0 152 Level     30be0000.ethernet
150 irq number is for tx/rx queue 1 receive/transmit buffer/frame done.
151 irq number is for tx/rx queue 2 receive/transmit buffer/frame done.
152 irq number is for tx/rx queue 0 receive/transmit buffer/frame done, 
mii interrupt and others.

i.MX7D enet has three queues for tx and rx.
It seems netdev pick tx queue 1 rate is very rare by __netdev_pick_tx().
> Anybody else seen this? Any idea?
>
> In 4.10 as well as 4.11-rc6 the interrupt counts were just over 65536...
> pure chance?
>
>
you can use ethtool to set the irq coalesce like:
ethtool -c eth0 rx-frames 80
ethtool -c eth0 rx-usecs 600
ethtool -c eth0 tx-frames 64
ethtool -c eth0 tx-usenc 700


You don't run any test case, just nfs mount rootfs ?
I will setup one imx7d sdb board to run it.

Andy

^ permalink raw reply

* Re: [PATCH net-next]smsc911x: Adding support for Micochip LAN9250 Ethernet controller
From: Andrew Lunn @ 2017-04-19  2:24 UTC (permalink / raw)
  To: David.Cai
  Cc: netdev, davem, UNGLinuxDriver, steve.glendinning, Ravi.Hegde,
	Woojung.Huh
In-Reply-To: <C3C28FB10418274EB7FD7C2B85C796A44124623F@CHN-SV-EXMX02.mchp-main.com>

On Wed, Apr 19, 2017 at 12:29:45AM +0000, David.Cai@microchip.com wrote:
> Attached is a patch which adding support for Microchip LAN9250 Ethernet controller.

Hi David

It looks like your mail system mangled your patch. Please use git
send-email.

> 
> Thanks,
> 
> =====================
> David Cai
> Principle Software Engineer
> Microchip Technology Inc.
> =====================
> 
> Signed-off-by: David Cai <david.cai@microchip.com>
> ---

You should probably also read the SubmittingPatches Documentation.

Everything before the --- is included in the commit log.

	   Andrew

^ permalink raw reply

* [PATCH net-next] tcp_cubic: fix typo in module param description
From: Chema Gonzalez @ 2017-04-19  2:22 UTC (permalink / raw)
  To: David Miller, Eric Dumazet; +Cc: netdev, Chema Gonzalez

From: Chema Gonzalez <chemag@gmail.com>

Signed-off-by: Chema Gonzalez <chemag@gmail.com>
---
 net/ipv4/tcp_cubic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c
index c99230efcd52..0683ba447d77 100644
--- a/net/ipv4/tcp_cubic.c
+++ b/net/ipv4/tcp_cubic.c
@@ -72,7 +72,7 @@ MODULE_PARM_DESC(tcp_friendliness, "turn on/off tcp friendliness");
 module_param(hystart, int, 0644);
 MODULE_PARM_DESC(hystart, "turn on/off hybrid slow start algorithm");
 module_param(hystart_detect, int, 0644);
-MODULE_PARM_DESC(hystart_detect, "hyrbrid slow start detection mechanisms"
+MODULE_PARM_DESC(hystart_detect, "hybrid slow start detection mechanisms"
 		 " 1: packet-train 2: delay 3: both packet-train and delay");
 module_param(hystart_low_window, int, 0644);
 MODULE_PARM_DESC(hystart_low_window, "lower bound cwnd for hybrid slow start");
-- 
2.12.2.816.g2cccc81164-goog

^ permalink raw reply related

* Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier
From: Jamal Hadi Salim @ 2017-04-19  2:21 UTC (permalink / raw)
  To: Cong Wang, Wolfgang Bumiller
  Cc: Linux Kernel Network Developers, David S. Miller
In-Reply-To: <CAM_iQpWQYsqGD3TZp=iXyK0auprgo5uUbRZRvtpnFjQWwCj5Gw@mail.gmail.com>

On 17-04-18 01:03 PM, Cong Wang wrote:
> On Tue, Apr 18, 2017 at 3:13 AM, Wolfgang Bumiller
> <w.bumiller@proxmox.com> wrote:

> police action...That is why I said we may need a TCA_POLICE_COOKIE.
>

Unless it is very old user space code (which wouldnt know what a
cookie is), dont think there's much use of direct policer access.

>> I'm thinking the first patch should be enough. (I've tested forcing the
>> other filters into the error path *without* this patch and couldn't
>> produce crashes or reference count problems (while with this patch
>> applied it was leaking reference counts on creation (which makes sense
>> considering tcf_hash_release is used and the ACT_P_CREATED case will
>> keep repeating)). (Whereas without both patches simply looking through
>> creating and deleting a policing filter pretty much always resulted in
>> crashes with various different backtraces.)
>>
>
> The action API's suck here.
>
> The idea is we should rollback everything when cookie setup fails.
>
> Taking another look, it seems the current code (without this patch) is
> correct:
>
> 1) When ->init() returns ACT_P_CREATED, we should rollback both
> act creation and module refcnt, the former is already taken care by
> tcf_hash_release(), the latter is at err_mod.
>
> 2) When ->init() returns !ACT_P_CREATED, we should rollback the
> the modification to the existing action and module refcnt, the former is
> impossible with current code (because we don't do copy and update)
> so we only do tcf_hash_release(), module refcnt needs to rollback
> like normal path.
>

Indeed. Allocate the cookie before init? That way, we fail early
and dont need to worry about restoring anything.
In the case of a replace, do you really want to call tcf_hash_release?

> Ideally, these action API's should handle it nicely, exposing the
> module_put()/module_get() is ugly and confusing.
>

lots of room for improvement.

cheers,
jamal

^ permalink raw reply

* Re: [PATCH net] xfrm: calculate L4 checksums also for GSO case before encrypting packets
From: Ansis Atteka @ 2017-04-19  2:10 UTC (permalink / raw)
  To: Steffen Klassert; +Cc: Ansis Atteka, netdev
In-Reply-To: <20170418090919.GD30104@secunet.com>

On 18 April 2017 at 02:09, Steffen Klassert
<steffen.klassert@secunet.com> wrote:
> On Thu, Apr 13, 2017 at 07:45:08PM -0700, Ansis Atteka wrote:
>> On 11 April 2017 at 00:07, Steffen Klassert <steffen.klassert@secunet.com>
>> wrote:
>> >
>> > What's wrong with the checksum provided by the GSO layer and
>> > why we have to do this unconditionally here?
>> >
>>
>> I believe with "GSO layer" you meant the skb_gso_segment() function
>> invocation from xfrm_output_gso()?
>>
>> If so, then the problem with that is that the list of the skb's returned by
>> that function could be in CHECKSUM_PARTIAL state if skb came from a UDP
>> tunnel such as Geneve:
>
> This should not happen. We don't announce checksum capabilities,
> so the GSO layer should generate the full checksum.
>
> __skb_udp_tunnel_segment() and udp4_ufo_fragment() add the
> NETIF_F_HW_CSUM flag to features. This is certainly wrong
> if the packet undergoes an IPsec transformation.
>
> I don't have a testcase for this, so not sure if this is
> your problem. Could you try the (untested) patch below?

I am still seeing the same ESP packet corruption issue with the patch
you attached.

However, after taking pointers from your patch I came up with this one
that may solve this problem once and for all (note, that I was seeing
this bug only with ixgbe NIC that supports tx csum offloads). I hope
it does not break any other IPsec tests that you have.

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index b2be1d9..7812501 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -29,6 +29,7 @@ static struct sk_buff
*__skb_udp_tunnel_segment(struct sk_buff *skb,
        u16 mac_len = skb->mac_len;
        int udp_offset, outer_hlen;
        __wsum partial;
+       bool need_ipsec;

        if (unlikely(!pskb_may_pull(skb, tnl_hlen)))
                goto out;
@@ -62,8 +63,10 @@ static struct sk_buff
*__skb_udp_tunnel_segment(struct sk_buff *skb,

        ufo = !!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP);

+       need_ipsec = skb_dst(skb) && dst_xfrm(skb_dst(skb));
        /* Try to offload checksum if possible */
        offload_csum = !!(need_csum &&
+                         !need_ipsec &&
                          (skb->dev->features &
                           (is_ipv6 ? (NETIF_F_HW_CSUM | NETIF_F_IPV6_CSUM) :
                                      (NETIF_F_HW_CSUM | NETIF_F_IP_CSUM))));

^ permalink raw reply related

* Re: [PATCH net-next 1/2 v2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Eric Dumazet @ 2017-04-19  1:49 UTC (permalink / raw)
  To: Jamal Hadi Salim; +Cc: davem, netdev, jiri, xiyou.wangcong
In-Reply-To: <1492564489-28854-1-git-send-email-jhs@emojatatu.com>

On Tue, 2017-04-18 at 21:14 -0400, Jamal Hadi Salim wrote:
> From: Jamal Hadi Salim <jhs@mojatatu.com>

So there is no more limit ? How user is supposed to size the buffer for
recvmsg() ?

>  	t->tca__pad1 = 0;
> @@ -1113,6 +1133,9 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
>  	if (ret > 0) {
>  		nla_nest_end(skb, nest);
>  		ret = skb->len;
> +		if (nla_put_u32(skb, TCAA_ACT_COUNT, cb->args[1]))
> +			goto out_module_put;
> +		cb->args[1] = 0;


I really do not see how you manage to get room to add one additional
attribute, if dump had to stop at N actions, filling whole skb.

You might be lucky because nla_put_u32() wants a bit less space than an
action ?

Presumably you need to reserve space before the dump of actions.

^ permalink raw reply

* Re: [PATCH net-next 2/2 v2] net sched actions: add time filter for action dumping
From: Jamal Hadi Salim @ 2017-04-19  1:39 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: davem, netdev, jiri, xiyou.wangcong
In-Reply-To: <1492565738.10587.155.camel@edumazet-glaptop3.roam.corp.google.com>

On 17-04-18 09:35 PM, Eric Dumazet wrote:
> On Tue, 2017-04-18 at 21:14 -0400, Jamal Hadi Salim wrote:
>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>>
>
>> +	if (tcaa[TCAA_ACT_TIME_FILTER])
>> +		msecs_filter = nla_get_u32(tcaa[TCAA_ACT_TIME_FILTER]);
>
>
> You forgot to add TCAA_ACT_TIME_FILTER in tcaa_policy
>

I will fix that.

> There is no guarantee user passed 32bit data here.
>

Did you mean because we dont have the tcaa_policy or
is it for some other reason?


cheers,
jamal

^ permalink raw reply

* Re: [PATCH net-next 1/1] net sched actions: add time filter for action dumping
From: Jamal Hadi Salim @ 2017-04-19  1:37 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: davem, netdev, xiyou.wangcong, eric.dumazet, jiri
In-Reply-To: <20170418151259.72eaa05a@cakuba.lan>

On 17-04-18 06:12 PM, Jakub Kicinski wrote:

> Noob question - would it be an option to establish some form of
> streaming statistic updates out of the kernel?

It would be more efficient, certainly. Current polling approach is not
as bad though (you send one extra kernel message) and is more reliable
(events generated by the kernel are not reliably delivered to user
space).

> The notion of user
> space trying to keep track of the time here seems a bit shaky.

You mean the patch i sent or suggestion you are making would be
more shaky?
I see both being fine from that perspective - you dont need 100%
accuracy. Just something that is within reason of a small delta
of time.

>Would
> it be an option with netlink to open a socket and request specific
> set of statistic to be dumped on it periodically if they changed?
> I guess it would be way more code that the patch you present...

Sensible but like you said more code.
For efficiency you will need to have some tweakables.
Possibly something that could specify "send me X events or whatever
youve accumulated in Y seconds" etc.

Note: Here we open one socket;
and every X seconds we ask the kernel to give us whatever changed
in the last X seconds or so.

There are some drawbacks. If it takes you longer than X seconds to
walk over everything then you are in trouble. The earlier patch
speeds us up a bit. And I have some other rough patches I am
where we could have multiple threads asking for both time and
specific index ranges to make sure we got everything in time.
For a couple of million actions, I havent found big need for
those yet;->

cheers,
jamal

^ permalink raw reply

* Re: [PATCH net-next 2/2 v2] net sched actions: add time filter for action dumping
From: Eric Dumazet @ 2017-04-19  1:35 UTC (permalink / raw)
  To: Jamal Hadi Salim; +Cc: davem, netdev, jiri, xiyou.wangcong
In-Reply-To: <1492564489-28854-2-git-send-email-jhs@emojatatu.com>

On Tue, 2017-04-18 at 21:14 -0400, Jamal Hadi Salim wrote:
> From: Jamal Hadi Salim <jhs@mojatatu.com>
> 

> +	if (tcaa[TCAA_ACT_TIME_FILTER])
> +		msecs_filter = nla_get_u32(tcaa[TCAA_ACT_TIME_FILTER]);


You forgot to add TCAA_ACT_TIME_FILTER in tcaa_policy

There is no guarantee user passed 32bit data here.

> +
>  	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
>  			cb->nlh->nlmsg_type, sizeof(*t), 0);
>  	if (!nlh)
>  		goto out_module_put;
>  
> +	if (msecs_filter) {
> +		unsigned long jiffy_msecs = msecs_to_jiffies(msecs_filter);
> +		jiffy_wanted = jiffies - jiffy_msecs;
> +	}
> +
>  	cb->args[2] = act_flags;
> +	cb->args[3] = jiffy_wanted;
>  
>  	t = nlmsg_data(nlh);
>  	t->tca_family = AF_UNSPEC;

^ permalink raw reply

* [PATCH net-next 2/2] Add Jiri Pirko as TC subsystem co-maintainer
From: Jamal Hadi Salim @ 2017-04-19  1:24 UTC (permalink / raw)
  To: davem; +Cc: netdev, eric.dumazet, jiri, xiyou.wangcong, Jamal Hadi Salim
In-Reply-To: <1492565040-29102-1-git-send-email-jhs@emojatatu.com>

From: Jamal Hadi Salim <jhs@mojatatu.com>

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 749bd84..a9df81e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12190,6 +12190,7 @@ F:	kernel/taskstats.c
 TC subsystem
 M:	Jamal Hadi Salim <jhs@mojatatu.com>
 M:	Cong Wang <xiyou.wangcong@gmail.com>
+M:	Jiri Pirko <jiri@resnulli.us>
 L:	netdev@vger.kernel.org
 S:	Maintained
 F:	include/net/pkt_cls.h
-- 
1.9.1

^ permalink raw reply related

* [PATCH net-next 1/2] Add Cong Wang as TC subsystem co-maintainer
From: Jamal Hadi Salim @ 2017-04-19  1:23 UTC (permalink / raw)
  To: davem; +Cc: netdev, eric.dumazet, jiri, xiyou.wangcong, Jamal Hadi Salim

From: Jamal Hadi Salim <jhs@mojatatu.com>

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 1973878..749bd84 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12189,6 +12189,7 @@ F:	kernel/taskstats.c
 
 TC subsystem
 M:	Jamal Hadi Salim <jhs@mojatatu.com>
+M:	Cong Wang <xiyou.wangcong@gmail.com>
 L:	netdev@vger.kernel.org
 S:	Maintained
 F:	include/net/pkt_cls.h
-- 
1.9.1

^ permalink raw reply related

* [PATCH net-next 2/2 v2] net sched actions: add time filter for action dumping
From: Jamal Hadi Salim @ 2017-04-19  1:14 UTC (permalink / raw)
  To: davem; +Cc: netdev, eric.dumazet, jiri, xiyou.wangcong, Jamal Hadi Salim
In-Reply-To: <1492564489-28854-1-git-send-email-jhs@emojatatu.com>

From: Jamal Hadi Salim <jhs@mojatatu.com>

This adds support for filtering based on time since last used.
When we are dumping a large number of actions it is useful to
have the option of filtering based on when the action was last
used to reduce the amount of data crossing to user space.

With this patch the user space app sets the TCAA_ACT_TIME_FILTER
attribute with the value in milliseconds with "time of interest
since now".  The kernel converts this to jiffies and does the
filtering comparison matching entries that have seen activity
since then and returns them to user space.
Old kernels and old tc continue to work in legacy mode since
they dont specify this attribute.

Some example (we have 400 actions bound to 400 filters); at installation
time using  hacked tc which sets the time of interest to 120 seconds:

prompt$ hackedtc actions ls action gact | grep index | wc -l
400

go get some coffee and  wait for > 120 seconds and try again:

prompt$ hackedtc actions ls action gact | grep index | wc -l
0

Lets see a filter bound to one of these actions:
..
filter pref 10 u32
filter pref 10 u32 fh 800: ht divisor 1
filter pref 10 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:10  (rule hit 2 success 1)
  match 7f000002/ffffffff at 12 (success 1 )
    action order 1: gact action pass
     random type none pass val 0
     index 23 ref 2 bind 1 installed 1145 sec used 802 sec
    Action statistics:
    Sent 84 bytes 1 pkt (dropped 0, overlimits 0 requeues 0)
    backlog 0b 0p requeues 0
....

that coffee took long, no?

Now lets ping -c 1 127.0.0.2, then run the actions again:

prompt$ hackedtc actions ls action gact | grep index | wc -l
1

More details please:

prompt$ hackedtc -s actions ls action gact
total acts 1 flags 0x3

    action order 0: gact action pass
     random type none pass val 0
     index 23 ref 2 bind 1 installed 1270 sec used 30 sec
    Action statistics:
    Sent 168 bytes 2 pkt (dropped 0, overlimits 0 requeues 0)
    backlog 0b 0p requeues 0

And the filter?

filter pref 10 u32
filter pref 10 u32 fh 800: ht divisor 1
filter pref 10 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:10  (rule hit 4 success 2)
  match 7f000002/ffffffff at 12 (success 2 )
    action order 1: gact action pass
     random type none pass val 0
     index 23 ref 2 bind 1 installed 1324 sec used 84 sec
    Action statistics:
    Sent 168 bytes 2 pkt (dropped 0, overlimits 0 requeues 0)
    backlog 0b 0p requeues 0

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 include/uapi/linux/rtnetlink.h |  1 +
 net/sched/act_api.c            | 23 +++++++++++++++++++++--
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index c7080ec..1b36cc0 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -680,6 +680,7 @@ enum {
 	TCAA_ACT_TAB,
 	TCAA_ACT_FLAGS,
 	TCAA_ACT_COUNT,
+	TCAA_ACT_TIME_FILTER,
 	__TCAA_MAX
 };
 
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 45e1cf7..cb5b23d 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -84,11 +84,12 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
 {
 	int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
 	unsigned short act_flags = cb->args[2];
+	unsigned long jiffy_filter = cb->args[3];
 	struct nlattr *nest;
 
 	spin_lock_bh(&hinfo->lock);
 
-	s_i = cb->args[0];
+	s_i = cb->args[4];
 
 	for (i = 0; i < (hinfo->hmask + 1); i++) {
 		struct hlist_head *head;
@@ -101,6 +102,11 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
 			if (index < s_i)
 				continue;
 
+			if (jiffy_filter &&
+			    time_after(jiffy_filter,
+				       (unsigned long)p->tcfa_tm.lastuse))
+				continue;
+
 			nest = nla_nest_start(skb, n_i);
 			if (nest == NULL)
 				goto nla_put_failure;
@@ -118,6 +124,9 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
 		}
 	}
 done:
+	if (index > 0)
+		cb->args[4] = index + 1;
+
 	spin_unlock_bh(&hinfo->lock);
 	if (n_i) {
 		cb->args[0] += n_i;
@@ -1090,13 +1099,14 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
 	struct tcamsg *t = (struct tcamsg *) nlmsg_data(cb->nlh);
 	struct nlattr *kind = NULL;
 	u32 act_flags = 0;
+	u32 msecs_filter = 0;
+	unsigned long jiffy_wanted = 0;
 
 	ret = nlmsg_parse(cb->nlh, sizeof(struct tcamsg), tcaa, TCAA_MAX,
 			  tcaa_policy, NULL);
 	if (ret < 0)
 		return ret;
 
-
 	kind = find_dump_kind(tcaa);
 	if (kind == NULL) {
 		pr_info("tc_dump_action: action bad kind\n");
@@ -1110,12 +1120,21 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
 	if (tcaa[TCAA_ACT_FLAGS])
 		act_flags = nla_get_u32(tcaa[TCAA_ACT_FLAGS]);
 
+	if (tcaa[TCAA_ACT_TIME_FILTER])
+		msecs_filter = nla_get_u32(tcaa[TCAA_ACT_TIME_FILTER]);
+
 	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
 			cb->nlh->nlmsg_type, sizeof(*t), 0);
 	if (!nlh)
 		goto out_module_put;
 
+	if (msecs_filter) {
+		unsigned long jiffy_msecs = msecs_to_jiffies(msecs_filter);
+		jiffy_wanted = jiffies - jiffy_msecs;
+	}
+
 	cb->args[2] = act_flags;
+	cb->args[3] = jiffy_wanted;
 
 	t = nlmsg_data(nlh);
 	t->tca_family = AF_UNSPEC;
-- 
1.9.1

^ permalink raw reply related

* [PATCH net-next 1/2 v2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jamal Hadi Salim @ 2017-04-19  1:14 UTC (permalink / raw)
  To: davem; +Cc: netdev, eric.dumazet, jiri, xiyou.wangcong, Jamal Hadi Salim

From: Jamal Hadi Salim <jhs@mojatatu.com>

When you dump hundreds of thousands of actions, getting only 32 per
dump batch even when the socket buffer and memory allocations allow
is inefficient.

With this change, the user will get as many as possibly fitting
within the given constraints available to the kernel.

A new top level TLV space is introduced. An attribute
TCAA_ACT_FLAGS is used to carry the flags indicating the user
is capable of processing these large dumps. Older user space which
doesnt set this flag doesnt get the large (than 32) batches.
The kernel uses the TCAA_ACT_COUNT attribute to tell the user how many
actions are put in a single batch. As such user space app knows how long
to iterate (independent of the type of action being dumped)
instead of hardcoded maximum of 32.

Some results dumping 1.5M actions, first unpatched tc which the
kernel doesnt help:

prompt$ time -p tc actions ls action gact | grep index | wc -l
1500000
real 1388.43
user 2.07
sys 1386.79

Now lets see a patched tc which sets the correct flags when requesting
a dump:

prompt$ time -p updatedtc actions ls action gact | grep index | wc -l
1500000
real 178.13
user 2.02
sys 176.96

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 MAINTAINERS                    |  1 +
 include/uapi/linux/rtnetlink.h | 21 +++++++++++++++++++--
 net/sched/act_api.c            | 43 ++++++++++++++++++++++++++++++++----------
 3 files changed, 53 insertions(+), 12 deletions(-)

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index cce0613..c7080ec 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -674,10 +674,27 @@ struct tcamsg {
 	unsigned char	tca__pad1;
 	unsigned short	tca__pad2;
 };
+
+enum {
+	TCAA_UNSPEC,
+	TCAA_ACT_TAB,
+	TCAA_ACT_FLAGS,
+	TCAA_ACT_COUNT,
+	__TCAA_MAX
+};
+
+#define TCAA_MAX (__TCAA_MAX - 1)
 #define TA_RTA(r)  ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg))))
 #define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
-#define TCA_ACT_TAB 1 /* attr type must be >=1 */	
-#define TCAA_MAX 1
+#define TCA_ACT_TAB TCAA_ACT_TAB
+/* tcamsg flags stored in attribute TCAA_ACT_FLAGS
+ *
+ * ACT_LARGE_DUMP_ON user->kernel to request for larger than TCA_ACT_MAX_PRIO
+ * actions in a dump. All dump responses will contain the number of actions
+ * being dumped stored in for user app's consumption in TCAA_ACT_COUNT
+ *
+ */
+#define ACT_LARGE_DUMP_ON		(1 << 0)
 
 /* New extended info filters for IFLA_EXT_MASK */
 #define RTEXT_FILTER_VF		(1 << 0)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 82b1d48..45e1cf7 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -83,6 +83,7 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
 			   struct netlink_callback *cb)
 {
 	int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
+	unsigned short act_flags = cb->args[2];
 	struct nlattr *nest;
 
 	spin_lock_bh(&hinfo->lock);
@@ -111,14 +112,18 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
 			}
 			nla_nest_end(skb, nest);
 			n_i++;
-			if (n_i >= TCA_ACT_MAX_PRIO)
+			if (!(act_flags & ACT_LARGE_DUMP_ON) &&
+			    n_i >= TCA_ACT_MAX_PRIO)
 				goto done;
 		}
 	}
 done:
 	spin_unlock_bh(&hinfo->lock);
-	if (n_i)
+	if (n_i) {
 		cb->args[0] += n_i;
+		if (act_flags & ACT_LARGE_DUMP_ON)
+			cb->args[1] = n_i;
+	}
 	return n_i;
 
 nla_put_failure:
@@ -993,11 +998,15 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
 	return tcf_add_notify(net, n, &actions, portid);
 }
 
+static const struct nla_policy tcaa_policy[TCAA_MAX + 1] = {
+	[TCAA_ACT_FLAGS]      = { .type = NLA_U32 },
+};
+
 static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
 			 struct netlink_ext_ack *extack)
 {
 	struct net *net = sock_net(skb->sk);
-	struct nlattr *tca[TCA_ACT_MAX + 1];
+	struct nlattr *tca[TCAA_MAX + 1];
 	u32 portid = skb ? NETLINK_CB(skb).portid : 0;
 	int ret = 0, ovr = 0;
 
@@ -1005,7 +1014,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
 	    !netlink_capable(skb, CAP_NET_ADMIN))
 		return -EPERM;
 
-	ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL,
+	ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCAA_MAX, tcaa_policy,
 			  extack);
 	if (ret < 0)
 		return ret;
@@ -1046,16 +1055,12 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
 	return ret;
 }
 
-static struct nlattr *find_dump_kind(const struct nlmsghdr *n)
+static struct nlattr *find_dump_kind(struct nlattr **nla)
 {
 	struct nlattr *tb1, *tb2[TCA_ACT_MAX + 1];
 	struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
-	struct nlattr *nla[TCAA_MAX + 1];
 	struct nlattr *kind;
 
-	if (nlmsg_parse(n, sizeof(struct tcamsg), nla, TCAA_MAX,
-			NULL, NULL) < 0)
-		return NULL;
 	tb1 = nla[TCA_ACT_TAB];
 	if (tb1 == NULL)
 		return NULL;
@@ -1081,9 +1086,18 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
 	struct nlattr *nest;
 	struct tc_action_ops *a_o;
 	int ret = 0;
+	struct nlattr *tcaa[TCAA_MAX + 1];
 	struct tcamsg *t = (struct tcamsg *) nlmsg_data(cb->nlh);
-	struct nlattr *kind = find_dump_kind(cb->nlh);
+	struct nlattr *kind = NULL;
+	u32 act_flags = 0;
+
+	ret = nlmsg_parse(cb->nlh, sizeof(struct tcamsg), tcaa, TCAA_MAX,
+			  tcaa_policy, NULL);
+	if (ret < 0)
+		return ret;
+
 
+	kind = find_dump_kind(tcaa);
 	if (kind == NULL) {
 		pr_info("tc_dump_action: action bad kind\n");
 		return 0;
@@ -1093,10 +1107,16 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
 	if (a_o == NULL)
 		return 0;
 
+	if (tcaa[TCAA_ACT_FLAGS])
+		act_flags = nla_get_u32(tcaa[TCAA_ACT_FLAGS]);
+
 	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
 			cb->nlh->nlmsg_type, sizeof(*t), 0);
 	if (!nlh)
 		goto out_module_put;
+
+	cb->args[2] = act_flags;
+
 	t = nlmsg_data(nlh);
 	t->tca_family = AF_UNSPEC;
 	t->tca__pad1 = 0;
@@ -1113,6 +1133,9 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
 	if (ret > 0) {
 		nla_nest_end(skb, nest);
 		ret = skb->len;
+		if (nla_put_u32(skb, TCAA_ACT_COUNT, cb->args[1]))
+			goto out_module_put;
+		cb->args[1] = 0;
 	} else
 		nlmsg_trim(skb, b);
 
-- 
1.9.1

^ permalink raw reply related

* Re: [PATCH net-next 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jamal Hadi Salim @ 2017-04-19  1:12 UTC (permalink / raw)
  To: davem; +Cc: netdev, eric.dumazet, jiri, xiyou.wangcong
In-Reply-To: <1492564082-28685-1-git-send-email-jhs@emojatatu.com>

Please ignore - I will resend. This is corrupt.
Sorry - I will label the next one as v2 even though it is v3.

cheers,
jamal

On 17-04-18 09:08 PM, Jamal Hadi Salim wrote:
> From: Jamal Hadi Salim <jhs@mojatatu.com>
>
> When you dump hundreds of thousands of actions, getting only 32 per
> dump batch even when the socket buffer and memory allocations allow
> is inefficient.
>
> With this change, the user will get as many as possibly fitting

^ permalink raw reply

* Re: net: heap out-of-bounds in fib6_clean_node/rt6_fill_node/fib6_age/fib6_prune_clone
From: Andrey Konovalov @ 2017-04-19  1:09 UTC (permalink / raw)
  To: David Ahern
  Cc: Dmitry Vyukov, Eric Dumazet, Mahesh Bandewar, Eric Dumazet,
	David Miller, Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
	Patrick McHardy, netdev, LKML, Cong Wang, syzkaller
In-Reply-To: <732d29dc-0240-cd1a-6973-5d14636ceaee@cumulusnetworks.com>

On Wed, Apr 19, 2017 at 1:20 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
> On 4/18/17 2:43 PM, Andrey Konovalov wrote:
>> I've finally managed to reproduce one of the crashes on commit
>> 4f7d029b9bf009fbee76bb10c0c4351a1870d2f3 (4.11-rc7).
>>
>> I'm not sure if this bug has the same root cause as the first one
>> reported in this thread, but it definitely has to do with ipv6
>> routing.
>>
>> C reproducer, syzkaller program and my .config are attached.

Just FYI, the reproducer uses interface number 9 inside a user
namespace, which is apparently ip6gre0.

1: lo: <LOOPBACK> mtu 65536 qdisc noop state DOWN mode DEFAULT qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: tunl0@NONE: <NOARP> mtu 1480 qdisc noop state DOWN mode DEFAULT qlen 1000
    link/ipip 0.0.0.0 brd 0.0.0.0
3: gre0@NONE: <NOARP> mtu 1476 qdisc noop state DOWN mode DEFAULT qlen 1000
    link/gre 0.0.0.0 brd 0.0.0.0
4: gretap0@NONE: <BROADCAST,MULTICAST> mtu 1462 qdisc noop state DOWN
mode DEFAULT qlen 1000
    link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
5: ip_vti0@NONE: <NOARP> mtu 1332 qdisc noop state DOWN mode DEFAULT qlen 1000
    link/ipip 0.0.0.0 brd 0.0.0.0
6: ip6_vti0@NONE: <NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT qlen 1000
    link/tunnel6 :: brd ::
7: sit0@NONE: <NOARP> mtu 1480 qdisc noop state DOWN mode DEFAULT qlen 1000
    link/sit 0.0.0.0 brd 0.0.0.0
8: ip6tnl0@NONE: <NOARP> mtu 1452 qdisc noop state DOWN mode DEFAULT qlen 1000
    link/tunnel6 :: brd ::
9: ip6gre0@NONE: <NOARP> mtu 1448 qdisc noop state DOWN mode DEFAULT qlen 1000
    link/[823] 00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00 brd
00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00

>>
>> Thanks!
>>
>> kasan: CONFIG_KASAN_INLINE enabled
>> kasan: GPF could be caused by NULL-ptr deref or user memory access
>> general protection fault: 0000 [#1] SMP KASAN
>> Modules linked in:
>> CPU: 1 PID: 4035 Comm: a.out Not tainted 4.11.0-rc7+ #250
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>> task: ffff880069809600 task.stack: ffff880062dc8000
>> RIP: 0010:ip6_rt_cache_alloc+0xa6/0x560 net/ipv6/route.c:975
>
> From a quick glance seems to be a different bug than Dmitry's.

It might be.

>

^ permalink raw reply

* [PATCH net-next 2/2] net sched actions: add time filter for action dumping
From: Jamal Hadi Salim @ 2017-04-19  1:08 UTC (permalink / raw)
  To: davem; +Cc: netdev, eric.dumazet, jiri, xiyou.wangcong, Jamal Hadi Salim
In-Reply-To: <1492564082-28685-1-git-send-email-jhs@emojatatu.com>

From: Jamal Hadi Salim <jhs@mojatatu.com>

This adds support for filtering based on time since last used.
When we are dumping a large number of actions it is useful to
have the option of filtering based on when the action was last
used to reduce the amount of data crossing to user space.

With this patch the user space app sets the TCAA_ACT_TIME_FILTER
attribute with the value in milliseconds with "time of interest
since now".  The kernel converts this to jiffies and does the
filtering comparison matching entries that have seen activity
since then and returns them to user space.
Old kernels and old tc continue to work in legacy mode since
they dont specify this attribute.

Some example (we have 400 actions bound to 400 filters); at installation
time using  hacked tc which sets the time of interest to 120 seconds:

prompt$ hackedtc actions ls action gact | grep index | wc -l
400

go get some coffee and  wait for > 120 seconds and try again:

prompt$ hackedtc actions ls action gact | grep index | wc -l
0

Lets see a filter bound to one of these actions:
..
filter pref 10 u32
filter pref 10 u32 fh 800: ht divisor 1
filter pref 10 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:10  (rule hit 2 success 1)
  match 7f000002/ffffffff at 12 (success 1 )
    action order 1: gact action pass
     random type none pass val 0
     index 23 ref 2 bind 1 installed 1145 sec used 802 sec
    Action statistics:
    Sent 84 bytes 1 pkt (dropped 0, overlimits 0 requeues 0)
    backlog 0b 0p requeues 0
....

that coffee took long, no?

Now lets ping -c 1 127.0.0.2, then run the actions again:

prompt$ hackedtc actions ls action gact | grep index | wc -l
1

More details please:

prompt$ hackedtc -s actions ls action gact
total acts 1 flags 0x3

    action order 0: gact action pass
     random type none pass val 0
     index 23 ref 2 bind 1 installed 1270 sec used 30 sec
    Action statistics:
    Sent 168 bytes 2 pkt (dropped 0, overlimits 0 requeues 0)
    backlog 0b 0p requeues 0

And the filter?

filter pref 10 u32
filter pref 10 u32 fh 800: ht divisor 1
filter pref 10 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:10  (rule hit 4 success 2)
  match 7f000002/ffffffff at 12 (success 2 )
    action order 1: gact action pass
     random type none pass val 0
     index 23 ref 2 bind 1 installed 1324 sec used 84 sec
    Action statistics:
    Sent 168 bytes 2 pkt (dropped 0, overlimits 0 requeues 0)
    backlog 0b 0p requeues 0

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 include/uapi/linux/rtnetlink.h |  1 +
 net/sched/act_api.c            | 23 +++++++++++++++++++++--
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index c7080ec..1b36cc0 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -680,6 +680,7 @@ enum {
 	TCAA_ACT_TAB,
 	TCAA_ACT_FLAGS,
 	TCAA_ACT_COUNT,
+	TCAA_ACT_TIME_FILTER,
 	__TCAA_MAX
 };
 
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 45e1cf7..cb5b23d 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -84,11 +84,12 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
 {
 	int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
 	unsigned short act_flags = cb->args[2];
+	unsigned long jiffy_filter = cb->args[3];
 	struct nlattr *nest;
 
 	spin_lock_bh(&hinfo->lock);
 
-	s_i = cb->args[0];
+	s_i = cb->args[4];
 
 	for (i = 0; i < (hinfo->hmask + 1); i++) {
 		struct hlist_head *head;
@@ -101,6 +102,11 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
 			if (index < s_i)
 				continue;
 
+			if (jiffy_filter &&
+			    time_after(jiffy_filter,
+				       (unsigned long)p->tcfa_tm.lastuse))
+				continue;
+
 			nest = nla_nest_start(skb, n_i);
 			if (nest == NULL)
 				goto nla_put_failure;
@@ -118,6 +124,9 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
 		}
 	}
 done:
+	if (index > 0)
+		cb->args[4] = index + 1;
+
 	spin_unlock_bh(&hinfo->lock);
 	if (n_i) {
 		cb->args[0] += n_i;
@@ -1090,13 +1099,14 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
 	struct tcamsg *t = (struct tcamsg *) nlmsg_data(cb->nlh);
 	struct nlattr *kind = NULL;
 	u32 act_flags = 0;
+	u32 msecs_filter = 0;
+	unsigned long jiffy_wanted = 0;
 
 	ret = nlmsg_parse(cb->nlh, sizeof(struct tcamsg), tcaa, TCAA_MAX,
 			  tcaa_policy, NULL);
 	if (ret < 0)
 		return ret;
 
-
 	kind = find_dump_kind(tcaa);
 	if (kind == NULL) {
 		pr_info("tc_dump_action: action bad kind\n");
@@ -1110,12 +1120,21 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
 	if (tcaa[TCAA_ACT_FLAGS])
 		act_flags = nla_get_u32(tcaa[TCAA_ACT_FLAGS]);
 
+	if (tcaa[TCAA_ACT_TIME_FILTER])
+		msecs_filter = nla_get_u32(tcaa[TCAA_ACT_TIME_FILTER]);
+
 	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
 			cb->nlh->nlmsg_type, sizeof(*t), 0);
 	if (!nlh)
 		goto out_module_put;
 
+	if (msecs_filter) {
+		unsigned long jiffy_msecs = msecs_to_jiffies(msecs_filter);
+		jiffy_wanted = jiffies - jiffy_msecs;
+	}
+
 	cb->args[2] = act_flags;
+	cb->args[3] = jiffy_wanted;
 
 	t = nlmsg_data(nlh);
 	t->tca_family = AF_UNSPEC;
-- 
1.9.1

^ permalink raw reply related

* [PATCH net-next 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jamal Hadi Salim @ 2017-04-19  1:08 UTC (permalink / raw)
  To: davem; +Cc: netdev, eric.dumazet, jiri, xiyou.wangcong, Jamal Hadi Salim

From: Jamal Hadi Salim <jhs@mojatatu.com>

When you dump hundreds of thousands of actions, getting only 32 per
dump batch even when the socket buffer and memory allocations allow
is inefficient.

With this change, the user will get as many as possibly fitting
within the given constraints available to the kernel.

A new top level TLV space is introduced. An attribute
TCAA_ACT_FLAGS is used to carry the flags indicating the user
is capable of processing these large dumps. Older user space which
doesnt set this flag doesnt get the large (than 32) batches.
The kernel uses the TCAA_ACT_COUNT attribute to tell the user how many
actions are put in a single batch. As such user space app knows how long
to iterate (independent of the type of action being dumped)
instead of hardcoded maximum of 32.

Some results dumping 1.5M actions, first unpatched tc which the
kernel doesnt help:

prompt$ time -p tc actions ls action gact | grep index | wc -l
1500000
real 1388.43
user 2.07
sys 1386.79

Now lets see a patched tc which sets the correct flags when requesting
a dump:

prompt$ time -p updatedtc actions ls action gact | grep index | wc -l
1500000
real 178.13
user 2.02
sys 176.96

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 MAINTAINERS                    |  1 +
 include/uapi/linux/rtnetlink.h | 21 +++++++++++++++++++--
 net/sched/act_api.c            | 43 ++++++++++++++++++++++++++++++++----------
 3 files changed, 53 insertions(+), 12 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7b4b828..94eadbc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12198,6 +12198,7 @@ F:	kernel/taskstats.c
 
 TC subsystem
 M:	Jamal Hadi Salim <jhs@mojatatu.com>
+M:	Jamal Hadi Salim <jhs@mojatatu.com>
 L:	netdev@vger.kernel.org
 S:	Maintained
 F:	include/net/pkt_cls.h
diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index cce0613..c7080ec 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -674,10 +674,27 @@ struct tcamsg {
 	unsigned char	tca__pad1;
 	unsigned short	tca__pad2;
 };
+
+enum {
+	TCAA_UNSPEC,
+	TCAA_ACT_TAB,
+	TCAA_ACT_FLAGS,
+	TCAA_ACT_COUNT,
+	__TCAA_MAX
+};
+
+#define TCAA_MAX (__TCAA_MAX - 1)
 #define TA_RTA(r)  ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg))))
 #define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
-#define TCA_ACT_TAB 1 /* attr type must be >=1 */	
-#define TCAA_MAX 1
+#define TCA_ACT_TAB TCAA_ACT_TAB
+/* tcamsg flags stored in attribute TCAA_ACT_FLAGS
+ *
+ * ACT_LARGE_DUMP_ON user->kernel to request for larger than TCA_ACT_MAX_PRIO
+ * actions in a dump. All dump responses will contain the number of actions
+ * being dumped stored in for user app's consumption in TCAA_ACT_COUNT
+ *
+ */
+#define ACT_LARGE_DUMP_ON		(1 << 0)
 
 /* New extended info filters for IFLA_EXT_MASK */
 #define RTEXT_FILTER_VF		(1 << 0)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 82b1d48..45e1cf7 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -83,6 +83,7 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
 			   struct netlink_callback *cb)
 {
 	int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
+	unsigned short act_flags = cb->args[2];
 	struct nlattr *nest;
 
 	spin_lock_bh(&hinfo->lock);
@@ -111,14 +112,18 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
 			}
 			nla_nest_end(skb, nest);
 			n_i++;
-			if (n_i >= TCA_ACT_MAX_PRIO)
+			if (!(act_flags & ACT_LARGE_DUMP_ON) &&
+			    n_i >= TCA_ACT_MAX_PRIO)
 				goto done;
 		}
 	}
 done:
 	spin_unlock_bh(&hinfo->lock);
-	if (n_i)
+	if (n_i) {
 		cb->args[0] += n_i;
+		if (act_flags & ACT_LARGE_DUMP_ON)
+			cb->args[1] = n_i;
+	}
 	return n_i;
 
 nla_put_failure:
@@ -993,11 +998,15 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
 	return tcf_add_notify(net, n, &actions, portid);
 }
 
+static const struct nla_policy tcaa_policy[TCAA_MAX + 1] = {
+	[TCAA_ACT_FLAGS]      = { .type = NLA_U32 },
+};
+
 static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
 			 struct netlink_ext_ack *extack)
 {
 	struct net *net = sock_net(skb->sk);
-	struct nlattr *tca[TCA_ACT_MAX + 1];
+	struct nlattr *tca[TCAA_MAX + 1];
 	u32 portid = skb ? NETLINK_CB(skb).portid : 0;
 	int ret = 0, ovr = 0;
 
@@ -1005,7 +1014,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
 	    !netlink_capable(skb, CAP_NET_ADMIN))
 		return -EPERM;
 
-	ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL,
+	ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCAA_MAX, tcaa_policy,
 			  extack);
 	if (ret < 0)
 		return ret;
@@ -1046,16 +1055,12 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
 	return ret;
 }
 
-static struct nlattr *find_dump_kind(const struct nlmsghdr *n)
+static struct nlattr *find_dump_kind(struct nlattr **nla)
 {
 	struct nlattr *tb1, *tb2[TCA_ACT_MAX + 1];
 	struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
-	struct nlattr *nla[TCAA_MAX + 1];
 	struct nlattr *kind;
 
-	if (nlmsg_parse(n, sizeof(struct tcamsg), nla, TCAA_MAX,
-			NULL, NULL) < 0)
-		return NULL;
 	tb1 = nla[TCA_ACT_TAB];
 	if (tb1 == NULL)
 		return NULL;
@@ -1081,9 +1086,18 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
 	struct nlattr *nest;
 	struct tc_action_ops *a_o;
 	int ret = 0;
+	struct nlattr *tcaa[TCAA_MAX + 1];
 	struct tcamsg *t = (struct tcamsg *) nlmsg_data(cb->nlh);
-	struct nlattr *kind = find_dump_kind(cb->nlh);
+	struct nlattr *kind = NULL;
+	u32 act_flags = 0;
+
+	ret = nlmsg_parse(cb->nlh, sizeof(struct tcamsg), tcaa, TCAA_MAX,
+			  tcaa_policy, NULL);
+	if (ret < 0)
+		return ret;
+
 
+	kind = find_dump_kind(tcaa);
 	if (kind == NULL) {
 		pr_info("tc_dump_action: action bad kind\n");
 		return 0;
@@ -1093,10 +1107,16 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
 	if (a_o == NULL)
 		return 0;
 
+	if (tcaa[TCAA_ACT_FLAGS])
+		act_flags = nla_get_u32(tcaa[TCAA_ACT_FLAGS]);
+
 	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
 			cb->nlh->nlmsg_type, sizeof(*t), 0);
 	if (!nlh)
 		goto out_module_put;
+
+	cb->args[2] = act_flags;
+
 	t = nlmsg_data(nlh);
 	t->tca_family = AF_UNSPEC;
 	t->tca__pad1 = 0;
@@ -1113,6 +1133,9 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
 	if (ret > 0) {
 		nla_nest_end(skb, nest);
 		ret = skb->len;
+		if (nla_put_u32(skb, TCAA_ACT_COUNT, cb->args[1]))
+			goto out_module_put;
+		cb->args[1] = 0;
 	} else
 		nlmsg_trim(skb, b);
 
-- 
1.9.1

^ permalink raw reply related

* Re: question about size of sk_buff and skb_shared_info
From: Eric Dumazet @ 2017-04-19  0:47 UTC (permalink / raw)
  To: Code Soldier1; +Cc: netdev
In-Reply-To: <CABGNeczXNPkWnhkviHhXv5OpVope_0BGNoB-nsGwALOptrwWdA@mail.gmail.com>


For the last time, do not top post on netdev.

On Tue, 2017-04-18 at 17:15 -0700, Code Soldier1 wrote:
> I am not suggesting that we do not do cache alignment. That is
> required for performance. I have gone through this exercise because I
> need to add a field to sk_buff and I want to do that without causing
> any adverse effects.
> 
> Now that we have discovered that there are 40 bytes that can be used
> without any adverse effect, may I increase skb->cb by 8 bytes ?
> 

skb->cb is already 48 bytes, not 40.

> If not then may I increase skb_shared_info -- However that would have
> to be by 64bytes.


You will have a very hard time to convince us that this 8 byte field is
needed on all skbs, regardless of current sk_buff size.



> 
> 
> On Tue, Apr 18, 2017 at 4:55 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > Please do not top post on netdev
> >
> > On Tue, 2017-04-18 at 16:26 -0700, Code Soldier1 wrote:
> >> Eric,
> >>
> >> This alignment flag is passed to the cache constructor and the
> >> allocation is indeed cache aligned. However, since the allocated size
> >> is not a multiple of the alignment, wont memory be wasted ?. We can
> >> get 40 extra bytes without any side effects since they are on the same
> >> cache line ?
> >
> > We _want_ to align skb to cache lines.
> >
> > Fact that few bytes might be wasted is unfortunate, but negligible.
> >
> > BTW, if you do kmalloc(1025), kmalloc() wastes 1023 bytes.
> >
> >
> >>
> >> kmem_cache_create() code does an ALIGN() to round up the size.
> >>
> >>         kasan_cache_create(cachep, &size, &flags);
> >>
> >>         size = ALIGN(size, cachep->align);
> >>
> >> This is the size used in calculate_slab_order() to calculate num. I am
> >> assuming that in the non debug case gfp_order will be 0.
> >>
> >> Perhaps I am missing something.
> >>
> >>
> >> On Tue, Apr 18, 2017 at 12:00 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >> > On Tue, 2017-04-18 at 10:34 -0700, Code Soldier1 wrote:
> >> >> Hi Folks,
> >> >>
> >> >> I am sure there is a reason for the current sizes of these structures,
> >> >> However the reason is not obvious to me. So please help me understand.
> >> >>
> >> >> Currently the size of sk_buff on an x86_64 system is 232 bytes -- Why
> >> >> is that. I expected it to be a multiple of 32/64 as they are the most
> >> >> common cache lines. Since the alignment calculation will align the
> >> >> structure with the hw cache line, it seems like we might be wasting
> >> >> space ?
> >> >>
> >> >> skb_shared_info on the other hand is perfectly aligned with a size of 320 bytes.
> >> >>
> >> >> Thanks,
> >> >>
> >> >
> >> > The alignment is there.
> >> > Look at skb_init() code, using SLAB_HWCACHE_ALIGN
> >> >
> >> >
> >> >
> >> >
> >> >
> >>
> >>
> >>
> >
> >
> 
> 
> 

^ permalink raw reply

* [PATCH net-next]smsc911x: Adding support for Micochip LAN9250 Ethernet controller
From: David.Cai @ 2017-04-19  0:29 UTC (permalink / raw)
  To: netdev, davem; +Cc: UNGLinuxDriver, steve.glendinning, Ravi.Hegde, Woojung.Huh

Attached is a patch which adding support for Microchip LAN9250 Ethernet controller.

Thanks,

=====================
David Cai
Principle Software Engineer
Microchip Technology Inc.
=====================

Signed-off-by: David Cai <david.cai@microchip.com>
---
drivers/net/ethernet/smsc/smsc911x.c | 32 +++++++++++++++++++++++++++-----
drivers/net/ethernet/smsc/smsc911x.h |  3 +++
2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index fa5ca09..22b1951 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -25,7 +25,7 @@
  *   LAN9215, LAN9216, LAN9217, LAN9218
  *   LAN9210, LAN9211
  *   LAN9220, LAN9221
- *   LAN89218
+ *   LAN89218,LAN9250
  *
  */

@@ -104,6 +104,9 @@ struct smsc911x_data {
               /* used to decide which workarounds apply */
               unsigned int generation;

+             /* used to decide which sub generation product work arounds to apply */
+             unsigned int sub_generation;
+
               /* device configuration (copied from platform_data during probe) */
               struct smsc911x_platform_config config;

@@ -1450,6 +1453,8 @@ static int smsc911x_soft_reset(struct smsc911x_data *pdata)
               unsigned int timeout;
               unsigned int temp;
               int ret;
+             unsigned int reset_offset = HW_CFG;
+             unsigned int reset_mask = HW_CFG_SRST_;

                /*
                * Make sure to power-up the PHY chip before doing a reset, otherwise
@@ -1476,15 +1481,23 @@ static int smsc911x_soft_reset(struct smsc911x_data *pdata)
                               }
               }

+             if (pdata->sub_generation) {
+                             /* special reset for  LAN9250 */
+                             reset_offset = RESET_CTL;
+                             reset_mask = RESET_CTL_DIGITAL_RST_;
+             }
+
               /* Reset the LAN911x */
-              smsc911x_reg_write(pdata, HW_CFG, HW_CFG_SRST_);
+             smsc911x_reg_write(pdata, reset_offset, reset_mask);
+
+             /* verify reset bit is cleared */
               timeout = 10;
               do {
                               udelay(10);
-                              temp = smsc911x_reg_read(pdata, HW_CFG);
-              } while ((--timeout) && (temp & HW_CFG_SRST_));
+                             temp = smsc911x_reg_read(pdata, reset_offset);
+             } while ((--timeout) && (temp & reset_mask));

-              if (unlikely(temp & HW_CFG_SRST_)) {
+             if (unlikely(temp & reset_mask)) {
                               SMSC_WARN(pdata, drv, "Failed to complete reset");
                               return -EIO;
               }
@@ -2251,6 +2264,9 @@ static int smsc911x_init(struct net_device *dev)
               /* Default generation to zero (all workarounds apply) */
               pdata->generation = 0;

+             /* Default sub_generation to zero */
+             pdata->sub_generation = 0;
+
               pdata->idrev = smsc911x_reg_read(pdata, ID_REV);
               switch (pdata->idrev & 0xFFFF0000) {
               case 0x01180000:
@@ -2278,6 +2294,12 @@ static int smsc911x_init(struct net_device *dev)
                               pdata->generation = 4;
                               break;

+             case 0x92500000:
+                             /* LAN9250 */
+                             pdata->generation = 4;
+                             pdata->sub_generation = 1;
+                             break;
+
               default:
                               SMSC_WARN(pdata, probe, "LAN911x not identified, idrev: 0x%08X",
                                                 pdata->idrev);
diff --git a/drivers/net/ethernet/smsc/smsc911x.h b/drivers/net/ethernet/smsc/smsc911x.h
index 54d6489..6b2d479 100644
--- a/drivers/net/ethernet/smsc/smsc911x.h
+++ b/drivers/net/ethernet/smsc/smsc911x.h
@@ -303,6 +303,9 @@
#define E2P_DATA_EEPROM_DATA_                    0x000000FF
#define LAN_REGISTER_EXTENT                              0x00000100

+#define RESET_CTL                                       0x1F8
+#define RESET_CTL_DIGITAL_RST_                       0x00000001
+
/*
  * MAC Control and Status Register (Indirect Address)
  * Offset (through the MAC_CSR CMD and DATA port)
-- 
2.7.4

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox