Re: [PATCH -v2 2/3] ACPI, APEI, Add APEI generic error status print support

[Huang Ying <ying.huang@intel.com>]

On Tue, 2010-11-30 at 11:40 +0800, Andrew Morton wrote:
> On Tue, 30 Nov 2010 11:29:12 +0800 Huang Ying <ying.huang@intel.com> 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@intel.com> 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?

The pfx (prefix) here is more than "loglevel", I prefix each line with
"[Hardware Error:]" to make it clear that this is a hardware error
reporting. I think that can be useful for some shared functions doing
printk, the prefix parameter can provide sufficient flexibility for
caller to use prefix like <module name> or <device ID>.

> > 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.

If my understanding is correct, Linus still don't like the idea of user
space hardware error tool.  On the other hand, if we need this tool, I
think printk is not a good tool-oriented hardware error reporting
interface for it, because:

- There is no overall format or record boundaries for printk, because
printk is traditionally for 1-2 lines.  This makes that printk is hard
to parse in general.

- Messages from different CPUs may be interleaved.

- Good error reporting is too verbose for kernel log

- printk has no internal priority support, so that high severity errors
has no more priority than low severity ones.


So my opinion is:

- Use printk as human oriented hardware error reporting.
- Use another tool oriented interface for user space hardware error tool
if necessary.

Do you agree?  Do you think printk can be used as a good tool-oriented
hardware error reporting interface too?


Best Regards,
Huang Ying