rfkill: prevent unnecessary event generation

PATCH/RFC Vitaly Wool 4 participants 10 messages Sep 6, 2012 – Sep 24, 2012 Rfkill

Share & tools

Vitaly Wool

author

2012-09-06 14:16 UTC #0 lore raw

Prevent unnecessary rfkill event generation when the state has not actually changed. These events have to be delivered to relevant userspace processes, causing these processes to wake up and do something while they could as well have slept. This obviously results in more CPU usage, longer time-to-sleep-again and therefore higher power consumption.

Signed-off-by Vitaly Wool <[email protected]>

Signed-off-by Mykyta Iziumtsev <[email protected]>

---
 net/rfkill/core.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

net/rfkill/core.c +7 -1

index 752b723..520617c 100644
@@ -256,6 +256,7 @@ static bool __rfkill_set_hw_state(struct rfkill *rfkill,
 static void rfkill_set_block(struct rfkill *rfkill, bool blocked)
 {
 	unsigned long flags;
+	bool prev, curr;
 	int err;
 
 	if (unlikely(rfkill->dev.power.power_state.event & PM_EVENT_SLEEP))

@@ -270,6 +271,8 @@ static void rfkill_set_block(struct rfkill *rfkill, bool blocked)
 		rfkill->ops->query(rfkill, rfkill->data);
 
 	spin_lock_irqsave(&rfkill->lock, flags);
+	prev = rfkill->state & RFKILL_BLOCK_SW;
+
 	if (rfkill->state & RFKILL_BLOCK_SW)
 		rfkill->state |= RFKILL_BLOCK_SW_PREV;
 	else

@@ -299,10 +302,13 @@ static void rfkill_set_block(struct rfkill *rfkill, bool blocked)
 	}
 	rfkill->state &= ~RFKILL_BLOCK_SW_SETCALL;
 	rfkill->state &= ~RFKILL_BLOCK_SW_PREV;
+	curr = rfkill->state & RFKILL_BLOCK_SW;
 	spin_unlock_irqrestore(&rfkill->lock, flags);
 
 	rfkill_led_trigger_event(rfkill);
-	rfkill_event(rfkill);
+
+	if (prev != curr)
+		rfkill_event(rfkill);
 }
 
 #ifdef CONFIG_RFKILL_INPUT

Signature · 1 line

1.7.5.4

Johannes Berg

maintainer

2012-09-06 15:02 UTC #1 lore raw

Quoted 9 lines · Vitaly Wool

Prevent unnecessary rfkill event generation when the state has not actually changed. These events have to be delivered to relevant userspace processes, causing these processes to wake up and do something while they could as well have slept. This obviously results in more CPU usage, longer time-to-sleep-again and therefore higher power consumption.
Signed-off-by Vitaly Wool <[email protected]>
Signed-off-by Mykyta Iziumtsev <[email protected]>

Looks fine to me, should I just pick it up?

johannes

Quoted 40 lines

---
  net/rfkill/core.c |    8 +++++++-
  1 files changed, 7 insertions(+), 1 deletions(-)

net/rfkill/core.c +7 -1

index 752b723..520617c 100644
@@ -256,6 +256,7 @@ static bool __rfkill_set_hw_state(struct rfkill *rfkill,
  static void rfkill_set_block(struct rfkill *rfkill, bool blocked)
  {
  	unsigned long flags;
+	bool prev, curr;
  	int err;
 
  	if (unlikely(rfkill->dev.power.power_state.event & PM_EVENT_SLEEP))

@@ -270,6 +271,8 @@ static void rfkill_set_block(struct rfkill *rfkill, bool blocked)
  		rfkill->ops->query(rfkill, rfkill->data);
 
  	spin_lock_irqsave(&rfkill->lock, flags);
+	prev = rfkill->state & RFKILL_BLOCK_SW;
+
  	if (rfkill->state & RFKILL_BLOCK_SW)
  		rfkill->state |= RFKILL_BLOCK_SW_PREV;
  	else

@@ -299,10 +302,13 @@ static void rfkill_set_block(struct rfkill *rfkill, bool blocked)
  	}
  	rfkill->state &= ~RFKILL_BLOCK_SW_SETCALL;
  	rfkill->state &= ~RFKILL_BLOCK_SW_PREV;
+	curr = rfkill->state & RFKILL_BLOCK_SW;
  	spin_unlock_irqrestore(&rfkill->lock, flags);
 
  	rfkill_led_trigger_event(rfkill);
-	rfkill_event(rfkill);
+
+	if (prev != curr)
+		rfkill_event(rfkill);
  }
 
  #ifdef CONFIG_RFKILL_INPUT

Vitaly Wool

2012-09-06 20:34 UTC #2 lore raw

Hi Johannes,

Quoted 9 lines · Johannes Berg

Quoted 7 lines · Vitaly Wool
Prevent unnecessary rfkill event generation when the state has not actually changed. These events have to be delivered to relevant userspace processes, causing these processes to wake up and do something while they could as well have slept. This obviously results in more CPU usage, longer time-to-sleep-again and therefore higher power consumption.

Quoted 5 lines

Quoted 2 lines
Signed-off-by Vitaly Wool <[email protected]>
Signed-off-by Mykyta Iziumtsev <[email protected]>
Looks fine to me, should I just pick it up?

if it's fine with you, yes, please do so :)

~Vitaly

Henrique de Moraes Holschuh

2012-09-06 22:44 UTC #3 lore raw

In reply to Vitaly Wool

Quoted 6 lines · Vitaly Wool

Prevent unnecessary rfkill event generation when the state has not actually changed. These events have to be delivered to relevant userspace processes, causing these processes to wake up and do something while they could as well have slept. This obviously results in more CPU usage, longer time-to-sleep-again and therefore higher power consumption.

Could this supress the first event when a switch is registered? Would that be a concern?

Quoted 32 lines

net/rfkill/core.c +7 -1

index 752b723..520617c 100644
@@ -256,6 +256,7 @@ static bool __rfkill_set_hw_state(struct rfkill *rfkill,
 static void rfkill_set_block(struct rfkill *rfkill, bool blocked)
 {
 	unsigned long flags;
+	bool prev, curr;
 	int err;
 	if (unlikely(rfkill->dev.power.power_state.event & PM_EVENT_SLEEP))

@@ -270,6 +271,8 @@ static void rfkill_set_block(struct rfkill *rfkill, bool blocked)
 		rfkill->ops->query(rfkill, rfkill->data);
 	spin_lock_irqsave(&rfkill->lock, flags);
+	prev = rfkill->state & RFKILL_BLOCK_SW;
+
 	if (rfkill->state & RFKILL_BLOCK_SW)
 		rfkill->state |= RFKILL_BLOCK_SW_PREV;
 	else

@@ -299,10 +302,13 @@ static void rfkill_set_block(struct rfkill *rfkill, bool blocked)
 	}
 	rfkill->state &= ~RFKILL_BLOCK_SW_SETCALL;
 	rfkill->state &= ~RFKILL_BLOCK_SW_PREV;
+	curr = rfkill->state & RFKILL_BLOCK_SW;
 	spin_unlock_irqrestore(&rfkill->lock, flags);
 	rfkill_led_trigger_event(rfkill);
-	rfkill_event(rfkill);
+
+	if (prev != curr)
+		rfkill_event(rfkill);
 }
 #ifdef CONFIG_RFKILL_INPUT

Signature · 4 lines

  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Vitaly Wool

2012-09-07 06:54 UTC #4 lore raw

Hi Henrique,

Quoted 11 lines · Henrique de Moraes Holschuh

Quoted 6 lines · Vitaly Wool
Prevent unnecessary rfkill event generation when the state has not actually changed. These events have to be delivered to relevant userspace processes, causing these processes to wake up and do something while they could as well have slept. This obviously results in more CPU usage, longer time-to-sleep-again and therefore higher power consumption.
Could this supress the first event when a switch is registered? Would that be a concern?

I'm not sure I got your question, but just in case, rfkill is initialized to all zeroes so rfkill->state is 0 and on each consequent _real_ change there (prev != curr) == true and the event will be generated.

~Vitaly

Johannes Berg

maintainer

2012-09-07 08:20 UTC #5 lore raw

Quoted 13 lines · Vitaly Wool

Hi Henrique,
Quoted 9 lines · Henrique de Moraes Holschuh
Quoted 6 lines · Vitaly Wool
Prevent unnecessary rfkill event generation when the state has not actually changed. These events have to be delivered to relevant userspace processes, causing these processes to wake up and do something while they could as well have slept. This obviously results in more CPU usage, longer time-to-sleep-again and therefore higher power consumption.

Quoted 7 lines

Quoted 2 lines
Could this supress the first event when a switch is registered? Would that be a concern?
I'm not sure I got your question, but just in case, rfkill is initialized to all zeroes so rfkill->state is 0 and on each consequent _real_ change there (prev != curr) == true and the event will be generated.

But maybe applications were expecting an event for registration? Or do we get that anyway maybe?

johannes

Vitaly Wool

2012-09-07 13:02 UTC #6 lore raw

Quoted 17 lines · Johannes Berg

Quoted 16 lines · Vitaly Wool
Hi Henrique,
Quoted 11 lines · Henrique de Moraes Holschuh
Quoted 6 lines · Vitaly Wool
Prevent unnecessary rfkill event generation when the state has not actually changed. These events have to be delivered to relevant userspace processes, causing these processes to wake up and do something while they could as well have slept. This obviously results in more CPU usage, longer time-to-sleep-again and therefore higher power consumption.
Could this supress the first event when a switch is registered? Would that be a concern?

Quoted 7 lines

Quoted 4 lines
I'm not sure I got your question, but just in case, rfkill is initialized to all zeroes so rfkill->state is 0 and on each consequent _real_ change there (prev != curr) == true and the event will be generated.
But maybe applications were expecting an event for registration? Or do we get that anyway maybe?

We should get them anyway. rfkill_fop_open() basically just pushes all the startup events into the list so the application will see all the _changes_ on registration. And at least wpa_supplicant reads out all these events on rfkill registration. If an application does something else, then I think it's buggy.

~Vitaly

Johannes Berg

maintainer

2012-09-07 13:03 UTC #7 lore raw

Quoted 5 lines · Vitaly Wool

Quoted 5 lines
Quoted 4 lines
I'm not sure I got your question, but just in case, rfkill is initialized to all zeroes so rfkill->state is 0 and on each consequent _real_ change there (prev != curr) == true and the event will be generated.

Quoted 8 lines

Quoted 2 lines
But maybe applications were expecting an event for registration? Or do we get that anyway maybe?
We should get them anyway. rfkill_fop_open() basically just pushes all the startup events into the list so the application will see all the _changes_ on registration. And at least wpa_supplicant reads out all these events on rfkill registration. If an application does something else, then I think it's buggy.

Aha, good point. I guess I'll apply the patch then :)

johannes

Vitaly Wool

2012-09-22 19:37 UTC #8 lore raw

Hi Johannes,

Quoted 10 lines · Johannes Berg

Quoted 8 lines · Vitaly Wool
Quoted 7 lines
Quoted 4 lines
I'm not sure I got your question, but just in case, rfkill is initialized to all zeroes so rfkill->state is 0 and on each consequent _real_ change there (prev != curr) == true and the event will be generated.
But maybe applications were expecting an event for registration? Or do we get that anyway maybe?

Quoted 7 lines

Quoted 5 lines
We should get them anyway. rfkill_fop_open() basically just pushes all the startup events into the list so the application will see all the _changes_ on registration. And at least wpa_supplicant reads out all these events on rfkill registration. If an application does something else, then I think it's buggy.
Aha, good point. I guess I'll apply the patch then :)

provided that I don't see the patch merged, is there anything else we should take care of before it's considered good enough?

Thanks,
Vitaly

Johannes Berg

maintainer

2012-09-24 08:38 UTC #9 lore raw

Quoted 19 lines · Vitaly Wool

Hi Johannes,
Quoted 16 lines · Johannes Berg
Quoted 13 lines · Vitaly Wool
Quoted 7 lines
Quoted 4 lines
I'm not sure I got your question, but just in case, rfkill is initialized to all zeroes so rfkill->state is 0 and on each consequent _real_ change there (prev != curr) == true and the event will be generated.
But maybe applications were expecting an event for registration? Or do we get that anyway maybe?
We should get them anyway. rfkill_fop_open() basically just pushes all the startup events into the list so the application will see all the _changes_ on registration. And at least wpa_supplicant reads out all these events on rfkill registration. If an application does something else, then I think it's buggy.

Quoted 4 lines

Quoted 1 line
Aha, good point. I guess I'll apply the patch then :)
provided that I don't see the patch merged, is there anything else we should take care of before it's considered good enough?

No, my mistake ... applied now.

johannes