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 <[email protected]>
Reviewed-by: Tzung-Bi Shih <[email protected]>
Signed-off-by: Prashant Malani <[email protected]>
---
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
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 <[email protected]>
> Reviewed-by: Tzung-Bi Shih <[email protected]>
> Signed-off-by: Prashant Malani <[email protected]>
> ---
> 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
>
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 <[email protected]>
> > Reviewed-by: Tzung-Bi Shih <[email protected]>
> > Signed-off-by: Prashant Malani <[email protected]>
> > ---
> > 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
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 <[email protected]>
> Reviewed-by: Tzung-Bi Shih <[email protected]>
> Signed-off-by: Prashant Malani <[email protected]>
FWIW:
Acked-by: Heikki Krogerus <[email protected]>
> ---
> 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
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 <[email protected]>
> > > Reviewed-by: Tzung-Bi Shih <[email protected]>
> > > Signed-off-by: Prashant Malani <[email protected]>
> > > ---
> > > 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 <[email protected]>
Tested-by: Alyssa Ross <[email protected]>
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
Hi Alyssa,
On Tue, Feb 1, 2022 at 4:17 AM Alyssa Ross <[email protected]> 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 <[email protected]>
> > > > Reviewed-by: Tzung-Bi Shih <[email protected]>
> > > > Signed-off-by: Prashant Malani <[email protected]>
> > > > ---
> > > > 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,
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.
[email protected]
Chromium OS Project
[email protected]