* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).