* Thomas Renninger <trenn@xxxxxxx> wrote:
> On Thursday 05 February 2009 13:33:31 Ingo Molnar wrote:
> >
> > * Thomas Renninger <trenn@xxxxxxx> wrote:
> >
> > > On Thursday 05 February 2009 13:02:03 Ingo Molnar wrote:
> > > >
> > > > * Thomas Renninger <trenn@xxxxxxx> wrote:
> > > >
> > > > > Signed-off-by: Thomas Renninger <trenn@xxxxxxx>
> > > > > ---
> > > > > arch/x86/kernel/cpu/cpufreq/powernow-k8.c | 12 ++++++------
> > > > > 1 files changed, 6 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> > > b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> > > > > index 83515f1..5aa832f 100644
> > > > > --- a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> > > > > +++ b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> > > > > @@ -1247,12 +1247,12 @@ static int __cpuinit
> powernowk8_cpu_init(struct
> > > cpufreq_policy *pol)
> > > > > * thing gets introduced
> > > > > */
> > > > > if (!print_once) {
> > > > > - WARN_ONCE(1, KERN_ERR FW_BUG PFX "Your BIOS "
> > > > > - "does not provide ACPI _PSS objects "
> > > > > - "in a way that Linux understands. "
> > > > > - "Please report this to the Linux ACPI"
> > > > > - " maintainers and complain to your "
> > > > > - "BIOS vendor.\n");
> > > > > + printk(KERN_ERR FW_BUG PFX "Your BIOS "
> > > > > + "does not provide ACPI _PSS objects "
> > > > > + "in a way that Linux understands. "
> > > > > + "Please report this to the Linux ACPI"
> > > > > + " maintainers and complain to your "
> > > > > + "BIOS vendor.\n");
> > > > > print_once++;
> > > >
> > > > hm, why the open-coded WARN_ONCE? (which print_once flag + the printk in
> > > > essence is)
> > > >
> > > > So please use WARN_ONCE(), and indent it all one tab to the left which
> will
> > > > solve at least part of that ugly 6-line split up thing. And if it's a
> > > > WARN_ONCE() then kerneloops.org will pick it up too.
> > > No.
> > > This happens if your BIOS is older than your CPU and you then miss
> cpufreq.
> > > This often happens on very new machines/CPUs. It could also happen that
> you
> > > have to wait a month or so until your vendor offers a new BIOS.
> > >
> > > We want to tell the user that it's not the kernel's fault, but we better
> do
> > > not spit out a huge backtrace, which is worthless anyway as it's the BIOS
> > > which is broken.
> >
> > That's fine and we can do that, but the text does not suggest that at all.
> >
> > The text says "please report this to the Linux ACPI maintainers" and that
> > Linux does not understand this - and closes with the suggestion that this
> > should be reported to the BIOS vendor too. That tells, to the user, that at
> > minimum Linux is confused.
> >
> > Such text directs the bugreports to _us_ kernel maintainers, not to the BIOS
> > vendors.
> >
> > A much clearer text and implementation would be to do something like:
> >
> > static const char ACPI_PSS_BIOS_BUG_MSG[] =
> > KERN_ERR "Your BIOS does not provide compatible ACPI _PSS objects.\n"
> > KERN_ERR "Complain to your BIOS vendor. This is not a kernel bug.\n";
>
> Yep, even better:
> KERN_ERR FW_BUG "Incompatible ACPI _PSS objects.\n"
> KERN_ERR FW_BUG "Complain to your BIOS vendor.\n";
>
> Then distributions easily can do:
> dmesg |grep '[Firmware Bug]'
>
> and reject the BIOS to get certified or throw a bug back to the
> vendor.
>
> I expect the long version still comes from the times when one
> could not be sure whether it's the Linux ACPI subsystem or the
> BIOS table which is wrong. I agree, that mentioning the kernel to
> possibly be fault, should be deleted.
>
> > [...]
> >
> > if (!print_once) {
> > printk(ACPI_PSS_BIOS_BUG_MSG);
> > print_once = 1;
> > }
> >
> > Note the improvements:
> >
> > - No more ugly linebreaks.
> >
> > - print_once++ was a poor solution as well - the standard thing is to set
> > 'once' flags to 1 - once and forever.
> Hm, it's only incremented once, I do not see why this is a poor solution.
It's not a huge issue - it's just somewhat suboptimal as a coding construct
because it's a bit dissimilar to how things are done typically. When i first
saw it i had to look again because it looked a bit odd - first i was unsure
whether the ++ has an actual _meaning_.
It seems like an insignifican detail (which it really is, if looked at in
isolation), but we have a huge, 10 MLOC kernel full of code. So we really
want elegant-looking, pleasant, predictable, efficient and standard patterns
of code everywhere.
> perfect would be printk_once() similar to WARN_ONCE.
> Andrew mentioned a discussion about implementing such a thing.
> IMO it would be worth it, I needed something like that three times in the
> last 7 patches.
Yeah - i just posted it.
> > - The 6-line split-up warning message does not obscure the code
> > itself anymore. The error condition is clear and clean and visually
> > unintrusive.
> >
> > - The original message text had no linebreak and was about two full lines
> > long when printed - in a single line. If the kernel prints such messages
> > that looks sloppy and confusing. If watched via a serial line then the
> > overlong portion can even be missed at first sight.
> >
> > - If someone hits that warning and sees it in the kernel log, then a
> > git grep ""Your BIOS does not provide compatible ACPI _PSS objects"
> > will come up with arch/x86/kernel/cpu/cpufreq/powernow-k8.c. With the
> > original code it would come up empty and the user/developer would perhaps
> > thing that it's perhaps the distro kernel that prints that warning, not
> > the upstream kernel.
> >
> > Could you please fix it in that fashion? Thanks,
> I fully agree with the "no line break" and not "not grepable" issues.
> I send something new, maybe not today.
Thanks.
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-next" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
