| Message ID | 20200718030533.171556-4-f.fainelli@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: Setup dsa_netdev_ops | expand |
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
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.
> 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
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.
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);
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(+)