[PATCH net-next 0/4] net: dsa: Setup dsa_netdev_ops

[PATCH net-next 0/4] net: dsa: Setup dsa_netdev_ops

[Florian Fainelli]

Hi David, Jakub,

This patch series addresses the overloading of a DSA CPU/management
interface's netdev_ops for the purpose of providing useful information
from the switch side.

Up until now we had duplicated the existing netdev_ops structure and
added specific function pointers to return information of interest. Here
we have a more controlled way of doing this by involving the specific
netdev_ops function pointers that we want to be patched, which is easier
for auditing code in the future. As a byproduct we can now maintain
netdev_ops pointer comparisons which would be failing before (no known
in tree problems because of that though).

Let me know if this approach looks reasonable to you and we might do the
same with our ethtool_ops overloading as well.

Thanks!

Florian Fainelli (4):
  net: Wrap ndo_do_ioctl() to prepare for DSA stacked ops
  net: dsa: Add wrappers for overloaded ndo_ops
  net: Call into DSA netdevice_ops wrappers
  net: dsa: Setup dsa_netdev_ops

 include/net/dsa.h    | 42 ++++++++++++++++++++++++++++++++++-
 net/core/dev.c       |  5 +++++
 net/core/dev_ioctl.c | 29 ++++++++++++++++++------
 net/dsa/master.c     | 52 +++++++++++---------------------------------
 4 files changed, 81 insertions(+), 47 deletions(-)

-- 
2.25.1

[PATCH net-next 1/4] net: Wrap ndo_do_ioctl() to prepare for DSA stacked ops

[Florian Fainelli]

In preparation for adding another layer of call into a DSA stacked ops
singleton, wrap the ndo_do_ioctl() call into dev_do_ioctl().

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/core/dev_ioctl.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 547b587c1950..a213c703c90a 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -225,6 +225,22 @@ static int net_hwtstamp_validate(struct ifreq *ifr)
 	return 0;
 }
 
+static int dev_do_ioctl(struct net_device *dev,
+			struct ifreq *ifr, unsigned int cmd)
+{
+	const struct net_device_ops *ops = dev->netdev_ops;
+	int err = -EOPNOTSUPP;
+
+	if (ops->ndo_do_ioctl) {
+		if (netif_device_present(dev))
+			err = ops->ndo_do_ioctl(dev, ifr, cmd);
+		else
+			err = -ENODEV;
+	}
+
+	return err;
+}
+
 /*
  *	Perform the SIOCxIFxxx calls, inside rtnl_lock()
  */
@@ -323,13 +339,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
 		    cmd == SIOCSHWTSTAMP ||
 		    cmd == SIOCGHWTSTAMP ||
 		    cmd == SIOCWANDEV) {
-			err = -EOPNOTSUPP;
-			if (ops->ndo_do_ioctl) {
-				if (netif_device_present(dev))
-					err = ops->ndo_do_ioctl(dev, ifr, cmd);
-				else
-					err = -ENODEV;
-			}
+			err = dev_do_ioctl(dev, ifr, cmd);
 		} else
 			err = -EINVAL;
 
-- 
2.25.1

[PATCH net-next 2/4] net: dsa: Add wrappers for overloaded ndo_ops

[Florian Fainelli]

Add definitions for the dsa_netdevice_ops structure which is a subset of
the net_device_ops structure for the specific operations that we care
about overlaying on top of the DSA CPU port net_device and provide
inline stubs that take core managing whether DSA code is reachable.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/net/dsa.h | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 6fa418ff1175..681ba2752514 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -86,6 +86,18 @@ struct dsa_device_ops {
 	enum dsa_tag_protocol proto;
 };
 
+/* This structure defines the control interfaces that are overlayed by the
+ * DSA layer on top of the DSA CPU/management net_device instance. This is
+ * used by the core net_device layer while calling various net_device_ops
+ * function pointers.
+ */
+struct dsa_netdevice_ops {
+	int (*ndo_do_ioctl)(struct net_device *dev, struct ifreq *ifr,
+			    int cmd);
+	int (*ndo_get_phys_port_name)(struct net_device *dev, char *name,
+				      size_t len);
+};
+
 #define DSA_TAG_DRIVER_ALIAS "dsa_tag-"
 #define MODULE_ALIAS_DSA_TAG_DRIVER(__proto)				\
 	MODULE_ALIAS(DSA_TAG_DRIVER_ALIAS __stringify(__proto##_VALUE))
@@ -217,6 +229,7 @@ struct dsa_port {
 	/*
 	 * Original copy of the master netdev net_device_ops
 	 */
+	const struct dsa_netdevice_ops *netdev_ops;
 	const struct net_device_ops *orig_ndo_ops;
 
 	bool setup;
@@ -679,6 +692,34 @@ static inline bool dsa_can_decode(const struct sk_buff *skb,
 	return false;
 }
 
+#if IS_ENABLED(CONFIG_NET_DSA)
+#define dsa_build_ndo_op(name, arg1_type, arg1_name, arg2_type, arg2_name) \
+static int inline dsa_##name(struct net_device *dev, arg1_type arg1_name, \
+			     arg2_type arg2_name)	\
+{							\
+	const struct dsa_netdevice_ops *ops;		\
+	int err = -EOPNOTSUPP;				\
+							\
+	if (!dev->dsa_ptr)				\
+		return err;				\
+							\
+	ops = dev->dsa_ptr->netdev_ops;			\
+	if (!ops || !ops->name)				\
+		return err;				\
+							\
+	return ops->name(dev, arg1_name, arg2_name);	\
+}
+#else
+#define dsa_build_ndo_op(name, ...)			\
+static inline int dsa_##name(struct net_device *dev, ...) \
+{							\
+	return -EOPNOTSUPP;				\
+}
+#endif
+
+dsa_build_ndo_op(ndo_do_ioctl, struct ifreq *, ifr, int, cmd);
+dsa_build_ndo_op(ndo_get_phys_port_name, char *, name, size_t, len);
+
 void dsa_unregister_switch(struct dsa_switch *ds);
 int dsa_register_switch(struct dsa_switch *ds);
 struct dsa_switch *dsa_switch_find(int tree_index, int sw_index);
-- 
2.25.1

[PATCH net-next 3/4] net: Call into DSA netdevice_ops wrappers

[Florian Fainelli]

Make the core net_device code call into our ndo_do_ioctl() and
ndo_get_phys_port_name() functions via the wrappers defined previously

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/core/dev.c       | 5 +++++
 net/core/dev_ioctl.c | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 062a00fdca9b..19f1abc26fcd 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -98,6 +98,7 @@
 #include <net/busy_poll.h>
 #include <linux/rtnetlink.h>
 #include <linux/stat.h>
+#include <net/dsa.h>
 #include <net/dst.h>
 #include <net/dst_metadata.h>
 #include <net/pkt_sched.h>
@@ -8602,6 +8603,10 @@ int dev_get_phys_port_name(struct net_device *dev,
 	const struct net_device_ops *ops = dev->netdev_ops;
 	int err;
 
+	err  = dsa_ndo_get_phys_port_name(dev, name, len);
+	if (err == 0 || err != -EOPNOTSUPP)
+		return err;
+
 	if (ops->ndo_get_phys_port_name) {
 		err = ops->ndo_get_phys_port_name(dev, name, len);
 		if (err != -EOPNOTSUPP)
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index a213c703c90a..b2cf9b7bb7b8 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -5,6 +5,7 @@
 #include <linux/rtnetlink.h>
 #include <linux/net_tstamp.h>
 #include <linux/wireless.h>
+#include <net/dsa.h>
 #include <net/wext.h>
 
 /*
@@ -231,6 +232,10 @@ static int dev_do_ioctl(struct net_device *dev,
 	const struct net_device_ops *ops = dev->netdev_ops;
 	int err = -EOPNOTSUPP;
 
+	err = dsa_ndo_do_ioctl(dev, ifr, cmd);
+	if (err == 0 || err != -EOPNOTSUPP)
+		return err;
+
 	if (ops->ndo_do_ioctl) {
 		if (netif_device_present(dev))
 			err = ops->ndo_do_ioctl(dev, ifr, cmd);
-- 
2.25.1

[PATCH net-next 4/4] net: dsa: Setup dsa_netdev_ops

[Florian Fainelli]

Now that we hav all the infrastructure in place for calling into the
dsa_ptr->netdev_ops function pointers, install them when we configure
the DSA CPU/management interface and tear them down. The flow is
unchanged from before, but now we preserve equality of tests when
network device drivers do tests like dev->netdev_ops == &foo_ops which
was not the case before since we were allocating an entirely new
structure.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/net/dsa.h |  1 -
 net/dsa/master.c  | 52 ++++++++++++-----------------------------------
 2 files changed, 13 insertions(+), 40 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 681ba2752514..c9f350303947 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -230,7 +230,6 @@ struct dsa_port {
 	 * Original copy of the master netdev net_device_ops
 	 */
 	const struct dsa_netdevice_ops *netdev_ops;
-	const struct net_device_ops *orig_ndo_ops;
 
 	bool setup;
 };
diff --git a/net/dsa/master.c b/net/dsa/master.c
index 480a61460c23..0a90911ae31b 100644
--- a/net/dsa/master.c
+++ b/net/dsa/master.c
@@ -220,12 +220,17 @@ static int dsa_master_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 		break;
 	}
 
-	if (cpu_dp->orig_ndo_ops && cpu_dp->orig_ndo_ops->ndo_do_ioctl)
-		err = cpu_dp->orig_ndo_ops->ndo_do_ioctl(dev, ifr, cmd);
+	if (dev->netdev_ops->ndo_do_ioctl)
+		err = dev->netdev_ops->ndo_do_ioctl(dev, ifr, cmd);
 
 	return err;
 }
 
+static const struct dsa_netdevice_ops dsa_netdev_ops = {
+	.ndo_do_ioctl = dsa_master_ioctl,
+	.ndo_get_phys_port_name = dsa_master_get_phys_port_name,
+};
+
 static int dsa_master_ethtool_setup(struct net_device *dev)
 {
 	struct dsa_port *cpu_dp = dev->dsa_ptr;
@@ -260,38 +265,10 @@ static void dsa_master_ethtool_teardown(struct net_device *dev)
 	cpu_dp->orig_ethtool_ops = NULL;
 }
 
-static int dsa_master_ndo_setup(struct net_device *dev)
-{
-	struct dsa_port *cpu_dp = dev->dsa_ptr;
-	struct dsa_switch *ds = cpu_dp->ds;
-	struct net_device_ops *ops;
-
-	if (dev->netdev_ops->ndo_get_phys_port_name)
-		return 0;
-
-	ops = devm_kzalloc(ds->dev, sizeof(*ops), GFP_KERNEL);
-	if (!ops)
-		return -ENOMEM;
-
-	cpu_dp->orig_ndo_ops = dev->netdev_ops;
-	if (cpu_dp->orig_ndo_ops)
-		memcpy(ops, cpu_dp->orig_ndo_ops, sizeof(*ops));
-
-	ops->ndo_get_phys_port_name = dsa_master_get_phys_port_name;
-	ops->ndo_do_ioctl = dsa_master_ioctl;
-
-	dev->netdev_ops  = ops;
-
-	return 0;
-}
-
-static void dsa_master_ndo_teardown(struct net_device *dev)
+static void dsa_netdev_ops_set(struct net_device *dev,
+			       const struct dsa_netdevice_ops *ops)
 {
-	struct dsa_port *cpu_dp = dev->dsa_ptr;
-
-	if (cpu_dp->orig_ndo_ops)
-		dev->netdev_ops = cpu_dp->orig_ndo_ops;
-	cpu_dp->orig_ndo_ops = NULL;
+	dev->dsa_ptr->netdev_ops = ops;
 }
 
 static ssize_t tagging_show(struct device *d, struct device_attribute *attr,
@@ -353,9 +330,7 @@ int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
 	if (ret)
 		return ret;
 
-	ret = dsa_master_ndo_setup(dev);
-	if (ret)
-		goto out_err_ethtool_teardown;
+	dsa_netdev_ops_set(dev, &dsa_netdev_ops);
 
 	ret = sysfs_create_group(&dev->dev.kobj, &dsa_group);
 	if (ret)
@@ -364,8 +339,7 @@ int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
 	return ret;
 
 out_err_ndo_teardown:
-	dsa_master_ndo_teardown(dev);
-out_err_ethtool_teardown:
+	dsa_netdev_ops_set(dev, NULL);
 	dsa_master_ethtool_teardown(dev);
 	return ret;
 }
@@ -373,7 +347,7 @@ int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
 void dsa_master_teardown(struct net_device *dev)
 {
 	sysfs_remove_group(&dev->dev.kobj, &dsa_group);
-	dsa_master_ndo_teardown(dev);
+	dsa_netdev_ops_set(dev, NULL);
 	dsa_master_ethtool_teardown(dev);
 	dsa_master_reset_mtu(dev);
 
-- 
2.25.1

Re: [PATCH net-next 1/4] net: Wrap ndo_do_ioctl() to prepare for DSA stacked ops

[Vladimir Oltean]

Hi Florian,

On Fri, Jul 17, 2020 at 08:05:30PM -0700, Florian Fainelli wrote:
> In preparation for adding another layer of call into a DSA stacked ops
> singleton, wrap the ndo_do_ioctl() call into dev_do_ioctl().
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---

I missed most of the context, but this looks interesting. Am I correct
in saying that this could save us from having to manually pass
phy_mii_ioctl() and that it could be generically handled here?

>  net/core/dev_ioctl.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index 547b587c1950..a213c703c90a 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -225,6 +225,22 @@ static int net_hwtstamp_validate(struct ifreq *ifr)
>  	return 0;
>  }
>  
> +static int dev_do_ioctl(struct net_device *dev,
> +			struct ifreq *ifr, unsigned int cmd)
> +{
> +	const struct net_device_ops *ops = dev->netdev_ops;
> +	int err = -EOPNOTSUPP;
> +
> +	if (ops->ndo_do_ioctl) {
> +		if (netif_device_present(dev))
> +			err = ops->ndo_do_ioctl(dev, ifr, cmd);
> +		else
> +			err = -ENODEV;
> +	}
> +
> +	return err;
> +}
> +
>  /*
>   *	Perform the SIOCxIFxxx calls, inside rtnl_lock()
>   */
> @@ -323,13 +339,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
>  		    cmd == SIOCSHWTSTAMP ||
>  		    cmd == SIOCGHWTSTAMP ||
>  		    cmd == SIOCWANDEV) {
> -			err = -EOPNOTSUPP;
> -			if (ops->ndo_do_ioctl) {
> -				if (netif_device_present(dev))
> -					err = ops->ndo_do_ioctl(dev, ifr, cmd);
> -				else
> -					err = -ENODEV;
> -			}
> +			err = dev_do_ioctl(dev, ifr, cmd);
>  		} else
>  			err = -EINVAL;
>  
> -- 
> 2.25.1
> 

Re: [PATCH net-next 1/4] net: Wrap ndo_do_ioctl() to prepare for DSA stacked ops

[Florian Fainelli]

On 7/18/2020 1:30 PM, Vladimir Oltean wrote:
> Hi Florian,
> 
> On Fri, Jul 17, 2020 at 08:05:30PM -0700, Florian Fainelli wrote:
>> In preparation for adding another layer of call into a DSA stacked ops
>> singleton, wrap the ndo_do_ioctl() call into dev_do_ioctl().
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
> 
> I missed most of the context, but this looks interesting. Am I correct
> in saying that this could save us from having to manually pass
> phy_mii_ioctl() and that it could be generically handled here?

The motivation for this work started with the realization while
untangling the ethtool/netlink and PHY library that tests like those:
dev->netdev_ops == &foo_ops would be defeated by the way DSA overloads
the DSA net_device operations. A better solution needed to be found.

You are correct that we could just put a call to phy_mii_ioctl() here as
well and avoid having drivers have to use phy_do_ioctl_running or roll
their own.
-- 
Florian

Re: [PATCH net-next 1/4] net: Wrap ndo_do_ioctl() to prepare for DSA stacked ops

[Vladimir Oltean]

On Sat, Jul 18, 2020 at 01:36:25PM -0700, Florian Fainelli wrote:
> 
> 
> On 7/18/2020 1:30 PM, Vladimir Oltean wrote:
> > Hi Florian,
> > 
> > On Fri, Jul 17, 2020 at 08:05:30PM -0700, Florian Fainelli wrote:
> >> In preparation for adding another layer of call into a DSA stacked ops
> >> singleton, wrap the ndo_do_ioctl() call into dev_do_ioctl().
> >>
> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >> ---
> > 
> > I missed most of the context, but this looks interesting. Am I correct
> > in saying that this could save us from having to manually pass
> > phy_mii_ioctl() and that it could be generically handled here?
> 
> The motivation for this work started with the realization while
> untangling the ethtool/netlink and PHY library that tests like those:
> dev->netdev_ops == &foo_ops would be defeated by the way DSA overloads
> the DSA net_device operations. A better solution needed to be found.
> 
> You are correct that we could just put a call to phy_mii_ioctl() here as
> well and avoid having drivers have to use phy_do_ioctl_running or roll
> their own.
> -- 
> Florian

I see.
The background for my question is that we came to realize that it would
be way cleaner if an Ethernet PHY with 1588 timestamping could just
overload .ndo_do_ioctl() and the ethtool .get_ts_info() of the host
network interface, very similarly to what DSA does.
So it's very good that you started this work, it looks like it provides
a very good foundation. Thanks!

-Vladimir

Re: [PATCH net-next 3/4] net: Call into DSA netdevice_ops wrappers

[Vladimir Oltean]

On Fri, Jul 17, 2020 at 08:05:32PM -0700, Florian Fainelli wrote:
> Make the core net_device code call into our ndo_do_ioctl() and
> ndo_get_phys_port_name() functions via the wrappers defined previously
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  net/core/dev.c       | 5 +++++
>  net/core/dev_ioctl.c | 5 +++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 062a00fdca9b..19f1abc26fcd 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -98,6 +98,7 @@
>  #include <net/busy_poll.h>
>  #include <linux/rtnetlink.h>
>  #include <linux/stat.h>
> +#include <net/dsa.h>
>  #include <net/dst.h>
>  #include <net/dst_metadata.h>
>  #include <net/pkt_sched.h>
> @@ -8602,6 +8603,10 @@ int dev_get_phys_port_name(struct net_device *dev,
>  	const struct net_device_ops *ops = dev->netdev_ops;
>  	int err;
>  
> +	err  = dsa_ndo_get_phys_port_name(dev, name, len);

Stupid question, but why must these be calls to an inline function whose
name is derived through macro concatenation and hardcoded for 2
arguments, then pass through an additional function pointer found in a
DSA-specific lookup table, and why cannot DSA instead simply export
these 2 symbols (with a static inline EOPNOTSUPP fallback), and simply
provide the implementation inside those?

> +	if (err == 0 || err != -EOPNOTSUPP)
> +		return err;
> +
>  	if (ops->ndo_get_phys_port_name) {
>  		err = ops->ndo_get_phys_port_name(dev, name, len);
>  		if (err != -EOPNOTSUPP)
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index a213c703c90a..b2cf9b7bb7b8 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -5,6 +5,7 @@
>  #include <linux/rtnetlink.h>
>  #include <linux/net_tstamp.h>
>  #include <linux/wireless.h>
> +#include <net/dsa.h>
>  #include <net/wext.h>
>  
>  /*
> @@ -231,6 +232,10 @@ static int dev_do_ioctl(struct net_device *dev,
>  	const struct net_device_ops *ops = dev->netdev_ops;
>  	int err = -EOPNOTSUPP;
>  
> +	err = dsa_ndo_do_ioctl(dev, ifr, cmd);
> +	if (err == 0 || err != -EOPNOTSUPP)
> +		return err;
> +
>  	if (ops->ndo_do_ioctl) {
>  		if (netif_device_present(dev))
>  			err = ops->ndo_do_ioctl(dev, ifr, cmd);
> -- 
> 2.25.1
> 

Thanks,
-Vladimir

Re: [PATCH net-next 3/4] net: Call into DSA netdevice_ops wrappers

[Florian Fainelli]

On 7/18/2020 2:18 PM, Vladimir Oltean wrote:
> On Fri, Jul 17, 2020 at 08:05:32PM -0700, Florian Fainelli wrote:
>> Make the core net_device code call into our ndo_do_ioctl() and
>> ndo_get_phys_port_name() functions via the wrappers defined previously
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  net/core/dev.c       | 5 +++++
>>  net/core/dev_ioctl.c | 5 +++++
>>  2 files changed, 10 insertions(+)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 062a00fdca9b..19f1abc26fcd 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -98,6 +98,7 @@
>>  #include <net/busy_poll.h>
>>  #include <linux/rtnetlink.h>
>>  #include <linux/stat.h>
>> +#include <net/dsa.h>
>>  #include <net/dst.h>
>>  #include <net/dst_metadata.h>
>>  #include <net/pkt_sched.h>
>> @@ -8602,6 +8603,10 @@ int dev_get_phys_port_name(struct net_device *dev,
>>  	const struct net_device_ops *ops = dev->netdev_ops;
>>  	int err;
>>  
>> +	err  = dsa_ndo_get_phys_port_name(dev, name, len);
> 
> Stupid question, but why must these be calls to an inline function whose
> name is derived through macro concatenation and hardcoded for 2
> arguments, then pass through an additional function pointer found in a
> DSA-specific lookup table, and why cannot DSA instead simply export
> these 2 symbols (with a static inline EOPNOTSUPP fallback), and simply
> provide the implementation inside those?

The macros could easily be changed to take a single argument list and
play tricks with arguments ordering etc. so I would decouple them from
the choice of using them.

If we have the core network stack reference DSA as a module then we
force DSA to be either built-in or not, which is not very practical,
people would still want a modular choice to be possible. The static
inline only wraps indirect function pointer calls using definitions
available at build time and actual function pointer substitution at run
time, so we avoid that problem entirely that way.
-- 
Florian

Re: [PATCH net-next 1/4] net: Wrap ndo_do_ioctl() to prepare for DSA stacked ops

[Andrew Lunn]

On Fri, Jul 17, 2020 at 08:05:30PM -0700, Florian Fainelli wrote:
> In preparation for adding another layer of call into a DSA stacked ops
> singleton, wrap the ndo_do_ioctl() call into dev_do_ioctl().
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 2/4] net: dsa: Add wrappers for overloaded ndo_ops

[Andrew Lunn]

> +#if IS_ENABLED(CONFIG_NET_DSA)
> +#define dsa_build_ndo_op(name, arg1_type, arg1_name, arg2_type, arg2_name) \
> +static int inline dsa_##name(struct net_device *dev, arg1_type arg1_name, \
> +			     arg2_type arg2_name)	\
> +{							\
> +	const struct dsa_netdevice_ops *ops;		\
> +	int err = -EOPNOTSUPP;				\
> +							\
> +	if (!dev->dsa_ptr)				\
> +		return err;				\
> +							\
> +	ops = dev->dsa_ptr->netdev_ops;			\
> +	if (!ops || !ops->name)				\
> +		return err;				\
> +							\
> +	return ops->name(dev, arg1_name, arg2_name);	\
> +}
> +#else
> +#define dsa_build_ndo_op(name, ...)			\
> +static inline int dsa_##name(struct net_device *dev, ...) \
> +{							\
> +	return -EOPNOTSUPP;				\
> +}
> +#endif
> +
> +dsa_build_ndo_op(ndo_do_ioctl, struct ifreq *, ifr, int, cmd);
> +dsa_build_ndo_op(ndo_get_phys_port_name, char *, name, size_t, len);

Hi Florian

I tend to avoid this sort of macro magic. Tools like
https://elixir.bootlin.com/ and other cross references have trouble
following it. The current macros only handle calls with two
parameters. And i doubt it is actually saving many lines of code, if
there are only two invocations.

      Andrew

Re: [PATCH net-next 3/4] net: Call into DSA netdevice_ops wrappers

[Andrew Lunn]

> If we have the core network stack reference DSA as a module then we
> force DSA to be either built-in or not, which is not very practical,
> people would still want a modular choice to be possible. The static
> inline only wraps indirect function pointer calls using definitions
> available at build time and actual function pointer substitution at run
> time, so we avoid that problem entirely that way.

Hi Florian

The jumping through the pointer avoids the inbuilt vs module problems.

The helpers themselves could be in a net/core/*.c file, rather than
static inline in a header. Is it worth adding a net/core/dsa.c for
code which must always be built in? At the moment, probably not.  But
if we have more such redirect, maybe it would be?

     Andrew

Re: [PATCH net-next 3/4] net: Call into DSA netdevice_ops wrappers

[Florian Fainelli]

On 7/19/2020 9:04 AM, Andrew Lunn wrote:
>> If we have the core network stack reference DSA as a module then we
>> force DSA to be either built-in or not, which is not very practical,
>> people would still want a modular choice to be possible. The static
>> inline only wraps indirect function pointer calls using definitions
>> available at build time and actual function pointer substitution at run
>> time, so we avoid that problem entirely that way.
> 
> Hi Florian
> 
> The jumping through the pointer avoids the inbuilt vs module problems.
> 
> The helpers themselves could be in a net/core/*.c file, rather than
> static inline in a header. Is it worth adding a net/core/dsa.c for
> code which must always be built in? At the moment, probably not.  But
> if we have more such redirect, maybe it would be?
I would continue to put what is DSA specific in net/dsa.h an not
introduce new files within net/core/ that we could easily miss while
updating DSA or we would need to update the MAINTAINERS file for etc.
-- 
Florian

Re: [PATCH net-next 2/4] net: dsa: Add wrappers for overloaded ndo_ops

[Florian Fainelli]

On 7/19/2020 8:40 AM, Andrew Lunn wrote:
>> +#if IS_ENABLED(CONFIG_NET_DSA)
>> +#define dsa_build_ndo_op(name, arg1_type, arg1_name, arg2_type, arg2_name) \
>> +static int inline dsa_##name(struct net_device *dev, arg1_type arg1_name, \
>> +			     arg2_type arg2_name)	\
>> +{							\
>> +	const struct dsa_netdevice_ops *ops;		\
>> +	int err = -EOPNOTSUPP;				\
>> +							\
>> +	if (!dev->dsa_ptr)				\
>> +		return err;				\
>> +							\
>> +	ops = dev->dsa_ptr->netdev_ops;			\
>> +	if (!ops || !ops->name)				\
>> +		return err;				\
>> +							\
>> +	return ops->name(dev, arg1_name, arg2_name);	\
>> +}
>> +#else
>> +#define dsa_build_ndo_op(name, ...)			\
>> +static inline int dsa_##name(struct net_device *dev, ...) \
>> +{							\
>> +	return -EOPNOTSUPP;				\
>> +}
>> +#endif
>> +
>> +dsa_build_ndo_op(ndo_do_ioctl, struct ifreq *, ifr, int, cmd);
>> +dsa_build_ndo_op(ndo_get_phys_port_name, char *, name, size_t, len);
> 
> Hi Florian
> 
> I tend to avoid this sort of macro magic. Tools like
> https://elixir.bootlin.com/ and other cross references have trouble
> following it. The current macros only handle calls with two
> parameters. And i doubt it is actually saving many lines of code, if
> there are only two invocations.

It saves about 20 lines of code for each new function that is added.
Since the boilerplate logic is always the same, if you prefer I could
provide it as a separate helper function and avoid the macro to generate
the function body, yes let's do that.
-- 
Florian