Messages in this thread | | | Date | Wed, 18 Oct 2023 12:24:17 +0800 | From | Tzung-Bi Shih <> | Subject | Re: [PATCH v1] FROMLIST: platform/chrome: cros_ec_lpc: Separate host command and irq disable |
| |
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.
| |