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.
