net,wireless: check against default_ethtool_ops

PATCH In review Stanislaw Gruszka 4 participants 7 messages Jan 7, 2013 2 messages not in the archive

Subsystems (3)

Networking Network drivers 802.11 (including cfg80211/NL80211)

lore

Share & tools

lore

Stanislaw Gruszka

author

2013-01-07 09:56 UTC #0 lore raw

PATCH 1/1 net,wireless: check against default_ethtool_ops

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

include/linux/netdevice.h +2 -0

index f8eda02..c98e1c3 100644
@@ -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 */

net/core/dev.c +2 -1

index c0946cb..4cd2168 100644
@@ -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

net/wireless/core.c +1 -1

index 14d9904..90915d4 100644
@@ -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 ||

Signature · 1 line

1.7.1

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jiri Pirko

2013-01-07 10:23 UTC #2 lore raw

Quoted 43 lines · Mon, Jan 07, 2013 at 10:55:49AM CET, [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.
Signed-off-by Stanislaw Gruszka <[email protected]>
Acked-by Ben Hutchings <bhutchings-s/n/[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(-)
include/linux/netdevice.h +2 -0
index f8eda02..c98e1c3 100644
@@ -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 */
net/core/dev.c +0 -0
index c0946cb..4cd2168 100644

net/core/dev.c struct netdev_queue *dev_ingress_queue_create(struct net_device *dev) · line 6012

Quoted 6 lines

net/core/dev.c +2 -1

@@ -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;
}

Quoted 23 lines

net/core/dev.c +0 -0

 /**
 *	alloc_netdev_mqs - allocate network device

net/wireless/core.c +1 -1

index 14d9904..90915d4 100644
@@ -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 ||

Signature · 6 lines

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

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Stanislaw Gruszka

author

2013-01-07 10:44 UTC #3 lore raw

Quoted 11 lines · Jiri Pirko

net/core/dev.c struct netdev_queue *dev_ingress_queue_create(struct net_device *dev) · line 6012
Quoted 3 lines
net/core/dev.c +2 -1
-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

Jiri Pirko

2013-01-07 11:11 UTC #4 lore raw

Quoted 8 lines · Mon, Jan 07, 2013 at 11:44:14AM CET, [email protected] wrote:

Quoted 7 lines · Jiri Pirko
net/core/dev.c struct netdev_queue *dev_ingress_queue_create(struct net_device *dev) · line 6012
Quoted 3 lines
net/core/dev.c +2 -1
-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:

Quoted 8 lines

Quoted 4 lines
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...

Quoted 2 lines

Stanislaw

Stanislaw Gruszka

author

2013-01-07 11:39 UTC #5 lore raw

Quoted 14 lines · Jiri Pirko

Quoted 13 lines · Mon, Jan 07, 2013 at 11:44:14AM CET, [email protected] wrote:
Quoted 11 lines · Jiri Pirko
net/core/dev.c struct netdev_queue *dev_ingress_queue_create(struct net_device *dev) · line 6012
Quoted 3 lines
net/core/dev.c +2 -1
-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;
}

Quoted 6 lines

Quoted 3 lines
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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jiri Pirko

2013-01-07 11:57 UTC #7 lore raw

Quoted 15 lines · Mon, Jan 07, 2013 at 12:20:12PM CET, [email protected]…

Quoted 14 lines · Jiri Pirko
Quoted 13 lines · Mon, Jan 07, 2013 at 11:44:14AM CET, [email protected]…
Quoted 11 lines · Jiri Pirko
Review
Quoted 3 lines
(quoted diff) +2 -1
-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;
}

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
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.

Quoted 2 lines

Stanislaw

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Michał Mirosław

2013-01-07 17:21 UTC #8 lore raw

In reply to Stanislaw Gruszka

2013/1/7 Stanislaw Gruszka <[email protected]>:

Quoted 6 lines

Since:
commit 2c60db037034d27f8c636403355d52872da92f81
Author: Eric Dumazet <[email protected]>
Date:   Sun Sep 16 09:17:26 2012 +0000

Quoted 6 lines

    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