[PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup

[PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup

[Kai-Heng Feng]

System cannot suspend more than 255 times because the driver doesn't
have corresponding acpi_disable_gpe() for acpi_enable_gpe(), so the GPE
refcount overflows.

Since PCI core and ACPI core already handles PCI PME wake and GPE wake
when the device has wakeup capability, use device_init_wakeup() to let
them do the wakeup setting work.

Also add a shutdown callback which uses pci_prepare_to_sleep() to let
PCI and ACPI set OOB wakeup for S5.

Fixes: 2e23a70edabe ("HID: intel-ish-hid: ipc: finish power flow for EHL OOB")
Cc: Jian Hui Lee <jianhui.lee@canonical.com>
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/hid/intel-ish-hid/ipc/pci-ish.c | 59 +++++++------------------
 1 file changed, 15 insertions(+), 44 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
index 55cb25038e63..65e7eeb2fa64 100644
--- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
+++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
@@ -119,42 +119,6 @@ static inline bool ish_should_leave_d0i3(struct pci_dev *pdev)
 	return !pm_resume_via_firmware() || pdev->device == CHV_DEVICE_ID;
 }
 
-static int enable_gpe(struct device *dev)
-{
-#ifdef CONFIG_ACPI
-	acpi_status acpi_sts;
-	struct acpi_device *adev;
-	struct acpi_device_wakeup *wakeup;
-
-	adev = ACPI_COMPANION(dev);
-	if (!adev) {
-		dev_err(dev, "get acpi handle failed\n");
-		return -ENODEV;
-	}
-	wakeup = &adev->wakeup;
-
-	acpi_sts = acpi_enable_gpe(wakeup->gpe_device, wakeup->gpe_number);
-	if (ACPI_FAILURE(acpi_sts)) {
-		dev_err(dev, "enable ose_gpe failed\n");
-		return -EIO;
-	}
-
-	return 0;
-#else
-	return -ENODEV;
-#endif
-}
-
-static void enable_pme_wake(struct pci_dev *pdev)
-{
-	if ((pci_pme_capable(pdev, PCI_D0) ||
-	     pci_pme_capable(pdev, PCI_D3hot) ||
-	     pci_pme_capable(pdev, PCI_D3cold)) && !enable_gpe(&pdev->dev)) {
-		pci_pme_active(pdev, true);
-		dev_dbg(&pdev->dev, "ish ipc driver pme wake enabled\n");
-	}
-}
-
 /**
  * ish_probe() - PCI driver probe callback
  * @pdev:	pci device
@@ -225,7 +189,7 @@ static int ish_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	/* Enable PME for EHL */
 	if (pdev->device == EHL_Ax_DEVICE_ID)
-		enable_pme_wake(pdev);
+		device_init_wakeup(dev, true);
 
 	ret = ish_init(ishtp);
 	if (ret)
@@ -248,6 +212,19 @@ static void ish_remove(struct pci_dev *pdev)
 	ish_device_disable(ishtp_dev);
 }
 
+
+/**
+ * ish_shutdown() - PCI driver shutdown callback
+ * @pdev:	pci device
+ *
+ * This function sets up wakeup for S5
+ */
+static void ish_shutdown(struct pci_dev *pdev)
+{
+	if (pdev->device == EHL_Ax_DEVICE_ID)
+		pci_prepare_to_sleep(pdev);
+}
+
 static struct device __maybe_unused *ish_resume_device;
 
 /* 50ms to get resume response */
@@ -370,13 +347,6 @@ 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);
 
-	/* add this to finish power flow for EHL */
-	if (dev->pdev->device == EHL_Ax_DEVICE_ID) {
-		pci_set_power_state(pdev, PCI_D0);
-		enable_pme_wake(pdev);
-		dev_dbg(dev->devc, "set power state to D0 for ehl\n");
-	}
-
 	ish_resume_device = device;
 	dev->resume_flag = 1;
 
@@ -392,6 +362,7 @@ static struct pci_driver ish_driver = {
 	.id_table = ish_pci_tbl,
 	.probe = ish_probe,
 	.remove = ish_remove,
+	.shutdown = ish_shutdown,
 	.driver.pm = &ish_pm_ops,
 };
 
-- 
2.34.1

Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup

[srinivas pandruvada]

Hi Even,

On Thu, 2023-09-14 at 12:18 +0800, Kai-Heng Feng wrote:
> System cannot suspend more than 255 times because the driver doesn't
> have corresponding acpi_disable_gpe() for acpi_enable_gpe(), so the
> GPE
> refcount overflows.
> 
> Since PCI core and ACPI core already handles PCI PME wake and GPE
> wake
> when the device has wakeup capability, use device_init_wakeup() to
> let
> them do the wakeup setting work.
> 
> Also add a shutdown callback which uses pci_prepare_to_sleep() to let
> PCI and ACPI set OOB wakeup for S5.
> 
Please test this change.

Thanks,
Srinivas

> Fixes: 2e23a70edabe ("HID: intel-ish-hid: ipc: finish power flow for
> EHL OOB")
> Cc: Jian Hui Lee <jianhui.lee@canonical.com>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/hid/intel-ish-hid/ipc/pci-ish.c | 59 +++++++----------------
> --
>  1 file changed, 15 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> index 55cb25038e63..65e7eeb2fa64 100644
> --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> @@ -119,42 +119,6 @@ static inline bool ish_should_leave_d0i3(struct
> pci_dev *pdev)
>         return !pm_resume_via_firmware() || pdev->device ==
> CHV_DEVICE_ID;
>  }
>  
> -static int enable_gpe(struct device *dev)
> -{
> -#ifdef CONFIG_ACPI
> -       acpi_status acpi_sts;
> -       struct acpi_device *adev;
> -       struct acpi_device_wakeup *wakeup;
> -
> -       adev = ACPI_COMPANION(dev);
> -       if (!adev) {
> -               dev_err(dev, "get acpi handle failed\n");
> -               return -ENODEV;
> -       }
> -       wakeup = &adev->wakeup;
> -
> -       acpi_sts = acpi_enable_gpe(wakeup->gpe_device, wakeup-
> >gpe_number);
> -       if (ACPI_FAILURE(acpi_sts)) {
> -               dev_err(dev, "enable ose_gpe failed\n");
> -               return -EIO;
> -       }
> -
> -       return 0;
> -#else
> -       return -ENODEV;
> -#endif
> -}
> -
> -static void enable_pme_wake(struct pci_dev *pdev)
> -{
> -       if ((pci_pme_capable(pdev, PCI_D0) ||
> -            pci_pme_capable(pdev, PCI_D3hot) ||
> -            pci_pme_capable(pdev, PCI_D3cold)) && !enable_gpe(&pdev-
> >dev)) {
> -               pci_pme_active(pdev, true);
> -               dev_dbg(&pdev->dev, "ish ipc driver pme wake
> enabled\n");
> -       }
> -}
> -
>  /**
>   * ish_probe() - PCI driver probe callback
>   * @pdev:      pci device
> @@ -225,7 +189,7 @@ static int ish_probe(struct pci_dev *pdev, const
> struct pci_device_id *ent)
>  
>         /* Enable PME for EHL */
>         if (pdev->device == EHL_Ax_DEVICE_ID)
> -               enable_pme_wake(pdev);
> +               device_init_wakeup(dev, true);
>  
>         ret = ish_init(ishtp);
>         if (ret)
> @@ -248,6 +212,19 @@ static void ish_remove(struct pci_dev *pdev)
>         ish_device_disable(ishtp_dev);
>  }
>  
> +
> +/**
> + * ish_shutdown() - PCI driver shutdown callback
> + * @pdev:      pci device
> + *
> + * This function sets up wakeup for S5
> + */
> +static void ish_shutdown(struct pci_dev *pdev)
> +{
> +       if (pdev->device == EHL_Ax_DEVICE_ID)
> +               pci_prepare_to_sleep(pdev);
> +}
> +
>  static struct device __maybe_unused *ish_resume_device;
>  
>  /* 50ms to get resume response */
> @@ -370,13 +347,6 @@ 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);
>  
> -       /* add this to finish power flow for EHL */
> -       if (dev->pdev->device == EHL_Ax_DEVICE_ID) {
> -               pci_set_power_state(pdev, PCI_D0);
> -               enable_pme_wake(pdev);
> -               dev_dbg(dev->devc, "set power state to D0 for
> ehl\n");
> -       }
> -
>         ish_resume_device = device;
>         dev->resume_flag = 1;
>  
> @@ -392,6 +362,7 @@ static struct pci_driver ish_driver = {
>         .id_table = ish_pci_tbl,
>         .probe = ish_probe,
>         .remove = ish_remove,
> +       .shutdown = ish_shutdown,
>         .driver.pm = &ish_pm_ops,
>  };
>  

Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup

[Bjorn Helgaas]

On Thu, Sep 14, 2023 at 12:18:05PM +0800, Kai-Heng Feng wrote:
> System cannot suspend more than 255 times because the driver doesn't
> have corresponding acpi_disable_gpe() for acpi_enable_gpe(), so the GPE
> refcount overflows.

How can a user know they are seeing this problem?  Is there a public
bug report for it?

> Since PCI core and ACPI core already handles PCI PME wake and GPE wake
> when the device has wakeup capability, use device_init_wakeup() to let
> them do the wakeup setting work.
> 
> Also add a shutdown callback which uses pci_prepare_to_sleep() to let
> PCI and ACPI set OOB wakeup for S5.

Is this logically required to be part of this patch, or could it be a
separate patch?

> Fixes: 2e23a70edabe ("HID: intel-ish-hid: ipc: finish power flow for EHL OOB")
> Cc: Jian Hui Lee <jianhui.lee@canonical.com>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/hid/intel-ish-hid/ipc/pci-ish.c | 59 +++++++------------------
>  1 file changed, 15 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> index 55cb25038e63..65e7eeb2fa64 100644
> --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> @@ -119,42 +119,6 @@ static inline bool ish_should_leave_d0i3(struct pci_dev *pdev)
>  	return !pm_resume_via_firmware() || pdev->device == CHV_DEVICE_ID;
>  }
>  
> -static int enable_gpe(struct device *dev)
> -{
> -#ifdef CONFIG_ACPI
> -	acpi_status acpi_sts;
> -	struct acpi_device *adev;
> -	struct acpi_device_wakeup *wakeup;
> -
> -	adev = ACPI_COMPANION(dev);
> -	if (!adev) {
> -		dev_err(dev, "get acpi handle failed\n");
> -		return -ENODEV;
> -	}
> -	wakeup = &adev->wakeup;
> -
> -	acpi_sts = acpi_enable_gpe(wakeup->gpe_device, wakeup->gpe_number);
> -	if (ACPI_FAILURE(acpi_sts)) {
> -		dev_err(dev, "enable ose_gpe failed\n");
> -		return -EIO;
> -	}
> -
> -	return 0;
> -#else
> -	return -ENODEV;
> -#endif
> -}
> -
> -static void enable_pme_wake(struct pci_dev *pdev)
> -{
> -	if ((pci_pme_capable(pdev, PCI_D0) ||
> -	     pci_pme_capable(pdev, PCI_D3hot) ||
> -	     pci_pme_capable(pdev, PCI_D3cold)) && !enable_gpe(&pdev->dev)) {
> -		pci_pme_active(pdev, true);
> -		dev_dbg(&pdev->dev, "ish ipc driver pme wake enabled\n");
> -	}
> -}

I LOVE the removal of all this code.  Thanks for doing it!

>  /**
>   * ish_probe() - PCI driver probe callback
>   * @pdev:	pci device
> @@ -225,7 +189,7 @@ static int ish_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  
>  	/* Enable PME for EHL */
>  	if (pdev->device == EHL_Ax_DEVICE_ID)
> -		enable_pme_wake(pdev);
> +		device_init_wakeup(dev, true);
>  
>  	ret = ish_init(ishtp);
>  	if (ret)
> @@ -248,6 +212,19 @@ static void ish_remove(struct pci_dev *pdev)
>  	ish_device_disable(ishtp_dev);
>  }
>  
> +
> +/**
> + * ish_shutdown() - PCI driver shutdown callback
> + * @pdev:	pci device
> + *
> + * This function sets up wakeup for S5
> + */
> +static void ish_shutdown(struct pci_dev *pdev)
> +{
> +	if (pdev->device == EHL_Ax_DEVICE_ID)
> +		pci_prepare_to_sleep(pdev);

There are only five drivers that use pci_prepare_to_sleep(), so I have
to ask what is special about this device that makes it necessary here?

It doesn't seem to match any of the scenarios mentioned in
Documentation/power/pci.rst for using pci_prepare_to_sleep().

Previously EHL_Ax_DEVICE_ID was used only in ish_probe(),
ish_resume(), and _dma_no_cache_snooping().  None of those look like
this, so this *looks* like new functionality that could/should be in a
separate patch.

> +}
> +
>  static struct device __maybe_unused *ish_resume_device;
>  
>  /* 50ms to get resume response */
> @@ -370,13 +347,6 @@ 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);
>  
> -	/* add this to finish power flow for EHL */
> -	if (dev->pdev->device == EHL_Ax_DEVICE_ID) {
> -		pci_set_power_state(pdev, PCI_D0);
> -		enable_pme_wake(pdev);
> -		dev_dbg(dev->devc, "set power state to D0 for ehl\n");
> -	}
> -
>  	ish_resume_device = device;
>  	dev->resume_flag = 1;
>  
> @@ -392,6 +362,7 @@ static struct pci_driver ish_driver = {
>  	.id_table = ish_pci_tbl,
>  	.probe = ish_probe,
>  	.remove = ish_remove,
> +	.shutdown = ish_shutdown,
>  	.driver.pm = &ish_pm_ops,
>  };
>  
> -- 
> 2.34.1
> 

RE: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup

[Xu, Even]

Hi, Srinivas,

Sure, I will test it.
As long term not working on EHL, I doesn't have EHL board on hand right now, I can test this patch on other ISH related platforms.
From the patch, it's focus on EHL platform, I assume Kai-Heng already verified the function on EHL board.
I don't think it will take effect on other platforms, anyway, I will test it on the platforms I have to provide cross platform verification.

Thanks!

Best Regards,
Even Xu

-----Original Message-----
From: srinivas pandruvada <srinivas.pandruvada@linux.intel.com> 
Sent: Friday, September 15, 2023 12:11 AM
To: Kai-Heng Feng <kai.heng.feng@canonical.com>; jikos@kernel.org; benjamin.tissoires@redhat.com
Cc: linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian Hui <jianhui.lee@canonical.com>; Xu, Even <even.xu@intel.com>; Zhang, Lixu <lixu.zhang@intel.com>; Ba, Najumon <najumon.ba@intel.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup

Hi Even,

On Thu, 2023-09-14 at 12:18 +0800, Kai-Heng Feng wrote:
> System cannot suspend more than 255 times because the driver doesn't 
> have corresponding acpi_disable_gpe() for acpi_enable_gpe(), so the 
> GPE refcount overflows.
> 
> Since PCI core and ACPI core already handles PCI PME wake and GPE wake 
> when the device has wakeup capability, use device_init_wakeup() to let 
> them do the wakeup setting work.
> 
> Also add a shutdown callback which uses pci_prepare_to_sleep() to let 
> PCI and ACPI set OOB wakeup for S5.
> 
Please test this change.

Thanks,
Srinivas

> Fixes: 2e23a70edabe ("HID: intel-ish-hid: ipc: finish power flow for 
> EHL OOB")
> Cc: Jian Hui Lee <jianhui.lee@canonical.com>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/hid/intel-ish-hid/ipc/pci-ish.c | 59 +++++++----------------
> --
>  1 file changed, 15 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> index 55cb25038e63..65e7eeb2fa64 100644
> --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> @@ -119,42 +119,6 @@ static inline bool ish_should_leave_d0i3(struct 
> pci_dev *pdev)
>         return !pm_resume_via_firmware() || pdev->device == 
> CHV_DEVICE_ID;
>  }
>  
> -static int enable_gpe(struct device *dev) -{ -#ifdef CONFIG_ACPI
> -       acpi_status acpi_sts;
> -       struct acpi_device *adev;
> -       struct acpi_device_wakeup *wakeup;
> -
> -       adev = ACPI_COMPANION(dev);
> -       if (!adev) {
> -               dev_err(dev, "get acpi handle failed\n");
> -               return -ENODEV;
> -       }
> -       wakeup = &adev->wakeup;
> -
> -       acpi_sts = acpi_enable_gpe(wakeup->gpe_device, wakeup-
> >gpe_number);
> -       if (ACPI_FAILURE(acpi_sts)) {
> -               dev_err(dev, "enable ose_gpe failed\n");
> -               return -EIO;
> -       }
> -
> -       return 0;
> -#else
> -       return -ENODEV;
> -#endif
> -}
> -
> -static void enable_pme_wake(struct pci_dev *pdev) -{
> -       if ((pci_pme_capable(pdev, PCI_D0) ||
> -            pci_pme_capable(pdev, PCI_D3hot) ||
> -            pci_pme_capable(pdev, PCI_D3cold)) && !enable_gpe(&pdev-
> >dev)) {
> -               pci_pme_active(pdev, true);
> -               dev_dbg(&pdev->dev, "ish ipc driver pme wake 
> enabled\n");
> -       }
> -}
> -
>  /**
>   * ish_probe() - PCI driver probe callback
>   * @pdev:      pci device
> @@ -225,7 +189,7 @@ static int ish_probe(struct pci_dev *pdev, const 
> struct pci_device_id *ent)
>  
>         /* Enable PME for EHL */
>         if (pdev->device == EHL_Ax_DEVICE_ID)
> -               enable_pme_wake(pdev);
> +               device_init_wakeup(dev, true);
>  
>         ret = ish_init(ishtp);
>         if (ret)
> @@ -248,6 +212,19 @@ static void ish_remove(struct pci_dev *pdev)
>         ish_device_disable(ishtp_dev);
>  }
>  
> +
> +/**
> + * ish_shutdown() - PCI driver shutdown callback
> + * @pdev:      pci device
> + *
> + * This function sets up wakeup for S5  */ static void 
> +ish_shutdown(struct pci_dev *pdev) {
> +       if (pdev->device == EHL_Ax_DEVICE_ID)
> +               pci_prepare_to_sleep(pdev); }
> +
>  static struct device __maybe_unused *ish_resume_device;
>  
>  /* 50ms to get resume response */
> @@ -370,13 +347,6 @@ 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);
>  
> -       /* add this to finish power flow for EHL */
> -       if (dev->pdev->device == EHL_Ax_DEVICE_ID) {
> -               pci_set_power_state(pdev, PCI_D0);
> -               enable_pme_wake(pdev);
> -               dev_dbg(dev->devc, "set power state to D0 for ehl\n");
> -       }
> -
>         ish_resume_device = device;
>         dev->resume_flag = 1;
>  
> @@ -392,6 +362,7 @@ static struct pci_driver ish_driver = {
>         .id_table = ish_pci_tbl,
>         .probe = ish_probe,
>         .remove = ish_remove,
> +       .shutdown = ish_shutdown,
>         .driver.pm = &ish_pm_ops,
>  };
>  

Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup

[Kai-Heng Feng]

On Fri, Sep 15, 2023 at 2:42 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, Sep 14, 2023 at 12:18:05PM +0800, Kai-Heng Feng wrote:
> > System cannot suspend more than 255 times because the driver doesn't
> > have corresponding acpi_disable_gpe() for acpi_enable_gpe(), so the GPE
> > refcount overflows.
>
> How can a user know they are seeing this problem?  Is there a public
> bug report for it?

Here's the error when it happens:
[46307.532037] intel_ish_ipc 0000:00:1d.0: enable ose_gpe failed

The ticket was filed privately.

>
> > Since PCI core and ACPI core already handles PCI PME wake and GPE wake
> > when the device has wakeup capability, use device_init_wakeup() to let
> > them do the wakeup setting work.
> >
> > Also add a shutdown callback which uses pci_prepare_to_sleep() to let
> > PCI and ACPI set OOB wakeup for S5.
>
> Is this logically required to be part of this patch, or could it be a
> separate patch?

Because that's what 2e23a70edabe ("HID: intel-ish-hid: ipc: finish
power flow for EHL OOB") intended to do, to enable wakeup from S5.

>
> > Fixes: 2e23a70edabe ("HID: intel-ish-hid: ipc: finish power flow for EHL OOB")
> > Cc: Jian Hui Lee <jianhui.lee@canonical.com>
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> >  drivers/hid/intel-ish-hid/ipc/pci-ish.c | 59 +++++++------------------
> >  1 file changed, 15 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > index 55cb25038e63..65e7eeb2fa64 100644
> > --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > @@ -119,42 +119,6 @@ static inline bool ish_should_leave_d0i3(struct pci_dev *pdev)
> >       return !pm_resume_via_firmware() || pdev->device == CHV_DEVICE_ID;
> >  }
> >
> > -static int enable_gpe(struct device *dev)
> > -{
> > -#ifdef CONFIG_ACPI
> > -     acpi_status acpi_sts;
> > -     struct acpi_device *adev;
> > -     struct acpi_device_wakeup *wakeup;
> > -
> > -     adev = ACPI_COMPANION(dev);
> > -     if (!adev) {
> > -             dev_err(dev, "get acpi handle failed\n");
> > -             return -ENODEV;
> > -     }
> > -     wakeup = &adev->wakeup;
> > -
> > -     acpi_sts = acpi_enable_gpe(wakeup->gpe_device, wakeup->gpe_number);
> > -     if (ACPI_FAILURE(acpi_sts)) {
> > -             dev_err(dev, "enable ose_gpe failed\n");
> > -             return -EIO;
> > -     }
> > -
> > -     return 0;
> > -#else
> > -     return -ENODEV;
> > -#endif
> > -}
> > -
> > -static void enable_pme_wake(struct pci_dev *pdev)
> > -{
> > -     if ((pci_pme_capable(pdev, PCI_D0) ||
> > -          pci_pme_capable(pdev, PCI_D3hot) ||
> > -          pci_pme_capable(pdev, PCI_D3cold)) && !enable_gpe(&pdev->dev)) {
> > -             pci_pme_active(pdev, true);
> > -             dev_dbg(&pdev->dev, "ish ipc driver pme wake enabled\n");
> > -     }
> > -}
>
> I LOVE the removal of all this code.  Thanks for doing it!

Thanks :)

>
> >  /**
> >   * ish_probe() - PCI driver probe callback
> >   * @pdev:    pci device
> > @@ -225,7 +189,7 @@ static int ish_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >
> >       /* Enable PME for EHL */
> >       if (pdev->device == EHL_Ax_DEVICE_ID)
> > -             enable_pme_wake(pdev);
> > +             device_init_wakeup(dev, true);
> >
> >       ret = ish_init(ishtp);
> >       if (ret)
> > @@ -248,6 +212,19 @@ static void ish_remove(struct pci_dev *pdev)
> >       ish_device_disable(ishtp_dev);
> >  }
> >
> > +
> > +/**
> > + * ish_shutdown() - PCI driver shutdown callback
> > + * @pdev:    pci device
> > + *
> > + * This function sets up wakeup for S5
> > + */
> > +static void ish_shutdown(struct pci_dev *pdev)
> > +{
> > +     if (pdev->device == EHL_Ax_DEVICE_ID)
> > +             pci_prepare_to_sleep(pdev);
>
> There are only five drivers that use pci_prepare_to_sleep(), so I have
> to ask what is special about this device that makes it necessary here?

The idea is that use pci_enable_wake() and pci_set_power_state() in
pci_prepare_to_sleep() to support S5 wakeup, by replacing the original
enable_pme_wake().

>
> It doesn't seem to match any of the scenarios mentioned in
> Documentation/power/pci.rst for using pci_prepare_to_sleep().
>
> Previously EHL_Ax_DEVICE_ID was used only in ish_probe(),
> ish_resume(), and _dma_no_cache_snooping().  None of those look like
> this, so this *looks* like new functionality that could/should be in a
> separate patch.

Because it's for S5 wakeup, so it's not a new functionality.

Kai-Heng

>
> > +}
> > +
> >  static struct device __maybe_unused *ish_resume_device;
> >
> >  /* 50ms to get resume response */
> > @@ -370,13 +347,6 @@ 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);
> >
> > -     /* add this to finish power flow for EHL */
> > -     if (dev->pdev->device == EHL_Ax_DEVICE_ID) {
> > -             pci_set_power_state(pdev, PCI_D0);
> > -             enable_pme_wake(pdev);
> > -             dev_dbg(dev->devc, "set power state to D0 for ehl\n");
> > -     }
> > -
> >       ish_resume_device = device;
> >       dev->resume_flag = 1;
> >
> > @@ -392,6 +362,7 @@ static struct pci_driver ish_driver = {
> >       .id_table = ish_pci_tbl,
> >       .probe = ish_probe,
> >       .remove = ish_remove,
> > +     .shutdown = ish_shutdown,
> >       .driver.pm = &ish_pm_ops,
> >  };
> >
> > --
> > 2.34.1
> >

Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup

[Kai-Heng Feng]

Hi Even,

On Fri, Sep 15, 2023 at 1:31 PM Xu, Even <even.xu@intel.com> wrote:
>
> Hi, Srinivas,
>
> Sure, I will test it.
> As long term not working on EHL, I doesn't have EHL board on hand right now, I can test this patch on other ISH related platforms.
> From the patch, it's focus on EHL platform, I assume Kai-Heng already verified the function on EHL board.

I only made sure the GPE overflow issue is fixed by the patch, but I
didn't test the S5 wakeup.
That's because I don't know how to test it on the EHL system I have.
I'll test it if you can let me know how to test the S5 wakeup.

Kai-Heng

> I don't think it will take effect on other platforms, anyway, I will test it on the platforms I have to provide cross platform verification.
>
> Thanks!
>
> Best Regards,
> Even Xu
>
> -----Original Message-----
> From: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
> Sent: Friday, September 15, 2023 12:11 AM
> To: Kai-Heng Feng <kai.heng.feng@canonical.com>; jikos@kernel.org; benjamin.tissoires@redhat.com
> Cc: linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian Hui <jianhui.lee@canonical.com>; Xu, Even <even.xu@intel.com>; Zhang, Lixu <lixu.zhang@intel.com>; Ba, Najumon <najumon.ba@intel.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
>
> Hi Even,
>
> On Thu, 2023-09-14 at 12:18 +0800, Kai-Heng Feng wrote:
> > System cannot suspend more than 255 times because the driver doesn't
> > have corresponding acpi_disable_gpe() for acpi_enable_gpe(), so the
> > GPE refcount overflows.
> >
> > Since PCI core and ACPI core already handles PCI PME wake and GPE wake
> > when the device has wakeup capability, use device_init_wakeup() to let
> > them do the wakeup setting work.
> >
> > Also add a shutdown callback which uses pci_prepare_to_sleep() to let
> > PCI and ACPI set OOB wakeup for S5.
> >
> Please test this change.
>
> Thanks,
> Srinivas
>
> > Fixes: 2e23a70edabe ("HID: intel-ish-hid: ipc: finish power flow for
> > EHL OOB")
> > Cc: Jian Hui Lee <jianhui.lee@canonical.com>
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> >  drivers/hid/intel-ish-hid/ipc/pci-ish.c | 59 +++++++----------------
> > --
> >  1 file changed, 15 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > index 55cb25038e63..65e7eeb2fa64 100644
> > --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > @@ -119,42 +119,6 @@ static inline bool ish_should_leave_d0i3(struct
> > pci_dev *pdev)
> >         return !pm_resume_via_firmware() || pdev->device ==
> > CHV_DEVICE_ID;
> >  }
> >
> > -static int enable_gpe(struct device *dev) -{ -#ifdef CONFIG_ACPI
> > -       acpi_status acpi_sts;
> > -       struct acpi_device *adev;
> > -       struct acpi_device_wakeup *wakeup;
> > -
> > -       adev = ACPI_COMPANION(dev);
> > -       if (!adev) {
> > -               dev_err(dev, "get acpi handle failed\n");
> > -               return -ENODEV;
> > -       }
> > -       wakeup = &adev->wakeup;
> > -
> > -       acpi_sts = acpi_enable_gpe(wakeup->gpe_device, wakeup-
> > >gpe_number);
> > -       if (ACPI_FAILURE(acpi_sts)) {
> > -               dev_err(dev, "enable ose_gpe failed\n");
> > -               return -EIO;
> > -       }
> > -
> > -       return 0;
> > -#else
> > -       return -ENODEV;
> > -#endif
> > -}
> > -
> > -static void enable_pme_wake(struct pci_dev *pdev) -{
> > -       if ((pci_pme_capable(pdev, PCI_D0) ||
> > -            pci_pme_capable(pdev, PCI_D3hot) ||
> > -            pci_pme_capable(pdev, PCI_D3cold)) && !enable_gpe(&pdev-
> > >dev)) {
> > -               pci_pme_active(pdev, true);
> > -               dev_dbg(&pdev->dev, "ish ipc driver pme wake
> > enabled\n");
> > -       }
> > -}
> > -
> >  /**
> >   * ish_probe() - PCI driver probe callback
> >   * @pdev:      pci device
> > @@ -225,7 +189,7 @@ static int ish_probe(struct pci_dev *pdev, const
> > struct pci_device_id *ent)
> >
> >         /* Enable PME for EHL */
> >         if (pdev->device == EHL_Ax_DEVICE_ID)
> > -               enable_pme_wake(pdev);
> > +               device_init_wakeup(dev, true);
> >
> >         ret = ish_init(ishtp);
> >         if (ret)
> > @@ -248,6 +212,19 @@ static void ish_remove(struct pci_dev *pdev)
> >         ish_device_disable(ishtp_dev);
> >  }
> >
> > +
> > +/**
> > + * ish_shutdown() - PCI driver shutdown callback
> > + * @pdev:      pci device
> > + *
> > + * This function sets up wakeup for S5  */ static void
> > +ish_shutdown(struct pci_dev *pdev) {
> > +       if (pdev->device == EHL_Ax_DEVICE_ID)
> > +               pci_prepare_to_sleep(pdev); }
> > +
> >  static struct device __maybe_unused *ish_resume_device;
> >
> >  /* 50ms to get resume response */
> > @@ -370,13 +347,6 @@ 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);
> >
> > -       /* add this to finish power flow for EHL */
> > -       if (dev->pdev->device == EHL_Ax_DEVICE_ID) {
> > -               pci_set_power_state(pdev, PCI_D0);
> > -               enable_pme_wake(pdev);
> > -               dev_dbg(dev->devc, "set power state to D0 for ehl\n");
> > -       }
> > -
> >         ish_resume_device = device;
> >         dev->resume_flag = 1;
> >
> > @@ -392,6 +362,7 @@ static struct pci_driver ish_driver = {
> >         .id_table = ish_pci_tbl,
> >         .probe = ish_probe,
> >         .remove = ish_remove,
> > +       .shutdown = ish_shutdown,
> >         .driver.pm = &ish_pm_ops,
> >  };
> >
>

RE: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup

[Xu, Even]

Hi, Kai-Heng,

I am also not familiar with this S5 wakeup test case.
I already sent out mails to ask for help on it.
Will come back to you once I get feedback.
Thanks!

Best Regards,
Even Xu

-----Original Message-----
From: Kai-Heng Feng <kai.heng.feng@canonical.com> 
Sent: Friday, September 15, 2023 2:01 PM
To: Xu, Even <even.xu@intel.com>
Cc: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>; jikos@kernel.org; benjamin.tissoires@redhat.com; linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian Hui <jianhui.lee@canonical.com>; Zhang, Lixu <lixu.zhang@intel.com>; Ba, Najumon <najumon.ba@intel.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup

Hi Even,

On Fri, Sep 15, 2023 at 1:31 PM Xu, Even <even.xu@intel.com> wrote:
>
> Hi, Srinivas,
>
> Sure, I will test it.
> As long term not working on EHL, I doesn't have EHL board on hand right now, I can test this patch on other ISH related platforms.
> From the patch, it's focus on EHL platform, I assume Kai-Heng already verified the function on EHL board.

I only made sure the GPE overflow issue is fixed by the patch, but I didn't test the S5 wakeup.
That's because I don't know how to test it on the EHL system I have.
I'll test it if you can let me know how to test the S5 wakeup.

Kai-Heng

> I don't think it will take effect on other platforms, anyway, I will test it on the platforms I have to provide cross platform verification.
>
> Thanks!
>
> Best Regards,
> Even Xu
>
> -----Original Message-----
> From: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
> Sent: Friday, September 15, 2023 12:11 AM
> To: Kai-Heng Feng <kai.heng.feng@canonical.com>; jikos@kernel.org; 
> benjamin.tissoires@redhat.com
> Cc: linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian Hui 
> <jianhui.lee@canonical.com>; Xu, Even <even.xu@intel.com>; Zhang, Lixu 
> <lixu.zhang@intel.com>; Ba, Najumon <najumon.ba@intel.com>; 
> linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
>
> Hi Even,
>
> On Thu, 2023-09-14 at 12:18 +0800, Kai-Heng Feng wrote:
> > System cannot suspend more than 255 times because the driver doesn't 
> > have corresponding acpi_disable_gpe() for acpi_enable_gpe(), so the 
> > GPE refcount overflows.
> >
> > Since PCI core and ACPI core already handles PCI PME wake and GPE 
> > wake when the device has wakeup capability, use device_init_wakeup() 
> > to let them do the wakeup setting work.
> >
> > Also add a shutdown callback which uses pci_prepare_to_sleep() to 
> > let PCI and ACPI set OOB wakeup for S5.
> >
> Please test this change.
>
> Thanks,
> Srinivas
>
> > Fixes: 2e23a70edabe ("HID: intel-ish-hid: ipc: finish power flow for 
> > EHL OOB")
> > Cc: Jian Hui Lee <jianhui.lee@canonical.com>
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> >  drivers/hid/intel-ish-hid/ipc/pci-ish.c | 59 
> > +++++++----------------
> > --
> >  1 file changed, 15 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > index 55cb25038e63..65e7eeb2fa64 100644
> > --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > @@ -119,42 +119,6 @@ static inline bool ish_should_leave_d0i3(struct 
> > pci_dev *pdev)
> >         return !pm_resume_via_firmware() || pdev->device == 
> > CHV_DEVICE_ID;  }
> >
> > -static int enable_gpe(struct device *dev) -{ -#ifdef CONFIG_ACPI
> > -       acpi_status acpi_sts;
> > -       struct acpi_device *adev;
> > -       struct acpi_device_wakeup *wakeup;
> > -
> > -       adev = ACPI_COMPANION(dev);
> > -       if (!adev) {
> > -               dev_err(dev, "get acpi handle failed\n");
> > -               return -ENODEV;
> > -       }
> > -       wakeup = &adev->wakeup;
> > -
> > -       acpi_sts = acpi_enable_gpe(wakeup->gpe_device, wakeup-
> > >gpe_number);
> > -       if (ACPI_FAILURE(acpi_sts)) {
> > -               dev_err(dev, "enable ose_gpe failed\n");
> > -               return -EIO;
> > -       }
> > -
> > -       return 0;
> > -#else
> > -       return -ENODEV;
> > -#endif
> > -}
> > -
> > -static void enable_pme_wake(struct pci_dev *pdev) -{
> > -       if ((pci_pme_capable(pdev, PCI_D0) ||
> > -            pci_pme_capable(pdev, PCI_D3hot) ||
> > -            pci_pme_capable(pdev, PCI_D3cold)) && !enable_gpe(&pdev-
> > >dev)) {
> > -               pci_pme_active(pdev, true);
> > -               dev_dbg(&pdev->dev, "ish ipc driver pme wake
> > enabled\n");
> > -       }
> > -}
> > -
> >  /**
> >   * ish_probe() - PCI driver probe callback
> >   * @pdev:      pci device
> > @@ -225,7 +189,7 @@ static int ish_probe(struct pci_dev *pdev, const 
> > struct pci_device_id *ent)
> >
> >         /* Enable PME for EHL */
> >         if (pdev->device == EHL_Ax_DEVICE_ID)
> > -               enable_pme_wake(pdev);
> > +               device_init_wakeup(dev, true);
> >
> >         ret = ish_init(ishtp);
> >         if (ret)
> > @@ -248,6 +212,19 @@ static void ish_remove(struct pci_dev *pdev)
> >         ish_device_disable(ishtp_dev);  }
> >
> > +
> > +/**
> > + * ish_shutdown() - PCI driver shutdown callback
> > + * @pdev:      pci device
> > + *
> > + * This function sets up wakeup for S5  */ static void 
> > +ish_shutdown(struct pci_dev *pdev) {
> > +       if (pdev->device == EHL_Ax_DEVICE_ID)
> > +               pci_prepare_to_sleep(pdev); }
> > +
> >  static struct device __maybe_unused *ish_resume_device;
> >
> >  /* 50ms to get resume response */
> > @@ -370,13 +347,6 @@ 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);
> >
> > -       /* add this to finish power flow for EHL */
> > -       if (dev->pdev->device == EHL_Ax_DEVICE_ID) {
> > -               pci_set_power_state(pdev, PCI_D0);
> > -               enable_pme_wake(pdev);
> > -               dev_dbg(dev->devc, "set power state to D0 for ehl\n");
> > -       }
> > -
> >         ish_resume_device = device;
> >         dev->resume_flag = 1;
> >
> > @@ -392,6 +362,7 @@ static struct pci_driver ish_driver = {
> >         .id_table = ish_pci_tbl,
> >         .probe = ish_probe,
> >         .remove = ish_remove,
> > +       .shutdown = ish_shutdown,
> >         .driver.pm = &ish_pm_ops,
> >  };
> >
>

RE: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup

[Xu, Even]

Hi, Kai-Heng,

I just got feedback, for testing EHL S5 wakeup feature, you need several steps to setup and access  "https://portal.devicewise.com/things/browse" to trigger wake.
But currently, our test account of this website are all out of data.
So maybe you need double check with the team who required you preparing the patch for the verification.
Thanks!

Best Regards,
Even Xu

-----Original Message-----
From: Xu, Even 
Sent: Friday, September 15, 2023 3:27 PM
To: Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>; jikos@kernel.org; benjamin.tissoires@redhat.com; linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian Hui <jianhui.lee@canonical.com>; Zhang, Lixu <Lixu.Zhang@intel.com>; Ba, Najumon <najumon.ba@intel.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: RE: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup

Hi, Kai-Heng,

I am also not familiar with this S5 wakeup test case.
I already sent out mails to ask for help on it.
Will come back to you once I get feedback.
Thanks!

Best Regards,
Even Xu

-----Original Message-----
From: Kai-Heng Feng <kai.heng.feng@canonical.com>
Sent: Friday, September 15, 2023 2:01 PM
To: Xu, Even <even.xu@intel.com>
Cc: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>; jikos@kernel.org; benjamin.tissoires@redhat.com; linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian Hui <jianhui.lee@canonical.com>; Zhang, Lixu <lixu.zhang@intel.com>; Ba, Najumon <najumon.ba@intel.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup

Hi Even,

On Fri, Sep 15, 2023 at 1:31 PM Xu, Even <even.xu@intel.com> wrote:
>
> Hi, Srinivas,
>
> Sure, I will test it.
> As long term not working on EHL, I doesn't have EHL board on hand right now, I can test this patch on other ISH related platforms.
> From the patch, it's focus on EHL platform, I assume Kai-Heng already verified the function on EHL board.

I only made sure the GPE overflow issue is fixed by the patch, but I didn't test the S5 wakeup.
That's because I don't know how to test it on the EHL system I have.
I'll test it if you can let me know how to test the S5 wakeup.

Kai-Heng

> I don't think it will take effect on other platforms, anyway, I will test it on the platforms I have to provide cross platform verification.
>
> Thanks!
>
> Best Regards,
> Even Xu
>
> -----Original Message-----
> From: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
> Sent: Friday, September 15, 2023 12:11 AM
> To: Kai-Heng Feng <kai.heng.feng@canonical.com>; jikos@kernel.org; 
> benjamin.tissoires@redhat.com
> Cc: linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian Hui 
> <jianhui.lee@canonical.com>; Xu, Even <even.xu@intel.com>; Zhang, Lixu 
> <lixu.zhang@intel.com>; Ba, Najumon <najumon.ba@intel.com>; 
> linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
>
> Hi Even,
>
> On Thu, 2023-09-14 at 12:18 +0800, Kai-Heng Feng wrote:
> > System cannot suspend more than 255 times because the driver doesn't 
> > have corresponding acpi_disable_gpe() for acpi_enable_gpe(), so the 
> > GPE refcount overflows.
> >
> > Since PCI core and ACPI core already handles PCI PME wake and GPE 
> > wake when the device has wakeup capability, use device_init_wakeup() 
> > to let them do the wakeup setting work.
> >
> > Also add a shutdown callback which uses pci_prepare_to_sleep() to 
> > let PCI and ACPI set OOB wakeup for S5.
> >
> Please test this change.
>
> Thanks,
> Srinivas
>
> > Fixes: 2e23a70edabe ("HID: intel-ish-hid: ipc: finish power flow for 
> > EHL OOB")
> > Cc: Jian Hui Lee <jianhui.lee@canonical.com>
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> >  drivers/hid/intel-ish-hid/ipc/pci-ish.c | 59
> > +++++++----------------
> > --
> >  1 file changed, 15 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > index 55cb25038e63..65e7eeb2fa64 100644
> > --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > @@ -119,42 +119,6 @@ static inline bool ish_should_leave_d0i3(struct 
> > pci_dev *pdev)
> >         return !pm_resume_via_firmware() || pdev->device == 
> > CHV_DEVICE_ID;  }
> >
> > -static int enable_gpe(struct device *dev) -{ -#ifdef CONFIG_ACPI
> > -       acpi_status acpi_sts;
> > -       struct acpi_device *adev;
> > -       struct acpi_device_wakeup *wakeup;
> > -
> > -       adev = ACPI_COMPANION(dev);
> > -       if (!adev) {
> > -               dev_err(dev, "get acpi handle failed\n");
> > -               return -ENODEV;
> > -       }
> > -       wakeup = &adev->wakeup;
> > -
> > -       acpi_sts = acpi_enable_gpe(wakeup->gpe_device, wakeup-
> > >gpe_number);
> > -       if (ACPI_FAILURE(acpi_sts)) {
> > -               dev_err(dev, "enable ose_gpe failed\n");
> > -               return -EIO;
> > -       }
> > -
> > -       return 0;
> > -#else
> > -       return -ENODEV;
> > -#endif
> > -}
> > -
> > -static void enable_pme_wake(struct pci_dev *pdev) -{
> > -       if ((pci_pme_capable(pdev, PCI_D0) ||
> > -            pci_pme_capable(pdev, PCI_D3hot) ||
> > -            pci_pme_capable(pdev, PCI_D3cold)) && !enable_gpe(&pdev-
> > >dev)) {
> > -               pci_pme_active(pdev, true);
> > -               dev_dbg(&pdev->dev, "ish ipc driver pme wake
> > enabled\n");
> > -       }
> > -}
> > -
> >  /**
> >   * ish_probe() - PCI driver probe callback
> >   * @pdev:      pci device
> > @@ -225,7 +189,7 @@ static int ish_probe(struct pci_dev *pdev, const 
> > struct pci_device_id *ent)
> >
> >         /* Enable PME for EHL */
> >         if (pdev->device == EHL_Ax_DEVICE_ID)
> > -               enable_pme_wake(pdev);
> > +               device_init_wakeup(dev, true);
> >
> >         ret = ish_init(ishtp);
> >         if (ret)
> > @@ -248,6 +212,19 @@ static void ish_remove(struct pci_dev *pdev)
> >         ish_device_disable(ishtp_dev);  }
> >
> > +
> > +/**
> > + * ish_shutdown() - PCI driver shutdown callback
> > + * @pdev:      pci device
> > + *
> > + * This function sets up wakeup for S5  */ static void 
> > +ish_shutdown(struct pci_dev *pdev) {
> > +       if (pdev->device == EHL_Ax_DEVICE_ID)
> > +               pci_prepare_to_sleep(pdev); }
> > +
> >  static struct device __maybe_unused *ish_resume_device;
> >
> >  /* 50ms to get resume response */
> > @@ -370,13 +347,6 @@ 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);
> >
> > -       /* add this to finish power flow for EHL */
> > -       if (dev->pdev->device == EHL_Ax_DEVICE_ID) {
> > -               pci_set_power_state(pdev, PCI_D0);
> > -               enable_pme_wake(pdev);
> > -               dev_dbg(dev->devc, "set power state to D0 for ehl\n");
> > -       }
> > -
> >         ish_resume_device = device;
> >         dev->resume_flag = 1;
> >
> > @@ -392,6 +362,7 @@ static struct pci_driver ish_driver = {
> >         .id_table = ish_pci_tbl,
> >         .probe = ish_probe,
> >         .remove = ish_remove,
> > +       .shutdown = ish_shutdown,
> >         .driver.pm = &ish_pm_ops,
> >  };
> >
>

Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup

[Kai-Heng Feng]

Hi Even,

On Mon, Sep 18, 2023 at 8:33 AM Xu, Even <even.xu@intel.com> wrote:
>
> Hi, Kai-Heng,
>
> I just got feedback, for testing EHL S5 wakeup feature, you need several steps to setup and access  "https://portal.devicewise.com/things/browse" to trigger wake.
> But currently, our test account of this website are all out of data.
> So maybe you need double check with the team who required you preparing the patch for the verification.

The patch is to solve the GPE refcount overflow, while maintaining S5
wakeup. I don't have any mean to test S5 wake.

So if you also don't have ways to verify S5 wake functionality, maybe
we can simply revert 2e23a70edabe  ("HID: intel-ish-hid: ipc: finish
power flow for EHL OOB") as alternative?

Kai-Heng

> Thanks!
>
> Best Regards,
> Even Xu
>
> -----Original Message-----
> From: Xu, Even
> Sent: Friday, September 15, 2023 3:27 PM
> To: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Cc: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>; jikos@kernel.org; benjamin.tissoires@redhat.com; linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian Hui <jianhui.lee@canonical.com>; Zhang, Lixu <Lixu.Zhang@intel.com>; Ba, Najumon <najumon.ba@intel.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
>
> Hi, Kai-Heng,
>
> I am also not familiar with this S5 wakeup test case.
> I already sent out mails to ask for help on it.
> Will come back to you once I get feedback.
> Thanks!
>
> Best Regards,
> Even Xu
>
> -----Original Message-----
> From: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Sent: Friday, September 15, 2023 2:01 PM
> To: Xu, Even <even.xu@intel.com>
> Cc: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>; jikos@kernel.org; benjamin.tissoires@redhat.com; linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian Hui <jianhui.lee@canonical.com>; Zhang, Lixu <lixu.zhang@intel.com>; Ba, Najumon <najumon.ba@intel.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
>
> Hi Even,
>
> On Fri, Sep 15, 2023 at 1:31 PM Xu, Even <even.xu@intel.com> wrote:
> >
> > Hi, Srinivas,
> >
> > Sure, I will test it.
> > As long term not working on EHL, I doesn't have EHL board on hand right now, I can test this patch on other ISH related platforms.
> > From the patch, it's focus on EHL platform, I assume Kai-Heng already verified the function on EHL board.
>
> I only made sure the GPE overflow issue is fixed by the patch, but I didn't test the S5 wakeup.
> That's because I don't know how to test it on the EHL system I have.
> I'll test it if you can let me know how to test the S5 wakeup.
>
> Kai-Heng
>
> > I don't think it will take effect on other platforms, anyway, I will test it on the platforms I have to provide cross platform verification.
> >
> > Thanks!
> >
> > Best Regards,
> > Even Xu
> >
> > -----Original Message-----
> > From: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
> > Sent: Friday, September 15, 2023 12:11 AM
> > To: Kai-Heng Feng <kai.heng.feng@canonical.com>; jikos@kernel.org;
> > benjamin.tissoires@redhat.com
> > Cc: linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian Hui
> > <jianhui.lee@canonical.com>; Xu, Even <even.xu@intel.com>; Zhang, Lixu
> > <lixu.zhang@intel.com>; Ba, Najumon <najumon.ba@intel.com>;
> > linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
> >
> > Hi Even,
> >
> > On Thu, 2023-09-14 at 12:18 +0800, Kai-Heng Feng wrote:
> > > System cannot suspend more than 255 times because the driver doesn't
> > > have corresponding acpi_disable_gpe() for acpi_enable_gpe(), so the
> > > GPE refcount overflows.
> > >
> > > Since PCI core and ACPI core already handles PCI PME wake and GPE
> > > wake when the device has wakeup capability, use device_init_wakeup()
> > > to let them do the wakeup setting work.
> > >
> > > Also add a shutdown callback which uses pci_prepare_to_sleep() to
> > > let PCI and ACPI set OOB wakeup for S5.
> > >
> > Please test this change.
> >
> > Thanks,
> > Srinivas
> >
> > > Fixes: 2e23a70edabe ("HID: intel-ish-hid: ipc: finish power flow for
> > > EHL OOB")
> > > Cc: Jian Hui Lee <jianhui.lee@canonical.com>
> > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > ---
> > >  drivers/hid/intel-ish-hid/ipc/pci-ish.c | 59
> > > +++++++----------------
> > > --
> > >  1 file changed, 15 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > > b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > > index 55cb25038e63..65e7eeb2fa64 100644
> > > --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > > +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > > @@ -119,42 +119,6 @@ static inline bool ish_should_leave_d0i3(struct
> > > pci_dev *pdev)
> > >         return !pm_resume_via_firmware() || pdev->device ==
> > > CHV_DEVICE_ID;  }
> > >
> > > -static int enable_gpe(struct device *dev) -{ -#ifdef CONFIG_ACPI
> > > -       acpi_status acpi_sts;
> > > -       struct acpi_device *adev;
> > > -       struct acpi_device_wakeup *wakeup;
> > > -
> > > -       adev = ACPI_COMPANION(dev);
> > > -       if (!adev) {
> > > -               dev_err(dev, "get acpi handle failed\n");
> > > -               return -ENODEV;
> > > -       }
> > > -       wakeup = &adev->wakeup;
> > > -
> > > -       acpi_sts = acpi_enable_gpe(wakeup->gpe_device, wakeup-
> > > >gpe_number);
> > > -       if (ACPI_FAILURE(acpi_sts)) {
> > > -               dev_err(dev, "enable ose_gpe failed\n");
> > > -               return -EIO;
> > > -       }
> > > -
> > > -       return 0;
> > > -#else
> > > -       return -ENODEV;
> > > -#endif
> > > -}
> > > -
> > > -static void enable_pme_wake(struct pci_dev *pdev) -{
> > > -       if ((pci_pme_capable(pdev, PCI_D0) ||
> > > -            pci_pme_capable(pdev, PCI_D3hot) ||
> > > -            pci_pme_capable(pdev, PCI_D3cold)) && !enable_gpe(&pdev-
> > > >dev)) {
> > > -               pci_pme_active(pdev, true);
> > > -               dev_dbg(&pdev->dev, "ish ipc driver pme wake
> > > enabled\n");
> > > -       }
> > > -}
> > > -
> > >  /**
> > >   * ish_probe() - PCI driver probe callback
> > >   * @pdev:      pci device
> > > @@ -225,7 +189,7 @@ static int ish_probe(struct pci_dev *pdev, const
> > > struct pci_device_id *ent)
> > >
> > >         /* Enable PME for EHL */
> > >         if (pdev->device == EHL_Ax_DEVICE_ID)
> > > -               enable_pme_wake(pdev);
> > > +               device_init_wakeup(dev, true);
> > >
> > >         ret = ish_init(ishtp);
> > >         if (ret)
> > > @@ -248,6 +212,19 @@ static void ish_remove(struct pci_dev *pdev)
> > >         ish_device_disable(ishtp_dev);  }
> > >
> > > +
> > > +/**
> > > + * ish_shutdown() - PCI driver shutdown callback
> > > + * @pdev:      pci device
> > > + *
> > > + * This function sets up wakeup for S5  */ static void
> > > +ish_shutdown(struct pci_dev *pdev) {
> > > +       if (pdev->device == EHL_Ax_DEVICE_ID)
> > > +               pci_prepare_to_sleep(pdev); }
> > > +
> > >  static struct device __maybe_unused *ish_resume_device;
> > >
> > >  /* 50ms to get resume response */
> > > @@ -370,13 +347,6 @@ 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);
> > >
> > > -       /* add this to finish power flow for EHL */
> > > -       if (dev->pdev->device == EHL_Ax_DEVICE_ID) {
> > > -               pci_set_power_state(pdev, PCI_D0);
> > > -               enable_pme_wake(pdev);
> > > -               dev_dbg(dev->devc, "set power state to D0 for ehl\n");
> > > -       }
> > > -
> > >         ish_resume_device = device;
> > >         dev->resume_flag = 1;
> > >
> > > @@ -392,6 +362,7 @@ static struct pci_driver ish_driver = {
> > >         .id_table = ish_pci_tbl,
> > >         .probe = ish_probe,
> > >         .remove = ish_remove,
> > > +       .shutdown = ish_shutdown,
> > >         .driver.pm = &ish_pm_ops,
> > >  };
> > >
> >

Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup

[srinivas pandruvada]

[-- Attachment #1: Type: text/plain, Size: 9819 bytes --]

Hi Kai-Heng,
On Mon, 2023-09-18 at 09:17 +0800, Kai-Heng Feng wrote:
> Hi Even,
> 
> On Mon, Sep 18, 2023 at 8:33 AM Xu, Even <even.xu@intel.com> wrote:
> > 
> > Hi, Kai-Heng,
> > 
> > I just got feedback, for testing EHL S5 wakeup feature, you need
> > several steps to setup and access 
> > "https://portal.devicewise.com/things/browse" to trigger wake.
> > But currently, our test account of this website are all out of
> > data.
> > So maybe you need double check with the team who required you
> > preparing the patch for the verification.
> 
> The patch is to solve the GPE refcount overflow, while maintaining S5
> wakeup. I don't have any mean to test S5 wake.
> 
The issue is not calling acpi_disable_gpe(). To reduce the scope of
change can we just add that instead of a adding new callbacks. This way
scope is reduced.

Something like the attached

Thanks,
Srinivas






> So if you also don't have ways to verify S5 wake functionality, maybe
> we can simply revert 2e23a70edabe  ("HID: intel-ish-hid: ipc: finish
> power flow for EHL OOB") as alternative?
> 
> Kai-Heng
> 
> > Thanks!
> > 
> > Best Regards,
> > Even Xu
> > 
> > -----Original Message-----
> > From: Xu, Even
> > Sent: Friday, September 15, 2023 3:27 PM
> > To: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > Cc: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>;
> > jikos@kernel.org; benjamin.tissoires@redhat.com;
> > linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian Hui
> > <jianhui.lee@canonical.com>; Zhang, Lixu <Lixu.Zhang@intel.com>;
> > Ba, Najumon <najumon.ba@intel.com>; linux-input@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > Subject: RE: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
> > 
> > Hi, Kai-Heng,
> > 
> > I am also not familiar with this S5 wakeup test case.
> > I already sent out mails to ask for help on it.
> > Will come back to you once I get feedback.
> > Thanks!
> > 
> > Best Regards,
> > Even Xu
> > 
> > -----Original Message-----
> > From: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > Sent: Friday, September 15, 2023 2:01 PM
> > To: Xu, Even <even.xu@intel.com>
> > Cc: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>;
> > jikos@kernel.org; benjamin.tissoires@redhat.com;
> > linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian Hui
> > <jianhui.lee@canonical.com>; Zhang, Lixu <lixu.zhang@intel.com>;
> > Ba, Najumon <najumon.ba@intel.com>; linux-input@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
> > 
> > Hi Even,
> > 
> > On Fri, Sep 15, 2023 at 1:31 PM Xu, Even <even.xu@intel.com> wrote:
> > > 
> > > Hi, Srinivas,
> > > 
> > > Sure, I will test it.
> > > As long term not working on EHL, I doesn't have EHL board on hand
> > > right now, I can test this patch on other ISH related platforms.
> > > From the patch, it's focus on EHL platform, I assume Kai-Heng
> > > already verified the function on EHL board.
> > 
> > I only made sure the GPE overflow issue is fixed by the patch, but
> > I didn't test the S5 wakeup.
> > That's because I don't know how to test it on the EHL system I
> > have.
> > I'll test it if you can let me know how to test the S5 wakeup.
> > 
> > Kai-Heng
> > 
> > > I don't think it will take effect on other platforms, anyway, I
> > > will test it on the platforms I have to provide cross platform
> > > verification.
> > > 
> > > Thanks!
> > > 
> > > Best Regards,
> > > Even Xu
> > > 
> > > -----Original Message-----
> > > From: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
> > > Sent: Friday, September 15, 2023 12:11 AM
> > > To: Kai-Heng Feng <kai.heng.feng@canonical.com>;
> > > jikos@kernel.org;
> > > benjamin.tissoires@redhat.com
> > > Cc: linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee,
> > > Jian Hui
> > > <jianhui.lee@canonical.com>; Xu, Even <even.xu@intel.com>; Zhang,
> > > Lixu
> > > <lixu.zhang@intel.com>; Ba, Najumon <najumon.ba@intel.com>;
> > > linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB
> > > wakeup
> > > 
> > > Hi Even,
> > > 
> > > On Thu, 2023-09-14 at 12:18 +0800, Kai-Heng Feng wrote:
> > > > System cannot suspend more than 255 times because the driver
> > > > doesn't
> > > > have corresponding acpi_disable_gpe() for acpi_enable_gpe(), so
> > > > the
> > > > GPE refcount overflows.
> > > > 
> > > > Since PCI core and ACPI core already handles PCI PME wake and
> > > > GPE
> > > > wake when the device has wakeup capability, use
> > > > device_init_wakeup()
> > > > to let them do the wakeup setting work.
> > > > 
> > > > Also add a shutdown callback which uses pci_prepare_to_sleep()
> > > > to
> > > > let PCI and ACPI set OOB wakeup for S5.
> > > > 
> > > Please test this change.
> > > 
> > > Thanks,
> > > Srinivas
> > > 
> > > > Fixes: 2e23a70edabe ("HID: intel-ish-hid: ipc: finish power
> > > > flow for
> > > > EHL OOB")
> > > > Cc: Jian Hui Lee <jianhui.lee@canonical.com>
> > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > > ---
> > > >  drivers/hid/intel-ish-hid/ipc/pci-ish.c | 59
> > > > +++++++----------------
> > > > --
> > > >  1 file changed, 15 insertions(+), 44 deletions(-)
> > > > 
> > > > diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > > > b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > > > index 55cb25038e63..65e7eeb2fa64 100644
> > > > --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > > > +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > > > @@ -119,42 +119,6 @@ static inline bool
> > > > ish_should_leave_d0i3(struct
> > > > pci_dev *pdev)
> > > >         return !pm_resume_via_firmware() || pdev->device ==
> > > > CHV_DEVICE_ID;  }
> > > > 
> > > > -static int enable_gpe(struct device *dev) -{ -#ifdef
> > > > CONFIG_ACPI
> > > > -       acpi_status acpi_sts;
> > > > -       struct acpi_device *adev;
> > > > -       struct acpi_device_wakeup *wakeup;
> > > > -
> > > > -       adev = ACPI_COMPANION(dev);
> > > > -       if (!adev) {
> > > > -               dev_err(dev, "get acpi handle failed\n");
> > > > -               return -ENODEV;
> > > > -       }
> > > > -       wakeup = &adev->wakeup;
> > > > -
> > > > -       acpi_sts = acpi_enable_gpe(wakeup->gpe_device, wakeup-
> > > > > gpe_number);
> > > > -       if (ACPI_FAILURE(acpi_sts)) {
> > > > -               dev_err(dev, "enable ose_gpe failed\n");
> > > > -               return -EIO;
> > > > -       }
> > > > -
> > > > -       return 0;
> > > > -#else
> > > > -       return -ENODEV;
> > > > -#endif
> > > > -}
> > > > -
> > > > -static void enable_pme_wake(struct pci_dev *pdev) -{
> > > > -       if ((pci_pme_capable(pdev, PCI_D0) ||
> > > > -            pci_pme_capable(pdev, PCI_D3hot) ||
> > > > -            pci_pme_capable(pdev, PCI_D3cold)) &&
> > > > !enable_gpe(&pdev-
> > > > > dev)) {
> > > > -               pci_pme_active(pdev, true);
> > > > -               dev_dbg(&pdev->dev, "ish ipc driver pme wake
> > > > enabled\n");
> > > > -       }
> > > > -}
> > > > -
> > > >  /**
> > > >   * ish_probe() - PCI driver probe callback
> > > >   * @pdev:      pci device
> > > > @@ -225,7 +189,7 @@ static int ish_probe(struct pci_dev *pdev,
> > > > const
> > > > struct pci_device_id *ent)
> > > > 
> > > >         /* Enable PME for EHL */
> > > >         if (pdev->device == EHL_Ax_DEVICE_ID)
> > > > -               enable_pme_wake(pdev);
> > > > +               device_init_wakeup(dev, true);
> > > > 
> > > >         ret = ish_init(ishtp);
> > > >         if (ret)
> > > > @@ -248,6 +212,19 @@ static void ish_remove(struct pci_dev
> > > > *pdev)
> > > >         ish_device_disable(ishtp_dev);  }
> > > > 
> > > > +
> > > > +/**
> > > > + * ish_shutdown() - PCI driver shutdown callback
> > > > + * @pdev:      pci device
> > > > + *
> > > > + * This function sets up wakeup for S5  */ static void
> > > > +ish_shutdown(struct pci_dev *pdev) {
> > > > +       if (pdev->device == EHL_Ax_DEVICE_ID)
> > > > +               pci_prepare_to_sleep(pdev); }
> > > > +
> > > >  static struct device __maybe_unused *ish_resume_device;
> > > > 
> > > >  /* 50ms to get resume response */
> > > > @@ -370,13 +347,6 @@ 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);
> > > > 
> > > > -       /* add this to finish power flow for EHL */
> > > > -       if (dev->pdev->device == EHL_Ax_DEVICE_ID) {
> > > > -               pci_set_power_state(pdev, PCI_D0);
> > > > -               enable_pme_wake(pdev);
> > > > -               dev_dbg(dev->devc, "set power state to D0 for
> > > > ehl\n");
> > > > -       }
> > > > -
> > > >         ish_resume_device = device;
> > > >         dev->resume_flag = 1;
> > > > 
> > > > @@ -392,6 +362,7 @@ static struct pci_driver ish_driver = {
> > > >         .id_table = ish_pci_tbl,
> > > >         .probe = ish_probe,
> > > >         .remove = ish_remove,
> > > > +       .shutdown = ish_shutdown,
> > > >         .driver.pm = &ish_pm_ops,
> > > >  };
> > > > 
> > > 


[-- Attachment #2: ehl_gpe.diff --]
[-- Type: text/x-patch, Size: 2393 bytes --]

diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
index 55cb25038e63..0d854e4f1f7c 100644
--- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
+++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
@@ -119,7 +119,7 @@ static inline bool ish_should_leave_d0i3(struct pci_dev *pdev)
 	return !pm_resume_via_firmware() || pdev->device == CHV_DEVICE_ID;
 }
 
-static int enable_gpe(struct device *dev)
+static int enable_gpe(struct device *dev, bool status)
 {
 #ifdef CONFIG_ACPI
 	acpi_status acpi_sts;
@@ -133,7 +133,11 @@ static int enable_gpe(struct device *dev)
 	}
 	wakeup = &adev->wakeup;
 
-	acpi_sts = acpi_enable_gpe(wakeup->gpe_device, wakeup->gpe_number);
+	if (status)
+		acpi_sts = acpi_enable_gpe(wakeup->gpe_device, wakeup->gpe_number);
+	else
+		acpi_sts = acpi_disable_gpe(wakeup->gpe_device, wakeup->gpe_number);
+
 	if (ACPI_FAILURE(acpi_sts)) {
 		dev_err(dev, "enable ose_gpe failed\n");
 		return -EIO;
@@ -145,13 +149,13 @@ static int enable_gpe(struct device *dev)
 #endif
 }
 
-static void enable_pme_wake(struct pci_dev *pdev)
+static void enable_pme_wake(struct pci_dev *pdev, status)
 {
 	if ((pci_pme_capable(pdev, PCI_D0) ||
 	     pci_pme_capable(pdev, PCI_D3hot) ||
-	     pci_pme_capable(pdev, PCI_D3cold)) && !enable_gpe(&pdev->dev)) {
-		pci_pme_active(pdev, true);
-		dev_dbg(&pdev->dev, "ish ipc driver pme wake enabled\n");
+	     pci_pme_capable(pdev, PCI_D3cold)) && !enable_gpe(&pdev->dev), status) {
+		pci_pme_active(pdev, status);
+		dev_dbg(&pdev->dev, "ish ipc driver pme wake enabled state %d\n", status);
 	}
 }
 
@@ -225,7 +229,7 @@ static int ish_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	/* Enable PME for EHL */
 	if (pdev->device == EHL_Ax_DEVICE_ID)
-		enable_pme_wake(pdev);
+		enable_pme_wake(pdev, true);
 
 	ret = ish_init(ishtp);
 	if (ret)
@@ -353,6 +357,9 @@ static int __maybe_unused ish_suspend(struct device *device)
 		ish_disable_dma(dev);
 	}
 
+	if (dev->pdev->device == EHL_Ax_DEVICE_ID)
+		enable_pme_wake(pdev, false);
+
 	return 0;
 }
 
@@ -373,7 +380,7 @@ static int __maybe_unused ish_resume(struct device *device)
 	/* add this to finish power flow for EHL */
 	if (dev->pdev->device == EHL_Ax_DEVICE_ID) {
 		pci_set_power_state(pdev, PCI_D0);
-		enable_pme_wake(pdev);
+		enable_pme_wake(pdev, true);
 		dev_dbg(dev->devc, "set power state to D0 for ehl\n");
 	}
 

Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup

[Kai-Heng Feng]

On Mon, Sep 18, 2023 at 11:57 PM srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> Hi Kai-Heng,
> On Mon, 2023-09-18 at 09:17 +0800, Kai-Heng Feng wrote:
> > Hi Even,
> >
> > On Mon, Sep 18, 2023 at 8:33 AM Xu, Even <even.xu@intel.com> wrote:
> > >
> > > Hi, Kai-Heng,
> > >
> > > I just got feedback, for testing EHL S5 wakeup feature, you need
> > > several steps to setup and access
> > > "https://portal.devicewise.com/things/browse" to trigger wake.
> > > But currently, our test account of this website are all out of
> > > data.
> > > So maybe you need double check with the team who required you
> > > preparing the patch for the verification.
> >
> > The patch is to solve the GPE refcount overflow, while maintaining S5
> > wakeup. I don't have any mean to test S5 wake.
> >
> The issue is not calling acpi_disable_gpe(). To reduce the scope of
> change can we just add that instead of a adding new callbacks. This way
> scope is reduced.

This patch does exactly the same thing by letting PCI and ACPI handle
the PME and GPE.
Though the change seems to be bigger, it actually reduces the duped
code, while keep the S5 wakeup ability intact.

Kai-Heng

>
> Something like the attached
>
> Thanks,
> Srinivas
>
>
>
>
>
>
> > So if you also don't have ways to verify S5 wake functionality, maybe
> > we can simply revert 2e23a70edabe  ("HID: intel-ish-hid: ipc: finish
> > power flow for EHL OOB") as alternative?
> >
> > Kai-Heng
> >
> > > Thanks!
> > >
> > > Best Regards,
> > > Even Xu
> > >
> > > -----Original Message-----
> > > From: Xu, Even
> > > Sent: Friday, September 15, 2023 3:27 PM
> > > To: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > Cc: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>;
> > > jikos@kernel.org; benjamin.tissoires@redhat.com;
> > > linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian Hui
> > > <jianhui.lee@canonical.com>; Zhang, Lixu <Lixu.Zhang@intel.com>;
> > > Ba, Najumon <najumon.ba@intel.com>; linux-input@vger.kernel.org;
> > > linux-kernel@vger.kernel.org
> > > Subject: RE: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
> > >
> > > Hi, Kai-Heng,
> > >
> > > I am also not familiar with this S5 wakeup test case.
> > > I already sent out mails to ask for help on it.
> > > Will come back to you once I get feedback.
> > > Thanks!
> > >
> > > Best Regards,
> > > Even Xu
> > >
> > > -----Original Message-----
> > > From: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > Sent: Friday, September 15, 2023 2:01 PM
> > > To: Xu, Even <even.xu@intel.com>
> > > Cc: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>;
> > > jikos@kernel.org; benjamin.tissoires@redhat.com;
> > > linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian Hui
> > > <jianhui.lee@canonical.com>; Zhang, Lixu <lixu.zhang@intel.com>;
> > > Ba, Najumon <najumon.ba@intel.com>; linux-input@vger.kernel.org;
> > > linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
> > >
> > > Hi Even,
> > >
> > > On Fri, Sep 15, 2023 at 1:31 PM Xu, Even <even.xu@intel.com> wrote:
> > > >
> > > > Hi, Srinivas,
> > > >
> > > > Sure, I will test it.
> > > > As long term not working on EHL, I doesn't have EHL board on hand
> > > > right now, I can test this patch on other ISH related platforms.
> > > > From the patch, it's focus on EHL platform, I assume Kai-Heng
> > > > already verified the function on EHL board.
> > >
> > > I only made sure the GPE overflow issue is fixed by the patch, but
> > > I didn't test the S5 wakeup.
> > > That's because I don't know how to test it on the EHL system I
> > > have.
> > > I'll test it if you can let me know how to test the S5 wakeup.
> > >
> > > Kai-Heng
> > >
> > > > I don't think it will take effect on other platforms, anyway, I
> > > > will test it on the platforms I have to provide cross platform
> > > > verification.
> > > >
> > > > Thanks!
> > > >
> > > > Best Regards,
> > > > Even Xu
> > > >
> > > > -----Original Message-----
> > > > From: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
> > > > Sent: Friday, September 15, 2023 12:11 AM
> > > > To: Kai-Heng Feng <kai.heng.feng@canonical.com>;
> > > > jikos@kernel.org;
> > > > benjamin.tissoires@redhat.com
> > > > Cc: linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee,
> > > > Jian Hui
> > > > <jianhui.lee@canonical.com>; Xu, Even <even.xu@intel.com>; Zhang,
> > > > Lixu
> > > > <lixu.zhang@intel.com>; Ba, Najumon <najumon.ba@intel.com>;
> > > > linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> > > > Subject: Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB
> > > > wakeup
> > > >
> > > > Hi Even,
> > > >
> > > > On Thu, 2023-09-14 at 12:18 +0800, Kai-Heng Feng wrote:
> > > > > System cannot suspend more than 255 times because the driver
> > > > > doesn't
> > > > > have corresponding acpi_disable_gpe() for acpi_enable_gpe(), so
> > > > > the
> > > > > GPE refcount overflows.
> > > > >
> > > > > Since PCI core and ACPI core already handles PCI PME wake and
> > > > > GPE
> > > > > wake when the device has wakeup capability, use
> > > > > device_init_wakeup()
> > > > > to let them do the wakeup setting work.
> > > > >
> > > > > Also add a shutdown callback which uses pci_prepare_to_sleep()
> > > > > to
> > > > > let PCI and ACPI set OOB wakeup for S5.
> > > > >
> > > > Please test this change.
> > > >
> > > > Thanks,
> > > > Srinivas
> > > >
> > > > > Fixes: 2e23a70edabe ("HID: intel-ish-hid: ipc: finish power
> > > > > flow for
> > > > > EHL OOB")
> > > > > Cc: Jian Hui Lee <jianhui.lee@canonical.com>
> > > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > > > ---
> > > > >  drivers/hid/intel-ish-hid/ipc/pci-ish.c | 59
> > > > > +++++++----------------
> > > > > --
> > > > >  1 file changed, 15 insertions(+), 44 deletions(-)
> > > > >
> > > > > diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > > > > b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > > > > index 55cb25038e63..65e7eeb2fa64 100644
> > > > > --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > > > > +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > > > > @@ -119,42 +119,6 @@ static inline bool
> > > > > ish_should_leave_d0i3(struct
> > > > > pci_dev *pdev)
> > > > >         return !pm_resume_via_firmware() || pdev->device ==
> > > > > CHV_DEVICE_ID;  }
> > > > >
> > > > > -static int enable_gpe(struct device *dev) -{ -#ifdef
> > > > > CONFIG_ACPI
> > > > > -       acpi_status acpi_sts;
> > > > > -       struct acpi_device *adev;
> > > > > -       struct acpi_device_wakeup *wakeup;
> > > > > -
> > > > > -       adev = ACPI_COMPANION(dev);
> > > > > -       if (!adev) {
> > > > > -               dev_err(dev, "get acpi handle failed\n");
> > > > > -               return -ENODEV;
> > > > > -       }
> > > > > -       wakeup = &adev->wakeup;
> > > > > -
> > > > > -       acpi_sts = acpi_enable_gpe(wakeup->gpe_device, wakeup-
> > > > > > gpe_number);
> > > > > -       if (ACPI_FAILURE(acpi_sts)) {
> > > > > -               dev_err(dev, "enable ose_gpe failed\n");
> > > > > -               return -EIO;
> > > > > -       }
> > > > > -
> > > > > -       return 0;
> > > > > -#else
> > > > > -       return -ENODEV;
> > > > > -#endif
> > > > > -}
> > > > > -
> > > > > -static void enable_pme_wake(struct pci_dev *pdev) -{
> > > > > -       if ((pci_pme_capable(pdev, PCI_D0) ||
> > > > > -            pci_pme_capable(pdev, PCI_D3hot) ||
> > > > > -            pci_pme_capable(pdev, PCI_D3cold)) &&
> > > > > !enable_gpe(&pdev-
> > > > > > dev)) {
> > > > > -               pci_pme_active(pdev, true);
> > > > > -               dev_dbg(&pdev->dev, "ish ipc driver pme wake
> > > > > enabled\n");
> > > > > -       }
> > > > > -}
> > > > > -
> > > > >  /**
> > > > >   * ish_probe() - PCI driver probe callback
> > > > >   * @pdev:      pci device
> > > > > @@ -225,7 +189,7 @@ static int ish_probe(struct pci_dev *pdev,
> > > > > const
> > > > > struct pci_device_id *ent)
> > > > >
> > > > >         /* Enable PME for EHL */
> > > > >         if (pdev->device == EHL_Ax_DEVICE_ID)
> > > > > -               enable_pme_wake(pdev);
> > > > > +               device_init_wakeup(dev, true);
> > > > >
> > > > >         ret = ish_init(ishtp);
> > > > >         if (ret)
> > > > > @@ -248,6 +212,19 @@ static void ish_remove(struct pci_dev
> > > > > *pdev)
> > > > >         ish_device_disable(ishtp_dev);  }
> > > > >
> > > > > +
> > > > > +/**
> > > > > + * ish_shutdown() - PCI driver shutdown callback
> > > > > + * @pdev:      pci device
> > > > > + *
> > > > > + * This function sets up wakeup for S5  */ static void
> > > > > +ish_shutdown(struct pci_dev *pdev) {
> > > > > +       if (pdev->device == EHL_Ax_DEVICE_ID)
> > > > > +               pci_prepare_to_sleep(pdev); }
> > > > > +
> > > > >  static struct device __maybe_unused *ish_resume_device;
> > > > >
> > > > >  /* 50ms to get resume response */
> > > > > @@ -370,13 +347,6 @@ 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);
> > > > >
> > > > > -       /* add this to finish power flow for EHL */
> > > > > -       if (dev->pdev->device == EHL_Ax_DEVICE_ID) {
> > > > > -               pci_set_power_state(pdev, PCI_D0);
> > > > > -               enable_pme_wake(pdev);
> > > > > -               dev_dbg(dev->devc, "set power state to D0 for
> > > > > ehl\n");
> > > > > -       }
> > > > > -
> > > > >         ish_resume_device = device;
> > > > >         dev->resume_flag = 1;
> > > > >
> > > > > @@ -392,6 +362,7 @@ static struct pci_driver ish_driver = {
> > > > >         .id_table = ish_pci_tbl,
> > > > >         .probe = ish_probe,
> > > > >         .remove = ish_remove,
> > > > > +       .shutdown = ish_shutdown,
> > > > >         .driver.pm = &ish_pm_ops,
> > > > >  };
> > > > >
> > > >
>

Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup

[srinivas pandruvada]

On Tue, 2023-09-19 at 15:36 +0800, Kai-Heng Feng wrote:
> On Mon, Sep 18, 2023 at 11:57 PM srinivas pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> > 
> > Hi Kai-Heng,
> > On Mon, 2023-09-18 at 09:17 +0800, Kai-Heng Feng wrote:
> > > Hi Even,
> > > 
> > > On Mon, Sep 18, 2023 at 8:33 AM Xu, Even <even.xu@intel.com>
> > > wrote:
> > > > 
> > > > Hi, Kai-Heng,
> > > > 
> > > > I just got feedback, for testing EHL S5 wakeup feature, you
> > > > need
> > > > several steps to setup and access
> > > > "https://portal.devicewise.com/things/browse" to trigger wake.
> > > > But currently, our test account of this website are all out of
> > > > data.
> > > > So maybe you need double check with the team who required you
> > > > preparing the patch for the verification.
> > > 
> > > The patch is to solve the GPE refcount overflow, while
> > > maintaining S5
> > > wakeup. I don't have any mean to test S5 wake.
> > > 
> > The issue is not calling acpi_disable_gpe(). To reduce the scope of
> > change can we just add that instead of a adding new callbacks. This
> > way
> > scope is reduced.
> 
> This patch does exactly the same thing by letting PCI and ACPI handle
> the PME and GPE.
> Though the change seems to be bigger, it actually reduces the duped
> code, while keep the S5 wakeup ability intact.
It may be doing the same. But with long chain of calls without
verification, I am not comfortable.
This can be another patch by itself to use the framework. 

But you are targeting a fix for overflow issue, which is separate from
the use of PCI/ACPI framework.

Thanks,
Srinivas

> 
> Kai-Heng
> 
> > 
> > Something like the attached
> > 
> > Thanks,
> > Srinivas
> > 
> > 
> > 
> > 
> > 
> > 
> > > So if you also don't have ways to verify S5 wake functionality,
> > > maybe
> > > we can simply revert 2e23a70edabe  ("HID: intel-ish-hid: ipc:
> > > finish
> > > power flow for EHL OOB") as alternative?
> > > 
> > > Kai-Heng
> > > 
> > > > Thanks!
> > > > 
> > > > Best Regards,
> > > > Even Xu
> > > > 
> > > > -----Original Message-----
> > > > From: Xu, Even
> > > > Sent: Friday, September 15, 2023 3:27 PM
> > > > To: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > > Cc: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>;
> > > > jikos@kernel.org; benjamin.tissoires@redhat.com;
> > > > linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian
> > > > Hui
> > > > <jianhui.lee@canonical.com>; Zhang, Lixu
> > > > <Lixu.Zhang@intel.com>;
> > > > Ba, Najumon <najumon.ba@intel.com>;
> > > > linux-input@vger.kernel.org;
> > > > linux-kernel@vger.kernel.org
> > > > Subject: RE: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB
> > > > wakeup
> > > > 
> > > > Hi, Kai-Heng,
> > > > 
> > > > I am also not familiar with this S5 wakeup test case.
> > > > I already sent out mails to ask for help on it.
> > > > Will come back to you once I get feedback.
> > > > Thanks!
> > > > 
> > > > Best Regards,
> > > > Even Xu
> > > > 
> > > > -----Original Message-----
> > > > From: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > > Sent: Friday, September 15, 2023 2:01 PM
> > > > To: Xu, Even <even.xu@intel.com>
> > > > Cc: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>;
> > > > jikos@kernel.org; benjamin.tissoires@redhat.com;
> > > > linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian
> > > > Hui
> > > > <jianhui.lee@canonical.com>; Zhang, Lixu
> > > > <lixu.zhang@intel.com>;
> > > > Ba, Najumon <najumon.ba@intel.com>;
> > > > linux-input@vger.kernel.org;
> > > > linux-kernel@vger.kernel.org
> > > > Subject: Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB
> > > > wakeup
> > > > 
> > > > Hi Even,
> > > > 
> > > > On Fri, Sep 15, 2023 at 1:31 PM Xu, Even <even.xu@intel.com>
> > > > wrote:
> > > > > 
> > > > > Hi, Srinivas,
> > > > > 
> > > > > Sure, I will test it.
> > > > > As long term not working on EHL, I doesn't have EHL board on
> > > > > hand
> > > > > right now, I can test this patch on other ISH related
> > > > > platforms.
> > > > > From the patch, it's focus on EHL platform, I assume Kai-Heng
> > > > > already verified the function on EHL board.
> > > > 
> > > > I only made sure the GPE overflow issue is fixed by the patch,
> > > > but
> > > > I didn't test the S5 wakeup.
> > > > That's because I don't know how to test it on the EHL system I
> > > > have.
> > > > I'll test it if you can let me know how to test the S5 wakeup.
> > > > 
> > > > Kai-Heng
> > > > 
> > > > > I don't think it will take effect on other platforms, anyway,
> > > > > I
> > > > > will test it on the platforms I have to provide cross
> > > > > platform
> > > > > verification.
> > > > > 
> > > > > Thanks!
> > > > > 
> > > > > Best Regards,
> > > > > Even Xu
> > > > > 
> > > > > -----Original Message-----
> > > > > From: srinivas pandruvada
> > > > > <srinivas.pandruvada@linux.intel.com>
> > > > > Sent: Friday, September 15, 2023 12:11 AM
> > > > > To: Kai-Heng Feng <kai.heng.feng@canonical.com>;
> > > > > jikos@kernel.org;
> > > > > benjamin.tissoires@redhat.com
> > > > > Cc: linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee,
> > > > > Jian Hui
> > > > > <jianhui.lee@canonical.com>; Xu, Even <even.xu@intel.com>;
> > > > > Zhang,
> > > > > Lixu
> > > > > <lixu.zhang@intel.com>; Ba, Najumon <najumon.ba@intel.com>;
> > > > > linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> > > > > Subject: Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB
> > > > > wakeup
> > > > > 
> > > > > Hi Even,
> > > > > 
> > > > > On Thu, 2023-09-14 at 12:18 +0800, Kai-Heng Feng wrote:
> > > > > > System cannot suspend more than 255 times because the
> > > > > > driver
> > > > > > doesn't
> > > > > > have corresponding acpi_disable_gpe() for
> > > > > > acpi_enable_gpe(), so
> > > > > > the
> > > > > > GPE refcount overflows.
> > > > > > 
> > > > > > Since PCI core and ACPI core already handles PCI PME wake
> > > > > > and
> > > > > > GPE
> > > > > > wake when the device has wakeup capability, use
> > > > > > device_init_wakeup()
> > > > > > to let them do the wakeup setting work.
> > > > > > 
> > > > > > Also add a shutdown callback which uses
> > > > > > pci_prepare_to_sleep()
> > > > > > to
> > > > > > let PCI and ACPI set OOB wakeup for S5.
> > > > > > 
> > > > > Please test this change.
> > > > > 
> > > > > Thanks,
> > > > > Srinivas
> > > > > 
> > > > > > Fixes: 2e23a70edabe ("HID: intel-ish-hid: ipc: finish power
> > > > > > flow for
> > > > > > EHL OOB")
> > > > > > Cc: Jian Hui Lee <jianhui.lee@canonical.com>
> > > > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > > > > ---
> > > > > >  drivers/hid/intel-ish-hid/ipc/pci-ish.c | 59
> > > > > > +++++++----------------
> > > > > > --
> > > > > >  1 file changed, 15 insertions(+), 44 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > > > > > b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > > > > > index 55cb25038e63..65e7eeb2fa64 100644
> > > > > > --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > > > > > +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > > > > > @@ -119,42 +119,6 @@ static inline bool
> > > > > > ish_should_leave_d0i3(struct
> > > > > > pci_dev *pdev)
> > > > > >         return !pm_resume_via_firmware() || pdev->device ==
> > > > > > CHV_DEVICE_ID;  }
> > > > > > 
> > > > > > -static int enable_gpe(struct device *dev) -{ -#ifdef
> > > > > > CONFIG_ACPI
> > > > > > -       acpi_status acpi_sts;
> > > > > > -       struct acpi_device *adev;
> > > > > > -       struct acpi_device_wakeup *wakeup;
> > > > > > -
> > > > > > -       adev = ACPI_COMPANION(dev);
> > > > > > -       if (!adev) {
> > > > > > -               dev_err(dev, "get acpi handle failed\n");
> > > > > > -               return -ENODEV;
> > > > > > -       }
> > > > > > -       wakeup = &adev->wakeup;
> > > > > > -
> > > > > > -       acpi_sts = acpi_enable_gpe(wakeup->gpe_device,
> > > > > > wakeup-
> > > > > > > gpe_number);
> > > > > > -       if (ACPI_FAILURE(acpi_sts)) {
> > > > > > -               dev_err(dev, "enable ose_gpe failed\n");
> > > > > > -               return -EIO;
> > > > > > -       }
> > > > > > -
> > > > > > -       return 0;
> > > > > > -#else
> > > > > > -       return -ENODEV;
> > > > > > -#endif
> > > > > > -}
> > > > > > -
> > > > > > -static void enable_pme_wake(struct pci_dev *pdev) -{
> > > > > > -       if ((pci_pme_capable(pdev, PCI_D0) ||
> > > > > > -            pci_pme_capable(pdev, PCI_D3hot) ||
> > > > > > -            pci_pme_capable(pdev, PCI_D3cold)) &&
> > > > > > !enable_gpe(&pdev-
> > > > > > > dev)) {
> > > > > > -               pci_pme_active(pdev, true);
> > > > > > -               dev_dbg(&pdev->dev, "ish ipc driver pme
> > > > > > wake
> > > > > > enabled\n");
> > > > > > -       }
> > > > > > -}
> > > > > > -
> > > > > >  /**
> > > > > >   * ish_probe() - PCI driver probe callback
> > > > > >   * @pdev:      pci device
> > > > > > @@ -225,7 +189,7 @@ static int ish_probe(struct pci_dev
> > > > > > *pdev,
> > > > > > const
> > > > > > struct pci_device_id *ent)
> > > > > > 
> > > > > >         /* Enable PME for EHL */
> > > > > >         if (pdev->device == EHL_Ax_DEVICE_ID)
> > > > > > -               enable_pme_wake(pdev);
> > > > > > +               device_init_wakeup(dev, true);
> > > > > > 
> > > > > >         ret = ish_init(ishtp);
> > > > > >         if (ret)
> > > > > > @@ -248,6 +212,19 @@ static void ish_remove(struct pci_dev
> > > > > > *pdev)
> > > > > >         ish_device_disable(ishtp_dev);  }
> > > > > > 
> > > > > > +
> > > > > > +/**
> > > > > > + * ish_shutdown() - PCI driver shutdown callback
> > > > > > + * @pdev:      pci device
> > > > > > + *
> > > > > > + * This function sets up wakeup for S5  */ static void
> > > > > > +ish_shutdown(struct pci_dev *pdev) {
> > > > > > +       if (pdev->device == EHL_Ax_DEVICE_ID)
> > > > > > +               pci_prepare_to_sleep(pdev); }
> > > > > > +
> > > > > >  static struct device __maybe_unused *ish_resume_device;
> > > > > > 
> > > > > >  /* 50ms to get resume response */
> > > > > > @@ -370,13 +347,6 @@ 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);
> > > > > > 
> > > > > > -       /* add this to finish power flow for EHL */
> > > > > > -       if (dev->pdev->device == EHL_Ax_DEVICE_ID) {
> > > > > > -               pci_set_power_state(pdev, PCI_D0);
> > > > > > -               enable_pme_wake(pdev);
> > > > > > -               dev_dbg(dev->devc, "set power state to D0
> > > > > > for
> > > > > > ehl\n");
> > > > > > -       }
> > > > > > -
> > > > > >         ish_resume_device = device;
> > > > > >         dev->resume_flag = 1;
> > > > > > 
> > > > > > @@ -392,6 +362,7 @@ static struct pci_driver ish_driver = {
> > > > > >         .id_table = ish_pci_tbl,
> > > > > >         .probe = ish_probe,
> > > > > >         .remove = ish_remove,
> > > > > > +       .shutdown = ish_shutdown,
> > > > > >         .driver.pm = &ish_pm_ops,
> > > > > >  };
> > > > > > 
> > > > > 
> > 

Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup

[Rafael J. Wysocki]

On Tue, Sep 19, 2023 at 6:54 PM srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> On Tue, 2023-09-19 at 15:36 +0800, Kai-Heng Feng wrote:
> > On Mon, Sep 18, 2023 at 11:57 PM srinivas pandruvada
> > <srinivas.pandruvada@linux.intel.com> wrote:
> > >
> > > Hi Kai-Heng,
> > > On Mon, 2023-09-18 at 09:17 +0800, Kai-Heng Feng wrote:
> > > > Hi Even,
> > > >
> > > > On Mon, Sep 18, 2023 at 8:33 AM Xu, Even <even.xu@intel.com>
> > > > wrote:
> > > > >
> > > > > Hi, Kai-Heng,
> > > > >
> > > > > I just got feedback, for testing EHL S5 wakeup feature, you
> > > > > need
> > > > > several steps to setup and access
> > > > > "https://portal.devicewise.com/things/browse" to trigger wake.
> > > > > But currently, our test account of this website are all out of
> > > > > data.
> > > > > So maybe you need double check with the team who required you
> > > > > preparing the patch for the verification.
> > > >
> > > > The patch is to solve the GPE refcount overflow, while
> > > > maintaining S5
> > > > wakeup. I don't have any mean to test S5 wake.
> > > >
> > > The issue is not calling acpi_disable_gpe(). To reduce the scope of
> > > change can we just add that instead of a adding new callbacks. This
> > > way
> > > scope is reduced.
> >
> > This patch does exactly the same thing by letting PCI and ACPI handle
> > the PME and GPE.
> > Though the change seems to be bigger, it actually reduces the duped
> > code, while keep the S5 wakeup ability intact.
> It may be doing the same. But with long chain of calls without
> verification, I am not comfortable.
> This can be another patch by itself to use the framework.

I agree.

Let's change one thing at a time.

> But you are targeting a fix for overflow issue, which is separate from
> the use of PCI/ACPI framework.

Yes, let's fix the bug first and make things look nicer separately.

Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup

[Kai-Heng Feng]

On Wed, Sep 20, 2023 at 2:00 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Sep 19, 2023 at 6:54 PM srinivas pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> >
> > On Tue, 2023-09-19 at 15:36 +0800, Kai-Heng Feng wrote:
> > > On Mon, Sep 18, 2023 at 11:57 PM srinivas pandruvada
> > > <srinivas.pandruvada@linux.intel.com> wrote:
> > > >
> > > > Hi Kai-Heng,
> > > > On Mon, 2023-09-18 at 09:17 +0800, Kai-Heng Feng wrote:
> > > > > Hi Even,
> > > > >
> > > > > On Mon, Sep 18, 2023 at 8:33 AM Xu, Even <even.xu@intel.com>
> > > > > wrote:
> > > > > >
> > > > > > Hi, Kai-Heng,
> > > > > >
> > > > > > I just got feedback, for testing EHL S5 wakeup feature, you
> > > > > > need
> > > > > > several steps to setup and access
> > > > > > "https://portal.devicewise.com/things/browse" to trigger wake.
> > > > > > But currently, our test account of this website are all out of
> > > > > > data.
> > > > > > So maybe you need double check with the team who required you
> > > > > > preparing the patch for the verification.
> > > > >
> > > > > The patch is to solve the GPE refcount overflow, while
> > > > > maintaining S5
> > > > > wakeup. I don't have any mean to test S5 wake.
> > > > >
> > > > The issue is not calling acpi_disable_gpe(). To reduce the scope of
> > > > change can we just add that instead of a adding new callbacks. This
> > > > way
> > > > scope is reduced.
> > >
> > > This patch does exactly the same thing by letting PCI and ACPI handle
> > > the PME and GPE.
> > > Though the change seems to be bigger, it actually reduces the duped
> > > code, while keep the S5 wakeup ability intact.
> > It may be doing the same. But with long chain of calls without
> > verification, I am not comfortable.
> > This can be another patch by itself to use the framework.
>
> I agree.
>
> Let's change one thing at a time.
>
> > But you are targeting a fix for overflow issue, which is separate from
> > the use of PCI/ACPI framework.
>
> Yes, let's fix the bug first and make things look nicer separately.

Right, please use the fix from Srinivas and I'll send a separate patch
to make things looks better.

Kai-Heng

Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup

[Rafael J. Wysocki]

On Thu, Sep 21, 2023 at 8:08 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> On Wed, Sep 20, 2023 at 2:00 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Tue, Sep 19, 2023 at 6:54 PM srinivas pandruvada
> > <srinivas.pandruvada@linux.intel.com> wrote:
> > >
> > > On Tue, 2023-09-19 at 15:36 +0800, Kai-Heng Feng wrote:
> > > > On Mon, Sep 18, 2023 at 11:57 PM srinivas pandruvada
> > > > <srinivas.pandruvada@linux.intel.com> wrote:
> > > > >
> > > > > Hi Kai-Heng,
> > > > > On Mon, 2023-09-18 at 09:17 +0800, Kai-Heng Feng wrote:
> > > > > > Hi Even,
> > > > > >
> > > > > > On Mon, Sep 18, 2023 at 8:33 AM Xu, Even <even.xu@intel.com>
> > > > > > wrote:
> > > > > > >
> > > > > > > Hi, Kai-Heng,
> > > > > > >
> > > > > > > I just got feedback, for testing EHL S5 wakeup feature, you
> > > > > > > need
> > > > > > > several steps to setup and access
> > > > > > > "https://portal.devicewise.com/things/browse" to trigger wake.
> > > > > > > But currently, our test account of this website are all out of
> > > > > > > data.
> > > > > > > So maybe you need double check with the team who required you
> > > > > > > preparing the patch for the verification.
> > > > > >
> > > > > > The patch is to solve the GPE refcount overflow, while
> > > > > > maintaining S5
> > > > > > wakeup. I don't have any mean to test S5 wake.
> > > > > >
> > > > > The issue is not calling acpi_disable_gpe(). To reduce the scope of
> > > > > change can we just add that instead of a adding new callbacks. This
> > > > > way
> > > > > scope is reduced.
> > > >
> > > > This patch does exactly the same thing by letting PCI and ACPI handle
> > > > the PME and GPE.
> > > > Though the change seems to be bigger, it actually reduces the duped
> > > > code, while keep the S5 wakeup ability intact.
> > > It may be doing the same. But with long chain of calls without
> > > verification, I am not comfortable.
> > > This can be another patch by itself to use the framework.
> >
> > I agree.
> >
> > Let's change one thing at a time.
> >
> > > But you are targeting a fix for overflow issue, which is separate from
> > > the use of PCI/ACPI framework.
> >
> > Yes, let's fix the bug first and make things look nicer separately.
>
> Right, please use the fix from Srinivas and I'll send a separate patch
> to make things looks better.

OK

Srinivas, please resend the patch with a changelog etc.