Messages in this thread | | | Subject | Re: [patch 04/15] cleanup clocksource selection | From | john stultz <> | Date | Fri, 14 Aug 2009 18:43:12 -0700 |
| |
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
| |