[patch V2 43/67] leds/trigger/cpu: Convert to hotplug state machine

[patch V2 43/67] leds/trigger/cpu: Convert to hotplug state machine

[Anna-Maria Gleixner]

[-- Attachment #1: 0042-leds-trigger-cpu-Convert-to-hotplug-state-machine.patch --]
[-- Type: text/plain, Size: 2713 bytes --]

From: Richard Cochran <rcochran@linutronix.de>

This is a straightforward conversion. We place this callback last
in the list so that the LED illuminates only after a successful
bring up sequence.

Signed-off-by: Richard Cochran <rcochran@linutronix.de>
Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-leds@vger.kernel.org
Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
---
 drivers/leds/trigger/ledtrig-cpu.c | 32 +++++++++++++++-----------------
 include/linux/cpuhotplug.h         |  1 +
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-cpu.c b/drivers/leds/trigger/ledtrig-cpu.c
index 938467f..4a6a182 100644
--- a/drivers/leds/trigger/ledtrig-cpu.c
+++ b/drivers/leds/trigger/ledtrig-cpu.c
@@ -92,25 +92,17 @@ static struct syscore_ops ledtrig_cpu_syscore_ops = {
 	.resume		= ledtrig_cpu_syscore_resume,
 };
 
-static int ledtrig_cpu_notify(struct notifier_block *self,
-					   unsigned long action, void *hcpu)
+static int ledtrig_starting_cpu(unsigned int cpu)
 {
-	switch (action & ~CPU_TASKS_FROZEN) {
-	case CPU_STARTING:
-		ledtrig_cpu(CPU_LED_START);
-		break;
-	case CPU_DYING:
-		ledtrig_cpu(CPU_LED_STOP);
-		break;
-	}
-
-	return NOTIFY_OK;
+	ledtrig_cpu(CPU_LED_START);
+	return 0;
 }
 
-
-static struct notifier_block ledtrig_cpu_nb = {
-	.notifier_call = ledtrig_cpu_notify,
-};
+static int ledtrig_dying_cpu(unsigned int cpu)
+{
+	ledtrig_cpu(CPU_LED_STOP);
+	return 0;
+}
 
 static int __init ledtrig_cpu_init(void)
 {
@@ -133,7 +125,13 @@ static int __init ledtrig_cpu_init(void)
 	}
 
 	register_syscore_ops(&ledtrig_cpu_syscore_ops);
-	register_cpu_notifier(&ledtrig_cpu_nb);
+
+	/*
+	 * FIXME: Why needs this to happen in the interrupt disabled
+	 * low level bringup phase of a cpu?
+	 */
+	cpuhp_setup_state(CPUHP_AP_LEDTRIG_STARTING, "AP_LEDTRIG_STARTING",
+			  ledtrig_starting_cpu, ledtrig_dying_cpu);
 
 	pr_info("ledtrig-cpu: registered to indicate activity on CPUs\n");
 
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index c052b04..ef7bfa6 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -43,6 +43,7 @@ enum cpuhp_state {
 	CPUHP_AP_QCOM_TIMER_STARTING,
 	CPUHP_AP_MIPS_GIC_TIMER_STARTING,
 	CPUHP_AP_KVM_STARTING,
+	CPUHP_AP_LEDTRIG_STARTING,
 	CPUHP_AP_NOTIFY_STARTING,
 	CPUHP_AP_ONLINE,
 	CPUHP_TEARDOWN_CPU,
-- 
2.8.1

Re: [patch V2 43/67] leds/trigger/cpu: Convert to hotplug state machine

[Jacek Anaszewski]

Hi Anna,

On 07/13/2016 07:16 PM, Anna-Maria Gleixner wrote:

 > -------- Original Message --------
 > Subject: [patch V2 43/67] leds/trigger/cpu: Convert to hotplug state 
machine
 > Date: Wed, 13 Jul 2016 17:16:45 +0000
 > From: Anna-Maria Gleixner <anna-maria@linutronix.de>
 > To: LKML <linux-kernel@vger.kernel.org>
 > CC: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar 
<mingo@kernel.org>, Thomas Gleixner <tglx@linutronix.de>, 
rt@linutronix.de, Richard Cochran <rcochran@linutronix.de>, Sebastian 
Andrzej Siewior <bigeasy@linutronix.de>, Jacek Anaszewski 
<j.anaszewski@samsung.com>, Linus Torvalds 
<torvalds@linux-foundation.org>, Linus Walleij 
<linus.walleij@linaro.org>, Paul Gortmaker 
<paul.gortmaker@windriver.com>, Richard Purdie <rpurdie@rpsys.net>, 
linux-leds@vger.kernel.org, Anna-Maria Gleixner <anna-maria@linutronix.de>
 >
 > From: Richard Cochran <rcochran@linutronix.de>
 >
 > This is a straightforward conversion. We place this callback last
 > in the list so that the LED illuminates only after a successful
 > bring up sequence.
 >
 > Signed-off-by: Richard Cochran <rcochran@linutronix.de>
 > Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
 > Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
 > Cc: Linus Torvalds <torvalds@linux-foundation.org>
 > Cc: Linus Walleij <linus.walleij@linaro.org>
 > Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
 > Cc: Peter Zijlstra <peterz@infradead.org>
 > Cc: Richard Purdie <rpurdie@rpsys.net>
 > Cc: Thomas Gleixner <tglx@linutronix.de>
 > Cc: linux-leds@vger.kernel.org
 > Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
 > ---
 >  drivers/leds/trigger/ledtrig-cpu.c | 32 +++++++++++++++-----------------
 >  include/linux/cpuhotplug.h         |  1 +
 >  2 files changed, 16 insertions(+), 17 deletions(-)
 >
 > diff --git a/drivers/leds/trigger/ledtrig-cpu.c 
b/drivers/leds/trigger/ledtrig-cpu.c
 > index 938467f..4a6a182 100644
 > --- a/drivers/leds/trigger/ledtrig-cpu.c
 > +++ b/drivers/leds/trigger/ledtrig-cpu.c
 > @@ -92,25 +92,17 @@ static struct syscore_ops ledtrig_cpu_syscore_ops = {
 >       .resume         = ledtrig_cpu_syscore_resume,
 >  };
 >
 > -static int ledtrig_cpu_notify(struct notifier_block *self,
 > -                                        unsigned long action, void 
*hcpu)
 > +static int ledtrig_starting_cpu(unsigned int cpu)
 >  {
 > -     switch (action & ~CPU_TASKS_FROZEN) {
 > -     case CPU_STARTING:
 > -             ledtrig_cpu(CPU_LED_START);
 > -             break;
 > -             break;
 > -     }
 > -
 > -     return NOTIFY_OK;
 > +     ledtrig_cpu(CPU_LED_START);
 > +     return 0;
 >  }
 >
 > -
 > -static struct notifier_block ledtrig_cpu_nb = {
 > -     .notifier_call = ledtrig_cpu_notify,
 > -};
 > +static int ledtrig_dying_cpu(unsigned int cpu)
 > +{
 > +     ledtrig_cpu(CPU_LED_STOP);
 > +     return 0;
 > +}
 >
 >  static int __init ledtrig_cpu_init(void)
 >  {
 > @@ -133,7 +125,13 @@ static int __init ledtrig_cpu_init(void)
 >       }
 >
 >       register_syscore_ops(&ledtrig_cpu_syscore_ops);
 > -     register_cpu_notifier(&ledtrig_cpu_nb);
 > +
 > +     /*
 > +      * FIXME: Why needs this to happen in the interrupt disabled
 > +      * low level bringup phase of a cpu?
 > +      */

Why wasn't it possible to clarify the issue before the
submission?

 > +     cpuhp_setup_state(CPUHP_AP_LEDTRIG_STARTING, "AP_LEDTRIG_STARTING",
 > +                       ledtrig_starting_cpu, ledtrig_dying_cpu);
 >
 >       pr_info("ledtrig-cpu: registered to indicate activity on CPUs\n");
 >
 > diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
 > index c052b04..ef7bfa6 100644
 > --- a/include/linux/cpuhotplug.h
 > +++ b/include/linux/cpuhotplug.h
 > @@ -43,6 +43,7 @@ enum cpuhp_state {
 >       CPUHP_AP_QCOM_TIMER_STARTING,
 >       CPUHP_AP_MIPS_GIC_TIMER_STARTING,
 >       CPUHP_AP_KVM_STARTING,
 > +     CPUHP_AP_LEDTRIG_STARTING,
 >       CPUHP_AP_NOTIFY_STARTING,
 >       CPUHP_AP_ONLINE,
 >       CPUHP_TEARDOWN_CPU,
 > --


-- 
Best regards,
Jacek Anaszewski

Re: [patch V2 43/67] leds/trigger/cpu: Convert to hotplug state machine

[Ingo Molnar]

* Jacek Anaszewski <j.anaszewski@samsung.com> wrote:

> > @@ -133,7 +125,13 @@ static int __init ledtrig_cpu_init(void)
> >       }
> >
> >       register_syscore_ops(&ledtrig_cpu_syscore_ops);
> > -     register_cpu_notifier(&ledtrig_cpu_nb);
> > +
> > +     /*
> > +      * FIXME: Why needs this to happen in the interrupt disabled
> > +      * low level bringup phase of a cpu?
> > +      */
> 
> Why wasn't it possible to clarify the issue before the
> submission?

Nothing has been applied yet (this submission is part of the clarification 
process), are you fine with this conversion as-is, with a separate patch doing any 
followup fixes - or would you prefer another approach?

This CPU hotplug series tries to do straightforward conversions to the CPU hotplug 
state machine without changing behavior - while pointing out any problems that 
were found along the way.

I agree that the addition of this FIXME should probably have been pointed out in 
the changelog as well, but this patch was submitted once already with no feedback 
received - and that's the historic pattern with such types of series: only a low 
percentage of all driver maintainers replies so we have to be proactive to a 
certain degree to have a chance of improving core kernel infrastructure. It's 
better to add FIXMEs with open questions that to introduce subtle problems.

Thanks,

	Ingo

Re: [patch V2 43/67] leds/trigger/cpu: Convert to hotplug state machine

[Jacek Anaszewski]

On 07/14/2016 09:47 AM, Ingo Molnar wrote:
>
> * Jacek Anaszewski <j.anaszewski@samsung.com> wrote:
>
>>> @@ -133,7 +125,13 @@ static int __init ledtrig_cpu_init(void)
>>>        }
>>>
>>>        register_syscore_ops(&ledtrig_cpu_syscore_ops);
>>> -     register_cpu_notifier(&ledtrig_cpu_nb);
>>> +
>>> +     /*
>>> +      * FIXME: Why needs this to happen in the interrupt disabled
>>> +      * low level bringup phase of a cpu?
>>> +      */
>>
>> Why wasn't it possible to clarify the issue before the
>> submission?
>
> Nothing has been applied yet (this submission is part of the clarification
> process), are you fine with this conversion as-is, with a separate patch doing any
> followup fixes - or would you prefer another approach?
>
> This CPU hotplug series tries to do straightforward conversions to the CPU hotplug
> state machine without changing behavior - while pointing out any problems that
> were found along the way.
>
> I agree that the addition of this FIXME should probably have been pointed out in
> the changelog as well, but this patch was submitted once already with no feedback
> received - and that's the historic pattern with such types of series: only a low
> percentage of all driver maintainers replies so we have to be proactive to a
> certain degree to have a chance of improving core kernel infrastructure. It's
> better to add FIXMEs with open questions that to introduce subtle problems.

Thanks for the explanation. I'm OK with that approach.

Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>

-- 
Best regards,
Jacek Anaszewski

Re: [patch V2 43/67] leds/trigger/cpu: Convert to hotplug state machine

[Peter Zijlstra]

On Thu, Jul 14, 2016 at 10:10:55AM +0200, Jacek Anaszewski wrote:
> On 07/14/2016 09:47 AM, Ingo Molnar wrote:
> >
> >* Jacek Anaszewski <j.anaszewski@samsung.com> wrote:
> >
> >>>@@ -133,7 +125,13 @@ static int __init ledtrig_cpu_init(void)
> >>>       }
> >>>
> >>>       register_syscore_ops(&ledtrig_cpu_syscore_ops);
> >>>-     register_cpu_notifier(&ledtrig_cpu_nb);
> >>>+
> >>>+     /*
> >>>+      * FIXME: Why needs this to happen in the interrupt disabled
> >>>+      * low level bringup phase of a cpu?
> >>>+      */

> Thanks for the explanation. I'm OK with that approach.

Since we have your attention, could you perhaps attempt to answer the
question so we can fix the fixme?

Re: [patch V2 43/67] leds/trigger/cpu: Convert to hotplug state machine

[Jacek Anaszewski]

On 07/14/2016 11:41 AM, Peter Zijlstra wrote:
> On Thu, Jul 14, 2016 at 10:10:55AM +0200, Jacek Anaszewski wrote:
>> On 07/14/2016 09:47 AM, Ingo Molnar wrote:
>>>
>>> * Jacek Anaszewski <j.anaszewski@samsung.com> wrote:
>>>
>>>>> @@ -133,7 +125,13 @@ static int __init ledtrig_cpu_init(void)
>>>>>        }
>>>>>
>>>>>        register_syscore_ops(&ledtrig_cpu_syscore_ops);
>>>>> -     register_cpu_notifier(&ledtrig_cpu_nb);
>>>>> +
>>>>> +     /*
>>>>> +      * FIXME: Why needs this to happen in the interrupt disabled
>>>>> +      * low level bringup phase of a cpu?
>>>>> +      */
>
>> Thanks for the explanation. I'm OK with that approach.
>
> Since we have your attention, could you perhaps attempt to answer the
> question so we can fix the fixme?
>

This is the way how all led triggers are being registered.

Once a trigger module is loaded it needs to be registered in
the LED Trigger core.

-- 
Best regards,
Jacek Anaszewski

Re: [patch V2 43/67] leds/trigger/cpu: Convert to hotplug state machine

[Thomas Gleixner]

On Thu, 14 Jul 2016, Jacek Anaszewski wrote:
> On 07/14/2016 11:41 AM, Peter Zijlstra wrote:
> > On Thu, Jul 14, 2016 at 10:10:55AM +0200, Jacek Anaszewski wrote:
> > > On 07/14/2016 09:47 AM, Ingo Molnar wrote:
> > > > 
> > > > * Jacek Anaszewski <j.anaszewski@samsung.com> wrote:
> > > > 
> > > > > > @@ -133,7 +125,13 @@ static int __init ledtrig_cpu_init(void)
> > > > > >        }
> > > > > > 
> > > > > >        register_syscore_ops(&ledtrig_cpu_syscore_ops);
> > > > > > -     register_cpu_notifier(&ledtrig_cpu_nb);
> > > > > > +
> > > > > > +     /*
> > > > > > +      * FIXME: Why needs this to happen in the interrupt disabled
> > > > > > +      * low level bringup phase of a cpu?
> > > > > > +      */
> > 
> > > Thanks for the explanation. I'm OK with that approach.
> > 
> > Since we have your attention, could you perhaps attempt to answer the
> > question so we can fix the fixme?
> > 
> 
> This is the way how all led triggers are being registered.
> 
> Once a trigger module is loaded it needs to be registered in
> the LED Trigger core.

That does not explain WHY this needs to happen in the low level bringup phase
of the CPU with interrupts disabled and can't be done from the normal ONLINE
callbacks in thread context.

Thanks,

	tglx

Re: [patch V2 43/67] leds/trigger/cpu: Convert to hotplug state machine

[Jacek Anaszewski]

On 07/14/2016 01:33 PM, Thomas Gleixner wrote:
> On Thu, 14 Jul 2016, Jacek Anaszewski wrote:
>> On 07/14/2016 11:41 AM, Peter Zijlstra wrote:
>>> On Thu, Jul 14, 2016 at 10:10:55AM +0200, Jacek Anaszewski wrote:
>>>> On 07/14/2016 09:47 AM, Ingo Molnar wrote:
>>>>>
>>>>> * Jacek Anaszewski <j.anaszewski@samsung.com> wrote:
>>>>>
>>>>>>> @@ -133,7 +125,13 @@ static int __init ledtrig_cpu_init(void)
>>>>>>>         }
>>>>>>>
>>>>>>>         register_syscore_ops(&ledtrig_cpu_syscore_ops);
>>>>>>> -     register_cpu_notifier(&ledtrig_cpu_nb);
>>>>>>> +
>>>>>>> +     /*
>>>>>>> +      * FIXME: Why needs this to happen in the interrupt disabled
>>>>>>> +      * low level bringup phase of a cpu?
>>>>>>> +      */
>>>
>>>> Thanks for the explanation. I'm OK with that approach.
>>>
>>> Since we have your attention, could you perhaps attempt to answer the
>>> question so we can fix the fixme?
>>>
>>
>> This is the way how all led triggers are being registered.
>>
>> Once a trigger module is loaded it needs to be registered in
>> the LED Trigger core.
>
> That does not explain WHY this needs to happen in the low level bringup phase
> of the CPU with interrupts disabled and can't be done from the normal ONLINE
> callbacks in thread context.

It was before my time in kernel, so I can only suppose that it was
the easiest way. Does it introduce some problems?

-- 
Best regards,
Jacek Anaszewski

Re: [patch V2 43/67] leds/trigger/cpu: Convert to hotplug state machine

[Sebastian Andrzej Siewior]

* Jacek Anaszewski | 2016-07-14 13:55:26 [+0200]:

>On 07/14/2016 01:33 PM, Thomas Gleixner wrote:
>>That does not explain WHY this needs to happen in the low level bringup phase
>>of the CPU with interrupts disabled and can't be done from the normal ONLINE
>>callbacks in thread context.
>
>It was before my time in kernel, so I can only suppose that it was
>the easiest way. Does it introduce some problems?

It was introduced by Pawel Moll in fba14ae8e924 ("ledtrig-cpu: Handle
CPU hot(un)plugging"). It does not introduce any problems but those
bring up / bring down levels are usually used by the core code. This
does not look like it needs to be done _that_ early or late during the
removal / addition of a CPU to the system.

Which means this looks like it could be moved to CPU_ONLINE /
CPU_DOWN_PREPARE which in turns means we could use dynamic IDs instead
of those hardcoded ones since the LED subsystem does not depend on other
components.

Sebastian

Re: [patch V2 43/67] leds/trigger/cpu: Convert to hotplug state machine

[Jacek Anaszewski]

Hi Sebastian,

On 07/15/2016 04:10 PM, Sebastian Andrzej Siewior wrote:
> * Jacek Anaszewski | 2016-07-14 13:55:26 [+0200]:
>
>> On 07/14/2016 01:33 PM, Thomas Gleixner wrote:
>>> That does not explain WHY this needs to happen in the low level bringup phase
>>> of the CPU with interrupts disabled and can't be done from the normal ONLINE
>>> callbacks in thread context.
>>
>> It was before my time in kernel, so I can only suppose that it was
>> the easiest way. Does it introduce some problems?
>
> It was introduced by Pawel Moll in fba14ae8e924 ("ledtrig-cpu: Handle
> CPU hot(un)plugging"). It does not introduce any problems but those
> bring up / bring down levels are usually used by the core code. This
> does not look like it needs to be done _that_ early or late during the
> removal / addition of a CPU to the system.
>
> Which means this looks like it could be moved to CPU_ONLINE /
> CPU_DOWN_PREPARE which in turns means we could use dynamic IDs instead
> of those hardcoded ones since the LED subsystem does not depend on other
> components.

 From the LED subsystem perspective I don't see any particular reason
for which it couldn't be accomplished from the ONLINE callbacks.

-- 
Best regards,
Jacek Anaszewski