lkml.org 
[lkml]   [2018]   [Sep]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 1/2] libata: add ledtrig support
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

\
 
 \ /
  Last update: 2018-09-19 15:02    [W:0.105 / U:1.908 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site