[PATCH] PM: Add device name to suspend_report_result()

[PATCH] PM: Add device name to suspend_report_result()

[Youngjin Jang]

From: "yj84.jang" <yj84.jang@samsung.com>

currently, suspend_report_result() prints only function information.
If any driver uses common pm function, nobody knows who called
failed function exactly.

So, device information is needed to recognize specific wrong driver.

e.g.)
PM: dpm_run_callback(): pm_generic_suspend+0x0/0x48 returns 0
PM: dpm_run_callback(): platform_pm_suspend+0x0/0x68 returns 0
after patch,
PM: dpm_run_callback(): pm_generic_suspend+0x0/0x48 (amba) returns 0
PM: dpm_run_callback(): platform_pm_suspend+0x0/0x68 (armv7-pmu) returns 0

Signed-off-by: yj84.jang <yj84.jang@samsung.com>
---
 drivers/base/power/main.c  | 10 +++++-----
 drivers/pci/pci-driver.c   | 14 +++++++-------
 drivers/pnp/driver.c       |  2 +-
 drivers/usb/core/hcd-pci.c |  4 ++--
 include/linux/pm.h         |  8 ++++----
 5 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 04ea92c..a762fe8 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -485,7 +485,7 @@ static int dpm_run_callback(pm_callback_t cb, struct device *dev,
 	trace_device_pm_callback_start(dev, info, state.event);
 	error = cb(dev);
 	trace_device_pm_callback_end(dev, error);
-	suspend_report_result(cb, error);
+	suspend_report_result(dev, cb, error);
 
 	initcall_debug_report(dev, calltime, cb, error);
 
@@ -1568,7 +1568,7 @@ static int legacy_suspend(struct device *dev, pm_message_t state,
 	trace_device_pm_callback_start(dev, info, state.event);
 	error = cb(dev, state);
 	trace_device_pm_callback_end(dev, error);
-	suspend_report_result(cb, error);
+	suspend_report_result(dev, cb, error);
 
 	initcall_debug_report(dev, calltime, cb, error);
 
@@ -1855,7 +1855,7 @@ static int device_prepare(struct device *dev, pm_message_t state)
 	device_unlock(dev);
 
 	if (ret < 0) {
-		suspend_report_result(callback, ret);
+		suspend_report_result(dev, callback, ret);
 		pm_runtime_put(dev);
 		return ret;
 	}
@@ -1960,10 +1960,10 @@ int dpm_suspend_start(pm_message_t state)
 }
 EXPORT_SYMBOL_GPL(dpm_suspend_start);
 
-void __suspend_report_result(const char *function, void *fn, int ret)
+void __suspend_report_result(const char *function, struct device *dev, void *fn, int ret)
 {
 	if (ret)
-		pr_err("%s(): %pS returns %d\n", function, fn, ret);
+		pr_err("%s(): %pS (%s) returns %d\n", function, fn, dev_driver_string(dev), ret);
 }
 EXPORT_SYMBOL_GPL(__suspend_report_result);
 
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 588588c..415f766 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -596,7 +596,7 @@ static int pci_legacy_suspend(struct device *dev, pm_message_t state)
 		int error;
 
 		error = drv->suspend(pci_dev, state);
-		suspend_report_result(drv->suspend, error);
+		suspend_report_result(dev, drv->suspend, error);
 		if (error)
 			return error;
 
@@ -775,7 +775,7 @@ static int pci_pm_suspend(struct device *dev)
 		int error;
 
 		error = pm->suspend(dev);
-		suspend_report_result(pm->suspend, error);
+		suspend_report_result(dev, pm->suspend, error);
 		if (error)
 			return error;
 
@@ -821,7 +821,7 @@ static int pci_pm_suspend_noirq(struct device *dev)
 		int error;
 
 		error = pm->suspend_noirq(dev);
-		suspend_report_result(pm->suspend_noirq, error);
+		suspend_report_result(dev, pm->suspend_noirq, error);
 		if (error)
 			return error;
 
@@ -1010,7 +1010,7 @@ static int pci_pm_freeze(struct device *dev)
 		int error;
 
 		error = pm->freeze(dev);
-		suspend_report_result(pm->freeze, error);
+		suspend_report_result(dev, pm->freeze, error);
 		if (error)
 			return error;
 	}
@@ -1030,7 +1030,7 @@ static int pci_pm_freeze_noirq(struct device *dev)
 		int error;
 
 		error = pm->freeze_noirq(dev);
-		suspend_report_result(pm->freeze_noirq, error);
+		suspend_report_result(dev, pm->freeze_noirq, error);
 		if (error)
 			return error;
 	}
@@ -1116,7 +1116,7 @@ static int pci_pm_poweroff(struct device *dev)
 		int error;
 
 		error = pm->poweroff(dev);
-		suspend_report_result(pm->poweroff, error);
+		suspend_report_result(dev, pm->poweroff, error);
 		if (error)
 			return error;
 	}
@@ -1154,7 +1154,7 @@ static int pci_pm_poweroff_noirq(struct device *dev)
 		int error;
 
 		error = pm->poweroff_noirq(dev);
-		suspend_report_result(pm->poweroff_noirq, error);
+		suspend_report_result(dev, pm->poweroff_noirq, error);
 		if (error)
 			return error;
 	}
diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c
index cc6757d..c02e7bf 100644
--- a/drivers/pnp/driver.c
+++ b/drivers/pnp/driver.c
@@ -171,7 +171,7 @@ static int __pnp_bus_suspend(struct device *dev, pm_message_t state)
 
 	if (pnp_drv->driver.pm && pnp_drv->driver.pm->suspend) {
 		error = pnp_drv->driver.pm->suspend(dev);
-		suspend_report_result(pnp_drv->driver.pm->suspend, error);
+		suspend_report_result(dev, pnp_drv->driver.pm->suspend, error);
 		if (error)
 			return error;
 	}
diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index d630ccc..dd44e37 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -446,7 +446,7 @@ static int suspend_common(struct device *dev, bool do_wakeup)
 				HCD_WAKEUP_PENDING(hcd->shared_hcd))
 			return -EBUSY;
 		retval = hcd->driver->pci_suspend(hcd, do_wakeup);
-		suspend_report_result(hcd->driver->pci_suspend, retval);
+		suspend_report_result(dev, hcd->driver->pci_suspend, retval);
 
 		/* Check again in case wakeup raced with pci_suspend */
 		if ((retval == 0 && do_wakeup && HCD_WAKEUP_PENDING(hcd)) ||
@@ -556,7 +556,7 @@ static int hcd_pci_suspend_noirq(struct device *dev)
 		dev_dbg(dev, "--> PCI %s\n",
 				pci_power_name(pci_dev->current_state));
 	} else {
-		suspend_report_result(pci_prepare_to_sleep, retval);
+		suspend_report_result(dev, pci_prepare_to_sleep, retval);
 		return retval;
 	}
 
diff --git a/include/linux/pm.h b/include/linux/pm.h
index e1e9402..cdccbb9 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -745,11 +745,11 @@ extern int dpm_suspend_late(pm_message_t state);
 extern int dpm_suspend(pm_message_t state);
 extern int dpm_prepare(pm_message_t state);
 
-extern void __suspend_report_result(const char *function, void *fn, int ret);
+extern void __suspend_report_result(const char *function, struct device *dev, void *fn, int ret);
 
-#define suspend_report_result(fn, ret)					\
+#define suspend_report_result(dev, fn, ret)				\
 	do {								\
-		__suspend_report_result(__func__, fn, ret);		\
+		__suspend_report_result(__func__, dev, fn, ret);	\
 	} while (0)
 
 extern int device_pm_wait_for_dev(struct device *sub, struct device *dev);
@@ -789,7 +789,7 @@ static inline int dpm_suspend_start(pm_message_t state)
 	return 0;
 }
 
-#define suspend_report_result(fn, ret)		do {} while (0)
+#define suspend_report_result(dev, fn, ret)	do {} while (0)
 
 static inline int device_pm_wait_for_dev(struct device *a, struct device *b)
 {
-- 
2.7.4

Re: [PATCH] PM: Add device name to suspend_report_result()

[Greg Kroah-Hartman]

On Wed, Mar 02, 2022 at 03:49:17PM +0900, Youngjin Jang wrote:
> From: "yj84.jang" <yj84.jang@samsung.com>

Please use a real name here, I doubt that is how you sign legal
documents :)

thanks,

greg k-h

Re: [PATCH] PM: Add device name to suspend_report_result()

[Greg Kroah-Hartman]

On Wed, Mar 02, 2022 at 03:49:17PM +0900, Youngjin Jang wrote:
> From: "yj84.jang" <yj84.jang@samsung.com>
> 
> currently, suspend_report_result() prints only function information.
> If any driver uses common pm function, nobody knows who called
> failed function exactly.
> 
> So, device information is needed to recognize specific wrong driver.
> 
> e.g.)
> PM: dpm_run_callback(): pm_generic_suspend+0x0/0x48 returns 0
> PM: dpm_run_callback(): platform_pm_suspend+0x0/0x68 returns 0
> after patch,
> PM: dpm_run_callback(): pm_generic_suspend+0x0/0x48 (amba) returns 0
> PM: dpm_run_callback(): platform_pm_suspend+0x0/0x68 (armv7-pmu) returns 0
> 
> Signed-off-by: yj84.jang <yj84.jang@samsung.com>
> ---
>  drivers/base/power/main.c  | 10 +++++-----
>  drivers/pci/pci-driver.c   | 14 +++++++-------
>  drivers/pnp/driver.c       |  2 +-
>  drivers/usb/core/hcd-pci.c |  4 ++--
>  include/linux/pm.h         |  8 ++++----
>  5 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 04ea92c..a762fe8 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -485,7 +485,7 @@ static int dpm_run_callback(pm_callback_t cb, struct device *dev,
>  	trace_device_pm_callback_start(dev, info, state.event);
>  	error = cb(dev);
>  	trace_device_pm_callback_end(dev, error);
> -	suspend_report_result(cb, error);
> +	suspend_report_result(dev, cb, error);
>  
>  	initcall_debug_report(dev, calltime, cb, error);
>  
> @@ -1568,7 +1568,7 @@ static int legacy_suspend(struct device *dev, pm_message_t state,
>  	trace_device_pm_callback_start(dev, info, state.event);
>  	error = cb(dev, state);
>  	trace_device_pm_callback_end(dev, error);
> -	suspend_report_result(cb, error);
> +	suspend_report_result(dev, cb, error);
>  
>  	initcall_debug_report(dev, calltime, cb, error);
>  
> @@ -1855,7 +1855,7 @@ static int device_prepare(struct device *dev, pm_message_t state)
>  	device_unlock(dev);
>  
>  	if (ret < 0) {
> -		suspend_report_result(callback, ret);
> +		suspend_report_result(dev, callback, ret);
>  		pm_runtime_put(dev);
>  		return ret;
>  	}
> @@ -1960,10 +1960,10 @@ int dpm_suspend_start(pm_message_t state)
>  }
>  EXPORT_SYMBOL_GPL(dpm_suspend_start);
>  
> -void __suspend_report_result(const char *function, void *fn, int ret)
> +void __suspend_report_result(const char *function, struct device *dev, void *fn, int ret)
>  {
>  	if (ret)
> -		pr_err("%s(): %pS returns %d\n", function, fn, ret);
> +		pr_err("%s(): %pS (%s) returns %d\n", function, fn, dev_driver_string(dev), ret);

If you have a struct device, please use dev_err().

thanks,

greg k-h

RE: [PATCH] PM: Add device name to suspend_report_result()

[장영진/TV S/W Lab(VD)/Staff Engineer/삼성전자]

> -----Original Message-----
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Wednesday, March 2, 2022 4:58 PM
> To: Youngjin Jang <yj84.jang@samsung.com>
> Cc: Rafael J. Wysocki <rafael@kernel.org>; Pavel Machek <pavel@ucw.cz>;
> Len Brown <len.brown@intel.com>; Bjorn Helgaas <bhelgaas@google.com>;
> linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> pci@vger.kernel.org; linux-acpi@vger.kernel.org; linux-
usb@vger.kernel.org;
> js07.lee@samsung.com
> Subject: Re: [PATCH] PM: Add device name to suspend_report_result()
> 
> On Wed, Mar 02, 2022 at 03:49:17PM +0900, Youngjin Jang wrote:
> > From: "yj84.jang" <yj84.jang@samsung.com>
> >
> > currently, suspend_report_result() prints only function information.
> > If any driver uses common pm function, nobody knows who called failed
> > function exactly.
> >
> > So, device information is needed to recognize specific wrong driver.
> >
> > e.g.)
> > PM: dpm_run_callback(): pm_generic_suspend+0x0/0x48 returns 0
> > PM: dpm_run_callback(): platform_pm_suspend+0x0/0x68 returns 0 after
> > patch,
> > PM: dpm_run_callback(): pm_generic_suspend+0x0/0x48 (amba) returns 0
> > PM: dpm_run_callback(): platform_pm_suspend+0x0/0x68 (armv7-pmu)
> > returns 0
> >
> > Signed-off-by: yj84.jang <yj84.jang@samsung.com>
> > ---
> >  drivers/base/power/main.c  | 10 +++++-----
> >  drivers/pci/pci-driver.c   | 14 +++++++-------
> >  drivers/pnp/driver.c       |  2 +-
> >  drivers/usb/core/hcd-pci.c |  4 ++--
> >  include/linux/pm.h         |  8 ++++----
> >  5 files changed, 19 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> > index 04ea92c..a762fe8 100644
> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -485,7 +485,7 @@ static int dpm_run_callback(pm_callback_t cb, struct
> device *dev,
> >  	trace_device_pm_callback_start(dev, info, state.event);
> >  	error = cb(dev);
> >  	trace_device_pm_callback_end(dev, error);
> > -	suspend_report_result(cb, error);
> > +	suspend_report_result(dev, cb, error);
> >
> >  	initcall_debug_report(dev, calltime, cb, error);
> >
> > @@ -1568,7 +1568,7 @@ static int legacy_suspend(struct device *dev,
> pm_message_t state,
> >  	trace_device_pm_callback_start(dev, info, state.event);
> >  	error = cb(dev, state);
> >  	trace_device_pm_callback_end(dev, error);
> > -	suspend_report_result(cb, error);
> > +	suspend_report_result(dev, cb, error);
> >
> >  	initcall_debug_report(dev, calltime, cb, error);
> >
> > @@ -1855,7 +1855,7 @@ static int device_prepare(struct device *dev,
> pm_message_t state)
> >  	device_unlock(dev);
> >
> >  	if (ret < 0) {
> > -		suspend_report_result(callback, ret);
> > +		suspend_report_result(dev, callback, ret);
> >  		pm_runtime_put(dev);
> >  		return ret;
> >  	}
> > @@ -1960,10 +1960,10 @@ int dpm_suspend_start(pm_message_t state)  }
> > EXPORT_SYMBOL_GPL(dpm_suspend_start);
> >
> > -void __suspend_report_result(const char *function, void *fn, int ret)
> > +void __suspend_report_result(const char *function, struct device
> > +*dev, void *fn, int ret)
> >  {
> >  	if (ret)
> > -		pr_err("%s(): %pS returns %d\n", function, fn, ret);
> > +		pr_err("%s(): %pS (%s) returns %d\n", function, fn,
> > +dev_driver_string(dev), ret);
> 
> If you have a struct device, please use dev_err().
> 
> thanks,
> 
> greg k-h

Hello,
Thanks for your review.

I think dev_err() is nice option, but we can see a minor issue.
Prefix log "PM: " would be lost, If I use dev_err() in this context.
As you know, all logs in power management include "PM :" prefix.

So, I think pr_err() with detail information would be better than dev_err().
- PM: amba 1740000.etm: dpm_run_callback(): pm_generic_resume+0x0/0x48
returns 0

Re: [PATCH] PM: Add device name to suspend_report_result()

['Greg Kroah-Hartman']

On Wed, Mar 02, 2022 at 08:00:14PM +0900, �念��/TV S/W Lab(VD)/Staff Engineer/�Ｚ���� wrote:
> > -----Original Message-----
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Sent: Wednesday, March 2, 2022 4:58 PM
> > To: Youngjin Jang <yj84.jang@samsung.com>
> > Cc: Rafael J. Wysocki <rafael@kernel.org>; Pavel Machek <pavel@ucw.cz>;
> > Len Brown <len.brown@intel.com>; Bjorn Helgaas <bhelgaas@google.com>;
> > linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> > pci@vger.kernel.org; linux-acpi@vger.kernel.org; linux-
> usb@vger.kernel.org;
> > js07.lee@samsung.com
> > Subject: Re: [PATCH] PM: Add device name to suspend_report_result()
> > 
> > On Wed, Mar 02, 2022 at 03:49:17PM +0900, Youngjin Jang wrote:
> > > From: "yj84.jang" <yj84.jang@samsung.com>
> > >
> > > currently, suspend_report_result() prints only function information.
> > > If any driver uses common pm function, nobody knows who called failed
> > > function exactly.
> > >
> > > So, device information is needed to recognize specific wrong driver.
> > >
> > > e.g.)
> > > PM: dpm_run_callback(): pm_generic_suspend+0x0/0x48 returns 0
> > > PM: dpm_run_callback(): platform_pm_suspend+0x0/0x68 returns 0 after
> > > patch,
> > > PM: dpm_run_callback(): pm_generic_suspend+0x0/0x48 (amba) returns 0
> > > PM: dpm_run_callback(): platform_pm_suspend+0x0/0x68 (armv7-pmu)
> > > returns 0
> > >
> > > Signed-off-by: yj84.jang <yj84.jang@samsung.com>
> > > ---
> > >  drivers/base/power/main.c  | 10 +++++-----
> > >  drivers/pci/pci-driver.c   | 14 +++++++-------
> > >  drivers/pnp/driver.c       |  2 +-
> > >  drivers/usb/core/hcd-pci.c |  4 ++--
> > >  include/linux/pm.h         |  8 ++++----
> > >  5 files changed, 19 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> > > index 04ea92c..a762fe8 100644
> > > --- a/drivers/base/power/main.c
> > > +++ b/drivers/base/power/main.c
> > > @@ -485,7 +485,7 @@ static int dpm_run_callback(pm_callback_t cb, struct
> > device *dev,
> > >  	trace_device_pm_callback_start(dev, info, state.event);
> > >  	error = cb(dev);
> > >  	trace_device_pm_callback_end(dev, error);
> > > -	suspend_report_result(cb, error);
> > > +	suspend_report_result(dev, cb, error);
> > >
> > >  	initcall_debug_report(dev, calltime, cb, error);
> > >
> > > @@ -1568,7 +1568,7 @@ static int legacy_suspend(struct device *dev,
> > pm_message_t state,
> > >  	trace_device_pm_callback_start(dev, info, state.event);
> > >  	error = cb(dev, state);
> > >  	trace_device_pm_callback_end(dev, error);
> > > -	suspend_report_result(cb, error);
> > > +	suspend_report_result(dev, cb, error);
> > >
> > >  	initcall_debug_report(dev, calltime, cb, error);
> > >
> > > @@ -1855,7 +1855,7 @@ static int device_prepare(struct device *dev,
> > pm_message_t state)
> > >  	device_unlock(dev);
> > >
> > >  	if (ret < 0) {
> > > -		suspend_report_result(callback, ret);
> > > +		suspend_report_result(dev, callback, ret);
> > >  		pm_runtime_put(dev);
> > >  		return ret;
> > >  	}
> > > @@ -1960,10 +1960,10 @@ int dpm_suspend_start(pm_message_t state)  }
> > > EXPORT_SYMBOL_GPL(dpm_suspend_start);
> > >
> > > -void __suspend_report_result(const char *function, void *fn, int ret)
> > > +void __suspend_report_result(const char *function, struct device
> > > +*dev, void *fn, int ret)
> > >  {
> > >  	if (ret)
> > > -		pr_err("%s(): %pS returns %d\n", function, fn, ret);
> > > +		pr_err("%s(): %pS (%s) returns %d\n", function, fn,
> > > +dev_driver_string(dev), ret);
> > 
> > If you have a struct device, please use dev_err().
> > 
> > thanks,
> > 
> > greg k-h
> 
> Hello,
> Thanks for your review.
> 
> I think dev_err() is nice option, but we can see a minor issue.
> Prefix log "PM: " would be lost, If I use dev_err() in this context.
> As you know, all logs in power management include "PM :" prefix.

Why does that matter?  Fix them all to use the struct device pointer and
then they will be properly unified with the rest of the kernel log
infrastructure.

thanks,

greg k-h

Re: [PATCH] PM: Add device name to suspend_report_result()

[Bjorn Helgaas]

On Wed, Mar 02, 2022 at 03:52:51PM +0100, 'Greg Kroah-Hartman' wrote:
> On Wed, Mar 02, 2022 at 08:00:14PM +0900, �念��/TV S/W Lab(VD)/Staff Engineer/�Ｚ���� wrote:
> > > -----Original Message-----
> > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Sent: Wednesday, March 2, 2022 4:58 PM
> > > To: Youngjin Jang <yj84.jang@samsung.com>
> > > Cc: Rafael J. Wysocki <rafael@kernel.org>; Pavel Machek <pavel@ucw.cz>;
> > > Len Brown <len.brown@intel.com>; Bjorn Helgaas <bhelgaas@google.com>;
> > > linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> > > pci@vger.kernel.org; linux-acpi@vger.kernel.org; linux-
> > usb@vger.kernel.org;
> > > js07.lee@samsung.com
> > > Subject: Re: [PATCH] PM: Add device name to suspend_report_result()
> > > 
> > > On Wed, Mar 02, 2022 at 03:49:17PM +0900, Youngjin Jang wrote:
> > > > From: "yj84.jang" <yj84.jang@samsung.com>
> > > >
> > > > currently, suspend_report_result() prints only function information.
> > > > If any driver uses common pm function, nobody knows who called failed
> > > > function exactly.
> > > >
> > > > So, device information is needed to recognize specific wrong driver.
> > > >
> > > > e.g.)
> > > > PM: dpm_run_callback(): pm_generic_suspend+0x0/0x48 returns 0
> > > > PM: dpm_run_callback(): platform_pm_suspend+0x0/0x68 returns 0 after
> > > > patch,
> > > > PM: dpm_run_callback(): pm_generic_suspend+0x0/0x48 (amba) returns 0
> > > > PM: dpm_run_callback(): platform_pm_suspend+0x0/0x68 (armv7-pmu)
> > > > returns 0

> > > > -		pr_err("%s(): %pS returns %d\n", function, fn, ret);
> > > > +		pr_err("%s(): %pS (%s) returns %d\n", function, fn,
> > > > +dev_driver_string(dev), ret);
> > > 
> > > If you have a struct device, please use dev_err().
> > 
> > I think dev_err() is nice option, but we can see a minor issue.
> > Prefix log "PM: " would be lost, If I use dev_err() in this context.
> > As you know, all logs in power management include "PM :" prefix.
> 
> Why does that matter?  Fix them all to use the struct device pointer and
> then they will be properly unified with the rest of the kernel log
> infrastructure.

You can #define dev_fmt if you need a prefix.

RE: [PATCH] PM: Add device name to suspend_report_result()

[장영진/TV S/W Lab(VD)/Staff Engineer/삼성전자]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Thursday, March 3, 2022 5:16 AM
> To: 'Greg Kroah-Hartman' <gregkh@linuxfoundation.org>
> Cc: �念��/TV S/W Lab(VD)/Staff Engineer/�Ｚ���� <yj84.jang@samsung.com>;
> 'Rafael J. Wysocki' <rafael@kernel.org>; 'Pavel Machek' <pavel@ucw.cz>;
> 'Len Brown' <len.brown@intel.com>; 'Bjorn Helgaas' <bhelgaas@google.com>;
> linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> pci@vger.kernel.org; linux-acpi@vger.kernel.org; linux-usb@vger.kernel.org;
> js07.lee@samsung.com
> Subject: Re: [PATCH] PM: Add device name to suspend_report_result()
> 
> On Wed, Mar 02, 2022 at 03:52:51PM +0100, 'Greg Kroah-Hartman' wrote:
> > On Wed, Mar 02, 2022 at 08:00:14PM +0900,  念  /TV S/W Lab(VD)/Staff
> Engineer/ Ｚ     wrote:
> > > > -----Original Message-----
> > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > Sent: Wednesday, March 2, 2022 4:58 PM
> > > > To: Youngjin Jang <yj84.jang@samsung.com>
> > > > Cc: Rafael J. Wysocki <rafael@kernel.org>; Pavel Machek
> > > > <pavel@ucw.cz>; Len Brown <len.brown@intel.com>; Bjorn Helgaas
> > > > <bhelgaas@google.com>; linux-pm@vger.kernel.org;
> > > > linux-kernel@vger.kernel.org; linux- pci@vger.kernel.org;
> > > > linux-acpi@vger.kernel.org; linux-
> > > usb@vger.kernel.org;
> > > > js07.lee@samsung.com
> > > > Subject: Re: [PATCH] PM: Add device name to
> > > > suspend_report_result()
> > > >
> > > > On Wed, Mar 02, 2022 at 03:49:17PM +0900, Youngjin Jang wrote:
> > > > > From: "yj84.jang" <yj84.jang@samsung.com>
> > > > >
> > > > > currently, suspend_report_result() prints only function
> information.
> > > > > If any driver uses common pm function, nobody knows who called
> > > > > failed function exactly.
> > > > >
> > > > > So, device information is needed to recognize specific wrong
> driver.
> > > > >
> > > > > e.g.)
> > > > > PM: dpm_run_callback(): pm_generic_suspend+0x0/0x48 returns 0
> > > > > PM: dpm_run_callback(): platform_pm_suspend+0x0/0x68 returns 0
> > > > > after patch,
> > > > > PM: dpm_run_callback(): pm_generic_suspend+0x0/0x48 (amba)
> > > > > returns 0
> > > > > PM: dpm_run_callback(): platform_pm_suspend+0x0/0x68 (armv7-pmu)
> > > > > returns 0
> 
> > > > > -		pr_err("%s(): %pS returns %d\n", function, fn, ret);
> > > > > +		pr_err("%s(): %pS (%s) returns %d\n", function, fn,
> > > > > +dev_driver_string(dev), ret);
> > > >
> > > > If you have a struct device, please use dev_err().
> > >
> > > I think dev_err() is nice option, but we can see a minor issue.
> > > Prefix log "PM: " would be lost, If I use dev_err() in this context.
> > > As you know, all logs in power management include "PM :" prefix.
> >
> > Why does that matter?  Fix them all to use the struct device pointer
> > and then they will be properly unified with the rest of the kernel log
> > infrastructure.
> 
> You can #define dev_fmt if you need a prefix.

I tested dev_fmt before, but I feel that not a good solution.
Because the readability is not so great than I expected.
I didn't want to break the PM logging rules.

Anyway, I got you guys opinion.
Let me try second patch with dev_err().

Re: [PATCH] PM: Add device name to suspend_report_result()

[Bjorn Helgaas]

On Thu, Mar 03, 2022 at 07:56:37AM +0900, 장영진/TV S/W Lab(VD)/Staff Engineer/삼성전자 wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@kernel.org>
> > Sent: Thursday, March 3, 2022 5:16 AM
> > To: 'Greg Kroah-Hartman' <gregkh@linuxfoundation.org>
> > Cc: �念��/TV S/W Lab(VD)/Staff Engineer/�Ｚ���� <yj84.jang@samsung.com>;
> > 'Rafael J. Wysocki' <rafael@kernel.org>; 'Pavel Machek' <pavel@ucw.cz>;
> > 'Len Brown' <len.brown@intel.com>; 'Bjorn Helgaas' <bhelgaas@google.com>;
> > linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> > pci@vger.kernel.org; linux-acpi@vger.kernel.org; linux-usb@vger.kernel.org;
> > js07.lee@samsung.com
> > Subject: Re: [PATCH] PM: Add device name to suspend_report_result()
> > 
> > On Wed, Mar 02, 2022 at 03:52:51PM +0100, 'Greg Kroah-Hartman' wrote:
> > > On Wed, Mar 02, 2022 at 08:00:14PM +0900,  念  /TV S/W Lab(VD)/Staff
> > Engineer/ Ｚ     wrote:
> > > > > -----Original Message-----
> > > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > Sent: Wednesday, March 2, 2022 4:58 PM
> > > > > To: Youngjin Jang <yj84.jang@samsung.com>
> > > > > Cc: Rafael J. Wysocki <rafael@kernel.org>; Pavel Machek
> > > > > <pavel@ucw.cz>; Len Brown <len.brown@intel.com>; Bjorn Helgaas
> > > > > <bhelgaas@google.com>; linux-pm@vger.kernel.org;
> > > > > linux-kernel@vger.kernel.org; linux- pci@vger.kernel.org;
> > > > > linux-acpi@vger.kernel.org; linux-
> > > > usb@vger.kernel.org;
> > > > > js07.lee@samsung.com
> > > > > Subject: Re: [PATCH] PM: Add device name to
> > > > > suspend_report_result()
> > > > >
> > > > > On Wed, Mar 02, 2022 at 03:49:17PM +0900, Youngjin Jang wrote:
> > > > > > From: "yj84.jang" <yj84.jang@samsung.com>

> > > > > > -		pr_err("%s(): %pS returns %d\n", function, fn, ret);
> > > > > > +		pr_err("%s(): %pS (%s) returns %d\n", function, fn,
> > > > > > +dev_driver_string(dev), ret);
> > > > >
> > > > > If you have a struct device, please use dev_err().
> > > >
> > > > I think dev_err() is nice option, but we can see a minor issue.
> > > > Prefix log "PM: " would be lost, If I use dev_err() in this context.
> > > > As you know, all logs in power management include "PM :" prefix.
> > >
> > > Why does that matter?  Fix them all to use the struct device pointer
> > > and then they will be properly unified with the rest of the kernel log
> > > infrastructure.
> > 
> > You can #define dev_fmt if you need a prefix.
> 
> I tested dev_fmt before, but I feel that not a good solution.
> Because the readability is not so great than I expected.
> I didn't want to break the PM logging rules.

I didn't catch your meaning here.  Some examples would probably help.

The patch above is from __suspend_report_result() in
drivers/base/power/main.c.  That file already defines both pr_fmt and
dev_fmt to be "PM: ", so I would expect dev_err() output to already
include "PM: ".