[RFC PATCH 0/4] CONFIG_COMPILE_TEST: additional examples and checkpatch rule

[RFC PATCH 0/4] CONFIG_COMPILE_TEST: additional examples and checkpatch rule

[Paul Gortmaker]

I only just noticed the existence of CONFIG_COMPILE_TEST (now in
mainline as of merge fc76a258d41 ("Merge tag 'driver-core-3.11-rc1' of
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core")

I like this; it parallels a discussion I had with Andrew a while ago:

	https://lkml.org/lkml/2013/3/7/456

I wish I had seen it earlier, as at the moment it only has three use case
instances/examples.  I have at my fingertips three more that were cases
where I wanted to add a real world dependency that limited the option to
those who it was useful to, but the rebuttals I got were that the
maintainers liked the compile coverage (see links in each commmit.)

It might be nice to have a few more examples showing proper dependency
usage of COMPILE_TEST, if the opportunity arises.  In the same vein as
having more available examples, we can teach checkpatch to spot when
people incorrectly try and use this in C (as has already happened.)
It might help avoid doing that education campaign via lkml lessons.

That said, I realize we are well into week one of the merge window,
so if need be, it isn't a problem for me to hang on to them for 3.12.

Thanks,
Paul.
---

[I made a temporary bad commit to test the new checkpatch rule.  I
[also build tested for OMAP2 with USB options on & COMPILE_TEST off]

Cc: Andy Whitcroft <apw@canonical.com>
Cc: Anton Vorontsov <cbou@mail.ru>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: Joe Perches <joe@perches.com>
Cc: Kishon Vijay Abraham I <kishon@ti.com>
Cc: Moiz Sonasath <m-sonasath@ti.com>

Paul Gortmaker (4):
  usb: limit OMAP related USB options to OMAP2PLUS platforms
  power: make goldfish_battery depend on GOLDFISH || COMPILE_TEST
  video: make goldfish video depend on GOLDFISH || COMPILE_TEST
  checkpatch: only allow COMPILE_TEST in Kconfig dependency lines

 drivers/power/Kconfig   | 2 +-
 drivers/usb/phy/Kconfig | 2 ++
 drivers/video/Kconfig   | 2 +-
 scripts/checkpatch.pl   | 6 ++++++
 4 files changed, 10 insertions(+), 2 deletions(-)

-- 
1.8.1.2

[PATCH 1/4] usb: limit OMAP related USB options to OMAP2PLUS platforms

[Paul Gortmaker]

commit 57f6ce072e35770a63be0c5d5e82f90d8da7d665 ("usb: phy:
add a new driver for usb3 phy") added the new Kconfig option
OMAP_USB3, but it had no dependencies whatsoever, and hence
became available across all arch/platforms.

Which presumably caused this to show up in x86 randconfig:

    warning: (USB_MUSB_HDRC && OMAP_USB3) selects \
        OMAP_CONTROL_USB which has unmet direct \
        dependencies (USB_SUPPORT && ARCH_OMAP2PLUS)

Then commit 6992819feb39cb9adac72170555d957d07f869f2 ("usb: phy:
fix Kconfig warning") was added.  However, this just deleted the
ARCH_OMAP2PLUS dependency from OMAP_CONTROL_USB, further
compounding the problem by opening up OMAP_CONTROL_USB to
all arch/platforms as well.

Earlier it was suggested[1] that we revert the change of 6992819feb
to restore the dependency, and add a same ARCH_OMAP2PLUS dependency
to the new OMAP_USB3 entry.  However that was discouraged on the
grounds of people wanting the extra sanity compile testing on x86,
even though the driver could probably never be used there.

Now we have CONFIG_COMPILE_TEST, so developers who value the ability
to compile drivers on an architecture that it never can be used for
can have that, and people who want dependencies to shield them from
seeing options that aren't relevant to their platform get what they
want too.

Here we restore the dependency but couple it with COMPILE_TEST, in
order to achieve both of the above goals.

[1] https://patchwork.kernel.org/patch/2194511/

Cc: Felipe Balbi <balbi@ti.com>
Cc: Kishon Vijay Abraham I <kishon@ti.com>
Cc: Moiz Sonasath <m-sonasath@ti.com>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/usb/phy/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
index a5a9552..f061f28 100644
--- a/drivers/usb/phy/Kconfig
+++ b/drivers/usb/phy/Kconfig
@@ -65,6 +65,7 @@ config NOP_USB_XCEIV
 
 config OMAP_CONTROL_USB
 	tristate "OMAP CONTROL USB Driver"
+	depends on ARCH_OMAP2PLUS || COMPILE_TEST
 	help
 	  Enable this to add support for the USB part present in the control
 	  module. This driver has API to power on the USB2 PHY and to write to
@@ -84,6 +85,7 @@ config OMAP_USB2
 
 config OMAP_USB3
 	tristate "OMAP USB3 PHY Driver"
+	depends on ARCH_OMAP2PLUS || COMPILE_TEST
 	select OMAP_CONTROL_USB
 	help
 	  Enable this to support the USB3 PHY that is part of SOC. This
-- 
1.8.1.2

Re: [PATCH 1/4] usb: limit OMAP related USB options to OMAP2PLUS platforms

[Felipe Balbi]

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

On Thu, Jul 04, 2013 at 01:39:10AM -0400, Paul Gortmaker wrote:
> commit 57f6ce072e35770a63be0c5d5e82f90d8da7d665 ("usb: phy:
> add a new driver for usb3 phy") added the new Kconfig option
> OMAP_USB3, but it had no dependencies whatsoever, and hence
> became available across all arch/platforms.
> 
> Which presumably caused this to show up in x86 randconfig:
> 
>     warning: (USB_MUSB_HDRC && OMAP_USB3) selects \
>         OMAP_CONTROL_USB which has unmet direct \
>         dependencies (USB_SUPPORT && ARCH_OMAP2PLUS)
> 
> Then commit 6992819feb39cb9adac72170555d957d07f869f2 ("usb: phy:
> fix Kconfig warning") was added.  However, this just deleted the
> ARCH_OMAP2PLUS dependency from OMAP_CONTROL_USB, further
> compounding the problem by opening up OMAP_CONTROL_USB to
> all arch/platforms as well.
> 
> Earlier it was suggested[1] that we revert the change of 6992819feb
> to restore the dependency, and add a same ARCH_OMAP2PLUS dependency
> to the new OMAP_USB3 entry.  However that was discouraged on the
> grounds of people wanting the extra sanity compile testing on x86,
> even though the driver could probably never be used there.
> 
> Now we have CONFIG_COMPILE_TEST, so developers who value the ability
> to compile drivers on an architecture that it never can be used for
> can have that, and people who want dependencies to shield them from
> seeing options that aren't relevant to their platform get what they
> want too.
> 
> Here we restore the dependency but couple it with COMPILE_TEST, in
> order to achieve both of the above goals.
> 
> [1] https://patchwork.kernel.org/patch/2194511/
> 
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> Cc: Moiz Sonasath <m-sonasath@ti.com>
> Cc: Jiri Slaby <jslaby@suse.cz>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

Acked-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

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

[PATCH 2/4] power: make goldfish_battery depend on GOLDFISH || COMPILE_TEST

[Paul Gortmaker]

Nearly all the other goldfish peripherals (mtd, keyboard, etc)
have a dependency on the main platform's GOLDFISH Kconfig item,
but this one got skipped.  Even with consistency as a
justification, there was initial resistance[1] from some people
to adding it however, as they wanted the extra compile coverage.

Now, with CONFIG_COMPILE_TEST, we have the middle ground that
will give people the coverage who want it, and let those who
don't want it to skip ever seeing the option presented.

[1] https://lkml.org/lkml/2013/2/27/333

Cc: Anton Vorontsov <cbou@mail.ru>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/power/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 7b8979c..dcc0d9e 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -357,7 +357,7 @@ config AB8500_BM
 
 config BATTERY_GOLDFISH
 	tristate "Goldfish battery driver"
-	depends on GENERIC_HARDIRQS
+	depends on GENERIC_HARDIRQS && (GOLDFISH || COMPILE_TEST)
 	help
 	  Say Y to enable support for the battery and AC power in the
 	  Goldfish emulator.
-- 
1.8.1.2

Re: [PATCH 2/4] power: make goldfish_battery depend on GOLDFISH || COMPILE_TEST

[Anton Vorontsov]

On Thu, Jul 04, 2013 at 01:39:11AM -0400, Paul Gortmaker wrote:
> Nearly all the other goldfish peripherals (mtd, keyboard, etc)
> have a dependency on the main platform's GOLDFISH Kconfig item,
> but this one got skipped.  Even with consistency as a
> justification, there was initial resistance[1] from some people
> to adding it however, as they wanted the extra compile coverage.
> 
> Now, with CONFIG_COMPILE_TEST, we have the middle ground that
> will give people the coverage who want it, and let those who
> don't want it to skip ever seeing the option presented.
> 
> [1] https://lkml.org/lkml/2013/2/27/333
> 
> Cc: Anton Vorontsov <cbou@mail.ru>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Jiri Slaby <jslaby@suse.cz>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---

So, CONFIG_AKPM... kinda. :) It works for me. Applied, thank you!

Anton

[PATCH 3/4] video: make goldfish video depend on GOLDFISH || COMPILE_TEST

[Paul Gortmaker]

Nearly all the other goldfish peripherals (mtd, keyboard, etc)
have a dependency on the main platform's GOLDFISH Kconfig item,
but this one got skipped.  It was suggested to add it earlier[1],
but that never got responded to or picked up -- presumably because
some developers desired the extra compile coverage at the cost of
exposing unusable options to end users.

With CONFIG_COMPILE_TEST, we can have the best of both worlds.
Add the GOLDFISH dependency, in conjunction with COMPILE_TEST.

[1] https://lkml.org/lkml/2013/2/27/169

Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/video/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 2e937bd..630b637 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -2199,7 +2199,7 @@ config FB_XILINX
 
 config FB_GOLDFISH
 	tristate "Goldfish Framebuffer"
-	depends on FB && HAS_DMA
+	depends on FB && HAS_DMA && (GOLDFISH || COMPILE_TEST)
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
-- 
1.8.1.2

[PATCH 4/4] checkpatch: only allow COMPILE_TEST in Kconfig dependency lines

[Paul Gortmaker]

The option CONFIG_COMPILE_TEST, added in commit 4bb1667255a
("build some drivers only when compile-testing") is meant to
give a middle ground[1] between those who want the widest compile
coverage possible (e.g. building sparc drivers for mips) and
those who want dependencies to represent real world systems
(e.g. don't allow me to see OMAP options when building x86).

As such, this addition is meant to be used in dependency lines,
properly or'd in with the real world hardware dependency.  Those
who select it, get wide compile coverage.  Those who do not, get
real world dependencies that match where the hardware is available
and/or where the driver is useful.

With that in mind, it is clear that this is _not_ to be used in
any C code with "#ifdef CONFIG_COMPILE_TEST" etc.  However there
will (and already has been) instances of people thinking this is
an OK practice[2].  So teach checkpatch to spot them as an error.

[1] https://lkml.org/lkml/2013/3/7/456
[2] https://lkml.org/lkml/2013/7/1/641

Cc: Joe Perches <joe@perches.com>
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 scripts/checkpatch.pl | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b954de5..c0871a3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1922,6 +1922,12 @@ sub process {
 			     "Use of CONFIG_EXPERIMENTAL is deprecated. For alternatives, see https://lkml.org/lkml/2012/10/23/580\n");
 		}
 
+# disallow the addition of CONFIG_COMPILE_TEST in #if(def).
+		if ($line =~ /^\+\s*\#\s*if.*\bCONFIG_COMPILE_TEST\b/) {
+			ERROR("CONFIG_COMPILE_TEST",
+			     "Use of COMPILE_TEST is only allowed in Kconfig dependency lines.\n");
+		}
+
 # check for RCS/CVS revision markers
 		if ($rawline =~ /^\+.*\$(Revision|Log|Id)(?:\$|)/) {
 			WARN("CVS_KEYWORD",
-- 
1.8.1.2

Re: [PATCH 4/4] checkpatch: only allow COMPILE_TEST in Kconfig dependency lines

[Geert Uytterhoeven]

On Thu, Jul 4, 2013 at 7:39 AM, Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
> With that in mind, it is clear that this is _not_ to be used in
> any C code with "#ifdef CONFIG_COMPILE_TEST" etc.  However there
> will (and already has been) instances of people thinking this is
> an OK practice[2].  So teach checkpatch to spot them as an error.

Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 4/4] checkpatch: only allow COMPILE_TEST in Kconfig dependency lines

[Arnd Bergmann]

On Thursday 04 July 2013, Paul Gortmaker wrote:
> The option CONFIG_COMPILE_TEST, added in commit 4bb1667255a
> ("build some drivers only when compile-testing") is meant to
> give a middle ground[1] between those who want the widest compile
> coverage possible (e.g. building sparc drivers for mips) and
> those who want dependencies to represent real world systems
> (e.g. don't allow me to see OMAP options when building x86).
> 
> As such, this addition is meant to be used in dependency lines,
> properly or'd in with the real world hardware dependency.  Those
> who select it, get wide compile coverage.  Those who do not, get
> real world dependencies that match where the hardware is available
> and/or where the driver is useful.
> 
> With that in mind, it is clear that this is _not_ to be used in
> any C code with "#ifdef CONFIG_COMPILE_TEST" etc.  However there
> will (and already has been) instances of people thinking this is
> an OK practice[2].  So teach checkpatch to spot them as an error.
> 
> [1] https://lkml.org/lkml/2013/3/7/456
> [2] https://lkml.org/lkml/2013/7/1/641
> 
> Cc: Joe Perches <joe@perches.com>
> Cc: Andy Whitcroft <apw@canonical.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Jiri Slaby <jslaby@suse.cz>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 4/4] checkpatch: only allow COMPILE_TEST in Kconfig dependency lines

[Joe Perches]

On Thu, 2013-07-04 at 01:39 -0400, Paul Gortmaker wrote:
> The option CONFIG_COMPILE_TEST, added in commit 4bb1667255a
> ("build some drivers only when compile-testing") is meant to
> give a middle ground[1] between those who want the widest compile
> coverage possible (e.g. building sparc drivers for mips) and
> those who want dependencies to represent real world systems
> (e.g. don't allow me to see OMAP options when building x86).
> 
> As such, this addition is meant to be used in dependency lines,
> properly or'd in with the real world hardware dependency.  Those
> who select it, get wide compile coverage.  Those who do not, get
> real world dependencies that match where the hardware is available
> and/or where the driver is useful.
> 
> With that in mind, it is clear that this is _not_ to be used in
> any C code with "#ifdef CONFIG_COMPILE_TEST" etc.  However there
> will (and already has been) instances of people thinking this is
> an OK practice[2].  So teach checkpatch to spot them as an error.
> 
> [1] https://lkml.org/lkml/2013/3/7/456
> [2] https://lkml.org/lkml/2013/7/1/641
> 
> Cc: Joe Perches <joe@perches.com>
> Cc: Andy Whitcroft <apw@canonical.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Jiri Slaby <jslaby@suse.cz>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>  scripts/checkpatch.pl | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index b954de5..c0871a3 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1922,6 +1922,12 @@ sub process {
>  			     "Use of CONFIG_EXPERIMENTAL is deprecated. For alternatives, see https://lkml.org/lkml/2012/10/23/580\n");
>  		}
>  
> +# disallow the addition of CONFIG_COMPILE_TEST in #if(def).
> +		if ($line =~ /^\+\s*\#\s*if.*\bCONFIG_COMPILE_TEST\b/) {
> +			ERROR("CONFIG_COMPILE_TEST",
> +			     "Use of COMPILE_TEST is only allowed in Kconfig dependency lines.\n");

Why not just look for \bCONFIG_COMPILE_TEST\b?

I see it's the same style as the CONFIG_EXPERIMENTAL above it,
but perhaps code could be written like

#if defined CONFIG_FOO || \
    defined CONFIG_BAR

and this wouldn't trigger.

Re: [PATCH 4/4] checkpatch: only allow COMPILE_TEST in Kconfig dependency lines

[Paul Gortmaker]

[Re: [PATCH 4/4] checkpatch: only allow COMPILE_TEST in Kconfig dependency lines] On 04/07/2013 (Thu 08:41) Joe Perches wrote:

> On Thu, 2013-07-04 at 01:39 -0400, Paul Gortmaker wrote:
> > The option CONFIG_COMPILE_TEST, added in commit 4bb1667255a
> > ("build some drivers only when compile-testing") is meant to
> > give a middle ground[1] between those who want the widest compile
> > coverage possible (e.g. building sparc drivers for mips) and
> > those who want dependencies to represent real world systems
> > (e.g. don't allow me to see OMAP options when building x86).
> > 
> > As such, this addition is meant to be used in dependency lines,
> > properly or'd in with the real world hardware dependency.  Those
> > who select it, get wide compile coverage.  Those who do not, get
> > real world dependencies that match where the hardware is available
> > and/or where the driver is useful.
> > 
> > With that in mind, it is clear that this is _not_ to be used in
> > any C code with "#ifdef CONFIG_COMPILE_TEST" etc.  However there
> > will (and already has been) instances of people thinking this is
> > an OK practice[2].  So teach checkpatch to spot them as an error.
> > 
> > [1] https://lkml.org/lkml/2013/3/7/456
> > [2] https://lkml.org/lkml/2013/7/1/641
> > 
> > Cc: Joe Perches <joe@perches.com>
> > Cc: Andy Whitcroft <apw@canonical.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > Cc: Jiri Slaby <jslaby@suse.cz>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> > ---
> >  scripts/checkpatch.pl | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index b954de5..c0871a3 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -1922,6 +1922,12 @@ sub process {
> >  			     "Use of CONFIG_EXPERIMENTAL is deprecated. For alternatives, see https://lkml.org/lkml/2012/10/23/580\n");
> >  		}
> >  
> > +# disallow the addition of CONFIG_COMPILE_TEST in #if(def).
> > +		if ($line =~ /^\+\s*\#\s*if.*\bCONFIG_COMPILE_TEST\b/) {
> > +			ERROR("CONFIG_COMPILE_TEST",
> > +			     "Use of COMPILE_TEST is only allowed in Kconfig dependency lines.\n");
> 
> Why not just look for \bCONFIG_COMPILE_TEST\b?
> 
> I see it's the same style as the CONFIG_EXPERIMENTAL above it,
> but perhaps code could be written like

That is exactly why.  I valued consistency and the fact that this was
already a tested and working check, vs. me reinventing my own and then
having a "Oh crap, I never thought of that false positive" moment.

I'd suggest leaving it consistent with the existing solution as lowest
risk and then later (post 3.11) you could come along and recode all
similar instances to increase their scope if you really wanted to.

P.
--

> 
> #if defined CONFIG_FOO || \
>     defined CONFIG_BAR
> 
> and this wouldn't trigger.
> 
>