* Viktor Rosendahl <Viktor.Rosendahl@xxxxxxxxx> [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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
