linux-kernel - Re: [PATCH 1/3] Introduce FW_INFO* functions and messages

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]

Date:	Wed, 04 Dec 2013 09:20:41 -0800
From:	Joe Perches <joe@...ches.com>
To:	Prarit Bhargava <prarit@...hat.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH 1/3] Introduce FW_INFO* functions and messages

On Wed, 2013-12-04 at 06:51 -0500, Prarit Bhargava wrote:
> 
> On 12/03/2013 04:21 PM, Andrew Morton wrote:
> > On Mon,  2 Dec 2013 10:19:37 -0500 Prarit Bhargava <prarit@...hat.com> wrote:
> > A slight simplification:
> > 
> >> +static inline char *dump_hadware_arch_desc(void)
> >> +{
> >> +	return NULL;
> >> +}
> > 
> > 	return "unavailable";
> > 
> >> +void warn_slowpath_fmt_dev(const char *file, int line,
> >> +			   struct device *dev, int level, const char *fmt, ...)
> >> +{
> >> +	struct slowpath_args args;
> >> +	static char fw_str[16] = "\0";
> >> +
> >> +	switch (level) {
> >> +	case 1:
> >> +		strcpy(fw_str, "[Firmware Info]");
> >> +		break;
> >> +	case 2:
> >> +		strcpy(fw_str, "[Firmware Warn]");
> >> +		break;
> >> +	case 3:
> >> +		strcpy(fw_str, "[Firmware Bug]");
> > 
> > What's With The Crazy Capitalization In This Code?  It's Illiterate!
> 
> Heh :) it's what the original FW_* strings were defined as.  I was hoping it was
> okay to change them but wasn't 100% sure if I should.  I'll definitely take that
> (and the suggestion above) into v2.
> 
> <snip>
> 
> > 
> > The general thrust of the patchset seems useful and important.  I do
> > agree with Joe's suggestion regarding the presentation, although it
> > isn't a show-stopper.
> 
> In V2, I'll take Joe's suggestion into account.  It isn't that difficult to
> rework those chunks of the patch into a single patch.
> 
> > 
> > I do wonder if it all should be generalised a bit - it creates a bunch
> > of infrastructure which is specific to system firmware issues, but
> > what's so special about firmware?  Why can't I use this infrastructure
> > to log netdev errors or acpi errors or PM errors or...?  But I didn't
> > think about it much ;)
> 
> Well ... I was thinking about this.  Some drivers do keep track of firmware
> versions for their devices and I think there is probably a way to unify all that.

Isn't that simply using the generic #define FW_<type>s?

> Also, I think after this initial change goes in I'm going to grep through the
> PCI & ACPI code to see what FW_* changes need to be made there.  AFAICT there
> are many locations in those areas which should be FW_*.
> 
> I'll get a [v2] out in a little while.  Thanks Joe & Andrew for looking.

I think you should not remove the current #defines.
You can just leave them alone.  Add the new facility
without removing the old capability and then as the
utility of your new system is proven, then the old uses
can be converted if appropriate.


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