* Re: [PATCH 06/12 net-next,v7] drivers: net: use flow action infrastructure
From: Jiri Pirko @ 2019-02-02 15:44 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: netdev, davem, thomas.lendacky, f.fainelli, ariel.elior,
michael.chan, santosh, madalin.bucur, yisen.zhuang, salil.mehta,
jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch, jakub.kicinski,
peppe.cavallaro, grygorii.strashko, andrew, vivien.didelot,
alexandre.torgue, joabreu, linux-net-drivers, ganeshgr, ogerlitz,
Manish.Chopra, marcelo.leitner, mkubecek, venkatkumar.duvvuru,
julia.lawall, john.fastabend, netfilter-devel, cphealy
In-Reply-To: <20190202115054.4880-7-pablo@netfilter.org>
Sat, Feb 02, 2019 at 12:50:48PM CET, pablo@netfilter.org wrote:
>This patch updates drivers to use the new flow action infrastructure.
>
>Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>---
>v7: rebase on top of net-next. Dropping previous Acked-by tags since
> this one is slightly large, and it would be good another look after
> this rebase.
Looks fine.
Acked-by: Jiri Pirko <jiri@mellanox.com>
^ permalink raw reply
* Re: [RFC net-next 01/13] switchdev: Add SWITCHDEV_PORT_ATTR_SET, SWITCHDEV_PORT_ATTR_GET
From: Jiri Pirko @ 2019-02-02 15:48 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, Michael Chan, David S. Miller, Derek Chickles,
Satanand Burla, Felix Manlunas, Saeed Mahameed, Leon Romanovsky,
Jiri Pirko, Ido Schimmel, Alexandre Belloni,
Microchip Linux Driver Support, Jakub Kicinski, Ioana Radulescu,
Ioana Ciornei, Greg Kroah-Hartman, Ivan Vecera, Andrew Lunn,
Vivien Didelot, Dirk van der Merwe, Francois H. Theron,
Simon Horman, Quentin Monnet, Daniel Borkmann, Eric Dumazet,
John Hurley, Edwin Peer, open list,
open list:MELLANOX MLX5 core VPI driver,
open list:NETRONOME ETHERNET DRIVERS, open list:STAGING SUBSYSTEM
In-Reply-To: <20190201220657.30170-2-f.fainelli@gmail.com>
Fri, Feb 01, 2019 at 11:06:45PM CET, f.fainelli@gmail.com wrote:
>In preparation for allowing switchdev enabled drivers to veto specific
>attribute settings from within the context of the caller, introduce a
>new switchdev notifier type for port attributes.
>
>Suggested-by: Ido Schimmel <idosch@mellanox.com>
>Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>---
> include/net/switchdev.h | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
>diff --git a/include/net/switchdev.h b/include/net/switchdev.h
>index 63843ae5dc81..e62fb2693e00 100644
>--- a/include/net/switchdev.h
>+++ b/include/net/switchdev.h
>@@ -145,6 +145,9 @@ enum switchdev_notifier_type {
> SWITCHDEV_VXLAN_FDB_ADD_TO_DEVICE,
> SWITCHDEV_VXLAN_FDB_DEL_TO_DEVICE,
> SWITCHDEV_VXLAN_FDB_OFFLOADED,
>+
>+ SWITCHDEV_PORT_ATTR_SET, /* Blocking. */
>+ SWITCHDEV_PORT_ATTR_GET, /* Blocking. */
Not an UAPI, so you can put this right next to PORT_OBJ_* if needed.
> };
>
> struct switchdev_notifier_info {
>@@ -167,6 +170,13 @@ struct switchdev_notifier_port_obj_info {
> bool handled;
> };
>
>+struct switchdev_notifier_port_attr_info {
>+ struct switchdev_notifier_info info; /* must be first */
>+ struct switchdev_attr *attr;
>+ struct switchdev_trans *trans;
>+ bool handled;
>+};
>+
> static inline struct net_device *
> switchdev_notifier_info_to_dev(const struct switchdev_notifier_info *info)
> {
>--
>2.17.1
>
^ permalink raw reply
* Re: [RFC net-next 00/13] Get rid of switchdev_ops
From: Jiri Pirko @ 2019-02-02 15:52 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, Michael Chan, David S. Miller, Derek Chickles,
Satanand Burla, Felix Manlunas, Saeed Mahameed, Leon Romanovsky,
Jiri Pirko, Ido Schimmel, Alexandre Belloni,
Microchip Linux Driver Support, Jakub Kicinski, Ioana Radulescu,
Ioana Ciornei, Greg Kroah-Hartman, Ivan Vecera, Andrew Lunn,
Vivien Didelot, Dirk van der Merwe, Francois H. Theron,
Simon Horman, Quentin Monnet, Daniel Borkmann, Eric Dumazet,
John Hurley, Edwin Peer, open list,
open list:MELLANOX MLX5 core VPI driver,
open list:NETRONOME ETHERNET DRIVERS, open list:STAGING SUBSYSTEM
In-Reply-To: <20190201220657.30170-1-f.fainelli@gmail.com>
Fri, Feb 01, 2019 at 11:06:44PM CET, f.fainelli@gmail.com wrote:
>Hi all,
>
>This patch series converts SWITCHDEV_PORT_ATTR_{GET,SET} to use a
>blocking notifier, similar to how SWITCHDEV_PORT_OBJ_{ADD,DEL} has been
>changed recently by Petr.
>
>This was suggested by Ido to help with a particular use case I have
>where I want to be able to veto a switchdev bridge attribute from a
>driver (multicast_snooping).
>
>Please review since I may not have gotten the driver abstraction right,
>especially for mlx5e and nfp since these are *hum* *hum* large drivers.
Looks fine. Thanks!
^ permalink raw reply
* Re: [PATCH iproute2-next] devlink: add info subcommand
From: Jiri Pirko @ 2019-02-02 15:59 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: dsahern, stephen, netdev, oss-drivers
In-Reply-To: <20190202000338.30820-1-jakub.kicinski@netronome.com>
Sat, Feb 02, 2019 at 01:03:38AM CET, jakub.kicinski@netronome.com wrote:
>Add support for reading the device serial number and versions
>from the kernel.
>
>RFCv2:
> - make info subcommand of dev.
Please add some examples of inputs and outputs.
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>---
> devlink/devlink.c | 207 +++++++++++++++++++++++++++++++++++++++++
> man/man8/devlink-dev.8 | 36 +++++++
> 2 files changed, 243 insertions(+)
>
>diff --git a/devlink/devlink.c b/devlink/devlink.c
>index 3651e90c1159..3ab046ace8f8 100644
>--- a/devlink/devlink.c
>+++ b/devlink/devlink.c
>@@ -199,6 +199,7 @@ 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_VERSIONS_TYPE BIT(25)
Why "type"? Confusing.
>
> struct dl_opts {
> uint32_t present; /* flags of present items */
>@@ -230,6 +231,7 @@ struct dl_opts {
> uint32_t region_snapshot_id;
> uint64_t region_address;
> uint64_t region_length;
>+ int versions_attr;
> };
>
> struct dl {
>@@ -383,6 +385,13 @@ static const enum mnl_attr_data_type devlink_policy[DEVLINK_ATTR_MAX + 1] = {
> [DEVLINK_ATTR_REGION_CHUNK_DATA] = MNL_TYPE_BINARY,
> [DEVLINK_ATTR_REGION_CHUNK_ADDR] = MNL_TYPE_U64,
> [DEVLINK_ATTR_REGION_CHUNK_LEN] = MNL_TYPE_U64,
>+ [DEVLINK_ATTR_INFO_DRIVER_NAME] = MNL_TYPE_STRING,
>+ [DEVLINK_ATTR_INFO_SERIAL_NUMBER] = MNL_TYPE_STRING,
>+ [DEVLINK_ATTR_INFO_VERSION_FIXED] = MNL_TYPE_NESTED,
>+ [DEVLINK_ATTR_INFO_VERSION_RUNNING] = MNL_TYPE_NESTED,
>+ [DEVLINK_ATTR_INFO_VERSION_STORED] = MNL_TYPE_NESTED,
>+ [DEVLINK_ATTR_INFO_VERSION_NAME] = MNL_TYPE_STRING,
>+ [DEVLINK_ATTR_INFO_VERSION_VALUE] = MNL_TYPE_STRING,
> };
>
> static int attr_cb(const struct nlattr *attr, void *data)
>@@ -943,6 +952,21 @@ static int param_cmode_get(const char *cmodestr,
> return 0;
> }
>
>+static int versions_type_get(const char *typestr, int *p_attr)
>+{
>+ if (strcmp(typestr, "fixed") == 0) {
>+ *p_attr = DEVLINK_ATTR_INFO_VERSION_FIXED;
>+ } else if (strcmp(typestr, "stored") == 0) {
>+ *p_attr = DEVLINK_ATTR_INFO_VERSION_STORED;
>+ } else if (strcmp(typestr, "running") == 0) {
>+ *p_attr = DEVLINK_ATTR_INFO_VERSION_RUNNING;
>+ } else {
>+ pr_err("Unknown versions type \"%s\"\n", typestr);
>+ return -EINVAL;
>+ }
>+ return 0;
>+}
>+
> static int dl_argv_parse(struct dl *dl, uint32_t o_required,
> uint32_t o_optional)
> {
>@@ -1178,6 +1202,19 @@ 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, "versions") &&
>+ (o_all & DL_OPT_VERSIONS_TYPE)) {
>+ const char *versionstr;
>+
>+ dl_arg_inc(dl);
>+ err = dl_argv_str(dl, &versionstr);
>+ if (err)
>+ return err;
>+ err = versions_type_get(versionstr,
>+ &opts->versions_attr);
>+ if (err)
>+ return err;
>+ o_found |= DL_OPT_VERSIONS_TYPE;
> } else {
> pr_err("Unknown option \"%s\"\n", dl_argv(dl));
> return -EINVAL;
>@@ -1443,6 +1480,9 @@ static void cmd_dev_help(void)
> pr_err(" devlink dev param set DEV name PARAMETER value VALUE cmode { permanent | driverinit | runtime }\n");
> pr_err(" devlink dev param show [DEV name PARAMETER]\n");
> pr_err(" devlink dev reload DEV\n");
>+ pr_err(" devlink dev info [ DEV [ { versions [ VTYPE ] } ] ]\n");
>+ pr_err("\n");
>+ pr_err(" VTYPE := { fixed | running | stored }\n");
So you would like to filter according to the version type? Why?
> }
>
> static bool cmp_arr_last_handle(struct dl *dl, const char *bus_name,
>@@ -1775,6 +1815,30 @@ static void pr_out_array_end(struct dl *dl)
> }
> }
>
>+static void pr_out_object_start(struct dl *dl, const char *name)
>+{
>+ if (dl->json_output) {
>+ jsonw_name(dl->jw, name);
>+ jsonw_start_object(dl->jw);
>+ } else {
>+ __pr_out_indent_inc();
>+ __pr_out_newline();
>+ pr_out("%s:", name);
>+ __pr_out_indent_inc();
>+ __pr_out_newline();
>+ }
>+}
>+
>+static void pr_out_object_end(struct dl *dl)
>+{
>+ if (dl->json_output) {
>+ jsonw_end_object(dl->jw);
>+ } else {
>+ __pr_out_indent_dec();
>+ __pr_out_indent_dec();
>+ }
>+}
>+
> static void pr_out_entry_start(struct dl *dl)
> {
> if (dl->json_output)
>@@ -2415,6 +2479,146 @@ static int cmd_dev_reload(struct dl *dl)
> return _mnlg_socket_sndrcv(dl->nlg, nlh, NULL, NULL);
> }
>
>+static void pr_out_versions_single(struct dl *dl, const struct nlmsghdr *nlh,
>+ const char *name, int type)
>+{
>+ struct nlattr *version;
>+
>+ if (dl->opts.versions_attr && dl->opts.versions_attr != type)
>+ return;
>+
>+ mnl_attr_for_each(version, nlh, sizeof(struct genlmsghdr)) {
>+ struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
>+ const char *ver_value;
>+ const char *ver_name;
>+ int err;
>+
>+ if (mnl_attr_get_type(version) != type)
>+ continue;
>+
>+ err = mnl_attr_parse_nested(version, attr_cb, tb);
>+ if (err != MNL_CB_OK)
>+ continue;
>+
>+ if (!tb[DEVLINK_ATTR_INFO_VERSION_NAME] ||
>+ !tb[DEVLINK_ATTR_INFO_VERSION_VALUE])
>+ continue;
>+
>+ if (name) {
>+ pr_out_object_start(dl, name);
>+ name = NULL;
>+ }
>+
>+ ver_name = mnl_attr_get_str(tb[DEVLINK_ATTR_INFO_VERSION_NAME]);
>+ ver_value = mnl_attr_get_str(tb[DEVLINK_ATTR_INFO_VERSION_VALUE]);
>+
>+ pr_out_str(dl, ver_name, ver_value);
>+ if (!dl->json_output)
>+ __pr_out_newline();
>+ }
>+
>+ if (!name)
>+ pr_out_object_end(dl);
>+}
>+
>+static void pr_out_info(struct dl *dl, const struct nlmsghdr *nlh,
>+ struct nlattr **tb, bool has_versions)
>+{
>+ __pr_out_handle_start(dl, tb, true, false);
>+
>+ __pr_out_indent_inc();
>+ if (!dl->opts.versions_attr && tb[DEVLINK_ATTR_INFO_DRIVER_NAME]) {
>+ struct nlattr *nla_drv = tb[DEVLINK_ATTR_INFO_DRIVER_NAME];
>+
>+ __pr_out_newline();
>+ pr_out_str(dl, "driver", mnl_attr_get_str(nla_drv));
>+ }
>+
>+ if (!dl->opts.versions_attr && tb[DEVLINK_ATTR_INFO_SERIAL_NUMBER]) {
>+ struct nlattr *nla_sn = tb[DEVLINK_ATTR_INFO_SERIAL_NUMBER];
>+
>+ __pr_out_newline();
>+ pr_out_str(dl, "serial_number", mnl_attr_get_str(nla_sn));
>+ }
>+ __pr_out_indent_dec();
>+
>+ if (has_versions) {
>+ pr_out_object_start(dl, "versions");
>+
>+ pr_out_versions_single(dl, nlh, "fixed",
>+ DEVLINK_ATTR_INFO_VERSION_FIXED);
>+ pr_out_versions_single(dl, nlh, "running",
>+ DEVLINK_ATTR_INFO_VERSION_RUNNING);
>+ pr_out_versions_single(dl, nlh, "stored",
>+ DEVLINK_ATTR_INFO_VERSION_STORED);
>+
>+ pr_out_object_end(dl);
>+ }
>+
>+ pr_out_handle_end(dl);
>+}
>+
>+static int cmd_versions_show_cb(const struct nlmsghdr *nlh, void *data)
>+{
>+ struct genlmsghdr *genl = mnl_nlmsg_get_payload(nlh);
>+ struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
>+ bool has_versions, has_info;
>+ struct dl *dl = data;
>+
>+ mnl_attr_parse(nlh, sizeof(*genl), attr_cb, tb);
>+
>+ if (!tb[DEVLINK_ATTR_BUS_NAME] || !tb[DEVLINK_ATTR_DEV_NAME])
>+ return MNL_CB_ERROR;
>+
>+ has_versions = tb[DEVLINK_ATTR_INFO_VERSION_FIXED] ||
>+ tb[DEVLINK_ATTR_INFO_VERSION_RUNNING] ||
>+ tb[DEVLINK_ATTR_INFO_VERSION_STORED];
>+ has_info = tb[DEVLINK_ATTR_INFO_DRIVER_NAME] ||
>+ tb[DEVLINK_ATTR_INFO_SERIAL_NUMBER] ||
>+ has_versions;
>+
>+ if (has_info)
>+ pr_out_info(dl, nlh, tb, has_versions);
>+
>+ return MNL_CB_OK;
>+}
>+
>+static void cmd_dev_info_help(void)
>+{
>+ pr_err("Usage: devlink dev info [ DEV [ { versions [ VTYPE ] } ] ]\n");
>+ pr_err("\n");
>+ pr_err(" VTYPE := { fixed | running | stored }\n");
>+}
>+
>+static int cmd_dev_info(struct dl *dl)
>+{
>+ struct nlmsghdr *nlh;
>+ uint16_t flags = NLM_F_REQUEST | NLM_F_ACK;
>+ int err;
>+
>+ if (dl_argv_match(dl, "help")) {
>+ cmd_dev_info_help();
>+ return 0;
>+ }
>+
>+ if (dl_argc(dl) == 0)
>+ flags |= NLM_F_DUMP;
>+
>+ nlh = mnlg_msg_prepare(dl->nlg, DEVLINK_CMD_INFO_GET, flags);
>+
>+ if (dl_argc(dl) > 0) {
>+ err = dl_argv_parse_put(nlh, dl, DL_OPT_HANDLE,
>+ DL_OPT_VERSIONS_TYPE);
>+ if (err)
>+ return err;
>+ }
>+
>+ pr_out_section_start(dl, "info");
>+ err = _mnlg_socket_sndrcv(dl->nlg, nlh, cmd_versions_show_cb, dl);
>+ pr_out_section_end(dl);
>+ return err;
>+}
>+
> static int cmd_dev(struct dl *dl)
> {
> if (dl_argv_match(dl, "help")) {
>@@ -2433,6 +2637,9 @@ static int cmd_dev(struct dl *dl)
> } else if (dl_argv_match(dl, "param")) {
> dl_arg_inc(dl);
> return cmd_dev_param(dl);
>+ } else if (dl_argv_match(dl, "info")) {
>+ dl_arg_inc(dl);
>+ return cmd_dev_info(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 d985da172aa0..5b05298f88bf 100644
>--- a/man/man8/devlink-dev.8
>+++ b/man/man8/devlink-dev.8
>@@ -63,6 +63,17 @@ devlink-dev \- devlink device configuration
> .BR "devlink dev reload"
> .IR DEV
>
>+.ti -8
>+.BR "devlink dev info"
>+.RI "[ " DEV
>+.RI "["
>+.BR versions
>+.RI "{ "
>+.BR fixed " | " running " | " stored
>+.RI "} "
>+.RI "]"
>+.RI "]"
>+
> .SH "DESCRIPTION"
> .SS devlink dev show - display devlink device attributes
>
>@@ -151,6 +162,31 @@ If this argument is omitted all parameters supported by devlink devices are list
> .I "DEV"
> - Specifies the devlink device to reload.
>
>+.SS devlink dev info - display device information.
>+Display device information provided by the driver. This command can be used
>+to query versions of the hardware components or device components which
>+can't be updated (
>+.I fixed
>+) as well as device firmware which can be updated. For firmware components
>+.I running
>+displays the versions of firmware currently loaded into the device, while
>+.I stored
>+reports the versions in device's flash.
>+.I Running
>+and
>+.I stored
>+versions may differ after flash has been updated, but before reboot.
>+
>+.PP
>+.I "DEV"
>+- specifies the devlink device to show.
>+If this argument is omitted all devices are listed.
>+
>+.PP
>+.BR versions " { " fixed " | " running " | " stored " } "
>+- specifies the versions category to show.
>+If this argument is omitted all categories are listed.
>+
> .SH "EXAMPLES"
> .PP
> devlink dev show
>--
>2.19.2
>
^ permalink raw reply
* Re: [PATCH net-next 00/12] code optimizations & bugfixes for HNS3 driver
From: David Miller @ 2019-02-02 16:48 UTC (permalink / raw)
To: tanhuazhong
Cc: netdev, linux-kernel, huangdaode, yisen.zhuang, salil.mehta,
linuxarm
In-Reply-To: <20190202143937.8924-1-tanhuazhong@huawei.com>
From: Huazhong Tan <tanhuazhong@huawei.com>
Date: Sat, 2 Feb 2019 22:39:25 +0800
> This patchset includes bugfixes and code optimizations for the HNS3
> ethernet controller driver
Series applied.
^ permalink raw reply
* Re: [PATCH bpf-next v4 0/3] tools/bpf: changes of libbpf debug interfaces
From: Alexei Starovoitov @ 2019-02-02 16:52 UTC (permalink / raw)
To: Yonghong Song
Cc: Arnaldo Carvalho de Melo, Magnus Karlsson, netdev,
Alexei Starovoitov, Daniel Borkmann, kernel-team
In-Reply-To: <20190202001413.3178000-1-yhs@fb.com>
On Fri, Feb 01, 2019 at 04:14:13PM -0800, Yonghong Song wrote:
> These are patches responding to my comments for
> Magnus's patch (https://patchwork.ozlabs.org/patch/1032848/).
> The goal is to make pr_* macros available to other C files
> than libbpf.c, and to simplify API function libbpf_set_print().
>
> Specifically, Patch #1 used global functions
> to facilitate pr_* macros in the header files so they
> are available in different C files.
> Patch #2 removes the global function libbpf_print_level_available()
> which is added in Patch 1.
> Patch #3 simplified libbpf_set_print() which takes only one print
> function with a debug level argument among others.
>
> Changelogs:
> v3 -> v4:
> . rename libbpf internal header util.h to libbpf_util.h
> . rename libbpf internal function libbpf_debug_print() to libbpf_print()
> v2 -> v3:
> . bailed out earlier in libbpf_debug_print() if __libbpf_pr is NULL
> . added missing LIBBPF_DEBUG level check in libbpf.c __base_pr().
> v1 -> v2:
> . Renamed global function libbpf_dprint() to libbpf_debug_print()
> to be more expressive.
> . Removed libbpf_dprint_level_available() as it is used only
> once in btf.c and we can remove it by optimizing for common cases.
>
> Yonghong Song (3):
> tools/bpf: move libbpf pr_* debug print functions to headers
> tools/bpf: print out btf log at LIBBPF_WARN level
> tools/bpf: simplify libbpf API function libbpf_set_print()
>
> tools/lib/bpf/btf.c | 110 +++++++++---------
> tools/lib/bpf/btf.h | 7 +-
> tools/lib/bpf/libbpf.c | 47 ++++----
> tools/lib/bpf/libbpf.h | 20 ++--
> tools/lib/bpf/libbpf_util.h | 30 +++++
> tools/lib/bpf/test_libbpf.cpp | 4 +-
> tools/perf/util/bpf-loader.c | 32 ++---
Overall looks good to me.
Arnaldo, could you ack the set, so we can take it into bpf-next?
^ permalink raw reply
* Re: [PATCH] net: dsa: slave: Don't propagate flag changes on down slave interfaces
From: Florian Fainelli @ 2019-02-02 17:05 UTC (permalink / raw)
To: Rundong Ge, andrew; +Cc: vivien.didelot, davem, netdev
In-Reply-To: <20190202142935.3090-1-rdong.ge@gmail.com>
Le 2/2/19 à 6:29 AM, Rundong Ge a écrit :
> The unbalance of master's promiscuity or allmulti will happen after ifdown
> and ifup a slave interface which is in a bridge.
>
> When we ifdown a slave interface , both the 'dsa_slave_close' and
> 'dsa_slave_change_rx_flags' will clear the master's flags. The flags
> of master will be decrease twice.
> In the other hand, if we ifup the slave interface again, since the
> slave's flags were cleared the 'dsa_slave_open' won't set the master's
> flag, only 'dsa_slave_change_rx_flags' that triggered by 'br_add_if'
> will set the master's flags. The flags of master is increase once.
>
> Only propagating flag changes when a slave interface is up makes
> sure this does not happen. The 'vlan_dev_change_rx_flags' had the
> same problem and was fixed, and changes here follows that fix.
VLAN code under net/8021q/vlan_dev.c::vlan_dev_change_rx_flags() appears
to do the same thing that you are proposing, so this looks fine to me.
Since that is a bugfix, we should probably add:
Fixes: 91da11f870f0 ("net: Distributed Switch Architecture protocol
support")
?
>
> Signed-off-by: Rundong Ge <rdong.ge@gmail.com>
> ---
> net/dsa/slave.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index a3fcc1d..b5e4482 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -140,11 +140,14 @@ static int dsa_slave_close(struct net_device *dev)
> static void dsa_slave_change_rx_flags(struct net_device *dev, int change)
> {
> struct net_device *master = dsa_slave_to_master(dev);
> -
> - if (change & IFF_ALLMULTI)
> - dev_set_allmulti(master, dev->flags & IFF_ALLMULTI ? 1 : -1);
> - if (change & IFF_PROMISC)
> - dev_set_promiscuity(master, dev->flags & IFF_PROMISC ? 1 : -1);
> + if (dev->flags & IFF_UP) {
> + if (change & IFF_ALLMULTI)
> + dev_set_allmulti(master,
> + dev->flags & IFF_ALLMULTI ? 1 : -1);
> + if (change & IFF_PROMISC)
> + dev_set_promiscuity(master,
> + dev->flags & IFF_PROMISC ? 1 : -1);
> + }
> }
>
> static void dsa_slave_set_rx_mode(struct net_device *dev)
>
--
Florian
^ permalink raw reply
* Re: [PATCH] net: dsa: slave: Don't propagate flag changes on down slave interfaces
From: Andrew Lunn @ 2019-02-02 17:16 UTC (permalink / raw)
To: Florian Fainelli; +Cc: Rundong Ge, vivien.didelot, davem, netdev
In-Reply-To: <7f8fadc6-1bb5-03b5-4f5e-a407e9116399@gmail.com>
On Sat, Feb 02, 2019 at 09:05:11AM -0800, Florian Fainelli wrote:
> Le 2/2/19 à 6:29 AM, Rundong Ge a écrit :
> > The unbalance of master's promiscuity or allmulti will happen after ifdown
> > and ifup a slave interface which is in a bridge.
> >
> > When we ifdown a slave interface , both the 'dsa_slave_close' and
> > 'dsa_slave_change_rx_flags' will clear the master's flags. The flags
> > of master will be decrease twice.
> > In the other hand, if we ifup the slave interface again, since the
> > slave's flags were cleared the 'dsa_slave_open' won't set the master's
> > flag, only 'dsa_slave_change_rx_flags' that triggered by 'br_add_if'
> > will set the master's flags. The flags of master is increase once.
> >
> > Only propagating flag changes when a slave interface is up makes
> > sure this does not happen. The 'vlan_dev_change_rx_flags' had the
> > same problem and was fixed, and changes here follows that fix.
>
> VLAN code under net/8021q/vlan_dev.c::vlan_dev_change_rx_flags() appears
> to do the same thing that you are proposing, so this looks fine to me.
> Since that is a bugfix, we should probably add:
Hi Rundong, Florian
I've seen issues with tcpdump causing the promiscuous count to go
negative. I wounder if this will fix that at well?
Andrew
^ permalink raw reply
* Re: [PATCH net-next v5 12/12] sock: Add SO_RCVTIMEO_NEW and SO_SNDTIMEO_NEW
From: Oliver Hartkopp @ 2019-02-02 17:15 UTC (permalink / raw)
To: Deepa Dinamani, davem, linux-kernel
Cc: netdev, arnd, y2038, ccaulfie, deller, paulus, ralf, rth,
cluster-devel, linuxppc-dev, linux-alpha, linux-arch, linux-mips,
linux-parisc, sparclinux
In-Reply-To: <20190202153454.7121-13-deepa.kernel@gmail.com>
Hi all,
On 02.02.19 16:34, Deepa Dinamani wrote:
> Add new socket timeout options that are y2038 safe.
(..)
>
> diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h
> index 9826d1db71d0..0d0fddb7e738 100644
> --- a/arch/alpha/include/uapi/asm/socket.h
> +++ b/arch/alpha/include/uapi/asm/socket.h
> @@ -119,19 +119,25 @@
> #define SO_TIMESTAMPNS_NEW 64
> #define SO_TIMESTAMPING_NEW 65
>
> -#if !defined(__KERNEL__)
> +#define SO_RCVTIMEO_NEW 66
> +#define SO_SNDTIMEO_NEW 67
>
> -#define SO_RCVTIMEO SO_RCVTIMEO_OLD
> -#define SO_SNDTIMEO SO_SNDTIMEO_OLD
> +#if !defined(__KERNEL__)
>
> #if __BITS_PER_LONG == 64
> #define SO_TIMESTAMP SO_TIMESTAMP_OLD
> #define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD
> #define SO_TIMESTAMPING SO_TIMESTAMPING_OLD
> +
> +#define SO_RCVTIMEO SO_RCVTIMEO_OLD
> +#define SO_SNDTIMEO SO_SNDTIMEO_OLD
Maybe I'm a bit late in this discussion as you are already in v5 ...
I can see patches making the transition in different steps where it
might be helpful to name them *_OLD and *_NEW to understand the patches.
But in the end the naming stays in the kernel for a very long time and I
remember myself (in early years) to name things 'new', 'new2', 'new3'.
In fact SO_TIMESTAMP_OLD should be named SO_TIMESTAMP32 and
SO_TIMESTAMP_NEW should be named SO_TIMESTAMP64.
Especially as it tells you what is 'inside', the naming of these new
introduced constants should be replaced with *32 and *64 names.
The documentation and the other comments still fit perfectly then.
Regards,
Oliver
^ permalink raw reply
* Re: [PATCH 02/18] dmaengine: imx-sdma: pass struct device to DMA API functions
From: Christoph Hellwig @ 2019-02-02 17:21 UTC (permalink / raw)
To: Vinod Koul
Cc: Christoph Hellwig, John Crispin, Dmitry Tarnyagin, Nicolas Ferre,
Sudip Mukherjee, Felipe Balbi, linux-mips, linux-kernel,
dmaengine, netdev, linux-usb, linux-fbdev, alsa-devel, iommu
In-Reply-To: <20190202101121.GE4296@vkoul-mobl>
On Sat, Feb 02, 2019 at 03:41:21PM +0530, Vinod Koul wrote:
> On 01-02-19, 09:47, Christoph Hellwig wrote:
> > The DMA API generally relies on a struct device to work properly, and
> > only barely works without one for legacy reasons. Pass the easily
> > available struct device from the platform_device to remedy this.
>
> This looks good to me but fails to apply. Can you please base it on
> dmaengine-next or linux-next please and resend
commit ceaf52265148d3a5ca24237fd1c709caa5f46184
Author: Andy Duan <fugang.duan@nxp.com>
Date: Fri Jan 11 14:29:49 2019 +0000
dmaengine: imx-sdma: pass ->dev to dma_alloc_coherent() API
in linux-next actually is equivalent to this patch, so we can drop
this one.
>
> Thanks
> --
> ~Vinod
---end quoted text---
^ permalink raw reply
* [PATCH] e1000e: Disable runtime PM on CNP+
From: Kai-Heng Feng @ 2019-02-02 17:40 UTC (permalink / raw)
To: jeffrey.t.kirsher; +Cc: intel-wired-lan, netdev, linux-kernel, Kai-Heng Feng
There are some new e1000e devices can only be woken up from D3 one time,
by plugging ethernet cable. Subsequent cable plugging does set PME bit
correctly, but it still doesn't get woken up.
Since e1000e connects to the root complex directly, we rely on ACPI to
wake it up. In this case, the GPE from _PRW only works once and stops
working after that. Though it appears to be a platform bug, e1000e
maintainers confirmed that I219 does not support D3.
So disable runtime PM on CNP+ chips. We may need to disable earlier
generations if this bug also hit older platforms.
Bugzilla: https://bugzilla.kernel.org/attachment.cgi?id=280819
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
drivers/net/ethernet/intel/e1000e/netdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 189f231075c2..9366b9d19a6f 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -7351,7 +7351,7 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
e1000_print_device_info(adapter);
- if (pci_dev_run_wake(pdev))
+ if (pci_dev_run_wake(pdev) && hw->mac.type < e1000_pch_cnp)
pm_runtime_put_noidle(&pdev->dev);
return 0;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] PCI / ACPI: Don't clear pme_poll on device that has unreliable ACPI wake
From: Kai Heng Feng @ 2019-02-02 17:46 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Rafael J. Wysocki, Len Brown, jeffrey.t.kirsher, intel-wired-lan,
netdev, linux-acpi, linux-pci, linux-kernel
In-Reply-To: <C7E9B3C9-7E20-4CEB-8B56-D9A8D2E76DD0@canonical.com>
Hi Bjorn,
> On Jan 28, 2019, at 3:51 PM, Kai Heng Feng <kai.heng.feng@canonical.com> wrote:
[snipped]
>> If I understand correctly, the bugzilla lspci
>> (https://bugzilla.kernel.org/attachment.cgi?id=280691) was collected
>> at point 8, and it shows PME_Status=1 when it should be 0.
>>
>> If we write a 1 to PME_Status to clear it, and it remains set, that's
>> obviously a hardware defect, and Intel should document that in an
>> erratum, and a quirk would be the appropriate way to work around it.
>> But I doubt that's what's happening.
>
> I’ll ask them if they can provide an erratum.
Got confirmed with e1000e folks, I219 (the device in question) doesn’t really support runtime D3.
I also checked the behavior of the device under Windows, and it stays at D0 all the time even when it’s not in use.
So I sent a patch [1] to disable it.
[1] https://lkml.org/lkml/2019/2/2/200
Kai-Heng
^ permalink raw reply
* [PATCH] net: dsa: Fix lockdep false positive splat
From: Marc Zyngier @ 2019-02-02 17:53 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller
Creating a macvtap on a DSA-backed interface results in the following
splat when lockdep is enabled:
[ 19.638080] IPv6: ADDRCONF(NETDEV_CHANGE): lan0: link becomes ready
[ 23.041198] device lan0 entered promiscuous mode
[ 23.043445] device eth0 entered promiscuous mode
[ 23.049255]
[ 23.049557] ============================================
[ 23.055021] WARNING: possible recursive locking detected
[ 23.060490] 5.0.0-rc3-00013-g56c857a1b8d3 #118 Not tainted
[ 23.066132] --------------------------------------------
[ 23.071598] ip/2861 is trying to acquire lock:
[ 23.076171] 00000000f61990cb (_xmit_ETHER){+...}, at: dev_set_rx_mode+0x1c/0x38
[ 23.083693]
[ 23.083693] but task is already holding lock:
[ 23.089696] 00000000ecf0c3b4 (_xmit_ETHER){+...}, at: dev_uc_add+0x24/0x70
[ 23.096774]
[ 23.096774] other info that might help us debug this:
[ 23.103494] Possible unsafe locking scenario:
[ 23.103494]
[ 23.109584] CPU0
[ 23.112093] ----
[ 23.114601] lock(_xmit_ETHER);
[ 23.117917] lock(_xmit_ETHER);
[ 23.121233]
[ 23.121233] *** DEADLOCK ***
[ 23.121233]
[ 23.127325] May be due to missing lock nesting notation
[ 23.127325]
[ 23.134315] 2 locks held by ip/2861:
[ 23.137987] #0: 000000003b766c72 (rtnl_mutex){+.+.}, at: rtnetlink_rcv_msg+0x338/0x4e0
[ 23.146231] #1: 00000000ecf0c3b4 (_xmit_ETHER){+...}, at: dev_uc_add+0x24/0x70
[ 23.153757]
[ 23.153757] stack backtrace:
[ 23.158243] CPU: 0 PID: 2861 Comm: ip Not tainted 5.0.0-rc3-00013-g56c857a1b8d3 #118
[ 23.166212] Hardware name: Globalscale Marvell ESPRESSOBin Board (DT)
[ 23.172843] Call trace:
[ 23.175358] dump_backtrace+0x0/0x188
[ 23.179116] show_stack+0x14/0x20
[ 23.182524] dump_stack+0xb4/0xec
[ 23.185928] __lock_acquire+0x123c/0x1860
[ 23.190048] lock_acquire+0xc8/0x248
[ 23.193724] _raw_spin_lock_bh+0x40/0x58
[ 23.197755] dev_set_rx_mode+0x1c/0x38
[ 23.201607] dev_set_promiscuity+0x3c/0x50
[ 23.205820] dsa_slave_change_rx_flags+0x5c/0x70
[ 23.210567] __dev_set_promiscuity+0x148/0x1e0
[ 23.215136] __dev_set_rx_mode+0x74/0x98
[ 23.219167] dev_uc_add+0x54/0x70
[ 23.222575] macvlan_open+0x170/0x1d0
[ 23.226336] __dev_open+0xe0/0x160
[ 23.229830] __dev_change_flags+0x16c/0x1b8
[ 23.234132] dev_change_flags+0x20/0x60
[ 23.238074] do_setlink+0x2d0/0xc50
[ 23.241658] __rtnl_newlink+0x5f8/0x6e8
[ 23.245601] rtnl_newlink+0x50/0x78
[ 23.249184] rtnetlink_rcv_msg+0x360/0x4e0
[ 23.253397] netlink_rcv_skb+0xe8/0x130
[ 23.257338] rtnetlink_rcv+0x14/0x20
[ 23.261012] netlink_unicast+0x190/0x210
[ 23.265043] netlink_sendmsg+0x288/0x350
[ 23.269075] sock_sendmsg+0x18/0x30
[ 23.272659] ___sys_sendmsg+0x29c/0x2c8
[ 23.276602] __sys_sendmsg+0x60/0xb8
[ 23.280276] __arm64_sys_sendmsg+0x1c/0x28
[ 23.284488] el0_svc_common+0xd8/0x138
[ 23.288340] el0_svc_handler+0x24/0x80
[ 23.292192] el0_svc+0x8/0xc
This looks fairly harmless (no actual deadlock occurs), and is
fixed in a similar way to c6894dec8ea9 ("bridge: fix lockdep
addr_list_lock false positive splat") by putting the addr_list_lock
in its own lockdep class.
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
net/dsa/master.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/dsa/master.c b/net/dsa/master.c
index 71bb15f491c8..54f5551fb799 100644
--- a/net/dsa/master.c
+++ b/net/dsa/master.c
@@ -205,6 +205,8 @@ static void dsa_master_reset_mtu(struct net_device *dev)
rtnl_unlock();
}
+static struct lock_class_key dsa_master_addr_list_lock_key;
+
int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
{
int ret;
@@ -218,6 +220,8 @@ int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
wmb();
dev->dsa_ptr = cpu_dp;
+ lockdep_set_class(&dev->addr_list_lock,
+ &dsa_master_addr_list_lock_key);
ret = dsa_master_ethtool_setup(dev);
if (ret)
--
2.20.1
^ permalink raw reply related
* Re: general protection fault in rds_recv_rcvbuf_delta
From: syzbot @ 2019-02-02 19:16 UTC (permalink / raw)
To: davem, linux-kernel, linux-rdma, netdev, rds-devel,
santosh.shilimkar, syzkaller-bugs
In-Reply-To: <000000000000445dd9057a7149f1@google.com>
syzbot has found a reproducer for the following crash on:
HEAD commit: cd984a5be215 Merge tag 'xtensa-20190201' of git://github.c..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=1725e4ff400000
kernel config: https://syzkaller.appspot.com/x/.config?x=2e0064f906afee10
dashboard link: https://syzkaller.appspot.com/bug?extid=4b4f8163c2e246df3c4c
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=11631328c00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1172c7ef400000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+4b4f8163c2e246df3c4c@syzkaller.appspotmail.com
kasan: CONFIG_KASAN_INLINE enabled
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] PREEMPT SMP KASAN
CPU: 0 PID: 8054 Comm: syz-executor390 Not tainted 5.0.0-rc4+ #56
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
RIP: 0010:rds_recv_rcvbuf_delta.part.0+0x14a/0x3f0 net/rds/recv.c:103
Code: c1 ea 03 80 3c 02 00 0f 85 6e 02 00 00 4c 8b a3 c0 04 00 00 48 b8 00
00 00 00 00 fc ff df 49 8d 7c 24 2c 48 89 fa 48 c1 ea 03 <0f> b6 14 02 48
89 f8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85 00
RSP: 0018:ffff88808fd9f670 EFLAGS: 00010007
RAX: dffffc0000000000 RBX: ffff8880a96a97c0 RCX: ffffffff8681f5bd
RDX: 0000000000000005 RSI: ffffffff8681f5cb RDI: 000000000000002c
RBP: ffff88808fd9f6a8 R08: ffff888086a1a000 R09: ffffed1011fb3ec4
R10: ffffed1011fb3ec3 R11: 0000000000000003 R12: 0000000000000000
R13: ffff8880a96a9ce4 R14: 000000000002e400 R15: ffff88809aeecc00
FS: 00000000026d4940(0000) GS:ffff8880ae800000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000424ec0 CR3: 000000008e163000 CR4: 00000000001406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
rds_recv_rcvbuf_delta net/rds/recv.c:379 [inline]
rds_recv_incoming+0x789/0x11f0 net/rds/recv.c:379
rds_loop_xmit+0xf3/0x2a0 net/rds/loop.c:96
rds_send_xmit+0x1113/0x2560 net/rds/send.c:355
rds_sendmsg+0x280a/0x3450 net/rds/send.c:1368
sock_sendmsg_nosec net/socket.c:621 [inline]
sock_sendmsg+0xdd/0x130 net/socket.c:631
___sys_sendmsg+0x806/0x930 net/socket.c:2116
__sys_sendmsg+0x105/0x1d0 net/socket.c:2154
__do_sys_sendmsg net/socket.c:2163 [inline]
__se_sys_sendmsg net/socket.c:2161 [inline]
__x64_sys_sendmsg+0x78/0xb0 net/socket.c:2161
do_syscall_64+0x103/0x610 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x4487e9
Code: e8 4c e8 ff ff 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 ab c5 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007fffa307f188 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00000000004487e9
RDX: 0000000000000000 RSI: 0000000020000340 RDI: 0000000000000004
RBP: 000000000000c806 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000004ab4b0
R13: 00000000004052b0 R14: 0000000000000000 R15: 0000000000000000
Modules linked in:
---[ end trace e9dbe4e38be82e2e ]---
RIP: 0010:rds_recv_rcvbuf_delta.part.0+0x14a/0x3f0 net/rds/recv.c:103
Code: c1 ea 03 80 3c 02 00 0f 85 6e 02 00 00 4c 8b a3 c0 04 00 00 48 b8 00
00 00 00 00 fc ff df 49 8d 7c 24 2c 48 89 fa 48 c1 ea 03 <0f> b6 14 02 48
89 f8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85 00
RSP: 0018:ffff88808fd9f670 EFLAGS: 00010007
RAX: dffffc0000000000 RBX: ffff8880a96a97c0 RCX: ffffffff8681f5bd
RDX: 0000000000000005 RSI: ffffffff8681f5cb RDI: 000000000000002c
RBP: ffff88808fd9f6a8 R08: ffff888086a1a000 R09: ffffed1011fb3ec4
R10: ffffed1011fb3ec3 R11: 0000000000000003 R12: 0000000000000000
R13: ffff8880a96a9ce4 R14: 000000000002e400 R15: ffff88809aeecc00
FS: 00000000026d4940(0000) GS:ffff8880ae800000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000424ec0 CR3: 000000008e163000 CR4: 00000000001406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
^ permalink raw reply
* Re: [PATCH] net: dsa: Fix lockdep false positive splat
From: Florian Fainelli @ 2019-02-02 19:30 UTC (permalink / raw)
To: Marc Zyngier, netdev, linux-kernel
Cc: Andrew Lunn, Vivien Didelot, David S. Miller
In-Reply-To: <20190202175329.5969-1-marc.zyngier@arm.com>
Le 2/2/19 à 9:53 AM, Marc Zyngier a écrit :
> Creating a macvtap on a DSA-backed interface results in the following
> splat when lockdep is enabled:
>
> [ 19.638080] IPv6: ADDRCONF(NETDEV_CHANGE): lan0: link becomes ready
> [ 23.041198] device lan0 entered promiscuous mode
> [ 23.043445] device eth0 entered promiscuous mode
> [ 23.049255]
> [ 23.049557] ============================================
> [ 23.055021] WARNING: possible recursive locking detected
> [ 23.060490] 5.0.0-rc3-00013-g56c857a1b8d3 #118 Not tainted
> [ 23.066132] --------------------------------------------
> [ 23.071598] ip/2861 is trying to acquire lock:
> [ 23.076171] 00000000f61990cb (_xmit_ETHER){+...}, at: dev_set_rx_mode+0x1c/0x38
> [ 23.083693]
> [ 23.083693] but task is already holding lock:
> [ 23.089696] 00000000ecf0c3b4 (_xmit_ETHER){+...}, at: dev_uc_add+0x24/0x70
> [ 23.096774]
> [ 23.096774] other info that might help us debug this:
> [ 23.103494] Possible unsafe locking scenario:
> [ 23.103494]
> [ 23.109584] CPU0
> [ 23.112093] ----
> [ 23.114601] lock(_xmit_ETHER);
> [ 23.117917] lock(_xmit_ETHER);
> [ 23.121233]
> [ 23.121233] *** DEADLOCK ***
> [ 23.121233]
> [ 23.127325] May be due to missing lock nesting notation
> [ 23.127325]
> [ 23.134315] 2 locks held by ip/2861:
> [ 23.137987] #0: 000000003b766c72 (rtnl_mutex){+.+.}, at: rtnetlink_rcv_msg+0x338/0x4e0
> [ 23.146231] #1: 00000000ecf0c3b4 (_xmit_ETHER){+...}, at: dev_uc_add+0x24/0x70
> [ 23.153757]
> [ 23.153757] stack backtrace:
> [ 23.158243] CPU: 0 PID: 2861 Comm: ip Not tainted 5.0.0-rc3-00013-g56c857a1b8d3 #118
> [ 23.166212] Hardware name: Globalscale Marvell ESPRESSOBin Board (DT)
> [ 23.172843] Call trace:
> [ 23.175358] dump_backtrace+0x0/0x188
> [ 23.179116] show_stack+0x14/0x20
> [ 23.182524] dump_stack+0xb4/0xec
> [ 23.185928] __lock_acquire+0x123c/0x1860
> [ 23.190048] lock_acquire+0xc8/0x248
> [ 23.193724] _raw_spin_lock_bh+0x40/0x58
> [ 23.197755] dev_set_rx_mode+0x1c/0x38
> [ 23.201607] dev_set_promiscuity+0x3c/0x50
> [ 23.205820] dsa_slave_change_rx_flags+0x5c/0x70
> [ 23.210567] __dev_set_promiscuity+0x148/0x1e0
> [ 23.215136] __dev_set_rx_mode+0x74/0x98
> [ 23.219167] dev_uc_add+0x54/0x70
> [ 23.222575] macvlan_open+0x170/0x1d0
> [ 23.226336] __dev_open+0xe0/0x160
> [ 23.229830] __dev_change_flags+0x16c/0x1b8
> [ 23.234132] dev_change_flags+0x20/0x60
> [ 23.238074] do_setlink+0x2d0/0xc50
> [ 23.241658] __rtnl_newlink+0x5f8/0x6e8
> [ 23.245601] rtnl_newlink+0x50/0x78
> [ 23.249184] rtnetlink_rcv_msg+0x360/0x4e0
> [ 23.253397] netlink_rcv_skb+0xe8/0x130
> [ 23.257338] rtnetlink_rcv+0x14/0x20
> [ 23.261012] netlink_unicast+0x190/0x210
> [ 23.265043] netlink_sendmsg+0x288/0x350
> [ 23.269075] sock_sendmsg+0x18/0x30
> [ 23.272659] ___sys_sendmsg+0x29c/0x2c8
> [ 23.276602] __sys_sendmsg+0x60/0xb8
> [ 23.280276] __arm64_sys_sendmsg+0x1c/0x28
> [ 23.284488] el0_svc_common+0xd8/0x138
> [ 23.288340] el0_svc_handler+0x24/0x80
> [ 23.292192] el0_svc+0x8/0xc
>
> This looks fairly harmless (no actual deadlock occurs), and is
> fixed in a similar way to c6894dec8ea9 ("bridge: fix lockdep
> addr_list_lock false positive splat") by putting the addr_list_lock
> in its own lockdep class.
Great timing, I was just looking at this after solving another one seen
with the bridge code on net-next. AFAIR you can also trigger this with
VLAN and pretty much anything that tries to push UC/MC address list down
to the master device.
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames
From: David Ahern @ 2019-02-02 21:27 UTC (permalink / raw)
To: Jesper Dangaard Brouer, David Miller
Cc: mst, makita.toshiaki, jasowang, netdev, virtualization, hawk,
Toke Høiland-Jørgensen
In-Reply-To: <20190131211555.3b15c81f@carbon>
On 1/31/19 1:15 PM, Jesper Dangaard Brouer wrote:
>>
>> David, Jesper, care to chime in where we ended up in that last thread
>> discussion this?
>
> IHMO packets RX and TX on a device need to be accounted, in standard
> counters, regardless of XDP. For XDP RX the packet is counted as RX,
> regardless if XDP choose to XDP_DROP. On XDP TX which is via
> XDP_REDIRECT or XDP_TX, the driver that transmit the packet need to
> account the packet in a TX counter (this if often delayed to DMA TX
> completion handling). We cannot break the expectation that RX and TX
> counter are visible to userspace stats tools. XDP should not make these
> packets invisible.
Agreed. What I was pushing on that last thread was Rx, Tx and dropped
are all accounted by the driver in standard stats. Basically if the
driver touched it, the driver's counters should indicate that.
The push back was on dropped packets and whether that counter should be
bumped on XDP_DROP.
^ permalink raw reply
* Re: [PATCH 02/12 net-next,v7] net/mlx5e: support for two independent packet edit actions
From: Or Gerlitz @ 2019-02-02 21:40 UTC (permalink / raw)
To: Tonghao Zhang
Cc: Pablo Neira Ayuso, Linux Kernel Network Developers, David Miller,
thomas.lendacky, Florian Fainelli, Ariel Elior, Michael Chan,
santosh, madalin.bucur, Zhuangyuzeng (Yisen), Salil Mehta,
Jeff Kirsher, Tariq Toukan, Saeed Mahameed, Jiri Pirko,
Ido Schimmel, Jakub Kicinski, peppe.cavallaro, grygorii.strashko,
Andrew Lunn, Vivien Didelot, alexandre.torgue, joabreu,
linux-net-drivers, Ganesh Goudar, Or Gerlitz, Manish.Chopra,
Marcelo Ricardo Leitner, mkubecek, venkatkumar.duvvuru,
Julia Lawall, John Fastabend, netfilter-devel, Chris Healy
In-Reply-To: <CAMDZJNVv20HWiv4zT__jhYv8RWfTdUJ+bnG1QsgJZTDL6XfV4A@mail.gmail.com>
On Sat, Feb 2, 2019 at 5:19 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> The patch [1] is ready too. David may decide which one will be applied
> firstly. and other is rebased ?.
Your patch is for net, net-next is rebased over net
> [1] http://patchwork.ozlabs.org/patch/1032952/
^ permalink raw reply
* Re: [PATCH 02/12 net-next,v7] net/mlx5e: support for two independent packet edit actions
From: Or Gerlitz @ 2019-02-02 21:43 UTC (permalink / raw)
To: Tonghao Zhang
Cc: Pablo Neira Ayuso, Linux Kernel Network Developers, David Miller,
thomas.lendacky, Florian Fainelli, Ariel Elior, Michael Chan,
santosh, madalin.bucur, Zhuangyuzeng (Yisen), Salil Mehta,
Jeff Kirsher, Tariq Toukan, Saeed Mahameed, Jiri Pirko,
Ido Schimmel, Jakub Kicinski, peppe.cavallaro, grygorii.strashko,
Andrew Lunn, Vivien Didelot, alexandre.torgue, joabreu,
linux-net-drivers, Ganesh Goudar, Or Gerlitz, Manish.Chopra,
Marcelo Ricardo Leitner, mkubecek, venkatkumar.duvvuru,
Julia Lawall, John Fastabend, netfilter-devel, Chris Healy
In-Reply-To: <CAMDZJNVv20HWiv4zT__jhYv8RWfTdUJ+bnG1QsgJZTDL6XfV4A@mail.gmail.com>
On Sat, Feb 2, 2019 at 5:19 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> The patch [1] is ready too. David may decide which one will be applied
> firstly. and other is rebased ?.
Your patch is for net, net-next is rebased over net
> [1] http://patchwork.ozlabs.org/patch/1032952/
^ permalink raw reply
* Re: [PATCH net-next] net: devlink: report cell size of shared buffers
From: Jakub Kicinski @ 2019-02-02 22:00 UTC (permalink / raw)
To: Ido Schimmel
Cc: davem@davemloft.net, jiri@resnulli.us, netdev@vger.kernel.org,
oss-drivers@netronome.com
In-Reply-To: <20190202123549.GA24795@splinter>
On Sat, 2 Feb 2019 12:35:51 +0000, Ido Schimmel wrote:
> On Fri, Feb 01, 2019 at 05:56:28PM -0800, Jakub Kicinski wrote:
> > Shared buffer allocation is usually done in cell increments.
> > Drivers will either round up the allocation or refuse the
> > configuration if it's not an exact multiple of cell size.
> > Drivers know exactly the cell size of shared buffer, so help
> > out users by providing this information in dumps.
> >
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Reviewed-by: Dirk van der Merwe <dirk.vandermerwe@netronome.com>
>
> Reviewed-by: Ido Schimmel <idosch@mellanox.com>
>
> I assume you're going to patch iproute2. Consider explaining what is
> cell size in devlink-sb manpage.
Will do, thanks!
^ permalink raw reply
* Re: [PATCH iproute2-next] devlink: add info subcommand
From: Jakub Kicinski @ 2019-02-02 22:06 UTC (permalink / raw)
To: Jiri Pirko; +Cc: dsahern, stephen, netdev, oss-drivers
In-Reply-To: <20190202155938.GE2778@nanopsycho>
On Sat, 2 Feb 2019 16:59:38 +0100, Jiri Pirko wrote:
> Sat, Feb 02, 2019 at 01:03:38AM CET, jakub.kicinski@netronome.com wrote:
> >Add support for reading the device serial number and versions
> >from the kernel.
> >
> >RFCv2:
> > - make info subcommand of dev.
>
> Please add some examples of inputs and outputs.
Sorry, yes, will do.
> >Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> >---
> > devlink/devlink.c | 207 +++++++++++++++++++++++++++++++++++++++++
> > man/man8/devlink-dev.8 | 36 +++++++
> > 2 files changed, 243 insertions(+)
> >
> >diff --git a/devlink/devlink.c b/devlink/devlink.c
> >index 3651e90c1159..3ab046ace8f8 100644
> >--- a/devlink/devlink.c
> >+++ b/devlink/devlink.c
> >@@ -199,6 +199,7 @@ 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_VERSIONS_TYPE BIT(25)
>
> Why "type"? Confusing.
Right, I think this dates back to day 1 when the whole thing was called
versions not info. How about DL_OPT_INFO_VERSION_TYPE?
> >
> > struct dl_opts {
> > uint32_t present; /* flags of present items */
> >@@ -943,6 +952,21 @@ static int param_cmode_get(const char *cmodestr,
> > return 0;
> > }
> >
> >+static int versions_type_get(const char *typestr, int *p_attr)
> >+{
> >+ if (strcmp(typestr, "fixed") == 0) {
> >+ *p_attr = DEVLINK_ATTR_INFO_VERSION_FIXED;
> >+ } else if (strcmp(typestr, "stored") == 0) {
> >+ *p_attr = DEVLINK_ATTR_INFO_VERSION_STORED;
> >+ } else if (strcmp(typestr, "running") == 0) {
> >+ *p_attr = DEVLINK_ATTR_INFO_VERSION_RUNNING;
> >+ } else {
> >+ pr_err("Unknown versions type \"%s\"\n", typestr);
> >+ return -EINVAL;
> >+ }
> >+ return 0;
> >+}
> >+
> > static int dl_argv_parse(struct dl *dl, uint32_t o_required,
> > uint32_t o_optional)
> > {
> >@@ -1178,6 +1202,19 @@ 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, "versions") &&
> >+ (o_all & DL_OPT_VERSIONS_TYPE)) {
> >+ const char *versionstr;
> >+
> >+ dl_arg_inc(dl);
> >+ err = dl_argv_str(dl, &versionstr);
> >+ if (err)
> >+ return err;
> >+ err = versions_type_get(versionstr,
> >+ &opts->versions_attr);
> >+ if (err)
> >+ return err;
> >+ o_found |= DL_OPT_VERSIONS_TYPE;
> > } else {
> > pr_err("Unknown option \"%s\"\n", dl_argv(dl));
> > return -EINVAL;
> >@@ -1443,6 +1480,9 @@ static void cmd_dev_help(void)
> > pr_err(" devlink dev param set DEV name PARAMETER value VALUE cmode { permanent | driverinit | runtime }\n");
> > pr_err(" devlink dev param show [DEV name PARAMETER]\n");
> > pr_err(" devlink dev reload DEV\n");
> >+ pr_err(" devlink dev info [ DEV [ { versions [ VTYPE ] } ] ]\n");
> >+ pr_err("\n");
> >+ pr_err(" VTYPE := { fixed | running | stored }\n");
>
> So you would like to filter according to the version type? Why?
if devlink dev info bus/train versions stored != devlink ... running
then reboot is needed. That one of main uses for reporting running vs
stored in sections, it's nice to be able to just compare the outputs.
^ permalink raw reply
* Re: Bluetooth: hci0: last event is not cmd complete (0x0f) with LG TV
From: Paul Menzel @ 2019-02-02 22:31 UTC (permalink / raw)
To: Marcel Holtmann, Johan Hedberg
Cc: David S. Miller, linux-bluetooth, netdev, LKML
In-Reply-To: <f2384806-ad02-4a6d-895c-9e07c4add513@molgen.mpg.de>
[-- Attachment #1: Type: text/plain, Size: 1201 bytes --]
Dear Linux folks,
On 01.02.19 22:34, Paul Menzel wrote:
> [attaching Linux messages, lspci and lsusb output]
>
> On 01.02.19 22:20, Paul Menzel wrote:
>> When trying to pair a Dell Latitude E7250 running Debian Sid/unstable
>> with Linux 4.20 and GNOME 3.30 with an LG TV, after starting the
>> pairing process the TV is listed. in Bluetooth dialog of GNOME setting.
>>
>> The TV displays the instructions below.
>>
>>> Complete the next three steps on your mobile device:
>>> 1. Turn ON Bluetooth.
>>> 2. Select the TV name from the list of available devices.
>>> • TV Name : 679
>>> 3. Confirm the connection request.
>>
>> Selecting the TV in the GNOME dialog, a dialog is shown on my system
>> with a PIN consisting of six numbers. With the dialog, Linux logs the
>> message below.
>>
>> Bluetooth: hci0: last event is not cmd complete (0x0f)
>>
>> But, the TV does not show any PIN. Confirming it anyway, nothing is
>> happening further.
>>
>> Is that supposed to work? It’d be great if you helped me to set this up.
Please find a trace attached. This time Linux 4.19.16 was running.
sudo tcpdump -i bluetooth0 -w bluetooth0_capture.pcap
Kind regards,
Paul
[-- Attachment #2: bluetooth0_capture.pcap --]
[-- Type: application/vnd.tcpdump.pcap, Size: 33808 bytes --]
^ permalink raw reply
* Re: [RFC 00/14] netlink/hierarchical stats
From: Roopa Prabhu @ 2019-02-02 23:14 UTC (permalink / raw)
To: Jakub Kicinski
Cc: David Miller, oss-drivers, netdev, Jiří Pírko,
Florian Fainelli, Andrew Lunn, Michal Kubecek, David Ahern,
Simon Horman, Brandeburg, Jesse, Maciek Fijałkowski,
vasundhara-v.volam, Michael Chan, shalomt, Ido Schimmel
In-Reply-To: <20190131113048.45bd149a@cakuba.hsd1.ca.comcast.net>
On Thu, Jan 31, 2019 at 11:31 AM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Thu, 31 Jan 2019 08:31:51 -0800, Roopa Prabhu wrote:
> > On Thu, Jan 31, 2019 at 8:16 AM Roopa Prabhu wrote:
> > > On Wed, Jan 30, 2019 at 4:24 PM Jakub Kicinski wrote:
> > > > On Wed, 30 Jan 2019 14:14:34 -0800, Roopa Prabhu wrote:
> > > >
> > > > My thinking was that we should leave truly custom/strange stats to
> > > > ethtool API which works quite well for that and at the same time be
> > > > very accepting of people adding new IDs to HSTAT (only requirement is
> > > > basically defining the meaning very clearly).
> > >
> > > that sounds reasonable. But the 'defining meaning clearly' gets tricky
> > > sometimes.
> > > The vendor who gets their ID or meaning first wins :) and the rest
> > > will have to live with
> > > ethtool and explain to rest of the world that ethtool is more reliable
> > > for their hardware :)
>
> Right, that's the trade off inherent to standardization. I don't see
> any way to work around the fact that the definition may not fit all.
>
> What I want as a end user and what I want for my customers is the
> ability to switch the NIC on their system and not spend two months
> "integrating" into their automation :( If the definition of statistics
> is not solid we're back to square one.
agree. And I am with you on standardizing them.
>
> > > I am also concerned that this getting the ID into common HSTAT ID
> > > space will slow down the process of adding new counters
> > > for vendors. Which will lead to vendors sticking with ethtool API.
>
> I feel like whatever we did here will end up looking much like the
> ethtool interface, which is why I decided to leave that part out.
> Ethtool -S works pretty well for custom stats. Standard and structured
> stats don't fit with it in any way, the two seem best left separate.
understand the fear. My only point was getting them together in a
single API is better so that they don't get developed separately and
we don't end up with duplicate stats code.
>
> > > It would be great if people can get all stats in one place and not
> > > rely on another API for 'more'.
>
> One place in the driver or for the user? I'm happy to add the code to
> ethtool to also dump hstats and render them in a standard way. In fact
> the tool I have for testing has a "simplified" output format which
> looks exactly like ethtool -S.
>
> One place for the driver to report is hard, as I said I think the
> custom stats are best left with ethtool. Adding an extra incentive to
> standardize.
>
> > > > For the first stab I looked at two drivers and added all the stats that
> > > > were common.
> > > >
> > > > Given this set is identifying statistics by ID - how would we make that
> > > > extensible to drivers? Would we go back to strings or have some
> > > > "driver specific" ID space?
> > >
> > > I was looking for ideas from you really, to see if you had considered
> > > this. agree per driver ID space seems ugly.
> > > ethtool strings are great today...if we can control the duplication.
> > > But thinking some more..., i did see some
> > > patches recently for vendor specific parameter (with ID) space in
> > > devlink. maybe something like that will be
> > > reasonable ?
>
> I thought about this for a year and I basically came to the conclusion
> I can't find any perfect solution, if there is one.
>
> The devlink parameters are useful, but as anticipated they became the
> laziest excuse of an ABI... Don't get me started ;)
>
> > > > Is there any particular type of statistic you'd expect drivers to want
> > > > to add? For NICs I think IEEE/RMON should pretty much cover the
> > > > silicon ones, but I don't know much about switches :)
> > >
> > > I will have to go through the list. But switch asics do support
> > > flexible stats/counters that can be attached at various points.
> > > And new chip versions come with more support. Having that flexibility
> > > to expose/extend such stats incrementally is very valuable on a per
> > > hardware/vendor basis.
>
> Yes, I'm not too familiar with those counters. Do they need to be
> enabled to start counting?
yes correct.
> Do they have performance impact?
I have not heard of any performance impact...but they are not enabled
by default because of limited counter resource pool.
> Can the
> "sample" events perf-style?
I don't think so
> How is the condition on which they trigger
> defined? Is it maybe just "match a packet and increment a counter"?
yes, something like that.
> Would such counters benefit from hierarchical structure?
hmm not sure.
One thing though, for most of these flexible counters and their
attachment points in hardware, we can count them on logical devices or
other objects in software like per vlan, vni, route stats etc.
>
> I was trying to cover the long standing use cases - namely the
> IEEE/RMON stats which all MAC have had for years and per queue stats
> which all drivers have had for years. But if we can cater to more
> cases I'm open.
>
> > Just want to clarify that I am suggesting a nested HSTATS extension
> > infra for drivers (just like ethtool).
> > 'Common stats' stays at the top-level.
>
> I got a concept of groups here. The dump generally looks like this:
>
> [root group A (say MAC stats)]
> [sub group RX]
> [sub group TX]
> [root group B (say PCIe stats)]
> [sub group RX]
> [sub group TX]
> [root group C (say per-q driver stats]
> [sub group RX]
> [q1 group]
> [q2 group]
> [q3 group]
> [sub group TX]
> [q1 group]
> [q2 group]
> [q3 group]
>
> Each root group representing a "point in the pipeline".
>
> So it's not too hard to add a root group with whatever, the questions
> are move how would it benefit over existing ethtool if the stats are
> custom anyway? Hm..
It wouldn't. I am only saying that the netlink stats API is the new
place to move stats.
Ethtool stats will have to move to netlink some day and I don't see a
need to draw a hardline on saying no to ethtool custom stats moving to
the netlink based common stats API. And unless there is a good
migration path for a new hardware stats API that is all inclusive,
there is a higher chance of continued development on the older
hardware stats API.
I have no objections to having a standard set of stats (this is
essentially what we have for software stats too).
I don't want to block your series from going forward without HW custom
stats extensions. And IIUC your API is extensible and does not
preclude anyone from adding the ability to include HW custom stats
extensions in the future with enough justification. That is good for
me.
To take a random example, we expose the following stats on our
switches via ethtool. I have not used them personally but they
correspond to respective hardware counters. Is there any room for such
stats in the new HSTATS netlink API or they will have to stick to
ethtool ?
I believe people will need per-queue counters for this.
HwIfOutWredDrops: 0
HwIfOutQ0WredDrops: 0
HwIfOutQ1WredDrops: 0
HwIfOutQ2WredDrops: 0
HwIfOutQ3WredDrops: 0
HwIfOutQ4WredDrops: 0
HwIfOutQ5WredDrops: 0
HwIfOutQ6WredDrops: 0
HwIfOutQ7WredDrops: 0
HwIfOutQ8WredDrops: 0
HwIfOutQ9WredDrops: 0
^ permalink raw reply
* [PATCH] Bluetooth: Fix decrementing reference count twice in releasing socket
From: Myungho Jung @ 2019-02-03 0:56 UTC (permalink / raw)
To: Marcel Holtmann, Johan Hedberg, David S. Miller
Cc: linux-bluetooth, netdev, linux-kernel
When releasing socket, it is possible to enter hci_sock_release() and
hci_sock_dev_event(HCI_DEV_UNREG) at the same time in different thread.
The reference count of hdev should be decremented only once from one of
them but if storing hdev to local variable in hci_sock_release() before
detached from socket and setting to NULL in hci_sock_dev_event(),
hci_dev_put(hdev) is unexpectedly called twice. This is resolved by
referencing hdev from socket after bt_sock_unlink() in
hci_sock_release().
Reported-by: syzbot+fdc00003f4efff43bc5b@syzkaller.appspotmail.com
Signed-off-by: Myungho Jung <mhjungk@gmail.com>
---
net/bluetooth/hci_sock.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 1506e1632394..d4e2a166ae17 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -831,8 +831,6 @@ static int hci_sock_release(struct socket *sock)
if (!sk)
return 0;
- hdev = hci_pi(sk)->hdev;
-
switch (hci_pi(sk)->channel) {
case HCI_CHANNEL_MONITOR:
atomic_dec(&monitor_promisc);
@@ -854,6 +852,7 @@ static int hci_sock_release(struct socket *sock)
bt_sock_unlink(&hci_sk_list, sk);
+ hdev = hci_pi(sk)->hdev;
if (hdev) {
if (hci_pi(sk)->channel == HCI_CHANNEL_USER) {
/* When releasing a user channel exclusive access,
--
2.17.1
^ permalink raw reply related
* Still need this?
From: Grace @ 2019-02-02 11:16 UTC (permalink / raw)
To: netdev
Want to retouch your photos? we can help you.
Deep etching or masking for your photos, or even adding clipping path.
Retouching also if needed.
Hopefully to start something for you soon.
Thanks,
Grace
Wordms
Ansbadch
^ permalink raw reply
* Re: [PATCH net-next v5 12/12] sock: Add SO_RCVTIMEO_NEW and SO_SNDTIMEO_NEW
From: Deepa Dinamani @ 2019-02-03 2:47 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: David S. Miller, Linux Kernel Mailing List,
Linux Network Devel Mailing List, Arnd Bergmann,
y2038 Mailman List, ccaulfie, Helge Deller, Paul Mackerras,
Ralf Baechle, Richard Henderson, cluster-devel, linuxppc-dev,
linux-alpha, linux-arch, linux-mips, Parisc List, sparclinux
In-Reply-To: <0fad3f4d-4c0e-d9f2-1af0-7890d40c19c0@hartkopp.net>
On Sat, Feb 2, 2019 at 9:15 AM Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>
> Hi all,
>
> On 02.02.19 16:34, Deepa Dinamani wrote:
> > Add new socket timeout options that are y2038 safe.
> (..)
> >
> > diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h
> > index 9826d1db71d0..0d0fddb7e738 100644
> > --- a/arch/alpha/include/uapi/asm/socket.h
> > +++ b/arch/alpha/include/uapi/asm/socket.h
> > @@ -119,19 +119,25 @@
> > #define SO_TIMESTAMPNS_NEW 64
> > #define SO_TIMESTAMPING_NEW 65
> >
> > -#if !defined(__KERNEL__)
> > +#define SO_RCVTIMEO_NEW 66
> > +#define SO_SNDTIMEO_NEW 67
> >
> > -#define SO_RCVTIMEO SO_RCVTIMEO_OLD
> > -#define SO_SNDTIMEO SO_SNDTIMEO_OLD
> > +#if !defined(__KERNEL__)
> >
> > #if __BITS_PER_LONG == 64
> > #define SO_TIMESTAMP SO_TIMESTAMP_OLD
> > #define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD
> > #define SO_TIMESTAMPING SO_TIMESTAMPING_OLD
> > +
> > +#define SO_RCVTIMEO SO_RCVTIMEO_OLD
> > +#define SO_SNDTIMEO SO_SNDTIMEO_OLD
>
> Maybe I'm a bit late in this discussion as you are already in v5 ...
>
> I can see patches making the transition in different steps where it
> might be helpful to name them *_OLD and *_NEW to understand the patches.
>
> But in the end the naming stays in the kernel for a very long time and I
> remember myself (in early years) to name things 'new', 'new2', 'new3'.
>
> In fact SO_TIMESTAMP_OLD should be named SO_TIMESTAMP32 and
> SO_TIMESTAMP_NEW should be named SO_TIMESTAMP64.
Hmm. SO_TIMESTAMP_OLD uses 64-bit time_t on a 64 bit machine. In fact,
there is no difference between the two options on a 64 bit machine. So
using SO_TIMESTAMP_32 is just wrong.
Likewise, SO_TIMESTAMP_64 naming somehow indicates that the existing
one was not, and that is also not true on a 64-bit machine.
Further, userspace will still continue to use SO_TIMESTAMP and the
macros take care of which option to select internally.
Moreover, these macros have been there for more than ten years
(introduced before 2.4) and there has been no change. I doubt you will
ever have NEW2.
I think this argument is similar to saying don’t name these macros
SO_TIMESTAMP because there might be SO_TIMESTAMP1 some day. There is
never a perfect name. SO_TIMESTAMPING is also not really descriptive.
> Especially as it tells you what is 'inside', the naming of these new introduced constants should be replaced with *32 and *64 names.
> The documentation and the other comments still fit perfectly then.
I would prefer not to do this, for reasons mentioned above. Since you
point out that it is easier to use this naming for intermediate steps,
I suggest that we leave the series as it is.
If you feel strongly to post a follow-on renaming patch, you could
discuss it with the subsystem maintainers, if you think there is a
correct but more descriptive naming.
-Deepa
> Regards,
> Oliver
^ 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