[PATCH v2] iio: adc: exynos5_adc: fix compilation warnings

[PATCH v2] iio: adc: exynos5_adc: fix compilation warnings

[Naveen Krishna Chatradhi]

Fixes the compilation warnings and potential NULL pointer
dereferencing pointed out by "Dan Carpenter".

Signed-off-by: Naveen Krishna Ch <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
Cc: Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
Series-to: linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
Changes since v1:
	Made the exynos_adc driver depends on CONFIG_OF.

 drivers/iio/adc/Kconfig      |    1 +
 drivers/iio/adc/exynos_adc.c |   21 ++++++++++++++-------
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index a40d3c2..9c45c0f 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -105,6 +105,7 @@ config AT91_ADC
 
 config EXYNOS_ADC
 	bool "Exynos ADC driver support"
+	depends on OF
 	help
 	  Core support for the ADC block found in the Samsung EXYNOS series
 	  of SoCs for drivers such as the touchscreen and hwmon to use to share
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index ed6fdd7..4fd5e3a 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -92,7 +92,7 @@ struct exynos_adc {
 	struct completion	completion;
 
 	u32			value;
-	unsigned int            version;
+	unsigned int		version;
 };
 
 static const struct of_device_id exynos_adc_match[] = {
@@ -102,12 +102,12 @@ static const struct of_device_id exynos_adc_match[] = {
 };
 MODULE_DEVICE_TABLE(of, exynos_adc_match);
 
-static inline unsigned int exynos_adc_get_version(struct platform_device *pdev)
+static inline int exynos_adc_get_version(struct platform_device *pdev)
 {
 	const struct of_device_id *match;
 
 	match = of_match_node(exynos_adc_match, pdev->dev.of_node);
-	return (unsigned int)match->data;
+	return (int)match->data;
 }
 
 static int exynos_read_raw(struct iio_dev *indio_dev,
@@ -117,7 +117,7 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
 				long mask)
 {
 	struct exynos_adc *info = iio_priv(indio_dev);
-	unsigned long timeout;
+	int timeout;
 	u32 con1, con2;
 
 	if (mask != IIO_CHAN_INFO_RAW)
@@ -255,7 +255,7 @@ static int exynos_adc_probe(struct platform_device *pdev)
 	struct iio_dev *indio_dev = NULL;
 	struct resource	*mem;
 	int ret = -ENODEV;
-	int irq;
+	int irq, version;
 
 	if (!np)
 		return ret;
@@ -268,6 +268,15 @@ static int exynos_adc_probe(struct platform_device *pdev)
 
 	info = iio_priv(indio_dev);
 
+	version = exynos_adc_get_version(pdev);
+	if (version < 0) {
+		dev_err(&pdev->dev, "no matching of_node, err = %d\n", version);
+		ret = version;
+		goto err_iio;
+	}
+
+	info->version = version;
+
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
 	info->regs = devm_request_and_ioremap(&pdev->dev, mem);
@@ -311,8 +320,6 @@ static int exynos_adc_probe(struct platform_device *pdev)
 		goto err_irq;
 	}
 
-	info->version = exynos_adc_get_version(pdev);
-
 	platform_set_drvdata(pdev, indio_dev);
 
 	indio_dev->name = dev_name(&pdev->dev);
-- 
1.7.9.5

Re: [PATCH v2] iio: adc: exynos5_adc: fix compilation warnings

[Doug Anderson]

Naveen,

On Wed, Mar 6, 2013 at 7:09 PM, Naveen Krishna Chatradhi
<ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> -       unsigned int            version;
> +       unsigned int            version;

Given that you've changed exynos_adc_get_version() to return an int,
shouldn't this be an int too (not unsigned)?


> -static inline unsigned int exynos_adc_get_version(struct platform_device *pdev)
> +static inline int exynos_adc_get_version(struct platform_device *pdev)
>  {
>         const struct of_device_id *match;
>
>         match = of_match_node(exynos_adc_match, pdev->dev.of_node);
> -       return (unsigned int)match->data;
> +       return (int)match->data;

Given that you're now checking for an error code below it seems like
you ought to generate one here.  ;)
...AKA: return an error if match is NULL--don't dereference NULL.


>  static int exynos_read_raw(struct iio_dev *indio_dev,
> @@ -117,7 +117,7 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
>                                 long mask)
>  {
>         struct exynos_adc *info = iio_priv(indio_dev);
> -       unsigned long timeout;
> +       int timeout;

Why change this to an "int" when
wait_for_completion_interruptible_timeout() returns a long?  I agree
with the removal of the "unsigned", though.


> +       version = exynos_adc_get_version(pdev);
> +       if (version < 0) {
> +               dev_err(&pdev->dev, "no matching of_node, err = %d\n", version);
> +               ret = version;
> +               goto err_iio;
> +       }
> +
> +       info->version = version;

Optional (and perhaps a matter of preference): I'd eliminate the
"version" variable here and just re-use "ret" for storing the result
of exynos_adc_get_version().


-Doug

[PATCH v3] iio: adc: exynos5_adc: fix compilation warnings

[Naveen Krishna Chatradhi]

Fixes the compilation warnings and potential NULL pointer
dereferencing pointed out by "Dan Carpenter".

Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
Cc: Jonathan Cameron <jic23@cam.ac.uk>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Series-To: linux-iio@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
To: Dan Carpenter <dan.carpenter@oracle.com>
---
Changes since v2:
1. Timeout should be long not int
2. Remove unnecessary variable version, use ret instead.

Apologize me for top posting.

Doug, There was a comment from Lars regarding the match not
being NULL, if driver depends on CONFIG_OF. So, i've removed
the NULL check in v2 of this patch.
	https://patchwork.kernel.org/patch/2222841/

I'm checking the return value of get_version() for -ve values before
assigning to info->version. So, i left the (unsigned int) unchanged.

If required, I will resubmit with both these comments addressed.

 drivers/iio/adc/Kconfig      |    1 +
 drivers/iio/adc/exynos_adc.c |   18 ++++++++++++------
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 04311f8..da67626 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -93,6 +93,7 @@ config AT91_ADC
 
 config EXYNOS_ADC
 	bool "Exynos ADC driver support"
+	depends on OF
 	help
 	  Core support for the ADC block found in the Samsung EXYNOS series
 	  of SoCs for drivers such as the touchscreen and hwmon to use to share
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index ed6fdd7..2d41bb5 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -92,7 +92,7 @@ struct exynos_adc {
 	struct completion	completion;
 
 	u32			value;
-	unsigned int            version;
+	unsigned int		version;
 };
 
 static const struct of_device_id exynos_adc_match[] = {
@@ -102,12 +102,12 @@ static const struct of_device_id exynos_adc_match[] = {
 };
 MODULE_DEVICE_TABLE(of, exynos_adc_match);
 
-static inline unsigned int exynos_adc_get_version(struct platform_device *pdev)
+static inline int exynos_adc_get_version(struct platform_device *pdev)
 {
 	const struct of_device_id *match;
 
 	match = of_match_node(exynos_adc_match, pdev->dev.of_node);
-	return (unsigned int)match->data;
+	return (int)match->data;
 }
 
 static int exynos_read_raw(struct iio_dev *indio_dev,
@@ -117,7 +117,7 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
 				long mask)
 {
 	struct exynos_adc *info = iio_priv(indio_dev);
-	unsigned long timeout;
+	long timeout;
 	u32 con1, con2;
 
 	if (mask != IIO_CHAN_INFO_RAW)
@@ -268,6 +268,14 @@ static int exynos_adc_probe(struct platform_device *pdev)
 
 	info = iio_priv(indio_dev);
 
+	ret = exynos_adc_get_version(pdev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "no matching of_node, err = %d\n", ret);
+		goto err_iio;
+	}
+
+	info->version = ret;
+
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
 	info->regs = devm_request_and_ioremap(&pdev->dev, mem);
@@ -311,8 +319,6 @@ static int exynos_adc_probe(struct platform_device *pdev)
 		goto err_irq;
 	}
 
-	info->version = exynos_adc_get_version(pdev);
-
 	platform_set_drvdata(pdev, indio_dev);
 
 	indio_dev->name = dev_name(&pdev->dev);
-- 
1.7.9.5

Re: [PATCH v3] iio: adc: exynos5_adc: fix compilation warnings

[Doug Anderson]

Naveen,

On Tue, Mar 12, 2013 at 9:48 PM, Naveen Krishna Chatradhi
<ch.naveen@samsung.com> wrote:
> Doug, There was a comment from Lars regarding the match not
> being NULL, if driver depends on CONFIG_OF. So, i've removed
> the NULL check in v2 of this patch.
>         https://patchwork.kernel.org/patch/2222841/
>
> I'm checking the return value of get_version() for -ve values before
> assigning to info->version. So, i left the (unsigned int) unchanged.

Hmmm, I guess this was the point that confused me.  I went back and
agree with Lars--it can't be NULL.  ...but that means that
exynos_adc_get_version() can't return an error, so why are we checking
for an error?

-Doug

Re: [PATCH v3] iio: adc: exynos5_adc: fix compilation warnings

[Lars-Peter Clausen]

On 03/13/2013 07:01 PM, Doug Anderson wrote:
> Naveen,
> 
> On Tue, Mar 12, 2013 at 9:48 PM, Naveen Krishna Chatradhi
> <ch.naveen@samsung.com> wrote:
>> Doug, There was a comment from Lars regarding the match not
>> being NULL, if driver depends on CONFIG_OF. So, i've removed
>> the NULL check in v2 of this patch.
>>         https://patchwork.kernel.org/patch/2222841/
>>
>> I'm checking the return value of get_version() for -ve values before
>> assigning to info->version. So, i left the (unsigned int) unchanged.
> 
> Hmmm, I guess this was the point that confused me.  I went back and
> agree with Lars--it can't be NULL.  ...but that means that
> exynos_adc_get_version() can't return an error, so why are we checking
> for an error?

Agreed. Adding the dependency on OF in Kconfig should be all that is needed.

- Lars

Re: [PATCH v3] iio: adc: exynos5_adc: fix compilation warnings

[Doug Anderson]

Lars,

On Wed, Mar 13, 2013 at 11:11 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> Agreed. Adding the dependency on OF in Kconfig should be all that is needed.

I think changing the timeout from 'unsigned long' to 'long' is also
legit (to match the actual type returned) and a good idea.

-Doug

Re: [PATCH v3] iio: adc: exynos5_adc: fix compilation warnings

[Lars-Peter Clausen]

On 03/13/2013 07:23 PM, Doug Anderson wrote:
> Lars,
> 
> On Wed, Mar 13, 2013 at 11:11 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> Agreed. Adding the dependency on OF in Kconfig should be all that is needed.
> 
> I think changing the timeout from 'unsigned long' to 'long' is also
> legit (to match the actual type returned) and a good idea.
> 
> -Doug

Yes, but that's a different issue and to be honest I didn't even realize
that the patch was trying to fix this as well. In my opinion it's best to
split this up into two patches one which fixes the OF dependency. The other
fixing the timeout issue. Cause there is more to it than just changing the type.

wait_for_completion_interruptible_timeout() may return
	1) 0, if there was a timeout, waiting for the completion
	2) > 0, if the completion was completeted, the returned value
	   the  remaining time.
	3) < 0, If it was interrupt while waiting for the completion

The code currently only handles 1) and 2), but it also needs to handle 3).
Since the completion has not been completed in case 3.

E.g. something like this should work:

	if (timeout == 0)
		return -ETIMEDOUT;
	else if(timeout < 0)
		return timeout;
	return 0;

- Lars

Re: [PATCH v3] iio: adc: exynos5_adc: fix compilation warnings

[Lars-Peter Clausen]

On 03/13/2013 07:35 PM, Lars-Peter Clausen wrote:
> On 03/13/2013 07:23 PM, Doug Anderson wrote:
>> Lars,
>>
>> On Wed, Mar 13, 2013 at 11:11 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>> Agreed. Adding the dependency on OF in Kconfig should be all that is needed.
>>
>> I think changing the timeout from 'unsigned long' to 'long' is also
>> legit (to match the actual type returned) and a good idea.
>>
>> -Doug
> 
> Yes, but that's a different issue and to be honest I didn't even realize
> that the patch was trying to fix this as well. In my opinion it's best to
> split this up into two patches one which fixes the OF dependency. The other
> fixing the timeout issue. Cause there is more to it than just changing the type.
> 
> wait_for_completion_interruptible_timeout() may return
> 	1) 0, if there was a timeout, waiting for the completion
> 	2) > 0, if the completion was completeted, the returned value
> 	   the  remaining time.
> 	3) < 0, If it was interrupt while waiting for the completion
> 
> The code currently only handles 1) and 2), but it also needs to handle 3).
> Since the completion has not been completed in case 3.
> 
> E.g. something like this should work:
> 
> 	if (timeout == 0)
> 		return -ETIMEDOUT;
> 	else if(timeout < 0)
> 		return timeout;
> 	return 0;
> 

I just saw, there is another issue related to this. The driver should call
INIT_COMPLETION(&info->completion) before starting the conversion. Otherwise
there may be a problem if we got interrupted while waiting for the
interrupt. E.g. imagine the following sequence.

1) Start conversion
2) Wait for completion
3) Abort waiting
4) Interrupt kicks in and marks the completion as done

Now if another conversion is started the completion will already be
completed and wait_for_completion_interruptible_timeout() will return right
away without waiting for the conversion to be finished.

- Lars

Re: [PATCH v3] iio: adc: exynos5_adc: fix compilation warnings

[Doug Anderson]

Lars,

On Wed, Mar 13, 2013 at 11:40 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> Yes, but that's a different issue and to be honest I didn't even realize
>> that the patch was trying to fix this as well. In my opinion it's best to
>> split this up into two patches one which fixes the OF dependency. The other
>> fixing the timeout issue. Cause there is more to it than just changing the type.

Sounds fair to split into two patches.  ;)


>> wait_for_completion_interruptible_timeout() may return
>>       1) 0, if there was a timeout, waiting for the completion
>>       2) > 0, if the completion was completeted, the returned value
>>          the  remaining time.
>>       3) < 0, If it was interrupt while waiting for the completion
>>
>> The code currently only handles 1) and 2), but it also needs to handle 3).
>> Since the completion has not been completed in case 3.
>>
>> E.g. something like this should work:
>>
>>       if (timeout == 0)
>>               return -ETIMEDOUT;
>>       else if(timeout < 0)
>>               return timeout;
>>       return 0;

Good catch.  Yes, that looks right to me.


> I just saw, there is another issue related to this. The driver should call
> INIT_COMPLETION(&info->completion) before starting the conversion. Otherwise
> there may be a problem if we got interrupted while waiting for the
> interrupt. E.g. imagine the following sequence.
>
> 1) Start conversion
> 2) Wait for completion
> 3) Abort waiting
> 4) Interrupt kicks in and marks the completion as done
>
> Now if another conversion is started the completion will already be
> completed and wait_for_completion_interruptible_timeout() will return right
> away without waiting for the conversion to be finished.

Another good catch.  ...but are you sure that your solution is enough?
 It seems like we'd also want to lock a spinlock in the ISR and to
consider the completion protected by the lock.  If we don't do that it
seems like you could get an interrupt _right_ after you re-init the
completion.

Notes:
* I thought we could perhaps just disable the interrupt after
wait_for_completion_interruptible_timeout() returns, but I'm not sure
that's guaranteed to work in a multiprocessor environment.

* I don't see any other iio/adc drivers using a spin_lock so maybe
there's a better way to do it (or I'm misunderstanding).  A quick scan
shows only two other iio/adc drivers even use request_irq().
at91_adc.c appears to suffer from similar problems to what we're
discussing here (only init the completion in probe).  ad_sigma_delta
at lest calls INIT_COMPLETION before waiting but seems like it still
has a small window of race (I'd have to dig more to be sure).


Perhaps someone will tell me that I'm just confused.  ;)

-Doug