On Wed, Sep 19, 2018 at 11:45:29AM +0800, Aditya Prayoga wrote: > From: Daniel Golle <daniel@xxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxx> > 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@xxxxxxxx> > > --- > > 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
