Re: [PATCH 10/11] LED: Add IDE disk activity LED trigger

[Bartlomiej Zolnierkiewicz]

Hi,

On 1/31/06, Richard Purdie <rpurdie@xxxxxxxxx> wrote:
> Hi,
>
> On Tue, 2006-01-31 at 15:46 +0100, Bartlomiej Zolnierkiewicz wrote:
> >
> > Why cannot existing block layer hook be used for this?
>
> The trigger is supposed to be reflecting actual hardware activity, not
> block layer activity.

Ben, code in pmac.c (+ block layer) seems to be doing something
different then Kconfig help entry states ("Blink laptop LED on drive
activity")?

> I'll experiment with the feasibility of the block later as I've always
> been uneasy about the hooks into the lower level layers. There are a
> number of issues to consider though.

At worst it should be handled at host driver level not device driver
(like pmac.c and hwif->act_led).

> 1. The block layer isn't always aware of device activity (eg. flash
> block erasing in mtd devices) (is this the case for IDE?).

Same is true for ide-disk changes - they are aware only of filesystem
activity (no flush cache, special commands, I/O taskfiles etc.).

> 2. Default trigger naming becomes problematic for led devices. Currently
> an MMC card reader's LED could set its trigger to say "mmc-disk" and end
> up with some kind of sensible activity light. (ignoring the more than
> one card reader case where all the lights would be synced :).
>
> A potential solution would be to add individual gendisk triggers by
> hooking add_disk/del_disk. The MMC read would presumably know its
> major/minor number before registering its LED.
>
> I'm not sure how to intercept disk activity for a given gendisk offhand.
> There is also a question of where the led_trigger pointers end up.
> struct gendisk may or may not be acceptable.

gendisk presents device at filesystem layer and it is
probably not the appropriate place to add LED handling

> 3. Matching something like all IDE disks becomes hard (and is actually
> more desirable than individual devices at times - see below).
>
> At first glance a potential solution would be to hook
> register_blkdev/unregister_blkdev and create yet more triggers but where
> do you hook the activity? There is no data structure the led trigger
> pointer can be part of either.
>
> These solutions are going to end up with a lot of unused led triggers on
> any given system.
>
> > Why are you adding LED_FULL event handling to a specific
> > device driver (ide-disk) but LED_OFF event handling to a generic
> > IDE end request function?
>
> The trigger started out as just being ide-disk.c based but there is no
> place where the IDE end request function could be hooked within it due
> to its use of generic functions. The trigger therefore had to move into
> more generic code. If there was a point in ide-disk where an IDE end
> request could be hooked it, it could be confined to that file.

Isn't ->end_request hook in ide_driver_t enough?

I see no reason why the custom ->end_request function cannot
be added to ide-disk.  All needed infrastructure is there.

> Alternatively it could be made to apply to all ide activity if a
> suitable start request point was found to hook into.

start_request() in ide-io.c

Thanks,
Bartlomiej
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/