Re: [patch 04/15] cleanup clocksource selection

[john stultz]

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@xxxxxxxxxx>
> > 
> > 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/