Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Wed, Sep 18, 2013 at 05:12:02PM +0100, Jonathan Cameron wrote:
> 
> 
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >On Wed, Sep 18, 2013 at 10:39:42AM +0100, Jonathan Cameron wrote:
> >> 
> >> 
> >> "Zubair Lutfullah :" <zubair.lutfullah@gmail.com> wrote:
> >> >On Tue, Sep 17, 2013 at 09:27:27PM -0700, Dmitry Torokhov wrote:
> >> >> Hi Zubair,
> >> >> 
> >> >> On Tue, Sep 17, 2013 at 09:44:07AM +0500, Zubair Lutfullah wrote:
> >> >> > +
> >> >> > +	ret = devm_request_threaded_irq(indio_dev->dev.parent,
> >> >> > +				irq,
> >> >> > +				pollfunc_th, pollfunc_bh,
> >> >> > +				flags, indio_dev->name,
> >> >> > +				indio_dev);
> >> >> > +	if (ret)
> >> >> > +		goto error_kfifo_free;
> >> >> > +
> >> >> > +	indio_dev->setup_ops = setup_ops;
> >> >> > +	indio_dev->modes |= INDIO_BUFFER_HARDWARE;
> >> >> > +
> >> >> > +	ret = iio_buffer_register(indio_dev,
> >> >> > +				  indio_dev->channels,
> >> >> > +				  indio_dev->num_channels);
> >> >> > +	if (ret)
> >> >> > +		goto error_free_irq;
> >> >> > +
> >> >> > +	return 0;
> >> >> > +
> >> >> > +error_free_irq:
> >> >> > +	devm_free_irq(indio_dev->dev.parent, irq, indio_dev);
> >> >> 
> >> >> What is the point of using devm_* here if you are doing explicit
> >> >> management of the resource anyway (you explicitly release it in
> >all
> >> >code
> >> >> paths)?
> >> >> 
> >> >> Thanks.
> >> >> 
> >> >> -- 
> >> >> Dmitry
> >> >
> >> >I admit I am unaware at the moment about how it works.
> >> >
> >> >I use devm and simply ignore the error path?
> >> >
> >> >The devm function header description said something about using
> >> >devm_free when freeing. And this is the way I am used to seeing 
> >> >error handling.
> >> 
> >
> >> The devm interfaces ensure this is all cleaned when the device is
> >> removed thus avoiding the need to free the stuff explicitly.  Device
> >> will get freed on deliberate remove and on an error from probe. Hence
> >> you can drop all calls to devm free. The devm free functions are only
> >> needed if you wish to free in order to reallocate. This might happen
> >> if you want to change a buffer size for instance.
> >
> >However in this case such conversion us dangerous. With all but IRQ
> >resource managed by the traditional methods they will be released first
> >with IRQ handler deregistered very last. Therefore if device is not
> >properly quiesced IRQ raised during driver unbinding is likely to
> >result
> >in kernel oops.
> >
> >IOW devm_request_irq() is very often evil (it is still useful if _all_
> >your resources are managed by devm_*).
> >
> >In case of your driver I'd recommend switching to
> >request_irq()/free_irq() instead.
> >
> >Thanks.
> 
> Pretty much all resources are devm managed in here
> 
> https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/adc/ti_am335x_adc.c?h=togreg&id=7a1aeba7ed0d5a1e83fd5a8ee2a2869430d40347


So we are guaranteed that that new kfifo that is being allocated just
before we requesting IRQ and will be freed way before we free the IRQ
will not be used by the IOTQ handler?

Thanks.

-- 
Dmitry