[1/5] cpuidle: Remove pointless stub

Message ID 20201015144431.9979-1-daniel.lezcano@linaro.org
State New
Headers show
Series
  • [1/5] cpuidle: Remove pointless stub
Related show

Commit Message

Daniel Lezcano Oct. 15, 2020, 2:44 p.m.
The cpuidle.h header is declaring functions with an empty stub when
cpuidle is not enabled. However these functions are only called from
the governors which depends on cpuidle. In other words, when the
function is called it is when cpuidle is enabled, there is no
situation when it is called with cpuidle disabled.

Remove the pointless stub.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 include/linux/cpuidle.h | 5 -----
 1 file changed, 5 deletions(-)

Comments

Rafael J. Wysocki Oct. 16, 2020, 3:24 p.m. | #1
On Thu, Oct 15, 2020 at 4:44 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>

> The cpuidle.h header is declaring functions with an empty stub when

> cpuidle is not enabled. However these functions are only called from

> the governors which depends on cpuidle. In other words, when the

> function is called it is when cpuidle is enabled, there is no

> situation when it is called with cpuidle disabled.

>

> Remove the pointless stub.

>

> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> ---

>  include/linux/cpuidle.h | 5 -----

>  1 file changed, 5 deletions(-)

>

> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h

> index 6175c77bf25e..74fdcc6106b1 100644

> --- a/include/linux/cpuidle.h

> +++ b/include/linux/cpuidle.h

> @@ -270,13 +270,8 @@ struct cpuidle_governor {

>         void (*reflect)         (struct cpuidle_device *dev, int index);

>  };

>

> -#ifdef CONFIG_CPU_IDLE

>  extern int cpuidle_register_governor(struct cpuidle_governor *gov);

>  extern s64 cpuidle_governor_latency_req(unsigned int cpu);

> -#else

> -static inline int cpuidle_register_governor(struct cpuidle_governor *gov)

> -{return 0;}

> -#endif

>

>  #define __CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter,                  \

>                                 idx,                                    \

> --


Applied (this patch alone) as 5.10-rc material with some minor edits
in the changelog, thanks!
Daniel Lezcano Oct. 16, 2020, 8:31 p.m. | #2
Hi Rafael,

On 16/10/2020 17:24, Rafael J. Wysocki wrote:
> On Thu, Oct 15, 2020 at 4:44 PM Daniel Lezcano

> <daniel.lezcano@linaro.org> wrote:

>>

>> The cpuidle.h header is declaring functions with an empty stub when

>> cpuidle is not enabled. However these functions are only called from

>> the governors which depends on cpuidle. In other words, when the

>> function is called it is when cpuidle is enabled, there is no

>> situation when it is called with cpuidle disabled.

>>

>> Remove the pointless stub.

>>

>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

>> ---


[ ... ]

> Applied (this patch alone) as 5.10-rc material with some minor edits

> in the changelog, thanks!


Does it mean you disagree the other patches? Or are you waiting for more
comments?


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Rafael J. Wysocki Nov. 5, 2020, 2:14 p.m. | #3
On Fri, Oct 16, 2020 at 10:31 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
>
> Hi Rafael,
>
> On 16/10/2020 17:24, Rafael J. Wysocki wrote:
> > On Thu, Oct 15, 2020 at 4:44 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> The cpuidle.h header is declaring functions with an empty stub when
> >> cpuidle is not enabled. However these functions are only called from
> >> the governors which depends on cpuidle. In other words, when the
> >> function is called it is when cpuidle is enabled, there is no
> >> situation when it is called with cpuidle disabled.
> >>
> >> Remove the pointless stub.
> >>
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> ---
>
> [ ... ]
>
> > Applied (this patch alone) as 5.10-rc material with some minor edits
> > in the changelog, thanks!
>
> Does it mean you disagree the other patches? Or are you waiting for more
> comments?

Actually, both.

My primary concern regarding the modularization of cpuidle governors
is that they really belong to the core kernel.  They access the
internals thereof and the behavior of the idle loop in general depends
on what they do.

OTOH IMV a potential benefit resulting from allowing them to be
modular is very small, at least from the mainline perspective.

IOW this case is very similar to the modularization of the schedutil
cpufreq governor which we decided not to do for similar reasons.
Daniel Lezcano Nov. 5, 2020, 3:31 p.m. | #4
Hi Rafael,

On 05/11/2020 15:14, Rafael J. Wysocki wrote:

[ ... ]

>> [ ... ]
>>
>>> Applied (this patch alone) as 5.10-rc material with some minor edits
>>> in the changelog, thanks!
>>
>> Does it mean you disagree the other patches? Or are you waiting for more
>> comments?
> 
> Actually, both.
> 
> My primary concern regarding the modularization of cpuidle governors
> is that they really belong to the core kernel.  They access the
> internals thereof and the behavior of the idle loop in general depends
> on what they do.
> 
> OTOH IMV a potential benefit resulting from allowing them to be
> modular is very small, at least from the mainline perspective.
> 
> IOW this case is very similar to the modularization of the schedutil
> cpufreq governor which we decided not to do for similar reasons.

Yes, I recall this discussion.

The purpose is to be more consistent with the governors in the other
frameworks which can be module. But as you mentioned it, the benefit is
small, so I'm fine to drop the series.

Thanks

  -- Daniel

Patch

diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 6175c77bf25e..74fdcc6106b1 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -270,13 +270,8 @@  struct cpuidle_governor {
 	void (*reflect)		(struct cpuidle_device *dev, int index);
 };
 
-#ifdef CONFIG_CPU_IDLE
 extern int cpuidle_register_governor(struct cpuidle_governor *gov);
 extern s64 cpuidle_governor_latency_req(unsigned int cpu);
-#else
-static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
-{return 0;}
-#endif
 
 #define __CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter,			\
 				idx,					\