2017-02-09 12:25 GMT-02:00 Pavel Machek <pavel@xxxxxx>:
> Hi!
>
>> Currently there is one CPU led trigger per cpu ('cpu0', 'cpu1', ...)
>>
>> This patch adds a new trigger, 'cpu', with brightness proportional to
>> the number of active CPUs.
>>
>> If multiple brightness levels aren't supported on the LED,
>> it effectively indicates if there is any CPU active.
>>
>> This is particularly useful on tiny linux boards with more CPU cores than LED pins.
>
>> +static struct led_trigger_cpu_all all_trig;
>> +
>> +static bool ledtrig_cpu_initialized;
>
> Umm. This variable looks quite like a lock, but without barriers etc. Are you sure that is ok?
>
>> @@ -47,26 +57,44 @@ static DEFINE_PER_CPU(struct led_trigger_cpu, cpu_trig);
>> void ledtrig_cpu(enum cpu_led_event ledevt)
>> {
>> struct led_trigger_cpu *trig = this_cpu_ptr(&cpu_trig);
>> + bool is_active = trig->cpu_is_active;
>> +
>> + /* Ignore if this is being called before initialization. */
>> + if (!ledtrig_cpu_initialized)
>> + return;
>>
>> /* Locate the correct CPU LED */
>> switch (ledevt) {
>> case CPU_LED_IDLE_END:
>> case CPU_LED_START:
>> /* Will turn the LED on, max brightness */
>> - led_trigger_event(trig->_trig, LED_FULL);
>> + is_active = true;
>> break;
>>
>> case CPU_LED_IDLE_START:
>> case CPU_LED_STOP:
>> case CPU_LED_HALTED:
>> /* Will turn the LED off */
>> - led_trigger_event(trig->_trig, LED_OFF);
>> + is_active = false;
>> break;
>>
>> default:
>> /* Will leave the LED as it is */
>> break;
>> }
>> +
>> + if (is_active != trig->cpu_is_active) {
>> + // Update trigger state
>
> C style comments, please.
>
>> + trig->cpu_is_active = is_active;
>> + atomic_add(is_active ? 1 : -1, &all_trig.num_active);
>
> In particular, are you sure that with all the activity during startup, it can't get
> desynchronized?
>
> Hmm. is_initialized should be atomic_t, or set_bit should be used. But it will
> probably not cause problems in practice.
>
> I guess the trigger actually will self-synchronize with the actual CPU state,
> eventually. So I guess it is ok, but...
Yes, things get messy during initialization, as ledtrig_cpu() gets called
before to ledtrig_cpu_init().
This code effectively ignores any event until the initialization has executed
(hence initialized flag) and catchs up later.
I just realized that I could instead remove the initialization of
trig->cpu_is_active and all_trig.num_active, and rely on the fact that
they are created with false/null. Would that be safe?
>
>> + led_trigger_event(all_trig._trig,
>> + LED_FULL * atomic_read(&all_trig.num_active) /
>> + num_present_cpus());
>
> DIV_ROUND_UP? :-)
Yes!
