[PATCH/RFC] rfkill: prevent unnecessary event generation

[PATCH/RFC] rfkill: prevent unnecessary event generation

[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 <vitalywool@gmail.com>
Signed-off-by: Mykyta Iziumtsev <nikita.izyumtsev@gmail.com>

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

diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index 752b723..520617c 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -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
-- 
1.7.5.4

Re: [PATCH/RFC] rfkill: prevent unnecessary event generation

[Johannes Berg]

On Thu, 2012-09-06 at 16:06 +0200, Vitaly Wool wrote:
> 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 <vitalywool@gmail.com>
> Signed-off-by: Mykyta Iziumtsev <nikita.izyumtsev@gmail.com>

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

johannes


> ---
>   net/rfkill/core.c |    8 +++++++-
>   1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/net/rfkill/core.c b/net/rfkill/core.c
> index 752b723..520617c 100644
> --- a/net/rfkill/core.c
> +++ b/net/rfkill/core.c
> @@ -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

Re: [PATCH/RFC] rfkill: prevent unnecessary event generation

[Vitaly Wool]

Hi Johannes,

On Thu, Sep 6, 2012 at 5:03 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Thu, 2012-09-06 at 16:06 +0200, Vitaly Wool wrote:
> > 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 <vitalywool@gmail.com>
> > Signed-off-by: Mykyta Iziumtsev <nikita.izyumtsev@gmail.com>
>
> Looks fine to me, should I just pick it up?
>

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

~Vitaly

Re: [PATCH/RFC] rfkill: prevent unnecessary event generation

[Henrique de Moraes Holschuh]

On Thu, 06 Sep 2012, Vitaly Wool wrote:
> 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?

> diff --git a/net/rfkill/core.c b/net/rfkill/core.c
> index 752b723..520617c 100644
> --- a/net/rfkill/core.c
> +++ b/net/rfkill/core.c
> @@ -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

-- 
  "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

Re: [PATCH/RFC] rfkill: prevent unnecessary event generation

[Vitaly Wool]

Hi Henrique,

On Fri, Sep 7, 2012 at 12:44 AM, Henrique de Moraes Holschuh
<hmh@hmh.eng.br> wrote:
>
> On Thu, 06 Sep 2012, Vitaly Wool wrote:
> > 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

Re: [PATCH/RFC] rfkill: prevent unnecessary event generation

[Johannes Berg]

On Fri, 2012-09-07 at 08:54 +0200, Vitaly Wool wrote:
> Hi Henrique,
> 
> On Fri, Sep 7, 2012 at 12:44 AM, Henrique de Moraes Holschuh
> <hmh@hmh.eng.br> wrote:
> >
> > On Thu, 06 Sep 2012, Vitaly Wool wrote:
> > > 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.

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

johannes

Re: [PATCH/RFC] rfkill: prevent unnecessary event generation

[Vitaly Wool]

On Fri, Sep 7, 2012 at 10:21 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Fri, 2012-09-07 at 08:54 +0200, Vitaly Wool wrote:
>> Hi Henrique,
>>
>> On Fri, Sep 7, 2012 at 12:44 AM, Henrique de Moraes Holschuh
>> <hmh@hmh.eng.br> wrote:
>> >
>> > On Thu, 06 Sep 2012, Vitaly Wool wrote:
>> > > 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.
>
> 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

Re: [PATCH/RFC] rfkill: prevent unnecessary event generation

[Johannes Berg]

On Fri, 2012-09-07 at 15:02 +0200, Vitaly Wool wrote:

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

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

johannes

Re: [PATCH/RFC] rfkill: prevent unnecessary event generation

[Vitaly Wool]

Hi Johannes,

On Fri, Sep 7, 2012 at 3:04 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2012-09-07 at 15:02 +0200, Vitaly Wool wrote:
>
>> >> 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.
>
> 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

Re: [PATCH/RFC] rfkill: prevent unnecessary event generation

[Johannes Berg]

On Sat, 2012-09-22 at 21:37 +0200, Vitaly Wool wrote:
> Hi Johannes,
> 
> On Fri, Sep 7, 2012 at 3:04 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Fri, 2012-09-07 at 15:02 +0200, Vitaly Wool wrote:
> >
> >> >> 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.
> >
> > 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