[PATCH v4 0/5] introduce LED block device activity trigger

[PATCH v4 0/5] introduce LED block device activity trigger

[Akinobu Mita]

This work is inspired by the report on linux-nvme mailing list.

disk-activity trigger not working for nvme disk:
http://lists.infradead.org/pipermail/linux-nvme/2019-July/025253.html

This LED block device activity trigger works with any block devices.

* v4
- Squash patch 'add interface to stop and restart polling disk stats' into
  the ledtrig-blk introduction patch
- Rename 'led' to 'led_trig' in struct ledtrig_blk

* v3
- Avoid the name collision with LED_OFF and LED_ON
- Add ABI documentation
- Add more detail to Kconfig help text

* v2
- Remove "move declaration of led_stop_software_blink() to linux/leds.h" patch
- Move the trigger implementation to drivers/leds/trigger
- s/blk_ledtrig/ledtrig_blk/
- Add CONFIG_LEDS_TRIGGER_BLOCK
- Fix wrong bitops usages
- Add interface to stop and restart polling disk stats
- Stop polling disk stats for scsi disk during runtime suspend

Akinobu Mita (5):
  block: umem: rename LED_* macros to MEMCTRL_LED_*
  scsi: mvsas: rename LED_* enums to SGPIO_LED_*
  scsi: nsp32: rename LED_* macros to EXT_PORT_LED_*
  block: introduce LED block device activity trigger
  scsi: sd: stop polling disk stats by ledtrig-blk during runtime
    suspend

 .../ABI/testing/sysfs-class-led-trigger-blk        |  37 +++
 block/genhd.c                                      |   2 +
 drivers/block/umem.c                               |  20 +-
 drivers/block/umem.h                               |  20 +-
 drivers/leds/trigger/Kconfig                       |   9 +
 drivers/leds/trigger/Makefile                      |   1 +
 drivers/leds/trigger/ledtrig-blk.c                 | 259 +++++++++++++++++++++
 drivers/scsi/mvsas/mv_94xx.c                       |   2 +-
 drivers/scsi/mvsas/mv_94xx.h                       |  24 +-
 drivers/scsi/nsp32.c                               |   7 +-
 drivers/scsi/nsp32.h                               |   4 +-
 drivers/scsi/sd.c                                  |  40 ++--
 include/linux/genhd.h                              |   3 +
 include/linux/leds.h                               |  38 +++
 14 files changed, 411 insertions(+), 55 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-blk
 create mode 100644 drivers/leds/trigger/ledtrig-blk.c

Cc: Frank Steiner <fsteiner-mail1@bio.ifi.lmu.de>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Dan Murphy <dmurphy@ti.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: GOTO Masanori <gotom@debian.or.jp>
Cc: YOKOTA Hiroshi <yokota@netlab.is.tsukuba.ac.jp>
Cc: Hannes Reinecke <hare@suse.com>
-- 
2.7.4

[PATCH v4 0/5] introduce LED block device activity trigger

[Akinobu Mita]

This work is inspired by the report on linux-nvme mailing list.

disk-activity trigger not working for nvme disk:
http://lists.infradead.org/pipermail/linux-nvme/2019-July/025253.html

This LED block device activity trigger works with any block devices.

* v4
- Squash patch 'add interface to stop and restart polling disk stats' into
  the ledtrig-blk introduction patch
- Rename 'led' to 'led_trig' in struct ledtrig_blk

* v3
- Avoid the name collision with LED_OFF and LED_ON
- Add ABI documentation
- Add more detail to Kconfig help text

* v2
- Remove "move declaration of led_stop_software_blink() to linux/leds.h" patch
- Move the trigger implementation to drivers/leds/trigger
- s/blk_ledtrig/ledtrig_blk/
- Add CONFIG_LEDS_TRIGGER_BLOCK
- Fix wrong bitops usages
- Add interface to stop and restart polling disk stats
- Stop polling disk stats for scsi disk during runtime suspend

Akinobu Mita (5):
  block: umem: rename LED_* macros to MEMCTRL_LED_*
  scsi: mvsas: rename LED_* enums to SGPIO_LED_*
  scsi: nsp32: rename LED_* macros to EXT_PORT_LED_*
  block: introduce LED block device activity trigger
  scsi: sd: stop polling disk stats by ledtrig-blk during runtime
    suspend

 .../ABI/testing/sysfs-class-led-trigger-blk        |  37 +++
 block/genhd.c                                      |   2 +
 drivers/block/umem.c                               |  20 +-
 drivers/block/umem.h                               |  20 +-
 drivers/leds/trigger/Kconfig                       |   9 +
 drivers/leds/trigger/Makefile                      |   1 +
 drivers/leds/trigger/ledtrig-blk.c                 | 259 +++++++++++++++++++++
 drivers/scsi/mvsas/mv_94xx.c                       |   2 +-
 drivers/scsi/mvsas/mv_94xx.h                       |  24 +-
 drivers/scsi/nsp32.c                               |   7 +-
 drivers/scsi/nsp32.h                               |   4 +-
 drivers/scsi/sd.c                                  |  40 ++--
 include/linux/genhd.h                              |   3 +
 include/linux/leds.h                               |  38 +++
 14 files changed, 411 insertions(+), 55 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-blk
 create mode 100644 drivers/leds/trigger/ledtrig-blk.c

Cc: Frank Steiner <fsteiner-mail1 at bio.ifi.lmu.de>
Cc: Jacek Anaszewski <jacek.anaszewski at gmail.com>
Cc: Pavel Machek <pavel at ucw.cz>
Cc: Dan Murphy <dmurphy at ti.com>
Cc: Jens Axboe <axboe at kernel.dk>
Cc: "James E.J. Bottomley" <jejb at linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen at oracle.com>
Cc: GOTO Masanori <gotom at debian.or.jp>
Cc: YOKOTA Hiroshi <yokota at netlab.is.tsukuba.ac.jp>
Cc: Hannes Reinecke <hare at suse.com>
-- 
2.7.4

[PATCH v4 1/5] block: umem: rename LED_* macros to MEMCTRL_LED_*

[Akinobu Mita]

The umem driver defines LED_* macros for MEMCTRLCMD_LEDCTRL register
values.  The LED_OFF and LED_ON macros conflict with the LED subsystem's
LED_OFF and LED_ON enums.

This renames these LED_* macros to MEMCTRL_LED_* in umem driver.

Cc: Frank Steiner <fsteiner-mail1@bio.ifi.lmu.de>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Dan Murphy <dmurphy@ti.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Hannes Reinecke <hare@suse.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/block/umem.c | 20 ++++++++++----------
 drivers/block/umem.h | 20 ++++++++++----------
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/block/umem.c b/drivers/block/umem.c
index 1f3f9e0..1109308 100644
--- a/drivers/block/umem.c
+++ b/drivers/block/umem.c
@@ -167,14 +167,14 @@ static int set_userbit(struct cardinfo *card, int bit, unsigned char state)
 }
 
 /*
- * NOTE: For the power LED, use the LED_POWER_* macros since they differ
+ * NOTE: For the power LED, use the MEMCTRL_LED_POWER_* macros since they differ
  */
 static void set_led(struct cardinfo *card, int shift, unsigned char state)
 {
 	unsigned char led;
 
 	led = readb(card->csr_remap + MEMCTRLCMD_LEDCTRL);
-	if (state == LED_FLIP)
+	if (state == MEMCTRL_LED_FLIP)
 		led ^= (1<<shift);
 	else {
 		led &= ~(0x03 << shift);
@@ -268,7 +268,7 @@ static void mm_start_io(struct cardinfo *card)
 
 
 	if (debug & DEBUG_LED_ON_TRANSFER)
-		set_led(card, LED_REMOVE, LED_ON);
+		set_led(card, MEMCTRL_LED_REMOVE, MEMCTRL_LED_ON);
 
 	desc = &page->desc[page->headcnt];
 	writel(0, card->csr_remap + DMA_PCI_ADDR);
@@ -477,7 +477,7 @@ static void process_page(unsigned long data)
 	}
 
 	if (debug & DEBUG_LED_ON_TRANSFER)
-		set_led(card, LED_REMOVE, LED_OFF);
+		set_led(card, MEMCTRL_LED_REMOVE, MEMCTRL_LED_OFF);
 
 	if (card->check_batteries) {
 		card->check_batteries = 0;
@@ -652,13 +652,13 @@ HW_TRACE(0x36);
 static void set_fault_to_battery_status(struct cardinfo *card)
 {
 	if (card->battery[0].good && card->battery[1].good)
-		set_led(card, LED_FAULT, LED_OFF);
+		set_led(card, MEMCTRL_LED_FAULT, MEMCTRL_LED_OFF);
 	else if (card->battery[0].warned || card->battery[1].warned)
-		set_led(card, LED_FAULT, LED_ON);
+		set_led(card, MEMCTRL_LED_FAULT, MEMCTRL_LED_ON);
 	else if (!card->battery[0].good && !card->battery[1].good)
-		set_led(card, LED_FAULT, LED_FLASH_7_0);
+		set_led(card, MEMCTRL_LED_FAULT, MEMCTRL_LED_FLASH_7_0);
 	else
-		set_led(card, LED_FAULT, LED_FLASH_3_5);
+		set_led(card, MEMCTRL_LED_FAULT, MEMCTRL_LED_FLASH_3_5);
 }
 
 static void init_battery_timer(void);
@@ -919,8 +919,8 @@ static int mm_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	}
 
 	/* Clear the LED's we control */
-	set_led(card, LED_REMOVE, LED_OFF);
-	set_led(card, LED_FAULT, LED_OFF);
+	set_led(card, MEMCTRL_LED_REMOVE, MEMCTRL_LED_OFF);
+	set_led(card, MEMCTRL_LED_FAULT, MEMCTRL_LED_OFF);
 
 	batt_status = readb(card->csr_remap + MEMCTRLSTATUS_BATTERY);
 
diff --git a/drivers/block/umem.h b/drivers/block/umem.h
index 5838497..cc9cb37 100644
--- a/drivers/block/umem.h
+++ b/drivers/block/umem.h
@@ -32,16 +32,16 @@
 #define  MEM_2_GB		0xe0
 
 #define MEMCTRLCMD_LEDCTRL	0x08
-#define  LED_REMOVE		2
-#define  LED_FAULT		4
-#define  LED_POWER		6
-#define	 LED_FLIP		255
-#define  LED_OFF		0x00
-#define  LED_ON			0x01
-#define  LED_FLASH_3_5		0x02
-#define  LED_FLASH_7_0		0x03
-#define  LED_POWER_ON		0x00
-#define  LED_POWER_OFF		0x01
+#define  MEMCTRL_LED_REMOVE	2
+#define  MEMCTRL_LED_FAULT	4
+#define  MEMCTRL_LED_POWER	6
+#define  MEMCTRL_LED_FLIP	255
+#define  MEMCTRL_LED_OFF	0x00
+#define  MEMCTRL_LED_ON		0x01
+#define  MEMCTRL_LED_FLASH_3_5	0x02
+#define  MEMCTRL_LED_FLASH_7_0	0x03
+#define  MEMCTRL_LED_POWER_ON	0x00
+#define  MEMCTRL_LED_POWER_OFF	0x01
 #define  USER_BIT1		0x01
 #define  USER_BIT2		0x02
 
-- 
2.7.4

[PATCH v4 1/5] block: umem: rename LED_* macros to MEMCTRL_LED_*

[Akinobu Mita]

The umem driver defines LED_* macros for MEMCTRLCMD_LEDCTRL register
values.  The LED_OFF and LED_ON macros conflict with the LED subsystem's
LED_OFF and LED_ON enums.

This renames these LED_* macros to MEMCTRL_LED_* in umem driver.

Cc: Frank Steiner <fsteiner-mail1 at bio.ifi.lmu.de>
Cc: Jacek Anaszewski <jacek.anaszewski at gmail.com>
Cc: Pavel Machek <pavel at ucw.cz>
Cc: Dan Murphy <dmurphy at ti.com>
Cc: Jens Axboe <axboe at kernel.dk>
Cc: "James E.J. Bottomley" <jejb at linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen at oracle.com>
Cc: Hannes Reinecke <hare at suse.com>
Acked-by: Pavel Machek <pavel at ucw.cz>
Signed-off-by: Akinobu Mita <akinobu.mita at gmail.com>
---
 drivers/block/umem.c | 20 ++++++++++----------
 drivers/block/umem.h | 20 ++++++++++----------
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/block/umem.c b/drivers/block/umem.c
index 1f3f9e0..1109308 100644
--- a/drivers/block/umem.c
+++ b/drivers/block/umem.c
@@ -167,14 +167,14 @@ static int set_userbit(struct cardinfo *card, int bit, unsigned char state)
 }
 
 /*
- * NOTE: For the power LED, use the LED_POWER_* macros since they differ
+ * NOTE: For the power LED, use the MEMCTRL_LED_POWER_* macros since they differ
  */
 static void set_led(struct cardinfo *card, int shift, unsigned char state)
 {
 	unsigned char led;
 
 	led = readb(card->csr_remap + MEMCTRLCMD_LEDCTRL);
-	if (state == LED_FLIP)
+	if (state == MEMCTRL_LED_FLIP)
 		led ^= (1<<shift);
 	else {
 		led &= ~(0x03 << shift);
@@ -268,7 +268,7 @@ static void mm_start_io(struct cardinfo *card)
 
 
 	if (debug & DEBUG_LED_ON_TRANSFER)
-		set_led(card, LED_REMOVE, LED_ON);
+		set_led(card, MEMCTRL_LED_REMOVE, MEMCTRL_LED_ON);
 
 	desc = &page->desc[page->headcnt];
 	writel(0, card->csr_remap + DMA_PCI_ADDR);
@@ -477,7 +477,7 @@ static void process_page(unsigned long data)
 	}
 
 	if (debug & DEBUG_LED_ON_TRANSFER)
-		set_led(card, LED_REMOVE, LED_OFF);
+		set_led(card, MEMCTRL_LED_REMOVE, MEMCTRL_LED_OFF);
 
 	if (card->check_batteries) {
 		card->check_batteries = 0;
@@ -652,13 +652,13 @@ HW_TRACE(0x36);
 static void set_fault_to_battery_status(struct cardinfo *card)
 {
 	if (card->battery[0].good && card->battery[1].good)
-		set_led(card, LED_FAULT, LED_OFF);
+		set_led(card, MEMCTRL_LED_FAULT, MEMCTRL_LED_OFF);
 	else if (card->battery[0].warned || card->battery[1].warned)
-		set_led(card, LED_FAULT, LED_ON);
+		set_led(card, MEMCTRL_LED_FAULT, MEMCTRL_LED_ON);
 	else if (!card->battery[0].good && !card->battery[1].good)
-		set_led(card, LED_FAULT, LED_FLASH_7_0);
+		set_led(card, MEMCTRL_LED_FAULT, MEMCTRL_LED_FLASH_7_0);
 	else
-		set_led(card, LED_FAULT, LED_FLASH_3_5);
+		set_led(card, MEMCTRL_LED_FAULT, MEMCTRL_LED_FLASH_3_5);
 }
 
 static void init_battery_timer(void);
@@ -919,8 +919,8 @@ static int mm_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	}
 
 	/* Clear the LED's we control */
-	set_led(card, LED_REMOVE, LED_OFF);
-	set_led(card, LED_FAULT, LED_OFF);
+	set_led(card, MEMCTRL_LED_REMOVE, MEMCTRL_LED_OFF);
+	set_led(card, MEMCTRL_LED_FAULT, MEMCTRL_LED_OFF);
 
 	batt_status = readb(card->csr_remap + MEMCTRLSTATUS_BATTERY);
 
diff --git a/drivers/block/umem.h b/drivers/block/umem.h
index 5838497..cc9cb37 100644
--- a/drivers/block/umem.h
+++ b/drivers/block/umem.h
@@ -32,16 +32,16 @@
 #define  MEM_2_GB		0xe0
 
 #define MEMCTRLCMD_LEDCTRL	0x08
-#define  LED_REMOVE		2
-#define  LED_FAULT		4
-#define  LED_POWER		6
-#define	 LED_FLIP		255
-#define  LED_OFF		0x00
-#define  LED_ON			0x01
-#define  LED_FLASH_3_5		0x02
-#define  LED_FLASH_7_0		0x03
-#define  LED_POWER_ON		0x00
-#define  LED_POWER_OFF		0x01
+#define  MEMCTRL_LED_REMOVE	2
+#define  MEMCTRL_LED_FAULT	4
+#define  MEMCTRL_LED_POWER	6
+#define  MEMCTRL_LED_FLIP	255
+#define  MEMCTRL_LED_OFF	0x00
+#define  MEMCTRL_LED_ON		0x01
+#define  MEMCTRL_LED_FLASH_3_5	0x02
+#define  MEMCTRL_LED_FLASH_7_0	0x03
+#define  MEMCTRL_LED_POWER_ON	0x00
+#define  MEMCTRL_LED_POWER_OFF	0x01
 #define  USER_BIT1		0x01
 #define  USER_BIT2		0x02
 
-- 
2.7.4

[PATCH v4 2/5] scsi: mvsas: rename LED_* enums to SGPIO_LED_*

[Akinobu Mita]

The mvsas driver declares LED_* enums for enum sgpio_led_status. The
LED_OFF and LED_ON enums cause redeclaration of enumerator with the
LED subsystem's LED_OFF and LED_ON enums.

This adds 'SGPIO_' prefix to these enums in mvsas driver.

Cc: Frank Steiner <fsteiner-mail1@bio.ifi.lmu.de>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Dan Murphy <dmurphy@ti.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Hannes Reinecke <hare@suse.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/scsi/mvsas/mv_94xx.c |  2 +-
 drivers/scsi/mvsas/mv_94xx.h | 24 ++++++++++++------------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/mvsas/mv_94xx.c b/drivers/scsi/mvsas/mv_94xx.c
index fc0b8eb..3558a625 100644
--- a/drivers/scsi/mvsas/mv_94xx.c
+++ b/drivers/scsi/mvsas/mv_94xx.c
@@ -1085,7 +1085,7 @@ static int mvs_94xx_gpio_write(struct mvs_prv_info *mvs_prv,
 				block &= ~((0x7 << MVS_SGPIO_DCTRL_ACT_SHIFT)
 					<< driveshift);
 					/* hardwire activity bit to SOF */
-				block |= LED_BLINKA_SOF << (
+				block |= SGPIO_LED_BLINKA_SOF << (
 					MVS_SGPIO_DCTRL_ACT_SHIFT +
 					driveshift);
 				break;
diff --git a/drivers/scsi/mvsas/mv_94xx.h b/drivers/scsi/mvsas/mv_94xx.h
index a243182..2c96ff1 100644
--- a/drivers/scsi/mvsas/mv_94xx.h
+++ b/drivers/scsi/mvsas/mv_94xx.h
@@ -275,23 +275,23 @@ enum sgpio_registers {
 };
 
 enum sgpio_led_status {
-	LED_OFF	= 0,
-	LED_ON	= 1,
-	LED_BLINKA	= 2,
-	LED_BLINKA_INV	= 3,
-	LED_BLINKA_SOF	= 4,
-	LED_BLINKA_EOF	= 5,
-	LED_BLINKB	= 6,
-	LED_BLINKB_INV	= 7,
+	SGPIO_LED_OFF		= 0,
+	SGPIO_LED_ON		= 1,
+	SGPIO_LED_BLINKA	= 2,
+	SGPIO_LED_BLINKA_INV	= 3,
+	SGPIO_LED_BLINKA_SOF	= 4,
+	SGPIO_LED_BLINKA_EOF	= 5,
+	SGPIO_LED_BLINKB	= 6,
+	SGPIO_LED_BLINKB_INV	= 7,
 };
 
-#define DEFAULT_SGPIO_BITS ((LED_BLINKA_SOF << \
+#define DEFAULT_SGPIO_BITS ((SGPIO_LED_BLINKA_SOF << \
 				MVS_SGPIO_DCTRL_ACT_SHIFT) << (8 * 3) | \
-			(LED_BLINKA_SOF << \
+			(SGPIO_LED_BLINKA_SOF << \
 				MVS_SGPIO_DCTRL_ACT_SHIFT) << (8 * 2) | \
-			(LED_BLINKA_SOF << \
+			(SGPIO_LED_BLINKA_SOF << \
 				MVS_SGPIO_DCTRL_ACT_SHIFT) << (8 * 1) | \
-			(LED_BLINKA_SOF << \
+			(SGPIO_LED_BLINKA_SOF << \
 				MVS_SGPIO_DCTRL_ACT_SHIFT) << (8 * 0))
 
 /*
-- 
2.7.4

[PATCH v4 2/5] scsi: mvsas: rename LED_* enums to SGPIO_LED_*

[Akinobu Mita]

The mvsas driver declares LED_* enums for enum sgpio_led_status. The
LED_OFF and LED_ON enums cause redeclaration of enumerator with the
LED subsystem's LED_OFF and LED_ON enums.

This adds 'SGPIO_' prefix to these enums in mvsas driver.

Cc: Frank Steiner <fsteiner-mail1 at bio.ifi.lmu.de>
Cc: Jacek Anaszewski <jacek.anaszewski at gmail.com>
Cc: Pavel Machek <pavel at ucw.cz>
Cc: Dan Murphy <dmurphy at ti.com>
Cc: Jens Axboe <axboe at kernel.dk>
Cc: "James E.J. Bottomley" <jejb at linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen at oracle.com>
Cc: Hannes Reinecke <hare at suse.com>
Reviewed-by: Hannes Reinecke <hare at suse.com>
Acked-by: Pavel Machek <pavel at ucw.cz>
Signed-off-by: Akinobu Mita <akinobu.mita at gmail.com>
---
 drivers/scsi/mvsas/mv_94xx.c |  2 +-
 drivers/scsi/mvsas/mv_94xx.h | 24 ++++++++++++------------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/mvsas/mv_94xx.c b/drivers/scsi/mvsas/mv_94xx.c
index fc0b8eb..3558a625 100644
--- a/drivers/scsi/mvsas/mv_94xx.c
+++ b/drivers/scsi/mvsas/mv_94xx.c
@@ -1085,7 +1085,7 @@ static int mvs_94xx_gpio_write(struct mvs_prv_info *mvs_prv,
 				block &= ~((0x7 << MVS_SGPIO_DCTRL_ACT_SHIFT)
 					<< driveshift);
 					/* hardwire activity bit to SOF */
-				block |= LED_BLINKA_SOF << (
+				block |= SGPIO_LED_BLINKA_SOF << (
 					MVS_SGPIO_DCTRL_ACT_SHIFT +
 					driveshift);
 				break;
diff --git a/drivers/scsi/mvsas/mv_94xx.h b/drivers/scsi/mvsas/mv_94xx.h
index a243182..2c96ff1 100644
--- a/drivers/scsi/mvsas/mv_94xx.h
+++ b/drivers/scsi/mvsas/mv_94xx.h
@@ -275,23 +275,23 @@ enum sgpio_registers {
 };
 
 enum sgpio_led_status {
-	LED_OFF	= 0,
-	LED_ON	= 1,
-	LED_BLINKA	= 2,
-	LED_BLINKA_INV	= 3,
-	LED_BLINKA_SOF	= 4,
-	LED_BLINKA_EOF	= 5,
-	LED_BLINKB	= 6,
-	LED_BLINKB_INV	= 7,
+	SGPIO_LED_OFF		= 0,
+	SGPIO_LED_ON		= 1,
+	SGPIO_LED_BLINKA	= 2,
+	SGPIO_LED_BLINKA_INV	= 3,
+	SGPIO_LED_BLINKA_SOF	= 4,
+	SGPIO_LED_BLINKA_EOF	= 5,
+	SGPIO_LED_BLINKB	= 6,
+	SGPIO_LED_BLINKB_INV	= 7,
 };
 
-#define DEFAULT_SGPIO_BITS ((LED_BLINKA_SOF << \
+#define DEFAULT_SGPIO_BITS ((SGPIO_LED_BLINKA_SOF << \
 				MVS_SGPIO_DCTRL_ACT_SHIFT) << (8 * 3) | \
-			(LED_BLINKA_SOF << \
+			(SGPIO_LED_BLINKA_SOF << \
 				MVS_SGPIO_DCTRL_ACT_SHIFT) << (8 * 2) | \
-			(LED_BLINKA_SOF << \
+			(SGPIO_LED_BLINKA_SOF << \
 				MVS_SGPIO_DCTRL_ACT_SHIFT) << (8 * 1) | \
-			(LED_BLINKA_SOF << \
+			(SGPIO_LED_BLINKA_SOF << \
 				MVS_SGPIO_DCTRL_ACT_SHIFT) << (8 * 0))
 
 /*
-- 
2.7.4

[PATCH v4 3/5] scsi: nsp32: rename LED_* macros to EXT_PORT_LED_*

[Akinobu Mita]

The nsp32 driver defines LED_ON and LED_OFF macros for EXT_PORT_DDR or
EXT_PORT register values.  The LED_OFF and LED_ON macros conflict with
the LED subsystem's LED_OFF and LED_ON enums.

This renames these LED_* macros to EXT_PORT_LED_* in nsp32 driver.

Cc: Frank Steiner <fsteiner-mail1@bio.ifi.lmu.de>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Dan Murphy <dmurphy@ti.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: GOTO Masanori <gotom@debian.or.jp>
Cc: YOKOTA Hiroshi <yokota@netlab.is.tsukuba.ac.jp>
Cc: Hannes Reinecke <hare@suse.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/scsi/nsp32.c | 7 ++++---
 drivers/scsi/nsp32.h | 4 ++--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/nsp32.c b/drivers/scsi/nsp32.c
index 70db792..8170358 100644
--- a/drivers/scsi/nsp32.c
+++ b/drivers/scsi/nsp32.c
@@ -763,7 +763,8 @@ static int nsp32_arbitration(struct scsi_cmnd *SCpnt, unsigned int base)
 	if (arbit & ARBIT_WIN) {
 		/* Arbitration succeeded */
 		SCpnt->result = DID_OK << 16;
-		nsp32_index_write1(base, EXT_PORT, LED_ON); /* PCI LED on */
+		/* PCI LED on */
+		nsp32_index_write1(base, EXT_PORT, EXT_PORT_LED_ON);
 	} else if (arbit & ARBIT_FAIL) {
 		/* Arbitration failed */
 		SCpnt->result = DID_BUS_BUSY << 16;
@@ -1137,8 +1138,8 @@ static int nsp32hw_init(nsp32_hw_data *data)
 	nsp32_write2(base, IRQ_CONTROL, 0);
 
 	/* PCI LED off */
-	nsp32_index_write1(base, EXT_PORT_DDR, LED_OFF);
-	nsp32_index_write1(base, EXT_PORT,     LED_OFF);
+	nsp32_index_write1(base, EXT_PORT_DDR, EXT_PORT_LED_OFF);
+	nsp32_index_write1(base, EXT_PORT,     EXT_PORT_LED_OFF);
 
 	return TRUE;
 }
diff --git a/drivers/scsi/nsp32.h b/drivers/scsi/nsp32.h
index ab0726c..a7553ea 100644
--- a/drivers/scsi/nsp32.h
+++ b/drivers/scsi/nsp32.h
@@ -306,8 +306,8 @@ typedef u16 u16_le;
 
 #define EXT_PORT_DDR		0x02	/* BASE+08, IDX+02, B, R/W */
 #define EXT_PORT		0x03	/* BASE+08, IDX+03, B, R/W */
-# define LED_ON	 (0)
-# define LED_OFF BIT(0)
+# define EXT_PORT_LED_ON	(0)
+# define EXT_PORT_LED_OFF	BIT(0)
 
 #define IRQ_SELECT		0x04	/* BASE+08, IDX+04, W, R/W */
 # define IRQSELECT_RESELECT_IRQ      BIT(0)
-- 
2.7.4

[PATCH v4 3/5] scsi: nsp32: rename LED_* macros to EXT_PORT_LED_*

[Akinobu Mita]

The nsp32 driver defines LED_ON and LED_OFF macros for EXT_PORT_DDR or
EXT_PORT register values.  The LED_OFF and LED_ON macros conflict with
the LED subsystem's LED_OFF and LED_ON enums.

This renames these LED_* macros to EXT_PORT_LED_* in nsp32 driver.

Cc: Frank Steiner <fsteiner-mail1 at bio.ifi.lmu.de>
Cc: Jacek Anaszewski <jacek.anaszewski at gmail.com>
Cc: Pavel Machek <pavel at ucw.cz>
Cc: Dan Murphy <dmurphy at ti.com>
Cc: Jens Axboe <axboe at kernel.dk>
Cc: "James E.J. Bottomley" <jejb at linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen at oracle.com>
Cc: GOTO Masanori <gotom at debian.or.jp>
Cc: YOKOTA Hiroshi <yokota at netlab.is.tsukuba.ac.jp>
Cc: Hannes Reinecke <hare at suse.com>
Reviewed-by: Hannes Reinecke <hare at suse.com>
Acked-by: Pavel Machek <pavel at ucw.cz>
Signed-off-by: Akinobu Mita <akinobu.mita at gmail.com>
---
 drivers/scsi/nsp32.c | 7 ++++---
 drivers/scsi/nsp32.h | 4 ++--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/nsp32.c b/drivers/scsi/nsp32.c
index 70db792..8170358 100644
--- a/drivers/scsi/nsp32.c
+++ b/drivers/scsi/nsp32.c
@@ -763,7 +763,8 @@ static int nsp32_arbitration(struct scsi_cmnd *SCpnt, unsigned int base)
 	if (arbit & ARBIT_WIN) {
 		/* Arbitration succeeded */
 		SCpnt->result = DID_OK << 16;
-		nsp32_index_write1(base, EXT_PORT, LED_ON); /* PCI LED on */
+		/* PCI LED on */
+		nsp32_index_write1(base, EXT_PORT, EXT_PORT_LED_ON);
 	} else if (arbit & ARBIT_FAIL) {
 		/* Arbitration failed */
 		SCpnt->result = DID_BUS_BUSY << 16;
@@ -1137,8 +1138,8 @@ static int nsp32hw_init(nsp32_hw_data *data)
 	nsp32_write2(base, IRQ_CONTROL, 0);
 
 	/* PCI LED off */
-	nsp32_index_write1(base, EXT_PORT_DDR, LED_OFF);
-	nsp32_index_write1(base, EXT_PORT,     LED_OFF);
+	nsp32_index_write1(base, EXT_PORT_DDR, EXT_PORT_LED_OFF);
+	nsp32_index_write1(base, EXT_PORT,     EXT_PORT_LED_OFF);
 
 	return TRUE;
 }
diff --git a/drivers/scsi/nsp32.h b/drivers/scsi/nsp32.h
index ab0726c..a7553ea 100644
--- a/drivers/scsi/nsp32.h
+++ b/drivers/scsi/nsp32.h
@@ -306,8 +306,8 @@ typedef u16 u16_le;
 
 #define EXT_PORT_DDR		0x02	/* BASE+08, IDX+02, B, R/W */
 #define EXT_PORT		0x03	/* BASE+08, IDX+03, B, R/W */
-# define LED_ON	 (0)
-# define LED_OFF BIT(0)
+# define EXT_PORT_LED_ON	(0)
+# define EXT_PORT_LED_OFF	BIT(0)
 
 #define IRQ_SELECT		0x04	/* BASE+08, IDX+04, W, R/W */
 # define IRQSELECT_RESELECT_IRQ      BIT(0)
-- 
2.7.4

[PATCH v4 4/5] block: introduce LED block device activity trigger

[Akinobu Mita]

This allows LEDs to be controlled by block device activity.

We already have ledtrig-disk (LED disk activity trigger), but the lower
level disk drivers need to utilize ledtrig_disk_activity() to make the
LED blink.

The LED block device trigger doesn't require the lower level drivers to
have any instrumentation. The activity is collected by polling the disk
stats.

Example:

echo block-nvme0n1 > /sys/class/leds/diy/trigger

The LED block device activity trigger periodically polls the disk stats
to collect the activity.  However, it is pointless to poll while the
block device is in quiescent state.  So there is an optional interface to
stop and restart polling disk stats for the lower-level block drivers.

Cc: Frank Steiner <fsteiner-mail1@bio.ifi.lmu.de>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Dan Murphy <dmurphy@ti.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Hannes Reinecke <hare@suse.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v4
- Squash patch 'add interface to stop and restart polling disk stats' into
  the ledtrig-blk introduction patch
- Rename 'led' to 'led_trig' in struct ledtrig_blk

 .../ABI/testing/sysfs-class-led-trigger-blk        |  37 +++
 block/genhd.c                                      |   2 +
 drivers/leds/trigger/Kconfig                       |   9 +
 drivers/leds/trigger/Makefile                      |   1 +
 drivers/leds/trigger/ledtrig-blk.c                 | 259 +++++++++++++++++++++
 include/linux/genhd.h                              |   3 +
 include/linux/leds.h                               |  38 +++
 7 files changed, 349 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-blk
 create mode 100644 drivers/leds/trigger/ledtrig-blk.c

diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-blk b/Documentation/ABI/testing/sysfs-class-led-trigger-blk
new file mode 100644
index 0000000..73472c3
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-blk
@@ -0,0 +1,37 @@
+What:		/sys/class/leds/<led>/interval
+Date:		Aug 2019
+KernelVersion:	5.4
+Contact:	linux-leds@vger.kernel.org
+Description:
+		Specifies the duration of the LED blink in milliseconds.
+		Defaults to 50 ms.
+
+What:		/sys/class/leds/<led>/read
+Date:		Aug 2019
+KernelVersion:	5.4
+Contact:	linux-leds@vger.kernel.org
+Description:
+		Signal data read on the block device.
+		If set to 0, the LED will not blink on data read.
+		If set to 1 (default), the LED will blink for the milliseconds
+		specified in interval to signal data read.
+
+What:		/sys/class/leds/<led>/write
+Date:		Aug 2019
+KernelVersion:	5.4
+Contact:	linux-leds@vger.kernel.org
+Description:
+		Signal data written on the block device.
+		If set to 0, the LED will not blink on data written.
+		If set to 1 (default), the LED will blink for the milliseconds
+		specified in interval to signal data written.
+
+What:		/sys/class/leds/<led>/discard
+Date:		Aug 2019
+KernelVersion:	5.4
+Contact:	linux-leds@vger.kernel.org
+Description:
+		Signal data discarded on the block device.
+		If set to 0, the LED will not blink on data discarded.
+		If set to 1 (default), the LED will blink for the milliseconds
+		specified in interval to signal data discarded.
diff --git a/block/genhd.c b/block/genhd.c
index 54f1f0d3..1c68861 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -745,6 +745,7 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
 
 	disk_add_events(disk);
 	blk_integrity_add(disk);
+	ledtrig_blk_register(disk);
 }
 
 void device_add_disk(struct device *parent, struct gendisk *disk,
@@ -766,6 +767,7 @@ void del_gendisk(struct gendisk *disk)
 	struct disk_part_iter piter;
 	struct hd_struct *part;
 
+	ledtrig_blk_unregister(disk);
 	blk_integrity_del(disk);
 	disk_del_events(disk);
 
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index ce9429c..e399a11 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -144,4 +144,13 @@ config LEDS_TRIGGER_AUDIO
 	  the audio mute and mic-mute changes.
 	  If unsure, say N
 
+config LEDS_TRIGGER_BLOCK
+	bool "LED Block device Trigger"
+	depends on BLOCK
+	help
+	  This allows LEDs to be controlled by block device activity.
+	  This trigger doesn't require the lower level drivers to have any
+	  instrumentation. The activity is collected by polling the disk stats.
+	  If unsure, say Y.
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 733a83e..60200eb 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -15,3 +15,4 @@ obj-$(CONFIG_LEDS_TRIGGER_PANIC)	+= ledtrig-panic.o
 obj-$(CONFIG_LEDS_TRIGGER_NETDEV)	+= ledtrig-netdev.o
 obj-$(CONFIG_LEDS_TRIGGER_PATTERN)	+= ledtrig-pattern.o
 obj-$(CONFIG_LEDS_TRIGGER_AUDIO)	+= ledtrig-audio.o
+obj-$(CONFIG_LEDS_TRIGGER_BLOCK)	+= ledtrig-blk.o
diff --git a/drivers/leds/trigger/ledtrig-blk.c b/drivers/leds/trigger/ledtrig-blk.c
new file mode 100644
index 0000000..de05e92
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-blk.c
@@ -0,0 +1,259 @@
+// SPDX-License-Identifier: GPL-2.0
+// LED Kernel Blockdev Trigger
+// Derived from ledtrig-netdev.c
+
+#include <linux/atomic.h>
+#include <linux/genhd.h>
+#include <linux/leds.h>
+#include <linux/workqueue.h>
+#include "../leds.h"
+
+enum ledtrig_blk_attr {
+	LEDTRIG_BLK_READ,
+	LEDTRIG_BLK_WRITE,
+	LEDTRIG_BLK_DISCARD
+};
+
+struct ledtrig_blk_data {
+	struct delayed_work work;
+	struct led_classdev *led_cdev;
+
+	atomic_t interval;
+	u64 last_activity;
+
+	unsigned long mode;
+};
+
+static ssize_t ledtrig_blk_attr_show(struct device *dev, char *buf,
+				     enum ledtrig_blk_attr attr)
+{
+	struct ledtrig_blk_data *trig_data = led_trigger_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", test_bit(attr, &trig_data->mode));
+}
+
+static ssize_t ledtrig_blk_attr_store(struct device *dev, const char *buf,
+				      size_t size, enum ledtrig_blk_attr attr)
+{
+	struct ledtrig_blk_data *trig_data = led_trigger_get_drvdata(dev);
+	unsigned long state;
+	int ret;
+
+	ret = kstrtoul(buf, 0, &state);
+	if (ret)
+		return ret;
+
+	if (state)
+		set_bit(attr, &trig_data->mode);
+	else
+		clear_bit(attr, &trig_data->mode);
+
+	return size;
+}
+
+static ssize_t read_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	return ledtrig_blk_attr_show(dev, buf, LEDTRIG_BLK_READ);
+}
+static ssize_t read_store(struct device *dev, struct device_attribute *attr,
+			  const char *buf, size_t size)
+{
+	return ledtrig_blk_attr_store(dev, buf, size, LEDTRIG_BLK_READ);
+}
+static DEVICE_ATTR_RW(read);
+
+static ssize_t write_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	return ledtrig_blk_attr_show(dev, buf, LEDTRIG_BLK_WRITE);
+}
+static ssize_t write_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t size)
+{
+	return ledtrig_blk_attr_store(dev, buf, size, LEDTRIG_BLK_WRITE);
+}
+static DEVICE_ATTR_RW(write);
+
+static ssize_t discard_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	return ledtrig_blk_attr_show(dev, buf, LEDTRIG_BLK_DISCARD);
+}
+static ssize_t discard_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t size)
+{
+	return ledtrig_blk_attr_store(dev, buf, size, LEDTRIG_BLK_DISCARD);
+}
+static DEVICE_ATTR_RW(discard);
+
+static ssize_t interval_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct ledtrig_blk_data *trig_data = led_trigger_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n",
+		       jiffies_to_msecs(atomic_read(&trig_data->interval)));
+}
+static ssize_t interval_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t size)
+{
+	struct ledtrig_blk_data *trig_data = led_trigger_get_drvdata(dev);
+	unsigned long value;
+	int ret;
+
+	ret = kstrtoul(buf, 0, &value);
+	if (ret)
+		return ret;
+
+	/* impose some basic bounds on the timer interval */
+	if (value >= 5 && value <= 10000) {
+		cancel_delayed_work_sync(&trig_data->work);
+		atomic_set(&trig_data->interval, msecs_to_jiffies(value));
+		schedule_delayed_work(&trig_data->work,
+				      atomic_read(&trig_data->interval) * 2);
+	}
+
+	return size;
+}
+static DEVICE_ATTR_RW(interval);
+
+static struct attribute *ledtrig_blk_attrs[] = {
+	&dev_attr_read.attr,
+	&dev_attr_write.attr,
+	&dev_attr_discard.attr,
+	&dev_attr_interval.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(ledtrig_blk);
+
+static void ledtrig_blk_work(struct work_struct *work)
+{
+	struct ledtrig_blk_data *trig_data =
+		container_of(work, struct ledtrig_blk_data, work.work);
+	struct gendisk *disk = container_of(trig_data->led_cdev->trigger,
+					    struct gendisk, led_trig.trig);
+	u64 activity = 0;
+
+	if (test_bit(LEDTRIG_BLK_READ, &trig_data->mode))
+		activity += part_stat_read(&disk->part0, ios[STAT_READ]);
+	if (test_bit(LEDTRIG_BLK_WRITE, &trig_data->mode))
+		activity += part_stat_read(&disk->part0, ios[STAT_WRITE]);
+	if (test_bit(LEDTRIG_BLK_DISCARD, &trig_data->mode))
+		activity += part_stat_read(&disk->part0, ios[STAT_DISCARD]);
+
+	if (trig_data->last_activity != activity) {
+		unsigned long interval;
+
+		led_stop_software_blink(trig_data->led_cdev);
+		interval = jiffies_to_msecs(atomic_read(&trig_data->interval));
+		led_blink_set_oneshot(trig_data->led_cdev, &interval, &interval,
+				      0);
+
+		trig_data->last_activity = activity;
+	}
+
+	if (atomic_read(&disk->led_trig.enable_count))
+		schedule_delayed_work(&trig_data->work,
+				      atomic_read(&trig_data->interval) * 2);
+}
+
+static int ledtrig_blk_activate(struct led_classdev *led_cdev)
+{
+	struct ledtrig_blk_data *trig_data;
+
+	trig_data = kzalloc(sizeof(*trig_data), GFP_KERNEL);
+	if (!trig_data)
+		return -ENOMEM;
+
+	trig_data->mode = BIT(LEDTRIG_BLK_READ) | BIT(LEDTRIG_BLK_WRITE) |
+			  BIT(LEDTRIG_BLK_DISCARD);
+
+	atomic_set(&trig_data->interval, msecs_to_jiffies(50));
+	trig_data->last_activity = 0;
+	trig_data->led_cdev = led_cdev;
+
+	INIT_DELAYED_WORK(&trig_data->work, ledtrig_blk_work);
+
+	led_set_trigger_data(led_cdev, trig_data);
+
+	schedule_delayed_work(&trig_data->work,
+			      atomic_read(&trig_data->interval) * 2);
+
+	return 0;
+}
+
+static void ledtrig_blk_deactivate(struct led_classdev *led_cdev)
+{
+	struct ledtrig_blk_data *trig_data = led_get_trigger_data(led_cdev);
+
+	cancel_delayed_work_sync(&trig_data->work);
+	kfree(trig_data);
+}
+
+void ledtrig_blk_disable(struct gendisk *disk)
+{
+	if (disk)
+		atomic_dec(&disk->led_trig.enable_count);
+}
+EXPORT_SYMBOL_GPL(ledtrig_blk_disable);
+
+void ledtrig_blk_enable(struct gendisk *disk)
+{
+	struct led_classdev *led_cdev;
+
+	if (!disk)
+		return;
+
+	atomic_inc(&disk->led_trig.enable_count);
+
+	read_lock(&disk->led_trig.trig.leddev_list_lock);
+
+	list_for_each_entry(led_cdev, &disk->led_trig.trig.led_cdevs,
+			    trig_list) {
+		struct ledtrig_blk_data *trig_data =
+			led_get_trigger_data(led_cdev);
+
+		schedule_delayed_work(&trig_data->work,
+				      atomic_read(&trig_data->interval) * 2);
+	}
+
+	read_unlock(&disk->led_trig.trig.leddev_list_lock);
+}
+EXPORT_SYMBOL_GPL(ledtrig_blk_enable);
+
+int ledtrig_blk_register(struct gendisk *disk)
+{
+	int ret;
+
+	disk->led_trig.trig.name = kasprintf(GFP_KERNEL, "block-%s",
+					disk->disk_name);
+	if (!disk->led_trig.trig.name)
+		return -ENOMEM;
+
+	disk->led_trig.trig.activate = ledtrig_blk_activate;
+	disk->led_trig.trig.deactivate = ledtrig_blk_deactivate;
+	disk->led_trig.trig.groups = ledtrig_blk_groups;
+
+	atomic_set(&disk->led_trig.enable_count, 1);
+
+	ret = led_trigger_register(&disk->led_trig.trig);
+	if (ret) {
+		kfree(disk->led_trig.trig.name);
+		disk->led_trig.trig.name = NULL;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(ledtrig_blk_register);
+
+void ledtrig_blk_unregister(struct gendisk *disk)
+{
+	if (!disk->led_trig.trig.name)
+		return;
+
+	led_trigger_unregister(&disk->led_trig.trig);
+	kfree(disk->led_trig.trig.name);
+	disk->led_trig.trig.name = NULL;
+}
+EXPORT_SYMBOL_GPL(ledtrig_blk_unregister);
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 8b5330d..d4fdb21 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -17,6 +17,7 @@
 #include <linux/percpu-refcount.h>
 #include <linux/uuid.h>
 #include <linux/blk_types.h>
+#include <linux/leds.h>
 #include <asm/local.h>
 
 #ifdef CONFIG_BLOCK
@@ -219,6 +220,8 @@ struct gendisk {
 	int node_id;
 	struct badblocks *bb;
 	struct lockdep_map lockdep_map;
+
+	struct ledtrig_blk led_trig;
 };
 
 static inline struct gendisk *part_to_disk(struct hd_struct *part)
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 9b2bf57..fd2eb7c 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -517,4 +517,42 @@ static inline void ledtrig_audio_set(enum led_audio type,
 }
 #endif
 
+struct gendisk;
+
+#ifdef CONFIG_LEDS_TRIGGER_BLOCK
+
+struct ledtrig_blk {
+	struct led_trigger trig;
+	atomic_t enable_count;
+};
+
+void ledtrig_blk_enable(struct gendisk *disk);
+void ledtrig_blk_disable(struct gendisk *disk);
+int ledtrig_blk_register(struct gendisk *disk);
+void ledtrig_blk_unregister(struct gendisk *disk);
+
+#else
+
+struct ledtrig_blk {
+};
+
+static inline void ledtrig_blk_enable(struct gendisk *disk)
+{
+}
+
+static inline void ledtrig_blk_disable(struct gendisk *disk)
+{
+}
+
+static inline int ledtrig_blk_register(struct gendisk *disk)
+{
+	return 0;
+}
+
+static inline void ledtrig_blk_unregister(struct gendisk *disk)
+{
+}
+
+#endif /* CONFIG_LEDS_TRIGGER_BLOCK */
+
 #endif		/* __LINUX_LEDS_H_INCLUDED */
-- 
2.7.4

[PATCH v4 4/5] block: introduce LED block device activity trigger

[Akinobu Mita]

This allows LEDs to be controlled by block device activity.

We already have ledtrig-disk (LED disk activity trigger), but the lower
level disk drivers need to utilize ledtrig_disk_activity() to make the
LED blink.

The LED block device trigger doesn't require the lower level drivers to
have any instrumentation. The activity is collected by polling the disk
stats.

Example:

echo block-nvme0n1 > /sys/class/leds/diy/trigger

The LED block device activity trigger periodically polls the disk stats
to collect the activity.  However, it is pointless to poll while the
block device is in quiescent state.  So there is an optional interface to
stop and restart polling disk stats for the lower-level block drivers.

Cc: Frank Steiner <fsteiner-mail1 at bio.ifi.lmu.de>
Cc: Jacek Anaszewski <jacek.anaszewski at gmail.com>
Cc: Pavel Machek <pavel at ucw.cz>
Cc: Dan Murphy <dmurphy at ti.com>
Cc: Jens Axboe <axboe at kernel.dk>
Cc: "James E.J. Bottomley" <jejb at linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen at oracle.com>
Cc: Hannes Reinecke <hare at suse.com>
Signed-off-by: Akinobu Mita <akinobu.mita at gmail.com>
---
* v4
- Squash patch 'add interface to stop and restart polling disk stats' into
  the ledtrig-blk introduction patch
- Rename 'led' to 'led_trig' in struct ledtrig_blk

 .../ABI/testing/sysfs-class-led-trigger-blk        |  37 +++
 block/genhd.c                                      |   2 +
 drivers/leds/trigger/Kconfig                       |   9 +
 drivers/leds/trigger/Makefile                      |   1 +
 drivers/leds/trigger/ledtrig-blk.c                 | 259 +++++++++++++++++++++
 include/linux/genhd.h                              |   3 +
 include/linux/leds.h                               |  38 +++
 7 files changed, 349 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-blk
 create mode 100644 drivers/leds/trigger/ledtrig-blk.c

diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-blk b/Documentation/ABI/testing/sysfs-class-led-trigger-blk
new file mode 100644
index 0000000..73472c3
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-blk
@@ -0,0 +1,37 @@
+What:		/sys/class/leds/<led>/interval
+Date:		Aug 2019
+KernelVersion:	5.4
+Contact:	linux-leds at vger.kernel.org
+Description:
+		Specifies the duration of the LED blink in milliseconds.
+		Defaults to 50 ms.
+
+What:		/sys/class/leds/<led>/read
+Date:		Aug 2019
+KernelVersion:	5.4
+Contact:	linux-leds at vger.kernel.org
+Description:
+		Signal data read on the block device.
+		If set to 0, the LED will not blink on data read.
+		If set to 1 (default), the LED will blink for the milliseconds
+		specified in interval to signal data read.
+
+What:		/sys/class/leds/<led>/write
+Date:		Aug 2019
+KernelVersion:	5.4
+Contact:	linux-leds at vger.kernel.org
+Description:
+		Signal data written on the block device.
+		If set to 0, the LED will not blink on data written.
+		If set to 1 (default), the LED will blink for the milliseconds
+		specified in interval to signal data written.
+
+What:		/sys/class/leds/<led>/discard
+Date:		Aug 2019
+KernelVersion:	5.4
+Contact:	linux-leds at vger.kernel.org
+Description:
+		Signal data discarded on the block device.
+		If set to 0, the LED will not blink on data discarded.
+		If set to 1 (default), the LED will blink for the milliseconds
+		specified in interval to signal data discarded.
diff --git a/block/genhd.c b/block/genhd.c
index 54f1f0d3..1c68861 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -745,6 +745,7 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
 
 	disk_add_events(disk);
 	blk_integrity_add(disk);
+	ledtrig_blk_register(disk);
 }
 
 void device_add_disk(struct device *parent, struct gendisk *disk,
@@ -766,6 +767,7 @@ void del_gendisk(struct gendisk *disk)
 	struct disk_part_iter piter;
 	struct hd_struct *part;
 
+	ledtrig_blk_unregister(disk);
 	blk_integrity_del(disk);
 	disk_del_events(disk);
 
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index ce9429c..e399a11 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -144,4 +144,13 @@ config LEDS_TRIGGER_AUDIO
 	  the audio mute and mic-mute changes.
 	  If unsure, say N
 
+config LEDS_TRIGGER_BLOCK
+	bool "LED Block device Trigger"
+	depends on BLOCK
+	help
+	  This allows LEDs to be controlled by block device activity.
+	  This trigger doesn't require the lower level drivers to have any
+	  instrumentation. The activity is collected by polling the disk stats.
+	  If unsure, say Y.
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 733a83e..60200eb 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -15,3 +15,4 @@ obj-$(CONFIG_LEDS_TRIGGER_PANIC)	+= ledtrig-panic.o
 obj-$(CONFIG_LEDS_TRIGGER_NETDEV)	+= ledtrig-netdev.o
 obj-$(CONFIG_LEDS_TRIGGER_PATTERN)	+= ledtrig-pattern.o
 obj-$(CONFIG_LEDS_TRIGGER_AUDIO)	+= ledtrig-audio.o
+obj-$(CONFIG_LEDS_TRIGGER_BLOCK)	+= ledtrig-blk.o
diff --git a/drivers/leds/trigger/ledtrig-blk.c b/drivers/leds/trigger/ledtrig-blk.c
new file mode 100644
index 0000000..de05e92
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-blk.c
@@ -0,0 +1,259 @@
+// SPDX-License-Identifier: GPL-2.0
+// LED Kernel Blockdev Trigger
+// Derived from ledtrig-netdev.c
+
+#include <linux/atomic.h>
+#include <linux/genhd.h>
+#include <linux/leds.h>
+#include <linux/workqueue.h>
+#include "../leds.h"
+
+enum ledtrig_blk_attr {
+	LEDTRIG_BLK_READ,
+	LEDTRIG_BLK_WRITE,
+	LEDTRIG_BLK_DISCARD
+};
+
+struct ledtrig_blk_data {
+	struct delayed_work work;
+	struct led_classdev *led_cdev;
+
+	atomic_t interval;
+	u64 last_activity;
+
+	unsigned long mode;
+};
+
+static ssize_t ledtrig_blk_attr_show(struct device *dev, char *buf,
+				     enum ledtrig_blk_attr attr)
+{
+	struct ledtrig_blk_data *trig_data = led_trigger_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", test_bit(attr, &trig_data->mode));
+}
+
+static ssize_t ledtrig_blk_attr_store(struct device *dev, const char *buf,
+				      size_t size, enum ledtrig_blk_attr attr)
+{
+	struct ledtrig_blk_data *trig_data = led_trigger_get_drvdata(dev);
+	unsigned long state;
+	int ret;
+
+	ret = kstrtoul(buf, 0, &state);
+	if (ret)
+		return ret;
+
+	if (state)
+		set_bit(attr, &trig_data->mode);
+	else
+		clear_bit(attr, &trig_data->mode);
+
+	return size;
+}
+
+static ssize_t read_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	return ledtrig_blk_attr_show(dev, buf, LEDTRIG_BLK_READ);
+}
+static ssize_t read_store(struct device *dev, struct device_attribute *attr,
+			  const char *buf, size_t size)
+{
+	return ledtrig_blk_attr_store(dev, buf, size, LEDTRIG_BLK_READ);
+}
+static DEVICE_ATTR_RW(read);
+
+static ssize_t write_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	return ledtrig_blk_attr_show(dev, buf, LEDTRIG_BLK_WRITE);
+}
+static ssize_t write_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t size)
+{
+	return ledtrig_blk_attr_store(dev, buf, size, LEDTRIG_BLK_WRITE);
+}
+static DEVICE_ATTR_RW(write);
+
+static ssize_t discard_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	return ledtrig_blk_attr_show(dev, buf, LEDTRIG_BLK_DISCARD);
+}
+static ssize_t discard_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t size)
+{
+	return ledtrig_blk_attr_store(dev, buf, size, LEDTRIG_BLK_DISCARD);
+}
+static DEVICE_ATTR_RW(discard);
+
+static ssize_t interval_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct ledtrig_blk_data *trig_data = led_trigger_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n",
+		       jiffies_to_msecs(atomic_read(&trig_data->interval)));
+}
+static ssize_t interval_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t size)
+{
+	struct ledtrig_blk_data *trig_data = led_trigger_get_drvdata(dev);
+	unsigned long value;
+	int ret;
+
+	ret = kstrtoul(buf, 0, &value);
+	if (ret)
+		return ret;
+
+	/* impose some basic bounds on the timer interval */
+	if (value >= 5 && value <= 10000) {
+		cancel_delayed_work_sync(&trig_data->work);
+		atomic_set(&trig_data->interval, msecs_to_jiffies(value));
+		schedule_delayed_work(&trig_data->work,
+				      atomic_read(&trig_data->interval) * 2);
+	}
+
+	return size;
+}
+static DEVICE_ATTR_RW(interval);
+
+static struct attribute *ledtrig_blk_attrs[] = {
+	&dev_attr_read.attr,
+	&dev_attr_write.attr,
+	&dev_attr_discard.attr,
+	&dev_attr_interval.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(ledtrig_blk);
+
+static void ledtrig_blk_work(struct work_struct *work)
+{
+	struct ledtrig_blk_data *trig_data =
+		container_of(work, struct ledtrig_blk_data, work.work);
+	struct gendisk *disk = container_of(trig_data->led_cdev->trigger,
+					    struct gendisk, led_trig.trig);
+	u64 activity = 0;
+
+	if (test_bit(LEDTRIG_BLK_READ, &trig_data->mode))
+		activity += part_stat_read(&disk->part0, ios[STAT_READ]);
+	if (test_bit(LEDTRIG_BLK_WRITE, &trig_data->mode))
+		activity += part_stat_read(&disk->part0, ios[STAT_WRITE]);
+	if (test_bit(LEDTRIG_BLK_DISCARD, &trig_data->mode))
+		activity += part_stat_read(&disk->part0, ios[STAT_DISCARD]);
+
+	if (trig_data->last_activity != activity) {
+		unsigned long interval;
+
+		led_stop_software_blink(trig_data->led_cdev);
+		interval = jiffies_to_msecs(atomic_read(&trig_data->interval));
+		led_blink_set_oneshot(trig_data->led_cdev, &interval, &interval,
+				      0);
+
+		trig_data->last_activity = activity;
+	}
+
+	if (atomic_read(&disk->led_trig.enable_count))
+		schedule_delayed_work(&trig_data->work,
+				      atomic_read(&trig_data->interval) * 2);
+}
+
+static int ledtrig_blk_activate(struct led_classdev *led_cdev)
+{
+	struct ledtrig_blk_data *trig_data;
+
+	trig_data = kzalloc(sizeof(*trig_data), GFP_KERNEL);
+	if (!trig_data)
+		return -ENOMEM;
+
+	trig_data->mode = BIT(LEDTRIG_BLK_READ) | BIT(LEDTRIG_BLK_WRITE) |
+			  BIT(LEDTRIG_BLK_DISCARD);
+
+	atomic_set(&trig_data->interval, msecs_to_jiffies(50));
+	trig_data->last_activity = 0;
+	trig_data->led_cdev = led_cdev;
+
+	INIT_DELAYED_WORK(&trig_data->work, ledtrig_blk_work);
+
+	led_set_trigger_data(led_cdev, trig_data);
+
+	schedule_delayed_work(&trig_data->work,
+			      atomic_read(&trig_data->interval) * 2);
+
+	return 0;
+}
+
+static void ledtrig_blk_deactivate(struct led_classdev *led_cdev)
+{
+	struct ledtrig_blk_data *trig_data = led_get_trigger_data(led_cdev);
+
+	cancel_delayed_work_sync(&trig_data->work);
+	kfree(trig_data);
+}
+
+void ledtrig_blk_disable(struct gendisk *disk)
+{
+	if (disk)
+		atomic_dec(&disk->led_trig.enable_count);
+}
+EXPORT_SYMBOL_GPL(ledtrig_blk_disable);
+
+void ledtrig_blk_enable(struct gendisk *disk)
+{
+	struct led_classdev *led_cdev;
+
+	if (!disk)
+		return;
+
+	atomic_inc(&disk->led_trig.enable_count);
+
+	read_lock(&disk->led_trig.trig.leddev_list_lock);
+
+	list_for_each_entry(led_cdev, &disk->led_trig.trig.led_cdevs,
+			    trig_list) {
+		struct ledtrig_blk_data *trig_data =
+			led_get_trigger_data(led_cdev);
+
+		schedule_delayed_work(&trig_data->work,
+				      atomic_read(&trig_data->interval) * 2);
+	}
+
+	read_unlock(&disk->led_trig.trig.leddev_list_lock);
+}
+EXPORT_SYMBOL_GPL(ledtrig_blk_enable);
+
+int ledtrig_blk_register(struct gendisk *disk)
+{
+	int ret;
+
+	disk->led_trig.trig.name = kasprintf(GFP_KERNEL, "block-%s",
+					disk->disk_name);
+	if (!disk->led_trig.trig.name)
+		return -ENOMEM;
+
+	disk->led_trig.trig.activate = ledtrig_blk_activate;
+	disk->led_trig.trig.deactivate = ledtrig_blk_deactivate;
+	disk->led_trig.trig.groups = ledtrig_blk_groups;
+
+	atomic_set(&disk->led_trig.enable_count, 1);
+
+	ret = led_trigger_register(&disk->led_trig.trig);
+	if (ret) {
+		kfree(disk->led_trig.trig.name);
+		disk->led_trig.trig.name = NULL;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(ledtrig_blk_register);
+
+void ledtrig_blk_unregister(struct gendisk *disk)
+{
+	if (!disk->led_trig.trig.name)
+		return;
+
+	led_trigger_unregister(&disk->led_trig.trig);
+	kfree(disk->led_trig.trig.name);
+	disk->led_trig.trig.name = NULL;
+}
+EXPORT_SYMBOL_GPL(ledtrig_blk_unregister);
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 8b5330d..d4fdb21 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -17,6 +17,7 @@
 #include <linux/percpu-refcount.h>
 #include <linux/uuid.h>
 #include <linux/blk_types.h>
+#include <linux/leds.h>
 #include <asm/local.h>
 
 #ifdef CONFIG_BLOCK
@@ -219,6 +220,8 @@ struct gendisk {
 	int node_id;
 	struct badblocks *bb;
 	struct lockdep_map lockdep_map;
+
+	struct ledtrig_blk led_trig;
 };
 
 static inline struct gendisk *part_to_disk(struct hd_struct *part)
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 9b2bf57..fd2eb7c 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -517,4 +517,42 @@ static inline void ledtrig_audio_set(enum led_audio type,
 }
 #endif
 
+struct gendisk;
+
+#ifdef CONFIG_LEDS_TRIGGER_BLOCK
+
+struct ledtrig_blk {
+	struct led_trigger trig;
+	atomic_t enable_count;
+};
+
+void ledtrig_blk_enable(struct gendisk *disk);
+void ledtrig_blk_disable(struct gendisk *disk);
+int ledtrig_blk_register(struct gendisk *disk);
+void ledtrig_blk_unregister(struct gendisk *disk);
+
+#else
+
+struct ledtrig_blk {
+};
+
+static inline void ledtrig_blk_enable(struct gendisk *disk)
+{
+}
+
+static inline void ledtrig_blk_disable(struct gendisk *disk)
+{
+}
+
+static inline int ledtrig_blk_register(struct gendisk *disk)
+{
+	return 0;
+}
+
+static inline void ledtrig_blk_unregister(struct gendisk *disk)
+{
+}
+
+#endif /* CONFIG_LEDS_TRIGGER_BLOCK */
+
 #endif		/* __LINUX_LEDS_H_INCLUDED */
-- 
2.7.4

[PATCH v4 5/5] scsi: sd: stop polling disk stats by ledtrig-blk during runtime suspend

[Akinobu Mita]

The LED block device activity trigger periodically polls the disk stats
to collect the activity.  However, it is pointless to poll while the
scsi device is in runtime suspend.

This stops polling disk stats when the device is successfully runtime
suspended, and restarts polling when the device is successfully runtime
resumed.

Cc: Frank Steiner <fsteiner-mail1@bio.ifi.lmu.de>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Dan Murphy <dmurphy@ti.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Hannes Reinecke <hare@suse.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/scsi/sd.c | 40 +++++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 149d406..5f73142 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3538,7 +3538,7 @@ static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
 {
 	struct scsi_disk *sdkp = dev_get_drvdata(dev);
 	struct scsi_sense_hdr sshdr;
-	int ret = 0;
+	int ret;
 
 	if (!sdkp)	/* E.g.: runtime suspend following sd_remove() */
 		return 0;
@@ -3550,18 +3550,16 @@ static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
 		if (ret) {
 			/* ignore OFFLINE device */
 			if (ret == -ENODEV)
-				return 0;
-
-			if (!scsi_sense_valid(&sshdr) ||
-			    sshdr.sense_key != ILLEGAL_REQUEST)
-				return ret;
+				goto success;
 
 			/*
 			 * sshdr.sense_key == ILLEGAL_REQUEST means this drive
 			 * doesn't support sync. There's not much to do and
 			 * suspend shouldn't fail.
 			 */
-			ret = 0;
+			if (!scsi_sense_valid(&sshdr) ||
+			    sshdr.sense_key != ILLEGAL_REQUEST)
+				return ret;
 		}
 	}
 
@@ -3569,11 +3567,14 @@ static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
 		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
 		/* an error is not worth aborting a system sleep */
 		ret = sd_start_stop_device(sdkp, 0);
-		if (ignore_stop_errors)
-			ret = 0;
+		if (ret && !ignore_stop_errors)
+			return ret;
 	}
 
-	return ret;
+success:
+	ledtrig_blk_disable(sdkp->disk);
+
+	return 0;
 }
 
 static int sd_suspend_system(struct device *dev)
@@ -3589,19 +3590,24 @@ static int sd_suspend_runtime(struct device *dev)
 static int sd_resume(struct device *dev)
 {
 	struct scsi_disk *sdkp = dev_get_drvdata(dev);
-	int ret;
 
 	if (!sdkp)	/* E.g.: runtime resume at the start of sd_probe() */
 		return 0;
 
-	if (!sdkp->device->manage_start_stop)
-		return 0;
+	if (sdkp->device->manage_start_stop) {
+		int ret;
+
+		sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
+		ret = sd_start_stop_device(sdkp, 1);
+		if (ret)
+			return ret;
 
-	sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
-	ret = sd_start_stop_device(sdkp, 1);
-	if (!ret)
 		opal_unlock_from_suspend(sdkp->opal_dev);
-	return ret;
+	}
+
+	ledtrig_blk_enable(sdkp->disk);
+
+	return 0;
 }
 
 /**
-- 
2.7.4

[PATCH v4 5/5] scsi: sd: stop polling disk stats by ledtrig-blk during runtime suspend

[Akinobu Mita]

The LED block device activity trigger periodically polls the disk stats
to collect the activity.  However, it is pointless to poll while the
scsi device is in runtime suspend.

This stops polling disk stats when the device is successfully runtime
suspended, and restarts polling when the device is successfully runtime
resumed.

Cc: Frank Steiner <fsteiner-mail1 at bio.ifi.lmu.de>
Cc: Jacek Anaszewski <jacek.anaszewski at gmail.com>
Cc: Pavel Machek <pavel at ucw.cz>
Cc: Dan Murphy <dmurphy at ti.com>
Cc: Jens Axboe <axboe at kernel.dk>
Cc: "James E.J. Bottomley" <jejb at linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen at oracle.com>
Cc: Hannes Reinecke <hare at suse.com>
Signed-off-by: Akinobu Mita <akinobu.mita at gmail.com>
---
 drivers/scsi/sd.c | 40 +++++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 149d406..5f73142 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3538,7 +3538,7 @@ static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
 {
 	struct scsi_disk *sdkp = dev_get_drvdata(dev);
 	struct scsi_sense_hdr sshdr;
-	int ret = 0;
+	int ret;
 
 	if (!sdkp)	/* E.g.: runtime suspend following sd_remove() */
 		return 0;
@@ -3550,18 +3550,16 @@ static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
 		if (ret) {
 			/* ignore OFFLINE device */
 			if (ret == -ENODEV)
-				return 0;
-
-			if (!scsi_sense_valid(&sshdr) ||
-			    sshdr.sense_key != ILLEGAL_REQUEST)
-				return ret;
+				goto success;
 
 			/*
 			 * sshdr.sense_key == ILLEGAL_REQUEST means this drive
 			 * doesn't support sync. There's not much to do and
 			 * suspend shouldn't fail.
 			 */
-			ret = 0;
+			if (!scsi_sense_valid(&sshdr) ||
+			    sshdr.sense_key != ILLEGAL_REQUEST)
+				return ret;
 		}
 	}
 
@@ -3569,11 +3567,14 @@ static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
 		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
 		/* an error is not worth aborting a system sleep */
 		ret = sd_start_stop_device(sdkp, 0);
-		if (ignore_stop_errors)
-			ret = 0;
+		if (ret && !ignore_stop_errors)
+			return ret;
 	}
 
-	return ret;
+success:
+	ledtrig_blk_disable(sdkp->disk);
+
+	return 0;
 }
 
 static int sd_suspend_system(struct device *dev)
@@ -3589,19 +3590,24 @@ static int sd_suspend_runtime(struct device *dev)
 static int sd_resume(struct device *dev)
 {
 	struct scsi_disk *sdkp = dev_get_drvdata(dev);
-	int ret;
 
 	if (!sdkp)	/* E.g.: runtime resume at the start of sd_probe() */
 		return 0;
 
-	if (!sdkp->device->manage_start_stop)
-		return 0;
+	if (sdkp->device->manage_start_stop) {
+		int ret;
+
+		sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
+		ret = sd_start_stop_device(sdkp, 1);
+		if (ret)
+			return ret;
 
-	sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
-	ret = sd_start_stop_device(sdkp, 1);
-	if (!ret)
 		opal_unlock_from_suspend(sdkp->opal_dev);
-	return ret;
+	}
+
+	ledtrig_blk_enable(sdkp->disk);
+
+	return 0;
 }
 
 /**
-- 
2.7.4

Re: [PATCH v4 5/5] scsi: sd: stop polling disk stats by ledtrig-blk during runtime suspend

[Jacek Anaszewski]

Hi Akinobu,

Thank you for the update.

Previously I forgot to mention one more thing - this patch does more
than it declares in the commit message, i.e. in addition to what is
declared it uses new ledtrig-blk trigger by calling
ledtrig_blk_enable()/ledtrig_blk_disable().

Those should be definitely split into a separate patch, preceding the
changes required for stopping polling disk stats.

Best regards,
Jacek Anaszewski

On 8/15/19 6:59 PM, Akinobu Mita wrote:
> The LED block device activity trigger periodically polls the disk stats
> to collect the activity.  However, it is pointless to poll while the
> scsi device is in runtime suspend.
> 
> This stops polling disk stats when the device is successfully runtime
> suspended, and restarts polling when the device is successfully runtime
> resumed.
> 
> Cc: Frank Steiner <fsteiner-mail1@bio.ifi.lmu.de>
> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Dan Murphy <dmurphy@ti.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  drivers/scsi/sd.c | 40 +++++++++++++++++++++++-----------------
>  1 file changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 149d406..5f73142 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3538,7 +3538,7 @@ static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
>  {
>  	struct scsi_disk *sdkp = dev_get_drvdata(dev);
>  	struct scsi_sense_hdr sshdr;
> -	int ret = 0;
> +	int ret;
>  
>  	if (!sdkp)	/* E.g.: runtime suspend following sd_remove() */
>  		return 0;
> @@ -3550,18 +3550,16 @@ static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
>  		if (ret) {
>  			/* ignore OFFLINE device */
>  			if (ret == -ENODEV)
> -				return 0;
> -
> -			if (!scsi_sense_valid(&sshdr) ||
> -			    sshdr.sense_key != ILLEGAL_REQUEST)
> -				return ret;
> +				goto success;
>  
>  			/*
>  			 * sshdr.sense_key == ILLEGAL_REQUEST means this drive
>  			 * doesn't support sync. There's not much to do and
>  			 * suspend shouldn't fail.
>  			 */
> -			ret = 0;
> +			if (!scsi_sense_valid(&sshdr) ||
> +			    sshdr.sense_key != ILLEGAL_REQUEST)
> +				return ret;
>  		}
>  	}
>  
> @@ -3569,11 +3567,14 @@ static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
>  		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
>  		/* an error is not worth aborting a system sleep */
>  		ret = sd_start_stop_device(sdkp, 0);
> -		if (ignore_stop_errors)
> -			ret = 0;
> +		if (ret && !ignore_stop_errors)
> +			return ret;
>  	}
>  
> -	return ret;
> +success:
> +	ledtrig_blk_disable(sdkp->disk);
> +
> +	return 0;
>  }
>  
>  static int sd_suspend_system(struct device *dev)
> @@ -3589,19 +3590,24 @@ static int sd_suspend_runtime(struct device *dev)
>  static int sd_resume(struct device *dev)
>  {
>  	struct scsi_disk *sdkp = dev_get_drvdata(dev);
> -	int ret;
>  
>  	if (!sdkp)	/* E.g.: runtime resume at the start of sd_probe() */
>  		return 0;
>  
> -	if (!sdkp->device->manage_start_stop)
> -		return 0;
> +	if (sdkp->device->manage_start_stop) {
> +		int ret;
> +
> +		sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
> +		ret = sd_start_stop_device(sdkp, 1);
> +		if (ret)
> +			return ret;
>  
> -	sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
> -	ret = sd_start_stop_device(sdkp, 1);
> -	if (!ret)
>  		opal_unlock_from_suspend(sdkp->opal_dev);
> -	return ret;
> +	}
> +
> +	ledtrig_blk_enable(sdkp->disk);
> +
> +	return 0;
>  }
>  
>  /**
> 

[PATCH v4 5/5] scsi: sd: stop polling disk stats by ledtrig-blk during runtime suspend

[Jacek Anaszewski]

Hi Akinobu,

Thank you for the update.

Previously I forgot to mention one more thing - this patch does more
than it declares in the commit message, i.e. in addition to what is
declared it uses new ledtrig-blk trigger by calling
ledtrig_blk_enable()/ledtrig_blk_disable().

Those should be definitely split into a separate patch, preceding the
changes required for stopping polling disk stats.

Best regards,
Jacek Anaszewski

On 8/15/19 6:59 PM, Akinobu Mita wrote:
> The LED block device activity trigger periodically polls the disk stats
> to collect the activity.  However, it is pointless to poll while the
> scsi device is in runtime suspend.
> 
> This stops polling disk stats when the device is successfully runtime
> suspended, and restarts polling when the device is successfully runtime
> resumed.
> 
> Cc: Frank Steiner <fsteiner-mail1 at bio.ifi.lmu.de>
> Cc: Jacek Anaszewski <jacek.anaszewski at gmail.com>
> Cc: Pavel Machek <pavel at ucw.cz>
> Cc: Dan Murphy <dmurphy at ti.com>
> Cc: Jens Axboe <axboe at kernel.dk>
> Cc: "James E.J. Bottomley" <jejb at linux.ibm.com>
> Cc: "Martin K. Petersen" <martin.petersen at oracle.com>
> Cc: Hannes Reinecke <hare at suse.com>
> Signed-off-by: Akinobu Mita <akinobu.mita at gmail.com>
> ---
>  drivers/scsi/sd.c | 40 +++++++++++++++++++++++-----------------
>  1 file changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 149d406..5f73142 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3538,7 +3538,7 @@ static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
>  {
>  	struct scsi_disk *sdkp = dev_get_drvdata(dev);
>  	struct scsi_sense_hdr sshdr;
> -	int ret = 0;
> +	int ret;
>  
>  	if (!sdkp)	/* E.g.: runtime suspend following sd_remove() */
>  		return 0;
> @@ -3550,18 +3550,16 @@ static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
>  		if (ret) {
>  			/* ignore OFFLINE device */
>  			if (ret == -ENODEV)
> -				return 0;
> -
> -			if (!scsi_sense_valid(&sshdr) ||
> -			    sshdr.sense_key != ILLEGAL_REQUEST)
> -				return ret;
> +				goto success;
>  
>  			/*
>  			 * sshdr.sense_key == ILLEGAL_REQUEST means this drive
>  			 * doesn't support sync. There's not much to do and
>  			 * suspend shouldn't fail.
>  			 */
> -			ret = 0;
> +			if (!scsi_sense_valid(&sshdr) ||
> +			    sshdr.sense_key != ILLEGAL_REQUEST)
> +				return ret;
>  		}
>  	}
>  
> @@ -3569,11 +3567,14 @@ static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
>  		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
>  		/* an error is not worth aborting a system sleep */
>  		ret = sd_start_stop_device(sdkp, 0);
> -		if (ignore_stop_errors)
> -			ret = 0;
> +		if (ret && !ignore_stop_errors)
> +			return ret;
>  	}
>  
> -	return ret;
> +success:
> +	ledtrig_blk_disable(sdkp->disk);
> +
> +	return 0;
>  }
>  
>  static int sd_suspend_system(struct device *dev)
> @@ -3589,19 +3590,24 @@ static int sd_suspend_runtime(struct device *dev)
>  static int sd_resume(struct device *dev)
>  {
>  	struct scsi_disk *sdkp = dev_get_drvdata(dev);
> -	int ret;
>  
>  	if (!sdkp)	/* E.g.: runtime resume at the start of sd_probe() */
>  		return 0;
>  
> -	if (!sdkp->device->manage_start_stop)
> -		return 0;
> +	if (sdkp->device->manage_start_stop) {
> +		int ret;
> +
> +		sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
> +		ret = sd_start_stop_device(sdkp, 1);
> +		if (ret)
> +			return ret;
>  
> -	sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
> -	ret = sd_start_stop_device(sdkp, 1);
> -	if (!ret)
>  		opal_unlock_from_suspend(sdkp->opal_dev);
> -	return ret;
> +	}
> +
> +	ledtrig_blk_enable(sdkp->disk);
> +
> +	return 0;
>  }
>  
>  /**
> 

Re: [PATCH v4 4/5] block: introduce LED block device activity trigger

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]

On Fri 2019-08-16 01:59:58, Akinobu Mita wrote:
> This allows LEDs to be controlled by block device activity.
> 
> We already have ledtrig-disk (LED disk activity trigger), but the lower
> level disk drivers need to utilize ledtrig_disk_activity() to make the
> LED blink.
> 
> The LED block device trigger doesn't require the lower level drivers to
> have any instrumentation. The activity is collected by polling the disk
> stats.
> 
> Example:
> 
> echo block-nvme0n1 > /sys/class/leds/diy/trigger

Lets use one trigger "block" and have the device as a parameter,
please.

We already have 1000 cpu triggers on 1000 cpu machines, and yes, its a
disaster we'll need to fix. Lets not repeat the same mistake here.

I guess it may be slightly more work. Sorry about that.

								Pavel

> +++ b/include/linux/leds.h
> +#else
> +
> +struct ledtrig_blk {
> +};
> +

Is the empty struct neccessary?

> +static inline void ledtrig_blk_enable(struct gendisk *disk)
> +{
> +}
> +
> +static inline void ledtrig_blk_disable(struct gendisk *disk)
> +{
> +}
> +
> +static inline int ledtrig_blk_register(struct gendisk *disk)
> +{
> +	return 0;
> +}
> +
> +static inline void ledtrig_blk_unregister(struct gendisk *disk)
> +{
> +}

Normally we put such empty functions on single lines...

Best regards,
									     Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH v4 4/5] block: introduce LED block device activity trigger

[Pavel Machek]

On Fri 2019-08-16 01:59:58, Akinobu Mita wrote:
> This allows LEDs to be controlled by block device activity.
> 
> We already have ledtrig-disk (LED disk activity trigger), but the lower
> level disk drivers need to utilize ledtrig_disk_activity() to make the
> LED blink.
> 
> The LED block device trigger doesn't require the lower level drivers to
> have any instrumentation. The activity is collected by polling the disk
> stats.
> 
> Example:
> 
> echo block-nvme0n1 > /sys/class/leds/diy/trigger

Lets use one trigger "block" and have the device as a parameter,
please.

We already have 1000 cpu triggers on 1000 cpu machines, and yes, its a
disaster we'll need to fix. Lets not repeat the same mistake here.

I guess it may be slightly more work. Sorry about that.

								Pavel

> +++ b/include/linux/leds.h
> +#else
> +
> +struct ledtrig_blk {
> +};
> +

Is the empty struct neccessary?

> +static inline void ledtrig_blk_enable(struct gendisk *disk)
> +{
> +}
> +
> +static inline void ledtrig_blk_disable(struct gendisk *disk)
> +{
> +}
> +
> +static inline int ledtrig_blk_register(struct gendisk *disk)
> +{
> +	return 0;
> +}
> +
> +static inline void ledtrig_blk_unregister(struct gendisk *disk)
> +{
> +}

Normally we put such empty functions on single lines...

Best regards,
									     Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20190817/55367839/attachment.sig>

Re: [PATCH v4 4/5] block: introduce LED block device activity trigger

[Jacek Anaszewski]

On 8/17/19 4:55 PM, Pavel Machek wrote:
> On Fri 2019-08-16 01:59:58, Akinobu Mita wrote:
>> This allows LEDs to be controlled by block device activity.
>>
>> We already have ledtrig-disk (LED disk activity trigger), but the lower
>> level disk drivers need to utilize ledtrig_disk_activity() to make the
>> LED blink.
>>
>> The LED block device trigger doesn't require the lower level drivers to
>> have any instrumentation. The activity is collected by polling the disk
>> stats.
>>
>> Example:
>>
>> echo block-nvme0n1 > /sys/class/leds/diy/trigger
> 
> Lets use one trigger "block" and have the device as a parameter,
> please.
> 
> We already have 1000 cpu triggers on 1000 cpu machines, and yes, its a
> disaster we'll need to fix. Lets not repeat the same mistake here.
> 
> I guess it may be slightly more work. Sorry about that.

We should be able to list available block devices to set,
so the problem would be not avoided anyway. And Greg already proposed
a solution for trigger file PAGE_SIZE overflow, so this should not pose
a big problem in the future once that is implemented.

> 								Pavel
> 
>> +++ b/include/linux/leds.h
>> +#else
>> +
>> +struct ledtrig_blk {
>> +};
>> +
> 
> Is the empty struct neccessary?

Yeah, I didn't like that too and tried to play with the code to turn
it into a pointer but it turned out to be non-trivial.

The thing is that struct ledtrig_blk is made a property of
struct gendisk, which then allows to get a pointer to the struct gendisk
of the registrar in ledtrig_blk_work() via container_of macro.

Initially I did not realize the problem and asked Akinobu to move the
trigger implementation to the LED subsystem since it seems to have
likely broad use and it would be good to have it visible in the LED
triggers config menu.

That move unfortunately entails the cumbersome dependency we're
discussing now. It's to be decided then if it wouldn't be cleaner
to have that trigger entirely implemented on gendisk side.

>> +static inline void ledtrig_blk_enable(struct gendisk *disk)
>> +{
>> +}
>> +
>> +static inline void ledtrig_blk_disable(struct gendisk *disk)
>> +{
>> +}
>> +
>> +static inline int ledtrig_blk_register(struct gendisk *disk)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline void ledtrig_blk_unregister(struct gendisk *disk)
>> +{
>> +}
> 
> Normally we put such empty functions on single lines...
> 
> Best regards,
> 									     Pavel
> 

-- 
Best regards,
Jacek Anaszewski

[PATCH v4 4/5] block: introduce LED block device activity trigger

[Jacek Anaszewski]

On 8/17/19 4:55 PM, Pavel Machek wrote:
> On Fri 2019-08-16 01:59:58, Akinobu Mita wrote:
>> This allows LEDs to be controlled by block device activity.
>>
>> We already have ledtrig-disk (LED disk activity trigger), but the lower
>> level disk drivers need to utilize ledtrig_disk_activity() to make the
>> LED blink.
>>
>> The LED block device trigger doesn't require the lower level drivers to
>> have any instrumentation. The activity is collected by polling the disk
>> stats.
>>
>> Example:
>>
>> echo block-nvme0n1 > /sys/class/leds/diy/trigger
> 
> Lets use one trigger "block" and have the device as a parameter,
> please.
> 
> We already have 1000 cpu triggers on 1000 cpu machines, and yes, its a
> disaster we'll need to fix. Lets not repeat the same mistake here.
> 
> I guess it may be slightly more work. Sorry about that.

We should be able to list available block devices to set,
so the problem would be not avoided anyway. And Greg already proposed
a solution for trigger file PAGE_SIZE overflow, so this should not pose
a big problem in the future once that is implemented.

> 								Pavel
> 
>> +++ b/include/linux/leds.h
>> +#else
>> +
>> +struct ledtrig_blk {
>> +};
>> +
> 
> Is the empty struct neccessary?

Yeah, I didn't like that too and tried to play with the code to turn
it into a pointer but it turned out to be non-trivial.

The thing is that struct ledtrig_blk is made a property of
struct gendisk, which then allows to get a pointer to the struct gendisk
of the registrar in ledtrig_blk_work() via container_of macro.

Initially I did not realize the problem and asked Akinobu to move the
trigger implementation to the LED subsystem since it seems to have
likely broad use and it would be good to have it visible in the LED
triggers config menu.

That move unfortunately entails the cumbersome dependency we're
discussing now. It's to be decided then if it wouldn't be cleaner
to have that trigger entirely implemented on gendisk side.

>> +static inline void ledtrig_blk_enable(struct gendisk *disk)
>> +{
>> +}
>> +
>> +static inline void ledtrig_blk_disable(struct gendisk *disk)
>> +{
>> +}
>> +
>> +static inline int ledtrig_blk_register(struct gendisk *disk)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline void ledtrig_blk_unregister(struct gendisk *disk)
>> +{
>> +}
> 
> Normally we put such empty functions on single lines...
> 
> Best regards,
> 									     Pavel
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v4 4/5] block: introduce LED block device activity trigger

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1596 bytes --]

On Sat 2019-08-17 22:07:43, Jacek Anaszewski wrote:
> On 8/17/19 4:55 PM, Pavel Machek wrote:
> > On Fri 2019-08-16 01:59:58, Akinobu Mita wrote:
> >> This allows LEDs to be controlled by block device activity.
> >>
> >> We already have ledtrig-disk (LED disk activity trigger), but the lower
> >> level disk drivers need to utilize ledtrig_disk_activity() to make the
> >> LED blink.
> >>
> >> The LED block device trigger doesn't require the lower level drivers to
> >> have any instrumentation. The activity is collected by polling the disk
> >> stats.
> >>
> >> Example:
> >>
> >> echo block-nvme0n1 > /sys/class/leds/diy/trigger
> > 
> > Lets use one trigger "block" and have the device as a parameter,
> > please.
> > 
> > We already have 1000 cpu triggers on 1000 cpu machines, and yes, its a
> > disaster we'll need to fix. Lets not repeat the same mistake here.
> > 
> > I guess it may be slightly more work. Sorry about that.
> 
> We should be able to list available block devices to set,
> so the problem would be not avoided anyway.

Should we? We need to list triggers, but we may not list all the devices...

> And Greg already proposed
> a solution for trigger file PAGE_SIZE overflow, so this should not pose
> a big problem in the future once that is implemented.

Which still leaves us with pretty big/ugly triggers file... and we do
not have the fix in the tree yet.

Best regards,

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v4 4/5] block: introduce LED block device activity trigger

[Jacek Anaszewski]

On 8/19/19 4:38 PM, Pavel Machek wrote:
> On Sat 2019-08-17 22:07:43, Jacek Anaszewski wrote:
>> On 8/17/19 4:55 PM, Pavel Machek wrote:
>>> On Fri 2019-08-16 01:59:58, Akinobu Mita wrote:
>>>> This allows LEDs to be controlled by block device activity.
>>>>
>>>> We already have ledtrig-disk (LED disk activity trigger), but the lower
>>>> level disk drivers need to utilize ledtrig_disk_activity() to make the
>>>> LED blink.
>>>>
>>>> The LED block device trigger doesn't require the lower level drivers to
>>>> have any instrumentation. The activity is collected by polling the disk
>>>> stats.
>>>>
>>>> Example:
>>>>
>>>> echo block-nvme0n1 > /sys/class/leds/diy/trigger
>>>
>>> Lets use one trigger "block" and have the device as a parameter,
>>> please.
>>>
>>> We already have 1000 cpu triggers on 1000 cpu machines, and yes, its a
>>> disaster we'll need to fix. Lets not repeat the same mistake here.
>>>
>>> I guess it may be slightly more work. Sorry about that.
>>
>> We should be able to list available block devices to set,
>> so the problem would be not avoided anyway.
> 
> Should we? We need to list triggers, but we may not list all the devices...

This is similar to usbport trigger that lists available
ports as files in a sub-directory. We might eventually go
in this direction.

>> And Greg already proposed
>> a solution for trigger file PAGE_SIZE overflow, so this should not pose
>> a big problem in the future once that is implemented.
> 
> Which still leaves us with pretty big/ugly triggers file... and we do
> not have the fix in the tree yet.

Still, we have that interface and must keep it. It implies the fix
will need to be applied anyway.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v4 4/5] block: introduce LED block device activity trigger

[Jacek Anaszewski]

On 8/19/19 8:22 PM, Jacek Anaszewski wrote:
> On 8/19/19 4:38 PM, Pavel Machek wrote:
>> On Sat 2019-08-17 22:07:43, Jacek Anaszewski wrote:
>>> On 8/17/19 4:55 PM, Pavel Machek wrote:
>>>> On Fri 2019-08-16 01:59:58, Akinobu Mita wrote:
>>>>> This allows LEDs to be controlled by block device activity.
>>>>>
>>>>> We already have ledtrig-disk (LED disk activity trigger), but the lower
>>>>> level disk drivers need to utilize ledtrig_disk_activity() to make the
>>>>> LED blink.
>>>>>
>>>>> The LED block device trigger doesn't require the lower level drivers to
>>>>> have any instrumentation. The activity is collected by polling the disk
>>>>> stats.
>>>>>
>>>>> Example:
>>>>>
>>>>> echo block-nvme0n1 > /sys/class/leds/diy/trigger
>>>>
>>>> Lets use one trigger "block" and have the device as a parameter,
>>>> please.
>>>>
>>>> We already have 1000 cpu triggers on 1000 cpu machines, and yes, its a
>>>> disaster we'll need to fix. Lets not repeat the same mistake here.
>>>>
>>>> I guess it may be slightly more work. Sorry about that.
>>>
>>> We should be able to list available block devices to set,
>>> so the problem would be not avoided anyway.
>>
>> Should we? We need to list triggers, but we may not list all the devices...
> 
> This is similar to usbport trigger that lists available
> ports as files in a sub-directory. We might eventually go
> in this direction.

I must withdraw this statement. This is not similar to usbport
trigger. The difference is that with ledtrig-block we have separate
triggers per each device and I am not aware if there is some centralized
mechanism similar to blocking_notifier_chain (usb_notifier_list
in drivers/usb/core/notify.c) available for block devices, that
would allow to gather all available block devs under common trigger.

Moreover I remember Greg once discouraged using notifier chains
as they are unsafe, so we would need some other solution anyway.

>>> And Greg already proposed
>>> a solution for trigger file PAGE_SIZE overflow, so this should not pose
>>> a big problem in the future once that is implemented.
>>
>> Which still leaves us with pretty big/ugly triggers file... and we do
>> not have the fix in the tree yet.
> 
> Still, we have that interface and must keep it. It implies the fix
> will need to be applied anyway.
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v4 4/5] block: introduce LED block device activity trigger

[Akinobu Mita]

2019年8月20日(火) 3:38 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>
> On 8/19/19 8:22 PM, Jacek Anaszewski wrote:
> > On 8/19/19 4:38 PM, Pavel Machek wrote:
> >> On Sat 2019-08-17 22:07:43, Jacek Anaszewski wrote:
> >>> On 8/17/19 4:55 PM, Pavel Machek wrote:
> >>>> On Fri 2019-08-16 01:59:58, Akinobu Mita wrote:
> >>>>> This allows LEDs to be controlled by block device activity.
> >>>>>
> >>>>> We already have ledtrig-disk (LED disk activity trigger), but the lower
> >>>>> level disk drivers need to utilize ledtrig_disk_activity() to make the
> >>>>> LED blink.
> >>>>>
> >>>>> The LED block device trigger doesn't require the lower level drivers to
> >>>>> have any instrumentation. The activity is collected by polling the disk
> >>>>> stats.
> >>>>>
> >>>>> Example:
> >>>>>
> >>>>> echo block-nvme0n1 > /sys/class/leds/diy/trigger
> >>>>
> >>>> Lets use one trigger "block" and have the device as a parameter,
> >>>> please.
> >>>>
> >>>> We already have 1000 cpu triggers on 1000 cpu machines, and yes, its a
> >>>> disaster we'll need to fix. Lets not repeat the same mistake here.
> >>>>
> >>>> I guess it may be slightly more work. Sorry about that.
> >>>
> >>> We should be able to list available block devices to set,
> >>> so the problem would be not avoided anyway.
> >>
> >> Should we? We need to list triggers, but we may not list all the devices...
> >
> > This is similar to usbport trigger that lists available
> > ports as files in a sub-directory. We might eventually go
> > in this direction.
>
> I must withdraw this statement. This is not similar to usbport
> trigger. The difference is that with ledtrig-block we have separate
> triggers per each device and I am not aware if there is some centralized
> mechanism similar to blocking_notifier_chain (usb_notifier_list
> in drivers/usb/core/notify.c) available for block devices, that
> would allow to gather all available block devs under common trigger.
>
> Moreover I remember Greg once discouraged using notifier chains
> as they are unsafe, so we would need some other solution anyway.

I start thinking that we should implement the LED block device activity
trigger in userspace.  The userspace application firstly activates
one-shot LED trigger and periodically reads /sys/block/<disk>/stat and
writes /sys/class/leds/<led>/shot if there is any disk activity.

Re: [PATCH v4 4/5] block: introduce LED block device activity trigger

[Jacek Anaszewski]

On 8/23/19 6:00 PM, Akinobu Mita wrote:
> 2019年8月20日(火) 3:38 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>>
>> On 8/19/19 8:22 PM, Jacek Anaszewski wrote:
>>> On 8/19/19 4:38 PM, Pavel Machek wrote:
>>>> On Sat 2019-08-17 22:07:43, Jacek Anaszewski wrote:
>>>>> On 8/17/19 4:55 PM, Pavel Machek wrote:
>>>>>> On Fri 2019-08-16 01:59:58, Akinobu Mita wrote:
>>>>>>> This allows LEDs to be controlled by block device activity.
>>>>>>>
>>>>>>> We already have ledtrig-disk (LED disk activity trigger), but the lower
>>>>>>> level disk drivers need to utilize ledtrig_disk_activity() to make the
>>>>>>> LED blink.
>>>>>>>
>>>>>>> The LED block device trigger doesn't require the lower level drivers to
>>>>>>> have any instrumentation. The activity is collected by polling the disk
>>>>>>> stats.
>>>>>>>
>>>>>>> Example:
>>>>>>>
>>>>>>> echo block-nvme0n1 > /sys/class/leds/diy/trigger
>>>>>>
>>>>>> Lets use one trigger "block" and have the device as a parameter,
>>>>>> please.
>>>>>>
>>>>>> We already have 1000 cpu triggers on 1000 cpu machines, and yes, its a
>>>>>> disaster we'll need to fix. Lets not repeat the same mistake here.
>>>>>>
>>>>>> I guess it may be slightly more work. Sorry about that.
>>>>>
>>>>> We should be able to list available block devices to set,
>>>>> so the problem would be not avoided anyway.
>>>>
>>>> Should we? We need to list triggers, but we may not list all the devices...
>>>
>>> This is similar to usbport trigger that lists available
>>> ports as files in a sub-directory. We might eventually go
>>> in this direction.
>>
>> I must withdraw this statement. This is not similar to usbport
>> trigger. The difference is that with ledtrig-block we have separate
>> triggers per each device and I am not aware if there is some centralized
>> mechanism similar to blocking_notifier_chain (usb_notifier_list
>> in drivers/usb/core/notify.c) available for block devices, that
>> would allow to gather all available block devs under common trigger.
>>
>> Moreover I remember Greg once discouraged using notifier chains
>> as they are unsafe, so we would need some other solution anyway.
> 
> I start thinking that we should implement the LED block device activity
> trigger in userspace.  The userspace application firstly activates
> one-shot LED trigger and periodically reads /sys/block/<disk>/stat and
> writes /sys/class/leds/<led>/shot if there is any disk activity.

This would suboptimal solution. I have another idea - let's get back
to the implementation of ledtrig-blk in genhd.c. We would be registering
one trigger on module initialization in a function with __init modifier.
Then we would need to add/remove triggers to the ledtrig-blk in
register_blkdev()/unregister_blkdev(). And registered triggers would
be listed in block_devs directory created by the trigger.

You can compare how drivers/usb/core/ledtrig-usbport.c maintains
similar directory of usb ports.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v4 4/5] block: introduce LED block device activity trigger

[Akinobu Mita]

2019年8月25日(日) 0:53 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>
> On 8/23/19 6:00 PM, Akinobu Mita wrote:
> > 2019年8月20日(火) 3:38 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
> >>
> >> On 8/19/19 8:22 PM, Jacek Anaszewski wrote:
> >>> On 8/19/19 4:38 PM, Pavel Machek wrote:
> >>>> On Sat 2019-08-17 22:07:43, Jacek Anaszewski wrote:
> >>>>> On 8/17/19 4:55 PM, Pavel Machek wrote:
> >>>>>> On Fri 2019-08-16 01:59:58, Akinobu Mita wrote:
> >>>>>>> This allows LEDs to be controlled by block device activity.
> >>>>>>>
> >>>>>>> We already have ledtrig-disk (LED disk activity trigger), but the lower
> >>>>>>> level disk drivers need to utilize ledtrig_disk_activity() to make the
> >>>>>>> LED blink.
> >>>>>>>
> >>>>>>> The LED block device trigger doesn't require the lower level drivers to
> >>>>>>> have any instrumentation. The activity is collected by polling the disk
> >>>>>>> stats.
> >>>>>>>
> >>>>>>> Example:
> >>>>>>>
> >>>>>>> echo block-nvme0n1 > /sys/class/leds/diy/trigger
> >>>>>>
> >>>>>> Lets use one trigger "block" and have the device as a parameter,
> >>>>>> please.
> >>>>>>
> >>>>>> We already have 1000 cpu triggers on 1000 cpu machines, and yes, its a
> >>>>>> disaster we'll need to fix. Lets not repeat the same mistake here.
> >>>>>>
> >>>>>> I guess it may be slightly more work. Sorry about that.
> >>>>>
> >>>>> We should be able to list available block devices to set,
> >>>>> so the problem would be not avoided anyway.
> >>>>
> >>>> Should we? We need to list triggers, but we may not list all the devices...
> >>>
> >>> This is similar to usbport trigger that lists available
> >>> ports as files in a sub-directory. We might eventually go
> >>> in this direction.
> >>
> >> I must withdraw this statement. This is not similar to usbport
> >> trigger. The difference is that with ledtrig-block we have separate
> >> triggers per each device and I am not aware if there is some centralized
> >> mechanism similar to blocking_notifier_chain (usb_notifier_list
> >> in drivers/usb/core/notify.c) available for block devices, that
> >> would allow to gather all available block devs under common trigger.
> >>
> >> Moreover I remember Greg once discouraged using notifier chains
> >> as they are unsafe, so we would need some other solution anyway.
> >
> > I start thinking that we should implement the LED block device activity
> > trigger in userspace.  The userspace application firstly activates
> > one-shot LED trigger and periodically reads /sys/block/<disk>/stat and
> > writes /sys/class/leds/<led>/shot if there is any disk activity.
>
> This would suboptimal solution. I have another idea - let's get back
> to the implementation of ledtrig-blk in genhd.c. We would be registering
> one trigger on module initialization in a function with __init modifier.
> Then we would need to add/remove triggers to the ledtrig-blk in
> register_blkdev()/unregister_blkdev(). And registered triggers would
> be listed in block_devs directory created by the trigger.
>
> You can compare how drivers/usb/core/ledtrig-usbport.c maintains
> similar directory of usb ports.

It could be possible, but I have yet another idea.  What about introducing
/proc/led-triggers and /sys/class/leds/<led>/current-trigger?
The /sys/class/leds/<led>/trigger will be obsoleted by these two files.

The /proc/led-triggers is read only and no PAGE_SIZE limitation by the
seq_file interface.  So we can list all triggers in this file.

The /sys/class/leds/<led>/current-trigger is almost identical to
/sys/class/leds/<led>/trigger.  The only difference is that
'current-trigger' only shows the current trigger name.

Re: [PATCH v4 4/5] block: introduce LED block device activity trigger

[Jacek Anaszewski]

On 8/27/19 4:03 PM, Akinobu Mita wrote:
> 2019年8月25日(日) 0:53 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>>
>> On 8/23/19 6:00 PM, Akinobu Mita wrote:
>>> 2019年8月20日(火) 3:38 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>>>>
>>>> On 8/19/19 8:22 PM, Jacek Anaszewski wrote:
>>>>> On 8/19/19 4:38 PM, Pavel Machek wrote:
>>>>>> On Sat 2019-08-17 22:07:43, Jacek Anaszewski wrote:
>>>>>>> On 8/17/19 4:55 PM, Pavel Machek wrote:
>>>>>>>> On Fri 2019-08-16 01:59:58, Akinobu Mita wrote:
>>>>>>>>> This allows LEDs to be controlled by block device activity.
>>>>>>>>>
>>>>>>>>> We already have ledtrig-disk (LED disk activity trigger), but the lower
>>>>>>>>> level disk drivers need to utilize ledtrig_disk_activity() to make the
>>>>>>>>> LED blink.
>>>>>>>>>
>>>>>>>>> The LED block device trigger doesn't require the lower level drivers to
>>>>>>>>> have any instrumentation. The activity is collected by polling the disk
>>>>>>>>> stats.
>>>>>>>>>
>>>>>>>>> Example:
>>>>>>>>>
>>>>>>>>> echo block-nvme0n1 > /sys/class/leds/diy/trigger
>>>>>>>>
>>>>>>>> Lets use one trigger "block" and have the device as a parameter,
>>>>>>>> please.
>>>>>>>>
>>>>>>>> We already have 1000 cpu triggers on 1000 cpu machines, and yes, its a
>>>>>>>> disaster we'll need to fix. Lets not repeat the same mistake here.
>>>>>>>>
>>>>>>>> I guess it may be slightly more work. Sorry about that.
>>>>>>>
>>>>>>> We should be able to list available block devices to set,
>>>>>>> so the problem would be not avoided anyway.
>>>>>>
>>>>>> Should we? We need to list triggers, but we may not list all the devices...
>>>>>
>>>>> This is similar to usbport trigger that lists available
>>>>> ports as files in a sub-directory. We might eventually go
>>>>> in this direction.
>>>>
>>>> I must withdraw this statement. This is not similar to usbport
>>>> trigger. The difference is that with ledtrig-block we have separate
>>>> triggers per each device and I am not aware if there is some centralized
>>>> mechanism similar to blocking_notifier_chain (usb_notifier_list
>>>> in drivers/usb/core/notify.c) available for block devices, that
>>>> would allow to gather all available block devs under common trigger.
>>>>
>>>> Moreover I remember Greg once discouraged using notifier chains
>>>> as they are unsafe, so we would need some other solution anyway.
>>>
>>> I start thinking that we should implement the LED block device activity
>>> trigger in userspace.  The userspace application firstly activates
>>> one-shot LED trigger and periodically reads /sys/block/<disk>/stat and
>>> writes /sys/class/leds/<led>/shot if there is any disk activity.
>>
>> This would suboptimal solution. I have another idea - let's get back
>> to the implementation of ledtrig-blk in genhd.c. We would be registering
>> one trigger on module initialization in a function with __init modifier.
>> Then we would need to add/remove triggers to the ledtrig-blk in
>> register_blkdev()/unregister_blkdev(). And registered triggers would
>> be listed in block_devs directory created by the trigger.
>>
>> You can compare how drivers/usb/core/ledtrig-usbport.c maintains
>> similar directory of usb ports.
> 
> It could be possible, but I have yet another idea.  What about introducing
> /proc/led-triggers and /sys/class/leds/<led>/current-trigger?
> The /sys/class/leds/<led>/trigger will be obsoleted by these two files.
> 
> The /proc/led-triggers is read only and no PAGE_SIZE limitation by the
> seq_file interface.  So we can list all triggers in this file.
> 
> The /sys/class/leds/<led>/current-trigger is almost identical to
> /sys/class/leds/<led>/trigger.  The only difference is that
> 'current-trigger' only shows the current trigger name.

There's not need to come up with yet another trigger interface.
We just need to convert sysfs trigger attribute type to binary.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v4 4/5] block: introduce LED block device activity trigger

[Akinobu Mita]

2019年8月28日(水) 6:23 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>
> On 8/27/19 4:03 PM, Akinobu Mita wrote:
> > 2019年8月25日(日) 0:53 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
> >>
> >> On 8/23/19 6:00 PM, Akinobu Mita wrote:
> >>> 2019年8月20日(火) 3:38 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
> >>>>
> >>>> On 8/19/19 8:22 PM, Jacek Anaszewski wrote:
> >>>>> On 8/19/19 4:38 PM, Pavel Machek wrote:
> >>>>>> On Sat 2019-08-17 22:07:43, Jacek Anaszewski wrote:
> >>>>>>> On 8/17/19 4:55 PM, Pavel Machek wrote:
> >>>>>>>> On Fri 2019-08-16 01:59:58, Akinobu Mita wrote:
> >>>>>>>>> This allows LEDs to be controlled by block device activity.
> >>>>>>>>>
> >>>>>>>>> We already have ledtrig-disk (LED disk activity trigger), but the lower
> >>>>>>>>> level disk drivers need to utilize ledtrig_disk_activity() to make the
> >>>>>>>>> LED blink.
> >>>>>>>>>
> >>>>>>>>> The LED block device trigger doesn't require the lower level drivers to
> >>>>>>>>> have any instrumentation. The activity is collected by polling the disk
> >>>>>>>>> stats.
> >>>>>>>>>
> >>>>>>>>> Example:
> >>>>>>>>>
> >>>>>>>>> echo block-nvme0n1 > /sys/class/leds/diy/trigger
> >>>>>>>>
> >>>>>>>> Lets use one trigger "block" and have the device as a parameter,
> >>>>>>>> please.
> >>>>>>>>
> >>>>>>>> We already have 1000 cpu triggers on 1000 cpu machines, and yes, its a
> >>>>>>>> disaster we'll need to fix. Lets not repeat the same mistake here.
> >>>>>>>>
> >>>>>>>> I guess it may be slightly more work. Sorry about that.
> >>>>>>>
> >>>>>>> We should be able to list available block devices to set,
> >>>>>>> so the problem would be not avoided anyway.
> >>>>>>
> >>>>>> Should we? We need to list triggers, but we may not list all the devices...
> >>>>>
> >>>>> This is similar to usbport trigger that lists available
> >>>>> ports as files in a sub-directory. We might eventually go
> >>>>> in this direction.
> >>>>
> >>>> I must withdraw this statement. This is not similar to usbport
> >>>> trigger. The difference is that with ledtrig-block we have separate
> >>>> triggers per each device and I am not aware if there is some centralized
> >>>> mechanism similar to blocking_notifier_chain (usb_notifier_list
> >>>> in drivers/usb/core/notify.c) available for block devices, that
> >>>> would allow to gather all available block devs under common trigger.
> >>>>
> >>>> Moreover I remember Greg once discouraged using notifier chains
> >>>> as they are unsafe, so we would need some other solution anyway.
> >>>
> >>> I start thinking that we should implement the LED block device activity
> >>> trigger in userspace.  The userspace application firstly activates
> >>> one-shot LED trigger and periodically reads /sys/block/<disk>/stat and
> >>> writes /sys/class/leds/<led>/shot if there is any disk activity.
> >>
> >> This would suboptimal solution. I have another idea - let's get back
> >> to the implementation of ledtrig-blk in genhd.c. We would be registering
> >> one trigger on module initialization in a function with __init modifier.
> >> Then we would need to add/remove triggers to the ledtrig-blk in
> >> register_blkdev()/unregister_blkdev(). And registered triggers would
> >> be listed in block_devs directory created by the trigger.
> >>
> >> You can compare how drivers/usb/core/ledtrig-usbport.c maintains
> >> similar directory of usb ports.
> >
> > It could be possible, but I have yet another idea.  What about introducing
> > /proc/led-triggers and /sys/class/leds/<led>/current-trigger?
> > The /sys/class/leds/<led>/trigger will be obsoleted by these two files.
> >
> > The /proc/led-triggers is read only and no PAGE_SIZE limitation by the
> > seq_file interface.  So we can list all triggers in this file.
> >
> > The /sys/class/leds/<led>/current-trigger is almost identical to
> > /sys/class/leds/<led>/trigger.  The only difference is that
> > 'current-trigger' only shows the current trigger name.
>
> There's not need to come up with yet another trigger interface.
> We just need to convert sysfs trigger attribute type to binary.

OK, I'll try it.