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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20101129194033.393e1f70.akpm@linux-foundation.org>
Date:	Mon, 29 Nov 2010 19:40:33 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Huang Ying <ying.huang@...el.com>
Cc:	Len Brown <lenb@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Andi Kleen <andi@...stfloor.org>,
	"Luck, Tony" <tony.luck@...el.com>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>
Subject: Re: [PATCH -v2 2/3] ACPI, APEI, Add APEI generic error status print
 support

On Tue, 30 Nov 2010 11:29:12 +0800 Huang Ying <ying.huang@...el.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@...el.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?

> 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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/