From: Marco Gerards <mgerards@xs4all.nl>
To: The development of GRUB 2 <grub-devel@gnu.org>
References: <20071015143857.GA26571@thorin> <87wstnqhdb.fsf@xs4all.nl>
	<20071016183420.GA4335@thorin> <87odeyrj7r.fsf@xs4all.nl>
	<20071016200647.GA13827@thorin>
Mail-Copies-To: mgerards@xs4all.nl
Date: Wed, 17 Oct 2007 12:33:49 +0200
In-Reply-To: <20071016200647.GA13827@thorin> (Robert Millan's message of "Tue, 
	16 Oct 2007 22:06:47 +0200")
Message-ID: <873awaqbci.fsf@xs4all.nl>
User-Agent: Gnus/5.110006 (No Gnus v0.6) Emacs/21.4 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Subject: Re: [PATCH] Implement grub_sleep() and grub_ticksleep()
Precedence: list
Reply-To: The development of GRUB 2 <grub-devel@gnu.org>

Robert Millan <rmh@aybabtu.com> writes:

> On Tue, Oct 16, 2007 at 08:46:16PM +0200, Marco Gerards wrote:
>> >
>> > That's what grub_ticksleep does.  grub_sleep() counts in seconds because
>> > I tried to mimic POSIX which seems to be a trend for grub_* functions.  I
>> > think it can be used for menu timeout although I didn't have time to look.
>> 
>> Right.  Although I do not like setting the time in
>> GRUB_TICKS_PER_SECOND for millisecond stuff, etc.  In that case
>> everyone has to implement the same functionality.
>
> Moved to grub_millisleep().

Good :)

>> > OTOH, this wouldn't be the first place in grub where __i386__ is tested ;-)
>> 
>> Oh?  Perhaps that code is wrong?
>
> Actually now that I check it's only in one file.  But the code is right afaict.

What I mean is that this might be a wrong approach in this other file
as well.

>> >> > +
>> >> > +void
>> >> > +grub_ticksleep (grub_uint32_t ticks)
>> >> > +{
>> >> > +  grub_uint32_t end_at;
>> >> > +  end_at = grub_get_rtc () + ticks;
>> >> > +  while (grub_get_rtc () < end_at)
>> >> > +    grub_cpu_idle ();
>> >> > +}
>> >> 
>> >> Why do you recreate this for every arch?  This seems portable as long
>> >> as you can sleep a bit from time to time.
>> >
>> > What if a platform provides a sleep-like mechanism, but not a get_rtc-like
>> > one?  You can implement sleep around get_rtc easily, but not the other way
>> > around.  This is the case for LB (simply because grub_get_rtc is not
>> > implemented yet), but it could also happen on platforms that are designed
>> > not to provide it or are just buggy.
>> 
>> Well, I have no objections to this approach.
>
> Ok.  I made it a bit better by implementing grub_millisleep_generic in
> kern/misc.c and making each port just use that, having the option to do it
> their way if preferred.

Great.

>> Are you sure init.c is
>> the right place?
>
> Mostly.  I've observed that for code that doesn't obviously belong somewhere
> else, it tends to be in init.c if it's in C and startup.S if it's in asm (in
> i386/pc/startup.S it actually gets to the extreme, since only a small part of
> it is used for "startup" as such).
>
> So I think init.c is fine.

Ok.

> 2007-10-15  Robert Millan  <rmh@aybabtu.com>
>
> 	* include/grub/time.h: New file.
> 	* include/grub/i386/time.h: Likewise.
> 	* include/grub/powerpc/time.h: Likewise.
> 	* include/grub/sparc64/time.h: Likewise.
>
> 	* include/grub/i386/pc/time.h (KERNEL_TIME_HEADER): Rename all
> 	instances to ...
> 	(KERNEL_MACHINE_TIME_HEADER): ... this.
> 	* include/grub/powerpc/ieee1275/time.h (KERNEL_TIME_HEADER): Rename all
> 	instances to ...
> 	(KERNEL_MACHINE_TIME_HEADER): ... this.
> 	* include/grub/sparc64/ieee1275/time.h (KERNEL_TIME_HEADER): Rename all
> 	instances to ...
> 	(KERNEL_MACHINE_TIME_HEADER): ... this.
>
> 	* kern/i386/efi/init.c: Include `grub/time.h'.

<grub/time.h> is preferred.

[...]

> +void
> +grub_millisleep_generic (grub_uint32_t ms)
> +{
> +  grub_uint32_t time;
> +  int i;
> +
> +  for (; ms > 0; ms -= TICK_DURATION_IN_MS)
> +    /* wait for the lowest fraction of milliseconds we can (rounded up) */
> +    for (i = 0; i < TICK_DURATION_IN_MS; i++)
> +      {
> +	/* wait for next tick */
> +	time = grub_get_rtc ();
> +	while (time == grub_get_rtc ())
> +	  grub_cpu_idle ();
> +      }
> +}

The problem with this is when TICK_DURATION_IN_MS is not very
accurate.  I think you can be more accurate if you use
TICKS_PER_SECOND and use it to calculate the total amount of ticks to
wait.  This will avoid rounding problems.  Accuracy is not always that
important, but I prefer to have it, if we can.

--
Marco