* [PATCH 2/2] iio: inkern: put the IIO device when mem alloc gets failed
@ 2012-09-17 8:44 Kim, Milo
2012-09-17 9:34 ` Lars-Peter Clausen
0 siblings, 1 reply; 5+ messages in thread
From: Kim, Milo @ 2012-09-17 8:44 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] iio: inkern: put the IIO device when mem alloc gets failed
2012-09-17 8:44 [PATCH 2/2] iio: inkern: put the IIO device when mem alloc gets failed Kim, Milo
@ 2012-09-17 9:34 ` Lars-Peter Clausen
2012-09-17 9:57 ` Kim, Milo
0 siblings, 1 reply; 5+ messages in thread
From: Lars-Peter Clausen @ 2012-09-17 9:34 UTC (permalink / raw)
To: Kim, Milo; +Cc: Jonathan Cameron, Jonathan Cameron, linux-iio, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH 2/2] iio: inkern: put the IIO device when mem alloc gets failed
2012-09-17 9:34 ` Lars-Peter Clausen
@ 2012-09-17 9:57 ` Kim, Milo
2012-09-17 19:58 ` Jonathan Cameron
0 siblings, 1 reply; 5+ messages in thread
From: Kim, Milo @ 2012-09-17 9:57 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Jonathan Cameron, Jonathan Cameron, linux-iio, linux-kernel
> > 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] iio: inkern: put the IIO device when mem alloc gets failed
2012-09-17 9:57 ` Kim, Milo
@ 2012-09-17 19:58 ` Jonathan Cameron
2012-09-18 4:56 ` Kim, Milo
0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Cameron @ 2012-09-17 19:58 UTC (permalink / raw)
To: Kim, Milo; +Cc: Lars-Peter Clausen, Jonathan Cameron, linux-iio, linux-kernel
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH 2/2] iio: inkern: put the IIO device when mem alloc gets failed
2012-09-17 19:58 ` Jonathan Cameron
@ 2012-09-18 4:56 ` Kim, Milo
0 siblings, 0 replies; 5+ messages in thread
From: Kim, Milo @ 2012-09-18 4:56 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Jonathan Cameron, linux-iio, linux-kernel
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-09-18 4:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-17 8:44 [PATCH 2/2] iio: inkern: put the IIO device when mem alloc gets failed Kim, Milo
2012-09-17 9:34 ` Lars-Peter Clausen
2012-09-17 9:57 ` Kim, Milo
2012-09-17 19:58 ` Jonathan Cameron
2012-09-18 4:56 ` Kim, Milo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).