* [PATCH] PM: Add device name to suspend_report_result()
[not found] <CGME20220302064921epcas1p19fbe8c017d776657caa696a3cef10093@epcas1p1.samsung.com>
@ 2022-03-02 6:49 ` Youngjin Jang
2022-03-02 7:56 ` Greg Kroah-Hartman
2022-03-02 7:57 ` Greg Kroah-Hartman
0 siblings, 2 replies; 8+ messages in thread
From: Youngjin Jang @ 2022-03-02 6:49 UTC (permalink / raw)
To: Rafael J. Wysocki, Pavel Machek, Len Brown, Greg Kroah-Hartman,
Bjorn Helgaas, linux-pm, linux-kernel, linux-pci, linux-acpi,
linux-usb
Cc: yj84.jang, js07.lee
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] PM: Add device name to suspend_report_result()
2022-03-02 6:49 ` [PATCH] PM: Add device name to suspend_report_result() Youngjin Jang
@ 2022-03-02 7:56 ` Greg Kroah-Hartman
2022-03-02 7:57 ` Greg Kroah-Hartman
1 sibling, 0 replies; 8+ messages in thread
From: Greg Kroah-Hartman @ 2022-03-02 7:56 UTC (permalink / raw)
To: Youngjin Jang
Cc: Rafael J. Wysocki, Pavel Machek, Len Brown, Bjorn Helgaas,
linux-pm, linux-kernel, linux-pci, linux-acpi, linux-usb,
js07.lee
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PM: Add device name to suspend_report_result()
2022-03-02 6:49 ` [PATCH] PM: Add device name to suspend_report_result() Youngjin Jang
2022-03-02 7:56 ` Greg Kroah-Hartman
@ 2022-03-02 7:57 ` Greg Kroah-Hartman
2022-03-02 11:00 ` 장영진/TV S/W Lab(VD)/Staff Engineer/삼성전자
1 sibling, 1 reply; 8+ messages in thread
From: Greg Kroah-Hartman @ 2022-03-02 7:57 UTC (permalink / raw)
To: Youngjin Jang
Cc: Rafael J. Wysocki, Pavel Machek, Len Brown, Bjorn Helgaas,
linux-pm, linux-kernel, linux-pci, linux-acpi, linux-usb,
js07.lee
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] PM: Add device name to suspend_report_result()
2022-03-02 7:57 ` Greg Kroah-Hartman
@ 2022-03-02 11:00 ` 장영진/TV S/W Lab(VD)/Staff Engineer/삼성전자
2022-03-02 14:52 ` 'Greg Kroah-Hartman'
0 siblings, 1 reply; 8+ messages in thread
From: 장영진/TV S/W Lab(VD)/Staff Engineer/삼성전자 @ 2022-03-02 11:00 UTC (permalink / raw)
To: 'Greg Kroah-Hartman'
Cc: 'Rafael J. Wysocki', 'Pavel Machek',
'Len Brown', 'Bjorn Helgaas',
linux-pm, linux-kernel, linux-pci, linux-acpi, linux-usb,
js07.lee
> -----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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PM: Add device name to suspend_report_result()
2022-03-02 11:00 ` 장영진/TV S/W Lab(VD)/Staff Engineer/삼성전자
@ 2022-03-02 14:52 ` 'Greg Kroah-Hartman'
2022-03-02 20:16 ` Bjorn Helgaas
0 siblings, 1 reply; 8+ messages in thread
From: 'Greg Kroah-Hartman' @ 2022-03-02 14:52 UTC (permalink / raw)
To: �念��/TV S/W Lab(VD)/Staff
Engineer/�Z����
Cc: 'Rafael J. Wysocki', 'Pavel Machek',
'Len Brown', 'Bjorn Helgaas',
linux-pm, linux-kernel, linux-pci, linux-acpi, linux-usb,
js07.lee
On Wed, Mar 02, 2022 at 08:00:14PM +0900, �念��/TV S/W Lab(VD)/Staff Engineer/�Z���� 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PM: Add device name to suspend_report_result()
2022-03-02 14:52 ` 'Greg Kroah-Hartman'
@ 2022-03-02 20:16 ` Bjorn Helgaas
2022-03-02 22:56 ` 장영진/TV S/W Lab(VD)/Staff Engineer/삼성전자
0 siblings, 1 reply; 8+ messages in thread
From: Bjorn Helgaas @ 2022-03-02 20:16 UTC (permalink / raw)
To: 'Greg Kroah-Hartman'
Cc: �念��/TV S/W Lab(VD)/Staff
Engineer/�Z����,
'Rafael J. Wysocki', 'Pavel Machek',
'Len Brown', 'Bjorn Helgaas',
linux-pm, linux-kernel, linux-pci, linux-acpi, linux-usb,
js07.lee
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/�Z���� 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] PM: Add device name to suspend_report_result()
2022-03-02 20:16 ` Bjorn Helgaas
@ 2022-03-02 22:56 ` 장영진/TV S/W Lab(VD)/Staff Engineer/삼성전자
2022-03-03 17:57 ` Bjorn Helgaas
0 siblings, 1 reply; 8+ messages in thread
From: 장영진/TV S/W Lab(VD)/Staff Engineer/삼성전자 @ 2022-03-02 22:56 UTC (permalink / raw)
To: 'Bjorn Helgaas', 'Greg Kroah-Hartman'
Cc: 'Rafael J. Wysocki', 'Pavel Machek',
'Len Brown', 'Bjorn Helgaas',
linux-pm, linux-kernel, linux-pci, linux-acpi, linux-usb,
js07.lee
> -----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/�Z���� <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/ Z 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().
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PM: Add device name to suspend_report_result()
2022-03-02 22:56 ` 장영진/TV S/W Lab(VD)/Staff Engineer/삼성전자
@ 2022-03-03 17:57 ` Bjorn Helgaas
0 siblings, 0 replies; 8+ messages in thread
From: Bjorn Helgaas @ 2022-03-03 17:57 UTC (permalink / raw)
To: 장영진/TV S/W Lab(VD)/Staff
Engineer/삼성전자
Cc: 'Greg Kroah-Hartman', 'Rafael J. Wysocki',
'Pavel Machek', 'Len Brown',
'Bjorn Helgaas',
linux-pm, linux-kernel, linux-pci, linux-acpi, linux-usb,
js07.lee
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/�Z���� <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/ Z 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: ".
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-03-03 17:57 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20220302064921epcas1p19fbe8c017d776657caa696a3cef10093@epcas1p1.samsung.com>
2022-03-02 6:49 ` [PATCH] PM: Add device name to suspend_report_result() Youngjin Jang
2022-03-02 7:56 ` Greg Kroah-Hartman
2022-03-02 7:57 ` Greg Kroah-Hartman
2022-03-02 11:00 ` 장영진/TV S/W Lab(VD)/Staff Engineer/삼성전자
2022-03-02 14:52 ` 'Greg Kroah-Hartman'
2022-03-02 20:16 ` Bjorn Helgaas
2022-03-02 22:56 ` 장영진/TV S/W Lab(VD)/Staff Engineer/삼성전자
2022-03-03 17:57 ` Bjorn Helgaas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.