[PATCH 0/4] PCI: pci_printk() removal (+ related cleanups)

include/linux/pci.h provides pci_printk() which is a low-level
interface with level that is only useful for AER due to error severity
variations.

This series cleans up shpchp logging wrappers to avoid using low-level
pci_printk() unnecessarily and replaces pci_printk() with aer_printk().

Ilpo Järvinen (4):
  PCI: shpchp: Remove logging from module init/exit functions
  PCI: shpchp: Change dbg() -> ctrl_dbg()
  PCI: shpchp: Cleanup logging and debug wrappers
  PCI: Descope pci_printk() to aer_printk()

 drivers/pci/hotplug/shpchp.h      | 18 +-----------------
 drivers/pci/hotplug/shpchp_core.c | 13 +------------
 drivers/pci/hotplug/shpchp_hpc.c  |  2 +-
 drivers/pci/pcie/aer.c            | 10 +++++++---
 include/linux/pci.h               |  3 ---
 5 files changed, 10 insertions(+), 36 deletions(-)

-- 
2.39.5

The logging in shpchp module init/exit functions is not very useful.
Remove it.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/hotplug/shpchp_core.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
index a92e28b72908..a10ce7be7f51 100644
--- a/drivers/pci/hotplug/shpchp_core.c
+++ b/drivers/pci/hotplug/shpchp_core.c

@@ -324,20 +324,12 @@ static struct pci_driver shpc_driver = {
 
 static int __init shpcd_init(void)
 {
-	int retval;
-
-	retval = pci_register_driver(&shpc_driver);
-	dbg("%s: pci_register_driver = %d\n", __func__, retval);
-	info(DRIVER_DESC " version: " DRIVER_VERSION "\n");
-
-	return retval;
+	return pci_register_driver(&shpc_driver);
 }
 
 static void __exit shpcd_cleanup(void)
 {
-	dbg("unload_shpchpd()\n");
 	pci_unregister_driver(&shpc_driver);
-	info(DRIVER_DESC " version: " DRIVER_VERSION " unloaded\n");
 }
 
 module_init(shpcd_init);

-- 
2.39.5

Convert the last user of dbg() to use ctrl_dbg().

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/hotplug/shpchp_hpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/shpchp_hpc.c b/drivers/pci/hotplug/shpchp_hpc.c
index 012b9e3fe5b0..bfbec7c1a6b1 100644
--- a/drivers/pci/hotplug/shpchp_hpc.c
+++ b/drivers/pci/hotplug/shpchp_hpc.c

@@ -675,7 +675,7 @@ static int shpc_get_cur_bus_speed(struct controller *ctrl)
 
  out:
 	bus->cur_bus_speed = bus_speed;
-	dbg("Current bus speed = %d\n", bus_speed);
+	ctrl_dbg(ctrl, "Current bus speed = %d\n", bus_speed);
 	return retval;
 }
 

-- 
2.39.5

The shpchp hotplug driver defines logging wrappers ctrl_*() and another
set of wrappers with generic names which are just duplicates of
existing generic printk() wrappers. Only the former are useful to
preserve as they handle the controller dereferencing (the latter are
also unused).

The "shpchp_debug" module parameter is used to enable debug logging.
The generic ability to turn on/off debug prints dynamically covers this
usecase already so there is no need to module specific debug handling.
The ctrl_dbg() wrapper also uses a low-level pci_printk() despite
always using KERN_DEBUG level.

Convert ctrl_dbg() to use the pci_dbg() and remove "shpchp_debug" check
from it.

Removing the non-ctrl variants of logging wrappers and "shpchp_debug"
module parameter as they are no longer used.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/hotplug/shpchp.h      | 18 +-----------------
 drivers/pci/hotplug/shpchp_core.c |  3 ---
 2 files changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/pci/hotplug/shpchp.h b/drivers/pci/hotplug/shpchp.h
index f0e2d2d54d71..f9e57dce010b 100644
--- a/drivers/pci/hotplug/shpchp.h
+++ b/drivers/pci/hotplug/shpchp.h

@@ -33,24 +33,8 @@ extern bool shpchp_poll_mode;
 extern int shpchp_poll_time;
 extern bool shpchp_debug;
 
-#define dbg(format, arg...)						\
-do {									\
-	if (shpchp_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 (shpchp_debug)					\
-			pci_printk(KERN_DEBUG, ctrl->pci_dev,		\
-					format, ## arg);		\
-	} while (0)
+	pci_dbg(ctrl->pci_dev, format, ## arg);
 #define ctrl_err(ctrl, format, arg...)					\
 	pci_err(ctrl->pci_dev, format, ## arg)
 #define ctrl_info(ctrl, format, arg...)					\

diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
index a10ce7be7f51..0c341453afc6 100644
--- a/drivers/pci/hotplug/shpchp_core.c
+++ b/drivers/pci/hotplug/shpchp_core.c

@@ -22,7 +22,6 @@
 #include "shpchp.h"
 
 /* Global variables */
-bool shpchp_debug;
 bool shpchp_poll_mode;
 int shpchp_poll_time;
 

@@ -33,10 +32,8 @@ int shpchp_poll_time;
 MODULE_AUTHOR(DRIVER_AUTHOR);
 MODULE_DESCRIPTION(DRIVER_DESC);
 
-module_param(shpchp_debug, bool, 0644);
 module_param(shpchp_poll_mode, bool, 0644);
 module_param(shpchp_poll_time, int, 0644);
-MODULE_PARM_DESC(shpchp_debug, "Debugging mode enabled or not");
 MODULE_PARM_DESC(shpchp_poll_mode, "Using polling mechanism for hot-plug events or not");
 MODULE_PARM_DESC(shpchp_poll_time, "Polling mechanism frequency, in seconds");
 

-- 
2.39.5

include/linux/pci.h provides low-level pci_printk() interface that is
only used by AER because it needs to print the same message with
different levels depending on the error severity. No other PCI code
uses that functionality and calls pci_<level>() logging functions
directly with the appropriate level.

Descope pci_printk() into AER as aer_printk().

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pcie/aer.c | 10 +++++++---
 include/linux/pci.h    |  3 ---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 80c5ba8d8296..bfc6b94dad4d 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c

@@ -17,6 +17,7 @@
 
 #include <linux/bitops.h>
 #include <linux/cper.h>
+#include <linux/dev_printk.h>
 #include <linux/pci.h>
 #include <linux/pci-acpi.h>
 #include <linux/sched.h>

@@ -35,6 +36,9 @@
 #include "../pci.h"
 #include "portdrv.h"
 
+#define aer_printk(level, pdev, fmt, arg...) \
+	dev_printk(level, &(pdev)->dev, fmt, ##arg)
+
 #define AER_ERROR_SOURCES_MAX		128
 
 #define AER_MAX_TYPEOF_COR_ERRS		16	/* as per PCI_ERR_COR_STATUS */

@@ -692,7 +696,7 @@ static void __aer_print_error(struct pci_dev *dev,
 		if (!errmsg)
 			errmsg = "Unknown Error Bit";
 
-		pci_printk(level, dev, "   [%2d] %-22s%s\n", i, errmsg,
+		aer_printk(level, dev, "   [%2d] %-22s%s\n", i, errmsg,
 				info->first_error == i ? " (First)" : "");
 	}
 	pci_dev_aer_stats_incr(dev, info);

@@ -715,11 +719,11 @@ void aer_print_error(struct pci_dev *dev, struct aer_err_info *info)
 
 	level = (info->severity == AER_CORRECTABLE) ? KERN_WARNING : KERN_ERR;
 
-	pci_printk(level, dev, "PCIe Bus Error: severity=%s, type=%s, (%s)\n",
+	aer_printk(level, dev, "PCIe Bus Error: severity=%s, type=%s, (%s)\n",
 		   aer_error_severity_string[info->severity],
 		   aer_error_layer[layer], aer_agent_string[agent]);
 
-	pci_printk(level, dev, "  device [%04x:%04x] error status/mask=%08x/%08x\n",
+	aer_printk(level, dev, "  device [%04x:%04x] error status/mask=%08x/%08x\n",
 		   dev->vendor, dev->device, info->status, info->mask);
 
 	__aer_print_error(dev, info);

diff --git a/include/linux/pci.h b/include/linux/pci.h
index db9b47ce3eef..02d23e795915 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h

@@ -2685,9 +2685,6 @@ void pci_uevent_ers(struct pci_dev *pdev, enum  pci_ers_result err_type);
 
 #include <linux/dma-mapping.h>
 
-#define pci_printk(level, pdev, fmt, arg...) \
-	dev_printk(level, &(pdev)->dev, fmt, ##arg)
-
 #define pci_emerg(pdev, fmt, arg...)	dev_emerg(&(pdev)->dev, fmt, ##arg)
 #define pci_alert(pdev, fmt, arg...)	dev_alert(&(pdev)->dev, fmt, ##arg)
 #define pci_crit(pdev, fmt, arg...)	dev_crit(&(pdev)->dev, fmt, ##arg)

-- 
2.39.5

On Mon, Dec 16, 2024 at 06:10:11PM +0200, Ilpo Järvinen wrote:

The shpchp hotplug driver defines logging wrappers ctrl_*() and another
set of wrappers with generic names which are just duplicates of
existing generic printk() wrappers. Only the former are useful to
preserve as they handle the controller dereferencing (the latter are
also unused).

The "shpchp_debug" module parameter is used to enable debug logging.
The generic ability to turn on/off debug prints dynamically covers this
usecase already so there is no need to module specific debug handling.
The ctrl_dbg() wrapper also uses a low-level pci_printk() despite
always using KERN_DEBUG level.

I think it's great to get rid of the module param.  Can you include
a hint about how users of shpchp_debug should now enable debug prints?

The one I have in my notes is to set CONFIG_DYNAMIC_DEBUG=y and boot
with 'dyndbg="file drivers/pci/* +p"'.

Convert ctrl_dbg() to use the pci_dbg() and remove "shpchp_debug" check
from it.

Removing the non-ctrl variants of logging wrappers and "shpchp_debug"
module parameter as they are no longer used.

-#define dbg(format, arg...)						\
-do {									\
-	if (shpchp_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)

The above are unused, aren't they?  Can we make a separate patch to
remove these, for ease of describing and reviewing?

Bjorn

On Mon, Dec 16, 2024 at 06:10:12PM +0200, Ilpo Järvinen wrote:

include/linux/pci.h provides low-level pci_printk() interface that is
only used by AER because it needs to print the same message with
different levels depending on the error severity. No other PCI code
uses that functionality and calls pci_<level>() logging functions
directly with the appropriate level.

Descope pci_printk() into AER as aer_printk().

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

I applied this patch by itself on pci/aer for v6.15.

We also have some work-in-progress on rate limiting errors, which
might conflict, but this is simple and shouldn't be hard to reconcile.

On Mon, Dec 16, 2024 at 06:10:08PM +0200, Ilpo Järvinen wrote:

include/linux/pci.h provides pci_printk() which is a low-level
interface with level that is only useful for AER due to error severity
variations.

This series cleans up shpchp logging wrappers to avoid using low-level
pci_printk() unnecessarily and replaces pci_printk() with aer_printk().

Ilpo Järvinen (4):
  PCI: shpchp: Remove logging from module init/exit functions
  PCI: shpchp: Change dbg() -> ctrl_dbg()

I applied the above to pci/hotplug for v6.15, thanks!

  PCI: shpchp: Cleanup logging and debug wrappers
  PCI: Descope pci_printk() to aer_printk()

 drivers/pci/hotplug/shpchp.h      | 18 +-----------------
 drivers/pci/hotplug/shpchp_core.c | 13 +------------
 drivers/pci/hotplug/shpchp_hpc.c  |  2 +-
 drivers/pci/pcie/aer.c            | 10 +++++++---
 include/linux/pci.h               |  3 ---
 5 files changed, 10 insertions(+), 36 deletions(-)

-- 
2.39.5

On Thu, 13 Feb 2025, Bjorn Helgaas wrote:

On Mon, Dec 16, 2024 at 06:10:12PM +0200, Ilpo Järvinen wrote:

quoted

include/linux/pci.h provides low-level pci_printk() interface that is
only used by AER because it needs to print the same message with
different levels depending on the error severity. No other PCI code
uses that functionality and calls pci_<level>() logging functions
directly with the appropriate level.

Descope pci_printk() into AER as aer_printk().

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

I applied this patch by itself on pci/aer for v6.15.

We also have some work-in-progress on rate limiting errors, which
might conflict, but this is simple and shouldn't be hard to reconcile.

quoted

---
 drivers/pci/pcie/aer.c | 10 +++++++---
 include/linux/pci.h    |  3 ---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 80c5ba8d8296..bfc6b94dad4d 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c

@@ -17,6 +17,7 @@
 
 #include <linux/bitops.h>
 #include <linux/cper.h>
+#include <linux/dev_printk.h>
 #include <linux/pci.h>
 #include <linux/pci-acpi.h>
 #include <linux/sched.h>

@@ -35,6 +36,9 @@
 #include "../pci.h"
 #include "portdrv.h"
 
+#define aer_printk(level, pdev, fmt, arg...) \
+	dev_printk(level, &(pdev)->dev, fmt, ##arg)
+
 #define AER_ERROR_SOURCES_MAX		128
 
 #define AER_MAX_TYPEOF_COR_ERRS		16	/* as per PCI_ERR_COR_STATUS */

@@ -692,7 +696,7 @@ static void __aer_print_error(struct pci_dev *dev,
 		if (!errmsg)
 			errmsg = "Unknown Error Bit";
 
-		pci_printk(level, dev, "   [%2d] %-22s%s\n", i, errmsg,
+		aer_printk(level, dev, "   [%2d] %-22s%s\n", i, errmsg,
 				info->first_error == i ? " (First)" : "");
 	}
 	pci_dev_aer_stats_incr(dev, info);

@@ -715,11 +719,11 @@ void aer_print_error(struct pci_dev *dev, struct aer_err_info *info)
 
 	level = (info->severity == AER_CORRECTABLE) ? KERN_WARNING : KERN_ERR;
 
-	pci_printk(level, dev, "PCIe Bus Error: severity=%s, type=%s, (%s)\n",
+	aer_printk(level, dev, "PCIe Bus Error: severity=%s, type=%s, (%s)\n",
 		   aer_error_severity_string[info->severity],
 		   aer_error_layer[layer], aer_agent_string[agent]);
 
-	pci_printk(level, dev, "  device [%04x:%04x] error status/mask=%08x/%08x\n",
+	aer_printk(level, dev, "  device [%04x:%04x] error status/mask=%08x/%08x\n",
 		   dev->vendor, dev->device, info->status, info->mask);
 
 	__aer_print_error(dev, info);

diff --git a/include/linux/pci.h b/include/linux/pci.h
index db9b47ce3eef..02d23e795915 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h

@@ -2685,9 +2685,6 @@ void pci_uevent_ers(struct pci_dev *pdev, enum  pci_ers_result err_type);
 
 #include <linux/dma-mapping.h>
 
-#define pci_printk(level, pdev, fmt, arg...) \
-	dev_printk(level, &(pdev)->dev, fmt, ##arg)

Both shpchp and aer do use pci_printk() before this series (it seems LKP 
has also catched it already).

If you split this series into different branches, this removal of 
pci_printk() has to be postponed until the next kernel release (fine for 
me if that's what you want to do, just remove this part from this patch 
and perhaps adjust the commit message to say it's to prepare for removal 
of the pci_printk()).

quoted

 #define pci_emerg(pdev, fmt, arg...)	dev_emerg(&(pdev)->dev, fmt, ##arg)
 #define pci_alert(pdev, fmt, arg...)	dev_alert(&(pdev)->dev, fmt, ##arg)
 #define pci_crit(pdev, fmt, arg...)	dev_crit(&(pdev)->dev, fmt, ##arg)

-- 
 i.

On Thu, 13 Feb 2025, Bjorn Helgaas wrote:

On Mon, Dec 16, 2024 at 06:10:11PM +0200, Ilpo Järvinen wrote:

quoted

The shpchp hotplug driver defines logging wrappers ctrl_*() and another
set of wrappers with generic names which are just duplicates of
existing generic printk() wrappers. Only the former are useful to
preserve as they handle the controller dereferencing (the latter are
also unused).

The "shpchp_debug" module parameter is used to enable debug logging.
The generic ability to turn on/off debug prints dynamically covers this
usecase already so there is no need to module specific debug handling.
The ctrl_dbg() wrapper also uses a low-level pci_printk() despite
always using KERN_DEBUG level.

I think it's great to get rid of the module param.  Can you include
a hint about how users of shpchp_debug should now enable debug prints?

The one I have in my notes is to set CONFIG_DYNAMIC_DEBUG=y and boot
with 'dyndbg="file drivers/pci/* +p"'.

Sure, I'll add the info and split the change as you suggested below.

quoted

Convert ctrl_dbg() to use the pci_dbg() and remove "shpchp_debug" check
from it.

Removing the non-ctrl variants of logging wrappers and "shpchp_debug"
module parameter as they are no longer used.

quoted

-#define dbg(format, arg...)						\
-do {									\
-	if (shpchp_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)

The above are unused, aren't they?  Can we make a separate patch to
remove these, for ease of describing and reviewing?

-- 
 i.

On Fri, Feb 14, 2025 at 01:56:47PM +0200, Ilpo Järvinen wrote:

On Thu, 13 Feb 2025, Bjorn Helgaas wrote:

quoted

On Mon, Dec 16, 2024 at 06:10:12PM +0200, Ilpo Järvinen wrote:

quoted

include/linux/pci.h provides low-level pci_printk() interface that is
only used by AER because it needs to print the same message with
different levels depending on the error severity. No other PCI code
uses that functionality and calls pci_<level>() logging functions
directly with the appropriate level.

Descope pci_printk() into AER as aer_printk().

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

I applied this patch by itself on pci/aer for v6.15.

We also have some work-in-progress on rate limiting errors, which
might conflict, but this is simple and shouldn't be hard to reconcile.

quoted

quoted

diff --git a/include/linux/pci.h b/include/linux/pci.h
index db9b47ce3eef..02d23e795915 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h

@@ -2685,9 +2685,6 @@ void pci_uevent_ers(struct pci_dev *pdev, enum  pci_ers_result err_type);
 
 #include <linux/dma-mapping.h>
 
-#define pci_printk(level, pdev, fmt, arg...) \
-	dev_printk(level, &(pdev)->dev, fmt, ##arg)

Both shpchp and aer do use pci_printk() before this series (it seems LKP 
has also catched it already).

If you split this series into different branches, this removal of 
pci_printk() has to be postponed until the next kernel release (fine for 
me if that's what you want to do, just remove this part from this patch 
and perhaps adjust the commit message to say it's to prepare for removal 
of the pci_printk()).

OK.  I dropped the pci_printk() removal for now.  I'm anticipating
more AER changes this cycle, so I'm trying to keep those isolated.

Bjorn