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

[Prarit Bhargava]

On 12/03/2013 04:21 PM, Andrew Morton wrote:
> On Mon,  2 Dec 2013 10:19:37 -0500 Prarit Bhargava <prarit@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/