[PATCH 2/2] iio: inkern: put the IIO device when mem alloc gets failed

[PATCH 2/2] iio: inkern: put the IIO device when mem alloc gets failed

[Kim, Milo]

 The reference count of the IIO device is increased if the IIO map has
 matched consumer name.
 After then, it tries to allocate the iio_channel which is used by the consumer.
 If memory allocation gets failed, the reference count should be decreased.

 This patch enables restoring the reference count of the IIO device.

Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
---
 drivers/iio/inkern.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 13748c0..aff034b 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -132,7 +132,7 @@ struct iio_channel *iio_channel_get(const char *name, const char *channel_name)
 
 	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
 	if (channel == NULL)
-		return ERR_PTR(-ENOMEM);
+		goto error_no_mem;
 
 	channel->indio_dev = c->indio_dev;
 
@@ -151,6 +151,9 @@ error_no_chan:
 	iio_device_put(c->indio_dev);
 	kfree(channel);
 	return ERR_PTR(-EINVAL);
+error_no_mem:
+	iio_device_put(c->indio_dev);
+	return ERR_PTR(-ENOMEM);
 }
 EXPORT_SYMBOL_GPL(iio_channel_get);
 
-- 
1.7.9.5


Best Regards,
Milo

Re: [PATCH 2/2] iio: inkern: put the IIO device when mem alloc gets failed

[Lars-Peter Clausen]

On 09/17/2012 10:44 AM, Kim, Milo wrote:
>  The reference count of the IIO device is increased if the IIO map has
>  matched consumer name.
>  After then, it tries to allocate the iio_channel which is used by the consumer.
>  If memory allocation gets failed, the reference count should be decreased.
> 
>  This patch enables restoring the reference count of the IIO device.
> 
> Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
> ---
>  drivers/iio/inkern.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 13748c0..aff034b 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -132,7 +132,7 @@ struct iio_channel *iio_channel_get(const char *name, const char *channel_name)
>  
>  	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
>  	if (channel == NULL)
> -		return ERR_PTR(-ENOMEM);
> +		goto error_no_mem;
>  
>  	channel->indio_dev = c->indio_dev;
>  
> @@ -151,6 +151,9 @@ error_no_chan:
>  	iio_device_put(c->indio_dev);
>  	kfree(channel);
>  	return ERR_PTR(-EINVAL);
> +error_no_mem:
> +	iio_device_put(c->indio_dev);
> +	return ERR_PTR(-ENOMEM);
>  }
>  EXPORT_SYMBOL_GPL(iio_channel_get);
>  

If you do it that way, you don't really need the goto, something like

error_no_chan:
  	kfree(channel);
error_no_mem:
	iio_device_put(c->indio_dev);
	return ret

would be better in my opinion. With ret being initialized in the if branch
before the goto.

- Lars

RE: [PATCH 2/2] iio: inkern: put the IIO device when mem alloc gets failed

[Kim, Milo]

> >  drivers/iio/inkern.c |    5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > index 13748c0..aff034b 100644
> > --- a/drivers/iio/inkern.c
> > +++ b/drivers/iio/inkern.c
> > @@ -132,7 +132,7 @@ struct iio_channel *iio_channel_get(const char
> *name, const char *channel_name)
> >
> >  	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> >  	if (channel == NULL)
> > -		return ERR_PTR(-ENOMEM);
> > +		goto error_no_mem;
> >
> >  	channel->indio_dev = c->indio_dev;
> >
> > @@ -151,6 +151,9 @@ error_no_chan:
> >  	iio_device_put(c->indio_dev);
> >  	kfree(channel);
> >  	return ERR_PTR(-EINVAL);
> > +error_no_mem:
> > +	iio_device_put(c->indio_dev);
> > +	return ERR_PTR(-ENOMEM);
> >  }
> >  EXPORT_SYMBOL_GPL(iio_channel_get);
> >
> 
> If you do it that way, you don't really need the goto, something like
> 
> error_no_chan:
>   	kfree(channel);
> error_no_mem:
> 	iio_device_put(c->indio_dev);
> 	return ret
> 
> would be better in my opinion. With ret being initialized in the if
> branch
> before the goto.

Thanks for your opinion. I was hesitating before sending ;) 

The reason why this patch need separate goto statement is as below.
 
There are two different types when requesting the channel.
One is a pointer of iio_channel, the other is the error.
So return type will be two - allocated iio_channel and integer.
For simplicity, I would use just one return variable when it runs successfully.
In error cases, do return as explicit name such like ERR_PTR() rather than
saving into local integer.

Just my two cents.
But your code has better readability.

Thank you.

Best Regards,
Milo

Re: [PATCH 2/2] iio: inkern: put the IIO device when mem alloc gets failed

[Jonathan Cameron]

On 09/17/2012 10:57 AM, Kim, Milo wrote:
>>>  drivers/iio/inkern.c |    5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
>>> index 13748c0..aff034b 100644
>>> --- a/drivers/iio/inkern.c
>>> +++ b/drivers/iio/inkern.c
>>> @@ -132,7 +132,7 @@ struct iio_channel *iio_channel_get(const char
>> *name, const char *channel_name)
>>>
>>>  	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
>>>  	if (channel == NULL)
>>> -		return ERR_PTR(-ENOMEM);
>>> +		goto error_no_mem;
>>>
>>>  	channel->indio_dev = c->indio_dev;
>>>
>>> @@ -151,6 +151,9 @@ error_no_chan:
>>>  	iio_device_put(c->indio_dev);
>>>  	kfree(channel);
>>>  	return ERR_PTR(-EINVAL);
>>> +error_no_mem:
>>> +	iio_device_put(c->indio_dev);
>>> +	return ERR_PTR(-ENOMEM);
>>>  }
>>>  EXPORT_SYMBOL_GPL(iio_channel_get);
>>>
>>
>> If you do it that way, you don't really need the goto, something like
>>
>> error_no_chan:
>>   	kfree(channel);
>> error_no_mem:
>> 	iio_device_put(c->indio_dev);
>> 	return ret
>>
>> would be better in my opinion. With ret being initialized in the if
>> branch
>> before the goto.
> 
> Thanks for your opinion. I was hesitating before sending ;) 
> 
> The reason why this patch need separate goto statement is as below.
>  
> There are two different types when requesting the channel.
> One is a pointer of iio_channel, the other is the error.
> So return type will be two - allocated iio_channel and integer.
> For simplicity, I would use just one return variable when it runs successfully.
> In error cases, do return as explicit name such like ERR_PTR() rather than
> saving into local integer.
> 
I can see your point, but Lars-Peter's way is the more commonly used approach
so lets go with the ancient arguement of making it look like what those
reading the code expect to see ;)

> Just my two cents.
> But your code has better readability.
> 
> Thank you.
> 
> Best Regards,
> Milo
> 
> 
> --
> 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 2/2] iio: inkern: put the IIO device when mem alloc gets failed

[Kim, Milo]

> I can see your point, but Lars-Peter's way is the more commonly used
> approach
> so lets go with the ancient arguement of making it look like what those
> reading the code expect to see ;)

Two chained patches were sent.
[PATCH 1/2] iio: inkern: put the IIO device when it fails to allocate memory
[PATCH 2/2] iio: inkern: clean up error return code

The first patch is same as previous one.
The second one is for fixing error return code which Lars had suggested.
I'd like to divide two things for better review.
Many thanks !

Best Regards,
Milo