[PATCH] thunderbolt: fix compile-test dependencies

[PATCH] thunderbolt: fix compile-test dependencies

[Arnd Bergmann]

Building the Apple thunderbolt driver on non-x86 machines now produces
a harmless warning:

warning: (THUNDERBOLT) selects APPLE_PROPERTIES which has unmet direct dependencies (EFI && EFI_STUB && X86)

As there is no compile-time dependency to the Apple properties support,
we can make that 'select' statement conditional on the dependencies
of that driver.

Fixes: c9cc3aaa0281 ("thunderbolt: Use Device ROM retrieved from EFI")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/thunderbolt/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/thunderbolt/Kconfig b/drivers/thunderbolt/Kconfig
index 0056df7f3c09..4e7d92193b65 100644
--- a/drivers/thunderbolt/Kconfig
+++ b/drivers/thunderbolt/Kconfig
@@ -1,7 +1,8 @@
 menuconfig THUNDERBOLT
 	tristate "Thunderbolt support for Apple devices"
+	depends on (EFI_STUB && X86) || COMPILE_TEST
 	depends on PCI
-	select APPLE_PROPERTIES
+	select APPLE_PROPERTIES if (X86 && EFI_STUB)
 	select CRC32
 	help
 	  Cactus Ridge Thunderbolt Controller driver
-- 
2.9.0

Re: [PATCH] thunderbolt: fix compile-test dependencies

[Lukas Wunner]

On Tue, Nov 15, 2016 at 04:58:53PM +0100, Arnd Bergmann wrote:
> Building the Apple thunderbolt driver on non-x86 machines now produces
> a harmless warning:
> 
> warning: (THUNDERBOLT) selects APPLE_PROPERTIES which has unmet direct dependencies (EFI && EFI_STUB && X86)
> 
> As there is no compile-time dependency to the Apple properties support,
> we can make that 'select' statement conditional on the dependencies
> of that driver.

Thanks Arnd, a commit fixing this is already on the tip.git efi/core
branch since this morning and will hopefully have landed in linux-next
by tomorrow:
http://git.kernel.org/tip/79f9cd35b05e3e91ccf9b4038a8b74b9362b5da7

Previous discussion on LKML:
https://lkml.org/lkml/2016/11/14/456

Another comment below...

> 
> Fixes: c9cc3aaa0281 ("thunderbolt: Use Device ROM retrieved from EFI")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/thunderbolt/Kconfig | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thunderbolt/Kconfig b/drivers/thunderbolt/Kconfig
> index 0056df7f3c09..4e7d92193b65 100644
> --- a/drivers/thunderbolt/Kconfig
> +++ b/drivers/thunderbolt/Kconfig
> @@ -1,7 +1,8 @@
>  menuconfig THUNDERBOLT
>  	tristate "Thunderbolt support for Apple devices"
> +	depends on (EFI_STUB && X86) || COMPILE_TEST

This addition isn't correct, the thunderbolt driver works without
the EFI stub, it just falls back to a hardcoded Device ROM.

In other words, using the Device ROM retrieved by the EFI stub is
an optional feature that is supposed to be enabled by default in
the config if the EFI stub is also enabled.  That is what the patch
now in tip.git does.

Nevertheless, thanks for your efforts and sorry for having caused
you extra work.

Lukas

>  	depends on PCI
> -	select APPLE_PROPERTIES
> +	select APPLE_PROPERTIES if (X86 && EFI_STUB)
>  	select CRC32
>  	help
>  	  Cactus Ridge Thunderbolt Controller driver
> -- 
> 2.9.0

Re: [PATCH] thunderbolt: fix compile-test dependencies

[Arnd Bergmann]

On Tuesday, November 15, 2016 5:21:34 PM CET Lukas Wunner wrote:
> On Tue, Nov 15, 2016 at 04:58:53PM +0100, Arnd Bergmann wrote:
> > diff --git a/drivers/thunderbolt/Kconfig b/drivers/thunderbolt/Kconfig
> > index 0056df7f3c09..4e7d92193b65 100644
> > --- a/drivers/thunderbolt/Kconfig
> > +++ b/drivers/thunderbolt/Kconfig
> > @@ -1,7 +1,8 @@
> >  menuconfig THUNDERBOLT
> >  	tristate "Thunderbolt support for Apple devices"
> > +	depends on (EFI_STUB && X86) || COMPILE_TEST
> 
> This addition isn't correct, the thunderbolt driver works without
> the EFI stub, it just falls back to a hardcoded Device ROM.

Ok, makes sense.

I also needed this one though:

diff --git a/drivers/thunderbolt/Kconfig b/drivers/thunderbolt/Kconfig
index bb0318ceaf93..de5d27ec67d6 100644
--- a/drivers/thunderbolt/Kconfig
+++ b/drivers/thunderbolt/Kconfig
@@ -1,7 +1,7 @@
 menuconfig THUNDERBOLT
 	tristate "Thunderbolt support for Apple devices"
 	depends on PCI
-	select APPLE_PROPERTIES if EFI_STUB
+	select APPLE_PROPERTIES if EFI_STUB && X86
 	select CRC32
 	help
 	  Cactus Ridge Thunderbolt Controller driver


Without that change, building on ARM with EFI_STUB enabled still
gives me a build failure.

	Arnd

Re: [PATCH] thunderbolt: fix compile-test dependencies

[Lukas Wunner]

On Thu, Nov 17, 2016 at 11:00:31AM +0100, Arnd Bergmann wrote:
> On Tuesday, November 15, 2016 5:21:34 PM CET Lukas Wunner wrote:
> > On Tue, Nov 15, 2016 at 04:58:53PM +0100, Arnd Bergmann wrote:
> > > diff --git a/drivers/thunderbolt/Kconfig b/drivers/thunderbolt/Kconfig
> > > index 0056df7f3c09..4e7d92193b65 100644
> > > --- a/drivers/thunderbolt/Kconfig
> > > +++ b/drivers/thunderbolt/Kconfig
> > > @@ -1,7 +1,8 @@
> > >  menuconfig THUNDERBOLT
> > >  	tristate "Thunderbolt support for Apple devices"
> > > +	depends on (EFI_STUB && X86) || COMPILE_TEST
> > 
> > This addition isn't correct, the thunderbolt driver works without
> > the EFI stub, it just falls back to a hardcoded Device ROM.
> 
> Ok, makes sense.
> 
> I also needed this one though:
> 
> diff --git a/drivers/thunderbolt/Kconfig b/drivers/thunderbolt/Kconfig
> index bb0318ceaf93..de5d27ec67d6 100644
> --- a/drivers/thunderbolt/Kconfig
> +++ b/drivers/thunderbolt/Kconfig
> @@ -1,7 +1,7 @@
>  menuconfig THUNDERBOLT
>  	tristate "Thunderbolt support for Apple devices"
>  	depends on PCI
> -	select APPLE_PROPERTIES if EFI_STUB
> +	select APPLE_PROPERTIES if EFI_STUB && X86
>  	select CRC32
>  	help
>  	  Cactus Ridge Thunderbolt Controller driver
> 
> 
> Without that change, building on ARM with EFI_STUB enabled still
> gives me a build failure.

Hm, so far Thunderbolt is (unfortunately) an Intel proprietary
technology that is only available on x86, so compiling anything
in drivers/thunderbolt/ on other arches doesn't seem to make much
sense.  So maybe a "depends on X86" would be more appropriate?

One could argue that compiling on other arches helps avoid x86-isms
in case Thunderbolt does become available on other arches one day,
then again it seems like an enormous waste of CPU cycles. *shrug*

Opinions?

Thanks,

Lukas

Re: [PATCH] thunderbolt: fix compile-test dependencies

[Lukas Wunner]

On Thu, Nov 17, 2016 at 03:10:11PM +0100, Arnd Bergmann wrote:
> On Thursday, November 17, 2016 2:53:55 PM CET Lukas Wunner wrote:
> > Hm, so far Thunderbolt is (unfortunately) an Intel proprietary
> > technology that is only available on x86, so compiling anything
> > in drivers/thunderbolt/ on other arches doesn't seem to make much
> > sense.  So maybe a "depends on X86" would be more appropriate?
> 
> I also found that we need "depends on ACPI" because APPLE_PROPERTIES
> does "select EFI_DEV_PATH_PARSER" and that requires APCI...

There's a series coming up to power the Thunderbolt controller
down when nothing is plugged in and this is done via ACPI.
This commit (slated for 4.11) was going to add a dependency on
ACPI anyway:
https://github.com/l1k/linux/commit/c1f379d5dee4

So adding "depends on ACPI" now would be fine I guess.


> > One could argue that compiling on other arches helps avoid x86-isms
> > in case Thunderbolt does become available on other arches one day,
> > then again it seems like an enormous waste of CPU cycles. *shrug*
> > 
> > Opinions?
> 
> We try to avoid adding architecture-specific dependencies that
> prevent build testing, and we are adding '|| COMPILE_TEST' to
> a lot of drivers for this. We could use 'depends on X86 ||
> COMPILE_TEST' here, but that wouldn't help the problem on ARM.

Yes, "depends on X86 || COMPILE_TEST" sounds like the right thing
to do, independently of the build breakage at hand.


> Another option would be to use 'depends on APPLE_PROPERTIES ||
> APPLE_PROPERTIES=n', which would force the thunderbolt driver
> to be a loadable module if APPLE_PROPERTIES is one, but otherwise
> just allow all configurations.

APPLE_PROPERTIES is bool, not tristate, so this would work.
However the solution you proposed earlier ("select APPLE_PROPERTIES if
(X86 && EFI_STUB)") is more explicit and easier to understand,
thus seems preferable to me.

Thanks!

Lukas

Re: [PATCH] thunderbolt: fix compile-test dependencies

[Arnd Bergmann]

On Thursday, November 17, 2016 4:12:07 PM CET Lukas Wunner wrote:
> On Thu, Nov 17, 2016 at 03:10:11PM +0100, Arnd Bergmann wrote:
> > On Thursday, November 17, 2016 2:53:55 PM CET Lukas Wunner wrote:
> > > Hm, so far Thunderbolt is (unfortunately) an Intel proprietary
> > > technology that is only available on x86, so compiling anything
> > > in drivers/thunderbolt/ on other arches doesn't seem to make much
> > > sense.  So maybe a "depends on X86" would be more appropriate?
> > 
> > I also found that we need "depends on ACPI" because APPLE_PROPERTIES
> > does "select EFI_DEV_PATH_PARSER" and that requires APCI...
> 
> There's a series coming up to power the Thunderbolt controller
> down when nothing is plugged in and this is done via ACPI.
> This commit (slated for 4.11) was going to add a dependency on
> ACPI anyway:
> https://github.com/l1k/linux/commit/c1f379d5dee4
> 
> So adding "depends on ACPI" now would be fine I guess.

It would take care of ARM, but not ARM64: ARM64 has ACPI
and EFI_STUB, but cannot build APPLE_PROPERTIES. Adding
the ACPI dependency by itself would not be sufficient.

> > > One could argue that compiling on other arches helps avoid x86-isms
> > > in case Thunderbolt does become available on other arches one day,
> > > then again it seems like an enormous waste of CPU cycles. *shrug*
> > > 
> > > Opinions?
> > 
> > We try to avoid adding architecture-specific dependencies that
> > prevent build testing, and we are adding '|| COMPILE_TEST' to
> > a lot of drivers for this. We could use 'depends on X86 ||
> > COMPILE_TEST' here, but that wouldn't help the problem on ARM.
> 
> Yes, "depends on X86 || COMPILE_TEST" sounds like the right thing
> to do, independently of the build breakage at hand.

Ok.

> > Another option would be to use 'depends on APPLE_PROPERTIES ||
> > APPLE_PROPERTIES=n', which would force the thunderbolt driver
> > to be a loadable module if APPLE_PROPERTIES is one, but otherwise
> > just allow all configurations.
> 
> APPLE_PROPERTIES is bool, not tristate, so this would work.
> However the solution you proposed earlier ("select APPLE_PROPERTIES if
> (X86 && EFI_STUB)") is more explicit and easier to understand,
> thus seems preferable to me.

Ok, sounds good. That should also fix ARM64 then.

	Arnd

Re: [PATCH] thunderbolt: fix compile-test dependencies

[Lukas Wunner]

On Thu, Nov 17, 2016 at 04:29:05PM +0100, Arnd Bergmann wrote:
> On Thursday, November 17, 2016 4:12:07 PM CET Lukas Wunner wrote:
> > On Thu, Nov 17, 2016 at 03:10:11PM +0100, Arnd Bergmann wrote:
> > > On Thursday, November 17, 2016 2:53:55 PM CET Lukas Wunner wrote:
> > > > Hm, so far Thunderbolt is (unfortunately) an Intel proprietary
> > > > technology that is only available on x86, so compiling anything
> > > > in drivers/thunderbolt/ on other arches doesn't seem to make much
> > > > sense.  So maybe a "depends on X86" would be more appropriate?
> > > 
> > > I also found that we need "depends on ACPI" because APPLE_PROPERTIES
> > > does "select EFI_DEV_PATH_PARSER" and that requires APCI...
> > 
> > There's a series coming up to power the Thunderbolt controller
> > down when nothing is plugged in and this is done via ACPI.
> > This commit (slated for 4.11) was going to add a dependency on
> > ACPI anyway:
> > https://github.com/l1k/linux/commit/c1f379d5dee4
> > 
> > So adding "depends on ACPI" now would be fine I guess.
> 
> It would take care of ARM, but not ARM64: ARM64 has ACPI
> and EFI_STUB, but cannot build APPLE_PROPERTIES. Adding
> the ACPI dependency by itself would not be sufficient.
> 
> > > > One could argue that compiling on other arches helps avoid x86-isms
> > > > in case Thunderbolt does become available on other arches one day,
> > > > then again it seems like an enormous waste of CPU cycles. *shrug*
> > > > 
> > > > Opinions?
> > > 
> > > We try to avoid adding architecture-specific dependencies that
> > > prevent build testing, and we are adding '|| COMPILE_TEST' to
> > > a lot of drivers for this. We could use 'depends on X86 ||
> > > COMPILE_TEST' here, but that wouldn't help the problem on ARM.
> > 
> > Yes, "depends on X86 || COMPILE_TEST" sounds like the right thing
> > to do, independently of the build breakage at hand.
> 
> Ok.
> 
> > > Another option would be to use 'depends on APPLE_PROPERTIES ||
> > > APPLE_PROPERTIES=n', which would force the thunderbolt driver
> > > to be a loadable module if APPLE_PROPERTIES is one, but otherwise
> > > just allow all configurations.
> > 
> > APPLE_PROPERTIES is bool, not tristate, so this would work.
> > However the solution you proposed earlier ("select APPLE_PROPERTIES if
> > (X86 && EFI_STUB)") is more explicit and easier to understand,
> > thus seems preferable to me.
> 
> Ok, sounds good. That should also fix ARM64 then.

Are you going to submit a fix for this or would you prefer me to do it?
Given the effort you've already put into it you might as well claim
credit for the fix, but I'll be happy to do it if that is preferred.

Thanks,

Lukas

Re: [PATCH] thunderbolt: fix compile-test dependencies

[Arnd Bergmann]

On Thursday, November 17, 2016 5:18:07 PM CET Lukas Wunner wrote:
> 
> > Ok, sounds good. That should also fix ARM64 then.
> 
> Are you going to submit a fix for this or would you prefer me to do it?
> Given the effort you've already put into it you might as well claim
> credit for the fix, but I'll be happy to do it if that is preferred.

I'd rather have you do it, less work for me that way, just add

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

Thanks,

	Arnd