diff mbox series

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

Message ID 20200718030533.171556-3-f.fainelli@gmail.com
State Changes Requested
Delegated to: David Miller
Headers show
Series net: dsa: Setup dsa_netdev_ops | expand

Commit Message

Florian Fainelli July 18, 2020, 3:05 a.m. UTC 
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(+)

Comments

kernel test robot July 18, 2020, 4:53 a.m. UTC | #1 
Hi Florian,

I love your patch! Perhaps something to improve:

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

url:    https://github.com/0day-ci/linux/commits/Florian-Fainelli/net-dsa-Setup-dsa_netdev_ops/20200718-110931
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git dcc82bb0727c08f93a91fa7532b950bafa2598f2
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c:18:
>> include/net/dsa.h:720:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration]
     720 | dsa_build_ndo_op(ndo_do_ioctl, struct ifreq *, ifr, int, cmd);
         | ^~~~~~~~~~~~~~~~
   include/net/dsa.h:721:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration]
     721 | dsa_build_ndo_op(ndo_get_phys_port_name, char *, name, size_t, len);
         | ^~~~~~~~~~~~~~~~

vim +/inline +720 include/net/dsa.h

   719	
 > 720	dsa_build_ndo_op(ndo_do_ioctl, struct ifreq *, ifr, int, cmd);
   721	dsa_build_ndo_op(ndo_get_phys_port_name, char *, name, size_t, len);
   722	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Florian Fainelli July 18, 2020, 6:53 p.m. UTC | #2 
On 7/17/2020 9:53 PM, kernel test robot wrote:
> Hi Florian,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on net-next/master]
> 
> url:    https://github.com/0day-ci/linux/commits/Florian-Fainelli/net-dsa-Setup-dsa_netdev_ops/20200718-110931
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git dcc82bb0727c08f93a91fa7532b950bafa2598f2
> config: i386-allyesconfig (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
> reproduce (this is a W=1 build):
>         # save the attached .config to linux build tree
>         make W=1 ARCH=i386 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All warnings (new ones prefixed by >>):
> 
>    In file included from drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c:18:
>>> include/net/dsa.h:720:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration]
>      720 | dsa_build_ndo_op(ndo_do_ioctl, struct ifreq *, ifr, int, cmd);
>          | ^~~~~~~~~~~~~~~~
>    include/net/dsa.h:721:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration]
>      721 | dsa_build_ndo_op(ndo_get_phys_port_name, char *, name, size_t, len);
>          | ^~~~~~~~~~~~~~~~
> 
> vim +/inline +720 include/net/dsa.h

This is a macro invocation, not function declaration so I am not exactly
sure why this is a problem here? I could capitalize the macro name if
that avoids the compiler thinking this is a function declaration or move
out the static inline away from the macro invocation.
Vladimir Oltean July 18, 2020, 8:18 p.m. UTC | #3 
On Sat, Jul 18, 2020 at 11:53:52AM -0700, Florian Fainelli wrote:
> 
> 
> On 7/17/2020 9:53 PM, kernel test robot wrote:
> > Hi Florian,
> > 
> > I love your patch! Perhaps something to improve:
> > 
> > [auto build test WARNING on net-next/master]
> > 
> > url:    https://github.com/0day-ci/linux/commits/Florian-Fainelli/net-dsa-Setup-dsa_netdev_ops/20200718-110931
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git dcc82bb0727c08f93a91fa7532b950bafa2598f2
> > config: i386-allyesconfig (attached as .config)
> > compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
> > reproduce (this is a W=1 build):
> >         # save the attached .config to linux build tree
> >         make W=1 ARCH=i386 
> > 
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp@intel.com>
> > 
> > All warnings (new ones prefixed by >>):
> > 
> >    In file included from drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c:18:
> >>> include/net/dsa.h:720:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration]
> >      720 | dsa_build_ndo_op(ndo_do_ioctl, struct ifreq *, ifr, int, cmd);
> >          | ^~~~~~~~~~~~~~~~
> >    include/net/dsa.h:721:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration]
> >      721 | dsa_build_ndo_op(ndo_get_phys_port_name, char *, name, size_t, len);
> >          | ^~~~~~~~~~~~~~~~
> > 
> > vim +/inline +720 include/net/dsa.h
> 
> This is a macro invocation, not function declaration so I am not exactly
> sure why this is a problem here? I could capitalize the macro name if
> that avoids the compiler thinking this is a function declaration or move
> out the static inline away from the macro invocation.
> -- 
> Florian

Maybe it wants 'static inline int' and not 'static int inline'?

+#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, \
            ^
            ~~~~~ here
+			     arg2_type arg2_name)	\
+{							\

-Vladimir
Florian Fainelli July 18, 2020, 8:20 p.m. UTC | #4 
On 7/18/2020 1:18 PM, Vladimir Oltean wrote:
> On Sat, Jul 18, 2020 at 11:53:52AM -0700, Florian Fainelli wrote:
>>
>>
>> On 7/17/2020 9:53 PM, kernel test robot wrote:
>>> Hi Florian,
>>>
>>> I love your patch! Perhaps something to improve:
>>>
>>> [auto build test WARNING on net-next/master]
>>>
>>> url:    https://github.com/0day-ci/linux/commits/Florian-Fainelli/net-dsa-Setup-dsa_netdev_ops/20200718-110931
>>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git dcc82bb0727c08f93a91fa7532b950bafa2598f2
>>> config: i386-allyesconfig (attached as .config)
>>> compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
>>> reproduce (this is a W=1 build):
>>>         # save the attached .config to linux build tree
>>>         make W=1 ARCH=i386 
>>>
>>> If you fix the issue, kindly add following tag as appropriate
>>> Reported-by: kernel test robot <lkp@intel.com>
>>>
>>> All warnings (new ones prefixed by >>):
>>>
>>>    In file included from drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c:18:
>>>>> include/net/dsa.h:720:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration]
>>>      720 | dsa_build_ndo_op(ndo_do_ioctl, struct ifreq *, ifr, int, cmd);
>>>          | ^~~~~~~~~~~~~~~~
>>>    include/net/dsa.h:721:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration]
>>>      721 | dsa_build_ndo_op(ndo_get_phys_port_name, char *, name, size_t, len);
>>>          | ^~~~~~~~~~~~~~~~
>>>
>>> vim +/inline +720 include/net/dsa.h
>>
>> This is a macro invocation, not function declaration so I am not exactly
>> sure why this is a problem here? I could capitalize the macro name if
>> that avoids the compiler thinking this is a function declaration or move
>> out the static inline away from the macro invocation.
>> -- 
>> Florian
> 
> Maybe it wants 'static inline int' and not 'static int inline'?

Oh yes indeed, thank you.
Andrew Lunn July 19, 2020, 3:40 p.m. UTC | #5 
> +#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
Florian Fainelli July 19, 2020, 4:10 p.m. UTC | #6 
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.
diff mbox series

Patch

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);