Re: [PATCH 1/3] Introduce FW_INFO* functions and messages
On Mon, 2013-12-16 at 08:01 -0500, Prarit Bhargava wrote:
> Sorry everyone, I was out on PTO for the past few weeks.
>
>
> On 12/06/2013 07:30 AM, Matt Fleming wrote:
> > On Thu, 05 Dec, at 07:55:03AM, Joe Perches wrote:
> >> On Thu, 2013-12-05 at 11:30 +, Matt Fleming wrote:
> >>> On Wed, 04 Dec, at 07:22:57PM, Arnd Bergmann wrote:
> The other part I noticed about this particular patchset is that it's
> not really "firmware" as such, but specifically PC wiht ACPI that
> gets covered here. So rather than generalizing the code, another
> option would be to narrow down the scope and make it
> acpi_{warn,info,dbg} instead.
> >>>
> >>> Making this specific to ACPI runs the risk of people introducing a
> >>> multitude of new logging functions for every subsystem, e.g.
> >>> efi_{warn,info,dbg}.
> >>
> >> There are many subsystem specific logging functions:
> >
> > Surely that's further justification to not introduce any more.
>
> That's what I was thinking when I saw this discussion.
I have no problem with introducing more _
functions/macros.
It can reduce overall object size quite a lot.
see commits like: 227842d1176019512d24236f7fb894f0fadd30d1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] Introduce FW_INFO* functions and messages
Sorry everyone, I was out on PTO for the past few weeks.
On 12/06/2013 07:30 AM, Matt Fleming wrote:
> On Thu, 05 Dec, at 07:55:03AM, Joe Perches wrote:
>> On Thu, 2013-12-05 at 11:30 +, Matt Fleming wrote:
>>> On Wed, 04 Dec, at 07:22:57PM, Arnd Bergmann wrote:
The other part I noticed about this particular patchset is that it's
not really "firmware" as such, but specifically PC wiht ACPI that
gets covered here. So rather than generalizing the code, another
option would be to narrow down the scope and make it
acpi_{warn,info,dbg} instead.
>>>
>>> Making this specific to ACPI runs the risk of people introducing a
>>> multitude of new logging functions for every subsystem, e.g.
>>> efi_{warn,info,dbg}.
>>
>> There are many subsystem specific logging functions:
>
> Surely that's further justification to not introduce any more.
That's what I was thinking when I saw this discussion.
>
>>> FWIW, I'd be interested in using something like this patch series to
>>> properly log EFI implementation bugs. The logging for EFI is currently
>>> done fairly haphazardly.
>>
>> I thought that was the point of embedding the existing
>> FW_INFO, FW_WARN and FW_BUG #defines in formats.
>>
>> Using logging message scraping to find faults is not
>> a great approach as message content is subject to change.
>
> I wasn't planning on using them to scrape the kernel logs, just for more
> informative messages.
Exactly. That's the whole point here -- the only mechanism that exists for
tracking firwmare related issues, like it or not, is the kernel log/dmesg/boot
log/whatever we're calling it these days. It's been done this way since the
beginning of time.
The problem I'm trying to solve, and as Andrew commented on, is a *real*
problem. The information we currently dump out is not useful to anyone.
Could this be expanded to other subsystems? Yes, without question. It's
actually the ACPI and PCI subsystems that I want to target next, however, both
of those will require a base change to FW_{INFO|WARN|BUG} to at least get us a
starting point.
P.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] Introduce FW_INFO* functions and messages
Sorry everyone, I was out on PTO for the past few weeks.
On 12/06/2013 07:30 AM, Matt Fleming wrote:
On Thu, 05 Dec, at 07:55:03AM, Joe Perches wrote:
On Thu, 2013-12-05 at 11:30 +, Matt Fleming wrote:
On Wed, 04 Dec, at 07:22:57PM, Arnd Bergmann wrote:
The other part I noticed about this particular patchset is that it's
not really firmware as such, but specifically PC wiht ACPI that
gets covered here. So rather than generalizing the code, another
option would be to narrow down the scope and make it
acpi_{warn,info,dbg} instead.
Making this specific to ACPI runs the risk of people introducing a
multitude of new logging functions for every subsystem, e.g.
efi_{warn,info,dbg}.
There are many subsystem specific logging functions:
Surely that's further justification to not introduce any more.
That's what I was thinking when I saw this discussion.
FWIW, I'd be interested in using something like this patch series to
properly log EFI implementation bugs. The logging for EFI is currently
done fairly haphazardly.
I thought that was the point of embedding the existing
FW_INFO, FW_WARN and FW_BUG #defines in formats.
Using logging message scraping to find faults is not
a great approach as message content is subject to change.
I wasn't planning on using them to scrape the kernel logs, just for more
informative messages.
Exactly. That's the whole point here -- the only mechanism that exists for
tracking firwmare related issues, like it or not, is the kernel log/dmesg/boot
log/whatever we're calling it these days. It's been done this way since the
beginning of time.
The problem I'm trying to solve, and as Andrew commented on, is a *real*
problem. The information we currently dump out is not useful to anyone.
Could this be expanded to other subsystems? Yes, without question. It's
actually the ACPI and PCI subsystems that I want to target next, however, both
of those will require a base change to FW_{INFO|WARN|BUG} to at least get us a
starting point.
P.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] Introduce FW_INFO* functions and messages
On Mon, 2013-12-16 at 08:01 -0500, Prarit Bhargava wrote:
Sorry everyone, I was out on PTO for the past few weeks.
On 12/06/2013 07:30 AM, Matt Fleming wrote:
On Thu, 05 Dec, at 07:55:03AM, Joe Perches wrote:
On Thu, 2013-12-05 at 11:30 +, Matt Fleming wrote:
On Wed, 04 Dec, at 07:22:57PM, Arnd Bergmann wrote:
The other part I noticed about this particular patchset is that it's
not really firmware as such, but specifically PC wiht ACPI that
gets covered here. So rather than generalizing the code, another
option would be to narrow down the scope and make it
acpi_{warn,info,dbg} instead.
Making this specific to ACPI runs the risk of people introducing a
multitude of new logging functions for every subsystem, e.g.
efi_{warn,info,dbg}.
There are many subsystem specific logging functions:
Surely that's further justification to not introduce any more.
That's what I was thinking when I saw this discussion.
I have no problem with introducing more subsystem_kern_level
functions/macros.
It can reduce overall object size quite a lot.
see commits like: 227842d1176019512d24236f7fb894f0fadd30d1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] Introduce FW_INFO* functions and messages
On Thu, 05 Dec, at 07:55:03AM, Joe Perches wrote:
> On Thu, 2013-12-05 at 11:30 +, Matt Fleming wrote:
> > On Wed, 04 Dec, at 07:22:57PM, Arnd Bergmann wrote:
> > > The other part I noticed about this particular patchset is that it's
> > > not really "firmware" as such, but specifically PC wiht ACPI that
> > > gets covered here. So rather than generalizing the code, another
> > > option would be to narrow down the scope and make it
> > > acpi_{warn,info,dbg} instead.
> >
> > Making this specific to ACPI runs the risk of people introducing a
> > multitude of new logging functions for every subsystem, e.g.
> > efi_{warn,info,dbg}.
>
> There are many subsystem specific logging functions:
Surely that's further justification to not introduce any more.
> > FWIW, I'd be interested in using something like this patch series to
> > properly log EFI implementation bugs. The logging for EFI is currently
> > done fairly haphazardly.
>
> I thought that was the point of embedding the existing
> FW_INFO, FW_WARN and FW_BUG #defines in formats.
>
> Using logging message scraping to find faults is not
> a great approach as message content is subject to change.
I wasn't planning on using them to scrape the kernel logs, just for more
informative messages.
--
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] Introduce FW_INFO* functions and messages
On Thu, 05 Dec, at 07:55:03AM, Joe Perches wrote:
On Thu, 2013-12-05 at 11:30 +, Matt Fleming wrote:
On Wed, 04 Dec, at 07:22:57PM, Arnd Bergmann wrote:
The other part I noticed about this particular patchset is that it's
not really firmware as such, but specifically PC wiht ACPI that
gets covered here. So rather than generalizing the code, another
option would be to narrow down the scope and make it
acpi_{warn,info,dbg} instead.
Making this specific to ACPI runs the risk of people introducing a
multitude of new logging functions for every subsystem, e.g.
efi_{warn,info,dbg}.
There are many subsystem specific logging functions:
Surely that's further justification to not introduce any more.
FWIW, I'd be interested in using something like this patch series to
properly log EFI implementation bugs. The logging for EFI is currently
done fairly haphazardly.
I thought that was the point of embedding the existing
FW_INFO, FW_WARN and FW_BUG #defines in formats.
Using logging message scraping to find faults is not
a great approach as message content is subject to change.
I wasn't planning on using them to scrape the kernel logs, just for more
informative messages.
--
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] Introduce FW_INFO* functions and messages
On Thu, 2013-12-05 at 11:30 +, Matt Fleming wrote:
> On Wed, 04 Dec, at 07:22:57PM, Arnd Bergmann wrote:
> > The other part I noticed about this particular patchset is that it's
> > not really "firmware" as such, but specifically PC wiht ACPI that
> > gets covered here. So rather than generalizing the code, another
> > option would be to narrow down the scope and make it
> > acpi_{warn,info,dbg} instead.
>
> Making this specific to ACPI runs the risk of people introducing a
> multitude of new logging functions for every subsystem, e.g.
> efi_{warn,info,dbg}.
There are many subsystem specific logging functions:
$ grep -rP --include=*.[ch] -oh "\b[a-z_]+_warn\b\s*\(" *| \
sed -r 's/\s*//g'|sort|uniq -c|sort -rn|head -25
3808 dev_warn(
1964 pr_warn(
468 netdev_warn(
194 xfs_warn(
120 xhci_warn(
102 ipoib_warn(
76 netif_warn(
64 tuner_warn(
53 hid_warn(
51 ata_dev_warn(
46 mthca_warn(
45 nv_warn(
41 ubi_warn(
32 wiphy_warn(
32 osm_warn(
31 rbd_warn(
28 ubifs_warn(
27 psmouse_warn(
27 e_warn(
25 blogic_warn(
23 en_warn(
23 ata_link_warn(
22 jfs_warn(
21 csio_warn(
21 cam_warn(
> FWIW, I'd be interested in using something like this patch series to
> properly log EFI implementation bugs. The logging for EFI is currently
> done fairly haphazardly.
I thought that was the point of embedding the existing
FW_INFO, FW_WARN and FW_BUG #defines in formats.
Using logging message scraping to find faults is not
a great approach as message content is subject to change.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] Introduce FW_INFO* functions and messages
On Wed, 04 Dec, at 07:22:57PM, Arnd Bergmann wrote:
> The other part I noticed about this particular patchset is that it's
> not really "firmware" as such, but specifically PC wiht ACPI that
> gets covered here. So rather than generalizing the code, another
> option would be to narrow down the scope and make it
> acpi_{warn,info,dbg} instead.
Making this specific to ACPI runs the risk of people introducing a
multitude of new logging functions for every subsystem, e.g.
efi_{warn,info,dbg}.
FWIW, I'd be interested in using something like this patch series to
properly log EFI implementation bugs. The logging for EFI is currently
done fairly haphazardly.
--
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] Introduce FW_INFO* functions and messages
On Wed, 04 Dec, at 07:22:57PM, Arnd Bergmann wrote:
The other part I noticed about this particular patchset is that it's
not really firmware as such, but specifically PC wiht ACPI that
gets covered here. So rather than generalizing the code, another
option would be to narrow down the scope and make it
acpi_{warn,info,dbg} instead.
Making this specific to ACPI runs the risk of people introducing a
multitude of new logging functions for every subsystem, e.g.
efi_{warn,info,dbg}.
FWIW, I'd be interested in using something like this patch series to
properly log EFI implementation bugs. The logging for EFI is currently
done fairly haphazardly.
--
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] Introduce FW_INFO* functions and messages
On Thu, 2013-12-05 at 11:30 +, Matt Fleming wrote:
On Wed, 04 Dec, at 07:22:57PM, Arnd Bergmann wrote:
The other part I noticed about this particular patchset is that it's
not really firmware as such, but specifically PC wiht ACPI that
gets covered here. So rather than generalizing the code, another
option would be to narrow down the scope and make it
acpi_{warn,info,dbg} instead.
Making this specific to ACPI runs the risk of people introducing a
multitude of new logging functions for every subsystem, e.g.
efi_{warn,info,dbg}.
There are many subsystem specific logging functions:
$ grep -rP --include=*.[ch] -oh \b[a-z_]+_warn\b\s*\( *| \
sed -r 's/\s*//g'|sort|uniq -c|sort -rn|head -25
3808 dev_warn(
1964 pr_warn(
468 netdev_warn(
194 xfs_warn(
120 xhci_warn(
102 ipoib_warn(
76 netif_warn(
64 tuner_warn(
53 hid_warn(
51 ata_dev_warn(
46 mthca_warn(
45 nv_warn(
41 ubi_warn(
32 wiphy_warn(
32 osm_warn(
31 rbd_warn(
28 ubifs_warn(
27 psmouse_warn(
27 e_warn(
25 blogic_warn(
23 en_warn(
23 ata_link_warn(
22 jfs_warn(
21 csio_warn(
21 cam_warn(
FWIW, I'd be interested in using something like this patch series to
properly log EFI implementation bugs. The logging for EFI is currently
done fairly haphazardly.
I thought that was the point of embedding the existing
FW_INFO, FW_WARN and FW_BUG #defines in formats.
Using logging message scraping to find faults is not
a great approach as message content is subject to change.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] Introduce FW_INFO* functions and messages
On Tuesday 03 December 2013, Andrew Morton wrote:
> 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 ;)
I had similar thoughts when I read this, but I can also remember a bunch
of very overdesigned attempts to reorganize and structure the kernel
logging code. I lot of time was wasted in the past for things that
ended up being too invasive or inconsistent.
The other part I noticed about this particular patchset is that it's
not really "firmware" as such, but specifically PC wiht ACPI that
gets covered here. So rather than generalizing the code, another
option would be to narrow down the scope and make it
acpi_{warn,info,dbg} instead.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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
> > 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.
>
>
>
> >
> > 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_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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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 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.
>
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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 pra...@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.
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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 pra...@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_types?
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] Introduce FW_INFO* functions and messages
On Tuesday 03 December 2013, Andrew Morton wrote:
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 ;)
I had similar thoughts when I read this, but I can also remember a bunch
of very overdesigned attempts to reorganize and structure the kernel
logging code. I lot of time was wasted in the past for things that
ended up being too invasive or inconsistent.
The other part I noticed about this particular patchset is that it's
not really firmware as such, but specifically PC wiht ACPI that
gets covered here. So rather than generalizing the code, another
option would be to narrow down the scope and make it
acpi_{warn,info,dbg} instead.
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] Introduce FW_INFO* functions and messages
On Mon, 2 Dec 2013 10:19:37 -0500 Prarit Bhargava wrote:
> Logging and tracking firmware bugs in the kernel has long been an issue
> for system administrators. The current kernel does not have a good
> uniform method of reporting firmware bugs and the code in the kernel is a
> mix of printk's and WARN_ONs. This causes problems for both system
> administrators and QA engineers who attempt to diagnose problems within
> the kernel.
>
> Using printk's is somewhat effective but lacks information useful for
> reporting a bug such as the system vendor or model, BIOS revision, etc.
> Using WARN_ONs is also questionable because the data like the backtrace
> and the list of modules is usually unnecessary for firmware issues as the
> warning stems from one call path during system or driver initialization.
> We have heard many complaints from users about the excess verbosity and
> confusing stacktraces for these messages.
>
> I'm proposing with this patch to do something similar to the WARN()
> mechanism that is currently implemented in the kernel. This
> patchset introduces FW_INFO() and FW_INFO_DEV() which logs output like:
>
> [ 230.661137] [Firmware Info]: pci_bus :00: at
> /home/prarit_modules/prarit.c:21 Your BIOS is broken because it is
> -ENOWORKY.
> [ 230.671076] [Firmware Info]: Intel Corporation SandyBridge Platform/To
> be filled by O.E.M., BIOS RMLCRB.86I.R3.27.D685.1305151733 05/15/2013
>
> instead of the verbose back traces we are currently seeing. These messages
> can be easily gleaned from /var/log/messages, etc., by automatic bug
> reporting tools and system administrators to properly report bugs to
> hardware vendors.
>
> I found an improperly classified FW_INFO in arch/x86/kernel/cpu/amd.c
> which that should be a FW_BUG.
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!
> + add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
> + break;
> + default:
> + strcpy(fw_str, "[Firmware Bug]");
> + break;
> + }
> +
> + if (dev)
> + pr_info("%s: %s %s ", fw_str,
> + dev_driver_string(dev), dev_name(dev));
> + pr_info("%s: at %s:%d\n", fw_str, file, line);
> +
> + args.fmt = fmt;
> + va_start(args.args, fmt);
> + printk(KERN_WARNING "%s: %pV", fw_str, );
> + va_end(args.args);
> +
> + if (dump_hardware_arch_desc())
> + pr_info("%s: System Info: %s\n", fw_str,
> + dump_hardware_arch_desc());
> + else
> + pr_info("%s: System Info: Hardware Unidentified\n", fw_str);
pr_info(""%s: system info: %s\n", fw_str, dump_hardware_arch_desc());
> +}
> +EXPORT_SYMBOL(warn_slowpath_fmt_dev);
> +
> +
> +char *dump_hardware_arch_desc(void)
> +{
> + if (dump_stack_arch_desc_str[0] != '\0')
> + return dump_stack_arch_desc_str;
> + return NULL;
return "unavaliable";
> +}
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.
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 ;)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] Introduce FW_INFO* functions and messages
On Mon, 2 Dec 2013 10:19:37 -0500 Prarit Bhargava pra...@redhat.com wrote:
Logging and tracking firmware bugs in the kernel has long been an issue
for system administrators. The current kernel does not have a good
uniform method of reporting firmware bugs and the code in the kernel is a
mix of printk's and WARN_ONs. This causes problems for both system
administrators and QA engineers who attempt to diagnose problems within
the kernel.
Using printk's is somewhat effective but lacks information useful for
reporting a bug such as the system vendor or model, BIOS revision, etc.
Using WARN_ONs is also questionable because the data like the backtrace
and the list of modules is usually unnecessary for firmware issues as the
warning stems from one call path during system or driver initialization.
We have heard many complaints from users about the excess verbosity and
confusing stacktraces for these messages.
I'm proposing with this patch to do something similar to the WARN()
mechanism that is currently implemented in the kernel. This
patchset introduces FW_INFO() and FW_INFO_DEV() which logs output like:
[ 230.661137] [Firmware Info]: pci_bus :00: at
/home/prarit_modules/prarit.c:21 Your BIOS is broken because it is
-ENOWORKY.
[ 230.671076] [Firmware Info]: Intel Corporation SandyBridge Platform/To
be filled by O.E.M., BIOS RMLCRB.86I.R3.27.D685.1305151733 05/15/2013
instead of the verbose back traces we are currently seeing. These messages
can be easily gleaned from /var/log/messages, etc., by automatic bug
reporting tools and system administrators to properly report bugs to
hardware vendors.
I found an improperly classified FW_INFO in arch/x86/kernel/cpu/amd.c
which that should be a FW_BUG.
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!
+ add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
+ break;
+ default:
+ strcpy(fw_str, [Firmware Bug]);
+ break;
+ }
+
+ if (dev)
+ pr_info(%s: %s %s , fw_str,
+ dev_driver_string(dev), dev_name(dev));
+ pr_info(%s: at %s:%d\n, fw_str, file, line);
+
+ args.fmt = fmt;
+ va_start(args.args, fmt);
+ printk(KERN_WARNING %s: %pV, fw_str, args);
+ va_end(args.args);
+
+ if (dump_hardware_arch_desc())
+ pr_info(%s: System Info: %s\n, fw_str,
+ dump_hardware_arch_desc());
+ else
+ pr_info(%s: System Info: Hardware Unidentified\n, fw_str);
pr_info(%s: system info: %s\n, fw_str, dump_hardware_arch_desc());
+}
+EXPORT_SYMBOL(warn_slowpath_fmt_dev);
+
+
+char *dump_hardware_arch_desc(void)
+{
+ if (dump_stack_arch_desc_str[0] != '\0')
+ return dump_stack_arch_desc_str;
+ return NULL;
return unavaliable;
+}
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.
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 ;)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/3] Introduce FW_INFO* functions and messages
Logging and tracking firmware bugs in the kernel has long been an issue
for system administrators. The current kernel does not have a good
uniform method of reporting firmware bugs and the code in the kernel is a
mix of printk's and WARN_ONs. This causes problems for both system
administrators and QA engineers who attempt to diagnose problems within
the kernel.
Using printk's is somewhat effective but lacks information useful for
reporting a bug such as the system vendor or model, BIOS revision, etc.
Using WARN_ONs is also questionable because the data like the backtrace
and the list of modules is usually unnecessary for firmware issues as the
warning stems from one call path during system or driver initialization.
We have heard many complaints from users about the excess verbosity and
confusing stacktraces for these messages.
I'm proposing with this patch to do something similar to the WARN()
mechanism that is currently implemented in the kernel. This
patchset introduces FW_INFO() and FW_INFO_DEV() which logs output like:
[ 230.661137] [Firmware Info]: pci_bus :00: at
/home/prarit_modules/prarit.c:21 Your BIOS is broken because it is
-ENOWORKY.
[ 230.671076] [Firmware Info]: Intel Corporation SandyBridge Platform/To
be filled by O.E.M., BIOS RMLCRB.86I.R3.27.D685.1305151733 05/15/2013
instead of the verbose back traces we are currently seeing. These messages
can be easily gleaned from /var/log/messages, etc., by automatic bug
reporting tools and system administrators to properly report bugs to
hardware vendors.
I found an improperly classified FW_INFO in arch/x86/kernel/cpu/amd.c
which that should be a FW_BUG.
Signed-off-by: Prarit Bhargava
Cc: Arnd Bergmann
Cc: Andrew Morton
Cc: Greg Kroah-Hartman
---
arch/x86/pci/mmconfig-shared.c | 15 +++---
include/asm-generic/bug.h | 35 +
include/linux/printk.h | 13 ++---
kernel/panic.c | 42
kernel/printk/printk.c | 12
5 files changed, 98 insertions(+), 19 deletions(-)
diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index 082e881..3cb0eff 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -498,15 +498,9 @@ static int __ref pci_mmcfg_check_reserved(struct device
*dev,
if (is_mmconf_reserved(is_acpi_reserved, cfg, dev, 0))
return 1;
- if (dev)
- dev_info(dev, FW_INFO
-"MMCONFIG at %pR not reserved in "
-"ACPI motherboard resources\n",
+ FW_INFO_DEV(dev, dev, "MMCONFIG at %pR not reserved in
ACPI motherboard resources\n",
>res);
- else
- pr_info(FW_INFO PREFIX
- "MMCONFIG at %pR not reserved in "
- "ACPI motherboard resources\n",
+ FW_INFO(!dev, PREFIX "MMCONFIG at %pR not reserved in
ACPI motherboard resources\n",
>res);
}
@@ -707,10 +701,7 @@ int pci_mmconfig_insert(struct device *dev, u16 seg, u8
start, u8 end,
cfg = pci_mmconfig_lookup(seg, start);
if (cfg) {
if (cfg->end_bus < end)
- dev_info(dev, FW_INFO
-"MMCONFIG for "
-"domain %04x [bus %02x-%02x] "
-"only partially covers this bridge\n",
+ FW_INFO_DEV(1, dev, "MMCONFIG for domain %04x [bus
%02x-%02x] only partially covers this bridge\n",
cfg->segment, cfg->start_bus, cfg->end_bus);
mutex_unlock(_mmcfg_lock);
return -EEXIST;
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 7d10f96..b3cb4ed 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -68,12 +68,17 @@ void warn_slowpath_fmt(const char *file, const int line,
extern __printf(4, 5)
void warn_slowpath_fmt_taint(const char *file, const int line, unsigned taint,
const char *fmt, ...);
+extern __printf(5, 6)
+void warn_slowpath_fmt_dev(const char *file, const int line,
+ struct device *dev, int level, const char *fmt, ...);
extern void warn_slowpath_null(const char *file, const int line);
#define WANT_WARN_ON_SLOWPATH
#define __WARN() warn_slowpath_null(__FILE__, __LINE__)
#define __WARN_printf(arg...) warn_slowpath_fmt(__FILE__, __LINE__, arg)
#define __WARN_printf_taint(taint, arg...) \
warn_slowpath_fmt_taint(__FILE__, __LINE__, taint, arg)
+#define __WARN_printf_dev(dev, level, arg...) \
+ warn_slowpath_fmt_dev(__FILE__, __LINE__, dev, level, arg)
[PATCH 1/3] Introduce FW_INFO* functions and messages
Logging and tracking firmware bugs in the kernel has long been an issue
for system administrators. The current kernel does not have a good
uniform method of reporting firmware bugs and the code in the kernel is a
mix of printk's and WARN_ONs. This causes problems for both system
administrators and QA engineers who attempt to diagnose problems within
the kernel.
Using printk's is somewhat effective but lacks information useful for
reporting a bug such as the system vendor or model, BIOS revision, etc.
Using WARN_ONs is also questionable because the data like the backtrace
and the list of modules is usually unnecessary for firmware issues as the
warning stems from one call path during system or driver initialization.
We have heard many complaints from users about the excess verbosity and
confusing stacktraces for these messages.
I'm proposing with this patch to do something similar to the WARN()
mechanism that is currently implemented in the kernel. This
patchset introduces FW_INFO() and FW_INFO_DEV() which logs output like:
[ 230.661137] [Firmware Info]: pci_bus :00: at
/home/prarit_modules/prarit.c:21 Your BIOS is broken because it is
-ENOWORKY.
[ 230.671076] [Firmware Info]: Intel Corporation SandyBridge Platform/To
be filled by O.E.M., BIOS RMLCRB.86I.R3.27.D685.1305151733 05/15/2013
instead of the verbose back traces we are currently seeing. These messages
can be easily gleaned from /var/log/messages, etc., by automatic bug
reporting tools and system administrators to properly report bugs to
hardware vendors.
I found an improperly classified FW_INFO in arch/x86/kernel/cpu/amd.c
which that should be a FW_BUG.
Signed-off-by: Prarit Bhargava pra...@redhat.com
Cc: Arnd Bergmann a...@arndb.de
Cc: Andrew Morton a...@linux-foundation.org
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
---
arch/x86/pci/mmconfig-shared.c | 15 +++---
include/asm-generic/bug.h | 35 +
include/linux/printk.h | 13 ++---
kernel/panic.c | 42
kernel/printk/printk.c | 12
5 files changed, 98 insertions(+), 19 deletions(-)
diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index 082e881..3cb0eff 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -498,15 +498,9 @@ static int __ref pci_mmcfg_check_reserved(struct device
*dev,
if (is_mmconf_reserved(is_acpi_reserved, cfg, dev, 0))
return 1;
- if (dev)
- dev_info(dev, FW_INFO
-MMCONFIG at %pR not reserved in
-ACPI motherboard resources\n,
+ FW_INFO_DEV(dev, dev, MMCONFIG at %pR not reserved in
ACPI motherboard resources\n,
cfg-res);
- else
- pr_info(FW_INFO PREFIX
- MMCONFIG at %pR not reserved in
- ACPI motherboard resources\n,
+ FW_INFO(!dev, PREFIX MMCONFIG at %pR not reserved in
ACPI motherboard resources\n,
cfg-res);
}
@@ -707,10 +701,7 @@ int pci_mmconfig_insert(struct device *dev, u16 seg, u8
start, u8 end,
cfg = pci_mmconfig_lookup(seg, start);
if (cfg) {
if (cfg-end_bus end)
- dev_info(dev, FW_INFO
-MMCONFIG for
-domain %04x [bus %02x-%02x]
-only partially covers this bridge\n,
+ FW_INFO_DEV(1, dev, MMCONFIG for domain %04x [bus
%02x-%02x] only partially covers this bridge\n,
cfg-segment, cfg-start_bus, cfg-end_bus);
mutex_unlock(pci_mmcfg_lock);
return -EEXIST;
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 7d10f96..b3cb4ed 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -68,12 +68,17 @@ void warn_slowpath_fmt(const char *file, const int line,
extern __printf(4, 5)
void warn_slowpath_fmt_taint(const char *file, const int line, unsigned taint,
const char *fmt, ...);
+extern __printf(5, 6)
+void warn_slowpath_fmt_dev(const char *file, const int line,
+ struct device *dev, int level, const char *fmt, ...);
extern void warn_slowpath_null(const char *file, const int line);
#define WANT_WARN_ON_SLOWPATH
#define __WARN() warn_slowpath_null(__FILE__, __LINE__)
#define __WARN_printf(arg...) warn_slowpath_fmt(__FILE__, __LINE__, arg)
#define __WARN_printf_taint(taint, arg...) \
warn_slowpath_fmt_taint(__FILE__, __LINE__, taint, arg)
+#define __WARN_printf_dev(dev, level, arg...) \
+
