[PATCH 0/4] PCI: Use PCIe service name in dmesg logs

[PATCH 0/4] PCI: Use PCIe service name in dmesg logs

[fred]

From: Frederick Lawler <fred@fredlawl.com>

In referrence to [1], PCIe services did not have unifrom logging via pci_*()
printk wrappers. This patch series first replaces left over dev_*() calls
for port services. Then, replaces all hotplug ctrl_*() calls with pci_*() to
bring hotplug in line with the other services, and removes the custom
ctrl_*() definitions. Lastly, to satisfy [1], add in dev_fmt()'s to each of
the services.

1. https://lore.kernel.org/linux-pci/20190308180149.GD214730@google.com/

Frederick Lawler (4):
  PCI: Replace dev_*() printk wrappers with pci_*() printk wrappers
  PCI: pciehp: Replace ctrl_*() with pci_*()
  PCI: pciehp: Remove unused macro definitions
  PCI/portdrv: Add dev_fmt() to port drivers

 drivers/pci/hotplug/pciehp.h       |  27 -----
 drivers/pci/hotplug/pciehp_core.c  |  25 +++--
 drivers/pci/hotplug/pciehp_ctrl.c  |  88 +++++++++--------
 drivers/pci/hotplug/pciehp_hpc.c   | 154 ++++++++++++++++-------------
 drivers/pci/hotplug/pciehp_pci.c   |  12 ++-
 drivers/pci/pcie/aer.c             |  16 +--
 drivers/pci/pcie/aer_inject.c      |   6 +-
 drivers/pci/pcie/bw_notification.c |   2 +
 drivers/pci/pcie/dpc.c             |  29 +++---
 drivers/pci/pcie/pme.c             |   2 +
 10 files changed, 187 insertions(+), 174 deletions(-)

--
2.17.1

[PATCH 1/4] PCI: Replace dev_*() printk wrappers with pci_*() printk wrappers

[fred]

From: Frederick Lawler <fred@fredlawl.com>

Replace remaining instances of dev_*() printk wrappers with pci_*()
printk wrappers. No functional change intended.

Signed-off-by: Frederick Lawler <fred@fredlawl.com>
---
 drivers/pci/pcie/aer.c        | 13 ++++++-------
 drivers/pci/pcie/aer_inject.c |  4 ++--
 drivers/pci/pcie/dpc.c        | 27 ++++++++++++---------------
 3 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index f8fc2114ad39..224d878a28b4 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -964,8 +964,7 @@ static bool find_source_device(struct pci_dev *parent,
 	pci_walk_bus(parent->subordinate, find_device_iter, e_info);
 
 	if (!e_info->error_dev_num) {
-		pci_printk(KERN_DEBUG, parent, "can't find device of ID%04x\n",
-			   e_info->id);
+		pci_dbg(parent, "can't find device of ID%04x\n", e_info->id);
 		return false;
 	}
 	return true;
@@ -1377,10 +1376,11 @@ static int aer_probe(struct pcie_device *dev)
 	int status;
 	struct aer_rpc *rpc;
 	struct device *device = &dev->device;
+	struct pci_dev *pdev = dev->port;
 
 	rpc = devm_kzalloc(device, sizeof(struct aer_rpc), GFP_KERNEL);
 	if (!rpc) {
-		dev_printk(KERN_DEBUG, device, "alloc AER rpc failed\n");
+		pci_dbg(pdev, "alloc AER rpc failed\n");
 		return -ENOMEM;
 	}
 	rpc->rpd = dev->port;
@@ -1389,13 +1389,12 @@ static int aer_probe(struct pcie_device *dev)
 	status = devm_request_threaded_irq(device, dev->irq, aer_irq, aer_isr,
 					   IRQF_SHARED, "aerdrv", dev);
 	if (status) {
-		dev_printk(KERN_DEBUG, device, "request AER IRQ %d failed\n",
-			   dev->irq);
+		pci_dbg(pdev, "request AER IRQ %d failed\n", dev->irq);
 		return status;
 	}
 
 	aer_enable_rootport(rpc);
-	dev_info(device, "AER enabled with IRQ %d\n", dev->irq);
+	pci_info(pdev, "AER enabled with IRQ %d\n", dev->irq);
 	return 0;
 }
 
@@ -1419,7 +1418,7 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
 	pci_write_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, reg32);
 
 	rc = pci_bus_error_reset(dev);
-	pci_printk(KERN_DEBUG, dev, "Root Port link has been reset\n");
+	pci_dbg(dev, "Root Port link has been reset\n");
 
 	/* Clear Root Error Status */
 	pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &reg32);
diff --git a/drivers/pci/pcie/aer_inject.c b/drivers/pci/pcie/aer_inject.c
index 95d4759664b3..610b617ae600 100644
--- a/drivers/pci/pcie/aer_inject.c
+++ b/drivers/pci/pcie/aer_inject.c
@@ -460,12 +460,12 @@ static int aer_inject(struct aer_error_inj *einj)
 	if (device) {
 		edev = to_pcie_device(device);
 		if (!get_service_data(edev)) {
-			dev_warn(&edev->device,
+			pci_warn(edev->port,
 				 "aer_inject: AER service is not initialized\n");
 			ret = -EPROTONOSUPPORT;
 			goto out_put;
 		}
-		dev_info(&edev->device,
+		pci_info(edev->port,
 			 "aer_inject: Injecting errors %08x/%08x into device %s\n",
 			 einj->cor_status, einj->uncor_status, pci_name(dev));
 		local_irq_disable();
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 7b77754a82de..72659286191b 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -100,7 +100,6 @@ static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
 {
 	unsigned long timeout = jiffies + HZ;
 	struct pci_dev *pdev = dpc->dev->port;
-	struct device *dev = &dpc->dev->device;
 	u16 cap = dpc->cap_pos, status;
 
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
@@ -110,7 +109,7 @@ static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
 		pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
 	}
 	if (status & PCI_EXP_DPC_RP_BUSY) {
-		dev_warn(dev, "DPC root port still busy\n");
+		pci_warn(pdev, "DPC root port still busy\n");
 		return -EBUSY;
 	}
 	return 0;
@@ -148,7 +147,6 @@ static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
 
 static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
 {
-	struct device *dev = &dpc->dev->device;
 	struct pci_dev *pdev = dpc->dev->port;
 	u16 cap = dpc->cap_pos, dpc_status, first_error;
 	u32 status, mask, sev, syserr, exc, dw0, dw1, dw2, dw3, log, prefix;
@@ -156,13 +154,13 @@ static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
 
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS, &status);
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_MASK, &mask);
-	dev_err(dev, "rp_pio_status: %#010x, rp_pio_mask: %#010x\n",
+	pci_err(pdev, "rp_pio_status: %#010x, rp_pio_mask: %#010x\n",
 		status, mask);
 
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_SEVERITY, &sev);
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_SYSERROR, &syserr);
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_EXCEPTION, &exc);
-	dev_err(dev, "RP PIO severity=%#010x, syserror=%#010x, exception=%#010x\n",
+	pci_err(pdev, "RP PIO severity=%#010x, syserror=%#010x, exception=%#010x\n",
 		sev, syserr, exc);
 
 	/* Get First Error Pointer */
@@ -171,7 +169,7 @@ static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
 
 	for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
 		if ((status & ~mask) & (1 << i))
-			dev_err(dev, "[%2d] %s%s\n", i, rp_pio_error_string[i],
+			pci_err(pdev, "[%2d] %s%s\n", i, rp_pio_error_string[i],
 				first_error == i ? " (First)" : "");
 	}
 
@@ -185,18 +183,18 @@ static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
 			      &dw2);
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG + 12,
 			      &dw3);
-	dev_err(dev, "TLP Header: %#010x %#010x %#010x %#010x\n",
+	pci_err(pdev, "TLP Header: %#010x %#010x %#010x %#010x\n",
 		dw0, dw1, dw2, dw3);
 
 	if (dpc->rp_log_size < 5)
 		goto clear_status;
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_IMPSPEC_LOG, &log);
-	dev_err(dev, "RP PIO ImpSpec Log %#010x\n", log);
+	pci_err(pdev, "RP PIO ImpSpec Log %#010x\n", log);
 
 	for (i = 0; i < dpc->rp_log_size - 5; i++) {
 		pci_read_config_dword(pdev,
 			cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG, &prefix);
-		dev_err(dev, "TLP Prefix Header: dw%d, %#010x\n", i, prefix);
+		pci_err(pdev, "TLP Prefix Header: dw%d, %#010x\n", i, prefix);
 	}
  clear_status:
 	pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS, status);
@@ -229,18 +227,17 @@ static irqreturn_t dpc_handler(int irq, void *context)
 	struct aer_err_info info;
 	struct dpc_dev *dpc = context;
 	struct pci_dev *pdev = dpc->dev->port;
-	struct device *dev = &dpc->dev->device;
 	u16 cap = dpc->cap_pos, status, source, reason, ext_reason;
 
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_SOURCE_ID, &source);
 
-	dev_info(dev, "DPC containment event, status:%#06x source:%#06x\n",
+	pci_info(pdev, "DPC containment event, status:%#06x source:%#06x\n",
 		 status, source);
 
 	reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN) >> 1;
 	ext_reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT) >> 5;
-	dev_warn(dev, "DPC %s detected\n",
+	pci_warn(pdev, "DPC %s detected\n",
 		 (reason == 0) ? "unmasked uncorrectable error" :
 		 (reason == 1) ? "ERR_NONFATAL" :
 		 (reason == 2) ? "ERR_FATAL" :
@@ -307,7 +304,7 @@ static int dpc_probe(struct pcie_device *dev)
 					   dpc_handler, IRQF_SHARED,
 					   "pcie-dpc", dpc);
 	if (status) {
-		dev_warn(device, "request IRQ%d failed: %d\n", dev->irq,
+		pci_warn(pdev, "request IRQ%d failed: %d\n", dev->irq,
 			 status);
 		return status;
 	}
@@ -319,7 +316,7 @@ static int dpc_probe(struct pcie_device *dev)
 	if (dpc->rp_extensions) {
 		dpc->rp_log_size = (cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
 		if (dpc->rp_log_size < 4 || dpc->rp_log_size > 9) {
-			dev_err(device, "RP PIO log size %u is invalid\n",
+			pci_err(pdev, "RP PIO log size %u is invalid\n",
 				dpc->rp_log_size);
 			dpc->rp_log_size = 0;
 		}
@@ -328,7 +325,7 @@ static int dpc_probe(struct pcie_device *dev)
 	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
 	pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, ctl);
 
-	dev_info(device, "DPC error containment capabilities: Int Msg #%d, RPExt%c PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n",
+	pci_info(pdev, "DPC error containment capabilities: Int Msg #%d, RPExt%c PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n",
 		cap & PCI_EXP_DPC_IRQ, FLAG(cap, PCI_EXP_DPC_CAP_RP_EXT),
 		FLAG(cap, PCI_EXP_DPC_CAP_POISONED_TLP),
 		FLAG(cap, PCI_EXP_DPC_CAP_SW_TRIGGER), dpc->rp_log_size,
-- 
2.17.1

Re: [PATCH 1/4] PCI: Replace dev_*() printk wrappers with pci_*() printk wrappers

[Andy Shevchenko]

On Sat, Apr 27, 2019 at 02:13:01PM -0500, fred@fredlawl.com wrote:
> From: Frederick Lawler <fred@fredlawl.com>
> 
> Replace remaining instances of dev_*() printk wrappers with pci_*()
> printk wrappers. No functional change intended.

> -		pci_printk(KERN_DEBUG, parent, "can't find device of ID%04x\n",
> -			   e_info->id);
> +		pci_dbg(parent, "can't find device of ID%04x\n", e_info->id);

These are not equivalent.

> -		dev_printk(KERN_DEBUG, device, "alloc AER rpc failed\n");
> +		pci_dbg(pdev, "alloc AER rpc failed\n");

Ditto.

> -		dev_printk(KERN_DEBUG, device, "request AER IRQ %d failed\n",
> -			   dev->irq);
> +		pci_dbg(pdev, "request AER IRQ %d failed\n", dev->irq);

Ditto.

And so on.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] PCI: Replace dev_*() printk wrappers with pci_*() printk wrappers

[Frederick Lawler]

Andy,

Andy Shevchenko wrote on 4/28/19 10:43 AM:
> On Sat, Apr 27, 2019 at 02:13:01PM -0500, fred@fredlawl.com wrote:
>> From: Frederick Lawler <fred@fredlawl.com>
>>
>> Replace remaining instances of dev_*() printk wrappers with pci_*()
>> printk wrappers. No functional change intended.
> 
>> -		pci_printk(KERN_DEBUG, parent, "can't find device of ID%04x\n",
>> -			   e_info->id);
>> +		pci_dbg(parent, "can't find device of ID%04x\n", e_info->id);
> 
> These are not equivalent.
> 
>> -		dev_printk(KERN_DEBUG, device, "alloc AER rpc failed\n");
>> +		pci_dbg(pdev, "alloc AER rpc failed\n");
> 
> Ditto.
> 
>> -		dev_printk(KERN_DEBUG, device, "request AER IRQ %d failed\n",
>> -			   dev->irq);
>> +		pci_dbg(pdev, "request AER IRQ %d failed\n", dev->irq);
> 
> Ditto.
> 
> And so on.
> 

Thanks for the review. Clearly this was an oversight on my part and I'll 
have that corrected. Thanks!


Frederick Lawler

Re: [PATCH 1/4] PCI: Replace dev_*() printk wrappers with pci_*() printk wrappers

[Bjorn Helgaas]

On Sat, Apr 27, 2019 at 02:13:01PM -0500, fred@fredlawl.com wrote:
> From: Frederick Lawler <fred@fredlawl.com>
> 
> Replace remaining instances of dev_*() printk wrappers with pci_*()
> printk wrappers. No functional change intended.
> 
> Signed-off-by: Frederick Lawler <fred@fredlawl.com>
> ---
>  drivers/pci/pcie/aer.c        | 13 ++++++-------
>  drivers/pci/pcie/aer_inject.c |  4 ++--
>  drivers/pci/pcie/dpc.c        | 27 ++++++++++++---------------
>  3 files changed, 20 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index f8fc2114ad39..224d878a28b4 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -964,8 +964,7 @@ static bool find_source_device(struct pci_dev *parent,
>  	pci_walk_bus(parent->subordinate, find_device_iter, e_info);
>  
>  	if (!e_info->error_dev_num) {
> -		pci_printk(KERN_DEBUG, parent, "can't find device of ID%04x\n",
> -			   e_info->id);
> +		pci_dbg(parent, "can't find device of ID%04x\n", e_info->id);

I don't like dev_dbg() and pci_dbg() because the behavior depends on
CONFIG_DYNAMIC_DEBUG, DEBUG, etc.

They may be fine for development, but for production, I want to be
able to users for "a complete dmesg log".  I don't want to have to ask
them to "please rebuild your kernel with CONFIG_DYNAMIC_DEBUG=n and
boot with 'dyndbg=xxx'"

For that reason I prefer the "pci_printk(KERN_DEBUG)" because that
output always goes to the dmesg log.

But "pci_printk(KERN_DEBUG)" is definitely ugly.  I think the way to
fix that is to convert it to "pci_info()" instead.

>  		return false;
>  	}
>  	return true;
> @@ -1377,10 +1376,11 @@ static int aer_probe(struct pcie_device *dev)
>  	int status;
>  	struct aer_rpc *rpc;
>  	struct device *device = &dev->device;
> +	struct pci_dev *pdev = dev->port;
>  
>  	rpc = devm_kzalloc(device, sizeof(struct aer_rpc), GFP_KERNEL);
>  	if (!rpc) {
> -		dev_printk(KERN_DEBUG, device, "alloc AER rpc failed\n");
> +		pci_dbg(pdev, "alloc AER rpc failed\n");

This hunk converts from using the pcie_device to the pci_dev, so it
belongs in a different patch.

>  		return -ENOMEM;
>  	}
>  	rpc->rpd = dev->port;
> @@ -1389,13 +1389,12 @@ static int aer_probe(struct pcie_device *dev)
>  	status = devm_request_threaded_irq(device, dev->irq, aer_irq, aer_isr,
>  					   IRQF_SHARED, "aerdrv", dev);
>  	if (status) {
> -		dev_printk(KERN_DEBUG, device, "request AER IRQ %d failed\n",
> -			   dev->irq);
> +		pci_dbg(pdev, "request AER IRQ %d failed\n", dev->irq);
>  		return status;

This one also.

>  	}
>  
>  	aer_enable_rootport(rpc);
> -	dev_info(device, "AER enabled with IRQ %d\n", dev->irq);
> +	pci_info(pdev, "AER enabled with IRQ %d\n", dev->irq);

And this, and many others below.  *This* patch should only convert

  - pci_printk(KERN_DEBUG, pdev, ...)
  + pci_info(pdev, ...)

and

  - dev_printk(KERN_DEBUG, pcie_dev, ...)
  + dev_info(pcie_dev, ...)

>  	return 0;
>  }
>  
> @@ -1419,7 +1418,7 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
>  	pci_write_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, reg32);
>  
>  	rc = pci_bus_error_reset(dev);
> -	pci_printk(KERN_DEBUG, dev, "Root Port link has been reset\n");
> +	pci_dbg(dev, "Root Port link has been reset\n");
>  
>  	/* Clear Root Error Status */
>  	pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &reg32);
> diff --git a/drivers/pci/pcie/aer_inject.c b/drivers/pci/pcie/aer_inject.c
> index 95d4759664b3..610b617ae600 100644
> --- a/drivers/pci/pcie/aer_inject.c
> +++ b/drivers/pci/pcie/aer_inject.c
> @@ -460,12 +460,12 @@ static int aer_inject(struct aer_error_inj *einj)
>  	if (device) {
>  		edev = to_pcie_device(device);
>  		if (!get_service_data(edev)) {
> -			dev_warn(&edev->device,
> +			pci_warn(edev->port,
>  				 "aer_inject: AER service is not initialized\n");
>  			ret = -EPROTONOSUPPORT;
>  			goto out_put;
>  		}
> -		dev_info(&edev->device,
> +		pci_info(edev->port,
>  			 "aer_inject: Injecting errors %08x/%08x into device %s\n",
>  			 einj->cor_status, einj->uncor_status, pci_name(dev));
>  		local_irq_disable();
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 7b77754a82de..72659286191b 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -100,7 +100,6 @@ static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
>  {
>  	unsigned long timeout = jiffies + HZ;
>  	struct pci_dev *pdev = dpc->dev->port;
> -	struct device *dev = &dpc->dev->device;
>  	u16 cap = dpc->cap_pos, status;
>  
>  	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
> @@ -110,7 +109,7 @@ static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
>  		pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
>  	}
>  	if (status & PCI_EXP_DPC_RP_BUSY) {
> -		dev_warn(dev, "DPC root port still busy\n");
> +		pci_warn(pdev, "DPC root port still busy\n");
>  		return -EBUSY;
>  	}
>  	return 0;
> @@ -148,7 +147,6 @@ static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
>  
>  static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
>  {
> -	struct device *dev = &dpc->dev->device;
>  	struct pci_dev *pdev = dpc->dev->port;
>  	u16 cap = dpc->cap_pos, dpc_status, first_error;
>  	u32 status, mask, sev, syserr, exc, dw0, dw1, dw2, dw3, log, prefix;
> @@ -156,13 +154,13 @@ static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
>  
>  	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS, &status);
>  	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_MASK, &mask);
> -	dev_err(dev, "rp_pio_status: %#010x, rp_pio_mask: %#010x\n",
> +	pci_err(pdev, "rp_pio_status: %#010x, rp_pio_mask: %#010x\n",
>  		status, mask);
>  
>  	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_SEVERITY, &sev);
>  	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_SYSERROR, &syserr);
>  	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_EXCEPTION, &exc);
> -	dev_err(dev, "RP PIO severity=%#010x, syserror=%#010x, exception=%#010x\n",
> +	pci_err(pdev, "RP PIO severity=%#010x, syserror=%#010x, exception=%#010x\n",
>  		sev, syserr, exc);
>  
>  	/* Get First Error Pointer */
> @@ -171,7 +169,7 @@ static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
>  
>  	for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
>  		if ((status & ~mask) & (1 << i))
> -			dev_err(dev, "[%2d] %s%s\n", i, rp_pio_error_string[i],
> +			pci_err(pdev, "[%2d] %s%s\n", i, rp_pio_error_string[i],
>  				first_error == i ? " (First)" : "");
>  	}
>  
> @@ -185,18 +183,18 @@ static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
>  			      &dw2);
>  	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG + 12,
>  			      &dw3);
> -	dev_err(dev, "TLP Header: %#010x %#010x %#010x %#010x\n",
> +	pci_err(pdev, "TLP Header: %#010x %#010x %#010x %#010x\n",
>  		dw0, dw1, dw2, dw3);
>  
>  	if (dpc->rp_log_size < 5)
>  		goto clear_status;
>  	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_IMPSPEC_LOG, &log);
> -	dev_err(dev, "RP PIO ImpSpec Log %#010x\n", log);
> +	pci_err(pdev, "RP PIO ImpSpec Log %#010x\n", log);
>  
>  	for (i = 0; i < dpc->rp_log_size - 5; i++) {
>  		pci_read_config_dword(pdev,
>  			cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG, &prefix);
> -		dev_err(dev, "TLP Prefix Header: dw%d, %#010x\n", i, prefix);
> +		pci_err(pdev, "TLP Prefix Header: dw%d, %#010x\n", i, prefix);
>  	}
>   clear_status:
>  	pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS, status);
> @@ -229,18 +227,17 @@ static irqreturn_t dpc_handler(int irq, void *context)
>  	struct aer_err_info info;
>  	struct dpc_dev *dpc = context;
>  	struct pci_dev *pdev = dpc->dev->port;
> -	struct device *dev = &dpc->dev->device;
>  	u16 cap = dpc->cap_pos, status, source, reason, ext_reason;
>  
>  	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
>  	pci_read_config_word(pdev, cap + PCI_EXP_DPC_SOURCE_ID, &source);
>  
> -	dev_info(dev, "DPC containment event, status:%#06x source:%#06x\n",
> +	pci_info(pdev, "DPC containment event, status:%#06x source:%#06x\n",
>  		 status, source);
>  
>  	reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN) >> 1;
>  	ext_reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT) >> 5;
> -	dev_warn(dev, "DPC %s detected\n",
> +	pci_warn(pdev, "DPC %s detected\n",
>  		 (reason == 0) ? "unmasked uncorrectable error" :
>  		 (reason == 1) ? "ERR_NONFATAL" :
>  		 (reason == 2) ? "ERR_FATAL" :
> @@ -307,7 +304,7 @@ static int dpc_probe(struct pcie_device *dev)
>  					   dpc_handler, IRQF_SHARED,
>  					   "pcie-dpc", dpc);
>  	if (status) {
> -		dev_warn(device, "request IRQ%d failed: %d\n", dev->irq,
> +		pci_warn(pdev, "request IRQ%d failed: %d\n", dev->irq,
>  			 status);
>  		return status;
>  	}
> @@ -319,7 +316,7 @@ static int dpc_probe(struct pcie_device *dev)
>  	if (dpc->rp_extensions) {
>  		dpc->rp_log_size = (cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
>  		if (dpc->rp_log_size < 4 || dpc->rp_log_size > 9) {
> -			dev_err(device, "RP PIO log size %u is invalid\n",
> +			pci_err(pdev, "RP PIO log size %u is invalid\n",
>  				dpc->rp_log_size);
>  			dpc->rp_log_size = 0;
>  		}
> @@ -328,7 +325,7 @@ static int dpc_probe(struct pcie_device *dev)
>  	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
>  	pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, ctl);
>  
> -	dev_info(device, "DPC error containment capabilities: Int Msg #%d, RPExt%c PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n",
> +	pci_info(pdev, "DPC error containment capabilities: Int Msg #%d, RPExt%c PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n",
>  		cap & PCI_EXP_DPC_IRQ, FLAG(cap, PCI_EXP_DPC_CAP_RP_EXT),
>  		FLAG(cap, PCI_EXP_DPC_CAP_POISONED_TLP),
>  		FLAG(cap, PCI_EXP_DPC_CAP_SW_TRIGGER), dpc->rp_log_size,
> -- 
> 2.17.1
> 

Re: [PATCH 1/4] PCI: Replace dev_*() printk wrappers with pci_*() printk wrappers

[Bjorn Helgaas]

On Sun, Apr 28, 2019 at 07:02:58PM -0500, Bjorn Helgaas wrote:
> On Sat, Apr 27, 2019 at 02:13:01PM -0500, fred@fredlawl.com wrote:
> > From: Frederick Lawler <fred@fredlawl.com>
> > 
> > Replace remaining instances of dev_*() printk wrappers with pci_*()
> > printk wrappers. No functional change intended.
> > 
> > Signed-off-by: Frederick Lawler <fred@fredlawl.com>
> > ---
> >  drivers/pci/pcie/aer.c        | 13 ++++++-------
> >  drivers/pci/pcie/aer_inject.c |  4 ++--
> >  drivers/pci/pcie/dpc.c        | 27 ++++++++++++---------------
> >  3 files changed, 20 insertions(+), 24 deletions(-)

> >  	aer_enable_rootport(rpc);
> > -	dev_info(device, "AER enabled with IRQ %d\n", dev->irq);
> > +	pci_info(pdev, "AER enabled with IRQ %d\n", dev->irq);
> 
> And this, and many others below.  *This* patch should only convert
> 
>   - pci_printk(KERN_DEBUG, pdev, ...)
>   + pci_info(pdev, ...)
> 
> and
> 
>   - dev_printk(KERN_DEBUG, pcie_dev, ...)
>   + dev_info(pcie_dev, ...)

Just to clarify, I do *want* both changes, just in separate patches.
So we'd have

  1) Convert KERN_DEBUG uses to pci_info() for pci_dev usage and to
     dev_info() for pcie_device usage.  I think pciehp is probably an
     exception to this; this patch shouldn't touch ctrl_dbg().

  2) Convert "dev_info(pcie_device)" to "pci_info(pci_dev)".  It might
     be worth doing this in separate patches for each service.  If we
     decide they're simple enough to combine, that's trivial for me to
     do.  It's a little more hassle to split things up afterwards.

     In pciehp, if you do this in the ctrl_*() definitions, it will
     make the patch much smaller.

  3) In pciehp, ctrl_dbg() could probably be changed to use pci_dbg()
     so we'd use the standard kernel dynamic debug stuff instead of
     having the pciehp-specific module parameter.

Thanks a lot for working on all this.  I think it will make the user
experience significantly simpler.

Bjorn

Re: [PATCH 1/4] PCI: Replace dev_*() printk wrappers with pci_*() printk wrappers

[Frederick Lawler]

Bjorn,

Bjorn Helgaas wrote on 4/28/19 7:52 PM:
> On Sun, Apr 28, 2019 at 07:02:58PM -0500, Bjorn Helgaas wrote:
>> On Sat, Apr 27, 2019 at 02:13:01PM -0500, fred@fredlawl.com wrote:
>>> From: Frederick Lawler <fred@fredlawl.com>
>>>
>>> Replace remaining instances of dev_*() printk wrappers with pci_*()
>>> printk wrappers. No functional change intended.
>>>
>>> Signed-off-by: Frederick Lawler <fred@fredlawl.com>
>>> ---
>>>   drivers/pci/pcie/aer.c        | 13 ++++++-------
>>>   drivers/pci/pcie/aer_inject.c |  4 ++--
>>>   drivers/pci/pcie/dpc.c        | 27 ++++++++++++---------------
>>>   3 files changed, 20 insertions(+), 24 deletions(-)
> 
>>>   	aer_enable_rootport(rpc);
>>> -	dev_info(device, "AER enabled with IRQ %d\n", dev->irq);
>>> +	pci_info(pdev, "AER enabled with IRQ %d\n", dev->irq);
>>
>> And this, and many others below.  *This* patch should only convert
>>
>>    - pci_printk(KERN_DEBUG, pdev, ...)
>>    + pci_info(pdev, ...)
>>
>> and
>>
>>    - dev_printk(KERN_DEBUG, pcie_dev, ...)
>>    + dev_info(pcie_dev, ...)
> 
> Just to clarify, I do *want* both changes, just in separate patches.
> So we'd have
> 
>    1) Convert KERN_DEBUG uses to pci_info() for pci_dev usage and to
>       dev_info() for pcie_device usage.  I think pciehp is probably an
>       exception to this; this patch shouldn't touch ctrl_dbg().
> 
>    2) Convert "dev_info(pcie_device)" to "pci_info(pci_dev)".  It might
>       be worth doing this in separate patches for each service.  If we
>       decide they're simple enough to combine, that's trivial for me to
>       do.  It's a little more hassle to split things up afterwards.
> 
>       In pciehp, if you do this in the ctrl_*() definitions, it will
>       make the patch much smaller.
> 
>    3) In pciehp, ctrl_dbg() could probably be changed to use pci_dbg()
>       so we'd use the standard kernel dynamic debug stuff instead of
>       having the pciehp-specific module parameter.
> 
> Thanks a lot for working on all this.  I think it will make the user
> experience significantly simpler.
> 
> Bjorn
> 

Will do, thanks!

Frederick Lawler

[PATCH 2/4] PCI: pciehp: Replace ctrl_*() with pci_*()

[fred]

From: Frederick Lawler <fred@fredlawl.com>

Hotplug useses custom ctrl_*() dev_*() printk wrappers for logging
messages. To make hotplug conform to pci logging, replace uses of these
wrappers with pci_*() printk wrappers. In addition, replace any
printk() calls with pr_*() wrappers.

Signed-off-by: Frederick Lawler <fred@fredlawl.com>
---
 drivers/pci/hotplug/pciehp_core.c |  22 +++--
 drivers/pci/hotplug/pciehp_ctrl.c |  86 +++++++++--------
 drivers/pci/hotplug/pciehp_hpc.c  | 149 ++++++++++++++++--------------
 drivers/pci/hotplug/pciehp_pci.c  |  10 +-
 4 files changed, 145 insertions(+), 122 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index fc5366b50e95..430a47556813 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -51,6 +51,7 @@ static int get_adapter_status(struct hotplug_slot *slot, u8 *value);
 
 static int init_slot(struct controller *ctrl)
 {
+	struct pci_dev *pdev = ctrl->pcie->port;
 	struct hotplug_slot_ops *ops;
 	char name[SLOT_NAME_SIZE];
 	int retval;
@@ -82,7 +83,7 @@ static int init_slot(struct controller *ctrl)
 	retval = pci_hp_initialize(&ctrl->hotplug_slot,
 				   ctrl->pcie->port->subordinate, 0, name);
 	if (retval) {
-		ctrl_err(ctrl, "pci_hp_initialize failed: error %d\n", retval);
+		pci_err(pdev, "pci_hp_initialize failed: error %d\n", retval);
 		kfree(ops);
 	}
 	return retval;
@@ -175,6 +176,7 @@ static int pciehp_probe(struct pcie_device *dev)
 {
 	int rc;
 	struct controller *ctrl;
+	struct pci_dev *pdev;
 
 	/* If this is not a "hotplug" service, we have no business here. */
 	if (dev->service != PCIE_PORT_SERVICE_HP)
@@ -182,39 +184,39 @@ static int pciehp_probe(struct pcie_device *dev)
 
 	if (!dev->port->subordinate) {
 		/* Can happen if we run out of bus numbers during probe */
-		dev_err(&dev->device,
-			"Hotplug bridge without secondary bus, ignoring\n");
+		pci_err(dev->port, "Hotplug bridge without secondary bus, ignoring\n");
 		return -ENODEV;
 	}
 
 	ctrl = pcie_init(dev);
 	if (!ctrl) {
-		dev_err(&dev->device, "Controller initialization failed\n");
+		pci_err(dev->port, "Controller initialization failed\n");
 		return -ENODEV;
 	}
 	set_service_data(dev, ctrl);
+	pdev = ctrl->pcie->port;
 
 	/* Setup the slot information structures */
 	rc = init_slot(ctrl);
 	if (rc) {
 		if (rc == -EBUSY)
-			ctrl_warn(ctrl, "Slot already registered by another hotplug driver\n");
+			pci_warn(pdev, "Slot already registered by another hotplug driver\n");
 		else
-			ctrl_err(ctrl, "Slot initialization failed (%d)\n", rc);
+			pci_err(pdev, "Slot initialization failed (%d)\n", rc);
 		goto err_out_release_ctlr;
 	}
 
 	/* Enable events after we have setup the data structures */
 	rc = pcie_init_notification(ctrl);
 	if (rc) {
-		ctrl_err(ctrl, "Notification initialization failed (%d)\n", rc);
+		pci_err(pdev, "Notification initialization failed (%d)\n", rc);
 		goto err_out_free_ctrl_slot;
 	}
 
 	/* Publish to user space */
 	rc = pci_hp_add(&ctrl->hotplug_slot);
 	if (rc) {
-		ctrl_err(ctrl, "Publication to user space failed (%d)\n", rc);
+		pci_err(pdev, "Publication to user space failed (%d)\n", rc);
 		goto err_out_shutdown_notification;
 	}
 
@@ -328,9 +330,9 @@ int __init pcie_hp_init(void)
 	int retval = 0;
 
 	retval = pcie_port_service_register(&hpdriver_portdrv);
-	dbg("pcie_port_service_register = %d\n", retval);
+	pr_debug("pcie_port_service_register = %d\n", retval);
 	if (retval)
-		dbg("Failure to register service\n");
+		pr_debug("Failure to register service\n");
 
 	return retval;
 }
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 3f3df4c29f6e..345c02b1e8d7 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -54,6 +54,7 @@ static void set_slot_off(struct controller *ctrl)
 static int board_added(struct controller *ctrl)
 {
 	int retval = 0;
+	struct pci_dev *pdev = ctrl->pcie->port;
 	struct pci_bus *parent = ctrl->pcie->port->subordinate;
 
 	if (POWER_CTRL(ctrl)) {
@@ -68,13 +69,13 @@ static int board_added(struct controller *ctrl)
 	/* Check link training status */
 	retval = pciehp_check_link_status(ctrl);
 	if (retval) {
-		ctrl_err(ctrl, "Failed to check link status\n");
+		pci_err(pdev, "Failed to check link status\n");
 		goto err_exit;
 	}
 
 	/* Check for a power fault */
 	if (ctrl->power_fault_detected || pciehp_query_power_fault(ctrl)) {
-		ctrl_err(ctrl, "Slot(%s): Power fault\n", slot_name(ctrl));
+		pci_err(pdev, "Slot(%s): Power fault\n", slot_name(ctrl));
 		retval = -EIO;
 		goto err_exit;
 	}
@@ -82,8 +83,8 @@ static int board_added(struct controller *ctrl)
 	retval = pciehp_configure_device(ctrl);
 	if (retval) {
 		if (retval != -EEXIST) {
-			ctrl_err(ctrl, "Cannot add device at %04x:%02x:00\n",
-				 pci_domain_nr(parent), parent->number);
+			pci_err(pdev, "Cannot add device at %04x:%02x:00\n",
+				pci_domain_nr(parent), parent->number);
 			goto err_exit;
 		}
 	}
@@ -152,18 +153,20 @@ void pciehp_queue_pushbutton_work(struct work_struct *work)
 
 void pciehp_handle_button_press(struct controller *ctrl)
 {
+	struct pci_dev *pdev = ctrl->pcie->port;
+
 	mutex_lock(&ctrl->state_lock);
 	switch (ctrl->state) {
 	case OFF_STATE:
 	case ON_STATE:
 		if (ctrl->state == ON_STATE) {
 			ctrl->state = BLINKINGOFF_STATE;
-			ctrl_info(ctrl, "Slot(%s): Powering off due to button press\n",
-				  slot_name(ctrl));
+			pci_info(pdev, "Slot(%s): Powering off due to button press\n",
+				 slot_name(ctrl));
 		} else {
 			ctrl->state = BLINKINGON_STATE;
-			ctrl_info(ctrl, "Slot(%s) Powering on due to button press\n",
-				  slot_name(ctrl));
+			pci_info(pdev, "Slot(%s) Powering on due to button press\n",
+				 slot_name(ctrl));
 		}
 		/* blink green LED and turn off amber */
 		pciehp_green_led_blink(ctrl);
@@ -177,7 +180,7 @@ void pciehp_handle_button_press(struct controller *ctrl)
 		 * press the attention again before the 5 sec. limit
 		 * expires to cancel hot-add or hot-remove
 		 */
-		ctrl_info(ctrl, "Slot(%s): Button cancel\n", slot_name(ctrl));
+		pci_info(pdev, "Slot(%s): Button cancel\n", slot_name(ctrl));
 		cancel_delayed_work(&ctrl->button_work);
 		if (ctrl->state == BLINKINGOFF_STATE) {
 			ctrl->state = ON_STATE;
@@ -187,12 +190,12 @@ void pciehp_handle_button_press(struct controller *ctrl)
 			pciehp_green_led_off(ctrl);
 		}
 		pciehp_set_attention_status(ctrl, 0);
-		ctrl_info(ctrl, "Slot(%s): Action canceled due to button press\n",
-			  slot_name(ctrl));
+		pci_info(pdev, "Slot(%s): Action canceled due to button press\n",
+			 slot_name(ctrl));
 		break;
 	default:
-		ctrl_err(ctrl, "Slot(%s): Ignoring invalid state %#x\n",
-			 slot_name(ctrl), ctrl->state);
+		pci_err(pdev, "Slot(%s): Ignoring invalid state %#x\n",
+			slot_name(ctrl), ctrl->state);
 		break;
 	}
 	mutex_unlock(&ctrl->state_lock);
@@ -216,6 +219,7 @@ void pciehp_handle_disable_request(struct controller *ctrl)
 void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
 {
 	bool present, link_active;
+	struct pci_dev *pdev = ctrl->pcie->port;
 
 	/*
 	 * If the slot is on and presence or link has changed, turn it off.
@@ -230,11 +234,11 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
 		ctrl->state = POWEROFF_STATE;
 		mutex_unlock(&ctrl->state_lock);
 		if (events & PCI_EXP_SLTSTA_DLLSC)
-			ctrl_info(ctrl, "Slot(%s): Link Down\n",
-				  slot_name(ctrl));
+			pci_info(pdev, "Slot(%s): Link Down\n",
+				 slot_name(ctrl));
 		if (events & PCI_EXP_SLTSTA_PDC)
-			ctrl_info(ctrl, "Slot(%s): Card not present\n",
-				  slot_name(ctrl));
+			pci_info(pdev, "Slot(%s): Card not present\n",
+				 slot_name(ctrl));
 		pciehp_disable_slot(ctrl, SURPRISE_REMOVAL);
 		break;
 	default:
@@ -259,11 +263,11 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
 		ctrl->state = POWERON_STATE;
 		mutex_unlock(&ctrl->state_lock);
 		if (present)
-			ctrl_info(ctrl, "Slot(%s): Card present\n",
-				  slot_name(ctrl));
+			pci_info(pdev, "Slot(%s): Card present\n",
+				 slot_name(ctrl));
 		if (link_active)
-			ctrl_info(ctrl, "Slot(%s): Link Up\n",
-				  slot_name(ctrl));
+			pci_info(pdev, "Slot(%s): Link Up\n",
+				 slot_name(ctrl));
 		ctrl->request_result = pciehp_enable_slot(ctrl);
 		break;
 	default:
@@ -274,13 +278,14 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
 
 static int __pciehp_enable_slot(struct controller *ctrl)
 {
+	struct pci_dev *pdev = ctrl->pcie->port;
 	u8 getstatus = 0;
 
 	if (MRL_SENS(ctrl)) {
 		pciehp_get_latch_status(ctrl, &getstatus);
 		if (getstatus) {
-			ctrl_info(ctrl, "Slot(%s): Latch open\n",
-				  slot_name(ctrl));
+			pci_info(pdev, "Slot(%s): Latch open\n",
+				 slot_name(ctrl));
 			return -ENODEV;
 		}
 	}
@@ -288,8 +293,8 @@ static int __pciehp_enable_slot(struct controller *ctrl)
 	if (POWER_CTRL(ctrl)) {
 		pciehp_get_power_status(ctrl, &getstatus);
 		if (getstatus) {
-			ctrl_info(ctrl, "Slot(%s): Already enabled\n",
-				  slot_name(ctrl));
+			pci_info(pdev, "Slot(%s): Already enabled\n",
+				 slot_name(ctrl));
 			return 0;
 		}
 	}
@@ -316,13 +321,14 @@ static int pciehp_enable_slot(struct controller *ctrl)
 
 static int __pciehp_disable_slot(struct controller *ctrl, bool safe_removal)
 {
+	struct pci_dev *pdev = ctrl->pcie->port;
 	u8 getstatus = 0;
 
 	if (POWER_CTRL(ctrl)) {
 		pciehp_get_power_status(ctrl, &getstatus);
 		if (!getstatus) {
-			ctrl_info(ctrl, "Slot(%s): Already disabled\n",
-				  slot_name(ctrl));
+			pci_info(pdev, "Slot(%s): Already disabled\n",
+				 slot_name(ctrl));
 			return -EINVAL;
 		}
 	}
@@ -349,6 +355,7 @@ static int pciehp_disable_slot(struct controller *ctrl, bool safe_removal)
 int pciehp_sysfs_enable_slot(struct hotplug_slot *hotplug_slot)
 {
 	struct controller *ctrl = to_ctrl(hotplug_slot);
+	struct pci_dev *pdev = ctrl->pcie->port;
 
 	mutex_lock(&ctrl->state_lock);
 	switch (ctrl->state) {
@@ -365,18 +372,18 @@ int pciehp_sysfs_enable_slot(struct hotplug_slot *hotplug_slot)
 			   !atomic_read(&ctrl->pending_events));
 		return ctrl->request_result;
 	case POWERON_STATE:
-		ctrl_info(ctrl, "Slot(%s): Already in powering on state\n",
-			  slot_name(ctrl));
+		pci_info(pdev, "Slot(%s): Already in powering on state\n",
+			 slot_name(ctrl));
 		break;
 	case BLINKINGOFF_STATE:
 	case ON_STATE:
 	case POWEROFF_STATE:
-		ctrl_info(ctrl, "Slot(%s): Already enabled\n",
-			  slot_name(ctrl));
+		pci_info(pdev, "Slot(%s): Already enabled\n",
+			 slot_name(ctrl));
 		break;
 	default:
-		ctrl_err(ctrl, "Slot(%s): Invalid state %#x\n",
-			 slot_name(ctrl), ctrl->state);
+		pci_err(pdev, "Slot(%s): Invalid state %#x\n",
+			slot_name(ctrl), ctrl->state);
 		break;
 	}
 	mutex_unlock(&ctrl->state_lock);
@@ -387,6 +394,7 @@ int pciehp_sysfs_enable_slot(struct hotplug_slot *hotplug_slot)
 int pciehp_sysfs_disable_slot(struct hotplug_slot *hotplug_slot)
 {
 	struct controller *ctrl = to_ctrl(hotplug_slot);
+	struct pci_dev *pdev = ctrl->pcie->port;
 
 	mutex_lock(&ctrl->state_lock);
 	switch (ctrl->state) {
@@ -398,18 +406,18 @@ int pciehp_sysfs_disable_slot(struct hotplug_slot *hotplug_slot)
 			   !atomic_read(&ctrl->pending_events));
 		return ctrl->request_result;
 	case POWEROFF_STATE:
-		ctrl_info(ctrl, "Slot(%s): Already in powering off state\n",
-			  slot_name(ctrl));
+		pci_info(pdev, "Slot(%s): Already in powering off state\n",
+			 slot_name(ctrl));
 		break;
 	case BLINKINGON_STATE:
 	case OFF_STATE:
 	case POWERON_STATE:
-		ctrl_info(ctrl, "Slot(%s): Already disabled\n",
-			  slot_name(ctrl));
+		pci_info(pdev, "Slot(%s): Already disabled\n",
+			 slot_name(ctrl));
 		break;
 	default:
-		ctrl_err(ctrl, "Slot(%s): Invalid state %#x\n",
-			 slot_name(ctrl), ctrl->state);
+		pci_err(pdev, "Slot(%s): Invalid state %#x\n",
+			slot_name(ctrl), ctrl->state);
 		break;
 	}
 	mutex_unlock(&ctrl->state_lock);
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 6a2365cd794e..5e5631fd0171 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -36,6 +36,7 @@ static int pciehp_poll(void *data);
 static inline int pciehp_request_irq(struct controller *ctrl)
 {
 	int retval, irq = ctrl->pcie->irq;
+	struct pci_dev *pdev = ctrl_dev(ctrl);
 
 	if (pciehp_poll_mode) {
 		ctrl->poll_thread = kthread_run(&pciehp_poll, ctrl,
@@ -48,8 +49,8 @@ static inline int pciehp_request_irq(struct controller *ctrl)
 	retval = request_threaded_irq(irq, pciehp_isr, pciehp_ist,
 				      IRQF_SHARED, MY_NAME, ctrl);
 	if (retval)
-		ctrl_err(ctrl, "Cannot get irq %d for the hotplug controller\n",
-			 irq);
+		pci_err(pdev, "Cannot get irq %d for the hotplug controller\n",
+			irq);
 	return retval;
 }
 
@@ -69,8 +70,8 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout)
 	while (true) {
 		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
 		if (slot_status == (u16) ~0) {
-			ctrl_info(ctrl, "%s: no response from device\n",
-				  __func__);
+			pci_info(pdev, "%s: no response from device\n",
+				 __func__);
 			return 0;
 		}
 
@@ -89,6 +90,7 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout)
 
 static void pcie_wait_cmd(struct controller *ctrl)
 {
+	struct pci_dev *pdev = ctrl_dev(ctrl);
 	unsigned int msecs = pciehp_poll_mode ? 2500 : 1000;
 	unsigned long duration = msecs_to_jiffies(msecs);
 	unsigned long cmd_timeout = ctrl->cmd_started + duration;
@@ -122,9 +124,9 @@ static void pcie_wait_cmd(struct controller *ctrl)
 		rc = pcie_poll_cmd(ctrl, jiffies_to_msecs(timeout));
 
 	if (!rc)
-		ctrl_info(ctrl, "Timeout on hotplug command %#06x (issued %u msec ago)\n",
-			  ctrl->slot_ctrl,
-			  jiffies_to_msecs(jiffies - ctrl->cmd_started));
+		pci_info(pdev, "Timeout on hotplug command %#06x (issued %u msec ago)\n",
+			 ctrl->slot_ctrl,
+			 jiffies_to_msecs(jiffies - ctrl->cmd_started));
 }
 
 #define CC_ERRATUM_MASK		(PCI_EXP_SLTCTL_PCC |	\
@@ -147,7 +149,7 @@ static void pcie_do_write_cmd(struct controller *ctrl, u16 cmd,
 
 	pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
 	if (slot_ctrl == (u16) ~0) {
-		ctrl_info(ctrl, "%s: no response from device\n", __func__);
+		pci_info(pdev, "%s: no response from device\n", __func__);
 		goto out;
 	}
 
@@ -209,7 +211,7 @@ bool pciehp_check_link_active(struct controller *ctrl)
 	ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
 
 	if (ret)
-		ctrl_dbg(ctrl, "%s: lnk_status = %x\n", __func__, lnk_status);
+		pci_dbg(pdev, "%s: lnk_status = %x\n", __func__, lnk_status);
 
 	return ret;
 }
@@ -233,9 +235,9 @@ static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
 	} while (delay > 0);
 
 	if (count > 1 && pciehp_debug)
-		printk(KERN_DEBUG "pci %04x:%02x:%02x.%d id reading try %d times with interval %d ms to get %08x\n",
-			pci_domain_nr(bus), bus->number, PCI_SLOT(devfn),
-			PCI_FUNC(devfn), count, step, l);
+		pr_debug("pci %04x:%02x:%02x.%d id reading try %d times with interval %d ms to get %08x\n",
+			 pci_domain_nr(bus), bus->number, PCI_SLOT(devfn),
+			 PCI_FUNC(devfn), count, step, l);
 
 	return found;
 }
@@ -258,11 +260,11 @@ int pciehp_check_link_status(struct controller *ctrl)
 			   &ctrl->pending_events);
 
 	pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
-	ctrl_dbg(ctrl, "%s: lnk_status = %x\n", __func__, lnk_status);
+	pci_dbg(pdev, "%s: lnk_status = %x\n", __func__, lnk_status);
 	if ((lnk_status & PCI_EXP_LNKSTA_LT) ||
 	    !(lnk_status & PCI_EXP_LNKSTA_NLW)) {
-		ctrl_err(ctrl, "link training error: status %#06x\n",
-			 lnk_status);
+		pci_err(pdev, "link training error: status %#06x\n",
+			lnk_status);
 		return -1;
 	}
 
@@ -287,7 +289,7 @@ static int __pciehp_link_set(struct controller *ctrl, bool enable)
 		lnk_ctrl |= PCI_EXP_LNKCTL_LD;
 
 	pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, lnk_ctrl);
-	ctrl_dbg(ctrl, "%s: lnk_ctrl = %x\n", __func__, lnk_ctrl);
+	pci_dbg(pdev, "%s: lnk_ctrl = %x\n", __func__, lnk_ctrl);
 	return 0;
 }
 
@@ -319,8 +321,8 @@ int pciehp_get_attention_status(struct hotplug_slot *hotplug_slot, u8 *status)
 	pci_config_pm_runtime_get(pdev);
 	pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
 	pci_config_pm_runtime_put(pdev);
-	ctrl_dbg(ctrl, "%s: SLOTCTRL %x, value read %x\n", __func__,
-		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, slot_ctrl);
+	pci_dbg(pdev, "%s: SLOTCTRL %x, value read %x\n", __func__,
+		pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, slot_ctrl);
 
 	switch (slot_ctrl & PCI_EXP_SLTCTL_AIC) {
 	case PCI_EXP_SLTCTL_ATTN_IND_ON:
@@ -346,8 +348,8 @@ void pciehp_get_power_status(struct controller *ctrl, u8 *status)
 	u16 slot_ctrl;
 
 	pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
-	ctrl_dbg(ctrl, "%s: SLOTCTRL %x value read %x\n", __func__,
-		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, slot_ctrl);
+	pci_dbg(pdev, "%s: SLOTCTRL %x value read %x\n", __func__,
+		pci_pcie_cap(pdev) + PCI_EXP_SLTCTL, slot_ctrl);
 
 	switch (slot_ctrl & PCI_EXP_SLTCTL_PCC) {
 	case PCI_EXP_SLTCTL_PWR_ON:
@@ -418,6 +420,7 @@ int pciehp_set_raw_indicator_status(struct hotplug_slot *hotplug_slot,
 
 void pciehp_set_attention_status(struct controller *ctrl, u8 value)
 {
+	struct pci_dev *pdev = ctrl_dev(ctrl);
 	u16 slot_cmd;
 
 	if (!ATTN_LED(ctrl))
@@ -437,44 +440,50 @@ void pciehp_set_attention_status(struct controller *ctrl, u8 value)
 		return;
 	}
 	pcie_write_cmd_nowait(ctrl, slot_cmd, PCI_EXP_SLTCTL_AIC);
-	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
-		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, slot_cmd);
+	pci_dbg(pdev, "%s: SLOTCTRL %x write cmd %x\n", __func__,
+		pci_pcie_cap(pdev) + PCI_EXP_SLTCTL, slot_cmd);
 }
 
 void pciehp_green_led_on(struct controller *ctrl)
 {
+	struct pci_dev *pdev = ctrl_dev(ctrl);
+
 	if (!PWR_LED(ctrl))
 		return;
 
 	pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_PWR_IND_ON,
 			      PCI_EXP_SLTCTL_PIC);
-	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
-		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL,
-		 PCI_EXP_SLTCTL_PWR_IND_ON);
+	pci_dbg(pdev, "%s: SLOTCTRL %x write cmd %x\n", __func__,
+		pci_pcie_cap(pdev) + PCI_EXP_SLTCTL,
+		PCI_EXP_SLTCTL_PWR_IND_ON);
 }
 
 void pciehp_green_led_off(struct controller *ctrl)
 {
+	struct pci_dev *pdev = ctrl_dev(ctrl);
+
 	if (!PWR_LED(ctrl))
 		return;
 
 	pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
 			      PCI_EXP_SLTCTL_PIC);
-	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
-		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL,
-		 PCI_EXP_SLTCTL_PWR_IND_OFF);
+	pci_dbg(pdev, "%s: SLOTCTRL %x write cmd %x\n", __func__,
+		pci_pcie_cap(pdev) + PCI_EXP_SLTCTL,
+		PCI_EXP_SLTCTL_PWR_IND_OFF);
 }
 
 void pciehp_green_led_blink(struct controller *ctrl)
 {
+	struct pci_dev *pdev = ctrl_dev(ctrl);
+
 	if (!PWR_LED(ctrl))
 		return;
 
 	pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_PWR_IND_BLINK,
 			      PCI_EXP_SLTCTL_PIC);
-	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
-		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL,
-		 PCI_EXP_SLTCTL_PWR_IND_BLINK);
+	pci_dbg(pdev, "%s: SLOTCTRL %x write cmd %x\n", __func__,
+		pci_pcie_cap(pdev) + PCI_EXP_SLTCTL,
+		PCI_EXP_SLTCTL_PWR_IND_BLINK);
 }
 
 int pciehp_power_on_slot(struct controller *ctrl)
@@ -491,23 +500,25 @@ int pciehp_power_on_slot(struct controller *ctrl)
 	ctrl->power_fault_detected = 0;
 
 	pcie_write_cmd(ctrl, PCI_EXP_SLTCTL_PWR_ON, PCI_EXP_SLTCTL_PCC);
-	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
-		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL,
-		 PCI_EXP_SLTCTL_PWR_ON);
+	pci_dbg(pdev, "%s: SLOTCTRL %x write cmd %x\n", __func__,
+		pci_pcie_cap(pdev) + PCI_EXP_SLTCTL,
+		PCI_EXP_SLTCTL_PWR_ON);
 
 	retval = pciehp_link_enable(ctrl);
 	if (retval)
-		ctrl_err(ctrl, "%s: Can not enable the link!\n", __func__);
+		pci_err(pdev, "%s: Can not enable the link!\n", __func__);
 
 	return retval;
 }
 
 void pciehp_power_off_slot(struct controller *ctrl)
 {
+	struct pci_dev *pdev = ctrl_dev(ctrl);
+
 	pcie_write_cmd(ctrl, PCI_EXP_SLTCTL_PWR_OFF, PCI_EXP_SLTCTL_PCC);
-	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
-		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL,
-		 PCI_EXP_SLTCTL_PWR_OFF);
+	pci_dbg(pdev, "%s: SLOTCTRL %x write cmd %x\n", __func__,
+		pci_pcie_cap(pdev) + PCI_EXP_SLTCTL,
+		PCI_EXP_SLTCTL_PWR_OFF);
 }
 
 static irqreturn_t pciehp_isr(int irq, void *dev_id)
@@ -542,7 +553,7 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
 
 	pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &status);
 	if (status == (u16) ~0) {
-		ctrl_info(ctrl, "%s: no response from device\n", __func__);
+		pci_info(pdev, "%s: no response from device\n", __func__);
 		if (parent)
 			pm_runtime_put(parent);
 		return IRQ_NONE;
@@ -570,7 +581,7 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
 	}
 
 	pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, events);
-	ctrl_dbg(ctrl, "pending interrupts %#06x from Slot Status\n", events);
+	pci_dbg(pdev, "pending interrupts %#06x from Slot Status\n", events);
 	if (parent)
 		pm_runtime_put(parent);
 
@@ -590,7 +601,7 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
 	}
 
 	if (pdev->ignore_hotplug) {
-		ctrl_dbg(ctrl, "ignoring hotplug event %#06x\n", events);
+		pci_dbg(pdev, "ignoring hotplug event %#06x\n", events);
 		return IRQ_HANDLED;
 	}
 
@@ -627,15 +638,15 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
 
 	/* Check Attention Button Pressed */
 	if (events & PCI_EXP_SLTSTA_ABP) {
-		ctrl_info(ctrl, "Slot(%s): Attention button pressed\n",
-			  slot_name(ctrl));
+		pci_info(pdev, "Slot(%s): Attention button pressed\n",
+			 slot_name(ctrl));
 		pciehp_handle_button_press(ctrl);
 	}
 
 	/* Check Power Fault Detected */
 	if ((events & PCI_EXP_SLTSTA_PFD) && !ctrl->power_fault_detected) {
 		ctrl->power_fault_detected = 1;
-		ctrl_err(ctrl, "Slot(%s): Power fault\n", slot_name(ctrl));
+		pci_err(pdev, "Slot(%s): Power fault\n", slot_name(ctrl));
 		pciehp_set_attention_status(ctrl, 1);
 		pciehp_green_led_off(ctrl);
 	}
@@ -679,6 +690,7 @@ static int pciehp_poll(void *data)
 
 static void pcie_enable_notification(struct controller *ctrl)
 {
+	struct pci_dev *pdev = ctrl_dev(ctrl);
 	u16 cmd, mask;
 
 	/*
@@ -711,12 +723,13 @@ static void pcie_enable_notification(struct controller *ctrl)
 		PCI_EXP_SLTCTL_DLLSCE);
 
 	pcie_write_cmd_nowait(ctrl, cmd, mask);
-	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
-		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, cmd);
+	pci_dbg(pdev, "%s: SLOTCTRL %x write cmd %x\n", __func__,
+		pci_pcie_cap(pdev) + PCI_EXP_SLTCTL, cmd);
 }
 
 static void pcie_disable_notification(struct controller *ctrl)
 {
+	struct pci_dev *pdev = ctrl_dev(ctrl);
 	u16 mask;
 
 	mask = (PCI_EXP_SLTCTL_PDCE | PCI_EXP_SLTCTL_ABPE |
@@ -724,8 +737,8 @@ static void pcie_disable_notification(struct controller *ctrl)
 		PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_CCIE |
 		PCI_EXP_SLTCTL_DLLSCE);
 	pcie_write_cmd(ctrl, 0, mask);
-	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
-		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, 0);
+	pci_dbg(pdev, "%s: SLOTCTRL %x write cmd %x\n", __func__,
+		pci_pcie_cap(pdev) + PCI_EXP_SLTCTL, 0);
 }
 
 void pcie_clear_hotplug_events(struct controller *ctrl)
@@ -785,15 +798,15 @@ int pciehp_reset_slot(struct hotplug_slot *hotplug_slot, int probe)
 	stat_mask |= PCI_EXP_SLTSTA_DLLSC;
 
 	pcie_write_cmd(ctrl, 0, ctrl_mask);
-	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
-		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, 0);
+	pci_dbg(pdev, "%s: SLOTCTRL %x write cmd %x\n", __func__,
+		pci_pcie_cap(pdev) + PCI_EXP_SLTCTL, 0);
 
 	rc = pci_bridge_secondary_bus_reset(ctrl->pcie->port);
 
 	pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, stat_mask);
 	pcie_write_cmd_nowait(ctrl, ctrl_mask, ctrl_mask);
-	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
-		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, ctrl_mask);
+	pci_dbg(pdev, "%s: SLOTCTRL %x write cmd %x\n", __func__,
+		pci_pcie_cap(pdev) + PCI_EXP_SLTCTL, ctrl_mask);
 
 	up_write(&ctrl->reset_lock);
 	return rc;
@@ -825,11 +838,11 @@ static inline void dbg_ctrl(struct controller *ctrl)
 	if (!pciehp_debug)
 		return;
 
-	ctrl_info(ctrl, "Slot Capabilities      : 0x%08x\n", ctrl->slot_cap);
+	pci_info(pdev, "Slot Capabilities      : 0x%08x\n", ctrl->slot_cap);
 	pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &reg16);
-	ctrl_info(ctrl, "Slot Status            : 0x%04x\n", reg16);
+	pci_info(pdev, "Slot Status            : 0x%04x\n", reg16);
 	pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &reg16);
-	ctrl_info(ctrl, "Slot Control           : 0x%04x\n", reg16);
+	pci_info(pdev, "Slot Control           : 0x%04x\n", reg16);
 }
 
 #define FLAG(x, y)	(((x) & (y)) ? '+' : '-')
@@ -881,19 +894,19 @@ struct controller *pcie_init(struct pcie_device *dev)
 		PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_CC |
 		PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC);
 
-	ctrl_info(ctrl, "Slot #%d AttnBtn%c PwrCtrl%c MRL%c AttnInd%c PwrInd%c HotPlug%c Surprise%c Interlock%c NoCompl%c LLActRep%c%s\n",
-		(slot_cap & PCI_EXP_SLTCAP_PSN) >> 19,
-		FLAG(slot_cap, PCI_EXP_SLTCAP_ABP),
-		FLAG(slot_cap, PCI_EXP_SLTCAP_PCP),
-		FLAG(slot_cap, PCI_EXP_SLTCAP_MRLSP),
-		FLAG(slot_cap, PCI_EXP_SLTCAP_AIP),
-		FLAG(slot_cap, PCI_EXP_SLTCAP_PIP),
-		FLAG(slot_cap, PCI_EXP_SLTCAP_HPC),
-		FLAG(slot_cap, PCI_EXP_SLTCAP_HPS),
-		FLAG(slot_cap, PCI_EXP_SLTCAP_EIP),
-		FLAG(slot_cap, PCI_EXP_SLTCAP_NCCS),
-		FLAG(link_cap, PCI_EXP_LNKCAP_DLLLARC),
-		pdev->broken_cmd_compl ? " (with Cmd Compl erratum)" : "");
+	pci_info(pdev, "Slot #%d AttnBtn%c PwrCtrl%c MRL%c AttnInd%c PwrInd%c HotPlug%c Surprise%c Interlock%c NoCompl%c LLActRep%c%s\n",
+		 (slot_cap & PCI_EXP_SLTCAP_PSN) >> 19,
+		 FLAG(slot_cap, PCI_EXP_SLTCAP_ABP),
+		 FLAG(slot_cap, PCI_EXP_SLTCAP_PCP),
+		 FLAG(slot_cap, PCI_EXP_SLTCAP_MRLSP),
+		 FLAG(slot_cap, PCI_EXP_SLTCAP_AIP),
+		 FLAG(slot_cap, PCI_EXP_SLTCAP_PIP),
+		 FLAG(slot_cap, PCI_EXP_SLTCAP_HPC),
+		 FLAG(slot_cap, PCI_EXP_SLTCAP_HPS),
+		 FLAG(slot_cap, PCI_EXP_SLTCAP_EIP),
+		 FLAG(slot_cap, PCI_EXP_SLTCAP_NCCS),
+		 FLAG(link_cap, PCI_EXP_LNKCAP_DLLLARC),
+		 pdev->broken_cmd_compl ? " (with Cmd Compl erratum)" : "");
 
 	/*
 	 * If empty slot's power status is on, turn power off.  The IRQ isn't
diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
index b9c1396db6fe..55967ce567f6 100644
--- a/drivers/pci/hotplug/pciehp_pci.c
+++ b/drivers/pci/hotplug/pciehp_pci.c
@@ -42,8 +42,8 @@ int pciehp_configure_device(struct controller *ctrl)
 		 * The device is already there. Either configured by the
 		 * boot firmware or a previous hotplug event.
 		 */
-		ctrl_dbg(ctrl, "Device %s already exists at %04x:%02x:00, skipping hot-add\n",
-			 pci_name(dev), pci_domain_nr(parent), parent->number);
+		pci_dbg(bridge, "Device %s already exists at %04x:%02x:00, skipping hot-add\n",
+			pci_name(dev), pci_domain_nr(parent), parent->number);
 		pci_dev_put(dev);
 		ret = -EEXIST;
 		goto out;
@@ -51,7 +51,7 @@ int pciehp_configure_device(struct controller *ctrl)
 
 	num = pci_scan_slot(parent, PCI_DEVFN(0, 0));
 	if (num == 0) {
-		ctrl_err(ctrl, "No new device found\n");
+		pci_err(bridge, "No new device found\n");
 		ret = -ENODEV;
 		goto out;
 	}
@@ -85,8 +85,8 @@ void pciehp_unconfigure_device(struct controller *ctrl, bool presence)
 	struct pci_bus *parent = ctrl->pcie->port->subordinate;
 	u16 command;
 
-	ctrl_dbg(ctrl, "%s: domain:bus:dev = %04x:%02x:00\n",
-		 __func__, pci_domain_nr(parent), parent->number);
+	pci_dbg(ctrl->pcie->port, "%s: domain:bus:dev = %04x:%02x:00\n",
+		__func__, pci_domain_nr(parent), parent->number);
 
 	if (!presence)
 		pci_walk_bus(parent, pci_dev_set_disconnected, NULL);
-- 
2.17.1

Re: [PATCH 2/4] PCI: pciehp: Replace ctrl_*() with pci_*()

[Lukas Wunner]

On Sat, Apr 27, 2019 at 02:13:02PM -0500, fred@fredlawl.com wrote:
> Hotplug useses custom ctrl_*() dev_*() printk wrappers for logging
> messages. To make hotplug conform to pci logging, replace uses of these
> wrappers with pci_*() printk wrappers. In addition, replace any
> printk() calls with pr_*() wrappers.

A lot of pciehp's messages are preceded by "Slot(%s): ", where %s is
replaced by the Physical Slot Number in the Slot Capabilities register
(which is cached in struct controller) plus an optional suffix if the
same PSN is used by multiple slots.  For some reason (probably a
historic artefact), this prefix is included only in *some* of the
messages.

I think it would be useful to make the messages consistent by *always*
including the "Slot(%s): " prefix.  However the prefix is unknown until
pci_hp_initialize() has been called.  I'd solve this by keeping the
ctrl_*() wrappers and amending them to print the "Slot(%s): " prefix,
then making sure that ctrl_*() is not called before pci_hp_initialize().
(pci_*() has to be used instead).


> @@ -182,39 +184,39 @@ static int pciehp_probe(struct pcie_device *dev)
>  
>  	if (!dev->port->subordinate) {
>  		/* Can happen if we run out of bus numbers during probe */
> -		dev_err(&dev->device,
> -			"Hotplug bridge without secondary bus, ignoring\n");
> +		pci_err(dev->port, "Hotplug bridge without secondary bus, ignoring\n");

Hm, the string was likely deliberately put on a new line to avoid
exceeding 80 chars, so I'd keep it that way.

Thanks,

Lukas

Re: [PATCH 2/4] PCI: pciehp: Replace ctrl_*() with pci_*()

[Bjorn Helgaas]

On Sat, Apr 27, 2019 at 10:03:01PM +0200, Lukas Wunner wrote:
> On Sat, Apr 27, 2019 at 02:13:02PM -0500, fred@fredlawl.com wrote:
> > Hotplug useses custom ctrl_*() dev_*() printk wrappers for logging
> > messages. To make hotplug conform to pci logging, replace uses of these
> > wrappers with pci_*() printk wrappers. In addition, replace any
> > printk() calls with pr_*() wrappers.
> 
> A lot of pciehp's messages are preceded by "Slot(%s): ", where %s is
> replaced by the Physical Slot Number in the Slot Capabilities register
> (which is cached in struct controller) plus an optional suffix if the
> same PSN is used by multiple slots.  For some reason (probably a
> historic artefact), this prefix is included only in *some* of the
> messages.
> 
> I think it would be useful to make the messages consistent by *always*
> including the "Slot(%s): " prefix.  However the prefix is unknown until
> pci_hp_initialize() has been called.  I'd solve this by keeping the
> ctrl_*() wrappers and amending them to print the "Slot(%s): " prefix,
> then making sure that ctrl_*() is not called before pci_hp_initialize().
> (pci_*() has to be used instead).

I was hoping to get rid of the ctrl_*() wrappers completely, but the
"Slot(%s)" prefix is actually a pretty good reason to keep them.

Moving the prefix from the call site to the ctrl_*() wrappers should be a
separate patch that doesn't change the output at all (except maybe
adding the prefix to messages that don't currently include it).

So you probably need three steps (each in a separate patch):

  1) Convert ctrl_*() to use pci_dev instead of pcie_device, something
     like this:

        + #define pr_fmt(fmt) "pciehp: " fmt
	+ #define dev_fmt pr_fmt

	  #define ctrl_info(ctrl, format, arg...) \
	-   dev_info(&ctrl->pcie->device, format, ## arg)
	+   pci_info(&ctrl->pcie->port, format, ## arg)

  2) Convert any output before pci_hp_initialize() from ctrl_*() to
     pci_*().

  3) Centralize the "Slot(%s): " prefix, something like this:

	  #define ctrl_info(ctrl, format, arg...) \
	-   pci_info(&ctrl->pcie->port, format, ## arg)
	+   pci_info(&ctrl->pcie->port, "Slot(%s): " format, slot_name(ctrl), ## arg)

	- ctrl_info(ctrl, "Slot(%s): ...", slot_name(ctrl));
	+ ctrl_info(ctrl, "...");

[PATCH 3/4] PCI: pciehp: Remove unused macro definitions

[fred]

From: Frederick Lawler <fred@fredlawl.com>

Now that all uses for the ctrl_*() printk wrappers are removed from
files and replaces with pci_*() or pr_*() printk wrappers, remove the
unused macro definitions. In addition to that, remove the MY_NAME macro.

Signed-off-by: Frederick Lawler <fred@fredlawl.com>
---
 drivers/pci/hotplug/pciehp.h     | 27 ---------------------------
 drivers/pci/hotplug/pciehp_hpc.c |  2 +-
 2 files changed, 1 insertion(+), 28 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 506e1d923a1f..7d3a32a1504a 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -25,37 +25,10 @@
 
 #include "../pcie/portdrv.h"
 
-#define MY_NAME	"pciehp"
-
 extern bool pciehp_poll_mode;
 extern int pciehp_poll_time;
 extern bool pciehp_debug;
 
-#define dbg(format, arg...)						\
-do {									\
-	if (pciehp_debug)						\
-		printk(KERN_DEBUG "%s: " format, MY_NAME, ## arg);	\
-} while (0)
-#define err(format, arg...)						\
-	printk(KERN_ERR "%s: " format, MY_NAME, ## arg)
-#define info(format, arg...)						\
-	printk(KERN_INFO "%s: " format, MY_NAME, ## arg)
-#define warn(format, arg...)						\
-	printk(KERN_WARNING "%s: " format, MY_NAME, ## arg)
-
-#define ctrl_dbg(ctrl, format, arg...)					\
-	do {								\
-		if (pciehp_debug)					\
-			dev_printk(KERN_DEBUG, &ctrl->pcie->device,	\
-					format, ## arg);		\
-	} while (0)
-#define ctrl_err(ctrl, format, arg...)					\
-	dev_err(&ctrl->pcie->device, format, ## arg)
-#define ctrl_info(ctrl, format, arg...)					\
-	dev_info(&ctrl->pcie->device, format, ## arg)
-#define ctrl_warn(ctrl, format, arg...)					\
-	dev_warn(&ctrl->pcie->device, format, ## arg)
-
 #define SLOT_NAME_SIZE 10
 
 /**
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 5e5631fd0171..28a132a0d9db 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -47,7 +47,7 @@ static inline int pciehp_request_irq(struct controller *ctrl)
 
 	/* Installs the interrupt handler */
 	retval = request_threaded_irq(irq, pciehp_isr, pciehp_ist,
-				      IRQF_SHARED, MY_NAME, ctrl);
+				      IRQF_SHARED, "pciehp", ctrl);
 	if (retval)
 		pci_err(pdev, "Cannot get irq %d for the hotplug controller\n",
 			irq);
-- 
2.17.1

Re: [PATCH 3/4] PCI: pciehp: Remove unused macro definitions

[Andy Shevchenko]

On Sat, Apr 27, 2019 at 02:13:03PM -0500, fred@fredlawl.com wrote:
> Now that all uses for the ctrl_*() printk wrappers are removed from
> files and replaces with pci_*() or pr_*() printk wrappers, remove the
> unused macro definitions. In addition to that, remove the MY_NAME macro.

>  extern bool pciehp_debug;

How it's used after all?

> -#define dbg(format, arg...)						\
> -do {									\
> -	if (pciehp_debug)						\
> -		printk(KERN_DEBUG "%s: " format, MY_NAME, ## arg);	\
> -} while (0)

> -#define ctrl_dbg(ctrl, format, arg...)					\
> -	do {								\
> -		if (pciehp_debug)					\
> -			dev_printk(KERN_DEBUG, &ctrl->pcie->device,	\
> -					format, ## arg);		\
> -	} while (0)

Besides ruining the pciehp_debug support this will make unequivalent behaviour.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] PCI: pciehp: Remove unused macro definitions

[Bjorn Helgaas]

On Sun, Apr 28, 2019 at 06:55:36PM +0300, Andy Shevchenko wrote:
> On Sat, Apr 27, 2019 at 02:13:03PM -0500, fred@fredlawl.com wrote:
> > Now that all uses for the ctrl_*() printk wrappers are removed from
> > files and replaces with pci_*() or pr_*() printk wrappers, remove the
> > unused macro definitions. In addition to that, remove the MY_NAME macro.
> 
> >  extern bool pciehp_debug;
> 
> How it's used after all?
> 
> > -#define dbg(format, arg...)						\
> > -do {									\
> > -	if (pciehp_debug)						\
> > -		printk(KERN_DEBUG "%s: " format, MY_NAME, ## arg);	\
> > -} while (0)
> 
> > -#define ctrl_dbg(ctrl, format, arg...)					\
> > -	do {								\
> > -		if (pciehp_debug)					\
> > -			dev_printk(KERN_DEBUG, &ctrl->pcie->device,	\
> > -					format, ## arg);		\
> > -	} while (0)
> 
> Besides ruining the pciehp_debug support this will make unequivalent behaviour.

I'm not super attached to pciehp_debug.  But perhaps pciehp is one
place where it would make sense to use pci_dbg().

There are a lot of uses of ctrl_dbg() and some of them look like
they're too low-level to just convert to pci_info(), e.g., info about
every command we write to the controller.  We probably don't need all
that info all the time.

But if we want to keep it, maybe we could convert it to use pci_dbg()
and the dynamic debug stuff.  I'm pretty sure the dyndbg syntax is
complicated enough to enable pciehp logging specifically.  Then we
could use that instead of the pciehp-specific module parameter.

Bjorn

[PATCH 4/4] PCI/portdrv: Add dev_fmt() to port drivers

[fred]

From: Frederick Lawler <fred@fredlawl.com>

Add dev_fmt() to port drivers.

Signed-off-by: Frederick Lawler <fred@fredlawl.com>
---
 drivers/pci/hotplug/pciehp_core.c  | 3 +++
 drivers/pci/hotplug/pciehp_ctrl.c  | 2 ++
 drivers/pci/hotplug/pciehp_hpc.c   | 3 +++
 drivers/pci/hotplug/pciehp_pci.c   | 2 ++
 drivers/pci/pcie/aer.c             | 3 +++
 drivers/pci/pcie/aer_inject.c      | 2 ++
 drivers/pci/pcie/bw_notification.c | 2 ++
 drivers/pci/pcie/dpc.c             | 2 ++
 drivers/pci/pcie/pme.c             | 2 ++
 9 files changed, 21 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 430a47556813..b07d713fd4bd 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -17,6 +17,9 @@
  *   Dely Sy <dely.l.sy@intel.com>"
  */
 
+#define pr_fmt(fmt) "pciehp: " fmt
+#define dev_fmt(fmt) pr_fmt(fmt)
+
 #include <linux/moduleparam.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 345c02b1e8d7..969a9c72f65d 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -13,6 +13,8 @@
  *
  */
 
+#define dev_fmt(fmt) "pciehp: " fmt
+
 #include <linux/kernel.h>
 #include <linux/types.h>
 #include <linux/pm_runtime.h>
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 28a132a0d9db..f2a3da105f5b 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -12,6 +12,9 @@
  * Send feedback to <greg@kroah.com>,<kristen.c.accardi@intel.com>
  */
 
+#define pr_fmt(fmt) "pciehp: " fmt
+#define dev_fmt(fmt) pr_fmt(fmt)
+
 #include <linux/kernel.h>
 #include <linux/types.h>
 #include <linux/jiffies.h>
diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
index 55967ce567f6..04ccd535aca7 100644
--- a/drivers/pci/hotplug/pciehp_pci.c
+++ b/drivers/pci/hotplug/pciehp_pci.c
@@ -13,6 +13,8 @@
  *
  */
 
+#define dev_fmt(fmt) "pciehp: " fmt
+
 #include <linux/kernel.h>
 #include <linux/types.h>
 #include <linux/pci.h>
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 224d878a28b4..6fd67285423d 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -12,6 +12,9 @@
  *    Andrew Patterson <andrew.patterson@hp.com>
  */
 
+#define pr_fmt(fmt) "AER: " fmt
+#define dev_fmt(fmt) pr_fmt(fmt)
+
 #include <linux/cper.h>
 #include <linux/pci.h>
 #include <linux/pci-acpi.h>
diff --git a/drivers/pci/pcie/aer_inject.c b/drivers/pci/pcie/aer_inject.c
index 610b617ae600..d4f6d49acd0c 100644
--- a/drivers/pci/pcie/aer_inject.c
+++ b/drivers/pci/pcie/aer_inject.c
@@ -12,6 +12,8 @@
  *     Huang Ying <ying.huang@intel.com>
  */
 
+#define dev_fmt(fmt) "AER: " fmt
+
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/irq.h>
diff --git a/drivers/pci/pcie/bw_notification.c b/drivers/pci/pcie/bw_notification.c
index d2eae3b7cc0f..a4bb90562cd5 100644
--- a/drivers/pci/pcie/bw_notification.c
+++ b/drivers/pci/pcie/bw_notification.c
@@ -14,6 +14,8 @@
  * and warns when links become degraded in operation.
  */
 
+#define dev_fmt(fmt) "BWN: " fmt
+
 #include "../pci.h"
 #include "portdrv.h"
 
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 72659286191b..b3c10cdc508a 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -6,6 +6,8 @@
  * Copyright (C) 2016 Intel Corp.
  */
 
+#define dev_fmt(fmt) "DPC: " fmt
+
 #include <linux/aer.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
index 54d593d10396..d6698423a6d6 100644
--- a/drivers/pci/pcie/pme.c
+++ b/drivers/pci/pcie/pme.c
@@ -7,6 +7,8 @@
  * Copyright (C) 2009 Rafael J. Wysocki <rjw@sisk.pl>, Novell Inc.
  */
 
+#define dev_fmt(fmt) "PME: " fmt
+
 #include <linux/pci.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
-- 
2.17.1

Re: [PATCH 4/4] PCI/portdrv: Add dev_fmt() to port drivers

[Bjorn Helgaas]

On Sat, Apr 27, 2019 at 02:13:04PM -0500, fred@fredlawl.com wrote:
> From: Frederick Lawler <fred@fredlawl.com>
> 
> Add dev_fmt() to port drivers.
> 
> Signed-off-by: Frederick Lawler <fred@fredlawl.com>
> ---
>  drivers/pci/hotplug/pciehp_core.c  | 3 +++
>  drivers/pci/hotplug/pciehp_ctrl.c  | 2 ++
>  drivers/pci/hotplug/pciehp_hpc.c   | 3 +++
>  drivers/pci/hotplug/pciehp_pci.c   | 2 ++
>  drivers/pci/pcie/aer.c             | 3 +++
>  drivers/pci/pcie/aer_inject.c      | 2 ++
>  drivers/pci/pcie/bw_notification.c | 2 ++
>  drivers/pci/pcie/dpc.c             | 2 ++
>  drivers/pci/pcie/pme.c             | 2 ++
>  9 files changed, 21 insertions(+)
> 
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index 430a47556813..b07d713fd4bd 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -17,6 +17,9 @@
>   *   Dely Sy <dely.l.sy@intel.com>"
>   */
>  
> +#define pr_fmt(fmt) "pciehp: " fmt
> +#define dev_fmt(fmt) pr_fmt(fmt)

This should be in the same patch that converts from using the pcie_device
to the pci_dev.  That way the "pciehp" that came from the pcie_device is
atomically replaced with the "pciehp" from pr_fmt()/dev_fmt().

If you do it in separate patches, there's an intermediate stage where
there's no prefix at all, and we want to avoid that.

BTW, in most cases you can simply do this, which is slightly simpler:

  #define dev_fmt pr_fmt

>  #include <linux/moduleparam.h>
>  #include <linux/kernel.h>
>  #include <linux/slab.h>
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 345c02b1e8d7..969a9c72f65d 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -13,6 +13,8 @@
>   *
>   */
>  
> +#define dev_fmt(fmt) "pciehp: " fmt
> +
>  #include <linux/kernel.h>
>  #include <linux/types.h>
>  #include <linux/pm_runtime.h>
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 28a132a0d9db..f2a3da105f5b 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -12,6 +12,9 @@
>   * Send feedback to <greg@kroah.com>,<kristen.c.accardi@intel.com>
>   */
>  
> +#define pr_fmt(fmt) "pciehp: " fmt
> +#define dev_fmt(fmt) pr_fmt(fmt)
> +
>  #include <linux/kernel.h>
>  #include <linux/types.h>
>  #include <linux/jiffies.h>
> diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
> index 55967ce567f6..04ccd535aca7 100644
> --- a/drivers/pci/hotplug/pciehp_pci.c
> +++ b/drivers/pci/hotplug/pciehp_pci.c
> @@ -13,6 +13,8 @@
>   *
>   */
>  
> +#define dev_fmt(fmt) "pciehp: " fmt
> +
>  #include <linux/kernel.h>
>  #include <linux/types.h>
>  #include <linux/pci.h>
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 224d878a28b4..6fd67285423d 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -12,6 +12,9 @@
>   *    Andrew Patterson <andrew.patterson@hp.com>
>   */
>  
> +#define pr_fmt(fmt) "AER: " fmt
> +#define dev_fmt(fmt) pr_fmt(fmt)
> +
>  #include <linux/cper.h>
>  #include <linux/pci.h>
>  #include <linux/pci-acpi.h>
> diff --git a/drivers/pci/pcie/aer_inject.c b/drivers/pci/pcie/aer_inject.c
> index 610b617ae600..d4f6d49acd0c 100644
> --- a/drivers/pci/pcie/aer_inject.c
> +++ b/drivers/pci/pcie/aer_inject.c
> @@ -12,6 +12,8 @@
>   *     Huang Ying <ying.huang@intel.com>
>   */
>  
> +#define dev_fmt(fmt) "AER: " fmt
> +
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/irq.h>
> diff --git a/drivers/pci/pcie/bw_notification.c b/drivers/pci/pcie/bw_notification.c
> index d2eae3b7cc0f..a4bb90562cd5 100644
> --- a/drivers/pci/pcie/bw_notification.c
> +++ b/drivers/pci/pcie/bw_notification.c
> @@ -14,6 +14,8 @@
>   * and warns when links become degraded in operation.
>   */
>  
> +#define dev_fmt(fmt) "BWN: " fmt
> +
>  #include "../pci.h"
>  #include "portdrv.h"
>  
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 72659286191b..b3c10cdc508a 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -6,6 +6,8 @@
>   * Copyright (C) 2016 Intel Corp.
>   */
>  
> +#define dev_fmt(fmt) "DPC: " fmt
> +
>  #include <linux/aer.h>
>  #include <linux/delay.h>
>  #include <linux/interrupt.h>
> diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
> index 54d593d10396..d6698423a6d6 100644
> --- a/drivers/pci/pcie/pme.c
> +++ b/drivers/pci/pcie/pme.c
> @@ -7,6 +7,8 @@
>   * Copyright (C) 2009 Rafael J. Wysocki <rjw@sisk.pl>, Novell Inc.
>   */
>  
> +#define dev_fmt(fmt) "PME: " fmt
> +
>  #include <linux/pci.h>
>  #include <linux/kernel.h>
>  #include <linux/errno.h>
> -- 
> 2.17.1
>