diff mbox series

[v2,3/4] pinctrl: amd: Use pm_pr_dbg to show debugging messages

Message ID 20230522200033.2605-3-mario.limonciello@amd.com
State New
Headers
Series [v2,1/4] include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume |

Commit Message

Mario Limonciello May 22, 2023, 8 p.m. UTC 
  To make the GPIO tracking around suspend easier for end users to
use, link it with pm_debug_messages.  This will make discovering
sources of spurious GPIOs around suspend easier.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pinctrl/pinctrl-amd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Andy Shevchenko May 23, 2023, 4:55 p.m. UTC | #1 
Mon, May 22, 2023 at 03:00:32PM -0500, Mario Limonciello kirjoitti:
> To make the GPIO tracking around suspend easier for end users to
> use, link it with pm_debug_messages.  This will make discovering
> sources of spurious GPIOs around suspend easier.

Unfortunatelly this has two regressions.

...

> -				dev_dbg(&gpio_dev->pdev->dev,
> -					"GPIO %d is active: 0x%x",
> -					irqnr + i, regval);
> +				pm_pr_dbg("GPIO %d is active: 0x%x",
> +					  irqnr + i, regval);

Regression 1: The device is now omitted from the output.
Regression 2: See https://stackoverflow.com/a/43957671/2511795
Rafael J. Wysocki May 24, 2023, 6:28 p.m. UTC | #2 
On Tue, May 23, 2023 at 6:55 PM <andy.shevchenko@gmail.com> wrote:
>
> Mon, May 22, 2023 at 03:00:32PM -0500, Mario Limonciello kirjoitti:
> > To make the GPIO tracking around suspend easier for end users to
> > use, link it with pm_debug_messages.  This will make discovering
> > sources of spurious GPIOs around suspend easier.
>
> Unfortunatelly this has two regressions.
>
> ...
>
> > -                             dev_dbg(&gpio_dev->pdev->dev,
> > -                                     "GPIO %d is active: 0x%x",
> > -                                     irqnr + i, regval);
> > +                             pm_pr_dbg("GPIO %d is active: 0x%x",
> > +                                       irqnr + i, regval);
>
> Regression 1: The device is now omitted from the output.

Right.

> Regression 2: See https://stackoverflow.com/a/43957671/2511795

Care to elaborate?  I'm not sure what you mean exactly.
Andy Shevchenko May 24, 2023, 7:57 p.m. UTC | #3 
On Wed, May 24, 2023 at 9:28 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, May 23, 2023 at 6:55 PM <andy.shevchenko@gmail.com> wrote:
> > Mon, May 22, 2023 at 03:00:32PM -0500, Mario Limonciello kirjoitti:

...

> > > -                             dev_dbg(&gpio_dev->pdev->dev,
> > > -                                     "GPIO %d is active: 0x%x",
> > > -                                     irqnr + i, regval);
> > > +                             pm_pr_dbg("GPIO %d is active: 0x%x",
> > > +                                       irqnr + i, regval);
> >
> > Regression 1: The device is now omitted from the output.
>
> Right.
>
> > Regression 2: See https://stackoverflow.com/a/43957671/2511795
>
> Care to elaborate?  I'm not sure what you mean exactly.

dev_dbg has 3 cases how it prints its content:
1/ With dynamic debug when it's enabled.
2/ With -DDEBUG if it's defined for the certain file(s) in the Makefile.
3/ No print.

pm_pr_dbg relies on CONFIG_PM_SLEEP_DEBUG, pm_debug_messages_on and
not on -DDEBUG. I haven't checked all relations between those 3, but
it seems to me that DEBUG is not equivalent to the others.
CONFIG_PM_SLEEP_DEBUG=n prevents printing with the dynamic debug on.

OTOH I dunno how this is relevant to the functionality of the driver
in question. Maybe it's okay to have such changes.
Mario Limonciello May 24, 2023, 8:25 p.m. UTC | #4 
On 5/24/2023 2:57 PM, Andy Shevchenko wrote:
> On Wed, May 24, 2023 at 9:28 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Tue, May 23, 2023 at 6:55 PM <andy.shevchenko@gmail.com> wrote:
>>> Mon, May 22, 2023 at 03:00:32PM -0500, Mario Limonciello kirjoitti:
> ...
>
>>>> -                             dev_dbg(&gpio_dev->pdev->dev,
>>>> -                                     "GPIO %d is active: 0x%x",
>>>> -                                     irqnr + i, regval);
>>>> +                             pm_pr_dbg("GPIO %d is active: 0x%x",
>>>> +                                       irqnr + i, regval);
>>> Regression 1: The device is now omitted from the output.
>> Right.
>>
>>> Regression 2: See https://stackoverflow.com/a/43957671/2511795
>> Care to elaborate?  I'm not sure what you mean exactly.
> dev_dbg has 3 cases how it prints its content:
> 1/ With dynamic debug when it's enabled.
> 2/ With -DDEBUG if it's defined for the certain file(s) in the Makefile.
> 3/ No print.
>
> pm_pr_dbg relies on CONFIG_PM_SLEEP_DEBUG, pm_debug_messages_on and
> not on -DDEBUG. I haven't checked all relations between those 3, but
> it seems to me that DEBUG is not equivalent to the others.
> CONFIG_PM_SLEEP_DEBUG=n prevents printing with the dynamic debug on.
>
> OTOH I dunno how this is relevant to the functionality of the driver
> in question. Maybe it's okay to have such changes.

The main reason for this debug statement in the first
place was for debugging sources of spurious wakeups.

As the statement is in the interrupt handler, turning
it on at "runtime" usually makes for a very noisy kernel
log because things like I2C touchpad will fire interrupts
constantly.
diff mbox series

Patch

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index f279b360c20d..43d3530bab48 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -30,6 +30,7 @@ 
 #include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinconf-generic.h>
 #include <linux/pinctrl/pinmux.h>
+#include <linux/suspend.h>
 
 #include "core.h"
 #include "pinctrl-utils.h"
@@ -636,9 +637,8 @@  static bool do_amd_gpio_irq_handler(int irq, void *dev_id)
 			regval = readl(regs + i);
 
 			if (regval & PIN_IRQ_PENDING)
-				dev_dbg(&gpio_dev->pdev->dev,
-					"GPIO %d is active: 0x%x",
-					irqnr + i, regval);
+				pm_pr_dbg("GPIO %d is active: 0x%x",
+					  irqnr + i, regval);
 
 			/* caused wake on resume context for shared IRQ */
 			if (irq < 0 && (regval & BIT(WAKE_STS_OFF)))