Re: oops with recent wireless-dev tree

Re: oops with recent wireless-dev tree

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 785 bytes --]

Hi Jochen,

[added CCs since it affects bridge code]

> If I read this correctly, the EIP in the last line corresponds to
> net/bridge/br_if.c, line 36:
> 
>     static int port_cost(struct net_device *dev)
>     {
> 	    if (dev->ethtool_ops->get_settings) {
>                                ^^^^
> 
> As far as I can figure out, dev->ethtool_ops is NULL and the crash
> happens while trying to derefernce ...->get_settings.
> 
> Is dev->ethtool_ops allowed to be NULL?  In this case the appended
> patch might be the correct fix.  At least it makes the oops disappear
> for me.  Another possible fix would be to add an ethtool_ops structure
> to the device created by b43.

I don't think adding ethtool_ops in mac80211 should be necessary.
Stephen?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: oops with recent wireless-dev tree

[Stephen Hemminger]

On Thu, 30 Aug 2007 14:05:30 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:

> Hi Jochen,
> 
> [added CCs since it affects bridge code]
> 
> > If I read this correctly, the EIP in the last line corresponds to
> > net/bridge/br_if.c, line 36:
> > 
> >     static int port_cost(struct net_device *dev)
> >     {
> > 	    if (dev->ethtool_ops->get_settings) {
> >                                ^^^^
> > 
> > As far as I can figure out, dev->ethtool_ops is NULL and the crash
> > happens while trying to derefernce ...->get_settings.
> > 
> > Is dev->ethtool_ops allowed to be NULL?  In this case the appended
> > patch might be the correct fix.  At least it makes the oops disappear
> > for me.  Another possible fix would be to add an ethtool_ops structure
> > to the device created by b43.
> 
> I don't think adding ethtool_ops in mac80211 should be necessary.
> Stephen?

Devices aren't required to have ethtool_ops. The code there used to
call ethtool directly, and it would handle the error cases. I'll rollup
a fix this morning.

The bug was introduced by this:

commit 61a44b9c4b20d40c41fd1b70a4ceb13b75ea79a4
Author: Matthew Wilcox <matthew@wil.cx>
Date:   Tue Jul 31 14:00:02 2007 -0700

    [NET]: ethtool ops are the only way
    
    During the transition to the ethtool_ops way of doing things, we supported
    calling the device's ->do_ioctl method to allow unconverted drivers to
    continue working.  Those days are long behind us, all in-tree drivers
    use the ethtool_ops way, and so we no longer need to support this.
    
    The bonding driver is the biggest beneficiary of this; it no longer
    needs to call ioctl() as a fallback if ethtool_ops aren't supported.
    
    Also put a proper copyright statement on ethtool.c.
    
    Signed-off-by: Matthew Wilcox <matthew@wil.cx>
    Signed-off-by: David S. Miller <davem@davemloft.net>

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: oops with recent wireless-dev tree

[Matthew Wilcox]

On Thu, Aug 30, 2007 at 07:49:49AM -0700, Stephen Hemminger wrote:
> > >     static int port_cost(struct net_device *dev)
> > >     {
> > > 	    if (dev->ethtool_ops->get_settings) {
> > >                                ^^^^
> > > 
> > > As far as I can figure out, dev->ethtool_ops is NULL and the crash
> > > happens while trying to derefernce ...->get_settings.
> 
> Devices aren't required to have ethtool_ops. The code there used to
> call ethtool directly, and it would handle the error cases. I'll rollup
> a fix this morning.

Yep, clearly my fault; that should read:

	if (dev->ethtool_ops && dev->ethtool_ops->get_settings) {

Since Stephen's already bagged fixing this, I shan't send a patch.
But if it includes something like the line above please add:

Acked-by: Matthew Wilcox <matthew-Ztpu424NOJ8@public.gmane.org>

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: oops with recent wireless-dev tree

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 262 bytes --]

On Thu, 2007-08-30 at 07:49 -0700, Stephen Hemminger wrote:

> Devices aren't required to have ethtool_ops. The code there used to
> call ethtool directly, and it would handle the error cases. I'll rollup
> a fix this morning.

Great, thanks.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: oops with recent wireless-dev tree

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 342 bytes --]

On Thu, 2007-08-30 at 07:49 -0700, Stephen Hemminger wrote:

> Devices aren't required to have ethtool_ops. The code there used to
> call ethtool directly, and it would handle the error cases. I'll rollup
> a fix this morning.

Great, thanks.

Jochen had a patch: http://marc.info/?l=linux-wireless&m=118842715026614&w=2

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[PATCH] bridge: fix OOPS when bridging device without ethtool

[Stephen Hemminger]

Bridge code calls ethtool to get speed. The conversion to using
only ethtool_ops broke the case of devices without ethtool_ops.
This is a new regression in 2.6.23.

Rearranged the switch to a logical order, and use gcc initializer.

Ps: speed should have been part of the network device structure from
    the start rather than burying it in ethtool.

Signed-off-by: Stephen Hemminger <shemminger-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>


--- a/net/bridge/br_if.c	2007-08-30 07:49:01.000000000 -0700
+++ b/net/bridge/br_if.c	2007-08-30 07:48:16.000000000 -0700
@@ -33,17 +33,17 @@
  */
 static int port_cost(struct net_device *dev)
 {
-	if (dev->ethtool_ops->get_settings) {
-		struct ethtool_cmd ecmd = { ETHTOOL_GSET };
-		int err = dev->ethtool_ops->get_settings(dev, &ecmd);
-		if (!err) {
+	if (dev->ethtool_ops && dev->ethtool_ops->get_settings) {
+		struct ethtool_cmd ecmd = { .cmd = ETHTOOL_GSET, };
+
+		if (!dev->ethtool_ops->get_settings(dev, &ecmd)) {
 			switch(ecmd.speed) {
-			case SPEED_100:
-				return 19;
-			case SPEED_1000:
-				return 4;
 			case SPEED_10000:
 				return 2;
+			case SPEED_1000:
+				return 4;
+			case SPEED_100:
+				return 19;
 			case SPEED_10:
 				return 100;
 			}

Re: oops with recent wireless-dev tree

[Matthew Wilcox]

On Thu, Aug 30, 2007 at 05:01:31PM +0200, Johannes Berg wrote:
> Jochen had a patch: http://marc.info/?l=linux-wireless&m=118842715026614&w=2

That's exactly the right patch, please add

Acked-by: Matthew Wilcox <matthew-Ztpu424NOJ8@public.gmane.org>

I just checked over the commit that introduced the bug, and I didn't
make the same mistake anywhere else.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] bridge: fix OOPS when bridging device without ethtool

[Matthew Wilcox]

On Thu, Aug 30, 2007 at 08:29:32AM -0700, Stephen Hemminger wrote:
> Bridge code calls ethtool to get speed. The conversion to using
> only ethtool_ops broke the case of devices without ethtool_ops.
> This is a new regression in 2.6.23.
> 
> Rearranged the switch to a logical order, and use gcc initializer.
> 
> Ps: speed should have been part of the network device structure from
>     the start rather than burying it in ethtool.

Feel free to do the conversion ;-)  One of the things I like about the
ethtool framework is it gives us a way to take stuff out of the drivers
and put it in the midlayer without disturbing userspace.

> Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

Acked-by: Matthew Wilcox <matthew@wil.cx>

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] bridge: fix OOPS when bridging device without ethtool

[David Miller]

From: Matthew Wilcox <matthew@wil.cx>
Date: Thu, 30 Aug 2007 10:48:13 -0600

> On Thu, Aug 30, 2007 at 08:29:32AM -0700, Stephen Hemminger wrote:
> > Bridge code calls ethtool to get speed. The conversion to using
> > only ethtool_ops broke the case of devices without ethtool_ops.
> > This is a new regression in 2.6.23.
> > 
> > Rearranged the switch to a logical order, and use gcc initializer.
> > 
> > Ps: speed should have been part of the network device structure from
> >     the start rather than burying it in ethtool.
> 
> Feel free to do the conversion ;-)  One of the things I like about the
> ethtool framework is it gives us a way to take stuff out of the drivers
> and put it in the midlayer without disturbing userspace.
> 
> > Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
> 
> Acked-by: Matthew Wilcox <matthew@wil.cx>

Applied, thanks!