From: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
To: Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc: "Xu, Even" <even.xu@intel.com>,
"jikos@kernel.org" <jikos@kernel.org>,
"benjamin.tissoires@redhat.com" <benjamin.tissoires@redhat.com>,
"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
"linux-pci@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-input@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
Date: Tue, 19 Sep 2023 09:54:41 -0700 [thread overview]
Message-ID: <67c85f083201ed2cda2cab198b40141ad21912a2.camel@linux.intel.com> (raw)
In-Reply-To: <CAAd53p6MA9YLbcXxpC8=YEtbO6frFJk1LQ1BNUgPk=r1_uR8iw@mail.gmail.com>
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,
> > > > > > };
> > > > > >
> > > > >
> >
next prev parent reply other threads:[~2023-09-19 16:54 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-14 4:18 [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup Kai-Heng Feng
2023-09-14 16:11 ` srinivas pandruvada
2023-09-15 5:31 ` Xu, Even
2023-09-15 6:00 ` Kai-Heng Feng
2023-09-15 7:26 ` Xu, Even
2023-09-18 0:33 ` Xu, Even
2023-09-18 1:17 ` Kai-Heng Feng
2023-09-18 15:57 ` srinivas pandruvada
2023-09-19 7:36 ` Kai-Heng Feng
2023-09-19 16:54 ` srinivas pandruvada [this message]
2023-09-19 17:59 ` Rafael J. Wysocki
2023-09-21 6:08 ` Kai-Heng Feng
2023-09-21 9:33 ` Rafael J. Wysocki
2023-09-14 18:42 ` Bjorn Helgaas
2023-09-15 5:57 ` Kai-Heng Feng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=67c85f083201ed2cda2cab198b40141ad21912a2.camel@linux.intel.com \
--to=srinivas.pandruvada@linux.intel.com \
--cc=benjamin.tissoires@redhat.com \
--cc=even.xu@intel.com \
--cc=jianhui.lee@canonical.com \
--cc=jikos@kernel.org \
--cc=kai.heng.feng@canonical.com \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=lixu.zhang@intel.com \
--cc=najumon.ba@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.