| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 8 Dec 2014 17:17:52 +0000
From: James Hogan <james.hogan@...tec.com>
To: Sifan Naeem <sifan.naeem@...tec.com>, <mchehab@....samsung.com>
CC: <linux-kernel@...r.kernel.org>, <linux-media@...r.kernel.org>,
<james.hartley@...tec.com>, <ezequiel.garcia@...tec.com>
Subject: Re: [PATCH 3/5] rc: img-ir: biphase enabled with workaround
On 04/12/14 15:38, Sifan Naeem wrote:
> Biphase decoding in the current img-ir has got a quirk, where multiple
> Interrupts are generated when an incomplete IR code is received by the
> decoder.
>
> Patch adds a work around for the quirk and enables biphase decoding.
>
> Signed-off-by: Sifan Naeem <sifan.naeem@...tec.com>
> ---
> drivers/media/rc/img-ir/img-ir-hw.c | 56 +++++++++++++++++++++++++++++++++--
> drivers/media/rc/img-ir/img-ir-hw.h | 2 ++
> 2 files changed, 55 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 4a1407b..a977467 100644
> --- a/drivers/media/rc/img-ir/img-ir-hw.c
> +++ b/drivers/media/rc/img-ir/img-ir-hw.c
> @@ -52,6 +52,11 @@ static struct img_ir_decoder *img_ir_decoders[] = {
>
> #define IMG_IR_QUIRK_CODE_BROKEN 0x1 /* Decode is broken */
> #define IMG_IR_QUIRK_CODE_LEN_INCR 0x2 /* Bit length needs increment */
> +/*
> + * The decoder generates rapid interrupts without actually having
> + * received any new data after an incomplete IR code is decoded.
> + */
> +#define IMG_IR_QUIRK_CODE_IRQ 0x4
>
> /* functions for preprocessing timings, ensuring max is set */
>
> @@ -547,6 +552,7 @@ static void img_ir_set_decoder(struct img_ir_priv *priv,
>
> /* stop the end timer and switch back to normal mode */
> del_timer_sync(&hw->end_timer);
> + del_timer_sync(&hw->suspend_timer);
FYI, this'll need rebasing due to conflicting with "img-ir/hw: Fix
potential deadlock stopping timer". The new del_timer_sync will need to
be when spin lock isn't held, i.e. still next to the other one, and
don't forget to ensure that suspend_timer doesn't get started if
hw->stopping.
> hw->mode = IMG_IR_M_NORMAL;
>
> /* clear the wakeup scancode filter */
> @@ -843,6 +849,26 @@ static void img_ir_end_timer(unsigned long arg)
> spin_unlock_irq(&priv->lock);
> }
>
> +/*
> + * Timer function to re-enable the current protocol after it had been
> + * cleared when invalid interrupts were generated due to a quirk in the
> + * img-ir decoder.
> + */
> +static void img_ir_suspend_timer(unsigned long arg)
> +{
> + struct img_ir_priv *priv = (struct img_ir_priv *)arg;
> +
You should take the spin lock for most of this function now that
"img-ir/hw: Fix potential deadlock stopping timer" is applied and it is
safe to do so.
> + img_ir_write(priv, IMG_IR_IRQ_CLEAR,
> + IMG_IR_IRQ_ALL & ~IMG_IR_IRQ_EDGE);
> +
> + /* Don't set IRQ if it has changed in a different context. */
Wouldn't hurt to clarify this while you're at it (it confused me for a
moment thinking it was concerned about the enabled raw event IRQs
(IMG_IR_IRQ_EDGE) changing).
Maybe "Don't overwrite enabled valid/match IRQs if they have already
been changed by e.g. a filter change".
Should you even be clearing IRQs in that case? Maybe safer to just treat
that case as a "return immediately without touching anything" sort of
situation.
> + if ((priv->hw.suspend_irqen & IMG_IR_IRQ_EDGE) ==
> + img_ir_read(priv, IMG_IR_IRQ_ENABLE))
> + img_ir_write(priv, IMG_IR_IRQ_ENABLE, priv->hw.suspend_irqen);
> + /* enable */
> + img_ir_write(priv, IMG_IR_CONTROL, priv->hw.reg_timings.ctrl);
> +}
> +
> #ifdef CONFIG_COMMON_CLK
> static void img_ir_change_frequency(struct img_ir_priv *priv,
> struct clk_notifier_data *change)
> @@ -908,15 +934,37 @@ void img_ir_isr_hw(struct img_ir_priv *priv, u32 irq_status)
> if (!hw->decoder)
> return;
>
> + ct = hw->decoder->control.code_type;
> +
> ir_status = img_ir_read(priv, IMG_IR_STATUS);
> - if (!(ir_status & (IMG_IR_RXDVAL | IMG_IR_RXDVALD2)))
> + if (!(ir_status & (IMG_IR_RXDVAL | IMG_IR_RXDVALD2))) {
> + if (!(priv->hw.ct_quirks[ct] & IMG_IR_QUIRK_CODE_IRQ))
(I suggest adding "|| hw->stopping" to this case)
> + return;
> + /*
> + * The below functionality is added as a work around to stop
> + * multiple Interrupts generated when an incomplete IR code is
> + * received by the decoder.
> + * The decoder generates rapid interrupts without actually
> + * having received any new data. After a single interrupt it's
> + * expected to clear up, but instead multiple interrupts are
> + * rapidly generated. only way to get out of this loop is to
> + * reset the control register after a short delay.
> + */
> + img_ir_write(priv, IMG_IR_CONTROL, 0);
> + hw->suspend_irqen = img_ir_read(priv, IMG_IR_IRQ_ENABLE);
You're reusing hw->suspend_irqen. What if you get this workaround being
activated between img_ir_enable_wake() and img_ir_disable_wake()? I
suggest just using a new img_ir_priv_hw member.
The rest looks reasonable to me, even if unfortunate that it is
necessary in the first place.
Thanks for the hard work!
Cheers
James
> + img_ir_write(priv, IMG_IR_IRQ_ENABLE,
> + hw->suspend_irqen & IMG_IR_IRQ_EDGE);
> +
> + /* Timer activated to re-enable the protocol. */
> + mod_timer(&hw->suspend_timer,
> + jiffies + msecs_to_jiffies(5));
> return;
> + }
> ir_status &= ~(IMG_IR_RXDVAL | IMG_IR_RXDVALD2);
> img_ir_write(priv, IMG_IR_STATUS, ir_status);
>
> len = (ir_status & IMG_IR_RXDLEN) >> IMG_IR_RXDLEN_SHIFT;
> /* some versions report wrong length for certain code types */
> - ct = hw->decoder->control.code_type;
> if (hw->ct_quirks[ct] & IMG_IR_QUIRK_CODE_LEN_INCR)
> ++len;
>
> @@ -958,7 +1006,7 @@ static void img_ir_probe_hw_caps(struct img_ir_priv *priv)
> hw->ct_quirks[IMG_IR_CODETYPE_PULSELEN]
> |= IMG_IR_QUIRK_CODE_LEN_INCR;
> hw->ct_quirks[IMG_IR_CODETYPE_BIPHASE]
> - |= IMG_IR_QUIRK_CODE_BROKEN;
> + |= IMG_IR_QUIRK_CODE_IRQ;
> hw->ct_quirks[IMG_IR_CODETYPE_2BITPULSEPOS]
> |= IMG_IR_QUIRK_CODE_BROKEN;
> }
> @@ -977,6 +1025,8 @@ int img_ir_probe_hw(struct img_ir_priv *priv)
>
> /* Set up the end timer */
> setup_timer(&hw->end_timer, img_ir_end_timer, (unsigned long)priv);
> + setup_timer(&hw->suspend_timer, img_ir_suspend_timer,
> + (unsigned long)priv);
>
> /* Register a clock notifier */
> if (!IS_ERR(priv->clk)) {
> diff --git a/drivers/media/rc/img-ir/img-ir-hw.h b/drivers/media/rc/img-ir/img-ir-hw.h
> index 5e59e8e..8578aa7 100644
> --- a/drivers/media/rc/img-ir/img-ir-hw.h
> +++ b/drivers/media/rc/img-ir/img-ir-hw.h
> @@ -221,6 +221,7 @@ enum img_ir_mode {
> * @rdev: Remote control device
> * @clk_nb: Notifier block for clock notify events.
> * @end_timer: Timer until repeat timeout.
> + * @suspend_timer: Timer to re-enable protocol.
> * @decoder: Current decoder settings.
> * @enabled_protocols: Currently enabled protocols.
> * @clk_hz: Current core clock rate in Hz.
> @@ -235,6 +236,7 @@ struct img_ir_priv_hw {
> struct rc_dev *rdev;
> struct notifier_block clk_nb;
> struct timer_list end_timer;
> + struct timer_list suspend_timer;
> const struct img_ir_decoder *decoder;
> u64 enabled_protocols;
> unsigned long clk_hz;
>
Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)
Powered by blists - more mailing lists