| Message ID | 1363150138-20819-1-git-send-email-ch.naveen@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)