Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [Patch 1/1] media: ti-vpe: cal: fix disable_irqs to only the
 intended target
To:     Benoit Parrot <bparrot@ti.com>, Hans Verkuil <hverkuil@xs4all.nl>
CC:     <linux-media@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, stable <stable@vger.kernel.org>
References: <20200302135652.9365-1-bparrot@ti.com>
From:   Tomi Valkeinen <tomi.valkeinen@ti.com>
Message-ID: <678cb62a-4fdf-3b57-2fe5-699c6bf02b2f@ti.com>
Date:   Wed, 4 Mar 2020 10:49:55 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.4.1
MIME-Version: 1.0
In-Reply-To: <20200302135652.9365-1-bparrot@ti.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@vger.kernel.org>
> Signed-off-by: Benoit Parrot <bparrot@ti.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.

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

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