Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 4789 bytes --]

On Fri, 6 Apr 2012 16:53:53 -0700 Andrew Morton <akpm@linux-foundation.org>
wrote:

> On Sun, 01 Apr 2012 13:53:59 -0600
> Shuah Khan <shuahkhan@gmail.com> wrote:
> 
> > LED infrastructure lacks support for one shot timer trigger and activation.
> > The current support allows for setting two timers, one for specifying how
> > long a state to be on, and the second for how long the state to be off. For
> > example, delay_on value specifies the time period an LED should stay in on
> > state, followed by a delay_off value that specifies how long the LED should
> > stay in off state. The on and off cycle repeats until the trigger gets
> > deactivated. There is no provision for one time activation to implement
> > features that require an on or off state to be held just once and then stay
> > in the original state forever.
> > 
> > This feature will help implement vibrate functionality which requires one
> > time activation of vibrate mode without a continuous vibrate on/off cycles.
> > 
> > >From 1ebe0fd67580da833f8f06fc3119445e9991100f Mon Sep 17 00:00:00 2001
> > From: Shuah Khan <shuahkhan@gmail.com>
> > Date: Sat, 31 Mar 2012 21:56:07 -0600
> > Subject: [PATCH] LEDS-One-Shot-Timer-Trigger-implementation
> 
> Should be "leds: one shot timer trigger implementation", please.
> 
> >
> > ...
> >
> > +This feature will help implement vibrate functionality which requires one
> > +time activation of vibrate mode without a continuous vibrate on/off cycles.
> 
> They make vibrating LED? ;)
> 
> What's going on here?  You're proposing to repurpose the LEDs code to
> drive vibration devices?  Or some devices couple a LED with a vibration
> device?

We use 'scsi' devices to drive SATA disks.
We use 'input' devices to beep the PC 'speaker'.

It is no great deviation from this pattern to use a 'led' device to trigger a
vibrator.

We could rename all of these subsystems, but that probably won't win us any
friends.
It may well make sense to create a 'vibrator' class which shared 99% of the
code of the 'led' class.  This is a bit like the 'backlight' class which also
drives LEDs (sometimes) and sets their brightness.  It is presumably helpful
for user-space to see the difference intention of the LEDS - some for display
backlight and some for signalling - and so also helpful to differentiate
output devices - some emit light, some emit kinetic energy.

It would be really nice to have a system-wide holistic way to address these
classification issues.  But as we tend to work in an incremental-development
approach, it is easiest just to add functionality where it seems to fit.

> > ...
> >
> > --- a/drivers/leds/ledtrig-timer.c
> > +++ b/drivers/leds/ledtrig-timer.c
> > @@ -24,6 +24,9 @@ static ssize_t led_delay_on_show(struct device *dev,
> >  {
> >  	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> >  
> > +	if (led_cdev->blink_delay_on == LED_TIMER_FOREVER)
> > +		return sprintf(buf, "forever\n");
> > +
> >  	return sprintf(buf, "%lu\n", led_cdev->blink_delay_on);
> >  }
> 
> This is a somewhat risky kernel interface change.  Previously the
> output was always a decimal string.  With this change it is either a
> decimal string or "forever".  Applications which were coded to the old
> interface will probably misinterpret the "forever" as a zero.  Or they
> will crash and burn.
> 
> A safer approach would be to just print the value.  So it comes out as
> 4294967295 or 18446744073709551615.  That's pretty nasty, and invites
> people to write programs which are busted on 32-bit userspace (or on
> 64-bit).
> 
> Maybe it was just a mistake to overload ->blink_delay_on in this
> fashion.  Perhaps the code will be cleaner if we add a couple of new
> booleans to the led_cdev.
> 
> Anyway, please have a think about this.
> 

I don't think it is very risky.  The output will continue to always be a
decimal string unless user-space explicitly chooses to write "forever".
So the risky step is to modify part of userspace to write 'forever' without
modifying other parts to recognise 'forever'.  I doubt anyone would actually
do that but in any case it is not the place of the kernel to care.  The
kernel change is perfectly safe (Well.... it would be if we trapped an
attempt to write 'MAXINT' and converted  it to 'MAXINT-1' - that is trivial
to do and then we would be perfectly safe).

However it might make sense to discard the 'forever' and instead introduce a
'repeat_count' attribute which defaults to 'forever' but could be explicitly
set to '1' (or 2 or 3).
I didn't advocate that at first as it seemed a bigger change than needed (and
incremental development is good), but maybe it is better to take that bigger
step.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]