lkml.org 
[lkml]   [2009]   [Aug]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [patch 04/15] cleanup clocksource selection
    From
    Date
    On Fri, 2009-08-14 at 18:42 -0700, john stultz wrote:
    > On Fri, 2009-08-14 at 15:47 +0200, Martin Schwidefsky wrote:
    > > plain text document attachment (clocksource-select.diff)
    > > From: Martin Schwidefsky <schwidefsky@de.ibm.com>
    > >
    > > If a non high-resolution clocksource is first set as override clock
    > > and then registered it becomes active even if the system is in
    > > on-shot mode. Move the override check from sysfs_override_clocksource
    > > to the clocksource selection. That fixes the bug and simplifies the
    > > code. The check in clocksource_register for double registration of
    > > the same clocksource is removed without replacement.
    > >
    > > To find the initial clocksource a new weak function in jiffies.c is
    > > defined that returns the jiffies clocksource. The architecture code
    > > can then override the weak function with a more suitable clocksource,
    > > e.g. the TOD clock on s390.
    >
    >
    > Hey Martin,
    >
    > So I found the issue I sent mail about earlier, this is patch breaks
    > sysfs clocksource overrides. The suggested fix is below:
    >
    >
    > > @@ -478,10 +478,6 @@ static ssize_t sysfs_override_clocksourc
    > > struct sysdev_attribute *attr,
    > > const char *buf, size_t count)
    > > {
    > > - struct clocksource *ovr = NULL;
    > > - size_t ret = count;
    > > - int len;
    > > -
    > > /* strings from sysfs write are not 0 terminated! */
    > > if (count >= sizeof(override_name))
    > > return -EINVAL;
    > > @@ -495,41 +491,11 @@ static ssize_t sysfs_override_clocksourc
    > > if (count > 0)
    > > memcpy(override_name, buf, count);
    > > override_name[count] = 0;
    > > -
    > > - len = strlen(override_name);
    > > - if (len) {
    > > - struct clocksource *cs;
    > > -
    > > - ovr = clocksource_override;
    > > - /* try to select it: */
    > > - list_for_each_entry(cs, &clocksource_list, list) {
    > > - if (strlen(cs->name) == len &&
    > > - !strcmp(cs->name, override_name))
    > > - ovr = cs;
    > > - }
    > > - }
    > > -
    > > - /*
    > > - * Check to make sure we don't switch to a non-highres capable
    > > - * clocksource if the tick code is in oneshot mode (highres or nohz)
    > > - */
    > > - if (tick_oneshot_mode_active() && ovr &&
    > > - !(ovr->flags & CLOCK_SOURCE_VALID_FOR_HRES)) {
    > > - printk(KERN_WARNING "%s clocksource is not HRT compatible. "
    > > - "Cannot switch while in HRT/NOHZ mode\n", ovr->name);
    > > - ovr = NULL;
    > > - override_name[0] = 0;
    > > - }
    > > -
    > > - /* Reselect, when the override name has changed */
    > > - if (ovr != clocksource_override) {
    > > - clocksource_override = ovr;
    > > - next_clocksource = select_clocksource();
    > > - }
    > > + clocksource_select();
    > >
    > > spin_unlock_irq(&clocksource_lock);
    > >
    > > - return ret;
    > > + return count;
    > > }
    >
    > Here when you return count, count may have been decremented in the code
    > above, which causes the writer to get back fewer bytes then what they
    > passed in, causing the last char to be repeatedly sent. This is what
    > causes the immediate switch back to the default clocksource (override
    > gets set to null), and the hang of the command writing to the sysfs
    > file.
    >
    > The fix is simply keeping the initial "size_t ret = count;" and the
    > final "return ret;"
    >

    Oh, and with this small fixup the entire stack seems to be working
    properly.

    thanks
    -john



    \
     
     \ /
      Last update: 2009-08-15 03:47    [W:2.422 / U:0.188 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site