[PATCH 0/2] Add support per ATA port ledtrigger on armada 38x

[PATCH 0/2] Add support per ATA port ledtrigger on armada 38x

[Aditya Prayoga]

Hi everyone,

This series adds support LED trigger for each ATA port indicating disk activity
in Armada 38x. I pick up the work done by Daniel Golle which can be found at [1]

Helios4 which is based on Armada 388, has four LEDs designed to indicate disk
activity on each SATA ports. As the final switch (CONFIG_ATA_LEDS) to enable
the codepath by default is no, it should not affect the rest of Armada 38x 
based boards.

[1] https://patchwork.ozlabs.org/patch/420733/ 
---
Notes
  checkpatch.pl complains about "WARNING: please write a paragraph that
    describes the config symbol fully" but I think the description is clear
    enough to ignore the warning.

  Some performance number tested on Western Digital Red 2TB WD20EFRX
    using fio randrw
    * CONFIG_ATA_LEDS disabled and selected trigger is none
      read : iops=326
      write : iops=109
    * CONFIG_ATA_LEDS disabled and selected trigger is disk-activity
      read : iops=325
      write : iops=108
    * CONFIG_ATA_LEDS enabled and selected trigger is ata1
      read : iops=325
      write : iops=108

---
Aditya Prayoga (1):
  ARM: mvebu: Enable ARCH_WANT_LIBATA_LEDS in Armada 38x

Daniel Golle (1):
  libata: add ledtrig support

 arch/arm/mach-mvebu/Kconfig |  1 +
 drivers/ata/Kconfig         | 16 +++++++++++++
 drivers/ata/libata-core.c   | 56 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/libata.h      |  7 ++++++
 4 files changed, 80 insertions(+)

-- 
2.7.4

[PATCH 1/2] libata: add ledtrig support

[Aditya Prayoga]

From: Daniel Golle <daniel@makrotopia.org>

This adds a LED trigger for each ATA port indicating disk activity.

As this is needed only on specific platforms (NAS SoCs and such),
these platforms should define ARCH_WANTS_LIBATA_LEDS if there
are boards with LED(s) intended to indicate ATA disk activity and
need the OS to take care of that.
In that way, if not selected, LED trigger support not will be
included in libata-core and both, codepaths and structures remain
untouched.

I'm currently deploying this for the oxnas target in OpenWrt
https://dev.openwrt.org/changeset/43675/

v2: rebased to kernel/git/tj/libata.git
    plus small corrections and comments added

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
URL: https://patchwork.ozlabs.org/patch/420733/
[Aditya Prayoga:
* Port forward
* Change ATA_LEDS default to no
* Reduce performance impact by moving ata_led_act() call from
    ata_qc_new() to ata_qc_complete()]
Signed-off-by: Aditya Prayoga <aditya@kobol.io>

---

If there is anything fundamentally wrong with that approach, I'd be
glad for some advise on how to do it better.
I conciously choose an #ifdev approach to make sure performance will
not be affected for non-users of that code.

Thanks

Daniel

---
---
 drivers/ata/Kconfig       | 16 ++++++++++++++
 drivers/ata/libata-core.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/libata.h    |  7 ++++++
 3 files changed, 79 insertions(+)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 39b181d..a2c64ce 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -46,6 +46,22 @@ config ATA_VERBOSE_ERROR
 
 	  If unsure, say Y.
 
+config ARCH_WANT_LIBATA_LEDS
+	bool
+
+config ATA_LEDS
+	bool "support ATA port LED triggers"
+	depends on ARCH_WANT_LIBATA_LEDS
+	select NEW_LEDS
+	select LEDS_CLASS
+	select LEDS_TRIGGERS
+	default n
+	help
+	  This option adds a LED trigger for each registered ATA port.
+	  It is used to drive disk activity leds connected via GPIO.
+
+	  If unsure, say N.
+
 config ATA_ACPI
 	bool "ATA ACPI Support"
 	depends on ACPI
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 599e01b..65228f5 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5105,6 +5105,30 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words)
 }
 
 /**
+ *	ata_led_act - Trigger port activity LED
+ *	@ap: indicating port
+ *
+ *	Blinks any LEDs registered to the trigger.
+ *	Commonly used with leds-gpio on NAS systems with disk activity
+ *	indicator LEDs.
+ *
+ *	LOCKING:
+ *	None.
+ */
+static inline void ata_led_act(struct ata_port *ap)
+{
+#ifdef CONFIG_ATA_LEDS
+#define LIBATA_BLINK_DELAY 20 /* ms */
+	unsigned long led_delay = LIBATA_BLINK_DELAY;
+
+	if (unlikely(!ap->ledtrig))
+		return;
+
+	led_trigger_blink_oneshot(ap->ledtrig, &led_delay, &led_delay, 0);
+#endif /* CONFIG_ATA_LEDS */
+}
+
+/**
  *	ata_qc_new_init - Request an available ATA command, and initialize it
  *	@dev: Device from whom we request an available command structure
  *	@tag: tag
@@ -5249,6 +5273,10 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
 	/* Trigger the LED (if available) */
 	ledtrig_disk_activity(!!(qc->tf.flags & ATA_TFLAG_WRITE));
 
+#ifdef CONFIG_ATA_LEDS
+	ata_led_act(ap);
+#endif
+
 	/* XXX: New EH and old EH use different mechanisms to
 	 * synchronize EH with regular execution path.
 	 *
@@ -6028,6 +6056,9 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
 	ap->stats.unhandled_irq = 1;
 	ap->stats.idle_irq = 1;
 #endif
+#ifdef CONFIG_ATA_LEDS
+	ap->ledtrig = kzalloc(sizeof(struct led_trigger), GFP_KERNEL);
+#endif
 	ata_sff_port_init(ap);
 
 	return ap;
@@ -6063,6 +6094,12 @@ static void ata_host_release(struct kref *kref)
 
 		kfree(ap->pmp_link);
 		kfree(ap->slave_link);
+#ifdef CONFIG_ATA_LEDS
+		if (ap->ledtrig) {
+			led_trigger_unregister(ap->ledtrig);
+			kfree(ap->ledtrig);
+		};
+#endif
 		kfree(ap);
 		host->ports[i] = NULL;
 	}
@@ -6527,6 +6564,25 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
 		host->ports[i]->local_port_no = i + 1;
 	}
 
+#ifdef CONFIG_ATA_LEDS
+	/* register LED triggers for all ports */
+	for (i = 0; i < host->n_ports; i++) {
+		if (unlikely(!host->ports[i]->ledtrig))
+			continue;
+
+		snprintf(host->ports[i]->ledtrig_name,
+			sizeof(host->ports[i]->ledtrig_name), "ata%u",
+			host->ports[i]->print_id);
+
+		host->ports[i]->ledtrig->name = host->ports[i]->ledtrig_name;
+
+		if (led_trigger_register(host->ports[i]->ledtrig)) {
+			kfree(host->ports[i]->ledtrig);
+			host->ports[i]->ledtrig = NULL;
+		}
+	}
+#endif
+
 	/* Create associated sysfs transport objects  */
 	for (i = 0; i < host->n_ports; i++) {
 		rc = ata_tport_add(host->dev,host->ports[i]);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 38c95d6..3cc5f63 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -38,6 +38,7 @@
 #include <linux/acpi.h>
 #include <linux/cdrom.h>
 #include <linux/sched.h>
+#include <linux/leds.h>
 
 /*
  * Define if arch has non-standard setup.  This is a _PCI_ standard
@@ -893,6 +894,12 @@ struct ata_port {
 #ifdef CONFIG_ATA_ACPI
 	struct ata_acpi_gtm	__acpi_init_gtm; /* use ata_acpi_init_gtm() */
 #endif
+
+#ifdef CONFIG_ATA_LEDS
+	struct led_trigger	*ledtrig;
+	char			ledtrig_name[8];
+#endif
+
 	/* owned by EH */
 	u8			sector_buf[ATA_SECT_SIZE] ____cacheline_aligned;
 };
-- 
2.7.4

[PATCH 2/2] ARM: mvebu: Enable ARCH_WANT_LIBATA_LEDS in Armada 38x

[Aditya Prayoga]

Enable hidden symbol ARCH_WANT_LIBATA_LEDS so CONFIG_ATA_LEDS can be
used in kernel configuration.

URL: https://lists.openwrt.org/pipermail/openwrt-
devel/2017-March/006582.html

Signed-off-by: Aditya Prayoga <aditya@kobol.io>
---
 arch/arm/mach-mvebu/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
index 2c20599..51f3256 100644
--- a/arch/arm/mach-mvebu/Kconfig
+++ b/arch/arm/mach-mvebu/Kconfig
@@ -68,6 +68,7 @@ config MACH_ARMADA_38X
 	select HAVE_SMP
 	select MACH_MVEBU_V7
 	select PINCTRL_ARMADA_38X
+	select ARCH_WANT_LIBATA_LEDS
 	help
 	  Say 'Y' here if you want your kernel to support boards based
 	  on the Marvell Armada 380/385 SoC with device tree.
-- 
2.7.4

Re: [PATCH 1/2] libata: add ledtrig support

[Andrew Lunn]

On Wed, Sep 19, 2018 at 11:45:29AM +0800, Aditya Prayoga wrote:
> From: Daniel Golle <daniel@makrotopia.org>
> 
> This adds a LED trigger for each ATA port indicating disk activity.
> 
> As this is needed only on specific platforms (NAS SoCs and such),
> these platforms should define ARCH_WANTS_LIBATA_LEDS if there
> are boards with LED(s) intended to indicate ATA disk activity and
> need the OS to take care of that.
> In that way, if not selected, LED trigger support not will be
> included in libata-core and both, codepaths and structures remain
> untouched.
> 
> I'm currently deploying this for the oxnas target in OpenWrt
> https://dev.openwrt.org/changeset/43675/
> 
> v2: rebased to kernel/git/tj/libata.git
>     plus small corrections and comments added
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> URL: https://patchwork.ozlabs.org/patch/420733/
> [Aditya Prayoga:
> * Port forward
> * Change ATA_LEDS default to no
> * Reduce performance impact by moving ata_led_act() call from
>     ata_qc_new() to ata_qc_complete()]
> Signed-off-by: Aditya Prayoga <aditya@kobol.io>
> 
> ---
> 
> If there is anything fundamentally wrong with that approach, I'd be
> glad for some advise on how to do it better.
> I conciously choose an #ifdev approach to make sure performance will
> not be affected for non-users of that code.

Hi Aditya

I remember something like this being discussed a long time ago, and it
was rejected because of the overheads. So it is good you have some
performance numbers. I will let others decide if the approach is
acceptable.

>  /**
> + *	ata_led_act - Trigger port activity LED
> + *	@ap: indicating port
> + *
> + *	Blinks any LEDs registered to the trigger.
> + *	Commonly used with leds-gpio on NAS systems with disk activity
> + *	indicator LEDs.
> + *
> + *	LOCKING:
> + *	None.
> + */
> +static inline void ata_led_act(struct ata_port *ap)

inline should not be used in C code, just header files. A function
this small the compiler is likely to inline anyway.

> +{
> +#ifdef CONFIG_ATA_LEDS

It is better to use if (IS_ENABLED(CONFIG_ATA_LEDS) {}.  That gets
turned into if(0) {}, so the code still gets compiled to find any
errors, but then optimised out. This is important if the code is going
to be disabled by default.

> +#define LIBATA_BLINK_DELAY 20 /* ms */
> +	unsigned long led_delay = LIBATA_BLINK_DELAY;
> +
> +	if (unlikely(!ap->ledtrig))
> +		return;
> +
> +	led_trigger_blink_oneshot(ap->ledtrig, &led_delay, &led_delay, 0);
> +#endif /* CONFIG_ATA_LEDS */
> +}
> +
> +/**
>   *	ata_qc_new_init - Request an available ATA command, and initialize it
>   *	@dev: Device from whom we request an available command structure
>   *	@tag: tag
> @@ -5249,6 +5273,10 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
>  	/* Trigger the LED (if available) */
>  	ledtrig_disk_activity(!!(qc->tf.flags & ATA_TFLAG_WRITE));
>  
> +#ifdef CONFIG_ATA_LEDS
> +	ata_led_act(ap);
> +#endif

No need for this #ifdef'ery.

> +
>  	/* XXX: New EH and old EH use different mechanisms to
>  	 * synchronize EH with regular execution path.
>  	 *
> @@ -6028,6 +6056,9 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
>  	ap->stats.unhandled_irq = 1;
>  	ap->stats.idle_irq = 1;
>  #endif
> +#ifdef CONFIG_ATA_LEDS
> +	ap->ledtrig = kzalloc(sizeof(struct led_trigger), GFP_KERNEL);

Maybe use devm_kzalloc() and devm_led_trigger_register()?

      Andrew

Re: [PATCH 1/2] libata: add ledtrig support

[kbuild test robot]

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

Hi Daniel,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on block/for-next]
[also build test ERROR on v4.19-rc4 next-20180919]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Aditya-Prayoga/Add-support-per-ATA-port-ledtrigger-on-armada-38x/20180920-002037
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: i386-randconfig-s1-201837 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   In file included from drivers/scsi/mvsas/mv_94xx.c:27:0:
>> drivers/scsi/mvsas/mv_94xx.h:294:2: error: redeclaration of enumerator 'LED_OFF'
     LED_OFF = 0,
     ^~~~~~~
   In file included from include/linux/libata.h:41:0,
                    from include/scsi/libsas.h:33,
                    from drivers/scsi/mvsas/mv_sas.h:43,
                    from drivers/scsi/mvsas/mv_94xx.c:26:
   include/linux/leds.h:30:2: note: previous definition of 'LED_OFF' was here
     LED_OFF  = 0,
     ^~~~~~~
   In file included from drivers/scsi/mvsas/mv_94xx.c:27:0:
>> drivers/scsi/mvsas/mv_94xx.h:295:2: error: redeclaration of enumerator 'LED_ON'
     LED_ON = 1,
     ^~~~~~
   In file included from include/linux/libata.h:41:0,
                    from include/scsi/libsas.h:33,
                    from drivers/scsi/mvsas/mv_sas.h:43,
                    from drivers/scsi/mvsas/mv_94xx.c:26:
   include/linux/leds.h:31:2: note: previous definition of 'LED_ON' was here
     LED_ON  = 1,
     ^~~~~~

vim +/LED_OFF +294 drivers/scsi/mvsas/mv_94xx.h

c56f5f1d Wilfried Weissmann 2015-12-27  292  
c56f5f1d Wilfried Weissmann 2015-12-27  293  enum sgpio_led_status {
c56f5f1d Wilfried Weissmann 2015-12-27 @294  	LED_OFF	= 0,
c56f5f1d Wilfried Weissmann 2015-12-27 @295  	LED_ON	= 1,
c56f5f1d Wilfried Weissmann 2015-12-27  296  	LED_BLINKA	= 2,
c56f5f1d Wilfried Weissmann 2015-12-27  297  	LED_BLINKA_INV	= 3,
c56f5f1d Wilfried Weissmann 2015-12-27  298  	LED_BLINKA_SOF	= 4,
c56f5f1d Wilfried Weissmann 2015-12-27  299  	LED_BLINKA_EOF	= 5,
c56f5f1d Wilfried Weissmann 2015-12-27  300  	LED_BLINKB	= 6,
c56f5f1d Wilfried Weissmann 2015-12-27  301  	LED_BLINKB_INV	= 7,
c56f5f1d Wilfried Weissmann 2015-12-27  302  };
c56f5f1d Wilfried Weissmann 2015-12-27  303  

:::::: The code at line 294 was first introduced by commit
:::::: c56f5f1de3a6ab8ec985edbc358e1fd8d4e36a65 mvsas: Add SGPIO support to Marvell 94xx

:::::: TO: Wilfried Weissmann <Wilfried.Weissmann@gmx.at>
:::::: CC: Martin K. Petersen <martin.petersen@oracle.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29628 bytes --]

Re: [PATCH 1/2] libata: add ledtrig support

[kbuild test robot]

Hi Daniel,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on block/for-next]
[also build test WARNING on v4.19-rc4 next-20180919]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Aditya-Prayoga/Add-support-per-ATA-port-ledtrigger-on-armada-38x/20180920-002037
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next


coccinelle warnings: (new ones prefixed by >>)

>> drivers/ata/libata-core.c:6101:3-4: Unneeded semicolon

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[PATCH] libata: fix semicolon.cocci warnings

[kbuild test robot]

From: kbuild test robot <fengguang.wu@intel.com>

drivers/ata/libata-core.c:6101:3-4: Unneeded semicolon


 Remove unneeded semicolon.

Generated by: scripts/coccinelle/misc/semicolon.cocci

Fixes: 0ea8bf5a708e ("libata: add ledtrig support")
CC: Daniel Golle <daniel@makrotopia.org>
Signed-off-by: kbuild test robot <fengguang.wu@intel.com>
---

url:    https://github.com/0day-ci/linux/commits/Aditya-Prayoga/Add-support-per-ATA-port-ledtrigger-on-armada-38x/20180920-002037
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next

 libata-core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6098,7 +6098,7 @@ static void ata_host_release(struct kref
 		if (ap->ledtrig) {
 			led_trigger_unregister(ap->ledtrig);
 			kfree(ap->ledtrig);
-		};
+		}
 #endif
 		kfree(ap);
 		host->ports[i] = NULL;

Re: [PATCH 1/2] libata: add ledtrig support

[Pavel Machek]

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

Hi!

> +#ifdef CONFIG_ATA_LEDS
> +	/* register LED triggers for all ports */
> +	for (i = 0; i < host->n_ports; i++) {
> +		if (unlikely(!host->ports[i]->ledtrig))
> +			continue;
> +
> +		snprintf(host->ports[i]->ledtrig_name,
> +			sizeof(host->ports[i]->ledtrig_name), "ata%u",
> +			host->ports[i]->print_id);

> +		host->ports[i]->ledtrig->name = host->ports[i]->ledtrig_name;
> +
> +		if (led_trigger_register(host->ports[i]->ledtrig)) {
> +			kfree(host->ports[i]->ledtrig);
> +			host->ports[i]->ledtrig = NULL;
> +		}
> +	}
> +#endif

No, we don't want you to register multiple triggers. We want one
trigger, than has parameter "which port to watch". (Number of triggers
is limited as by sysfs limitations).

Otherwise yes, ata trigger makes sense.
									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 2/2] ARM: mvebu: Enable ARCH_WANT_LIBATA_LEDS in Armada 38x

[Pavel Machek]

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

On Wed 2018-09-19 11:45:30, Aditya Prayoga wrote:
> Enable hidden symbol ARCH_WANT_LIBATA_LEDS so CONFIG_ATA_LEDS can be
> used in kernel configuration.

Should that be hidden symbol and should that be architecture specific?

For a notebook, I may want scrolllock LED to indicate ATA activity
(because I'm using USB keyboard and can't see the disk led).

Hmm. And while at it... it would be nice to have option to watch all
ATA ports combined, as well.

									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 1/2] libata: add ledtrig support

[Daniel Golle]

Hi!

On Thu, Sep 20, 2018 at 09:23:54AM +0200, Pavel Machek wrote:
> Hi!
> 
> > +#ifdef CONFIG_ATA_LEDS
> > +	/* register LED triggers for all ports */
> > +	for (i = 0; i < host->n_ports; i++) {
> > +		if (unlikely(!host->ports[i]->ledtrig))
> > +			continue;
> > +
> > +		snprintf(host->ports[i]->ledtrig_name,
> > +			sizeof(host->ports[i]->ledtrig_name), "ata%u",
> > +			host->ports[i]->print_id);
> 
> > +		host->ports[i]->ledtrig->name = host->ports[i]->ledtrig_name;
> > +
> > +		if (led_trigger_register(host->ports[i]->ledtrig)) {
> > +			kfree(host->ports[i]->ledtrig);
> > +			host->ports[i]->ledtrig = NULL;
> > +		}
> > +	}
> > +#endif
> 
> No, we don't want you to register multiple triggers. We want one
> trigger, than has parameter "which port to watch". (Number of triggers
> is limited as by sysfs limitations).

Back then I implemented it that way to be able to define the
default trigger for each LED in device tree and "trigger-sources"
didn't exist yet (it was introduced for USB ports and isn't yet used
for anything other than that)
However, the problem till today is also that ATA ports are often not
individual device-tree objects we can refer to, see for example
marvell,armada-370-sata which appears as one opaque controller. Ie.
all SATA drivers have to be converted to expose individual ports on
device-tree before the trigger-sources approach can be applied...


> 
> Otherwise yes, ata trigger makes sense.
> 									Pavel
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/2] libata: add ledtrig support

[Aditya Prayoga]

Hi Andrew,

thank you for your feedback. It seem i also need to resolve the issue
reported by kbuild test robot.

Aditya

Re: [PATCH 1/2] libata: add ledtrig support

[Pavel Machek]

Hi!

> > > +#ifdef CONFIG_ATA_LEDS
> > > +	/* register LED triggers for all ports */
> > > +	for (i = 0; i < host->n_ports; i++) {
> > > +		if (unlikely(!host->ports[i]->ledtrig))
> > > +			continue;
> > > +
> > > +		snprintf(host->ports[i]->ledtrig_name,
> > > +			sizeof(host->ports[i]->ledtrig_name), "ata%u",
> > > +			host->ports[i]->print_id);
> > 
> > > +		host->ports[i]->ledtrig->name = host->ports[i]->ledtrig_name;
> > > +
> > > +		if (led_trigger_register(host->ports[i]->ledtrig)) {
> > > +			kfree(host->ports[i]->ledtrig);
> > > +			host->ports[i]->ledtrig = NULL;
> > > +		}
> > > +	}
> > > +#endif
> > 
> > No, we don't want you to register multiple triggers. We want one
> > trigger, than has parameter "which port to watch". (Number of triggers
> > is limited as by sysfs limitations).
> 
> Back then I implemented it that way to be able to define the
> default trigger for each LED in device tree and "trigger-sources"
> didn't exist yet (it was introduced for USB ports and isn't yet used
> for anything other than that)

I see why you did it... BUt I believe we still want single trigger solution...

> However, the problem till today is also that ATA ports are often not
> individual device-tree objects we can refer to, see for example
> marvell,armada-370-sata which appears as one opaque controller. Ie.
> all SATA drivers have to be converted to expose individual ports on
> device-tree before the trigger-sources approach can be applied...

Yep, well... something to do in SATA then.

Perhaps this should also have an option for single LED for _any_ SATA activity,
and 90% devices will be happy with that?

									Pavel

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

Re: [PATCH 1/2] libata: add ledtrig support

[Daniel Golle]

Hi Pavel,

On Fri, Sep 21, 2018 at 12:04:49AM +0200, Pavel Machek wrote:
> Hi!
> 
> > > > +#ifdef CONFIG_ATA_LEDS
> > > > +	/* register LED triggers for all ports */
> > > > +	for (i = 0; i < host->n_ports; i++) {
> > > > +		if (unlikely(!host->ports[i]->ledtrig))
> > > > +			continue;
> > > > +
> > > > +		snprintf(host->ports[i]->ledtrig_name,
> > > > +			sizeof(host->ports[i]->ledtrig_name), "ata%u",
> > > > +			host->ports[i]->print_id);
> > > 
> > > > +		host->ports[i]->ledtrig->name = host->ports[i]->ledtrig_name;
> > > > +
> > > > +		if (led_trigger_register(host->ports[i]->ledtrig)) {
> > > > +			kfree(host->ports[i]->ledtrig);
> > > > +			host->ports[i]->ledtrig = NULL;
> > > > +		}
> > > > +	}
> > > > +#endif
> > > 
> > > No, we don't want you to register multiple triggers. We want one
> > > trigger, than has parameter "which port to watch". (Number of triggers
> > > is limited as by sysfs limitations).
> > 
> > Back then I implemented it that way to be able to define the
> > default trigger for each LED in device tree and "trigger-sources"
> > didn't exist yet (it was introduced for USB ports and isn't yet used
> > for anything other than that)
> 
> I see why you did it... BUt I believe we still want single trigger solution...
> 
> > However, the problem till today is also that ATA ports are often not
> > individual device-tree objects we can refer to, see for example
> > marvell,armada-370-sata which appears as one opaque controller. Ie.
> > all SATA drivers have to be converted to expose individual ports on
> > device-tree before the trigger-sources approach can be applied...
> 
> Yep, well... something to do in SATA then.
> 
> Perhaps this should also have an option for single LED for _any_ SATA activity,
> and 90% devices will be happy with that?

The whole reason for not using one of the existing disk-activity
triggers was to address individual SATA ports to individual LEDs of NAS
devices (in my case Shuttle KD20)...

Re: [PATCH 2/2] ARM: mvebu: Enable ARCH_WANT_LIBATA_LEDS in Armada 38x

[Aditya Prayoga]

On Thu, Sep 20, 2018 at 2:26 PM Pavel Machek <pavel@ucw.cz> wrote:
>
> On Wed 2018-09-19 11:45:30, Aditya Prayoga wrote:
> > Enable hidden symbol ARCH_WANT_LIBATA_LEDS so CONFIG_ATA_LEDS can be
> > used in kernel configuration.
>
> Should that be hidden symbol and should that be architecture specific?
>

The intention was to minimize the risk of the ledtrig code being
included by accident. The
code itself does not have architecture specific instruction

> For a notebook, I may want scrolllock LED to indicate ATA activity
> (because I'm using USB keyboard and can't see the disk led).
>
> Hmm. And while at it... it would be nice to have option to watch all
> ATA ports combined, as well.

but there is disk-activity trigger for that purpose.

Aditya

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