Messages in this thread | | | From | Bjorn Helgaas <> | Subject | Re: [PATCH 1/3] Introduce interface to report BIOS bugs | Date | Wed, 20 Aug 2008 12:37:42 -0600 |
| |
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
| |