Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1372399 > unrolled thread

[PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release}

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release} Laxman Dewangan <ldewangan@nvidia.com> - 2016-04-06 12:50 +0200
    Re: [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release} Daniel Baluta <daniel.baluta@gmail.com> - 2016-04-06 15:50 +0200
      Re: [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release} Laxman Dewangan <ldewangan@nvidia.com> - 2016-04-06 17:20 +0200
        Re: [PATCH 1/3] iio: core: Add devm_ APIs for  iio_channel_{get,release} Jonathan Cameron <jic23@kernel.org> - 2016-04-10 16:10 +0200
          Re: [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release} Laxman Dewangan <ldewangan@nvidia.com> - 2016-04-10 19:50 +0200
    Re: [PATCH 1/3] iio: core: Add devm_ APIs for  iio_channel_{get,release} Jonathan Cameron <jic23@kernel.org> - 2016-04-10 16:10 +0200

#1372399 — [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release}

Some of kernel driver uses the IIO framework to get the sensor
value via ADC or IIO HW driver. The client driver get iio channel
by iio_channel_get() and release it by calling iio_channel_release().

Add resource managed version (devm_*) of these APIs so that if client
calls the devm_iio_channel_get() then it need not to release it explicitly,
it can be done by managed device framework when driver get un-binded.

This reduces the code in error path and also need of .remove callback in
some cases.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/iio/inkern.c         | 48 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/iio/consumer.h | 27 +++++++++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 734a004..18e623f 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -356,6 +356,54 @@ void iio_channel_release(struct iio_channel *channel)
 }
 EXPORT_SYMBOL_GPL(iio_channel_release);
 
+static void devm_iio_channel_free(struct device *dev, void *res)
+{
+	struct iio_channel *channel = *(struct iio_channel **)res;
+
+	iio_channel_release(channel);
+}
+
+static int devm_iio_channel_match(struct device *dev, void *res, void *data)
+{
+	struct iio_channel **r = res;
+
+	if (!r || !*r) {
+		WARN_ON(!r || !*r);
+		return 0;
+	}
+
+	return *r == data;
+}
+
+struct iio_channel *devm_iio_channel_get(struct device *dev,
+					 const char *channel_name)
+{
+	struct iio_channel **ptr, *channel;
+
+	ptr = devres_alloc(devm_iio_channel_free, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	channel = iio_channel_get(dev, channel_name);
+	if (IS_ERR(channel)) {
+		devres_free(ptr);
+		return channel;
+	}
+
+	*ptr = channel;
+	devres_add(dev, ptr);
+
+	return channel;
+}
+EXPORT_SYMBOL_GPL(devm_iio_channel_get);
+
+void devm_iio_channel_release(struct device *dev, struct iio_channel *channel)
+{
+	WARN_ON(devres_release(dev, devm_iio_channel_free,
+			       devm_iio_channel_match, channel));
+}
+EXPORT_SYMBOL_GPL(devm_iio_channel_release);
+
 struct iio_channel *iio_channel_get_all(struct device *dev)
 {
 	const char *name;
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index fad5867..e1e033d 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -49,6 +49,33 @@ struct iio_channel *iio_channel_get(struct device *dev,
 void iio_channel_release(struct iio_channel *chan);
 
 /**
+ * devm_iio_channel_get() - Resource managed version of iio_channel_get().
+ * @dev:		Pointer to consumer device. Device name must match
+ *			the name of the device as provided in the iio_map
+ *			with which the desired provider to consumer mapping
+ *			was registered.
+ * @consumer_channel:	Unique name to identify the channel on the consumer
+ *			side. This typically describes the channels use within
+ *			the consumer. E.g. 'battery_voltage'
+ *
+ * Returns a pointer to negative errno if it is not able to get the iio channel
+ * otherwise returns valid pointer for iio channel.
+ *
+ * The allocated iio channel is automatically released when the device is
+ * unbound.
+ */
+struct iio_channel *devm_iio_channel_get(struct device *dev,
+					 const char *consumer_channel);
+/**
+ * devm_iio_channel_release() - Resource managed version of
+ *				iio_channel_release().
+ * @dev:		Pointer to consumer device for which resource
+ *			is allocared.
+ * @chan:		The channel to be released.
+ */
+void devm_iio_channel_release(struct device *dev, struct iio_channel *chan);
+
+/**
  * iio_channel_get_all() - get all channels associated with a client
  * @dev:		Pointer to consumer device.
  *
-- 
2.1.4

[toc] | [next] | [standalone]

#1372530

On Wed, Apr 6, 2016 at 1:31 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
> Some of kernel driver uses the IIO framework to get the sensor
> value via ADC or IIO HW driver. The client driver get iio channel
> by iio_channel_get() and release it by calling iio_channel_release().
>
> Add resource managed version (devm_*) of these APIs so that if client
> calls the devm_iio_channel_get() then it need not to release it explicitly,
> it can be done by managed device framework when driver get un-binded.
>
> This reduces the code in error path and also need of .remove callback in
> some cases.
>

Please provide at least one example of code that uses this API.

[toc] | [prev] | [next] | [standalone]

#1372593

Hi Daniel,


On Wednesday 06 April 2016 07:19 PM, Daniel Baluta wrote:
> On Wed, Apr 6, 2016 at 1:31 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>> Some of kernel driver uses the IIO framework to get the sensor
>> value via ADC or IIO HW driver. The client driver get iio channel
>> by iio_channel_get() and release it by calling iio_channel_release().
>>
>> Add resource managed version (devm_*) of these APIs so that if client
>> calls the devm_iio_channel_get() then it need not to release it explicitly,
>> it can be done by managed device framework when driver get un-binded.
>>
>> This reduces the code in error path and also need of .remove callback in
>> some cases.
>>
> Please provide at least one example of code that uses this API.

Most of client for this APIs are in other subsystem.
When I was working on the patch
[PATCH 2/2] thermal: generic-adc: Add ADC based thermal sensor driver

if I have devm_iio_channel_get() then I can get .remove callback at all.

I did not use this new APIs in my patch because they are in different 
subsystem.

[toc] | [prev] | [next] | [standalone]

#1374784 — Re: [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release}

On 06/04/16 15:58, Laxman Dewangan wrote:
> Hi Daniel,
> 
> 
> On Wednesday 06 April 2016 07:19 PM, Daniel Baluta wrote:
>> On Wed, Apr 6, 2016 at 1:31 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>>> Some of kernel driver uses the IIO framework to get the sensor
>>> value via ADC or IIO HW driver. The client driver get iio channel
>>> by iio_channel_get() and release it by calling iio_channel_release().
>>>
>>> Add resource managed version (devm_*) of these APIs so that if client
>>> calls the devm_iio_channel_get() then it need not to release it explicitly,
>>> it can be done by managed device framework when driver get un-binded.
>>>
>>> This reduces the code in error path and also need of .remove callback in
>>> some cases.
>>>
>> Please provide at least one example of code that uses this API.
> 
> Most of client for this APIs are in other subsystem.
> When I was working on the patch
> [PATCH 2/2] thermal: generic-adc: Add ADC based thermal sensor driver
> 
> if I have devm_iio_channel_get() then I can get .remove callback at all.
> 
> I did not use this new APIs in my patch because they are in different subsystem.
It's actually worse than that having taken a quick look at the generic-adc thermal patch
you reference above.
(perhaps worth cc'ing linux-iio for next version of that).

Without this devm function set you have a race in remove in which I think you can
get attempts to access the channels after they have been released...

> +
> +	gti->tz_dev = devm_thermal_zone_of_sensor_register(&pdev->dev, 0,
> +				gti, &gadc_thermal_ops);
> +	if (IS_ERR(gti->tz_dev)) {
> +		ret = PTR_ERR(gti->tz_dev);
> +		dev_err(&pdev->dev, "Thermal zone sensor register failed: %d\n",
> +			ret);
> +		goto sensor_fail;
> +	}
This will get cleaned up in remove 'after' the iio_channels are released.
Hence you have a race in which they can probably be accessed after release
(unless some magic is going on that I've missed).
> +
> +	return 0;
> +
> +sensor_fail:
> +	iio_channel_release(gti->channel);
> +	return ret;
> +}
> +
> +static int gadc_thermal_remove(struct platform_device *pdev)
> +{
> +	struct gadc_thermal_info *gti = platform_get_drvdata(pdev);
> +
> +	iio_channel_release(gti->channel);
> +
> +	return 0;
> +}
> +


> 
> 
> 

[toc] | [prev] | [next] | [standalone]

#1374822

On Sunday 10 April 2016 07:35 PM, Jonathan Cameron wrote:
> On 06/04/16 15:58, Laxman Dewangan wrote:
>> Hi Daniel,
>>
>>
>> On Wednesday 06 April 2016 07:19 PM, Daniel Baluta wrote:
>>> On Wed, Apr 6, 2016 at 1:31 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>>>> Some of kernel driver uses the IIO framework to get the sensor
>>>> value via ADC or IIO HW driver. The client driver get iio channel
>>>> by iio_channel_get() and release it by calling iio_channel_release().
>>>>
>>>> Add resource managed version (devm_*) of these APIs so that if client
>>>> calls the devm_iio_channel_get() then it need not to release it explicitly,
>>>> it can be done by managed device framework when driver get un-binded.
>>>>
>>>> This reduces the code in error path and also need of .remove callback in
>>>> some cases.
>>>>
>>> Please provide at least one example of code that uses this API.
>> Most of client for this APIs are in other subsystem.
>> When I was working on the patch
>> [PATCH 2/2] thermal: generic-adc: Add ADC based thermal sensor driver
>>
>> if I have devm_iio_channel_get() then I can get .remove callback at all.
>>
>> I did not use this new APIs in my patch because they are in different subsystem.
> It's actually worse than that having taken a quick look at the generic-adc thermal patch
> you reference above.
> (perhaps worth cc'ing linux-iio for next version of that).
Sure. I will CC.

>
> Without this devm function set you have a race in remove in which I think you can
> get attempts to access the channels after they have been released...
Yaah, possibly race for very small time possible.

The limitation of devm_ api usage is that, we can keep using this till 
we have devm_ api continuous and if some resource are not there for 
devm_ then we can not use further.
Possibly, I need to wait for the devm_iio_channel_get() to merge and 
available for all subsystem to use (next release) and then only I can 
use devm_thermal_zone_of_sensor_register().

[toc] | [prev] | [next] | [standalone]

#1374787 — Re: [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release}

On 06/04/16 11:31, Laxman Dewangan wrote:
> Some of kernel driver uses the IIO framework to get the sensor
> value via ADC or IIO HW driver. The client driver get iio channel
> by iio_channel_get() and release it by calling iio_channel_release().
> 
> Add resource managed version (devm_*) of these APIs so that if client
> calls the devm_iio_channel_get() then it need not to release it explicitly,
> it can be done by managed device framework when driver get un-binded.
> 
> This reduces the code in error path and also need of .remove callback in
> some cases.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

I'm fine with this - but would like it to sit for a few more days to see
if it gets any other feedback.
> ---
>  drivers/iio/inkern.c         | 48 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/iio/consumer.h | 27 +++++++++++++++++++++++++
>  2 files changed, 75 insertions(+)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 734a004..18e623f 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -356,6 +356,54 @@ void iio_channel_release(struct iio_channel *channel)
>  }
>  EXPORT_SYMBOL_GPL(iio_channel_release);
>  
> +static void devm_iio_channel_free(struct device *dev, void *res)
> +{
> +	struct iio_channel *channel = *(struct iio_channel **)res;
> +
> +	iio_channel_release(channel);
> +}
> +
> +static int devm_iio_channel_match(struct device *dev, void *res, void *data)
> +{
> +	struct iio_channel **r = res;
> +
> +	if (!r || !*r) {
> +		WARN_ON(!r || !*r);
> +		return 0;
> +	}
> +
> +	return *r == data;
> +}
> +
> +struct iio_channel *devm_iio_channel_get(struct device *dev,
> +					 const char *channel_name)
> +{
> +	struct iio_channel **ptr, *channel;
> +
> +	ptr = devres_alloc(devm_iio_channel_free, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	channel = iio_channel_get(dev, channel_name);
> +	if (IS_ERR(channel)) {
> +		devres_free(ptr);
> +		return channel;
> +	}
> +
> +	*ptr = channel;
> +	devres_add(dev, ptr);
> +
> +	return channel;
> +}
> +EXPORT_SYMBOL_GPL(devm_iio_channel_get);
> +
> +void devm_iio_channel_release(struct device *dev, struct iio_channel *channel)
> +{
> +	WARN_ON(devres_release(dev, devm_iio_channel_free,
> +			       devm_iio_channel_match, channel));
> +}
> +EXPORT_SYMBOL_GPL(devm_iio_channel_release);
> +
>  struct iio_channel *iio_channel_get_all(struct device *dev)
>  {
>  	const char *name;
> diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> index fad5867..e1e033d 100644
> --- a/include/linux/iio/consumer.h
> +++ b/include/linux/iio/consumer.h
> @@ -49,6 +49,33 @@ struct iio_channel *iio_channel_get(struct device *dev,
>  void iio_channel_release(struct iio_channel *chan);
>  
>  /**
> + * devm_iio_channel_get() - Resource managed version of iio_channel_get().
> + * @dev:		Pointer to consumer device. Device name must match
> + *			the name of the device as provided in the iio_map
> + *			with which the desired provider to consumer mapping
> + *			was registered.
> + * @consumer_channel:	Unique name to identify the channel on the consumer
> + *			side. This typically describes the channels use within
> + *			the consumer. E.g. 'battery_voltage'
> + *
> + * Returns a pointer to negative errno if it is not able to get the iio channel
> + * otherwise returns valid pointer for iio channel.
> + *
> + * The allocated iio channel is automatically released when the device is
> + * unbound.
> + */
> +struct iio_channel *devm_iio_channel_get(struct device *dev,
> +					 const char *consumer_channel);
> +/**
> + * devm_iio_channel_release() - Resource managed version of
> + *				iio_channel_release().
> + * @dev:		Pointer to consumer device for which resource
> + *			is allocared.
> + * @chan:		The channel to be released.
> + */
> +void devm_iio_channel_release(struct device *dev, struct iio_channel *chan);
> +
> +/**
>   * iio_channel_get_all() - get all channels associated with a client
>   * @dev:		Pointer to consumer device.
>   *
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web