netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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	[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

* 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).