* [PATCH] x86/PCI: Add missing log facility and move to use pr_ macros in pcbios.c @ 2019-08-25 18:25 Krzysztof Wilczynski 2019-08-27 22:47 ` Bjorn Helgaas 2019-08-28 17:51 ` [PATCH v2] " Krzysztof Wilczynski 0 siblings, 2 replies; 9+ messages in thread From: Krzysztof Wilczynski @ 2019-08-25 18:25 UTC (permalink / raw) To: Bjorn Helgaas Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, linux-pci, linux-kernel Make the log facility used to print warnings to be KERN_WARNING explicitly, rather than rely on the current (or default) value of the MESSAGE_LOGLEVEL_DEFAULT set in Kconfig. This will make all the warnings in the arch/x86/pci/pcbios.c to be printed consistently at the same log facility. Replace printk(KERN_<level> ...) with corresponding pr_ macros, while adding the missing log facility. Signed-off-by: Krzysztof Wilczynski <kw@linux.com> --- arch/x86/pci/pcbios.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/arch/x86/pci/pcbios.c b/arch/x86/pci/pcbios.c index 9c97d814125e..0c3673f50bce 100644 --- a/arch/x86/pci/pcbios.c +++ b/arch/x86/pci/pcbios.c @@ -47,7 +47,7 @@ static inline void set_bios_x(void) pcibios_enabled = 1; set_memory_x(PAGE_OFFSET + BIOS_BEGIN, (BIOS_END - BIOS_BEGIN) >> PAGE_SHIFT); if (__supported_pte_mask & _PAGE_NX) - printk(KERN_INFO "PCI: PCI BIOS area is rw and x. Use pci=nobios if you want it NX.\n"); + pr_info("PCI: PCI BIOS area is rw and x. Use pci=nobios if you want it NX.\n"); } /* @@ -111,10 +111,10 @@ static unsigned long __init bios32_service(unsigned long service) case 0: return address + entry; case 0x80: /* Not present */ - printk(KERN_WARNING "bios32_service(0x%lx): not present\n", service); + pr_warn("bios32_service(0x%lx): not present\n", service); return 0; default: /* Shouldn't happen */ - printk(KERN_WARNING "bios32_service(0x%lx): returned 0x%x -- BIOS bug!\n", + pr_warn("bios32_service(0x%lx): returned 0x%x -- BIOS bug!\n", service, return_code); return 0; } @@ -163,11 +163,11 @@ static int __init check_pcibios(void) DBG("PCI: BIOS probe returned s=%02x hw=%02x ver=%02x.%02x l=%02x\n", status, hw_mech, major_ver, minor_ver, pcibios_last_bus); if (status || signature != PCI_SIGNATURE) { - printk (KERN_ERR "PCI: BIOS BUG #%x[%08x] found\n", + pr_err("PCI: BIOS BUG #%x[%08x] found\n", status, signature); return 0; } - printk(KERN_INFO "PCI: PCI BIOS revision %x.%02x entry at 0x%lx, last bus=%d\n", + pr_info("PCI: PCI BIOS revision %x.%02x entry at 0x%lx, last bus=%d\n", major_ver, minor_ver, pcibios_entry, pcibios_last_bus); #ifdef CONFIG_PCI_DIRECT if (!(hw_mech & PCIBIOS_HW_TYPE1)) @@ -316,13 +316,13 @@ static const struct pci_raw_ops *__init pci_find_bios(void) if (sum != 0) continue; if (check->fields.revision != 0) { - printk("PCI: unsupported BIOS32 revision %d at 0x%p\n", + pr_warn("PCI: unsupported BIOS32 revision %d at 0x%p\n", check->fields.revision, check); continue; } DBG("PCI: BIOS32 Service Directory structure at 0x%p\n", check); if (check->fields.entry >= 0x100000) { - printk("PCI: BIOS32 entry (0x%p) in high memory, " + pr_warn("PCI: BIOS32 entry (0x%p) in high memory, " "cannot use.\n", check); return NULL; } else { @@ -386,7 +386,7 @@ struct irq_routing_table * pcibios_get_irq_routing_table(void) : "memory"); DBG("OK ret=%d, size=%d, map=%x\n", ret, opt.size, map); if (ret & 0xff00) - printk(KERN_ERR "PCI: Error %02x when fetching IRQ routing table.\n", (ret >> 8) & 0xff); + pr_err("PCI: Error %02x when fetching IRQ routing table.\n", (ret >> 8) & 0xff); else if (opt.size) { rt = kmalloc(sizeof(struct irq_routing_table) + opt.size, GFP_KERNEL); if (rt) { @@ -394,7 +394,7 @@ struct irq_routing_table * pcibios_get_irq_routing_table(void) rt->size = opt.size + sizeof(struct irq_routing_table); rt->exclusive_irqs = map; memcpy(rt->slots, (void *) page, opt.size); - printk(KERN_INFO "PCI: Using BIOS Interrupt Routing Table\n"); + pr_info("PCI: Using BIOS Interrupt Routing Table\n"); } } free_page(page); -- 2.22.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] x86/PCI: Add missing log facility and move to use pr_ macros in pcbios.c 2019-08-25 18:25 [PATCH] x86/PCI: Add missing log facility and move to use pr_ macros in pcbios.c Krzysztof Wilczynski @ 2019-08-27 22:47 ` Bjorn Helgaas 2019-08-28 10:59 ` Krzysztof Wilczynski 2019-08-28 17:51 ` [PATCH v2] " Krzysztof Wilczynski 1 sibling, 1 reply; 9+ messages in thread From: Bjorn Helgaas @ 2019-08-27 22:47 UTC (permalink / raw) To: Krzysztof Wilczynski Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, linux-pci, linux-kernel On Sun, Aug 25, 2019 at 08:25:57PM +0200, Krzysztof Wilczynski wrote: > Make the log facility used to print warnings to be KERN_WARNING > explicitly, rather than rely on the current (or default) value > of the MESSAGE_LOGLEVEL_DEFAULT set in Kconfig. This will make > all the warnings in the arch/x86/pci/pcbios.c to be printed > consistently at the same log facility. This is slightly confusing. There are only two messages that didn't supply a log level, so the avoidance of MESSAGE_LOGLEVEL_DEFAULT applies to those. The rest already supplied a log level, so converting printk(KERN_INFO) to pr_info() is purely simplification. > Replace printk(KERN_<level> ...) with corresponding pr_ macros, > while adding the missing log facility. Might be worth doing this as well: #define pr_fmt(fmt) "PCI: " fmt and removing the "PCI: " prefix from the messages. This would change the "bios32_service" output slightly, but I think the change would be a good one. > Signed-off-by: Krzysztof Wilczynski <kw@linux.com> > --- > arch/x86/pci/pcbios.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/pci/pcbios.c b/arch/x86/pci/pcbios.c > index 9c97d814125e..0c3673f50bce 100644 > --- a/arch/x86/pci/pcbios.c > +++ b/arch/x86/pci/pcbios.c > @@ -47,7 +47,7 @@ static inline void set_bios_x(void) > pcibios_enabled = 1; > set_memory_x(PAGE_OFFSET + BIOS_BEGIN, (BIOS_END - BIOS_BEGIN) >> PAGE_SHIFT); > if (__supported_pte_mask & _PAGE_NX) > - printk(KERN_INFO "PCI: PCI BIOS area is rw and x. Use pci=nobios if you want it NX.\n"); > + pr_info("PCI: PCI BIOS area is rw and x. Use pci=nobios if you want it NX.\n"); > } > > /* > @@ -111,10 +111,10 @@ static unsigned long __init bios32_service(unsigned long service) > case 0: > return address + entry; > case 0x80: /* Not present */ > - printk(KERN_WARNING "bios32_service(0x%lx): not present\n", service); > + pr_warn("bios32_service(0x%lx): not present\n", service); > return 0; > default: /* Shouldn't happen */ > - printk(KERN_WARNING "bios32_service(0x%lx): returned 0x%x -- BIOS bug!\n", > + pr_warn("bios32_service(0x%lx): returned 0x%x -- BIOS bug!\n", > service, return_code); > return 0; > } > @@ -163,11 +163,11 @@ static int __init check_pcibios(void) > DBG("PCI: BIOS probe returned s=%02x hw=%02x ver=%02x.%02x l=%02x\n", > status, hw_mech, major_ver, minor_ver, pcibios_last_bus); > if (status || signature != PCI_SIGNATURE) { > - printk (KERN_ERR "PCI: BIOS BUG #%x[%08x] found\n", > + pr_err("PCI: BIOS BUG #%x[%08x] found\n", > status, signature); > return 0; > } > - printk(KERN_INFO "PCI: PCI BIOS revision %x.%02x entry at 0x%lx, last bus=%d\n", > + pr_info("PCI: PCI BIOS revision %x.%02x entry at 0x%lx, last bus=%d\n", > major_ver, minor_ver, pcibios_entry, pcibios_last_bus); > #ifdef CONFIG_PCI_DIRECT > if (!(hw_mech & PCIBIOS_HW_TYPE1)) > @@ -316,13 +316,13 @@ static const struct pci_raw_ops *__init pci_find_bios(void) > if (sum != 0) > continue; > if (check->fields.revision != 0) { > - printk("PCI: unsupported BIOS32 revision %d at 0x%p\n", > + pr_warn("PCI: unsupported BIOS32 revision %d at 0x%p\n", > check->fields.revision, check); > continue; > } > DBG("PCI: BIOS32 Service Directory structure at 0x%p\n", check); > if (check->fields.entry >= 0x100000) { > - printk("PCI: BIOS32 entry (0x%p) in high memory, " > + pr_warn("PCI: BIOS32 entry (0x%p) in high memory, " > "cannot use.\n", check); > return NULL; > } else { > @@ -386,7 +386,7 @@ struct irq_routing_table * pcibios_get_irq_routing_table(void) > : "memory"); > DBG("OK ret=%d, size=%d, map=%x\n", ret, opt.size, map); > if (ret & 0xff00) > - printk(KERN_ERR "PCI: Error %02x when fetching IRQ routing table.\n", (ret >> 8) & 0xff); > + pr_err("PCI: Error %02x when fetching IRQ routing table.\n", (ret >> 8) & 0xff); > else if (opt.size) { > rt = kmalloc(sizeof(struct irq_routing_table) + opt.size, GFP_KERNEL); > if (rt) { > @@ -394,7 +394,7 @@ struct irq_routing_table * pcibios_get_irq_routing_table(void) > rt->size = opt.size + sizeof(struct irq_routing_table); > rt->exclusive_irqs = map; > memcpy(rt->slots, (void *) page, opt.size); > - printk(KERN_INFO "PCI: Using BIOS Interrupt Routing Table\n"); > + pr_info("PCI: Using BIOS Interrupt Routing Table\n"); > } > } > free_page(page); > -- > 2.22.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] x86/PCI: Add missing log facility and move to use pr_ macros in pcbios.c 2019-08-27 22:47 ` Bjorn Helgaas @ 2019-08-28 10:59 ` Krzysztof Wilczynski 0 siblings, 0 replies; 9+ messages in thread From: Krzysztof Wilczynski @ 2019-08-28 10:59 UTC (permalink / raw) To: Bjorn Helgaas Cc: Krzysztof Wilczynski, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, linux-pci, linux-kernel Hello Bjorn, Thank you for the feedback. [...] >> Make the log facility used to print warnings to be KERN_WARNING >> explicitly, rather than rely on the current (or default) value >> of the MESSAGE_LOGLEVEL_DEFAULT set in Kconfig. This will make >> all the warnings in the arch/x86/pci/pcbios.c to be printed >> consistently at the same log facility. > > This is slightly confusing. There are only two messages that didn't > supply a log level, so the avoidance of MESSAGE_LOGLEVEL_DEFAULT > applies to those. Good point. I will update both the wording and the explanation so that it would be more accurate and make a whole lot more sense. [...] > Might be worth doing this as well: > > #define pr_fmt(fmt) "PCI: " fmt > > and removing the "PCI: " prefix from the messages. This would change > the "bios32_service" output slightly, but I think the change would be > a good one. Will do. The v2 have all the improvements. Thank you! Krzysztof ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] x86/PCI: Add missing log facility and move to use pr_ macros in pcbios.c 2019-08-25 18:25 [PATCH] x86/PCI: Add missing log facility and move to use pr_ macros in pcbios.c Krzysztof Wilczynski 2019-08-27 22:47 ` Bjorn Helgaas @ 2019-08-28 17:51 ` Krzysztof Wilczynski 2019-08-28 18:02 ` Joe Perches 2019-08-29 17:11 ` [PATCH v3] x86/PCI: Add missing log facility and move to use pr_ macros Krzysztof Wilczynski 1 sibling, 2 replies; 9+ messages in thread From: Krzysztof Wilczynski @ 2019-08-28 17:51 UTC (permalink / raw) To: Bjorn Helgaas Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, linux-pci, linux-kernel Add missing log facility where two instances of printk() that did not use any (so it would be using MESSAGE_LOGLEVEL_DEFAULT set in Kconfig) to make all the warnings in the arch/x86/pci/pcbios.c to be printed consistently at the same log facility. Also resolve the following checkpatch.pl script warning: WARNING: printk() should include KERN_<LEVEL> facility level While adding the missing log facility move over to using pr_ macros over using printk(KERN_<level> ...) and DBG(). Signed-off-by: Krzysztof Wilczynski <kw@linux.com> --- Changes in v2: Change wording and include checkpatch.pl script warning. Leverage pr_fmt and remove "PCI: " prefix used throught. Move to pr_debug() over using DBG() from arch/x86/include/asm/pci_x86.h. arch/x86/pci/pcbios.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/arch/x86/pci/pcbios.c b/arch/x86/pci/pcbios.c index 9c97d814125e..ddcefb25c55e 100644 --- a/arch/x86/pci/pcbios.c +++ b/arch/x86/pci/pcbios.c @@ -3,6 +3,8 @@ * BIOS32 and PCI BIOS handling. */ +#define pr_fmt(fmt) "PCI: " fmt + #include <linux/pci.h> #include <linux/init.h> #include <linux/slab.h> @@ -47,7 +49,7 @@ static inline void set_bios_x(void) pcibios_enabled = 1; set_memory_x(PAGE_OFFSET + BIOS_BEGIN, (BIOS_END - BIOS_BEGIN) >> PAGE_SHIFT); if (__supported_pte_mask & _PAGE_NX) - printk(KERN_INFO "PCI: PCI BIOS area is rw and x. Use pci=nobios if you want it NX.\n"); + pr_info("PCI BIOS area is rw and x. Use pci=nobios if you want it NX.\n"); } /* @@ -111,10 +113,10 @@ static unsigned long __init bios32_service(unsigned long service) case 0: return address + entry; case 0x80: /* Not present */ - printk(KERN_WARNING "bios32_service(0x%lx): not present\n", service); + pr_warn("bios32_service(0x%lx): not present\n", service); return 0; default: /* Shouldn't happen */ - printk(KERN_WARNING "bios32_service(0x%lx): returned 0x%x -- BIOS bug!\n", + pr_warn("bios32_service(0x%lx): returned 0x%x -- BIOS bug!\n", service, return_code); return 0; } @@ -160,14 +162,14 @@ static int __init check_pcibios(void) minor_ver = ebx & 0xff; if (pcibios_last_bus < 0) pcibios_last_bus = ecx & 0xff; - DBG("PCI: BIOS probe returned s=%02x hw=%02x ver=%02x.%02x l=%02x\n", + pr_debug("BIOS probe returned s=%02x hw=%02x ver=%02x.%02x l=%02x\n", status, hw_mech, major_ver, minor_ver, pcibios_last_bus); if (status || signature != PCI_SIGNATURE) { - printk (KERN_ERR "PCI: BIOS BUG #%x[%08x] found\n", + pr_err("BIOS BUG #%x[%08x] found\n", status, signature); return 0; } - printk(KERN_INFO "PCI: PCI BIOS revision %x.%02x entry at 0x%lx, last bus=%d\n", + pr_info("PCI BIOS revision %x.%02x entry at 0x%lx, last bus=%d\n", major_ver, minor_ver, pcibios_entry, pcibios_last_bus); #ifdef CONFIG_PCI_DIRECT if (!(hw_mech & PCIBIOS_HW_TYPE1)) @@ -316,18 +318,18 @@ static const struct pci_raw_ops *__init pci_find_bios(void) if (sum != 0) continue; if (check->fields.revision != 0) { - printk("PCI: unsupported BIOS32 revision %d at 0x%p\n", + pr_warn("unsupported BIOS32 revision %d at 0x%p\n", check->fields.revision, check); continue; } - DBG("PCI: BIOS32 Service Directory structure at 0x%p\n", check); + pr_debug("BIOS32 Service Directory structure at 0x%p\n", check); if (check->fields.entry >= 0x100000) { - printk("PCI: BIOS32 entry (0x%p) in high memory, " + pr_warn("BIOS32 entry (0x%p) in high memory, " "cannot use.\n", check); return NULL; } else { unsigned long bios32_entry = check->fields.entry; - DBG("PCI: BIOS32 Service Directory entry at 0x%lx\n", + pr_debug("BIOS32 Service Directory entry at 0x%lx\n", bios32_entry); bios32_indirect.address = bios32_entry + PAGE_OFFSET; set_bios_x(); @@ -366,7 +368,7 @@ struct irq_routing_table * pcibios_get_irq_routing_table(void) opt.size = PAGE_SIZE; opt.segment = __KERNEL_DS; - DBG("PCI: Fetching IRQ routing table... "); + pr_debug("Fetching IRQ routing table... "); __asm__("push %%es\n\t" "push %%ds\n\t" "pop %%es\n\t" @@ -384,9 +386,9 @@ struct irq_routing_table * pcibios_get_irq_routing_table(void) "S" (&pci_indirect), "m" (opt) : "memory"); - DBG("OK ret=%d, size=%d, map=%x\n", ret, opt.size, map); + pr_debug("OK ret=%d, size=%d, map=%x\n", ret, opt.size, map); if (ret & 0xff00) - printk(KERN_ERR "PCI: Error %02x when fetching IRQ routing table.\n", (ret >> 8) & 0xff); + pr_err("Error %02x when fetching IRQ routing table.\n", (ret >> 8) & 0xff); else if (opt.size) { rt = kmalloc(sizeof(struct irq_routing_table) + opt.size, GFP_KERNEL); if (rt) { @@ -394,7 +396,7 @@ struct irq_routing_table * pcibios_get_irq_routing_table(void) rt->size = opt.size + sizeof(struct irq_routing_table); rt->exclusive_irqs = map; memcpy(rt->slots, (void *) page, opt.size); - printk(KERN_INFO "PCI: Using BIOS Interrupt Routing Table\n"); + pr_info("Using BIOS Interrupt Routing Table\n"); } } free_page(page); -- 2.22.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] x86/PCI: Add missing log facility and move to use pr_ macros in pcbios.c 2019-08-28 17:51 ` [PATCH v2] " Krzysztof Wilczynski @ 2019-08-28 18:02 ` Joe Perches 2019-08-28 18:40 ` Krzysztof Wilczynski 2019-08-29 17:11 ` [PATCH v3] x86/PCI: Add missing log facility and move to use pr_ macros Krzysztof Wilczynski 1 sibling, 1 reply; 9+ messages in thread From: Joe Perches @ 2019-08-28 18:02 UTC (permalink / raw) To: Krzysztof Wilczynski, Bjorn Helgaas Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, linux-pci, linux-kernel On Wed, 2019-08-28 at 19:51 +0200, Krzysztof Wilczynski wrote: > Add missing log facility where two instances of printk() that did not > use any (so it would be using MESSAGE_LOGLEVEL_DEFAULT set in Kconfig) > to make all the warnings in the arch/x86/pci/pcbios.c to be printed > consistently at the same log facility. Also resolve the following > checkpatch.pl script warning: > > WARNING: printk() should include KERN_<LEVEL> facility level > > While adding the missing log facility move over to using pr_ macros > over using printk(KERN_<level> ...) and DBG(). > > Signed-off-by: Krzysztof Wilczynski <kw@linux.com> > --- > Changes in v2: > Change wording and include checkpatch.pl script warning. > Leverage pr_fmt and remove "PCI: " prefix used throught. > Move to pr_debug() over using DBG() from arch/x86/include/asm/pci_x86.h. You might also consider the checkpatch output for this patch. arch/x86/pci/pcbios.c:116: WARNING: line over 80 characters arch/x86/pci/pcbios.c:116: WARNING: Prefer using '"%s...", __func__' to using 'bios32_service', this function's name, in a string arch/x86/pci/pcbios.c:119: WARNING: Prefer using '"%s...", __func__' to using 'bios32_service', this function's name, in a string arch/x86/pci/pcbios.c:391: WARNING: line over 80 characters ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] x86/PCI: Add missing log facility and move to use pr_ macros in pcbios.c 2019-08-28 18:02 ` Joe Perches @ 2019-08-28 18:40 ` Krzysztof Wilczynski 2019-08-28 18:43 ` Joe Perches 0 siblings, 1 reply; 9+ messages in thread From: Krzysztof Wilczynski @ 2019-08-28 18:40 UTC (permalink / raw) To: Joe Perches Cc: Krzysztof Wilczynski, Bjorn Helgaas, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, linux-pci, linux-kernel Hello Joe, Thank you for feedback. [...] >> Move to pr_debug() over using DBG() from >> arch/x86/include/asm/pci_x86.h. > > You might also consider the checkpatch output for this patch. > > arch/x86/pci/pcbios.c:116: WARNING: line over 80 characters > arch/x86/pci/pcbios.c:116: WARNING: Prefer using '"%s...", __func__' > to using 'bios32_service', this function's name, in a string > arch/x86/pci/pcbios.c:119: WARNING: Prefer using '"%s...", __func__' > to using 'bios32_service', this function's name, in a string > arch/x86/pci/pcbios.c:391: WARNING: line over 80 characters Good point. The lines over 80 characters wide would be taken care of when moving to using the pr_ macros as the line length will now be shorter contrary to when the e.g., printk(KERNEL_INFO ...), etc., was used. The other warnings I am going to address in v3. I was thinking of replacing the following: pr_warn("bios32_service(0x%lx): not present\n", service); With something that looks like this: pr_warn("BIOS32 Service(0x%lx): not present\n", service); Using "bios32_service" name directly or even moving to __func__ feels a lot like an implementation detail is exposed to the end user. I am not sure how useful that could be. Also, we are already using log lines starting with "BIOS32", thus it seemed like following them would be the most sensible choice, especially to keep messages consistent. What do you think? Krzysztof ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] x86/PCI: Add missing log facility and move to use pr_ macros in pcbios.c 2019-08-28 18:40 ` Krzysztof Wilczynski @ 2019-08-28 18:43 ` Joe Perches 2019-08-28 18:58 ` Krzysztof Wilczynski 0 siblings, 1 reply; 9+ messages in thread From: Joe Perches @ 2019-08-28 18:43 UTC (permalink / raw) To: Krzysztof Wilczynski Cc: Krzysztof Wilczynski, Bjorn Helgaas, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, linux-pci, linux-kernel On Wed, 2019-08-28 at 20:40 +0200, Krzysztof Wilczynski wrote: > Hello Joe, > > Thank you for feedback. > [...] > > > Move to pr_debug() over using DBG() from > > > arch/x86/include/asm/pci_x86.h. > > > > You might also consider the checkpatch output for this patch. > > > > arch/x86/pci/pcbios.c:116: WARNING: line over 80 characters > > arch/x86/pci/pcbios.c:116: WARNING: Prefer using '"%s...", __func__' > > to using 'bios32_service', this function's name, in a string > > arch/x86/pci/pcbios.c:119: WARNING: Prefer using '"%s...", __func__' > > to using 'bios32_service', this function's name, in a string > > arch/x86/pci/pcbios.c:391: WARNING: line over 80 characters > > Good point. > > The lines over 80 characters wide would be taken care of when > moving to using the pr_ macros as the line length will now be > shorter contrary to when the e.g., printk(KERNEL_INFO ...), > etc., was used. Not really, those were the warnings checkpatch emits on your actual patch. > The other warnings I am going to address in v3. I was thinking > of replacing the following: > > pr_warn("bios32_service(0x%lx): not present\n", service); > > With something that looks like this: > > pr_warn("BIOS32 Service(0x%lx): not present\n", service); > > Using "bios32_service" name directly or even moving to __func__ > feels a lot like an implementation detail is exposed to the > end user. I am not sure how useful that could be. Also, > we are already using log lines starting with "BIOS32", thus > it seemed like following them would be the most sensible > choice, especially to keep messages consistent. > > What do you think? Fine with me, your patch, your choices. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] x86/PCI: Add missing log facility and move to use pr_ macros in pcbios.c 2019-08-28 18:43 ` Joe Perches @ 2019-08-28 18:58 ` Krzysztof Wilczynski 0 siblings, 0 replies; 9+ messages in thread From: Krzysztof Wilczynski @ 2019-08-28 18:58 UTC (permalink / raw) To: Joe Perches Cc: Krzysztof Wilczynski, Bjorn Helgaas, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, linux-pci, linux-kernel Hello Joe, [...] >> The lines over 80 characters wide would be taken care of when >> moving to using the pr_ macros as the line length will now be >> shorter contrary to when the e.g., printk(KERNEL_INFO ...), >> etc., was used. > > Not really, those were the warnings checkpatch > emits on your actual patch. Ah! Yes, very true. Sorry about that. I will address these in v3, of course. Thank you for correcting me! Krzysztof ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] x86/PCI: Add missing log facility and move to use pr_ macros 2019-08-28 17:51 ` [PATCH v2] " Krzysztof Wilczynski 2019-08-28 18:02 ` Joe Perches @ 2019-08-29 17:11 ` Krzysztof Wilczynski 1 sibling, 0 replies; 9+ messages in thread From: Krzysztof Wilczynski @ 2019-08-29 17:11 UTC (permalink / raw) To: Bjorn Helgaas Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, linux-pci, linux-kernel Add missing log facility where two instances of printk() that did not use any (so it would be using MESSAGE_LOGLEVEL_DEFAULT set in Kconfig) to make all the warnings in the arch/x86/pci/pcbios.c to be printed consistently at the same log facility. This resolves the following checkpatch.pl script warning: WARNING: printk() should include KERN_<LEVEL> facility level While adding the missing log facility move over to using pr_ macros over using printk(KERN_<level> ...) and DBG(). Also resolve the additional errors and warnings reported by the checkpatch.pl script: ERROR: trailing whitespace ERROR: "foo * bar" should be "foo *bar" ERROR: switch and case should be at the same indent WARNING: please, no space before tabs WARNING: line over 80 characters WARNING: quoted string split across lines WARNING: __packed is preferred over __attribute__((packed)) WARNING: Prefer using '"%s...", __func__' to using 'bios32_service', this function's name, in a string Signed-off-by: Krzysztof Wilczynski <kw@linux.com> --- Changes in v3: Remove name of the file from the subject. Address review feedback of v2, and resolve more checkpatch.pl script errors and warnings. Changes in v2: Change wording and include checkpatch.pl script warning. Leverage pr_fmt and remove "PCI: " prefix used throught. Move to pr_debug() over using DBG() from arch/x86/include/asm/pci_x86.h. arch/x86/pci/pcbios.c | 77 +++++++++++++++++++++++-------------------- 1 file changed, 41 insertions(+), 36 deletions(-) diff --git a/arch/x86/pci/pcbios.c b/arch/x86/pci/pcbios.c index 9c97d814125e..dd8ca5636953 100644 --- a/arch/x86/pci/pcbios.c +++ b/arch/x86/pci/pcbios.c @@ -3,6 +3,8 @@ * BIOS32 and PCI BIOS handling. */ +#define pr_fmt(fmt) "PCI: " fmt + #include <linux/pci.h> #include <linux/init.h> #include <linux/slab.h> @@ -47,15 +49,15 @@ static inline void set_bios_x(void) pcibios_enabled = 1; set_memory_x(PAGE_OFFSET + BIOS_BEGIN, (BIOS_END - BIOS_BEGIN) >> PAGE_SHIFT); if (__supported_pte_mask & _PAGE_NX) - printk(KERN_INFO "PCI: PCI BIOS area is rw and x. Use pci=nobios if you want it NX.\n"); + pr_info("PCI BIOS area is rw and x. Use pci=nobios if you want it NX.\n"); } /* * This is the standard structure used to identify the entry point * to the BIOS32 Service Directory, as documented in - * Standard BIOS 32-bit Service Directory Proposal - * Revision 0.4 May 24, 1993 - * Phoenix Technologies Ltd. + * Standard BIOS 32-bit Service Directory Proposal + * Revision 0.4 May 24, 1993 + * Phoenix Technologies Ltd. * Norwood, MA * and the PCI BIOS specification. */ @@ -67,7 +69,7 @@ union bios32 { unsigned char revision; /* Revision level, 0 */ unsigned char length; /* Length in paragraphs should be 01 */ unsigned char checksum; /* All bytes must add up to zero */ - unsigned char reserved[5]; /* Must be zero */ + unsigned char reserved[5]; /* Must be zero */ } fields; char chars[16]; }; @@ -108,15 +110,16 @@ static unsigned long __init bios32_service(unsigned long service) local_irq_restore(flags); switch (return_code) { - case 0: - return address + entry; - case 0x80: /* Not present */ - printk(KERN_WARNING "bios32_service(0x%lx): not present\n", service); - return 0; - default: /* Shouldn't happen */ - printk(KERN_WARNING "bios32_service(0x%lx): returned 0x%x -- BIOS bug!\n", - service, return_code); - return 0; + case 0: + return address + entry; + case 0x80: /* Not present */ + pr_warn("%s(0x%lx): not present\n", + __func__, service); + return 0; + default: /* Shouldn't happen */ + pr_warn("%s(0x%lx): returned 0x%x -- BIOS bug!\n", + __func__, service, return_code); + return 0; } } @@ -140,8 +143,7 @@ static int __init check_pcibios(void) pci_indirect.address = pcibios_entry + PAGE_OFFSET; local_irq_save(flags); - __asm__( - "lcall *(%%edi); cld\n\t" + __asm__("lcall *(%%edi); cld\n\t" "jc 1f\n\t" "xor %%ah, %%ah\n" "1:" @@ -160,14 +162,15 @@ static int __init check_pcibios(void) minor_ver = ebx & 0xff; if (pcibios_last_bus < 0) pcibios_last_bus = ecx & 0xff; - DBG("PCI: BIOS probe returned s=%02x hw=%02x ver=%02x.%02x l=%02x\n", - status, hw_mech, major_ver, minor_ver, pcibios_last_bus); + pr_debug("BIOS probe returned s=%02x hw=%02x ver=%02x.%02x l=%02x\n", + status, hw_mech, major_ver, minor_ver, + pcibios_last_bus); if (status || signature != PCI_SIGNATURE) { - printk (KERN_ERR "PCI: BIOS BUG #%x[%08x] found\n", - status, signature); + pr_err("BIOS BUG #%x[%08x] found\n", + status, signature); return 0; } - printk(KERN_INFO "PCI: PCI BIOS revision %x.%02x entry at 0x%lx, last bus=%d\n", + pr_info("PCI BIOS revision %x.%02x entry at 0x%lx, last bus=%d\n", major_ver, minor_ver, pcibios_entry, pcibios_last_bus); #ifdef CONFIG_PCI_DIRECT if (!(hw_mech & PCIBIOS_HW_TYPE1)) @@ -239,7 +242,7 @@ static int pci_bios_write(unsigned int seg, unsigned int bus, u16 number = 0; WARN_ON(seg); - if ((bus > 255) || (devfn > 255) || (reg > 255)) + if ((bus > 255) || (devfn > 255) || (reg > 255)) return -EINVAL; raw_spin_lock_irqsave(&pci_config_lock, flags); @@ -316,19 +319,19 @@ static const struct pci_raw_ops *__init pci_find_bios(void) if (sum != 0) continue; if (check->fields.revision != 0) { - printk("PCI: unsupported BIOS32 revision %d at 0x%p\n", + pr_warn("unsupported BIOS32 revision %d at 0x%p\n", check->fields.revision, check); continue; } - DBG("PCI: BIOS32 Service Directory structure at 0x%p\n", check); + pr_debug("BIOS32 Service Directory structure at 0x%p\n", check); if (check->fields.entry >= 0x100000) { - printk("PCI: BIOS32 entry (0x%p) in high memory, " - "cannot use.\n", check); + pr_warn("BIOS32 entry (0x%p) in high memory, cannot use.\n", + check); return NULL; } else { unsigned long bios32_entry = check->fields.entry; - DBG("PCI: BIOS32 Service Directory entry at 0x%lx\n", - bios32_entry); + pr_debug("BIOS32 Service Directory entry at 0x%lx\n", + bios32_entry); bios32_indirect.address = bios32_entry + PAGE_OFFSET; set_bios_x(); if (check_pcibios()) @@ -348,9 +351,9 @@ struct irq_routing_options { u16 size; struct irq_info *table; u16 segment; -} __attribute__((packed)); +} __packed; -struct irq_routing_table * pcibios_get_irq_routing_table(void) +struct irq_routing_table *pcibios_get_irq_routing_table(void) { struct irq_routing_options opt; struct irq_routing_table *rt = NULL; @@ -366,7 +369,7 @@ struct irq_routing_table * pcibios_get_irq_routing_table(void) opt.size = PAGE_SIZE; opt.segment = __KERNEL_DS; - DBG("PCI: Fetching IRQ routing table... "); + pr_debug("Fetching IRQ routing table... "); __asm__("push %%es\n\t" "push %%ds\n\t" "pop %%es\n\t" @@ -384,17 +387,19 @@ struct irq_routing_table * pcibios_get_irq_routing_table(void) "S" (&pci_indirect), "m" (opt) : "memory"); - DBG("OK ret=%d, size=%d, map=%x\n", ret, opt.size, map); + pr_debug("OK ret=%d, size=%d, map=%x\n", ret, opt.size, map); if (ret & 0xff00) - printk(KERN_ERR "PCI: Error %02x when fetching IRQ routing table.\n", (ret >> 8) & 0xff); + pr_err("Error %02x when fetching IRQ routing table.\n", + (ret >> 8) & 0xff); else if (opt.size) { - rt = kmalloc(sizeof(struct irq_routing_table) + opt.size, GFP_KERNEL); + rt = kmalloc(sizeof(struct irq_routing_table) + opt.size, + GFP_KERNEL); if (rt) { memset(rt, 0, sizeof(struct irq_routing_table)); rt->size = opt.size + sizeof(struct irq_routing_table); rt->exclusive_irqs = map; memcpy(rt->slots, (void *) page, opt.size); - printk(KERN_INFO "PCI: Using BIOS Interrupt Routing Table\n"); + pr_info("Using BIOS Interrupt Routing Table\n"); } } free_page(page); @@ -421,7 +426,7 @@ EXPORT_SYMBOL(pcibios_set_irq_routing); void __init pci_pcbios_init(void) { - if ((pci_probe & PCI_PROBE_BIOS) + if ((pci_probe & PCI_PROBE_BIOS) && ((raw_pci_ops = pci_find_bios()))) { pci_bios_present = 1; } -- 2.22.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-08-29 17:12 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-25 18:25 [PATCH] x86/PCI: Add missing log facility and move to use pr_ macros in pcbios.c Krzysztof Wilczynski 2019-08-27 22:47 ` Bjorn Helgaas 2019-08-28 10:59 ` Krzysztof Wilczynski 2019-08-28 17:51 ` [PATCH v2] " Krzysztof Wilczynski 2019-08-28 18:02 ` Joe Perches 2019-08-28 18:40 ` Krzysztof Wilczynski 2019-08-28 18:43 ` Joe Perches 2019-08-28 18:58 ` Krzysztof Wilczynski 2019-08-29 17:11 ` [PATCH v3] x86/PCI: Add missing log facility and move to use pr_ macros Krzysztof Wilczynski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).