[PATCH] hid: intel_ish-hid: ipc: register more pm callbacks to support hibernation

[PATCH] hid: intel_ish-hid: ipc: register more pm callbacks to support hibernation

[Srinivas Pandruvada]

From: Even Xu <even.xu@intel.com>

Current ish driver only register resume/suspend PM callbacks which
don't support hibernation (suspend to disk). Now use the
SIMPLE_DEV_PM_OPS() MACRO instead of struct dev_pm_ops directly.
The suspend and resume functions will now be used for both suspend
to RAM and hibernation.

If power management is disable, SIMPLE_DEV_PM_OPS will do nothing,
the suspend and resume related functions won't be used, so mark them
as __maybe_unused to clarify that this is intended behavior, and
remove #ifdefs for power management.

Signed-off-by: Even Xu <even.xu@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ipc/pci-ish.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
index 582e449be9fe..a2c53ea3b5ed 100644
--- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
+++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
@@ -205,8 +205,7 @@ static void ish_remove(struct pci_dev *pdev)
 	kfree(ishtp_dev);
 }
 
-#ifdef CONFIG_PM
-static struct device *ish_resume_device;
+static struct device __maybe_unused *ish_resume_device;
 
 /* 50ms to get resume response */
 #define WAIT_FOR_RESUME_ACK_MS		50
@@ -220,7 +219,7 @@ static struct device *ish_resume_device;
  * in that case a simple resume message is enough, others we need
  * a reset sequence.
  */
-static void ish_resume_handler(struct work_struct *work)
+static void __maybe_unused ish_resume_handler(struct work_struct *work)
 {
 	struct pci_dev *pdev = to_pci_dev(ish_resume_device);
 	struct ishtp_device *dev = pci_get_drvdata(pdev);
@@ -262,7 +261,7 @@ static void ish_resume_handler(struct work_struct *work)
  *
  * Return: 0 to the pm core
  */
-static int ish_suspend(struct device *device)
+static int __maybe_unused ish_suspend(struct device *device)
 {
 	struct pci_dev *pdev = to_pci_dev(device);
 	struct ishtp_device *dev = pci_get_drvdata(pdev);
@@ -288,7 +287,7 @@ static int ish_suspend(struct device *device)
 	return 0;
 }
 
-static DECLARE_WORK(resume_work, ish_resume_handler);
+static __maybe_unused DECLARE_WORK(resume_work, ish_resume_handler);
 /**
  * ish_resume() - ISH resume callback
  * @device:	device pointer
@@ -297,7 +296,7 @@ static DECLARE_WORK(resume_work, ish_resume_handler);
  *
  * Return: 0 to the pm core
  */
-static int ish_resume(struct device *device)
+static int __maybe_unused ish_resume(struct device *device)
 {
 	struct pci_dev *pdev = to_pci_dev(device);
 	struct ishtp_device *dev = pci_get_drvdata(pdev);
@@ -311,21 +310,14 @@ static int ish_resume(struct device *device)
 	return 0;
 }
 
-static const struct dev_pm_ops ish_pm_ops = {
-	.suspend = ish_suspend,
-	.resume = ish_resume,
-};
-#define ISHTP_ISH_PM_OPS	(&ish_pm_ops)
-#else
-#define ISHTP_ISH_PM_OPS	NULL
-#endif /* CONFIG_PM */
+static SIMPLE_DEV_PM_OPS(ish_pm_ops, ish_suspend, ish_resume);
 
 static struct pci_driver ish_driver = {
 	.name = KBUILD_MODNAME,
 	.id_table = ish_pci_tbl,
 	.probe = ish_probe,
 	.remove = ish_remove,
-	.driver.pm = ISHTP_ISH_PM_OPS,
+	.driver.pm = &ish_pm_ops,
 };
 
 module_pci_driver(ish_driver);
-- 
2.13.6

Re: [PATCH] hid: intel_ish-hid: ipc: register more pm callbacks to support hibernation

[Jiri Kosina]

On Sun, 10 Jun 2018, Srinivas Pandruvada wrote:

> From: Even Xu <even.xu@intel.com>
> 
> Current ish driver only register resume/suspend PM callbacks which
> don't support hibernation (suspend to disk). Now use the
> SIMPLE_DEV_PM_OPS() MACRO instead of struct dev_pm_ops directly.
> The suspend and resume functions will now be used for both suspend
> to RAM and hibernation.
> 
> If power management is disable, SIMPLE_DEV_PM_OPS will do nothing,
> the suspend and resume related functions won't be used, so mark them
> as __maybe_unused to clarify that this is intended behavior, and
> remove #ifdefs for power management.

This describes details the patch does on code level, but what are the user 
observable effects? Hibernation resume doesn't fail any more? Hibernation 
is possible (and wasn't before)? Did kernel crash while trying to 
hibernate and this is the fix? Or ... ?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] hid: intel_ish-hid: ipc: register more pm callbacks to support hibernation

[Srinivas Pandruvada]

On Tue, 2018-06-12 at 16:53 +0200, Jiri Kosina wrote:
> On Sun, 10 Jun 2018, Srinivas Pandruvada wrote:
> 
> > From: Even Xu <even.xu@intel.com>
> > 
> > Current ish driver only register resume/suspend PM callbacks which
> > don't support hibernation (suspend to disk). Now use the
> > SIMPLE_DEV_PM_OPS() MACRO instead of struct dev_pm_ops directly.
> > The suspend and resume functions will now be used for both suspend
> > to RAM and hibernation.
> > 
> > If power management is disable, SIMPLE_DEV_PM_OPS will do nothing,
> > the suspend and resume related functions won't be used, so mark
> > them
> > as __maybe_unused to clarify that this is intended behavior, and
> > remove #ifdefs for power management.
> 
> This describes details the patch does on code level, but what are the
> user 
> observable effects? Hibernation resume doesn't fail any more?
> Hibernation 
> is possible (and wasn't before)? Did kernel crash while trying to 
> hibernate and this is the fix? Or ... ?
Even,
Can you add more details and resubmit ASAP?

Basically after hiberation, the ISH can't resume properly and user may
not see sensor events (for example: screen rotation may not work).
User will not see a crash or panic or anything except the following
message in log:
hid-sensor-hub 001F:8086:22D8.0001: timeout waiting for response from 
ISHTP device 

So this is adding support for S4/hiberbation to ISH.



Thanks,
Srinivas

> 
> Thanks,
> 

Re: [PATCH] hid: intel_ish-hid: ipc: register more pm callbacks to support hibernation

[Srinivas Pandruvada]

On Tue, 2018-06-12 at 16:53 +0200, Jiri Kosina wrote:
> On Sun, 10 Jun 2018, Srinivas Pandruvada wrote:
> 
> > From: Even Xu <even.xu@intel.com>
> > 
> > Current ish driver only register resume/suspend PM callbacks which
> > don't support hibernation (suspend to disk). Now use the
> > SIMPLE_DEV_PM_OPS() MACRO instead of struct dev_pm_ops directly.
> > The suspend and resume functions will now be used for both suspend
> > to RAM and hibernation.
> > 
> > If power management is disable, SIMPLE_DEV_PM_OPS will do nothing,
> > the suspend and resume related functions won't be used, so mark
> > them
> > as __maybe_unused to clarify that this is intended behavior, and
> > remove #ifdefs for power management.
> 
> This describes details the patch does on code level, but what are the
> user 
> observable effects? Hibernation resume doesn't fail any more?
> Hibernation 
> is possible (and wasn't before)? Did kernel crash while trying to 
> hibernate and this is the fix? Or ... ?
Even,
Can you add more details and resubmit ASAP?

Basically after hiberation, the ISH can't resume properly and user may
not see sensor events (for example: screen rotation may not work).
User will not see a crash or panic or anything except the following
message in log:
hid-sensor-hub 001F:8086:22D8.0001: timeout waiting for response from 
ISHTP device 

So this is adding support for S4/hiberbation to ISH.



Thanks,
Srinivas

> 
> Thanks,
> 

RE: [PATCH] hid: intel_ish-hid: ipc: register more pm callbacks to support hibernation

[Xu, Even]

Ok, sure, I will update patch comments and resubmit.
Thanks Srinivas!

Best Regards,
Even Xu



-----Original Message-----
From: Srinivas Pandruvada [mailto:srinivas.pandruvada@linux.intel.com] 
Sent: Tuesday, June 12, 2018 11:31 PM
To: Jiri Kosina <jikos@kernel.org>; Xu, Even <even.xu@intel.com>
Cc: benjamin.tissoires@redhat.com; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; Xu, Even <even.xu@intel.com>
Subject: Re: [PATCH] hid: intel_ish-hid: ipc: register more pm callbacks to support hibernation

On Tue, 2018-06-12 at 16:53 +0200, Jiri Kosina wrote:
> On Sun, 10 Jun 2018, Srinivas Pandruvada wrote:
> 
> > From: Even Xu <even.xu@intel.com>
> > 
> > Current ish driver only register resume/suspend PM callbacks which 
> > don't support hibernation (suspend to disk). Now use the
> > SIMPLE_DEV_PM_OPS() MACRO instead of struct dev_pm_ops directly.
> > The suspend and resume functions will now be used for both suspend 
> > to RAM and hibernation.
> > 
> > If power management is disable, SIMPLE_DEV_PM_OPS will do nothing, 
> > the suspend and resume related functions won't be used, so mark them 
> > as __maybe_unused to clarify that this is intended behavior, and 
> > remove #ifdefs for power management.
> 
> This describes details the patch does on code level, but what are the 
> user observable effects? Hibernation resume doesn't fail any more?
> Hibernation
> is possible (and wasn't before)? Did kernel crash while trying to 
> hibernate and this is the fix? Or ... ?
Even,
Can you add more details and resubmit ASAP?

Basically after hiberation, the ISH can't resume properly and user may not see sensor events (for example: screen rotation may not work).
User will not see a crash or panic or anything except the following message in log:
hid-sensor-hub 001F:8086:22D8.0001: timeout waiting for response from ISHTP device 

So this is adding support for S4/hiberbation to ISH.



Thanks,
Srinivas

> 
> Thanks,
> 

Re: [PATCH] hid: intel_ish-hid: ipc: register more pm callbacks to support hibernation

[Srinivas Pandruvada]

On Wed, 2018-06-13 at 00:05 +0000, Xu, Even wrote:
> Ok, sure, I will update patch comments and resubmit.
Can you also add in the sign-off area


Cc: stable@vger.kernel.org

Thanks,
Srinivas

> Thanks Srinivas!
> 
> Best Regards,
> Even Xu
> 
> 
> 
> -----Original Message-----
> From: Srinivas Pandruvada [mailto:srinivas.pandruvada@linux.intel.com
> ] 
> Sent: Tuesday, June 12, 2018 11:31 PM
> To: Jiri Kosina <jikos@kernel.org>; Xu, Even <even.xu@intel.com>
> Cc: benjamin.tissoires@redhat.com; linux-input@vger.kernel.org; linux
> -kernel@vger.kernel.org; Xu, Even <even.xu@intel.com>
> Subject: Re: [PATCH] hid: intel_ish-hid: ipc: register more pm
> callbacks to support hibernation
> 
> On Tue, 2018-06-12 at 16:53 +0200, Jiri Kosina wrote:
> > On Sun, 10 Jun 2018, Srinivas Pandruvada wrote:
> > 
> > > From: Even Xu <even.xu@intel.com>
> > > 
> > > Current ish driver only register resume/suspend PM callbacks
> > > which 
> > > don't support hibernation (suspend to disk). Now use the
> > > SIMPLE_DEV_PM_OPS() MACRO instead of struct dev_pm_ops directly.
> > > The suspend and resume functions will now be used for both
> > > suspend 
> > > to RAM and hibernation.
> > > 
> > > If power management is disable, SIMPLE_DEV_PM_OPS will do
> > > nothing, 
> > > the suspend and resume related functions won't be used, so mark
> > > them 
> > > as __maybe_unused to clarify that this is intended behavior, and 
> > > remove #ifdefs for power management.
> > 
> > This describes details the patch does on code level, but what are
> > the 
> > user observable effects? Hibernation resume doesn't fail any more?
> > Hibernation
> > is possible (and wasn't before)? Did kernel crash while trying to 
> > hibernate and this is the fix? Or ... ?
> 
> Even,
> Can you add more details and resubmit ASAP?
> 
> Basically after hiberation, the ISH can't resume properly and user
> may not see sensor events (for example: screen rotation may not
> work).
> User will not see a crash or panic or anything except the following
> message in log:
> hid-sensor-hub 001F:8086:22D8.0001: timeout waiting for response from
> ISHTP device 
> 
> So this is adding support for S4/hiberbation to ISH.
> 
> 
> 
> Thanks,
> Srinivas
> 
> > 
> > Thanks,
> > 

RE: [PATCH] hid: intel_ish-hid: ipc: register more pm callbacks to support hibernation

[Xu, Even]

Hi, Jiri Kosina,

If without this patch, the platform with ISH, its hibernation resume will take more than 10s because of ISH resume failure, it will also cause ISH not functional.
With this patch, everything will go will.

Best Regards,
Even Xu


-----Original Message-----
From: Jiri Kosina [mailto:jikos@kernel.org] 
Sent: Tuesday, June 12, 2018 10:53 PM
To: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: benjamin.tissoires@redhat.com; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; Xu, Even <even.xu@intel.com>
Subject: Re: [PATCH] hid: intel_ish-hid: ipc: register more pm callbacks to support hibernation

On Sun, 10 Jun 2018, Srinivas Pandruvada wrote:

> From: Even Xu <even.xu@intel.com>
> 
> Current ish driver only register resume/suspend PM callbacks which 
> don't support hibernation (suspend to disk). Now use the
> SIMPLE_DEV_PM_OPS() MACRO instead of struct dev_pm_ops directly.
> The suspend and resume functions will now be used for both suspend to 
> RAM and hibernation.
> 
> If power management is disable, SIMPLE_DEV_PM_OPS will do nothing, the 
> suspend and resume related functions won't be used, so mark them as 
> __maybe_unused to clarify that this is intended behavior, and remove 
> #ifdefs for power management.

This describes details the patch does on code level, but what are the user observable effects? Hibernation resume doesn't fail any more? Hibernation is possible (and wasn't before)? Did kernel crash while trying to hibernate and this is the fix? Or ... ?

Thanks,

--
Jiri Kosina
SUSE Labs

[PATCH] hid: intel_ish-hid: ipc: register more pm callbacks to support hibernation

[Even Xu]

Current ISH driver only registers suspend/resume PM callbacks which don't
support hibernation (suspend to disk). Basically after hiberation, the ISH
can't resume properly and user may not see sensor events
(for example: screen rotation may not work).

User will not see a crash or panic or anything except the following message
in log:
hid-sensor-hub 001F:8086:22D8.0001: timeout waiting for response from ISHTP device

So this patch adds support for S4/hiberbation to ISH by using the
SIMPLE_DEV_PM_OPS() MACRO instead of struct dev_pm_ops directly. The suspend
and resume functions will now be used for both suspend to RAM and hibernation.

If power management is disabled, SIMPLE_DEV_PM_OPS will do nothing, the suspend
and resume related functions won't be used, so mark them as __maybe_unused to
clarify that this is the intended behavior, and remove #ifdefs for power
management.

Cc: stable@vger.kernel.org
Signed-off-by: Even Xu <even.xu@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ipc/pci-ish.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
index 582e449..a2c53ea 100644
--- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
+++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
@@ -205,8 +205,7 @@ static void ish_remove(struct pci_dev *pdev)
 	kfree(ishtp_dev);
 }
 
-#ifdef CONFIG_PM
-static struct device *ish_resume_device;
+static struct device __maybe_unused *ish_resume_device;
 
 /* 50ms to get resume response */
 #define WAIT_FOR_RESUME_ACK_MS		50
@@ -220,7 +219,7 @@ static struct device *ish_resume_device;
  * in that case a simple resume message is enough, others we need
  * a reset sequence.
  */
-static void ish_resume_handler(struct work_struct *work)
+static void __maybe_unused ish_resume_handler(struct work_struct *work)
 {
 	struct pci_dev *pdev = to_pci_dev(ish_resume_device);
 	struct ishtp_device *dev = pci_get_drvdata(pdev);
@@ -262,7 +261,7 @@ static void ish_resume_handler(struct work_struct *work)
  *
  * Return: 0 to the pm core
  */
-static int ish_suspend(struct device *device)
+static int __maybe_unused ish_suspend(struct device *device)
 {
 	struct pci_dev *pdev = to_pci_dev(device);
 	struct ishtp_device *dev = pci_get_drvdata(pdev);
@@ -288,7 +287,7 @@ static int ish_suspend(struct device *device)
 	return 0;
 }
 
-static DECLARE_WORK(resume_work, ish_resume_handler);
+static __maybe_unused DECLARE_WORK(resume_work, ish_resume_handler);
 /**
  * ish_resume() - ISH resume callback
  * @device:	device pointer
@@ -297,7 +296,7 @@ static DECLARE_WORK(resume_work, ish_resume_handler);
  *
  * Return: 0 to the pm core
  */
-static int ish_resume(struct device *device)
+static int __maybe_unused ish_resume(struct device *device)
 {
 	struct pci_dev *pdev = to_pci_dev(device);
 	struct ishtp_device *dev = pci_get_drvdata(pdev);
@@ -311,21 +310,14 @@ static int ish_resume(struct device *device)
 	return 0;
 }
 
-static const struct dev_pm_ops ish_pm_ops = {
-	.suspend = ish_suspend,
-	.resume = ish_resume,
-};
-#define ISHTP_ISH_PM_OPS	(&ish_pm_ops)
-#else
-#define ISHTP_ISH_PM_OPS	NULL
-#endif /* CONFIG_PM */
+static SIMPLE_DEV_PM_OPS(ish_pm_ops, ish_suspend, ish_resume);
 
 static struct pci_driver ish_driver = {
 	.name = KBUILD_MODNAME,
 	.id_table = ish_pci_tbl,
 	.probe = ish_probe,
 	.remove = ish_remove,
-	.driver.pm = ISHTP_ISH_PM_OPS,
+	.driver.pm = &ish_pm_ops,
 };
 
 module_pci_driver(ish_driver);
-- 
2.7.4

Re: [PATCH] hid: intel_ish-hid: ipc: register more pm callbacks to support hibernation

[Jiri Kosina]

On Fri, 12 Feb 2016, Even Xu wrote:

> Current ISH driver only registers suspend/resume PM callbacks which don't
> support hibernation (suspend to disk). Basically after hiberation, the ISH
> can't resume properly and user may not see sensor events
> (for example: screen rotation may not work).
> 
> User will not see a crash or panic or anything except the following message
> in log:
> hid-sensor-hub 001F:8086:22D8.0001: timeout waiting for response from ISHTP device
> 
> So this patch adds support for S4/hiberbation to ISH by using the
> SIMPLE_DEV_PM_OPS() MACRO instead of struct dev_pm_ops directly. The suspend
> and resume functions will now be used for both suspend to RAM and hibernation.
> 
> If power management is disabled, SIMPLE_DEV_PM_OPS will do nothing, the suspend
> and resume related functions won't be used, so mark them as __maybe_unused to
> clarify that this is the intended behavior, and remove #ifdefs for power
> management.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Even Xu <even.xu@intel.com>
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

Applied to hid.git#for-4.18/upstream-fixes. Thanks,

-- 
Jiri Kosina
SUSE Labs