LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] irqchip: mediatek: Fix error return code detection
@ 2014-12-08 15:03 Yingjoe Chen
  2014-12-08 20:30 ` Thomas Gleixner
  2014-12-10  9:55 ` [PATCH v2] irqchip: mediatek: Use IS_ERR() instead of NULL pointer check Yingjoe Chen
  0 siblings, 2 replies; 10+ messages in thread
From: Yingjoe Chen @ 2014-12-08 15:03 UTC (permalink / raw)
  To: Beniamino Galvani, Jason Cooper
  Cc: Thomas Gleixner, Marc Zyngier, srv_heupstream, hc.yen, yh.chen,
	linux-kernel, Matthias Brugger, nathan.chung, Sascha Hauer,
	eddie.huang, yingjoe.chen, linux-arm-kernel, Yingjoe Chen

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 <yingjoe.chen@mediatek.com>
---
 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] irqchip: mediatek: Fix error return code detection
  2014-12-08 15:03 [PATCH] irqchip: mediatek: Fix error return code detection Yingjoe Chen
@ 2014-12-08 20:30 ` Thomas Gleixner
  2014-12-09  6:11   ` Yingjoe Chen
  2014-12-10  9:55 ` [PATCH v2] irqchip: mediatek: Use IS_ERR() instead of NULL pointer check Yingjoe Chen
  1 sibling, 1 reply; 10+ messages in thread
From: Thomas Gleixner @ 2014-12-08 20:30 UTC (permalink / raw)
  To: Yingjoe Chen
  Cc: Beniamino Galvani, Jason Cooper, Marc Zyngier, srv_heupstream,
	hc.yen, yh.chen, linux-kernel, Matthias Brugger, nathan.chung,
	Sascha Hauer, eddie.huang, yingjoe.chen, linux-arm-kernel

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 <yingjoe.chen@mediatek.com>

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 <b.galvani@gmail.com>
Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>

Can you see the difference?

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] irqchip: mediatek: Fix error return code detection
  2014-12-08 20:30 ` Thomas Gleixner
@ 2014-12-09  6:11   ` Yingjoe Chen
  0 siblings, 0 replies; 10+ messages in thread
From: Yingjoe Chen @ 2014-12-09  6:11 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Beniamino Galvani, Jason Cooper, Marc Zyngier, srv_heupstream,
	hc.yen, yh.chen, linux-kernel, Matthias Brugger, nathan.chung,
	Sascha Hauer, eddie.huang, yingjoe.chen, linux-arm-kernel

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 <yingjoe.chen@mediatek.com>
> 
> 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 <b.galvani@gmail.com>
> Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> 
> 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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2] irqchip: mediatek: Use IS_ERR() instead of NULL pointer check
  2014-12-08 15:03 [PATCH] irqchip: mediatek: Fix error return code detection Yingjoe Chen
  2014-12-08 20:30 ` Thomas Gleixner
@ 2014-12-10  9:55 ` Yingjoe Chen
  2014-12-10 12:14   ` Beniamino Galvani
                     ` (2 more replies)
  1 sibling, 3 replies; 10+ messages in thread
From: Yingjoe Chen @ 2014-12-10  9:55 UTC (permalink / raw)
  To: Thomas Gleixner, Beniamino Galvani, Jason Cooper
  Cc: Marc Zyngier, srv_heupstream, hc.yen, yh.chen, linux-kernel,
	Matthias Brugger, nathan.chung, Sascha Hauer, eddie.huang,
	yingjoe.chen, linux-arm-kernel, Yingjoe Chen

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 <b.galvani@gmail.com>
Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
---
 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] irqchip: mediatek: Use IS_ERR() instead of NULL pointer check
  2014-12-10  9:55 ` [PATCH v2] irqchip: mediatek: Use IS_ERR() instead of NULL pointer check Yingjoe Chen
@ 2014-12-10 12:14   ` Beniamino Galvani
  2014-12-10 12:53     ` Jason Cooper
  2014-12-12 15:15   ` [PATCH v3] irqchip: mtk-sysirq: " Yingjoe Chen
  2015-01-07  2:08   ` [PATCH v2] irqchip: mediatek: " Jason Cooper
  2 siblings, 1 reply; 10+ messages in thread
From: Beniamino Galvani @ 2014-12-10 12:14 UTC (permalink / raw)
  To: Yingjoe Chen
  Cc: Thomas Gleixner, Jason Cooper, Marc Zyngier, srv_heupstream,
	hc.yen, yh.chen, linux-kernel, Matthias Brugger, nathan.chung,
	Sascha Hauer, eddie.huang, yingjoe.chen, linux-arm-kernel

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 <b.galvani@gmail.com>

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 <yingjoe.chen@mediatek.com>
> ---
>  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
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] irqchip: mediatek: Use IS_ERR() instead of NULL pointer check
  2014-12-10 12:14   ` Beniamino Galvani
@ 2014-12-10 12:53     ` Jason Cooper
  2014-12-10 13:55       ` Yingjoe Chen
  2014-12-10 20:08       ` Beniamino Galvani
  0 siblings, 2 replies; 10+ messages in thread
From: Jason Cooper @ 2014-12-10 12:53 UTC (permalink / raw)
  To: Beniamino Galvani
  Cc: Yingjoe Chen, Thomas Gleixner, Marc Zyngier, srv_heupstream,
	hc.yen, yh.chen, linux-kernel, Matthias Brugger, nathan.chung,
	Sascha Hauer, eddie.huang, yingjoe.chen, linux-arm-kernel

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 <b.galvani@gmail.com>
> 
> 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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] irqchip: mediatek: Use IS_ERR() instead of NULL pointer check
  2014-12-10 12:53     ` Jason Cooper
@ 2014-12-10 13:55       ` Yingjoe Chen
  2014-12-10 20:08       ` Beniamino Galvani
  1 sibling, 0 replies; 10+ messages in thread
From: Yingjoe Chen @ 2014-12-10 13:55 UTC (permalink / raw)
  To: Jason Cooper
  Cc: Beniamino Galvani, Thomas Gleixner, Marc Zyngier, srv_heupstream,
	hc.yen, yh.chen, linux-kernel, Matthias Brugger, nathan.chung,
	Sascha Hauer, eddie.huang, yingjoe.chen, linux-arm-kernel

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 <b.galvani@gmail.com>
> > 
> > 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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] irqchip: mediatek: Use IS_ERR() instead of NULL pointer check
  2014-12-10 12:53     ` Jason Cooper
  2014-12-10 13:55       ` Yingjoe Chen
@ 2014-12-10 20:08       ` Beniamino Galvani
  1 sibling, 0 replies; 10+ messages in thread
From: Beniamino Galvani @ 2014-12-10 20:08 UTC (permalink / raw)
  To: Jason Cooper
  Cc: Yingjoe Chen, Thomas Gleixner, Marc Zyngier, srv_heupstream,
	hc.yen, yh.chen, linux-kernel, Matthias Brugger, nathan.chung,
	Sascha Hauer, eddie.huang, yingjoe.chen, linux-arm-kernel

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 <b.galvani@gmail.com>
> > 
> > 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3] irqchip: mtk-sysirq: Use IS_ERR() instead of NULL pointer check
  2014-12-10  9:55 ` [PATCH v2] irqchip: mediatek: Use IS_ERR() instead of NULL pointer check Yingjoe Chen
  2014-12-10 12:14   ` Beniamino Galvani
@ 2014-12-12 15:15   ` Yingjoe Chen
  2015-01-07  2:08   ` [PATCH v2] irqchip: mediatek: " Jason Cooper
  2 siblings, 0 replies; 10+ messages in thread
From: Yingjoe Chen @ 2014-12-12 15:15 UTC (permalink / raw)
  To: Beniamino Galvani, Jason Cooper
  Cc: yingjoe.chen, srv_heupstream, Marc Zyngier, hc.yen, yh.chen,
	linux-kernel, nathan.chung, Sascha Hauer, Matthias Brugger,
	Yingjoe Chen, Thomas Gleixner, eddie.huang, linux-arm-kernel

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 <b.galvani@gmail.com>
Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
---
 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] irqchip: mediatek: Use IS_ERR() instead of NULL pointer check
  2014-12-10  9:55 ` [PATCH v2] irqchip: mediatek: Use IS_ERR() instead of NULL pointer check Yingjoe Chen
  2014-12-10 12:14   ` Beniamino Galvani
  2014-12-12 15:15   ` [PATCH v3] irqchip: mtk-sysirq: " Yingjoe Chen
@ 2015-01-07  2:08   ` Jason Cooper
  2 siblings, 0 replies; 10+ messages in thread
From: Jason Cooper @ 2015-01-07  2:08 UTC (permalink / raw)
  To: Yingjoe Chen
  Cc: Thomas Gleixner, Beniamino Galvani, Marc Zyngier, srv_heupstream,
	hc.yen, yh.chen, linux-kernel, Matthias Brugger, nathan.chung,
	Sascha Hauer, eddie.huang, yingjoe.chen, linux-arm-kernel

Yingjoe,

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 <b.galvani@gmail.com>
> Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> ---
>  drivers/irqchip/irq-mtk-sysirq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

I applied V2 to irqchip/urgent, and hand-added the changes from V3
because the email formatting was wrong.  I also tweaked the subject
line.

thx,

Jason.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2015-01-07  2:08 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-08 15:03 [PATCH] irqchip: mediatek: Fix error return code detection Yingjoe Chen
2014-12-08 20:30 ` Thomas Gleixner
2014-12-09  6:11   ` Yingjoe Chen
2014-12-10  9:55 ` [PATCH v2] irqchip: mediatek: Use IS_ERR() instead of NULL pointer check Yingjoe Chen
2014-12-10 12:14   ` Beniamino Galvani
2014-12-10 12:53     ` Jason Cooper
2014-12-10 13:55       ` Yingjoe Chen
2014-12-10 20:08       ` Beniamino Galvani
2014-12-12 15:15   ` [PATCH v3] irqchip: mtk-sysirq: " Yingjoe Chen
2015-01-07  2:08   ` [PATCH v2] irqchip: mediatek: " Jason Cooper

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).