| Message ID | 20130107095548.GA6931@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Mon, Jan 07, 2013 at 10:55:49AM CET, sgruszka@redhat.com wrote: >Since: > >commit 2c60db037034d27f8c636403355d52872da92f81 >Author: Eric Dumazet <edumazet@google.com> >Date: Sun Sep 16 09:17:26 2012 +0000 > > net: provide a default dev->ethtool_ops > >wireless core does not correctly assign ethtool_ops. In order to fix >the problem, and avoid assigning ethtool_ops on each individual cfg80211 >drivers, we check against default_ethool_ops pointer instead of NULL in >wireless core. > >Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> >Acked-by: Ben Hutchings <bhutchings@solarflare.com> >Cc: stable@vger.kernel.org # 3.7+ >--- >This should go directly through net tree since patch include core >net specific changes. > > include/linux/netdevice.h | 2 ++ > net/core/dev.c | 3 ++- > net/wireless/core.c | 2 +- > 3 files changed, 5 insertions(+), 2 deletions(-) > >diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >index f8eda02..c98e1c3 100644 >--- a/include/linux/netdevice.h >+++ b/include/linux/netdevice.h >@@ -60,6 +60,8 @@ struct wireless_dev; > #define SET_ETHTOOL_OPS(netdev,ops) \ > ( (netdev)->ethtool_ops = (ops) ) > >+extern const struct ethtool_ops default_ethtool_ops; >+ > /* hardware address assignment types */ > #define NET_ADDR_PERM 0 /* address is permanent (default) */ > #define NET_ADDR_RANDOM 1 /* address is generated randomly */ >diff --git a/net/core/dev.c b/net/core/dev.c >index c0946cb..4cd2168 100644 >--- a/net/core/dev.c >+++ b/net/core/dev.c >@@ -6008,7 +6008,8 @@ struct netdev_queue *dev_ingress_queue_create(struct net_device *dev) > return queue; > } > >-static const struct ethtool_ops default_ethtool_ops; >+const struct ethtool_ops default_ethtool_ops; >+EXPORT_SYMBOL_GPL(default_ethtool_ops); I think that default_ethtool_ops should stay static. Wouldn't it be nicer to introduce a helper like: bool dev_has_default_ethtool_ops(struct net_device *dev) { return dev->ethtool_ops == &default_ethtool_ops; } > > /** > * alloc_netdev_mqs - allocate network device >diff --git a/net/wireless/core.c b/net/wireless/core.c >index 14d9904..90915d4 100644 >--- a/net/wireless/core.c >+++ b/net/wireless/core.c >@@ -866,7 +866,7 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb, > /* allow mac80211 to determine the timeout */ > wdev->ps_timeout = -1; > >- if (!dev->ethtool_ops) >+ if (dev->ethtool_ops == &default_ethtool_ops) > dev->ethtool_ops = &cfg80211_ethtool_ops; > > if ((wdev->iftype == NL80211_IFTYPE_STATION || >-- >1.7.1 > >-- >To unsubscribe from this list: send the line "unsubscribe netdev" in >the body of a message to majordomo@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 07, 2013 at 11:23:07AM +0100, Jiri Pirko wrote: > >-static const struct ethtool_ops default_ethtool_ops; > >+const struct ethtool_ops default_ethtool_ops; > >+EXPORT_SYMBOL_GPL(default_ethtool_ops); > > I think that default_ethtool_ops should stay static. Wouldn't it be > nicer to introduce a helper like: > > bool dev_has_default_ethtool_ops(struct net_device *dev) > { > return dev->ethtool_ops == &default_ethtool_ops; > } Then I still have to export this function. So with your approch, number of exported symbols will be the same, but there will be few more lines of code. Stanislaw -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mon, Jan 07, 2013 at 11:44:14AM CET, sgruszka@redhat.com wrote: >On Mon, Jan 07, 2013 at 11:23:07AM +0100, Jiri Pirko wrote: >> >-static const struct ethtool_ops default_ethtool_ops; >> >+const struct ethtool_ops default_ethtool_ops; >> >+EXPORT_SYMBOL_GPL(default_ethtool_ops); >> >> I think that default_ethtool_ops should stay static. Wouldn't it be >> nicer to introduce a helper like: >> >> bool dev_has_default_ethtool_ops(struct net_device *dev) >> { >> return dev->ethtool_ops == &default_ethtool_ops; >> } > >Then I still have to export this function. So with your approch, number >of exported symbols will be the same, but there will be few more lines >of code. I think it's always better to add few more lines in order to prevent possible confusion which exporting default_ethtool_ops might introduce... > >Stanislaw -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 07, 2013 at 12:11:08PM +0100, Jiri Pirko wrote: > Mon, Jan 07, 2013 at 11:44:14AM CET, sgruszka@redhat.com wrote: > >On Mon, Jan 07, 2013 at 11:23:07AM +0100, Jiri Pirko wrote: > >> >-static const struct ethtool_ops default_ethtool_ops; > >> >+const struct ethtool_ops default_ethtool_ops; > >> >+EXPORT_SYMBOL_GPL(default_ethtool_ops); > >> > >> I think that default_ethtool_ops should stay static. Wouldn't it be > >> nicer to introduce a helper like: > >> > >> bool dev_has_default_ethtool_ops(struct net_device *dev) > >> { > >> return dev->ethtool_ops == &default_ethtool_ops; > >> } > > > >Then I still have to export this function. So with your approch, number > >of exported symbols will be the same, but there will be few more lines > >of code. > > I think it's always better to add few more lines in order to prevent possible > confusion which exporting default_ethtool_ops might introduce... What possible confusion it might cause? Stanislaw -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mon, Jan 07, 2013 at 12:20:12PM CET, sgruszka@redhat.com wrote: >On Mon, Jan 07, 2013 at 12:11:08PM +0100, Jiri Pirko wrote: >> Mon, Jan 07, 2013 at 11:44:14AM CET, sgruszka@redhat.com wrote: >> >On Mon, Jan 07, 2013 at 11:23:07AM +0100, Jiri Pirko wrote: >> >> >-static const struct ethtool_ops default_ethtool_ops; >> >> >+const struct ethtool_ops default_ethtool_ops; >> >> >+EXPORT_SYMBOL_GPL(default_ethtool_ops); >> >> >> >> I think that default_ethtool_ops should stay static. Wouldn't it be >> >> nicer to introduce a helper like: >> >> >> >> bool dev_has_default_ethtool_ops(struct net_device *dev) >> >> { >> >> return dev->ethtool_ops == &default_ethtool_ops; >> >> } >> > >> >Then I still have to export this function. So with your approch, number >> >of exported symbols will be the same, but there will be few more lines >> >of code. >> >> I think it's always better to add few more lines in order to prevent possible >> confusion which exporting default_ethtool_ops might introduce... > >What possible confusion it might cause? Someone would possibly like to do: dev->netdev_ops = &default_ethtool_ops in drivers for example... + I just do not think that exporting structs is the correct way in order to do anything. > >Stanislaw -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2013/1/7 Stanislaw Gruszka <sgruszka@redhat.com>: > Since: > > commit 2c60db037034d27f8c636403355d52872da92f81 > Author: Eric Dumazet <edumazet@google.com> > Date: Sun Sep 16 09:17:26 2012 +0000 > > net: provide a default dev->ethtool_ops > > wireless core does not correctly assign ethtool_ops. In order to fix > the problem, and avoid assigning ethtool_ops on each individual cfg80211 > drivers, we check against default_ethool_ops pointer instead of NULL in > wireless core. [...] You could instead move the assignment of default ethtool_ops to just after call_netdevice_notifiers(NETDEV_REGISTER) in register_netdevice() or just after call_netdevice_notifiers(NETDEV_POST_INIT) and initialize the default wireless ethtool_ops in NETDEV_POST_INIT hook. That will avoid the export. Either way is good because register_netdevice() is called under RTNL, so ethtool_ops can't be called until it returns. NETDEV_POST_INIT seams more natural to me, but it's not a strong opinion. Best Regards, Michał Mirosław -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index f8eda02..c98e1c3 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -60,6 +60,8 @@ struct wireless_dev; #define SET_ETHTOOL_OPS(netdev,ops) \ ( (netdev)->ethtool_ops = (ops) ) +extern const struct ethtool_ops default_ethtool_ops; + /* hardware address assignment types */ #define NET_ADDR_PERM 0 /* address is permanent (default) */ #define NET_ADDR_RANDOM 1 /* address is generated randomly */ diff --git a/net/core/dev.c b/net/core/dev.c index c0946cb..4cd2168 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -6008,7 +6008,8 @@ struct netdev_queue *dev_ingress_queue_create(struct net_device *dev) return queue; } -static const struct ethtool_ops default_ethtool_ops; +const struct ethtool_ops default_ethtool_ops; +EXPORT_SYMBOL_GPL(default_ethtool_ops); /** * alloc_netdev_mqs - allocate network device diff --git a/net/wireless/core.c b/net/wireless/core.c index 14d9904..90915d4 100644 --- a/net/wireless/core.c +++ b/net/wireless/core.c @@ -866,7 +866,7 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb, /* allow mac80211 to determine the timeout */ wdev->ps_timeout = -1; - if (!dev->ethtool_ops) + if (dev->ethtool_ops == &default_ethtool_ops) dev->ethtool_ops = &cfg80211_ethtool_ops; if ((wdev->iftype == NL80211_IFTYPE_STATION ||
Since: commit 2c60db037034d27f8c636403355d52872da92f81 Author: Eric Dumazet <edumazet@google.com> Date: Sun Sep 16 09:17:26 2012 +0000 net: provide a default dev->ethtool_ops wireless core does not correctly assign ethtool_ops. In order to fix the problem, and avoid assigning ethtool_ops on each individual cfg80211 drivers, we check against default_ethool_ops pointer instead of NULL in wireless core. Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> Acked-by: Ben Hutchings <bhutchings@solarflare.com> Cc: stable@vger.kernel.org # 3.7+ --- This should go directly through net tree since patch include core net specific changes. include/linux/netdevice.h | 2 ++ net/core/dev.c | 3 ++- net/wireless/core.c | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-)