linux-kernel - Re: [PATCH 1/1] leds: add "kickable" LED trigger

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1334617524.2842.7.camel@lorien2>
Date:	Mon, 16 Apr 2012 17:05:24 -0600
From:	Shuah Khan <shuahkhan@...il.com>
To:	Jonas Bonn <jonas@...thpole.se>
Cc:	shuahkhan@...il.com, akpm@...ux-foundation.org, neilb@...e.de,
	linux-kernel@...r.kernel.org, richard.purdie@...uxfoundation.org
Subject: Re: [PATCH 1/1] leds: add "kickable" LED trigger

On Tue, 2012-04-17 at 00:33 +0200, Jonas Bonn wrote:
> Hi Shuah,
> 
> Nice work!  If I may comment on this briefly:
> 
> i)  The semantics of this patch make me cringe.  It's bad enough that
> this is a "LED" trigger; having a "vibrating LED" is even worse.
> Really, this is a GPIO trigger (which is reasonably logical); secondly,
> this trigger is about maintaining a transient state given some criteria.
> 
> ii)  Why force the trigger to a fixed duration?  The point, if I
> understand correctly, is to return to the "rest state" after a given
> time in case the triggering entity dies; but if the triggering entity
> (userspace application) is still alive, why not let it prolong the
> "active state" by triggering again.  This is along the lines of the
> 'kickable' trigger patch I posted earlier.
> 
> iii)  I don't see the point of maintaining the state.  The state is
> maintained by the fact that you have running timer; if the timer is not
> running, the state is the "rest state".
> 
> I'd suggest:
> 
> i)  Calling this ledtrig-transient
> ii)  Having a property 'duration' that determines how long the
> GPIO/LED/signal is 'active'
> iii) Having a property 'activate' (or 'tickle'!) that just sets the
> 'active' state if it isn't already active; writing to this properly
> while active just resets the timer to 0 so that another full 'duration'
> can pass before the LED deactivates.
> iv)  Maybe having a property 'active-state' to decide whether the LED is
> on or off when active; this isn't strictly necessary as the LED class
> already has the active-low property to invert the meaning of the ON
> state.
> 
> This would allow this trigger to be used for a bunch of different use
> cases:
> 
> i)   Control of vibrator by userspace app.
> ii)  Use of LED by userspace app as activity indicator.
> iii) Use of LED by userspace app as a kind of watchdog indicator -- as
> long as the app is alive, it can keep the LED illuminated, if it dies
> the LED will be extinguished automatically.
> iv)  Use by any userspace app that needs a transient GPIO output.
> 
> I hope that makes sense.  It mostly comes down to semantics... the
> implementation is mostly fine, with the exception of maintaining the
> illumination state that I don't see the need for.
> 
> /Jonas

Jonas,

Thanks for the feedback. I better change the name before too long :)
Thanks for summarizing the use-cases. I like your duration and activate
idea. I am testing the patch now and will incorporate your ideas. I
would like to include you in the thread when I send it out if you don't
mind. Thanks again,

-- Shuah

--
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/