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

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

[Ilpo Järvinen]

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

[PATCH 1/4] PCI: shpchp: Remove logging from module init/exit functions

[Ilpo Järvinen]

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

[PATCH 2/4] PCI: shpchp: Change dbg() -> ctrl_dbg()

[Ilpo Järvinen]

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

[PATCH 3/4] PCI: shpchp: Cleanup logging and debug wrappers

[Ilpo Järvinen]

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

[PATCH 4/4] PCI: Descope pci_printk() to aer_printk()

[Ilpo Järvinen]

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

Re: [PATCH 3/4] PCI: shpchp: Cleanup logging and debug wrappers

[Bjorn Helgaas]

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

Re: [PATCH 4/4] PCI: Descope pci_printk() to aer_printk()

[Bjorn Helgaas]

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.

> ---
>  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
> 

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

[Bjorn Helgaas]

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
> 

Re: [PATCH 4/4] PCI: Descope pci_printk() to aer_printk()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3865 bytes --]

On Thu, 13 Feb 2025, Bjorn Helgaas wrote:

> 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.
> 
> > ---
> >  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()).

> >  #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.

Re: [PATCH 3/4] PCI: shpchp: Cleanup logging and debug wrappers

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1939 bytes --]

On Thu, 13 Feb 2025, Bjorn Helgaas wrote:

> 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"'.

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

> > 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?

-- 
 i.

Re: [PATCH 4/4] PCI: Descope pci_printk() to aer_printk()

[Bjorn Helgaas]

On Fri, Feb 14, 2025 at 01:56:47PM +0200, Ilpo Järvinen wrote:
> On Thu, 13 Feb 2025, Bjorn Helgaas wrote:
> > 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.

> > > 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