* [PATCH net-next v4 0/5] Add MPLS actions to TC
From: John Hurley @ 2019-07-01 12:30 UTC (permalink / raw)
To: netdev
Cc: davem, jiri, xiyou.wangcong, dsahern, simon.horman,
jakub.kicinski, oss-drivers, John Hurley
This patchset introduces a new TC action module that allows the
manipulation of the MPLS headers of packets. The code impliments
functionality including push, pop, and modify.
Also included are tests for the new funtionality. Note that these will
require iproute2 changes to be submitted soon.
NOTE: these patches are applied to net-next along with the patch:
[PATCH net 1/1] net: openvswitch: fix csum updates for MPLS actions
This patch has been accepted into net but, at time of posting, is not yet
in net-next.
v3-v4:
- refactor and reuse OvS code (Cong Wang)
- use csum API rather than skb_post*rscum to update skb->csum (Cong Wang)
- remove unnecessary warning (Cong Wang)
- add comments to uapi attributes (David Ahern)
- set strict type policy check for TCA_MPLS_UNSPEC (David Ahern)
- expand/improve extack messages (David Ahern)
- add option to manually set BOS
v2-v3:
- remove a few unnecessary line breaks (Jiri Pirko)
- retract hw offload patch from set (resubmit with driver changes) (Jiri)
v1->v2:
- ensure TCA_ID_MPLS does not conflict with TCA_ID_CTINFO (Davide Caratti)
John Hurley (5):
net: core: move push MPLS functionality from OvS to core helper
net: core: move pop MPLS functionality from OvS to core helper
net: core: add MPLS update core helper and use in OvS
net: sched: add mpls manipulation actions to TC
selftests: tc-tests: actions: add MPLS tests
include/linux/skbuff.h | 3 +
include/net/tc_act/tc_mpls.h | 29 +
include/uapi/linux/pkt_cls.h | 3 +-
include/uapi/linux/tc_act/tc_mpls.h | 33 +
net/core/skbuff.c | 140 ++++
net/openvswitch/actions.c | 81 +-
net/sched/Kconfig | 11 +
net/sched/Makefile | 1 +
net/sched/act_mpls.c | 413 +++++++++++
.../tc-testing/tc-tests/actions/mpls.json | 812 +++++++++++++++++++++
10 files changed, 1453 insertions(+), 73 deletions(-)
create mode 100644 include/net/tc_act/tc_mpls.h
create mode 100644 include/uapi/linux/tc_act/tc_mpls.h
create mode 100644 net/sched/act_mpls.c
create mode 100644 tools/testing/selftests/tc-testing/tc-tests/actions/mpls.json
--
2.7.4
^ permalink raw reply
* [PATCH net-next 3/3] net/mlx5e: Register devlink ports for physical link, PCI PF, VFs
From: Parav Pandit @ 2019-07-01 12:27 UTC (permalink / raw)
To: netdev; +Cc: jiri, saeedm, jakub.kicinski, Parav Pandit
In-Reply-To: <20190701122734.18770-1-parav@mellanox.com>
Register devlink port of physical port, PCI PF and PCI VF flavour
for each PF, VF when a given devlink instance is in switchdev mode.
Implement ndo_get_devlink_port callback API to make use of registered
devlink ports.
This eliminates ndo_get_phys_port_name() and ndo_get_port_parent_id()
callbacks. Hence, remove them.
An example output with 2 VFs, without a PF and single uplink port is
below.
$ devlink port show
pci/0000:06:00.0/65535: type eth netdev ens2f0 flavour physical
pci/0000:05:00.0/1: type eth netdev eth1 flavour pcivf pfnum 0 vfnum 0
pci/0000:05:00.0/2: type eth netdev eth2 flavour pcivf pfnum 0 vfnum 1
Reviewed-by: Roi Dayan <roid@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Parav Pandit <parav@mellanox.com>
---
.../net/ethernet/mellanox/mlx5/core/en_rep.c | 108 +++++++++++++-----
.../net/ethernet/mellanox/mlx5/core/en_rep.h | 1 +
2 files changed, 78 insertions(+), 31 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
index 330034fcdfc5..aa47be3c139f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
@@ -37,6 +37,7 @@
#include <net/act_api.h>
#include <net/netevent.h>
#include <net/arp.h>
+#include <net/devlink.h>
#include "eswitch.h"
#include "en.h"
@@ -1119,32 +1120,6 @@ static int mlx5e_rep_close(struct net_device *dev)
return ret;
}
-static int mlx5e_rep_get_phys_port_name(struct net_device *dev,
- char *buf, size_t len)
-{
- struct mlx5e_priv *priv = netdev_priv(dev);
- struct mlx5e_rep_priv *rpriv = priv->ppriv;
- struct mlx5_eswitch_rep *rep = rpriv->rep;
- unsigned int fn;
- int ret;
-
- fn = PCI_FUNC(priv->mdev->pdev->devfn);
- if (fn >= MLX5_MAX_PORTS)
- return -EOPNOTSUPP;
-
- if (rep->vport == MLX5_VPORT_UPLINK)
- ret = snprintf(buf, len, "p%d", fn);
- else if (rep->vport == MLX5_VPORT_PF)
- ret = snprintf(buf, len, "pf%d", fn);
- else
- ret = snprintf(buf, len, "pf%dvf%d", fn, rep->vport - 1);
-
- if (ret >= len)
- return -EOPNOTSUPP;
-
- return 0;
-}
-
static int
mlx5e_rep_setup_tc_cls_flower(struct mlx5e_priv *priv,
struct tc_cls_flower_offload *cls_flower, int flags)
@@ -1298,17 +1273,24 @@ static int mlx5e_uplink_rep_set_vf_vlan(struct net_device *dev, int vf, u16 vlan
return 0;
}
+static struct devlink_port *mlx5e_get_devlink_port(struct net_device *dev)
+{
+ struct mlx5e_priv *priv = netdev_priv(dev);
+ struct mlx5e_rep_priv *rpriv = priv->ppriv;
+
+ return &rpriv->dl_port;
+}
+
static const struct net_device_ops mlx5e_netdev_ops_rep = {
.ndo_open = mlx5e_rep_open,
.ndo_stop = mlx5e_rep_close,
.ndo_start_xmit = mlx5e_xmit,
- .ndo_get_phys_port_name = mlx5e_rep_get_phys_port_name,
.ndo_setup_tc = mlx5e_rep_setup_tc,
+ .ndo_get_devlink_port = mlx5e_get_devlink_port,
.ndo_get_stats64 = mlx5e_rep_get_stats,
.ndo_has_offload_stats = mlx5e_rep_has_offload_stats,
.ndo_get_offload_stats = mlx5e_rep_get_offload_stats,
.ndo_change_mtu = mlx5e_rep_change_mtu,
- .ndo_get_port_parent_id = mlx5e_rep_get_port_parent_id,
};
static const struct net_device_ops mlx5e_netdev_ops_uplink_rep = {
@@ -1316,8 +1298,8 @@ static const struct net_device_ops mlx5e_netdev_ops_uplink_rep = {
.ndo_stop = mlx5e_close,
.ndo_start_xmit = mlx5e_xmit,
.ndo_set_mac_address = mlx5e_uplink_rep_set_mac,
- .ndo_get_phys_port_name = mlx5e_rep_get_phys_port_name,
.ndo_setup_tc = mlx5e_rep_setup_tc,
+ .ndo_get_devlink_port = mlx5e_get_devlink_port,
.ndo_get_stats64 = mlx5e_get_stats,
.ndo_has_offload_stats = mlx5e_rep_has_offload_stats,
.ndo_get_offload_stats = mlx5e_rep_get_offload_stats,
@@ -1330,7 +1312,6 @@ static const struct net_device_ops mlx5e_netdev_ops_uplink_rep = {
.ndo_get_vf_config = mlx5e_get_vf_config,
.ndo_get_vf_stats = mlx5e_get_vf_stats,
.ndo_set_vf_vlan = mlx5e_uplink_rep_set_vf_vlan,
- .ndo_get_port_parent_id = mlx5e_rep_get_port_parent_id,
.ndo_set_features = mlx5e_set_features,
};
@@ -1731,6 +1712,55 @@ static const struct mlx5e_profile mlx5e_uplink_rep_profile = {
.max_tc = MLX5E_MAX_NUM_TC,
};
+static bool
+is_devlink_port_supported(const struct mlx5_core_dev *dev,
+ const struct mlx5e_rep_priv *rpriv)
+{
+ return rpriv->rep->vport == MLX5_VPORT_UPLINK ||
+ rpriv->rep->vport == MLX5_VPORT_PF ||
+ mlx5_eswitch_is_vf_vport(dev->priv.eswitch, rpriv->rep->vport);
+}
+
+static int register_devlink_port(struct mlx5_core_dev *dev,
+ struct mlx5e_rep_priv *rpriv)
+{
+ struct devlink *devlink = priv_to_devlink(dev);
+ struct mlx5_eswitch_rep *rep = rpriv->rep;
+ struct netdev_phys_item_id ppid = {};
+ int ret;
+
+ if (!is_devlink_port_supported(dev, rpriv))
+ return 0;
+
+ ret = mlx5e_rep_get_port_parent_id(rpriv->netdev, &ppid);
+ if (ret)
+ return ret;
+
+ if (rep->vport == MLX5_VPORT_UPLINK)
+ devlink_port_attrs_set(&rpriv->dl_port,
+ DEVLINK_PORT_FLAVOUR_PHYSICAL,
+ PCI_FUNC(dev->pdev->devfn), false, 0,
+ &ppid.id[0], ppid.id_len);
+ else if (rep->vport == MLX5_VPORT_PF)
+ devlink_port_attrs_pci_pf_set(&rpriv->dl_port, rep->vport,
+ &ppid.id[0], ppid.id_len,
+ dev->pdev->devfn);
+ else if (mlx5_eswitch_is_vf_vport(dev->priv.eswitch, rpriv->rep->vport))
+ devlink_port_attrs_pci_vf_set(&rpriv->dl_port, rep->vport,
+ &ppid.id[0], ppid.id_len,
+ dev->pdev->devfn,
+ rep->vport - 1);
+
+ return devlink_port_register(devlink, &rpriv->dl_port, rep->vport);
+}
+
+static void unregister_devlink_port(struct mlx5_core_dev *dev,
+ struct mlx5e_rep_priv *rpriv)
+{
+ if (is_devlink_port_supported(dev, rpriv))
+ devlink_port_unregister(&rpriv->dl_port);
+}
+
/* e-Switch vport representors */
static int
mlx5e_vport_rep_load(struct mlx5_core_dev *dev, struct mlx5_eswitch_rep *rep)
@@ -1782,15 +1812,27 @@ mlx5e_vport_rep_load(struct mlx5_core_dev *dev, struct mlx5_eswitch_rep *rep)
goto err_detach_netdev;
}
+ err = register_devlink_port(dev, rpriv);
+ if (err) {
+ esw_warn(dev, "Failed to register devlink port %d\n",
+ rep->vport);
+ goto err_neigh_cleanup;
+ }
+
err = register_netdev(netdev);
if (err) {
pr_warn("Failed to register representor netdev for vport %d\n",
rep->vport);
- goto err_neigh_cleanup;
+ goto err_devlink_cleanup;
}
+ if (is_devlink_port_supported(dev, rpriv))
+ devlink_port_type_eth_set(&rpriv->dl_port, netdev);
return 0;
+err_devlink_cleanup:
+ unregister_devlink_port(dev, rpriv);
+
err_neigh_cleanup:
mlx5e_rep_neigh_cleanup(rpriv);
@@ -1813,9 +1855,13 @@ mlx5e_vport_rep_unload(struct mlx5_eswitch_rep *rep)
struct mlx5e_rep_priv *rpriv = mlx5e_rep_to_rep_priv(rep);
struct net_device *netdev = rpriv->netdev;
struct mlx5e_priv *priv = netdev_priv(netdev);
+ struct mlx5_core_dev *dev = priv->mdev;
void *ppriv = priv->ppriv;
+ if (is_devlink_port_supported(dev, rpriv))
+ devlink_port_type_clear(&rpriv->dl_port);
unregister_netdev(netdev);
+ unregister_devlink_port(dev, rpriv);
mlx5e_rep_neigh_cleanup(rpriv);
mlx5e_detach_netdev(priv);
if (rep->vport == MLX5_VPORT_UPLINK)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
index d4585f3b8cb2..c56e6ee4350c 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
@@ -86,6 +86,7 @@ struct mlx5e_rep_priv {
struct mlx5_flow_handle *vport_rx_rule;
struct list_head vport_sqs_list;
struct mlx5_rep_uplink_priv uplink_priv; /* valid for uplink rep */
+ struct devlink_port dl_port;
};
static inline
--
2.19.2
^ permalink raw reply related
* [PATCH net-next 2/3] devlink: Introduce PCI VF port flavour and port attribute
From: Parav Pandit @ 2019-07-01 12:27 UTC (permalink / raw)
To: netdev; +Cc: jiri, saeedm, jakub.kicinski, Parav Pandit
In-Reply-To: <20190701122734.18770-1-parav@mellanox.com>
In an eswitch, PCI VF may have port which is normally represented using
a representor netdevice.
To have better visibility of eswitch port, its association with VF,
its representor netdevice and port number, introduce a PCI VF
port flavour.
When devlink port flavour is PCI VF, fill up PCI VF attributes of
the port.
Extend port name creation using PCI PF and VF number scheme on best
effort basis, so that vendor drivers can skip defining their own scheme.
$ devlink port show
pci/0000:05:00.0/0: type eth netdev eth0 flavour pcipf pfnum 0
pci/0000:05:00.0/1: type eth netdev eth1 flavour pcivf pfnum 0 vfnum 0
pci/0000:05:00.0/2: type eth netdev eth2 flavour pcivf pfnum 0 vfnum 1
Acked-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Parav Pandit <parav@mellanox.com>
---
include/net/devlink.h | 11 +++++++++++
include/uapi/linux/devlink.h | 6 ++++++
net/core/devlink.c | 36 ++++++++++++++++++++++++++++++++++++
3 files changed, 53 insertions(+)
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 8db9c0e83fb5..dff7c7797f3e 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -42,6 +42,11 @@ struct devlink_port_pci_pf_attrs {
u16 pf; /* Associated PCI PF for this port. */
};
+struct devlink_port_pci_vf_attrs {
+ u16 pf; /* Associated PCI PF for this port. */
+ u16 vf; /* Associated PCI VF for of the PCI PF for this port. */
+};
+
struct devlink_port_attrs {
u8 set:1,
split:1,
@@ -52,6 +57,7 @@ struct devlink_port_attrs {
struct netdev_phys_item_id switch_id;
union {
struct devlink_port_pci_pf_attrs pci_pf;
+ struct devlink_port_pci_vf_attrs pci_vf;
};
};
@@ -601,6 +607,11 @@ void devlink_port_attrs_pci_pf_set(struct devlink_port *devlink_port,
u32 port_number,
const unsigned char *switch_id,
unsigned char switch_id_len, u16 pf);
+void devlink_port_attrs_pci_vf_set(struct devlink_port *devlink_port,
+ u32 port_number,
+ const unsigned char *switch_id,
+ unsigned char switch_id_len,
+ u16 pf, u16 vf);
int devlink_sb_register(struct devlink *devlink, unsigned int sb_index,
u32 size, u16 ingress_pools_count,
u16 egress_pools_count, u16 ingress_tc_count,
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index f7323884c3fe..ffc993256527 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -173,6 +173,10 @@ enum devlink_port_flavour {
* the PCI PF. It is an internal
* port that faces the PCI PF.
*/
+ DEVLINK_PORT_FLAVOUR_PCI_VF, /* Represents eswitch port
+ * for the PCI VF. It is an internal
+ * port that faces the PCI VF.
+ */
};
enum devlink_param_cmode {
@@ -342,6 +346,8 @@ enum devlink_attr {
DEVLINK_ATTR_FLASH_UPDATE_STATUS_TOTAL, /* u64 */
DEVLINK_ATTR_PORT_PCI_PF_NUMBER, /* u16 */
+ DEVLINK_ATTR_PORT_PCI_VF_NUMBER, /* u16 */
+
/* add new attributes above here, update the policy in devlink.c */
__DEVLINK_ATTR_MAX,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 001f9e2c96f0..d62c4591351b 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -521,6 +521,12 @@ static int devlink_nl_port_attrs_put(struct sk_buff *msg,
if (nla_put_u16(msg, DEVLINK_ATTR_PORT_PCI_PF_NUMBER,
attrs->pci_pf.pf))
return -EMSGSIZE;
+ } else if (devlink_port->attrs.flavour == DEVLINK_PORT_FLAVOUR_PCI_VF) {
+ if (nla_put_u16(msg, DEVLINK_ATTR_PORT_PCI_PF_NUMBER,
+ attrs->pci_vf.pf) ||
+ nla_put_u16(msg, DEVLINK_ATTR_PORT_PCI_VF_NUMBER,
+ attrs->pci_vf.vf))
+ return -EMSGSIZE;
}
if (!attrs->split)
return 0;
@@ -5820,6 +5826,32 @@ void devlink_port_attrs_pci_pf_set(struct devlink_port *devlink_port,
}
EXPORT_SYMBOL_GPL(devlink_port_attrs_pci_pf_set);
+/**
+ * devlink_port_attrs_pci_vf_set - Set PCI VF port attributes
+ *
+ * @devlink_port: devlink port
+ * @port_number: number of the port that is facing a VF
+ * @pf: associated PF for the devlink port instance
+ * @vf: associated VF of a PF for the devlink port instance
+ * @switch_id: if the port is part of switch, this is buffer with ID,
+ * otwerwise this is NULL
+ * @switch_id_len: length of the switch_id buffer
+ */
+void devlink_port_attrs_pci_vf_set(struct devlink_port *devlink_port,
+ u32 port_number,
+ const unsigned char *switch_id,
+ unsigned char switch_id_len,
+ u16 pf, u16 vf)
+{
+ struct devlink_port_attrs *attrs = &devlink_port->attrs;
+
+ __devlink_port_attrs_set(devlink_port, DEVLINK_PORT_FLAVOUR_PCI_VF,
+ port_number, switch_id, switch_id_len);
+ attrs->pci_vf.pf = pf;
+ attrs->pci_vf.vf = vf;
+}
+EXPORT_SYMBOL_GPL(devlink_port_attrs_pci_vf_set);
+
static int __devlink_port_phys_port_name_get(struct devlink_port *devlink_port,
char *name, size_t len)
{
@@ -5847,6 +5879,10 @@ static int __devlink_port_phys_port_name_get(struct devlink_port *devlink_port,
case DEVLINK_PORT_FLAVOUR_PCI_PF:
n = snprintf(name, len, "pf%u", attrs->pci_pf.pf);
break;
+ case DEVLINK_PORT_FLAVOUR_PCI_VF:
+ n = snprintf(name, len, "pf%uvf%u",
+ attrs->pci_vf.pf, attrs->pci_vf.vf);
+ break;
}
if (n >= len)
--
2.19.2
^ permalink raw reply related
* [PATCH net-next 1/3] devlink: Introduce PCI PF port flavour and port attribute
From: Parav Pandit @ 2019-07-01 12:27 UTC (permalink / raw)
To: netdev; +Cc: jiri, saeedm, jakub.kicinski, Parav Pandit
In-Reply-To: <20190701122734.18770-1-parav@mellanox.com>
In an eswitch, PCI PF may have port which is normally represented
using a representor netdevice.
To have better visibility of eswitch port, its association with
PF, a representor netdevice and port number, introduce a PCI PF port
flavour and port attriute.
When devlink port flavour is PCI PF, fill up PCI PF attributes of the
port.
Extend port name creation using PCI PF number on best effort basis.
So that vendor drivers can skip defining their own scheme.
$ devlink port show
pci/0000:05:00.0/0: type eth netdev eth0 flavour pcipf pfnum 0
Acked-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Parav Pandit <parav@mellanox.com>
---
include/net/devlink.h | 11 ++++++
include/uapi/linux/devlink.h | 5 +++
net/core/devlink.c | 71 +++++++++++++++++++++++++++++-------
3 files changed, 73 insertions(+), 14 deletions(-)
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 6625ea068d5e..8db9c0e83fb5 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -38,6 +38,10 @@ struct devlink {
char priv[0] __aligned(NETDEV_ALIGN);
};
+struct devlink_port_pci_pf_attrs {
+ u16 pf; /* Associated PCI PF for this port. */
+};
+
struct devlink_port_attrs {
u8 set:1,
split:1,
@@ -46,6 +50,9 @@ struct devlink_port_attrs {
u32 port_number; /* same value as "split group" */
u32 split_subport_number;
struct netdev_phys_item_id switch_id;
+ union {
+ struct devlink_port_pci_pf_attrs pci_pf;
+ };
};
struct devlink_port {
@@ -590,6 +597,10 @@ void devlink_port_attrs_set(struct devlink_port *devlink_port,
u32 split_subport_number,
const unsigned char *switch_id,
unsigned char switch_id_len);
+void devlink_port_attrs_pci_pf_set(struct devlink_port *devlink_port,
+ u32 port_number,
+ const unsigned char *switch_id,
+ unsigned char switch_id_len, u16 pf);
int devlink_sb_register(struct devlink *devlink, unsigned int sb_index,
u32 size, u16 ingress_pools_count,
u16 egress_pools_count, u16 ingress_tc_count,
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 5287b42c181f..f7323884c3fe 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -169,6 +169,10 @@ enum devlink_port_flavour {
DEVLINK_PORT_FLAVOUR_DSA, /* Distributed switch architecture
* interconnect port.
*/
+ DEVLINK_PORT_FLAVOUR_PCI_PF, /* Represents eswitch port for
+ * the PCI PF. It is an internal
+ * port that faces the PCI PF.
+ */
};
enum devlink_param_cmode {
@@ -337,6 +341,7 @@ enum devlink_attr {
DEVLINK_ATTR_FLASH_UPDATE_STATUS_DONE, /* u64 */
DEVLINK_ATTR_FLASH_UPDATE_STATUS_TOTAL, /* u64 */
+ DEVLINK_ATTR_PORT_PCI_PF_NUMBER, /* u16 */
/* add new attributes above here, update the policy in devlink.c */
__DEVLINK_ATTR_MAX,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 89c533778135..001f9e2c96f0 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -517,6 +517,11 @@ static int devlink_nl_port_attrs_put(struct sk_buff *msg,
return -EMSGSIZE;
if (nla_put_u32(msg, DEVLINK_ATTR_PORT_NUMBER, attrs->port_number))
return -EMSGSIZE;
+ if (devlink_port->attrs.flavour == DEVLINK_PORT_FLAVOUR_PCI_PF) {
+ if (nla_put_u16(msg, DEVLINK_ATTR_PORT_PCI_PF_NUMBER,
+ attrs->pci_pf.pf))
+ return -EMSGSIZE;
+ }
if (!attrs->split)
return 0;
if (nla_put_u32(msg, DEVLINK_ATTR_PORT_SPLIT_GROUP, attrs->port_number))
@@ -5738,6 +5743,30 @@ void devlink_port_type_clear(struct devlink_port *devlink_port)
}
EXPORT_SYMBOL_GPL(devlink_port_type_clear);
+static void __devlink_port_attrs_set(struct devlink_port *devlink_port,
+ enum devlink_port_flavour flavour,
+ u32 port_number,
+ const unsigned char *switch_id,
+ unsigned char switch_id_len)
+{
+ struct devlink_port_attrs *attrs = &devlink_port->attrs;
+
+ if (WARN_ON(devlink_port->registered))
+ return;
+ attrs->set = true;
+ attrs->flavour = flavour;
+ attrs->port_number = port_number;
+ if (switch_id) {
+ attrs->switch_port = true;
+ if (WARN_ON(switch_id_len > MAX_PHYS_ITEM_ID_LEN))
+ switch_id_len = MAX_PHYS_ITEM_ID_LEN;
+ memcpy(attrs->switch_id.id, switch_id, switch_id_len);
+ attrs->switch_id.id_len = switch_id_len;
+ } else {
+ attrs->switch_port = false;
+ }
+}
+
/**
* devlink_port_attrs_set - Set port attributes
*
@@ -5761,25 +5790,36 @@ void devlink_port_attrs_set(struct devlink_port *devlink_port,
{
struct devlink_port_attrs *attrs = &devlink_port->attrs;
- if (WARN_ON(devlink_port->registered))
- return;
- attrs->set = true;
- attrs->flavour = flavour;
- attrs->port_number = port_number;
+ __devlink_port_attrs_set(devlink_port, flavour, port_number,
+ switch_id, switch_id_len);
attrs->split = split;
attrs->split_subport_number = split_subport_number;
- if (switch_id) {
- attrs->switch_port = true;
- if (WARN_ON(switch_id_len > MAX_PHYS_ITEM_ID_LEN))
- switch_id_len = MAX_PHYS_ITEM_ID_LEN;
- memcpy(attrs->switch_id.id, switch_id, switch_id_len);
- attrs->switch_id.id_len = switch_id_len;
- } else {
- attrs->switch_port = false;
- }
}
EXPORT_SYMBOL_GPL(devlink_port_attrs_set);
+/**
+ * devlink_port_attrs_pci_pf_set - Set PCI PF port attributes
+ *
+ * @devlink_port: devlink port
+ * @port_number: number of the port that is facing a PF
+ * @pf: associated PF for the devlink port instance
+ * @switch_id: if the port is part of switch, this is buffer with ID,
+ * otwerwise this is NULL
+ * @switch_id_len: length of the switch_id buffer
+ */
+void devlink_port_attrs_pci_pf_set(struct devlink_port *devlink_port,
+ u32 port_number,
+ const unsigned char *switch_id,
+ unsigned char switch_id_len, u16 pf)
+{
+ struct devlink_port_attrs *attrs = &devlink_port->attrs;
+
+ __devlink_port_attrs_set(devlink_port, DEVLINK_PORT_FLAVOUR_PCI_PF,
+ port_number, switch_id, switch_id_len);
+ attrs->pci_pf.pf = pf;
+}
+EXPORT_SYMBOL_GPL(devlink_port_attrs_pci_pf_set);
+
static int __devlink_port_phys_port_name_get(struct devlink_port *devlink_port,
char *name, size_t len)
{
@@ -5804,6 +5844,9 @@ static int __devlink_port_phys_port_name_get(struct devlink_port *devlink_port,
*/
WARN_ON(1);
return -EINVAL;
+ case DEVLINK_PORT_FLAVOUR_PCI_PF:
+ n = snprintf(name, len, "pf%u", attrs->pci_pf.pf);
+ break;
}
if (n >= len)
--
2.19.2
^ permalink raw reply related
* [PATCH net-next 0/3] devlink: Introduce PCI PF, VF ports and attributes
From: Parav Pandit @ 2019-07-01 12:27 UTC (permalink / raw)
To: netdev; +Cc: jiri, saeedm, jakub.kicinski, Parav Pandit
This patchset carry forwards the work initiated in [1] and discussion
futher concluded at [2].
To improve visibility of representor netdevice, its association with
PF or VF, physical port, two new devlink port flavours are added as
PCI PF and PCI VF ports.
A sample eswitch view can be seen below, which will be futher extended to
mdev subdevices of a PCI function in future.
Patch-1,2 extends devlink port attributes and port flavour.
Patch-3 extends mlx5 driver to register devlink ports for PF, VF and
physical link.
+---+ +---+
vf| | | | pf
+-+-+ +-+-+
physical link <---------+ | |
| | |
| | |
+-+-+ +-+-+ +-+-+
| 1 | | 2 | | 3 |
+--+---+-----+---+------+---+--+
| physical vf pf |
| port port port |
| |
| eswitch |
| |
+------------------------------+
[1] https://www.spinics.net/lists/netdev/msg555797.html
[2] https://marc.info/?l=linux-netdev&m=155354609408485&w=2
Parav Pandit (3):
devlink: Introduce PCI PF port flavour and port attribute
devlink: Introduce PCI VF port flavour and port attribute
net/mlx5e: Register devlink ports for physical link, PCI PF, VFs
.../net/ethernet/mellanox/mlx5/core/en_rep.c | 108 +++++++++++++-----
.../net/ethernet/mellanox/mlx5/core/en_rep.h | 1 +
include/net/devlink.h | 22 ++++
include/uapi/linux/devlink.h | 11 ++
net/core/devlink.c | 107 ++++++++++++++---
5 files changed, 204 insertions(+), 45 deletions(-)
--
2.19.2
^ permalink raw reply
* Re: [PATCH net-next v2 06/10] net: stmmac: Do not disable interrupts when cleaning TX
From: Willem de Bruijn @ 2019-07-01 12:23 UTC (permalink / raw)
To: Jose Abreu
Cc: linux-kernel, Network Development, Joao Pinto, David S . Miller,
Giuseppe Cavallaro, Alexandre Torgue
In-Reply-To: <BN8PR12MB326638B0BA74DA762C89DF54D3F90@BN8PR12MB3266.namprd12.prod.outlook.com>
On Mon, Jul 1, 2019 at 6:15 AM Jose Abreu <Jose.Abreu@synopsys.com> wrote:
>
> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
>
> > By the
> >
> > if ((status & handle_rx) && (chan < priv->plat->rx_queues_to_use)) {
> > stmmac_disable_dma_irq(priv, priv->ioaddr, chan);
> > napi_schedule_irqoff(&ch->rx_napi);
> > }
> >
> > branch directly above? If so, is it possible to have fewer rx than tx
> > queues and miss this?
>
> Yes, it is possible.
And that is not a problem?
>
> > this logic seems more complex than needed?
> >
> > if (status)
> > status |= handle_rx | handle_tx;
> >
> > if ((status & handle_rx) && (chan < priv->plat->rx_queues_to_use)) {
> >
> > }
> >
> > if ((status & handle_tx) && (chan < priv->plat->tx_queues_to_use)) {
> >
> > }
> >
> > status & handle_rx implies status & handle_tx and vice versa.
>
> This is removed in patch 09/10.
>
> > > - if (work_done < budget && napi_complete_done(napi, work_done))
> > > - stmmac_enable_dma_irq(priv, priv->ioaddr, chan);
> > > + if (work_done < budget)
> > > + napi_complete_done(napi, work_done);
> >
> > It does seem odd that stmmac_napi_poll_rx and stmmac_napi_poll_tx both
> > call stmmac_enable_dma_irq(..) independent of the other. Shouldn't the
> > IRQ remain masked while either is active or scheduled? That is almost
> > what this patch does, though not exactly.
>
> After patch 09/10 the interrupts will only be disabled by RX NAPI and
> re-enabled by it again. I can do some tests on whether disabling
> interrupts independently gives more performance but I wouldn't expect so
> because the real bottleneck when I do iperf3 tests is the RX path ...
Sharing the IRQ sounds fine. My only concern was TX-only IRQs in case
more TX than RX queues are configured. If that is possible with this
driver.
^ permalink raw reply
* Re: [BUG] net: dm9600: false link status
From: Corentin Labbe @ 2019-07-01 12:20 UTC (permalink / raw)
To: Andrew Lunn; +Cc: jacmet, davem, netdev, linux-usb, linux-kernel
In-Reply-To: <20190627144339.GG31189@lunn.ch>
On Thu, Jun 27, 2019 at 04:43:39PM +0200, Andrew Lunn wrote:
> On Thu, Jun 27, 2019 at 03:21:37PM +0200, Corentin Labbe wrote:
> > Hello
> >
> > I own an USB dongle which is a "Davicom DM96xx USB 10/100 Ethernet".
> > According to the CHIP_ID, it is a DM9620.
> >
> > Since I needed for bringing network to uboot for a board, I have started to create its uboot's driver.
> > My uboot driver is based on the dm9600 Linux driver.
> >
> > The dongle was working but very very slowy (24Kib/s).
> > After some debug i found that the main problem was that it always link to 10Mbit/s Half-duplex. (according to the MAC registers)
> >
> > For checking the status of the dongle I have plugged it on a Linux box which give me:
> > dm9601 6-2:1.0 enp0s29f0u2: link up, 100Mbps, full-duplex, lpa 0xFFFF
> >
> > But in fact the Linux driver is tricked.
> >
> > I have added debug of MDIO write/read and got:
> > [157550.926974] dm9601 6-2:1.0 (unnamed net_device) (uninitialized): dm9601_mdio_write() phy_id=0x00, loc=0x00, val=0x8000
>
> Writing the reset bit. Ideally you should read back the register and
> wait for this bit to clear. Try adding this, and see if this helps, or
> you get 0xffff.
>
I get 0xFFFF
> > [157550.931962] dm9601 6-2:1.0 (unnamed net_device) (uninitialized): dm9601_mdio_write() phy_id=0x00, loc=0x04, val=0x05e1
>
> Advertisement control register.
>
> > [157550.951967] dm9601 6-2:1.0 (unnamed net_device) (uninitialized): dm9601_mdio_read() phy_id=0x00, loc=0x00, returns=0xffff
>
> And now things are bad. In theory, the power down bit is set, and some
> PHYs don't respond properly when powered down. However, it is unclear
> how it got into this state. Did the reset kill it, or setting the
> advertisement? Or is the PHY simply not responding at all. The MDIO
> data lines have a pull up, so if the device does not respond, reads
> give 0xffff.
>
> Maybe also check register 0, bit 7, EXT_PHY. Is it 0, indicating the
> internal PHY should be used?
>
> You could also try reading PHY registers 2 and 3 and see if you can
> get a valid looking PHY ID. Maybe try that before hitting the reset
> bit?
>
Always get 0xFFFF before and after
Note that the eeprom dump via ethtool -e suffer the same problem.
> > So it exsists two problem:
> > - Linux saying 100Mbps, full-duplex even if it is false.
>
> The driver is using the old mii code, not a phy driver. So i cannot
> help too much with linux. But if you can get the MDIO bus working
> reliably, it should be possible to move this over to phylib. The
> internal PHY appears to have all the standard registers, so the
> generic PHY driver has a good chance of working.
>
I have investigated more and in fact I dont have a dm9620.
I own another diferent dongle in my stock that i never used and fun fact, it has the same VID/PID and the same bug.
I opened both dongle and all chips have no marking and got only 4x8 pins.
Since dm9620 have 64 pins, my dongles are clearly not such hw.
I googled the inscription written on the second case and found a dm9620 clone/counterfeiting named qf9700/sr9700.
But thoses chips should have marking (according to some photos on the web), so I probably own a counterfeiting of a clone/counterfeiting.
Note that the sr9700 driver is mainline but fail to work with my dongle.
At least, now i know that have a chip not designed to work with an external PHY, so all EXTPHY registers could be ignored.
My last ressort is to brute force all values until something happen.
My simple tries, write 0xsomeval everywhere, lead to something, phy/eeprom return now 0x000y.
Probably, this chip doesnt have any PHY...
Regards
^ permalink raw reply
* Re: kernel panic: corrupted stack end in dput
From: syzbot @ 2019-07-01 12:14 UTC (permalink / raw)
To: ast, daniel, john.fastabend, linux-fsdevel, linux-kernel, netdev,
syzkaller-bugs, viro
In-Reply-To: <000000000000a5d3cb058c9a64f0@google.com>
syzbot has bisected this bug to:
commit e9db4ef6bf4ca9894bb324c76e01b8f1a16b2650
Author: John Fastabend <john.fastabend@gmail.com>
Date: Sat Jun 30 13:17:47 2018 +0000
bpf: sockhash fix omitted bucket lock in sock_close
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=17f7f4d5a00000
start commit: 7b75e49d net: dsa: mv88e6xxx: wait after reset deactivation
git tree: net
final crash: https://syzkaller.appspot.com/x/report.txt?x=140ff4d5a00000
console output: https://syzkaller.appspot.com/x/log.txt?x=100ff4d5a00000
kernel config: https://syzkaller.appspot.com/x/.config?x=e7c31a94f66cc0aa
dashboard link: https://syzkaller.appspot.com/bug?extid=d88a977731a9888db7ba
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=130f49bda00000
Reported-by: syzbot+d88a977731a9888db7ba@syzkaller.appspotmail.com
Fixes: e9db4ef6bf4c ("bpf: sockhash fix omitted bucket lock in sock_close")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply
* [PATCH v3] ss: introduce switch to print exact value of data rates
From: Tomasz Torcz @ 2019-07-01 11:52 UTC (permalink / raw)
To: dsahern; +Cc: netdev, Tomasz Torcz
In-Reply-To: <c28e349c-25b7-7a09-b2b3-5e64294bb089@gmail.com>
Introduce -X/--exact switch to disable human-friendly printing
of data rates. Without the switch (default), data is presented as MBps/Kbps.
Signed-off-by: Tomasz Torcz <tomasz.torcz@nordea.com>
---
man/man8/ss.8 | 3 +++
misc/ss.c | 12 ++++++++++--
2 files changed, 13 insertions(+), 2 deletions(-)
Changes in v3:
- updated ss man page with new option
diff --git a/man/man8/ss.8 b/man/man8/ss.8
index 9054fab9..2ba5fda2 100644
--- a/man/man8/ss.8
+++ b/man/man8/ss.8
@@ -290,6 +290,9 @@ that parsing /proc/net/tcp is painful.
.B \-E, \-\-events
Continually display sockets as they are destroyed
.TP
+.B \-X, \-\-exact
+Show exact bandwidth values, instead of human-readable
+.TP
.B \-Z, \-\-context
As the
.B \-p
diff --git a/misc/ss.c b/misc/ss.c
index 99c06d31..ba1bfff6 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -110,6 +110,7 @@ static int resolve_services = 1;
int preferred_family = AF_UNSPEC;
static int show_options;
int show_details;
+static int show_human_readable = 1;
static int show_users;
static int show_mem;
static int show_tcpinfo;
@@ -2361,7 +2362,9 @@ static int proc_inet_split_line(char *line, char **loc, char **rem, char **data)
static char *sprint_bw(char *buf, double bw)
{
- if (bw > 1000000.)
+ if (!show_human_readable)
+ sprintf(buf, "%.0f", bw);
+ else if (bw > 1000000.)
sprintf(buf, "%.1fM", bw / 1000000.);
else if (bw > 1000.)
sprintf(buf, "%.1fK", bw / 1000.);
@@ -4883,6 +4886,7 @@ static void _usage(FILE *dest)
" --tos show tos and priority information\n"
" -b, --bpf show bpf filter socket information\n"
" -E, --events continually display sockets as they are destroyed\n"
+" -X, --exact show exact bandwidth values, instead of human-readable\n"
" -Z, --context display process SELinux security contexts\n"
" -z, --contexts display process and socket SELinux security contexts\n"
" -N, --net switch to the specified network namespace name\n"
@@ -5031,6 +5035,7 @@ static const struct option long_opts[] = {
{ "no-header", 0, 0, 'H' },
{ "xdp", 0, 0, OPT_XDPSOCK},
{ "oneline", 0, 0, 'O' },
+ { "exact", 0, 0, 'X' },
{ 0 }
};
@@ -5046,7 +5051,7 @@ int main(int argc, char *argv[])
int state_filter = 0;
while ((ch = getopt_long(argc, argv,
- "dhaletuwxnro460spbEf:miA:D:F:vVzZN:KHSO",
+ "dhaletuwxXnro460spbEf:miA:D:F:vVzZN:KHSO",
long_opts, NULL)) != EOF) {
switch (ch) {
case 'n':
@@ -5097,6 +5102,9 @@ int main(int argc, char *argv[])
case 'x':
filter_af_set(¤t_filter, AF_UNIX);
break;
+ case 'X':
+ show_human_readable = 0;
+ break;
case OPT_VSOCK:
filter_af_set(¤t_filter, AF_VSOCK);
break;
--
2.21.0
^ permalink raw reply related
* [PATCH] net: dst.h: Fix shifting signed 32-bit value by 31 bits problem
From: Vandana BN @ 2019-07-01 11:55 UTC (permalink / raw)
To: David S. Miller, netdev, linux-kernel
Cc: skhan, gregkh, linux-kernel-mentees, Vandana BN
Fix DST_FEATURE_ECN_CA to use "U" cast to avoid shifting signed
32-bit value by 31 bits problem.
Signed-off-by: Vandana BN <bnvandana@gmail.com>
---
include/net/dst.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/net/dst.h b/include/net/dst.h
index 12b31c602cb0..095c5daf9403 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -183,7 +183,7 @@ static inline void dst_metric_set(struct dst_entry *dst, int metric, u32 val)
}
/* Kernel-internal feature bits that are unallocated in user space. */
-#define DST_FEATURE_ECN_CA (1 << 31)
+#define DST_FEATURE_ECN_CA (1U << 31)
#define DST_FEATURE_MASK (DST_FEATURE_ECN_CA)
#define DST_FEATURE_ECN_MASK (DST_FEATURE_ECN_CA | RTAX_FEATURE_ECN)
--
2.17.1
^ permalink raw reply related
* Re: KASAN: use-after-free Write in xfrm_hash_rebuild
From: Florian Westphal @ 2019-07-01 11:46 UTC (permalink / raw)
To: syzbot
Cc: davem, fw, herbert, linux-kernel, netdev, steffen.klassert,
syzkaller-bugs
In-Reply-To: <000000000000f66c6e058c7cd4e0@google.com>
syzbot <syzbot+0165480d4ef07360eeda@syzkaller.appspotmail.com> wrote:
> syzbot has bisected this bug to:
>
> commit 1548bc4e0512700cf757192c106b3a20ab639223
> Author: Florian Westphal <fw@strlen.de>
> Date: Fri Jan 4 13:17:02 2019 +0000
>
> xfrm: policy: delete inexact policies from inexact list on hash rebuild
I'm looking at this now.
^ permalink raw reply
* Re: [PATCH v5 net-next 1/6] xdp: allow same allocator usage
From: Jesper Dangaard Brouer @ 2019-07-01 11:40 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: grygorii.strashko, hawk, davem, ast, linux-kernel, linux-omap,
xdp-newbies, ilias.apalodimas, netdev, daniel, jakub.kicinski,
john.fastabend, brouer
In-Reply-To: <20190630172348.5692-2-ivan.khoronzhuk@linaro.org>
I'm very skeptical about this approach.
On Sun, 30 Jun 2019 20:23:43 +0300
Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> XDP rxqs can be same for ndevs running under same rx napi softirq.
> But there is no ability to register same allocator for both rxqs,
> by fact it can same rxq but has different ndev as a reference.
This description is not very clear. It can easily be misunderstood.
It is an absolute requirement that each RX-queue have their own
page_pool object/allocator. (This where the performance comes from) as
the page_pool have NAPI protected array for alloc and XDP_DROP recycle.
Your driver/hardware seems to have special case, where a single
RX-queue can receive packets for two different net_device'es.
Do you violate this XDP devmap redirect assumption[1]?
[1] https://github.com/torvalds/linux/blob/v5.2-rc7/kernel/bpf/devmap.c#L324-L329
> Due to last changes allocator destroy can be defered till the moment
> all packets are recycled by destination interface, afterwards it's
> freed. In order to schedule allocator destroy only after all users are
> unregistered, add refcnt to allocator object and schedule to destroy
> only it reaches 0.
The guiding principles when designing an API, is to make it easy to
use, but also make it hard to misuse.
Your API change makes it easy to misuse the API. As it make it easy to
(re)use the allocator pointer (likely page_pool) for multiple
xdp_rxq_info structs. It is only valid for your use-case, because you
have hardware where a single RX-queue delivers to two different
net_devices. For other normal use-cases, this will be a violation.
If I was a user of this API, and saw your xdp_allocator_get(), then I
would assume that this was the normal case. As minimum, we need to add
a comment in the code, about this specific/intended use-case. I
through about detecting the misuse, by adding a queue_index to
xdp_mem_allocator, that can be checked against, when calling
xdp_rxq_info_reg_mem_model() with another xdp_rxq_info struct (to catch
the obvious mistake where queue_index mismatch).
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
> include/net/xdp_priv.h | 1 +
> net/core/xdp.c | 46 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 47 insertions(+)
>
> diff --git a/include/net/xdp_priv.h b/include/net/xdp_priv.h
> index 6a8cba6ea79a..995b21da2f27 100644
> --- a/include/net/xdp_priv.h
> +++ b/include/net/xdp_priv.h
> @@ -18,6 +18,7 @@ struct xdp_mem_allocator {
> struct rcu_head rcu;
> struct delayed_work defer_wq;
> unsigned long defer_warn;
> + unsigned long refcnt;
> };
>
> #endif /* __LINUX_NET_XDP_PRIV_H__ */
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index b29d7b513a18..a44621190fdc 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -98,6 +98,18 @@ bool __mem_id_disconnect(int id, bool force)
> WARN(1, "Request remove non-existing id(%d), driver bug?", id);
> return true;
> }
> +
> + /* to avoid calling hash lookup twice, decrement refcnt here till it
> + * reaches zero, then it can be called from workqueue afterwards.
> + */
> + if (xa->refcnt)
> + xa->refcnt--;
> +
> + if (xa->refcnt) {
> + mutex_unlock(&mem_id_lock);
> + return true;
> + }
> +
> xa->disconnect_cnt++;
>
> /* Detects in-flight packet-pages for page_pool */
> @@ -312,6 +324,33 @@ static bool __is_supported_mem_type(enum xdp_mem_type type)
> return true;
> }
>
> +static struct xdp_mem_allocator *xdp_allocator_get(void *allocator)
API wise, when you have "get" operation, you usually also have a "put"
operation...
> +{
> + struct xdp_mem_allocator *xae, *xa = NULL;
> + struct rhashtable_iter iter;
> +
> + mutex_lock(&mem_id_lock);
> + rhashtable_walk_enter(mem_id_ht, &iter);
> + do {
> + rhashtable_walk_start(&iter);
> +
> + while ((xae = rhashtable_walk_next(&iter)) && !IS_ERR(xae)) {
> + if (xae->allocator == allocator) {
> + xae->refcnt++;
> + xa = xae;
> + break;
> + }
> + }
> +
> + rhashtable_walk_stop(&iter);
> +
> + } while (xae == ERR_PTR(-EAGAIN));
> + rhashtable_walk_exit(&iter);
> + mutex_unlock(&mem_id_lock);
> +
> + return xa;
> +}
> +
> int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
> enum xdp_mem_type type, void *allocator)
> {
> @@ -347,6 +386,12 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
> }
> }
>
> + xdp_alloc = xdp_allocator_get(allocator);
> + if (xdp_alloc) {
> + xdp_rxq->mem.id = xdp_alloc->mem.id;
> + return 0;
> + }
> +
The allocator pointer (in-practice) becomes the identifier for the
mem.id (which rhashtable points to xdp_mem_allocator object).
> xdp_alloc = kzalloc(sizeof(*xdp_alloc), gfp);
> if (!xdp_alloc)
> return -ENOMEM;
> @@ -360,6 +405,7 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
> xdp_rxq->mem.id = id;
> xdp_alloc->mem = xdp_rxq->mem;
> xdp_alloc->allocator = allocator;
> + xdp_alloc->refcnt = 1;
>
> /* Insert allocator into ID lookup table */
> ptr = rhashtable_insert_slow(mem_id_ht, &id, &xdp_alloc->node);
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: KMSAN: uninit-value in ax88178_bind
From: syzbot @ 2019-07-01 11:38 UTC (permalink / raw)
To: allison, davem, glider, gregkh, linux-kernel, linux-usb, lynxis,
marcel.ziswiler, netdev, opensource, syzkaller-bugs, tglx,
yang.wei9, zhang.run
In-Reply-To: <000000000000cba2b6058ac09eeb@google.com>
syzbot has found a reproducer for the following crash on:
HEAD commit: 41550654 [UPSTREAM] KVM: x86: degrade WARN to pr_warn_rate..
git tree: kmsan
console output: https://syzkaller.appspot.com/x/log.txt?x=1577f4d5a00000
kernel config: https://syzkaller.appspot.com/x/.config?x=40511ad0c5945201
dashboard link: https://syzkaller.appspot.com/bug?extid=abd25d675d47f23f188c
compiler: clang version 9.0.0 (/home/glider/llvm/clang
80fee25776c2fb61e74c1ecb1a523375c2500b69)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=17ea8283a00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11a1f57ba00000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+abd25d675d47f23f188c@syzkaller.appspotmail.com
usb 1-1: config 0 has no interface number 0
usb 1-1: New USB device found, idVendor=04bb, idProduct=0930,
bcdDevice=d2.4a
usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0
usb 1-1: config 0 descriptor??
==================================================================
BUG: KMSAN: uninit-value in is_valid_ether_addr
include/linux/etherdevice.h:195 [inline]
BUG: KMSAN: uninit-value in asix_set_netdev_dev_addr
drivers/net/usb/asix_devices.c:61 [inline]
BUG: KMSAN: uninit-value in ax88178_bind+0x635/0xad0
drivers/net/usb/asix_devices.c:1075
CPU: 1 PID: 30 Comm: kworker/1:1 Not tainted 5.2.0-rc4+ #7
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Workqueue: usb_hub_wq hub_event
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x191/0x1f0 lib/dump_stack.c:113
kmsan_report+0x162/0x2d0 mm/kmsan/kmsan.c:611
__msan_warning+0x75/0xe0 mm/kmsan/kmsan_instr.c:304
is_valid_ether_addr include/linux/etherdevice.h:195 [inline]
asix_set_netdev_dev_addr drivers/net/usb/asix_devices.c:61 [inline]
ax88178_bind+0x635/0xad0 drivers/net/usb/asix_devices.c:1075
usbnet_probe+0x10d3/0x3950 drivers/net/usb/usbnet.c:1722
usb_probe_interface+0xd19/0x1310 drivers/usb/core/driver.c:361
really_probe+0x1344/0x1d90 drivers/base/dd.c:513
driver_probe_device+0x1ba/0x510 drivers/base/dd.c:670
__device_attach_driver+0x5b8/0x790 drivers/base/dd.c:777
bus_for_each_drv+0x28e/0x3b0 drivers/base/bus.c:454
__device_attach+0x489/0x750 drivers/base/dd.c:843
device_initial_probe+0x4a/0x60 drivers/base/dd.c:890
bus_probe_device+0x131/0x390 drivers/base/bus.c:514
device_add+0x25b5/0x2df0 drivers/base/core.c:2111
usb_set_configuration+0x309f/0x3710 drivers/usb/core/message.c:2027
generic_probe+0xe7/0x280 drivers/usb/core/generic.c:210
usb_probe_device+0x146/0x200 drivers/usb/core/driver.c:266
really_probe+0x1344/0x1d90 drivers/base/dd.c:513
driver_probe_device+0x1ba/0x510 drivers/base/dd.c:670
__device_attach_driver+0x5b8/0x790 drivers/base/dd.c:777
bus_for_each_drv+0x28e/0x3b0 drivers/base/bus.c:454
__device_attach+0x489/0x750 drivers/base/dd.c:843
device_initial_probe+0x4a/0x60 drivers/base/dd.c:890
bus_probe_device+0x131/0x390 drivers/base/bus.c:514
device_add+0x25b5/0x2df0 drivers/base/core.c:2111
usb_new_device+0x23e5/0x2fb0 drivers/usb/core/hub.c:2534
hub_port_connect drivers/usb/core/hub.c:5089 [inline]
hub_port_connect_change drivers/usb/core/hub.c:5204 [inline]
port_event drivers/usb/core/hub.c:5350 [inline]
hub_event+0x5853/0x7320 drivers/usb/core/hub.c:5432
process_one_work+0x1572/0x1f00 kernel/workqueue.c:2269
worker_thread+0x111b/0x2460 kernel/workqueue.c:2415
kthread+0x4b5/0x4f0 kernel/kthread.c:256
ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355
Local variable description: ----buf@ax88178_bind
Variable was created at:
ax88178_bind+0x60/0xad0 drivers/net/usb/asix_devices.c:1064
usbnet_probe+0x10d3/0x3950 drivers/net/usb/usbnet.c:1722
==================================================================
^ permalink raw reply
* Re: [PATCH v2 2/2] rt2x00usb: remove unnecessary rx flag checks
From: Stanislaw Gruszka @ 2019-07-01 11:07 UTC (permalink / raw)
To: Soeren Moch
Cc: Helmut Schaa, Kalle Valo, David S. Miller, linux-wireless, netdev,
linux-kernel
In-Reply-To: <20190701105314.9707-2-smoch@web.de>
On Mon, Jul 01, 2019 at 12:53:14PM +0200, Soeren Moch wrote:
> In contrast to the TX path, there is no need to separately read the transfer
> status from the device after receiving RX data. Consequently, there is no
> real STATUS_PENDING RX processing queue entry state.
> Remove the unnecessary ENTRY_DATA_STATUS_PENDING flag checks from the RX path.
> Also remove the misleading comment about reading RX status from device.
>
> Suggested-by: Stanislaw Gruszka <sgruszka@redhat.com>
> Signed-off-by: Soeren Moch <smoch@web.de>
Acked-by: Stanislaw Gruszka <sgruszka@redhat.com>
^ permalink raw reply
* Re: [PATCH v2 1/2] rt2x00usb: fix rx queue hang
From: Stanislaw Gruszka @ 2019-07-01 11:07 UTC (permalink / raw)
To: Soeren Moch
Cc: stable, Helmut Schaa, Kalle Valo, David S. Miller, linux-wireless,
netdev, linux-kernel
In-Reply-To: <20190701105314.9707-1-smoch@web.de>
On Mon, Jul 01, 2019 at 12:53:13PM +0200, Soeren Moch wrote:
> Since commit ed194d136769 ("usb: core: remove local_irq_save() around
> ->complete() handler") the handler rt2x00usb_interrupt_rxdone() is
> not running with interrupts disabled anymore. So this completion handler
> is not guaranteed to run completely before workqueue processing starts
> for the same queue entry.
> Be sure to set all other flags in the entry correctly before marking
> this entry ready for workqueue processing. This way we cannot miss error
> conditions that need to be signalled from the completion handler to the
> worker thread.
> Note that rt2x00usb_work_rxdone() processes all available entries, not
> only such for which queue_work() was called.
>
> This patch is similar to what commit df71c9cfceea ("rt2x00: fix order
> of entry flags modification") did for TX processing.
>
> This fixes a regression on a RT5370 based wifi stick in AP mode, which
> suddenly stopped data transmission after some period of heavy load. Also
> stopping the hanging hostapd resulted in the error message "ieee80211
> phy0: rt2x00queue_flush_queue: Warning - Queue 14 failed to flush".
> Other operation modes are probably affected as well, this just was
> the used testcase.
>
> Fixes: ed194d136769 ("usb: core: remove local_irq_save() around ->complete() handler")
> Cc: stable@vger.kernel.org # 4.20+
> Signed-off-by: Soeren Moch <smoch@web.de>
Acked-by: Stanislaw Gruszka <sgruszka@redhat.com>
^ permalink raw reply
* [PATCH v2] let proc net directory inodes reflect to active net namespace
From: Hallsmark, Per @ 2019-07-01 11:06 UTC (permalink / raw)
To: Alexey Dobriyan, David S. Miller
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
Hallsmark, Per
[-- Attachment #1: Type: text/plain, Size: 6130 bytes --]
Hi,
Linux kernel recently got a bugfix 1fde6f21d90f ("proc: fix /proc/net/* after setns(2)"),
but unfortunately it only solves the issue for procfs net file inodes so they get correct
content after a process change namespace.
Checking on a v5.2-rc6 kernel :
sh-4.4# sh netns_procfs_test.sh
[ 16.451640] ip (108) used greatest stack depth: 12264 bytes left
Before net namespace change :
==== /proc/net/dev ====
Inter-| Receive | Transmit
face |bytes packets errs drop fifo frame compressed multicast|bytes packd
eth0: 0 0 0 0 0 0 0 0 0 0
lo: 0 0 0 0 0 0 0 0 0 0
if_default: 0 0 0 0 0 0 0 0 0 0
sit0: 0 0 0 0 0 0 0 0 0 0
==== files in /proc/net/dev_snmp6 ====
.
..
lo
eth0
sit0
if_default
After net namespace change :
==== /proc/net/dev ====
Inter-| Receive | Transmit
face |bytes packets errs drop fifo frame compressed multicast|bytes packd
sit0: 0 0 0 0 0 0 0 0 0 0
if_other: 0 0 0 0 0 0 0 0 0 0
lo: 0 0 0 0 0 0 0 0 0 0
==== files in /proc/net/dev_snmp6 ====
.
..
lo
eth0
sit0
if_default
This kernel is fixed for file inode bug but suffers dir inode bug
sh-4.4#
As can be seen above, after the namespace change we see new content in procfs net/dev
but the listing of procfs net/dev_snmp6 still shows entries from previous namespace.
We need to apply similar bugfix for directory creation in procfs net as the mentioned
commit do for files.
Checking on a v5.2-rc6 kernel with bugfixes :
sh-4.4# sh netns_procfs_test.sh
[ 745.993882] ip (108) used greatest stack depth: 12264 bytes left
Before net namespace change :
==== /proc/net/dev ====
Inter-| Receive | Transmit
face |bytes packets errs drop fifo frame compressed multicast|bytes packd
lo: 0 0 0 0 0 0 0 0 0 0
sit0: 0 0 0 0 0 0 0 0 0 0
eth0: 0 0 0 0 0 0 0 0 0 0
if_default: 0 0 0 0 0 0 0 0 0 0
==== files in /proc/net/dev_snmp6 ====
.
..
lo
eth0
sit0
if_default
After net namespace change :
==== /proc/net/dev ====
Inter-| Receive | Transmit
face |bytes packets errs drop fifo frame compressed multicast|bytes packd
if_other: 0 0 0 0 0 0 0 0 0 0
sit0: 0 0 0 0 0 0 0 0 0 0
lo: 0 0 0 0 0 0 0 0 0 0
==== files in /proc/net/dev_snmp6 ====
.
..
lo
sit0
if_other
This kernel is fixed for both file and dir inode bug
sh-4.4#
Here we see that the directory procfs net/dev_snmp6 is updated according to the namespace
change.
The fix is two commits, first updates proc_net_mkdir() entries similar to mentioned patch
and second one is changing net/ipv6/proc.c to use proc_net_mkdir() instead.
Speaking about proc_net_mkdir()...
[phallsma@arn-phallsma-l3 linux]$ git grep proc_mkdir | grep proc_net
drivers/isdn/divert/divert_procfs.c: isdn_proc_entry = proc_mkdir("isdn", init_net.proc_net);
drivers/isdn/hysdn/hysdn_procconf.c: hysdn_proc_entry = proc_mkdir(PROC_SUBDIR_NAME, init_net.proc_net);
drivers/net/bonding/bond_procfs.c: bn->proc_dir = proc_mkdir(DRV_NAME, bn->net->proc_net);
drivers/net/wireless/intel/ipw2x00/libipw_module.c: libipw_proc = proc_mkdir(DRV_PROCNAME, init_net.proc_net);
drivers/net/wireless/intersil/hostap/hostap_main.c: hostap_proc = proc_mkdir("hostap", init_net.proc_net);
drivers/staging/rtl8192u/ieee80211/ieee80211_module.c: ieee80211_proc = proc_mkdir(DRV_NAME, init_net.proc_net);
drivers/staging/rtl8192u/r8192U_core.c: rtl8192_proc = proc_mkdir(RTL819XU_MODULE_NAME, init_net.proc_net);
net/appletalk/atalk_proc.c: if (!proc_mkdir("atalk", init_net.proc_net))
net/core/pktgen.c: pn->proc_dir = proc_mkdir(PG_PROC_DIR, pn->net->proc_net);
net/ipv4/netfilter/ipt_CLUSTERIP.c: cn->procdir = proc_mkdir("ipt_CLUSTERIP", net->proc_net);
net/ipv6/proc.c: net->mib.proc_net_devsnmp6 = proc_mkdir("dev_snmp6", net->proc_net);
net/llc/llc_proc.c: llc_proc_dir = proc_mkdir("llc", init_net.proc_net);
net/netfilter/xt_hashlimit.c: hashlimit_net->ipt_hashlimit = proc_mkdir("ipt_hashlimit", net->proc_net);
net/netfilter/xt_hashlimit.c: hashlimit_net->ip6t_hashlimit = proc_mkdir("ip6t_hashlimit", net->proc_net);
net/netfilter/xt_recent.c: recent_net->xt_recent = proc_mkdir("xt_recent", net->proc_net);
net/sunrpc/cache.c: cd->procfs = proc_mkdir(cd->name, sn->proc_net_rpc);
net/sunrpc/stats.c: sn->proc_net_rpc = proc_mkdir("rpc", net->proc_net);
net/x25/x25_proc.c: if (!proc_mkdir("x25", init_net.proc_net))
[phallsma@arn-phallsma-l3 linux]$
IMHO all code should use proc_net_mkdir() instead of proc_mkdir() for procfs net entries,
or am I missing something here? If not possible to changeover to proc_net_mkdir() there
is a need for duplicating my first commit at those places. I'm fixing the one for dev_snmp6()
which is what I've tested as well.
Also wonder if it all is optimal. Wouldn't it be better to re-enable dcache for these (files as well as directories)
and in addition have kernel drop dcache in case of a namespace change?
Attaching patches and app/script for verifying.
I'm not on the mailing lists so please keep me on CC in case of responding.
Best regards,
Per
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: netns_procfs_test.c --]
[-- Type: text/x-csrc; name="netns_procfs_test.c", Size: 3920 bytes --]
/*
Verifier of /proc/net and net namespace consistency
Author : Per Hallsmark <per.hallsmark@windriver.com>
First setup a net namespace and add a veth so we get something to test with
ip netns add netns_other
ip link add if_default type veth peer name if_other
ip link set if_other netns netns_other
Run test app without first reading some info from /proc/net in default namespace
./netns_procfs_test
Run test app again, this time reading some info from /proc/net in default namespace
before changing net namespace
./netns_procfs_test cached
On a bugfree kernel, the output after "After net namespace change" should be same as
in first test run, meaning we should see if_other in the /proc/net/dev dump and we
should see if_other in the directory.
The issue is not visible if it is different processes doing the readings since then
the inode's aren't cached.
*/
#define _GNU_SOURCE
#include <sched.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
#include <string.h>
#include <assert.h>
#include <fcntl.h>
#include <errno.h>
#include <limits.h>
#include <sys/types.h>
#include <sys/mount.h>
#include <errno.h>
#include <dirent.h>
#define NETNS_RUN_DIR "/var/run/netns"
static int get_nsfd(const char *ns)
{
char path[strlen(NETNS_RUN_DIR) + strlen(ns) + 2];
int fd;
snprintf(path, sizeof(path), "%s/%s", NETNS_RUN_DIR, ns);
fd = open(path, O_RDONLY, 0);
return fd;
}
static int dump_file(const char *fn, const int checkforbug)
{
int fd;
char buf[512];
ssize_t len;
char *wholebuf = NULL;
ssize_t wholelen = 0;
int status = 0;
printf("==== %s ====\n", fn);
fd = open(fn, O_RDONLY);
if (fd == -1) {
printf("Couldn't open %s (%s)\n", fn, strerror(errno));
return -1;
}
do {
len = read(fd, buf, sizeof(buf)-1);
buf[len] = 0;
wholebuf = realloc(wholebuf, wholelen + len + 1);
sprintf(&wholebuf[wholelen], "%s", buf);
wholelen += len;
} while (len > 0);
printf("%s\n", wholebuf);
if (checkforbug && strstr(wholebuf, "if_other")) {
status = 1;
}
free(wholebuf);
close(fd);
return status;
}
static int dump_dir(const char *dn, const int checkforbug)
{
DIR *dir;
struct dirent *dir_entry;
int status = 0;
printf("==== files in %s ====\n", dn);
dir = opendir(dn);
if (!dir) {
printf("Couldn't open %s (%s)\n", dn, strerror(errno));
return -1;
}
while ((dir_entry = readdir(dir)) != NULL) {
printf(" %s", dir_entry->d_name);
if (checkforbug && !strcmp(dir_entry->d_name, "if_other")) {
status = 2;
}
printf("\n");
}
closedir(dir);
return status;
}
static int switch_ns(const char *ns)
{
int nsfd = get_nsfd(ns);
if (setns(nsfd, CLONE_NEWNET) < 0) {
fprintf(stderr, "Unable to switch to namespace(fd=%d): %s.\n",
nsfd, strerror(errno));
exit(-1);
}
return 0;
}
int main (int argc, char **argv)
{
int bugcheck = 0;
int checkforbug = 1;
if (argc == 2 && !strcmp(argv[1], "cached")) {
// access /proc/net a bit so we dcache file and
// directory inodes
printf("Before net namespace change :\n");
dump_file("/proc/net/dev", 0);
dump_dir("/proc/net/dev_snmp6", 0);
} else {
// we cannot check for bug if not run with cached arg
checkforbug = 0;
}
// change namespace
switch_ns("netns_other");
// access /proc/net again, if all works we should see
// new info because of net namespace change
printf("\n\nAfter net namespace change :\n");
bugcheck += dump_file("/proc/net/dev", 1);
bugcheck += dump_dir("/proc/net/dev_snmp6", 1);
if (!checkforbug)
return 0;
switch (bugcheck) {
case 1 :
printf("This kernel is fixed for file inode bug but suffers dir inode bug\n");
break;
case 3 :
printf("This kernel is fixed for both file and dir inode bug\n");
break;
default :
printf("This kernel suffers both file and dir inode bug\n");
}
return bugcheck != 3;
}
[-- Attachment #3: netns_procfs_test.sh --]
[-- Type: application/x-shellscript, Size: 144 bytes --]
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0001-Make-directory-inodes-in-proc-net-adhere-to-net-name.patch --]
[-- Type: text/x-patch; name="0001-Make-directory-inodes-in-proc-net-adhere-to-net-name.patch", Size: 1925 bytes --]
From e01095d0c6ca03a0c8d83e48023646b3d60a4be8 Mon Sep 17 00:00:00 2001
From: Per Hallsmark <per.hallsmark@windriver.com>
Date: Wed, 19 Jun 2019 15:46:39 +0200
Subject: [PATCH 1/2] Make directory inodes in /proc/net adhere to net
namespace
This patch fixes /proc/net directory inodes in similar way as
commit 1fde6f21d90f ("proc: fix /proc/net/* after setns(2)")
fixes file inodes.
Signed-off-by: Per Hallsmark <per.hallsmark@windriver.com>
---
fs/proc/proc_net.c | 14 ++++++++++++++
include/linux/proc_fs.h | 7 ++-----
2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c
index 76ae278df1c4..c73cf5637b9d 100644
--- a/fs/proc/proc_net.c
+++ b/fs/proc/proc_net.c
@@ -55,6 +55,20 @@ static void pde_force_lookup(struct proc_dir_entry *pde)
pde->proc_dops = &proc_net_dentry_ops;
}
+struct proc_dir_entry *proc_net_mkdir(struct net *net, const char *name,
+ struct proc_dir_entry *parent)
+{
+ struct proc_dir_entry *pde;
+
+ pde = proc_mkdir_data(name, 0, parent, net);
+ if (!pde)
+ return NULL;
+ pde->proc_dops = &proc_net_dentry_ops;
+
+ return pde;
+}
+EXPORT_SYMBOL(proc_net_mkdir);
+
static int seq_open_net(struct inode *inode, struct file *file)
{
unsigned int state_size = PDE(inode)->state_size;
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index 52a283ba0465..608b8a10e338 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -124,11 +124,8 @@ static inline struct pid *tgid_pidfd_to_pid(const struct file *file)
struct net;
-static inline struct proc_dir_entry *proc_net_mkdir(
- struct net *net, const char *name, struct proc_dir_entry *parent)
-{
- return proc_mkdir_data(name, 0, parent, net);
-}
+extern struct proc_dir_entry *proc_net_mkdir(
+ struct net *net, const char *name, struct proc_dir_entry *parent);
struct ns_common;
int open_related_ns(struct ns_common *ns,
--
2.20.1
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0002-net-Directories-created-in-proc-net-should-be-done-v.patch --]
[-- Type: text/x-patch; name="0002-net-Directories-created-in-proc-net-should-be-done-v.patch", Size: 976 bytes --]
From 5f3f942d1cb1140074677cd08204a1f89c609fef Mon Sep 17 00:00:00 2001
From: Per Hallsmark <per.hallsmark@windriver.com>
Date: Thu, 20 Jun 2019 10:21:31 +0200
Subject: [PATCH 2/2] net: Directories created in /proc/net should be done via
proc_net_mkdir()
Directories created in /proc/net should be done via proc_net_mkdir()
Signed-off-by: Per Hallsmark <per.hallsmark@windriver.com>
---
net/ipv6/proc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/ipv6/proc.c b/net/ipv6/proc.c
index 4a8da679866e..3728c57e93dc 100644
--- a/net/ipv6/proc.c
+++ b/net/ipv6/proc.c
@@ -282,7 +282,8 @@ static int __net_init ipv6_proc_init_net(struct net *net)
snmp6_seq_show, NULL))
goto proc_snmp6_fail;
- net->mib.proc_net_devsnmp6 = proc_mkdir("dev_snmp6", net->proc_net);
+ net->mib.proc_net_devsnmp6 = proc_net_mkdir(net,
+ "dev_snmp6", net->proc_net);
if (!net->mib.proc_net_devsnmp6)
goto proc_dev_snmp6_fail;
return 0;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] rt2x00: fix rx queue hang
From: Stanislaw Gruszka @ 2019-07-01 11:04 UTC (permalink / raw)
To: Soeren Moch
Cc: Helmut Schaa, Kalle Valo, David S. Miller, linux-wireless, netdev,
linux-kernel, stable
In-Reply-To: <06c55c1d-6da6-76b2-f6e7-c8eeccd5aa35@web.de>
On Mon, Jul 01, 2019 at 12:49:50PM +0200, Soeren Moch wrote:
> Hello!
>
> On 29.06.19 10:50, Stanislaw Gruszka wrote:
> > Hello
> >
> > On Wed, Jun 26, 2019 at 03:28:00PM +0200, Soeren Moch wrote:
> >> Hi Stanislaw,
> >>
> >> the good news is: your patch below also solves the issue for me. But
> >> removing the ENTRY_DATA_STATUS_PENDING check in
> >> rt2x00usb_kick_rx_entry() alone does not help, while removing this check
> >> in rt2x00usb_work_rxdone() alone does the trick.
> >>
> >> So the real race seems to be that the flags set in the completion
> >> handler are not yet visible on the cpu core running the workqueue. And
> >> because the worker is not rescheduled when aborted, the entry can just
> >> wait forever.
> >> Do you think this could make sense?
> > Yes.
> >
> >>> I'm somewhat reluctant to change the order, because TX processing
> >>> might relay on it (we first mark we wait for TX status and
> >>> then mark entry is no longer owned by hardware).
> >> OK, maybe it's just good luck that changing the order solves the rx
> >> problem. Or can memory barriers associated with the spinlock in
> >> rt2x00lib_dmadone() be responsible for that?
> >> (I'm testing on a armv7 system, Cortex-A9 quadcore.)
> > I'm not sure, rt2x00queue_index_inc() also disable/enable interrupts,
> > so maybe that make race not reproducible.
> I tested some more, the race is between setting ENTRY_DATA_IO_FAILED (if
> needed) and enabling workqueue processing. This enabling was done via
> ENTRY_DATA_STATUS_PENDING in my patch. So setting
> ENTRY_DATA_STATUS_PENDING behind the spinlock in
> rt2x00lib_dmadone()/rt2x00queue_index_inc() moved this very close to
> setting of ENTRY_DATA_IO_FAILED (if needed). While still in the wrong
> order, this made it very unlikely for the race to show up.
> >
> >> While looking at it, why we double-clear ENTRY_OWNER_DEVICE_DATA in
> >> rt2x00usb_interrupt_rxdone() directly and in rt2x00lib_dmadone() again,
> > rt2x00lib_dmadone() is called also on other palaces (error paths)
> > when we have to clear flags.
> Yes, but also clearing ENTRY_OWNER_DEVICE_DATA in
> rt2x00usb_interrupt_rxdone() directly is not necessary and can lead to
> the wrong processing order.
> >> while not doing the same for tx?
> > If I remember correctly we have some races on rx (not happened on tx)
> > that was solved by using test_and_clear_bit(ENTRY_OWNER_DEVICE_DATA).
> I searched in the history, it actually was the other way around. You
> changed test_and_clear_bit() to test_bit() in the TX path. I think this
> is also the right way to go in RX.
> >> Would it make more sense to possibly
> >> set ENTRY_DATA_IO_FAILED before clearing ENTRY_OWNER_DEVICE_DATA in
> >> rt2x00usb_interrupt_rxdone() as for tx?
> > I don't think so, ENTRY_DATA_IO_FAILED should be only set on error
> > case.
>
> Yes of course. But if the error occurs, it should be signalled before
> enabling the workqueue processing, see the race described above.
>
> After some more testing I'm convinced that this would be the right fix
> for this problem. I will send a v2 of this patch accordingly.
Great, now I understand the problem. Thank you very much!
Stanislaw
^ permalink raw reply
* Re: [PATCH 2/3 bpf-next] i40e: Support zero-copy XDP_TX on the RX path for AF_XDP sockets.
From: Magnus Karlsson @ 2019-07-01 11:04 UTC (permalink / raw)
To: Jonathan Lemon
Cc: Network Development, Björn Töpel, Karlsson, Magnus,
Jakub Kicinski, jeffrey.t.kirsher, kernel-team
In-Reply-To: <20190628221555.3009654-3-jonathan.lemon@gmail.com>
On Sat, Jun 29, 2019 at 12:18 AM Jonathan Lemon
<jonathan.lemon@gmail.com> wrote:
>
> When the XDP program attached to a zero-copy AF_XDP socket returns XDP_TX,
> queue the umem frame on the XDP TX ring. Space on the recycle stack is
> pre-allocated when the xsk is created. (taken from tx_ring, since the
> xdp ring is not initialized yet)
>
> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
> ---
> drivers/net/ethernet/intel/i40e/i40e_txrx.h | 1 +
> drivers/net/ethernet/intel/i40e/i40e_xsk.c | 54 +++++++++++++++++++--
> 2 files changed, 51 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.h b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> index 100e92d2982f..3e7954277737 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> @@ -274,6 +274,7 @@ static inline unsigned int i40e_txd_use_count(unsigned int size)
> #define I40E_TX_FLAGS_TSYN BIT(8)
> #define I40E_TX_FLAGS_FD_SB BIT(9)
> #define I40E_TX_FLAGS_UDP_TUNNEL BIT(10)
> +#define I40E_TX_FLAGS_ZC_FRAME BIT(11)
> #define I40E_TX_FLAGS_VLAN_MASK 0xffff0000
> #define I40E_TX_FLAGS_VLAN_PRIO_MASK 0xe0000000
> #define I40E_TX_FLAGS_VLAN_PRIO_SHIFT 29
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> index ce8650d06962..020f9859215d 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> @@ -91,7 +91,8 @@ static int i40e_xsk_umem_enable(struct i40e_vsi *vsi, struct xdp_umem *umem,
> qid >= netdev->real_num_tx_queues)
> return -EINVAL;
>
> - if (!xsk_umem_recycle_alloc(umem, vsi->rx_rings[0]->count))
> + if (!xsk_umem_recycle_alloc(umem, vsi->rx_rings[0]->count +
> + vsi->tx_rings[0]->count))
> return -ENOMEM;
>
> err = i40e_xsk_umem_dma_map(vsi, umem);
> @@ -175,6 +176,48 @@ int i40e_xsk_umem_setup(struct i40e_vsi *vsi, struct xdp_umem *umem,
> i40e_xsk_umem_disable(vsi, qid);
> }
>
> +static int i40e_xmit_rcvd_zc(struct i40e_ring *rx_ring, struct xdp_buff *xdp)
This function looks very much like i40e_xmit_xdp_ring(). How can we
refactor them to make them share more code and not lose performance at
the same time? This comment is also valid for the ixgbe driver patch
that follows.
Thanks: Magnus
> +{
> + struct i40e_ring *xdp_ring;
> + struct i40e_tx_desc *tx_desc;
> + struct i40e_tx_buffer *tx_bi;
> + struct xdp_frame *xdpf;
> + dma_addr_t dma;
> +
> + xdp_ring = rx_ring->vsi->xdp_rings[rx_ring->queue_index];
> +
> + if (!unlikely(I40E_DESC_UNUSED(xdp_ring))) {
> + xdp_ring->tx_stats.tx_busy++;
> + return I40E_XDP_CONSUMED;
> + }
> + xdpf = convert_to_xdp_frame_keep_zc(xdp);
> + if (unlikely(!xdpf))
> + return I40E_XDP_CONSUMED;
> + xdpf->handle = xdp->handle;
> +
> + dma = xdp_umem_get_dma(rx_ring->xsk_umem, xdp->handle);
> + tx_bi = &xdp_ring->tx_bi[xdp_ring->next_to_use];
> + tx_bi->bytecount = xdpf->len;
> + tx_bi->gso_segs = 1;
> + tx_bi->xdpf = xdpf;
> + tx_bi->tx_flags = I40E_TX_FLAGS_ZC_FRAME;
> +
> + tx_desc = I40E_TX_DESC(xdp_ring, xdp_ring->next_to_use);
> + tx_desc->buffer_addr = cpu_to_le64(dma);
> + tx_desc->cmd_type_offset_bsz = build_ctob(I40E_TX_DESC_CMD_ICRC |
> + I40E_TX_DESC_CMD_EOP,
> + 0, xdpf->len, 0);
> + smp_wmb();
> +
> + xdp_ring->next_to_use++;
> + if (xdp_ring->next_to_use == xdp_ring->count)
> + xdp_ring->next_to_use = 0;
> +
> + tx_bi->next_to_watch = tx_desc;
> +
> + return I40E_XDP_TX;
> +}
> +
> /**
> * i40e_run_xdp_zc - Executes an XDP program on an xdp_buff
> * @rx_ring: Rx ring
> @@ -187,7 +230,6 @@ int i40e_xsk_umem_setup(struct i40e_vsi *vsi, struct xdp_umem *umem,
> static int i40e_run_xdp_zc(struct i40e_ring *rx_ring, struct xdp_buff *xdp)
> {
> int err, result = I40E_XDP_PASS;
> - struct i40e_ring *xdp_ring;
> struct bpf_prog *xdp_prog;
> u32 act;
>
> @@ -202,8 +244,7 @@ static int i40e_run_xdp_zc(struct i40e_ring *rx_ring, struct xdp_buff *xdp)
> case XDP_PASS:
> break;
> case XDP_TX:
> - xdp_ring = rx_ring->vsi->xdp_rings[rx_ring->queue_index];
> - result = i40e_xmit_xdp_tx_ring(xdp, xdp_ring);
> + result = i40e_xmit_rcvd_zc(rx_ring, xdp);
> break;
> case XDP_REDIRECT:
> err = xdp_do_redirect(rx_ring->netdev, xdp, xdp_prog);
> @@ -628,6 +669,11 @@ static bool i40e_xmit_zc(struct i40e_ring *xdp_ring, unsigned int budget)
> static void i40e_clean_xdp_tx_buffer(struct i40e_ring *tx_ring,
> struct i40e_tx_buffer *tx_bi)
> {
> + if (tx_bi->tx_flags & I40E_TX_FLAGS_ZC_FRAME) {
> + xsk_umem_recycle_addr(tx_ring->xsk_umem, tx_bi->xdpf->handle);
> + tx_bi->tx_flags = 0;
> + return;
> + }
> xdp_return_frame(tx_bi->xdpf);
> dma_unmap_single(tx_ring->dev,
> dma_unmap_addr(tx_bi, dma),
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH 1/3 bpf-next] net: add convert_to_xdp_frame_keep_zc function
From: Magnus Karlsson @ 2019-07-01 11:02 UTC (permalink / raw)
To: Jonathan Lemon
Cc: Network Development, Björn Töpel, Karlsson, Magnus,
Jakub Kicinski, jeffrey.t.kirsher, kernel-team
In-Reply-To: <20190628221555.3009654-2-jonathan.lemon@gmail.com>
On Sat, Jun 29, 2019 at 12:19 AM Jonathan Lemon
<jonathan.lemon@gmail.com> wrote:
>
> Add a function which converts a ZC xdp_buff to a an xdp_frame, while
nit: "a an" -> "an"
> keeping the zc backing storage. This will be used to support TX of
> received AF_XDP frames.
>
> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
> ---
> include/net/xdp.h | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/xdp.h b/include/net/xdp.h
> index 40c6d3398458..abe5f47ff0a5 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h
> @@ -82,6 +82,7 @@ struct xdp_frame {
> */
> struct xdp_mem_info mem;
> struct net_device *dev_rx; /* used by cpumap */
> + unsigned long handle;
> };
>
> /* Clear kernel pointers in xdp_frame */
> @@ -95,15 +96,12 @@ struct xdp_frame *xdp_convert_zc_to_xdp_frame(struct xdp_buff *xdp);
>
> /* Convert xdp_buff to xdp_frame */
> static inline
> -struct xdp_frame *convert_to_xdp_frame(struct xdp_buff *xdp)
> +struct xdp_frame *__convert_to_xdp_frame(struct xdp_buff *xdp)
> {
> struct xdp_frame *xdp_frame;
> int metasize;
> int headroom;
>
> - if (xdp->rxq->mem.type == MEM_TYPE_ZERO_COPY)
> - return xdp_convert_zc_to_xdp_frame(xdp);
> -
> /* Assure headroom is available for storing info */
> headroom = xdp->data - xdp->data_hard_start;
> metasize = xdp->data - xdp->data_meta;
> @@ -125,6 +123,20 @@ struct xdp_frame *convert_to_xdp_frame(struct xdp_buff *xdp)
> return xdp_frame;
> }
>
> +static inline
> +struct xdp_frame *convert_to_xdp_frame(struct xdp_buff *xdp)
> +{
> + if (xdp->rxq->mem.type == MEM_TYPE_ZERO_COPY)
> + return xdp_convert_zc_to_xdp_frame(xdp);
> + return __convert_to_xdp_frame(xdp);
> +}
> +
> +static inline
> +struct xdp_frame *convert_to_xdp_frame_keep_zc(struct xdp_buff *xdp)
> +{
> + return __convert_to_xdp_frame(xdp);
> +}
> +
> void xdp_return_frame(struct xdp_frame *xdpf);
> void xdp_return_frame_rx_napi(struct xdp_frame *xdpf);
> void xdp_return_buff(struct xdp_buff *xdp);
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH 0/3 bpf-next] intel: AF_XDP support for TX of RX packets
From: Magnus Karlsson @ 2019-07-01 11:01 UTC (permalink / raw)
To: Jonathan Lemon
Cc: Network Development, Björn Töpel, Karlsson, Magnus,
Jakub Kicinski, jeffrey.t.kirsher, kernel-team
In-Reply-To: <20190628221555.3009654-1-jonathan.lemon@gmail.com>
On Sat, Jun 29, 2019 at 12:18 AM Jonathan Lemon
<jonathan.lemon@gmail.com> wrote:
>
> NOTE: This patch depends on my previous "xsk: reuse cleanup" patch,
> sent to netdev earlier.
>
> The motivation is to have packets which were received on a zero-copy
> AF_XDP socket, and which returned a TX verdict from the bpf program,
> queued directly on the TX ring (if they're in the same napi context).
>
> When these TX packets are completed, they are placed back onto the
> reuse queue, as there isn't really any other place to handle them.
>
> Space in the reuse queue is preallocated at init time for both the
> RX and TX rings. Another option would have a smaller TX queue size
> and count in-flight TX packets, dropping any which exceed the reuseq
> size - this approach is omitted for simplicity.
This should speed up XDP_TX under ZC substantially, which of course is
a good thing. Would be great if you could add some performance
numbers.
As other people have pointed out, it would have been great if we had a
page pool we could return the buffers to. But we do not so there are
only two options: keep it in the kernel on the reuse queue in this
case, or return the buffer to user space with a length of zero
indicating that there is no packet data. Just a transfer of ownership.
But let us go with the former one as you have done in this patch set,
as we have so far have always tried to reuse the buffers inside the
kernel. But the latter option might be good to have in store as a
solution for other problems.
/Magnus
>
> Jonathan Lemon (3):
> net: add convert_to_xdp_frame_keep_zc function
> i40e: Support zero-copy XDP_TX on the RX path for AF_XDP sockets.
> ixgbe: Support zero-copy XDP_TX on the RX path for AF_XDP sockets.
>
> drivers/net/ethernet/intel/i40e/i40e_txrx.h | 1 +
> drivers/net/ethernet/intel/i40e/i40e_xsk.c | 54 ++++++++++++--
> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 1 +
> drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 74 +++++++++++++++++---
> include/net/xdp.h | 20 ++++--
> 5 files changed, 134 insertions(+), 16 deletions(-)
>
> --
> 2.17.1
>
^ permalink raw reply
* [PATCH v2 1/2] rt2x00usb: fix rx queue hang
From: Soeren Moch @ 2019-07-01 10:53 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: Soeren Moch, stable, Helmut Schaa, Kalle Valo, David S. Miller,
linux-wireless, netdev, linux-kernel
Since commit ed194d136769 ("usb: core: remove local_irq_save() around
->complete() handler") the handler rt2x00usb_interrupt_rxdone() is
not running with interrupts disabled anymore. So this completion handler
is not guaranteed to run completely before workqueue processing starts
for the same queue entry.
Be sure to set all other flags in the entry correctly before marking
this entry ready for workqueue processing. This way we cannot miss error
conditions that need to be signalled from the completion handler to the
worker thread.
Note that rt2x00usb_work_rxdone() processes all available entries, not
only such for which queue_work() was called.
This patch is similar to what commit df71c9cfceea ("rt2x00: fix order
of entry flags modification") did for TX processing.
This fixes a regression on a RT5370 based wifi stick in AP mode, which
suddenly stopped data transmission after some period of heavy load. Also
stopping the hanging hostapd resulted in the error message "ieee80211
phy0: rt2x00queue_flush_queue: Warning - Queue 14 failed to flush".
Other operation modes are probably affected as well, this just was
the used testcase.
Fixes: ed194d136769 ("usb: core: remove local_irq_save() around ->complete() handler")
Cc: stable@vger.kernel.org # 4.20+
Signed-off-by: Soeren Moch <smoch@web.de>
---
Changes in v2:
complete rework
Cc: Stanislaw Gruszka <sgruszka@redhat.com>
Cc: Helmut Schaa <helmut.schaa@googlemail.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
drivers/net/wireless/ralink/rt2x00/rt2x00usb.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c b/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c
index 67b81c7221c4..7e3a621b9c0d 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c
@@ -372,14 +372,9 @@ static void rt2x00usb_interrupt_rxdone(struct urb *urb)
struct queue_entry *entry = (struct queue_entry *)urb->context;
struct rt2x00_dev *rt2x00dev = entry->queue->rt2x00dev;
- if (!test_and_clear_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags))
+ if (!test_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags))
return;
- /*
- * Report the frame as DMA done
- */
- rt2x00lib_dmadone(entry);
-
/*
* Check if the received data is simply too small
* to be actually valid, or if the urb is signaling
@@ -388,6 +383,11 @@ static void rt2x00usb_interrupt_rxdone(struct urb *urb)
if (urb->actual_length < entry->queue->desc_size || urb->status)
set_bit(ENTRY_DATA_IO_FAILED, &entry->flags);
+ /*
+ * Report the frame as DMA done
+ */
+ rt2x00lib_dmadone(entry);
+
/*
* Schedule the delayed work for reading the RX status
* from the device.
--
2.17.1
^ permalink raw reply related
* [PATCH v2 2/2] rt2x00usb: remove unnecessary rx flag checks
From: Soeren Moch @ 2019-07-01 10:53 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: Soeren Moch, Helmut Schaa, Kalle Valo, David S. Miller,
linux-wireless, netdev, linux-kernel
In-Reply-To: <20190701105314.9707-1-smoch@web.de>
In contrast to the TX path, there is no need to separately read the transfer
status from the device after receiving RX data. Consequently, there is no
real STATUS_PENDING RX processing queue entry state.
Remove the unnecessary ENTRY_DATA_STATUS_PENDING flag checks from the RX path.
Also remove the misleading comment about reading RX status from device.
Suggested-by: Stanislaw Gruszka <sgruszka@redhat.com>
Signed-off-by: Soeren Moch <smoch@web.de>
---
Changes in v2:
new patch
Cc: Stanislaw Gruszka <sgruszka@redhat.com>
Cc: Helmut Schaa <helmut.schaa@googlemail.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
drivers/net/wireless/ralink/rt2x00/rt2x00usb.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c b/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c
index 7e3a621b9c0d..bc2dfef0de22 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c
@@ -349,8 +349,7 @@ static void rt2x00usb_work_rxdone(struct work_struct *work)
while (!rt2x00queue_empty(rt2x00dev->rx)) {
entry = rt2x00queue_get_entry(rt2x00dev->rx, Q_INDEX_DONE);
- if (test_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags) ||
- !test_bit(ENTRY_DATA_STATUS_PENDING, &entry->flags))
+ if (test_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags))
break;
/*
@@ -389,8 +388,7 @@ static void rt2x00usb_interrupt_rxdone(struct urb *urb)
rt2x00lib_dmadone(entry);
/*
- * Schedule the delayed work for reading the RX status
- * from the device.
+ * Schedule the delayed work for processing RX data
*/
queue_work(rt2x00dev->workqueue, &rt2x00dev->rxdone_work);
}
@@ -402,8 +400,7 @@ static bool rt2x00usb_kick_rx_entry(struct queue_entry *entry, void *data)
struct queue_entry_priv_usb *entry_priv = entry->priv_data;
int status;
- if (test_and_set_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags) ||
- test_bit(ENTRY_DATA_STATUS_PENDING, &entry->flags))
+ if (test_and_set_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags))
return false;
rt2x00lib_dmastart(entry);
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] rt2x00: fix rx queue hang
From: Soeren Moch @ 2019-07-01 10:49 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: Helmut Schaa, Kalle Valo, David S. Miller, linux-wireless, netdev,
linux-kernel, stable
In-Reply-To: <20190629085041.GA2854@redhat.com>
Hello!
On 29.06.19 10:50, Stanislaw Gruszka wrote:
> Hello
>
> On Wed, Jun 26, 2019 at 03:28:00PM +0200, Soeren Moch wrote:
>> Hi Stanislaw,
>>
>> the good news is: your patch below also solves the issue for me. But
>> removing the ENTRY_DATA_STATUS_PENDING check in
>> rt2x00usb_kick_rx_entry() alone does not help, while removing this check
>> in rt2x00usb_work_rxdone() alone does the trick.
>>
>> So the real race seems to be that the flags set in the completion
>> handler are not yet visible on the cpu core running the workqueue. And
>> because the worker is not rescheduled when aborted, the entry can just
>> wait forever.
>> Do you think this could make sense?
> Yes.
>
>>> I'm somewhat reluctant to change the order, because TX processing
>>> might relay on it (we first mark we wait for TX status and
>>> then mark entry is no longer owned by hardware).
>> OK, maybe it's just good luck that changing the order solves the rx
>> problem. Or can memory barriers associated with the spinlock in
>> rt2x00lib_dmadone() be responsible for that?
>> (I'm testing on a armv7 system, Cortex-A9 quadcore.)
> I'm not sure, rt2x00queue_index_inc() also disable/enable interrupts,
> so maybe that make race not reproducible.
I tested some more, the race is between setting ENTRY_DATA_IO_FAILED (if
needed) and enabling workqueue processing. This enabling was done via
ENTRY_DATA_STATUS_PENDING in my patch. So setting
ENTRY_DATA_STATUS_PENDING behind the spinlock in
rt2x00lib_dmadone()/rt2x00queue_index_inc() moved this very close to
setting of ENTRY_DATA_IO_FAILED (if needed). While still in the wrong
order, this made it very unlikely for the race to show up.
>
>> While looking at it, why we double-clear ENTRY_OWNER_DEVICE_DATA in
>> rt2x00usb_interrupt_rxdone() directly and in rt2x00lib_dmadone() again,
> rt2x00lib_dmadone() is called also on other palaces (error paths)
> when we have to clear flags.
Yes, but also clearing ENTRY_OWNER_DEVICE_DATA in
rt2x00usb_interrupt_rxdone() directly is not necessary and can lead to
the wrong processing order.
>> while not doing the same for tx?
> If I remember correctly we have some races on rx (not happened on tx)
> that was solved by using test_and_clear_bit(ENTRY_OWNER_DEVICE_DATA).
I searched in the history, it actually was the other way around. You
changed test_and_clear_bit() to test_bit() in the TX path. I think this
is also the right way to go in RX.
>> Would it make more sense to possibly
>> set ENTRY_DATA_IO_FAILED before clearing ENTRY_OWNER_DEVICE_DATA in
>> rt2x00usb_interrupt_rxdone() as for tx?
> I don't think so, ENTRY_DATA_IO_FAILED should be only set on error
> case.
Yes of course. But if the error occurs, it should be signalled before
enabling the workqueue processing, see the race described above.
After some more testing I'm convinced that this would be the right fix
for this problem. I will send a v2 of this patch accordingly.
>
>>> However on RX
>>> side ENTRY_DATA_STATUS_PENDING bit make no sense as we do not
>>> wait for status. We should remove ENTRY_DATA_STATUS_PENDING on
>>> RX side and perhaps this also will solve issue you observe.
>> I agree that removing the unnecessary checks is a good idea in any case.
>>> Could you please check below patch, if it fixes the problem as well?
>> At least I could not trigger the problem within transferring 10GB of
>> data. But maybe the probability for triggering this bug is just lower
>> because ENTRY_OWNER_DEVICE_DATA is cleared some time before
>> ENTRY_DATA_STATUS_PENDING is set?
> Not sure. Anyway, could you post patch removing not needed checks
> with proper description/changelog ?
>
OK, I will do so.
Soeren
^ permalink raw reply
* Re: [PATCH V3] can: flexcan: fix stop mode acknowledgment
From: Marc Kleine-Budde @ 2019-07-01 10:37 UTC (permalink / raw)
To: Joakim Zhang, linux-can@vger.kernel.org
Cc: dl-linux-imx, wg@grandegger.com, netdev@vger.kernel.org
In-Reply-To: <20190619074035.25719-1-qiangqing.zhang@nxp.com>
[-- Attachment #1.1: Type: text/plain, Size: 992 bytes --]
On 6/19/19 9:42 AM, Joakim Zhang wrote:
> To enter stop mode, the CPU should manually assert a global Stop Mode
> request and check the acknowledgment asserted by FlexCAN. The CPU must
> only consider the FlexCAN in stop mode when both request and
> acknowledgment conditions are satisfied.
>
> Fixes: de3578c198c6 ("can: flexcan: add self wakeup support")
> Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
>
> ChangeLog:
> V1->V2:
> * regmap_read()-->regmap_read_poll_timeout()
> V2->V3:
> * change the way of error return, it will make easy for function
> extension.
Please rebase to linux-next/master, as this is a fix.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* RE: [PATCH net-next v2 00/10] net: stmmac: 10GbE using XGMAC
From: Jose Abreu @ 2019-07-01 10:19 UTC (permalink / raw)
To: David Miller, Jose.Abreu@synopsys.com
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
Joao.Pinto@synopsys.com, peppe.cavallaro@st.com,
alexandre.torgue@st.com
In-Reply-To: <20190628.092415.219171929303857748.davem@davemloft.net>
From: David Miller <davem@davemloft.net>
> About the Kconfig change, maybe it just doesn't make sense to list all
> of the various speeds the chip supports... just a thought.
What about: "STMicroelectronics Multi-Gigabit Ethernet driver" ?
Or, just "STMicroelectronics Ethernet driver" ?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox