[PATCH] edac, highbank: remove dependency on ARCH_HIGHBANK

[PATCH] edac, highbank: remove dependency on ARCH_HIGHBANK

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

In order to enable on arm64 and improve the build coverage, remove the
dependency on ARCH_HIGHBANK.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
Cc: Doug Thompson <dougthompson@xmission.com>
Cc: Robert Richter <rric@kernel.org>
Cc: linux-edac@vger.kernel.org
---
 drivers/edac/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 878f090..fc8aaaf 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -328,14 +328,14 @@ config EDAC_TILE
 
 config EDAC_HIGHBANK_MC
 	tristate "Highbank Memory Controller"
-	depends on EDAC_MM_EDAC && ARCH_HIGHBANK
+	depends on EDAC_MM_EDAC
 	help
 	  Support for error detection and correction on the
 	  Calxeda Highbank memory controller.
 
 config EDAC_HIGHBANK_L2
 	tristate "Highbank L2 Cache"
-	depends on EDAC_MM_EDAC && ARCH_HIGHBANK
+	depends on EDAC_MM_EDAC
 	help
 	  Support for error detection and correction on the
 	  Calxeda Highbank memory controller.
-- 
1.8.1.2

Re: [PATCH] edac, highbank: remove dependency on ARCH_HIGHBANK

[Borislav Petkov]

On Tue, Nov 05, 2013 at 09:25:52PM -0600, Rob Herring wrote:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> In order to enable on arm64 and improve the build coverage, remove the
> dependency on ARCH_HIGHBANK.
> 
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> Cc: Doug Thompson <dougthompson@xmission.com>
> Cc: Robert Richter <rric@kernel.org>
> Cc: linux-edac@vger.kernel.org
> ---
>  drivers/edac/Kconfig | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 878f090..fc8aaaf 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -328,14 +328,14 @@ config EDAC_TILE
>  
>  config EDAC_HIGHBANK_MC
>  	tristate "Highbank Memory Controller"
> -	depends on EDAC_MM_EDAC && ARCH_HIGHBANK
> +	depends on EDAC_MM_EDAC

Please, no, this'll get enabled on other arches where this driver is
simply N/A. It should only depend on the ARM arches it supports.

-- 
Regards/Gruss,
Boris.

Re: [PATCH] edac, highbank: remove dependency on ARCH_HIGHBANK

[Rob Herring]

On 11/06/2013 08:20 AM, Borislav Petkov wrote:
> On Tue, Nov 05, 2013 at 09:25:52PM -0600, Rob Herring wrote:
>> From: Rob Herring <rob.herring@calxeda.com>
>>
>> In order to enable on arm64 and improve the build coverage, remove the
>> dependency on ARCH_HIGHBANK.
>>
>> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
>> Cc: Doug Thompson <dougthompson@xmission.com>
>> Cc: Robert Richter <rric@kernel.org>
>> Cc: linux-edac@vger.kernel.org
>> ---
>>  drivers/edac/Kconfig | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
>> index 878f090..fc8aaaf 100644
>> --- a/drivers/edac/Kconfig
>> +++ b/drivers/edac/Kconfig
>> @@ -328,14 +328,14 @@ config EDAC_TILE
>>  
>>  config EDAC_HIGHBANK_MC
>>  	tristate "Highbank Memory Controller"
>> -	depends on EDAC_MM_EDAC && ARCH_HIGHBANK
>> +	depends on EDAC_MM_EDAC
> 
> Please, no, this'll get enabled on other arches where this driver is
> simply N/A. It should only depend on the ARM arches it supports.

Other maintainers disagree because it reduces build coverage. Having
done some cross arch clean-up, I have to agree with that position.
Iterating builds over different arches is bad enough, but iterating over
all configs for each arch is a pain. It would be nice to have a uniform
policy here.

Rob

Re: [PATCH] edac, highbank: remove dependency on ARCH_HIGHBANK

[Borislav Petkov]

On Wed, Nov 06, 2013 at 10:15:04AM -0600, Rob Herring wrote:
> Other maintainers disagree because it reduces build coverage.

What possible build coverage are we talking about when building highbank
on x86, for example? And what does a build failure in that configuration
tell you if it builds and boots fine on your hardware?

> Having done some cross arch clean-up, I have to agree with that
> position. Iterating builds over different arches is bad enough, but
> iterating over all configs for each arch is a pain.

So make highbank depend only on the arches for which it is supposed to
work but not for *all* arches. Look at how the other edac drivers do it,
x86 ones depend on X86, tile on tile, etc, etc. Why should highbank be
special?

> It would be nice to have a uniform policy here.

Simple: drivers should be selectable/buildable only on the arches
they're supposed to run on.

-- 
Regards/Gruss,
Boris.

Re: [PATCH] edac, highbank: remove dependency on ARCH_HIGHBANK

[Rob Herring]

On 11/06/2013 10:54 AM, Borislav Petkov wrote:
> On Wed, Nov 06, 2013 at 10:15:04AM -0600, Rob Herring wrote:
>> Other maintainers disagree because it reduces build coverage.
> 
> What possible build coverage are we talking about when building highbank
> on x86, for example? And what does a build failure in that configuration
> tell you if it builds and boots fine on your hardware?

Using some interface which is arch specific and should not be used in a
driver. We used to have tons of mach dependencies in "ARM" drivers that
took a lot of effort to clean-up. Requiring all drivers to build on
allyesconfigs would have prevented that. The same thing applies at the
architecture level. Of course there are exceptions, but if we only limit
the exceptions then they are easy to find.

>> Having done some cross arch clean-up, I have to agree with that
>> position. Iterating builds over different arches is bad enough, but
>> iterating over all configs for each arch is a pain.
> 
> So make highbank depend only on the arches for which it is supposed to
> work but not for *all* arches. Look at how the other edac drivers do it,
> x86 ones depend on X86, tile on tile, etc, etc. Why should highbank be
> special?
> 
>> It would be nice to have a uniform policy here.
> 
> Simple: drivers should be selectable/buildable only on the arches
> they're supposed to run on.

Like I said, not all maintainers agree. David M rejected patches from me
restricting our network driver to ARM.

Rob

Re: [PATCH] edac, highbank: remove dependency on ARCH_HIGHBANK

[Borislav Petkov]

On Wed, Nov 06, 2013 at 11:17:41AM -0600, Rob Herring wrote:
> Using some interface which is arch specific and should not be used in a
> driver.

Well, sorry, but this is not something which justifies enabling building
of compilation units on arches for which they don't apply. Rather, this
should be caught during review.

I find it highly unfair to waste cycles building stuff on an arch for
which the drivers aren't meant for. And it doesn't make any sense anyway
- I'd like all*config to get finished at some point soonish and not
build the whole world.

And, btw, highbank triggers the following on AMD64:

  CC [M]  drivers/edac/edac_pci.o
  CC [M]  drivers/edac/highbank_mc_edac.o
  CC [M]  drivers/edac/highbank_l2_edac.o
drivers/edac/highbank_mc_edac.c: In function ‘highbank_mc_probe’:
drivers/edac/highbank_mc_edac.c:210:2: warning: large integer implicitly truncated to unsigned type [-Woverflow]
  dimm->nr_pages = (~0UL >> PAGE_SHIFT) + 1;
  ^

which is clearly a bug since ->nr_pages becomes 0.

BUT(!), this bug doesn't make any sense on x86 since highbank won't load
there and yours is a 32-bit arm. (it'll probably make some sense on
arm64 assuming the native long type is 8 bytes there).

And people would start reporting those bugs and we're going to start
asking on which arch are you compiling just to realize that a bug like
that doesn't make any sense on an N/A arch.

Oh, and then other edac drivers would simply fail building:

  CC [M]  drivers/edac/octeon_edac-pc.o
drivers/edac/octeon_edac-pc.c:21:29: fatal error: asm/octeon/cvmx.h: No such file or directory
 #include <asm/octeon/cvmx.h>
                             ^
compilation terminated.
make[1]: *** [drivers/edac/octeon_edac-pc.o] Error 1

simply because x86/include doesn't have that header.

So let's not waste any more time with this topic. I think there are a
numerous other ways for achieving build coverage which make *way* *more*
sense than this.

-- 
Regards/Gruss,
Boris.

Re: [PATCH] edac, highbank: remove dependency on ARCH_HIGHBANK

[Rob Herring]

On 11/07/2013 06:27 AM, Borislav Petkov wrote:
> On Wed, Nov 06, 2013 at 11:17:41AM -0600, Rob Herring wrote:
>> Using some interface which is arch specific and should not be used in a
>> driver.
> 
> Well, sorry, but this is not something which justifies enabling building
> of compilation units on arches for which they don't apply. Rather, this
> should be caught during review.
> 
> I find it highly unfair to waste cycles building stuff on an arch for
> which the drivers aren't meant for. And it doesn't make any sense anyway
> - I'd like all*config to get finished at some point soonish and not
> build the whole world.
> 
> And, btw, highbank triggers the following on AMD64:
> 
>   CC [M]  drivers/edac/edac_pci.o
>   CC [M]  drivers/edac/highbank_mc_edac.o
>   CC [M]  drivers/edac/highbank_l2_edac.o
> drivers/edac/highbank_mc_edac.c: In function ‘highbank_mc_probe’:
> drivers/edac/highbank_mc_edac.c:210:2: warning: large integer implicitly truncated to unsigned type [-Woverflow]
>   dimm->nr_pages = (~0UL >> PAGE_SHIFT) + 1;
>   ^

See, we are benefiting already. I already found this thanks to the 0-day
builder (which doesn't build arm64). This is a 64-bit vs. 32-bit bug and
I need this driver enabled for arm64, so this needs to be fixed.

> which is clearly a bug since ->nr_pages becomes 0.
> 
> BUT(!), this bug doesn't make any sense on x86 since highbank won't load
> there and yours is a 32-bit arm. (it'll probably make some sense on
> arm64 assuming the native long type is 8 bytes there).
> 
> And people would start reporting those bugs and we're going to start
> asking on which arch are you compiling just to realize that a bug like
> that doesn't make any sense on an N/A arch.
> 
> Oh, and then other edac drivers would simply fail building:
> 
>   CC [M]  drivers/edac/octeon_edac-pc.o
> drivers/edac/octeon_edac-pc.c:21:29: fatal error: asm/octeon/cvmx.h: No such file or directory
>  #include <asm/octeon/cvmx.h>
>                              ^
> compilation terminated.
> make[1]: *** [drivers/edac/octeon_edac-pc.o] Error 1
> 
> simply because x86/include doesn't have that header.

Exactly the mess of arch or mach headers in drivers I referred to.

> So let's not waste any more time with this topic. I think there are a
> numerous other ways for achieving build coverage which make *way* *more*
> sense than this.

Re: [PATCH] edac, highbank: remove dependency on ARCH_HIGHBANK

[Borislav Petkov]

On Thu, Nov 07, 2013 at 07:23:10AM -0600, Rob Herring wrote:
> See, we are benefiting already.

You can catch this when building on arm64, i.e. basically one of the
target architectures for which the driver is meant for.

-- 
Regards/Gruss,
Boris.

Re: [PATCH] edac, highbank: remove dependency on ARCH_HIGHBANK

[Robert Richter]

On 07.11.13 14:33:29, Borislav Petkov wrote:
> On Thu, Nov 07, 2013 at 07:23:10AM -0600, Rob Herring wrote:
> > See, we are benefiting already.
> 
> You can catch this when building on arm64, i.e. basically one of the
> target architectures for which the driver is meant for.

Can't we just take

 depends on EDAC_MM_EDAC && (ARCH_MULTI_V7 || ARM64)

as a first step to increase build coverage?

There is also COMPILE_TEST that could be used which is exactly what we
want here: "Compile also drivers which will not load":

 depends on EDAC_MM_EDAC && (ARCH_MULTI_V7 || ARM64 || COMPILE_TEST)

This should enable it with allmodconfig.

-Robert

Re: [PATCH] edac, highbank: remove dependency on ARCH_HIGHBANK

[Borislav Petkov]

On Fri, Nov 08, 2013 at 02:59:18PM +0100, Robert Richter wrote:
> Can't we just take
>
>  depends on EDAC_MM_EDAC && (ARCH_MULTI_V7 || ARM64)
>
> as a first step to increase build coverage?

Yep, that's basically what I was suggesting.

> There is also COMPILE_TEST that could be used which is exactly what we
> want here: "Compile also drivers which will not load":
> 
>  depends on EDAC_MM_EDAC && (ARCH_MULTI_V7 || ARM64 || COMPILE_TEST)
> 
> This should enable it with allmodconfig.

You could do this but the first is already coverage enough IMO and, as I
said previously, compiling this thing on other architectures could raise
bugs/issues which are not necessarily such on arm/arm64, for example.

-- 
Regards/Gruss,
Boris.

Re: [PATCH] edac, highbank: remove dependency on ARCH_HIGHBANK

[Robert Richter]

On 08.11.13 15:47:21, Borislav Petkov wrote:
> >  depends on EDAC_MM_EDAC && (ARCH_MULTI_V7 || ARM64 || COMPILE_TEST)
> > 
> > This should enable it with allmodconfig.
> 
> You could do this but the first is already coverage enough IMO and, as I
> said previously, compiling this thing on other architectures could raise
> bugs/issues which are not necessarily such on arm/arm64, for example.

The config option was introduced for this, enable it to be built on
all archs but have an option to disable it on archs where it is not
supposed to run on. I rather prefer COMPILE_TEST. Could we agree on
this?

Thanks,

-Robert

Re: [PATCH] edac, highbank: remove dependency on ARCH_HIGHBANK

[Borislav Petkov]

On Mon, Nov 11, 2013 at 11:29:03AM +0100, Robert Richter wrote:
> The config option was introduced for this, enable it to be built on
> all archs but have an option to disable it on archs where it is not
> supposed to run on. I rather prefer COMPILE_TEST. Could we agree on
> this?

Sure.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] edac, highbank: remove dependency on ARCH_HIGHBANK

[Robert Richter]

On 05.11.13 21:25:52, Rob Herring wrote:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> In order to enable on arm64 and improve the build coverage, remove the
> dependency on ARCH_HIGHBANK.
> 
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> Cc: Doug Thompson <dougthompson@xmission.com>
> Cc: Robert Richter <rric@kernel.org>
> Cc: linux-edac@vger.kernel.org

Rob, I will apply your patch with the changes we discussed if this is
ok for you.

-Robert

> ---
>  drivers/edac/Kconfig | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 878f090..fc8aaaf 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -328,14 +328,14 @@ config EDAC_TILE
>  
>  config EDAC_HIGHBANK_MC
>  	tristate "Highbank Memory Controller"
> -	depends on EDAC_MM_EDAC && ARCH_HIGHBANK
> +	depends on EDAC_MM_EDAC
>  	help
>  	  Support for error detection and correction on the
>  	  Calxeda Highbank memory controller.
>  
>  config EDAC_HIGHBANK_L2
>  	tristate "Highbank L2 Cache"
> -	depends on EDAC_MM_EDAC && ARCH_HIGHBANK
> +	depends on EDAC_MM_EDAC
>  	help
>  	  Support for error detection and correction on the
>  	  Calxeda Highbank memory controller.
> -- 
> 1.8.1.2
> 

Re: [PATCH] edac, highbank: remove dependency on ARCH_HIGHBANK

[Rob Herring]

On 11/11/2013 05:09 AM, Robert Richter wrote:
> On 05.11.13 21:25:52, Rob Herring wrote:
>> From: Rob Herring <rob.herring@calxeda.com>
>>
>> In order to enable on arm64 and improve the build coverage, remove the
>> dependency on ARCH_HIGHBANK.
>>
>> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
>> Cc: Doug Thompson <dougthompson@xmission.com>
>> Cc: Robert Richter <rric@kernel.org>
>> Cc: linux-edac@vger.kernel.org
> 
> Rob, I will apply your patch with the changes we discussed if this is
> ok for you.

Thanks. It should be (ARM || ARM64 || COMPILE_TEST) rather than
ARM_MULTI_V7. The L2 ECC driver can be ARM only.

Rob

> 
> -Robert
> 
>> ---
>>  drivers/edac/Kconfig | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
>> index 878f090..fc8aaaf 100644
>> --- a/drivers/edac/Kconfig
>> +++ b/drivers/edac/Kconfig
>> @@ -328,14 +328,14 @@ config EDAC_TILE
>>  
>>  config EDAC_HIGHBANK_MC
>>  	tristate "Highbank Memory Controller"
>> -	depends on EDAC_MM_EDAC && ARCH_HIGHBANK
>> +	depends on EDAC_MM_EDAC
>>  	help
>>  	  Support for error detection and correction on the
>>  	  Calxeda Highbank memory controller.
>>  
>>  config EDAC_HIGHBANK_L2
>>  	tristate "Highbank L2 Cache"
>> -	depends on EDAC_MM_EDAC && ARCH_HIGHBANK
>> +	depends on EDAC_MM_EDAC
>>  	help
>>  	  Support for error detection and correction on the
>>  	  Calxeda Highbank memory controller.
>> -- 
>> 1.8.1.2
>>