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

[Marco Gerards <mgerards@xs4all.nl>]

Robert Millan <rmh@aybabtu.com> writes:

Hi,

[...]

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

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.

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

Most of the time we use the arch specific header files.  That is what
they are for.

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

Oh?  Perhaps that code is wrong?

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

Well, I have no objections to this approach.  Are you sure init.c is
the right place?

--
Marco