linux-kernel - Re: [RFC] led-class: always implement blinking

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: <1285104248.1290.551.camel@rex>
Date:	Tue, 21 Sep 2010 22:24:08 +0100
From:	Richard Purdie <rpurdie@...ys.net>
To:	Johannes Berg <johannes@...solutions.net>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC] led-class: always implement blinking

On Tue, 2010-09-21 at 21:48 +0200, Johannes Berg wrote:
> On Tue, 2010-09-21 at 12:44 -0700, Andrew Morton wrote:
> > On Fri, 20 Aug 2010 11:21:42 +0200
> > Johannes Berg <johannes@...solutions.net> wrote:
> > 
> > > +static int led_blink_set(struct led_classdev *led_cdev,
> > > +			 unsigned long *delay_on, unsigned long *delay_off)
> 
> > > +	if (*delay_on == led_cdev->blink_delay_on &&
> > > +	    *delay_off == led_cdev->blink_delay_off)
> > > +		return 0;
> > > +
> > > +	/* deactivate previous settings */
> > > +	del_timer_sync(&led_cdev->blink_timer);
> > > +
> > > +	led_cdev->blink_delay_on = *delay_on;
> > > +	led_cdev->blink_delay_off = *delay_off;
> 
> > delay_on and delay_off could have been pass-by-value rather than
> > pass-by-reference?  That would clean up some gunk in callers, too.
> > 
> > If there was some reason for doing it with pass-by-reference then that
> > reason should have been documented!
> 
> Well, this function gets assigned to led_cdev->blink_set(), which is a
> function pointer that takes pass-by-reference arguments. The comment
> there says:
> 
>         /* Activate hardware accelerated blink, delays are in
>          * miliseconds and if none is provided then a sensible default
>          * should be chosen. The call can adjust the timings if it can't
>          * match the values specified exactly. */
>         int             (*blink_set)(struct led_classdev *led_cdev,
>                                      unsigned long *delay_on,
>                                      unsigned long *delay_off);
> 
> but the software implementation doesn't adjust the timings, of course. I
> suppose the "adjust the timings" was also meant to update the values.

The idea was that hardware fallbacks would let the caller know what
values it had actually fallen back to. The software fallback using
timers is generic so doesn't need to change the values.

I've been meaning to look more closely at the patch but I haven't got to
it yet, sorry :(.

Richard

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