Add an LED trigger for IDE disk activity to the IDE subsystem.
Signed-off-by: Richard Purdie <[email protected]>
Index: linux-2.6.15/drivers/ide/ide-disk.c
===================================================================
--- linux-2.6.15.orig/drivers/ide/ide-disk.c 2006-01-29 14:43:00.000000000 +0000
+++ linux-2.6.15/drivers/ide/ide-disk.c 2006-01-29 15:22:48.000000000 +0000
@@ -60,6 +60,7 @@
#include <linux/genhd.h>
#include <linux/slab.h>
#include <linux/delay.h>
+#include <linux/leds.h>
#define _IDE_DISK
@@ -297,6 +298,8 @@
}
}
+extern struct led_trigger *ide_led_trigger;
+
/*
* 268435455 == 137439 MB or 28bit limit
* 320173056 == 163929 MB or 48bit addressing
@@ -315,6 +318,8 @@
return ide_stopped;
}
+ led_trigger_event(ide_led_trigger, LED_FULL);
+
pr_debug("%s: %sing: block=%llu, sectors=%lu, buffer=0x%08lx\n",
drive->name, rq_data_dir(rq) == READ ? "read" : "writ",
block, rq->nr_sectors, (unsigned long)rq->buffer);
Index: linux-2.6.15/drivers/ide/ide-io.c
===================================================================
--- linux-2.6.15.orig/drivers/ide/ide-io.c 2006-01-29 14:37:20.000000000 +0000
+++ linux-2.6.15/drivers/ide/ide-io.c 2006-01-29 15:28:00.000000000 +0000
@@ -48,6 +48,7 @@
#include <linux/device.h>
#include <linux/kmod.h>
#include <linux/scatterlist.h>
+#include <linux/leds.h>
#include <asm/byteorder.h>
#include <asm/irq.h>
@@ -93,6 +94,8 @@
}
EXPORT_SYMBOL(__ide_end_request);
+extern struct led_trigger *ide_led_trigger;
+
/**
* ide_end_request - complete an IDE I/O
* @drive: IDE device for the I/O
@@ -123,6 +126,9 @@
ret = __ide_end_request(drive, rq, uptodate, nr_sectors);
spin_unlock_irqrestore(&ide_lock, flags);
+
+ led_trigger_event(ide_led_trigger, LED_OFF);
+
return ret;
}
EXPORT_SYMBOL(ide_end_request);
Index: linux-2.6.15/drivers/ide/ide.c
===================================================================
--- linux-2.6.15.orig/drivers/ide/ide.c 2006-01-29 14:43:00.000000000 +0000
+++ linux-2.6.15/drivers/ide/ide.c 2006-01-29 15:23:59.000000000 +0000
@@ -154,6 +154,7 @@
#include <linux/seq_file.h>
#include <linux/device.h>
#include <linux/bitops.h>
+#include <linux/leds.h>
#include <asm/byteorder.h>
#include <asm/irq.h>
@@ -1990,6 +1991,8 @@
EXPORT_SYMBOL_GPL(ide_bus_type);
+INIT_LED_TRIGGER_GLOBAL(ide_led_trigger);
+
/*
* This is gets invoked once during initialization, to set *everything* up
*/
@@ -2036,6 +2039,8 @@
#ifdef CONFIG_PROC_FS
proc_ide_create();
#endif
+ led_trigger_register_simple("ide-disk", &ide_led_trigger);
+
return 0;
}
@@ -2068,6 +2073,8 @@
{
int index;
+ led_trigger_unregister_simple(ide_led_trigger);
+
for (index = 0; index < MAX_HWIFS; ++index)
ide_unregister(index);
Hi,
Why cannot existing block layer hook be used for this?
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?
This solution has very limited flexibility (disk accesses for
all IDE ports will be registered as coming from the same
source) but I guess it is fine?
Thanks,
Bartlomiej
On 1/31/06, Richard Purdie <[email protected]> wrote:
> Add an LED trigger for IDE disk activity to the IDE subsystem.
>
> Signed-off-by: Richard Purdie <[email protected]>
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.
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.
1. The block layer isn't always aware of device activity (eg. flash
block erasing in mtd devices) (is this the case for IDE?).
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.
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.
Alternatively it could be made to apply to all ide activity if a
suitable start request point was found to hook into.
> This solution has very limited flexibility (disk accesses for
> all IDE ports will be registered as coming from the same
> source) but I guess it is fine?
For users in the handheld world, that's desirable as the trigger shows
any IDE disk activity. You normally only have a small number of leds to
work with (say 1 or 2).
I can imagine some users wanting to be able to get this information per
IDE port but the code to do that would be a lot more invasive with more
overhead. It might be easier and more flexible through the block layer
which is something I'll investigate. I get the feeling things start to
scale out of proportion and control though.
Regards,
Richard
Hi,
On 1/31/06, Richard Purdie <[email protected]> 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
On Tue, Jan 31 2006, Bartlomiej Zolnierkiewicz wrote:
> Hi,
>
> On 1/31/06, Richard Purdie <[email protected]> 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 doubt it really matters a lot, since either the activity will be done
right after (the LED will likely still be on), or the drive is already
busy doing stuff (in which case the LED is on anyways). So while the
trigger point might not be at the instant we start drive activity, it's
really close.
You could move the block layer trigger from add_request() to
elevator.c:elv_next_request() instead, right where it sets REQ_STARTED
to improve the start trigger point. Since that can happen at irq time
(whereas the add_request() cannot), it's likely more expensive.
The goal of the activity led for powerbook was not really to be 100%
accurate, but be able to tell whether the drive was doing io or not.
It's nice feedback to have for the user.
That said, the LED stuff should be able to handle pmac as well, so why
not add it generically instead of clamping it into the ide layer in odd
places?
--
Jens Axboe
On Tue, Jan 31 2006, Richard Purdie 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.
>
> 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.
>
> 1. The block layer isn't always aware of device activity (eg. flash
> block erasing in mtd devices) (is this the case for IDE?).
>
> 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.
>
> 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.
Perhaps a generic solution isn't feasible, because this isn't really a
generic problem. The LED stuff has very limited use - you mention
embedded platforms, perhaps they should just be doing this on their own?
Generally I'm finding a hard time justifying an LED api, honestly. It
just feels like one of those things where the actual abstraction ends up
being a lot bigger than code needed. Abstracting and creating an API
isn't always useful.
--
Jens Axboe
On 31/01/06 21:35 +0100, "Jens Axboe" wrote:
> Perhaps a generic solution isn't feasible, because this isn't really a
> generic problem. The LED stuff has very limited use - you mention
> embedded platforms, perhaps they should just be doing this on their own?
Possibly, but what you'll find is that many different embedded platforms
end up wanting similar behavior, and if they all do it on their own, that
ends up in a mess.
Take the Alchemy platform for example - we have a bank of LEDs at our
disposal, and we too would like to use them to do very similar things
as the ARM/Xscale folks - things like show IDE/SD/MTD traffic, or the
status of a battery. Rather then go in and hack that stuff in ourselves,
I would much rather just define a few API hooks and getting the rest for
free (or really cheap).
Perhaps it can be hidden behind a CONFIG_EMBEDDED or something so that
the desktop platforms aren't bothered by it, but speaking for the two
embedded platforms I'm attached to, my vote for this LED class is a "yes".
--
Jordan Crouse
Senior Linux Engineer
AMD - Personal Connectivity Solutions Group
<http://www.amd.com/embeddedprocessors>
On Tue, 2006-01-31 at 21:35 +0100, Jens Axboe wrote:
> Perhaps a generic solution isn't feasible, because this isn't really a
> generic problem.
I agree. I think that email goes to show that totally generic led
triggers aren't achievable, desirable or useful.
> The LED stuff has very limited use - you mention
> embedded platforms, perhaps they should just be doing this on their own?
I am convinced the led class core and the led trigger *core* should be
in the mainline kernel. The alternative is for everyone to invent their
own versions and end up in a mess. The arm LED code is one example of
something done adhoc which no other arch can benefit from.
The core code doesn't touch anything outside of drivers/leds and can be
hidden behind any config options found to be appropriate.
> Generally I'm finding a hard time justifying an LED api, honestly. It
> just feels like one of those things where the actual abstraction ends up
> being a lot bigger than code needed. Abstracting and creating an API
> isn't always useful.
Nobody seems to have any issues with the led class or the led drivers
themselves. The triggers are the controversial aspect. The trigger API
is in a way too powerful as it can let you use anything as an LED
trigger. This leads to people asking for anything and everything as a
trigger.
Perhaps the best solution might be to allow the LED class core, the
triggers *core* and led drivers into the kernel but be extremely
selective about which triggers are allowed (if any).
I think there is a case for including specific triggers like the sharpsl
charging trigger as if we're going to have sharpsl charging code in the
kernel (which we have), it might as well connect up to the charging led
it was built for.
If all other more generic triggers are rejected, I can live with that.
Maintaining a handful of trigger patches outside the kernel, most of
them a few lines long is much easier than maintaining a whole subsystem.
Would that be acceptable?
Richard
> 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.
The hook in the block layer was Jens idea :)
> 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.).
It wouldn't be too hard to kick the led on DMA start and off after a
timeout kicked from DMA end...
> 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.
I'm not sure ide-disk is the right spot ... what if one wants LEDs for
CD-ROMs or other IDE devices like flash cards ?
> > 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
On Wed, 2006-02-01 at 09:08 +1100, Benjamin Herrenschmidt wrote:
> > 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.
>
> I'm not sure ide-disk is the right spot ... what if one wants LEDs for
> CD-ROMs or other IDE devices like flash cards ?
Flash cards mainly go through ide-cs which uses ide-disk.
There are several options:
1. Have the trigger on ide-disk activity (confined to ide-disk.c).
2. Have the trigger on ide-io activity (confined to ide-io.c).
3. Do something else.
4. Don't add disk activity (or any?) triggers in mainline.
Thanks to a generic API, adding triggers anywhere is fairly trivial but
probably not without controversy so they need to be carefully placed, if
at all.
We could always wait and see which of the above there is demand for. I
suspect options 1 or 2 would cover most people's needs. We're never
going to satisfy everyone (but anyone is free to apply simple patches to
get their desired trigger).
I'd rather go with option 4 than block all the led class/driver code
over any controversial triggers.
Richard
On 1/31/06, Jordan Crouse <[email protected]> wrote:
> On 31/01/06 21:35 +0100, "Jens Axboe" wrote:
> > Perhaps a generic solution isn't feasible, because this isn't really a
> > generic problem. The LED stuff has very limited use - you mention
> > embedded platforms, perhaps they should just be doing this on their own?
>
> Possibly, but what you'll find is that many different embedded platforms
> end up wanting similar behavior, and if they all do it on their own, that
> ends up in a mess.
Indeed. This LED code is exactly what we need for just about every
linux handheld in existence. Please include it.
Matt
>
>Add an LED trigger for IDE disk activity to the IDE subsystem.
>
Since I am not a real user of the led subsystem - what LED should be lit
anyway? only the motherboard one does, and it seems to be connected
directly to the IDE chip - I would want this as a compile-time option so I
don't pay any extra time for any led stuff.
>Signed-off-by: Richard Purdie <[email protected]>
Jan Engelhardt
--
On Tue, Jan 31, 2006 at 02:22:49PM -0700, Jordan Crouse wrote:
>...
> Perhaps it can be hidden behind a CONFIG_EMBEDDED or something so that
> the desktop platforms aren't bothered by it, but speaking for the two
> embedded platforms I'm attached to, my vote for this LED class is a "yes".
CONFIG_EMBEDDED does _not_ mean "this is an embedded device" (opposed
to desktop/server/...).
It means "show me some 'use this only if you know what you are doing'
options for additional space savings".
> Jordan Crouse
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
Hi!
> > These solutions are going to end up with a lot of unused led triggers on
> > any given system.
>
> Perhaps a generic solution isn't feasible, because this isn't really a
> generic problem. The LED stuff has very limited use - you mention
> embedded platforms, perhaps they should just be doing this on their
> own?
LEDs need userland API to be useful in some cases (mail LED). At that
point saying "everybody make your own API" is very bad idea.
(And we have old, ARM-private API at least, already. Idea is to do it
once and do it right).
Pavel
--
Thanks, Sharp!
On Wed, 2006-02-01 17:09:06 +0100, Jan Engelhardt <[email protected]> wrote:
> >Add an LED trigger for IDE disk activity to the IDE subsystem.
>
> Since I am not a real user of the led subsystem - what LED should be lit
> anyway? only the motherboard one does, and it seems to be connected
> directly to the IDE chip - I would want this as a compile-time option so I
> don't pay any extra time for any led stuff.
Well, look at PARISC. These machines do have a HDD LED, but it needs
to be manually driven by the I/O subsystem.
The same would be for PCMCIA-attached ATA drives, where some embedded
stuff has a HDD LED, which is GPIO-connected.
MfG, JBG
--
Jan-Benedict Glaw [email protected] . +49-172-7608481 _ O _
"Eine Freie Meinung in einem Freien Kopf | Gegen Zensur | Gegen Krieg _ _ O
für einen Freien Staat voll Freier Bürger" | im Internet! | im Irak! O O O
ret = do_actions((curr | FREE_SPEECH) & ~(NEW_COPYRIGHT_LAW | DRM | TCPA));
Hi,
On Tue, 2006-01-31 at 18:44 +0100, Bartlomiej Zolnierkiewicz wrote:
> > 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.
Not quite as I tried that once and it didn't intercept every
->end_request call. I've just traced this to an explicit call to
ide_end_request() rather than drv->end_request() in ide-taskfile.c
The patch below might or might not be an appropriate fix. With this
applied, the led trigger simplifies to:
http://www.rpsys.net/openzaurus/patches/led_ide-r3.patch
Richard
Ensure ide-taskfile.c calls any driver specific end_request function
if present.
Signed-off-by: Richard Purdie <[email protected]>
Index: linux-2.6.15/drivers/ide/ide-taskfile.c
===================================================================
--- linux-2.6.15.orig/drivers/ide/ide-taskfile.c 2006-01-03 03:21:10.000000000 +0000
+++ linux-2.6.15/drivers/ide/ide-taskfile.c 2006-02-04 14:02:23.000000000 +0000
@@ -372,7 +372,13 @@
}
}
- ide_end_request(drive, 1, rq->hard_nr_sectors);
+ if (rq->rq_disk) {
+ ide_driver_t *drv;
+
+ drv = *(ide_driver_t **)rq->rq_disk->private_data;;
+ drv->end_request(drive, 1, rq->hard_nr_sectors);
+ } else
+ ide_end_request(drive, 1, rq->hard_nr_sectors);
}
/*