nand-disk LED trigger: to remove, or not to remove
Jacek Anaszewski
j.anaszewski at samsung.com
Fri Apr 8 00:38:06 PDT 2016
Hi Ezequiel, Boris,
On 04/08/2016 05:02 AM, Ezequiel Garcia wrote:
> On 7 April 2016 at 21:32, Boris Brezillon
> <boris.brezillon at free-electrons.com> wrote:
>> Hi Ezequiel,
>>
>> On Tue, 5 Apr 2016 16:51:20 -0300
>> Ezequiel Garcia <ezequiel at vanguardiasur.com.ar> wrote:
>>
>>> Due to the way the 'nand-disk' LED trigger is implemented,
>>> it currently does not work correctly for all NAND drivers.
>>>
>>> This is somewhat related to an old thread, where we discussed
>>> the addition of an "mtd" LED trigger. See:
>>>
>>> http://www.spinics.net/lists/linux-leds/msg01181.html
>>>
>>> My question is:
>>>
>>> * given that nobody has complained about "nand-disk"
>>> working on just some NAND drivers, and...
>>> * given that nobody has complained (except that 2013 patch)
>>> about lacking a generic MTD LED trigger...
>>>
>>> Does it make any sense to have such a trigger at all?
>>> In other words, should we simply get rid of "nand-disk" trigger?
>>>
>>> In case the answer is "We want to keep some LED trigger",
>>> then here's a patch for you to f̶l̶a̶m̶e̶ review:
>>
>> I agree, we should either drop the whole thing or make it available to
>> all MTD devices. And since people might use that, I'd say we should
>> make it available to all MTD devices.
>>
>
> Alright then, let's do it.
>
>>>
>>> From 88c7102bb67056b443da323bd3e28b60aca948a2 Mon Sep 17 00:00:00 2001
>>> From: Ezequiel Garcia <ezequiel at vanguardiasur.com.ar>
>>> Date: Sat, 2 Apr 2016 18:35:50 -0300
>>> Subject: [PATCH] leds: trigger: Introduce a MTD (NAND/NOR) trigger
>>>
>>> This commit introduces a MTD trigger for flash (NAND/NOR) device
>>> activity. The implementation is copied from IDE disk.
>>>
>>> This deprecates the "nand-disk" LED trigger, but for backwards
>>> compatibility, we still keep the "nand-disk" trigger around.
>>>
>>> The motivation for deprecating the "nand-disk" LED trigger is that
>>> it only works for NAND drivers, whereas the "mtd" LED trigger
>>> is more generic (in fact, "nand-disk" currently only works for
>>> certain NAND drivers).
>>>
>>> TODO: Measure how the trigger affects MTD I/O performance.
>>> It should be cheap because the blink is deferred, but still
>>> it makes sense to provide some hard numbers.
>>
>> Hm, I don't expect that much overhead from this feature, and if people
>> really want to remove all the overhead, they can just disable
>> LEDS_TRIGGER_MTD.
>>
>>>
>>> Signed-off-by: Ezequiel Garcia <ezequiel at vanguardiasur.com.ar>
>>
>> Few minor comments below, otherwise it looks good to me.
>>
>>> ---
>>> drivers/leds/trigger/Kconfig | 8 +++++++
>>> drivers/leds/trigger/Makefile | 1 +
>>> drivers/leds/trigger/ledtrig-mtd.c | 48 ++++++++++++++++++++++++++++++++++++++
>>> drivers/mtd/mtdcore.c | 4 ++++
>>> drivers/mtd/nand/nand_base.c | 29 +----------------------
>>> include/linux/leds.h | 6 +++++
>>> 6 files changed, 68 insertions(+), 28 deletions(-)
>>> create mode 100644 drivers/leds/trigger/ledtrig-mtd.c
>>>
>>> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
>>> index 554f5bfbeced..b7044a0530c7 100644
>>> --- a/drivers/leds/trigger/Kconfig
>>> +++ b/drivers/leds/trigger/Kconfig
>>> @@ -115,4 +115,12 @@ config LEDS_TRIGGER_PANIC
>>> This allows LEDs to be configured to blink on a kernel panic.
>>> If unsure, say Y.
>>>
>>> +config LEDS_TRIGGER_MTD
>>> + bool "LED MTD (NAND/NOR) Trigger"
>>> + depends on MTD
>>> + depends on LEDS_TRIGGERS
>>> + help
>>> + This allows LEDs to be controlled by MTD activity.
>>> + If unsure, say N.
>>> +
>>> endif # LEDS_TRIGGERS
>>> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
>>> index 547bf5c80e52..80e32add3b07 100644
>>> --- a/drivers/leds/trigger/Makefile
>>> +++ b/drivers/leds/trigger/Makefile
>>> @@ -9,3 +9,4 @@ obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON) += ledtrig-default-on.o
>>> obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT) += ledtrig-transient.o
>>> obj-$(CONFIG_LEDS_TRIGGER_CAMERA) += ledtrig-camera.o
>>> obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o
>>> +obj-$(CONFIG_LEDS_TRIGGER_MTD) += ledtrig-mtd.o
>>> diff --git a/drivers/leds/trigger/ledtrig-mtd.c b/drivers/leds/trigger/ledtrig-mtd.c
>>> new file mode 100644
>>> index 000000000000..badf72aa1f5f
>>> --- /dev/null
>>> +++ b/drivers/leds/trigger/ledtrig-mtd.c
>>> @@ -0,0 +1,48 @@
>>> +/*
>>> + * LED MTD trigger
>>> + *
>>> + * Copyright 2016 Ezequiel Garcia <ezequiel at vanguardiasur.com.ar>
>>> + *
>>> + * Based on LED IDE-Disk Activity Trigger
>>> + *
>>> + * Copyright 2006 Openedhand Ltd.
>>> + *
>>> + * Author: Richard Purdie <rpurdie at openedhand.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + */
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/init.h>
>>> +#include <linux/leds.h>
>>> +
>>> +#define BLINK_DELAY 30
>>> +
>>> +DEFINE_LED_TRIGGER(ledtrig_mtd);
>>> +DEFINE_LED_TRIGGER(ledtrig_nand);
>>> +static unsigned long blink_delay = BLINK_DELAY;
>>> +
>>> +void ledtrig_mtd_activity(void)
>>> +{
>>> + led_trigger_blink_oneshot(ledtrig_nand,
>>> + &blink_delay, &blink_delay, 0);
>>> + led_trigger_blink_oneshot(ledtrig_mtd,
>>> + &blink_delay, &blink_delay, 0);
>>
>> I'd recommend using a local variable for blink_delay, since this is
>> something that seems to be modified by led_trigger_blink_oneshot().
>> I don't know what led_trigger_blink_oneshot() does with those
>> pointers, but making it a global variable seems dangerous to me (in
>> case of concurrent calls to ledtrig_mtd_activity())
>>
>
> OK, I'll revisit this. FWIW, it's copy-pasted from the ide-disk.
I agree - blink_delay can be altered by a LED class driver in case
it implements blink_set op, which would have global impact on all
LED class drivers registered on this trigger.
Ezequiel, please make the blink_delay variable local to
ledtrig_mtd_activity. We well need also to provide a fix for
ledtrig-ide-disk.c.
>>> +}
>>> +EXPORT_SYMBOL(ledtrig_mtd_activity);
>>
>> [...]
>>
>>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>>> index f203a8f89d30..19eb10278bea 100644
>>> --- a/include/linux/leds.h
>>> +++ b/include/linux/leds.h
>>> @@ -329,6 +329,12 @@ extern void ledtrig_ide_activity(void);
>>> static inline void ledtrig_ide_activity(void) {}
>>> #endif
>>>
>>> +#ifdef CONFIG_LEDS_TRIGGER_MTD
>>> +extern void ledtrig_mtd_activity(void);
>>
>> You can drop the 'extern' specifier here.
>>
>
> I know, but it's declared like this in the leds.h header,
> and I'm a big fan of code consistency :-)
>
We'll need to create a patch that removes all redundant extern modifiers
from leds.h, but please keep it in this one for consistency.
--
Best regards,
Jacek Anaszewski
More information about the linux-mtd
mailing list