Fixes the compilation warnings and potential NULL pointer
dereferencing pointed out by "Dan Carpenter".
Signed-off-by: Naveen Krishna Ch <[email protected]>
Cc: Jonathan Cameron <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>
Series-to: [email protected]
Cc: [email protected]
Cc: Dan Carpenter <[email protected]>
---
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
Naveen,
On Wed, Mar 6, 2013 at 7:09 PM, Naveen Krishna Chatradhi
<[email protected]> 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
Fixes the compilation warnings and potential NULL pointer
dereferencing pointed out by "Dan Carpenter".
Signed-off-by: Naveen Krishna Chatradhi <[email protected]>
Cc: Jonathan Cameron <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>
Series-To: [email protected]
Cc: [email protected]
To: Dan Carpenter <[email protected]>
---
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
Naveen,
On Tue, Mar 12, 2013 at 9:48 PM, Naveen Krishna Chatradhi
<[email protected]> 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
On 03/13/2013 07:01 PM, Doug Anderson wrote:
> Naveen,
>
> On Tue, Mar 12, 2013 at 9:48 PM, Naveen Krishna Chatradhi
> <[email protected]> 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
Lars,
On Wed, Mar 13, 2013 at 11:11 AM, Lars-Peter Clausen <[email protected]> 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
On 03/13/2013 07:23 PM, Doug Anderson wrote:
> Lars,
>
> On Wed, Mar 13, 2013 at 11:11 AM, Lars-Peter Clausen <[email protected]> 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
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 <[email protected]> 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
Lars,
On Wed, Mar 13, 2013 at 11:40 AM, Lars-Peter Clausen <[email protected]> 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