From: Marco Gerards <mgerards@xs4all.nl>
To: The development of GRUB 2 <grub-devel@gnu.org>
References: <20071015143857.GA26571@thorin>
Mail-Copies-To: mgerards@xs4all.nl
Date: Tue, 16 Oct 2007 16:11:28 +0200
In-Reply-To: <20071015143857.GA26571@thorin> (Robert Millan's message of "Mon, 
	15 Oct 2007 16:38:57 +0200")
Message-ID: <87wstnqhdb.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:

> This patch implements grub_sleep() and grub_ticksleep().

Great!

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

Better just repeat the same process for the last two lines.

> 	* kern/i386/efi/init.c: Include `grub/time.h'.
> 	(grub_ticksleep): New function.
> 	* kern/i386/pc/init.c: Likewise.
> 	* kern/powerpc/ieee1275/init.c: Likewise.
> 	* kern/sparc64/ieee1275/init.c: Likewise.

Please repeat is.  Only use likewise when the change to one function
is the same.  Better do too much instead of not enough.

> +#include <grub/symbol.h>
> +#include <grub/machine/time.h>
> +
> +void EXPORT_FUNC(grub_ticksleep) (grub_uint32_t ticks);
> +
> +static __inline void
> +grub_sleep (grub_uint32_t s)
> +{
> +  grub_ticksleep (s * GRUB_TICKS_PER_SECOND);
> +}

Sleeping entire seconds is a bit much.  Can you also add this for
smaller time instances?  If you divide, please round up so that no
waiting at all is avoided.  Most of the time you can better wait a bit
too long, I think.

> +static __inline void
> +grub_cpu_idle ()
> +{
> +#if defined(__i386__)
> +  __asm__ __volatile__ ("hlt");
> +  /* FIXME: add other CPUs here */
> +#endif
> +}

This should go into a arch specific headerfile.

> +#endif /* ! KERNEL_TIME_HEADER */
> diff -Nur grub2/kern/i386/efi/init.c grub2.ticks/kern/i386/efi/init.c
> --- grub2/kern/i386/efi/init.c	2007-07-22 01:32:27.000000000 +0200
> +++ grub2.ticks/kern/i386/efi/init.c	2007-10-15 16:28:06.000000000 +0200
> @@ -25,6 +25,16 @@
>  #include <grub/cache.h>
>  #include <grub/kernel.h>
>  #include <grub/efi/efi.h>
> +#include <grub/time.h>
> +
> +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.

--
Marco