nand-disk LED trigger: to remove, or not to remove

[Ezequiel Garcia]

On 7 April 2016 at 21:37, Boris Brezillon
<boris.brezillon at free-electrons.com> wrote:
> On Wed, 6 Apr 2016 11:03:16 -0700
> Brian Norris <computersforpeace at gmail.com> wrote:
>
>> On Tue, Apr 05, 2016 at 04:51:20PM -0300, Ezequiel Garcia 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?
>>
>> I don't have much opinion about the LED trigger, except that it'd be
>> nice if it either worked consistently or was removed.
>>
>> > 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:
>> >
>> > 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.
>> >
>> > Signed-off-by: Ezequiel Garcia <ezequiel at vanguardiasur.com.ar>
>>
>> [...]
>>
>> Notably, your patch changes the behavior pretty significantly. Instead
>> of triggering for individual NAND wait periods (very fine-grained) you
>> only trigger for entire write/read/erase operations.
>
> Hm, I don't think the blinking frequency can be considered a stable
> ABI :-). Anyway, most of the time, read/write coming from FS are
> done on a per-page basis (except for the UBI/UBIFS maintenance
> operations), so it should pretty much match the existing behavior.
>
>> That may be OK,
>> especially if it's modelled after IDE.
>>
>> I'd also note that you missed a few APIs (e.g., mtd_{read,write}_oob()).
>
> Yep, I forgot to mention that in my review.
>

Ah, yes, I wasn't hesitating about blinking on OOB activity (for no
good reason).
I'll include it on a v2.

Thanks for the reviews!
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar