Re: [PATCH] led/ledtrig-heartbeat: Convert timers to use

[Kees Cook]

On Fri, Oct 6, 2017 at 1:18 PM, Jacek Anaszewski
 wrote:
> Hi Kees,
>
> Thanks for the patch.
>
> On 10/05/2017 02:49 AM, Kees Cook wrote:
>> Instead of using .data directly, convert to from_timer. Since the
>> trigger_data is allocated separately, the led_cdev must be explicitly
>> tracked for the callback.
>>
>> Cc: Richard Purdie 
>> Cc: Jacek Anaszewski 
>> Cc: Pavel Machek 
>> Cc: Zhang Bo 
>> Cc: Linus Walleij 
>> Cc: Ingo Molnar 
>> Cc: linux-l...@vger.kernel.org
>> Cc: Thomas Gleixner 
>> Signed-off-by: Kees Cook 
>> ---
>> This requires commit 686fef928bba ("timer: Prepare to change timer
>> callback argument type") in v4.14-rc3, but should be otherwise
>> stand-alone.
>> ---
>>  drivers/leds/trigger/ledtrig-heartbeat.c | 16 ++--
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c 
>> b/drivers/leds/trigger/ledtrig-heartbeat.c
>> index e95ea65380c8..f0896de410b8 100644
>> --- a/drivers/leds/trigger/ledtrig-heartbeat.c
>> +++ b/drivers/leds/trigger/ledtrig-heartbeat.c
>> @@ -25,19 +25,23 @@
>>  static int panic_heartbeats;
>>
>>  struct heartbeat_trig_data {
>> + struct led_classdev *led_cdev;
>>   unsigned int phase;
>>   unsigned int period;
>>   struct timer_list timer;
>>   unsigned int invert;
>>  };
>>
>> -static void led_heartbeat_function(unsigned long data)
>> +static void led_heartbeat_function(struct timer_list *t)
>>  {
>> - struct led_classdev *led_cdev = (struct led_classdev *) data;
>> - struct heartbeat_trig_data *heartbeat_data = led_cdev->trigger_data;
>> + struct heartbeat_trig_data *heartbeat_data =
>> + from_timer(heartbeat_data, t, timer);
>> + struct led_classdev *led_cdev;
>>   unsigned long brightness = LED_OFF;
>>   unsigned long delay = 0;
>>
>> + led_cdev = heartbeat_data->led_cdev;
>> +
>>   if (unlikely(panic_heartbeats)) {
>>   led_set_brightness_nosleep(led_cdev, LED_OFF);
>>   return;
>> @@ -127,18 +131,18 @@ static void heartbeat_trig_activate(struct 
>> led_classdev *led_cdev)
>>   return;
>>
>>   led_cdev->trigger_data = heartbeat_data;
>> + heartbeat_data->led_cdev = led_cdev;
>>   rc = device_create_file(led_cdev->dev, _attr_invert);
>>   if (rc) {
>>   kfree(led_cdev->trigger_data);
>>   return;
>>   }
>>
>> - setup_timer(_data->timer,
>> - led_heartbeat_function, (unsigned long) led_cdev);
>> + timer_setup(_data->timer, led_heartbeat_function, 0);
>>   heartbeat_data->phase = 0;
>>   if (!led_cdev->blink_brightness)
>>   led_cdev->blink_brightness = led_cdev->max_brightness;
>> - led_heartbeat_function(heartbeat_data->timer.data);
>> + led_heartbeat_function(_data->timer);
>>   set_bit(LED_BLINK_SW, _cdev->work_flags);
>>   led_cdev->activated = true;
>>  }
>>
>
> It seems that the patch title is somewhat incomplete, so I amended
> it as follows:
>
> s/to use/to use timer_setup()/

Yeah, thanks for fixing that. This got pointed out elsewhere and I've
since hunted down and fixed my scripts to not accidentally truncate
subjects. *sigh* :)

> and applied to the for-next branch of linux-leds.git after rebasing
> it onto 4.14-rc3.

Thanks!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] led/ledtrig-heartbeat: Convert timers to use

[Kees Cook]

On Fri, Oct 6, 2017 at 1:18 PM, Jacek Anaszewski
 wrote:
> Hi Kees,
>
> Thanks for the patch.
>
> On 10/05/2017 02:49 AM, Kees Cook wrote:
>> Instead of using .data directly, convert to from_timer. Since the
>> trigger_data is allocated separately, the led_cdev must be explicitly
>> tracked for the callback.
>>
>> Cc: Richard Purdie 
>> Cc: Jacek Anaszewski 
>> Cc: Pavel Machek 
>> Cc: Zhang Bo 
>> Cc: Linus Walleij 
>> Cc: Ingo Molnar 
>> Cc: linux-l...@vger.kernel.org
>> Cc: Thomas Gleixner 
>> Signed-off-by: Kees Cook 
>> ---
>> This requires commit 686fef928bba ("timer: Prepare to change timer
>> callback argument type") in v4.14-rc3, but should be otherwise
>> stand-alone.
>> ---
>>  drivers/leds/trigger/ledtrig-heartbeat.c | 16 ++--
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c 
>> b/drivers/leds/trigger/ledtrig-heartbeat.c
>> index e95ea65380c8..f0896de410b8 100644
>> --- a/drivers/leds/trigger/ledtrig-heartbeat.c
>> +++ b/drivers/leds/trigger/ledtrig-heartbeat.c
>> @@ -25,19 +25,23 @@
>>  static int panic_heartbeats;
>>
>>  struct heartbeat_trig_data {
>> + struct led_classdev *led_cdev;
>>   unsigned int phase;
>>   unsigned int period;
>>   struct timer_list timer;
>>   unsigned int invert;
>>  };
>>
>> -static void led_heartbeat_function(unsigned long data)
>> +static void led_heartbeat_function(struct timer_list *t)
>>  {
>> - struct led_classdev *led_cdev = (struct led_classdev *) data;
>> - struct heartbeat_trig_data *heartbeat_data = led_cdev->trigger_data;
>> + struct heartbeat_trig_data *heartbeat_data =
>> + from_timer(heartbeat_data, t, timer);
>> + struct led_classdev *led_cdev;
>>   unsigned long brightness = LED_OFF;
>>   unsigned long delay = 0;
>>
>> + led_cdev = heartbeat_data->led_cdev;
>> +
>>   if (unlikely(panic_heartbeats)) {
>>   led_set_brightness_nosleep(led_cdev, LED_OFF);
>>   return;
>> @@ -127,18 +131,18 @@ static void heartbeat_trig_activate(struct 
>> led_classdev *led_cdev)
>>   return;
>>
>>   led_cdev->trigger_data = heartbeat_data;
>> + heartbeat_data->led_cdev = led_cdev;
>>   rc = device_create_file(led_cdev->dev, _attr_invert);
>>   if (rc) {
>>   kfree(led_cdev->trigger_data);
>>   return;
>>   }
>>
>> - setup_timer(_data->timer,
>> - led_heartbeat_function, (unsigned long) led_cdev);
>> + timer_setup(_data->timer, led_heartbeat_function, 0);
>>   heartbeat_data->phase = 0;
>>   if (!led_cdev->blink_brightness)
>>   led_cdev->blink_brightness = led_cdev->max_brightness;
>> - led_heartbeat_function(heartbeat_data->timer.data);
>> + led_heartbeat_function(_data->timer);
>>   set_bit(LED_BLINK_SW, _cdev->work_flags);
>>   led_cdev->activated = true;
>>  }
>>
>
> It seems that the patch title is somewhat incomplete, so I amended
> it as follows:
>
> s/to use/to use timer_setup()/

Yeah, thanks for fixing that. This got pointed out elsewhere and I've
since hunted down and fixed my scripts to not accidentally truncate
subjects. *sigh* :)

> and applied to the for-next branch of linux-leds.git after rebasing
> it onto 4.14-rc3.

Thanks!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] led/ledtrig-heartbeat: Convert timers to use

[Jacek Anaszewski]

Hi Kees,

Thanks for the patch.

On 10/05/2017 02:49 AM, Kees Cook wrote:
> Instead of using .data directly, convert to from_timer. Since the
> trigger_data is allocated separately, the led_cdev must be explicitly
> tracked for the callback.
> 
> Cc: Richard Purdie 
> Cc: Jacek Anaszewski 
> Cc: Pavel Machek 
> Cc: Zhang Bo 
> Cc: Linus Walleij 
> Cc: Ingo Molnar 
> Cc: linux-l...@vger.kernel.org
> Cc: Thomas Gleixner 
> Signed-off-by: Kees Cook 
> ---
> This requires commit 686fef928bba ("timer: Prepare to change timer
> callback argument type") in v4.14-rc3, but should be otherwise
> stand-alone.
> ---
>  drivers/leds/trigger/ledtrig-heartbeat.c | 16 ++--
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c 
> b/drivers/leds/trigger/ledtrig-heartbeat.c
> index e95ea65380c8..f0896de410b8 100644
> --- a/drivers/leds/trigger/ledtrig-heartbeat.c
> +++ b/drivers/leds/trigger/ledtrig-heartbeat.c
> @@ -25,19 +25,23 @@
>  static int panic_heartbeats;
>  
>  struct heartbeat_trig_data {
> + struct led_classdev *led_cdev;
>   unsigned int phase;
>   unsigned int period;
>   struct timer_list timer;
>   unsigned int invert;
>  };
>  
> -static void led_heartbeat_function(unsigned long data)
> +static void led_heartbeat_function(struct timer_list *t)
>  {
> - struct led_classdev *led_cdev = (struct led_classdev *) data;
> - struct heartbeat_trig_data *heartbeat_data = led_cdev->trigger_data;
> + struct heartbeat_trig_data *heartbeat_data =
> + from_timer(heartbeat_data, t, timer);
> + struct led_classdev *led_cdev;
>   unsigned long brightness = LED_OFF;
>   unsigned long delay = 0;
>  
> + led_cdev = heartbeat_data->led_cdev;
> +
>   if (unlikely(panic_heartbeats)) {
>   led_set_brightness_nosleep(led_cdev, LED_OFF);
>   return;
> @@ -127,18 +131,18 @@ static void heartbeat_trig_activate(struct led_classdev 
> *led_cdev)
>   return;
>  
>   led_cdev->trigger_data = heartbeat_data;
> + heartbeat_data->led_cdev = led_cdev;
>   rc = device_create_file(led_cdev->dev, _attr_invert);
>   if (rc) {
>   kfree(led_cdev->trigger_data);
>   return;
>   }
>  
> - setup_timer(_data->timer,
> - led_heartbeat_function, (unsigned long) led_cdev);
> + timer_setup(_data->timer, led_heartbeat_function, 0);
>   heartbeat_data->phase = 0;
>   if (!led_cdev->blink_brightness)
>   led_cdev->blink_brightness = led_cdev->max_brightness;
> - led_heartbeat_function(heartbeat_data->timer.data);
> + led_heartbeat_function(_data->timer);
>   set_bit(LED_BLINK_SW, _cdev->work_flags);
>   led_cdev->activated = true;
>  }
> 

It seems that the patch title is somewhat incomplete, so I amended
it as follows:

s/to use/to use timer_setup()/

and applied to the for-next branch of linux-leds.git after rebasing
it onto 4.14-rc3.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] led/ledtrig-heartbeat: Convert timers to use

[Jacek Anaszewski]

Hi Kees,

Thanks for the patch.

On 10/05/2017 02:49 AM, Kees Cook wrote:
> Instead of using .data directly, convert to from_timer. Since the
> trigger_data is allocated separately, the led_cdev must be explicitly
> tracked for the callback.
> 
> Cc: Richard Purdie 
> Cc: Jacek Anaszewski 
> Cc: Pavel Machek 
> Cc: Zhang Bo 
> Cc: Linus Walleij 
> Cc: Ingo Molnar 
> Cc: linux-l...@vger.kernel.org
> Cc: Thomas Gleixner 
> Signed-off-by: Kees Cook 
> ---
> This requires commit 686fef928bba ("timer: Prepare to change timer
> callback argument type") in v4.14-rc3, but should be otherwise
> stand-alone.
> ---
>  drivers/leds/trigger/ledtrig-heartbeat.c | 16 ++--
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c 
> b/drivers/leds/trigger/ledtrig-heartbeat.c
> index e95ea65380c8..f0896de410b8 100644
> --- a/drivers/leds/trigger/ledtrig-heartbeat.c
> +++ b/drivers/leds/trigger/ledtrig-heartbeat.c
> @@ -25,19 +25,23 @@
>  static int panic_heartbeats;
>  
>  struct heartbeat_trig_data {
> + struct led_classdev *led_cdev;
>   unsigned int phase;
>   unsigned int period;
>   struct timer_list timer;
>   unsigned int invert;
>  };
>  
> -static void led_heartbeat_function(unsigned long data)
> +static void led_heartbeat_function(struct timer_list *t)
>  {
> - struct led_classdev *led_cdev = (struct led_classdev *) data;
> - struct heartbeat_trig_data *heartbeat_data = led_cdev->trigger_data;
> + struct heartbeat_trig_data *heartbeat_data =
> + from_timer(heartbeat_data, t, timer);
> + struct led_classdev *led_cdev;
>   unsigned long brightness = LED_OFF;
>   unsigned long delay = 0;
>  
> + led_cdev = heartbeat_data->led_cdev;
> +
>   if (unlikely(panic_heartbeats)) {
>   led_set_brightness_nosleep(led_cdev, LED_OFF);
>   return;
> @@ -127,18 +131,18 @@ static void heartbeat_trig_activate(struct led_classdev 
> *led_cdev)
>   return;
>  
>   led_cdev->trigger_data = heartbeat_data;
> + heartbeat_data->led_cdev = led_cdev;
>   rc = device_create_file(led_cdev->dev, _attr_invert);
>   if (rc) {
>   kfree(led_cdev->trigger_data);
>   return;
>   }
>  
> - setup_timer(_data->timer,
> - led_heartbeat_function, (unsigned long) led_cdev);
> + timer_setup(_data->timer, led_heartbeat_function, 0);
>   heartbeat_data->phase = 0;
>   if (!led_cdev->blink_brightness)
>   led_cdev->blink_brightness = led_cdev->max_brightness;
> - led_heartbeat_function(heartbeat_data->timer.data);
> + led_heartbeat_function(_data->timer);
>   set_bit(LED_BLINK_SW, _cdev->work_flags);
>   led_cdev->activated = true;
>  }
> 

It seems that the patch title is somewhat incomplete, so I amended
it as follows:

s/to use/to use timer_setup()/

and applied to the for-next branch of linux-leds.git after rebasing
it onto 4.14-rc3.

-- 
Best regards,
Jacek Anaszewski