[RFC PATCH net-next] net: dsa: replace NETDEV_PRE_CHANGE_HWTSTAMP notifier with a stub

[RFC PATCH net-next] net: dsa: replace NETDEV_PRE_CHANGE_HWTSTAMP notifier with a stub

[Vladimir Oltean]

There was a sort of rush surrounding commit 88c0a6b503b7 ("net: create a
netdev notifier for DSA to reject PTP on DSA master"), due to a desire
to convert DSA's attempt to deny TX timestamping on a DSA master to
something that doesn't block the kernel-wide API conversion from
ndo_eth_ioctl() to ndo_hwtstamp_set().

What was required was a mechanism that did not depend on ndo_eth_ioctl(),
and what was provided was a mechanism that did not depend on
ndo_eth_ioctl(), while at the same time introducing something that
wasn't absolutely necessary - a new netdev notifier.

There have been objections from Jakub Kicinski that using notifiers in
general when they are not absolutely necessary creates complications to
the control flow and difficulties to maintainers who look at the code.
So there is a desire to not use notifiers.

Take the model of udp_tunnel_nic_ops and introduce a stub mechanism,
through which net/core/dev_ioctl.c can call into DSA even when
CONFIG_NET_DSA=m.

Compared to the code that existed prior to the notifier conversion, aka
what was added in commits:
- 4cfab3566710 ("net: dsa: Add wrappers for overloaded ndo_ops")
- 3369afba1e46 ("net: Call into DSA netdevice_ops wrappers")

this is different because we are not overloading any struct
net_device_ops of the DSA master anymore, but rather, we are exposing a
rather specific functionality which is orthogonal to which API is used
to enable it - ndo_eth_ioctl() or ndo_hwtstamp_set().

Also, what is similar is that both approaches use function pointers to
get from built-in code to DSA.

Since the new functionality does not overload the NDO of any DSA master,
there is no point in replicating the function pointers towards
__dsa_master_hwtstamp_validate() once for every CPU port (dev->dsa_ptr).
But rather, it is fine to introduce a singleton struct dsa_stubs,
built-in to the kernel, which contains a single function pointer to
__dsa_master_hwtstamp_validate().

I find this approach rather preferable to what we had originally,
because dev->dsa_ptr->netdev_ops->ndo_do_ioctl() used to require going
through struct dsa_port (dev->dsa_ptr), and so, this was incompatible
with any attempts to add any data encapsulation and hide DSA data
structures from the outside world.

Link: https://lore.kernel.org/netdev/20230403083019.120b72fd@kernel.org/
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/linux/netdevice.h |  6 -----
 include/net/dsa_stubs.h   | 49 +++++++++++++++++++++++++++++++++++++++
 net/Makefile              |  2 +-
 net/core/dev.c            |  2 +-
 net/core/dev_ioctl.c      | 12 ++--------
 net/dsa/Makefile          |  6 +++++
 net/dsa/dsa.c             | 27 +++++++++++++++++++++
 net/dsa/master.c          |  2 +-
 net/dsa/master.h          |  2 +-
 net/dsa/slave.c           | 10 --------
 net/dsa/stubs.c           | 13 +++++++++++
 11 files changed, 101 insertions(+), 30 deletions(-)
 create mode 100644 include/net/dsa_stubs.h
 create mode 100644 net/dsa/stubs.c

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index a740be3bb911..1c25b39681b3 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2878,7 +2878,6 @@ enum netdev_cmd {
 	NETDEV_OFFLOAD_XSTATS_REPORT_USED,
 	NETDEV_OFFLOAD_XSTATS_REPORT_DELTA,
 	NETDEV_XDP_FEAT_CHANGE,
-	NETDEV_PRE_CHANGE_HWTSTAMP,
 };
 const char *netdev_cmd_to_name(enum netdev_cmd cmd);
 
@@ -2929,11 +2928,6 @@ struct netdev_notifier_pre_changeaddr_info {
 	const unsigned char *dev_addr;
 };
 
-struct netdev_notifier_hwtstamp_info {
-	struct netdev_notifier_info info; /* must be first */
-	struct kernel_hwtstamp_config *config;
-};
-
 enum netdev_offload_xstats_type {
 	NETDEV_OFFLOAD_XSTATS_TYPE_L3 = 1,
 };
diff --git a/include/net/dsa_stubs.h b/include/net/dsa_stubs.h
new file mode 100644
index 000000000000..27a1ad85c038
--- /dev/null
+++ b/include/net/dsa_stubs.h
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * include/net/dsa_stubs.h - Stubs for the Distributed Switch Architecture framework
+ */
+
+#include <linux/mutex.h>
+#include <linux/netdevice.h>
+#include <linux/net_tstamp.h>
+#include <net/dsa.h>
+
+#if IS_ENABLED(CONFIG_NET_DSA)
+
+extern const struct dsa_stubs *dsa_stubs;
+extern struct mutex dsa_stubs_lock;
+
+struct dsa_stubs {
+	int (*master_hwtstamp_validate)(struct net_device *dev,
+					const struct kernel_hwtstamp_config *config,
+					struct netlink_ext_ack *extack);
+};
+
+static inline int dsa_master_hwtstamp_validate(struct net_device *dev,
+					       const struct kernel_hwtstamp_config *config,
+					       struct netlink_ext_ack *extack)
+{
+	int err;
+
+	if (!netdev_uses_dsa(dev))
+		return 0;
+
+	mutex_lock(&dsa_stubs_lock);
+
+	if (dsa_stubs)
+		err = dsa_stubs->master_hwtstamp_validate(dev, config, extack);
+
+	mutex_unlock(&dsa_stubs_lock);
+
+	return err;
+}
+
+#else
+
+static inline int dsa_master_hwtstamp_validate(struct net_device *dev,
+					       const struct kernel_hwtstamp_config *config)
+{
+	return 0;
+}
+
+#endif
diff --git a/net/Makefile b/net/Makefile
index 0914bea9c335..87592009366f 100644
--- a/net/Makefile
+++ b/net/Makefile
@@ -24,7 +24,7 @@ obj-$(CONFIG_PACKET)		+= packet/
 obj-$(CONFIG_NET_KEY)		+= key/
 obj-$(CONFIG_BRIDGE)		+= bridge/
 obj-$(CONFIG_NET_DEVLINK)	+= devlink/
-obj-$(CONFIG_NET_DSA)		+= dsa/
+obj-y				+= dsa/
 obj-$(CONFIG_ATALK)		+= appletalk/
 obj-$(CONFIG_X25)		+= x25/
 obj-$(CONFIG_LAPB)		+= lapb/
diff --git a/net/core/dev.c b/net/core/dev.c
index 7ce5985be84b..480600a075ce 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1612,7 +1612,7 @@ const char *netdev_cmd_to_name(enum netdev_cmd cmd)
 	N(SVLAN_FILTER_PUSH_INFO) N(SVLAN_FILTER_DROP_INFO)
 	N(PRE_CHANGEADDR) N(OFFLOAD_XSTATS_ENABLE) N(OFFLOAD_XSTATS_DISABLE)
 	N(OFFLOAD_XSTATS_REPORT_USED) N(OFFLOAD_XSTATS_REPORT_DELTA)
-	N(XDP_FEAT_CHANGE) N(PRE_CHANGE_HWTSTAMP)
+	N(XDP_FEAT_CHANGE)
 	}
 #undef N
 	return "UNKNOWN_NETDEV_EVENT";
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 6d772837eb3f..3730945ee294 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -7,7 +7,7 @@
 #include <linux/net_tstamp.h>
 #include <linux/wireless.h>
 #include <linux/if_bridge.h>
-#include <net/dsa.h>
+#include <net/dsa_stubs.h>
 #include <net/wext.h>
 
 #include "dev.h"
@@ -259,9 +259,6 @@ static int dev_get_hwtstamp(struct net_device *dev, struct ifreq *ifr)
 
 static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
 {
-	struct netdev_notifier_hwtstamp_info info = {
-		.info.dev = dev,
-	};
 	struct kernel_hwtstamp_config kernel_cfg;
 	struct netlink_ext_ack extack = {};
 	struct hwtstamp_config cfg;
@@ -276,12 +273,7 @@ static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
 	if (err)
 		return err;
 
-	info.info.extack = &extack;
-	info.config = &kernel_cfg;
-
-	err = call_netdevice_notifiers_info(NETDEV_PRE_CHANGE_HWTSTAMP,
-					    &info.info);
-	err = notifier_to_errno(err);
+	err = dsa_master_hwtstamp_validate(dev, &kernel_cfg, &extack);
 	if (err) {
 		if (extack._msg)
 			netdev_err(dev, "%s\n", extack._msg);
diff --git a/net/dsa/Makefile b/net/dsa/Makefile
index cc7e93a562fe..3835de286116 100644
--- a/net/dsa/Makefile
+++ b/net/dsa/Makefile
@@ -1,4 +1,10 @@
 # SPDX-License-Identifier: GPL-2.0
+
+# the stubs are built-in whenever DSA is built-in or module
+ifdef CONFIG_NET_DSA
+obj-y := stubs.o
+endif
+
 # the core
 obj-$(CONFIG_NET_DSA) += dsa_core.o
 dsa_core-y += \
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index e5f156940c67..119d480b3d53 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -17,6 +17,7 @@
 #include <linux/of.h>
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>
+#include <net/dsa_stubs.h>
 #include <net/sch_generic.h>
 
 #include "devlink.h"
@@ -1702,6 +1703,28 @@ bool dsa_mdb_present_in_other_db(struct dsa_switch *ds, int port,
 }
 EXPORT_SYMBOL_GPL(dsa_mdb_present_in_other_db);
 
+static const struct dsa_stubs __dsa_stubs = {
+	.master_hwtstamp_validate = __dsa_master_hwtstamp_validate,
+};
+
+static void dsa_register_stubs(void)
+{
+	mutex_lock(&dsa_stubs_lock);
+
+	dsa_stubs = &__dsa_stubs;
+
+	mutex_unlock(&dsa_stubs_lock);
+}
+
+static void dsa_unregister_stubs(void)
+{
+	mutex_lock(&dsa_stubs_lock);
+
+	dsa_stubs = NULL;
+
+	mutex_unlock(&dsa_stubs_lock);
+}
+
 static int __init dsa_init_module(void)
 {
 	int rc;
@@ -1721,6 +1744,8 @@ static int __init dsa_init_module(void)
 	if (rc)
 		goto netlink_register_fail;
 
+	dsa_register_stubs();
+
 	return 0;
 
 netlink_register_fail:
@@ -1735,6 +1760,8 @@ module_init(dsa_init_module);
 
 static void __exit dsa_cleanup_module(void)
 {
+	dsa_unregister_stubs();
+
 	rtnl_link_unregister(&dsa_link_ops);
 
 	dsa_slave_unregister_notifier();
diff --git a/net/dsa/master.c b/net/dsa/master.c
index c2cabe6248b1..6be89ab0cc01 100644
--- a/net/dsa/master.c
+++ b/net/dsa/master.c
@@ -198,7 +198,7 @@ static void dsa_master_get_strings(struct net_device *dev, uint32_t stringset,
 /* Deny PTP operations on master if there is at least one switch in the tree
  * that is PTP capable.
  */
-int dsa_master_pre_change_hwtstamp(struct net_device *dev,
+int __dsa_master_hwtstamp_validate(struct net_device *dev,
 				   const struct kernel_hwtstamp_config *config,
 				   struct netlink_ext_ack *extack)
 {
diff --git a/net/dsa/master.h b/net/dsa/master.h
index 80842f4e27f7..76e39d3ec909 100644
--- a/net/dsa/master.h
+++ b/net/dsa/master.h
@@ -15,7 +15,7 @@ int dsa_master_lag_setup(struct net_device *lag_dev, struct dsa_port *cpu_dp,
 			 struct netlink_ext_ack *extack);
 void dsa_master_lag_teardown(struct net_device *lag_dev,
 			     struct dsa_port *cpu_dp);
-int dsa_master_pre_change_hwtstamp(struct net_device *dev,
+int __dsa_master_hwtstamp_validate(struct net_device *dev,
 				   const struct kernel_hwtstamp_config *config,
 				   struct netlink_ext_ack *extack);
 
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 8abc1658ac47..36bb7533ddd6 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -3419,16 +3419,6 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
 
 		return NOTIFY_OK;
 	}
-	case NETDEV_PRE_CHANGE_HWTSTAMP: {
-		struct netdev_notifier_hwtstamp_info *info = ptr;
-		int err;
-
-		if (!netdev_uses_dsa(dev))
-			return NOTIFY_DONE;
-
-		err = dsa_master_pre_change_hwtstamp(dev, info->config, extack);
-		return notifier_from_errno(err);
-	}
 	default:
 		break;
 	}
diff --git a/net/dsa/stubs.c b/net/dsa/stubs.c
new file mode 100644
index 000000000000..0b91f1d06028
--- /dev/null
+++ b/net/dsa/stubs.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Stubs for DSA functionality called by the core network stack.
+ * These are necessary because CONFIG_NET_DSA can be a module, and built-in
+ * code cannot directly call symbols exported by modules.
+ */
+#include <net/dsa_stubs.h>
+
+DEFINE_MUTEX(dsa_stubs_lock);
+EXPORT_SYMBOL_GPL(dsa_stubs_lock);
+
+const struct dsa_stubs *dsa_stubs;
+EXPORT_SYMBOL_GPL(dsa_stubs);
-- 
2.34.1

Re: [RFC PATCH net-next] net: dsa: replace NETDEV_PRE_CHANGE_HWTSTAMP notifier with a stub

[Vladimir Oltean]

On Wed, Apr 05, 2023 at 07:51:15PM +0300, Vladimir Oltean wrote:
> +#if IS_ENABLED(CONFIG_NET_DSA)
> +
> +static inline int dsa_master_hwtstamp_validate(struct net_device *dev,
> +					       const struct kernel_hwtstamp_config *config,
> +					       struct netlink_ext_ack *extack)
> +{
> +}
> +
> +#else
> +
> +static inline int dsa_master_hwtstamp_validate(struct net_device *dev,
> +					       const struct kernel_hwtstamp_config *config)
> +{
> +	return 0;
> +}
> +
> +#endif

This happened again, I need to do something about it... I ran
format-patch before I added this last hunk to the commit:

diff --git a/include/net/dsa_stubs.h b/include/net/dsa_stubs.h
index 27a1ad85c038..9556f9fb5a86 100644
--- a/include/net/dsa_stubs.h
+++ b/include/net/dsa_stubs.h
@@ -41,7 +41,8 @@ static inline int dsa_master_hwtstamp_validate(struct net_device *dev,
 #else
 
 static inline int dsa_master_hwtstamp_validate(struct net_device *dev,
-					       const struct kernel_hwtstamp_config *config)
+					       const struct kernel_hwtstamp_config *config,
+					       struct netlink_ext_ack *extack)
 {
 	return 0;
 }

so that the build with CONFIG_NET_DSA=n passes too (which I did test).

It's now fixed on my computer, but whoever is curious to test this will
have to fix it up manually; I won't resend the RFC just for this.

Re: [RFC PATCH net-next] net: dsa: replace NETDEV_PRE_CHANGE_HWTSTAMP notifier with a stub

[Simon Horman]

On Wed, Apr 05, 2023 at 07:51:15PM +0300, Vladimir Oltean wrote:
> There was a sort of rush surrounding commit 88c0a6b503b7 ("net: create a
> netdev notifier for DSA to reject PTP on DSA master"), due to a desire
> to convert DSA's attempt to deny TX timestamping on a DSA master to
> something that doesn't block the kernel-wide API conversion from
> ndo_eth_ioctl() to ndo_hwtstamp_set().
> 
> What was required was a mechanism that did not depend on ndo_eth_ioctl(),
> and what was provided was a mechanism that did not depend on
> ndo_eth_ioctl(), while at the same time introducing something that
> wasn't absolutely necessary - a new netdev notifier.
> 
> There have been objections from Jakub Kicinski that using notifiers in
> general when they are not absolutely necessary creates complications to
> the control flow and difficulties to maintainers who look at the code.
> So there is a desire to not use notifiers.
> 
> Take the model of udp_tunnel_nic_ops and introduce a stub mechanism,
> through which net/core/dev_ioctl.c can call into DSA even when
> CONFIG_NET_DSA=m.
> 
> Compared to the code that existed prior to the notifier conversion, aka
> what was added in commits:
> - 4cfab3566710 ("net: dsa: Add wrappers for overloaded ndo_ops")
> - 3369afba1e46 ("net: Call into DSA netdevice_ops wrappers")
> 
> this is different because we are not overloading any struct
> net_device_ops of the DSA master anymore, but rather, we are exposing a
> rather specific functionality which is orthogonal to which API is used
> to enable it - ndo_eth_ioctl() or ndo_hwtstamp_set().
> 
> Also, what is similar is that both approaches use function pointers to
> get from built-in code to DSA.
> 
> Since the new functionality does not overload the NDO of any DSA master,
> there is no point in replicating the function pointers towards
> __dsa_master_hwtstamp_validate() once for every CPU port (dev->dsa_ptr).
> But rather, it is fine to introduce a singleton struct dsa_stubs,
> built-in to the kernel, which contains a single function pointer to
> __dsa_master_hwtstamp_validate().
> 
> I find this approach rather preferable to what we had originally,
> because dev->dsa_ptr->netdev_ops->ndo_do_ioctl() used to require going
> through struct dsa_port (dev->dsa_ptr), and so, this was incompatible
> with any attempts to add any data encapsulation and hide DSA data
> structures from the outside world.
> 
> Link: https://lore.kernel.org/netdev/20230403083019.120b72fd@kernel.org/
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

...

> diff --git a/include/net/dsa_stubs.h b/include/net/dsa_stubs.h
> new file mode 100644
> index 000000000000..27a1ad85c038
> --- /dev/null
> +++ b/include/net/dsa_stubs.h
> @@ -0,0 +1,49 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * include/net/dsa_stubs.h - Stubs for the Distributed Switch Architecture framework
> + */
> +
> +#include <linux/mutex.h>
> +#include <linux/netdevice.h>
> +#include <linux/net_tstamp.h>
> +#include <net/dsa.h>
> +
> +#if IS_ENABLED(CONFIG_NET_DSA)
> +
> +extern const struct dsa_stubs *dsa_stubs;
> +extern struct mutex dsa_stubs_lock;
> +
> +struct dsa_stubs {
> +	int (*master_hwtstamp_validate)(struct net_device *dev,
> +					const struct kernel_hwtstamp_config *config,
> +					struct netlink_ext_ack *extack);
> +};
> +
> +static inline int dsa_master_hwtstamp_validate(struct net_device *dev,
> +					       const struct kernel_hwtstamp_config *config,
> +					       struct netlink_ext_ack *extack)
> +{
> +	int err;
> +
> +	if (!netdev_uses_dsa(dev))
> +		return 0;
> +
> +	mutex_lock(&dsa_stubs_lock);
> +
> +	if (dsa_stubs)
> +		err = dsa_stubs->master_hwtstamp_validate(dev, config, extack);
> +
> +	mutex_unlock(&dsa_stubs_lock);

nit: clang-16 tells me that err is uninitialised here if dsa_stubs is false.

> +
> +	return err;
> +}
> +
> +#else

...

> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 8abc1658ac47..36bb7533ddd6 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -3419,16 +3419,6 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
>  
>  		return NOTIFY_OK;
>  	}
> -	case NETDEV_PRE_CHANGE_HWTSTAMP: {
> -		struct netdev_notifier_hwtstamp_info *info = ptr;
> -		int err;
> -
> -		if (!netdev_uses_dsa(dev))
> -			return NOTIFY_DONE;
> -
> -		err = dsa_master_pre_change_hwtstamp(dev, info->config, extack);

nit: clang-16 tell me that extack is now unused in this function.

> -		return notifier_from_errno(err);
> -	}
>  	default:
>  		break;
>  	}

Re: [RFC PATCH net-next] net: dsa: replace NETDEV_PRE_CHANGE_HWTSTAMP notifier with a stub

[Vladimir Oltean]

On Wed, Apr 05, 2023 at 10:05:15PM +0200, Simon Horman wrote:
> nit: clang-16 tells me that err is uninitialised here if dsa_stubs is false.
> nit: clang-16 tell me that extack is now unused in this function.

Thanks, all good points. Thank clang-16 on my behalf!

Re: [RFC PATCH net-next] net: dsa: replace NETDEV_PRE_CHANGE_HWTSTAMP notifier with a stub

[Vladimir Oltean]

On Wed, Apr 05, 2023 at 11:07:05PM +0300, Vladimir Oltean wrote:
> On Wed, Apr 05, 2023 at 10:05:15PM +0200, Simon Horman wrote:
> > nit: clang-16 tells me that err is uninitialised here if dsa_stubs is false.
> > nit: clang-16 tell me that extack is now unused in this function.
> 
> Thanks, all good points. Thank clang-16 on my behalf!

Perhaps I shouldn't have left it there. I'll start looking into setting
up a toolchain with that for my own builds.

Re: [RFC PATCH net-next] net: dsa: replace NETDEV_PRE_CHANGE_HWTSTAMP notifier with a stub

[Vladimir Oltean]

On Wed, Apr 05, 2023 at 10:05:15PM +0200, Simon Horman wrote:
> > +static inline int dsa_master_hwtstamp_validate(struct net_device *dev,
> > +					       const struct kernel_hwtstamp_config *config,
> > +					       struct netlink_ext_ack *extack)
> > +{
> > +	int err;
> > +
> > +	if (!netdev_uses_dsa(dev))
> > +		return 0;
> > +
> > +	mutex_lock(&dsa_stubs_lock);
> > +
> > +	if (dsa_stubs)
> > +		err = dsa_stubs->master_hwtstamp_validate(dev, config, extack);
> > +
> > +	mutex_unlock(&dsa_stubs_lock);
> 
> nit: clang-16 tells me that err is uninitialised here if dsa_stubs is false.

In fact, clang-16 is saying something much smarter than that, because I
did test this code path and it did work reliably (not like an uninitialized
return value would).

It's saying that when netdev_uses_dsa() returns true, the DSA module has
surely been loaded, so the stubs have surely been registered, so the
mutex_lock() and the check for the NULL quality of dsa_stubs are
completely redundant and can be removed.

> > +
> > +	return err;
> > +}
> > +
> > +#else

Re: [RFC PATCH net-next] net: dsa: replace NETDEV_PRE_CHANGE_HWTSTAMP notifier with a stub

[Jakub Kicinski]

On Wed,  5 Apr 2023 19:51:15 +0300 Vladimir Oltean wrote:
> There have been objections from Jakub Kicinski that using notifiers in
> general when they are not absolutely necessary creates complications to
> the control flow and difficulties to maintainers who look at the code.
> So there is a desire to not use notifiers.

LGTM, thank you!

Re: [RFC PATCH net-next] net: dsa: replace NETDEV_PRE_CHANGE_HWTSTAMP notifier with a stub

[kernel test robot]

Hi Vladimir,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Vladimir-Oltean/net-dsa-replace-NETDEV_PRE_CHANGE_HWTSTAMP-notifier-with-a-stub/20230406-005302
patch link:    https://lore.kernel.org/r/20230405165115.3744445-1-vladimir.oltean%40nxp.com
patch subject: [RFC PATCH net-next] net: dsa: replace NETDEV_PRE_CHANGE_HWTSTAMP notifier with a stub
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20230406/202304061008.Hep5lPGA-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/4788a6c004ab7f7210b1d27777e733eab85db17e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Vladimir-Oltean/net-dsa-replace-NETDEV_PRE_CHANGE_HWTSTAMP-notifier-with-a-stub/20230406-005302
        git checkout 4788a6c004ab7f7210b1d27777e733eab85db17e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 olddefconfig
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/net/ethernet/google/gve/ net/dsa/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304061008.Hep5lPGA-lkp@intel.com/

All warnings (new ones prefixed by >>):

   net/dsa/slave.c: In function 'dsa_slave_netdevice_event':
>> net/dsa/slave.c:3292:33: warning: unused variable 'extack' [-Wunused-variable]
    3292 |         struct netlink_ext_ack *extack = netdev_notifier_info_to_extack(ptr);
         |                                 ^~~~~~


vim +/extack +3292 net/dsa/slave.c

acc43b7bf52a01 Vladimir Oltean   2022-09-11  3288  
4ede74e73b5b54 Vladimir Oltean   2021-06-27  3289  static int dsa_slave_netdevice_event(struct notifier_block *nb,
4ede74e73b5b54 Vladimir Oltean   2021-06-27  3290  				     unsigned long event, void *ptr)
4ede74e73b5b54 Vladimir Oltean   2021-06-27  3291  {
88c0a6b503b7f4 Vladimir Oltean   2023-04-02 @3292  	struct netlink_ext_ack *extack = netdev_notifier_info_to_extack(ptr);
4ede74e73b5b54 Vladimir Oltean   2021-06-27  3293  	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
4ede74e73b5b54 Vladimir Oltean   2021-06-27  3294  
4ede74e73b5b54 Vladimir Oltean   2021-06-27  3295  	switch (event) {
4ede74e73b5b54 Vladimir Oltean   2021-06-27  3296  	case NETDEV_PRECHANGEUPPER: {
4ede74e73b5b54 Vladimir Oltean   2021-06-27  3297  		struct netdev_notifier_changeupper_info *info = ptr;
4ede74e73b5b54 Vladimir Oltean   2021-06-27  3298  		int err;
4ede74e73b5b54 Vladimir Oltean   2021-06-27  3299  
4ede74e73b5b54 Vladimir Oltean   2021-06-27  3300  		err = dsa_slave_prechangeupper_sanity_check(dev, info);
0498277ee17bb4 Vladimir Oltean   2022-08-19  3301  		if (notifier_to_errno(err))
4ede74e73b5b54 Vladimir Oltean   2021-06-27  3302  			return err;
4ede74e73b5b54 Vladimir Oltean   2021-06-27  3303  
4f03dcc6b9a073 Vladimir Oltean   2022-08-19  3304  		err = dsa_master_prechangeupper_sanity_check(dev, info);
4f03dcc6b9a073 Vladimir Oltean   2022-08-19  3305  		if (notifier_to_errno(err))
4f03dcc6b9a073 Vladimir Oltean   2022-08-19  3306  			return err;
4f03dcc6b9a073 Vladimir Oltean   2022-08-19  3307  
acc43b7bf52a01 Vladimir Oltean   2022-09-11  3308  		err = dsa_lag_master_prechangelower_sanity_check(dev, info);
acc43b7bf52a01 Vladimir Oltean   2022-09-11  3309  		if (notifier_to_errno(err))
acc43b7bf52a01 Vladimir Oltean   2022-09-11  3310  			return err;
acc43b7bf52a01 Vladimir Oltean   2022-09-11  3311  
920a33cd72314f Vladimir Oltean   2022-08-19  3312  		err = dsa_bridge_prechangelower_sanity_check(dev, info);
920a33cd72314f Vladimir Oltean   2022-08-19  3313  		if (notifier_to_errno(err))
920a33cd72314f Vladimir Oltean   2022-08-19  3314  			return err;
920a33cd72314f Vladimir Oltean   2022-08-19  3315  
4c3f80d22b2eca Vladimir Oltean   2022-08-19  3316  		err = dsa_slave_prechangeupper(dev, ptr);
4c3f80d22b2eca Vladimir Oltean   2022-08-19  3317  		if (notifier_to_errno(err))
4c3f80d22b2eca Vladimir Oltean   2022-08-19  3318  			return err;
7491894532341c Vladimir Oltean   2021-06-27  3319  
4c3f80d22b2eca Vladimir Oltean   2022-08-19  3320  		err = dsa_slave_lag_prechangeupper(dev, ptr);
4c3f80d22b2eca Vladimir Oltean   2022-08-19  3321  		if (notifier_to_errno(err))
4c3f80d22b2eca Vladimir Oltean   2022-08-19  3322  			return err;
7491894532341c Vladimir Oltean   2021-06-27  3323  
8350129930d2d7 Vladimir Oltean   2020-09-21  3324  		break;
2b13840672340e Vladimir Oltean   2020-09-21  3325  	}
4c3f80d22b2eca Vladimir Oltean   2022-08-19  3326  	case NETDEV_CHANGEUPPER: {
4c3f80d22b2eca Vladimir Oltean   2022-08-19  3327  		int err;
4c3f80d22b2eca Vladimir Oltean   2022-08-19  3328  
4c3f80d22b2eca Vladimir Oltean   2022-08-19  3329  		err = dsa_slave_changeupper(dev, ptr);
4c3f80d22b2eca Vladimir Oltean   2022-08-19  3330  		if (notifier_to_errno(err))
4c3f80d22b2eca Vladimir Oltean   2022-08-19  3331  			return err;
058102a6e9eb19 Tobias Waldekranz 2021-01-13  3332  
4c3f80d22b2eca Vladimir Oltean   2022-08-19  3333  		err = dsa_slave_lag_changeupper(dev, ptr);
4c3f80d22b2eca Vladimir Oltean   2022-08-19  3334  		if (notifier_to_errno(err))
4c3f80d22b2eca Vladimir Oltean   2022-08-19  3335  			return err;
058102a6e9eb19 Tobias Waldekranz 2021-01-13  3336  
acc43b7bf52a01 Vladimir Oltean   2022-09-11  3337  		err = dsa_master_changeupper(dev, ptr);
acc43b7bf52a01 Vladimir Oltean   2022-09-11  3338  		if (notifier_to_errno(err))
acc43b7bf52a01 Vladimir Oltean   2022-09-11  3339  			return err;
acc43b7bf52a01 Vladimir Oltean   2022-09-11  3340  
058102a6e9eb19 Tobias Waldekranz 2021-01-13  3341  		break;
4c3f80d22b2eca Vladimir Oltean   2022-08-19  3342  	}
058102a6e9eb19 Tobias Waldekranz 2021-01-13  3343  	case NETDEV_CHANGELOWERSTATE: {
058102a6e9eb19 Tobias Waldekranz 2021-01-13  3344  		struct netdev_notifier_changelowerstate_info *info = ptr;
058102a6e9eb19 Tobias Waldekranz 2021-01-13  3345  		struct dsa_port *dp;
0a6d58a70a39d9 Dan Carpenter     2022-10-14  3346  		int err = 0;
058102a6e9eb19 Tobias Waldekranz 2021-01-13  3347  
acc43b7bf52a01 Vladimir Oltean   2022-09-11  3348  		if (dsa_slave_dev_check(dev)) {
058102a6e9eb19 Tobias Waldekranz 2021-01-13  3349  			dp = dsa_slave_to_port(dev);
058102a6e9eb19 Tobias Waldekranz 2021-01-13  3350  
058102a6e9eb19 Tobias Waldekranz 2021-01-13  3351  			err = dsa_port_lag_change(dp, info->lower_state_info);
acc43b7bf52a01 Vladimir Oltean   2022-09-11  3352  		}
acc43b7bf52a01 Vladimir Oltean   2022-09-11  3353  
acc43b7bf52a01 Vladimir Oltean   2022-09-11  3354  		/* Mirror LAG port events on DSA masters that are in
acc43b7bf52a01 Vladimir Oltean   2022-09-11  3355  		 * a LAG towards their respective switch CPU ports
acc43b7bf52a01 Vladimir Oltean   2022-09-11  3356  		 */
acc43b7bf52a01 Vladimir Oltean   2022-09-11  3357  		if (netdev_uses_dsa(dev)) {
acc43b7bf52a01 Vladimir Oltean   2022-09-11  3358  			dp = dev->dsa_ptr;
acc43b7bf52a01 Vladimir Oltean   2022-09-11  3359  
acc43b7bf52a01 Vladimir Oltean   2022-09-11  3360  			err = dsa_port_lag_change(dp, info->lower_state_info);
acc43b7bf52a01 Vladimir Oltean   2022-09-11  3361  		}
acc43b7bf52a01 Vladimir Oltean   2022-09-11  3362  
058102a6e9eb19 Tobias Waldekranz 2021-01-13  3363  		return notifier_from_errno(err);
058102a6e9eb19 Tobias Waldekranz 2021-01-13  3364  	}
295ab96f478d0f Vladimir Oltean   2022-02-02  3365  	case NETDEV_CHANGE:
295ab96f478d0f Vladimir Oltean   2022-02-02  3366  	case NETDEV_UP: {
295ab96f478d0f Vladimir Oltean   2022-02-02  3367  		/* Track state of master port.
295ab96f478d0f Vladimir Oltean   2022-02-02  3368  		 * DSA driver may require the master port (and indirectly
295ab96f478d0f Vladimir Oltean   2022-02-02  3369  		 * the tagger) to be available for some special operation.
295ab96f478d0f Vladimir Oltean   2022-02-02  3370  		 */
295ab96f478d0f Vladimir Oltean   2022-02-02  3371  		if (netdev_uses_dsa(dev)) {
295ab96f478d0f Vladimir Oltean   2022-02-02  3372  			struct dsa_port *cpu_dp = dev->dsa_ptr;
295ab96f478d0f Vladimir Oltean   2022-02-02  3373  			struct dsa_switch_tree *dst = cpu_dp->ds->dst;
295ab96f478d0f Vladimir Oltean   2022-02-02  3374  
295ab96f478d0f Vladimir Oltean   2022-02-02  3375  			/* Track when the master port is UP */
295ab96f478d0f Vladimir Oltean   2022-02-02  3376  			dsa_tree_master_oper_state_change(dst, dev,
295ab96f478d0f Vladimir Oltean   2022-02-02  3377  							  netif_oper_up(dev));
295ab96f478d0f Vladimir Oltean   2022-02-02  3378  
295ab96f478d0f Vladimir Oltean   2022-02-02  3379  			/* Track when the master port is ready and can accept
295ab96f478d0f Vladimir Oltean   2022-02-02  3380  			 * packet.
295ab96f478d0f Vladimir Oltean   2022-02-02  3381  			 * NETDEV_UP event is not enough to flag a port as ready.
295ab96f478d0f Vladimir Oltean   2022-02-02  3382  			 * We also have to wait for linkwatch_do_dev to dev_activate
295ab96f478d0f Vladimir Oltean   2022-02-02  3383  			 * and emit a NETDEV_CHANGE event.
295ab96f478d0f Vladimir Oltean   2022-02-02  3384  			 * We check if a master port is ready by checking if the dev
295ab96f478d0f Vladimir Oltean   2022-02-02  3385  			 * have a qdisc assigned and is not noop.
295ab96f478d0f Vladimir Oltean   2022-02-02  3386  			 */
295ab96f478d0f Vladimir Oltean   2022-02-02  3387  			dsa_tree_master_admin_state_change(dst, dev,
295ab96f478d0f Vladimir Oltean   2022-02-02  3388  							   !qdisc_tx_is_noop(dev));
295ab96f478d0f Vladimir Oltean   2022-02-02  3389  
295ab96f478d0f Vladimir Oltean   2022-02-02  3390  			return NOTIFY_OK;
295ab96f478d0f Vladimir Oltean   2022-02-02  3391  		}
295ab96f478d0f Vladimir Oltean   2022-02-02  3392  
295ab96f478d0f Vladimir Oltean   2022-02-02  3393  		return NOTIFY_DONE;
295ab96f478d0f Vladimir Oltean   2022-02-02  3394  	}
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3395  	case NETDEV_GOING_DOWN: {
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3396  		struct dsa_port *dp, *cpu_dp;
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3397  		struct dsa_switch_tree *dst;
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3398  		LIST_HEAD(close_list);
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3399  
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3400  		if (!netdev_uses_dsa(dev))
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3401  			return NOTIFY_DONE;
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3402  
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3403  		cpu_dp = dev->dsa_ptr;
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3404  		dst = cpu_dp->ds->dst;
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3405  
295ab96f478d0f Vladimir Oltean   2022-02-02  3406  		dsa_tree_master_admin_state_change(dst, dev, false);
295ab96f478d0f Vladimir Oltean   2022-02-02  3407  
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3408  		list_for_each_entry(dp, &dst->ports, list) {
d0004a020bb502 Vladimir Oltean   2021-10-20  3409  			if (!dsa_port_is_user(dp))
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3410  				continue;
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3411  
7136097e119964 Vladimir Oltean   2022-08-19  3412  			if (dp->cpu_dp != cpu_dp)
7136097e119964 Vladimir Oltean   2022-08-19  3413  				continue;
7136097e119964 Vladimir Oltean   2022-08-19  3414  
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3415  			list_add(&dp->slave->close_list, &close_list);
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3416  		}
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3417  
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3418  		dev_close_many(&close_list, true);
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3419  
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3420  		return NOTIFY_OK;
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3421  	}
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3422  	default:
c0a8a9c2749365 Vladimir Oltean   2021-02-05  3423  		break;
cc1d5bda17c8eb Florian Fainelli  2019-02-20  3424  	}
6debb68a2d1acd Vivien Didelot    2016-03-13  3425  
b73adef67765b7 Florian Fainelli  2015-02-24  3426  	return NOTIFY_DONE;
b73adef67765b7 Florian Fainelli  2015-02-24  3427  }
88e4f0ca4e4e77 Vivien Didelot    2017-02-03  3428  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [RFC PATCH net-next] net: dsa: replace NETDEV_PRE_CHANGE_HWTSTAMP notifier with a stub

[kernel test robot]

Hi Vladimir,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Vladimir-Oltean/net-dsa-replace-NETDEV_PRE_CHANGE_HWTSTAMP-notifier-with-a-stub/20230406-005302
patch link:    https://lore.kernel.org/r/20230405165115.3744445-1-vladimir.oltean%40nxp.com
patch subject: [RFC PATCH net-next] net: dsa: replace NETDEV_PRE_CHANGE_HWTSTAMP notifier with a stub
config: arm-randconfig-r023-20230403 (https://download.01.org/0day-ci/archive/20230406/202304061058.ZYSqgL8r-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/intel-lab-lkp/linux/commit/4788a6c004ab7f7210b1d27777e733eab85db17e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Vladimir-Oltean/net-dsa-replace-NETDEV_PRE_CHANGE_HWTSTAMP-notifier-with-a-stub/20230406-005302
        git checkout 4788a6c004ab7f7210b1d27777e733eab85db17e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash net/dsa/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304061058.ZYSqgL8r-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from net/dsa/stubs.c:7:
>> include/net/dsa_stubs.h:33:2: warning: variable 'err' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
           if (dsa_stubs)
           ^~~~~~~~~~~~~~
   include/linux/compiler.h:56:28: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:58:30: note: expanded from macro '__trace_if_var'
   #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/net/dsa_stubs.h:38:9: note: uninitialized use occurs here
           return err;
                  ^~~
   include/net/dsa_stubs.h:33:2: note: remove the 'if' if its condition is always true
           if (dsa_stubs)
           ^~~~~~~~~~~~~~
   include/linux/compiler.h:56:23: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                         ^
   include/net/dsa_stubs.h:26:9: note: initialize the variable 'err' to silence this warning
           int err;
                  ^
                   = 0
   1 warning generated.


vim +33 include/net/dsa_stubs.h

    21	
    22	static inline int dsa_master_hwtstamp_validate(struct net_device *dev,
    23						       const struct kernel_hwtstamp_config *config,
    24						       struct netlink_ext_ack *extack)
    25	{
    26		int err;
    27	
    28		if (!netdev_uses_dsa(dev))
    29			return 0;
    30	
    31		mutex_lock(&dsa_stubs_lock);
    32	
  > 33		if (dsa_stubs)
    34			err = dsa_stubs->master_hwtstamp_validate(dev, config, extack);
    35	
    36		mutex_unlock(&dsa_stubs_lock);
    37	
    38		return err;
    39	}
    40	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [RFC PATCH net-next] net: dsa: replace NETDEV_PRE_CHANGE_HWTSTAMP notifier with a stub

[kernel test robot]

Hi Vladimir,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Vladimir-Oltean/net-dsa-replace-NETDEV_PRE_CHANGE_HWTSTAMP-notifier-with-a-stub/20230406-005302
patch link:    https://lore.kernel.org/r/20230405165115.3744445-1-vladimir.oltean%40nxp.com
patch subject: [RFC PATCH net-next] net: dsa: replace NETDEV_PRE_CHANGE_HWTSTAMP notifier with a stub
config: mips-bcm63xx_defconfig (https://download.01.org/0day-ci/archive/20230406/202304061254.dcuhkOKf-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mips-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/4788a6c004ab7f7210b1d27777e733eab85db17e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Vladimir-Oltean/net-dsa-replace-NETDEV_PRE_CHANGE_HWTSTAMP-notifier-with-a-stub/20230406-005302
        git checkout 4788a6c004ab7f7210b1d27777e733eab85db17e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash fs// net/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304061254.dcuhkOKf-lkp@intel.com/

All errors (new ones prefixed by >>):

>> net/core/dev_ioctl.c:276:55: error: too many arguments to function call, expected 2, have 3
           err = dsa_master_hwtstamp_validate(dev, &kernel_cfg, &extack);
                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~                   ^~~~~~~
   include/net/dsa_stubs.h:43:19: note: 'dsa_master_hwtstamp_validate' declared here
   static inline int dsa_master_hwtstamp_validate(struct net_device *dev,
                     ^
   1 error generated.


vim +276 net/core/dev_ioctl.c

   259	
   260	static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
   261	{
   262		struct kernel_hwtstamp_config kernel_cfg;
   263		struct netlink_ext_ack extack = {};
   264		struct hwtstamp_config cfg;
   265		int err;
   266	
   267		if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
   268			return -EFAULT;
   269	
   270		hwtstamp_config_to_kernel(&kernel_cfg, &cfg);
   271	
   272		err = net_hwtstamp_validate(&kernel_cfg);
   273		if (err)
   274			return err;
   275	
 > 276		err = dsa_master_hwtstamp_validate(dev, &kernel_cfg, &extack);
   277		if (err) {
   278			if (extack._msg)
   279				netdev_err(dev, "%s\n", extack._msg);
   280			return err;
   281		}
   282	
   283		return dev_eth_ioctl(dev, ifr, SIOCSHWTSTAMP);
   284	}
   285	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [RFC PATCH net-next] net: dsa: replace NETDEV_PRE_CHANGE_HWTSTAMP notifier with a stub

[kernel test robot]

Hi Vladimir,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Vladimir-Oltean/net-dsa-replace-NETDEV_PRE_CHANGE_HWTSTAMP-notifier-with-a-stub/20230406-005302
patch link:    https://lore.kernel.org/r/20230405165115.3744445-1-vladimir.oltean%40nxp.com
patch subject: [RFC PATCH net-next] net: dsa: replace NETDEV_PRE_CHANGE_HWTSTAMP notifier with a stub
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20230406/202304061324.mnmSnwNK-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/4788a6c004ab7f7210b1d27777e733eab85db17e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Vladimir-Oltean/net-dsa-replace-NETDEV_PRE_CHANGE_HWTSTAMP-notifier-with-a-stub/20230406-005302
        git checkout 4788a6c004ab7f7210b1d27777e733eab85db17e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash fs// net/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304061324.mnmSnwNK-lkp@intel.com/

All errors (new ones prefixed by >>):

   net/core/dev_ioctl.c: In function 'dev_set_hwtstamp':
>> net/core/dev_ioctl.c:276:15: error: too many arguments to function 'dsa_master_hwtstamp_validate'
     276 |         err = dsa_master_hwtstamp_validate(dev, &kernel_cfg, &extack);
         |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from net/core/dev_ioctl.c:10:
   include/net/dsa_stubs.h:43:19: note: declared here
      43 | static inline int dsa_master_hwtstamp_validate(struct net_device *dev,
         |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/dsa_master_hwtstamp_validate +276 net/core/dev_ioctl.c

   259	
   260	static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
   261	{
   262		struct kernel_hwtstamp_config kernel_cfg;
   263		struct netlink_ext_ack extack = {};
   264		struct hwtstamp_config cfg;
   265		int err;
   266	
   267		if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
   268			return -EFAULT;
   269	
   270		hwtstamp_config_to_kernel(&kernel_cfg, &cfg);
   271	
   272		err = net_hwtstamp_validate(&kernel_cfg);
   273		if (err)
   274			return err;
   275	
 > 276		err = dsa_master_hwtstamp_validate(dev, &kernel_cfg, &extack);
   277		if (err) {
   278			if (extack._msg)
   279				netdev_err(dev, "%s\n", extack._msg);
   280			return err;
   281		}
   282	
   283		return dev_eth_ioctl(dev, ifr, SIOCSHWTSTAMP);
   284	}
   285	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [RFC PATCH net-next] net: dsa: replace NETDEV_PRE_CHANGE_HWTSTAMP notifier with a stub

[Simon Horman]

On Wed, Apr 05, 2023 at 11:08:52PM +0300, Vladimir Oltean wrote:
> On Wed, Apr 05, 2023 at 11:07:05PM +0300, Vladimir Oltean wrote:
> > On Wed, Apr 05, 2023 at 10:05:15PM +0200, Simon Horman wrote:
> > > nit: clang-16 tells me that err is uninitialised here if dsa_stubs is false.
> > > nit: clang-16 tell me that extack is now unused in this function.
> > 
> > Thanks, all good points. Thank clang-16 on my behalf!
> 
> Perhaps I shouldn't have left it there. I'll start looking into setting
> up a toolchain with that for my own builds.

I did this a few weeks back using the toolchain at the link below.
It was quite a simple process. And well worth the effort.

https://mirrors.edge.kernel.org/pub/tools/llvm/

Re: [RFC PATCH net-next] net: dsa: replace NETDEV_PRE_CHANGE_HWTSTAMP notifier with a stub

[Simon Horman]

On Wed, Apr 05, 2023 at 11:31:07PM +0300, Vladimir Oltean wrote:
> On Wed, Apr 05, 2023 at 10:05:15PM +0200, Simon Horman wrote:
> > > +static inline int dsa_master_hwtstamp_validate(struct net_device *dev,
> > > +					       const struct kernel_hwtstamp_config *config,
> > > +					       struct netlink_ext_ack *extack)
> > > +{
> > > +	int err;
> > > +
> > > +	if (!netdev_uses_dsa(dev))
> > > +		return 0;
> > > +
> > > +	mutex_lock(&dsa_stubs_lock);
> > > +
> > > +	if (dsa_stubs)
> > > +		err = dsa_stubs->master_hwtstamp_validate(dev, config, extack);
> > > +
> > > +	mutex_unlock(&dsa_stubs_lock);
> > 
> > nit: clang-16 tells me that err is uninitialised here if dsa_stubs is false.
> 
> In fact, clang-16 is saying something much smarter than that, because I
> did test this code path and it did work reliably (not like an uninitialized
> return value would).
> 
> It's saying that when netdev_uses_dsa() returns true, the DSA module has
> surely been loaded, so the stubs have surely been registered, so the
> mutex_lock() and the check for the NULL quality of dsa_stubs are
> completely redundant and can be removed.

For the record, what it had to say, when used with W=1, was:

In file included from net/dsa/dsa.c:20:
./include/net/dsa_stubs.h:33:6: error: variable 'err' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
        if (dsa_stubs)
            ^~~~~~~~~
./include/net/dsa_stubs.h:38:9: note: uninitialized use occurs here
        return err;
               ^~~
./include/net/dsa_stubs.h:33:2: note: remove the 'if' if its condition is always true
        if (dsa_stubs)
        ^~~~~~~~~~~~~~
./include/net/dsa_stubs.h:26:9: note: initialize the variable 'err' to silence this warning
        int err;
               ^
                = 0
1 error generated.

--

In file included from net/dsa/stubs.c:7:
./include/net/dsa_stubs.h:33:6: error: variable 'err' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
        if (dsa_stubs)
            ^~~~~~~~~
./include/net/dsa_stubs.h:38:9: note: uninitialized use occurs here
        return err;
               ^~~
./include/net/dsa_stubs.h:33:2: note: remove the 'if' if its condition is always true
        if (dsa_stubs)
        ^~~~~~~~~~~~~~
./include/net/dsa_stubs.h:26:9: note: initialize the variable 'err' to silence this warning
        int err;
               ^
                = 0
1 error generated.

Re: [RFC PATCH net-next] net: dsa: replace NETDEV_PRE_CHANGE_HWTSTAMP notifier with a stub

[Vladimir Oltean]

On Thu, Apr 06, 2023 at 09:07:11AM +0200, Simon Horman wrote:
> On Wed, Apr 05, 2023 at 11:08:52PM +0300, Vladimir Oltean wrote:
> > On Wed, Apr 05, 2023 at 11:07:05PM +0300, Vladimir Oltean wrote:
> > > On Wed, Apr 05, 2023 at 10:05:15PM +0200, Simon Horman wrote:
> > > > nit: clang-16 tells me that err is uninitialised here if dsa_stubs is false.
> > > > nit: clang-16 tell me that extack is now unused in this function.
> > > 
> > > Thanks, all good points. Thank clang-16 on my behalf!
> > 
> > Perhaps I shouldn't have left it there. I'll start looking into setting
> > up a toolchain with that for my own builds.
> 
> I did this a few weeks back using the toolchain at the link below.
> It was quite a simple process. And well worth the effort.
> 
> https://mirrors.edge.kernel.org/pub/tools/llvm/

Thanks, I did set this up yesterday, using Documentation/kbuild/llvm.rst
as a guide.