From: Tzung-Bi Shih <tzungbi@kernel.org>
To: Lalith Rajendran <lalithkraj@chromium.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
Benson Leung <bleung@chromium.org>,
Guenter Roeck <groeck@chromium.org>,
chrome-platform@lists.linux.dev
Subject: Re: [PATCH v1] FROMLIST: platform/chrome: cros_ec_lpc: Separate host command and irq disable
Date: Wed, 18 Oct 2023 12:24:17 +0800 [thread overview]
Message-ID: <ZS9d8R9OunZ6Xyu9@google.com> (raw)
In-Reply-To: <20231017124047.1.Icc99145043c8d44142bb5ca64ea4c63a417c267b@changeid>
On Tue, Oct 17, 2023 at 12:40:48PM -0500, Lalith Rajendran wrote:
> Both cros host command and irq disable were moved to suspend
> prepare stage from late suspend recently. This is causing EC
> to report MKBP event timeouts during suspend stress testing.
> When the MKBP event timeouts happen during suspend, subsequent
> wakeup of AP by EC using MKBP doesn't happen properly. Although
It needs a Fixes tag. Probably:
Fixes: 4b9abbc132b8 ("platform/chrome: cros_ec_lpc: Move host command to prepare/complete")
> there are other issues to debug here, this change move the irq
> disabling part back to late suspend stage which is a general
> suggestion from the suspend kernel documentaiton to do irq
> disable as late as possible.
s/Although there ... this change move/\nMove/.
Also, please remove "FROMLIST: " from the commit title.
> @@ -321,17 +321,8 @@ void cros_ec_unregister(struct cros_ec_device *ec_dev)
> EXPORT_SYMBOL(cros_ec_unregister);
>
> #ifdef CONFIG_PM_SLEEP
> -/**
> - * cros_ec_suspend() - Handle a suspend operation for the ChromeOS EC device.
> - * @ec_dev: Device to suspend.
> - *
> - * This can be called by drivers to handle a suspend event.
> - *
> - * Return: 0 on success or negative error code.
> - */
> -int cros_ec_suspend(struct cros_ec_device *ec_dev)
> +static int cros_ec_send_suspend_event(struct cros_ec_device *ec_dev)
> {
> - struct device *dev = ec_dev->dev;
> int ret;
> u8 sleep_event;
>
> @@ -343,7 +334,26 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
> if (ret < 0)
> dev_dbg(ec_dev->dev, "Error %d sending suspend event to ec\n",
> ret);
> + return 0;
It was obvious in older cros_ec_suspend() but looks like a mistake in
cros_ec_send_suspend_event().
Either:
- Add a comment here for ignoring the `ret` intentionally.
- Make cros_ec_send_suspend_event() returns void.
I would prefer the latter as the newer cros_ec_suspend() also ignores the
return values.
> +static int cros_ec_disable_irq(struct cros_ec_device *ec_dev)
> +{
> + struct device *dev = ec_dev->dev;
> if (device_may_wakeup(dev))
> ec_dev->wake_enabled = !enable_irq_wake(ec_dev->irq);
> else
> @@ -354,6 +364,35 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
>
> return 0;
Same here, I would suggest to make it return void.
> +/**
> + * cros_ec_suspend() - Handle a suspend operation for the ChromeOS EC device.
> + * @ec_dev: Device to suspend.
> + *
> + * This can be called by drivers to handle a suspend event.
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int cros_ec_suspend(struct cros_ec_device *ec_dev)
> +{
> + cros_ec_send_suspend_event(ec_dev);
> + cros_ec_disable_irq(ec_dev);
cros_ec_suspend() ignores all return values.
> -/**
> - * cros_ec_resume() - Handle a resume operation for the ChromeOS EC device.
> - * @ec_dev: Device to resume.
> - *
> - * This can be called by drivers to handle a resume event.
> - *
> - * Return: 0 on success or negative error code.
> - */
> -int cros_ec_resume(struct cros_ec_device *ec_dev)
> +static int cros_ec_send_resume_event(struct cros_ec_device *ec_dev)
> {
> int ret;
> u8 sleep_event;
>
> - ec_dev->suspended = false;
> - enable_irq(ec_dev->irq);
> -
> sleep_event = (!IS_ENABLED(CONFIG_ACPI) || pm_suspend_via_firmware()) ?
> HOST_SLEEP_EVENT_S3_RESUME :
> HOST_SLEEP_EVENT_S0IX_RESUME;
> @@ -394,6 +422,25 @@ int cros_ec_resume(struct cros_ec_device *ec_dev)
> if (ret < 0)
> dev_dbg(ec_dev->dev, "Error %d sending resume event to ec\n",
> ret);
> + return 0;
Ditto.
> +static int cros_ec_enable_irq(struct cros_ec_device *ec_dev)
> +{
> + ec_dev->suspended = false;
> + enable_irq(ec_dev->irq);
>
> if (ec_dev->wake_enabled)
> disable_irq_wake(ec_dev->irq);
> @@ -407,6 +454,35 @@ int cros_ec_resume(struct cros_ec_device *ec_dev)
>
> return 0;
Ditto.
> +/**
> + * cros_ec_resume() - Handle a resume operation for the ChromeOS EC device.
> + * @ec_dev: Device to resume.
> + *
> + * This can be called by drivers to handle a resume event.
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int cros_ec_resume(struct cros_ec_device *ec_dev)
> +{
> + cros_ec_enable_irq(ec_dev);
> + cros_ec_send_resume_event(ec_dev);
Ditto.
prev parent reply other threads:[~2023-10-18 4:24 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-17 17:40 [PATCH v1] FROMLIST: platform/chrome: cros_ec_lpc: Separate host command and irq disable Lalith Rajendran
2023-10-18 4:24 ` Tzung-Bi Shih [this message]
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=ZS9d8R9OunZ6Xyu9@google.com \
--to=tzungbi@kernel.org \
--cc=bleung@chromium.org \
--cc=chrome-platform@lists.linux.dev \
--cc=groeck@chromium.org \
--cc=lalithkraj@chromium.org \
--cc=linux-kernel@vger.kernel.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).