[0/4] Cleanup arm64 driver dependencies
mbox series

Message ID cover.1568239378.git.amit.kucheria@linaro.org
Headers show
Series
  • Cleanup arm64 driver dependencies
Related show

Message

Amit Kucheria Sept. 11, 2019, 10:18 p.m. UTC
I was using initcall_debugging on a QCOM platform and ran across a bunch of
driver initcalls that are enabled even if their SoC support is disabled.

Here are some fixups for a subset of them.

Amit Kucheria (4):
  arm64: Kconfig: Fix XGENE driver dependencies
  arm64: Kconfig: Fix BRCMSTB driver dependencies
  arm64: Kconfig: Fix VEXPRESS driver dependencies
  arm64: Kconfig: Fix EXYNOS driver dependencies

 arch/arm64/Kconfig.platforms   | 3 +++
 drivers/bus/Kconfig            | 3 ++-
 drivers/clk/Kconfig            | 3 ++-
 drivers/clk/versatile/Kconfig  | 4 ++--
 drivers/gpio/Kconfig           | 1 +
 drivers/pci/controller/Kconfig | 1 +
 drivers/phy/Kconfig            | 1 +
 drivers/power/reset/Kconfig    | 3 ++-
 drivers/regulator/Kconfig      | 1 +
 drivers/soc/bcm/Kconfig        | 1 +
 10 files changed, 16 insertions(+), 5 deletions(-)

Comments

Arnd Bergmann Sept. 12, 2019, 9:29 a.m. UTC | #1
On Thu, Sep 12, 2019 at 12:18 AM Amit Kucheria <amit.kucheria@linaro.org> wrote:
>
> I was using initcall_debugging on a QCOM platform and ran across a bunch of
> driver initcalls that are enabled even if their SoC support is disabled.
>
> Here are some fixups for a subset of them.

The idea seems reasonable, disabling a platform may just turn off
all the drivers that are not useful elsewhere, but there are mistakes
in a lot of your changes, so I'm certainly not applying these for 5.4.

Generally speaking, the way that works best is

config SUBSYS_DRIVER_FOO
       tristate "SUBSYS support for FOO platform"
       depends on ARCH_FOO || COMPILE_TEST
       depends on SUBSYS
       default "m" if ARCH_FOO

This means it's enabled as a loadable module by default (use
default "y" instead where necessary) as long as the platform
is enabled, but an x86 allmodconfig build also includes it
because of COMPILE_TEST, while any configuration without
ARCH_FOO that is not compile-testing cannot enable it.

       Arnd
Mark Brown Sept. 12, 2019, 9:46 a.m. UTC | #2
On Thu, Sep 12, 2019 at 03:48:44AM +0530, Amit Kucheria wrote:

> I was using initcall_debugging on a QCOM platform and ran across a bunch of
> driver initcalls that are enabled even if their SoC support is disabled.

What exactly is the problem you're trying to fix here?  For the
drivers I looked at these were bog standard register the driver
with the subsystem type initcalls on optional drivers so not
doing anything particularly disruptive or anything like that.
For any given system that's going to be an issue for the
overwhelming majority of drivers on the tree, including those
that aren't associated with any particular architecture.
Amit Kucheria Sept. 12, 2019, 9:47 a.m. UTC | #3
Hi Arnd,

On Thu, Sep 12, 2019 at 2:59 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Sep 12, 2019 at 12:18 AM Amit Kucheria <amit.kucheria@linaro.org> wrote:
> >
> > I was using initcall_debugging on a QCOM platform and ran across a bunch of
> > driver initcalls that are enabled even if their SoC support is disabled.
> >
> > Here are some fixups for a subset of them.
>
> The idea seems reasonable, disabling a platform may just turn off
> all the drivers that are not useful elsewhere, but there are mistakes
> in a lot of your changes, so I'm certainly not applying these for 5.4.

OK, thanks for confirming that you have no objections to such changes, per-se.

I'll spend some more time ensuring COMPILE_TEST coverage for these
cleanups. I only focused on quickly cleaning up my initcall_debug
output for now.

> Generally speaking, the way that works best is
>
> config SUBSYS_DRIVER_FOO
>        tristate "SUBSYS support for FOO platform"
>        depends on ARCH_FOO || COMPILE_TEST
>        depends on SUBSYS
>        default "m" if ARCH_FOO
>
> This means it's enabled as a loadable module by default (use
> default "y" instead where necessary) as long as the platform
> is enabled, but an x86 allmodconfig build also includes it
> because of COMPILE_TEST, while any configuration without
> ARCH_FOO that is not compile-testing cannot enable it.

How would you like to handle defconfigs which list a driver
explicitly? Should we add ARCH_FOO to those defconfigs or remove
DRIVER_FOO from them?

Regards,
Amit
Mark Brown Sept. 12, 2019, 10:03 a.m. UTC | #4
On Thu, Sep 12, 2019 at 11:29:00AM +0200, Arnd Bergmann wrote:

> Generally speaking, the way that works best is

> config SUBSYS_DRIVER_FOO
>        tristate "SUBSYS support for FOO platform"
>        depends on ARCH_FOO || COMPILE_TEST
>        depends on SUBSYS
>        default "m" if ARCH_FOO

> This means it's enabled as a loadable module by default (use
> default "y" instead where necessary) as long as the platform
> is enabled, but an x86 allmodconfig build also includes it
> because of COMPILE_TEST, while any configuration without
> ARCH_FOO that is not compile-testing cannot enable it.

Indeed, though we shouldn't be adding any default m/y to things
that don't already default on.
Amit Kucheria Sept. 12, 2019, 10:03 a.m. UTC | #5
On Thu, Sep 12, 2019 at 3:17 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Sep 12, 2019 at 03:48:44AM +0530, Amit Kucheria wrote:
>
> > I was using initcall_debugging on a QCOM platform and ran across a bunch of
> > driver initcalls that are enabled even if their SoC support is disabled.
>
> What exactly is the problem you're trying to fix here?  For the
> drivers I looked at these were bog standard register the driver
> with the subsystem type initcalls on optional drivers so not
> doing anything particularly disruptive or anything like that.

I was trying to prune the defconfig only to drivers that make sense on
the SoC. e.g. Why should I see a brcmstb_soc_device_early_init() call
on a QCOM system when I've disabled ARCH_BRCMSTB?

I came across this while trying to figure out how to make thermal and
cpufreq frameworks initialise as early as possible.

> For any given system that's going to be an issue for the
> overwhelming majority of drivers on the tree, including those
> that aren't associated with any particular architecture.

Indeed. From a quick check, MFD and GPIO has a bunch of 'generic'
drivers that aren't SoC-specific. I'm sure there are several such
drivers in regulator framework too. They don't need to be 'fixed'.

I was just trying to ring-fence obvious SoC-specific drivers behind a
ARCH_FOO dependency since they seemed like low-hanging fruit. Let me
know if it isn't a good use of everyone's time.

Regards,
Amit
Mark Brown Sept. 12, 2019, 10:53 a.m. UTC | #6
On Thu, Sep 12, 2019 at 03:33:20PM +0530, Amit Kucheria wrote:
> On Thu, Sep 12, 2019 at 3:17 PM Mark Brown <broonie@kernel.org> wrote:

> > > I was using initcall_debugging on a QCOM platform and ran across a bunch of
> > > driver initcalls that are enabled even if their SoC support is disabled.

> > What exactly is the problem you're trying to fix here?  For the
> > drivers I looked at these were bog standard register the driver
> > with the subsystem type initcalls on optional drivers so not
> > doing anything particularly disruptive or anything like that.

> I was trying to prune the defconfig only to drivers that make sense on
> the SoC. e.g. Why should I see a brcmstb_soc_device_early_init() call
> on a QCOM system when I've disabled ARCH_BRCMSTB?

So this is really just the standard make Kconfig easier to use by
filtering out noise thing.  It'd be clearer if you said that in
the changelog, and like the review comments have been saying you
need to leave in an || COMPILE_TEST in there otherwise it's
actively harmful.

> I came across this while trying to figure out how to make thermal and
> cpufreq frameworks initialise as early as possible.

AFAICT you'd also have been happy if you just built these drivers
modular?