[PATCH 1/4] pinctrl: dove: Prevent NULL dereference if of_match_device returns NULL

of_match_device() may return NULL.

Signed-off-by: Axel Lin <redacted>
---
 drivers/pinctrl/mvebu/pinctrl-dove.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/mvebu/pinctrl-dove.c b/drivers/pinctrl/mvebu/pinctrl-dove.c
index ffe74b2..a8b9b42 100644
--- a/drivers/pinctrl/mvebu/pinctrl-dove.c
+++ b/drivers/pinctrl/mvebu/pinctrl-dove.c

@@ -578,8 +578,12 @@ static struct of_device_id dove_pinctrl_of_match[] __devinitdata = {
 
 static int __devinit dove_pinctrl_probe(struct platform_device *pdev)
 {
-	const struct of_device_id *match =
-		of_match_device(dove_pinctrl_of_match, &pdev->dev);
+	const struct of_device_id *match;
+
+	match = of_match_device(dove_pinctrl_of_match, &pdev->dev);
+	if (!match)
+		return -ENODEV;
+
 	pdev->dev.platform_data = match->data;
 
 	/*

-- 
1.7.9.5

of_match_device() may return NULL.

Signed-off-by: Axel Lin <redacted>
---
 drivers/pinctrl/mvebu/pinctrl-kirkwood.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/mvebu/pinctrl-kirkwood.c b/drivers/pinctrl/mvebu/pinctrl-kirkwood.c
index 9a74ef6..fe885ca 100644
--- a/drivers/pinctrl/mvebu/pinctrl-kirkwood.c
+++ b/drivers/pinctrl/mvebu/pinctrl-kirkwood.c

@@ -444,8 +444,11 @@ static struct of_device_id kirkwood_pinctrl_of_match[] __devinitdata = {
 
 static int __devinit kirkwood_pinctrl_probe(struct platform_device *pdev)
 {
-	const struct of_device_id *match =
-		of_match_device(kirkwood_pinctrl_of_match, &pdev->dev);
+	const struct of_device_id *match;
+
+	match = of_match_device(kirkwood_pinctrl_of_match, &pdev->dev);
+	if (!match)
+		return -ENODEV;
 	pdev->dev.platform_data = match->data;
 	return mvebu_pinctrl_probe(pdev);
 }

-- 
1.7.9.5

Dear Axel Lin,

On Thu, 15 Nov 2012 12:52:39 +0800, Axel Lin wrote:

of_match_device() may return NULL.

Signed-off-by: Axel Lin <redacted>

Why? We already discussed together that this situation could not
happen: if you are in the probe() function, then there *must* be a
match, because the probe() function precisely gets called when there is
a match.

So unless you give more details about what the problem is, I don't
think this patch and the similar one you sent for Dove are useful.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

of_match_device() may return NULL.

Signed-off-by: Axel Lin <redacted>
---
 drivers/pinctrl/pinctrl-at91.c |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index 8490a55..32006c8 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c

@@ -829,13 +829,18 @@ static int __devinit at91_pinctrl_probe_dt(struct platform_device *pdev,
 	uint32_t *tmp;
 	struct device_node *np = pdev->dev.of_node;
 	struct device_node *child;
+	const struct of_device_id *match;
 
 	if (!np)
 		return -ENODEV;
 
+	match = of_match_device(at91_pinctrl_of_match, &pdev->dev)->data;
+	if (!match)
+		return -ENODEV;
+
 	info->dev = &pdev->dev;
-	info->ops = (struct at91_pinctrl_mux_ops*)
-		of_match_device(at91_pinctrl_of_match, &pdev->dev)->data;
+	info->ops = (struct at91_pinctrl_mux_ops *) match->data;
+
 	at91_pinctrl_child_count(info, np);
 
 	if (info->nbanks < 1) {

@@ -1359,6 +1364,7 @@ static struct of_device_id at91_gpio_of_match[] __devinitdata = {
 static int __devinit at91_gpio_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
+	const struct of_device_id *match;
 	struct resource *res;
 	struct at91_gpio_chip *at91_chip = NULL;
 	struct gpio_chip *chip;

@@ -1399,8 +1405,11 @@ static int __devinit at91_gpio_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	at91_chip->ops = (struct at91_pinctrl_mux_ops*)
-		of_match_device(at91_gpio_of_match, &pdev->dev)->data;
+	match = of_match_device(at91_gpio_of_match, &pdev->dev);
+	if (!match)
+		return -ENODEV;
+
+	at91_chip->ops = (struct at91_pinctrl_mux_ops *) match->data;
 	at91_chip->pioc_virq = irq;
 	at91_chip->pioc_idx = alias_idx;
 

-- 
1.7.9.5

of_match_device() may return NULL.

Signed-off-by: Axel Lin <redacted>
---
This resend CC Jean-Christophe.

 drivers/pinctrl/pinctrl-at91.c |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index 8490a55..32006c8 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c

@@ -829,13 +829,18 @@ static int __devinit at91_pinctrl_probe_dt(struct platform_device *pdev,
 	uint32_t *tmp;
 	struct device_node *np = pdev->dev.of_node;
 	struct device_node *child;
+	const struct of_device_id *match;
 
 	if (!np)
 		return -ENODEV;
 
+	match = of_match_device(at91_pinctrl_of_match, &pdev->dev)->data;
+	if (!match)
+		return -ENODEV;
+
 	info->dev = &pdev->dev;
-	info->ops = (struct at91_pinctrl_mux_ops*)
-		of_match_device(at91_pinctrl_of_match, &pdev->dev)->data;
+	info->ops = (struct at91_pinctrl_mux_ops *) match->data;
+
 	at91_pinctrl_child_count(info, np);
 
 	if (info->nbanks < 1) {

@@ -1359,6 +1364,7 @@ static struct of_device_id at91_gpio_of_match[] __devinitdata = {
 static int __devinit at91_gpio_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
+	const struct of_device_id *match;
 	struct resource *res;
 	struct at91_gpio_chip *at91_chip = NULL;
 	struct gpio_chip *chip;

@@ -1399,8 +1405,11 @@ static int __devinit at91_gpio_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	at91_chip->ops = (struct at91_pinctrl_mux_ops*)
-		of_match_device(at91_gpio_of_match, &pdev->dev)->data;
+	match = of_match_device(at91_gpio_of_match, &pdev->dev);
+	if (!match)
+		return -ENODEV;
+
+	at91_chip->ops = (struct at91_pinctrl_mux_ops *) match->data;
 	at91_chip->pioc_virq = irq;
 	at91_chip->pioc_idx = alias_idx;
 

-- 
1.7.9.5

On 11/15/2012 05:58 AM, Axel Lin :

of_match_device() may return NULL.

Signed-off-by: Axel Lin <redacted>

Seems sensible,

Acked-by: Nicolas Ferre <redacted>

-- 
Nicolas Ferre

On 12:58 Thu 15 Nov     , Axel Lin wrote:

of_match_device() may return NULL.

this is not possible on at91

and I do a oups here as if we have a NULL pointer which means the driver is
wrong

Best Regards,
J.

On 11/15/2012 11:00 AM, Jean-Christophe PLAGNIOL-VILLARD :

On 12:58 Thu 15 Nov     , Axel Lin wrote:

quoted

of_match_device() may return NULL.

this is not possible on at91

and I do a oups here as if we have a NULL pointer which means the driver is
wrong

Well, okay, but it does not prevent from adding a supplementary check to
mimic every other pinctrl driver and use a common return path to the
of_match_device() function.

So I am not completely against this patch (that I why I added my Acked-by).

quoted

Signed-off-by: Axel Lin <redacted>
---
This resend CC Jean-Christophe.

 drivers/pinctrl/pinctrl-at91.c |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index 8490a55..32006c8 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c

@@ -829,13 +829,18 @@ static int __devinit at91_pinctrl_probe_dt(struct platform_device *pdev,
 	uint32_t *tmp;
 	struct device_node *np = pdev->dev.of_node;
 	struct device_node *child;
+	const struct of_device_id *match;
 
 	if (!np)
 		return -ENODEV;
 
+	match = of_match_device(at91_pinctrl_of_match, &pdev->dev)->data;
+	if (!match)
+		return -ENODEV;
+
 	info->dev = &pdev->dev;
-	info->ops = (struct at91_pinctrl_mux_ops*)
-		of_match_device(at91_pinctrl_of_match, &pdev->dev)->data;
+	info->ops = (struct at91_pinctrl_mux_ops *) match->data;
+
 	at91_pinctrl_child_count(info, np);
 
 	if (info->nbanks < 1) {

@@ -1359,6 +1364,7 @@ static struct of_device_id at91_gpio_of_match[] __devinitdata = {
 static int __devinit at91_gpio_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
+	const struct of_device_id *match;
 	struct resource *res;
 	struct at91_gpio_chip *at91_chip = NULL;
 	struct gpio_chip *chip;

@@ -1399,8 +1405,11 @@ static int __devinit at91_gpio_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	at91_chip->ops = (struct at91_pinctrl_mux_ops*)
-		of_match_device(at91_gpio_of_match, &pdev->dev)->data;
+	match = of_match_device(at91_gpio_of_match, &pdev->dev);
+	if (!match)
+		return -ENODEV;
+
+	at91_chip->ops = (struct at91_pinctrl_mux_ops *) match->data;
 	at91_chip->pioc_virq = irq;
 	at91_chip->pioc_idx = alias_idx;
 

-- 
1.7.9.5

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Nicolas Ferre

On 09:36 Fri 16 Nov     , Nicolas Ferre wrote:

On 11/15/2012 11:00 AM, Jean-Christophe PLAGNIOL-VILLARD :

quoted

On 12:58 Thu 15 Nov     , Axel Lin wrote:

quoted

of_match_device() may return NULL.

this is not possible on at91

and I do a oups here as if we have a NULL pointer which means the driver is
wrong

Well, okay, but it does not prevent from adding a supplementary check to
mimic every other pinctrl driver and use a common return path to the
of_match_device() function.

So I am not completely against this patch (that I why I added my Acked-by).

here it's a silent warning a oops it clear we have a bug

Best Regards,
J.

quoted

quoted

Signed-off-by: Axel Lin <redacted>
---
This resend CC Jean-Christophe.

 drivers/pinctrl/pinctrl-at91.c |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index 8490a55..32006c8 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c

@@ -829,13 +829,18 @@ static int __devinit at91_pinctrl_probe_dt(struct platform_device *pdev,
 	uint32_t *tmp;
 	struct device_node *np = pdev->dev.of_node;
 	struct device_node *child;
+	const struct of_device_id *match;
 
 	if (!np)
 		return -ENODEV;
 
+	match = of_match_device(at91_pinctrl_of_match, &pdev->dev)->data;
+	if (!match)
+		return -ENODEV;
+
 	info->dev = &pdev->dev;
-	info->ops = (struct at91_pinctrl_mux_ops*)
-		of_match_device(at91_pinctrl_of_match, &pdev->dev)->data;
+	info->ops = (struct at91_pinctrl_mux_ops *) match->data;
+
 	at91_pinctrl_child_count(info, np);
 
 	if (info->nbanks < 1) {

@@ -1359,6 +1364,7 @@ static struct of_device_id at91_gpio_of_match[] __devinitdata = {
 static int __devinit at91_gpio_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
+	const struct of_device_id *match;
 	struct resource *res;
 	struct at91_gpio_chip *at91_chip = NULL;
 	struct gpio_chip *chip;

@@ -1399,8 +1405,11 @@ static int __devinit at91_gpio_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	at91_chip->ops = (struct at91_pinctrl_mux_ops*)
-		of_match_device(at91_gpio_of_match, &pdev->dev)->data;
+	match = of_match_device(at91_gpio_of_match, &pdev->dev);
+	if (!match)
+		return -ENODEV;
+
+	at91_chip->ops = (struct at91_pinctrl_mux_ops *) match->data;
 	at91_chip->pioc_virq = irq;
 	at91_chip->pioc_idx = alias_idx;
 

-- 
1.7.9.5

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Nicolas Ferre

of_match_device() may return NULL.

Signed-off-by: Axel Lin <redacted>
---
 drivers/pinctrl/pinctrl-nomadik.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-nomadik.c b/drivers/pinctrl/pinctrl-nomadik.c
index 238060e..40bd1b3 100644
--- a/drivers/pinctrl/pinctrl-nomadik.c
+++ b/drivers/pinctrl/pinctrl-nomadik.c

@@ -1863,9 +1863,14 @@ static int __devinit nmk_pinctrl_probe(struct platform_device *pdev)
 
 	if (platid)
 		version = platid->driver_data;
-	else if (np)
-		version = (unsigned int)
-			of_match_device(nmk_pinctrl_match, &pdev->dev)->data;
+	else if (np) {
+		const struct of_device_id *match;
+
+		match = of_match_device(nmk_pinctrl_match, &pdev->dev);
+		if (!match)
+			return -ENODEV;
+		version = (unsigned int) match->data;
+	}
 
 	/* Poke in other ASIC variants here */
 	if (version == PINCTRL_NMK_STN8815)

-- 
1.7.9.5

On Thu, Nov 15, 2012 at 5:56 AM, Axel Lin [off-list ref] wrote:

AFAICT this can actually happen so patch applied, unless Lee
speaks against it.

Patches 1-3/4 are dropped though, as the maintainers didn't seem
to like them.

Yours,
Linus Walleij

On Thu, 15 Nov 2012, Linus Walleij wrote:

On Thu, Nov 15, 2012 at 5:56 AM, Axel Lin [off-list ref] wrote:

quoted

of_match_device() may return NULL.

Signed-off-by: Axel Lin <redacted>
---
 drivers/pinctrl/pinctrl-nomadik.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-nomadik.c b/drivers/pinctrl/pinctrl-nomadik.c
index 238060e..40bd1b3 100644
--- a/drivers/pinctrl/pinctrl-nomadik.c
+++ b/drivers/pinctrl/pinctrl-nomadik.c

@@ -1863,9 +1863,14 @@ static int __devinit nmk_pinctrl_probe(struct platform_device *pdev)

        if (platid)
                version = platid->driver_data;
-       else if (np)
-               version = (unsigned int)
-                       of_match_device(nmk_pinctrl_match, &pdev->dev)->data;
+       else if (np) {
+               const struct of_device_id *match;
+
+               match = of_match_device(nmk_pinctrl_match, &pdev->dev);
+               if (!match)
+                       return -ENODEV;
+               version = (unsigned int) match->data;
+       }

AFAICT this can actually happen so patch applied, unless Lee
speaks against it.

At the very least it make the code easy to read.

Acked-by: Lee Jones <redacted>

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Dear Axel Lin,

On Thu, 15 Nov 2012 12:51:36 +0800, Axel Lin wrote:

of_match_device() may return NULL.

Signed-off-by: Axel Lin <redacted>

Could you detail under what conditions of_match_device() may return
NULL in the specific case of this driver? This of_match_device() call
is using the same dove_pinctrl_of_match array that is used to ->probe()
this driver. So I don't see how you can get into ->probe() without
having a matching entry.

Am I missing something?

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

2012/11/15 Thomas Petazzoni [off-list ref]

Dear Axel Lin,

On Thu, 15 Nov 2012 12:51:36 +0800, Axel Lin wrote:

quoted

of_match_device() may return NULL.

Signed-off-by: Axel Lin <redacted>

Could you detail under what conditions of_match_device() may return
NULL in the specific case of this driver? This of_match_device() call
is using the same dove_pinctrl_of_match array that is used to ->probe()
this driver. So I don't see how you can get into ->probe() without
having a matching entry.

Am I missing something?

 hi Thomas,
You are right.

I was thinking when the case CONFIG_OF is not selected,
of_match_ptr(dove_pinctrl_of_match) is NULL and of_match_device returns
NULL.
But this driver only works when CONFIG_OF is selected.

BTW, I found there is no way to compile the dove and kirkwood pinctrl
drivers.
I need add below patch to compile these two drivers, how do you think about
below patch?
( In dove, the USE_OF is optional, it is selected only when MACH_DOVE_DT is
enabled )

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 14f8160..dda6785 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig

@@ -531,6 +531,8 @@ config ARCH_DOVE
        select CPU_V7
        select GENERIC_CLOCKEVENTS
        select MIGHT_HAVE_PCI
+       select PINCTRL
+       select PINCTRL_DOVE if USE_OF
        select PLAT_ORION_LEGACY
        select USB_ARCH_HAS_EHCI
        help

@@ -542,6 +544,8 @@ config ARCH_KIRKWOOD
        select CPU_FEROCEON
        select GENERIC_CLOCKEVENTS
        select PCI
+       select PINCTRL
+       select PINCTRL_KIRKWOOD
        select PLAT_ORION_LEGACY
        help
          Support for the following Marvell Kirkwood series SoCs:

Regards,
Axel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121115/be2e1627/attachment-0001.html>

Dear Axel Lin,

On Thu, 15 Nov 2012 21:44:07 +0800, Axel Lin wrote:

See "[PATCH 1/5] ARM: Kirkwood: Allow use of pinctrl" and "[PATCH 4/5]
ARM: Dove: Make use of pinctrl driver" which have been posted by Andrew
Lunn on October, 24th.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com