Message ID | 20130108153850.GA10464@redhat.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
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*()? -- 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 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 -- 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
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. -- 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
From: David Miller <davem@davemloft.net> Date: Thu, 10 Jan 2013 02:07:29 -0800 (PST) > 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. 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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 :-) 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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- 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/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:
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(-)