linux-kernel - Re: [Patch 1/1] media: ti-vpe: cal: fix disable

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200304131714.rpykdxhgqxmaxyx5@ti.com>
Date:   Wed, 4 Mar 2020 07:17:14 -0600
From:   Benoit Parrot <bparrot@...com>
To:     Tomi Valkeinen <tomi.valkeinen@...com>
CC:     Hans Verkuil <hverkuil@...all.nl>, <linux-media@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        stable <stable@...r.kernel.org>
Subject: Re: [Patch 1/1] media: ti-vpe: cal: fix disable_irqs to only the
 intended target

Tomi,

Thanks for the review.

Tomi Valkeinen <tomi.valkeinen@...com> wrote on Wed [2020-Mar-04 10:49:55 +0200]:
> On 02/03/2020 15:56, Benoit Parrot wrote:
> > disable_irqs() was mistakenly disabling all interrupts when called.
> > This cause all port stream to stop even if only stopping one of them.
> > 
> > Cc: stable <stable@...r.kernel.org>
> > Signed-off-by: Benoit Parrot <bparrot@...com>
> > ---
> >   drivers/media/platform/ti-vpe/cal.c | 16 ++++++++--------
> >   1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> > index 6e009e479be3..6d4cbb8782ed 100644
> > --- a/drivers/media/platform/ti-vpe/cal.c
> > +++ b/drivers/media/platform/ti-vpe/cal.c
> > @@ -722,16 +722,16 @@ static void enable_irqs(struct cal_ctx *ctx)
> >   
> >   static void disable_irqs(struct cal_ctx *ctx)
> >   {
> > +	u32 val;
> > +
> >   	/* Disable IRQ_WDMA_END 0/1 */
> > -	reg_write_field(ctx->dev,
> > -			CAL_HL_IRQENABLE_CLR(2),
> > -			CAL_HL_IRQ_CLEAR,
> > -			CAL_HL_IRQ_MASK(ctx->csi2_port));
> > +	val = 0;
> > +	set_field(&val, CAL_HL_IRQ_CLEAR, CAL_HL_IRQ_MASK(ctx->csi2_port));
> > +	reg_write(ctx->dev, CAL_HL_IRQENABLE_CLR(2), val);
> >   	/* Disable IRQ_WDMA_START 0/1 */
> > -	reg_write_field(ctx->dev,
> > -			CAL_HL_IRQENABLE_CLR(3),
> > -			CAL_HL_IRQ_CLEAR,
> > -			CAL_HL_IRQ_MASK(ctx->csi2_port));
> > +	val = 0;
> > +	set_field(&val, CAL_HL_IRQ_CLEAR, CAL_HL_IRQ_MASK(ctx->csi2_port));
> > +	reg_write(ctx->dev, CAL_HL_IRQENABLE_CLR(3), val);
> >   	/* Todo: Add VC_IRQ and CSI2_COMPLEXIO_IRQ handling */
> >   	reg_write(ctx->dev, CAL_CSI2_VC_IRQENABLE(1), 0);
> >   }
> > 
> 
> I think the above works. But the enable_irqs is broken too, even if it doesn't cause any issues. Both IRQ_SET and IRQ_CLR are not supposed to be "modified" by a read-mod-write operation, but just written to.
>

Well maybe not consistent now, that disable_irqs has been modified but not
broken per se.

> The macros used also make the code very difficult to read. Something like this fixes both irq enable and disable, and makes it readable:

Ah but the mask macro are all consistent with each other, whether that
create one too many level of abstraction is a different subject. This setup
was oroginally requested by the maintainer. So I'll "fix" enable_irqs also
but I'll keep the macro as is.

Benoit

> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index c8b1290c9e2b..660653031a0b 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -707,15 +707,9 @@ static void cal_quickdump_regs(struct cal_dev *dev)
>  static void enable_irqs(struct cal_ctx *ctx)
>  {
>  	/* Enable IRQ_WDMA_END 0/1 */
> -	reg_write_field(ctx->dev,
> -			CAL_HL_IRQENABLE_SET(2),
> -			CAL_HL_IRQ_ENABLE,
> -			CAL_HL_IRQ_MASK(ctx->csi2_port));
> +	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(2), 1 << (ctx->csi2_port - 1));
>  	/* Enable IRQ_WDMA_START 0/1 */
> -	reg_write_field(ctx->dev,
> -			CAL_HL_IRQENABLE_SET(3),
> -			CAL_HL_IRQ_ENABLE,
> -			CAL_HL_IRQ_MASK(ctx->csi2_port));
> +	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(3), 1 << (ctx->csi2_port - 1));
>  	/* Todo: Add VC_IRQ and CSI2_COMPLEXIO_IRQ handling */
>  	reg_write(ctx->dev, CAL_CSI2_VC_IRQENABLE(1), 0xFF000000);
>  }
> @@ -723,15 +717,9 @@ static void enable_irqs(struct cal_ctx *ctx)
>  static void disable_irqs(struct cal_ctx *ctx)
>  {
>  	/* Disable IRQ_WDMA_END 0/1 */
> -	reg_write_field(ctx->dev,
> -			CAL_HL_IRQENABLE_CLR(2),
> -			CAL_HL_IRQ_CLEAR,
> -			CAL_HL_IRQ_MASK(ctx->csi2_port));
> +	reg_write(ctx->dev, CAL_HL_IRQENABLE_CLR(2), 1 << (ctx->csi2_port - 1));
>  	/* Disable IRQ_WDMA_START 0/1 */
> -	reg_write_field(ctx->dev,
> -			CAL_HL_IRQENABLE_CLR(3),
> -			CAL_HL_IRQ_CLEAR,
> -			CAL_HL_IRQ_MASK(ctx->csi2_port));
> +	reg_write(ctx->dev, CAL_HL_IRQENABLE_CLR(3), 1 << (ctx->csi2_port - 1));
>  	/* Todo: Add VC_IRQ and CSI2_COMPLEXIO_IRQ handling */
>  	reg_write(ctx->dev, CAL_CSI2_VC_IRQENABLE(1), 0);
>  }
> 
>  Tomi
> 
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki