* Re: [PATCH net-next] net: phy: mscc: the extended page access register is 16 bits
From: Alexandre Belloni @ 2018-07-30 14:13 UTC (permalink / raw)
To: Quentin Schulz
Cc: andrew, f.fainelli, davem, netdev, linux-kernel, thomas.petazzoni
In-Reply-To: <20180730125313.3405-1-quentin.schulz@bootlin.com>
On 30/07/2018 14:53:13+0200, Quentin Schulz wrote:
> The Extended Page Access is a 16-bit register, so change the page
> parameter of vsc85xx_phy_page_set to a u16.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
> drivers/net/phy/mscc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
> index 650c2667d523..84ca9ff40ae0 100644
> --- a/drivers/net/phy/mscc.c
> +++ b/drivers/net/phy/mscc.c
> @@ -123,7 +123,7 @@ static const struct vsc8531_edge_rate_table edge_table[] = {
> };
> #endif /* CONFIG_OF_MDIO */
>
> -static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
> +static int vsc85xx_phy_page_set(struct phy_device *phydev, u16 page)
> {
> int rc;
>
> --
> 2.17.1
>
--
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v5 bpf-next 7/9] xdp: Helpers for disabling napi_direct of xdp_return_frame
From: Jesper Dangaard Brouer @ 2018-07-30 12:33 UTC (permalink / raw)
To: Toshiaki Makita
Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Toshiaki Makita,
Jakub Kicinski, brouer
In-Reply-To: <20180726144032.2116-8-toshiaki.makita1@gmail.com>
On Thu, 26 Jul 2018 23:40:30 +0900
Toshiaki Makita <toshiaki.makita1@gmail.com> wrote:
> From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>
> We need some mechanism to disable napi_direct on calling
> xdp_return_frame_rx_napi() from some context.
> When veth gets support of XDP_REDIRECT, it will redirects packets which
> are redirected from other devices. On redirection veth will reuse
> xdp_mem_info of the redirection source device to make return_frame work.
> But in this case .ndo_xdp_xmit() called from veth redirection uses
> xdp_mem_info which is not guarded by NAPI, because the .ndo_xdp_xmit()
> is not called directly from the rxq which owns the xdp_mem_info.
Hmm "not guarded by NAPI" sounds scary to me, as XDP depends heavily on
being protected by NAPI. But it does look like you handle this is
earlier patches...
> This approach introduces a flag in bpf_redirect_info to indicate that
> napi_direct should be disabled even when _rx_napi variant is used as
> well as helper functions to use it.
>
> A NAPI handler who wants to use this flag needs to call
> xdp_set_return_frame_no_direct() before processing packets, and call
> xdp_clear_return_frame_no_direct() after xdp_do_flush_map() before
> exiting NAPI.
Okay, so it still runs under NAPI. It should be okay then. Also such
that the bpf_redirect_info is "stable", in the sense that we cannot
change the CPU we are running on, given bpf_redirect_info is a per-cpu thing.
> v4:
> - Use bpf_redirect_info for storing the flag instead of xdp_mem_info to
> avoid per-frame copy cost.
>
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
> include/linux/filter.h | 25 +++++++++++++++++++++++++
> net/core/xdp.c | 6 ++++--
> 2 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 4717af8b95e6..2b072dab32c0 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -543,10 +543,14 @@ struct bpf_redirect_info {
> struct bpf_map *map;
> struct bpf_map *map_to_flush;
> unsigned long map_owner;
> + u32 kern_flags;
> };
>
> DECLARE_PER_CPU(struct bpf_redirect_info, bpf_redirect_info);
>
> +/* flags for bpf_redirect_info kern_flags */
> +#define BPF_RI_F_RF_NO_DIRECT BIT(0) /* no napi_direct on return_frame */
> +
> /* Compute the linear packet data range [data, data_end) which
> * will be accessed by various program types (cls_bpf, act_bpf,
> * lwt, ...). Subsystems allowing direct data access must (!)
> @@ -775,6 +779,27 @@ static inline bool bpf_dump_raw_ok(void)
> struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
> const struct bpf_insn *patch, u32 len);
>
> +static inline bool xdp_return_frame_no_direct(void)
> +{
> + struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
> +
> + return ri->kern_flags & BPF_RI_F_RF_NO_DIRECT;
> +}
> +
> +static inline void xdp_set_return_frame_no_direct(void)
> +{
> + struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
> +
> + ri->kern_flags |= BPF_RI_F_RF_NO_DIRECT;
> +}
> +
> +static inline void xdp_clear_return_frame_no_direct(void)
> +{
> + struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
> +
> + ri->kern_flags &= ~BPF_RI_F_RF_NO_DIRECT;
> +}
> +
> static inline int xdp_ok_fwd_dev(const struct net_device *fwd,
> unsigned int pktlen)
> {
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 57285383ed00..3dd99e1c04f5 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -330,10 +330,12 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
> /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
> xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
> page = virt_to_head_page(data);
> - if (xa)
> + if (xa) {
> + napi_direct &= !xdp_return_frame_no_direct();
> page_pool_put_page(xa->page_pool, page, napi_direct);
> - else
> + } else {
> put_page(page);
> + }
> rcu_read_unlock();
> break;
> case MEM_TYPE_PAGE_SHARED:
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* [PATCH net-next v5 4/4] act_mirred: use TC_ACT_REINSERT when possible
From: Paolo Abeni @ 2018-07-30 12:30 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, Daniel Borkmann,
Marcelo Ricardo Leitner, Eyal Birger, David S. Miller
In-Reply-To: <cover.1532934532.git.pabeni@redhat.com>
When mirred is invoked from the ingress path, and it wants to redirect
the processed packet, it can now use the TC_ACT_REINSERT action,
filling the tcf_result accordingly, and avoiding a per packet
skb_clone().
Overall this gives a ~10% improvement in forwarding performance for the
TC S/W data path and TC S/W performances are now comparable to the
kernel openvswitch datapath.
v1 -> v2: use ACT_MIRRED instead of ACT_REDIRECT
v2 -> v3: updated after action rename, fixed typo into the commit
message
v3 -> v4: updated again after action rename, added more comments to
the code (JiriP), skip the optimization if the control action
need to touch the tcf_result (Paolo)
v4 -> v5: fix sparse warning (kbuild bot)
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
Note: I had to do some minor change in the last chunk since v4, so
I did not include Cong's ack, added to such revision
---
net/sched/act_mirred.c | 53 ++++++++++++++++++++++++++++++++++--------
1 file changed, 43 insertions(+), 10 deletions(-)
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index eeb335f03102..b26d060da08e 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -25,6 +25,7 @@
#include <net/net_namespace.h>
#include <net/netlink.h>
#include <net/pkt_sched.h>
+#include <net/pkt_cls.h>
#include <linux/tc_act/tc_mirred.h>
#include <net/tc_act/tc_mirred.h>
@@ -49,6 +50,18 @@ static bool tcf_mirred_act_wants_ingress(int action)
}
}
+static bool tcf_mirred_can_reinsert(int action)
+{
+ switch (action) {
+ case TC_ACT_SHOT:
+ case TC_ACT_STOLEN:
+ case TC_ACT_QUEUED:
+ case TC_ACT_TRAP:
+ return true;
+ }
+ return false;
+}
+
static void tcf_mirred_release(struct tc_action *a)
{
struct tcf_mirred *m = to_mirred(a);
@@ -171,10 +184,13 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
struct tcf_result *res)
{
struct tcf_mirred *m = to_mirred(a);
+ struct sk_buff *skb2 = skb;
bool m_mac_header_xmit;
struct net_device *dev;
- struct sk_buff *skb2;
int retval, err = 0;
+ bool use_reinsert;
+ bool want_ingress;
+ bool is_redirect;
int m_eaction;
int mac_len;
@@ -196,16 +212,25 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
goto out;
}
- skb2 = skb_clone(skb, GFP_ATOMIC);
- if (!skb2)
- goto out;
+ /* we could easily avoid the clone only if called by ingress and clsact;
+ * since we can't easily detect the clsact caller, skip clone only for
+ * ingress - that covers the TC S/W datapath.
+ */
+ is_redirect = tcf_mirred_is_act_redirect(m_eaction);
+ use_reinsert = skb_at_tc_ingress(skb) && is_redirect &&
+ tcf_mirred_can_reinsert(retval);
+ if (!use_reinsert) {
+ skb2 = skb_clone(skb, GFP_ATOMIC);
+ if (!skb2)
+ goto out;
+ }
/* If action's target direction differs than filter's direction,
* and devices expect a mac header on xmit, then mac push/pull is
* needed.
*/
- if (skb_at_tc_ingress(skb) != tcf_mirred_act_wants_ingress(m_eaction) &&
- m_mac_header_xmit) {
+ want_ingress = tcf_mirred_act_wants_ingress(m_eaction);
+ if (skb_at_tc_ingress(skb) != want_ingress && m_mac_header_xmit) {
if (!skb_at_tc_ingress(skb)) {
/* caught at egress, act ingress: pull mac */
mac_len = skb_network_header(skb) - skb_mac_header(skb);
@@ -216,15 +241,23 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
}
}
+ skb2->skb_iif = skb->dev->ifindex;
+ skb2->dev = dev;
+
/* mirror is always swallowed */
- if (tcf_mirred_is_act_redirect(m_eaction)) {
+ if (is_redirect) {
skb2->tc_redirected = 1;
skb2->tc_from_ingress = skb2->tc_at_ingress;
+
+ /* let's the caller reinsert the packet, if possible */
+ if (use_reinsert) {
+ res->ingress = want_ingress;
+ res->qstats = this_cpu_ptr(m->common.cpu_qstats);
+ return TC_ACT_REINSERT;
+ }
}
- skb2->skb_iif = skb->dev->ifindex;
- skb2->dev = dev;
- if (!tcf_mirred_act_wants_ingress(m_eaction))
+ if (!want_ingress)
err = dev_queue_xmit(skb2);
else
err = netif_receive_skb(skb2);
--
2.17.1
^ permalink raw reply related
* [PATCH net-next v5 3/4] net/tc: introduce TC_ACT_REINSERT.
From: Paolo Abeni @ 2018-07-30 12:30 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, Daniel Borkmann,
Marcelo Ricardo Leitner, Eyal Birger, David S. Miller
In-Reply-To: <cover.1532934532.git.pabeni@redhat.com>
This is similar TC_ACT_REDIRECT, but with a slightly different
semantic:
- on ingress the mirred skbs are passed to the target device
network stack without any additional check not scrubbing.
- the rcu-protected stats provided via the tcf_result struct
are updated on error conditions.
This new tcfa_action value is not exposed to the user-space
and can be used only internally by clsact.
v1 -> v2: do not touch TC_ACT_REDIRECT code path, introduce
a new action type instead
v2 -> v3:
- rename the new action value TC_ACT_REINJECT, update the
helper accordingly
- take care of uncloned reinjected packets in XDP generic
hook
v3 -> v4:
- renamed again the new action value (JiriP)
v4 -> v5:
- fix build error with !NET_CLS_ACT (kbuild bot)
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
Note: since I had to change the netif_receive_generic_xdp chunk
in respect to revision v4, so I did not include Cong's ack, added
to such revision
---
include/net/pkt_cls.h | 3 +++
include/net/sch_generic.h | 28 ++++++++++++++++++++++++++++
net/core/dev.c | 6 +++++-
3 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 6d02f31abba8..22bfc3a13c25 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -7,6 +7,9 @@
#include <net/sch_generic.h>
#include <net/act_api.h>
+/* TC action not accessible from user space */
+#define TC_ACT_REINSERT (TC_ACT_VALUE_MAX + 1)
+
/* Basic packet classifier frontend definitions. */
struct tcf_walker {
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index bcae181c1857..a6d00093f35e 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -235,6 +235,12 @@ struct tcf_result {
u32 classid;
};
const struct tcf_proto *goto_tp;
+
+ /* used by the TC_ACT_REINSERT action */
+ struct {
+ bool ingress;
+ struct gnet_stats_queue *qstats;
+ };
};
};
@@ -569,6 +575,15 @@ static inline void skb_reset_tc(struct sk_buff *skb)
#endif
}
+static inline bool skb_is_tc_redirected(const struct sk_buff *skb)
+{
+#ifdef CONFIG_NET_CLS_ACT
+ return skb->tc_redirected;
+#else
+ return false;
+#endif
+}
+
static inline bool skb_at_tc_ingress(const struct sk_buff *skb)
{
#ifdef CONFIG_NET_CLS_ACT
@@ -1108,4 +1123,17 @@ void mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp,
void mini_qdisc_pair_init(struct mini_Qdisc_pair *miniqp, struct Qdisc *qdisc,
struct mini_Qdisc __rcu **p_miniq);
+static inline void skb_tc_reinsert(struct sk_buff *skb, struct tcf_result *res)
+{
+ struct gnet_stats_queue *stats = res->qstats;
+ int ret;
+
+ if (res->ingress)
+ ret = netif_receive_skb(skb);
+ else
+ ret = dev_queue_xmit(skb);
+ if (ret && stats)
+ qstats_overlimit_inc(res->qstats);
+}
+
#endif
diff --git a/net/core/dev.c b/net/core/dev.c
index 89031b5fef9f..38b0c414d780 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4252,7 +4252,7 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
/* Reinjected packets coming from act_mirred or similar should
* not get XDP generic processing.
*/
- if (skb_cloned(skb))
+ if (skb_cloned(skb) || skb_is_tc_redirected(skb))
return XDP_PASS;
/* XDP packets must be linear and must have sufficient headroom
@@ -4602,6 +4602,10 @@ sch_handle_ingress(struct sk_buff *skb, struct packet_type **pt_prev, int *ret,
__skb_push(skb, skb->mac_len);
skb_do_redirect(skb);
return NULL;
+ case TC_ACT_REINSERT:
+ /* this does not scrub the packet, and updates stats on error */
+ skb_tc_reinsert(skb, &cl_res);
+ return NULL;
default:
break;
}
--
2.17.1
^ permalink raw reply related
* [PATCH net-next v5 2/4] tc/act: remove unneeded RCU lock in action callback
From: Paolo Abeni @ 2018-07-30 12:30 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, Daniel Borkmann,
Marcelo Ricardo Leitner, Eyal Birger, David S. Miller
In-Reply-To: <cover.1532934532.git.pabeni@redhat.com>
Each lockless action currently does its own RCU locking in ->act().
This allows using plain RCU accessor, even if the context
is really RCU BH.
This change drops the per action RCU lock, replace the accessors
with the _bh variant, cleans up a bit the surrounding code and
documents the RCU status in the relevant header.
No functional nor performance change is intended.
The goal of this patch is clarifying that the RCU critical section
used by the tc actions extends up to the classifier's caller.
v1 -> v2:
- preserve rcu lock in act_bpf: it's needed by eBPF helpers,
as pointed out by Daniel
v3 -> v4:
- fixed some typos in the commit message (JiriP)
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
include/net/act_api.h | 2 +-
include/net/sch_generic.h | 2 ++
net/sched/act_csum.c | 12 +++---------
net/sched/act_ife.c | 5 +----
net/sched/act_mirred.c | 4 +---
net/sched/act_sample.c | 4 +---
net/sched/act_skbedit.c | 10 +++-------
net/sched/act_skbmod.c | 21 +++++++++------------
net/sched/act_tunnel_key.c | 6 +-----
net/sched/act_vlan.c | 19 +++++++------------
10 files changed, 29 insertions(+), 56 deletions(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index 683ce41053d9..8c9bc02d05e1 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -85,7 +85,7 @@ struct tc_action_ops {
size_t size;
struct module *owner;
int (*act)(struct sk_buff *, const struct tc_action *,
- struct tcf_result *);
+ struct tcf_result *); /* called under RCU BH lock*/
int (*dump)(struct sk_buff *, struct tc_action *, int, int);
void (*cleanup)(struct tc_action *);
int (*lookup)(struct net *net, struct tc_action **a, u32 index,
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index c5432362dc26..bcae181c1857 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -285,6 +285,8 @@ struct tcf_proto {
/* Fast access part */
struct tcf_proto __rcu *next;
void __rcu *root;
+
+ /* called under RCU BH lock*/
int (*classify)(struct sk_buff *,
const struct tcf_proto *,
struct tcf_result *);
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 4e8c383f379e..648a3a35b720 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -561,15 +561,14 @@ static int tcf_csum(struct sk_buff *skb, const struct tc_action *a,
u32 update_flags;
int action;
- rcu_read_lock();
- params = rcu_dereference(p->params);
+ params = rcu_dereference_bh(p->params);
tcf_lastuse_update(&p->tcf_tm);
bstats_cpu_update(this_cpu_ptr(p->common.cpu_bstats), skb);
action = READ_ONCE(p->tcf_action);
if (unlikely(action == TC_ACT_SHOT))
- goto drop_stats;
+ goto drop;
update_flags = params->update_flags;
switch (tc_skb_protocol(skb)) {
@@ -583,16 +582,11 @@ static int tcf_csum(struct sk_buff *skb, const struct tc_action *a,
break;
}
-unlock:
- rcu_read_unlock();
return action;
drop:
- action = TC_ACT_SHOT;
-
-drop_stats:
qstats_drop_inc(this_cpu_ptr(p->common.cpu_qstats));
- goto unlock;
+ return TC_ACT_SHOT;
}
static int tcf_csum_dump(struct sk_buff *skb, struct tc_action *a, int bind,
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 3d6e265758c0..df4060e32d43 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -820,14 +820,11 @@ static int tcf_ife_act(struct sk_buff *skb, const struct tc_action *a,
struct tcf_ife_params *p;
int ret;
- rcu_read_lock();
- p = rcu_dereference(ife->params);
+ p = rcu_dereference_bh(ife->params);
if (p->flags & IFE_ENCODE) {
ret = tcf_ife_encode(skb, a, res, p);
- rcu_read_unlock();
return ret;
}
- rcu_read_unlock();
return tcf_ife_decode(skb, a, res);
}
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 6afd89a36c69..eeb335f03102 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -181,11 +181,10 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
tcf_lastuse_update(&m->tcf_tm);
bstats_cpu_update(this_cpu_ptr(m->common.cpu_bstats), skb);
- rcu_read_lock();
m_mac_header_xmit = READ_ONCE(m->tcfm_mac_header_xmit);
m_eaction = READ_ONCE(m->tcfm_eaction);
retval = READ_ONCE(m->tcf_action);
- dev = rcu_dereference(m->tcfm_dev);
+ dev = rcu_dereference_bh(m->tcfm_dev);
if (unlikely(!dev)) {
pr_notice_once("tc mirred: target device is gone\n");
goto out;
@@ -236,7 +235,6 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
if (tcf_mirred_is_act_redirect(m_eaction))
retval = TC_ACT_SHOT;
}
- rcu_read_unlock();
return retval;
}
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index 3079e7be5bde..2608ccc83e5e 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -140,8 +140,7 @@ static int tcf_sample_act(struct sk_buff *skb, const struct tc_action *a,
bstats_cpu_update(this_cpu_ptr(s->common.cpu_bstats), skb);
retval = READ_ONCE(s->tcf_action);
- rcu_read_lock();
- psample_group = rcu_dereference(s->psample_group);
+ psample_group = rcu_dereference_bh(s->psample_group);
/* randomly sample packets according to rate */
if (psample_group && (prandom_u32() % s->rate == 0)) {
@@ -165,7 +164,6 @@ static int tcf_sample_act(struct sk_buff *skb, const struct tc_action *a,
skb_pull(skb, skb->mac_len);
}
- rcu_read_unlock();
return retval;
}
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index da56e6938c9e..a6db47ebec11 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -43,8 +43,7 @@ static int tcf_skbedit(struct sk_buff *skb, const struct tc_action *a,
tcf_lastuse_update(&d->tcf_tm);
bstats_cpu_update(this_cpu_ptr(d->common.cpu_bstats), skb);
- rcu_read_lock();
- params = rcu_dereference(d->params);
+ params = rcu_dereference_bh(d->params);
action = READ_ONCE(d->tcf_action);
if (params->flags & SKBEDIT_F_PRIORITY)
@@ -77,14 +76,11 @@ static int tcf_skbedit(struct sk_buff *skb, const struct tc_action *a,
}
if (params->flags & SKBEDIT_F_PTYPE)
skb->pkt_type = params->ptype;
-
-unlock:
- rcu_read_unlock();
return action;
+
err:
qstats_drop_inc(this_cpu_ptr(d->common.cpu_qstats));
- action = TC_ACT_SHOT;
- goto unlock;
+ return TC_ACT_SHOT;
}
static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX + 1] = {
diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index cdc6bacfb190..c437c6d51a71 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -41,20 +41,14 @@ static int tcf_skbmod_run(struct sk_buff *skb, const struct tc_action *a,
* then MAX_EDIT_LEN needs to change appropriately
*/
err = skb_ensure_writable(skb, MAX_EDIT_LEN);
- if (unlikely(err)) { /* best policy is to drop on the floor */
- qstats_overlimit_inc(this_cpu_ptr(d->common.cpu_qstats));
- return TC_ACT_SHOT;
- }
+ if (unlikely(err)) /* best policy is to drop on the floor */
+ goto drop;
- rcu_read_lock();
action = READ_ONCE(d->tcf_action);
- if (unlikely(action == TC_ACT_SHOT)) {
- qstats_overlimit_inc(this_cpu_ptr(d->common.cpu_qstats));
- rcu_read_unlock();
- return action;
- }
+ if (unlikely(action == TC_ACT_SHOT))
+ goto drop;
- p = rcu_dereference(d->skbmod_p);
+ p = rcu_dereference_bh(d->skbmod_p);
flags = p->flags;
if (flags & SKBMOD_F_DMAC)
ether_addr_copy(eth_hdr(skb)->h_dest, p->eth_dst);
@@ -62,7 +56,6 @@ static int tcf_skbmod_run(struct sk_buff *skb, const struct tc_action *a,
ether_addr_copy(eth_hdr(skb)->h_source, p->eth_src);
if (flags & SKBMOD_F_ETYPE)
eth_hdr(skb)->h_proto = p->eth_type;
- rcu_read_unlock();
if (flags & SKBMOD_F_SWAPMAC) {
u16 tmpaddr[ETH_ALEN / 2]; /* ether_addr_copy() requirement */
@@ -73,6 +66,10 @@ static int tcf_skbmod_run(struct sk_buff *skb, const struct tc_action *a,
}
return action;
+
+drop:
+ qstats_overlimit_inc(this_cpu_ptr(d->common.cpu_qstats));
+ return TC_ACT_SHOT;
}
static const struct nla_policy skbmod_policy[TCA_SKBMOD_MAX + 1] = {
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index f811850fd1d0..d42d9e112789 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -31,9 +31,7 @@ static int tunnel_key_act(struct sk_buff *skb, const struct tc_action *a,
struct tcf_tunnel_key_params *params;
int action;
- rcu_read_lock();
-
- params = rcu_dereference(t->params);
+ params = rcu_dereference_bh(t->params);
tcf_lastuse_update(&t->tcf_tm);
bstats_cpu_update(this_cpu_ptr(t->common.cpu_bstats), skb);
@@ -53,8 +51,6 @@ static int tunnel_key_act(struct sk_buff *skb, const struct tc_action *a,
break;
}
- rcu_read_unlock();
-
return action;
}
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index ad37f308175a..15a0ee214c9c 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -40,11 +40,9 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
if (skb_at_tc_ingress(skb))
skb_push_rcsum(skb, skb->mac_len);
- rcu_read_lock();
-
action = READ_ONCE(v->tcf_action);
- p = rcu_dereference(v->vlan_p);
+ p = rcu_dereference_bh(v->vlan_p);
switch (p->tcfv_action) {
case TCA_VLAN_ACT_POP:
@@ -61,7 +59,7 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
case TCA_VLAN_ACT_MODIFY:
/* No-op if no vlan tag (either hw-accel or in-payload) */
if (!skb_vlan_tagged(skb))
- goto unlock;
+ goto out;
/* extract existing tag (and guarantee no hw-accel tag) */
if (skb_vlan_tag_present(skb)) {
tci = skb_vlan_tag_get(skb);
@@ -86,18 +84,15 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
BUG();
}
- goto unlock;
-
-drop:
- action = TC_ACT_SHOT;
- qstats_drop_inc(this_cpu_ptr(v->common.cpu_qstats));
-
-unlock:
- rcu_read_unlock();
+out:
if (skb_at_tc_ingress(skb))
skb_pull_rcsum(skb, skb->mac_len);
return action;
+
+drop:
+ qstats_drop_inc(this_cpu_ptr(v->common.cpu_qstats));
+ return TC_ACT_SHOT;
}
static const struct nla_policy vlan_policy[TCA_VLAN_MAX + 1] = {
--
2.17.1
^ permalink raw reply related
* [PATCH net-next v5 1/4] net/sched: user-space can't set unknown tcfa_action values
From: Paolo Abeni @ 2018-07-30 12:30 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, Daniel Borkmann,
Marcelo Ricardo Leitner, Eyal Birger, David S. Miller
In-Reply-To: <cover.1532934532.git.pabeni@redhat.com>
Currently, when initializing an action, the user-space can specify
and use arbitrary values for the tcfa_action field. If the value
is unknown by the kernel, is implicitly threaded as TC_ACT_UNSPEC.
This change explicitly checks for unknown values at action creation
time, and explicitly convert them to TC_ACT_UNSPEC. No functional
changes are introduced, but this will allow introducing tcfa_action
values not exposed to user-space in a later patch.
Note: we can't use the above to hide TC_ACT_REDIRECT from user-space,
as the latter is already part of uAPI.
v3 -> v4:
- use an helper to check for action validity (JiriP)
- emit an extack for invalid actions (JiriP)
v4 -> v5:
- keep messages on a single line, drop net_warn (Marcelo)
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
include/uapi/linux/pkt_cls.h | 6 ++++--
net/sched/act_api.c | 14 ++++++++++++++
2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index b4512254036b..48e5b5d49a34 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -45,6 +45,7 @@ enum {
* the skb and act like everything
* is alright.
*/
+#define TC_ACT_VALUE_MAX TC_ACT_TRAP
/* There is a special kind of actions called "extended actions",
* which need a value parameter. These have a local opcode located in
@@ -55,11 +56,12 @@ enum {
#define __TC_ACT_EXT_SHIFT 28
#define __TC_ACT_EXT(local) ((local) << __TC_ACT_EXT_SHIFT)
#define TC_ACT_EXT_VAL_MASK ((1 << __TC_ACT_EXT_SHIFT) - 1)
-#define TC_ACT_EXT_CMP(combined, opcode) \
- (((combined) & (~TC_ACT_EXT_VAL_MASK)) == opcode)
+#define TC_ACT_EXT_OPCODE(combined) ((combined) & (~TC_ACT_EXT_VAL_MASK))
+#define TC_ACT_EXT_CMP(combined, opcode) (TC_ACT_EXT_OPCODE(combined) == opcode)
#define TC_ACT_JUMP __TC_ACT_EXT(1)
#define TC_ACT_GOTO_CHAIN __TC_ACT_EXT(2)
+#define TC_ACT_EXT_OPCODE_MAX TC_ACT_GOTO_CHAIN
/* Action type identifiers*/
enum {
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index b43df1e25c6d..229d63c99be2 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -786,6 +786,15 @@ static struct tc_cookie *nla_memdup_cookie(struct nlattr **tb)
return c;
}
+static bool tcf_action_valid(int action)
+{
+ int opcode = TC_ACT_EXT_OPCODE(action);
+
+ if (!opcode)
+ return action <= TC_ACT_VALUE_MAX;
+ return opcode <= TC_ACT_EXT_OPCODE_MAX || action == TC_ACT_UNSPEC;
+}
+
struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
struct nlattr *nla, struct nlattr *est,
char *name, int ovr, int bind,
@@ -895,6 +904,11 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
}
}
+ if (!tcf_action_valid(a->tcfa_action)) {
+ NL_SET_ERR_MSG(extack, "invalid action value, using TC_ACT_UNSPEC instead");
+ a->tcfa_action = TC_ACT_UNSPEC;
+ }
+
return a;
err_mod:
--
2.17.1
^ permalink raw reply related
* [PATCH net-next v5 0/4] TC: refactor act_mirred packets re-injection
From: Paolo Abeni @ 2018-07-30 12:30 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, Daniel Borkmann,
Marcelo Ricardo Leitner, Eyal Birger, David S. Miller
This series is aimed at improving the act_mirred redirect performances.
Such action is used by OVS to represent TC S/W flows, and it's current largest
bottle-neck is the need for a skb_clone() for each packet.
The first 2 patches introduce some cleanup and safeguards to allow extending
tca_result - we will use it to store RCU protected redirect information - and
introduce a clear separation between user-space accessible tcfa_action
values and internal values accessible only by the kernel.
Then a new tcfa_action value is introduced: TC_ACT_REINJECT, similar to
TC_ACT_REDIRECT, but preserving the mirred semantic. Such value is not
accessible from user-space.
The last patch exploits the newly introduced infrastructure in the act_mirred
action, to avoid a skb_clone, when possible.
Overall this the above gives a ~10% performance improvement in forwarding tput,
when using the TC S/W datapath.
v1 -> v2:
- preserve the rcu lock in act_bpf
- add and use a new action value to reinject the packets, preserving the mirred
semantic
v2 -> v3:
- renamed to new action as TC_ACT_REINJECT
- TC_ACT_REINJECT is not exposed to user-space
v3 -> v4:
- dropped the TC_ACT_REDIRECT patch
- report failure via extack, too
- rename the new action as TC_ACT_REINSERT
- skip clone only if the control action don't touch tcf_result
v4 -> v5:
- fix a couple of build issue reported by kbuild bot
- dont split messages
Paolo Abeni (4):
net/sched: user-space can't set unknown tcfa_action values
tc/act: remove unneeded RCU lock in action callback
net/tc: introduce TC_ACT_REINSERT.
act_mirred: use TC_ACT_REINSERT when possible
include/net/act_api.h | 2 +-
include/net/pkt_cls.h | 3 ++
include/net/sch_generic.h | 30 +++++++++++++++++++
include/uapi/linux/pkt_cls.h | 6 ++--
net/core/dev.c | 6 +++-
net/sched/act_api.c | 14 +++++++++
net/sched/act_csum.c | 12 ++------
net/sched/act_ife.c | 5 +---
net/sched/act_mirred.c | 57 ++++++++++++++++++++++++++++--------
net/sched/act_sample.c | 4 +--
net/sched/act_skbedit.c | 10 ++-----
net/sched/act_skbmod.c | 21 ++++++-------
net/sched/act_tunnel_key.c | 6 +---
net/sched/act_vlan.c | 19 +++++-------
14 files changed, 126 insertions(+), 69 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: [PATCH] net/phy: Micrel KSZ8061 PHY link failure after cable connect
From: Andrew Lunn @ 2018-07-30 14:03 UTC (permalink / raw)
To: Onnasch, Alexander (EXT)
Cc: Florian Fainelli, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <AM6PR01MB4262FB2EBD4AFBC05C02F9B6F02F0@AM6PR01MB4262.eurprd01.prod.exchangelabs.com>
On Mon, Jul 30, 2018 at 02:00:44PM +0000, Onnasch, Alexander (EXT) wrote:
> Hi Andrew
>
> thanks for your feedback. I was on holiday, thus just delayed, not forgotten...
> Sorry for top-posting - odd company default mail setup.
But you did it again....
Your email client should not be forcing you to top post. So please
don't.
Andrew
^ permalink raw reply
* Re: [PATCH net-next] net: phy: mscc: the extended page access register is 16 bits
From: Andrew Lunn @ 2018-07-30 14:01 UTC (permalink / raw)
To: Quentin Schulz
Cc: f.fainelli, davem, netdev, alexandre.belloni, linux-kernel,
thomas.petazzoni
In-Reply-To: <20180730125313.3405-1-quentin.schulz@bootlin.com>
On Mon, Jul 30, 2018 at 02:53:13PM +0200, Quentin Schulz wrote:
> The Extended Page Access is a 16-bit register, so change the page
> parameter of vsc85xx_phy_page_set to a u16.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* [PATCH] net/phy: Micrel KSZ8061 PHY link failure after cable connect
From: Alexander Onnasch @ 2018-07-30 14:01 UTC (permalink / raw)
Cc: alexander.onnasch, Andrew Lunn, Florian Fainelli, netdev,
linux-kernel
With Micrel KSZ8061 PHY, the link may occasionally not come up after
Ethernet cable connect. The vendor's (Microchip, former Micrel) errata
sheet 80000688A.pdf describes the problem and possible workarounds in
detail, see below.
The patch implements workaround 1, which permanently fixes the issue.
DESCRIPTION
Link-up may not occur properly when the Ethernet cable is initially
connected. This issue occurs more commonly when the cable is connected
slowly, but it may occur any time a cable is connected. This issue occurs
in the auto-negotiation circuit, and will not occur if auto-negotiation
is disabled (which requires that the two link partners be set to the
same speed and duplex).
END USER IMPLICATIONS
When this issue occurs, link is not established. Subsequent cable
plug/unplug cycles will not correct the issue.
WORK AROUND
There are four approaches to work around this issue:
1. This issue can be prevented by setting bit 15 in MMD device address 1,
register 2, prior to connecting the cable or prior to setting the
Restart Auto-Negotiation bit in register 0h.The MMD registers are
accessed via the indirect access registers Dh and Eh, or via the Micrel
EthUtil utility as shown here:
• If using the EthUtil utility (usually with a Micrel KSZ8061
Evaluation Board), type the following commands:
> address 1
> mmd 1
> iw 2 b61a
• Alternatively, write the following registers to write to the
indirect MMD register:
Write register Dh, data 0001h
Write register Eh, data 0002h
Write register Dh, data 4001h
Write register Eh, data B61Ah
2. The issue can be avoided by disabling auto-negotiation in the KSZ8061,
either by the strapping option, or by clearing bit 12 in register 0h.
Care must be taken to ensure that the KSZ8061 and the link partner
will link with the same speed and duplex. Note that the KSZ8061
defaults to full-duplex when auto-negotiation is off, but other
devices may default to half-duplex in the event of failed
auto-negotiation.
3. The issue can be avoided by connecting the cable prior to powering-up
or resetting the KSZ8061, and leaving it plugged in thereafter.
4. If the above measures are not taken and the problem occurs, link can
be recovered by setting the Restart Auto-Negotiation bit in
register 0h, or by resetting or power cycling the device. Reset may
be either hardware reset or software reset (register 0h, bit 15).
PLAN
This errata will not be corrected in a future revision.
Signed-off-by: Alexander Onnasch <alexander.onnasch@landisgyr.com>
---
drivers/net/phy/micrel.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 6c45ff6..eb85cf4 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -339,6 +339,17 @@ static int ksz8041_config_aneg(struct phy_device *phydev)
return genphy_config_aneg(phydev);
}
+static int ksz8061_config_init(struct phy_device *phydev)
+{
+ int ret;
+
+ ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_DEVID1, 0xB61A);
+ if (ret)
+ return ret;
+
+ return kszphy_config_init(phydev);
+}
+
static int ksz9021_load_values_from_of(struct phy_device *phydev,
const struct device_node *of_node,
u16 reg,
@@ -938,7 +949,7 @@ static struct phy_driver ksphy_driver[] = {
.phy_id_mask = MICREL_PHY_ID_MASK,
.features = PHY_BASIC_FEATURES,
.flags = PHY_HAS_INTERRUPT,
- .config_init = kszphy_config_init,
+ .config_init = ksz8061_config_init,
.config_aneg = genphy_config_aneg,
.read_status = genphy_read_status,
.ack_interrupt = kszphy_ack_interrupt,
--
2.7.4
^ permalink raw reply related
* RE: [PATCH] net/phy: Micrel KSZ8061 PHY link failure after cable connect
From: Onnasch, Alexander (EXT) @ 2018-07-30 14:00 UTC (permalink / raw)
To: Andrew Lunn
Cc: Florian Fainelli, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20180619152858.GD26796@lunn.ch>
Hi Andrew
thanks for your feedback. I was on holiday, thus just delayed, not forgotten...
Sorry for top-posting - odd company default mail setup.
I checked again phy_write_mmd(), you are right !
Patch with changed implementation will follow.
Best regards, Alex
-----Original Message-----
From: Andrew Lunn <andrew@lunn.ch>
Sent: Dienstag, 19. Juni 2018 17:29
To: Onnasch, Alexander (EXT) <Alexander.Onnasch@landisgyr.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net/phy: Micrel KSZ8061 PHY link failure after cable connect
On Tue, Jun 19, 2018 at 02:23:41PM +0000, Onnasch, Alexander (EXT) wrote:
> Hi Andrew
> thanks for the hint. But actually I cannot confirm - or I don't see it yet.
>
> Without having tested, just from the code, the struct phy_driver instance for PHY_ID_KSZ8061 in micrel.c does not have a .write_mmd function assigned, thus phy_write_mmd should evaluate to its else-clause (see below) and not to mdiobus_write (as in phy_write).
>
> Also the ksz8061_extended_write() function which I have added uses the same principle as already existing HW-specific functions in micrel.c for simular reasons (kszphy_extended_write and ksz9031_extended_write).
> They use phy_write all over the place in that file and never phy_write_mmd - for whatever reason they had.
> Thus I thought it would be a good idea ...
Hi Alexander
Please don't top post. And wrap your lines at around 75 characters
> struct mii_bus *bus = phydev->mdio.bus;
> int phy_addr = phydev->mdio.addr;
>
> mutex_lock(&bus->mdio_lock);
> mmd_phy_indirect(bus, phy_addr, devad, regnum);
>
> /* Write the data into MMD's selected register */
> bus->write(bus, phy_addr, MII_MMD_DATA, val);
> mutex_unlock(&bus->mdio_lock);
> > +static int ksz8061_extended_write(struct phy_device *phydev,
> > + u8 mode, u32 dev_addr, u32 regnum, u16 val) {
> > + phy_write(phydev, MII_KSZ8061RN_MMD_CTRL_REG, dev_addr);
> > + phy_write(phydev, MII_KSZ8061RN_MMD_REGDATA_REG, regnum);
> > + phy_write(phydev, MII_KSZ8061RN_MMD_CTRL_REG, (mode << 14) | dev_addr);
> > + return phy_write(phydev, MII_KSZ8061RN_MMD_REGDATA_REG, val); }
>
> Hi Alexander
>
> This looks a lot like phy_write_mmd().
Look closely at the two implementations. Look at what
mmd_phy_indirect() does. I _think_ these are identical. So don't add your own helper, please use the core code.
Andrew
^ permalink raw reply
* Re: [PATCH 3/4] dt-bindings: net: phy: mscc: vsc8531: fix missing "/bits/ 8" in example
From: Andrew Lunn @ 2018-07-30 13:58 UTC (permalink / raw)
To: Quentin Schulz
Cc: f.fainelli, davem, robh+dt, mark.rutland, devicetree, netdev,
alexandre.belloni, linux-kernel, thomas.petazzoni
In-Reply-To: <20180730130236.3837-3-quentin.schulz@bootlin.com>
On Mon, Jul 30, 2018 at 03:02:35PM +0200, Quentin Schulz wrote:
> The "vsc8531,led-N-mode" property is read as a u8 in the driver and
> there aren't a lot of modes anyway.
>
> Without the "/bits/ 8" in front of the value of the property, the
> value is stored as an u32 resulting in of_read_property_u8 to always
> return 0.
Hi Quentin
on big endian systems. I'm expect this worked on little endian ARM. I
think the development work was done on a hacked RPi, if i remember
correctly.
>
> Fix the example so that people using the property can actually use it.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
> ---
> Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> index 664d9d0543fc..4c7d1d384df0 100644
> --- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> +++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> @@ -63,6 +63,6 @@ Example:
> compatible = "ethernet-phy-id0007.0570";
> vsc8531,vddmac = <3300>;
> vsc8531,edge-slowdown = <7>;
> - vsc8531,led-0-mode = <LINK_1000_ACTIVITY>;
> - vsc8531,led-1-mode = <LINK_100_ACTIVITY>;
> + vsc8531,led-0-mode = /bits/ 8 <LINK_1000_ACTIVITY>;
> + vsc8531,led-1-mode = /bits/ 8 <LINK_100_ACTIVITY>;
I don't know the device tree language well enough...
Would this work?
vsc8531,led-1-mode = < /bits/ 8 LINK_100_ACTIVITY>;
If so, you can make it part of the #define.
Andrew
^ permalink raw reply
* Re: [PATCH 2/4] dt-bindings: net: phy: mscc: vsc8531: remove compatible from required properties
From: Andrew Lunn @ 2018-07-30 13:53 UTC (permalink / raw)
To: Quentin Schulz
Cc: f.fainelli, davem, robh+dt, mark.rutland, devicetree, netdev,
alexandre.belloni, linux-kernel, thomas.petazzoni
In-Reply-To: <20180730130236.3837-2-quentin.schulz@bootlin.com>
On Mon, Jul 30, 2018 at 03:02:34PM +0200, Quentin Schulz wrote:
> Compatible isn't a required property for PHYs so let's remove it from
> the binding DT of the VSC8531 PHYs.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next 10/10] net: mscc: ocelot: make use of SerDes PHYs for handling their configuration
From: Andrew Lunn @ 2018-07-30 13:50 UTC (permalink / raw)
To: Quentin Schulz
Cc: alexandre.belloni, ralf, paul.burton, jhogan, robh+dt,
mark.rutland, davem, kishon, f.fainelli, linux-mips, devicetree,
linux-kernel, netdev, allan.nielsen, thomas.petazzoni
In-Reply-To: <0ce1b3e8466064741dc6e484f87bbe48542cb978.1532954208.git-series.quentin.schulz@bootlin.com>
On Mon, Jul 30, 2018 at 02:43:55PM +0200, Quentin Schulz wrote:
> + err = of_get_phy_mode(portnp);
> + if (err < 0)
> + ocelot->ports[port]->phy_mode = PHY_INTERFACE_MODE_NA;
> + else
> + ocelot->ports[port]->phy_mode = err;
> +
> + if (ocelot->ports[port]->phy_mode == PHY_INTERFACE_MODE_NA)
> + continue;
> +
> + if (ocelot->ports[port]->phy_mode == PHY_INTERFACE_MODE_SGMII)
> + phy_mode = PHY_MODE_SGMII;
> + else
> + phy_mode = PHY_MODE_QSGMII;
Hi Quentin
Say somebody puts RGMII as the phy-mode? It would be better to verify
it is only SGMII or QSGMII and return -EINVAL otherwise.
> +
> + serdes = devm_of_phy_get(ocelot->dev, portnp, NULL);
> + if (IS_ERR(serdes)) {
> + if (PTR_ERR(serdes) == -EPROBE_DEFER) {
> + dev_err(ocelot->dev, "deferring probe\n");
dev_dbg() ? It is not really an error.
> + err = -EPROBE_DEFER;
> + goto err_probe_ports;
> + }
> +
> + dev_err(ocelot->dev, "missing SerDes phys for port%d\n",
> + port);
> + err = -ENODEV;
err = PTR_ERR(serdes) so we get the actual error?
> goto err_probe_ports;
> }
> +
> + ocelot->ports[port]->serdes = serdes;
> }
>
> register_netdevice_notifier(&ocelot_netdevice_nb);
Andrew
^ permalink raw reply
* Re: [net-next 10/13] net/mlx5e: Add support for XDP_REDIRECT in device-out side
From: Jesper Dangaard Brouer @ 2018-07-30 12:10 UTC (permalink / raw)
To: Saeed Mahameed
Cc: brouer, David S. Miller, netdev, Tariq Toukan, Eugenia Emantayev
In-Reply-To: <20180726225647.11926-11-saeedm@mellanox.com>
On Thu, 26 Jul 2018 15:56:44 -0700 Saeed Mahameed <saeedm@mellanox.com> wrote:
> +int mlx5e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames,
> + u32 flags)
> +{
> + struct mlx5e_priv *priv = netdev_priv(dev);
> + struct mlx5e_xdpsq *sq;
> + int drops = 0;
> + int sq_num;
> + int i;
> +
> + if (unlikely(!test_bit(MLX5E_STATE_OPENED, &priv->state)))
> + return -ENETDOWN;
> +
> + if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
> + return -EINVAL;
> +
> + sq_num = smp_processor_id();
> +
> + if (unlikely(sq_num >= priv->channels.num))
> + return -ENXIO;
> +
> + sq = &priv->channels.c[sq_num]->xdpsq;
> +
> + if (unlikely(!test_bit(MLX5E_SQ_STATE_ENABLED, &sq->state)))
> + return -ENETDOWN;
> +
> + for (i = 0; i < n; i++) {
> + struct xdp_frame *xdpf = frames[i];
> + struct mlx5e_xdp_info xdpi;
> +
> + xdpi.dma_addr = dma_map_single(sq->pdev, xdpf->data, xdpf->len,
> + DMA_TO_DEVICE);
> + if (unlikely(dma_mapping_error(sq->pdev, xdpi.dma_addr))) {
> + drops++;
I think you are missing a xdp_return_frame_rx_napi(xdpf) here.
> + continue;
> + }
> +
> + xdpi.xdpf = xdpf;
> +
> + if (unlikely(!mlx5e_xmit_xdp_frame(sq, &xdpi))) {
> + xdp_return_frame_rx_napi(xdpf);
> + drops++;
> + }
> + }
> +
> + if (flags & XDP_XMIT_FLUSH)
> + mlx5e_xmit_xdp_doorbell(sq);
> +
> + return n - drops;
> +}
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* [PATCH][can-next] can: ucan: fix spelling mistake: "resumbmitting" -> "resubmitting"
From: Colin King @ 2018-07-30 13:40 UTC (permalink / raw)
To: Wolfgang Grandegger, Marc Kleine-Budde, David S . Miller,
linux-can, netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Trivial fix to spelling mistake in netdev_dbg error message
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/can/usb/ucan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/can/usb/ucan.c b/drivers/net/can/usb/ucan.c
index 0678a38b1af4..b9b5f6ef5bb2 100644
--- a/drivers/net/can/usb/ucan.c
+++ b/drivers/net/can/usb/ucan.c
@@ -719,7 +719,7 @@ static void ucan_read_bulk_callback(struct urb *urb)
up->in_ep_size,
urb->transfer_buffer,
urb->transfer_dma);
- netdev_dbg(up->netdev, "not resumbmitting urb; status: %d\n",
+ netdev_dbg(up->netdev, "not resubmitting urb; status: %d\n",
urb->status);
return;
default:
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 07/10] dt-bindings: phy: add DT binding for Microsemi Ocelot SerDes muxing
From: Andrew Lunn @ 2018-07-30 13:38 UTC (permalink / raw)
To: Quentin Schulz
Cc: alexandre.belloni, ralf, paul.burton, jhogan, robh+dt,
mark.rutland, davem, kishon, f.fainelli, linux-mips, devicetree,
linux-kernel, netdev, allan.nielsen, thomas.petazzoni
In-Reply-To: <cd75c96640cc7fe306ee355acb1db85adb5b796f.1532954208.git-series.quentin.schulz@bootlin.com>
> +- compatible: should be "mscc,vsc7514-serdes"
> +- #phy-cells : from the generic phy bindings, must be 3. The first number
> + defines the kind of Serdes (1 for SERDES1G_X, 6 for
> + SERDES6G_X), the second defines the macros in the specified
> + kind of Serdes (X for SERDES1G_X or SERDES6G_X) and the
> + last one defines the input port to use for a given SerDes
> + macro,
Hi Quentin
Maybe add #defines in an include file to make the binding less
magic?
Andrew
^ permalink raw reply
* Re: [PATCH] bpf: verifier: BPF_MOV don't mark dst reg if src == dst
From: Daniel Borkmann @ 2018-07-30 12:03 UTC (permalink / raw)
To: Arthur Fabre; +Cc: Alexei Starovoitov, Alexei Starovoitov, Network Development
In-Reply-To: <CAOn4ftv3eev+U8gYF07YLLsp5F6E=aWrGg0VS+fdk-zFkTdYqg@mail.gmail.com>
On 07/30/2018 12:58 PM, Arthur Fabre wrote:
> On Mon, Jul 30, 2018 at 10:10 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 07/30/2018 09:44 AM, Arthur Fabre wrote:
>>> On Sun, Jul 29, 2018 at 4:59 PM, Alexei Starovoitov
>>> <alexei.starovoitov@gmail.com> wrote:
>>>> On Thu, Jul 26, 2018 at 1:08 AM, Arthur Fabre <afabre@cloudflare.com> wrote:
>>>>> When check_alu_op() handles a BPF_MOV between two registers,
>>>>> it calls check_reg_arg() on the dst register, marking it as unbounded.
>>>>> If the src and dst register are the same, this marks the src as
>>>>> unbounded, which can lead to unexpected errors for further checks that
>>>>> rely on bounds info.
>>>>>
>>>>> check_alu_op() now only marks the dst register as unbounded if it
>>>>> different from the src register.
>>>>>
>>>>> Signed-off-by: Arthur Fabre <afabre@cloudflare.com>
>>>>> ---
>>>>> kernel/bpf/verifier.c | 5 +++--
>>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>>>> index 63aaac52a265..ddfe3c544a80 100644
>>>>> --- a/kernel/bpf/verifier.c
>>>>> +++ b/kernel/bpf/verifier.c
>>>>> @@ -3238,8 +3238,9 @@ static int check_alu_op(struct bpf_verifier_env
>>>>> *env, struct bpf_insn *insn)
>>>>> }
>>>>> }
>>>>>
>>>>> - /* check dest operand */
>>>>> - err = check_reg_arg(env, insn->dst_reg, DST_OP);
>>>>> + /* check dest operand, only mark if dest != src */
>>>>> + err = check_reg_arg(env, insn->dst_reg,
>>>>> + insn->dst_reg == insn->src_reg ?
>>>>> DST_OP_NO_MARK : DST_OP);
>>>>
>>>> that doesn't look correct for 32-bit mov.
>>>> Is that the case you're trying to improve?
>>>
>>> The patch was originally for 64-bit mov only
>>
>> Hmm, I'm not sure that is infact the case. The check_alu_op() is handled for
>> 32 and 64 bit alu op case. So in the opcode == BPF_MOV case the check_reg_arg()
>> on the dst register is done for both at that point, whereas retaining any
>> current state should only be valid in 64 bit mov case, e.g. think of pointer
>> types, these really need to be scratched here. I think it would make sense that
>> after checking src operand we hold a temporary copy of its state and use that
>> for setting regs[insn->dst_reg] later on under BPF_ALU64.
>
> The check_alu_op() call handles 32bit and 64bit cases, but then in the
> 32bit case
> mark_reg_unknown() is called, discarding all the dst register state.
> I think this is equivalent to keeping a copy of dst and always marking
> dst as unknown.
>
> I think we could actually always use check_reg_arg() with DST_OP_NO_MARK:
>
> In the 32bit case, we call mark_reg_unknown() anyways.
>
> In the 64bit case, we copy src to dst, so marking dst as unknown is pointless.
>
> For plain BPF, we call __mark_reg_known() anyways.
For imms this approach would be buggy since we leave a stale reg->off behind
which is uncleared from previous reg state. So for them the mark_reg_unknown()
is useful in the sense that it clears all reg state whereas __mark_reg_known()
might only initialize a subset of it.
^ permalink raw reply
* Re: [RFC bpf-next 3/6] xsk: don't allow umem replace at stack level
From: Björn Töpel @ 2018-07-30 12:00 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
Björn Töpel, Karlsson, Magnus, oss-drivers, Netdev
In-Reply-To: <20180726214148.2087-4-jakub.kicinski@netronome.com>
Den tors 26 juli 2018 kl 23:44 skrev Jakub Kicinski
<jakub.kicinski@netronome.com>:
>
> Currently drivers have to check if they already have a umem
> installed for a given queue and return an error if so. Make
> better use of XDP_QUERY_XSK_UMEM and move this functionality
> to the core.
>
> We need to keep rtnl across the calls now.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> ---
> include/linux/netdevice.h | 7 ++++---
> net/xdp/xdp_umem.c | 37 ++++++++++++++++++++++++++++---------
> 2 files changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 6717dc7e8fbf..a5a34f0fb485 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -872,10 +872,10 @@ struct netdev_bpf {
> struct {
> struct bpf_offloaded_map *offmap;
> };
> - /* XDP_SETUP_XSK_UMEM */
> + /* XDP_QUERY_XSK_UMEM, XDP_SETUP_XSK_UMEM */
> struct {
> - struct xdp_umem *umem;
> - u16 queue_id;
> + struct xdp_umem *umem; /* out for query*/
> + u16 queue_id; /* in for query */
> } xsk;
> };
> };
> @@ -3566,6 +3566,7 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
> int fd, u32 flags);
> u32 __dev_xdp_query(struct net_device *dev, bpf_op_t xdp_op,
> enum bpf_netdev_command cmd);
> +int xdp_umem_query(struct net_device *dev, u16 queue_id);
>
> int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
> int dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
> diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
> index c199d66b5f3f..911ca6d3cb5a 100644
> --- a/net/xdp/xdp_umem.c
> +++ b/net/xdp/xdp_umem.c
> @@ -11,6 +11,8 @@
> #include <linux/slab.h>
> #include <linux/bpf.h>
> #include <linux/mm.h>
> +#include <linux/netdevice.h>
> +#include <linux/rtnetlink.h>
>
> #include "xdp_umem.h"
> #include "xsk_queue.h"
> @@ -40,6 +42,21 @@ void xdp_del_sk_umem(struct xdp_umem *umem, struct xdp_sock *xs)
> }
> }
>
> +int xdp_umem_query(struct net_device *dev, u16 queue_id)
> +{
> + struct netdev_bpf bpf;
> +
> + ASSERT_RTNL();
> +
> + memset(&bpf, 0, sizeof(bpf));
> + bpf.command = XDP_QUERY_XSK_UMEM;
> + bpf.xsk.queue_id = queue_id;
> +
> + if (!dev->netdev_ops->ndo_bpf)
> + return 0;
> + return dev->netdev_ops->ndo_bpf(dev, &bpf) ?: !!bpf.xsk.umem;
> +}
> +
> int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
> u32 queue_id, u16 flags)
> {
> @@ -62,28 +79,30 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
> bpf.command = XDP_QUERY_XSK_UMEM;
>
> rtnl_lock();
> - err = dev->netdev_ops->ndo_bpf(dev, &bpf);
> - rtnl_unlock();
> -
> - if (err)
> - return force_zc ? -ENOTSUPP : 0;
> + err = xdp_umem_query(dev, queue_id);
> + if (err) {
> + err = err < 0 ? -ENOTSUPP : -EBUSY;
> + goto err_rtnl_unlock;
> + }
>
> bpf.command = XDP_SETUP_XSK_UMEM;
> bpf.xsk.umem = umem;
> bpf.xsk.queue_id = queue_id;
>
> - rtnl_lock();
> err = dev->netdev_ops->ndo_bpf(dev, &bpf);
> - rtnl_unlock();
> -
> if (err)
> - return force_zc ? err : 0; /* fail or fallback */
> + goto err_rtnl_unlock;
> + rtnl_unlock();
>
> dev_hold(dev);
> umem->dev = dev;
> umem->queue_id = queue_id;
> umem->zc = true;
> return 0;
> +
> +err_rtnl_unlock:
> + rtnl_unlock();
> + return force_zc ? err : 0; /* fail or fallback */
> }
>
> static void xdp_umem_clear_dev(struct xdp_umem *umem)
> --
> 2.17.1
>
Nice!
For a non-RFC version,
Acked-by: Björn Töpel <bjorn.topel@intel.com>
^ permalink raw reply
* Re: [PATCH 07/10] dt-bindings: phy: add DT binding for Microsemi Ocelot SerDes muxing
From: Andrew Lunn @ 2018-07-30 13:34 UTC (permalink / raw)
To: Quentin Schulz
Cc: alexandre.belloni, ralf, paul.burton, jhogan, robh+dt,
mark.rutland, davem, kishon, f.fainelli, linux-mips, devicetree,
linux-kernel, netdev, allan.nielsen, thomas.petazzoni
In-Reply-To: <cd75c96640cc7fe306ee355acb1db85adb5b796f.1532954208.git-series.quentin.schulz@bootlin.com>
> +Required properties:
> +
> +- compatible: should be "mscc,vsc7514-serdes"
> +- #phy-cells : from the generic phy bindings, must be 3. The first number
> + defines the kind of Serdes (1 for SERDES1G_X, 6 for
> + SERDES6G_X), the second defines the macros in the specified
> + kind of Serdes (X for SERDES1G_X or SERDES6G_X) and the
> + last one defines the input port to use for a given SerDes
> + macro,
It looks like there are some space vs tab issues here.
> +
> +Example:
> +
> + serdes: serdes {
Maybe this should be serdes-mux? The SERDES itself should have some
registers somewhere. If you ever decide to make use of phylink,
e.g. to support SFP, you are going to need to know if the SERDES is
up. So you might need to add the actual SERDES device, in addition to
the mux for the SERDES.
> + compatible = "mscc,vsc7514-serdes";
> + #phy-cells = <3>;
> + };
^ permalink raw reply
* Re: [RFC PATCH net-next v2 06/17] ethtool: support for netlink notifications
From: Jiri Pirko @ 2018-07-30 13:16 UTC (permalink / raw)
To: Michal Kubecek
Cc: netdev, linux-kernel, David Miller, Florian Fainelli,
Roopa Prabhu, Jakub Kicinski, John W. Linville
In-Reply-To: <e0e0433fd250dc331caa23aa739d7841e9602e0f.1532953989.git.mkubecek@suse.cz>
Mon, Jul 30, 2018 at 02:53:12PM CEST, mkubecek@suse.cz wrote:
[...]
>diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>index c1295c7a452e..c4b0c575d57e 100644
>--- a/include/linux/netdevice.h
>+++ b/include/linux/netdevice.h
>@@ -2444,6 +2444,7 @@ enum netdev_cmd {
> NETDEV_CVLAN_FILTER_DROP_INFO,
> NETDEV_SVLAN_FILTER_PUSH_INFO,
> NETDEV_SVLAN_FILTER_DROP_INFO,
>+ NETDEV_ETHTOOL,
I don't understand why this goes through netdev notifier. What's the
reason?
^ permalink raw reply
* Re: [RFC bpf-next 2/6] xsk: refactor xdp_umem_assign_dev()
From: Björn Töpel @ 2018-07-30 11:41 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
Björn Töpel, Karlsson, Magnus, oss-drivers, Netdev
In-Reply-To: <20180726214148.2087-3-jakub.kicinski@netronome.com>
Den tors 26 juli 2018 kl 23:44 skrev Jakub Kicinski
<jakub.kicinski@netronome.com>:
>
> Return early and only take the ref on dev once there is no possibility
> of failing.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> ---
> net/xdp/xdp_umem.c | 49 ++++++++++++++++++++--------------------------
> 1 file changed, 21 insertions(+), 28 deletions(-)
>
> diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
> index f47abb46c587..c199d66b5f3f 100644
> --- a/net/xdp/xdp_umem.c
> +++ b/net/xdp/xdp_umem.c
> @@ -56,41 +56,34 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
> if (force_copy)
> return 0;
>
> - dev_hold(dev);
> + if (!dev->netdev_ops->ndo_bpf || !dev->netdev_ops->ndo_xsk_async_xmit)
> + return force_zc ? -ENOTSUPP : 0; /* fail or fallback */
>
> - if (dev->netdev_ops->ndo_bpf && dev->netdev_ops->ndo_xsk_async_xmit) {
> - bpf.command = XDP_QUERY_XSK_UMEM;
> + bpf.command = XDP_QUERY_XSK_UMEM;
>
> - rtnl_lock();
> - err = dev->netdev_ops->ndo_bpf(dev, &bpf);
> - rtnl_unlock();
> + rtnl_lock();
> + err = dev->netdev_ops->ndo_bpf(dev, &bpf);
> + rtnl_unlock();
>
> - if (err) {
> - dev_put(dev);
> - return force_zc ? -ENOTSUPP : 0;
> - }
> + if (err)
> + return force_zc ? -ENOTSUPP : 0;
>
> - bpf.command = XDP_SETUP_XSK_UMEM;
> - bpf.xsk.umem = umem;
> - bpf.xsk.queue_id = queue_id;
> + bpf.command = XDP_SETUP_XSK_UMEM;
> + bpf.xsk.umem = umem;
> + bpf.xsk.queue_id = queue_id;
>
> - rtnl_lock();
> - err = dev->netdev_ops->ndo_bpf(dev, &bpf);
> - rtnl_unlock();
> + rtnl_lock();
> + err = dev->netdev_ops->ndo_bpf(dev, &bpf);
> + rtnl_unlock();
>
> - if (err) {
> - dev_put(dev);
> - return force_zc ? err : 0; /* fail or fallback */
> - }
> -
> - umem->dev = dev;
> - umem->queue_id = queue_id;
> - umem->zc = true;
> - return 0;
> - }
> + if (err)
> + return force_zc ? err : 0; /* fail or fallback */
>
> - dev_put(dev);
> - return force_zc ? -ENOTSUPP : 0; /* fail or fallback */
> + dev_hold(dev);
> + umem->dev = dev;
> + umem->queue_id = queue_id;
> + umem->zc = true;
> + return 0;
> }
>
> static void xdp_umem_clear_dev(struct xdp_umem *umem)
> --
> 2.17.1
>
Much cleaner! Please spin this w/o the RFC tag.
Acked-by: Björn Töpel <bjorn.topel@gmail.com>
Björn
^ permalink raw reply
* Re: [LKP] 7acf9d4237 BUG: kernel hang in test stage
From: Dmitry Safonov @ 2018-07-30 13:08 UTC (permalink / raw)
To: kernel test robot; +Cc: netdev, linux-kernel, LKP, David Miller
In-Reply-To: <20180730081219.GC30690@shao2-debian>
Thanks for the report, looking into this..
On Mon, 2018-07-30 at 16:12 +0800, kernel test robot wrote:
> Greetings,
>
> 0day kernel testing robot got the below dmesg and the first bad
> commit is
>
> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git master
>
> commit 7acf9d4237c46894e0fa0492dd96314a41742e84
> Author: Dmitry Safonov <dima@arista.com>
> AuthorDate: Fri Jul 27 16:54:44 2018 +0100
> Commit: David S. Miller <davem@davemloft.net>
> CommitDate: Sun Jul 29 12:50:19 2018 -0700
>
> netlink: Do not subscribe to non-existent groups
>
> Make ABI more strict about subscribing to group > ngroups.
> Code doesn't check for that and it looks bogus.
> (one can subscribe to non-existing group)
> Still, it's possible to bind() to all possible groups with (-1)
>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Steffen Klassert <steffen.klassert@secunet.com>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Dmitry Safonov <dima@arista.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
>
> 9939a46d90 NET: stmmac: align DMA stuff to largest cache line length
> 7acf9d4237 netlink: Do not subscribe to non-existent groups
> 25432eba9c openvswitch: meter: Fix setting meter id for new entries
> +--------------------------------------------------+------------+--
> ----------+------------+
> > | 9939a46d90 |
> > 7acf9d4237 | 25432eba9c |
>
> +--------------------------------------------------+------------+--
> ----------+------------+
> > boot_successes | 20 |
> > 4 | 4 |
> > boot_failures | 22 |
> > 11 | 11 |
> > INFO:task_blocked_for_more_than#seconds |
> > 22 | | |
> > Kernel_panic-not_syncing:hung_task:blocked_tasks |
> > 22 | | |
> > BUG:kernel_hang_in_test_stage | 0 |
> > 11 | 11 |
>
> +--------------------------------------------------+------------+--
> ----------+------------+
>
> [ 1414.113020] Writes: Total: 373492244 Max/Min: 0/0 Fail: 0
> [ 1472.149785] CE: Reprogramming failure. Giving up
> [ 1475.552294] Writes: Total: 387526813 Max/Min: 0/0 Fail: 0
> [ 1509.501097] CE: Reprogramming failure. Giving up
> [ 1536.992336] Writes: Total: 403788741 Max/Min: 0/0 Fail: 0
> BUG: kernel hang in test stage
>
>
> # HH:MM
> RESULT GOOD BAD GOOD_BUT_DIRTY DIRTY_NOT_BAD
> git bisect start 0a930dcfc77cb12bbf9bffd69fbadcb0c237f577
> d72e90f33aa4709ebecc5005562f52335e106a60 --
> git bisect bad f605359102be537b6217dc49600f4369d1f04932 #
> 07:19 B 0 11 27 0 Merge 'm68k/master' into devel-
> catchup-201807300507
> git bisect bad 375347c493a490f493fafa02f30fa23179acbdb9 #
> 07:53 B 0 11 27 0 Merge 'linux-review/Todd-
> Poynor/staging-gasket-fixes-and-cleanups/20180730-034316' into devel-
> catchup-201807300507
> git bisect good 86b34080126c3f4254a3f9e8d5dcb447d9233bc8 #
> 08:05 G 11 0 6 6 0day base guard for 'devel-catchup-
> 201807300507'
> git bisect bad ed28433e6f289d33dd6b4bbf627a60396234db58 #
> 08:40 B 0 1 17 0 Merge 'net/master' into devel-
> catchup-201807300507
> git bisect good 5b891af7fca14526b2a87c6f38b004e2df655ef4 #
> 08:54 G 11 0 11 11 bpf: Replace [u]int32_t and
> [u]int64_t in libbpf
> git bisect good c259b4fb33ee6e7667bf1d34bf0803b7c5fdbdce #
> 09:03 G 10 0 3 3 netdevsim: don't leak devlink
> resources
> git bisect good b0753408aadf32c7ece9e6b765017881e54af833 #
> 09:17 G 11 0 3 3 net: mdio-mux: bcm-iproc: fix wrong
> getter and setter pair
> git bisect good 71eb5255f55bdb484d35ff7c9a1803f453dfbf82 #
> 09:25 G 11 0 11 11 bpf: use GFP_ATOMIC instead of
> GFP_KERNEL in bpf_parse_prog()
> git bisect good 6d27c6dd1012e7be748ec05da78556319718fbe7 #
> 09:39 G 11 0 4 4 Merge branch 'net-socket-Fix-
> potential-spectre-v1-gadgets'
> git bisect good 9939a46d90c6c76f4533d534dbadfa7b39dc6acc #
> 09:55 G 11 0 7 7 NET: stmmac: align DMA stuff to
> largest cache line length
> git bisect bad 25432eba9cd8f2ef5afef55be811b010a004b5fa #
> 10:28 B 0 11 27 0 openvswitch: meter: Fix setting meter
> id for new entries
> git bisect bad 7acf9d4237c46894e0fa0492dd96314a41742e84 #
> 11:03 B 0 6 22 0 netlink: Do not subscribe to non-
> existent groups
> # first bad commit: [7acf9d4237c46894e0fa0492dd96314a41742e84]
> netlink: Do not subscribe to non-existent groups
> git bisect good 9939a46d90c6c76f4533d534dbadfa7b39dc6acc #
> 11:14 G 31 0 15 22 NET: stmmac: align DMA stuff to
> largest cache line length
> # extra tests with debug options
> git bisect bad 7acf9d4237c46894e0fa0492dd96314a41742e84 #
> 11:48 B 0 11 27 0 netlink: Do not subscribe to non-
> existent groups
> # extra tests on HEAD of linux-devel/devel-catchup-201807300507
> git bisect bad 0a930dcfc77cb12bbf9bffd69fbadcb0c237f577 #
> 11:49 B 0 13 31 0 0day head guard for 'devel-catchup-
> 201807300507'
> # extra tests on tree/branch net/master
> git bisect bad 25432eba9cd8f2ef5afef55be811b010a004b5fa #
> 11:50 B 0 11 27 0 openvswitch: meter: Fix setting meter
> id for new entries
> # extra tests with first bad commit reverted
> git bisect good e61543e57747e8fd80390af39504365a392cef2b #
> 12:04 G 11 0 4 4 Revert "netlink: Do not subscribe to
> non-existent groups"
>
> ---
> 0-DAY kernel test infrastructure Open Source
> Technology Center
> https://lists.01.org/pipermail/lkp Intel
> Corporation
^ permalink raw reply
* Re: [RFC PATCH net-next v2 00/17] ethtool netlink interface (WiP)
From: Jiri Pirko @ 2018-07-30 13:07 UTC (permalink / raw)
To: Michal Kubecek
Cc: netdev, linux-kernel, David Miller, Florian Fainelli,
Roopa Prabhu, Jakub Kicinski, John W. Linville
In-Reply-To: <cover.1532953989.git.mkubecek@suse.cz>
Mon, Jul 30, 2018 at 02:52:42PM CEST, mkubecek@suse.cz wrote:
>A netlink based interface for ethtool is a recurring discussion theme;
>such discussion happens from time to time but never seems to reach any
>clear conclusion except that netlink interface is desperately needed.
>I'm sending this hoping that having a proposal (even if partial) on the
>table could help move the discussion further.
>
>The interface used for communication between ethtool and kernel is based on
>ioctl() and suffers from many problems. The most pressing seems the be the
>lack of extensibility. While some of the newer commands use structures
>designed to allow future extensions (e.g. GFEATURES or TEST), most either
>allow no extension at all (GPAUSEPARAM, GCOALESCE) or only limited set of
>reserved fields (GDRVINFO, GEEE). Even most of those which support future
>extensions limit the data types that can be used.
>
>This series aims to provide an alternative interface based on netlink which
>is what other network configuration utilities use. In particular, it uses
>generic netlink (family "ethtool"). The goal is to provide an interface
>which would be extensible, flexible and practical both for ethtool and for
>other network configuration tools (e.g. wicked, systemd-networkd or
>NetworkManager).
>
>The interface is documented in Documentation/networking/ethtool-netlink.txt
>
>A series for ethtool utility will follow shortly.
>
>Basic concepts:
>
>- the interface is based on generic netlink (family name "ethtool")
>
>- the goal is to provide all features of ioctl interface but allow
> easier future extensions
>
>- inextensibility of ioctl interface resulted in way too many commands,
> many of them obsoleted by newer ones; reduce the number by ignoring the
> obsolete commands and grouping some together
>
>- for "set" type commands, netlink allows providing only the attributes to
> be changed; therefore we don't need a get-modify-set cycle (which is
> inherently racy), userspace can simply say what it wants to change
>
>- provide notifications to multicast group "monitor" like rtnetlink
> does, i.e. in the form of messages close to replies to "get" requests
>
>- allow dump requests to get some information about all network defices
> providing it
>
>- be less dependent on ethtool and kernel being in sync; allow e.g. saying
> "ethtool -s eth0 advertise foo off" without ethtool knowing what "foo"
> means; it's kernel's job to know what mode "xyz" is and if it exists
> and is supported
>
>Main changes again RFC v1:
>
>- support dumps for all "get" requests
>- provide notifications for changes related to supported request types
>- support getting string sets (both global and per device)
>- support getting/setting device features
>- get rid of family specific header, everything passed as attributes
>- split netlink code into multiple files in net/ethtool/ directory
>
>ToDo / open questions:
>
>- as some comments in discussion on v1 pointed out, some features of
> ethtool would rather belong to devlink; phy_tunables and phy_stats
> seem to be candidates, maybe part of drvinfo; are there more?
>
>- another question is where to do the split; should ethtool use devlink
> API for these or can we provide them in ethtool API as well but with
> devlink backend (for communication with NIC)
My idea was to provide a compat layer for it. But I also wanted to
change the implementation of driver callbacks and provide compat layer
for it too. Current ethtool callbacks are very tightly wrapped around
the uapi structs.
Please see the last slide:
http://vger.kernel.org/netconf2018_files/JiriPirko_netconf2018.pdf
I started to work on POC implementation but got distracted.
Perhaps we can sync over a beer :)
>
>- currently, all communication with drivers via ethtool_ops is done
> under RTNL as this is what ioctl interface does and I suspect many
> ethtool_ops rely on that; can we do without RTNL?
>
>- notifications are sent whenever a change is done via netlink API or
> ioctl API and for netdev features also whenever they are updated using
> netdev_change_features(); it would be desirable to notify also about
> link state and negotiation result (speed/duplex and partner link
> modes) but it would be more tricky
>
>- find reasonable format for data transfers (e.g. eeprom dump or flash);
> I don't have clear idea how big these can get and if 64 KB limit on
> attribute size (including nested ones) is a problem; if so, dumps can
> be an answer for dumps, some kind of multi-message requests would be
> needed for flashes
This is another thing that should go into devlink as it is not
netdev-handled. We have now a "region" facility there which can be
re-used.
>
>- while the netlink interface allows easy future extensions, ethtool_ops
> interface does not; some settings could be implemented using tunables and
> accessed via relevant netlink messages (as well as tunables) from
> userspace but in the long term, something better will be needed
>
>- it would be nice if driver could provide useful error/warning messages to
> be passed to userspace via extended ACK; example: while testing, I found
> a driver which only allows values 0, 1, 3 and 10000 for certain parameter
> but the only way poor user can find out is either by trying all values or
> by checking driver source
>
>- some of the functions for GET_SETTINGS and GET_PARAMS are quite
> similar (e.g. ethtool_get_*); it might be beneficial to introduce some
> "ops", leave only "parse", "prepare", "size" and "fill" handlers and
> make the rest generic (like ethnl_dumpit()).
>
>- the counts and sizes in GET_DRVINFO reply seem to be a relic of the
> past and if userspace needs them, there are (or will be) other ways to
> get them; they should most likely go
>
>
>Michal Kubecek (17):
> netlink: introduce nla_put_bitfield32()
> ethtool: move to its own directory
> ethtool: introduce ethtool netlink interface
> ethtool: helper functions for netlink interface
> ethtool: netlink bitset handling
> ethtool: support for netlink notifications
> ethtool: implement EVENT notifications
> ethtool: implement GET_STRSET message
> ethtool: implement GET_DRVINFO message
> ethtool: implement GET_SETTINGS message
> ethtool: implement GET_SETTINGS request for features
> ethtool: implement SET_SETTINGS notification
> ethtool: implement SET_SETTINGS message
> ethtool: implement SET_SETTINGS request for features
> ethtool: implement GET_PARAMS message
> ethtool: implement SET_PARAMS notification
> ethtool: implement SET_PARAMS message
>
> Documentation/networking/ethtool-netlink.txt | 558 ++++++++
> include/linux/ethtool_netlink.h | 17 +
> include/linux/netdevice.h | 25 +
> include/net/netlink.h | 15 +
> include/uapi/linux/ethtool.h | 7 +
> include/uapi/linux/ethtool_netlink.h | 325 +++++
> net/Kconfig | 7 +
> net/Makefile | 2 +-
> net/core/Makefile | 2 +-
> net/core/dev.c | 27 +-
> net/ethtool/Makefile | 7 +
> net/ethtool/common.c | 242 ++++
> net/ethtool/common.h | 26 +
> net/ethtool/drvinfo.c | 131 ++
> net/{core/ethtool.c => ethtool/ioctl.c} | 310 ++---
> net/ethtool/netlink.c | 840 ++++++++++++
> net/ethtool/netlink.h | 169 +++
> net/ethtool/params.c | 1008 ++++++++++++++
> net/ethtool/settings.c | 1230 ++++++++++++++++++
> net/ethtool/strset.c | 552 ++++++++
> 20 files changed, 5269 insertions(+), 231 deletions(-)
> create mode 100644 Documentation/networking/ethtool-netlink.txt
> create mode 100644 include/linux/ethtool_netlink.h
> create mode 100644 include/uapi/linux/ethtool_netlink.h
> create mode 100644 net/ethtool/Makefile
> create mode 100644 net/ethtool/common.c
> create mode 100644 net/ethtool/common.h
> create mode 100644 net/ethtool/drvinfo.c
> rename net/{core/ethtool.c => ethtool/ioctl.c} (88%)
> create mode 100644 net/ethtool/netlink.c
> create mode 100644 net/ethtool/netlink.h
> create mode 100644 net/ethtool/params.c
> create mode 100644 net/ethtool/settings.c
> create mode 100644 net/ethtool/strset.c
>
>--
>2.18.0
>
^ permalink raw reply
* [PATCH net-next] fib_rules: NULL check before kfree is not needed
From: YueHaibing @ 2018-07-30 13:07 UTC (permalink / raw)
To: davem, roopa
Cc: linux-kernel, netdev, idosch, dsahern, ktkhai, sharpd, YueHaibing
kfree(NULL) is safe,so this removes NULL check before freeing the mem
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
net/core/fib_rules.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c
index f64aa13..0ff3953 100644
--- a/net/core/fib_rules.c
+++ b/net/core/fib_rules.c
@@ -924,8 +924,7 @@ int fib_nl_delrule(struct sk_buff *skb, struct nlmsghdr *nlh,
return 0;
errout:
- if (nlrule)
- kfree(nlrule);
+ kfree(nlrule);
rules_ops_put(ops);
return err;
}
--
2.7.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