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

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

[Richard Purdie]

Add an LED trigger for IDE disk activity to the ide-disk driver.

Signed-off-by: Richard Purdie <rpurdie@rpsys.net>

Index: linux-2.6.15/drivers/ide/ide-disk.c
===================================================================
--- linux-2.6.15.orig/drivers/ide/ide-disk.c	2006-02-04 13:35:37.000000000 +0000
+++ linux-2.6.15/drivers/ide/ide-disk.c	2006-02-04 15:17:18.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
 
@@ -80,6 +81,8 @@
 
 static DECLARE_MUTEX(idedisk_ref_sem);
 
+INIT_LED_TRIGGER(ide_led_trigger);
+
 #define to_ide_disk(obj) container_of(obj, struct ide_disk_obj, kref)
 
 #define ide_disk_g(disk) \
@@ -311,10 +314,12 @@
 
 	if (!blk_fs_request(rq)) {
 		blk_dump_rq_flags(rq, "ide_do_rw_disk - bad command");
-		ide_end_request(drive, 0, 0);
+		ide_end_rw_disk(drive, 0, 0);
 		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);
@@ -325,6 +330,12 @@
 	return __ide_do_rw_disk(drive, rq, block);
 }
 
+static int ide_end_rw_disk(ide_drive_t *drive, int uptodate, int nr_sectors)
+{
+	led_trigger_event(ide_led_trigger, LED_OFF);
+	ide_end_request(drive, uptodate, nr_sectors);
+}
+
 /*
  * Queries for true maximum capacity of the drive.
  * Returns maximum LBA address (> 0) of the drive, 0 if failed.
@@ -1097,7 +1108,7 @@
 	.media			= ide_disk,
 	.supports_dsc_overlap	= 0,
 	.do_request		= ide_do_rw_disk,
-	.end_request		= ide_end_request,
+	.end_request		= ide_end_rw_disk,
 	.error			= __ide_error,
 	.abort			= __ide_abort,
 	.proc			= idedisk_proc,
@@ -1259,11 +1270,13 @@
 
 static void __exit idedisk_exit (void)
 {
+	led_trigger_unregister_simple(ide_led_trigger);
 	driver_unregister(&idedisk_driver.gen_driver);
 }
 
 static int __init idedisk_init(void)
 {
+	led_trigger_register_simple("ide-disk", &ide_led_trigger);
 	return driver_register(&idedisk_driver.gen_driver);
 }
 

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

[Bartlomiej Zolnierkiewicz]

Hi,

Generally it looks fine, some minor comments below.

On 2/5/06, Richard Purdie <rpurdie@rpsys.net> wrote:
> Add an LED trigger for IDE disk activity to the ide-disk driver.

filesystem activity

> Signed-off-by: Richard Purdie <rpurdie@rpsys.net>
>
> Index: linux-2.6.15/drivers/ide/ide-disk.c
> ===================================================================
> --- linux-2.6.15.orig/drivers/ide/ide-disk.c    2006-02-04 13:35:37.000000000 +0000
> +++ linux-2.6.15/drivers/ide/ide-disk.c 2006-02-04 15:17:18.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
>
> @@ -80,6 +81,8 @@
>
>  static DECLARE_MUTEX(idedisk_ref_sem);
>
> +INIT_LED_TRIGGER(ide_led_trigger);
> +
>  #define to_ide_disk(obj) container_of(obj, struct ide_disk_obj, kref)
>
>  #define ide_disk_g(disk) \
> @@ -311,10 +314,12 @@
>
>         if (!blk_fs_request(rq)) {
>                 blk_dump_rq_flags(rq, "ide_do_rw_disk - bad command");
> -               ide_end_request(drive, 0, 0);
> +               ide_end_rw_disk(drive, 0, 0);
>                 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);
> @@ -325,6 +330,12 @@
>         return __ide_do_rw_disk(drive, rq, block);
>  }
>
> +static int ide_end_rw_disk(ide_drive_t *drive, int uptodate, int nr_sectors)

ide_end_rw_disk() is used before it is declared.

Does it compile?

> +{
> +       led_trigger_event(ide_led_trigger, LED_OFF);

It should check for blk_fs_request().

->end_request() can be used for other request types.

> +       ide_end_request(drive, uptodate, nr_sectors);
> +}
> +
>  /*
>   * Queries for true maximum capacity of the drive.
>   * Returns maximum LBA address (> 0) of the drive, 0 if failed.
> @@ -1097,7 +1108,7 @@
>         .media                  = ide_disk,
>         .supports_dsc_overlap   = 0,
>         .do_request             = ide_do_rw_disk,
> -       .end_request            = ide_end_request,
> +       .end_request            = ide_end_rw_disk,
>         .error                  = __ide_error,
>         .abort                  = __ide_abort,
>         .proc                   = idedisk_proc,
> @@ -1259,11 +1270,13 @@
>
>  static void __exit idedisk_exit (void)
>  {
> +       led_trigger_unregister_simple(ide_led_trigger);
>         driver_unregister(&idedisk_driver.gen_driver);

Shouldn't ordering be reverse to this in idedisk_init()?
First driver_unregister(), then led_trigger_unregister_simple()?

>  }
>
>  static int __init idedisk_init(void)
>  {
> +       led_trigger_register_simple("ide-disk", &ide_led_trigger);
>         return driver_register(&idedisk_driver.gen_driver);

What does happen if driver_register() fails?

Bartlomiej

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

[Richard Purdie]

IDE disk activity fixes: 

Catch failure case of driver_register().
Only trigger on blk_fs end requests.

Signed-off-by: Richard Purdie <rpurdie@rpsys.net>

Index: linux-2.6.15/drivers/ide/ide-disk.c
===================================================================
--- linux-2.6.15.orig/drivers/ide/ide-disk.c	2006-02-08 01:09:23.000000000 +0000
+++ linux-2.6.15/drivers/ide/ide-disk.c	2006-02-08 01:08:39.000000000 +0000
@@ -302,7 +302,8 @@
 
 static int ide_end_rw_disk(ide_drive_t *drive, int uptodate, int nr_sectors)
 {
-	led_trigger_event(ide_led_trigger, LED_OFF);
+	if (blk_fs_request(HWGROUP(drive)->rq))
+		led_trigger_event(ide_led_trigger, LED_OFF);
 	return ide_end_request(drive, uptodate, nr_sectors);
 }
 
@@ -1251,8 +1252,10 @@
 
 static int __init idedisk_init(void)
 {
-	led_trigger_register_simple("ide-disk", &ide_led_trigger);
-	return driver_register(&idedisk_driver.gen_driver);
+	int ret = driver_register(&idedisk_driver.gen_driver);
+	if (ret >= 0)
+		led_trigger_register_simple("ide-disk", &ide_led_trigger);
+	return ret;
 }
 
 MODULE_ALIAS("ide:*m-disk*");

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

[Richard Purdie]

On Mon, 2006-02-06 at 17:53 +0100, Bartlomiej Zolnierkiewicz wrote:
> > +{
> > +       led_trigger_event(ide_led_trigger, LED_OFF);
> 
> It should check for blk_fs_request().
> 
> ->end_request() can be used for other request types.
> 
> > +       ide_end_request(drive, uptodate, nr_sectors);
> > +}
> > +
> >
> >  static void __exit idedisk_exit (void)
> >  {
> > +       led_trigger_unregister_simple(ide_led_trigger);
> >         driver_unregister(&idedisk_driver.gen_driver);
> 
> Shouldn't ordering be reverse to this in idedisk_init()?
> First driver_unregister(), then led_trigger_unregister_simple()?

The other issues should have been addressed in -mm now, thanks. An event
call after unregistering a trigger will not trouble the led trigger code
as it was designed to withstand this. In -mm it now matches the order in
the init function but this is purely cosmetic.

Richard