[patch] bus: mvebu-mbus: potential forever loop in mvebu_mbus_init()

[patch] bus: mvebu-mbus: potential forever loop in mvebu_mbus_init()

[Dan Carpenter]

"of_id->compatible" is an array and not a pointer so it can never be
NULL.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I'm not able to compile this.

diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
index 2394e97..c646a76 100644
--- a/drivers/bus/mvebu-mbus.c
+++ b/drivers/bus/mvebu-mbus.c
@@ -734,11 +734,11 @@ int __init mvebu_mbus_init(const char *soc, phys_addr_t mbuswins_phys_base,
 {
 	const struct of_device_id *of_id;
 
-	for (of_id = of_mvebu_mbus_ids; of_id->compatible; of_id++)
+	for (of_id = of_mvebu_mbus_ids; of_id->compatible[0]; of_id++)
 		if (!strcmp(of_id->compatible, soc))
 			break;
 
-	if (!of_id->compatible) {
+	if (!of_id->compatible[0]) {
 		pr_err("could not find a matching SoC family\n");
 		return -ENODEV;
 	}

Re: [patch] bus: mvebu-mbus: potential forever loop in mvebu_mbus_init()

[Jason Cooper]

Dan,

On Wed, Nov 13, 2013 at 10:50:24AM +0300, Dan Carpenter wrote:
> "of_id->compatible" is an array and not a pointer so it can never be
> NULL.

Thanks for pointing this out.

> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I'm not able to compile this.
> 
> diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
> index 2394e97..c646a76 100644
> --- a/drivers/bus/mvebu-mbus.c
> +++ b/drivers/bus/mvebu-mbus.c
> @@ -734,11 +734,11 @@ int __init mvebu_mbus_init(const char *soc, phys_addr_t mbuswins_phys_base,
>  {
>  	const struct of_device_id *of_id;
>  
> -	for (of_id = of_mvebu_mbus_ids; of_id->compatible; of_id++)
> +	for (of_id = of_mvebu_mbus_ids; of_id->compatible[0]; of_id++)
>  		if (!strcmp(of_id->compatible, soc))
>  			break;

Ezequiel et al,

The comment above the declaration for of_mvebu_mbus_ids says:

/*
 * The driver doesn't yet have a DT binding because the details of
 * this DT binding still need to be sorted out. However, as a
 * preparation, we already use of_device_id to match a SoC description
 * string against the SoC specific details of this driver.
 */

I believe we have a binding now.  ;-)  Care to look this over and
provide a more complete cleanup patch?

thx,

Jason.

Re: [patch] bus: mvebu-mbus: potential forever loop in mvebu_mbus_init()

[Ezequiel Garcia]

On Wed, Nov 13, 2013 at 10:50:24AM +0300, Dan Carpenter wrote:
> "of_id->compatible" is an array and not a pointer so it can never be
> NULL.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I'm not able to compile this.
> 
> diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
> index 2394e97..c646a76 100644
> --- a/drivers/bus/mvebu-mbus.c
> +++ b/drivers/bus/mvebu-mbus.c
> @@ -734,11 +734,11 @@ int __init mvebu_mbus_init(const char *soc, phys_addr_t mbuswins_phys_base,
>  {
>  	const struct of_device_id *of_id;
>  
> -	for (of_id = of_mvebu_mbus_ids; of_id->compatible; of_id++)
> +	for (of_id = of_mvebu_mbus_ids; of_id->compatible[0]; of_id++)
>  		if (!strcmp(of_id->compatible, soc))
>  			break;
>  
> -	if (!of_id->compatible) {
> +	if (!of_id->compatible[0]) {
>  		pr_err("could not find a matching SoC family\n");
>  		return -ENODEV;
>  	}

Nice catch.

Just for the sake of it, I reproduced the bug, and then tested it's
solved by the patch. The issue only hits the old platforms that aren't
converted to DT.

Acked-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

IMHO, this is a "potential" bug, trigerred only if the code itself is
wrong (which is not) and it's not serious enough to hit stable.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] bus: mvebu-mbus: potential forever loop in mvebu_mbus_init()

[Ezequiel Garcia]

On Wed, Nov 13, 2013 at 10:18:30AM -0500, Jason Cooper wrote:
> Dan,
> 
> On Wed, Nov 13, 2013 at 10:50:24AM +0300, Dan Carpenter wrote:
> > "of_id->compatible" is an array and not a pointer so it can never be
> > NULL.
> 
> Thanks for pointing this out.
> 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > I'm not able to compile this.
> > 
> > diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
> > index 2394e97..c646a76 100644
> > --- a/drivers/bus/mvebu-mbus.c
> > +++ b/drivers/bus/mvebu-mbus.c
> > @@ -734,11 +734,11 @@ int __init mvebu_mbus_init(const char *soc, phys_addr_t mbuswins_phys_base,
> >  {
> >  	const struct of_device_id *of_id;
> >  
> > -	for (of_id = of_mvebu_mbus_ids; of_id->compatible; of_id++)
> > +	for (of_id = of_mvebu_mbus_ids; of_id->compatible[0]; of_id++)
> >  		if (!strcmp(of_id->compatible, soc))
> >  			break;
> 
> Ezequiel et al,
> 
> The comment above the declaration for of_mvebu_mbus_ids says:
> 
> /*
>  * The driver doesn't yet have a DT binding because the details of
>  * this DT binding still need to be sorted out. However, as a
>  * preparation, we already use of_device_id to match a SoC description
>  * string against the SoC specific details of this driver.
>  */
> 
> I believe we have a binding now.  ;-)  Care to look this over and
> provide a more complete cleanup patch?
> 

Sure.

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] bus: mvebu-mbus: potential forever loop in mvebu_mbus_init()

[Jason Cooper]

On Wed, Nov 13, 2013 at 10:50:24AM +0300, Dan Carpenter wrote:
> "of_id->compatible" is an array and not a pointer so it can never be
> NULL.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I'm not able to compile this.

Compile tested with mvebu_defconfig for ARCH=arm.  Applied to
mvebu/drivers

thx,

Jason.

Re: [patch] bus: mvebu-mbus: potential forever loop in mvebu_mbus_init()

[Ezequiel Garcia]

On Wed, Nov 13, 2013 at 02:32:32PM -0300, Ezequiel Garcia wrote:
> On Wed, Nov 13, 2013 at 10:50:24AM +0300, Dan Carpenter wrote:
> > "of_id->compatible" is an array and not a pointer so it can never be
> > NULL.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > I'm not able to compile this.
> > 
> > diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
> > index 2394e97..c646a76 100644
> > --- a/drivers/bus/mvebu-mbus.c
> > +++ b/drivers/bus/mvebu-mbus.c
> > @@ -734,11 +734,11 @@ int __init mvebu_mbus_init(const char *soc, phys_addr_t mbuswins_phys_base,
> >  {
> >  	const struct of_device_id *of_id;
> >  
> > -	for (of_id = of_mvebu_mbus_ids; of_id->compatible; of_id++)
> > +	for (of_id = of_mvebu_mbus_ids; of_id->compatible[0]; of_id++)
> >  		if (!strcmp(of_id->compatible, soc))
> >  			break;
> >  
> > -	if (!of_id->compatible) {
> > +	if (!of_id->compatible[0]) {
> >  		pr_err("could not find a matching SoC family\n");
> >  		return -ENODEV;
> >  	}
> 
> Nice catch.
> 
> Just for the sake of it, I reproduced the bug, and then tested it's
> solved by the patch. The issue only hits the old platforms that aren't
> converted to DT.
> 
> Acked-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> 

Jason:

FWIW, in case you've missed this mail, I tested this when Dan reported
it. See above.

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] bus: mvebu-mbus: potential forever loop in mvebu_mbus_init()

[Jason Cooper]

On Sun, Nov 24, 2013 at 08:20:59AM -0300, Ezequiel Garcia wrote:
> On Wed, Nov 13, 2013 at 02:32:32PM -0300, Ezequiel Garcia wrote:
> > On Wed, Nov 13, 2013 at 10:50:24AM +0300, Dan Carpenter wrote:
> > > "of_id->compatible" is an array and not a pointer so it can never be
> > > NULL.
> > > 
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > ---
> > > I'm not able to compile this.
> > > 
> > > diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
> > > index 2394e97..c646a76 100644
> > > --- a/drivers/bus/mvebu-mbus.c
> > > +++ b/drivers/bus/mvebu-mbus.c
> > > @@ -734,11 +734,11 @@ int __init mvebu_mbus_init(const char *soc, phys_addr_t mbuswins_phys_base,
> > >  {
> > >  	const struct of_device_id *of_id;
> > >  
> > > -	for (of_id = of_mvebu_mbus_ids; of_id->compatible; of_id++)
> > > +	for (of_id = of_mvebu_mbus_ids; of_id->compatible[0]; of_id++)
> > >  		if (!strcmp(of_id->compatible, soc))
> > >  			break;
> > >  
> > > -	if (!of_id->compatible) {
> > > +	if (!of_id->compatible[0]) {
> > >  		pr_err("could not find a matching SoC family\n");
> > >  		return -ENODEV;
> > >  	}
> > 
> > Nice catch.
> > 
> > Just for the sake of it, I reproduced the bug, and then tested it's
> > solved by the patch. The issue only hits the old platforms that aren't
> > converted to DT.
> > 
> > Acked-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > 
> 
> Jason:
> 
> FWIW, in case you've missed this mail, I tested this when Dan reported
> it. See above.

Fixed.  Sorry I missed that.

thx,

Jason.