On Tue, Oct 16, 2007 at 04:11:28PM +0200, Marco Gerards wrote:
> Robert Millan <[EMAIL PROTECTED]> writes:
>
> > This patch implements grub_sleep() and grub_ticksleep().
>
> Great!
>
> > 2007-10-15 Robert Millan <[EMAIL PROTECTED]>
> >
> > * 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 /.)
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
