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 |
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
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.
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
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.
> +#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
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 --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);
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(+)