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

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

[Lars-Peter Clausen]

VGhlIGNvZGUgaXMgYWN0dWFsbHkgZmluZSBhcyBpdCBpcy4gVGhlIGlpb19rZmlmb19mcmVlKCkg
ZnVuY3Rpb24gZHJvcHMgYSByZWZlcmVuY2UsIGJ1dCBkb2VzIG5vdCBmcmVlIHRoZSBidWZmZXIn
cyBtZW1vcnkgaWYgc29tZWJvZHkgZWxzZSBpcyBzdGlsbCBob2xkaW5nIGEgcmVmZXJlbmNlLgoK
LSBMYXJzCgpTZWJhc3RpYW4gQW5kcnplaiBTaWV3aW9yIDxiaWdlYXN5QGxpbnV0cm9uaXguZGU+
IHdyb3RlOgoKPlNpbmNlIGNvbW1pdCA5ZTY5YzkgKCJpaW86IEFkZCByZWZlcmVuY2UgY291bnRp
bmcgZm9yIGJ1ZmZlcnMiKSB0aGUgaWlvCj5jb3JlIGZyZWVzIHRoZSBidWZmZXIuIFNvIGlmIHRo
ZSBkcml2ZXIgZG9lcyBpdCBhcyB3ZWxsIHRoZW4gYmFkIHRoaW5ncwo+d2lsbCBoYXBwZW4uCj4K
PkNjOiBMYXJzLVBldGVyIENsYXVzZW4gPGxhcnNAbWV0YWZvby5kZT4KPlNpZ25lZC1vZmYtYnk6
IFNlYmFzdGlhbiBBbmRyemVqIFNpZXdpb3IgPGJpZ2Vhc3lAbGludXRyb25peC5kZT4KPi0tLQo+
Cj5CeSBubyBtZWFucyBJIGFtIHRoZSBleHBlcnQgaGVyZSBidXQgdGhlIG90aGVyIHVzZXJzIG9m
IGlpb19rZmlmb19mcmVlKCkgbGlrZQo+Cj5kcml2ZXJzL2lpby9pbmR1c3RyaWFsaW8tdHJpZ2dl
cmVkLWJ1ZmZlci5jOiAgICBpaW9fa2ZpZm9fZnJlZShpbmRpb19kZXYtPmJ1ZmZlcik7Cj5kcml2
ZXJzL2lpby9pbmR1c3RyaWFsaW8tdHJpZ2dlcmVkLWJ1ZmZlci5jOiAgICBpaW9fa2ZpZm9fZnJl
ZShpbmRpb19kZXYtPmJ1ZmZlcik7Cj4KPmFyZSBwcm9iYWJseSB3cm9uZywgdG9vLiBOb3QgdG8g
bWVudGlvbiBzdGFnaW5nLiBTbyAqSSogdGhpbmsgaWlvX2tmaWZvX2ZyZWUoKQo+c2hvdWxkIHZh
bmlzaCBmcm9tIHRoZSB0cmVlLgo+Cj4gZHJpdmVycy9paW8vYWRjL3RpX2FtMzM1eF9hZGMuYyB8
IDMgLS0tCj4gMSBmaWxlIGNoYW5nZWQsIDMgZGVsZXRpb25zKC0pCj4KPmRpZmYgLS1naXQgYS9k
cml2ZXJzL2lpby9hZGMvdGlfYW0zMzV4X2FkYy5jIGIvZHJpdmVycy9paW8vYWRjL3RpX2FtMzM1
eF9hZGMuYwo+aW5kZXggZjk3NGRlYS4uM2Q3MGYwOSAxMDA2NDQKPi0tLSBhL2RyaXZlcnMvaWlv
L2FkYy90aV9hbTMzNXhfYWRjLmMKPisrKyBiL2RyaXZlcnMvaWlvL2FkYy90aV9hbTMzNXhfYWRj
LmMKPkBAIC0yNTQsOCArMjU0LDYgQEAgc3RhdGljIGludCB0aWFkY19paW9fYnVmZmVyZWRfaGFy
ZHdhcmVfc2V0dXAoc3RydWN0IGlpb19kZXYgKmluZGlvX2RldiwKPiAKPiBlcnJvcl9mcmVlX2ly
cToKPiAJZnJlZV9pcnEoaXJxLCBpbmRpb19kZXYpOwo+LWVycm9yX2tmaWZvX2ZyZWU6Cj4tCWlp
b19rZmlmb19mcmVlKGluZGlvX2Rldi0+YnVmZmVyKTsKPiAJcmV0dXJuIHJldDsKPiB9Cj4gCj5A
QCAtMjY0LDcgKzI2Miw2IEBAIHN0YXRpYyB2b2lkIHRpYWRjX2lpb19idWZmZXJlZF9oYXJkd2Fy
ZV9yZW1vdmUoc3RydWN0IGlpb19kZXYgKmluZGlvX2RldikKPiAJc3RydWN0IHRpYWRjX2Rldmlj
ZSAqYWRjX2RldiA9IGlpb19wcml2KGluZGlvX2Rldik7Cj4gCj4gCWZyZWVfaXJxKGFkY19kZXYt
Pm1mZF90c2NhZGMtPmlycSwgaW5kaW9fZGV2KTsKPi0JaWlvX2tmaWZvX2ZyZWUoaW5kaW9fZGV2
LT5idWZmZXIpOwo+IAlpaW9fYnVmZmVyX3VucmVnaXN0ZXIoaW5kaW9fZGV2KTsKPiB9Cj4gCj4t
LSAKPjEuOC40LnJjMwo+Cj4tLQo+VG8gdW5zdWJzY3JpYmUgZnJvbSB0aGlzIGxpc3Q6IHNlbmQg
dGhlIGxpbmUgInVuc3Vic2NyaWJlIGxpbnV4LWlpbyIgaW4KPnRoZSBib2R5IG9mIGEgbWVzc2Fn
ZSB0byBtYWpvcmRvbW9Admdlci5rZXJuZWwub3JnCj5Nb3JlIG1ham9yZG9tbyBpbmZvIGF0ICBo
dHRwOi8vdmdlci5rZXJuZWwub3JnL21ham9yZG9tby1pbmZvLmh0bWwK

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

[Jonathan Cameron]

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?
> 
> Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
>> Since commit 9e69c9 ("iio: Add reference counting for buffers") the iio
>> core frees the buffer. So if the driver does it as well then bad things
>> will happen.
>>
>> Cc: Lars-Peter Clausen <lars@metafoo.de>
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> ---
>>
>> By no means I am the expert here but the other users of iio_kfifo_free() like
>>
>> drivers/iio/industrialio-triggered-buffer.c:    iio_kfifo_free(indio_dev->buffer);
>> drivers/iio/industrialio-triggered-buffer.c:    iio_kfifo_free(indio_dev->buffer);
>>
>> are probably wrong, too. Not to mention staging. So *I* think iio_kfifo_free()
>> should vanish from the tree.
>>
>> drivers/iio/adc/ti_am335x_adc.c | 3 ---
>> 1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
>> index f974dea..3d70f09 100644
>> --- a/drivers/iio/adc/ti_am335x_adc.c
>> +++ b/drivers/iio/adc/ti_am335x_adc.c
>> @@ -254,8 +254,6 @@ static int tiadc_iio_buffered_hardware_setup(struct iio_dev *indio_dev,
>>
>> error_free_irq:
>> 	free_irq(irq, indio_dev);
>> -error_kfifo_free:
>> -	iio_kfifo_free(indio_dev->buffer);
>> 	return ret;
>> }
>>
>> @@ -264,7 +262,6 @@ 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);
>> }
>>
>> -- 
>> 1.8.4.rc3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> N�����r��y���b�X��ǧv�^�)޺{.n�+����{��*"��^n�r���z�\x1a��h����&��\x1e�G���h�\x03(�階�ݢj"��\x1a�^[m�����z�ޖ���f���h���~�mml==
> 

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

[Sebastian Andrzej Siewior]

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.

Sebastian

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

[Lars-Peter Clausen]

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)

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

[Sebastian Andrzej Siewior]

On 10/24/2013 11:41 AM, Lars-Peter Clausen wrote:
> 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)

Yep, that works, thanks.

Shouldn't the two

         tiadc_iio_buffered_hardware_remove(indio_dev);
         tiadc_channels_remove(indio_dev);

in tiadc_remove() be reversed in their call order? The second alter is
accessing the buffer which is released by the former one.

btw: is all this ref counting really required? I mean I would assume
allocate buffer in one place (at probe time) release it remove time
should be enough.

Sebastian

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

[Lars-Peter Clausen]

On 10/24/2013 12:34 PM, Sebastian Andrzej Siewior wrote:
> On 10/24/2013 11:41 AM, Lars-Peter Clausen wrote:
>> 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)
> 
> Yep, that works, thanks.
> 
> Shouldn't the two
> 
>          tiadc_iio_buffered_hardware_remove(indio_dev);
>          tiadc_channels_remove(indio_dev);
> 
> in tiadc_remove() be reversed in their call order? The second alter is
> accessing the buffer which is released by the former one.
> 

As far as I can see tiadc_channels_remove() only does a
kfree(indio_dev->channels), so it does not access the buffer at all.

> btw: is all this ref counting really required? I mean I would assume
> allocate buffer in one place (at probe time) release it remove time
> should be enough.

It is required. Userspace may still be reading from the buffer when the
driver frees it. So we need proper refcounting here.

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

[Sebastian Andrzej Siewior]

On 10/24/2013 12:44 PM, Lars-Peter Clausen wrote:
>> Shouldn't the two
>>
>>          tiadc_iio_buffered_hardware_remove(indio_dev);
>>          tiadc_channels_remove(indio_dev);
>>
>> in tiadc_remove() be reversed in their call order? The second alter is
>> accessing the buffer which is released by the former one.
>>
> 
> As far as I can see tiadc_channels_remove() only does a
> kfree(indio_dev->channels), so it does not access the buffer at all.

I'm sorry I meant
         iio_kfifo_free(indio_dev->buffer);
         iio_buffer_unregister(indio_dev);

in tiadc_iio_buffered_hardware_remove()

>> btw: is all this ref counting really required? I mean I would assume
>> allocate buffer in one place (at probe time) release it remove time
>> should be enough.
> 
> It is required. Userspace may still be reading from the buffer when the
> driver frees it. So we need proper refcounting here.

Ach okay then.

Sebastian

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

[Jonathan Cameron]

On 10/24/13 11:44, Lars-Peter Clausen wrote:
> On 10/24/2013 12:34 PM, Sebastian Andrzej Siewior wrote:
>> On 10/24/2013 11:41 AM, Lars-Peter Clausen wrote:
>>> 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)
>>
>> Yep, that works, thanks.
>>
>> Shouldn't the two
>>
>>          tiadc_iio_buffered_hardware_remove(indio_dev);
>>          tiadc_channels_remove(indio_dev);
>>
>> in tiadc_remove() be reversed in their call order? The second alter is
>> accessing the buffer which is released by the former one.
>>
> 
> As far as I can see tiadc_channels_remove() only does a
> kfree(indio_dev->channels), so it does not access the buffer at all.
Certainly seems to be true...
> 
>> btw: is all this ref counting really required? I mean I would assume
>> allocate buffer in one place (at probe time) release it remove time
>> should be enough.
> 
> It is required. Userspace may still be reading from the buffer when the
> driver frees it. So we need proper refcounting here.

Lars, can you do a clean version of the above with a reported-by from Sebastian
then Sebastian can you ack (if you are happy with it of course!)

Thanks,

Jonathan

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

[Jonathan Cameron]

On 10/24/13 12:50, Jonathan Cameron wrote:
> On 10/24/13 11:44, Lars-Peter Clausen wrote:
>> On 10/24/2013 12:34 PM, Sebastian Andrzej Siewior wrote:
>>> On 10/24/2013 11:41 AM, Lars-Peter Clausen wrote:
>>>> 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)
>>>
>>> Yep, that works, thanks.
>>>
>>> Shouldn't the two
>>>
>>>          tiadc_iio_buffered_hardware_remove(indio_dev);
>>>          tiadc_channels_remove(indio_dev);
>>>
>>> in tiadc_remove() be reversed in their call order? The second alter is
>>> accessing the buffer which is released by the former one.
>>>
>>
>> As far as I can see tiadc_channels_remove() only does a
>> kfree(indio_dev->channels), so it does not access the buffer at all.
> Certainly seems to be true...
>>
>>> btw: is all this ref counting really required? I mean I would assume
>>> allocate buffer in one place (at probe time) release it remove time
>>> should be enough.
>>
>> It is required. Userspace may still be reading from the buffer when the
>> driver frees it. So we need proper refcounting here.
> 
> Lars, can you do a clean version of the above with a reported-by from Sebastian
> then Sebastian can you ack (if you are happy with it of course!)
> 
Lars, I've turned the above into a coherent patch and applied it to the fixes-togreg
branch.  I've added your Signed-off-by: Shout if you would prefer not.

Also a reported by for Sebastian.

I didn't want this patch to fall through the cracks given it's been around a while now.

Jonathan

> Thanks,
> 
> Jonathan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Lars-Peter Clausen]

On 11/09/2013 01:34 PM, Jonathan Cameron wrote:
> On 10/24/13 12:50, Jonathan Cameron wrote:
>> On 10/24/13 11:44, Lars-Peter Clausen wrote:
>>> On 10/24/2013 12:34 PM, Sebastian Andrzej Siewior wrote:
>>>> On 10/24/2013 11:41 AM, Lars-Peter Clausen wrote:
>>>>> 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)
>>>>
>>>> Yep, that works, thanks.
>>>>
>>>> Shouldn't the two
>>>>
>>>>          tiadc_iio_buffered_hardware_remove(indio_dev);
>>>>          tiadc_channels_remove(indio_dev);
>>>>
>>>> in tiadc_remove() be reversed in their call order? The second alter is
>>>> accessing the buffer which is released by the former one.
>>>>
>>>
>>> As far as I can see tiadc_channels_remove() only does a
>>> kfree(indio_dev->channels), so it does not access the buffer at all.
>> Certainly seems to be true...
>>>
>>>> btw: is all this ref counting really required? I mean I would assume
>>>> allocate buffer in one place (at probe time) release it remove time
>>>> should be enough.
>>>
>>> It is required. Userspace may still be reading from the buffer when the
>>> driver frees it. So we need proper refcounting here.
>>
>> Lars, can you do a clean version of the above with a reported-by from Sebastian
>> then Sebastian can you ack (if you are happy with it of course!)
>>
> Lars, I've turned the above into a coherent patch and applied it to the fixes-togreg
> branch.  I've added your Signed-off-by: Shout if you would prefer not.
> 
> Also a reported by for Sebastian.
> 
> I didn't want this patch to fall through the cracks given it's been around a while now.

It was still on my radar. The plan was to send it once the merge window has
closed.

- Lars

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

[Jonathan Cameron]

Lars-Peter Clausen <lars@metafoo.de> wrote:
>On 11/09/2013 01:34 PM, Jonathan Cameron wrote:
>> On 10/24/13 12:50, Jonathan Cameron wrote:
>>> On 10/24/13 11:44, Lars-Peter Clausen wrote:
>>>> On 10/24/2013 12:34 PM, Sebastian Andrzej Siewior wrote:
>>>>> On 10/24/2013 11:41 AM, Lars-Peter Clausen wrote:
>>>>>> 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)
>>>>>
>>>>> Yep, that works, thanks.
>>>>>
>>>>> Shouldn't the two
>>>>>
>>>>>          tiadc_iio_buffered_hardware_remove(indio_dev);
>>>>>          tiadc_channels_remove(indio_dev);
>>>>>
>>>>> in tiadc_remove() be reversed in their call order? The second
>alter is
>>>>> accessing the buffer which is released by the former one.
>>>>>
>>>>
>>>> As far as I can see tiadc_channels_remove() only does a
>>>> kfree(indio_dev->channels), so it does not access the buffer at
>all.
>>> Certainly seems to be true...
>>>>
>>>>> btw: is all this ref counting really required? I mean I would
>assume
>>>>> allocate buffer in one place (at probe time) release it remove
>time
>>>>> should be enough.
>>>>
>>>> It is required. Userspace may still be reading from the buffer when
>the
>>>> driver frees it. So we need proper refcounting here.
>>>
>>> Lars, can you do a clean version of the above with a reported-by
>from Sebastian
>>> then Sebastian can you ack (if you are happy with it of course!)
>>>
>> Lars, I've turned the above into a coherent patch and applied it to
>the fixes-togreg
>> branch.  I've added your Signed-off-by: Shout if you would prefer
>not.
>> 
>> Also a reported by for Sebastian.
>> 
>> I didn't want this patch to fall through the cracks given it's been
>around a while now.
>
>It was still on my radar. The plan was to send it once the merge window
>has
>closed.
Fair enough. I just got impatient :)
>
>- Lars

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

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

[Sebastian Andrzej Siewior]

Since commit 9e69c9 ("iio: Add reference counting for buffers") the iio
core frees the buffer. So if the driver does it as well then bad things
will happen.

Cc: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---

By no means I am the expert here but the other users of iio_kfifo_free() like

drivers/iio/industrialio-triggered-buffer.c:    iio_kfifo_free(indio_dev->buffer);
drivers/iio/industrialio-triggered-buffer.c:    iio_kfifo_free(indio_dev->buffer);

are probably wrong, too. Not to mention staging. So *I* think iio_kfifo_free()
should vanish from the tree.

 drivers/iio/adc/ti_am335x_adc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index f974dea..3d70f09 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -254,8 +254,6 @@ static int tiadc_iio_buffered_hardware_setup(struct iio_dev *indio_dev,
 
 error_free_irq:
 	free_irq(irq, indio_dev);
-error_kfifo_free:
-	iio_kfifo_free(indio_dev->buffer);
 	return ret;
 }
 
@@ -264,7 +262,6 @@ 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);
 }
 
-- 
1.8.4.rc3