lkml.org 
[lkml]   [2012]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 4/4 v4] rtc: add rtc-driver for HID sensors of type time
    Am 14.12.2012 17:33, schrieb Lars-Peter Clausen:
    > On 12/14/2012 04:24 PM, Alexander Holler wrote:
    >> Am 14.12.2012 15:34, schrieb Lars-Peter Clausen:
    >>> On 12/14/2012 03:29 PM, Alexander Holler wrote:
    >>>> Am 14.12.2012 15:15, schrieb Alexander Holler:
    >>>>> Am 14.12.2012 14:08, schrieb Alexander Holler:
    >>>>>> Am 14.12.2012 10:42, schrieb Lars-Peter Clausen:
    >>>>>
    >>>>>>> And another thing I've overlooked before:
    >>>>>>> wait_for_completion_interruptible_timeout can either return a positive
    >>>>>>> number when the completion was completed, 0 in case of an timeout, or a
    >>>>>>> negative error code in case it was interrupted. You need to handle all
    >>>>>>> three. E.g. something like this.
    >>>>>>>
    >>>>>>> ret = wait_for_completion_interruptible_timeout(...)
    >>>>>>> if (ret == 0)
    >>>>>>> return -EIO;
    >>>>>>> if (ret < 0)
    >>>>>>> return ret
    >>>>>>>
    >>>>>>
    >>>>>> Hmpf, the only working approach to use some in kernel functions really
    >>>>>> is to the read source yourself and don't trust anything else. :/
    >>>>>
    >>>>> Anyway, my approach doesn't work as it introduces a race condition:
    >>>>>
    >>>>>
    >>>>> /* get a report with all values through requesting one value */
    >>>>> sensor_hub_input_attr_get_raw_value(...)
    >>>>>
    >>>>> /* race if this task goes to slepp and the values were
    >>>>> received before it could call the below wait...
    >>>>>
    >>>>> /* wait for all values (event) */
    >>>>> if (!wait_for_completion_interruptible_timeout(...))
    >>>>>
    >>>>>
    >>>>> I'll have to look for a mechanism how to avoid that. So v5 might need
    >>>>> some time.
    >>>>
    >>>> Sorry for the noise. That INIT_COMPLETION() before the sensor...() does
    >>>> exactly that. So it's enough if I handle the different return situations of
    >>>> wait_for...().
    >>>>
    >>>> I will just use if(wait...()<=0) return -EIO.
    >>>>
    >>>
    >>> No, that's wrong. You should really return the error code returned by
    >>> wait_for_completion_interruptible_timeout(). This will make sure that
    >>> userspace restarts the syscall if necessary.
    >>
    >> Sorry for my ignorance, but which reasons for interruption do exist
    >> which doesn't kill the userspace too? The error number -ESYSRESTART
    >> doesn't offer a hint.
    >
    >
    > Well I'm not an expert on this either, but as far as I know any signal the
    > process is listening on can cause an interruption. Most signals won't kill
    > the process though. More on the whole restart stuff:
    > http://lwn.net/Articles/528935/

    I've come to the conclusion that using
    wait_for_completion_interruptible_timeout() isn't what should be used
    here and will switch to wait_for_completion_killable_timeout(). Here are
    my reasons:

    1. I don't think any caller of rtc_ops.read_time will be prepared to
    handle -ESYSRESTART.

    2. Someone wants the time. Beeing interruptible seems to mean the
    process might be interrupted by signal processing with an unknown long
    duration which could decrease the accuracy even below the desired one
    second (which doesn't look unlikely, signal processing in userspace is
    unpredictable and (mis)used for all kind of stuff).

    3. I've primarily used it to be friendly in means of killable, but
    didn't know that ..._killable() does exist. ;)

    Btw., here is a imho good and especially short introduction about the
    task states along with some pointers to more comprehensive resources:
    http://www.ibm.com/developerworks/linux/library/l-task-killable/

    v5 of that patch will follow. Hopefully the last one necessary.

    Regards,

    Alexander


    \
     
     \ /
      Last update: 2012-12-14 23:01    [W:8.216 / U:0.388 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site