* [PATCH] mac80211: use oneshot blink API for LED triggers @ 2013-07-24 0:09 Fabio Baltieri 2013-07-24 0:09 ` Fabio Baltieri 0 siblings, 1 reply; 6+ messages in thread From: Fabio Baltieri @ 2013-07-24 0:09 UTC (permalink / raw) To: Johannes Berg, John W. Linville Cc: linux-wireless, netdev, linux-kernel, Fabio Baltieri Hi Johannes, John, This patch changes the mac80211 LED driver to use the generic oneshot LED API. The current mac80211 blink code seems to lead to very short tx blinks and toggles the LED on tx. The oneshot API ensure a complete LED on-off cycle for each blink event, thus getting a better result for an activity indicator: one visible blink for sporadic events and a steady on-off blink sequence on constant traffic. Would you consider this patch for wireless-next? Thanks, Fabio Fabio Baltieri (1): mac80211: use oneshot blink API for LED triggers net/mac80211/ieee80211_i.h | 1 - net/mac80211/led.c | 21 +++++++++------------ net/mac80211/led.h | 2 +- net/mac80211/status.c | 2 +- net/mac80211/tx.c | 1 - 5 files changed, 11 insertions(+), 16 deletions(-) -- 1.8.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] mac80211: use oneshot blink API for LED triggers 2013-07-24 0:09 [PATCH] mac80211: use oneshot blink API for LED triggers Fabio Baltieri @ 2013-07-24 0:09 ` Fabio Baltieri 2013-07-24 8:14 ` Johannes Berg 0 siblings, 1 reply; 6+ messages in thread From: Fabio Baltieri @ 2013-07-24 0:09 UTC (permalink / raw) To: Johannes Berg, John W. Linville Cc: linux-wireless, netdev, linux-kernel, Fabio Baltieri Changes mac80211 LED trigger code to use the generic led_trigger_blink_oneshot() API for transmit and receive activity indication. This gives a better feedback to the user, as with the new API each activity event results in a visible blink, while a constant traffic results in a continuous blink at constant rate. Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com> --- net/mac80211/ieee80211_i.h | 1 - net/mac80211/led.c | 21 +++++++++------------ net/mac80211/led.h | 2 +- net/mac80211/status.c | 2 +- net/mac80211/tx.c | 1 - 5 files changed, 11 insertions(+), 16 deletions(-) diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h index 8412a30..1178139 100644 --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -1063,7 +1063,6 @@ struct ieee80211_local { u32 dot11TransmittedFrameCount; #ifdef CONFIG_MAC80211_LEDS - int tx_led_counter, rx_led_counter; struct led_trigger *tx_led, *rx_led, *assoc_led, *radio_led; struct tpt_led_trigger *tpt_led_trigger; char tx_led_name[32], rx_led_name[32], diff --git a/net/mac80211/led.c b/net/mac80211/led.c index bcffa69..9bce15e 100644 --- a/net/mac80211/led.c +++ b/net/mac80211/led.c @@ -12,27 +12,24 @@ #include <linux/export.h> #include "led.h" +#define MAC80211_BLINK_DELAY 50 /* ms */ + void ieee80211_led_rx(struct ieee80211_local *local) { + unsigned long led_delay = MAC80211_BLINK_DELAY; if (unlikely(!local->rx_led)) return; - if (local->rx_led_counter++ % 2 == 0) - led_trigger_event(local->rx_led, LED_OFF); - else - led_trigger_event(local->rx_led, LED_FULL); + led_trigger_blink_oneshot(local->rx_led, + &led_delay, &led_delay, 0); } -/* q is 1 if a packet was enqueued, 0 if it has been transmitted */ -void ieee80211_led_tx(struct ieee80211_local *local, int q) +void ieee80211_led_tx(struct ieee80211_local *local) { + unsigned long led_delay = MAC80211_BLINK_DELAY; if (unlikely(!local->tx_led)) return; - /* not sure how this is supposed to work ... */ - local->tx_led_counter += 2*q-1; - if (local->tx_led_counter % 2 == 0) - led_trigger_event(local->tx_led, LED_OFF); - else - led_trigger_event(local->tx_led, LED_FULL); + led_trigger_blink_oneshot(local->tx_led, + &led_delay, &led_delay, 0); } void ieee80211_led_assoc(struct ieee80211_local *local, bool associated) diff --git a/net/mac80211/led.h b/net/mac80211/led.h index e0275d9..6ca2f29 100644 --- a/net/mac80211/led.h +++ b/net/mac80211/led.h @@ -13,7 +13,7 @@ #ifdef CONFIG_MAC80211_LEDS void ieee80211_led_rx(struct ieee80211_local *local); -void ieee80211_led_tx(struct ieee80211_local *local, int q); +void ieee80211_led_tx(struct ieee80211_local *local); void ieee80211_led_assoc(struct ieee80211_local *local, bool associated); void ieee80211_led_radio(struct ieee80211_local *local, diff --git a/net/mac80211/status.c b/net/mac80211/status.c index 4343920..e3c889b 100644 --- a/net/mac80211/status.c +++ b/net/mac80211/status.c @@ -557,7 +557,7 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb) rcu_read_unlock(); - ieee80211_led_tx(local, 0); + ieee80211_led_tx(local); /* SNMP counters * Fragments are passed to low-level drivers as separate skbs, so these diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 4105d0c..6c4e60d 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -1281,7 +1281,6 @@ static bool __ieee80211_tx(struct ieee80211_local *local, txpending); ieee80211_tpt_led_trig_tx(local, fc, led_len); - ieee80211_led_tx(local, 1); WARN_ON_ONCE(!skb_queue_empty(skbs)); -- 1.8.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mac80211: use oneshot blink API for LED triggers 2013-07-24 0:09 ` Fabio Baltieri @ 2013-07-24 8:14 ` Johannes Berg 2013-07-24 8:53 ` Fabio Baltieri 0 siblings, 1 reply; 6+ messages in thread From: Johannes Berg @ 2013-07-24 8:14 UTC (permalink / raw) To: Fabio Baltieri; +Cc: John W. Linville, linux-wireless, netdev, linux-kernel On Wed, 2013-07-24 at 02:09 +0200, Fabio Baltieri wrote: > Changes mac80211 LED trigger code to use the generic > led_trigger_blink_oneshot() API for transmit and receive activity > indication. > > This gives a better feedback to the user, as with the new API each > activity event results in a visible blink, while a constant traffic > results in a continuous blink at constant rate. This seems a little pointless since our throughput-based trigger can do very similar (but somewhat better) behaviour? Maybe that should just be the default instead, with some sane default setup values? (Regardless of that, you also have indentation problems in your patch) johannes ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mac80211: use oneshot blink API for LED triggers 2013-07-24 8:14 ` Johannes Berg @ 2013-07-24 8:53 ` Fabio Baltieri [not found] ` <20130724085337.GA7096-lYxsXyORJ9v/PtFMR13I2A@public.gmane.org> 0 siblings, 1 reply; 6+ messages in thread From: Fabio Baltieri @ 2013-07-24 8:53 UTC (permalink / raw) To: Johannes Berg; +Cc: John W. Linville, linux-wireless, netdev, linux-kernel On Wed, Jul 24, 2013 at 10:14:25AM +0200, Johannes Berg wrote: > On Wed, 2013-07-24 at 02:09 +0200, Fabio Baltieri wrote: > > Changes mac80211 LED trigger code to use the generic > > led_trigger_blink_oneshot() API for transmit and receive activity > > indication. > > > > This gives a better feedback to the user, as with the new API each > > activity event results in a visible blink, while a constant traffic > > results in a continuous blink at constant rate. > > This seems a little pointless since our throughput-based trigger can do > very similar (but somewhat better) behaviour? Maybe that should just be > the default instead, with some sane default setup values? Ok but that requires driver specific support and it's only implemented on a subset of currently available drivers. This at least makes the basic tx/rx indication capability a better. > (Regardless of that, you also have indentation problems in your patch) Ok, I guess you are referring to the this: + led_trigger_blink_oneshot(local->tx_led, + &led_delay, &led_delay, 0); So, are you definitely rejecting this patch or should I fix indentation and send a v2? Thanks, Fabio -- Fabio Baltieri ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <20130724085337.GA7096-lYxsXyORJ9v/PtFMR13I2A@public.gmane.org>]
* Re: [PATCH] mac80211: use oneshot blink API for LED triggers [not found] ` <20130724085337.GA7096-lYxsXyORJ9v/PtFMR13I2A@public.gmane.org> @ 2013-07-25 7:58 ` Johannes Berg 2013-07-25 9:58 ` Fabio Baltieri 0 siblings, 1 reply; 6+ messages in thread From: Johannes Berg @ 2013-07-25 7:58 UTC (permalink / raw) To: Fabio Baltieri Cc: John W. Linville, linux-wireless-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Wed, 2013-07-24 at 10:53 +0200, Fabio Baltieri wrote: > On Wed, Jul 24, 2013 at 10:14:25AM +0200, Johannes Berg wrote: > > On Wed, 2013-07-24 at 02:09 +0200, Fabio Baltieri wrote: > > > Changes mac80211 LED trigger code to use the generic > > > led_trigger_blink_oneshot() API for transmit and receive activity > > > indication. > > > > > > This gives a better feedback to the user, as with the new API each > > > activity event results in a visible blink, while a constant traffic > > > results in a continuous blink at constant rate. > > > > This seems a little pointless since our throughput-based trigger can do > > very similar (but somewhat better) behaviour? Maybe that should just be > > the default instead, with some sane default setup values? > > Ok but that requires driver specific support and it's only implemented > on a subset of currently available drivers. Not necessarily, it's just done that way because we expected everyone to have their own idea of what the blink speeds should be, possibly even depending on the maximum speed the hardware can do etc. > This at least makes the basic tx/rx indication capability a better. Fair enough. > > (Regardless of that, you also have indentation problems in your patch) > > Ok, I guess you are referring to the this: > > + led_trigger_blink_oneshot(local->tx_led, > + &led_delay, &led_delay, 0); Yes, that was the place I noticed, didn't check more closely. > So, are you definitely rejecting this patch or should I fix indentation > and send a v2? I think I'll take it, I kinda hope nobody will really care much about it but the behaviour looks better and the code is simpler, so ... :) johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mac80211: use oneshot blink API for LED triggers 2013-07-25 7:58 ` Johannes Berg @ 2013-07-25 9:58 ` Fabio Baltieri 0 siblings, 0 replies; 6+ messages in thread From: Fabio Baltieri @ 2013-07-25 9:58 UTC (permalink / raw) To: Johannes Berg; +Cc: John W. Linville, linux-wireless, netdev, linux-kernel On Thu, Jul 25, 2013 at 09:58:21AM +0200, Johannes Berg wrote: > > So, are you definitely rejecting this patch or should I fix indentation > > and send a v2? > > I think I'll take it, I kinda hope nobody will really care much about it > but the behaviour looks better and the code is simpler, so ... :) Good enough... I'll send the v2 then. :-) Thanks, Fabio -- Fabio Baltieri ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-07-25 9:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-24 0:09 [PATCH] mac80211: use oneshot blink API for LED triggers Fabio Baltieri
2013-07-24 0:09 ` Fabio Baltieri
2013-07-24 8:14 ` Johannes Berg
2013-07-24 8:53 ` Fabio Baltieri
[not found] ` <20130724085337.GA7096-lYxsXyORJ9v/PtFMR13I2A@public.gmane.org>
2013-07-25 7:58 ` Johannes Berg
2013-07-25 9:58 ` Fabio Baltieri
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).