Re: [PATCH 1/1] twl4030-madc: Fix arbitrarily initialized function pointer

[Tony Lindgren <tony@atomide.com>]

* Viktor Rosendahl <Viktor.Rosendahl@nokia.com> [080703 18:37]:
> 
> > 
> > > +		req.func_cb	= NULL;
> > 
> > maybe below is a better patch:
> > 
> > diff --git a/drivers/i2c/chips/twl4030-madc.c
> > b/drivers/i2c/chips/twl4030-madc.c
> > index 72b126b..6d8915e 100644
> > --- a/drivers/i2c/chips/twl4030-madc.c
> > +++ b/drivers/i2c/chips/twl4030-madc.c
> > @@ -360,7 +360,7 @@ static int twl4030_madc_ioctl(struct inode *inode,
> > struct file *filp,
> >  
> >         switch (cmd) {
> >         case TWL4030_MADC_IOCX_ADC_RAW_READ: {
> > -               struct twl4030_madc_request req;
> > +               static struct twl4030_madc_request req;
> >                 if (par.channel >= TWL4030_MADC_MAX_CHANNELS)
> >                         return -EINVAL;
> 
> I don't like this idea because:
> 
> - It's fragile. This struct, which is not a const, gets initialized only
> once but we are still passing a pointer to it, expecting that a fairly
> complex function will not modify it. This assertion is probably true
> today but makes it easier for somebody to create a bug in the future.
> 
> - You introduce another shared datum and it is only protected by the BKL
> in fs/ioctl.c:vfs_ioctl().
> 
> - I didn't see any argument why this variable should be static. Making
> local variables static just to get cheap zero initialization is a weird
> thing to do IMO.

Pushing the original patch.

Tony