* Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
From: Stanislav Fomichev @ 2018-11-08 21:29 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Stanislav Fomichev, netdev, linux-kselftest, ast, daniel, shuah,
quentin.monnet, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181108114630.28907570@cakuba.hsd1.ca.comcast.net>
On 11/08, Jakub Kicinski wrote:
> On Wed, 7 Nov 2018 21:39:57 -0800, Stanislav Fomichev wrote:
> > This commit adds support for loading/attaching/detaching flow
> > dissector program. The structure of the flow dissector program is
> > assumed to be the same as in the selftests:
> >
> > * flow_dissector section with the main entry point
> > * a bunch of tail call progs
> > * a jmp_table map that is populated with the tail call progs
>
> Could you split the loadall changes and the flow_dissector changes into
> two separate patches?
Sure, will do, but let's first agree on the semantical differences of
load vs loadall.
So far *load* actually loads _all_ progs (via bpf_object__load), but pins
only the first program. Is that what we want? I wonder whether the
assumption there was that there is only single program in the object.
Should we load only the first program in *load*?
If we add *loadall*, then the difference would be:
*load*:
* loads all maps and only the first program, pins only the first
program
*loadall*:
* loads all maps and all programs, pins everything (maps and programs)
Is this the expected behavior?
^ permalink raw reply
* [PATCH net-next] net: phy: improve struct phy_device member interrupts handling
From: Heiner Kallweit @ 2018-11-08 21:36 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev@vger.kernel.org
As a heritage from the very early days of phylib member interrupts is
defined as u32 even though it's just a flag whether interrupts are
enabled. So we can change it to a bitfield member. In addition change
the code dealing with this member in a way that it's clear we're
dealing with a bool value.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
Actually this member isn't needed at all and could be replaced with
a parameter in phy_driver->config_intr. But this would mean an API
change, maybe I come up with a proposal later.
---
drivers/net/phy/phy.c | 2 +-
include/linux/phy.h | 10 +++++-----
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index dd5bff955..a5e6acfe6 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -115,7 +115,7 @@ static int phy_clear_interrupt(struct phy_device *phydev)
*
* Returns 0 on success or < 0 on error.
*/
-static int phy_config_interrupt(struct phy_device *phydev, u32 interrupts)
+static int phy_config_interrupt(struct phy_device *phydev, bool interrupts)
{
phydev->interrupts = interrupts;
if (phydev->drv->config_intr)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5dd85c441..fc90af152 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -263,8 +263,8 @@ static inline struct mii_bus *devm_mdiobus_alloc(struct device *dev)
void devm_mdiobus_free(struct device *dev, struct mii_bus *bus);
struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
-#define PHY_INTERRUPT_DISABLED 0x0
-#define PHY_INTERRUPT_ENABLED 0x80000000
+#define PHY_INTERRUPT_DISABLED 0
+#define PHY_INTERRUPT_ENABLED 1
/* PHY state machine states:
*
@@ -410,6 +410,9 @@ struct phy_device {
/* The most recently read link state */
unsigned link:1;
+ /* Interrupts are enabled */
+ unsigned interrupts:1;
+
enum phy_state state;
u32 dev_flags;
@@ -425,9 +428,6 @@ struct phy_device {
int pause;
int asym_pause;
- /* Enabled Interrupts */
- u32 interrupts;
-
/* Union of PHY and Attached devices' supported modes */
/* See mii.h for more info */
u32 supported;
--
2.19.1
^ permalink raw reply related
* Re: [PATCH v3 1/2] kretprobe: produce sane stack traces
From: Masami Hiramatsu @ 2018-11-09 7:15 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Steven Rostedt, Naveen N. Rao, Anil S Keshavamurthy,
David S. Miller, Masami Hiramatsu, Jonathan Corbet,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Shuah Khan,
Alexei Starovoitov, Daniel Borkmann, Brendan Gregg,
Christian Brauner, Aleksa Sarai, netdev, linux-doc
In-Reply-To: <20181108080448.rggfn4zawi3por23@yavin>
On Thu, 8 Nov 2018 19:04:48 +1100
Aleksa Sarai <cyphar@cyphar.com> wrote:
> On 2018-11-08, Aleksa Sarai <cyphar@cyphar.com> wrote:
> > I will attach what I have at the moment to hopefully explain what the
> > issue I've found is (re-using the kretprobe architecture but with the
> > shadow-stack idea).
>
> Here is the patch I have at the moment (it works, except for the
> question I have about how to handle the top-level pt_regs -- I've marked
> that code with XXX).
>
> --
> Aleksa Sarai
> Senior Software Engineer (Containers)
> SUSE Linux GmbH
> <https://www.cyphar.com/>
>
> --8<---------------------------------------------------------------------
>
> Since the return address is modified by kretprobe, the various unwinders
> can produce invalid and confusing stack traces. ftrace mostly solved
> this problem by teaching each unwinder how to find the original return
> address for stack trace purposes. This same technique can be applied to
> kretprobes by simply adding a pointer to where the return address was
> replaced in the stack, and then looking up the relevant
> kretprobe_instance when a stack trace is requested.
>
> [WIP: This is currently broken because the *first entry* will not be
> overwritten since it looks like the stack pointer is different
> when we are provided pt_regs. All other addresses are correctly
> handled.]
>
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
> arch/x86/events/core.c | 6 +++-
> arch/x86/include/asm/ptrace.h | 1 +
> arch/x86/kernel/kprobes/core.c | 5 ++--
> arch/x86/kernel/stacktrace.c | 10 +++++--
> arch/x86/kernel/unwind_frame.c | 2 ++
> arch/x86/kernel/unwind_guess.c | 5 +++-
> arch/x86/kernel/unwind_orc.c | 2 ++
> include/linux/kprobes.h | 15 +++++++++-
> kernel/kprobes.c | 55 ++++++++++++++++++++++++++++++++++
> 9 files changed, 93 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index de32741d041a..d71062702179 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -2371,7 +2371,11 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *re
> return;
> }
>
> - if (perf_callchain_store(entry, regs->ip))
> + /* XXX: Currently broken -- stack_addr(regs) doesn't match entry. */
> + addr = regs->ip;
> + //addr = ftrace_graph_ret_addr(current, &state.graph_idx, addr, stack_addr(regs));
> + addr = kretprobe_ret_addr(current, addr, stack_addr(regs));
> + if (perf_callchain_store(entry, addr))
> return;
>
> for (unwind_start(&state, current, regs, NULL); !unwind_done(&state);
> diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
> index ee696efec99f..c4dfafd43e11 100644
> --- a/arch/x86/include/asm/ptrace.h
> +++ b/arch/x86/include/asm/ptrace.h
> @@ -172,6 +172,7 @@ static inline unsigned long kernel_stack_pointer(struct pt_regs *regs)
> return regs->sp;
> }
> #endif
> +#define stack_addr(regs) ((unsigned long *) kernel_stack_pointer(regs))
No, you should use kernel_stack_pointer(regs) itself instead of stack_addr().
>
> #define GET_IP(regs) ((regs)->ip)
> #define GET_FP(regs) ((regs)->bp)
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index b0d1e81c96bb..eb4da885020c 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -69,8 +69,6 @@
> DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
> DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
>
> -#define stack_addr(regs) ((unsigned long *)kernel_stack_pointer(regs))
I don't like keeping this meaningless macro... this should be replaced with generic
kernel_stack_pointer() macro.
> -
> #define W(row, b0, b1, b2, b3, b4, b5, b6, b7, b8, b9, ba, bb, bc, bd, be, bf)\
> (((b0##UL << 0x0)|(b1##UL << 0x1)|(b2##UL << 0x2)|(b3##UL << 0x3) | \
> (b4##UL << 0x4)|(b5##UL << 0x5)|(b6##UL << 0x6)|(b7##UL << 0x7) | \
> @@ -568,7 +566,8 @@ void arch_prepare_kretprobe(struct kretprobe_instance *ri, struct pt_regs *regs)
> {
> unsigned long *sara = stack_addr(regs);
>
> - ri->ret_addr = (kprobe_opcode_t *) *sara;
> + ri->ret_addrp = (kprobe_opcode_t **) sara;
> + ri->ret_addr = *ri->ret_addrp;
>
> /* Replace the return addr with trampoline addr */
> *sara = (unsigned long) &kretprobe_trampoline;
> diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
> index 7627455047c2..8a4fb3109d6b 100644
> --- a/arch/x86/kernel/stacktrace.c
> +++ b/arch/x86/kernel/stacktrace.c
> @@ -8,6 +8,7 @@
> #include <linux/sched/task_stack.h>
> #include <linux/stacktrace.h>
> #include <linux/export.h>
> +#include <linux/kprobes.h>
> #include <linux/uaccess.h>
> #include <asm/stacktrace.h>
> #include <asm/unwind.h>
> @@ -37,8 +38,13 @@ static void noinline __save_stack_trace(struct stack_trace *trace,
> struct unwind_state state;
> unsigned long addr;
>
> - if (regs)
> - save_stack_address(trace, regs->ip, nosched);
> + if (regs) {
> + /* XXX: Currently broken -- stack_addr(regs) doesn't match entry. */
> + addr = regs->ip;
Since this part is for storing regs->ip as a top of call-stack, this
seems correct code. Stack unwind will be done next block.
> + //addr = ftrace_graph_ret_addr(current, &state.graph_idx, addr, stack_addr(regs));
so func graph return trampoline address will be shown only when unwinding stack entries.
I mean func-graph tracer is not used as an event, so it never kicks stackdump.
> + addr = kretprobe_ret_addr(current, addr, stack_addr(regs));
But since kretprobe will be an event, which can kick the stackdump.
BTW, from kretprobe, regs->ip should always be the trampoline handler,
see arch/x86/kernel/kprobes/core.c:772 :-)
So it must be fixed always.
> + save_stack_address(trace, addr, nosched);
> + }
>
> for (unwind_start(&state, task, regs, NULL); !unwind_done(&state);
> unwind_next_frame(&state)) {
> diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
> index 3dc26f95d46e..47062427e9a3 100644
> --- a/arch/x86/kernel/unwind_frame.c
> +++ b/arch/x86/kernel/unwind_frame.c
> @@ -1,4 +1,5 @@
> #include <linux/sched.h>
> +#include <linux/kprobes.h>
> #include <linux/sched/task.h>
> #include <linux/sched/task_stack.h>
> #include <linux/interrupt.h>
> @@ -270,6 +271,7 @@ static bool update_stack_state(struct unwind_state *state,
> addr = READ_ONCE_TASK_STACK(state->task, *addr_p);
> state->ip = ftrace_graph_ret_addr(state->task, &state->graph_idx,
> addr, addr_p);
> + state->ip = kretprobe_ret_addr(state->task, state->ip, addr_p);
> }
>
> /* Save the original stack pointer for unwind_dump(): */
> diff --git a/arch/x86/kernel/unwind_guess.c b/arch/x86/kernel/unwind_guess.c
> index 4f0e17b90463..554fd7c5c331 100644
> --- a/arch/x86/kernel/unwind_guess.c
> +++ b/arch/x86/kernel/unwind_guess.c
> @@ -1,5 +1,6 @@
> #include <linux/sched.h>
> #include <linux/ftrace.h>
> +#include <linux/kprobes.h>
> #include <asm/ptrace.h>
> #include <asm/bitops.h>
> #include <asm/stacktrace.h>
> @@ -14,8 +15,10 @@ unsigned long unwind_get_return_address(struct unwind_state *state)
>
> addr = READ_ONCE_NOCHECK(*state->sp);
>
> - return ftrace_graph_ret_addr(state->task, &state->graph_idx,
> + addr = ftrace_graph_ret_addr(state->task, &state->graph_idx,
> addr, state->sp);
> + addr = kretprobe_ret_addr(state->task, addr, state->sp);
> + return addr;
> }
> EXPORT_SYMBOL_GPL(unwind_get_return_address);
>
> diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
> index 26038eacf74a..b6393500d505 100644
> --- a/arch/x86/kernel/unwind_orc.c
> +++ b/arch/x86/kernel/unwind_orc.c
> @@ -1,5 +1,6 @@
> #include <linux/module.h>
> #include <linux/sort.h>
> +#include <linux/kprobes.h>
> #include <asm/ptrace.h>
> #include <asm/stacktrace.h>
> #include <asm/unwind.h>
> @@ -462,6 +463,7 @@ bool unwind_next_frame(struct unwind_state *state)
>
> state->ip = ftrace_graph_ret_addr(state->task, &state->graph_idx,
> state->ip, (void *)ip_p);
> + state->ip = kretprobe_ret_addr(state->task, state->ip, (void *)ip_p);
>
> state->sp = sp;
> state->regs = NULL;
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index e909413e4e38..3a01f9998064 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -172,6 +172,7 @@ struct kretprobe_instance {
> struct hlist_node hlist;
> struct kretprobe *rp;
> kprobe_opcode_t *ret_addr;
> + kprobe_opcode_t **ret_addrp;
> struct task_struct *task;
> char data[0];
> };
> @@ -203,6 +204,7 @@ static inline int kprobes_built_in(void)
> extern void arch_prepare_kretprobe(struct kretprobe_instance *ri,
> struct pt_regs *regs);
> extern int arch_trampoline_kprobe(struct kprobe *p);
> +extern void kretprobe_trampoline(void);
> #else /* CONFIG_KRETPROBES */
> static inline void arch_prepare_kretprobe(struct kretprobe *rp,
> struct pt_regs *regs)
> @@ -212,6 +214,9 @@ static inline int arch_trampoline_kprobe(struct kprobe *p)
> {
> return 0;
> }
> +static inline void kretprobe_trampoline(void)
> +{
> +}
> #endif /* CONFIG_KRETPROBES */
>
> extern struct kretprobe_blackpoint kretprobe_blacklist[];
> @@ -341,7 +346,7 @@ struct kprobe *get_kprobe(void *addr);
> void kretprobe_hash_lock(struct task_struct *tsk,
> struct hlist_head **head, unsigned long *flags);
> void kretprobe_hash_unlock(struct task_struct *tsk, unsigned long *flags);
> -struct hlist_head * kretprobe_inst_table_head(struct task_struct *tsk);
> +struct hlist_head *kretprobe_inst_table_head(struct task_struct *tsk);
>
> /* kprobe_running() will just return the current_kprobe on this CPU */
> static inline struct kprobe *kprobe_running(void)
> @@ -371,6 +376,9 @@ void unregister_kretprobe(struct kretprobe *rp);
> int register_kretprobes(struct kretprobe **rps, int num);
> void unregister_kretprobes(struct kretprobe **rps, int num);
>
> +unsigned long kretprobe_ret_addr(struct task_struct *tsk, unsigned long ret,
> + unsigned long *retp);
> +
> void kprobe_flush_task(struct task_struct *tk);
> void recycle_rp_inst(struct kretprobe_instance *ri, struct hlist_head *head);
>
> @@ -425,6 +433,11 @@ static inline void unregister_kretprobe(struct kretprobe *rp)
> static inline void unregister_kretprobes(struct kretprobe **rps, int num)
> {
> }
> +unsigned long kretprobe_ret_addr(struct task_struct *task, unsigned long ret,
> + unsigned long *retp)
> +{
> + return ret;
> +}
> static inline void kprobe_flush_task(struct task_struct *tk)
> {
> }
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 90e98e233647..ed78141664ec 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -83,6 +83,11 @@ static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash)
> return &(kretprobe_table_locks[hash].lock);
> }
>
> +struct hlist_head *kretprobe_inst_table_head(struct task_struct *tsk)
> +{
> + return &kretprobe_inst_table[hash_ptr(tsk, KPROBE_HASH_BITS)];
> +}
> +
> /* Blacklist -- list of struct kprobe_blacklist_entry */
> static LIST_HEAD(kprobe_blacklist);
>
> @@ -1206,6 +1211,15 @@ __releases(hlist_lock)
> }
> NOKPROBE_SYMBOL(kretprobe_table_unlock);
>
> +static bool kretprobe_hash_is_locked(struct task_struct *tsk)
> +{
> + unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
> + raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
> +
> + return raw_spin_is_locked(hlist_lock);
> +}
> +NOKPROBE_SYMBOL(kretprobe_hash_is_locked);
> +
> /*
> * This function is called from finish_task_switch when task tk becomes dead,
> * so that we can recycle any function-return probe instances associated
> @@ -1856,6 +1870,41 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> }
> NOKPROBE_SYMBOL(pre_handler_kretprobe);
>
> +unsigned long kretprobe_ret_addr(struct task_struct *tsk, unsigned long ret,
> + unsigned long *retp)
> +{
> + struct kretprobe_instance *ri;
> + unsigned long flags = 0;
> + struct hlist_head *head;
> + bool need_lock;
> +
> + if (likely(ret != (unsigned long) &kretprobe_trampoline))
> + return ret;
> +
> + need_lock = !kretprobe_hash_is_locked(tsk);
> + if (WARN_ON(need_lock))
> + kretprobe_hash_lock(tsk, &head, &flags);
> + else
> + head = kretprobe_inst_table_head(tsk);
This may not work unless this is called from the kretprobe handler context,
since if we are out of kretprobe handler context, another CPU can lock the
hash table and it can be detected by kretprobe_hash_is_locked();.
So, we should check we are in the kretprobe handler context if tsk == current,
if not, we definately can lock the hash lock without any warning. This can
be something like;
if (is_kretprobe_handler_context()) {
// kretprobe_hash_lock(current == tsk) has been locked by caller
if (tsk != current && kretprobe_hash(tsk) != kretprobe_hash(current))
// the hash of tsk and current can be same.
need_lock = true;
} else
// we should take a lock for tsk.
need_lock = true;
Thank you,
> +
> + hlist_for_each_entry(ri, head, hlist) {
> + if (ri->task != current)
> + continue;
> + if (ri->ret_addr == (kprobe_opcode_t *) &kretprobe_trampoline)
> + continue;
> + if (ri->ret_addrp == (kprobe_opcode_t **) retp) {
> + ret = (unsigned long) ri->ret_addr;
> + goto out;
> + }
> + }
> +
> +out:
> + if (need_lock)
> + kretprobe_hash_unlock(tsk, &flags);
> + return ret;
> +}
> +NOKPROBE_SYMBOL(kretprobe_ret_addr);
> +
> bool __weak arch_kprobe_on_func_entry(unsigned long offset)
> {
> return !offset;
> @@ -2005,6 +2054,12 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> }
> NOKPROBE_SYMBOL(pre_handler_kretprobe);
>
> +unsigned long kretprobe_ret_addr(struct task_struct *tsk, unsigned long ret,
> + unsigned long *retp)
> +{
> + return ret;
> +}
> +NOKPROBE_SYMBOL(kretprobe_ret_addr);
> #endif /* CONFIG_KRETPROBES */
>
> /* Set the kprobe gone and remove its instruction buffer. */
> --
> 2.19.1
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCH v3 1/2] kretprobe: produce sane stack traces
From: Masami Hiramatsu @ 2018-11-09 7:26 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Aleksa Sarai, Steven Rostedt, Naveen N. Rao, Anil S Keshavamurthy,
David S. Miller, Masami Hiramatsu, Jonathan Corbet,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Shuah Khan,
Alexei Starovoitov, Daniel Borkmann, Brendan Gregg,
Christian Brauner, Aleksa Sarai, netd
In-Reply-To: <20181108144437.l3proaovfqm5osnr@treble>
On Thu, 8 Nov 2018 08:44:37 -0600
Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Thu, Nov 08, 2018 at 07:04:48PM +1100, Aleksa Sarai wrote:
> > On 2018-11-08, Aleksa Sarai <cyphar@cyphar.com> wrote:
> > > I will attach what I have at the moment to hopefully explain what the
> > > issue I've found is (re-using the kretprobe architecture but with the
> > > shadow-stack idea).
> >
> > Here is the patch I have at the moment (it works, except for the
> > question I have about how to handle the top-level pt_regs -- I've marked
> > that code with XXX).
> >
> > --
> > Aleksa Sarai
> > Senior Software Engineer (Containers)
> > SUSE Linux GmbH
> > <https://www.cyphar.com/>
> >
> > --8<---------------------------------------------------------------------
> >
> > Since the return address is modified by kretprobe, the various unwinders
> > can produce invalid and confusing stack traces. ftrace mostly solved
> > this problem by teaching each unwinder how to find the original return
> > address for stack trace purposes. This same technique can be applied to
> > kretprobes by simply adding a pointer to where the return address was
> > replaced in the stack, and then looking up the relevant
> > kretprobe_instance when a stack trace is requested.
> >
> > [WIP: This is currently broken because the *first entry* will not be
> > overwritten since it looks like the stack pointer is different
> > when we are provided pt_regs. All other addresses are correctly
> > handled.]
>
> When you see this problem, what does regs->ip point to? If it's
> pointing to generated code, then we don't _currently_ have a way of
> dealing with that. If it's pointing to a real function, we can fix that
> with unwind hints.
As I replied, If the stackdump is called from kretprobe event, regs->ip
always points trampoline function. Otherwise (maybe from kprobe event,
or panic, BUG etc.) it always be the address which the event occurs.
So fixing regs->ip is correct.
Thank you,
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
From: Jakub Kicinski @ 2018-11-08 21:51 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Quentin Monnet, Stanislav Fomichev, netdev, linux-kselftest, ast,
daniel, shuah, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181108212012.wijumgzknr5b7gdx@mini-arch>
On Thu, 8 Nov 2018 13:20:12 -0800, Stanislav Fomichev wrote:
> On 11/08, Jakub Kicinski wrote:
> > On Thu, 8 Nov 2018 18:21:24 +0000, Quentin Monnet wrote:
> > > >>> @@ -79,8 +82,11 @@ DESCRIPTION
> > > >>> contain a dot character ('.'), which is reserved for future
> > > >>> extensions of *bpffs*.
> > > >>> - **bpftool prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> > > >>> + **bpftool prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> > > >>> Load bpf program from binary *OBJ* and pin as *FILE*.
> > > >>> + **bpftool prog load** will pin only the first bpf program
> > > >>> + from the *OBJ*, **bpftool prog loadall** will pin all maps
> > > >>> + and programs from the *OBJ*.
> > > >>
> > > >> This could be improved regarding maps: with "bpftool prog load" I think we
> > > >> also load and pin all maps, but your description implies this is only the
> > > >> case with "loadall"
> > > > I don't think we pin any maps with `bpftool prog load`, we certainly load
> > > > them, but we don't pin any afaict. Can you point me to the code where we
> > > > pin the maps?
> > > >
> > >
> > > My bad. I read "pin" but thought "load". It does not pin them indeed,
> > > sorry about that.
> >
> > Right, but I don't see much reason why prog loadall should pin maps.
> It does seem convenient to have an option to pin everything, so we
> don't require anything else to find the ids of the maps.
> If we are pinning all progs, might as well pin the maps, why not? See
> my example in the commit message, for example, where I just hard code
> the expected map name. Convenient :-)
Sure. I can see how its convenient to your use case. A lot of people
will be very used to finding out the maps because if program is loaded
with iproute2 tools neither programs nor maps get pinned.
Please add a "pinmaps MAP_DIR" optional parameter as a separate patch.
> > The reason to pin program(s) is to hold some reference and to be able
> > to find them. Since we have the programs pinned we should be able to
> > find their maps with relative ease.
> >
> > $ bpftool prog show pinned /sys/fs/bpf/prog
> > 7: cgroup_skb tag 2a142ef67aaad174 gpl
> > loaded_at 2018-11-08T11:02:25-0800 uid 0
> > xlated 296B jited 229B memlock 4096B map_ids 6,7
> >
> > possibly:
> >
> > $ bpftool -j prog show pinned /sys/fs/bpf/prog | jq '.map_ids[0]'
> > 6
> >
> > Moreover, I think program and map names may collide making ELFs
> > unloadable..
> It doesn't sound like a big problem, sounds like a constraint we can live
> with.
^ permalink raw reply
* Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
From: Jakub Kicinski @ 2018-11-08 21:52 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Quentin Monnet, Stanislav Fomichev, netdev, linux-kselftest, ast,
daniel, shuah, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181108212539.esqjw4k5hz2pxowu@mini-arch>
On Thu, 8 Nov 2018 13:25:39 -0800, Stanislav Fomichev wrote:
> > > > + goto err_close_obj;
> > > > + }
> > > > +
> > > > const char *sec_name = bpf_program__title(prog, false);
> > > >
> > > > err = libbpf_prog_type_by_name(sec_name, &attr.prog_type,
> > > > @@ -936,8 +958,13 @@ static int do_load(int argc, char **argv)
> > > > goto err_close_obj;
> > > > }
> > > > }
> > > > - bpf_program__set_type(prog, attr.prog_type);
> > > > - bpf_program__set_expected_attach_type(prog, expected_attach_type);
> > > > +
> > > > + bpf_object__for_each_program(pos, obj) {
> > > > + bpf_program__set_ifindex(pos, ifindex);
> > > > + bpf_program__set_type(pos, attr.prog_type);
> > > > + bpf_program__set_expected_attach_type(pos,
> > > > + expected_attach_type);
> > > > + }
> > >
> > > I still believe you can have programs of different types here, and be
> > > able to load them. I tried it and managed to have it working fine. If no
> > > type is provided from command line we can retrieve types for each
> > > program from its section name. If a type is provided on the command
> > > line, we can do the same, but I am not sure we should do it, or impose
> > > that type for all programs instead.
> >
> > attr->prog_type is one per object, though. How do we set that one?
> Isn't it used only in __bpf_object__open_xattr to require/not-require kernel
> version in the bpf prog?
>
> It will probably work quite nicely for both of our options:
>
> * type not specified: prog_type = BPF_PROG_TYPE_UNSPEC, need kernel
> version (over cautious?)
> * type specified (and applied to all progs): using bpf_prog_type__needs_kver
> to require/not requqire kernel version
Right, but they you can't infer it from the program name, since there's
multiple.
^ permalink raw reply
* Re: [net-next 06/12] i40e/ixgbe/igb: fail on new WoL flag setting WAKE_MAGICSECURE
From: Jeff Kirsher @ 2018-11-08 21:53 UTC (permalink / raw)
To: Michal Kubecek, Kevin Easton
Cc: davem, Todd Fujinaka, netdev, nhorman, sassmann
In-Reply-To: <20181108064250.GB24800@unicorn.suse.cz>
[-- Attachment #1: Type: text/plain, Size: 4488 bytes --]
On Thu, 2018-11-08 at 07:42 +0100, Michal Kubecek wrote:
> On Thu, Nov 08, 2018 at 06:05:26AM +0000, Kevin Easton wrote:
> > On Wed, Nov 07, 2018 at 02:48:24PM -0800, Jeff Kirsher wrote:
> > > From: Todd Fujinaka <todd.fujinaka@intel.com>
> > >
> > > There's a new flag for setting WoL filters that is only
> > > enabled on one manufacturer's NICs, and it's not ours. Fail
> > > with EOPNOTSUPP.
> > >
> > > Signed-off-by: Todd Fujinaka <todd.fujinaka@intel.com>
> > > Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> > > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > > ---
> > > drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 3 ++-
> > > drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 +-
> > > drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 3 ++-
> > > 3 files changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > > b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > > index 9f8464f80783..9c1211ad2c6b 100644
> > > --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > > +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > > @@ -2377,7 +2377,8 @@ static int i40e_set_wol(struct net_device
> > > *netdev, struct ethtool_wolinfo *wol)
> > > return -EOPNOTSUPP;
> > >
> > > /* only magic packet is supported */
> > > - if (wol->wolopts && (wol->wolopts != WAKE_MAGIC))
> > > + if (wol->wolopts && (wol->wolopts != WAKE_MAGIC)
> > > + | (wol->wolopts != WAKE_FILTER))
> > > return -EOPNOTSUPP;
> >
> > This doesn't look right. WAKE_MAGIC and WAKE_FILTER are distinct, so
> >
> > (wol->wolopts != WAKE_MAGIC) | (wol->wolopts != WAKE_FILTER)
> >
> > will always be 1.
>
> Right. Also, using "|" with logical values is rather confusing. While
> the result works as expected, its priority is higher than priority of
> && (which would not be true for ||), making the code counterintuitive.
>
> BtW, the patch subject is also wrong, the newly added flag it is dealing
> with is WAKE_FILTER, not WAKE_MAGICSECURE.
After looking into this this more, this does appear to be incorrect. The
author of this change is currently out on vacation, so once he gets back, I
will address both Kevin's and your concerns.
>
> > It looks like the existing test in this driver was fine - it *only*
> > accepted wol->wolopts of either 0 or WAKE_MAGIC, it was already
> > rejecting everything else including WAKE_FILTER.
>
> Another way to write the check would be
>
> if (wol->wolopts & ~WAKE_MAGIC)
>
> > > diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > index 5acf3b743876..c57671068245 100644
> > > --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > @@ -2113,7 +2113,7 @@ static int igb_set_wol(struct net_device
> > > *netdev, struct ethtool_wolinfo *wol)
> > > {
> > > struct igb_adapter *adapter = netdev_priv(netdev);
> > >
> > > - if (wol->wolopts & (WAKE_ARP | WAKE_MAGICSECURE))
> > > + if (wol->wolopts & (WAKE_ARP | WAKE_MAGICSECURE | WAKE_FILTER))
> > > return -EOPNOTSUPP;
> > >
> > > if (!(adapter->flags & IGB_FLAG_WOL_SUPPORTED))
>
> I would also suggest taking the opposite approach here, i.e. listing the
> flags which _are_ supported so that we don't have to update the code if
> another wol flag is added (or userspace sends an invalid one):
>
> #define SUPPORTED_WOL_MODES \
> (WAKE_PHY | WAKE_UCAST | WAKE_MCAST | WAKE_BCAST | WAKE_MAGIC)
> ...
> if (wol->wolopts & ~SUPPORTED_WOL_MODES)
> return -EOPNOTSUPP;
>
>
> > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> > > b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> > > index 732b1e6ecc43..acba067cc15a 100644
> > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> > > @@ -2206,7 +2206,8 @@ static int ixgbe_set_wol(struct net_device
> > > *netdev, struct ethtool_wolinfo *wol)
> > > {
> > > struct ixgbe_adapter *adapter = netdev_priv(netdev);
> > >
> > > - if (wol->wolopts & (WAKE_PHY | WAKE_ARP | WAKE_MAGICSECURE))
> > > + if (wol->wolopts & (WAKE_PHY | WAKE_ARP | WAKE_MAGICSECURE |
> > > + WAKE_FILTER))
> > > return -EOPNOTSUPP;
> > >
> > > if (ixgbe_wol_exclusion(adapter, wol))
>
> ...and here.
>
> Michal Kubecek
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
From: Jakub Kicinski @ 2018-11-08 21:54 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Stanislav Fomichev, netdev, linux-kselftest, ast, daniel, shuah,
quentin.monnet, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181108212940.wvfnkanxqs2owcy6@mini-arch>
On Thu, 8 Nov 2018 13:29:40 -0800, Stanislav Fomichev wrote:
> On 11/08, Jakub Kicinski wrote:
> > On Wed, 7 Nov 2018 21:39:57 -0800, Stanislav Fomichev wrote:
> > > This commit adds support for loading/attaching/detaching flow
> > > dissector program. The structure of the flow dissector program is
> > > assumed to be the same as in the selftests:
> > >
> > > * flow_dissector section with the main entry point
> > > * a bunch of tail call progs
> > > * a jmp_table map that is populated with the tail call progs
> >
> > Could you split the loadall changes and the flow_dissector changes into
> > two separate patches?
> Sure, will do, but let's first agree on the semantical differences of
> load vs loadall.
>
> So far *load* actually loads _all_ progs (via bpf_object__load), but pins
> only the first program. Is that what we want? I wonder whether the
> assumption there was that there is only single program in the object.
> Should we load only the first program in *load*?
>
> If we add *loadall*, then the difference would be:
> *load*:
> * loads all maps and only the first program, pins only the first
> program
> *loadall*:
> * loads all maps and all programs, pins everything (maps and programs)
>
> Is this the expected behavior?
Loading all programs and maps for "load" is just a libbpf limitation we
can remove at some point.
^ permalink raw reply
* [PATCH net-next 0/2] net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt
From: Heiner Kallweit @ 2018-11-08 21:54 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev@vger.kernel.org
Flag PHY_HAS_INTERRUPT is used only here for this small check. I think
using interrupts isn't possible if a driver defines neither
config_intr nor ack_interrupts callback. So we can replace checking
flag PHY_HAS_INTERRUPT with checking for these callbacks.
This allows to remove this flag from a lot of driver configs, let's
start with the Realtek driver.
Heiner Kallweit (2):
net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt
net: phy: realtek: remove flag PHY_HAS_INTERRUPT from driver configs
drivers/net/phy/phy_device.c | 5 +++--
drivers/net/phy/realtek.c | 7 -------
2 files changed, 3 insertions(+), 9 deletions(-)
--
2.19.1
^ permalink raw reply
* [PATCH net-next 1/2] net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt
From: Heiner Kallweit @ 2018-11-08 21:55 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <193e3970-8ce2-1221-357a-7b7f9f6aea76@gmail.com>
Flag PHY_HAS_INTERRUPT is used only here for this small check. I think
using interrupts isn't possible if a driver defines neither
config_intr nor ack_interrupts callback. So we can replace checking
flag PHY_HAS_INTERRUPT with checking for these callbacks.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/phy_device.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index d165a2c82..33e51b955 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2104,8 +2104,9 @@ static int phy_probe(struct device *dev)
/* Disable the interrupt if the PHY doesn't support it
* but the interrupt is still a valid one
*/
- if (!(phydrv->flags & PHY_HAS_INTERRUPT) &&
- phy_interrupt_is_valid(phydev))
+ if (!phydrv->config_intr &&
+ !phydrv->ack_interrupt &&
+ phy_interrupt_is_valid(phydev))
phydev->irq = PHY_POLL;
if (phydrv->flags & PHY_IS_INTERNAL)
--
2.19.1
^ permalink raw reply related
* [PATCH net-next 2/2] net: phy: realtek: remove flag PHY_HAS_INTERRUPT from driver configs
From: Heiner Kallweit @ 2018-11-08 21:58 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <193e3970-8ce2-1221-357a-7b7f9f6aea76@gmail.com>
Now that flag PHY_HAS_INTERRUPT has been replaced with a check for
callbacks config_intr and ack_interrupt, we can remove setting this
flag from driver configs.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/realtek.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index abff4cdc9..3985b4a4d 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -216,12 +216,10 @@ static struct phy_driver realtek_drvs[] = {
.phy_id = 0x00008201,
.name = "RTL8201CP Ethernet",
.features = PHY_BASIC_FEATURES,
- .flags = PHY_HAS_INTERRUPT,
}, {
.phy_id = 0x001cc816,
.name = "RTL8201F Fast Ethernet",
.features = PHY_BASIC_FEATURES,
- .flags = PHY_HAS_INTERRUPT,
.ack_interrupt = &rtl8201_ack_interrupt,
.config_intr = &rtl8201_config_intr,
.suspend = genphy_suspend,
@@ -239,7 +237,6 @@ static struct phy_driver realtek_drvs[] = {
.phy_id = 0x001cc912,
.name = "RTL8211B Gigabit Ethernet",
.features = PHY_GBIT_FEATURES,
- .flags = PHY_HAS_INTERRUPT,
.ack_interrupt = &rtl821x_ack_interrupt,
.config_intr = &rtl8211b_config_intr,
.read_mmd = &genphy_read_mmd_unsupported,
@@ -257,7 +254,6 @@ static struct phy_driver realtek_drvs[] = {
.phy_id = 0x001cc914,
.name = "RTL8211DN Gigabit Ethernet",
.features = PHY_GBIT_FEATURES,
- .flags = PHY_HAS_INTERRUPT,
.ack_interrupt = rtl821x_ack_interrupt,
.config_intr = rtl8211e_config_intr,
.suspend = genphy_suspend,
@@ -266,7 +262,6 @@ static struct phy_driver realtek_drvs[] = {
.phy_id = 0x001cc915,
.name = "RTL8211E Gigabit Ethernet",
.features = PHY_GBIT_FEATURES,
- .flags = PHY_HAS_INTERRUPT,
.ack_interrupt = &rtl821x_ack_interrupt,
.config_intr = &rtl8211e_config_intr,
.suspend = genphy_suspend,
@@ -275,7 +270,6 @@ static struct phy_driver realtek_drvs[] = {
.phy_id = 0x001cc916,
.name = "RTL8211F Gigabit Ethernet",
.features = PHY_GBIT_FEATURES,
- .flags = PHY_HAS_INTERRUPT,
.config_init = &rtl8211f_config_init,
.ack_interrupt = &rtl8211f_ack_interrupt,
.config_intr = &rtl8211f_config_intr,
@@ -287,7 +281,6 @@ static struct phy_driver realtek_drvs[] = {
.phy_id = 0x001cc961,
.name = "RTL8366RB Gigabit Ethernet",
.features = PHY_GBIT_FEATURES,
- .flags = PHY_HAS_INTERRUPT,
.config_init = &rtl8366rb_config_init,
.suspend = genphy_suspend,
.resume = genphy_resume,
--
2.19.1
^ permalink raw reply related
* [Patch net] ip: fix csum_sub() with csum_block_sub()
From: Cong Wang @ 2018-11-08 22:04 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, Paolo Abeni, Eric Dumazet, Willem de Bruijn
When subtracting the checksum of a block of data,
csum_block_sub() must be used to respect the offset.
We learned this lesson from both commit d55bef5059dd
("net: fix pskb_trim_rcsum_slow() with odd trim offset") and
commit d48051c5b837 ("net/mlx5e: fix csum adjustments caused by RXFCS").
Fixes: ca4ef4574f1e ("ip: fix IP_CHECKSUM handling")
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Willem de Bruijn <willemb@google.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/ipv4/ip_sockglue.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index fffcc130900e..0d69f0823c08 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -122,7 +122,10 @@ static void ip_cmsg_recv_checksum(struct msghdr *msg, struct sk_buff *skb,
if (offset != 0) {
int tend_off = skb_transport_offset(skb) + tlen;
- csum = csum_sub(csum, skb_checksum(skb, tend_off, offset, 0));
+
+ csum = csum_block_sub(csum,
+ skb_checksum(skb, tend_off, offset, 0),
+ tend_off);
}
put_cmsg(msg, SOL_IP, IP_CHECKSUM, sizeof(__wsum), &csum);
--
2.19.1
^ permalink raw reply related
* [Patch net-next v2] net: move __skb_checksum_complete*() to skbuff.c
From: Cong Wang @ 2018-11-08 22:05 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, Stefano Brivio
__skb_checksum_complete_head() and __skb_checksum_complete()
are both declared in skbuff.h, they fit better in skbuff.c
than datagram.c.
Cc: Stefano Brivio <sbrivio@redhat.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/core/datagram.c | 43 -------------------------------------------
net/core/skbuff.c | 43 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 43 insertions(+), 43 deletions(-)
diff --git a/net/core/datagram.c b/net/core/datagram.c
index 57f3a6fcfc1e..07983b90d2bd 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c
@@ -728,49 +728,6 @@ static int skb_copy_and_csum_datagram(const struct sk_buff *skb, int offset,
return -EFAULT;
}
-__sum16 __skb_checksum_complete_head(struct sk_buff *skb, int len)
-{
- __sum16 sum;
-
- sum = csum_fold(skb_checksum(skb, 0, len, skb->csum));
- if (likely(!sum)) {
- if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE) &&
- !skb->csum_complete_sw)
- netdev_rx_csum_fault(skb->dev);
- }
- if (!skb_shared(skb))
- skb->csum_valid = !sum;
- return sum;
-}
-EXPORT_SYMBOL(__skb_checksum_complete_head);
-
-__sum16 __skb_checksum_complete(struct sk_buff *skb)
-{
- __wsum csum;
- __sum16 sum;
-
- csum = skb_checksum(skb, 0, skb->len, 0);
-
- /* skb->csum holds pseudo checksum */
- sum = csum_fold(csum_add(skb->csum, csum));
- if (likely(!sum)) {
- if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE) &&
- !skb->csum_complete_sw)
- netdev_rx_csum_fault(skb->dev);
- }
-
- if (!skb_shared(skb)) {
- /* Save full packet checksum */
- skb->csum = csum;
- skb->ip_summed = CHECKSUM_COMPLETE;
- skb->csum_complete_sw = 1;
- skb->csum_valid = !sum;
- }
-
- return sum;
-}
-EXPORT_SYMBOL(__skb_checksum_complete);
-
/**
* skb_copy_and_csum_datagram_msg - Copy and checksum skb to user iovec.
* @skb: skbuff
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index b4ee5c8b928f..5cb4b3440153 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2645,6 +2645,49 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
}
EXPORT_SYMBOL(skb_copy_and_csum_bits);
+__sum16 __skb_checksum_complete_head(struct sk_buff *skb, int len)
+{
+ __sum16 sum;
+
+ sum = csum_fold(skb_checksum(skb, 0, len, skb->csum));
+ if (likely(!sum)) {
+ if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE) &&
+ !skb->csum_complete_sw)
+ netdev_rx_csum_fault(skb->dev);
+ }
+ if (!skb_shared(skb))
+ skb->csum_valid = !sum;
+ return sum;
+}
+EXPORT_SYMBOL(__skb_checksum_complete_head);
+
+__sum16 __skb_checksum_complete(struct sk_buff *skb)
+{
+ __wsum csum;
+ __sum16 sum;
+
+ csum = skb_checksum(skb, 0, skb->len, 0);
+
+ /* skb->csum holds pseudo checksum */
+ sum = csum_fold(csum_add(skb->csum, csum));
+ if (likely(!sum)) {
+ if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE) &&
+ !skb->csum_complete_sw)
+ netdev_rx_csum_fault(skb->dev);
+ }
+
+ if (!skb_shared(skb)) {
+ /* Save full packet checksum */
+ skb->csum = csum;
+ skb->ip_summed = CHECKSUM_COMPLETE;
+ skb->csum_complete_sw = 1;
+ skb->csum_valid = !sum;
+ }
+
+ return sum;
+}
+EXPORT_SYMBOL(__skb_checksum_complete);
+
static __wsum warn_crc32c_csum_update(const void *buff, int len, __wsum sum)
{
net_warn_ratelimited(
--
2.19.1
^ permalink raw reply related
* Re: [PATCH net-next 0/4] Remove VLAN_TAG_PRESENT from drivers
From: Michał Mirosław @ 2018-11-08 22:11 UTC (permalink / raw)
To: Leon Romanovsky
Cc: netdev, Claudiu Manoil, Faisal Latif, Pravin B Shelar,
Shiraz Saleem, dev, linux-rdma
In-Reply-To: <20181108185005.GR3695@mtr-leonro.mtl.com>
On Thu, Nov 08, 2018 at 08:50:05PM +0200, Leon Romanovsky wrote:
> On Thu, Nov 08, 2018 at 06:44:46PM +0100, Michał Mirosław wrote:
> > This series removes VLAN_TAG_PRESENT use from network drivers in
> > preparation to removing its special meaning.
> Can you please give an extra explanation why it is removed?
> Such series come out-of-blue, for people who are not following
> netdev mailing list closely (drivers/infiniband/*).
This is one of the steps to remove VLAN_TAG_PRESENT overlap with CFI/DEI
bit of VLAN tag. Currently this overlap causes Linux kernel to always
clear CFI/DEI in packets.
There is skb_vlan_tag_present() that drivers should use to check if
the tag in skb is valid.
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: [PATCH 3/3] iwlwifi: Load firmware exclusively for Intel WiFi
From: Marcel Holtmann @ 2018-11-09 7:49 UTC (permalink / raw)
To: João Paulo Rechi Vita
Cc: Kai-Heng Feng, Luca Coelho, Kalle Valo, Emmanuel Grumbach,
Johannes Berg, David S. Miller, Intel Linux Wireless,
open list:NFC SUBSYSTEM, netdev, linux-kernel, linux,
João Paulo Rechi Vita
In-Reply-To: <20181109000800.15431-1-jprvita@gmail.com>
Hi Joao Paulo,
>>>> I think Canonical were facing some wifi fw load error from some 8260
>>>> earlier module during the BT still loading the fw.
>>>> I believe we had later 8260 sku that fixed this issue.
>>>
>>> But there are already 8260 that is affected by this bug in the wild.
>>>
>>> Search "Bluetooth: hci0: Failed to send firmware data (-38)” and there are lots of user are affected.
>>
>> which SKUs are these actually. What are the initial details about the boot loader. For the Bluetooth side, you should be able to grab them from dmesg or by running btmon.
>>
>> So I am not in favor of this kind of hack and creating dependencies between drivers. If you only have a hammer, then everything looks like a nail. And this is a massive hammer trying to squash everything. This problem needs to be debugged. And this starts by providing affected SKU information and firmware information. So get the details about the SKU and its Bluetooth and WiFi boot loaders.
>>
>
> I have a Lenovo Yoga 900 which presents this problem and has the same bootloader / firmware information as Kai-Heng already posted:
>
> [ 5.992426] Bluetooth: Core ver 2.22
> [ 5.992438] Bluetooth: HCI device and connection manager initialized
> [ 5.992442] Bluetooth: HCI socket layer initialized
> [ 5.992444] Bluetooth: L2CAP socket layer initialized
> [ 5.992450] Bluetooth: SCO socket layer initialized
> [ 6.004941] Bluetooth: hci0: Bootloader revision 0.0 build 2 week 52 2014
> [ 6.010922] Bluetooth: hci0: Device revision is 5
> [ 6.010923] Bluetooth: hci0: Secure boot is enabled
> [ 6.010924] Bluetooth: hci0: OTP lock is enabled
> [ 6.010925] Bluetooth: hci0: API lock is enabled
> [ 6.010926] Bluetooth: hci0: Debug lock is disabled
> [ 6.010927] Bluetooth: hci0: Minimum firmware build 1 week 10 2014
> [ 6.014253] bluetooth hci0: firmware: direct-loading firmware intel/ibt-11-5.sfi
> [ 6.014256] Bluetooth: hci0: Found device firmware: intel/ibt-11-5.sfi
> [ 6.613961] Bluetooth: BNEP (Ethernet Emulation) ver 1.3
> [ 6.613966] Bluetooth: BNEP filters: protocol multicast
> [ 6.613974] Bluetooth: BNEP socket layer initialized
> [ 6.983804] Bluetooth: hci0: Failed to send firmware data (-38)
>
> And the following product id and revision, from usb-devices:
>
> T: Bus=01 Lev=01 Prnt=01 Port=06 Cnt=02 Dev#= 3 Spd=12 MxCh= 0
> D: Ver= 2.00 Cls=e0(wlcon) Sub=01 Prot=01 MxPS=64 #Cfgs= 1
> P: Vendor=8087 ProdID=0a2b Rev=00.01
> C: #Ifs= 2 Cfg#= 1 Atr=e0 MxPwr=100mA
> I: If#= 0 Alt= 0 #EPs= 3 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
> I: If#= 1 Alt= 0 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
>
> I understand the drawbacks with the approach presented here and lack of clear explanation of the problem, but I can confirm these patches work around the problem on my machine. Is there any extra info or test result I can provide to help debug this? I can also dedicate time to help write a different solution if some guidance is provided.
>
> Kai-Heng, did you end up filling a Bugzilla entry for this?
>
> Please CC-me on the replies as I'm not receiving emails from linux-bluetooth or linux-wireless anymore.
our hardware teams from the Bluetooth and WiFi side really need to look at this. An inter-dependency between the firmware loading of two otherwise independent drivers is really not what I want to see upstream. However I have no idea which boot loaders are affected or if this is something that might be even possible to be fixed in operational firmware. If you can create a binary btmon trace file with the error happening and getting really every single message from the boot we might get a bit further to understand where it goes wrong.
Regards
Marcel
^ permalink raw reply
* Re: [Patch net] ip: fix csum_sub() with csum_block_sub()
From: Eric Dumazet @ 2018-11-08 22:16 UTC (permalink / raw)
To: Cong Wang, Dimitris Michailidis; +Cc: netdev, Paolo Abeni, Willem de Bruijn
In-Reply-To: <20181108220407.17691-1-xiyou.wangcong@gmail.com>
On Thu, Nov 8, 2018 at 2:04 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> When subtracting the checksum of a block of data,
> csum_block_sub() must be used to respect the offset.
>
> We learned this lesson from both commit d55bef5059dd
> ("net: fix pskb_trim_rcsum_slow() with odd trim offset") and
> commit d48051c5b837 ("net/mlx5e: fix csum adjustments caused by RXFCS").
>
> Fixes: ca4ef4574f1e ("ip: fix IP_CHECKSUM handling")
I do not believe you fix any bug here...
I do not know of any inet protocol having odd header sizes.
^ permalink raw reply
* Re: [PATCH 4/5] phy: mvebu-cp110-comphy: convert to use eth phy mode and submode
From: Russell King - ARM Linux @ 2018-11-08 22:21 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, Kishon Vijay Abraham I, Alexandre Belloni,
Quentin Schulz, Manu Gautam, Tony Lindgren, netdev,
Antoine Tenart, Sekhar Nori, linux-kernel, Maxime Ripard,
Chen-Yu Tsai, Chunfeng Yun, linux-mediatek, Vivek Gautam,
Carlo Caione, linux-amlogic, linux-arm-kernel, Matthias Brugger
In-Reply-To: <20181108003617.10334-5-grygorii.strashko@ti.com>
On Wed, Nov 07, 2018 at 06:36:16PM -0600, Grygorii Strashko wrote:
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 7a37a37..fb28b71 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -1165,28 +1165,17 @@ static void mvpp22_gop_setup_irq(struct mvpp2_port *port)
> */
> static int mvpp22_comphy_init(struct mvpp2_port *port)
> {
> - enum phy_mode mode;
> + int submode;
> int ret;
>
> if (!port->comphy)
> return 0;
>
> - switch (port->phy_interface) {
> - case PHY_INTERFACE_MODE_SGMII:
> - case PHY_INTERFACE_MODE_1000BASEX:
> - mode = PHY_MODE_SGMII;
> - break;
> - case PHY_INTERFACE_MODE_2500BASEX:
> - mode = PHY_MODE_2500SGMII;
> - break;
> - case PHY_INTERFACE_MODE_10GKR:
> - mode = PHY_MODE_10GKR;
> - break;
> - default:
> - return -EINVAL;
> - }
> + submode = port->phy_interface;
> + if (submode == PHY_INTERFACE_MODE_1000BASEX)
> + submode = PHY_INTERFACE_MODE_SGMII;
If the intention is to move the ethernet PHY mode into the generic PHY
layer, I'd suggest that the equivalence of 1000base-X and SGMII is
handled in the generic PHY driver rather than ethernet drivers. Just
move this into this hunk of the comphy driver:
> @@ -517,10 +519,14 @@ static int mvebu_comphy_set_mode(struct phy *phy,
> {
> struct mvebu_comphy_lane *lane = phy_get_drvdata(phy);
>
> - if (mvebu_comphy_get_mux(lane->id, lane->port, mode) < 0)
> + if (mode != PHY_MODE_ETHERNET)
> + return -EINVAL;
> +
> + if (mvebu_comphy_get_mux(lane->id, lane->port, mode, submode) < 0)
> return -EINVAL;
>
> lane->mode = mode;
> + lane->submode = submode;
> return 0;
> }
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [Patch net] ip: fix csum_sub() with csum_block_sub()
From: Cong Wang @ 2018-11-08 22:22 UTC (permalink / raw)
To: Eric Dumazet
Cc: Dimitris Michailidis, Linux Kernel Network Developers,
Paolo Abeni, Willem de Bruijn
In-Reply-To: <CANn89iLqG5bEsJPfeNsm51P6UEkJOu4j0hFFODTHb4W+vqAe=w@mail.gmail.com>
On Thu, Nov 8, 2018 at 2:17 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Nov 8, 2018 at 2:04 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > When subtracting the checksum of a block of data,
> > csum_block_sub() must be used to respect the offset.
> >
> > We learned this lesson from both commit d55bef5059dd
> > ("net: fix pskb_trim_rcsum_slow() with odd trim offset") and
> > commit d48051c5b837 ("net/mlx5e: fix csum adjustments caused by RXFCS").
> >
> > Fixes: ca4ef4574f1e ("ip: fix IP_CHECKSUM handling")
>
> I do not believe you fix any bug here...
>
> I do not know of any inet protocol having odd header sizes.
That offset is payload offset, but yeah, the payload offset must be
aligned some way. Good point!
Let's drop it.
^ permalink raw reply
* Re: [PATCH 04/20] octeontx2-af: NPC MCAM entry alloc/free support
From: David Miller @ 2018-11-08 22:22 UTC (permalink / raw)
To: sunil.kovvuri; +Cc: netdev, arnd, linux-soc, sgoutham
In-Reply-To: <1541702161-30673-5-git-send-email-sunil.kovvuri@gmail.com>
From: sunil.kovvuri@gmail.com
Date: Fri, 9 Nov 2018 00:05:45 +0530
> +int rvu_mbox_handler_NPC_MCAM_ALLOC_ENTRY(struct rvu *rvu,
> + struct npc_mcam_alloc_entry_req *req,
> + struct npc_mcam_alloc_entry_rsp *rsp);
> +int rvu_mbox_handler_NPC_MCAM_FREE_ENTRY(struct rvu *rvu,
> + struct npc_mcam_free_entry_req *req,
> + struct msg_rsp *rsp);
Please don't use capitalized letters or StUdLyCaPs for function and variable
names.
Keep capitalized letters for CPP macros.
This is how programmers visually can see if something is a real C declaration
or some CPP stuff.
^ permalink raw reply
* Re: [PATCH net-next] net: phy: improve struct phy_device member interrupts handling
From: Andrew Lunn @ 2018-11-08 22:24 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Florian Fainelli, David Miller, netdev@vger.kernel.org
In-Reply-To: <44805a11-3063-4f1f-2fd2-b268155d4935@gmail.com>
On Thu, Nov 08, 2018 at 10:36:33PM +0100, Heiner Kallweit wrote:
> As a heritage from the very early days of phylib member interrupts is
> defined as u32 even though it's just a flag whether interrupts are
> enabled. So we can change it to a bitfield member. In addition change
> the code dealing with this member in a way that it's clear we're
> dealing with a bool value.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> Actually this member isn't needed at all and could be replaced with
> a parameter in phy_driver->config_intr. But this would mean an API
> change, maybe I come up with a proposal later.
> ---
> drivers/net/phy/phy.c | 2 +-
> include/linux/phy.h | 10 +++++-----
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index dd5bff955..a5e6acfe6 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -115,7 +115,7 @@ static int phy_clear_interrupt(struct phy_device *phydev)
> *
> * Returns 0 on success or < 0 on error.
> */
> -static int phy_config_interrupt(struct phy_device *phydev, u32 interrupts)
> +static int phy_config_interrupt(struct phy_device *phydev, bool interrupts)
> {
> phydev->interrupts = interrupts;
> if (phydev->drv->config_intr)
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 5dd85c441..fc90af152 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -263,8 +263,8 @@ static inline struct mii_bus *devm_mdiobus_alloc(struct device *dev)
> void devm_mdiobus_free(struct device *dev, struct mii_bus *bus);
> struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
>
> -#define PHY_INTERRUPT_DISABLED 0x0
> -#define PHY_INTERRUPT_ENABLED 0x80000000
> +#define PHY_INTERRUPT_DISABLED 0
> +#define PHY_INTERRUPT_ENABLED 1
Hi Heiner
Since this is passed around as a bool, false/true would be better than
0/1.
Andrew
^ permalink raw reply
* Re: [PATCH net-next 0/2] net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt
From: Andrew Lunn @ 2018-11-08 22:30 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Florian Fainelli, David Miller, netdev@vger.kernel.org
In-Reply-To: <193e3970-8ce2-1221-357a-7b7f9f6aea76@gmail.com>
On Thu, Nov 08, 2018 at 10:54:50PM +0100, Heiner Kallweit wrote:
> Flag PHY_HAS_INTERRUPT is used only here for this small check. I think
> using interrupts isn't possible if a driver defines neither
> config_intr nor ack_interrupts callback. So we can replace checking
> flag PHY_HAS_INTERRUPT with checking for these callbacks.
>
> This allows to remove this flag from a lot of driver configs, let's
> start with the Realtek driver.
Hi Heiner
Ideally, we should do this to all the drivers, not just one. If we
leave PHY_HAS_INTERRUPT, people are going to use it. That then makes
the realtek driver then different to all the other drivers, and at
some point somebody will do something which breaks it because they
don't know the realtek driver is special.
Andrew
^ permalink raw reply
* Re: [PATCH net-next 1/2] net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt
From: Florian Fainelli @ 2018-11-08 22:33 UTC (permalink / raw)
To: Heiner Kallweit, Andrew Lunn, David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <1ad7d0ef-7e7e-93b2-3b3c-3c4704b3b0cc@gmail.com>
On 11/8/18 1:55 PM, Heiner Kallweit wrote:
> Flag PHY_HAS_INTERRUPT is used only here for this small check. I think
> using interrupts isn't possible if a driver defines neither
> config_intr nor ack_interrupts callback. So we can replace checking
> flag PHY_HAS_INTERRUPT with checking for these callbacks.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/net/phy/phy_device.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index d165a2c82..33e51b955 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -2104,8 +2104,9 @@ static int phy_probe(struct device *dev)
> /* Disable the interrupt if the PHY doesn't support it
> * but the interrupt is still a valid one
> */
> - if (!(phydrv->flags & PHY_HAS_INTERRUPT) &&
> - phy_interrupt_is_valid(phydev))
> + if (!phydrv->config_intr &&
> + !phydrv->ack_interrupt &&
> + phy_interrupt_is_valid(phydev))
> phydev->irq = PHY_POLL;
I would introduce an inline helper function which checks for
drv->config_intr and config_ack_interrupt, that way if we ever have to
introduce an additional function in the future, we just update the
helper to check for that.
Other than that, LGTM
--
Florian
^ permalink raw reply
* Re: [PATCH net-next 0/2] net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt
From: Florian Fainelli @ 2018-11-08 22:34 UTC (permalink / raw)
To: Andrew Lunn, Heiner Kallweit; +Cc: David Miller, netdev@vger.kernel.org
In-Reply-To: <20181108223028.GI5259@lunn.ch>
On 11/8/18 2:30 PM, Andrew Lunn wrote:
> On Thu, Nov 08, 2018 at 10:54:50PM +0100, Heiner Kallweit wrote:
>> Flag PHY_HAS_INTERRUPT is used only here for this small check. I think
>> using interrupts isn't possible if a driver defines neither
>> config_intr nor ack_interrupts callback. So we can replace checking
>> flag PHY_HAS_INTERRUPT with checking for these callbacks.
>>
>> This allows to remove this flag from a lot of driver configs, let's
>> start with the Realtek driver.
>
> Hi Heiner
>
> Ideally, we should do this to all the drivers, not just one. If we
> leave PHY_HAS_INTERRUPT, people are going to use it. That then makes
> the realtek driver then different to all the other drivers, and at
> some point somebody will do something which breaks it because they
> don't know the realtek driver is special.
Agreed.
--
Florian
^ permalink raw reply
* Re: [PATCH net-next] net: phy: improve struct phy_device member interrupts handling
From: Heiner Kallweit @ 2018-11-08 22:39 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Florian Fainelli, David Miller, netdev@vger.kernel.org
In-Reply-To: <20181108222436.GH5259@lunn.ch>
On 08.11.2018 23:24, Andrew Lunn wrote:
> On Thu, Nov 08, 2018 at 10:36:33PM +0100, Heiner Kallweit wrote:
>> As a heritage from the very early days of phylib member interrupts is
>> defined as u32 even though it's just a flag whether interrupts are
>> enabled. So we can change it to a bitfield member. In addition change
>> the code dealing with this member in a way that it's clear we're
>> dealing with a bool value.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> Actually this member isn't needed at all and could be replaced with
>> a parameter in phy_driver->config_intr. But this would mean an API
>> change, maybe I come up with a proposal later.
>> ---
>> drivers/net/phy/phy.c | 2 +-
>> include/linux/phy.h | 10 +++++-----
>> 2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index dd5bff955..a5e6acfe6 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -115,7 +115,7 @@ static int phy_clear_interrupt(struct phy_device *phydev)
>> *
>> * Returns 0 on success or < 0 on error.
>> */
>> -static int phy_config_interrupt(struct phy_device *phydev, u32 interrupts)
>> +static int phy_config_interrupt(struct phy_device *phydev, bool interrupts)
>> {
>> phydev->interrupts = interrupts;
>> if (phydev->drv->config_intr)
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index 5dd85c441..fc90af152 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -263,8 +263,8 @@ static inline struct mii_bus *devm_mdiobus_alloc(struct device *dev)
>> void devm_mdiobus_free(struct device *dev, struct mii_bus *bus);
>> struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
>>
>> -#define PHY_INTERRUPT_DISABLED 0x0
>> -#define PHY_INTERRUPT_ENABLED 0x80000000
>> +#define PHY_INTERRUPT_DISABLED 0
>> +#define PHY_INTERRUPT_ENABLED 1
>
> Hi Heiner
>
> Since this is passed around as a bool, false/true would be better than
> 0/1.
>
I thought about that before submitting the patch. I preferred to go with
0/1 because we assign this value to a 1 bit bitfield member which supports
values 0/1 only. So neither option is perfect IMO.
> Andrew
>
Heiner
^ permalink raw reply
* [PATCH net] net: mvneta: Don't advertise 2.5G modes
From: Maxime Chevallier @ 2018-11-09 8:17 UTC (permalink / raw)
To: davem
Cc: Maxime Chevallier, netdev, linux-kernel, Antoine Tenart,
thomas.petazzoni, gregory.clement, Andrew Lunn, Russell King,
linux-arm-kernel
Using 2.5G speed relies on the SerDes lanes being configured
accordingly. The lanes have to be reconfigured to switch between
1G and 2.5G, and for now only the bootloader does this configuration.
In the case we add a Comphy driver to handle switching the lanes
dynamically, it's better for now to stick with supporting only 1G and
add advertisement for 2.5G once we really are capable of handling both
speeds without problem.
Since the interface mode is initialy taken from the DT, we want to make
sure that adding comphy support won't break boards that don't update
their dtb.
Fixes: da58a931f248 ("net: mvneta: Add support for 2500Mbps SGMII")
Reported-by: Andrew Lunn <andrew@lunn.ch>
Reported-by: Russell King <linux@armlinux.org.uk>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
drivers/net/ethernet/marvell/mvneta.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 5bfd349bf41a..c19ecd153499 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -3343,7 +3343,6 @@ static void mvneta_validate(struct net_device *ndev, unsigned long *supported,
if (state->interface != PHY_INTERFACE_MODE_NA &&
state->interface != PHY_INTERFACE_MODE_QSGMII &&
state->interface != PHY_INTERFACE_MODE_SGMII &&
- state->interface != PHY_INTERFACE_MODE_2500BASEX &&
!phy_interface_mode_is_8023z(state->interface) &&
!phy_interface_mode_is_rgmii(state->interface)) {
bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
@@ -3357,14 +3356,9 @@ static void mvneta_validate(struct net_device *ndev, unsigned long *supported,
/* Asymmetric pause is unsupported */
phylink_set(mask, Pause);
- /* We cannot use 1Gbps when using the 2.5G interface. */
- if (state->interface == PHY_INTERFACE_MODE_2500BASEX) {
- phylink_set(mask, 2500baseT_Full);
- phylink_set(mask, 2500baseX_Full);
- } else {
- phylink_set(mask, 1000baseT_Full);
- phylink_set(mask, 1000baseX_Full);
- }
+ /* Half-duplex at speeds higher than 100Mbit is unsupported */
+ phylink_set(mask, 1000baseT_Full);
+ phylink_set(mask, 1000baseX_Full);
if (!phy_interface_mode_is_8023z(state->interface)) {
/* 10M and 100M are only supported in non-802.3z mode */
--
2.11.0
^ 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