Re: [PATCH v4 4/6] x86/time: introduce probing logic for the wallclock

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Sep 05, 2024 at 05:58:47PM +0200, Jan Beulich wrote:
> On 04.09.2024 17:31, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -1291,14 +1291,23 @@ static bool __get_cmos_time(struct rtc_time *rtc)
> >      return t1 <= SECONDS(1) && t2 < MILLISECS(3);
> >  }
> >  
> > -static bool cmos_probe(struct rtc_time *rtc_p, bool cmos_rtc_probe)
> > +static bool __initdata cmos_rtc_probe;
> > +boolean_param("cmos-rtc-probe", cmos_rtc_probe);
> > +
> > +static bool __init cmos_probe(void)
> 
> I'm sorry for not paying attention to this earlier, but "cmos" alone
> is misleading here and perhaps even more so for cmos_read(). These
> aren't about the CMOS (storage) but the CMOS RTC. May I suggest
> cmos_rtc_{probe,read}() instead?

I've assumed that those living in time.c would be clear enough it's
the CMOS RTC, but I'm fine with renaming to cmos_rtc_{probe,read}().

> 
> >  {
> >      unsigned int seconds = 60;
> >  
> > +    if ( !(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) )
> > +        return true;
> > +
> > +    if ( !cmos_rtc_probe )
> > +        return false;
> 
> With this I think ...
> 
> >      for ( ; ; )
> >      {
> > -        bool success = __get_cmos_time(rtc_p);
> > -        struct rtc_time rtc = *rtc_p;
> > +        struct rtc_time rtc;
> > +        bool success = __get_cmos_time(&rtc);
> >  
> >          if ( likely(!cmos_rtc_probe) )
> >              return true;
> 
> ... this ends up being dead code.

Indeed, I've missed to remove that one when moving the check outside
of the for loop.

Thanks, Roger.