diff mbox series

[1/2] libata: add ledtrig support

Message ID 1537328730-9156-2-git-send-email-aditya@kobol.io
State Not Applicable
Delegated to: David Miller
Headers show
Series Add support per ATA port ledtrigger on armada 38x | expand

Commit Message

Aditya Prayoga Sept. 19, 2018, 3:45 a.m. UTC 
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(+)

Comments

Andrew Lunn Sept. 19, 2018, 1:01 p.m. UTC | #1 
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
kernel test robot Sept. 19, 2018, 6:36 p.m. UTC | #2 
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
kernel test robot Sept. 19, 2018, 9:49 p.m. UTC | #3 
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
Pavel Machek Sept. 20, 2018, 7:23 a.m. UTC | #4 
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
Daniel Golle Sept. 20, 2018, 8:24 a.m. UTC | #5 
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
Aditya Prayoga Sept. 20, 2018, 9:53 a.m. UTC | #6 
Hi Andrew,

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

Aditya
Pavel Machek Sept. 20, 2018, 10:04 p.m. UTC | #7 
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
Daniel Golle Sept. 21, 2018, 6:31 a.m. UTC | #8 
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)...
diff mbox series

Patch

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;
 };