Re: [PATCH 1/3] Introduce interface to report BIOS bugs

Bjorn Helgaas
2008-08-20T18:37:48Z

On Wednesday 20 August 2008 11:02:04 am Thomas Renninger wrote:
> From: Christian Kornacker <ckornacker@suse.de>
>
> This is mostly needed for ACPI systems.
> ACPI introduces an endless amount of possible BIOS
> bugs like wrong values, missing functions, etc.
> The kernel has to sanity check all of them and should
> report BIOS bugs as such to the user.

I can't quite decide whether the whole idea is over-engineering
or not. I guess my hesitation is mainly that things like this take
ongoing maintenance to keep them valuable, and that's often where
things fall apart.

> +#define FW_EMERG KERN_EMERG /* System cannot boot */
> +#define FW_ALERT KERN_ALERT /* Risk of HW or data damage,
> + e.g. overheating, dmraid */
> +#define FW_CRIT KERN_CRIT /* A major device is not functional
> + e.g. hpet, lapic, network... */
> +#define FW_ERR KERN_ERR /* A major device is not working
> + as expected, e.g. cpufreq stuck
> + to lowest freq, lowered
> + performance, increased power
> + consumption... */
> +#define FW_WARN KERN_WARNING /* A minor device does not work
> + or is not fully functional,
> + e.g. backlight brightness,
> + Hotplug capabilities of a
> + device that should be
> + hot-plugable will not work */
> +#define FW_INFO KERN_INFO /* Anything else related to BIOS
> + that is worth mentioning */
> +
> +
> +#ifdef CONFIG_REPORT_FIRMWARE_BUGS
> + #define FW_PRINT_WARN(severity, fmt, args...) printk("%s[BIOS]: " fmt "\n", \
> + severity, ##args)
> +#else
> + #define FW_PRINT_WARN(severity, fmt, args...) do { } while (0)
> +#endif
> +
> +#define FW_PRINT_CRIT(severity, fmt, args...) printk("%s[BIOS]: " fmt "\n", \
> + severity, ##args)

I think there are too many possibilities (FW_PRINT_WARN vs FW_PRINT_CRIT,
then one of FW_INFO, FW_WARN, FW_ERR, FW_CRIT, FW_ALERT, FW_EMERG).
A simpler interface with only one or two choices would give 90% of the
benefit.

My preference would be to *not* add a newline inside the interface.
Everybody knows printk needs a newline, and it's simpler if all the
printk variants follow that same rule.

The "BIOS" string is very x86-centric. I'd prefer something like
"firmware" or "FW" that's also applicable to non-x86 systems.

I'm on a real dev_printk() kick at the moment, so I'd like to see
a way to hook a message to *something*, whether it's a specific
device, an ACPI method, a table at a specific physical address, etc.
For example, this:

+ FW_PRINT_CRIT(FW_ERR, PFX "No ACPI _PSS objects for CPU"
+ " other than CPU0. Complain to your BIOS"
+ " vendor");

would be nicer if it could report the specific CPU device.
Admittedly, many of the places you touch don't currently have
an idea of a "device." But sometimes that's a deficiency in
the current Linux implementation, so I think your interface
should at least allow a device.

Maybe even something as simple as:

#define FW_BUG "[FW bug]: "

would be sufficient, with the idea that people could do this:

dev_err(&dev->dev, FW_BUG "interrupts left enabled\n");

I think the user-space value derives from having a consistent string
to grep for, so this gives you that. I'm not sure what value we get
from adding the new FW_PRINT_CRIT()/FW_PRINT_WARN() interfaces in the
kernel.

Bjorn