* [Patch 1/1] media: ti-vpe: cal: fix disable_irqs to only the intended target
@ 2020-03-02 13:56 Benoit Parrot
2020-03-04 8:49 ` Tomi Valkeinen
2020-03-04 15:35 ` Sasha Levin
0 siblings, 2 replies; 4+ messages in thread
From: Benoit Parrot @ 2020-03-02 13:56 UTC (permalink / raw)
To: Hans Verkuil
Cc: linux-media, devicetree, linux-kernel, Tomi Valkeinen,
Benoit Parrot, stable
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);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Patch 1/1] media: ti-vpe: cal: fix disable_irqs to only the intended target
2020-03-02 13:56 [Patch 1/1] media: ti-vpe: cal: fix disable_irqs to only the intended target Benoit Parrot
@ 2020-03-04 8:49 ` Tomi Valkeinen
2020-03-04 13:17 ` Benoit Parrot
2020-03-04 15:35 ` Sasha Levin
1 sibling, 1 reply; 4+ messages in thread
From: Tomi Valkeinen @ 2020-03-04 8:49 UTC (permalink / raw)
To: Benoit Parrot, Hans Verkuil; +Cc: linux-media, devicetree, linux-kernel, stable
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Patch 1/1] media: ti-vpe: cal: fix disable_irqs to only the intended target
2020-03-04 8:49 ` Tomi Valkeinen
@ 2020-03-04 13:17 ` Benoit Parrot
0 siblings, 0 replies; 4+ messages in thread
From: Benoit Parrot @ 2020-03-04 13:17 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Hans Verkuil, linux-media, devicetree, linux-kernel, stable
Tomi,
Thanks for the review.
Tomi Valkeinen <tomi.valkeinen@ti.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@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.
>
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Patch 1/1] media: ti-vpe: cal: fix disable_irqs to only the intended target
2020-03-02 13:56 [Patch 1/1] media: ti-vpe: cal: fix disable_irqs to only the intended target Benoit Parrot
2020-03-04 8:49 ` Tomi Valkeinen
@ 2020-03-04 15:35 ` Sasha Levin
1 sibling, 0 replies; 4+ messages in thread
From: Sasha Levin @ 2020-03-04 15:35 UTC (permalink / raw)
To: Sasha Levin, Benoit Parrot, Hans Verkuil
Cc: linux-media, devicetree, stable, stable
Hi
[This is an automated email]
This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all
The bot has tested the following trees: v5.5.7, v5.4.23, v4.19.107, v4.14.172, v4.9.215, v4.4.215.
v5.5.7: Build OK!
v5.4.23: Build OK!
v4.19.107: Build OK!
v4.14.172: Build OK!
v4.9.215: Build OK!
v4.4.215: Failed to apply! Possible dependencies:
343e89a792a5 ("[media] media: ti-vpe: Add CAL v4l2 camera capture driver")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
--
Thanks
Sasha
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-03-04 15:36 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-02 13:56 [Patch 1/1] media: ti-vpe: cal: fix disable_irqs to only the intended target Benoit Parrot
2020-03-04 8:49 ` Tomi Valkeinen
2020-03-04 13:17 ` Benoit Parrot
2020-03-04 15:35 ` Sasha Levin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.