This fix an error handling bug reported by Beniamino, this is based on
mtk intpol patches [1]
Joe.C
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305808.html
====================
of_io_request_and_map() return ERR_PTR wrapped error code instead of
NULL when fail, fix code in mtk_sysirq_of_init() to correctly handle
this.
Signed-off-by: Yingjoe Chen <[email protected]>
---
drivers/irqchip/irq-mtk-sysirq.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-mtk-sysirq.c b/drivers/irqchip/irq-mtk-sysirq.c
index 7e342df..0b0d2c0 100644
--- a/drivers/irqchip/irq-mtk-sysirq.c
+++ b/drivers/irqchip/irq-mtk-sysirq.c
@@ -137,9 +137,9 @@ static int __init mtk_sysirq_of_init(struct device_node *node,
return -ENOMEM;
chip_data->intpol_base = of_io_request_and_map(node, 0, "intpol");
- if (!chip_data->intpol_base) {
+ if (IS_ERR(chip_data->intpol_base)) {
pr_err("mtk_sysirq: unable to map sysirq register\n");
- ret = -ENOMEM;
+ ret = PTR_ERR(chip_data->intpol_base);
goto out_free;
}
--
1.8.1.1.dirty
On Mon, 8 Dec 2014, Yingjoe Chen wrote:
> This fix an error handling bug reported by Beniamino, this is based on
> mtk intpol patches [1]
>
> Joe.C
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305808.html
>
> ====================
> of_io_request_and_map() return ERR_PTR wrapped error code instead of
> NULL when fail, fix code in mtk_sysirq_of_init() to correctly handle
> this.
>
> Signed-off-by: Yingjoe Chen <[email protected]>
This is not a proper changelog. Let me write up a proper one:
Subject: irqchip: mediatek: Use IS_ERR() instead of NULL pointer check
Beniamino reported a kernel oops caused by an invalid DT file for the
mediatek interrupt polarity extension.
The reason is that the interrupt polarity support for mediatek chips
merily checks for at NULL pointer instead of a casted error return
value in mtk_sysirq_of_init() so any other casted error value passes
the NULL pointer check and causes a kernel panic when dereferenced.
Use IS_ERR() and return the error value via PTR_ERR().
Reported-and-tested-by: Beniamino Galvani <[email protected]>
Signed-off-by: Yingjoe Chen <[email protected]>
Can you see the difference?
Thanks,
tglx
On Mon, 2014-12-08 at 21:30 +0100, Thomas Gleixner wrote:
> On Mon, 8 Dec 2014, Yingjoe Chen wrote:
>
> > This fix an error handling bug reported by Beniamino, this is based on
> > mtk intpol patches [1]
> >
> > Joe.C
> >
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305808.html
> >
> > ====================
> > of_io_request_and_map() return ERR_PTR wrapped error code instead of
> > NULL when fail, fix code in mtk_sysirq_of_init() to correctly handle
> > this.
> >
> > Signed-off-by: Yingjoe Chen <[email protected]>
>
> This is not a proper changelog. Let me write up a proper one:
>
> Subject: irqchip: mediatek: Use IS_ERR() instead of NULL pointer check
>
> Beniamino reported a kernel oops caused by an invalid DT file for the
> mediatek interrupt polarity extension.
>
> The reason is that the interrupt polarity support for mediatek chips
> merily checks for at NULL pointer instead of a casted error return
> value in mtk_sysirq_of_init() so any other casted error value passes
> the NULL pointer check and causes a kernel panic when dereferenced.
>
> Use IS_ERR() and return the error value via PTR_ERR().
>
> Reported-and-tested-by: Beniamino Galvani <[email protected]>
> Signed-off-by: Yingjoe Chen <[email protected]>
>
> Can you see the difference?
>
> Thanks,
>
> tglx
Thanks for the review.
This changelog describe issue and why we need this change more clearly.
I'll prepare a new patch with this changelog, Thanks.
Joe.C
Beniamino reported a kernel oops caused by an invalid DT file for the
mediatek interrupt polarity extension.
The reason is that the interrupt polarity support for mediatek chips
merely checks for NULL pointer instead of a casted error return
value in mtk_sysirq_of_init() so any other casted error value passes
the NULL pointer check and causes a kernel panic when dereferenced.
Use IS_ERR() and return the error value via PTR_ERR().
Reported-and-tested-by: Beniamino Galvani <[email protected]>
Signed-off-by: Yingjoe Chen <[email protected]>
---
drivers/irqchip/irq-mtk-sysirq.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-mtk-sysirq.c b/drivers/irqchip/irq-mtk-sysirq.c
index 7e342df..0b0d2c0 100644
--- a/drivers/irqchip/irq-mtk-sysirq.c
+++ b/drivers/irqchip/irq-mtk-sysirq.c
@@ -137,9 +137,9 @@ static int __init mtk_sysirq_of_init(struct device_node *node,
return -ENOMEM;
chip_data->intpol_base = of_io_request_and_map(node, 0, "intpol");
- if (!chip_data->intpol_base) {
+ if (IS_ERR(chip_data->intpol_base)) {
pr_err("mtk_sysirq: unable to map sysirq register\n");
- ret = -ENOMEM;
+ ret = PTR_ERR(chip_data->intpol_base);
goto out_free;
}
--
1.8.1.1.dirty
On Wed, Dec 10, 2014 at 05:55:02PM +0800, Yingjoe Chen wrote:
> Beniamino reported a kernel oops caused by an invalid DT file for the
> mediatek interrupt polarity extension.
>
> The reason is that the interrupt polarity support for mediatek chips
> merely checks for NULL pointer instead of a casted error return
> value in mtk_sysirq_of_init() so any other casted error value passes
> the NULL pointer check and causes a kernel panic when dereferenced.
>
> Use IS_ERR() and return the error value via PTR_ERR().
>
> Reported-and-tested-by: Beniamino Galvani <[email protected]>
Well, actually I only reported the bug and didn't do any test. Can the
"-and-tested" portion of the tag be dropped while applying the patch?
Beniamino
> Signed-off-by: Yingjoe Chen <[email protected]>
> ---
> drivers/irqchip/irq-mtk-sysirq.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/irqchip/irq-mtk-sysirq.c b/drivers/irqchip/irq-mtk-sysirq.c
> index 7e342df..0b0d2c0 100644
> --- a/drivers/irqchip/irq-mtk-sysirq.c
> +++ b/drivers/irqchip/irq-mtk-sysirq.c
> @@ -137,9 +137,9 @@ static int __init mtk_sysirq_of_init(struct device_node *node,
> return -ENOMEM;
>
> chip_data->intpol_base = of_io_request_and_map(node, 0, "intpol");
> - if (!chip_data->intpol_base) {
> + if (IS_ERR(chip_data->intpol_base)) {
> pr_err("mtk_sysirq: unable to map sysirq register\n");
> - ret = -ENOMEM;
> + ret = PTR_ERR(chip_data->intpol_base);
> goto out_free;
> }
>
> --
> 1.8.1.1.dirty
>
On Wed, Dec 10, 2014 at 01:14:43PM +0100, Beniamino Galvani wrote:
> On Wed, Dec 10, 2014 at 05:55:02PM +0800, Yingjoe Chen wrote:
> > Beniamino reported a kernel oops caused by an invalid DT file for the
> > mediatek interrupt polarity extension.
> >
> > The reason is that the interrupt polarity support for mediatek chips
> > merely checks for NULL pointer instead of a casted error return
> > value in mtk_sysirq_of_init() so any other casted error value passes
> > the NULL pointer check and causes a kernel panic when dereferenced.
> >
> > Use IS_ERR() and return the error value via PTR_ERR().
> >
> > Reported-and-tested-by: Beniamino Galvani <[email protected]>
>
> Well, actually I only reported the bug and didn't do any test. Can the
> "-and-tested" portion of the tag be dropped while applying the patch?
I'd prefer that it be tested before applying. Would you mind confirming
that the oops is gone with this patch applied?
thx,
Jason.
On Wed, 2014-12-10 at 07:53 -0500, Jason Cooper wrote:
> On Wed, Dec 10, 2014 at 01:14:43PM +0100, Beniamino Galvani wrote:
> > On Wed, Dec 10, 2014 at 05:55:02PM +0800, Yingjoe Chen wrote:
> > > Beniamino reported a kernel oops caused by an invalid DT file for the
> > > mediatek interrupt polarity extension.
> > >
> > > The reason is that the interrupt polarity support for mediatek chips
> > > merely checks for NULL pointer instead of a casted error return
> > > value in mtk_sysirq_of_init() so any other casted error value passes
> > > the NULL pointer check and causes a kernel panic when dereferenced.
> > >
> > > Use IS_ERR() and return the error value via PTR_ERR().
> > >
> > > Reported-and-tested-by: Beniamino Galvani <[email protected]>
> >
> > Well, actually I only reported the bug and didn't do any test. Can the
> > "-and-tested" portion of the tag be dropped while applying the patch?
>
> I'd prefer that it be tested before applying. Would you mind confirming
> that the oops is gone with this patch applied?
Hi,
I'm not sure if that count, but I tested with incorrect DTS node
with/without the patch, to make sure the oops is gone with the patch.
Please note this driver is necessary to boot and kernel uart driver,
even without the oops you still can't boot to shell and you'll need
earlyprintk to see the error message.
Joe.C
On Wed, Dec 10, 2014 at 07:53:13AM -0500, Jason Cooper wrote:
> On Wed, Dec 10, 2014 at 01:14:43PM +0100, Beniamino Galvani wrote:
> > On Wed, Dec 10, 2014 at 05:55:02PM +0800, Yingjoe Chen wrote:
> > > Beniamino reported a kernel oops caused by an invalid DT file for the
> > > mediatek interrupt polarity extension.
> > >
> > > The reason is that the interrupt polarity support for mediatek chips
> > > merely checks for NULL pointer instead of a casted error return
> > > value in mtk_sysirq_of_init() so any other casted error value passes
> > > the NULL pointer check and causes a kernel panic when dereferenced.
> > >
> > > Use IS_ERR() and return the error value via PTR_ERR().
> > >
> > > Reported-and-tested-by: Beniamino Galvani <[email protected]>
> >
> > Well, actually I only reported the bug and didn't do any test. Can the
> > "-and-tested" portion of the tag be dropped while applying the patch?
>
> I'd prefer that it be tested before applying. Would you mind confirming
> that the oops is gone with this patch applied?
Probably the commit message is misleading about this, but I don't own
any Mediatek device and never used the driver. I only reported [1] a
possible bug in the driver found through code analysis.
Anyway, I suppose that the tests done by Yingjoe are enough to get the
patch merged.
Beniamino
[1] https://lkml.org/lkml/2014/11/29/105
Hi,
Small fix to the commit message and the reported by tag, the code stay
the same.
Thanks
Joe.C
=============================================
Beniamino noticed a bug that an invalid DT file for the mediatek
interrupt polarity extension will cause kernel oops.
The reason is that the interrupt polarity support for mediatek chips
merely checks for NULL pointer instead of a casted error return
value in mtk_sysirq_of_init() so any other casted error value passes
the NULL pointer check and causes a kernel panic when dereferenced.
Use IS_ERR() and return the error value via PTR_ERR().
Reported-by: Beniamino Galvani <[email protected]>
Signed-off-by: Yingjoe Chen <[email protected]>
---
drivers/irqchip/irq-mtk-sysirq.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-mtk-sysirq.c b/drivers/irqchip/irq-mtk-sysirq.c
index 7e342df..0b0d2c0 100644
--- a/drivers/irqchip/irq-mtk-sysirq.c
+++ b/drivers/irqchip/irq-mtk-sysirq.c
@@ -137,9 +137,9 @@ static int __init mtk_sysirq_of_init(struct device_node *node,
return -ENOMEM;
chip_data->intpol_base = of_io_request_and_map(node, 0, "intpol");
- if (!chip_data->intpol_base) {
+ if (IS_ERR(chip_data->intpol_base)) {
pr_err("mtk_sysirq: unable to map sysirq register\n");
- ret = -ENOMEM;
+ ret = PTR_ERR(chip_data->intpol_base);
goto out_free;
}
--
1.8.1.1.dirty