Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
From: srinivas pandruvada
Date: Mon Sep 18 2023 - 12:34:01 EST
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@xxxxxxxxx> 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@xxxxxxxxxxxxx>
> > Cc: srinivas pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>;
> > jikos@xxxxxxxxxx; benjamin.tissoires@xxxxxxxxxx;
> > linux-pm@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; Lee, Jian Hui
> > <jianhui.lee@xxxxxxxxxxxxx>; Zhang, Lixu <Lixu.Zhang@xxxxxxxxx>;
> > Ba, Najumon <najumon.ba@xxxxxxxxx>; linux-input@xxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxx>
> > Sent: Friday, September 15, 2023 2:01 PM
> > To: Xu, Even <even.xu@xxxxxxxxx>
> > Cc: srinivas pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>;
> > jikos@xxxxxxxxxx; benjamin.tissoires@xxxxxxxxxx;
> > linux-pm@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; Lee, Jian Hui
> > <jianhui.lee@xxxxxxxxxxxxx>; Zhang, Lixu <lixu.zhang@xxxxxxxxx>;
> > Ba, Najumon <najumon.ba@xxxxxxxxx>; linux-input@xxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > > Sent: Friday, September 15, 2023 12:11 AM
> > > To: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>;
> > > jikos@xxxxxxxxxx;
> > > benjamin.tissoires@xxxxxxxxxx
> > > Cc: linux-pm@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; Lee,
> > > Jian Hui
> > > <jianhui.lee@xxxxxxxxxxxxx>; Xu, Even <even.xu@xxxxxxxxx>; Zhang,
> > > Lixu
> > > <lixu.zhang@xxxxxxxxx>; Ba, Najumon <najumon.ba@xxxxxxxxx>;
> > > linux-input@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > > 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@xxxxxxxxxxxxx>
> > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
> > > > ---
> > > > 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,
> > > > };
> > > >
> > >
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");
}