[PATCH 0/2] rc-core: consistent rc_repeat() usage

[PATCH 0/2] rc-core: consistent rc_repeat() usage

[David Härdeman]

These two patches are a reworked version of these two patches:
http://www.mail-archive.com/linux-media@vger.kernel.org/msg112170.html
http://www.mail-archive.com/linux-media@vger.kernel.org/msg112163.html

The first patch changes the NEC and Sanyo decoders as discussed in those
threads, moving the keydown check into rc_repeat() where the proper
locking is done.

The second patch I'd consider optional, it removes the input events for
repeat messages which I consider to be rather pointless.

---

David Härdeman (2):
      rc-core: consistent use of rc_repeat()
      rc-main: remove input events for repeat messages


 drivers/media/rc/ir-nec-decoder.c   |   10 +++-------
 drivers/media/rc/ir-sanyo-decoder.c |   10 +++-------
 drivers/media/rc/rc-main.c          |   13 ++++---------
 3 files changed, 10 insertions(+), 23 deletions(-)

--
David Härdeman

[PATCH 1/2] rc-core: consistent use of rc_repeat()

[David Härdeman]

The NEC decoder and the Sanyo decoders check if dev->keypressed is true
before calling rc_repeat (without holding dev->keylock).

Meanwhile, the XMP and JVC decoders do no such checks.

This patch makes sure all users of rc_repeat() do so consistently by removing
extra checks in NEC/Sanyo and modifying the check a bit in rc_repeat() so that
no input event is generated if the key isn't pressed.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/ir-nec-decoder.c   |   10 +++-------
 drivers/media/rc/ir-sanyo-decoder.c |   10 +++-------
 drivers/media/rc/rc-main.c          |    6 +++---
 3 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/drivers/media/rc/ir-nec-decoder.c b/drivers/media/rc/ir-nec-decoder.c
index 3ce850314dca..75b9137f6faf 100644
--- a/drivers/media/rc/ir-nec-decoder.c
+++ b/drivers/media/rc/ir-nec-decoder.c
@@ -88,13 +88,9 @@ static int ir_nec_decode(struct rc_dev *dev, struct ir_raw_event ev)
 			data->state = STATE_BIT_PULSE;
 			return 0;
 		} else if (eq_margin(ev.duration, NEC_REPEAT_SPACE, NEC_UNIT / 2)) {
-			if (!dev->keypressed) {
-				IR_dprintk(1, "Discarding last key repeat: event after key up\n");
-			} else {
-				rc_repeat(dev);
-				IR_dprintk(1, "Repeat last key\n");
-				data->state = STATE_TRAILER_PULSE;
-			}
+			rc_repeat(dev);
+			IR_dprintk(1, "Repeat last key\n");
+			data->state = STATE_TRAILER_PULSE;
 			return 0;
 		}
 
diff --git a/drivers/media/rc/ir-sanyo-decoder.c b/drivers/media/rc/ir-sanyo-decoder.c
index 520bb77dcb62..e6a906a34f90 100644
--- a/drivers/media/rc/ir-sanyo-decoder.c
+++ b/drivers/media/rc/ir-sanyo-decoder.c
@@ -110,13 +110,9 @@ static int ir_sanyo_decode(struct rc_dev *dev, struct ir_raw_event ev)
 			break;
 
 		if (!data->count && geq_margin(ev.duration, SANYO_REPEAT_SPACE, SANYO_UNIT / 2)) {
-			if (!dev->keypressed) {
-				IR_dprintk(1, "SANYO discarding last key repeat: event after key up\n");
-			} else {
-				rc_repeat(dev);
-				IR_dprintk(1, "SANYO repeat last key\n");
-				data->state = STATE_INACTIVE;
-			}
+			rc_repeat(dev);
+			IR_dprintk(1, "SANYO repeat last key\n");
+			data->state = STATE_INACTIVE;
 			return 0;
 		}
 
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index a9eba0013525..7387bd4d75b0 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -616,12 +616,12 @@ void rc_repeat(struct rc_dev *dev)
 
 	spin_lock_irqsave(&dev->keylock, flags);
 
-	input_event(dev->input_dev, EV_MSC, MSC_SCAN, dev->last_scancode);
-	input_sync(dev->input_dev);
-
 	if (!dev->keypressed)
 		goto out;
 
+	input_event(dev->input_dev, EV_MSC, MSC_SCAN, dev->last_scancode);
+	input_sync(dev->input_dev);
+
 	dev->keyup_jiffies = jiffies + msecs_to_jiffies(IR_KEYPRESS_TIMEOUT);
 	mod_timer(&dev->timer_keyup, dev->keyup_jiffies);
 

[PATCH 2/2] rc-main: remove input events for repeat messages

[David Härdeman]

Protocols like NEC generate around 10 repeat events per second.

The input events are not very useful for userspace but still waste power
by waking up every listener. So let's remove them (MSC_SCAN events
are still generated for the initial keypress).

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/rc-main.c |   13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 7387bd4d75b0..9f490aa11bc4 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -616,16 +616,11 @@ void rc_repeat(struct rc_dev *dev)
 
 	spin_lock_irqsave(&dev->keylock, flags);
 
-	if (!dev->keypressed)
-		goto out;
-
-	input_event(dev->input_dev, EV_MSC, MSC_SCAN, dev->last_scancode);
-	input_sync(dev->input_dev);
-
-	dev->keyup_jiffies = jiffies + msecs_to_jiffies(IR_KEYPRESS_TIMEOUT);
-	mod_timer(&dev->timer_keyup, dev->keyup_jiffies);
+	if (dev->keypressed) {
+		dev->keyup_jiffies = jiffies + msecs_to_jiffies(IR_KEYPRESS_TIMEOUT);
+		mod_timer(&dev->timer_keyup, dev->keyup_jiffies);
+	}
 
-out:
 	spin_unlock_irqrestore(&dev->keylock, flags);
 }
 EXPORT_SYMBOL_GPL(rc_repeat);

Re: [PATCH 2/2] rc-main: remove input events for repeat messages

[Sean Young]

On Thu, Jun 22, 2017 at 09:24:00PM +0200, David Härdeman wrote:
> Protocols like NEC generate around 10 repeat events per second.
> 
> The input events are not very useful for userspace but still waste power
> by waking up every listener. So let's remove them (MSC_SCAN events
> are still generated for the initial keypress).
> 
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
>  drivers/media/rc/rc-main.c |   13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 7387bd4d75b0..9f490aa11bc4 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -616,16 +616,11 @@ void rc_repeat(struct rc_dev *dev)
>  
>  	spin_lock_irqsave(&dev->keylock, flags);
>  
> -	if (!dev->keypressed)
> -		goto out;
> -
> -	input_event(dev->input_dev, EV_MSC, MSC_SCAN, dev->last_scancode);
> -	input_sync(dev->input_dev);

I don't agree with this. It's good to see something in user space when
a repeat received. This is useful for debugging purposes.


Sean

> -
> -	dev->keyup_jiffies = jiffies + msecs_to_jiffies(IR_KEYPRESS_TIMEOUT);
> -	mod_timer(&dev->timer_keyup, dev->keyup_jiffies);
> +	if (dev->keypressed) {
> +		dev->keyup_jiffies = jiffies + msecs_to_jiffies(IR_KEYPRESS_TIMEOUT);
> +		mod_timer(&dev->timer_keyup, dev->keyup_jiffies);
> +	}
>  
> -out:
>  	spin_unlock_irqrestore(&dev->keylock, flags);
>  }
>  EXPORT_SYMBOL_GPL(rc_repeat);

Re: [PATCH 2/2] rc-main: remove input events for repeat messages

[David Härdeman]

On Sat, Jul 01, 2017 at 01:20:50PM +0100, Sean Young wrote:
>On Thu, Jun 22, 2017 at 09:24:00PM +0200, David Härdeman wrote:
>> Protocols like NEC generate around 10 repeat events per second.
>> 
>> The input events are not very useful for userspace but still waste power
>> by waking up every listener. So let's remove them (MSC_SCAN events
>> are still generated for the initial keypress).
>> 
>> Signed-off-by: David Härdeman <david@hardeman.nu>
>> ---
>>  drivers/media/rc/rc-main.c |   13 ++++---------
>>  1 file changed, 4 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
>> index 7387bd4d75b0..9f490aa11bc4 100644
>> --- a/drivers/media/rc/rc-main.c
>> +++ b/drivers/media/rc/rc-main.c
>> @@ -616,16 +616,11 @@ void rc_repeat(struct rc_dev *dev)
>>  
>>  	spin_lock_irqsave(&dev->keylock, flags);
>>  
>> -	if (!dev->keypressed)
>> -		goto out;
>> -
>> -	input_event(dev->input_dev, EV_MSC, MSC_SCAN, dev->last_scancode);
>> -	input_sync(dev->input_dev);
>
>I don't agree with this. It's good to see something in user space when
>a repeat received. This is useful for debugging purposes.

Not going to press the issue, but dev_dbg might be another option if
debugging is the intended use-case?

-- 
David Härdeman