Netdev List
 help / color / mirror / Atom feed
* [PATCH net-next] Revert "net: sched: route: don't set arg->stop in route4_walk() when empty"
From: Vlad Buslov @ 2019-02-27 13:48 UTC (permalink / raw)
  To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, Vlad Buslov

This reverts commit 3027ff41f67c ("net: sched: route: don't set arg->stop
in route4_walk() when empty")

Cls API function tcf_proto_is_empty() was changed in commit
6676d5e416ee ("net: sched: set dedicated tcf_walker flag when tp is empty")
to no longer depend on arg->stop to determine that classifier instance is
empty. Instead, it adds dedicated arg->nonempty field, which makes the fix
in route classifier no longer necessary.

Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
 net/sched/cls_route.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index f006af23b64a..319f68cc233f 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -567,7 +567,10 @@ static void route4_walk(struct tcf_proto *tp, struct tcf_walker *arg,
 	struct route4_head *head = rtnl_dereference(tp->root);
 	unsigned int h, h1;
 
-	if (head == NULL || arg->stop)
+	if (head == NULL)
+		arg->stop = 1;
+
+	if (arg->stop)
 		return;
 
 	for (h = 0; h <= 256; h++) {
-- 
2.13.6


^ permalink raw reply related

* [PATCH RESEND ethtool] ethtool: add 10000baseR_FEC link mode name
From: Michal Kubecek @ 2019-02-27 13:49 UTC (permalink / raw)
  To: John W. Linville; +Cc: netdev

ETHTOOL_LINK_MODE_10000baseR_FEC_BIT link mode is listed in
ethtool_link_mode_bit_indices array but doesn't have its name assigned in
the table in dump_link_caps() (so that it's not shown in "ethtool <dev>"
output) and is not listed in the manual page. Add it to both.

Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 ethtool.8.in | 1 +
 ethtool.c    | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/ethtool.8.in b/ethtool.8.in
index 5a26cff5fb33..3d288eaadf23 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -633,6 +633,7 @@ lB	l	lB.
 0x1000	10000baseT Full
 0x40000	10000baseKX4 Full
 0x80000	10000baseKR Full
+0x100000	10000baseR_FEC
 0x40000000000	10000baseCR  Full
 0x80000000000	10000baseSR  Full
 0x100000000000	10000baseLR  Full
diff --git a/ethtool.c b/ethtool.c
index fb4c0886ca84..b6a1eaa383a3 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -633,6 +633,8 @@ static void dump_link_caps(const char *prefix, const char *an_prefix,
 		  "10000baseKX4/Full" },
 		{ 0, ETHTOOL_LINK_MODE_10000baseKR_Full_BIT,
 		  "10000baseKR/Full" },
+		{ 0, ETHTOOL_LINK_MODE_10000baseR_FEC_BIT,
+		  "10000baseR_FEC" },
 		{ 0, ETHTOOL_LINK_MODE_20000baseMLD2_Full_BIT,
 		  "20000baseMLD2/Full" },
 		{ 0, ETHTOOL_LINK_MODE_20000baseKR2_Full_BIT,
-- 
2.20.1


^ permalink raw reply related

* Re: [PATCH iproute2-next] devlink: add support for updating device flash
From: Jiri Pirko @ 2019-02-27 13:44 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: dsahern, stephen, oss-drivers, netdev
In-Reply-To: <20190226202014.23626-1-jakub.kicinski@netronome.com>

Tue, Feb 26, 2019 at 09:20:14PM CET, jakub.kicinski@netronome.com wrote:
>Add new command for updating flash of devices via devlink API.
>Example:
>
>$ cp flash-boot.bin /lib/firmware/
>$ devlink dev flash pci/0000:05:00.0 file flash-boot.bin
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Acked-by: Jiri Pirko <jiri@mellanox.com>

Thanks!

>---
> devlink/devlink.c      | 54 ++++++++++++++++++++++++++++++++++++++++++
> man/man8/devlink-dev.8 | 32 +++++++++++++++++++++++++
> 2 files changed, 86 insertions(+)
>
>diff --git a/devlink/devlink.c b/devlink/devlink.c
>index 960cdda99b5b..5c6cac1f76dd 100644
>--- a/devlink/devlink.c
>+++ b/devlink/devlink.c
>@@ -199,6 +199,8 @@ static void ifname_map_free(struct ifname_map *ifname_map)
> #define DL_OPT_REGION_SNAPSHOT_ID	BIT(22)
> #define DL_OPT_REGION_ADDRESS		BIT(23)
> #define DL_OPT_REGION_LENGTH		BIT(24)
>+#define DL_OPT_FLASH_FILE_NAME	BIT(25)
>+#define DL_OPT_FLASH_COMPONENT	BIT(26)
> 
> struct dl_opts {
> 	uint32_t present; /* flags of present items */
>@@ -230,6 +232,8 @@ struct dl_opts {
> 	uint32_t region_snapshot_id;
> 	uint64_t region_address;
> 	uint64_t region_length;
>+	const char *flash_file_name;
>+	const char *flash_component;
> };
> 
> struct dl {
>@@ -1185,6 +1189,20 @@ static int dl_argv_parse(struct dl *dl, uint32_t o_required,
> 			if (err)
> 				return err;
> 			o_found |= DL_OPT_REGION_LENGTH;
>+		} else if (dl_argv_match(dl, "file") &&
>+			   (o_all & DL_OPT_FLASH_FILE_NAME)) {
>+			dl_arg_inc(dl);
>+			err = dl_argv_str(dl, &opts->flash_file_name);
>+			if (err)
>+				return err;
>+			o_found |= DL_OPT_FLASH_FILE_NAME;
>+		} else if (dl_argv_match(dl, "component") &&
>+			   (o_all & DL_OPT_FLASH_COMPONENT)) {
>+			dl_arg_inc(dl);
>+			err = dl_argv_str(dl, &opts->flash_component);
>+			if (err)
>+				return err;
>+			o_found |= DL_OPT_FLASH_COMPONENT;
> 		} else {
> 			pr_err("Unknown option \"%s\"\n", dl_argv(dl));
> 			return -EINVAL;
>@@ -1389,6 +1407,12 @@ static void dl_opts_put(struct nlmsghdr *nlh, struct dl *dl)
> 	if (opts->present & DL_OPT_REGION_LENGTH)
> 		mnl_attr_put_u64(nlh, DEVLINK_ATTR_REGION_CHUNK_LEN,
> 				 opts->region_length);
>+	if (opts->present & DL_OPT_FLASH_FILE_NAME)
>+		mnl_attr_put_strz(nlh, DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME,
>+				  opts->flash_file_name);
>+	if (opts->present & DL_OPT_FLASH_COMPONENT)
>+		mnl_attr_put_strz(nlh, DEVLINK_ATTR_FLASH_UPDATE_COMPONENT,
>+				  opts->flash_component);
> }
> 
> static int dl_argv_parse_put(struct nlmsghdr *nlh, struct dl *dl,
>@@ -1451,6 +1475,7 @@ static void cmd_dev_help(void)
> 	pr_err("       devlink dev param show [DEV name PARAMETER]\n");
> 	pr_err("       devlink dev reload DEV\n");
> 	pr_err("       devlink dev info [ DEV ]\n");
>+	pr_err("       devlink dev flash DEV file PATH [ component NAME ]\n");
> }
> 
> static bool cmp_arr_last_handle(struct dl *dl, const char *bus_name,
>@@ -2583,6 +2608,32 @@ static int cmd_dev_info(struct dl *dl)
> 	return err;
> }
> 
>+static void cmd_dev_flash_help(void)
>+{
>+	pr_err("Usage: devlink dev flash DEV file PATH [ component NAME ]\n");
>+}
>+
>+static int cmd_dev_flash(struct dl *dl)
>+{
>+	struct nlmsghdr *nlh;
>+	int err;
>+
>+	if (dl_argv_match(dl, "help") || dl_no_arg(dl)) {
>+		cmd_dev_flash_help();
>+		return 0;
>+	}
>+
>+	nlh = mnlg_msg_prepare(dl->nlg, DEVLINK_CMD_FLASH_UPDATE,
>+			       NLM_F_REQUEST | NLM_F_ACK);
>+
>+	err = dl_argv_parse_put(nlh, dl, DL_OPT_HANDLE | DL_OPT_FLASH_FILE_NAME,
>+				DL_OPT_FLASH_COMPONENT);
>+	if (err)
>+		return err;
>+
>+	return _mnlg_socket_sndrcv(dl->nlg, nlh, NULL, NULL);
>+}
>+
> static int cmd_dev(struct dl *dl)
> {
> 	if (dl_argv_match(dl, "help")) {
>@@ -2604,6 +2655,9 @@ static int cmd_dev(struct dl *dl)
> 	} else if (dl_argv_match(dl, "info")) {
> 		dl_arg_inc(dl);
> 		return cmd_dev_info(dl);
>+	} else if (dl_argv_match(dl, "flash")) {
>+		dl_arg_inc(dl);
>+		return cmd_dev_flash(dl);
> 	}
> 	pr_err("Command \"%s\" not found\n", dl_argv(dl));
> 	return -ENOENT;
>diff --git a/man/man8/devlink-dev.8 b/man/man8/devlink-dev.8
>index 47838371fecd..1804463b2321 100644
>--- a/man/man8/devlink-dev.8
>+++ b/man/man8/devlink-dev.8
>@@ -69,6 +69,16 @@ devlink-dev \- devlink device configuration
> .IR DEV
> .RI "]"
> 
>+.ti -8
>+.BR "devlink dev flash"
>+.IR DEV
>+.BR file
>+.IR PATH
>+.RI "["
>+.BR target
>+.IR ID
>+.RI "]"
>+
> .SH "DESCRIPTION"
> .SS devlink dev show - display devlink device attributes
> 
>@@ -177,6 +187,28 @@ versions may differ after flash has been updated, but before reboot.
> - specifies the devlink device to show.
> If this argument is omitted all devices are listed.
> 
>+.SS devlink dev flash - write device's non-volatile memory.
>+
>+.PP
>+.I "DEV"
>+- specifies the devlink device to write to.
>+
>+.BR file
>+.I PATH
>+- Path to the file which will be written into device's flash. The path needs
>+to be relative to one of the directories searched by the kernel firmware loaded,
>+such as /lib/firmware.
>+
>+.BR component
>+.I NAME
>+- If device stores multiple firmware images in non-volatile memory, this
>+parameter may be used to indicate which firmware image should be written.
>+The value of
>+.I NAME
>+should match the component names from
>+.B "devlink dev info"
>+and may be driver-dependent.
>+
> .SH "EXAMPLES"
> .PP
> devlink dev show
>-- 
>2.19.2
>

^ permalink raw reply

* Re: [PATCH net-next 3/3] net: dsa: microchip: add other KSZ9477 switch variants
From: Andrew Lunn @ 2019-02-27 13:33 UTC (permalink / raw)
  To: Tristram.Ha; +Cc: Florian Fainelli, Pavel Machek, UNGLinuxDriver, netdev
In-Reply-To: <1551224265-9304-4-git-send-email-Tristram.Ha@microchip.com>

On Tue, Feb 26, 2019 at 03:37:45PM -0800, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
> 
> Add other switches in KSZ9477 family.
> 
> KSZ9896 is a switch with 6 ports; the last one is typically used to
> connect to MAC.
> KSZ9567 is same as KSZ9897 but with 1588 PTP capability.
> KSZ8567 is same as KSZ9567 but without gigabit capability.
> KSZ9563 is same as KSZ9893 but with 1588 PTP capability.
> KSZ8563 is same as KSZ9563 but without gigabit capability.
> KSZ8565 is a switch with 5 ports; however, port 7 has to be used to
> connect to MAC.  This chip can only be set through device tree.
> 
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> ---
>  drivers/net/dsa/microchip/ksz9477.c     | 93 ++++++++++++++++++++++++++++++++-
>  drivers/net/dsa/microchip/ksz9477_spi.c |  3 ++
>  drivers/net/dsa/microchip/ksz_common.c  |  4 ++
>  3 files changed, 98 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index 3bb548a..81e7c2f 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -1264,6 +1264,32 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  		ksz_pread16(dev, port, REG_PORT_PHY_INT_ENABLE, &data16);
>  }
>  
> +#define KSZ_CHIP_NAME_SIZE		18
> +
> +static char *ksz9477_chip_names[KSZ_CHIP_NAME_SIZE] = {
> +	"Microchip KSZ9477",

This looks wrong. You are defining an array of 18 elements, not an
array of pointers to 18 byte chars.


> +	"Microchip KSZ9897",
> +	"Microchip KSZ9896",
> +	"Microchip KSZ9567",
> +	"Microchip KSZ8567",
> +	"Microchip KSZ8565",
> +	"Microchip KSZ9893",
> +	"Microchip KSZ9563",
> +	"Microchip KSZ8563",
> +};
> +
> +enum {
> +	KSZ9477_SW_CHIP,
> +	KSZ9897_SW_CHIP,
> +	KSZ9896_SW_CHIP,
> +	KSZ9567_SW_CHIP,
> +	KSZ8567_SW_CHIP,
> +	KSZ8565_SW_CHIP,
> +	KSZ9893_SW_CHIP,
> +	KSZ9563_SW_CHIP,
> +	KSZ8563_SW_CHIP,
> +};

There should be a one-to-one mapping between this enum and the array
above. You can make this clear using Designated Initializers.

> +
>  static void ksz9477_config_cpu_port(struct dsa_switch *ds)
>  {
>  	struct ksz_device *dev = ds->priv;
> @@ -1314,7 +1340,8 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds)
>  		p->vid_member = (1 << i);
>  		p->member = dev->port_mask;
>  		ksz9477_port_stp_state_set(ds, i, BR_STATE_DISABLED);
> -		p->on = 1;
> +		if (!dsa_is_unused_port(ds, i))
> +			p->on = 1;
>  		if (i < dev->phy_port_cnt)
>  			p->phy = 1;
>  		if (dev->chip_id == 0x00947700 && i == 6) {
> @@ -1406,6 +1433,7 @@ static u32 ksz9477_get_port_addr(int port, int offset)
>  
>  static int ksz9477_switch_detect(struct ksz_device *dev)
>  {
> +	int chip = -1;
>  	u8 data8;
>  	u8 id_hi;
>  	u8 id_lo;
> @@ -1448,6 +1476,12 @@ static int ksz9477_switch_detect(struct ksz_device *dev)
>  			dev->features &= ~GBIT_SUPPORT;
>  		dev->mib_port_cnt = 3;
>  		dev->phy_port_cnt = 2;
> +		if (!(data8 & SW_AVB_ABLE))
> +			chip = KSZ9893_SW_CHIP;
> +		else if (data8 & SW_QW_ABLE)
> +			chip = KSZ8563_SW_CHIP;
> +		else
> +			chip = KSZ9563_SW_CHIP;
>  	} else {
>  		/* Chip uses new XMII register definitions. */
>  		dev->features |= NEW_XMII;
> @@ -1455,6 +1489,37 @@ static int ksz9477_switch_detect(struct ksz_device *dev)
>  		/* Chip does not support gigabit. */
>  		if (!(data8 & SW_GIGABIT_ABLE))
>  			dev->features &= ~GBIT_SUPPORT;
> +		if ((id_lo & 0xf) == 6)
> +			dev->mib_port_cnt = 6;

> +		if (id_hi == FAMILY_ID_94)
> +			chip = KSZ9477_SW_CHIP;
> +		else if (id_hi == FAMILY_ID_98 && id_lo == CHIP_ID_97)
> +			chip = KSZ9897_SW_CHIP;
> +		else if (id_hi == FAMILY_ID_98 && id_lo == CHIP_ID_96)
> +			chip = KSZ9896_SW_CHIP;
> +		else if (id_hi == FAMILY_ID_95 && id_lo == CHIP_ID_67)
> +			chip = KSZ9567_SW_CHIP;
> +		else if (id_hi == FAMILY_ID_85 && id_lo == CHIP_ID_67)
> +			chip = KSZ8567_SW_CHIP;
> +		if (id_lo == CHIP_ID_67) {
> +			id_hi = FAMILY_ID_98;
> +			id_lo = CHIP_ID_97;
> +		} else if (id_lo == CHIP_ID_66) {
> +			id_hi = FAMILY_ID_98;
> +			id_lo = CHIP_ID_96;
> +		}

Maybe add a mask to ksz_chip_data table, so you can mask id_low,
id_high and then compare against chip_id?

> @@ -1495,6 +1564,15 @@ struct ksz_chip_data {
>  		.port_cnt = 7,		/* total physical port count */
>  	},
>  	{
> +		.chip_id = 0x00989600,
> +		.dev_name = "KSZ9896",
> +		.num_vlans = 4096,
> +		.num_alus = 4096,
> +		.num_statics = 16,
> +		.cpu_ports = 0x3F,	/* can be configured as cpu port */
> +		.port_cnt = 6,		/* total port count */
> +	},

  Andrew

^ permalink raw reply

* Re: [PATCH net-next 3/3] net: dsa: microchip: add other KSZ9477 switch variants
From: Andrew Lunn @ 2019-02-27 13:23 UTC (permalink / raw)
  To: Tristram.Ha; +Cc: Florian Fainelli, Pavel Machek, UNGLinuxDriver, netdev
In-Reply-To: <1551224265-9304-4-git-send-email-Tristram.Ha@microchip.com>

> +	if (dev->dev->of_node) {
> +		char name[80];
> +
> +		/* KSZ8565 chip can only be set through device tree. */
> +		if (!of_modalias_node(dev->dev->of_node, name, sizeof(name))) {
> +			if (!strcmp(name, "ksz8565")) {
> +				chip = KSZ8565_SW_CHIP;
> +				id_hi = FAMILY_ID_98;
> +				id_lo = 0x95;
> +			}
> +		}

of_device_is_compatible() seems like a better call use use.

	  Andrew

^ permalink raw reply

* Re: [PATCH v4 perf,bpf 12/15] perf, bpf: enable annotation of bpf program
From: Jiri Olsa @ 2019-02-27 13:22 UTC (permalink / raw)
  To: Song Liu
  Cc: netdev, linux-kernel, ast, daniel, kernel-team, peterz, acme,
	jolsa, namhyung
In-Reply-To: <20190226002019.3748539-13-songliubraving@fb.com>

On Mon, Feb 25, 2019 at 04:20:16PM -0800, Song Liu wrote:

SNIP

> +		if (!opts->hide_src_code && srcline) {
> +			args->offset = -1;
> +			args->line = strdup(srcline);
> +			args->line_nr = 0;
> +			args->ms.sym  = sym;
> +			dl = disasm_line__new(args);
> +			if (dl)
> +				annotation_line__add(&dl->al,
> +						     &notes->src->source);

please use { } around multiline if leg

jirka

^ permalink raw reply

* Re: [PATCH v4 perf,bpf 07/15] perf, bpf: save bpf_prog_info information as headers to perf.data
From: Jiri Olsa @ 2019-02-27 13:21 UTC (permalink / raw)
  To: Song Liu
  Cc: netdev, linux-kernel, ast, daniel, kernel-team, peterz, acme,
	jolsa, namhyung
In-Reply-To: <20190226002019.3748539-8-songliubraving@fb.com>

On Mon, Feb 25, 2019 at 04:20:11PM -0800, Song Liu wrote:

SNIP

> +		if (info_len > sizeof(struct bpf_prog_info)) {
> +			pr_warning("detected invalid bpf_prog_info\n");
> +			goto out;
> +		}
> +
> +		info_linear = malloc(sizeof(struct bpf_prog_info_linear) +
> +				     data_len);
> +		if (!info_linear)
> +			goto out;
> +		info_linear->info_len = sizeof(struct bpf_prog_info);
> +		info_linear->data_len = data_len;
> +		if (do_read_u64(ff, (u64 *)(&info_linear->arrays)))
> +			goto out;
> +		if (__do_read(ff, &info_linear->info, info_len))
> +			goto out;
> +		if (info_len < sizeof(struct bpf_prog_info))
> +			memset(((void *)(&info_linear->info)) + info_len, 0,
> +			       sizeof(struct bpf_prog_info) - info_len);
> +
> +		if (__do_read(ff, info_linear->data, data_len))
> +			goto out;
> +
> +		/* endian mismatch, drop the info, continue */

so there's no way we can swap the data? why?

jirka

> +		if (ff->ph->needs_swap) {
> +			free(info_linear);
> +			continue;
> +		}

SNIP

^ permalink raw reply

* Re: [PATCH v4 perf,bpf 12/15] perf, bpf: enable annotation of bpf program
From: Jiri Olsa @ 2019-02-27 13:21 UTC (permalink / raw)
  To: Song Liu
  Cc: netdev, linux-kernel, ast, daniel, kernel-team, peterz, acme,
	jolsa, namhyung
In-Reply-To: <20190226002019.3748539-13-songliubraving@fb.com>

On Mon, Feb 25, 2019 at 04:20:16PM -0800, Song Liu wrote:

SNIP

> diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> index 093352e93d50..0bc9ec69f38a 100644
> --- a/tools/perf/util/util.c
> +++ b/tools/perf/util/util.c
> @@ -506,3 +506,13 @@ const char *perf_tip(const char *dirpath)
>  
>  	return tip;
>  }
> +
> +void get_exec_path(char *tpath, size_t size)
> +{
> +	const char *path = "/proc/self/exe";
> +	ssize_t len;
> +
> +	len = readlink(path, tpath, size - 1);
> +	assert(len > 0);
> +	tpath[len] = 0;
> +}

we have now helper for that, please rebase this on top of
arnaldo's perf/core:

  94816add0005 perf tools: Add perf_exe() helper to find perf binary

jirka

^ permalink raw reply

* Re: [PATCH v4 perf,bpf 06/15] perf, bpf: save bpf_prog_info in a rbtree in perf_env
From: Jiri Olsa @ 2019-02-27 13:21 UTC (permalink / raw)
  To: Song Liu
  Cc: netdev, linux-kernel, ast, daniel, kernel-team, peterz, acme,
	jolsa, namhyung
In-Reply-To: <20190226002019.3748539-7-songliubraving@fb.com>

On Mon, Feb 25, 2019 at 04:20:10PM -0800, Song Liu wrote:

SNIP

> diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h
> index d01b8355f4ca..33ef4b2d2a29 100644
> --- a/tools/perf/util/env.h
> +++ b/tools/perf/util/env.h
> @@ -3,7 +3,9 @@
>  #define __PERF_ENV_H
>  
>  #include <linux/types.h>
> +#include <linux/rbtree.h>
>  #include "cpumap.h"
> +#include "rwsem.h"
>  
>  struct cpu_topology_map {
>  	int	socket_id;
> @@ -64,8 +66,19 @@ struct perf_env {
>  	struct memory_node	*memory_nodes;
>  	unsigned long long	 memory_bsize;
>  	u64                     clockid_res_ns;
> +
> +	/*
> +	 * bpf_info_lock protects bpf rbtrees. This is needed because the
> +	 * trees are accessed by different threads in perf-top
> +	 */
> +	struct {
> +		struct rw_semaphore	lock;
> +		struct rb_root		prog_infos;

could be just 'infos'                   ^^^^ is already in struct name


> +	} bpf_progs;
>  };
>  


jirka

^ permalink raw reply

* Re: [PATCH v4 perf,bpf 06/15] perf, bpf: save bpf_prog_info in a rbtree in perf_env
From: Jiri Olsa @ 2019-02-27 13:21 UTC (permalink / raw)
  To: Song Liu
  Cc: netdev, linux-kernel, ast, daniel, kernel-team, peterz, acme,
	jolsa, namhyung
In-Reply-To: <20190226002019.3748539-7-songliubraving@fb.com>

On Mon, Feb 25, 2019 at 04:20:10PM -0800, Song Liu wrote:

SNIP

> @@ -38,6 +116,12 @@ void perf_env__exit(struct perf_env *env)
>  	zfree(&env->memory_nodes);
>  }
>  
> +static void init_bpf_rb_trees(struct perf_env *env)
> +{
> +	env->bpf_progs.prog_infos = RB_ROOT;
> +	init_rwsem(&env->bpf_progs.lock);
> +}
> +
>  int perf_env__set_cmdline(struct perf_env *env, int argc, const char *argv[])
>  {
>  	int i;
> @@ -59,6 +143,7 @@ int perf_env__set_cmdline(struct perf_env *env, int argc, const char *argv[])
>  
>  	env->nr_cmdline = argc;
>  
> +	init_bpf_rb_trees(env);

this souldn't be in perf_env__set_cmdline,
it's there to set the cmdline

struct bpf_progs is first in perf_env that
needs other than zero initialization, so
I think we need to add perf_env__init function,
that will do that for all the paths that uses
perf_env


jirka

^ permalink raw reply

* Re: [PATCH v4 perf,bpf 06/15] perf, bpf: save bpf_prog_info in a rbtree in perf_env
From: Jiri Olsa @ 2019-02-27 13:21 UTC (permalink / raw)
  To: Song Liu
  Cc: netdev, linux-kernel, ast, daniel, kernel-team, peterz, acme,
	jolsa, namhyung
In-Reply-To: <20190226002019.3748539-7-songliubraving@fb.com>

On Mon, Feb 25, 2019 at 04:20:10PM -0800, Song Liu wrote:
> bpf_prog_info contains information necessary to annotate bpf programs.
> This patch saves bpf_prog_info for bpf programs loaded in the system.
> 
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
>  tools/perf/util/bpf-event.c | 32 +++++++++++++-
>  tools/perf/util/bpf-event.h |  7 ++-
>  tools/perf/util/env.c       | 85 +++++++++++++++++++++++++++++++++++++
>  tools/perf/util/env.h       | 17 ++++++++
>  4 files changed, 139 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/bpf-event.c b/tools/perf/util/bpf-event.c
> index ff7ee149ec46..ce81b2c43a51 100644
> --- a/tools/perf/util/bpf-event.c
> +++ b/tools/perf/util/bpf-event.c
> @@ -10,6 +10,7 @@
>  #include "debug.h"
>  #include "symbol.h"
>  #include "machine.h"
> +#include "env.h"
>  #include "session.h"
>  
>  #define ptr_to_u64(ptr)    ((__u64)(unsigned long)(ptr))
> @@ -54,17 +55,28 @@ static int perf_event__synthesize_one_bpf_prog(struct perf_session *session,
>  	struct bpf_event *bpf_event = &event->bpf_event;
>  	struct bpf_prog_info_linear *info_linear;
>  	struct perf_tool *tool = session->tool;
> +	struct bpf_prog_info_node *info_node;
>  	struct bpf_prog_info *info;
>  	struct btf *btf = NULL;
>  	bool has_btf = false;
> +	struct perf_env *env;
>  	u32 sub_prog_cnt, i;
>  	int err = 0;
>  	u64 arrays;
>  
> +	/*
> +	 * for perf-record and perf-report use header.env;
> +	 * otherwise, use global perf_env.
> +	 */
> +	env = session->data ? &session->header.env : &perf_env;
> +
>  	arrays = 1UL << BPF_PROG_INFO_JITED_KSYMS;
>  	arrays |= 1UL << BPF_PROG_INFO_JITED_FUNC_LENS;
>  	arrays |= 1UL << BPF_PROG_INFO_FUNC_INFO;
>  	arrays |= 1UL << BPF_PROG_INFO_PROG_TAGS;
> +	arrays |= 1UL << BPF_PROG_INFO_JITED_INSNS;
> +	arrays |= 1UL << BPF_PROG_INFO_LINE_INFO;
> +	arrays |= 1UL << BPF_PROG_INFO_JITED_LINE_INFO;
>  
>  	info_linear = bpf_program__get_prog_info_linear(fd, arrays);
>  	if (IS_ERR_OR_NULL(info_linear)) {
> @@ -153,8 +165,8 @@ static int perf_event__synthesize_one_bpf_prog(struct perf_session *session,
>  						     machine, process);
>  	}
>  
> -	/* Synthesize PERF_RECORD_BPF_EVENT */
>  	if (opts->bpf_event) {
> +		/* Synthesize PERF_RECORD_BPF_EVENT */
>  		*bpf_event = (struct bpf_event){
>  			.header = {
>  				.type = PERF_RECORD_BPF_EVENT,
> @@ -167,6 +179,24 @@ static int perf_event__synthesize_one_bpf_prog(struct perf_session *session,
>  		memcpy(bpf_event->tag, info->tag, BPF_TAG_SIZE);
>  		memset((void *)event + event->header.size, 0, machine->id_hdr_size);
>  		event->header.size += machine->id_hdr_size;
> +
> +		/* save bpf_prog_info to env */

why do we save this to perf_env in here? we just
synthesize the same data as event, so report and
top will read it and fill perf_env again, right?

could you please explain the whole flow of the
bpf events and its respective data in perf_env
and put it into the changelog

> +		info_node = malloc(sizeof(struct bpf_prog_info_node));
> +
> +		/*
> +		 * Do not bail out for !info_node, as we still want to
> +		 * call  perf_tool__process_synth_event()

well, we are out of memory, so I dont think perf_tool__process_synth_event
will get too far.. also the perf_env data would be inconsistent with what
you store as event.. how can that work?

thanks,
jirka

> +		 */
> +		if (info_node) {
> +			info_node->info_linear = info_linear;
> +			perf_env__insert_bpf_prog_info(env, info_node);
> +			info_linear = NULL;
> +		}
> +

SNIP

^ permalink raw reply

* Re: [PATCH v4 perf,bpf 15/15] perf, bpf: save information about short living bpf programs
From: Jiri Olsa @ 2019-02-27 13:21 UTC (permalink / raw)
  To: Song Liu
  Cc: netdev, linux-kernel, ast, daniel, kernel-team, peterz, acme,
	jolsa, namhyung
In-Reply-To: <20190226002019.3748539-16-songliubraving@fb.com>

On Mon, Feb 25, 2019 at 04:20:19PM -0800, Song Liu wrote:

SNIP

> +	btf_id = info_linear->info.btf_id;
> +
> +	info_node = malloc(sizeof(struct bpf_prog_info_node));
> +	if (info_node) {
> +		info_node->info_linear = info_linear;
> +		perf_env__insert_bpf_prog_info(env, info_node);
> +	} else
> +		free(info_linear);
> +
> +	if (btf_id == 0)
> +		goto out;
> +
> +	if (btf__get_from_id(btf_id, &btf)) {
> +		pr_debug("%s: failed to get BTF of id %u, aborting\n",
> +			 __func__, btf_id);
> +		goto out;
> +	}
> +	perf_env__fetch_btf(env, btf_id, btf);

so is this the main reason we are doing this? getting the btf
data for bpf prog ids and store them?

please describe the whole bpf events/features data flow in
changelog as I asked in previous email

thanks,
jirka

^ permalink raw reply

* Re: [PATCH v4 perf,bpf 14/15] perf: introduce side band thread
From: Jiri Olsa @ 2019-02-27 13:21 UTC (permalink / raw)
  To: Song Liu
  Cc: netdev, linux-kernel, ast, daniel, kernel-team, peterz, acme,
	jolsa, namhyung
In-Reply-To: <20190226002019.3748539-15-songliubraving@fb.com>

On Mon, Feb 25, 2019 at 04:20:18PM -0800, Song Liu wrote:

SNIP

> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 8c902276d4b4..61b87c8111e6 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -19,6 +19,7 @@
>  #include "debug.h"
>  #include "units.h"
>  #include "asm/bug.h"
> +#include "bpf-event.h"
>  #include <signal.h>
>  #include <unistd.h>
>  
> @@ -1841,3 +1842,102 @@ struct perf_evsel *perf_evlist__reset_weak_group(struct perf_evlist *evsel_list,
>  	}
>  	return leader;
>  }
> +
> +static struct perf_evlist *sb_evlist;
> +pthread_t poll_thread;

so some of the things are static and some like poll_args
you alloced on the stack.. I dont like this interface,
could we come up with something generic? perhaps
encapsulated in perf_evlist, like:

struct perf_evlist {
	...
	struct {
		pthread_t	th;
		int		state;
	} thread;
};

typedef int (perf_evlist__thread_cb_t)(perf_evlist, union perf_event *event,....)

perf_evlist__start_thread(perf_evlist, perf_evlist__thread_cb_t cb);
perf_evlist__stop_thread(perf_evlist);


jirka

^ permalink raw reply

* Re: [PATCH net-next 2/3] net: dsa: microchip: add KSZ9893 switch support
From: Andrew Lunn @ 2019-02-27 13:14 UTC (permalink / raw)
  To: Tristram.Ha; +Cc: Florian Fainelli, Pavel Machek, UNGLinuxDriver, netdev
In-Reply-To: <1551224265-9304-3-git-send-email-Tristram.Ha@microchip.com>

>  static int ksz9477_phy_read16(struct dsa_switch *ds, int addr, int reg)
> @@ -353,7 +363,7 @@ static int ksz9477_phy_read16(struct dsa_switch *ds, int addr, int reg)
>  			val = 0x796d;
>  			break;
>  		case MII_PHYSID1:
> -			val = 0x0022;
> +			val = KSZ9477_ID_HI;
>  			break;
>  		case MII_PHYSID2:
>  			val = 0x1631;

Hi Tristram

This seems like an unrelated change, so should be in a separate
patch. And if you change PHYID1, it would also make sense to change
PHYID2 to a #define.


Please split the tagging changes into a patch of there own.

       Andrew

^ permalink raw reply

* Re: [PATCH net-next 8/8] devlink: fix kdoc
From: Jiri Pirko @ 2019-02-27 13:13 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: davem, oss-drivers, netdev
In-Reply-To: <20190226182436.23811-9-jakub.kicinski@netronome.com>

Tue, Feb 26, 2019 at 07:24:36PM CET, jakub.kicinski@netronome.com wrote:
>devlink suffers from a few kdoc warnings:
>
>net/core/devlink.c:5292: warning: Function parameter or member 'dev' not described in 'devlink_register'
>net/core/devlink.c:5351: warning: Function parameter or member 'port_index' not described in 'devlink_port_register'
>net/core/devlink.c:5753: warning: Function parameter or member 'parent_resource_id' not described in 'devlink_resource_register'
>net/core/devlink.c:5753: warning: Function parameter or member 'size_params' not described in 'devlink_resource_register'
>net/core/devlink.c:5753: warning: Excess function parameter 'top_hierarchy' description in 'devlink_resource_register'
>net/core/devlink.c:5753: warning: Excess function parameter 'reload_required' description in 'devlink_resource_register'
>net/core/devlink.c:5753: warning: Excess function parameter 'parent_reosurce_id' description in 'devlink_resource_register'
>net/core/devlink.c:6451: warning: Function parameter or member 'region' not described in 'devlink_region_snapshot_create'
>net/core/devlink.c:6451: warning: Excess function parameter 'devlink_region' description in 'devlink_region_snapshot_create'
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Acked-by: Jiri Pirko <jiri@mellanox.com>

^ permalink raw reply

* Re: [PATCH net-next v2 8/8] net: Remove switchdev_ops
From: Ido Schimmel @ 2019-02-27 13:13 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: netdev@vger.kernel.org, David S. Miller, open list,
	open list:STAGING SUBSYSTEM, moderated list:ETHERNET BRIDGE,
	Jiri Pirko, andrew@lunn.ch, vivien.didelot@gmail.com
In-Reply-To: <20190227011427.16487-9-f.fainelli@gmail.com>

On Tue, Feb 26, 2019 at 05:14:27PM -0800, Florian Fainelli wrote:
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
> index b00f6f74f91a..995426ea9a43 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
> @@ -3660,7 +3660,6 @@ static int mlxsw_sp_port_create(struct mlxsw_sp *mlxsw_sp, u8 local_port,
>  	}
>  	mlxsw_sp_port->default_vlan = mlxsw_sp_port_vlan;
>  
> -	mlxsw_sp_port_switchdev_init(mlxsw_sp_port);
>  	mlxsw_sp->ports[local_port] = mlxsw_sp_port;
>  	err = register_netdev(dev);
>  	if (err) {
> @@ -3677,7 +3676,6 @@ static int mlxsw_sp_port_create(struct mlxsw_sp *mlxsw_sp, u8 local_port,
>  
>  err_register_netdev:
>  	mlxsw_sp->ports[local_port] = NULL;
> -	mlxsw_sp_port_switchdev_fini(mlxsw_sp_port);
>  	mlxsw_sp_port_vlan_destroy(mlxsw_sp_port_vlan);
>  err_port_vlan_create:
>  err_port_pvid_set:
> @@ -3720,7 +3718,6 @@ static void mlxsw_sp_port_remove(struct mlxsw_sp *mlxsw_sp, u8 local_port)
>  	mlxsw_core_port_clear(mlxsw_sp->core, local_port, mlxsw_sp);
>  	unregister_netdev(mlxsw_sp_port->dev); /* This calls ndo_stop */
>  	mlxsw_sp->ports[local_port] = NULL;
> -	mlxsw_sp_port_switchdev_fini(mlxsw_sp_port);
>  	mlxsw_sp_port_vlan_flush(mlxsw_sp_port, true);
>  	mlxsw_sp_port_nve_fini(mlxsw_sp_port);
>  	mlxsw_sp_tc_qdisc_fini(mlxsw_sp_port);
> @@ -4441,12 +4438,6 @@ static int mlxsw_sp_init(struct mlxsw_core *mlxsw_core,
>  		goto err_span_init;
>  	}
>  
> -	err = mlxsw_sp_switchdev_init(mlxsw_sp);

I missed that and got a trace as soon as I tried to enslave a port. You
should only remove mlxsw_sp_port_switchdev_init() and not
mlxsw_sp_switchdev_init()

> -	if (err) {
> -		dev_err(mlxsw_sp->bus_info->dev, "Failed to initialize switchdev\n");
> -		goto err_switchdev_init;
> -	}
> -
>  	err = mlxsw_sp_counter_pool_init(mlxsw_sp);
>  	if (err) {
>  		dev_err(mlxsw_sp->bus_info->dev, "Failed to init counter pool\n");
> @@ -4517,8 +4508,6 @@ static int mlxsw_sp_init(struct mlxsw_core *mlxsw_core,
>  err_afa_init:
>  	mlxsw_sp_counter_pool_fini(mlxsw_sp);
>  err_counter_pool_init:
> -	mlxsw_sp_switchdev_fini(mlxsw_sp);
> -err_switchdev_init:
>  	mlxsw_sp_span_fini(mlxsw_sp);
>  err_span_init:
>  	mlxsw_sp_lag_fini(mlxsw_sp);
> @@ -4585,7 +4574,6 @@ static void mlxsw_sp_fini(struct mlxsw_core *mlxsw_core)
>  	mlxsw_sp_nve_fini(mlxsw_sp);
>  	mlxsw_sp_afa_fini(mlxsw_sp);
>  	mlxsw_sp_counter_pool_fini(mlxsw_sp);
> -	mlxsw_sp_switchdev_fini(mlxsw_sp);
>  	mlxsw_sp_span_fini(mlxsw_sp);
>  	mlxsw_sp_lag_fini(mlxsw_sp);
>  	mlxsw_sp_buffers_fini(mlxsw_sp);
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
> index a61c1130d9e3..da6278b0caa4 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
> @@ -407,8 +407,6 @@ extern const struct mlxsw_sp_sb_vals mlxsw_sp2_sb_vals;
>  /* spectrum_switchdev.c */
>  int mlxsw_sp_switchdev_init(struct mlxsw_sp *mlxsw_sp);
>  void mlxsw_sp_switchdev_fini(struct mlxsw_sp *mlxsw_sp);
> -void mlxsw_sp_port_switchdev_init(struct mlxsw_sp_port *mlxsw_sp_port);
> -void mlxsw_sp_port_switchdev_fini(struct mlxsw_sp_port *mlxsw_sp_port);
>  int mlxsw_sp_rif_fdb_op(struct mlxsw_sp *mlxsw_sp, const char *mac, u16 fid,
>  			bool adding);
>  void
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
> index c1aedfea3a31..f6ce386c3036 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
> @@ -1938,10 +1938,6 @@ static struct mlxsw_sp_port *mlxsw_sp_lag_rep_port(struct mlxsw_sp *mlxsw_sp,
>  	return NULL;
>  }
>  
> -static const struct switchdev_ops mlxsw_sp_port_switchdev_ops = {
> -	.switchdev_port_attr_set	= mlxsw_sp_port_attr_set,
> -};
> -
>  static int
>  mlxsw_sp_bridge_8021q_port_join(struct mlxsw_sp_bridge_device *bridge_device,
>  				struct mlxsw_sp_bridge_port *bridge_port,
> @@ -3545,11 +3541,3 @@ void mlxsw_sp_switchdev_fini(struct mlxsw_sp *mlxsw_sp)
>  	kfree(mlxsw_sp->bridge);
>  }
>  
> -void mlxsw_sp_port_switchdev_init(struct mlxsw_sp_port *mlxsw_sp_port)
> -{
> -	mlxsw_sp_port->dev->switchdev_ops = &mlxsw_sp_port_switchdev_ops;
> -}
> -
> -void mlxsw_sp_port_switchdev_fini(struct mlxsw_sp_port *mlxsw_sp_port)
> -{
> -}

^ permalink raw reply

* Re: [PATCH net-next 6/8] devlink: introduce port's peer netdevs
From: Jiri Pirko @ 2019-02-27 13:08 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: davem, oss-drivers, netdev
In-Reply-To: <20190226182436.23811-7-jakub.kicinski@netronome.com>

Tue, Feb 26, 2019 at 07:24:34PM CET, jakub.kicinski@netronome.com wrote:
>Devlink ports represent ports of a switch device (or SR-IOV
>NIC which has an embedded switch). In case of SR-IOV when
>PCIe PFs are exposed the PFs which are directly connected
>to the local machine may also spawn PF netdev (much like
>VFs have a port/"repr" and an actual VF netdev).
>
>Allow devlink to expose such linking. There is currently no
>way to find out which netdev corresponds to which PF.
>
>Example:
>
>$ devlink port
>pci/0000:82:00.0/0: type eth netdev p4p1 flavour physical
>pci/0000:82:00.0/10000: type eth netdev eth1 flavour pci_pf pf 0 peer_netdev enp130s0
>pci/0000:82:00.0/10001: type eth netdev eth0 flavour pci_vf pf 0 vf 0
>pci/0000:82:00.0/10002: type eth netdev eth2 flavour pci_vf pf 0 vf 1

Peer as the other side of a "virtual cable". For PF, that is probably
sufficient. But I think what a "peer of devlink port" should be "a
devlink port".

Not sure about VF.

Consider a simple problem of setting up a VF mac address. In legacy, you
do it like this:
$ ip link set eth2 vf 1 mac 00:52:44:11:22:33
However, in new model, you so far cannot do that.

What I was thinking about was some "dummy peer" which would be on the
host. Not sure if only as a "dummy peer devlink port" or even as some
sort of "dummy netdev".

One way or another, it would provide the user some info about which VF
representor is connected to which VF in VM (mac mapping).

^ permalink raw reply

* Re: [PATCH net] sctp: get sctphdr by offset in sctp_compute_cksum
From: Xin Long @ 2019-02-27 12:53 UTC (permalink / raw)
  To: Neil Horman; +Cc: LKML, network dev, linux-sctp, davem, Marcelo Ricardo Leitner
In-Reply-To: <20190226122902.GA13064@hmswarspite.think-freely.org>

On Tue, Feb 26, 2019 at 8:29 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Tue, Feb 26, 2019 at 12:15:54AM +0800, Xin Long wrote:
> > On Mon, Feb 25, 2019 at 10:08 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > >
> > > On Mon, Feb 25, 2019 at 09:20:44PM +0800, Xin Long wrote:
> > > > On Mon, Feb 25, 2019 at 8:47 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > >
> > > > > On Mon, Feb 25, 2019 at 07:25:37PM +0800, Xin Long wrote:
> > > > > > sctp_hdr(skb) only works when skb->transport_header is set properly.
> > > > > >
> > > > > > But in the path of nf_conntrack_in: sctp_packet() -> sctp_error()
> > > > > >
> > > > > > skb->transport_header is not guaranteed to be right value for sctp.
> > > > > > It will cause to fail to check the checksum for sctp packets.
> > > > > >
> > > > > > So fix it by using offset, which is always right in all places.
> > > > > >
> > > > > > Fixes: e6d8b64b34aa ("net: sctp: fix and consolidate SCTP checksumming code")
> > > > > > Reported-by: Li Shuang <shuali@redhat.com>
> > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > > ---
> > > > > >  include/net/sctp/checksum.h | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/include/net/sctp/checksum.h b/include/net/sctp/checksum.h
> > > > > > index 32ee65a..1c6e6c0 100644
> > > > > > --- a/include/net/sctp/checksum.h
> > > > > > +++ b/include/net/sctp/checksum.h
> > > > > > @@ -61,7 +61,7 @@ static inline __wsum sctp_csum_combine(__wsum csum, __wsum csum2,
> > > > > >  static inline __le32 sctp_compute_cksum(const struct sk_buff *skb,
> > > > > >                                       unsigned int offset)
> > > > > >  {
> > > > > > -     struct sctphdr *sh = sctp_hdr(skb);
> > > > > > +     struct sctphdr *sh = (struct sctphdr *)(skb->data + offset);
> > > > > >       const struct skb_checksum_ops ops = {
> > > > > >               .update  = sctp_csum_update,
> > > > > >               .combine = sctp_csum_combine,
> > > > > > --
> > > > > > 2.1.0
> > > > > >
> > > > > >
> > > > > Shouldn't you use skb_set_transport_header and skb_transport_header here?
> > > > you mean:
> > > > skb_set_transport_header(skb, offset);
> > > > sh = sctp_hdr(skb);
> > > > ?
> > > >
> > > > There's no place counting on here to set transport_header.
> > > > It will be a kinda redundant job, yet skb is 'const'.
> > > >
> > > I'm not sure what you mean by "theres no place counting here".  We have the
> > > transport header offset, and you're doing the exact same computation that that
> > > function does.  It seems like we should use it in case the underlying
> > > implementation changes.
> > 1. skb_set_transport_header() and sctp_hdr() are like:
> > skb->transport_header = skb->data - skb->head;
> > skb->transport_header += offset
> > sh = skb->head + skb->transport_header;
> >
> > 2. in this patch:
> > sh = (struct sctphdr *)(skb->data + offset);  only
> >
> > I think the 2nd one is better.
> >
> > I feel it's weird to set transport_header here if it's only for
> > sctp_hdr(skb) in here.
> >
> > As for "underlying implementation changes", I don't know exactly the case
> > but there are quite a few places doing things like:
> > *hdr = (struct *hdr *)(skb->data + hdroff);
> >
> > I'd think it's safe. no?
> >
> Safe, yes, it just doesn't seem right.  I know you've pointed out several places
> below that rapidly compute transport offsets in a one-off fashion, but at this
> same time, the other primary transports (tcp and udp), all seems to use the
> transport header to do their work (linearizing as necessecary, which sctp also
> does in sctp_rcv, at least in most cases).
> > >
> > > I understand what you are saying regarding the use of a const variable there,
> > > but perhaps thats an argument for removing the const storage classifier.  Better
> > > still, it would be good to figure out why all paths to this function don't
> > > already set the transport header offset to begin with (addressing your redundant
> > > comment)
> > The issue was reported when going to nf_conntrack by br_netfilter's
> > bridge-nf-call-iptables.
> > As you can see on nf_conntrack_in() path, even iphdr is got by:
> >    iph = skb_header_pointer(skb, nhoff, sizeof(_iph), &_iph);
> > It's impossible to set skb->transport_header when we're not sure iphdr
> > in linearized memory.
> >
> But if the skb isn't linearized, computing the transport header manually isn't
> going to help you anyway.  You can see that in skb_header_pointer.  If the
> offset they are trying to get to is outside the bounds of the length of the skb
> (i.e. the fragmented case), it calls skb_copy_bits to linearize the needed
> segment.  It seems we should be doing something simmilar.  In most cases we are
> already linearized from sctp_rcv (possibly all, I need to think about that). All
> I'm really saying is that by using the skb apis we insulate ourselves from
> potential changes in how skbs might work in the future.  I'm not strictly bound
> to setting the transport header, but we should definately be getting the
> transport header via the skb utility functions wherever possible.
Okay, I will change to fix it with the below patch if you agree.
I've confirmed this won't affect netfilter.

diff --git a/net/netfilter/nf_conntrack_proto_sctp.c
b/net/netfilter/nf_conntrack_proto_sctp.c
index d53e3e7..6b53cd2 100644
--- a/net/netfilter/nf_conntrack_proto_sctp.c
+++ b/net/netfilter/nf_conntrack_proto_sctp.c
@@ -343,7 +343,9 @@ static bool sctp_error(struct sk_buff *skb,
                        logmsg = "nf_ct_sctp: failed to read header ";
                        goto out_invalid;
                }
-               sh = (const struct sctphdr *)(skb->data + dataoff);
+               /* sctp_compute_cksum() depends on correct transport header */
+               skb_set_transport_header(skb, dataoff);
+               sh = sctp_hdr(skb);

^ permalink raw reply related

* [PATCH][net-next] ethtool: Use explicit designated initializers for .cmd
From: Li RongQing @ 2019-02-27 12:47 UTC (permalink / raw)
  To: netdev

Initialize the .cmd member by using a designated struct
initializer. This fixes warning of missing field initializers,
and makes code a little easier to read.

Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 net/core/ethtool.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 47558ffae423..d4918ffddda8 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1717,7 +1717,7 @@ static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
 
 static int ethtool_get_pauseparam(struct net_device *dev, void __user *useraddr)
 {
-	struct ethtool_pauseparam pauseparam = { ETHTOOL_GPAUSEPARAM };
+	struct ethtool_pauseparam pauseparam = { .cmd = ETHTOOL_GPAUSEPARAM };
 
 	if (!dev->ethtool_ops->get_pauseparam)
 		return -EOPNOTSUPP;
@@ -2503,7 +2503,7 @@ static int set_phy_tunable(struct net_device *dev, void __user *useraddr)
 
 static int ethtool_get_fecparam(struct net_device *dev, void __user *useraddr)
 {
-	struct ethtool_fecparam fecparam = { ETHTOOL_GFECPARAM };
+	struct ethtool_fecparam fecparam = { .cmd = ETHTOOL_GFECPARAM };
 	int rc;
 
 	if (!dev->ethtool_ops->get_fecparam)
-- 
2.16.2


^ permalink raw reply related

* [PATCH net v1] net/mlx5e: Correctly use the namespace type when allocating pedit action
From: xiangxia.m.yue @ 2019-02-26 12:28 UTC (permalink / raw)
  To: saeedm, gerlitz.or, roid; +Cc: netdev, Tonghao Zhang, Pablo Neira Ayuso

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

The capacity of FDB offloading and NIC offloading table are
different, and when allocating the pedit actions, we should
use the correct namespace type.

Fixes: c500c86b0c75d ("net/mlx5e: support for two independent packet edit actions")
Cc: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 3a02b22..467ef9e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -2635,7 +2635,7 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
 
 	if (hdrs[TCA_PEDIT_KEY_EX_CMD_SET].pedits ||
 	    hdrs[TCA_PEDIT_KEY_EX_CMD_ADD].pedits) {
-		err = alloc_tc_pedit_action(priv, MLX5_FLOW_NAMESPACE_KERNEL,
+		err = alloc_tc_pedit_action(priv, MLX5_FLOW_NAMESPACE_FDB,
 					    parse_attr, hdrs, extack);
 		if (err)
 			return err;
-- 
1.8.3.1


^ permalink raw reply related

* [PATCH net-next v3 3/4] net/mlx5e: Deletes unnecessary setting of esw_attr->parse_attr
From: xiangxia.m.yue @ 2019-02-26 12:27 UTC (permalink / raw)
  To: saeedm, gerlitz.or, roid; +Cc: netdev, Tonghao Zhang
In-Reply-To: <1551184063-40628-1-git-send-email-xiangxia.m.yue@gmail.com>

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

This patch deletes unnecessary setting of the esw_attr->parse_attr
to parse_attr in parse_tc_fdb_actions() because it is already done
by the mlx5e_flow_esw_attr_init() function.

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 336a111..56ac50d 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -2558,7 +2558,6 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
 					out_dev->ifindex;
 				parse_attr->tun_info[attr->out_count] = *info;
 				encap = false;
-				attr->parse_attr = parse_attr;
 				attr->dests[attr->out_count].flags |=
 					MLX5_ESW_DEST_ENCAP;
 				attr->out_count++;
-- 
1.8.3.1


^ permalink raw reply related

* [PATCH net-next v3 4/4] net/mlx5e: Return -EOPNOTSUPP when attempting to offload an unsupported action
From: xiangxia.m.yue @ 2019-02-26 12:27 UTC (permalink / raw)
  To: saeedm, gerlitz.or, roid; +Cc: netdev, Tonghao Zhang
In-Reply-To: <1551184063-40628-1-git-send-email-xiangxia.m.yue@gmail.com>

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

* Now the encapsulation is not supported for mlx5 VFs. When we try to
offload that action, the -EINVAL is returned, but not -EOPNOTSUPP.
This patch changes the returned value and ignore to confuse user.
The command is shown as below [1].

* When max modify header action is zero, we return -EOPNOTSUPP
directly. In this way, we can ignore wrong message info (e.g.
"mlx5: parsed 0 pedit actions, can't do more"). This happens when
offloading pedit actions on mlx(cx4) VFs. The command is shown as below [2].

For example: (p2p1_0 is VF net device)
[1]
$ tc filter add dev p2p1_0 protocol ip  parent ffff: prio 1 flower skip_sw \
    src_mac e4:11:22:33:44:01    \
    action tunnel_key set        \
    src_ip 1.1.1.100        \
    dst_ip 1.1.1.200        \
    dst_port 4789 id 100        \
    action mirred egress redirect dev vxlan0

[2]
$ tc filter add dev p2p1_0 parent ffff: protocol ip prio 1 \
    flower skip_sw dst_mac 00:10:56:fb:64:e8 \
    dst_ip 1.1.1.100 src_ip 1.1.1.200 \
    action pedit ex munge eth src set 00:10:56:b4:5d:20

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 56ac50d..3a02b22 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -2035,7 +2035,7 @@ static int parse_tc_pedit_action(struct mlx5e_priv *priv,
 				 struct netlink_ext_ack *extack)
 {
 	u8 cmd = (act->id == FLOW_ACTION_MANGLE) ? 0 : 1;
-	int err = -EOPNOTSUPP;
+	int max_actions, err = -EOPNOTSUPP;
 	u32 mask, val, offset;
 	u8 htype;
 
@@ -2047,6 +2047,17 @@ static int parse_tc_pedit_action(struct mlx5e_priv *priv,
 		goto out_err;
 	}
 
+        if (namespace == MLX5_FLOW_NAMESPACE_FDB) /* FDB offloading */
+                max_actions = MLX5_CAP_ESW_FLOWTABLE_FDB(priv->mdev, max_modify_header_actions);
+        else /* namespace is MLX5_FLOW_NAMESPACE_KERNEL - NIC offloading */
+                max_actions = MLX5_CAP_FLOWTABLE_NIC_RX(priv->mdev, max_modify_header_actions);
+
+	if (!max_actions) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "don't support pedit actions, can't offload");
+		goto out_err;
+	}
+
 	mask = act->mangle.mask;
 	val = act->mangle.val;
 	offset = act->mangle.offset;
@@ -2294,7 +2305,8 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
 			}
 			break;
 		default:
-			return -EINVAL;
+			NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
+			return -EOPNOTSUPP;
 		}
 	}
 
@@ -2616,7 +2628,8 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
 			break;
 			}
 		default:
-			return -EINVAL;
+			NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
+			return -EOPNOTSUPP;
 		}
 	}
 
-- 
1.8.3.1


^ permalink raw reply related

* [PATCH net-next v3 2/4] net/mlx5e: Remove 'parse_attr' argument in parse_tc_fdb_actions()
From: xiangxia.m.yue @ 2019-02-26 12:27 UTC (permalink / raw)
  To: saeedm, gerlitz.or, roid; +Cc: netdev, Tonghao Zhang
In-Reply-To: <1551184063-40628-1-git-send-email-xiangxia.m.yue@gmail.com>

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

This patch is a little improvement. Simplify the parse_tc_fdb_actions().

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index b38986e..336a111 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -2467,13 +2467,13 @@ static int parse_tc_vlan_action(struct mlx5e_priv *priv,
 
 static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
 				struct flow_action *flow_action,
-				struct mlx5e_tc_flow_parse_attr *parse_attr,
 				struct mlx5e_tc_flow *flow,
 				struct netlink_ext_ack *extack)
 {
 	struct pedit_headers_action hdrs[2] = {};
 	struct mlx5_eswitch *esw = priv->mdev->priv.eswitch;
 	struct mlx5_esw_flow_attr *attr = flow->esw_attr;
+	struct mlx5e_tc_flow_parse_attr *parse_attr = attr->parse_attr;
 	struct mlx5e_rep_priv *rpriv = priv->ppriv;
 	const struct ip_tunnel_info *info = NULL;
 	const struct flow_action_entry *act;
@@ -2788,7 +2788,7 @@ static bool is_peer_flow_needed(struct mlx5e_tc_flow *flow)
 	if (err)
 		goto err_free;
 
-	err = parse_tc_fdb_actions(priv, &rule->action, parse_attr, flow, extack);
+	err = parse_tc_fdb_actions(priv, &rule->action, flow, extack);
 	if (err)
 		goto err_free;
 
-- 
1.8.3.1


^ permalink raw reply related

* [PATCH net-next v3 1/4] net/mlx5e: Make the log friendly when decapsulation offload not supported
From: xiangxia.m.yue @ 2019-02-26 12:27 UTC (permalink / raw)
  To: saeedm, gerlitz.or, roid; +Cc: netdev, Tonghao Zhang
In-Reply-To: <1551184063-40628-1-git-send-email-xiangxia.m.yue@gmail.com>

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

If we try to offload decapsulation actions to VFs hw, we get the log [1].
It's not friendly, because the kind of net device is null, and we don't
know what '0' means.

[1] "mlx5_core 0000:05:01.2 vf_0: decapsulation offload is not supported for  net device (0)"

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
index bdcc5e7..6cbfbfa 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
@@ -84,7 +84,7 @@ static const char *mlx5e_netdev_kind(struct net_device *dev)
 	if (dev->rtnl_link_ops)
 		return dev->rtnl_link_ops->kind;
 	else
-		return "";
+		return "unknown";
 }
 
 static int mlx5e_route_lookup_ipv6(struct mlx5e_priv *priv,
@@ -620,8 +620,10 @@ int mlx5e_tc_tun_parse(struct net_device *filter_dev,
 						headers_c, headers_v);
 	} else {
 		netdev_warn(priv->netdev,
-			    "decapsulation offload is not supported for %s net device (%d)\n",
-			    mlx5e_netdev_kind(filter_dev), tunnel_type);
+			    "decapsulation offload is not supported for %s (kind: \"%s\")\n",
+			    netdev_name(filter_dev),
+			    mlx5e_netdev_kind(filter_dev));
+
 		return -EOPNOTSUPP;
 	}
 	return err;
-- 
1.8.3.1


^ permalink raw reply related

* [PATCH net-next v3 0/4] net/mlx5e: Make little improvement for mlx5e
From: xiangxia.m.yue @ 2019-02-26 12:27 UTC (permalink / raw)
  To: saeedm, gerlitz.or, roid; +Cc: netdev, Tonghao Zhang

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

This serial patches are not bugfixes, and just little improvement for mlx5e.
v2->v3:
Patch 1 and 5 are merged to patch 4 now.

v1->v2
Patch 1: remove the duplicated error messages and stick to extack only
Patch 2: use the 'unknown' instead of empty string
Patch 5: is new patch

Tonghao Zhang (4):
  net/mlx5e: Make the log friendly when decapsulation offload not
    supported
  net/mlx5e: Remove 'parse_attr' argument in parse_tc_fdb_actions()
  net/mlx5e: Deletes unnecessary setting of esw_attr->parse_attr
  net/mlx5e: Return -EOPNOTSUPP when attempting to offload an
    unsupported action

 .../net/ethernet/mellanox/mlx5/core/en/tc_tun.c    |  8 +++++---
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c    | 24 ++++++++++++++++------
 2 files changed, 23 insertions(+), 9 deletions(-)

-- 
1.8.3.1


^ permalink raw reply


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