Adding such probing allows to clearly separate init vs runtime code, and to
place the probing logic into the init section for the CMOS case.  Note both
the Xen shared_info page wallclock, and the EFI wallclock don't really have any
probing-specific logic.  The shared_info wallclock will always be there if
booted as a Xen guest, while the EFI_GET_TIME method probing relies on checking
if it returns a value different than 0.

The panic message printed when Xen is unable to find a viable wallclock source
has been adjusted slightly, I believe the printed guidance still provides the
same amount of information to the user.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - New in this version.
---
 xen/arch/x86/time.c | 116 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 92 insertions(+), 24 deletions(-)

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 10840757b22c..8402131d7b6a 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1292,14 +1292,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)
 {
     unsigned int seconds = 60;
 
+    if ( !(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) )
+        return true;
+
+    if ( !cmos_rtc_probe )
+        return false;
+
     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;
@@ -1329,28 +1338,13 @@ static bool cmos_probe(struct rtc_time *rtc_p, bool cmos_rtc_probe)
     return false;
 }
 
-static unsigned long get_cmos_time(void)
+
+static unsigned long cmos_read(void)
 {
-    unsigned long res;
     struct rtc_time rtc;
-    static bool __read_mostly cmos_rtc_probe;
-    boolean_param("cmos-rtc-probe", cmos_rtc_probe);
+    bool success = __get_cmos_time(&rtc);
 
-    if ( efi_enabled(EFI_RS) )
-    {
-        res = efi_get_time();
-        if ( res )
-            return res;
-    }
-
-    if ( likely(!(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC)) )
-        cmos_rtc_probe = false;
-    else if ( system_state < SYS_STATE_smp_boot && !cmos_rtc_probe )
-        panic("System with no CMOS RTC advertised must be booted from EFI"
-              " (or with command line option \"cmos-rtc-probe\")\n");
-
-    if ( !cmos_probe(&rtc, cmos_rtc_probe) )
-        panic("No CMOS RTC found - system must be booted from EFI\n");
+    ASSERT(success);
 
     return mktime(rtc.year, rtc.mon, rtc.day, rtc.hour, rtc.min, rtc.sec);
 }
@@ -1533,12 +1527,82 @@ void rtc_guest_write(unsigned int port, unsigned int data)
     }
 }
 
-static unsigned long get_wallclock_time(void)
+static enum {
+    WALLCLOCK_UNSET,
+    WALLCLOCK_XEN,
+    WALLCLOCK_CMOS,
+    WALLCLOCK_EFI,
+} wallclock_source __ro_after_init;
+
+static const char *wallclock_type_to_string(void)
 {
+    switch ( wallclock_source )
+    {
+    case WALLCLOCK_XEN:
+        return "XEN";
+
+    case WALLCLOCK_CMOS:
+        return "CMOS RTC";
+
+    case WALLCLOCK_EFI:
+        return "EFI";
+
+    case WALLCLOCK_UNSET:
+        return "UNSET";
+    }
+
+    ASSERT_UNREACHABLE();
+    return "";
+}
+
+static void __init probe_wallclock(void)
+{
+    ASSERT(wallclock_source == WALLCLOCK_UNSET);
+
     if ( xen_guest )
+    {
+        wallclock_source = WALLCLOCK_XEN;
+        return;
+    }
+    if ( efi_enabled(EFI_RS) && efi_get_time() )
+    {
+        wallclock_source = WALLCLOCK_EFI;
+        return;
+    }
+    if ( cmos_probe() )
+    {
+        wallclock_source = WALLCLOCK_CMOS;
+        return;
+    }
+
+    panic("No usable wallclock found, probed:%s%s%s\n%s",
+          !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
+          cmos_rtc_probe ? " CMOS" : "",
+          efi_enabled(EFI_RS) ? " EFI" : "",
+          !cmos_rtc_probe ? "Try with command line option \"cmos-rtc-probe\"\n"
+           : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n" : "");
+}
+
+static unsigned long get_wallclock_time(void)
+{
+    switch ( wallclock_source )
+    {
+    case WALLCLOCK_XEN:
         return read_xen_wallclock();
 
-    return get_cmos_time();
+    case WALLCLOCK_CMOS:
+        return cmos_read();
+
+    case WALLCLOCK_EFI:
+        return efi_get_time();
+
+    case WALLCLOCK_UNSET:
+        /* Unexpected state - handled by the ASSERT_UNREACHABLE() below. */
+        break;
+    }
+
+    ASSERT_UNREACHABLE();
+    return 0;
 }
 
 /***************************************************************************
@@ -2463,6 +2527,10 @@ int __init init_xen_time(void)
 
     open_softirq(TIME_CALIBRATE_SOFTIRQ, local_time_calibration);
 
+    probe_wallclock();
+
+    printk(XENLOG_INFO "Wallclock source: %s\n", wallclock_type_to_string());
+
     /* NB. get_wallclock_time() can take over one second to execute. */
     do_settime(get_wallclock_time(), 0, NOW());
 
-- 
2.46.0

On 03.09.2024 15:03, Roger Pau Monne wrote:
> Adding such probing allows to clearly separate init vs runtime code, and to
> place the probing logic into the init section for the CMOS case.  Note both
> the Xen shared_info page wallclock, and the EFI wallclock don't really have any
> probing-specific logic.  The shared_info wallclock will always be there if
> booted as a Xen guest, while the EFI_GET_TIME method probing relies on checking
> if it returns a value different than 0.
> 
> The panic message printed when Xen is unable to find a viable wallclock source
> has been adjusted slightly, I believe the printed guidance still provides the
> same amount of information to the user.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Looks a little involved, but I'm largely fine with it; just a couple of
more or less cosmetic remarks:

> @@ -1329,28 +1338,13 @@ static bool cmos_probe(struct rtc_time *rtc_p, bool cmos_rtc_probe)
>      return false;
>  }
>  
> -static unsigned long get_cmos_time(void)
> +
> +static unsigned long cmos_read(void)
>  {
> -    unsigned long res;
>      struct rtc_time rtc;
> -    static bool __read_mostly cmos_rtc_probe;
> -    boolean_param("cmos-rtc-probe", cmos_rtc_probe);
> +    bool success = __get_cmos_time(&rtc);
>  
> -    if ( efi_enabled(EFI_RS) )
> -    {
> -        res = efi_get_time();
> -        if ( res )
> -            return res;
> -    }
> -
> -    if ( likely(!(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC)) )
> -        cmos_rtc_probe = false;
> -    else if ( system_state < SYS_STATE_smp_boot && !cmos_rtc_probe )
> -        panic("System with no CMOS RTC advertised must be booted from EFI"
> -              " (or with command line option \"cmos-rtc-probe\")\n");
> -
> -    if ( !cmos_probe(&rtc, cmos_rtc_probe) )
> -        panic("No CMOS RTC found - system must be booted from EFI\n");
> +    ASSERT(success);

I'm not convinced of this assertion: It's either too much (compared to
what we had so far) or not enough, considering the behavior ...

>      return mktime(rtc.year, rtc.mon, rtc.day, rtc.hour, rtc.min, rtc.sec);
>  }

... with a release build.

> @@ -1533,12 +1527,82 @@ void rtc_guest_write(unsigned int port, unsigned int data)
>      }
>  }
>  
> -static unsigned long get_wallclock_time(void)
> +static enum {
> +    WALLCLOCK_UNSET,
> +    WALLCLOCK_XEN,
> +    WALLCLOCK_CMOS,
> +    WALLCLOCK_EFI,
> +} wallclock_source __ro_after_init;
> +
> +static const char *wallclock_type_to_string(void)

__init ?

>  {
> +    switch ( wallclock_source )
> +    {
> +    case WALLCLOCK_XEN:
> +        return "XEN";
> +
> +    case WALLCLOCK_CMOS:
> +        return "CMOS RTC";
> +
> +    case WALLCLOCK_EFI:
> +        return "EFI";
> +
> +    case WALLCLOCK_UNSET:
> +        return "UNSET";
> +    }
> +
> +    ASSERT_UNREACHABLE();
> +    return "";
> +}
> +
> +static void __init probe_wallclock(void)
> +{
> +    ASSERT(wallclock_source == WALLCLOCK_UNSET);
> +
>      if ( xen_guest )
> +    {
> +        wallclock_source = WALLCLOCK_XEN;
> +        return;
> +    }
> +    if ( efi_enabled(EFI_RS) && efi_get_time() )
> +    {
> +        wallclock_source = WALLCLOCK_EFI;
> +        return;
> +    }
> +    if ( cmos_probe() )
> +    {
> +        wallclock_source = WALLCLOCK_CMOS;
> +        return;
> +    }
> +
> +    panic("No usable wallclock found, probed:%s%s%s\n%s",
> +          !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
> +          cmos_rtc_probe ? " CMOS" : "",
> +          efi_enabled(EFI_RS) ? " EFI" : "",
> +          !cmos_rtc_probe ? "Try with command line option \"cmos-rtc-probe\"\n"
> +           : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n" : "");

This last argument is sufficiently complex that I think it is pretty
important for the question marks and colons to respectively align with
one another, even if this may mean one or two more lines of code.

Jan

On Tue, Sep 03, 2024 at 05:32:27PM +0200, Jan Beulich wrote:
> On 03.09.2024 15:03, Roger Pau Monne wrote:
> > Adding such probing allows to clearly separate init vs runtime code, and to
> > place the probing logic into the init section for the CMOS case.  Note both
> > the Xen shared_info page wallclock, and the EFI wallclock don't really have any
> > probing-specific logic.  The shared_info wallclock will always be there if
> > booted as a Xen guest, while the EFI_GET_TIME method probing relies on checking
> > if it returns a value different than 0.
> > 
> > The panic message printed when Xen is unable to find a viable wallclock source
> > has been adjusted slightly, I believe the printed guidance still provides the
> > same amount of information to the user.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Looks a little involved, but I'm largely fine with it; just a couple of
> more or less cosmetic remarks:
> 
> > @@ -1329,28 +1338,13 @@ static bool cmos_probe(struct rtc_time *rtc_p, bool cmos_rtc_probe)
> >      return false;
> >  }
> >  
> > -static unsigned long get_cmos_time(void)
> > +
> > +static unsigned long cmos_read(void)
> >  {
> > -    unsigned long res;
> >      struct rtc_time rtc;
> > -    static bool __read_mostly cmos_rtc_probe;
> > -    boolean_param("cmos-rtc-probe", cmos_rtc_probe);
> > +    bool success = __get_cmos_time(&rtc);
> >  
> > -    if ( efi_enabled(EFI_RS) )
> > -    {
> > -        res = efi_get_time();
> > -        if ( res )
> > -            return res;
> > -    }
> > -
> > -    if ( likely(!(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC)) )
> > -        cmos_rtc_probe = false;
> > -    else if ( system_state < SYS_STATE_smp_boot && !cmos_rtc_probe )
> > -        panic("System with no CMOS RTC advertised must be booted from EFI"
> > -              " (or with command line option \"cmos-rtc-probe\")\n");
> > -
> > -    if ( !cmos_probe(&rtc, cmos_rtc_probe) )
> > -        panic("No CMOS RTC found - system must be booted from EFI\n");
> > +    ASSERT(success);
> 
> I'm not convinced of this assertion: It's either too much (compared to
> what we had so far) or not enough, considering the behavior ...
> 
> >      return mktime(rtc.year, rtc.mon, rtc.day, rtc.hour, rtc.min, rtc.sec);
> >  }
> 
> ... with a release build.

My reasoning was that on a debug build we want to spot any such
issues (as it's likely a symptom the RTC is misbehaving?) but on a release
build we should rather return an incorrect wallclock time rather than
panicking.  I can remove the ASSERT and local variable altogether if
you prefer.

> 
> > @@ -1533,12 +1527,82 @@ void rtc_guest_write(unsigned int port, unsigned int data)
> >      }
> >  }
> >  
> > -static unsigned long get_wallclock_time(void)
> > +static enum {
> > +    WALLCLOCK_UNSET,
> > +    WALLCLOCK_XEN,
> > +    WALLCLOCK_CMOS,
> > +    WALLCLOCK_EFI,
> > +} wallclock_source __ro_after_init;
> > +
> > +static const char *wallclock_type_to_string(void)
> 
> __init ?
> 
> >  {
> > +    switch ( wallclock_source )
> > +    {
> > +    case WALLCLOCK_XEN:
> > +        return "XEN";
> > +
> > +    case WALLCLOCK_CMOS:
> > +        return "CMOS RTC";
> > +
> > +    case WALLCLOCK_EFI:
> > +        return "EFI";
> > +
> > +    case WALLCLOCK_UNSET:
> > +        return "UNSET";
> > +    }
> > +
> > +    ASSERT_UNREACHABLE();
> > +    return "";
> > +}
> > +
> > +static void __init probe_wallclock(void)
> > +{
> > +    ASSERT(wallclock_source == WALLCLOCK_UNSET);
> > +
> >      if ( xen_guest )
> > +    {
> > +        wallclock_source = WALLCLOCK_XEN;
> > +        return;
> > +    }
> > +    if ( efi_enabled(EFI_RS) && efi_get_time() )
> > +    {
> > +        wallclock_source = WALLCLOCK_EFI;
> > +        return;
> > +    }
> > +    if ( cmos_probe() )
> > +    {
> > +        wallclock_source = WALLCLOCK_CMOS;
> > +        return;
> > +    }
> > +
> > +    panic("No usable wallclock found, probed:%s%s%s\n%s",
> > +          !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
> > +          cmos_rtc_probe ? " CMOS" : "",
> > +          efi_enabled(EFI_RS) ? " EFI" : "",
> > +          !cmos_rtc_probe ? "Try with command line option \"cmos-rtc-probe\"\n"
> > +           : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n" : "");
> 
> This last argument is sufficiently complex that I think it is pretty
> important for the question marks and colons to respectively align with
> one another, even if this may mean one or two more lines of code.

I had it that way originally, but then it seemed the extra
indentation made it less readable.  Will see how can I adjust it, my
preference would be for:

    panic("No usable wallclock found, probed:%s%s%s\n%s",
          !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
          cmos_rtc_probe ? " CMOS" : "",
          efi_enabled(EFI_RS) ? " EFI" : "",
          !cmos_rtc_probe ? "Try with command line option \"cmos-rtc-probe\"\n"
                          : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n"
                                                 : "");

But that exceeds the 80 columns limit.

Thanks, Roger.

On 04.09.2024 12:58, Roger Pau Monné wrote:
> On Tue, Sep 03, 2024 at 05:32:27PM +0200, Jan Beulich wrote:
>> On 03.09.2024 15:03, Roger Pau Monne wrote:
>>> @@ -1329,28 +1338,13 @@ static bool cmos_probe(struct rtc_time *rtc_p, bool cmos_rtc_probe)
>>>      return false;
>>>  }
>>>  
>>> -static unsigned long get_cmos_time(void)
>>> +
>>> +static unsigned long cmos_read(void)
>>>  {
>>> -    unsigned long res;
>>>      struct rtc_time rtc;
>>> -    static bool __read_mostly cmos_rtc_probe;
>>> -    boolean_param("cmos-rtc-probe", cmos_rtc_probe);
>>> +    bool success = __get_cmos_time(&rtc);
>>>  
>>> -    if ( efi_enabled(EFI_RS) )
>>> -    {
>>> -        res = efi_get_time();
>>> -        if ( res )
>>> -            return res;
>>> -    }
>>> -
>>> -    if ( likely(!(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC)) )
>>> -        cmos_rtc_probe = false;
>>> -    else if ( system_state < SYS_STATE_smp_boot && !cmos_rtc_probe )
>>> -        panic("System with no CMOS RTC advertised must be booted from EFI"
>>> -              " (or with command line option \"cmos-rtc-probe\")\n");
>>> -
>>> -    if ( !cmos_probe(&rtc, cmos_rtc_probe) )
>>> -        panic("No CMOS RTC found - system must be booted from EFI\n");
>>> +    ASSERT(success);
>>
>> I'm not convinced of this assertion: It's either too much (compared to
>> what we had so far) or not enough, considering the behavior ...
>>
>>>      return mktime(rtc.year, rtc.mon, rtc.day, rtc.hour, rtc.min, rtc.sec);
>>>  }
>>
>> ... with a release build.
> 
> My reasoning was that on a debug build we want to spot any such
> issues (as it's likely a symptom the RTC is misbehaving?) but on a release
> build we should rather return an incorrect wallclock time rather than
> panicking.  I can remove the ASSERT and local variable altogether if
> you prefer.

I would prefer that, yes, but I also won't insist.

>>> @@ -1533,12 +1527,82 @@ void rtc_guest_write(unsigned int port, unsigned int data)
>>>      }
>>>  }
>>>  
>>> -static unsigned long get_wallclock_time(void)
>>> +static enum {
>>> +    WALLCLOCK_UNSET,
>>> +    WALLCLOCK_XEN,
>>> +    WALLCLOCK_CMOS,
>>> +    WALLCLOCK_EFI,
>>> +} wallclock_source __ro_after_init;
>>> +
>>> +static const char *wallclock_type_to_string(void)
>>
>> __init ?
>>
>>>  {
>>> +    switch ( wallclock_source )
>>> +    {
>>> +    case WALLCLOCK_XEN:
>>> +        return "XEN";
>>> +
>>> +    case WALLCLOCK_CMOS:
>>> +        return "CMOS RTC";
>>> +
>>> +    case WALLCLOCK_EFI:
>>> +        return "EFI";
>>> +
>>> +    case WALLCLOCK_UNSET:
>>> +        return "UNSET";
>>> +    }
>>> +
>>> +    ASSERT_UNREACHABLE();
>>> +    return "";
>>> +}
>>> +
>>> +static void __init probe_wallclock(void)
>>> +{
>>> +    ASSERT(wallclock_source == WALLCLOCK_UNSET);
>>> +
>>>      if ( xen_guest )
>>> +    {
>>> +        wallclock_source = WALLCLOCK_XEN;
>>> +        return;
>>> +    }
>>> +    if ( efi_enabled(EFI_RS) && efi_get_time() )
>>> +    {
>>> +        wallclock_source = WALLCLOCK_EFI;
>>> +        return;
>>> +    }
>>> +    if ( cmos_probe() )
>>> +    {
>>> +        wallclock_source = WALLCLOCK_CMOS;
>>> +        return;
>>> +    }
>>> +
>>> +    panic("No usable wallclock found, probed:%s%s%s\n%s",
>>> +          !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
>>> +          cmos_rtc_probe ? " CMOS" : "",
>>> +          efi_enabled(EFI_RS) ? " EFI" : "",
>>> +          !cmos_rtc_probe ? "Try with command line option \"cmos-rtc-probe\"\n"
>>> +           : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n" : "");
>>
>> This last argument is sufficiently complex that I think it is pretty
>> important for the question marks and colons to respectively align with
>> one another, even if this may mean one or two more lines of code.
> 
> I had it that way originally, but then it seemed the extra
> indentation made it less readable.  Will see how can I adjust it, my
> preference would be for:
> 
>     panic("No usable wallclock found, probed:%s%s%s\n%s",
>           !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
>           cmos_rtc_probe ? " CMOS" : "",
>           efi_enabled(EFI_RS) ? " EFI" : "",
>           !cmos_rtc_probe ? "Try with command line option \"cmos-rtc-probe\"\n"
>                           : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n"
>                                                  : "");
> 
> But that exceeds the 80 columns limit.

Right, formally the above would be my preference, too. Here two shorter-
lines alternatives:

    panic("No usable wallclock found, probed:%s%s%s\n%s",
          !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
          cmos_rtc_probe ? " CMOS" : "",
          efi_enabled(EFI_RS) ? " EFI" : "",
          !cmos_rtc_probe
          ? "Try with command line option \"cmos-rtc-probe\"\n"
          : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n"
                                 : "");

    panic("No usable wallclock found, probed:%s%s%s\n%s",
          !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
          cmos_rtc_probe ? " CMOS" : "",
          efi_enabled(EFI_RS) ? " EFI" : "",
          !cmos_rtc_probe
              ? "Try with command line option \"cmos-rtc-probe\"\n"
              : !efi_enabled(EFI_RS)
                  ? "System must be booted from EFI\n"
                  : "");

Either of these or anything more or less similar will do imo, just as
long as the ? vs : alignment is there.

One thing I notice only now: The trailing %s will be a little odd if
the "" variant is used in the last argument. That'll produce "(XEN) "
with nothing following in the log. Which usually is a sign of some
strange breakage.

Jan

On Wed, Sep 04, 2024 at 01:49:36PM +0200, Jan Beulich wrote:
> On 04.09.2024 12:58, Roger Pau Monné wrote:
> > I had it that way originally, but then it seemed the extra
> > indentation made it less readable.  Will see how can I adjust it, my
> > preference would be for:
> > 
> >     panic("No usable wallclock found, probed:%s%s%s\n%s",
> >           !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
> >           cmos_rtc_probe ? " CMOS" : "",
> >           efi_enabled(EFI_RS) ? " EFI" : "",
> >           !cmos_rtc_probe ? "Try with command line option \"cmos-rtc-probe\"\n"
> >                           : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n"
> >                                                  : "");
> > 
> > But that exceeds the 80 columns limit.
> 
> Right, formally the above would be my preference, too. Here two shorter-
> lines alternatives:
> 
>     panic("No usable wallclock found, probed:%s%s%s\n%s",
>           !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
>           cmos_rtc_probe ? " CMOS" : "",
>           efi_enabled(EFI_RS) ? " EFI" : "",
>           !cmos_rtc_probe
>           ? "Try with command line option \"cmos-rtc-probe\"\n"
>           : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n"
>                                  : "");
> 
>     panic("No usable wallclock found, probed:%s%s%s\n%s",
>           !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
>           cmos_rtc_probe ? " CMOS" : "",
>           efi_enabled(EFI_RS) ? " EFI" : "",
>           !cmos_rtc_probe
>               ? "Try with command line option \"cmos-rtc-probe\"\n"
>               : !efi_enabled(EFI_RS)
>                   ? "System must be booted from EFI\n"
>                   : "");
> 
> Either of these or anything more or less similar will do imo, just as
> long as the ? vs : alignment is there.

I think I prefer the second variant, as indentation is clearer there.

> 
> One thing I notice only now: The trailing %s will be a little odd if
> the "" variant is used in the last argument. That'll produce "(XEN) "
> with nothing following in the log. Which usually is a sign of some
> strange breakage.

I've tested this and it doesn't produce an extra newline if the string
parameter is "".  IOW:

printk("FOO\n%s", "");

Results in:

(XEN) [    2.230603] TSC deadline timer enabled
(XEN) [    2.235654] FOO
(XEN) [    2.238682] Wallclock source: EFI

Thanks, Roger.

On 04.09.2024 14:30, Roger Pau Monné wrote:
> On Wed, Sep 04, 2024 at 01:49:36PM +0200, Jan Beulich wrote:
>> On 04.09.2024 12:58, Roger Pau Monné wrote:
>>> I had it that way originally, but then it seemed the extra
>>> indentation made it less readable.  Will see how can I adjust it, my
>>> preference would be for:
>>>
>>>     panic("No usable wallclock found, probed:%s%s%s\n%s",
>>>           !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
>>>           cmos_rtc_probe ? " CMOS" : "",
>>>           efi_enabled(EFI_RS) ? " EFI" : "",
>>>           !cmos_rtc_probe ? "Try with command line option \"cmos-rtc-probe\"\n"
>>>                           : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n"
>>>                                                  : "");
>>>
>>> But that exceeds the 80 columns limit.
>>
>> Right, formally the above would be my preference, too. Here two shorter-
>> lines alternatives:
>>
>>     panic("No usable wallclock found, probed:%s%s%s\n%s",
>>           !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
>>           cmos_rtc_probe ? " CMOS" : "",
>>           efi_enabled(EFI_RS) ? " EFI" : "",
>>           !cmos_rtc_probe
>>           ? "Try with command line option \"cmos-rtc-probe\"\n"
>>           : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n"
>>                                  : "");
>>
>>     panic("No usable wallclock found, probed:%s%s%s\n%s",
>>           !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
>>           cmos_rtc_probe ? " CMOS" : "",
>>           efi_enabled(EFI_RS) ? " EFI" : "",
>>           !cmos_rtc_probe
>>               ? "Try with command line option \"cmos-rtc-probe\"\n"
>>               : !efi_enabled(EFI_RS)
>>                   ? "System must be booted from EFI\n"
>>                   : "");
>>
>> Either of these or anything more or less similar will do imo, just as
>> long as the ? vs : alignment is there.
> 
> I think I prefer the second variant, as indentation is clearer there.
> 
>>
>> One thing I notice only now: The trailing %s will be a little odd if
>> the "" variant is used in the last argument. That'll produce "(XEN) "
>> with nothing following in the log. Which usually is a sign of some
>> strange breakage.
> 
> I've tested this and it doesn't produce an extra newline if the string
> parameter is "".  IOW:
> 
> printk("FOO\n%s", "");
> 
> Results in:
> 
> (XEN) [    2.230603] TSC deadline timer enabled
> (XEN) [    2.235654] FOO
> (XEN) [    2.238682] Wallclock source: EFI

Oh, my mistake. Format string processing of course comes before the
determination of line breaks within what is to be output.

Jan