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

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

[Bartlomiej Zolnierkiewicz]

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

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

[Jens Axboe]

On Tue, Jan 31 2006, Bartlomiej Zolnierkiewicz wrote:
> 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 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

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

[Benjamin Herrenschmidt]

> 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

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

[Richard Purdie]

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

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

[Richard Purdie]

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 <rpurdie@rpsys.net>

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);
 }
 
 /*