Re: [PATCH] Implement grub_sleep() and grub_ticksleep()

[Robert Millan <rmh@aybabtu.com>]

On Tue, Oct 16, 2007 at 04:11:28PM +0200, Marco Gerards wrote:
> 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.

Ok

> > +#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?

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.

> > +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.

Is this really necessary?  It simplifies things a lot, since every cpu would
need a time.h just for that, whereas currently non-i386 gets a dummy stub for
free.

OTOH, this wouldn't be the first place in grub where __i386__ is tested ;-)

> > +#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.

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.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)