Patchwork Yet more fixes to etherh.c

login
register
mail settings
Submitter Russell King - ARM Linux
Date Feb. 21, 2009, 7:36 p.m.
Message ID <20090221193633.GD16596@n2100.arm.linux.org.uk>
Download mbox | patch
Permalink /patch/23519/
State Accepted
Delegated to: David Miller
Headers show

Comments

Russell King - ARM Linux - Feb. 21, 2009, 7:36 p.m.
Further to a71558d, this is round five of fixes to make etherh work
again. As mainline kernels stand, the fixes in b9a9b4b were the wrong
approach.

The 8390 driver was structured by Al Viro to allow the flexibility required
by platforms.  lib8390.c contains the core code which drivers explicitly
include:
- 8390.c includes lib8390.c to provide the standard ISA based driver.
- etherh.c includes it with the accessors defined for RiscPC platforms,
  where it is addressed via the MMIO accessors with a device dependent
  register spacing.

Other platform drivers do something similar.

However, b9a9b4b caused the kernel to contain not only the etherh private
build of lib8390 (included in etherh.c) but also lib8390.c itself, and
referred the new net_device_ops methods to the ISA version.  The result
of this is is not pretty:

Unable to handle kernel paging request at virtual address 12032030
pgd = c8330000
[12032030] *pgd=00000000
Internal error: Oops: 18331805 [#1]
Modules linked in: ipv6
CPU: 0    Not tainted  (2.6.29-rc3 #167)
PC is at do_set_multicast_list+0xd0/0x190
LR is at bitrev32+0x28/0x34
pc : [<c017aab4>]    lr : [<c0139120>]    psr: a0000093
sp : c8321d9c  ip : c8321d84  fp : c8321dbc
r10: c80c6800  r9 : 00000000  r8 : c80c6b60
r7 : c80c6b80  r6 : cc80c800  r5 : c80c6800  r4 : 00000000
r3 : cc80c80c  r2 : 00000004  r1 : 00000007  r0 : e0000000
Flags: NzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
...

Fix up b9a9b4b by making etherh's net_device_ops refer to the internal
lib8390 functions, and remove the build of the ISA 8390.c driver.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

--
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
David Miller - Feb. 22, 2009, 7:44 a.m.
From: Russell King - ARM Linux <linux@arm.linux.org.uk>
Date: Sat, 21 Feb 2009 19:36:33 +0000

> Further to a71558d, this is round five of fixes to make etherh work
> again. As mainline kernels stand, the fixes in b9a9b4b were the wrong
> approach.
 ...
> Fix up b9a9b4b by making etherh's net_device_ops refer to the internal
> lib8390 functions, and remove the build of the ISA 8390.c driver.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

Applied to net-next-2.6, thanks Russell.

You mention this as a fix against mainline, but your patch
only applied to net-next-2.6 because the "eth_set_mac_addr"
fix to this driver only exists there.

Let me know if you want me to propagate both changes to net-2.6
and push to Linus.
--
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
Russell King - ARM Linux - Feb. 22, 2009, 8:19 a.m.
On Sat, Feb 21, 2009 at 11:44:48PM -0800, David Miller wrote:
> From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Date: Sat, 21 Feb 2009 19:36:33 +0000
> 
> > Further to a71558d, this is round five of fixes to make etherh work
> > again. As mainline kernels stand, the fixes in b9a9b4b were the wrong
> > approach.
>  ...
> > Fix up b9a9b4b by making etherh's net_device_ops refer to the internal
> > lib8390 functions, and remove the build of the ISA 8390.c driver.
> > 
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
> Applied to net-next-2.6, thanks Russell.
> 
> You mention this as a fix against mainline, but your patch
> only applied to net-next-2.6 because the "eth_set_mac_addr"
> fix to this driver only exists there.

Hmm, I don't see the problem.  What's currently in mainline is:

        .ndo_set_mac_address    = eth_mac_addr,

which is what's in the context lines of this patch.  I pushed this
myself in a71558d.

> Let me know if you want me to propagate both changes to net-2.6
> and push to Linus.

Definitely, these patches are fixing regressions, so should be submitted
to Linus for 2.6.29.
--
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
David Miller - Feb. 22, 2009, 8:24 a.m.
From: Russell King - ARM Linux <linux@arm.linux.org.uk>
Date: Sun, 22 Feb 2009 08:19:47 +0000

> On Sat, Feb 21, 2009 at 11:44:48PM -0800, David Miller wrote:
> > From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> > Date: Sat, 21 Feb 2009 19:36:33 +0000
> > 
> > > Further to a71558d, this is round five of fixes to make etherh work
> > > again. As mainline kernels stand, the fixes in b9a9b4b were the wrong
> > > approach.
> >  ...
> > > Fix up b9a9b4b by making etherh's net_device_ops refer to the internal
> > > lib8390 functions, and remove the build of the ISA 8390.c driver.
> > > 
> > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > 
> > Applied to net-next-2.6, thanks Russell.
> > 
> > You mention this as a fix against mainline, but your patch
> > only applied to net-next-2.6 because the "eth_set_mac_addr"
> > fix to this driver only exists there.
> 
> Hmm, I don't see the problem.  What's currently in mainline is:
> 
>         .ndo_set_mac_address    = eth_mac_addr,

Which didn't go in via the net-2.6 tree, sigh... :-/

Russell, pick your transport medium, either send ARM network driver
fixes via me or straight to Linus.

Not some mixture of both, that's only going to lead to confusion,
just like it did here.

I put that "eth_mac_addr" fix into net-next-2.6, and you then sent it
straight to Linus.
--
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
Russell King - ARM Linux - Feb. 22, 2009, 8:45 a.m.
On Sun, Feb 22, 2009 at 12:24:14AM -0800, David Miller wrote:
> From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Date: Sun, 22 Feb 2009 08:19:47 +0000
> 
> > On Sat, Feb 21, 2009 at 11:44:48PM -0800, David Miller wrote:
> > > From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> > > Date: Sat, 21 Feb 2009 19:36:33 +0000
> > > 
> > > > Further to a71558d, this is round five of fixes to make etherh work
> > > > again. As mainline kernels stand, the fixes in b9a9b4b were the wrong
> > > > approach.
> > >  ...
> > > > Fix up b9a9b4b by making etherh's net_device_ops refer to the internal
> > > > lib8390 functions, and remove the build of the ISA 8390.c driver.
> > > > 
> > > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > > 
> > > Applied to net-next-2.6, thanks Russell.
> > > 
> > > You mention this as a fix against mainline, but your patch
> > > only applied to net-next-2.6 because the "eth_set_mac_addr"
> > > fix to this driver only exists there.
> > 
> > Hmm, I don't see the problem.  What's currently in mainline is:
> > 
> >         .ndo_set_mac_address    = eth_mac_addr,
> 
> Which didn't go in via the net-2.6 tree, sigh... :-/
> 
> Russell, pick your transport medium, either send ARM network driver
> fixes via me or straight to Linus.
> 
> Not some mixture of both, that's only going to lead to confusion,
> just like it did here.
> 
> I put that "eth_mac_addr" fix into net-next-2.6, and you then sent it
> straight to Linus.

Hmm, so someone else submitted the same fix for that regression caused
by fe96aaa.

Given that the eth_mac_addr change is a regression fix, the question has
to be asked: why is it queued for the next merge window?

In any case, I'm more than willing to push this through the ARM tree, but
at the same time I'm aware that people get upset if they're not copied on
the patches.  That's why I CC'd you with 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
David Miller - Feb. 22, 2009, 10:39 a.m.
From: Russell King - ARM Linux <linux@arm.linux.org.uk>
Date: Sun, 22 Feb 2009 08:45:58 +0000

> On Sun, Feb 22, 2009 at 12:24:14AM -0800, David Miller wrote:
> > From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> > Date: Sun, 22 Feb 2009 08:19:47 +0000
> > 
> > > Hmm, I don't see the problem.  What's currently in mainline is:
> > > 
> > >         .ndo_set_mac_address    = eth_mac_addr,
> > 
> > Which didn't go in via the net-2.6 tree, sigh... :-/
> > 
> > Russell, pick your transport medium, either send ARM network driver
> > fixes via me or straight to Linus.
> > 
> > Not some mixture of both, that's only going to lead to confusion,
> > just like it did here.
> > 
> > I put that "eth_mac_addr" fix into net-next-2.6, and you then sent it
> > straight to Linus.
> 
> Hmm, so someone else submitted the same fix for that regression caused
> by fe96aaa.

That someone else was you:

commit 5376071069ec8a7e6a8112beab16fc24f5139475
 ...
    Merge master.kernel.org:/home/rmk/linux-2.6-arm
    
    * master.kernel.org:/home/rmk/linux-2.6-arm: (22 commits)

which brought in:

commit a71558d0eca1bbb23737f832297926666f9b36db
Author: Russell King <rmk@dyn-67.arm.linux.org.uk>
Date:   Tue Jan 27 22:32:29 2009 +0000

    [ARM] etherh: continue fixing build failure
    
    Further to 483a2b3a3182abcb7fcea986d7ea13e793bb00b1, also fix:
    
    drivers/net/arm/etherh.c:649: error: 'eth_set_mac_addr' undeclared here (not in a function)
    
    Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

This was my entire point.

> Given that the eth_mac_addr change is a regression fix, the question has
> to be asked: why is it queued for the next merge window?

Because I thought the regression only existed in net-next-2.6,
probably due to poor communication from the patch submitter :)

> In any case, I'm more than willing to push this through the ARM tree, but
> at the same time I'm aware that people get upset if they're not copied on
> the patches.  That's why I CC'd you with it.

All you need to do is explicitly tell me where a bug fix goes,
and I can get it into Linus's tree in less than a day.

It's all about communication and not doing things like submitting
changes behind my back after I've explicitly replied with an
email saying "Applied" to your patch.
--
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
Russell King - ARM Linux - Feb. 22, 2009, 11:46 a.m.
On Sun, Feb 22, 2009 at 02:39:33AM -0800, David Miller wrote:
> From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Date: Sun, 22 Feb 2009 08:45:58 +0000
> 
> > On Sun, Feb 22, 2009 at 12:24:14AM -0800, David Miller wrote:
> > > From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> > > Date: Sun, 22 Feb 2009 08:19:47 +0000
> > > 
> > > > Hmm, I don't see the problem.  What's currently in mainline is:
> > > > 
> > > >         .ndo_set_mac_address    = eth_mac_addr,
> > > 
> > > Which didn't go in via the net-2.6 tree, sigh... :-/
> > > 
> > > Russell, pick your transport medium, either send ARM network driver
> > > fixes via me or straight to Linus.
> > > 
> > > Not some mixture of both, that's only going to lead to confusion,
> > > just like it did here.
> > > 
> > > I put that "eth_mac_addr" fix into net-next-2.6, and you then sent it
> > > straight to Linus.
> > 
> > Hmm, so someone else submitted the same fix for that regression caused
> > by fe96aaa.
> 
> That someone else was you:
> 
> commit 5376071069ec8a7e6a8112beab16fc24f5139475
>  ...
>     Merge master.kernel.org:/home/rmk/linux-2.6-arm
>     
>     * master.kernel.org:/home/rmk/linux-2.6-arm: (22 commits)
> 
> which brought in:
> 
> commit a71558d0eca1bbb23737f832297926666f9b36db
> Author: Russell King <rmk@dyn-67.arm.linux.org.uk>
> Date:   Tue Jan 27 22:32:29 2009 +0000
> 
>     [ARM] etherh: continue fixing build failure
>     
>     Further to 483a2b3a3182abcb7fcea986d7ea13e793bb00b1, also fix:
>     
>     drivers/net/arm/etherh.c:649: error: 'eth_set_mac_addr' undeclared here (not in a function)
>     
>     Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
> This was my entire point.

Yes, I know I put that into mainline via my tree...

> > Given that the eth_mac_addr change is a regression fix, the question has
> > to be asked: why is it queued for the next merge window?
> 
> Because I thought the regression only existed in net-next-2.6,
> probably due to poor communication from the patch submitter :)
> 
> > In any case, I'm more than willing to push this through the ARM tree, but
> > at the same time I'm aware that people get upset if they're not copied on
> > the patches.  That's why I CC'd you with it.
> 
> All you need to do is explicitly tell me where a bug fix goes,
> and I can get it into Linus's tree in less than a day.
> 
> It's all about communication and not doing things like submitting
> changes behind my back after I've explicitly replied with an
> email saying "Applied" to your patch.

... but I must have forgotten that I'd already sent it to you.  I don't
seem to have a record of sending it.

What I do have is that I raised the issue of the ndo_set_mac_addr in
January on netdev.  To that I got a reply from Stephen about it, to
which I asked whether he wanted me to fix it via my tree.  The response
I got in private was "yes" (and I won't reveal the rest of it because
it refers to his travel around that time.)

In reply to that I sent Stephen a patch, which being a reply to a private
message couldn't be CC'd back to netdev.

However, coincidentally, it seems that you fixed the precise error I
raised (thanks) but missed the eth_set_mac_addr mis-spelling, so I dropped
my original patch.  I didn't get any further response from on the issue,
so I'd assumed two weeks later that it was dead and fixed the remaining
issue.

So, as far as I can see, I never sent you a patch for to fix the
eth_set_mac_addr -> eth_mac_addr change.  If you have that in your
tree, someone else must have sent it to you (maybe Stephen forwarded
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
David Miller - Feb. 22, 2009, 12:29 p.m.
From: Russell King - ARM Linux <linux@arm.linux.org.uk>
Date: Sun, 22 Feb 2009 11:46:01 +0000

> So, as far as I can see, I never sent you a patch for to fix the
> eth_set_mac_addr -> eth_mac_addr change.  If you have that in your
> tree, someone else must have sent it to you (maybe Stephen forwarded
> it?)

Nevermind, I see what happened, I got your commit when I merged
upstream into net-next-2.6 to resolve some merge conflicts.

Ok, I'll revert today's patch from my tree and let it propagate via
your's.
--
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
Russell King - ARM Linux - Feb. 22, 2009, 12:34 p.m.
On Sun, Feb 22, 2009 at 04:29:52AM -0800, David Miller wrote:
> From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Date: Sun, 22 Feb 2009 11:46:01 +0000
> 
> > So, as far as I can see, I never sent you a patch for to fix the
> > eth_set_mac_addr -> eth_mac_addr change.  If you have that in your
> > tree, someone else must have sent it to you (maybe Stephen forwarded
> > it?)
> 
> Nevermind, I see what happened, I got your commit when I merged
> upstream into net-next-2.6 to resolve some merge conflicts.
> 
> Ok, I'll revert today's patch from my tree and let it propagate via
> your's.

I don't mind how it gets in, as long as it does.  Since you seem happy
for it to propagate via mine, can I assume you're acking 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
David Miller - Feb. 22, 2009, 12:36 p.m.
From: Russell King - ARM Linux <linux@arm.linux.org.uk>
Date: Sun, 22 Feb 2009 12:34:58 +0000

> On Sun, Feb 22, 2009 at 04:29:52AM -0800, David Miller wrote:
> > From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> > Date: Sun, 22 Feb 2009 11:46:01 +0000
> > 
> > > So, as far as I can see, I never sent you a patch for to fix the
> > > eth_set_mac_addr -> eth_mac_addr change.  If you have that in your
> > > tree, someone else must have sent it to you (maybe Stephen forwarded
> > > it?)
> > 
> > Nevermind, I see what happened, I got your commit when I merged
> > upstream into net-next-2.6 to resolve some merge conflicts.
> > 
> > Ok, I'll revert today's patch from my tree and let it propagate via
> > your's.
> 
> I don't mind how it gets in, as long as it does.  Since you seem happy
> for it to propagate via mine, can I assume you're acking it?

Yep

Acked-by: David S. Miller <davem@davemloft.net>
--
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
Russell King - ARM Linux - Feb. 22, 2009, 12:39 p.m.
On Sun, Feb 22, 2009 at 04:36:47AM -0800, David Miller wrote:
> From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Date: Sun, 22 Feb 2009 12:34:58 +0000
> > I don't mind how it gets in, as long as it does.  Since you seem happy
> > for it to propagate via mine, can I assume you're acking it?
> 
> Yep
> 
> Acked-by: David S. Miller <davem@davemloft.net>

Thanks, applied.  Should hit Linus' tree around Thursday.
--
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

Patch

diff --git a/drivers/net/arm/Makefile b/drivers/net/arm/Makefile
index c69c0cd..811a3cc 100644
--- a/drivers/net/arm/Makefile
+++ b/drivers/net/arm/Makefile
@@ -4,7 +4,7 @@ 
 #
 
 obj-$(CONFIG_ARM_AM79C961A)	+= am79c961a.o
-obj-$(CONFIG_ARM_ETHERH)	+= etherh.o ../8390.o
+obj-$(CONFIG_ARM_ETHERH)	+= etherh.o
 obj-$(CONFIG_ARM_ETHER3)	+= ether3.o
 obj-$(CONFIG_ARM_ETHER1)	+= ether1.o
 obj-$(CONFIG_ARM_AT91_ETHER)	+= at91_ether.o
diff --git a/drivers/net/arm/etherh.c b/drivers/net/arm/etherh.c
index 54b52e5..f52f668 100644
--- a/drivers/net/arm/etherh.c
+++ b/drivers/net/arm/etherh.c
@@ -641,15 +641,15 @@  static const struct net_device_ops etherh_netdev_ops = {
 	.ndo_open		= etherh_open,
 	.ndo_stop		= etherh_close,
 	.ndo_set_config		= etherh_set_config,
-	.ndo_start_xmit		= ei_start_xmit,
-	.ndo_tx_timeout		= ei_tx_timeout,
-	.ndo_get_stats		= ei_get_stats,
-	.ndo_set_multicast_list = ei_set_multicast_list,
+	.ndo_start_xmit		= __ei_start_xmit,
+	.ndo_tx_timeout		= __ei_tx_timeout,
+	.ndo_get_stats		= __ei_get_stats,
+	.ndo_set_multicast_list = __ei_set_multicast_list,
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_set_mac_address	= eth_mac_addr,
 	.ndo_change_mtu		= eth_change_mtu,
 #ifdef CONFIG_NET_POLL_CONTROLLER
-	.ndo_poll_controller	= ei_poll,
+	.ndo_poll_controller	= __ei_poll,
 #endif
 };