linux-kernel - Re: [PATCH v2 1/3] leds: add device activity LED triggers

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: <20151001144718.GQ26335@jcartwri.amer.corp.natinst.com>
Date:	Thu, 1 Oct 2015 09:47:18 -0500
From:	Josh Cartwright <joshc@...com>
To:	Maciek Borzecki <maciek.borzecki@...il.com>
Cc:	linux-kernel@...r.kernel.org, Richard Purdie <rpurdie@...ys.net>,
	Jacek Anaszewski <j.anaszewski@...sung.com>,
	linux-leds@...r.kernel.org, linux-doc@...r.kernel.org,
	Jonathan Corbet <corbet@....net>
Subject: Re: [PATCH v2 1/3] leds: add device activity LED triggers

Hello Maciek-

Some architectural questions below:

On Thu, Oct 01, 2015 at 04:04:31PM +0200, Maciek Borzecki wrote:
> The patch adds LED triggers for indicating an activity on a selected
> device. The drivers that intend to use triggers need to register
> respective devices using ledtrig_dev_add(). Triggers are generated by
> explicitly calling ledtrig_dev_activity().
> 
> Signed-off-by: Maciek Borzecki <maciek.borzecki@...il.com>
> ---
[..]
> +struct ledtrig_dev_data {
> +	char name[MAX_NAME_LEN];
> +	dev_t dev;
> +	struct led_trigger *trig;
> +	struct list_head node;
> +};
> +
> +/**
> + * ledtrig_dev_activity - signal activity on device
> + * @dev: device
> + *
> + * Fires a trigger assigned to @dev device.
> + */
> +void ledtrig_dev_activity(dev_t dev)

It seems a bit strange to me to associate a device LED trigger with
dev_t.  Some devices don't expose a dev node, some devices expose
multiple dev nodes...

Is there a reason why you are not tying to the device model?

> +{
> +	struct ledtrig_dev_data *dev_trig;
> +
> +	if (!down_read_trylock(&devs_list_lock))
> +		return;
> +
> +	list_for_each_entry(dev_trig, &devs_list, node) {
> +		if (dev_trig->dev == dev) {
> +			led_trigger_blink_oneshot(dev_trig->trig,
> +						  &blink_delay,
> +						  &blink_delay,
> +						  0);
> +			break;
> +		}
> +	}
> +	up_read(&devs_list_lock);
> +}
> +EXPORT_SYMBOL(ledtrig_dev_activity);

Not _GPL?

  Josh

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)