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...) \ +