| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-id: <56161E9A.2000408@samsung.com>
Date: Thu, 08 Oct 2015 09:43:22 +0200
From: Jacek Anaszewski <j.anaszewski@...sung.com>
To: jiri.prchal@...ignal.cz
Cc: rpurdie@...ys.net, linux-leds@...r.kernel.org, cooloney@...il.com,
kyungmin.park@...sung.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] leds: triggers: add invert to heartbeat
Hi Jiri,
On 10/07/2015 04:14 PM, Jiří Prchal wrote:
> Hi Jacek,
> comments below...
>
> On 7.10.2015 15:32, Jacek Anaszewski wrote:
>> Hi Jiri,
>>
>> Thanks for the patch. It's nice in general, but please see my
>> comments below.
>>
>> On 10/07/2015 11:31 AM, Jiri Prchal wrote:
>>> This patcht adds possibility to invert heartbeat blinking. The
>>> inverted LED is
>>> more time ON then OFF.
>>> It's because it looks better when the heartbeat LED is next to other
>>> LED which
>>> is most time ON.
>>> The invert value is exported same way via sysfs in file invert like
>>> oneshot. I
>>> get inspiration from this trigger.
>>
>> Please adjust commit message so as it wouldn't exceed 75 characters line
>> length limit. scripts/checkpatch.pl is useful for catching this kind of
>> problems.
>>
>>> Signed-off-by: Jiri Prchal <jiri.prchal@...ignal.cz>
>>> ---
>>> drivers/leds/trigger/ledtrig-heartbeat.c | 49
>>> ++++++++++++++++++++++++++++++--
>>> 1 file changed, 47 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c
>>> b/drivers/leds/trigger/ledtrig-heartbeat.c
>>> index fea6871..c09c30b 100644
>>> --- a/drivers/leds/trigger/ledtrig-heartbeat.c
>>> +++ b/drivers/leds/trigger/ledtrig-heartbeat.c
>>> @@ -27,6 +27,7 @@ struct heartbeat_trig_data {
>>> unsigned int phase;
>>> unsigned int period;
>>> struct timer_list timer;
>>> + unsigned int invert;
>>> };
>>>
>>> static void led_heartbeat_function(unsigned long data)
>>> @@ -56,21 +57,27 @@ static void led_heartbeat_function(unsigned long
>>> data)
>>> msecs_to_jiffies(heartbeat_data->period);
>>> delay = msecs_to_jiffies(70);
>>> heartbeat_data->phase++;
>>> - brightness = led_cdev->max_brightness;
>>> + if (!heartbeat_data->invert)
>>> + brightness = led_cdev->max_brightness;
>>> break;
>>> case 1:
>>> delay = heartbeat_data->period / 4 - msecs_to_jiffies(70);
>>> heartbeat_data->phase++;
>>> + if (heartbeat_data->invert)
>>> + brightness = led_cdev->max_brightness;
>>
>> As you are at it, could you change, in a separate patch,
>> led_cdev->max_brightness to led_cdev->brightness? I think there is no
>> reason for which we shouldn't allow other values than max.
> I changed it but it doesn't blink at all.
Indeed, we can't rely on led_cdev->brightness as it is being zeroed
in the meantime.
We would have to use another variable for storing current brightness.
Probably we could use led_cdev->delayed_set_value, similarly as in
case of software blinking. It would require changes in
led_set_brightness(), so that it updated led_cdev->delayed_set_value
not only when timer trigger is enabled, but also in case other triggers
are.
I'll take care of that after recent patch set with LED core
improvements is applied.
>>
>>> break;
>>> case 2:
>>> delay = msecs_to_jiffies(70);
>>> heartbeat_data->phase++;
>>> - brightness = led_cdev->max_brightness;
>>> + if (!heartbeat_data->invert)
>>> + brightness = led_cdev->max_brightness;
>>> break;
>>> default:
>>> delay = heartbeat_data->period - heartbeat_data->period / 4 -
>>> msecs_to_jiffies(70);
>>> heartbeat_data->phase = 0;
>>> + if (heartbeat_data->invert)
>>> + brightness = led_cdev->max_brightness;
>>> break;
>>> }
>>>
>>> @@ -78,15 +85,50 @@ static void led_heartbeat_function(unsigned long
>>> data)
>>> mod_timer(&heartbeat_data->timer, jiffies + delay);
>>> }
>>>
>>> +static ssize_t led_invert_show(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>> + struct heartbeat_trig_data *heartbeat_data =
>>> led_cdev->trigger_data;
>>> +
>>> + return sprintf(buf, "%u\n", heartbeat_data->invert);
>>> +}
>>> +
>>> +static ssize_t led_invert_store(struct device *dev,
>>> + struct device_attribute *attr, const char *buf, size_t size)
>>> +{
>>> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>> + struct heartbeat_trig_data *heartbeat_data =
>>> led_cdev->trigger_data;
>>> + unsigned long state;
>>> + int ret;
>>> +
>>> + ret = kstrtoul(buf, 0, &state);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + heartbeat_data->invert = !!state;
>>> +
>>> + return size;
>>> +}
>>> +
>>> +static DEVICE_ATTR(invert, 0644, led_invert_show, led_invert_store);
>>> +
>>> static void heartbeat_trig_activate(struct led_classdev *led_cdev)
>>> {
>>> struct heartbeat_trig_data *heartbeat_data;
>>> + int rc;
>>>
>>> heartbeat_data = kzalloc(sizeof(*heartbeat_data), GFP_KERNEL);
>>> if (!heartbeat_data)
>>> return;
>>>
>>> led_cdev->trigger_data = heartbeat_data;
>>> + rc = device_create_file(led_cdev->dev, &dev_attr_invert);
>>> + if (rc) {
>>> + kfree(led_cdev->trigger_data);
>>> + return;
>>> + }
>>> +
>>> setup_timer(&heartbeat_data->timer,
>>> led_heartbeat_function, (unsigned long) led_cdev);
>>> heartbeat_data->phase = 0;
>>> @@ -100,9 +142,12 @@ static void heartbeat_trig_deactivate(struct
>>> led_classdev *led_cdev)
>>>
>>> if (led_cdev->activated) {
>>> del_timer_sync(&heartbeat_data->timer);
>>> + device_remove_file(led_cdev->dev, &dev_attr_invert);
>>> kfree(heartbeat_data);
>>> led_cdev->activated = false;
>>> }
>>> +
>>> + led_set_brightness(led_cdev, LED_OFF);
>>
>> I believe this is a fix. Could you split it into a separate patch
>> and explain its merit?
> I'm not sure of necessity of it, I copied it from oneshot.
> Without it could leave LED in ON state. But never happens to me.
> Should I produce separate patch to discuss it in separate thread?
This is not necessary. led_trigger_remove() calls
let_trigger_set(led_cdev, NULL), which in turn calls
led_set_brightness(led_cdev, LED_OFF).
Please remove above line and adjust commit message format
so that checkpatch.pl doesn't complain.
>>
>>> }
>>>
>>> static struct led_trigger heartbeat_led_trigger = {
>>>
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-leds" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists