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]

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



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.

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.

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