linux-kernel - Re: [PATCH] drivers: pci: Define pr_fmt() and use pr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20190419132033.GJ126710@google.com>
Date:   Fri, 19 Apr 2019 08:20:33 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Mohan Kumar <mohankumar718@...il.com>
Cc:     linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        Frederick Lawler <fred@...dlawl.com>
Subject: Re: [PATCH] drivers: pci: Define pr_fmt() and use pr_*() instead of
 printk()

[+cc Frederick because I think he's also doing some printk changes.
I don't think they will overlap, so it's just FYI]

Hi Mohan,

This subject line is better, but still doesn't match the convention.
Here's what it would look like if I applied it:

  $ git log --oneline --no-merges drivers/pci/quirks.c | head -5
  7b1068d23f9d drivers: pci: Define pr_fmt() and use pr_*() instead of printk()
  f57a98e1b713 PCI: Work around Synopsys duplicate Device ID (HAPS USB3, NXP i.MX)
  01926f6b321b PCI: Add ACS quirk for HXT SD4800
  1d09d57728fe PCI: Mark expected switch fall-through
  03e6742584af PCI: Override Synopsys USB 3.x HAPS device class

I want it to start with "PCI: " so your patch matches the previous
ones.

On Thu, Apr 18, 2019 at 09:41:04PM +0300, Mohan Kumar wrote:
> Define a pr_fmt() macro that convert all of the explicit printk() calls
> into corresponding pr_*().

  - Ignore drivers/pci/hotplug/ for now.  Many of those drivers define
    their own logging macros that make things messier.

  - Convert these in one patch for all of drivers/pci/ (except
    drivers/pci/hotplug/).  This is pure cleanup and doesn't change
    any behavior, so shouldn't be very controversial:

      printk(KERN_WARN) to pr_warn()
      printk(KERN_ERR)  to pr_err()

  - In a second patch, add "#define pr_fmt()" where useful.  This
    should be a separate patch to make it easier to review.  But this
    also shouldn't change any behavior, so should be pretty
    straightforward.

  - Convert these in a third patch.  This is separate because it
    involves a behavior change and may require some discussion and
    tweaking of individual situations:

      pci_printk(KERN_DEBUG) to pci_info()
      dev_printk(KERN_DEBUG) to dev_info()
      printk(KERN_DEBUG)     to pr_info()

Thanks for your work on this!  Your changes will definitely make the
PCI core more consistent, which makes it more pleasant to work in.

Bjorn

> Signed-off-by: Mohan Kumar <mohankumar718@...il.com>
> ---
>  drivers/pci/pci-stub.c | 11 +++++------
>  drivers/pci/quirks.c   | 10 +++++-----
>  2 files changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pci/pci-stub.c b/drivers/pci/pci-stub.c
> index 66f8a59..f946bf9 100644
> --- a/drivers/pci/pci-stub.c
> +++ b/drivers/pci/pci-stub.c
> @@ -16,6 +16,8 @@
>   * .../0000:00:19.0/driver -> ../../../bus/pci/drivers/pci-stub
>   */
>  
> +#define pr_fmt(fmt) "pci-stub: " fmt
> +
>  #include <linux/module.h>
>  #include <linux/pci.h>
>  
> @@ -66,20 +68,17 @@ static int __init pci_stub_init(void)
>  				&class, &class_mask);
>  
>  		if (fields < 2) {
> -			printk(KERN_WARNING
> -			       "pci-stub: invalid id string \"%s\"\n", id);
> +			pr_warn("invalid id string \"%s\"\n", id);
>  			continue;
>  		}
>  
> -		printk(KERN_INFO
> -		       "pci-stub: add %04X:%04X sub=%04X:%04X cls=%08X/%08X\n",
> +		pr_info("add %04X:%04X sub=%04X:%04X cls=%08X/%08X\n",
>  		       vendor, device, subvendor, subdevice, class, class_mask);
>  
>  		rc = pci_add_dynid(&stub_driver, vendor, device,
>  				   subvendor, subdevice, class, class_mask, 0);
>  		if (rc)
> -			printk(KERN_WARNING
> -			       "pci-stub: failed to add dynamic id (%d)\n", rc);
> +			pr_warn("failed to add dynamic id (%d)\n", rc);
>  	}
>  
>  	return 0;
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index f9cd4d4..3b367b7 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -11,6 +11,7 @@
>   * Init/reset quirks for USB host controllers should be in the USB quirks
>   * file, where their drivers can use them.
>   */
> +#define pr_fmt(fmt) "PCI: " fmt
>  
>  #include <linux/types.h>
>  #include <linux/kernel.h>
> @@ -159,8 +160,7 @@ static int __init pci_apply_final_quirks(void)
>  	u8 tmp;
>  
>  	if (pci_cache_line_size)
> -		printk(KERN_DEBUG "PCI: CLS %u bytes\n",
> -		       pci_cache_line_size << 2);
> +		pr_debug("CLS %u bytes\n", pci_cache_line_size << 2);
>  
>  	pci_apply_fixup_final_quirks = true;
>  	for_each_pci_dev(dev) {
> @@ -177,7 +177,7 @@ static int __init pci_apply_final_quirks(void)
>  			if (!tmp || cls == tmp)
>  				continue;
>  
> -			printk(KERN_DEBUG "PCI: CLS mismatch (%u != %u), using %u bytes\n",
> +			pr_debug("CLS mismatch (%u != %u), using %u bytes\n",
>  			       cls << 2, tmp << 2,
>  			       pci_dfl_cache_line_size << 2);
>  			pci_cache_line_size = pci_dfl_cache_line_size;
> @@ -185,7 +185,7 @@ static int __init pci_apply_final_quirks(void)
>  	}
>  
>  	if (!pci_cache_line_size) {
> -		printk(KERN_DEBUG "PCI: CLS %u bytes, default %u\n",
> +		pr_debug("CLS %u bytes, default %u\n",
>  		       cls << 2, pci_dfl_cache_line_size << 2);
>  		pci_cache_line_size = cls ? cls : pci_dfl_cache_line_size;
>  	}
> @@ -2613,7 +2613,7 @@ static void nvbridge_check_legacy_irq_routing(struct pci_dev *dev)
>  	pci_read_config_dword(dev, 0x74, &cfg);
>  
>  	if (cfg & ((1 << 2) | (1 << 15))) {
> -		printk(KERN_INFO "Rewriting IRQ routing register on MCP55\n");
> +		pr_info("Rewriting IRQ routing register on MCP55\n");
>  		cfg &= ~((1 << 2) | (1 << 15));
>  		pci_write_config_dword(dev, 0x74, cfg);
>  	}
> -- 
> 2.7.4
>