[PATCH v2] platform/chrome: cros_ec_typec: Check for EC device

[PATCH v2] platform/chrome: cros_ec_typec: Check for EC device

[Prashant Malani]

The Type C ACPI device on older Chromebooks is not generated correctly
(since their EC firmware doesn't support the new commands required). In
such cases, the crafted ACPI device doesn't have an EC parent, and it is
therefore not useful (it shouldn't be generated in the first place since
the EC firmware doesn't support any of the Type C commands).

To handle devices which use these older firmware revisions, check for
the parent EC device handle, and fail the probe if it's not found.

Fixes: fdc6b21e2444 ("platform/chrome: Add Type C connector class driver")
Reported-by: Alyssa Ross <hi@alyssa.is>
Reviewed-by: Tzung-Bi Shih <tzungbi@google.com>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---
Hi Alyssa, could you kindly test this with your existing setup? Thanks!

Changes in v2:
- Added newlines as suggested by reviewers.
- Added Reviewed-by tag from Tzung-Bi.

 drivers/platform/chrome/cros_ec_typec.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 5de0bfb0bc4d..952c1756f59e 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -1075,7 +1075,13 @@ static int cros_typec_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	typec->dev = dev;
+
 	typec->ec = dev_get_drvdata(pdev->dev.parent);
+	if (!typec->ec) {
+		dev_err(dev, "couldn't find parent EC device\n");
+		return -ENODEV;
+	}
+
 	platform_set_drvdata(pdev, typec);
 
 	ret = cros_typec_get_cmd_version(typec);
-- 
2.35.0.rc0.227.g00780c9af4-goog

Re: [PATCH v2] platform/chrome: cros_ec_typec: Check for EC device

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 1959 bytes --]

On Wed, Jan 26, 2022 at 07:02:20PM +0000, Prashant Malani wrote:
> The Type C ACPI device on older Chromebooks is not generated correctly
> (since their EC firmware doesn't support the new commands required). In
> such cases, the crafted ACPI device doesn't have an EC parent, and it is
> therefore not useful (it shouldn't be generated in the first place since
> the EC firmware doesn't support any of the Type C commands).
>
> To handle devices which use these older firmware revisions, check for
> the parent EC device handle, and fail the probe if it's not found.
>
> Fixes: fdc6b21e2444 ("platform/chrome: Add Type C connector class driver")
> Reported-by: Alyssa Ross <hi@alyssa.is>
> Reviewed-by: Tzung-Bi Shih <tzungbi@google.com>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> Hi Alyssa, could you kindly test this with your existing setup? Thanks!

Hi Prashant, I'm happy to test, but I'm on vacation until the end of the
week so probably won't get a chance before Monday.

I'm guessing I should be testing with latest upstream coreboot (now that
your fix there has been applied)?

> Changes in v2:
> - Added newlines as suggested by reviewers.
> - Added Reviewed-by tag from Tzung-Bi.
>
>  drivers/platform/chrome/cros_ec_typec.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 5de0bfb0bc4d..952c1756f59e 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -1075,7 +1075,13 @@ static int cros_typec_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>
>  	typec->dev = dev;
> +
>  	typec->ec = dev_get_drvdata(pdev->dev.parent);
> +	if (!typec->ec) {
> +		dev_err(dev, "couldn't find parent EC device\n");
> +		return -ENODEV;
> +	}
> +
>  	platform_set_drvdata(pdev, typec);
>
>  	ret = cros_typec_get_cmd_version(typec);
> --
> 2.35.0.rc0.227.g00780c9af4-goog
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] platform/chrome: cros_ec_typec: Check for EC device

[Prashant Malani]

Hi Alyssa,

On Jan 26 23:04, Alyssa Ross wrote:
> On Wed, Jan 26, 2022 at 07:02:20PM +0000, Prashant Malani wrote:
> > The Type C ACPI device on older Chromebooks is not generated correctly
> > (since their EC firmware doesn't support the new commands required). In
> > such cases, the crafted ACPI device doesn't have an EC parent, and it is
> > therefore not useful (it shouldn't be generated in the first place since
> > the EC firmware doesn't support any of the Type C commands).
> >
> > To handle devices which use these older firmware revisions, check for
> > the parent EC device handle, and fail the probe if it's not found.
> >
> > Fixes: fdc6b21e2444 ("platform/chrome: Add Type C connector class driver")
> > Reported-by: Alyssa Ross <hi@alyssa.is>
> > Reviewed-by: Tzung-Bi Shih <tzungbi@google.com>
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> > Hi Alyssa, could you kindly test this with your existing setup? Thanks!
> 
> Hi Prashant, I'm happy to test, but I'm on vacation until the end of the
> week so probably won't get a chance before Monday.

No worries, whenever you get the chance is fine.

> 
> I'm guessing I should be testing with latest upstream coreboot (now that
> your fix there has been applied)?

You should use the coreboot with which you discovered the crash, so the
one which *doesn't* contain the fix.

Thanks again!

-Prashant

Re: [PATCH v2] platform/chrome: cros_ec_typec: Check for EC device

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 2302 bytes --]

Hi Prashant,

On Wed, Jan 26, 2022 at 11:08:14PM +0000, Prashant Malani wrote:
> Hi Alyssa,
>
> On Jan 26 23:04, Alyssa Ross wrote:
> > On Wed, Jan 26, 2022 at 07:02:20PM +0000, Prashant Malani wrote:
> > > The Type C ACPI device on older Chromebooks is not generated correctly
> > > (since their EC firmware doesn't support the new commands required). In
> > > such cases, the crafted ACPI device doesn't have an EC parent, and it is
> > > therefore not useful (it shouldn't be generated in the first place since
> > > the EC firmware doesn't support any of the Type C commands).
> > >
> > > To handle devices which use these older firmware revisions, check for
> > > the parent EC device handle, and fail the probe if it's not found.
> > >
> > > Fixes: fdc6b21e2444 ("platform/chrome: Add Type C connector class driver")
> > > Reported-by: Alyssa Ross <hi@alyssa.is>
> > > Reviewed-by: Tzung-Bi Shih <tzungbi@google.com>
> > > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > > ---
> > > Hi Alyssa, could you kindly test this with your existing setup? Thanks!
> >
> > Hi Prashant, I'm happy to test, but I'm on vacation until the end of the
> > week so probably won't get a chance before Monday.
>
> No worries, whenever you get the chance is fine.
>
> >
> > I'm guessing I should be testing with latest upstream coreboot (now that
> > your fix there has been applied)?
>
> You should use the coreboot with which you discovered the crash, so the
> one which *doesn't* contain the fix.

I applied this patch to Linux 5.17-rc2 and tested with the
coreboot_tiano-eve-mrchromebox-20210806.rom firmware.

The Oops has been replaced with
"cros-ec-typec: GOOG0014:00: couldn't find parent EC device".
My laptop now reboots correctly — the hang on reboot is gone.

The cros_ec_typec driver ends up being loaded, but no devices are bound
to it.  This differs from the behaviour with upstream coreboot
(cabf9e33a7), where cros_ec_typec does not end up being loaded.

Assuming all that's the intended behaviour:

Reviewed-by: Alyssa Ross <hi@alyssa.is>
Tested-by: Alyssa Ross <hi@alyssa.is>

BTW: if you need anything else tested on an eve running upstream Linux
and Coreboot in future, I'm happy to be CCed. :)

> Thanks again!
>
> -Prashant

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] platform/chrome: cros_ec_typec: Check for EC device

[Prashant Malani]

Hi Alyssa,

On Tue, Feb 1, 2022 at 4:17 AM Alyssa Ross <hi@alyssa.is> wrote:
>
> Hi Prashant,
>
> On Wed, Jan 26, 2022 at 11:08:14PM +0000, Prashant Malani wrote:
> > Hi Alyssa,
> >
> > On Jan 26 23:04, Alyssa Ross wrote:
> > > On Wed, Jan 26, 2022 at 07:02:20PM +0000, Prashant Malani wrote:
> > > > The Type C ACPI device on older Chromebooks is not generated correctly
> > > > (since their EC firmware doesn't support the new commands required). In
> > > > such cases, the crafted ACPI device doesn't have an EC parent, and it is
> > > > therefore not useful (it shouldn't be generated in the first place since
> > > > the EC firmware doesn't support any of the Type C commands).
> > > >
> > > > To handle devices which use these older firmware revisions, check for
> > > > the parent EC device handle, and fail the probe if it's not found.
> > > >
> > > > Fixes: fdc6b21e2444 ("platform/chrome: Add Type C connector class driver")
> > > > Reported-by: Alyssa Ross <hi@alyssa.is>
> > > > Reviewed-by: Tzung-Bi Shih <tzungbi@google.com>
> > > > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > > > ---
> > > > Hi Alyssa, could you kindly test this with your existing setup? Thanks!
> > >
> > > Hi Prashant, I'm happy to test, but I'm on vacation until the end of the
> > > week so probably won't get a chance before Monday.
> >
> > No worries, whenever you get the chance is fine.
> >
> > >
> > > I'm guessing I should be testing with latest upstream coreboot (now that
> > > your fix there has been applied)?
> >
> > You should use the coreboot with which you discovered the crash, so the
> > one which *doesn't* contain the fix.
>
> I applied this patch to Linux 5.17-rc2 and tested with the
> coreboot_tiano-eve-mrchromebox-20210806.rom firmware.
>
> The Oops has been replaced with
> "cros-ec-typec: GOOG0014:00: couldn't find parent EC device".
> My laptop now reboots correctly — the hang on reboot is gone.
>
> The cros_ec_typec driver ends up being loaded, but no devices are bound
> to it.  This differs from the behaviour with upstream coreboot
> (cabf9e33a7), where cros_ec_typec does not end up being loaded.
>
> Assuming all that's the intended behaviour:

Yep, that's WAI. Thanks a lot for testing this, and yes, we'll be sure
to reach out
for future help with validating changes.

Best regards,

Re: [PATCH v2] platform/chrome: cros_ec_typec: Check for EC device

[Heikki Krogerus]

On Wed, Jan 26, 2022 at 07:02:20PM +0000, Prashant Malani wrote:
> The Type C ACPI device on older Chromebooks is not generated correctly
> (since their EC firmware doesn't support the new commands required). In
> such cases, the crafted ACPI device doesn't have an EC parent, and it is
> therefore not useful (it shouldn't be generated in the first place since
> the EC firmware doesn't support any of the Type C commands).
> 
> To handle devices which use these older firmware revisions, check for
> the parent EC device handle, and fail the probe if it's not found.
> 
> Fixes: fdc6b21e2444 ("platform/chrome: Add Type C connector class driver")
> Reported-by: Alyssa Ross <hi@alyssa.is>
> Reviewed-by: Tzung-Bi Shih <tzungbi@google.com>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>

FWIW:

Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> Hi Alyssa, could you kindly test this with your existing setup? Thanks!
> 
> Changes in v2:
> - Added newlines as suggested by reviewers.
> - Added Reviewed-by tag from Tzung-Bi.
> 
>  drivers/platform/chrome/cros_ec_typec.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 5de0bfb0bc4d..952c1756f59e 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -1075,7 +1075,13 @@ static int cros_typec_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	typec->dev = dev;
> +
>  	typec->ec = dev_get_drvdata(pdev->dev.parent);
> +	if (!typec->ec) {
> +		dev_err(dev, "couldn't find parent EC device\n");
> +		return -ENODEV;
> +	}
> +
>  	platform_set_drvdata(pdev, typec);
>  
>  	ret = cros_typec_get_cmd_version(typec);
> -- 
> 2.35.0.rc0.227.g00780c9af4-goog

-- 
heikki

Re: [PATCH v2] platform/chrome: cros_ec_typec: Check for EC device

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 862 bytes --]

On Wed, 26 Jan 2022 19:02:20 +0000, Prashant Malani wrote:
> The Type C ACPI device on older Chromebooks is not generated correctly
> (since their EC firmware doesn't support the new commands required). In
> such cases, the crafted ACPI device doesn't have an EC parent, and it is
> therefore not useful (it shouldn't be generated in the first place since
> the EC firmware doesn't support any of the Type C commands).
> 
> To handle devices which use these older firmware revisions, check for
> the parent EC device handle, and fail the probe if it's not found.

Applied, thanks!

[1/1] platform/chrome: cros_ec_typec: Check for EC device
      commit: ffebd90532728086007038986900426544e3df4e

Best regards,
-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]