[PATCH] mfd: check for NULL platform data

[PATCH] mfd: check for NULL platform data

[Fabio Estevam]

Avoid kernel crash if platform data is NULL.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/mfd/mc13xxx-core.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
index a2ac2ed..b4d6bb1 100644
--- a/drivers/mfd/mc13xxx-core.c
+++ b/drivers/mfd/mc13xxx-core.c
@@ -757,6 +757,9 @@ err_revision:
 
 	mc13xxx_unlock(mc13xxx);
 
+	if (pdata == NULL)
+		return 0;
+
 	if (pdata->flags & MC13XXX_USE_ADC)
 		mc13xxx_add_subdevice(mc13xxx, "%s-adc");
 
-- 
1.6.0.4



      

Re: [PATCH] mfd: check for NULL platform data

[Uwe Kleine-König]

Hi Fabio,

On Thu, Nov 11, 2010 at 06:58:14AM -0800, Fabio Estevam wrote:
> Avoid kernel crash if platform data is NULL.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  drivers/mfd/mc13xxx-core.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
> index a2ac2ed..b4d6bb1 100644
> --- a/drivers/mfd/mc13xxx-core.c
> +++ b/drivers/mfd/mc13xxx-core.c
> @@ -757,6 +757,9 @@ err_revision:
>  
>  	mc13xxx_unlock(mc13xxx);
>  
> +	if (pdata == NULL)
> +		return 0;
> +
I'm not sure it's really needed to catch this error.  Not passing pdata
isn't sensible and then maybe failing with a bang is better than
handling silently.

And if you want to break probing, do you really want to return 0, i.e.
let the binding succeed?  IMHO (if you really want to handle pdata ==
NULL) you should fail before allocating the private data with
-ESOMETHINGSENSIBLE.

Thanks,
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] mfd: check for NULL platform data

[Samuel Ortiz]

Hi Uwe,

On Thu, Nov 11, 2010 at 04:44:07PM +0100, Uwe Kleine-König wrote:
> Hi Fabio,
> 
> On Thu, Nov 11, 2010 at 06:58:14AM -0800, Fabio Estevam wrote:
> > Avoid kernel crash if platform data is NULL.
> > 
> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> > ---
> >  drivers/mfd/mc13xxx-core.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
> > index a2ac2ed..b4d6bb1 100644
> > --- a/drivers/mfd/mc13xxx-core.c
> > +++ b/drivers/mfd/mc13xxx-core.c
> > @@ -757,6 +757,9 @@ err_revision:
> >  
> >  	mc13xxx_unlock(mc13xxx);
> >  
> > +	if (pdata == NULL)
> > +		return 0;
> > +
> I'm not sure it's really needed to catch this error.  Not passing pdata
> isn't sensible and then maybe failing with a bang is better than
> handling silently.
> 
> And if you want to break probing, do you really want to return 0, i.e.
> let the binding succeed?  IMHO (if you really want to handle pdata ==
> NULL) you should fail before allocating the private data with
> -ESOMETHINGSENSIBLE.
Agreed. Besides failing silently, Fabio's patch leaks the mc13xxx pointer and
doesn't free the requested IRQ line.
So NAK for now, although I wouldn't be against checking for a NULL pdata at
the very begining of the probe routine.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/