[PATCH] netdev: mv643xx_eth: Prevent build on PPC32

[PATCH] netdev: mv643xx_eth: Prevent build on PPC32

[Josh Boyer]

Commit 452503ebc (ARM: Orion: Eth: Add clk/clkdev support.) added use of
the clk driver API which results in compile errors on architectures that
don't implement the clk API.

ERROR: "clk_enable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_disable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_put" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_get_rate" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_get" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!

Selecting CLKDEV_LOOKUP doesn't fix this either, as the build then fails with:

In file included from drivers/clk/clkdev.c:21:0:
include/linux/clkdev.h:15:24: fatal error: asm/clkdev.h: No such file or directory

So we just prevent this from building at all on PPC32.

Signed-off-by: Josh Boyer <jwboyer@redhat.com>
---
 drivers/net/ethernet/marvell/Kconfig |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/Kconfig b/drivers/net/ethernet/marvell/Kconfig
index 0029934..628f5b1 100644
--- a/drivers/net/ethernet/marvell/Kconfig
+++ b/drivers/net/ethernet/marvell/Kconfig
@@ -20,7 +20,7 @@ if NET_VENDOR_MARVELL
 
 config MV643XX_ETH
 	tristate "Marvell Discovery (643XX) and Orion ethernet support"
-	depends on (MV64X60 || PPC32 || PLAT_ORION) && INET
+	depends on (MV64X60 || PLAT_ORION) && INET
 	select INET_LRO
 	select PHYLIB
 	---help---
-- 
1.7.10.2

Re: [PATCH] netdev: mv643xx_eth: Prevent build on PPC32

[Ben Hutchings]

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

On Tue, 2012-06-05 at 15:28 -0400, Josh Boyer wrote:
> Commit 452503ebc (ARM: Orion: Eth: Add clk/clkdev support.) added use of
> the clk driver API which results in compile errors on architectures that
> don't implement the clk API.
> 
> ERROR: "clk_enable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> ERROR: "clk_disable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> ERROR: "clk_put" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> ERROR: "clk_get_rate" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> ERROR: "clk_get" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> 
> Selecting CLKDEV_LOOKUP doesn't fix this either, as the build then fails with:
> 
> In file included from drivers/clk/clkdev.c:21:0:
> include/linux/clkdev.h:15:24: fatal error: asm/clkdev.h: No such file or directory
> 
> So we just prevent this from building at all on PPC32.
[...]

This dependency was introduced by:

commit 16b817579fb61050f1abcc0e81089974328a9c27
Author: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date:   Sat Apr 16 15:24:30 2005 -0700

    [PATCH] ppc32: MV643XX ethernet is an option for Pegasos

commit 06ede91017d015a03cf8c1c87b3ff668f9a846e0
Author: Dale Farnsworth <dale@farnsworth.org>
Date:   Wed Sep 20 12:24:34 2006 -0700

    [PATCH] mv643xx_eth: restrict to 32-bit PPC_MULTIPLATFORM

If Pegasos is still supposed to be supported then this needs to be fixed
properly.

Ben.

-- 
Ben Hutchings
I haven't lost my mind; it's backed up on tape somewhere.

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

Re: [PATCH] netdev: mv643xx_eth: Prevent build on PPC32

[Lennert Buytenhek]

On Tue, Jun 05, 2012 at 03:28:21PM -0400, Josh Boyer wrote:

> Commit 452503ebc (ARM: Orion: Eth: Add clk/clkdev support.) added use of
> the clk driver API which results in compile errors on architectures that
> don't implement the clk API.
> 
> ERROR: "clk_enable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> ERROR: "clk_disable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> ERROR: "clk_put" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> ERROR: "clk_get_rate" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> ERROR: "clk_get" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> 
> Selecting CLKDEV_LOOKUP doesn't fix this either, as the build then fails with:
> 
> In file included from drivers/clk/clkdev.c:21:0:
> include/linux/clkdev.h:15:24: fatal error: asm/clkdev.h: No such file or directory
> 
> So we just prevent this from building at all on PPC32.

If the PPC32 dependency is no longer relevant (e.g. if Pegasos platform
support was removed from the kernel), then the commit message should
mention that -- the above reasoning is a poor sole justification for
this change.

Re: [PATCH] netdev: mv643xx_eth: Prevent build on PPC32

[Josh Boyer]

On Wed, Jun 06, 2012 at 12:30:05AM +0100, Ben Hutchings wrote:
> On Tue, 2012-06-05 at 15:28 -0400, Josh Boyer wrote:
> > Commit 452503ebc (ARM: Orion: Eth: Add clk/clkdev support.) added use of
> > the clk driver API which results in compile errors on architectures that
> > don't implement the clk API.
> > 
> > ERROR: "clk_enable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> > ERROR: "clk_disable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> > ERROR: "clk_put" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> > ERROR: "clk_get_rate" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> > ERROR: "clk_get" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> > 
> > Selecting CLKDEV_LOOKUP doesn't fix this either, as the build then fails with:
> > 
> > In file included from drivers/clk/clkdev.c:21:0:
> > include/linux/clkdev.h:15:24: fatal error: asm/clkdev.h: No such file or directory
> > 
> > So we just prevent this from building at all on PPC32.
> [...]
> 
> This dependency was introduced by:
> 
> commit 16b817579fb61050f1abcc0e81089974328a9c27
> Author: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Date:   Sat Apr 16 15:24:30 2005 -0700
> 
>     [PATCH] ppc32: MV643XX ethernet is an option for Pegasos
> 
> commit 06ede91017d015a03cf8c1c87b3ff668f9a846e0
> Author: Dale Farnsworth <dale@farnsworth.org>
> Date:   Wed Sep 20 12:24:34 2006 -0700
> 
>     [PATCH] mv643xx_eth: restrict to 32-bit PPC_MULTIPLATFORM
> 
> If Pegasos is still supposed to be supported then this needs to be fixed
> properly.

The proper fix, from my minimal looking, was one of:

1) revert the change for ARM that introduced th clk stuff
2) do a similar change as the original commit but with a bunch of
#ifdef-ery
3) implement the clkdev API stuff for 32-bit ppc

Honestly, I'd go for either 1 or 2.  The commit that introduced it was
broken to begin with, but that isn't my call.

josh

Re: [PATCH] netdev: mv643xx_eth: Prevent build on PPC32

[Josh Boyer]

On Wed, Jun 06, 2012 at 02:49:07AM +0200, Lennert Buytenhek wrote:
> On Tue, Jun 05, 2012 at 03:28:21PM -0400, Josh Boyer wrote:
> 
> > Commit 452503ebc (ARM: Orion: Eth: Add clk/clkdev support.) added use of
> > the clk driver API which results in compile errors on architectures that
> > don't implement the clk API.
> > 
> > ERROR: "clk_enable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> > ERROR: "clk_disable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> > ERROR: "clk_put" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> > ERROR: "clk_get_rate" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> > ERROR: "clk_get" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> > 
> > Selecting CLKDEV_LOOKUP doesn't fix this either, as the build then fails with:
> > 
> > In file included from drivers/clk/clkdev.c:21:0:
> > include/linux/clkdev.h:15:24: fatal error: asm/clkdev.h: No such file or directory
> > 
> > So we just prevent this from building at all on PPC32.
> 
> If the PPC32 dependency is no longer relevant (e.g. if Pegasos platform
> support was removed from the kernel), then the commit message should
> mention that -- the above reasoning is a poor sole justification for
> this change.

You are correct.  As it stands, it's no better than just breaking it
outright with commit 452503ebc.  I've described the 3 possible solutions
in my other reply.

josh

Re: [PATCH] netdev: mv643xx_eth: Prevent build on PPC32

[Lennert Buytenhek]

On Tue, Jun 05, 2012 at 10:40:14PM -0400, Josh Boyer wrote:

> > > Commit 452503ebc (ARM: Orion: Eth: Add clk/clkdev support.) added use of
> > > the clk driver API which results in compile errors on architectures that
> > > don't implement the clk API.
> > > 
> > > ERROR: "clk_enable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> > > ERROR: "clk_disable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> > > ERROR: "clk_put" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> > > ERROR: "clk_get_rate" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> > > ERROR: "clk_get" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
> > > 
> > > Selecting CLKDEV_LOOKUP doesn't fix this either, as the build then fails with:
> > > 
> > > In file included from drivers/clk/clkdev.c:21:0:
> > > include/linux/clkdev.h:15:24: fatal error: asm/clkdev.h: No such file or directory
> > > 
> > > So we just prevent this from building at all on PPC32.
> > 
> > If the PPC32 dependency is no longer relevant (e.g. if Pegasos platform
> > support was removed from the kernel), then the commit message should
> > mention that -- the above reasoning is a poor sole justification for
> > this change.
> 
> You are correct.  As it stands, it's no better than just breaking it
> outright with commit 452503ebc.

ACK.  If I'd have seen that commit ("ARM: Orion: Eth: Add clk/clkdev
support.") come by I would have said something about it, but noone
bothered to CC me on it -- and it doesn't seem that it was CCed to
netdev@ either...?

Re: [PATCH] netdev: mv643xx_eth: Prevent build on PPC32

[Andrew Lunn]

> The proper fix, from my minimal looking, was one of:
> 
> 1) revert the change for ARM that introduced th clk stuff
> 2) do a similar change as the original commit but with a bunch of
> #ifdef-ery
> 3) implement the clkdev API stuff for 32-bit ppc
> 
> Honestly, I'd go for either 1 or 2.  The commit that introduced it was
> broken to begin with, but that isn't my call.

I broke it. Sorry.

At the time, there was a push to remove all the #ifdefs. The following
patchset was doing this:

https://lkml.org/lkml/2012/4/21/94

it would provide dummy implementations for those systems without clk
support. However, it seems that patch set never made it in, and i did
not declare my dependency on it.

I'm happy to add #ifdef. However, i would first like to understand
what was 'broken to begin with'.

     Thanks
	Andrew

Re: [PATCH] netdev: mv643xx_eth: Prevent build on PPC32

[Josh Boyer]

On Wed, Jun 06, 2012 at 07:29:10AM +0200, Andrew Lunn wrote:
> > The proper fix, from my minimal looking, was one of:
> > 
> > 1) revert the change for ARM that introduced th clk stuff
> > 2) do a similar change as the original commit but with a bunch of
> > #ifdef-ery
> > 3) implement the clkdev API stuff for 32-bit ppc
> > 
> > Honestly, I'd go for either 1 or 2.  The commit that introduced it was
> > broken to begin with, but that isn't my call.
> 
> I broke it. Sorry.
> 
> At the time, there was a push to remove all the #ifdefs. The following
> patchset was doing this:
> 
> https://lkml.org/lkml/2012/4/21/94
> 
> it would provide dummy implementations for those systems without clk
> support. However, it seems that patch set never made it in, and i did
> not declare my dependency on it.
> 
> I'm happy to add #ifdef. However, i would first like to understand
> what was 'broken to begin with'.

Simply that a commit was introduced that did not build on all the
existing platforms the driver supports.  The world is not ARM, or x86,
or PPC32, etc.  I haven't looked to see if it would still function
correctly in the presence of a dummy clk implementation, but if not that
would also be bad.

josh

Re: [PATCH] netdev: mv643xx_eth: Prevent build on PPC32

[Mark Brown]

On Tue, Jun 05, 2012 at 10:38:42PM -0400, Josh Boyer wrote:

> 1) revert the change for ARM that introduced th clk stuff
> 2) do a similar change as the original commit but with a bunch of
> #ifdef-ery
> 3) implement the clkdev API stuff for 32-bit ppc

> Honestly, I'd go for either 1 or 2.  The commit that introduced it was
> broken to begin with, but that isn't my call.

There's a change going in which stubs out the clock API when not used
which should resolve the immediate issue, though really the best thing
here is just to enable use of the generic clock API if the platform
doesn't have one of its own - it's not just platforms that need clocks
so we really want to get that rolled out as widely as possible.

This sort of issue is just the tip of the iceberg in terms of what it's
useful to do with the API.

Re: [PATCH] netdev: mv643xx_eth: Prevent build on PPC32

[Josh Boyer]

On Fri, Jun 08, 2012 at 12:51:15AM +0100, Mark Brown wrote:
> On Tue, Jun 05, 2012 at 10:38:42PM -0400, Josh Boyer wrote:
> 
> > 1) revert the change for ARM that introduced th clk stuff
> > 2) do a similar change as the original commit but with a bunch of
> > #ifdef-ery
> > 3) implement the clkdev API stuff for 32-bit ppc
> 
> > Honestly, I'd go for either 1 or 2.  The commit that introduced it was
> > broken to begin with, but that isn't my call.
> 
> There's a change going in which stubs out the clock API when not used
> which should resolve the immediate issue, though really the best thing
> here is just to enable use of the generic clock API if the platform
> doesn't have one of its own - it's not just platforms that need clocks
> so we really want to get that rolled out as widely as possible.

Sounds great.  I have no objections with any of those plans.

> This sort of issue is just the tip of the iceberg in terms of what it's
> useful to do with the API.

Yes, sounds like it.  All I ask is that people test their patches along
the way so things don't get broken.  I mean, it's great we have an
iceberg but I don't want tons of drivers on other architectures running
into the thing and sinking because people aren't being careful.  Except
maybe the one already appropriately (nick)named.

josh

Re: [PATCH] netdev: mv643xx_eth: Prevent build on PPC32

[Mark Brown]

On Thu, Jun 07, 2012 at 07:55:51PM -0400, Josh Boyer wrote:
> On Fri, Jun 08, 2012 at 12:51:15AM +0100, Mark Brown wrote:

> > This sort of issue is just the tip of the iceberg in terms of what it's
> > useful to do with the API.

> Yes, sounds like it.  All I ask is that people test their patches along
> the way so things don't get broken.  I mean, it's great we have an
> iceberg but I don't want tons of drivers on other architectures running
> into the thing and sinking because people aren't being careful.  Except
> maybe the one already appropriately (nick)named.

It's really hard to blame the submitters here - this really isn't the
sort of API that you'd expect to only be available conditionally so this
isn't something that one would expect to have to worry about.  It's a
product of the age of the clock API and the glacial progress on the
generic clock API.

Re: [PATCH] netdev: mv643xx_eth: Prevent build on PPC32

[Josh Boyer]

On Fri, Jun 08, 2012 at 01:34:45AM +0100, Mark Brown wrote:
> On Thu, Jun 07, 2012 at 07:55:51PM -0400, Josh Boyer wrote:
> > On Fri, Jun 08, 2012 at 12:51:15AM +0100, Mark Brown wrote:
> 
> > > This sort of issue is just the tip of the iceberg in terms of what it's
> > > useful to do with the API.
> 
> > Yes, sounds like it.  All I ask is that people test their patches along
> > the way so things don't get broken.  I mean, it's great we have an
> > iceberg but I don't want tons of drivers on other architectures running
> > into the thing and sinking because people aren't being careful.  Except
> > maybe the one already appropriately (nick)named.
> 
> It's really hard to blame the submitters here - this really isn't the
> sort of API that you'd expect to only be available conditionally so this
> isn't something that one would expect to have to worry about.  It's a
> product of the age of the clock API and the glacial progress on the
> generic clock API.

I'm not placing blame.  I'm declaring people should be cautious going
forward.  5 arches have the clock API.  21 don't.  Whatever reasons
there are for that, I don't care.  It should be a big warning sign.

It might even be beneficial to put some Kconfig dependencies on both
CONFIG_COMMON_CLK (which is somewhat misleadingly named) and
CONFIG_CLKDEV_LOOKUP so those are only selectable on those 5 arches.
Something like:

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 4864407..3f49c22 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -1,6 +1,7 @@
 
 config CLKDEV_LOOKUP
 	bool
+	depends on (ARM || SUPERH || MIPS || C6X || BLACKFIN)
 	select HAVE_CLK
 
 config HAVE_CLK_PREPARE
@@ -11,6 +12,7 @@ config HAVE_MACH_CLKDEV
 
 config COMMON_CLK
 	bool
+	depends on (ARM || SUPERH || MIPS || C6X || BLACKFIN)
 	select HAVE_CLK_PREPARE
 	select CLKDEV_LOOKUP
 	---help---

Regardless, hopefully things like this will get hit in linux-next in the
future.  I believe the only reason that it wasn't this time is that
none of the PPC defconfigs build in linux-next bother to build the
driver at all.

josh

Re: [PATCH] netdev: mv643xx_eth: Prevent build on PPC32

[Mark Brown]

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

On Thu, Jun 07, 2012 at 09:04:03PM -0400, Josh Boyer wrote:

> I'm not placing blame.  I'm declaring people should be cautious going
> forward.  5 arches have the clock API.  21 don't.  Whatever reasons
> there are for that, I don't care.  It should be a big warning sign.

My point here is that it's a warning sign for the API, not really for
the drivers that use it.

> It might even be beneficial to put some Kconfig dependencies on both
> CONFIG_COMMON_CLK (which is somewhat misleadingly named) and
> CONFIG_CLKDEV_LOOKUP so those are only selectable on those 5 arches.
> Something like:

>  config CLKDEV_LOOKUP
>  	bool
> +	depends on (ARM || SUPERH || MIPS || C6X || BLACKFIN)
>  	select HAVE_CLK

This is a really bad approach.  It's sending totally the wrong message
about where we want to be (we want to have the clock API available
everywhere) and more importantly it still means that drivers need to go
on carrying around ifdefery or unhelpful dependencies which is just lots
of pointless work.  A very large proportion of the drivers that use
clocks are just making sure clocks are enabled when the device is active
to integrate with system wide power optimisation and don't actually care
if there are clocks there at all, we should be making their life as easy
as possible.

A much better approach is get the stubs mentioned earlier merged
(they're already on their way) faster.  That way there are no compile
time dependencies and the problem goes away unless the driver is doing
something more active with clocks like managing the clock rate.

In the case of CLKDEV_LOOKUP the symbol should only be selected by an
architecture anyway, it's a layer on top of the architecture clock code.

> Regardless, hopefully things like this will get hit in linux-next in the
> future.  I believe the only reason that it wasn't this time is that
> none of the PPC defconfigs build in linux-next bother to build the
> driver at all.

They do generally, people do randconfig and allXconfig builds all the
time.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]