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

[Joe Perches <joe@perches.com>]

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