Re: [PATCH 1/2] iio: adc: ti_am335x_adc: do not free the kfifo twice

[Lars-Peter Clausen <lars@metafoo.de>]

On 10/24/2013 11:25 AM, Sebastian Andrzej Siewior wrote:
> On 10/24/2013 12:13 PM, Jonathan Cameron wrote:
>> On 10/23/13 18:44, Lars-Peter Clausen wrote:
>>> The code is actually fine as it is. The iio_kfifo_free() function drops a reference, but does not free the buffer's memory if somebody else is still holding a reference.
>>>
>>> - Lars
>> I'm guessing this came from a crash though.... Sebastian, what motivated the patch?
> 
> tiadc_iio_buffered_hardware_remove() in staging-next looks like this:
> 
>  static void tiadc_iio_buffered_hardware_remove(struct iio_dev *indio_dev)
>  {
>          struct tiadc_device *adc_dev = iio_priv(indio_dev);
> 
>          free_irq(adc_dev->mfd_tscadc->irq, indio_dev);
>          iio_kfifo_free(indio_dev->buffer);
>          iio_buffer_unregister(indio_dev);
>  }
> 
> and is called rmmod time. iio_kfifo_free() cleans up the buffer (there
> is not a second reference so the memory is gone.
> iio_buffer_unregister() crashes later because
> 
> void iio_buffer_unregister(struct iio_dev *indio_dev)
> {
>         kfree(indio_dev->buffer->scan_mask);
>         kfree(indio_dev->buffer->scan_el_group.attrs);
> 
> iio_free_chan_devattr_list(&indio_dev->buffer->scan_el_dev_attr_list);
>  }
> 
> the first kree() is looking at ->buffer which is gone by now. So at
> first I changed switched the order of the two (iio_kfifi_free() and
> iio_buffer_unregister()) and I had a crash later because iio_core
> removes the buffer as well.
> So I dropped the iio_kfifo_free() call since it seems to be already
> done.

The driver seems to be missing the iio_buffer_attach() call. Something like
this should fix the problem:

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index ef54d8a..bf9c89c 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -229,12 +229,15 @@ static int tiadc_iio_buffered_hardware_setup(struct
iio_dev *indio_dev,
 	unsigned long flags,
 	const struct iio_buffer_setup_ops *setup_ops)
 {
+	struct iio_buffer *buffer;
 	int ret;

-	indio_dev->buffer = iio_kfifo_allocate(indio_dev);
-	if (!indio_dev->buffer)
+	buffer = iio_kfifo_allocate(indio_dev);
+	if (!buffer)
 		return -ENOMEM;

+	iio_device_attach_buffer(indio_dev, buffer);
+
 	ret = request_threaded_irq(irq,	pollfunc_th, pollfunc_bh,
 				flags, indio_dev->name, indio_dev);
 	if (ret)