Re: [PATCH] x86: honor ACPI FADT flag indicating absence of a CMOS RTC

[Ingo Molnar <mingo@kernel.org>]

* Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 18.10.13 at 08:29, Ingo Molnar <mingo@kernel.org> wrote:
> > * Jan Beulich <JBeulich@suse.com> wrote:
> > 
> >> We shouldn't be creating a corresponding platform device in that case.
> > 
> > There's a sad lack of context in the changelog, how was it found, does 
> > this address any problem/bug observed in practice, etc?
> 
> This was a result from code review in Xen hypervisor code in the context 
> of ACPI 5 work there, which lead me to check whether Linux would honor 
> that flag. No known issue in practice so far. Once the below got 
> clarified, I can certainly extend the description in an eventual 
> resubmission (albeit I would have though that fixing the not honoring of 
> a firmware flag should speak for itself).

I thought we've been through this before, firmware related changes never 
'speak for themselves'!

The quality and effect of firmware flags varies wildly (sometimes they are 
good and avoid problems, sometimes they are crap and cause problems) and 
it's important to know the full context.

So in this area there's no such thing as being overly verbose in a 
changelog.

> >> +#ifdef CONFIG_ACPI
> >> +	if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC)
> >> +		return -ENODEV;
> >> +#endif
> > 
> > It might also be prudent to emit a KERN_INFO line telling that we don't 
> > create the device - so that people who suddenly see unexpected breakage or 
> > change in behavior have a chance to see what we've done?
> 
> There are so many other -ENODEV return paths here which don't
> emit messages that this seemed inappropriate, the more that a
> message _is_ being issued if the platform RTC device does get
> registered (and hence one could judge by the message
> disappearing between before and after the patch got applied).

I'm trying to be conservative and protect users against potential bad side 
effects of such a change. It's routine. We can mark it as temporary and 
zap it a few kernel releases later.

Thanks,

	Ingo