[v4,14/16] PM / devfreq: tegra: Enable COMPILE_TEST for the driver
diff mbox series

Message ID 20190501233815.32643-15-digetx@gmail.com
State New
Headers show
Series
  • NVIDIA Tegra devfreq improvements and Tegra20/30 support
Related show

Commit Message

Dmitry Osipenko May 1, 2019, 11:38 p.m. UTC
The driver's compilation doesn't have any specific dependencies, hence
the COMPILE_TEST option can be supported in Kconfig.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Thierry Reding June 4, 2019, 11:20 a.m. UTC | #1
On Thu, May 02, 2019 at 02:38:13AM +0300, Dmitry Osipenko wrote:
> The driver's compilation doesn't have any specific dependencies, hence
> the COMPILE_TEST option can be supported in Kconfig.
> 
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 56db9dc05edb..a6bba6e1e7d9 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -93,7 +93,7 @@ config ARM_EXYNOS_BUS_DEVFREQ
>  
>  config ARM_TEGRA_DEVFREQ
>  	tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
> -	depends on ARCH_TEGRA
> +	depends on ARCH_TEGRA || COMPILE_TEST
>  	select PM_OPP
>  	help
>  	  This adds the DEVFREQ driver for the Tegra family of SoCs.

You need to be careful with these. You're using I/O register accessors,
which are not supported on the UM architecture, for example.

This may end up getting flagged during build testing.

Thierry
Dmitry Osipenko June 4, 2019, 1:53 p.m. UTC | #2
04.06.2019 14:20, Thierry Reding пишет:
> On Thu, May 02, 2019 at 02:38:13AM +0300, Dmitry Osipenko wrote:
>> The driver's compilation doesn't have any specific dependencies, hence
>> the COMPILE_TEST option can be supported in Kconfig.
>>
>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/devfreq/Kconfig | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> index 56db9dc05edb..a6bba6e1e7d9 100644
>> --- a/drivers/devfreq/Kconfig
>> +++ b/drivers/devfreq/Kconfig
>> @@ -93,7 +93,7 @@ config ARM_EXYNOS_BUS_DEVFREQ
>>  
>>  config ARM_TEGRA_DEVFREQ
>>  	tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
>> -	depends on ARCH_TEGRA
>> +	depends on ARCH_TEGRA || COMPILE_TEST
>>  	select PM_OPP
>>  	help
>>  	  This adds the DEVFREQ driver for the Tegra family of SoCs.
> 
> You need to be careful with these. You're using I/O register accessors,
> which are not supported on the UM architecture, for example.
> 
> This may end up getting flagged during build testing.

We have similar cases in other drivers and it doesn't cause any known
problems because (I think) build-bots are aware of this detail. Hence
there is no real need to be overreactive here and in this particular
case it's better to react to real problems once they show up (we already
did that by fixing build breakage caused by a CLK API problem found by
bot in v3). Does it sound like a good argument to you? ACK?
Thierry Reding June 4, 2019, 2:10 p.m. UTC | #3
On Tue, Jun 04, 2019 at 04:53:17PM +0300, Dmitry Osipenko wrote:
> 04.06.2019 14:20, Thierry Reding пишет:
> > On Thu, May 02, 2019 at 02:38:13AM +0300, Dmitry Osipenko wrote:
> >> The driver's compilation doesn't have any specific dependencies, hence
> >> the COMPILE_TEST option can be supported in Kconfig.
> >>
> >> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >>  drivers/devfreq/Kconfig | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> >> index 56db9dc05edb..a6bba6e1e7d9 100644
> >> --- a/drivers/devfreq/Kconfig
> >> +++ b/drivers/devfreq/Kconfig
> >> @@ -93,7 +93,7 @@ config ARM_EXYNOS_BUS_DEVFREQ
> >>  
> >>  config ARM_TEGRA_DEVFREQ
> >>  	tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
> >> -	depends on ARCH_TEGRA
> >> +	depends on ARCH_TEGRA || COMPILE_TEST
> >>  	select PM_OPP
> >>  	help
> >>  	  This adds the DEVFREQ driver for the Tegra family of SoCs.
> > 
> > You need to be careful with these. You're using I/O register accessors,
> > which are not supported on the UM architecture, for example.
> > 
> > This may end up getting flagged during build testing.
> 
> We have similar cases in other drivers and it doesn't cause any known
> problems because (I think) build-bots are aware of this detail. Hence

I don't understand how the build-bots would be aware of this detail.
Unless you explicitly state what the dependencies are, how would the
build-bots know? Perhaps there's some logic built-in somewhere that I
don't know about?

> there is no real need to be overreactive here and in this particular
> case it's better to react to real problems once they show up (we already
> did that by fixing build breakage caused by a CLK API problem found by
> bot in v3). Does it sound like a good argument to you? ACK?

No. This strikes me as a strange argument. I'm pointing out a potential
source of problems and you just brush it aside claiming that it's not
true, or even if it was true that we'll see eventually and we can fix it
up when there's an actual problem.

Why would you want to have to fix things up if you can avoid breakage in
the first place by being a little proactive?

Thierry
Thierry Reding June 4, 2019, 2:18 p.m. UTC | #4
On Tue, Jun 04, 2019 at 04:10:31PM +0200, Thierry Reding wrote:
> On Tue, Jun 04, 2019 at 04:53:17PM +0300, Dmitry Osipenko wrote:
> > 04.06.2019 14:20, Thierry Reding пишет:
> > > On Thu, May 02, 2019 at 02:38:13AM +0300, Dmitry Osipenko wrote:
> > >> The driver's compilation doesn't have any specific dependencies, hence
> > >> the COMPILE_TEST option can be supported in Kconfig.
> > >>
> > >> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > >> ---
> > >>  drivers/devfreq/Kconfig | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> > >> index 56db9dc05edb..a6bba6e1e7d9 100644
> > >> --- a/drivers/devfreq/Kconfig
> > >> +++ b/drivers/devfreq/Kconfig
> > >> @@ -93,7 +93,7 @@ config ARM_EXYNOS_BUS_DEVFREQ
> > >>  
> > >>  config ARM_TEGRA_DEVFREQ
> > >>  	tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
> > >> -	depends on ARCH_TEGRA
> > >> +	depends on ARCH_TEGRA || COMPILE_TEST
> > >>  	select PM_OPP
> > >>  	help
> > >>  	  This adds the DEVFREQ driver for the Tegra family of SoCs.
> > > 
> > > You need to be careful with these. You're using I/O register accessors,
> > > which are not supported on the UM architecture, for example.
> > > 
> > > This may end up getting flagged during build testing.
> > 
> > We have similar cases in other drivers and it doesn't cause any known
> > problems because (I think) build-bots are aware of this detail. Hence
> 
> I don't understand how the build-bots would be aware of this detail.
> Unless you explicitly state what the dependencies are, how would the
> build-bots know? Perhaps there's some logic built-in somewhere that I
> don't know about?

So looks like COMPILE_TEST has a !UML dependency, so this might just
work.

Acked-by: Thierry Reding <treding@nvidia.com>
Dmitry Osipenko June 4, 2019, 2:41 p.m. UTC | #5
04.06.2019 17:18, Thierry Reding пишет:
> On Tue, Jun 04, 2019 at 04:10:31PM +0200, Thierry Reding wrote:
>> On Tue, Jun 04, 2019 at 04:53:17PM +0300, Dmitry Osipenko wrote:
>>> 04.06.2019 14:20, Thierry Reding пишет:
>>>> On Thu, May 02, 2019 at 02:38:13AM +0300, Dmitry Osipenko wrote:
>>>>> The driver's compilation doesn't have any specific dependencies, hence
>>>>> the COMPILE_TEST option can be supported in Kconfig.
>>>>>
>>>>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>> ---
>>>>>  drivers/devfreq/Kconfig | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>>>> index 56db9dc05edb..a6bba6e1e7d9 100644
>>>>> --- a/drivers/devfreq/Kconfig
>>>>> +++ b/drivers/devfreq/Kconfig
>>>>> @@ -93,7 +93,7 @@ config ARM_EXYNOS_BUS_DEVFREQ
>>>>>  
>>>>>  config ARM_TEGRA_DEVFREQ
>>>>>  	tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
>>>>> -	depends on ARCH_TEGRA
>>>>> +	depends on ARCH_TEGRA || COMPILE_TEST
>>>>>  	select PM_OPP
>>>>>  	help
>>>>>  	  This adds the DEVFREQ driver for the Tegra family of SoCs.
>>>>
>>>> You need to be careful with these. You're using I/O register accessors,
>>>> which are not supported on the UM architecture, for example.
>>>>
>>>> This may end up getting flagged during build testing.
>>>
>>> We have similar cases in other drivers and it doesn't cause any known
>>> problems because (I think) build-bots are aware of this detail. Hence
>>
>> I don't understand how the build-bots would be aware of this detail.
>> Unless you explicitly state what the dependencies are, how would the
>> build-bots know? Perhaps there's some logic built-in somewhere that I
>> don't know about?
> 
> So looks like COMPILE_TEST has a !UML dependency, so this might just
> work.
> 
> Acked-by: Thierry Reding <treding@nvidia.com>
> 

Thank you very much for the clarification! Certainly that would caused
problems already since there are such cases all over the kernel,
including Tegra drivers.
Thierry Reding June 4, 2019, 2:50 p.m. UTC | #6
On Tue, Jun 04, 2019 at 05:41:53PM +0300, Dmitry Osipenko wrote:
> 04.06.2019 17:18, Thierry Reding пишет:
> > On Tue, Jun 04, 2019 at 04:10:31PM +0200, Thierry Reding wrote:
> >> On Tue, Jun 04, 2019 at 04:53:17PM +0300, Dmitry Osipenko wrote:
> >>> 04.06.2019 14:20, Thierry Reding пишет:
> >>>> On Thu, May 02, 2019 at 02:38:13AM +0300, Dmitry Osipenko wrote:
> >>>>> The driver's compilation doesn't have any specific dependencies, hence
> >>>>> the COMPILE_TEST option can be supported in Kconfig.
> >>>>>
> >>>>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> >>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >>>>> ---
> >>>>>  drivers/devfreq/Kconfig | 2 +-
> >>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> >>>>> index 56db9dc05edb..a6bba6e1e7d9 100644
> >>>>> --- a/drivers/devfreq/Kconfig
> >>>>> +++ b/drivers/devfreq/Kconfig
> >>>>> @@ -93,7 +93,7 @@ config ARM_EXYNOS_BUS_DEVFREQ
> >>>>>  
> >>>>>  config ARM_TEGRA_DEVFREQ
> >>>>>  	tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
> >>>>> -	depends on ARCH_TEGRA
> >>>>> +	depends on ARCH_TEGRA || COMPILE_TEST
> >>>>>  	select PM_OPP
> >>>>>  	help
> >>>>>  	  This adds the DEVFREQ driver for the Tegra family of SoCs.
> >>>>
> >>>> You need to be careful with these. You're using I/O register accessors,
> >>>> which are not supported on the UM architecture, for example.
> >>>>
> >>>> This may end up getting flagged during build testing.
> >>>
> >>> We have similar cases in other drivers and it doesn't cause any known
> >>> problems because (I think) build-bots are aware of this detail. Hence
> >>
> >> I don't understand how the build-bots would be aware of this detail.
> >> Unless you explicitly state what the dependencies are, how would the
> >> build-bots know? Perhaps there's some logic built-in somewhere that I
> >> don't know about?
> > 
> > So looks like COMPILE_TEST has a !UML dependency, so this might just
> > work.
> > 
> > Acked-by: Thierry Reding <treding@nvidia.com>
> > 
> 
> Thank you very much for the clarification! Certainly that would caused
> problems already since there are such cases all over the kernel,
> including Tegra drivers.

In the cases that I'm aware of we used to explicitly list all the
dependencies that would've otherwise been pulled in by the ARCH_TEGRA
dependency to make sure there were no issues.

Now that we've been discussing this I vaguely recall a discussion about
the only real issue nowadays being HAS_IOMEM and since that's only
missing on UML, that may have been the reason for why the !UML
dependency was added.

Yes, looks like that was it:

	commit bc083a64b6c035135c0f80718f9e9192cc0867c6
	Author: Richard Weinberger <richard@nod.at>
	Date:   Tue Aug 2 14:03:27 2016 -0700

	    init/Kconfig: make COMPILE_TEST depend on !UML

	    UML is a bit special since it does not have iomem nor dma.  That means a
	    lot of drivers will not build if they miss a dependency on HAS_IOMEM.
	    s390 used to have the same issues but since it gained PCI support UML is
	    the only stranger.

	    We are tired of patching dozens of new drivers after every merge window
	    just to un-break allmod/yesconfig UML builds.  One could argue that a
	    decent driver has to know on what it depends and therefore a missing
	    HAS_IOMEM dependency is a clear driver bug.  But the dependency not
	    obvious and not everyone does UML builds with COMPILE_TEST enabled when
	    developing a device driver.

	    A possible solution to make these builds succeed on UML would be
	    providing stub functions for ioremap() and friends which fail upon
	    runtime.  Another one is simply disabling COMPILE_TEST for UML.  Since
	    it is the least hassle and does not force use to fake iomem support
	    let's do the latter.

	    Link: http://lkml.kernel.org/r/1466152995-28367-1-git-send-email-richard@nod.at
	    Signed-off-by: Richard Weinberger <richard@nod.at>
	    Acked-by: Arnd Bergmann <arnd@arndb.de>
	    Cc: Al Viro <viro@zeniv.linux.org.uk>
	    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
	    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Oh wow... almost three years now.

Thierry
Dmitry Osipenko June 4, 2019, 3:15 p.m. UTC | #7
04.06.2019 17:50, Thierry Reding пишет:
> On Tue, Jun 04, 2019 at 05:41:53PM +0300, Dmitry Osipenko wrote:
>> 04.06.2019 17:18, Thierry Reding пишет:
>>> On Tue, Jun 04, 2019 at 04:10:31PM +0200, Thierry Reding wrote:
>>>> On Tue, Jun 04, 2019 at 04:53:17PM +0300, Dmitry Osipenko wrote:
>>>>> 04.06.2019 14:20, Thierry Reding пишет:
>>>>>> On Thu, May 02, 2019 at 02:38:13AM +0300, Dmitry Osipenko wrote:
>>>>>>> The driver's compilation doesn't have any specific dependencies, hence
>>>>>>> the COMPILE_TEST option can be supported in Kconfig.
>>>>>>>
>>>>>>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>> ---
>>>>>>>  drivers/devfreq/Kconfig | 2 +-
>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>>>>>> index 56db9dc05edb..a6bba6e1e7d9 100644
>>>>>>> --- a/drivers/devfreq/Kconfig
>>>>>>> +++ b/drivers/devfreq/Kconfig
>>>>>>> @@ -93,7 +93,7 @@ config ARM_EXYNOS_BUS_DEVFREQ
>>>>>>>  
>>>>>>>  config ARM_TEGRA_DEVFREQ
>>>>>>>  	tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
>>>>>>> -	depends on ARCH_TEGRA
>>>>>>> +	depends on ARCH_TEGRA || COMPILE_TEST
>>>>>>>  	select PM_OPP
>>>>>>>  	help
>>>>>>>  	  This adds the DEVFREQ driver for the Tegra family of SoCs.
>>>>>>
>>>>>> You need to be careful with these. You're using I/O register accessors,
>>>>>> which are not supported on the UM architecture, for example.
>>>>>>
>>>>>> This may end up getting flagged during build testing.
>>>>>
>>>>> We have similar cases in other drivers and it doesn't cause any known
>>>>> problems because (I think) build-bots are aware of this detail. Hence
>>>>
>>>> I don't understand how the build-bots would be aware of this detail.
>>>> Unless you explicitly state what the dependencies are, how would the
>>>> build-bots know? Perhaps there's some logic built-in somewhere that I
>>>> don't know about?
>>>
>>> So looks like COMPILE_TEST has a !UML dependency, so this might just
>>> work.
>>>
>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>
>>
>> Thank you very much for the clarification! Certainly that would caused
>> problems already since there are such cases all over the kernel,
>> including Tegra drivers.
> 
> In the cases that I'm aware of we used to explicitly list all the
> dependencies that would've otherwise been pulled in by the ARCH_TEGRA
> dependency to make sure there were no issues.
> 
> Now that we've been discussing this I vaguely recall a discussion about
> the only real issue nowadays being HAS_IOMEM and since that's only
> missing on UML, that may have been the reason for why the !UML
> dependency was added.
> 
> Yes, looks like that was it:
> 
> 	commit bc083a64b6c035135c0f80718f9e9192cc0867c6
> 	Author: Richard Weinberger <richard@nod.at>
> 	Date:   Tue Aug 2 14:03:27 2016 -0700
> 
> 	    init/Kconfig: make COMPILE_TEST depend on !UML
> 
> 	    UML is a bit special since it does not have iomem nor dma.  That means a
> 	    lot of drivers will not build if they miss a dependency on HAS_IOMEM.
> 	    s390 used to have the same issues but since it gained PCI support UML is
> 	    the only stranger.
> 
> 	    We are tired of patching dozens of new drivers after every merge window
> 	    just to un-break allmod/yesconfig UML builds.  One could argue that a
> 	    decent driver has to know on what it depends and therefore a missing
> 	    HAS_IOMEM dependency is a clear driver bug.  But the dependency not
> 	    obvious and not everyone does UML builds with COMPILE_TEST enabled when
> 	    developing a device driver.
> 
> 	    A possible solution to make these builds succeed on UML would be
> 	    providing stub functions for ioremap() and friends which fail upon
> 	    runtime.  Another one is simply disabling COMPILE_TEST for UML.  Since
> 	    it is the least hassle and does not force use to fake iomem support
> 	    let's do the latter.
> 
> 	    Link: http://lkml.kernel.org/r/1466152995-28367-1-git-send-email-richard@nod.at
> 	    Signed-off-by: Richard Weinberger <richard@nod.at>
> 	    Acked-by: Arnd Bergmann <arnd@arndb.de>
> 	    Cc: Al Viro <viro@zeniv.linux.org.uk>
> 	    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> 	    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

That was a wise solution. Thank you very much again for the detailed
comment! Very appreciate that!

> Oh wow... almost three years now.

Please don't pay much attention to the time, it never stops. Live in the
moment :)

Patch
diff mbox series

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 56db9dc05edb..a6bba6e1e7d9 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -93,7 +93,7 @@  config ARM_EXYNOS_BUS_DEVFREQ
 
 config ARM_TEGRA_DEVFREQ
 	tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
-	depends on ARCH_TEGRA
+	depends on ARCH_TEGRA || COMPILE_TEST
 	select PM_OPP
 	help
 	  This adds the DEVFREQ driver for the Tegra family of SoCs.