* Re: [PATCH bpf 0/2] nfp: bpf: fix two ALU32 code-gen bugs
From: Daniel Borkmann @ 2019-02-22 23:09 UTC (permalink / raw)
To: Jiong Wang, alexei.starovoitov; +Cc: netdev, oss-drivers
In-Reply-To: <1550874964-16769-1-git-send-email-jiong.wang@netronome.com>
On 02/22/2019 11:36 PM, Jiong Wang wrote:
> code-gen for BPF_ALU | BPF_XOR | BPF_K is wrong when imm is -1, also high
> 32-bit of 64-bit register should always be cleared.
>
> This set fixed both bugs.
>
> Jiong Wang (2):
> nfp: bpf: fix code-gen bug on BPF_ALU | BPF_XOR | BPF_K
> nfp: bpf: fix ALU32 high bits clearance bug
>
> drivers/net/ethernet/netronome/nfp/bpf/jit.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
From: Jann Horn @ 2019-02-22 23:11 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Linus Torvalds, David Miller, Masami Hiramatsu, Steven Rostedt,
Andy Lutomirski, Linux List Kernel Mailing, Ingo Molnar,
Andrew Morton, stable, Changbin Du, Kees Cook, Andrew Lutomirski,
Daniel Borkmann, Netdev, bpf
In-Reply-To: <20190222225103.o5rr5zr4fq77jdg4@ast-mbp.dhcp.thefacebook.com>
On Fri, Feb 22, 2019 at 11:51 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Feb 22, 2019 at 01:59:10PM -0800, Linus Torvalds wrote:
> > On Fri, Feb 22, 2019 at 1:38 PM David Miller <davem@davemloft.net> wrote:
> > >
> > > Don't be surprised if we see more separation like this in the future too.
> >
> > Yes, with the whole meltdown fiasco, there's actually more pressure to
> > add more support for separation of kernel/user address spaces. As Andy
> > pointed out, it's been discussed as a future wish-list for x86-64 too.
> >
> > But yeah, right now the *common* architectures all distinguish kernel
> > and user space by pointers (ie x86-64, arm64 and powerpc).
>
> That's all fine. I'm missing rationale for making probe_kernel_read()
> fail on user addresses.
> What is fundamentally wrong with a function probe_any_address_read() ?
I think what Linus is saying is: There are some scenarios (like a
system with the old 4G/4G X86 patch) where *the same* address can
refer to two different pieces of memory, depending on whether you
interpret it as a kernel pointer or a user pointer. So for example, if
your BPF program tries to read tsk->comm, it works, but if the BPF
program then tries to read from PT_REGS_PARM2(ctx), it's going to
actually interpret the userspace address as a kernel address and read
completely different memory.
On top of that, from the security angle, this means that if a user
passes a kernel pointer into a syscall, they can trick a tracing BPF
program into looking at random kernel memory instead of the user's
memory. That may or may not be problematic, depending on what you do
afterwards with the data you've read. (For example, if this is a
service that collects performance data and then saves it to some
world-readable location on disk because the data it is collecting
(including comm strings) isn't supposed to be sensitive, you might
have a problem.)
> For context, typical bpf kprobe program looks like this:
> #define probe_read(P) \
> ({typeof(P) val = 0; bpf_probe_read(&val, sizeof(val), &P); val;})
> SEC("kprobe/__set_task_comm")
> int bpf_prog(struct pt_regs *ctx)
> {
> struct signal_struct *signal;
> struct task_struct *tsk;
> char oldcomm[16] = {};
> char newcomm[16] = {};
> u16 oom_score_adj;
> u32 pid;
>
> tsk = (void *)PT_REGS_PARM1(ctx);
> pid = probe_read(tsk->pid);
> bpf_probe_read(oldcomm, sizeof(oldcomm), &tsk->comm);
> bpf_probe_read(newcomm, sizeof(newcomm), (void *)PT_REGS_PARM2(ctx));
> signal = probe_read(tsk->signal);
> oom_score_adj = probe_read(signal->oom_score_adj);
> ...
> }
>
> where PT_REGS_PARMx macros are defined per architecture.
> On x86 it's #define PT_REGS_PARM1(x) ((x)->di)
>
> The program writer has to know the meaning of function arguments.
> In this example they need to know that __set_task_comm is defined as
> void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec) in the kernel.
>
> Right now these programs just call bpf_probe_read() on whatever data
> they need to access and not differentiating whether it's user or kernel.
>
> One idea we discussed is to split bpf_probe_read() into kernel_read and user_read
> helpers, but in the BPF verifier we cannot determine which address space
> the program wants to access. The prog writer needs to manually analyze the program
> to use correct one. But mistakes are possible and cannot be fatal.
> On the kernel side we have to be safe.
> Both probe_kernel_read and probe_user_read must not panic if a pointer
> from wrong address space was passed.
>
> Hence my preference is to keep probe_kernel_read as "try read any address".
> The function can be renamed to indicate so.
>
^ permalink raw reply
* Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
From: David Miller @ 2019-02-22 23:16 UTC (permalink / raw)
To: jannh
Cc: alexei.starovoitov, torvalds, mhiramat, rostedt, luto,
linux-kernel, mingo, akpm, stable, changbin.du, keescook, luto,
daniel, netdev, bpf
In-Reply-To: <CAG48ez0nMEMP5-eSFWmLKs30zApgZ57zqU1Od9cROBYQvoJxsg@mail.gmail.com>
From: Jann Horn <jannh@google.com>
Date: Sat, 23 Feb 2019 00:11:58 +0100
> I think what Linus is saying is: There are some scenarios (like a
> system with the old 4G/4G X86 patch) where *the same* address can
> refer to two different pieces of memory, depending on whether you
> interpret it as a kernel pointer or a user pointer.
Exactly.
On sparc64 the kernel is mapped exactly at the same virtual addresses
as userspace processes usually are mapped, even 32-bit ones. The
difference is the MMU context only.
^ permalink raw reply
* Re: [PATCH net 0/2] bnxt_en: firmware message delay fixes.
From: David Miller @ 2019-02-22 23:21 UTC (permalink / raw)
To: michael.chan; +Cc: netdev
In-Reply-To: <1550707652-14747-1-git-send-email-michael.chan@broadcom.com>
From: Michael Chan <michael.chan@broadcom.com>
Date: Wed, 20 Feb 2019 19:07:30 -0500
> We were seeing some intermittent firmware message timeouts in our lab and
> these 2 small patches fix them. Please apply to stable as well. Thanks.
Applied and queued up, thanks!
^ permalink raw reply
* Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
From: Linus Torvalds @ 2019-02-22 23:16 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David Miller, Masami Hiramatsu, Steven Rostedt, Andy Lutomirski,
Linux List Kernel Mailing, Ingo Molnar, Andrew Morton, stable,
Changbin Du, Jann Horn, Kees Cook, Andrew Lutomirski,
Daniel Borkmann, Netdev, bpf
In-Reply-To: <20190222225103.o5rr5zr4fq77jdg4@ast-mbp.dhcp.thefacebook.com>
On Fri, Feb 22, 2019 at 2:51 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> That's all fine. I'm missing rationale for making probe_kernel_read()
> fail on user addresses.
Because it already WON'T WORK in general!
> What is fundamentally wrong with a function probe_any_address_read() ?
What part of "the same pointer value can be a user address and a
kernel address" is not getting through?
The user address space and the kernel address space have separate page
tables on some architectures. We used to avoid it on x86, because
switching address spaces was expensive, but even on x86 some vendors
did it on 32-bit simply to get 4GB of user (and kernel) address space.
And now we end up doing it anyway just because of meltdown.
So a kernel pointer value of 0x12345678 could be a value kernel
pointer pointing to some random kmalloc'ed kernel memory, and a user
pointer value of 0x12345678 could be a valid _user_ pointer pointing
to some user mapping.
See?
If you access a user pointer, you need to use a user accessor function
(eg "get_user()"), while if you access a kernel pointer you need to
just dereference it directly (unless you can't trust it, in which case
you need to use a _different_ accessor function).
The fact that user and kernel pointers happen to be distinct on x86-64
(right now) is just a random implementation detail.
Really.
I didn't realize how many people seem to have been confused about
this. But it's always been true. It's just that the common
architectures have had that "one single address space for both kernel
and user pointers" in practice.
In fact, the *very* first kernel version had separate address spaces
for kernel and user mode even on x86 (using segments, not paging). So
it has literally been true since day one in Linux that a kernel
address can be indistinguishable from a user address from a pure value
standpoint.
Linus
^ permalink raw reply
* Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
From: Nadav Amit @ 2019-02-22 23:22 UTC (permalink / raw)
To: Jann Horn
Cc: Andy Lutomirski, Alexei Starovoitov, Steven Rostedt,
Linus Torvalds, Masami Hiramatsu, Linux List Kernel Mailing,
Ingo Molnar, Andrew Morton, Changbin Du, Kees Cook,
Andy Lutomirski, Daniel Borkmann, Network Development,
bpf@vger.kernel.org, Rick Edgecombe, Dave Hansen,
Peter Zijlstra (Intel), Igor Stoppa
In-Reply-To: <CAG48ez1Dm04kg2x3O=OOmBFFBSDr+RmnhBYChcwYv5zLbPGudQ@mail.gmail.com>
> On Feb 22, 2019, at 3:02 PM, Jann Horn <jannh@google.com> wrote:
>
> On Fri, Feb 22, 2019 at 11:39 PM Nadav Amit <namit@vmware.com> wrote:
>>> On Feb 22, 2019, at 2:21 PM, Nadav Amit <namit@vmware.com> wrote:
>>>
>>>> On Feb 22, 2019, at 2:17 PM, Jann Horn <jannh@google.com> wrote:
>>>>
>>>> On Fri, Feb 22, 2019 at 11:08 PM Nadav Amit <namit@vmware.com> wrote:
>>>>>> On Feb 22, 2019, at 1:43 PM, Jann Horn <jannh@google.com> wrote:
>>>>>>
>>>>>> (adding some people from the text_poke series to the thread, removing stable@)
>>>>>>
>>>>>> On Fri, Feb 22, 2019 at 8:55 PM Andy Lutomirski <luto@amacapital.net> wrote:
>>>>>>>> On Feb 22, 2019, at 11:34 AM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>>>>>>>>> On Fri, Feb 22, 2019 at 02:30:26PM -0500, Steven Rostedt wrote:
>>>>>>>>> On Fri, 22 Feb 2019 11:27:05 -0800
>>>>>>>>> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>>>>>>>>>
>>>>>>>>>>> On Fri, Feb 22, 2019 at 09:43:14AM -0800, Linus Torvalds wrote:
>>>>>>>>>>>
>>>>>>>>>>> Then we should still probably fix up "__probe_kernel_read()" to not
>>>>>>>>>>> allow user accesses. The easiest way to do that is actually likely to
>>>>>>>>>>> use the "unsafe_get_user()" functions *without* doing a
>>>>>>>>>>> uaccess_begin(), which will mean that modern CPU's will simply fault
>>>>>>>>>>> on a kernel access to user space.
>>>>>>>>>>
>>>>>>>>>> On bpf side the bpf_probe_read() helper just calls probe_kernel_read()
>>>>>>>>>> and users pass both user and kernel addresses into it and expect
>>>>>>>>>> that the helper will actually try to read from that address.
>>>>>>>>>>
>>>>>>>>>> If __probe_kernel_read will suddenly start failing on all user addresses
>>>>>>>>>> it will break the expectations.
>>>>>>>>>> How do we solve it in bpf_probe_read?
>>>>>>>>>> Call probe_kernel_read and if that fails call unsafe_get_user byte-by-byte
>>>>>>>>>> in the loop?
>>>>>>>>>> That's doable, but people already complain that bpf_probe_read() is slow
>>>>>>>>>> and shows up in their perf report.
>>>>>>>>>
>>>>>>>>> We're changing kprobes to add a specific flag to say that we want to
>>>>>>>>> differentiate between kernel or user reads. Can this be done with
>>>>>>>>> bpf_probe_read()? If it's showing up in perf report, I doubt a single
>>>>>>>>
>>>>>>>> so you're saying you will break existing kprobe scripts?
>>>>>>>> I don't think it's a good idea.
>>>>>>>> It's not acceptable to break bpf_probe_read uapi.
>>>>>>>
>>>>>>> If so, the uapi is wrong: a long-sized number does not reliably identify an address if you don’t separately know whether it’s a user or kernel address. s390x and 4G:4G x86_32 are the notable exceptions. I have lobbied for RISC-V and future x86_64 to join the crowd. I don’t know whether I’ll win this fight, but the uapi will probably have to change for at least s390x.
>>>>>>>
>>>>>>> What to do about existing scripts is a different question.
>>>>>>
>>>>>> This lack of logical separation between user and kernel addresses
>>>>>> might interact interestingly with the text_poke series, specifically
>>>>>> "[PATCH v3 05/20] x86/alternative: Initialize temporary mm for
>>>>>> patching" (https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20190221234451.17632-6-rick.p.edgecombe%40intel.com%2F&data=02%7C01%7Cnamit%40vmware.com%7Cd03df2db76624da8eb2008d69919e41a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636864733821233906&sdata=ky5iTrsCceoPwVW5N9FB4sDspwGEQ8MTlRE4b1Bqn54%3D&reserved=0)
>>>>>> and "[PATCH v3 06/20] x86/alternative: Use temporary mm for text
>>>>>> poking" (https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20190221234451.17632-7-rick.p.edgecombe%40intel.com%2F&data=02%7C01%7Cnamit%40vmware.com%7Cd03df2db76624da8eb2008d69919e41a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636864733821233906&sdata=EJs8doLrfFfMTKiVHmWjmpnpWolmuuZ5pxOmEMcI0ew%3D&reserved=0),
>>>>>> right? If someone manages to get a tracing BPF program to trigger in a
>>>>>> task that has switched to the patching mm, could they use
>>>>>> bpf_probe_write_user() - which uses probe_kernel_write() after
>>>>>> checking that KERNEL_DS isn't active and that access_ok() passes - to
>>>>>> overwrite kernel text that is mapped writable in the patching mm?
>>>>>
>>>>> Yes, this is a good point. I guess text_poke() should be defined with
>>>>> “__kprobes” and open-code memcpy.
>>>>>
>>>>> Does it sound reasonable?
>>>>
>>>> Doesn't __text_poke() as implemented in the proposed patch use a
>>>> couple other kernel functions, too? Like switch_mm_irqs_off() and
>>>> pte_clear() (which can be a call into a separate function on paravirt
>>>> kernels)?
>>>
>>> I will move the pte_clear() to be done after the poking mm was unloaded.
>>> Give me a few minutes to send a sketch of what I think should be done.
>>
>> Err.. You are right, I don’t see an easy way of preventing a kprobe from
>> being set on switch_mm_irqs_off(), and open-coding this monster is too ugly.
>>
>> The reasonable solution seems to me as taking all the relevant pieces of
>> code (and data) that might be used during text-poking and encapsulating them, so they
>> will be set in a memory area which cannot be kprobe'd. This can also be
>> useful to write-protect data structures of code that calls text_poke(),
>> e.g., static-keys. It can also protect data on that stack that is used
>> during text_poke() from being overwritten from another core.
>>
>> This solution is somewhat similar to Igor Stoppa’s idea of using “enclaves”
>> when doing write-rarely operations.
>>
>> Right now, I think that text_poke() will keep being susceptible to such
>> an attack, unless you have a better suggestion.
>
> A relatively simple approach might be to teach BPF not to run kprobe
> programs and such in contexts where current->mm isn't the active mm?
> Maybe using nmi_uaccess_okay(), or something like that? It looks like
> perf_callchain_user() also already uses that. Except that a lot of
> this code is x86-specific…
I keep having in mind how to reduce the TCB that is used while text_poke()
is running, but for the specific issue here, I think your approach would
be fine, and trace_call_bpf() can be modified to do what you ask for.
But, I am not sure that relying on current->mm gets us any more security,
relatively to having another unrelated explicit kprobe-disable indication,
which is cleaner from design point-of-view.
I can see how we get “some more security” if our decision whether kprobes
should be enabled was purely based on some hardware register (e.g., CR3) and
we could unequivocally realize whether kprobes eBPF should be on/off without
memory accesses (e.g., PCID bit set). Yet, I am not sure it worth it.
What do you say?
^ permalink raw reply
* Re: [PATCH] Set rtm_table to RT_TABLE_COMPAT for ipv6 for tables > 255
From: David Miller @ 2019-02-22 23:22 UTC (permalink / raw)
To: kalash; +Cc: netdev, linux-kernel
In-Reply-To: <20190221002304.GA24732@us178.sjc.aristanetworks.com>
From: Kalash Nainwal <kalash@arista.com>
Date: Wed, 20 Feb 2019 16:23:04 -0800
> Set rtm_table to RT_TABLE_COMPAT for ipv6 for tables > 255 to
> keep legacy software happy. This is similar to what was done for
> ipv4 in commit 709772e6e065 ("net: Fix routing tables with
> id > 255 for legacy software").
>
> Signed-off-by: Kalash Nainwal <kalash@arista.com>
Applied and queued up for -stable.
^ permalink raw reply
* Re: [tipc-discussion][net v2 1/1] tipc: fix race condition causing hung sendto
From: David Miller @ 2019-02-22 23:24 UTC (permalink / raw)
To: tung.q.nguyen; +Cc: netdev, tipc-discussion
In-Reply-To: <20190221033121.4220-1-tung.q.nguyen@dektech.com.au>
From: Tung Nguyen <tung.q.nguyen@dektech.com.au>
Date: Thu, 21 Feb 2019 10:31:21 +0700
> When sending multicast messages via blocking socket,
This patch does not apply cleanly to net, please respin:
[davem@localhost net]$ git am --signoff tipc-discussion-net-v2-1-1-tipc-fix-race-condition-causing-hung-sendto.patch
Applying: tipc: fix race condition causing hung sendto
error: patch failed: net/tipc/socket.c:379
error: net/tipc/socket.c: patch does not apply
Patch failed at 0001 tipc: fix race condition causing hung sendto
hint: Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
^ permalink raw reply
* Re: [Patch net-next] net_sched: initialize net pointer inside tcf_exts_init()
From: David Miller @ 2019-02-22 23:27 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev, jhs, jiri
In-Reply-To: <20190221053742.30907-1-xiyou.wangcong@gmail.com>
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Wed, 20 Feb 2019 21:37:42 -0800
> For tcindex filter, it is too late to initialize the
> net pointer in tcf_exts_validate(), as tcf_exts_get_net()
> requires a non-NULL net pointer. We can just move its
> initialization into tcf_exts_init(), which just requires
> an additional parameter.
>
> This makes the code in tcindex_alloc_perfect_hash()
> prettier.
>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Applied, thanks Cong.
^ permalink raw reply
* Re: [PATCH] Set rtm_table to RT_TABLE_COMPAT for ipv6 for tables > 255
From: Kalash Nainwal @ 2019-02-22 23:28 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-kernel
In-Reply-To: <20190222.152226.1935918245873046733.davem@davemloft.net>
On Fri, Feb 22, 2019 at 3:22 PM David Miller <davem@davemloft.net> wrote:
>
> From: Kalash Nainwal <kalash@arista.com>
> Date: Wed, 20 Feb 2019 16:23:04 -0800
>
> > Set rtm_table to RT_TABLE_COMPAT for ipv6 for tables > 255 to
> > keep legacy software happy. This is similar to what was done for
> > ipv4 in commit 709772e6e065 ("net: Fix routing tables with
> > id > 255 for legacy software").
> >
> > Signed-off-by: Kalash Nainwal <kalash@arista.com>
>
> Applied and queued up for -stable.
Thanks Dave.
^ permalink raw reply
* Re: [PATCH net-next v4 0/2] net: phy: at803x: Update delays for RGMII modes
From: David Miller @ 2019-02-22 23:32 UTC (permalink / raw)
To: vkoul
Cc: linux-arm-msm, bjorn.andersson, netdev, niklas.cassel, andrew,
f.fainelli, nsekhar, peter.ujfalusi, marc.w.gonzalez
In-Reply-To: <20190221102315.12946-1-vkoul@kernel.org>
From: Vinod Koul <vkoul@kernel.org>
Date: Thu, 21 Feb 2019 15:53:13 +0530
> Peter[1] reported that patch cd28d1d6e52e: ("net: phy: at803x: Disable
> phy delay for RGMII mode") caused regression on am335x-evmsk board.
> This board expects the Phy delay to be enabled but specified RGMII mode
> which refers to delays being disabled. So fix this by disabling delay only
> for RGMII mode and enable for RGMII_ID and RGMII_TXID/RXID modes.
>
> While at it, as pointed by Dave, don't inline the helpers.
>
> [1]: https://www.spinics.net/lists/netdev/msg550749.html
>
> Changes in v4:
> - fix log & comments nbased on Marc's feedback
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH] isdn_common: Mark expected switch fall-throughs
From: Gustavo A. R. Silva @ 2019-02-22 23:34 UTC (permalink / raw)
To: David Miller; +Cc: isdn, netdev, linux-kernel, keescook
In-Reply-To: <20190222.115023.1085574898692218507.davem@davemloft.net>
On 2/22/19 1:50 PM, David Miller wrote:
>>
>> This patch is part of the ongoing efforts to enable
>> -Wimplicit-fallthrough.
>>
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>
> Applied.
>
Thanks, Dave.
I wonder if you can take this one too:
https://lore.kernel.org/lkml/20190129180612.GA28650@embeddedor/
--
Gustavo
^ permalink raw reply
* Re: [PATCH] mdio_bus: Fix use-after-free on device_register fails
From: David Miller @ 2019-02-22 23:35 UTC (permalink / raw)
To: yuehaibing; +Cc: andrew, f.fainelli, hkallweit1, linux-kernel, netdev
In-Reply-To: <20190221144201.11488-1-yuehaibing@huawei.com>
From: Yue Haibing <yuehaibing@huawei.com>
Date: Thu, 21 Feb 2019 22:42:01 +0800
> From: YueHaibing <yuehaibing@huawei.com>
>
> KASAN has found use-after-free in fixed_mdio_bus_init,
> commit 0c692d07842a ("drivers/net/phy/mdio_bus.c: call
> put_device on device_register() failure") call put_device()
> while device_register() fails,give up the last reference
> to the device and allow mdiobus_release to be executed
> ,kfreeing the bus. However in most drives, mdiobus_free
> be called to free the bus while mdiobus_register fails.
> use-after-free occurs when access bus again, this patch
> revert it to let mdiobus_free free the bus.
>
> KASAN report details as below:
...
> Fixes: 0c692d07842a ("drivers/net/phy/mdio_bus.c: call put_device on device_register() failure")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* [PATCH bpf-next 0/4] bpf: per program stats
From: Alexei Starovoitov @ 2019-02-22 23:36 UTC (permalink / raw)
To: davem; +Cc: daniel, netdev, bpf, kernel-team
Introduce per program stats to monitor the usage BPF
Alexei Starovoitov (4):
bpf: enable program stats
bpf: expose program stats via bpf_prog_info
tools/bpf: sync bpf.h into tools
tools/bpftool: recognize bpf_prog_info runtime and runcnt
include/linux/bpf.h | 7 ++++
include/linux/filter.h | 16 ++++++++-
include/uapi/linux/bpf.h | 2 ++
kernel/bpf/core.c | 13 +++++++
kernel/bpf/syscall.c | 29 ++++++++++++++--
kernel/bpf/verifier.c | 5 +++
kernel/sysctl.c | 34 +++++++++++++++++++
.../bpftool/Documentation/bpftool-prog.rst | 4 ++-
tools/bpf/bpftool/prog.c | 7 ++++
tools/include/uapi/linux/bpf.h | 2 ++
10 files changed, 115 insertions(+), 4 deletions(-)
--
2.20.0
^ permalink raw reply
* [PATCH bpf-next 2/4] bpf: expose program stats via bpf_prog_info
From: Alexei Starovoitov @ 2019-02-22 23:36 UTC (permalink / raw)
To: davem; +Cc: daniel, netdev, bpf, kernel-team
In-Reply-To: <20190222233644.1487087-1-ast@kernel.org>
Return bpf program runtime and runcnt via bpf_prog_info
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
include/uapi/linux/bpf.h | 2 ++
kernel/bpf/syscall.c | 5 +++++
2 files changed, 7 insertions(+)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index bcdd2474eee7..d2cb85d85b39 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -2813,6 +2813,8 @@ struct bpf_prog_info {
__u32 jited_line_info_rec_size;
__u32 nr_prog_tags;
__aligned_u64 prog_tags;
+ __u64 runtime;
+ __u64 runcnt;
} __attribute__((aligned(8)));
struct bpf_map_info {
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 079e7fa14f39..6c55aeb2e1b7 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2142,6 +2142,7 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
struct bpf_prog_info __user *uinfo = u64_to_user_ptr(attr->info.info);
struct bpf_prog_info info = {};
u32 info_len = attr->info.info_len;
+ struct bpf_prog_stats stats;
char __user *uinsns;
u32 ulen;
int err;
@@ -2181,6 +2182,10 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
if (err)
return err;
+ bpf_prog_get_stats(prog, &stats);
+ info.runtime = stats.nsecs;
+ info.runcnt = stats.cnt;
+
if (!capable(CAP_SYS_ADMIN)) {
info.jited_prog_len = 0;
info.xlated_prog_len = 0;
--
2.20.0
^ permalink raw reply related
* [PATCH bpf-next 3/4] tools/bpf: sync bpf.h into tools
From: Alexei Starovoitov @ 2019-02-22 23:36 UTC (permalink / raw)
To: davem; +Cc: daniel, netdev, bpf, kernel-team
In-Reply-To: <20190222233644.1487087-1-ast@kernel.org>
sync bpf.h into tools directory
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
tools/include/uapi/linux/bpf.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index bcdd2474eee7..d2cb85d85b39 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -2813,6 +2813,8 @@ struct bpf_prog_info {
__u32 jited_line_info_rec_size;
__u32 nr_prog_tags;
__aligned_u64 prog_tags;
+ __u64 runtime;
+ __u64 runcnt;
} __attribute__((aligned(8)));
struct bpf_map_info {
--
2.20.0
^ permalink raw reply related
* [PATCH bpf-next 1/4] bpf: enable program stats
From: Alexei Starovoitov @ 2019-02-22 23:36 UTC (permalink / raw)
To: davem; +Cc: daniel, netdev, bpf, kernel-team
In-Reply-To: <20190222233644.1487087-1-ast@kernel.org>
JITed BPF programs are indistinguishable from kernel functions, but unlike
kernel code BPF code can be changed often.
Typical approach of "perf record" + "perf report" profiling and tunning of
kernel code works just as well for BPF programs, but kernel code doesn't
need to be monitored whereas BPF programs do.
Users load and run large amount of BPF programs.
These BPF stats allow tools monitor the usage of BPF on the server.
The monitoring tools will turn sysctl kernel.bpf_stats_enabled
on and off for few seconds to sample average cost of the programs.
Aggregated data over hours and days will provide an insight into cost of BPF
and alarms can trigger in case given program suddenly gets more expensive.
The cost of two sched_clock() per program invocation adds ~20 nsec.
Fast BPF progs (like selftests/bpf/progs/test_pkt_access.c) will slow down
from ~40 nsec to ~60 nsec.
static_key minimizes the cost of the stats collection.
There is no measurable difference before/after this patch
with kernel.bpf_stats_enabled=0
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
include/linux/bpf.h | 7 +++++++
include/linux/filter.h | 16 +++++++++++++++-
kernel/bpf/core.c | 13 +++++++++++++
kernel/bpf/syscall.c | 24 ++++++++++++++++++++++--
kernel/bpf/verifier.c | 5 +++++
kernel/sysctl.c | 34 ++++++++++++++++++++++++++++++++++
6 files changed, 96 insertions(+), 3 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index de18227b3d95..14260674bc57 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -340,6 +340,11 @@ enum bpf_cgroup_storage_type {
#define MAX_BPF_CGROUP_STORAGE_TYPE __BPF_CGROUP_STORAGE_MAX
+struct bpf_prog_stats {
+ u64 cnt;
+ u64 nsecs;
+};
+
struct bpf_prog_aux {
atomic_t refcnt;
u32 used_map_cnt;
@@ -389,6 +394,7 @@ struct bpf_prog_aux {
* main prog always has linfo_idx == 0
*/
u32 linfo_idx;
+ struct bpf_prog_stats __percpu *stats;
union {
struct work_struct work;
struct rcu_head rcu;
@@ -559,6 +565,7 @@ void bpf_map_area_free(void *base);
void bpf_map_init_from_attr(struct bpf_map *map, union bpf_attr *attr);
extern int sysctl_unprivileged_bpf_disabled;
+extern int sysctl_bpf_stats_enabled;
int bpf_map_new_fd(struct bpf_map *map, int flags);
int bpf_prog_new_fd(struct bpf_prog *prog);
diff --git a/include/linux/filter.h b/include/linux/filter.h
index f32b3eca5a04..7b14235b6f7d 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -533,7 +533,21 @@ struct sk_filter {
struct bpf_prog *prog;
};
-#define BPF_PROG_RUN(filter, ctx) ({ cant_sleep(); (*(filter)->bpf_func)(ctx, (filter)->insnsi); })
+DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
+
+#define BPF_PROG_RUN(prog, ctx) ({ \
+ u32 ret; \
+ cant_sleep(); \
+ if (static_branch_unlikely(&bpf_stats_enabled_key)) { \
+ u64 start = sched_clock(); \
+ ret = (*(prog)->bpf_func)(ctx, (prog)->insnsi); \
+ this_cpu_inc(prog->aux->stats->cnt); \
+ this_cpu_add(prog->aux->stats->nsecs, \
+ sched_clock() - start); \
+ } else { \
+ ret = (*(prog)->bpf_func)(ctx, (prog)->insnsi); \
+ } \
+ ret; })
#define BPF_SKB_CB_LEN QDISC_CB_PRIV_LEN
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index ef88b167959d..574747f98d44 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -95,6 +95,13 @@ struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags)
return NULL;
}
+ aux->stats = alloc_percpu_gfp(struct bpf_prog_stats, gfp_flags);
+ if (!aux->stats) {
+ kfree(aux);
+ vfree(fp);
+ return NULL;
+ }
+
fp->pages = size / PAGE_SIZE;
fp->aux = aux;
fp->aux->prog = fp;
@@ -231,6 +238,8 @@ struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size,
void __bpf_prog_free(struct bpf_prog *fp)
{
+ if (fp->aux && !fp->is_func)
+ free_percpu(fp->aux->stats);
kfree(fp->aux);
vfree(fp);
}
@@ -2069,6 +2078,10 @@ int __weak skb_copy_bits(const struct sk_buff *skb, int offset, void *to,
return -EFAULT;
}
+DEFINE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
+EXPORT_SYMBOL(bpf_stats_enabled_key);
+int sysctl_bpf_stats_enabled __read_mostly;
+
/* All definitions of tracepoints related to BPF. */
#define CREATE_TRACE_POINTS
#include <linux/bpf_trace.h>
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index ec7c552af76b..079e7fa14f39 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1283,24 +1283,44 @@ static int bpf_prog_release(struct inode *inode, struct file *filp)
return 0;
}
+static void bpf_prog_get_stats(const struct bpf_prog *prog,
+ struct bpf_prog_stats *stats)
+{
+ u64 nsecs = 0, cnt = 0;
+ int cpu;
+
+ for_each_possible_cpu(cpu) {
+ nsecs += per_cpu(prog->aux->stats->nsecs, cpu);
+ cnt += per_cpu(prog->aux->stats->cnt, cpu);
+ }
+ stats->nsecs = nsecs;
+ stats->cnt = cnt;
+}
+
#ifdef CONFIG_PROC_FS
static void bpf_prog_show_fdinfo(struct seq_file *m, struct file *filp)
{
const struct bpf_prog *prog = filp->private_data;
char prog_tag[sizeof(prog->tag) * 2 + 1] = { };
+ struct bpf_prog_stats stats;
+ bpf_prog_get_stats(prog, &stats);
bin2hex(prog_tag, prog->tag, sizeof(prog->tag));
seq_printf(m,
"prog_type:\t%u\n"
"prog_jited:\t%u\n"
"prog_tag:\t%s\n"
"memlock:\t%llu\n"
- "prog_id:\t%u\n",
+ "prog_id:\t%u\n"
+ "runtime:\t%llu\n"
+ "runcnt:\t%llu\n",
prog->type,
prog->jited,
prog_tag,
prog->pages * 1ULL << PAGE_SHIFT,
- prog->aux->id);
+ prog->aux->id,
+ stats.nsecs,
+ stats.cnt);
}
#endif
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1b9496c41383..dff61920a316 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7330,6 +7330,11 @@ static int jit_subprogs(struct bpf_verifier_env *env)
if (bpf_prog_calc_tag(func[i]))
goto out_free;
func[i]->is_func = 1;
+ /* BPF_PROG_RUN doesn't call subprogs directly,
+ * hence stats are reused from main prog
+ */
+ free_percpu(func[i]->aux->stats);
+ func[i]->aux->stats = prog->aux->stats;
func[i]->aux->func_idx = i;
/* the btf and func_info will be freed only at prog->aux */
func[i]->aux->btf = prog->aux->btf;
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index ba4d9e85feb8..86e0771352f2 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -224,6 +224,9 @@ static int proc_dostring_coredump(struct ctl_table *table, int write,
#endif
static int proc_dopipe_max_size(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos);
+static int proc_dointvec_minmax_bpf_stats(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp,
+ loff_t *ppos);
#ifdef CONFIG_MAGIC_SYSRQ
/* Note: sysrq code uses its own private copy */
@@ -1230,6 +1233,15 @@ static struct ctl_table kern_table[] = {
.extra2 = &one,
},
#endif
+ {
+ .procname = "bpf_stats_enabled",
+ .data = &sysctl_bpf_stats_enabled,
+ .maxlen = sizeof(sysctl_bpf_stats_enabled),
+ .mode = 0644,
+ .proc_handler = proc_dointvec_minmax_bpf_stats,
+ .extra1 = &zero,
+ .extra2 = &one,
+ },
#if defined(CONFIG_TREE_RCU) || defined(CONFIG_PREEMPT_RCU)
{
.procname = "panic_on_rcu_stall",
@@ -3260,6 +3272,28 @@ int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int write,
#endif /* CONFIG_PROC_SYSCTL */
+static int proc_dointvec_minmax_bpf_stats(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp,
+ loff_t *ppos)
+{
+ int ret, bpf_stats = *(int *)table->data;
+ struct ctl_table tmp = *table;
+
+ if (write && !capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ tmp.data = &bpf_stats;
+ ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
+ if (write && !ret) {
+ *(int *)table->data = bpf_stats;
+ if (bpf_stats)
+ static_branch_enable(&bpf_stats_enabled_key);
+ else
+ static_branch_disable(&bpf_stats_enabled_key);
+ }
+ return ret;
+}
+
/*
* No sense putting this after each symbol definition, twice,
* exception granted :-)
--
2.20.0
^ permalink raw reply related
* [PATCH bpf-next 4/4] tools/bpftool: recognize bpf_prog_info runtime and runcnt
From: Alexei Starovoitov @ 2019-02-22 23:36 UTC (permalink / raw)
To: davem; +Cc: daniel, netdev, bpf, kernel-team
In-Reply-To: <20190222233644.1487087-1-ast@kernel.org>
$ bpftool p s
1: kprobe tag a56587d488d216c9 gpl runtime 79786 runcnt 8
loaded_at 2019-02-22T12:22:51-0800 uid 0
xlated 352B not jited memlock 4096B
$ bpftool --json --pretty p s
[{
"id": 1,
"type": "kprobe",
"tag": "a56587d488d216c9",
"gpl_compatible": true,
"runtime": 79786,
"runcnt": 8,
"loaded_at": 1550866971,
"uid": 0,
"bytes_xlated": 352,
"jited": false,
"bytes_memlock": 4096
}
]
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
tools/bpf/bpftool/Documentation/bpftool-prog.rst | 4 +++-
tools/bpf/bpftool/prog.c | 7 +++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index 12bc1e2d4b46..102b180d3add 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -171,7 +171,7 @@ EXAMPLES
::
- 10: xdp name some_prog tag 005a3d2123620c8b gpl
+ 10: xdp name some_prog tag 005a3d2123620c8b gpl runtime 81632 runcnt 10
loaded_at 2017-09-29T20:11:00+0000 uid 0
xlated 528B jited 370B memlock 4096B map_ids 10
@@ -184,6 +184,8 @@ EXAMPLES
"type": "xdp",
"tag": "005a3d2123620c8b",
"gpl_compatible": true,
+ "runtime": 81632,
+ "runcnt": 10,
"loaded_at": 1506715860,
"uid": 0,
"bytes_xlated": 528,
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index db978c8d76a8..8065ba11b9d5 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -214,6 +214,10 @@ static void print_prog_json(struct bpf_prog_info *info, int fd)
info->tag[4], info->tag[5], info->tag[6], info->tag[7]);
jsonw_bool_field(json_wtr, "gpl_compatible", info->gpl_compatible);
+ if (info->runtime) {
+ jsonw_uint_field(json_wtr, "runtime", info->runtime);
+ jsonw_uint_field(json_wtr, "runcnt", info->runcnt);
+ }
print_dev_json(info->ifindex, info->netns_dev, info->netns_ino);
@@ -277,6 +281,9 @@ static void print_prog_plain(struct bpf_prog_info *info, int fd)
fprint_hex(stdout, info->tag, BPF_TAG_SIZE, "");
print_dev_plain(info->ifindex, info->netns_dev, info->netns_ino);
printf("%s", info->gpl_compatible ? " gpl" : "");
+ if (info->runtime)
+ printf(" runtime %lld runcnt %lld",
+ info->runtime, info->runcnt);
printf("\n");
if (info->load_time) {
--
2.20.0
^ permalink raw reply related
* Re: [PATCH net-next v3 4/4] dt-bindings: net: freescale: enetc: Add connection bindings for ENETC ethernet nodes
From: Rob Herring @ 2019-02-22 23:37 UTC (permalink / raw)
To: Claudiu Manoil
Cc: Shawn Guo, Li Yang, David S . Miller, alexandru.marginean,
linux-arm-kernel, devicetree, netdev, linux-kernel
In-Reply-To: <1550847859-17346-5-git-send-email-claudiu.manoil@nxp.com>
On Fri, Feb 22, 2019 at 05:04:19PM +0200, Claudiu Manoil wrote:
> Define connection bindings (external PHY connections and internal links)
> for the ENETC on-chip ethernet controllers.
>
> Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
> ---
> v3 - added this patch to the set
>
> .../devicetree/bindings/net/fsl-enetc.txt | 109 +++++++++++++++++++++
> 1 file changed, 109 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/fsl-enetc.txt
>
> diff --git a/Documentation/devicetree/bindings/net/fsl-enetc.txt b/Documentation/devicetree/bindings/net/fsl-enetc.txt
> new file mode 100644
> index 0000000..2fbb998
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/fsl-enetc.txt
> @@ -0,0 +1,109 @@
> +* ENETC ethernet nodes - external connection bindings
> +
> +The ENETC ethernet controllers are PCIe integrated endpoints
> +(IEPs), on-chip devices discoverable as standard PCIe endpoints,
> +integrated into Freescale SoCs. The ENETC devices are self
> +contained, the information needed for device initialization
> +is available in hardware (PCIe ECAM area). However, depending
> +on board design, their external connections are configurable.
> +As usual for SoCs, device tree nodes may be used to define these
> +external connections. The rest of the document specifies how
> +external connections for ENETC ethernet controllers may be
> +defined via device tree nodes.
> +
> +Silicon (SoC) availability (<SoC name>: <SoC DT path/name>)
> + - LS1028A: [arch/arm64] [...]freescale/fsl-ls1028a.dtsi
This doesn't belong in bindings.
> +
> +
> +* ENETC nodes
> +
> +Defined in the SoC device tree as "pci" child nodes of the
> +"pci-host-ecam-generic" compatible "pcie" parent node also known
> +as the Integrated Endpoint Root Complex (IERC) SoC node.
The host controller attachment is also outside the scope of this
binding.
> +
> +Structure - example (LS1028A):
> +
> + pcie@1f0000000 {
> + compatible = "pci-host-ecam-generic";
> + device_type = "pci";
> + ...
> + enetc_port0: pci@0,0 {
The node name 'pci' is reserved for bridges. This should match the
device class if possible (ethernet).
> + reg = <0x000000 0 0 0 0>;
> + };
> + enetc_port1: pci@0,1 {
> + reg = <0x000100 0 0 0 0>;
> + };
> + ...
> + }
> +
> +Each ENETC node has a device number and a function number (expressed
> +by its "reg" property and pci node name, i.e. "pci@0,1" represents
> +device number 0 and functions number 1). Only the standard pci "reg"
> +property is needed here.
There should be a compatible too.
> +For easy reference, each ENETC node is tagged by a handle having the
> +following format:
> +
> + "enetc_port<idx>" where, idx = 0 .. n-1;
> + n - #of available ENETC nodes (Ports) on
> + the given SoC.
These are just source labels, so really they can be anything.
> +
> +NOTES
> +i. The SoC H/W Reference Manual provides a mapping of the ENETC Port
> +numbers to the PCIe Device Number/ Function Number.
> +Example (LS1028):
> + All ENETC PFs (PCIe Physical Functions) have Device Number 0.
> + Port 0 - PF0 (pci@0,0)
> + Port 1 - PF1 (pci@0,1)
> + Port 2 - PF2 (pci@0,2)
> + Port 3 - PF6 (pci@0,6)
> +
> +ii. The SoC H/W Reference Manual also defines which ENETC Ports may have
> +external connections (external ports) and which ones are internally
> +connected to a on-chip L2 switch for instance (internal ports).
> +
> +
> +* Defining connections for ENETC nodes
> +
> +To define external connections for the external ENETC Ports on a given
> +board/platform, the board device tree should include the SoC device tree
> +and reference the external ports by their "enetc_port<idx>" handle.
SoC vs. board files are convention, but not really part of the binding
ABI.
> +
> +Following cases arise, defining all possible connection bindings:
> +
> +1) The ENETC Port is connected to a MDIO configurable PHY:
> +
> + In this case, the ENETC node should include a "mdio" sub-node
> + that in turn should contain the "phy" node describing the
> + external phy. ENETC Port node structure (example):
> +
> + &enetc_port0 {
Please just show the full DT example, not separate chunks.
> + phy-handle = <&sgmii_phy0>;
> + phy-connection-type = "sgmii";
> +
> + mdio {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + sgmii_phy0: ethernet-phy@2 {
> + reg = <0x2>;
> + };
> + };
> + };
> +
> + All properties are mandatory for this case, their bindings already
> + defined (see ethernet.txt and phy.txt).
> +
> +2) The ENETC Port has a fixed-link connection:
> +
> + In this case, the ENETC Port node defines a fixed link connection,
> + with the following structure (example):
> +
> + &enetc_port2 {
> + fixed-link {
> + speed = <1000>;
> + full-duplex;
> + };
> + };
> +
> + The "fixed-link" node properties are standard (as defined in
> + fixed-link.txt).
> + This connection type also applies to the internal ENETC Ports.
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH net-next v2 0/7] net: phy: marvell10g: Add 2.5GBaseT support
From: Maxime Chevallier @ 2019-02-22 23:37 UTC (permalink / raw)
To: davem
Cc: Maxime Chevallier, netdev, linux-kernel, Andrew Lunn,
Florian Fainelli, Heiner Kallweit, Russell King, linux-arm-kernel,
Antoine Tenart, thomas.petazzoni, gregory.clement, miquel.raynal,
nadavh, stefanc, mw
This series adds the missing bits necessary to fully support 2.5GBaseT
in the Marvell Alaska PHYs.
The main points for that support are :
- Making use of the .get_features call, recently introduced by Heiner
and Andrew, that allows having a fully populated list of supported
modes, including 2500BaseT.
- Configuring the MII to 2500BaseX when establishing a link at 2.5G
- Adding a small quirk to take into account the fact that some PHYs in
the family won't report the correct supported abilities
The rest of the series consists of small cosmetic improvements such as
using the correct helper to set a linkmode bit and adding macros for the
PHY ids.
We also add support for the 88E2110 PHY, which doesn't require the
quirk, and support for 2500BaseT in the PPv2 driver, in order to have a
fully working setup on the MacchiatoBin board.
Changes since V1 : Fixed formatting issue in patch 01, rebased.
Maxime Chevallier (7):
net: phy: marvell10g: Use get_features to get the PHY abilities
net: phy: marvell10g: Use linkmode_set_bit helper instead of __set_bit
net: phy: marvell10g: Use 2500BASEX when using 2.5GBASET
net: phy: marvell10g: Use a #define for 88X3310 family id
net: phy: marvell10g: Force reading of 2.5/5G
net: mvpp2: Add 2.5GBaseT support
net: phy: marvell10g: add support for the 88x2110 PHY
.../net/ethernet/marvell/mvpp2/mvpp2_main.c | 1 +
drivers/net/phy/marvell10g.c | 93 ++++++++++++++++---
include/linux/marvell_phy.h | 2 +
3 files changed, 82 insertions(+), 14 deletions(-)
--
2.19.2
^ permalink raw reply
* [PATCH net-next v2 1/7] net: phy: marvell10g: Use get_features to get the PHY abilities
From: Maxime Chevallier @ 2019-02-22 23:37 UTC (permalink / raw)
To: davem
Cc: Maxime Chevallier, netdev, linux-kernel, Andrew Lunn,
Florian Fainelli, Heiner Kallweit, Russell King, linux-arm-kernel,
Antoine Tenart, thomas.petazzoni, gregory.clement, miquel.raynal,
nadavh, stefanc, mw
In-Reply-To: <20190222233744.25735-1-maxime.chevallier@bootlin.com>
The Alaska family of 10G PHYs has more abilities than the ones listed in
PHY_10GBIT_FULL_FEATURES, the exact list depending on the model.
Make use of the newly introduced .get_features call to build this list,
using genphy_c45_pma_read_abilities to build the list of supported
linkmodes, and adding autoneg ability based on what's reported by the AN
MMD.
.config_init is still used to validate the interface_mode.
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V2: Added missing blank line
drivers/net/phy/marvell10g.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 586ae1bc5a50..89920b10d75b 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -233,8 +233,6 @@ static int mv3310_resume(struct phy_device *phydev)
static int mv3310_config_init(struct phy_device *phydev)
{
- int ret, val;
-
/* Check that the PHY interface type is compatible */
if (phydev->interface != PHY_INTERFACE_MODE_SGMII &&
phydev->interface != PHY_INTERFACE_MODE_XAUI &&
@@ -242,6 +240,13 @@ static int mv3310_config_init(struct phy_device *phydev)
phydev->interface != PHY_INTERFACE_MODE_10GKR)
return -ENODEV;
+ return 0;
+}
+
+static int mv3310_get_features(struct phy_device *phydev)
+{
+ int ret, val;
+
if (phydev->c45_ids.devices_in_package & MDIO_DEVS_AN) {
val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
if (val < 0)
@@ -416,7 +421,7 @@ static struct phy_driver mv3310_drivers[] = {
.phy_id = 0x002b09aa,
.phy_id_mask = MARVELL_PHY_ID_MASK,
.name = "mv88x3310",
- .features = PHY_10GBIT_FEATURES,
+ .get_features = mv3310_get_features,
.soft_reset = gen10g_no_soft_reset,
.config_init = mv3310_config_init,
.probe = mv3310_probe,
--
2.19.2
^ permalink raw reply related
* [PATCH net-next v2 2/7] net: phy: marvell10g: Use linkmode_set_bit helper instead of __set_bit
From: Maxime Chevallier @ 2019-02-22 23:37 UTC (permalink / raw)
To: davem
Cc: Maxime Chevallier, netdev, linux-kernel, Andrew Lunn,
Florian Fainelli, Heiner Kallweit, Russell King, linux-arm-kernel,
Antoine Tenart, thomas.petazzoni, gregory.clement, miquel.raynal,
nadavh, stefanc, mw
In-Reply-To: <20190222233744.25735-1-maxime.chevallier@bootlin.com>
Cosmetic patch making use of helpers dedicated to linkmodes handling.
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
drivers/net/phy/marvell10g.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 89920b10d75b..821ef1b2f8cc 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -253,8 +253,8 @@ static int mv3310_get_features(struct phy_device *phydev)
return val;
if (val & MDIO_AN_STAT1_ABLE)
- __set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
- phydev->supported);
+ linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+ phydev->supported);
}
ret = genphy_c45_pma_read_abilities(phydev);
--
2.19.2
^ permalink raw reply related
* [PATCH net-next v2 3/7] net: phy: marvell10g: Use 2500BASEX when using 2.5GBASET
From: Maxime Chevallier @ 2019-02-22 23:37 UTC (permalink / raw)
To: davem
Cc: Maxime Chevallier, netdev, linux-kernel, Andrew Lunn,
Florian Fainelli, Heiner Kallweit, Russell King, linux-arm-kernel,
Antoine Tenart, thomas.petazzoni, gregory.clement, miquel.raynal,
nadavh, stefanc, mw
In-Reply-To: <20190222233744.25735-1-maxime.chevallier@bootlin.com>
The Marvell Alaska family of PHYs supports 2.5GBaseT and 5GBaseT modes,
as defined in the 802.3bz specification.
Upon establishing a 2.5GBASET link, the PHY will reconfigure it's MII
interface to 2500BASEX.
At 5G, the PHY will reconfigure it's interface to 5GBASE-R, but this
mode isn't supported by any MAC for now.
This was tested with :
- The 88X3310, which is on the MacchiatoBin
- The 88E2010, an Alaska PHY that has no fiber interfaces, and is
limited to 5G maximum speed.
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
drivers/net/phy/marvell10g.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 821ef1b2f8cc..9342d8c2ff7f 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -235,6 +235,7 @@ static int mv3310_config_init(struct phy_device *phydev)
{
/* Check that the PHY interface type is compatible */
if (phydev->interface != PHY_INTERFACE_MODE_SGMII &&
+ phydev->interface != PHY_INTERFACE_MODE_2500BASEX &&
phydev->interface != PHY_INTERFACE_MODE_XAUI &&
phydev->interface != PHY_INTERFACE_MODE_RXAUI &&
phydev->interface != PHY_INTERFACE_MODE_10GKR)
@@ -313,18 +314,29 @@ static int mv3310_aneg_done(struct phy_device *phydev)
static void mv3310_update_interface(struct phy_device *phydev)
{
if ((phydev->interface == PHY_INTERFACE_MODE_SGMII ||
+ phydev->interface == PHY_INTERFACE_MODE_2500BASEX ||
phydev->interface == PHY_INTERFACE_MODE_10GKR) && phydev->link) {
/* The PHY automatically switches its serdes interface (and
- * active PHYXS instance) between Cisco SGMII and 10GBase-KR
- * modes according to the speed. Florian suggests setting
- * phydev->interface to communicate this to the MAC. Only do
- * this if we are already in either SGMII or 10GBase-KR mode.
+ * active PHYXS instance) between Cisco SGMII, 10GBase-KR and
+ * 2500BaseX modes according to the speed. Florian suggests
+ * setting phydev->interface to communicate this to the MAC.
+ * Only do this if we are already in one of the above modes.
*/
- if (phydev->speed == SPEED_10000)
+ switch (phydev->speed) {
+ case SPEED_10000:
phydev->interface = PHY_INTERFACE_MODE_10GKR;
- else if (phydev->speed >= SPEED_10 &&
- phydev->speed < SPEED_10000)
+ break;
+ case SPEED_2500:
+ phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
+ break;
+ case SPEED_1000:
+ case SPEED_100:
+ case SPEED_10:
phydev->interface = PHY_INTERFACE_MODE_SGMII;
+ break;
+ default:
+ break;
+ }
}
}
--
2.19.2
^ permalink raw reply related
* [PATCH net-next v2 4/7] net: phy: marvell10g: Use a #define for 88X3310 family id
From: Maxime Chevallier @ 2019-02-22 23:37 UTC (permalink / raw)
To: davem
Cc: Maxime Chevallier, netdev, linux-kernel, Andrew Lunn,
Florian Fainelli, Heiner Kallweit, Russell King, linux-arm-kernel,
Antoine Tenart, thomas.petazzoni, gregory.clement, miquel.raynal,
nadavh, stefanc, mw
In-Reply-To: <20190222233744.25735-1-maxime.chevallier@bootlin.com>
The PHY ID corresponding to the 88X3310 is also used for other PHYs in
the same family, such as the 88E2010. Use a #define for the PHY id, that
ignores the last nibble.
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
drivers/net/phy/marvell10g.c | 4 ++--
include/linux/marvell_phy.h | 1 +
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 9342d8c2ff7f..9c0b8f16cec5 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -430,7 +430,7 @@ static int mv3310_read_status(struct phy_device *phydev)
static struct phy_driver mv3310_drivers[] = {
{
- .phy_id = 0x002b09aa,
+ .phy_id = MARVELL_PHY_ID_88X3310,
.phy_id_mask = MARVELL_PHY_ID_MASK,
.name = "mv88x3310",
.get_features = mv3310_get_features,
@@ -448,7 +448,7 @@ static struct phy_driver mv3310_drivers[] = {
module_phy_driver(mv3310_drivers);
static struct mdio_device_id __maybe_unused mv3310_tbl[] = {
- { 0x002b09aa, MARVELL_PHY_ID_MASK },
+ { MARVELL_PHY_ID_88X3310, MARVELL_PHY_ID_MASK },
{ },
};
MODULE_DEVICE_TABLE(mdio, mv3310_tbl);
diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h
index 1eb6f244588d..70c17345e118 100644
--- a/include/linux/marvell_phy.h
+++ b/include/linux/marvell_phy.h
@@ -20,6 +20,7 @@
#define MARVELL_PHY_ID_88E1540 0x01410eb0
#define MARVELL_PHY_ID_88E1545 0x01410ea0
#define MARVELL_PHY_ID_88E3016 0x01410e60
+#define MARVELL_PHY_ID_88X3310 0x002b09a0
/* The MV88e6390 Ethernet switch contains embedded PHYs. These PHYs do
* not have a model ID. So the switch driver traps reads to the ID2
--
2.19.2
^ permalink raw reply related
* [PATCH net-next v2 5/7] net: phy: marvell10g: Force reading of 2.5/5G
From: Maxime Chevallier @ 2019-02-22 23:37 UTC (permalink / raw)
To: davem
Cc: Maxime Chevallier, netdev, linux-kernel, Andrew Lunn,
Florian Fainelli, Heiner Kallweit, Russell King, linux-arm-kernel,
Antoine Tenart, thomas.petazzoni, gregory.clement, miquel.raynal,
nadavh, stefanc, mw
In-Reply-To: <20190222233744.25735-1-maxime.chevallier@bootlin.com>
As per 802.3bz, if bit 14 of (1.11) "PMA Extended Abilities" indicates
whether or not we should read register (1.21) "2.52/5G PMA Extended
Abilities", which contains information on the support of 2.5GBASET and
5GBASET.
After testing on several variants of PHYS of this family, it appears
that bit 14 in (1.11) isn't always set when it should be.
PHYs 88X3310 (on MacchiatoBin) and 88E2010 do support 2.5G and 5GBASET,
but don't have 1.11.14 set. Their register 1.21 is filled with the
correct values, indicating 2.5G and 5G support.
PHYs 88E2110 do have their 1.11.14 bit set, as it should.
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
drivers/net/phy/marvell10g.c | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 9c0b8f16cec5..8f354c3f3876 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -27,6 +27,9 @@
#include <linux/marvell_phy.h>
#include <linux/phy.h>
+#define MV_PHY_ALASKA_NBT_QUIRK_MASK 0xfffffffe
+#define MV_PHY_ALASKA_NBT_QUIRK_REV (MARVELL_PHY_ID_88X3310 | 0xa)
+
enum {
MV_PCS_BASE_T = 0x0000,
MV_PCS_BASE_R = 0x1000,
@@ -231,6 +234,23 @@ static int mv3310_resume(struct phy_device *phydev)
return mv3310_hwmon_config(phydev, true);
}
+/* Some PHYs in the Alaska family such as the 88X3310 and the 88E2010
+ * don't set bit 14 in PMA Extended Abilities (1.11), although they do
+ * support 2.5GBASET and 5GBASET. For these models, we can still read their
+ * 2.5G/5G extended abilities register (1.21). We detect these models based on
+ * the PMA device identifier, with a mask matching models known to have this
+ * issue
+ */
+static bool mv3310_has_pma_ngbaset_quirk(struct phy_device *phydev)
+{
+ if (!(phydev->c45_ids.devices_in_package & MDIO_DEVS_PMAPMD))
+ return false;
+
+ /* Only some revisions of the 88X3310 family PMA seem to be impacted */
+ return (phydev->c45_ids.device_ids[MDIO_MMD_PMAPMD] &
+ MV_PHY_ALASKA_NBT_QUIRK_MASK) == MV_PHY_ALASKA_NBT_QUIRK_REV;
+}
+
static int mv3310_config_init(struct phy_device *phydev)
{
/* Check that the PHY interface type is compatible */
@@ -262,6 +282,21 @@ static int mv3310_get_features(struct phy_device *phydev)
if (ret)
return ret;
+ if (mv3310_has_pma_ngbaset_quirk(phydev)) {
+ val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD,
+ MDIO_PMA_NG_EXTABLE);
+ if (val < 0)
+ return val;
+
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
+ phydev->supported,
+ val & MDIO_PMA_NG_EXTABLE_2_5GBT);
+
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+ phydev->supported,
+ val & MDIO_PMA_NG_EXTABLE_5GBT);
+ }
+
return 0;
}
--
2.19.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox