[PATCH] platform/chrome: cros_ec_proto: Update version on GET_NEXT_EVENT failure

[PATCH] platform/chrome: cros_ec_proto: Update version on GET_NEXT_EVENT failure

[Patryk Duda]

Some EC based devices (e.g. Fingerpint MCU) can jump to RO part of the
firmware (intentionally or due to device reboot). The RO part doesn't
change during the device lifecycle, so it won't support newer version
of EC_CMD_GET_NEXT_EVENT command.

Function cros_ec_query_all() is responsible for finding maximum
supported MKBP event version. It's usually called when the device is
running RW part of the firmware, so the command version can be
potentially higher than version supported by the RO.

The problem was fixed by updating maximum supported version when the
device returns EC_RES_INVALID_VERSION (mapped to -ENOPROTOOPT). That way
the kernel will use highest common version supported by RO and RW.

Fixes: 3300fdd630d4 ("platform/chrome: cros_ec: handle MKBP more events flag")
Cc: <stable@vger.kernel.org> # 5.10+
Signed-off-by: Patryk Duda <pdk@semihalf.com>
---
When Fingerprint MCU is rebooted (e.g. as a part of tests) it jumps to
the RO image and performs RW image signature check. If kernel calls
EC_CMD_GET_NEXT_EVENT FPMCU RO will respond with EC_RES_INVALID_VERSION
because it's older than RW and supports up to version 1 of the command.
As a result kernel keeps trying to get MKBP events and effectively
blocks FPMCU from jumping to RW image.

Before patch 3300fdd630d4 the driver called version 1 of the command.
If the device responded with EC_RES_INVALID_VERSION, the driver would
use version 0 of the command.

Best regards,
Patryk

 drivers/platform/chrome/cros_ec_proto.c | 32 +++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index ff767dccdf0f6..0a131045d50a9 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -750,6 +750,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
 	u8 event_type;
 	u32 host_event;
 	int ret;
+	u32 ver_mask = 0;
 
 	/*
 	 * Default value for wake_event.
@@ -771,6 +772,37 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
 		return get_keyboard_state_event(ec_dev);
 
 	ret = get_next_event(ec_dev);
+	/*
+	 * -ENOPROTOOPT is returned when EC returns EC_RES_INVALID_VERSION.
+	 * This can occur when EC based device (e.g. Fingerprint MCU) jumps to
+	 * the RO image which doesn't support newer version of the command. In
+	 * this case we will attempt to update maximum supported version of the
+	 * EC_CMD_GET_NEXT_EVENT.
+	 */
+	if (ret == -ENOPROTOOPT) {
+		dev_dbg(ec_dev->dev,
+			"GET_NEXT_EVENT returned invalid version error.\n");
+		ret = cros_ec_get_host_command_version_mask(ec_dev,
+							EC_CMD_GET_NEXT_EVENT,
+							&ver_mask);
+		if (ret < 0 || ver_mask == 0)
+			/*
+			 * Do not change the MKBP supported version if we can't
+			 * obtain supported version correctly. Please note that
+			 * calling EC_CMD_GET_NEXT_EVENT returned
+			 * EC_RES_INVALID_VERSION which means that the command
+			 * is present.
+			 */
+			return -ENOPROTOOPT;
+
+		ec_dev->mkbp_event_supported = fls(ver_mask);
+		dev_dbg(ec_dev->dev, "MKBP support version changed to %u\n",
+			ec_dev->mkbp_event_supported - 1);
+
+		/* Try to get next event with new MKBP support version set. */
+		ret = get_next_event(ec_dev);
+	}
+
 	if (ret <= 0)
 		return ret;
 
-- 
2.31.0

Re: [PATCH] platform/chrome: cros_ec_proto: Update version on GET_NEXT_EVENT failure

[Guenter Roeck]

On Thu, Jul 14, 2022 at 9:09 AM Patryk Duda <pdk@semihalf.com> wrote:
>
> Some EC based devices (e.g. Fingerpint MCU) can jump to RO part of the
> firmware (intentionally or due to device reboot). The RO part doesn't
> change during the device lifecycle, so it won't support newer version
> of EC_CMD_GET_NEXT_EVENT command.
>
> Function cros_ec_query_all() is responsible for finding maximum
> supported MKBP event version. It's usually called when the device is
> running RW part of the firmware, so the command version can be
> potentially higher than version supported by the RO.
>
> The problem was fixed by updating maximum supported version when the
> device returns EC_RES_INVALID_VERSION (mapped to -ENOPROTOOPT). That way
> the kernel will use highest common version supported by RO and RW.
>
> Fixes: 3300fdd630d4 ("platform/chrome: cros_ec: handle MKBP more events flag")
> Cc: <stable@vger.kernel.org> # 5.10+
> Signed-off-by: Patryk Duda <pdk@semihalf.com>

Nice catch. That may explain some hung task issues. observed in
EC_CMD_GET_NEXT_EVENT handling

Reviewed-by: Guenter Roeck <groeck@chromium.org>

> ---
> When Fingerprint MCU is rebooted (e.g. as a part of tests) it jumps to
> the RO image and performs RW image signature check. If kernel calls
> EC_CMD_GET_NEXT_EVENT FPMCU RO will respond with EC_RES_INVALID_VERSION
> because it's older than RW and supports up to version 1 of the command.
> As a result kernel keeps trying to get MKBP events and effectively
> blocks FPMCU from jumping to RW image.
>
> Before patch 3300fdd630d4 the driver called version 1 of the command.
> If the device responded with EC_RES_INVALID_VERSION, the driver would
> use version 0 of the command.
>
> Best regards,
> Patryk
>
>  drivers/platform/chrome/cros_ec_proto.c | 32 +++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index ff767dccdf0f6..0a131045d50a9 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -750,6 +750,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
>         u8 event_type;
>         u32 host_event;
>         int ret;
> +       u32 ver_mask = 0;
>
>         /*
>          * Default value for wake_event.
> @@ -771,6 +772,37 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
>                 return get_keyboard_state_event(ec_dev);
>
>         ret = get_next_event(ec_dev);
> +       /*
> +        * -ENOPROTOOPT is returned when EC returns EC_RES_INVALID_VERSION.
> +        * This can occur when EC based device (e.g. Fingerprint MCU) jumps to
> +        * the RO image which doesn't support newer version of the command. In
> +        * this case we will attempt to update maximum supported version of the
> +        * EC_CMD_GET_NEXT_EVENT.
> +        */
> +       if (ret == -ENOPROTOOPT) {
> +               dev_dbg(ec_dev->dev,
> +                       "GET_NEXT_EVENT returned invalid version error.\n");
> +               ret = cros_ec_get_host_command_version_mask(ec_dev,
> +                                                       EC_CMD_GET_NEXT_EVENT,
> +                                                       &ver_mask);
> +               if (ret < 0 || ver_mask == 0)
> +                       /*
> +                        * Do not change the MKBP supported version if we can't
> +                        * obtain supported version correctly. Please note that
> +                        * calling EC_CMD_GET_NEXT_EVENT returned
> +                        * EC_RES_INVALID_VERSION which means that the command
> +                        * is present.
> +                        */
> +                       return -ENOPROTOOPT;
> +
> +               ec_dev->mkbp_event_supported = fls(ver_mask);
> +               dev_dbg(ec_dev->dev, "MKBP support version changed to %u\n",
> +                       ec_dev->mkbp_event_supported - 1);
> +
> +               /* Try to get next event with new MKBP support version set. */
> +               ret = get_next_event(ec_dev);
> +       }
> +
>         if (ret <= 0)
>                 return ret;
>
> --
> 2.31.0
>

Re: [PATCH] platform/chrome: cros_ec_proto: Update version on GET_NEXT_EVENT failure

[Tzung-Bi Shih]

On Thu, Jul 14, 2022 at 06:09:51PM +0200, Patryk Duda wrote:
> @@ -750,6 +750,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
>  	u8 event_type;
>  	u32 host_event;
>  	int ret;
> +	u32 ver_mask = 0;

Drop the initialization.  Ideally, `ver_mask` wouldn't be touched if `ret` is
less than 0.

[PATCH v1] platform/chrome: cros_ec_proto: Update version on GET_NEXT_EVENT failure

[Patryk Duda]

Some EC based devices (e.g. Fingerpint MCU) can jump to RO part of the
firmware (intentionally or due to device reboot). The RO part doesn't
change during the device lifecycle, so it won't support newer version
of EC_CMD_GET_NEXT_EVENT command.

Function cros_ec_query_all() is responsible for finding maximum
supported MKBP event version. It's usually called when the device is
running RW part of the firmware, so the command version can be
potentially higher than version supported by the RO.

The problem was fixed by updating maximum supported version when the
device returns EC_RES_INVALID_VERSION (mapped to -ENOPROTOOPT). That way
the kernel will use highest common version supported by RO and RW.

Fixes: 3300fdd630d4 ("platform/chrome: cros_ec: handle MKBP more events flag")
Cc: <stable@vger.kernel.org> # 5.10+
Signed-off-by: Patryk Duda <pdk@semihalf.com>
---
When Fingerprint MCU is rebooted (e.g. as a part of tests) it jumps to
the RO image and performs RW image signature check. If kernel calls
EC_CMD_GET_NEXT_EVENT FPMCU RO will respond with EC_RES_INVALID_VERSION
because it's older than RW and supports up to version 1 of the command.
As a result kernel keeps trying to get MKBP events and effectively
blocks FPMCU from jumping to RW image.

Before patch 3300fdd630d4 the driver called version 1 of the command.
If the device responded with EC_RES_INVALID_VERSION, the driver would
use version 0 of the command.

Best regards,
Patryk

v0 -> v1
- Dropped `ver_mask` initialization.

 drivers/platform/chrome/cros_ec_proto.c | 32 +++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index ff767dccdf0f..c1df8e7e48af 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -750,6 +750,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
 	u8 event_type;
 	u32 host_event;
 	int ret;
+	u32 ver_mask;
 
 	/*
 	 * Default value for wake_event.
@@ -771,6 +772,37 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
 		return get_keyboard_state_event(ec_dev);
 
 	ret = get_next_event(ec_dev);
+	/*
+	 * -ENOPROTOOPT is returned when EC returns EC_RES_INVALID_VERSION.
+	 * This can occur when EC based device (e.g. Fingerprint MCU) jumps to
+	 * the RO image which doesn't support newer version of the command. In
+	 * this case we will attempt to update maximum supported version of the
+	 * EC_CMD_GET_NEXT_EVENT.
+	 */
+	if (ret == -ENOPROTOOPT) {
+		dev_dbg(ec_dev->dev,
+			"GET_NEXT_EVENT returned invalid version error.\n");
+		ret = cros_ec_get_host_command_version_mask(ec_dev,
+							EC_CMD_GET_NEXT_EVENT,
+							&ver_mask);
+		if (ret < 0 || ver_mask == 0)
+			/*
+			 * Do not change the MKBP supported version if we can't
+			 * obtain supported version correctly. Please note that
+			 * calling EC_CMD_GET_NEXT_EVENT returned
+			 * EC_RES_INVALID_VERSION which means that the command
+			 * is present.
+			 */
+			return -ENOPROTOOPT;
+
+		ec_dev->mkbp_event_supported = fls(ver_mask);
+		dev_dbg(ec_dev->dev, "MKBP support version changed to %u\n",
+			ec_dev->mkbp_event_supported - 1);
+
+		/* Try to get next event with new MKBP support version set. */
+		ret = get_next_event(ec_dev);
+	}
+
 	if (ret <= 0)
 		return ret;
 
-- 
2.35.1

Re: [PATCH v1] platform/chrome: cros_ec_proto: Update version on GET_NEXT_EVENT failure

[Tzung-Bi Shih]

On Tue, Aug 02, 2022 at 05:41:28PM +0200, Patryk Duda wrote:
> Fixes: 3300fdd630d4 ("platform/chrome: cros_ec: handle MKBP more events flag")
> Cc: <stable@vger.kernel.org> # 5.10+

Any concerns if removing the Cc tag?  As I think a Fixes tag should be
sufficient.  On a related note, why did you specify for only 5.10+?

> Signed-off-by: Patryk Duda <pdk@semihalf.com>

You should collect the Reviewed-by tags the patch already got as dropping
`ver_mask` initialization isn't a big change.  I could do that for the patch
this time.

[...]
> v0 -> v1
> - Dropped `ver_mask` initialization.

Please start versioning from v1 next time.

Re: [PATCH v1] platform/chrome: cros_ec_proto: Update version on GET_NEXT_EVENT failure

[Patryk Duda]

czw., 4 sie 2022 o 11:00 Tzung-Bi Shih <tzungbi@kernel.org> napisał(a):
>
> On Tue, Aug 02, 2022 at 05:41:28PM +0200, Patryk Duda wrote:
> > Fixes: 3300fdd630d4 ("platform/chrome: cros_ec: handle MKBP more events flag")
> > Cc: <stable@vger.kernel.org> # 5.10+
>
> Any concerns if removing the Cc tag?  As I think a Fixes tag should be
> sufficient.  On a related note, why did you specify for only 5.10+?
Submitting patches document mentions that 'Fixes:' helps stable kernel
team in determining which stable kernel versions should receive the fix.

However, there is a note:
"Attaching a Fixes: tag does not subvert the stable kernel rules process
nor the requirement to Cc: stable@vger.kernel.org on all stable patch
candidates."
https://www.kernel.org/doc/html/v5.19/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes

Stable kernel versions older than 5.10 don't suffer from this issue.
>
>
> > Signed-off-by: Patryk Duda <pdk@semihalf.com>
>
> You should collect the Reviewed-by tags the patch already got as dropping
> `ver_mask` initialization isn't a big change.  I could do that for the patch
> this time.
Thank you!
>
> [...]
> > v0 -> v1
> > - Dropped `ver_mask` initialization.
>
> Please start versioning from v1 next time.
Ok, thanks!

Re: [PATCH v1] platform/chrome: cros_ec_proto: Update version on GET_NEXT_EVENT failure

[patchwork-bot+chrome-platform]

Hello:

This patch was applied to chrome-platform/linux.git (for-kernelci)
by Tzung-Bi Shih <tzungbi@kernel.org>:

On Tue,  2 Aug 2022 17:41:28 +0200 you wrote:
> Some EC based devices (e.g. Fingerpint MCU) can jump to RO part of the
> firmware (intentionally or due to device reboot). The RO part doesn't
> change during the device lifecycle, so it won't support newer version
> of EC_CMD_GET_NEXT_EVENT command.
> 
> Function cros_ec_query_all() is responsible for finding maximum
> supported MKBP event version. It's usually called when the device is
> running RW part of the firmware, so the command version can be
> potentially higher than version supported by the RO.
> 
> [...]

Here is the summary with links:
  - [v1] platform/chrome: cros_ec_proto: Update version on GET_NEXT_EVENT failure
    https://git.kernel.org/chrome-platform/c/c2b1dc63ba41

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v1] platform/chrome: cros_ec_proto: Update version on GET_NEXT_EVENT failure

[patchwork-bot+chrome-platform]

Hello:

This patch was applied to chrome-platform/linux.git (for-kernelci)
by Tzung-Bi Shih <tzungbi@kernel.org>:

On Tue,  2 Aug 2022 17:41:28 +0200 you wrote:
> Some EC based devices (e.g. Fingerpint MCU) can jump to RO part of the
> firmware (intentionally or due to device reboot). The RO part doesn't
> change during the device lifecycle, so it won't support newer version
> of EC_CMD_GET_NEXT_EVENT command.
> 
> Function cros_ec_query_all() is responsible for finding maximum
> supported MKBP event version. It's usually called when the device is
> running RW part of the firmware, so the command version can be
> potentially higher than version supported by the RO.
> 
> [...]

Here is the summary with links:
  - [v1] platform/chrome: cros_ec_proto: Update version on GET_NEXT_EVENT failure
    https://git.kernel.org/chrome-platform/c/f74c7557ed0d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v1] platform/chrome: cros_ec_proto: Update version on GET_NEXT_EVENT failure

[patchwork-bot+chrome-platform]

Hello:

This patch was applied to chrome-platform/linux.git (for-next)
by Tzung-Bi Shih <tzungbi@kernel.org>:

On Tue,  2 Aug 2022 17:41:28 +0200 you wrote:
> Some EC based devices (e.g. Fingerpint MCU) can jump to RO part of the
> firmware (intentionally or due to device reboot). The RO part doesn't
> change during the device lifecycle, so it won't support newer version
> of EC_CMD_GET_NEXT_EVENT command.
> 
> Function cros_ec_query_all() is responsible for finding maximum
> supported MKBP event version. It's usually called when the device is
> running RW part of the firmware, so the command version can be
> potentially higher than version supported by the RO.
> 
> [...]

Here is the summary with links:
  - [v1] platform/chrome: cros_ec_proto: Update version on GET_NEXT_EVENT failure
    https://git.kernel.org/chrome-platform/c/f74c7557ed0d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html