On Tue, 30 Nov 2010 11:29:12 +0800 Huang Ying <ying.huang@xxxxxxxxx> wrote: > On Tue, 2010-11-30 at 11:03 +0800, Andrew Morton wrote: > > On Tue, 30 Nov 2010 10:51:40 +0800 Huang Ying <ying.huang@xxxxxxxxx> wrote: > > > > > printk is one of the methods to report hardware errors to user space. > > > Hardware error information reported by firmware to Linux kernel is in > > > the format of APEI generic error status (struct > > > acpi_hes_generic_status). This patch adds print support for the > > > format, so that the corresponding hardware error information can be > > > reported to user space via printk. > > > > > > PCIe AER information print is not implemented yet. Will refactor the > > > original PCIe AER information printing code to avoid code duplicating. > > > > > > ... > > > > > > +#define pr_pfx(pfx, fmt, ...) \ > > > + printk("%s" fmt, pfx, ##__VA_ARGS__) > > > > hm, why does so much code create little printk helper macros. Isn't > > there something generic somewhere? > > Sorry, I do not find the generic code for this helper. But I think this > macro may be helpful for others too, who need to determine the log level > only at runtime. Here corrected errors should have log level: > KERN_WARNING, while uncorrected errors should have log level: KERN_ERR. Oh, is that what it does. Replacing "pfx" everywhere with "loglevel" (or similar) would have been much clearer? > Do you think it is a good idea to make this macro generic? hm, maybe. It's the sort of thing which gives rise to much chin-scratching, which is why people usually avoid doing it ;) If the macro is well-named and its intended use is quite clear then yes, it's probably worth pursuing. > > This patchset appears to implement a new kernel->userspace interface. > > But that interface isn't actually described anywhere, so reviewers must > > reverse-engineer the interface from the implementation to be able to > > review the interface. Nobody bothers doing that so we end up with an > > unreviewed interface, which we must maintain for eternity. > > > > Please fully document all proposed interfaces? > > Sorry. I don't realize that printk-ing something means implementing a > new kernel->userspace interface. I think the messages resulted are > self-explaining for human. Is it sufficient just to add example messages > in patch description? Well normally a printk() isn't really considered a "userspace interface". This allows us to change them even though there surely _are_ existing tools which treat particular messages as a userspace interface. But I don't recall hearing of much breakage from changed kernel printks. However in this case you are avowedly treating the printks as a userspace interface, with the intention that software be written to parse them, yes? So once they're in place, we cannot change them? That makes it more important. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html