[PATCH 01/17] leds: create a trigger for CPU activity

[Bryan Wu]

On Thu, Aug 4, 2011 at 12:12 AM, Linus Walleij <linus.walleij at linaro.org> wrote:
> On Wed, Aug 3, 2011 at 5:30 PM, Jamie Iles <jamie at jamieiles.com> wrote:
>>> > So shouldn't this be doing a
>>> > __get_cpu_var(ledtrig_cpu_triggers) and a kfree() on that (and setting
>>> > to NULL)?
>>>
>>> You could do that but why? There is no way the pointer value
>>> can make any harm. If activate() is called again it will be overwritten
>>> with the new pointer.
>>
>> Yes, but can't ledtrig_cpu() potentially still be called after
>> deactivation, then the:
>>
>>        if (!trigdata)
>>                return;
>>
>> would be incorrect?  My main point though was that it looks like
>> led->trigger_data is the processor id and kfree() is being called on
>> that.
>
> You're right, it just works fine because trigdata is always NULL...
>
>>> > Also, where does led->trigger_data get assigned with the cpu id?
>>>
>>> The activate() function is called on every core and if the CPU ID
>>> is not equal to the assigned CPU it just exits, here:
>>>
>>> +     int my_cpu = smp_processor_id();
>>> +     unsigned long target_cpu = (unsigned long) led->trigger_data;
>>> +
>>> +     if (target_cpu != my_cpu)
>>> +             return;
>>
>> I understand that bit, but I can't see anything that actually assigns
>> led->trigger_data in the first place.
>
> You're right, it's because nothing really does. And since
> kfree(NULL); is fine this works, and makes the above thing work
> fine as well.
>
> Since led->trigger_data is never used we shouldn't touch it in
> I believe, Bryan can you just take it out and replace with something
> like this (totally untested):
>
> static void ledtrig_cpu_deactivate_cpu(struct led_classdev *led)
> {
>       struct ledtrig_cpu_data *cpu_data = __get_cpu_var(ledtrig_cpu_triggers);
>       int my_cpu = smp_processor_id();
>       unsigned long target_cpu = (unsigned long) led->trigger_data;
>
>       if (target_cpu != my_cpu)
>               return;
>
>       kfree(cpu_data);
>       __get_cpu_var(ledtrig_cpu_triggers) = NULL;
> }
>
> static void ledtrig_cpu_activate(struct led_classdev *led)
> {
>       on_each_cpu(ledtrig_cpu_deactivate_cpu, led, 1);
> }
>
> Bryan can you test this change?
>

No problem, I've reworked this patchset and included this updates.
Please find patches here:

git://kernel.ubuntu.com/roc/linux-2.6 leds
gitweb: http://kernel.ubuntu.com/git?p=roc/linux-2.6/.git;a=shortlog;h=refs/heads/leds

Please help to test on your hardware.

Thanks,
-- 
Bryan Wu <bryan.wu at canonical.com>
Kernel Developer    +86.138-1617-6545 Mobile
Ubuntu Kernel Team
Canonical Ltd.      www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com