[REVIEW PATCH 0/2] img-ir: Some more fixes

[REVIEW PATCH 0/2] img-ir: Some more fixes

[James Hogan]

A few more fixes for the img-ir RC driver in addition to the ones I
posted a couple of weeks ago.

The first patch fixes some broken behaviour when the same protocol is
set twice and the effective scancode filter gets cleared as a result.

The second patch fixes a potential deadlock and lockdep splat due to the
repeat end timer being del_timer_sync'd with the spin lock held when
changing protocols.

The second patch here depends on "img-ir/hw: Always read data to clear
buffer" (patch 1 in the previous img-ir patchset) to avoid conflicts, so
that patch should be applied first.

I've tagged both for stable (v3.15+).

Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
Cc: Sifan Naeem <sifan.naeem@imgtec.com>
Cc: linux-media@vger.kernel.org

James Hogan (2):
  img-ir/hw: Avoid clearing filter for no-op protocol change
  img-ir/hw: Fix potential deadlock stopping timer

 drivers/media/rc/img-ir/img-ir-hw.c | 28 +++++++++++++++++++++++++---
 drivers/media/rc/img-ir/img-ir-hw.h |  3 +++
 2 files changed, 28 insertions(+), 3 deletions(-)

-- 
2.0.4

[REVIEW PATCH 1/2] img-ir/hw: Avoid clearing filter for no-op protocol change

[James Hogan]

When the img-ir driver is asked to change protocol, if the chosen
decoder is already loaded then don't call img_ir_set_decoder(), so as
not to clear the current filter.

This is important because store_protocol() does not refresh the scancode
filter with the new protocol if the set of enabled protocols hasn't
actually changed, but it will still call the change_protocol() callback,
resulting in the filter being disabled in the hardware.

The problem can be reproduced by setting a filter, and then setting the
protocol to the same protocol that is already set:
$ echo nec > protocols
$ echo 0xffff > filter_mask
$ echo nec > protocols

After this, messages which don't match the filter still get received.

Reported-by: Sifan Naeem <sifan.naeem@imgtec.com>
Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
Cc: <stable@vger.kernel.org> # v3.15+
Cc: linux-media@vger.kernel.org
---
 drivers/media/rc/img-ir/img-ir-hw.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/rc/img-ir/img-ir-hw.c b/drivers/media/rc/img-ir/img-ir-hw.c
index 9db065344b41..1566337c1059 100644
--- a/drivers/media/rc/img-ir/img-ir-hw.c
+++ b/drivers/media/rc/img-ir/img-ir-hw.c
@@ -643,6 +643,12 @@ static int img_ir_change_protocol(struct rc_dev *dev, u64 *ir_type)
 			continue;
 		if (*ir_type & dec->type) {
 			*ir_type &= dec->type;
+			/*
+			 * We don't want to clear the filter if nothing is
+			 * changing as it won't get set again.
+			 */
+			if (dec == hw->decoder)
+				return 0;
 			img_ir_set_decoder(priv, dec, *ir_type);
 			goto success;
 		}
-- 
2.0.4

[REVIEW PATCH 2/2] img-ir/hw: Fix potential deadlock stopping timer

[James Hogan]

The end timer is used for switching back from repeat code timings when
no repeat codes have been received for a certain amount of time. When
the protocol is changed, the end timer is deleted synchronously with
del_timer_sync(), however this takes place while holding the main spin
lock, and the timer handler also needs to acquire the spin lock.

This opens the possibility of a deadlock on an SMP system if the
protocol is changed just as the repeat timer is expiring. One CPU could
end up in img_ir_set_decoder() holding the lock and waiting for the end
timer to complete, while the other CPU is stuck in the timer handler
spinning on the lock held by the first CPU.

Lockdep also spots a possible lock inversion in the same code, since
img_ir_set_decoder() acquires the img-ir lock before the timer lock, but
the timer handler will try and acquire them the other way around:

=========================================================
[ INFO: possible irq lock inversion dependency detected ]
3.18.0-rc5+ #957 Not tainted
---------------------------------------------------------
swapper/0/0 just changed the state of lock:
 (((&hw->end_timer))){+.-...}, at: [<4006ae5c>] _call_timer_fn+0x0/0xfc
but this lock was taken by another, HARDIRQ-safe lock in the past:
 (&(&priv->lock)->rlock#2){-.....}

and interrupts could create inverse lock ordering between them.

other info that might help us debug this:
 Possible interrupt unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(((&hw->end_timer)));
                               local_irq_disable();
                               lock(&(&priv->lock)->rlock#2);
                               lock(((&hw->end_timer)));
  <Interrupt>
    lock(&(&priv->lock)->rlock#2);

 *** DEADLOCK ***

This is fixed by releasing the main spin lock while performing the
del_timer_sync() call. The timer is prevented from restarting before the
lock is reacquired by a new "stopping" flag which img_ir_handle_data()
checks before updating the timer.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
Cc: Sifan Naeem <sifan.naeem@imgtec.com>
Cc: <stable@vger.kernel.org> # v3.15+
Cc: linux-media@vger.kernel.org
---
This patch depends on "img-ir/hw: Always read data to clear buffer" from
my recent img-ir patchset to avoid a conflict.
---
 drivers/media/rc/img-ir/img-ir-hw.c | 22 +++++++++++++++++++---
 drivers/media/rc/img-ir/img-ir-hw.h |  3 +++
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/media/rc/img-ir/img-ir-hw.c b/drivers/media/rc/img-ir/img-ir-hw.c
index 1566337c1059..76eaa323ae51 100644
--- a/drivers/media/rc/img-ir/img-ir-hw.c
+++ b/drivers/media/rc/img-ir/img-ir-hw.c
@@ -530,6 +530,22 @@ static void img_ir_set_decoder(struct img_ir_priv *priv,
 	u32 ir_status, irq_en;
 	spin_lock_irq(&priv->lock);
 
+	/*
+	 * First record that the protocol is being stopped so that the end timer
+	 * isn't restarted while we're trying to stop it.
+	 */
+	hw->stopping = true;
+
+	/*
+	 * Release the lock to stop the end timer, since the end timer handler
+	 * acquires the lock and we don't want to deadlock waiting for it.
+	 */
+	spin_unlock_irq(&priv->lock);
+	del_timer_sync(&hw->end_timer);
+	spin_lock_irq(&priv->lock);
+
+	hw->stopping = false;
+
 	/* switch off and disable interrupts */
 	img_ir_write(priv, IMG_IR_CONTROL, 0);
 	irq_en = img_ir_read(priv, IMG_IR_IRQ_ENABLE);
@@ -547,8 +563,7 @@ static void img_ir_set_decoder(struct img_ir_priv *priv,
 	img_ir_read(priv, IMG_IR_DATA_LW);
 	img_ir_read(priv, IMG_IR_DATA_UP);
 
-	/* stop the end timer and switch back to normal mode */
-	del_timer_sync(&hw->end_timer);
+	/* switch back to normal mode */
 	hw->mode = IMG_IR_M_NORMAL;
 
 	/* clear the wakeup scancode filter */
@@ -825,7 +840,8 @@ static void img_ir_handle_data(struct img_ir_priv *priv, u32 len, u64 raw)
 	}
 
 
-	if (dec->repeat) {
+	/* we mustn't update the end timer while trying to stop it */
+	if (dec->repeat && !hw->stopping) {
 		unsigned long interval;
 
 		img_ir_begin_repeat(priv);
diff --git a/drivers/media/rc/img-ir/img-ir-hw.h b/drivers/media/rc/img-ir/img-ir-hw.h
index a8c6a8d40206..5c2b216c5fe3 100644
--- a/drivers/media/rc/img-ir/img-ir-hw.h
+++ b/drivers/media/rc/img-ir/img-ir-hw.h
@@ -211,6 +211,8 @@ enum img_ir_mode {
  * @flags:		IMG_IR_F_*.
  * @filters:		HW filters (derived from scancode filters).
  * @mode:		Current decode mode.
+ * @stopping:		Indicates that decoder is being taken down and timers
+ *			should not be restarted.
  * @suspend_irqen:	Saved IRQ enable mask over suspend.
  */
 struct img_ir_priv_hw {
@@ -226,6 +228,7 @@ struct img_ir_priv_hw {
 	struct img_ir_filter		filters[RC_FILTER_MAX];
 
 	enum img_ir_mode		mode;
+	bool				stopping;
 	u32				suspend_irqen;
 };
 
-- 
2.0.4

Re: [REVIEW PATCH 1/2] img-ir/hw: Avoid clearing filter for no-op protocol change

[Mauro Carvalho Chehab]

Em Mon, 1 Dec 2014 12:55:09 +0000
James Hogan <james.hogan@imgtec.com> escreveu:

> When the img-ir driver is asked to change protocol, if the chosen
> decoder is already loaded then don't call img_ir_set_decoder(), so as
> not to clear the current filter.
> 
> This is important because store_protocol() does not refresh the scancode
> filter with the new protocol if the set of enabled protocols hasn't
> actually changed, but it will still call the change_protocol() callback,
> resulting in the filter being disabled in the hardware.
> 
> The problem can be reproduced by setting a filter, and then setting the
> protocol to the same protocol that is already set:
> $ echo nec > protocols
> $ echo 0xffff > filter_mask
> $ echo nec > protocols
> 
> After this, messages which don't match the filter still get received.

This should be fixed at the RC core, as this is not driver-specific.

Regards,
Mauro

> 
> Reported-by: Sifan Naeem <sifan.naeem@imgtec.com>
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
> Cc: <stable@vger.kernel.org> # v3.15+
> Cc: linux-media@vger.kernel.org
> ---
>  drivers/media/rc/img-ir/img-ir-hw.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/media/rc/img-ir/img-ir-hw.c b/drivers/media/rc/img-ir/img-ir-hw.c
> index 9db065344b41..1566337c1059 100644
> --- a/drivers/media/rc/img-ir/img-ir-hw.c
> +++ b/drivers/media/rc/img-ir/img-ir-hw.c
> @@ -643,6 +643,12 @@ static int img_ir_change_protocol(struct rc_dev *dev, u64 *ir_type)
>  			continue;
>  		if (*ir_type & dec->type) {
>  			*ir_type &= dec->type;
> +			/*
> +			 * We don't want to clear the filter if nothing is
> +			 * changing as it won't get set again.
> +			 */
> +			if (dec == hw->decoder)
> +				return 0;
>  			img_ir_set_decoder(priv, dec, *ir_type);
>  			goto success;
>  		}

Re: [REVIEW PATCH 1/2] img-ir/hw: Avoid clearing filter for no-op protocol change

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 1276 bytes --]

Hi Mauro,

On 04/12/14 17:38, Mauro Carvalho Chehab wrote:
> Em Mon, 1 Dec 2014 12:55:09 +0000
> James Hogan <james.hogan@imgtec.com> escreveu:
> 
>> When the img-ir driver is asked to change protocol, if the chosen
>> decoder is already loaded then don't call img_ir_set_decoder(), so as
>> not to clear the current filter.
>>
>> This is important because store_protocol() does not refresh the scancode
>> filter with the new protocol if the set of enabled protocols hasn't
>> actually changed, but it will still call the change_protocol() callback,
>> resulting in the filter being disabled in the hardware.
>>
>> The problem can be reproduced by setting a filter, and then setting the
>> protocol to the same protocol that is already set:
>> $ echo nec > protocols
>> $ echo 0xffff > filter_mask
>> $ echo nec > protocols
>>
>> After this, messages which don't match the filter still get received.
> 
> This should be fixed at the RC core, as this is not driver-specific.

Yes, you're right. I've fixed there and attempted backporting, and the
problem appears to have actually been introduced in commit da6e162d6a46
("[media] rc-core: simplify sysfs code") which went into v3.17.

I'll send a v2.

Thanks
James

> 
> Regards,
> Mauro


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]