[PATCH v1] kbuild: Disable compile testing if HAVE_LEGACY_CLK enabled

[PATCH v1] kbuild: Disable compile testing if HAVE_LEGACY_CLK enabled

[Dmitry Osipenko]

There are couple older platforms that can't be compile-tested because they
partially implement CLK API. It causes build failure of kernel drivers due
to the missing symbols of the unimplemented part of CLK API.

These platforms are: ARM EP93XX, ARM OMAP1, m68k ColdFire, MIPS AR7,
                     MIPS Ralink.

Disable compile-testing for HAVE_LEGACY_CLK=y.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 init/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/init/Kconfig b/init/Kconfig
index 173a474012d7..42701b04be00 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -131,7 +131,7 @@ config INIT_ENV_ARG_LIMIT
 
 config COMPILE_TEST
 	bool "Compile also drivers which will not load"
-	depends on HAS_IOMEM
+	depends on HAS_IOMEM && !HAVE_LEGACY_CLK
 	help
 	  Some drivers can be compiled on a different platform than they are
 	  intended to be run on. Despite they cannot be loaded there (or even
-- 
2.30.2

Re: [PATCH v1] kbuild: Disable compile testing if HAVE_LEGACY_CLK enabled

[Geert Uytterhoeven]

Hi Dmitry,

On Mon, May 24, 2021 at 1:26 AM Dmitry Osipenko <digetx@gmail.com> wrote:
> There are couple older platforms that can't be compile-tested because they
> partially implement CLK API. It causes build failure of kernel drivers due
> to the missing symbols of the unimplemented part of CLK API.
>
> These platforms are: ARM EP93XX, ARM OMAP1, m68k ColdFire, MIPS AR7,
>                      MIPS Ralink.
>
> Disable compile-testing for HAVE_LEGACY_CLK=y.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>

Thanks for your patch!

> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -131,7 +131,7 @@ config INIT_ENV_ARG_LIMIT
>
>  config COMPILE_TEST
>         bool "Compile also drivers which will not load"
> -       depends on HAS_IOMEM
> +       depends on HAS_IOMEM && !HAVE_LEGACY_CLK

That sounds a bit drastic to me.  Usually we just try to implement the
missing functionality, or provide stubs.
Which functions are missing?

>         help
>           Some drivers can be compiled on a different platform than they are
>           intended to be run on. Despite they cannot be loaded there (or even

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 v1] kbuild: Disable compile testing if HAVE_LEGACY_CLK enabled

[Dmitry Osipenko]

24.05.2021 11:54, Geert Uytterhoeven пишет:
> Hi Dmitry,
> 
> On Mon, May 24, 2021 at 1:26 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>> There are couple older platforms that can't be compile-tested because they
>> partially implement CLK API. It causes build failure of kernel drivers due
>> to the missing symbols of the unimplemented part of CLK API.
>>
>> These platforms are: ARM EP93XX, ARM OMAP1, m68k ColdFire, MIPS AR7,
>>                      MIPS Ralink.
>>
>> Disable compile-testing for HAVE_LEGACY_CLK=y.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> 
> Thanks for your patch!
> 
>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -131,7 +131,7 @@ config INIT_ENV_ARG_LIMIT
>>
>>  config COMPILE_TEST
>>         bool "Compile also drivers which will not load"
>> -       depends on HAS_IOMEM
>> +       depends on HAS_IOMEM && !HAVE_LEGACY_CLK
> 
> That sounds a bit drastic to me.  Usually we just try to implement the
> missing functionality, or provide stubs.
> Which functions are missing?

Everything that belongs to CONFIG_COMMON_CLK needs stubs.

That is everything under CONFIG_HAVE_CLK [1], excluding functions
belonging to clk-devres.o and clk-bulk.o [2]. The HAVE_LEGACY_CLK
selects HAVE_CLK, but the COMMON_CLK is under HAVE_CLK too.

[1]
https://elixir.bootlin.com/linux/v5.13-rc3/source/include/linux/clk.h#L786
[2]
https://elixir.bootlin.com/linux/v5.13-rc3/source/drivers/clk/Makefile#L3

This problem is repeated over and over again for the past years. Some
maintainers are adding "depends on COMMON_CLK" for COMPILE_TEST of each
driver, but this doesn't solve the root of the problem, and thus, it's
constantly reoccurring.

Recently Krzysztof Kozlowski added couple more clk stubs for MIPS, but
still lots of stubs are missing. Some platforms don't have any stubs at
all and apparently nobody cares to fix them.

There 3 possible solutions:

1. Factor out COMMON_CLK from HAVE_LEGACY_CLK, if this is possible
2. Build stubs universally, maybe using weak functions.
3. Disable compile-testing for HAVE_LEGACY_CLK

The third option is the simplest. If anyone will care to fix those
legacy platforms properly, then compile-testing could be re-enabled for
them. This is my proposal.

Re: [PATCH v1] kbuild: Disable compile testing if HAVE_LEGACY_CLK enabled

[Krzysztof Kozlowski]

On 24/05/2021 08:39, Dmitry Osipenko wrote:
> 24.05.2021 11:54, Geert Uytterhoeven пишет:
>> Hi Dmitry,
>>
>> On Mon, May 24, 2021 at 1:26 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>>> There are couple older platforms that can't be compile-tested because they
>>> partially implement CLK API. It causes build failure of kernel drivers due
>>> to the missing symbols of the unimplemented part of CLK API.
>>>
>>> These platforms are: ARM EP93XX, ARM OMAP1, m68k ColdFire, MIPS AR7,
>>>                      MIPS Ralink.
>>>
>>> Disable compile-testing for HAVE_LEGACY_CLK=y.
>>>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>
>> Thanks for your patch!
>>
>>> --- a/init/Kconfig
>>> +++ b/init/Kconfig
>>> @@ -131,7 +131,7 @@ config INIT_ENV_ARG_LIMIT
>>>
>>>  config COMPILE_TEST
>>>         bool "Compile also drivers which will not load"
>>> -       depends on HAS_IOMEM
>>> +       depends on HAS_IOMEM && !HAVE_LEGACY_CLK
>>
>> That sounds a bit drastic to me.  Usually we just try to implement the
>> missing functionality, or provide stubs.
>> Which functions are missing?
> 
> Everything that belongs to CONFIG_COMMON_CLK needs stubs.
> 
> That is everything under CONFIG_HAVE_CLK [1], excluding functions
> belonging to clk-devres.o and clk-bulk.o [2]. The HAVE_LEGACY_CLK
> selects HAVE_CLK, but the COMMON_CLK is under HAVE_CLK too.
> 
> [1]
> https://elixir.bootlin.com/linux/v5.13-rc3/source/include/linux/clk.h#L786
> [2]
> https://elixir.bootlin.com/linux/v5.13-rc3/source/drivers/clk/Makefile#L3
> 
> This problem is repeated over and over again for the past years. Some
> maintainers are adding "depends on COMMON_CLK" for COMPILE_TEST of each
> driver, but this doesn't solve the root of the problem, and thus, it's
> constantly reoccurring.
> 
> Recently Krzysztof Kozlowski added couple more clk stubs for MIPS, but
> still lots of stubs are missing. Some platforms don't have any stubs at
> all and apparently nobody cares to fix them.
> 
> There 3 possible solutions:
> 
> 1. Factor out COMMON_CLK from HAVE_LEGACY_CLK, if this is possible
> 2. Build stubs universally, maybe using weak functions.

I vote for this one - global stubs.

Or for a new one:
4. Disable COMPILE_TEST for specific platforms (mentioned in commit
msg). Eventually could be like:
config RALINK
	depends !COMPILE_TEST || (COMPILE_TEST && COMMON_CLK)

Why? Because it is expected that a driver requiring/using missing clock
stubs won't run on such legacy platform. Currently it cannot run because
simply missing stubs will break build. Option (2) would make them
compileable but not runnable, which is fine. However having a build time
failure is better, so maybe let's just isolate platforms which are very
poor in compile testing and don't enable them for most of the configs.


Best regards,
Krzysztof

Re: [PATCH v1] kbuild: Disable compile testing if HAVE_LEGACY_CLK enabled

[Geert Uytterhoeven]

Hi Krzysztof,

On Tue, May 25, 2021 at 2:04 PM Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
> On 24/05/2021 08:39, Dmitry Osipenko wrote:
> > 24.05.2021 11:54, Geert Uytterhoeven пишет:
> >> On Mon, May 24, 2021 at 1:26 AM Dmitry Osipenko <digetx@gmail.com> wrote:
> >>> There are couple older platforms that can't be compile-tested because they
> >>> partially implement CLK API. It causes build failure of kernel drivers due
> >>> to the missing symbols of the unimplemented part of CLK API.
> >>>
> >>> These platforms are: ARM EP93XX, ARM OMAP1, m68k ColdFire, MIPS AR7,
> >>>                      MIPS Ralink.
> >>>
> >>> Disable compile-testing for HAVE_LEGACY_CLK=y.
> >>>
> >>> Reported-by: kernel test robot <lkp@intel.com>
> >>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >>
> >> Thanks for your patch!
> >>
> >>> --- a/init/Kconfig
> >>> +++ b/init/Kconfig
> >>> @@ -131,7 +131,7 @@ config INIT_ENV_ARG_LIMIT
> >>>
> >>>  config COMPILE_TEST
> >>>         bool "Compile also drivers which will not load"
> >>> -       depends on HAS_IOMEM
> >>> +       depends on HAS_IOMEM && !HAVE_LEGACY_CLK
> >>
> >> That sounds a bit drastic to me.  Usually we just try to implement the
> >> missing functionality, or provide stubs.
> >> Which functions are missing?
> >
> > Everything that belongs to CONFIG_COMMON_CLK needs stubs.
> >
> > That is everything under CONFIG_HAVE_CLK [1], excluding functions
> > belonging to clk-devres.o and clk-bulk.o [2]. The HAVE_LEGACY_CLK
> > selects HAVE_CLK, but the COMMON_CLK is under HAVE_CLK too.
> >
> > [1]
> > https://elixir.bootlin.com/linux/v5.13-rc3/source/include/linux/clk.h#L786
> > [2]
> > https://elixir.bootlin.com/linux/v5.13-rc3/source/drivers/clk/Makefile#L3
> >
> > This problem is repeated over and over again for the past years. Some
> > maintainers are adding "depends on COMMON_CLK" for COMPILE_TEST of each
> > driver, but this doesn't solve the root of the problem, and thus, it's
> > constantly reoccurring.
> >
> > Recently Krzysztof Kozlowski added couple more clk stubs for MIPS, but
> > still lots of stubs are missing. Some platforms don't have any stubs at
> > all and apparently nobody cares to fix them.
> >
> > There 3 possible solutions:
> >
> > 1. Factor out COMMON_CLK from HAVE_LEGACY_CLK, if this is possible
> > 2. Build stubs universally, maybe using weak functions.
>
> I vote for this one - global stubs.

Yep.

> Or for a new one:
> 4. Disable COMPILE_TEST for specific platforms (mentioned in commit
> msg). Eventually could be like:
> config RALINK
>         depends !COMPILE_TEST || (COMPILE_TEST && COMMON_CLK)

That's a neat idea!

Of course there's a fifth option:

5. Convert legacy platforms to COMMON_CLK.

Which is already happening for ARM EP93XX.

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 v1] kbuild: Disable compile testing if HAVE_LEGACY_CLK enabled

[Dmitry Osipenko]

25.05.2021 15:19, Geert Uytterhoeven пишет:
...
>>> There 3 possible solutions:
>>>
>>> 1. Factor out COMMON_CLK from HAVE_LEGACY_CLK, if this is possible
>>> 2. Build stubs universally, maybe using weak functions.
>>
>> I vote for this one - global stubs.
> 
> Yep.
> 
>> Or for a new one:
>> 4. Disable COMPILE_TEST for specific platforms (mentioned in commit
>> msg). Eventually could be like:
>> config RALINK
>>         depends !COMPILE_TEST || (COMPILE_TEST && COMMON_CLK)
> 
> That's a neat idea!
> 
> Of course there's a fifth option:
> 
> 5. Convert legacy platforms to COMMON_CLK.
> 
> Which is already happening for ARM EP93XX.

I'll try to take a closer look at alternative options, thank you for
yours input.

Re: [PATCH v1] kbuild: Disable compile testing if HAVE_LEGACY_CLK enabled

[Randy Dunlap]

On 5/25/21 2:29 PM, Dmitry Osipenko wrote:
> 25.05.2021 15:19, Geert Uytterhoeven пишет:
> ...
>>>> There 3 possible solutions:
>>>>
>>>> 1. Factor out COMMON_CLK from HAVE_LEGACY_CLK, if this is possible
>>>> 2. Build stubs universally, maybe using weak functions.
>>>
>>> I vote for this one - global stubs.
>>
>> Yep.
>>
>>> Or for a new one:
>>> 4. Disable COMPILE_TEST for specific platforms (mentioned in commit
>>> msg). Eventually could be like:
>>> config RALINK
>>>         depends !COMPILE_TEST || (COMPILE_TEST && COMMON_CLK)
>>
>> That's a neat idea!
>>
>> Of course there's a fifth option:
>>
>> 5. Convert legacy platforms to COMMON_CLK.
>>
>> Which is already happening for ARM EP93XX.
> 
> I'll try to take a closer look at alternative options, thank you for
> yours input.

Hi,

If you are messing around with HAVE_LEGACY_CLK and COMMON_CLK
and don't mind, please have a look at this report from
'kernel test robot':
  https://lore.kernel.org/lkml/202103162301.oomY9NwI-lkp@intel.com/


Maybe this one is more MIPS-specific. I dunno.
I tried to come up with a patch for it and failed.

I will be happy to built-test any patches that you come up with.

Maybe I have just looked at this too long...

thanks.
-- 
~Randy

Re: [PATCH v1] kbuild: Disable compile testing if HAVE_LEGACY_CLK enabled

[Arnd Bergmann]

On Wed, May 26, 2021 at 2:39 AM Randy Dunlap <rdunlap@infradead.org> wrote:
> On 5/25/21 2:29 PM, Dmitry Osipenko wrote:
> > 25.05.2021 15:19, Geert Uytterhoeven пишет:
> If you are messing around with HAVE_LEGACY_CLK and COMMON_CLK
> and don't mind, please have a look at this report from
> 'kernel test robot':
>   https://lore.kernel.org/lkml/202103162301.oomY9NwI-lkp@intel.com/
>
>
> Maybe this one is more MIPS-specific. I dunno.
> I tried to come up with a patch for it and failed.
>
> I will be happy to built-test any patches that you come up with.
>
> Maybe I have just looked at this too long...

This is exactly the case that Dmitry is trying to solve. The problem
is that bcm63xx
and lantiq set CONFIG_HAVE_LEGACY_CLK but lack the clk_set_parent()
and clk_get_parent() functions. We could add the same stubs for these
two that we have for the other legacy clk implementations, or rework the
handling of legacy clocks completely.

       Arnd

Re: [PATCH v1] kbuild: Disable compile testing if HAVE_LEGACY_CLK enabled

[Arnd Bergmann]

On Mon, May 24, 2021 at 2:39 PM Dmitry Osipenko <digetx@gmail.com> wrote:
> 24.05.2021 11:54, Geert Uytterhoeven пишет:
> > Hi Dmitry,
> >
> > On Mon, May 24, 2021 at 1:26 AM Dmitry Osipenko <digetx@gmail.com> wrote:
> >> There are couple older platforms that can't be compile-tested because they
> >> partially implement CLK API. It causes build failure of kernel drivers due
> >> to the missing symbols of the unimplemented part of CLK API.
> >>
> >> These platforms are: ARM EP93XX, ARM OMAP1, m68k ColdFire, MIPS AR7,
> >>                      MIPS Ralink.

There is also most of arch/sh/, with the one exception of J2.

On MIPS, I see BCM63XX and Lantiq in addition to AR7 and Ralink.

I think the patches to convert ep93xx are under discussion at the moment,
and I have previously done an implementation for omap1 that I have meant
to dig out for a while.

Both of the Arm platforms should work fine with compile testing, as I
have previously added the stubs as necessary.

> >> @@ -131,7 +131,7 @@ config INIT_ENV_ARG_LIMIT
> >>
> >>  config COMPILE_TEST
> >>         bool "Compile also drivers which will not load"
> >> -       depends on HAS_IOMEM
> >> +       depends on HAS_IOMEM && !HAVE_LEGACY_CLK
> >
> > That sounds a bit drastic to me.  Usually we just try to implement the
> > missing functionality, or provide stubs.
> > Which functions are missing?
>
> Everything that belongs to CONFIG_COMMON_CLK needs stubs.
>
> That is everything under CONFIG_HAVE_CLK [1], excluding functions
> belonging to clk-devres.o and clk-bulk.o [2]. The HAVE_LEGACY_CLK
> selects HAVE_CLK, but the COMMON_CLK is under HAVE_CLK too.
>
> [1]
> https://elixir.bootlin.com/linux/v5.13-rc3/source/include/linux/clk.h#L786
> [2]
> https://elixir.bootlin.com/linux/v5.13-rc3/source/drivers/clk/Makefile#L3
>
> This problem is repeated over and over again for the past years. Some
> maintainers are adding "depends on COMMON_CLK" for COMPILE_TEST of each
> driver, but this doesn't solve the root of the problem, and thus, it's
> constantly reoccurring.

These dependencies tend to reflect the actual requirements though: if a
driver calls the clk interfaces, it is likely to require them at
runtime as well.

If a driver that calls clk_get_rate() does not have a fallback to deal with '0'
getting returned, this means that building it without CONFIG_COMMON_CLK
gives you a driver that can't work on any hardware at all. In this case

  depends on COMMON_CLK || COMPILE_TEST

does reflect what we want.depends onb

> Recently Krzysztof Kozlowski added couple more clk stubs for MIPS, but
> still lots of stubs are missing. Some platforms don't have any stubs at
> all and apparently nobody cares to fix them.
>
> There 3 possible solutions:
>
> 1. Factor out COMMON_CLK from HAVE_LEGACY_CLK, if this is possible
> 2. Build stubs universally, maybe using weak functions.
> 3. Disable compile-testing for HAVE_LEGACY_CLK
>
> The third option is the simplest. If anyone will care to fix those
> legacy platforms properly, then compile-testing could be re-enabled for
> them. This is my proposal.

It is pretty much a given that arch/sh won't be converted to common clk
at this point, and I think disabling COMPILE_TEST on arch/sh globabally
makes sense.

Converting coldfire and ar7/lantiq/bcm63xx/ralink to common_clk shouldn't
actually be too hard, but I don't think it's a priority for anyone at
the moment.

One other idea I can think of is to redefine CONFIG_HAVE_CLK. At the
moment, this is defined as (COMMON_CLK || HAVE_LEGACY_CLK),
not literally, but for all practical purposes. If HAVE_CLK is disabled,
we get the stubs, so the build failures you saw happen very rarely
on drivers that use clk interfaces but have no such Kconfig dependency.

If we change this to define it as

config HAVE_CLK
        def_bool COMMON_CLK || COMPILE_TEST

while annotating the sh/coldfire/ar7/bcm63xx/lantiq/ralink specific
drivers as 'depends on HAVE_CLK || HAVE_LEGACY_CLK', then
we can compile test any driver, but actually get the build failures
for drivers that lack a 'depends on COMMON_CLK' and can fix
them up more easily. This probably requires adding a ton of
missing dependencies first that happened to get ignored in the
past.

If we don't do this, another approach would be to provide
global stubs for anything that isn't already provided by
the platforms. Here is a list of which interfaces each version
actually has implemented (Y), stubbed out (S) or lacks (N):

                omap1   ep93xx  ar7     bcm63xx lantiq  ralink  coldf   sh
clk_enable      Y       Y       S       Y       Y       S       Y       Y
clk_disable     Y       Y       S       Y       Y       S       Y       Y
clk_get_rate    Y       Y       Y       Y       Y       Y       S       Y
clk_round_rate  Y       S       S       S       Y       S       S       Y
clk_set_rate    Y       Y       S       S       Y       S       S       Y
clk_set_parent  S       S       S       N       N       S       S       Y
clk_get_parent  S       Y       S       N       N       S       S       Y
clk_get         N       N       Y       N       N       N       Y       N
clk_put         N       N       S       N       N       N       S       N

- ar7 and coldfire are the only ones that lack the generic
  clkdev lookup but instead provide their own clk_get/clk_put
  functions. This can probably get addressed easily.

- The clk_get_parent/set_parent functions are only
  provided on ep93xx and sh. Once ep93xx is out of the
  way, we could remove the private stubs and always
  provide the global ones, except for sh.

- The remaining five functions
  (enable/disable/get_rate/set_rate/round_rate) have a
  consistent interface across all eight platforms, they
  are always exported in that case.

       Arnd