Messages in this thread | | | Date | Tue, 31 Jan 2006 18:44:41 +0100 | From | Bartlomiej Zolnierkiewicz <> | Subject | Re: [PATCH 10/11] LED: Add IDE disk activity LED trigger |
| |
Hi,
On 1/31/06, Richard Purdie <rpurdie@rpsys.net> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |