| Message ID | 20190828175120.22164-1-kw@linux.com |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v2] x86/PCI: Add missing log facility and move to use pr_ macros in pcbios.c | expand |
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
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
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.
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
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);
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(-)