Since:
commit 2c60db037034d27f8c636403355d52872da92f81
Author: Eric Dumazet <[email protected]>
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 <[email protected]>
Acked-by: Ben Hutchings <[email protected]>
Cc: [email protected] # 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);
/**
* 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
On Mon, Jan 07, 2013 at 12:11:08PM +0100, Jiri Pirko wrote:
> Mon, Jan 07, 2013 at 11:44:14AM CET, [email protected] 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
2013/1/7 Stanislaw Gruszka <[email protected]>:
> Since:
>
> commit 2c60db037034d27f8c636403355d52872da92f81
> Author: Eric Dumazet <[email protected]>
> 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
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
Mon, Jan 07, 2013 at 11:44:14AM CET, [email protected] 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
Mon, Jan 07, 2013 at 10:55:49AM CET, [email protected] wrote:
>Since:
>
>commit 2c60db037034d27f8c636403355d52872da92f81
>Author: Eric Dumazet <[email protected]>
>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 <[email protected]>
>Acked-by: Ben Hutchings <[email protected]>
>Cc: [email protected] # 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 [email protected]
>More majordomo info at http://vger.kernel.org/majordomo-info.html
Mon, Jan 07, 2013 at 12:20:12PM CET, [email protected] wrote:
>On Mon, Jan 07, 2013 at 12:11:08PM +0100, Jiri Pirko wrote:
>> Mon, Jan 07, 2013 at 11:44:14AM CET, [email protected] 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