lkml.org 
[lkml]   [2013]   [Mar]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v3] iio: adc: exynos5_adc: fix compilation warnings
    From
    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


    \
     
     \ /
      Last update: 2013-03-13 22:04    [W:4.873 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site