Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Wed, 18 Oct 2023 12:24:17 +0800
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
Message-ID: <ZS9d8R9OunZ6Xyu9@google.com>
References: <20231017124047.1.Icc99145043c8d44142bb5ca64ea4c63a417c267b@changeid>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231017124047.1.Icc99145043c8d44142bb5ca64ea4c63a417c267b@changeid>
Precedence: bulk

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.