schedule_timeout sleeps too long after dividing CPU frequency

Commit Message

Mason May 20, 2015, 4:21 p.m. UTC

Hello Russell,

Thanks for your detailed answer.

On 18/05/2015 13:54, Russell King - ARM Linux wrote:

> For a SoC where WFI is not programmed to cause anything other than
> the architecture specified dormant behaviour, WFI will not cause the
> TWD to stop.

According to the hardware engineers, my SoC does not support any
low-power modes.

But I didn't see the "clean" way to make the kernel aware of this.
Is this an acceptable patch? (I have my doubts.)




Regards.

Comments

Arnd Bergmann May 20, 2015, 6:50 p.m. UTC | #1

On Wednesday 20 May 2015 18:21:45 Mason wrote:
> 
> On 18/05/2015 13:54, Russell King - ARM Linux wrote:
> 
> > For a SoC where WFI is not programmed to cause anything other than
> > the architecture specified dormant behaviour, WFI will not cause the
> > TWD to stop.
> 
> According to the hardware engineers, my SoC does not support any
> low-power modes.
> 
> But I didn't see the "clean" way to make the kernel aware of this.
> Is this an acceptable patch? (I have my doubts.)

No, this is clearly broken.

> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> index 6591e26..300f13a 100644
> --- a/arch/arm/kernel/smp_twd.c
> +++ b/arch/arm/kernel/smp_twd.c
> @@ -295,6 +295,10 @@ static void twd_timer_setup(void)
>         clk->name = "local_timer";
>         clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
>                         CLOCK_EVT_FEAT_C3STOP;
> +#ifdef CONFIG_TANGOX
> +       /*** Tango does not implement low power modes ***/
> +       clk->features &= ~CLOCK_EVT_FEAT_C3STOP;
> +#endif
>         clk->rating = 350;
>         clk->set_mode = twd_set_mode;
>         clk->set_next_event = twd_set_next_event;

This will disable the feature on all machines that are configured 
in the kernel. You have to make a run-time decision instead, e.g.
based on a boolean DT property of the twd node.

	Arnd

Mason May 20, 2015, 7:34 p.m. UTC | #2

On 20/05/2015 20:50, Arnd Bergmann wrote:

> On Wednesday 20 May 2015 18:21:45 Mason wrote:
>>
>> On 18/05/2015 13:54, Russell King - ARM Linux wrote:
>>
>>> For a SoC where WFI is not programmed to cause anything other than
>>> the architecture specified dormant behaviour, WFI will not cause the
>>> TWD to stop.
>>
>> According to the hardware engineers, my SoC does not support any
>> low-power modes.
>>
>> But I didn't see the "clean" way to make the kernel aware of this.
>> Is this an acceptable patch? (I have my doubts.)
> 
> No, this is clearly broken.

I don't see it. Could you be more explicit?

>> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
>> index 6591e26..300f13a 100644
>> --- a/arch/arm/kernel/smp_twd.c
>> +++ b/arch/arm/kernel/smp_twd.c
>> @@ -295,6 +295,10 @@ static void twd_timer_setup(void)
>>         clk->name = "local_timer";
>>         clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
>>                         CLOCK_EVT_FEAT_C3STOP;
>> +#ifdef CONFIG_TANGOX
>> +       /*** Tango does not implement low power modes ***/
>> +       clk->features &= ~CLOCK_EVT_FEAT_C3STOP;
>> +#endif
>>         clk->rating = 350;
>>         clk->set_mode = twd_set_mode;
>>         clk->set_next_event = twd_set_next_event;
> 
> This will disable the feature on all machines that are configured 
> in the kernel.

What do you mean, "disable the feature"?

My proposed patch doesn't change the default, which is to set
CLOCK_EVT_FEAT_C3STOP unconditionally for all machines.

And then only for some platforms (in this case only TANGOX)
CLOCK_EVT_FEAT_C3STOP is *removed* from the features list.

Regards.

Russell King - ARM Linux May 20, 2015, 8:14 p.m. UTC | #3

On Wed, May 20, 2015 at 09:34:16PM +0200, Mason wrote:
> On 20/05/2015 20:50, Arnd Bergmann wrote:
> >> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> >> index 6591e26..300f13a 100644
> >> --- a/arch/arm/kernel/smp_twd.c
> >> +++ b/arch/arm/kernel/smp_twd.c
> >> @@ -295,6 +295,10 @@ static void twd_timer_setup(void)
> >>         clk->name = "local_timer";
> >>         clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
> >>                         CLOCK_EVT_FEAT_C3STOP;
> >> +#ifdef CONFIG_TANGOX
> >> +       /*** Tango does not implement low power modes ***/
> >> +       clk->features &= ~CLOCK_EVT_FEAT_C3STOP;
> >> +#endif
> >>         clk->rating = 350;
> >>         clk->set_mode = twd_set_mode;
> >>         clk->set_next_event = twd_set_next_event;
> > 
> > This will disable the feature on all machines that are configured 
> > in the kernel.
> 
> What do you mean, "disable the feature"?
> 
> My proposed patch doesn't change the default, which is to set
> CLOCK_EVT_FEAT_C3STOP unconditionally for all machines.
> 
> And then only for some platforms (in this case only TANGOX)
> CLOCK_EVT_FEAT_C3STOP is *removed* from the features list.

So we build a kernel containing both tangox and OMAP together, and
OMAP starts misbehaving because CLOCK_EVT_FEAT_C3STOP is no longer
set.  That's a regression caused by your change, even if you didn't
intend for anyone else to enable your option.

Mason May 20, 2015, 8:41 p.m. UTC | #4

On 20/05/2015 22:14, Russell King - ARM Linux wrote:
> On Wed, May 20, 2015 at 09:34:16PM +0200, Mason wrote:
>> On 20/05/2015 20:50, Arnd Bergmann wrote:
>>>> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
>>>> index 6591e26..300f13a 100644
>>>> --- a/arch/arm/kernel/smp_twd.c
>>>> +++ b/arch/arm/kernel/smp_twd.c
>>>> @@ -295,6 +295,10 @@ static void twd_timer_setup(void)
>>>>         clk->name = "local_timer";
>>>>         clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
>>>>                         CLOCK_EVT_FEAT_C3STOP;
>>>> +#ifdef CONFIG_TANGOX
>>>> +       /*** Tango does not implement low power modes ***/
>>>> +       clk->features &= ~CLOCK_EVT_FEAT_C3STOP;
>>>> +#endif
>>>>         clk->rating = 350;
>>>>         clk->set_mode = twd_set_mode;
>>>>         clk->set_next_event = twd_set_next_event;
>>>
>>> This will disable the feature on all machines that are configured 
>>> in the kernel.
>>
>> What do you mean, "disable the feature"?
>>
>> My proposed patch doesn't change the default, which is to set
>> CLOCK_EVT_FEAT_C3STOP unconditionally for all machines.
>>
>> And then only for some platforms (in this case only TANGOX)
>> CLOCK_EVT_FEAT_C3STOP is *removed* from the features list.
> 
> So we build a kernel containing both tangox and OMAP together, and
> OMAP starts misbehaving because CLOCK_EVT_FEAT_C3STOP is no longer
> set.  That's a regression caused by your change, even if you didn't
> intend for anyone else to enable your option.

Oh... I didn't even think it made sense (and was supported)
to select more than one machine...

Is this related to CONFIG_ARCH_MULTIPLATFORM?

Regards.

Arnd Bergmann May 20, 2015, 8:52 p.m. UTC | #5

On Wednesday 20 May 2015 22:41:33 Mason wrote:
> 
> Oh... I didn't even think it made sense (and was supported)
> to select more than one machine...
> 
> Is this related to CONFIG_ARCH_MULTIPLATFORM?

Yes. In the old days, each platform had its own entry in the top-level
choice statement, which meant they were mutually exclusive. Most of
them are converted now and can be put into a single kernel with
CONFIG_ARCH_MULTIPLATFORM, and we stopped accepting new ones that
don't do this a few years ago.

I have a patch series that converts all remaining ARMv6 and ARMv7
platforms that don't are still standalone to use CONFIG_ARCH_MULTIPLATFORM
as well.

	Arnd

Patch

diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index 6591e26..300f13a 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -295,6 +295,10 @@  static void twd_timer_setup(void)
        clk->name = "local_timer";
        clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
                        CLOCK_EVT_FEAT_C3STOP;
+#ifdef CONFIG_TANGOX
+       /*** Tango does not implement low power modes ***/
+       clk->features &= ~CLOCK_EVT_FEAT_C3STOP;
+#endif
        clk->rating = 350;
        clk->set_mode = twd_set_mode;
        clk->set_next_event = twd_set_next_event;