[PATCH 1/2] Input/spear-keyboard: Fix for balancing the enable_irq_wake in Power Mgmt

From: Deepak Sikri <redacted>

This patch handles the fix for unbalanced irq for the cases when
enable_irq_wake fails, and a warning related to same is displayed
on the console. The workaround is handled at the driver level.

Signed-off-by: Deepak Sikri <redacted>
Signed-off-by: Viresh Kumar <redacted>
Signed-off-by: Rajeev Kumar <redacted>
---
 drivers/input/keyboard/spear-keyboard.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/input/keyboard/spear-keyboard.c b/drivers/input/keyboard/spear-keyboard.c
index 0e7f686..6eb2dd8 100644
--- a/drivers/input/keyboard/spear-keyboard.c
+++ b/drivers/input/keyboard/spear-keyboard.c

@@ -58,6 +58,7 @@ struct spear_kbd {
 	void __iomem *io_base;
 	struct clk *clk;
 	unsigned int irq;
+	unsigned int irq_wake;
 	unsigned short last_key;
 	unsigned int mode;
 	unsigned short keycodes[256];

@@ -287,8 +288,10 @@ static int spear_kbd_suspend(struct device *dev)
 	if (input_dev->users)
 		clk_enable(kbd->clk);
 
-	if (device_may_wakeup(&pdev->dev))
-		enable_irq_wake(kbd->irq);
+	if (device_may_wakeup(&pdev->dev)) {
+		if (!enable_irq_wake(kbd->irq))
+			kbd->irq_wake = 1;
+	}
 
 	mutex_unlock(&input_dev->mutex);
 

@@ -303,8 +306,12 @@ static int spear_kbd_resume(struct device *dev)
 
 	mutex_lock(&input_dev->mutex);
 
-	if (device_may_wakeup(&pdev->dev))
-		disable_irq_wake(kbd->irq);
+	if (device_may_wakeup(&pdev->dev)) {
+		if (kbd->irq_wake) {
+			kbd->irq_wake = 0;
+			disable_irq_wake(kbd->irq);
+		}
+	}
 
 	if (input_dev->users)
 		clk_enable(kbd->clk);

-- 
1.7.8.110.g4cb5d

Thaw and poweroff routines are missing for spear-keyboard. They are required for:
- Error case scenarios during freeze
- Using test features, of hibernate.

Signed-off-by: Viresh Kumar <redacted>
Signed-off-by: Viresh Kumar <redacted>
---
 drivers/input/keyboard/spear-keyboard.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/drivers/input/keyboard/spear-keyboard.c b/drivers/input/keyboard/spear-keyboard.c
index 6eb2dd8..6e83828 100644
--- a/drivers/input/keyboard/spear-keyboard.c
+++ b/drivers/input/keyboard/spear-keyboard.c

@@ -321,10 +321,7 @@ static int spear_kbd_resume(struct device *dev)
 	return 0;
 }
 
-static const struct dev_pm_ops spear_kbd_pm_ops = {
-	.suspend	= spear_kbd_suspend,
-	.resume		= spear_kbd_resume,
-};
+static SIMPLE_DEV_PM_OPS(spear_kbd_pm_ops, spear_kbd_suspend, spear_kbd_resume);
 #endif
 
 static struct platform_driver spear_kbd_driver = {

-- 
1.7.8.110.g4cb5d

On 2/23/2012 3:10 PM, Viresh KUMAR wrote:

Thaw and poweroff routines are missing for spear-keyboard. They are required for:
- Error case scenarios during freeze
- Using test features, of hibernate.

Signed-off-by: Viresh Kumar <redacted>
Signed-off-by: Viresh Kumar <redacted>

Sorry for the duplicate SOB :(

-- 
viresh

On Thursday 23 February 2012 03:10 PM, Viresh Kumar wrote:

Thaw and poweroff routines are missing for spear-keyboard. They are required for:
- Error case scenarios during freeze
- Using test features, of hibernate.

Not a comment doubt

When is thaw called and when is power off called?

Now that you have converted to simple dev pm can you remove the #ifdef
from pm.

+static SIMPLE_DEV_PM_OPS(spear_kbd_pm_ops, spear_kbd_suspend, spear_kbd_resume);
 #endif
 
 static struct platform_driver spear_kbd_driver = {

On 2/23/2012 4:59 PM, Shubhrajyoti wrote:

quoted

quoted

- Using test features, of hibernate.

Not a comment doubt

When is thaw called and when is power off called?

Firstly freeze is called, after that snapshot of memory is taken
(to be saved in disk). Now, we need resume harddisk, so we get our
thaw routines called (analogous to resume). Now system is up again. Save
image to disk and call poweroff() to finally shutdown.

Refer Documentation/power/devices.txt to get complete sequence of calls.

quoted

quoted

diff --git a/drivers/input/keyboard/spear-keyboard.c b/drivers/input/keyboard/spear-keyboard.c
index 6eb2dd8..6e83828 100644
--- a/drivers/input/keyboard/spear-keyboard.c
+++ b/drivers/input/keyboard/spear-keyboard.c

@@ -321,10 +321,7 @@ static int spear_kbd_resume(struct device *dev)
 	return 0;
 }
 
-static const struct dev_pm_ops spear_kbd_pm_ops = {
-	.suspend	= spear_kbd_suspend,
-	.resume		= spear_kbd_resume,
-};

Now that you have converted to simple dev pm can you remove the #ifdef
from pm.

I didn't get it completely. CONFIG_PM Macro's are still required.

-- 
viresh

On 2/23/2012 5:26 PM, Viresh Kumar wrote:

quoted

quoted

quoted

quoted

quoted

quoted

-static const struct dev_pm_ops spear_kbd_pm_ops = {
-	.suspend	= spear_kbd_suspend,
-	.resume		= spear_kbd_resume,
-};

Now that you have converted to simple dev pm can you remove the #ifdef
from pm.

I didn't get it completely. CONFIG_PM Macro's are still required.

Sorry. Got it now. Will remove it. :)

-- 
viresh

On Thursday 23 February 2012 05:26 PM, Viresh Kumar wrote:

On 2/23/2012 4:59 PM, Shubhrajyoti wrote:

quoted

quoted

quoted

- Using test features, of hibernate.

Not a comment doubt

When is thaw called and when is power off called?

Firstly freeze is called, after that snapshot of memory is taken
(to be saved in disk). Now, we need resume harddisk, so we get our
thaw routines called (analogous to resume). Now system is up again. Save
image to disk and call poweroff() to finally shutdown.

Refer Documentation/power/devices.txt to get complete sequence of calls.

Makes sense thanks.

quoted

quoted

quoted

diff --git a/drivers/input/keyboard/spear-keyboard.c b/drivers/input/keyboard/spear-keyboard.c
index 6eb2dd8..6e83828 100644
--- a/drivers/input/keyboard/spear-keyboard.c
+++ b/drivers/input/keyboard/spear-keyboard.c

@@ -321,10 +321,7 @@ static int spear_kbd_resume(struct device *dev)
 	return 0;
 }
 
-static const struct dev_pm_ops spear_kbd_pm_ops = {
-	.suspend	= spear_kbd_suspend,
-	.resume		= spear_kbd_resume,
-};

Now that you have converted to simple dev pm can you remove the #ifdef
from pm.

I didn't get it completely. CONFIG_PM Macro's are still required.

The dev pm makes it empty so not required.

Thaw and poweroff routines are missing for spear-keyboard. They are required for:
- Error case scenarios during freeze
- Using test features, of hibernate.

Signed-off-by: Viresh Kumar <redacted>
Signed-off-by: Rajeev Kumar <redacted>
---
Changes since V1:
- moved spear_kbd_pm_ops's definition/usage outside of CONFIG_PM macro

 drivers/input/keyboard/spear-keyboard.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/input/keyboard/spear-keyboard.c b/drivers/input/keyboard/spear-keyboard.c
index 6eb2dd8..782e947 100644
--- a/drivers/input/keyboard/spear-keyboard.c
+++ b/drivers/input/keyboard/spear-keyboard.c

@@ -320,22 +320,17 @@ static int spear_kbd_resume(struct device *dev)
 
 	return 0;
 }
-
-static const struct dev_pm_ops spear_kbd_pm_ops = {
-	.suspend	= spear_kbd_suspend,
-	.resume		= spear_kbd_resume,
-};
 #endif
 
+static SIMPLE_DEV_PM_OPS(spear_kbd_pm_ops, spear_kbd_suspend, spear_kbd_resume);
+
 static struct platform_driver spear_kbd_driver = {
 	.probe		= spear_kbd_probe,
 	.remove		= __devexit_p(spear_kbd_remove),
 	.driver		= {
 		.name	= "keyboard",
 		.owner	= THIS_MODULE,
-#ifdef CONFIG_PM
 		.pm	= &spear_kbd_pm_ops,
-#endif
 	},
 };
 module_platform_driver(spear_kbd_driver);

-- 
1.7.8.110.g4cb5d

On Thu, Feb 23, 2012 at 03:10:51PM +0530, Viresh Kumar wrote:

From: Deepak Sikri <redacted>

This patch handles the fix for unbalanced irq for the cases when
enable_irq_wake fails, and a warning related to same is displayed
on the console. The workaround is handled at the driver level.

So why is the device marked as wakeup capable?

Thanks.

-- 
Dmitry

Hi,


On 2/24/2012 2:04 PM, Dmitry Torokhov wrote:

On Thu, Feb 23, 2012 at 03:10:51PM +0530, Viresh Kumar wrote:

quoted

From: Deepak Sikri<redacted>

This patch handles the fix for unbalanced irq for the cases when
enable_irq_wake fails, and a warning related to same is displayed
on the console. The workaround is handled at the driver level.

So why is the device marked as wakeup capable?

The device is capable of wake up. There are cases in which the 
enable_irq_wake call may fail.

This was specifically observed for ARM cortex architecture kernel 
implementation for GIC (PL390).
The 2.6.37 kernel versions and before do not have the callbacks related 
to function that help to set up
the wake up interrupts  in GIC, and enable_irq_wake returns with an 
error code -ENXIO.

Now if the user tries to disable the wake up interrupts in the resume 
routines without checking in for the cases
where the enable_irq_wake failed, kernel returns a warning related to 
unbalanced irq as we are trying to disable
an interrupt which was not enabled.

However, in case the enable_irq_wake is successful ( as was in case of 
ARM9 architectures with VIC callbacks
present for setting up wake up interrupts present), this check allowed 
to keep track of success cases.



Regards
Deepak

On Fri, Feb 24, 2012 at 02:28:25PM +0530, deepaksi wrote:

Hi,


On 2/24/2012 2:04 PM, Dmitry Torokhov wrote:

quoted

On Thu, Feb 23, 2012 at 03:10:51PM +0530, Viresh Kumar wrote:

quoted

From: Deepak Sikri<redacted>

This patch handles the fix for unbalanced irq for the cases when
enable_irq_wake fails, and a warning related to same is displayed
on the console. The workaround is handled at the driver level.

So why is the device marked as wakeup capable?

The device is capable of wake up. There are cases in which the
enable_irq_wake call may fail.

In this case I'd say the proper action would be to abort suspend instead
of continuing.

This was specifically observed for ARM cortex architecture kernel
implementation for GIC (PL390).
The 2.6.37 kernel versions and before do not have the callbacks
related to function that help to set up
the wake up interrupts  in GIC, and enable_irq_wake returns with an
error code -ENXIO.

Right, but current mainline does have it set up properly, correct? IOW
we do not really expect enable_irq_wake() to fail if device is wakeup
capable, correct?

Thanks.

-- 
Dmitry

On 2/24/2012 2:32 PM, Dmitry Torokhov wrote:

On Fri, Feb 24, 2012 at 02:28:25PM +0530, deepaksi wrote:

quoted

Hi,


On 2/24/2012 2:04 PM, Dmitry Torokhov wrote:

quoted

On Thu, Feb 23, 2012 at 03:10:51PM +0530, Viresh Kumar wrote:

quoted

From: Deepak Sikri<redacted>

This patch handles the fix for unbalanced irq for the cases when
enable_irq_wake fails, and a warning related to same is displayed
on the console. The workaround is handled at the driver level.

So why is the device marked as wakeup capable?

The device is capable of wake up. There are cases in which the
enable_irq_wake call may fail.

In this case I'd say the proper action would be to abort suspend instead
of continuing.

ok

quoted

This was specifically observed for ARM cortex architecture kernel
implementation for GIC (PL390).
The 2.6.37 kernel versions and before do not have the callbacks
related to function that help to set up
the wake up interrupts  in GIC, and enable_irq_wake returns with an
error code -ENXIO.

Right, but current mainline does have it set up properly, correct? IOW
we do not really expect enable_irq_wake() to fail if device is wakeup
capable, correct?

Thanks.

I agree the current mainline does have the support now. This was added 
some time back for 2.6.37.
Please do not consider this patch for the mainline.

Thanks
Deepak