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

[Marco Gerards]

Robert Millan <address@hidden> 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  <address@hidden>
>
>       * 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