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

[PATCH] 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>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Alyssa, could you kindly test this with your existing setup? Thanks!

 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..7188f1d72f68 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -1076,6 +1076,12 @@ static int cros_typec_probe(struct platform_device *pdev)
 
 	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] platform/chrome: cros_ec_typec: Check for EC device

[Tzung-Bi Shih]

On Wed, Jan 26, 2022 at 01:22:03AM +0000, Prashant Malani wrote:
> Fixes: fdc6b21e2444 ("platform/chrome: Add Type C connector class driver")
> Reported-by: Alyssa Ross <hi@alyssa.is>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>

With a minor comment,
Reviewed-by: Tzung-Bi Shih <tzungbi@google.com>

> @@ -1076,6 +1076,12 @@ static int cros_typec_probe(struct platform_device *pdev)
>  
>  	typec->dev = dev;
>  	typec->ec = dev_get_drvdata(pdev->dev.parent);
> +

I would prefer to remove the blank line to make it look like an integrated block.

> +	if (!typec->ec) {
> +		dev_err(dev, "couldn't find parent EC device\n");
> +		return -ENODEV;
> +	}
> +

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

[Prashant Malani]

Hi Tzung-Bi,

Thanks for your review.

On Tue, Jan 25, 2022 at 7:46 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
>
> On Wed, Jan 26, 2022 at 01:22:03AM +0000, Prashant Malani wrote:
> > Fixes: fdc6b21e2444 ("platform/chrome: Add Type C connector class driver")
> > Reported-by: Alyssa Ross <hi@alyssa.is>
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
>
> With a minor comment,
> Reviewed-by: Tzung-Bi Shih <tzungbi@google.com>
>
> > @@ -1076,6 +1076,12 @@ static int cros_typec_probe(struct platform_device *pdev)
> >
> >       typec->dev = dev;
> >       typec->ec = dev_get_drvdata(pdev->dev.parent);
> > +
>
> I would prefer to remove the blank line to make it look like an integrated block.

I actually prefer it as it is. typec->dev is not really part of this
"integrated block", and I don't want to add another space there.
In any case, since this is a very minor style nit, I will address it
in case there is another version required due to other comments.

>
> > +     if (!typec->ec) {
> > +             dev_err(dev, "couldn't find parent EC device\n");
> > +             return -ENODEV;
> > +     }
> > +

Best,

-Prashant

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

[Guenter Roeck]

On Tue, Jan 25, 2022 at 8:05 PM Prashant Malani <pmalani@chromium.org> wrote:
>
> Hi Tzung-Bi,
>
> Thanks for your review.
>
> On Tue, Jan 25, 2022 at 7:46 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
> >
> > On Wed, Jan 26, 2022 at 01:22:03AM +0000, Prashant Malani wrote:
> > > Fixes: fdc6b21e2444 ("platform/chrome: Add Type C connector class driver")
> > > Reported-by: Alyssa Ross <hi@alyssa.is>
> > > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> >
> > With a minor comment,
> > Reviewed-by: Tzung-Bi Shih <tzungbi@google.com>
> >
> > > @@ -1076,6 +1076,12 @@ static int cros_typec_probe(struct platform_device *pdev)
> > >
> > >       typec->dev = dev;
> > >       typec->ec = dev_get_drvdata(pdev->dev.parent);
> > > +
> >
> > I would prefer to remove the blank line to make it look like an integrated block.
>
> I actually prefer it as it is. typec->dev is not really part of this
> "integrated block", and I don't want to add another space there.

But on the other side the check is part of the "integrated block".
Maybe add an empty line between the two assignments if you want a
separation.

> In any case, since this is a very minor style nit, I will address it
> in case there is another version required due to other comments.
>
> >
> > > +     if (!typec->ec) {
> > > +             dev_err(dev, "couldn't find parent EC device\n");
> > > +             return -ENODEV;
> > > +     }
> > > +
>
> Best,
>
> -Prashant

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

[Prashant Malani]

Hey Guenter,

On Wed, Jan 26, 2022 at 7:33 AM Guenter Roeck <groeck@google.com> wrote:
>
> On Tue, Jan 25, 2022 at 8:05 PM Prashant Malani <pmalani@chromium.org> wrote:
> >
> > Hi Tzung-Bi,
> >
> > Thanks for your review.
> >
> > On Tue, Jan 25, 2022 at 7:46 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
> > >
> > > On Wed, Jan 26, 2022 at 01:22:03AM +0000, Prashant Malani wrote:
> > > > Fixes: fdc6b21e2444 ("platform/chrome: Add Type C connector class driver")
> > > > Reported-by: Alyssa Ross <hi@alyssa.is>
> > > > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > >
> > > With a minor comment,
> > > Reviewed-by: Tzung-Bi Shih <tzungbi@google.com>
> > >
> > > > @@ -1076,6 +1076,12 @@ static int cros_typec_probe(struct platform_device *pdev)
> > > >
> > > >       typec->dev = dev;
> > > >       typec->ec = dev_get_drvdata(pdev->dev.parent);
> > > > +
> > >
> > > I would prefer to remove the blank line to make it look like an integrated block.
> >
> > I actually prefer it as it is. typec->dev is not really part of this
> > "integrated block", and I don't want to add another space there.
>
> But on the other side the check is part of the "integrated block".
> Maybe add an empty line between the two assignments if you want a
> separation.

OK. I'll add the space before it.

Thanks,