net: set default_ethtool_ops in register_netdevice

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

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Stanislaw Gruszka

author

2013-01-08 15:39 UTC #0 lore raw

PATCH v2 1/1 net: set default_ethtool_ops in register_netdevice

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, move assignement of default_ethtool_ops to register_netdevice(). This is safe because both register_netdevice() and dev_ethtool() are protected by RTNL lock.

Patch is besed on hint of Michał Mirosław.

Signed-off-by Stanislaw Gruszka <[email protected]>

Cc [email protected] # 3.7+

---
v1 -> v2: change order of default_ethtool_ops initialization to avoid the problem. Change the subject accordingly.

 net/core/dev.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

net/core/dev.c +4 -4

index 515473e..3196820 100644
@@ -5710,6 +5710,8 @@ static int netif_alloc_netdev_queues(struct net_device *dev)
 	return 0;
 }
 
+static const struct ethtool_ops default_ethtool_ops;
+
 /**
  *	register_netdevice	- register a network device
  *	@dev: device to register

@@ -5830,6 +5832,8 @@ int register_netdevice(struct net_device *dev)
 	    dev->rtnl_link_state == RTNL_LINK_INITIALIZED)
 		rtmsg_ifinfo(RTM_NEWLINK, dev, ~0U);
 
+	if (!dev->ethtool_ops)
+		dev->ethtool_ops = &default_ethtool_ops;
 out:
 	return ret;

@@ -6119,8 +6123,6 @@ struct netdev_queue *dev_ingress_queue_create(struct net_device *dev)
 	return queue;
 }
 
-static const struct ethtool_ops default_ethtool_ops;
-
 /**
  *	alloc_netdev_mqs - allocate network device
  *	@sizeof_priv:	size of private data to allocate space for

@@ -6208,8 +6210,6 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 
 	strcpy(dev->name, name);
 	dev->group = INIT_NETDEV_GROUP;
-	if (!dev->ethtool_ops)
-		dev->ethtool_ops = &default_ethtool_ops;
 	return dev;
 
 free_all:

Signature · 1 line

1.7.1

David Miller

maintainer

2013-01-10 07:57 UTC #1 lore raw

From: Stanislaw Gruszka <[email protected]>
Date: Tue, 8 Jan 2013 16:38:51 +0100

Quoted 13 lines

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, move assignement of default_ethtool_ops to register_netdevice(). This is safe because both register_netdevice() and dev_ethtool() are protected by RTNL lock.

Quoted 7 lines

Patch is besed on hint of Michał Mirosław.
Signed-off-by Stanislaw Gruszka <[email protected]>
Cc [email protected] # 3.7+
---
v1 -> v2: change order of default_ethtool_ops initialization to avoid the problem. Change the subject accordingly.

I don't understand this. Why is the assignment of default_ethtool_ops at netdev allocation time not working? Is wireless really not using alloc_netdev*()?

Stanislaw Gruszka

author

2013-01-10 08:36 UTC #2 lore raw

Quoted 24 lines · David Miller

From: Stanislaw Gruszka <[email protected]>
Date: Tue, 8 Jan 2013 16:38:51 +0100
Quoted 13 lines
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, move assignement of default_ethtool_ops to register_netdevice(). This is safe because both register_netdevice() and dev_ethtool() are protected by RTNL lock.
Quoted 7 lines
Patch is besed on hint of Michał Mirosław.
Signed-off-by Stanislaw Gruszka <[email protected]>
Cc [email protected] # 3.7+
---
v1 -> v2: change order of default_ethtool_ops initialization to avoid the problem. Change the subject accordingly.

Quoted 3 lines

I don't understand this. Why is the assignment of default_ethtool_ops at netdev allocation time not working? Is wireless really not using alloc_netdev*()?

It does. This is done on individual cfg80211 drivers , i.e. on mac80211 or full mac drivers. After alloc_netdev*() call, some cfg80211 drivers provide they own ethtool_ops, but some do not. For them, wireless core provide generic cfg80211_ethtool_ops, which is assigned in NETDEV_REGISTER notify call:

	if (!dev->ethtool_ops)
		dev->ethtool_ops = &cfg80211_ethtool_ops;

But after Eric's commit, dev->ethtool_ops is no longer NULL (on cfg80211 drivers without custom ethtool_ops), but points to &default_ethtool_ops.

Stanislaw

David Miller

maintainer

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

From: Stanislaw Gruszka <[email protected]>
Date: Thu, 10 Jan 2013 09:36:42 +0100

Quoted 35 lines · David Miller

Quoted 24 lines
From: Stanislaw Gruszka <[email protected]>
Date: Tue, 8 Jan 2013 16:38:51 +0100
Quoted 13 lines
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, move assignement of default_ethtool_ops to register_netdevice(). This is safe because both register_netdevice() and dev_ethtool() are protected by RTNL lock.
Quoted 7 lines
Patch is besed on hint of Michał Mirosław.
Signed-off-by Stanislaw Gruszka <[email protected]>
Cc [email protected] # 3.7+
---
v1 -> v2: change order of default_ethtool_ops initialization to avoid the problem. Change the subject accordingly.
Quoted 3 lines
I don't understand this. Why is the assignment of default_ethtool_ops at netdev allocation time not working? Is wireless really not using alloc_netdev*()?
It does. This is done on individual cfg80211 drivers , i.e. on mac80211 or full mac drivers. After alloc_netdev*() call, some cfg80211 drivers provide they own ethtool_ops, but some do not. For them, wireless core provide generic cfg80211_ethtool_ops, which is assigned in NETDEV_REGISTER notify call:

Quoted 5 lines

	if (!dev->ethtool_ops)
		dev->ethtool_ops = &cfg80211_ethtool_ops;
But after Eric's commit, dev->ethtool_ops is no longer NULL (on cfg80211 drivers without custom ethtool_ops), but points to &default_ethtool_ops.

The whole idea is to remove these kinds of NULL tests against dev->ethtool_ops, thus creating the invariant that given any netdev one must never be able to see a non-NULL value there.

A message that is not in the archive (sent off-list or not yet received): 20130110.020729.1358020934840866117.davem-fT/PcQaiUtIeIZ0/[email protected]

David Miller

2013-01-10 10:12 UTC #5 lore raw

From: David Miller <davem-fT/PcQaiUtIeIZ0/[email protected]>
Date: Thu, 10 Jan 2013 02:07:29 -0800 (PST)

Quoted 41 lines

From: Stanislaw Gruszka <[email protected]>
Date: Thu, 10 Jan 2013 09:36:42 +0100
Quoted 38 lines · David Miller
Quoted 24 lines
From: Stanislaw Gruszka <[email protected]>
Date: Tue, 8 Jan 2013 16:38:51 +0100
Quoted 13 lines
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, move assignement of default_ethtool_ops to register_netdevice(). This is safe because both register_netdevice() and dev_ethtool() are protected by RTNL lock.
Quoted 7 lines
Patch is besed on hint of Michał Mirosław.
Signed-off-by Stanislaw Gruszka <[email protected]>
Cc [email protected] # 3.7+
---
v1 -> v2: change order of default_ethtool_ops initialization to avoid the problem. Change the subject accordingly.
Quoted 3 lines
I don't understand this. Why is the assignment of default_ethtool_ops at netdev allocation time not working? Is wireless really not using alloc_netdev*()?
It does. This is done on individual cfg80211 drivers , i.e. on mac80211 or full mac drivers. After alloc_netdev*() call, some cfg80211 drivers provide they own ethtool_ops, but some do not. For them, wireless core provide generic cfg80211_ethtool_ops, which is assigned in NETDEV_REGISTER notify call:
	if (!dev->ethtool_ops)
		dev->ethtool_ops = &cfg80211_ethtool_ops;

Quoted 6 lines

Quoted 2 lines
But after Eric's commit, dev->ethtool_ops is no longer NULL (on cfg80211 drivers without custom ethtool_ops), but points to &default_ethtool_ops.
The whole idea is to remove these kinds of NULL tests against dev->ethtool_ops, thus creating the invariant that given any netdev one must never be able to see a non-NULL value there.

Of course I meant that we should never see a NULL value in dev->ethtool_ops.

I would suggest fixing this by making the wireless core express it's intentions, that it wants to use a different default ethtool ops, to the netdev allocation layer.

You have two way to do this:

1) Add default_ethtool_ops argument to alloc_netdev*().

2) Add a "netdev_set_default_ethtool_ops(netdev, ops)"

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

A message that is not in the archive (sent off-list or not yet received): 20130110.021202.232771737304117743.davem-fT/PcQaiUtIeIZ0/[email protected]

Johannes Berg

2013-01-10 11:41 UTC #7 lore raw

Quoted 52 lines · David Miller

Quoted 38 lines
Quoted 38 lines · David Miller
Quoted 24 lines
From: Stanislaw Gruszka <[email protected]>
Date: Tue, 8 Jan 2013 16:38:51 +0100
Quoted 13 lines
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, move assignement of default_ethtool_ops to register_netdevice(). This is safe because both register_netdevice() and dev_ethtool() are protected by RTNL lock.
Quoted 7 lines
Patch is besed on hint of Michał Mirosław.
Signed-off-by Stanislaw Gruszka <[email protected]>
Cc [email protected] # 3.7+
---
v1 -> v2: change order of default_ethtool_ops initialization to avoid the problem. Change the subject accordingly.
Quoted 3 lines
I don't understand this. Why is the assignment of default_ethtool_ops at netdev allocation time not working? Is wireless really not using alloc_netdev*()?
It does. This is done on individual cfg80211 drivers , i.e. on mac80211 or full mac drivers. After alloc_netdev*() call, some cfg80211 drivers provide they own ethtool_ops, but some do not. For them, wireless core provide generic cfg80211_ethtool_ops, which is assigned in NETDEV_REGISTER notify call:
     if (!dev->ethtool_ops)
             dev->ethtool_ops = &cfg80211_ethtool_ops;
Quoted 6 lines
Quoted 2 lines
But after Eric's commit, dev->ethtool_ops is no longer NULL (on cfg80211 drivers without custom ethtool_ops), but points to &default_ethtool_ops.
The whole idea is to remove these kinds of NULL tests against dev->ethtool_ops, thus creating the invariant that given any netdev one must never be able to see a non-NULL value there.
Of course I meant that we should never see a NULL value in dev->ethtool_ops.
I would suggest fixing this by making the wireless core express it's intentions, that it wants to use a different default ethtool ops, to the netdev allocation layer.

Quoted 5 lines

You have two way to do this:
1) Add default_ethtool_ops argument to alloc_netdev*().
2) Add a "netdev_set_default_ethtool_ops(netdev, ops)"

Neither of these work as (I think) you think they would :-)

The core wireless code doesn't allocate/register the netdev itself, the driver does. It has to follow some constraints, but the wireless core then hooks into the netdev notifier chain to be informed. While in the notifier chain, it attempts to set ethtool ops, but they've long been set already to the defaults. So that means the second way you suggested won't work at that point any more, short of having the wireless core code export its ethtool ops and drivers setting them.

Now arguably the wireless core code could provide its own alloc_netdev*() wrappers, but is that really worth it? There are a lot of possibilities there that we'd have to wrap.

johannes

David Miller

maintainer

2013-01-10 21:00 UTC #8 lore raw

From: Johannes Berg <[email protected]>
Date: Thu, 10 Jan 2013 12:41:43 +0100

Quoted 54 lines · David Miller

Quoted 53 lines
Quoted 39 lines
Quoted 39 lines · David Miller
Quoted 24 lines
From: Stanislaw Gruszka <[email protected]>
Date: Tue, 8 Jan 2013 16:38:51 +0100
Quoted 13 lines
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, move assignement of default_ethtool_ops to register_netdevice(). This is safe because both register_netdevice() and dev_ethtool() are protected by RTNL lock.
Quoted 7 lines
Patch is besed on hint of Michał Mirosław.
Signed-off-by Stanislaw Gruszka <[email protected]>
Cc [email protected] # 3.7+
---
v1 -> v2: change order of default_ethtool_ops initialization to avoid the problem. Change the subject accordingly.
Quoted 3 lines
I don't understand this. Why is the assignment of default_ethtool_ops at netdev allocation time not working? Is wireless really not using alloc_netdev*()?
It does. This is done on individual cfg80211 drivers , i.e. on mac80211 or full mac drivers. After alloc_netdev*() call, some cfg80211 drivers provide they own ethtool_ops, but some do not. For them, wireless core provide generic cfg80211_ethtool_ops, which is assigned in NETDEV_REGISTER notify call:
     if (!dev->ethtool_ops)
             dev->ethtool_ops = &cfg80211_ethtool_ops;
Quoted 6 lines
Quoted 2 lines
But after Eric's commit, dev->ethtool_ops is no longer NULL (on cfg80211 drivers without custom ethtool_ops), but points to &default_ethtool_ops.
The whole idea is to remove these kinds of NULL tests against dev->ethtool_ops, thus creating the invariant that given any netdev one must never be able to see a non-NULL value there.
Of course I meant that we should never see a NULL value in dev->ethtool_ops.
I would suggest fixing this by making the wireless core express it's intentions, that it wants to use a different default ethtool ops, to the netdev allocation layer.

Quoted 7 lines

Quoted 5 lines
You have two way to do this:
1) Add default_ethtool_ops argument to alloc_netdev*().
2) Add a "netdev_set_default_ethtool_ops(netdev, ops)"
Neither of these work as (I think) you think they would :-)

2) works because all netdev_set_ethtool_ops() is going to do is compare netdev->ops against &default_ethtool_ops as the test, and therefore you can replace the above cfg80211 test and assignment with a call to it.