[PATCH v2] net: set default_ethtool_ops in register_netdevice

[PATCH v2] net: set default_ethtool_ops in register_netdevice

[Stanislaw Gruszka]

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, 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 <sgruszka@redhat.com>
Cc: stable@vger.kernel.org # 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(-)


diff --git a/net/core/dev.c b/net/core/dev.c
index 515473e..3196820 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -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:
-- 
1.7.1

Re: [PATCH v2] net: set default_ethtool_ops in register_netdevice

[David Miller]

From: Stanislaw Gruszka <sgruszka@redhat.com>
Date: Tue, 8 Jan 2013 16:38:51 +0100

> 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, 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 <sgruszka@redhat.com>
> Cc: stable@vger.kernel.org # 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*()?

Re: [PATCH v2] net: set default_ethtool_ops in register_netdevice

[Stanislaw Gruszka]

On Wed, Jan 09, 2013 at 11:57:38PM -0800, David Miller wrote:
> From: Stanislaw Gruszka <sgruszka@redhat.com>
> Date: Tue, 8 Jan 2013 16:38:51 +0100
> 
> > 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, 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 <sgruszka@redhat.com>
> > Cc: stable@vger.kernel.org # 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*()?

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

Re: [PATCH v2] net: set default_ethtool_ops in register_netdevice

[David Miller]

From: Stanislaw Gruszka <sgruszka@redhat.com>
Date: Thu, 10 Jan 2013 09:36:42 +0100

> On Wed, Jan 09, 2013 at 11:57:38PM -0800, David Miller wrote:
>> From: Stanislaw Gruszka <sgruszka@redhat.com>
>> Date: Tue, 8 Jan 2013 16:38:51 +0100
>> 
>> > 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, 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 <sgruszka@redhat.com>
>> > Cc: stable@vger.kernel.org # 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*()?
> 
> 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.

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.

Re: [PATCH v2] net: set default_ethtool_ops in register_netdevice

[David Miller]

From: David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Date: Thu, 10 Jan 2013 02:07:29 -0800 (PST)

> From: Stanislaw Gruszka <sgruszka-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Date: Thu, 10 Jan 2013 09:36:42 +0100
> 
>> On Wed, Jan 09, 2013 at 11:57:38PM -0800, David Miller wrote:
>>> From: Stanislaw Gruszka <sgruszka-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> Date: Tue, 8 Jan 2013 16:38:51 +0100
>>> 
>>> > Since:
>>> > 
>>> > commit 2c60db037034d27f8c636403355d52872da92f81
>>> > Author: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>>> > 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 <sgruszka-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> > Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org # 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*()?
>> 
>> 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.
> 
> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] net: set default_ethtool_ops in register_netdevice

[Johannes Berg]

On Thu, 2013-01-10 at 02:12 -0800, David Miller wrote:

> >> On Wed, Jan 09, 2013 at 11:57:38PM -0800, David Miller wrote:
> >>> From: Stanislaw Gruszka <sgruszka-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>> Date: Tue, 8 Jan 2013 16:38:51 +0100
> >>> 
> >>> > Since:
> >>> > 
> >>> > commit 2c60db037034d27f8c636403355d52872da92f81
> >>> > Author: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> >>> > 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 <sgruszka-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>> > Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org # 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*()?
> >> 
> >> 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.
> > 
> > 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)"

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


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] net: set default_ethtool_ops in register_netdevice

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Thu, 10 Jan 2013 12:41:43 +0100

> On Thu, 2013-01-10 at 02:12 -0800, David Miller wrote:
> 
>> >> On Wed, Jan 09, 2013 at 11:57:38PM -0800, David Miller wrote:
>> >>> From: Stanislaw Gruszka <sgruszka@redhat.com>
>> >>> Date: Tue, 8 Jan 2013 16:38:51 +0100
>> >>> 
>> >>> > 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, 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 <sgruszka@redhat.com>
>> >>> > Cc: stable@vger.kernel.org # 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*()?
>> >> 
>> >> 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.
>> > 
>> > 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)"
> 
> 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.