[PATCH] platform/chrome: cros_ec_typec: Handle NULL EC pointer during probe.

[PATCH] platform/chrome: cros_ec_typec: Handle NULL EC pointer during probe.

[Daniil Lunev]

Missing EC in device hierarchy causes NULL pointer to be returned to the
probe function which leads to NULL pointer dereference when trying to
send a command to the EC. This can be the case if the device is missing
or incorrectly configured in the firmware blob. Even if the situation
occures, the driver shall not cause a kernel panic as the condition is
not critical for the system functions.

Signed-off-by: Daniil Lunev <dlunev@chromium.org>
---

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

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 874269c07073..30d99c930445 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -301,6 +301,11 @@ 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, "Failed to get Cros EC data\n");
+		return -EINVAL;
+	}
+
 	platform_set_drvdata(pdev, typec);
 
 	ret = cros_typec_get_cmd_version(typec);
-- 
2.24.1

Re: [PATCH] platform/chrome: cros_ec_typec: Handle NULL EC pointer during probe.

[Enric Balletbo i Serra]

Hi Daniil,

Thank you for the patch.

On 28/4/20 3:02, Daniil Lunev wrote:
> Missing EC in device hierarchy causes NULL pointer to be returned to the
> probe function which leads to NULL pointer dereference when trying to
> send a command to the EC. This can be the case if the device is missing
> or incorrectly configured in the firmware blob. Even if the situation

There is any production device with a buggy firmware outside? Or this is just
for defensive programming while developing the firmware? Which device is
affected for this issue?

Thanks,
 Enric

> occures, the driver shall not cause a kernel panic as the condition is
> not critical for the system functions.
> 
> Signed-off-by: Daniil Lunev <dlunev@chromium.org>
> ---
> 
>  drivers/platform/chrome/cros_ec_typec.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 874269c07073..30d99c930445 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -301,6 +301,11 @@ 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, "Failed to get Cros EC data\n");
> +		return -EINVAL;
> +	}
> +
>  	platform_set_drvdata(pdev, typec);
>  
>  	ret = cros_typec_get_cmd_version(typec);
> 

Re: [PATCH] platform/chrome: cros_ec_typec: Handle NULL EC pointer during probe.

[Daniil Lunev]

[to make it appear on the mailing list as I didn't realize I was in
hypertext sending mode]

On Thu, Apr 30, 2020 at 10:11 AM Daniil Lunev <dlunev@chromium.org> wrote:
>
> Hi Enric.
> I encountered the issue on a Hatch device when trying running 5.4 kernel on that. After talking to Prashant it seems that any device with coreboot built before a certain point (a particular fix for device hierarchy in ACPI tables of Chrome devices which happened in mid-April) will not be able to correctly initialize the driver and will get a kernel panic trying to do so.
> Thanks,
> Daniil
>
> On Thu, Apr 30, 2020 at 7:58 AM Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote:
>>
>> Hi Daniil,
>>
>> Thank you for the patch.
>>
>> On 28/4/20 3:02, Daniil Lunev wrote:
>> > Missing EC in device hierarchy causes NULL pointer to be returned to the
>> > probe function which leads to NULL pointer dereference when trying to
>> > send a command to the EC. This can be the case if the device is missing
>> > or incorrectly configured in the firmware blob. Even if the situation
>>
>> There is any production device with a buggy firmware outside? Or this is just
>> for defensive programming while developing the firmware? Which device is
>> affected for this issue?
>>
>> Thanks,
>>  Enric
>>
>> > occures, the driver shall not cause a kernel panic as the condition is
>> > not critical for the system functions.
>> >
>> > Signed-off-by: Daniil Lunev <dlunev@chromium.org>
>> > ---
>> >
>> >  drivers/platform/chrome/cros_ec_typec.c | 5 +++++
>> >  1 file changed, 5 insertions(+)
>> >
>> > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
>> > index 874269c07073..30d99c930445 100644
>> > --- a/drivers/platform/chrome/cros_ec_typec.c
>> > +++ b/drivers/platform/chrome/cros_ec_typec.c
>> > @@ -301,6 +301,11 @@ 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, "Failed to get Cros EC data\n");
>> > +             return -EINVAL;
>> > +     }
>> > +
>> >       platform_set_drvdata(pdev, typec);
>> >
>> >       ret = cros_typec_get_cmd_version(typec);
>> >

Re: [PATCH] platform/chrome: cros_ec_typec: Handle NULL EC pointer during probe.

[Prashant Malani]

On Wed, Apr 29, 2020 at 5:38 PM Daniil Lunev <dlunev@chromium.org> wrote:
>
> [to make it appear on the mailing list as I didn't realize I was in
> hypertext sending mode]
>
> On Thu, Apr 30, 2020 at 10:11 AM Daniil Lunev <dlunev@chromium.org> wrote:
> >
> > Hi Enric.
> > I encountered the issue on a Hatch device when trying running 5.4 kernel on that. After talking to Prashant it seems that any device with coreboot built before a certain point (a particular fix for device hierarchy in ACPI tables of Chrome devices which happened in mid-April) will not be able to correctly initialize the driver and will get a kernel panic trying to do so.

A clarifying detail here: This should not be seen in any current
*production* device. No prod device firmware will carry the erroneous
ACPI device entry.

> > Thanks,
> > Daniil
> >
> > On Thu, Apr 30, 2020 at 7:58 AM Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote:
> >>
> >> Hi Daniil,
> >>
> >> Thank you for the patch.
> >>
> >> On 28/4/20 3:02, Daniil Lunev wrote:
> >> > Missing EC in device hierarchy causes NULL pointer to be returned to the
> >> > probe function which leads to NULL pointer dereference when trying to
> >> > send a command to the EC. This can be the case if the device is missing
> >> > or incorrectly configured in the firmware blob. Even if the situation
> >>
> >> There is any production device with a buggy firmware outside? Or this is just
> >> for defensive programming while developing the firmware? Which device is
> >> affected for this issue?
> >>
> >> Thanks,
> >>  Enric
> >>
> >> > occures, the driver shall not cause a kernel panic as the condition is
> >> > not critical for the system functions.
> >> >
> >> > Signed-off-by: Daniil Lunev <dlunev@chromium.org>
> >> > ---
> >> >
> >> >  drivers/platform/chrome/cros_ec_typec.c | 5 +++++
> >> >  1 file changed, 5 insertions(+)
> >> >
> >> > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> >> > index 874269c07073..30d99c930445 100644
> >> > --- a/drivers/platform/chrome/cros_ec_typec.c
> >> > +++ b/drivers/platform/chrome/cros_ec_typec.c
> >> > @@ -301,6 +301,11 @@ 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, "Failed to get Cros EC data\n");
> >> > +             return -EINVAL;
> >> > +     }
> >> > +
> >> >       platform_set_drvdata(pdev, typec);
> >> >
> >> >       ret = cros_typec_get_cmd_version(typec);
> >> >

Re: [PATCH] platform/chrome: cros_ec_typec: Handle NULL EC pointer during probe.

[Enric Balletbo i Serra]

Hi Prashant,

On 30/4/20 2:43, Prashant Malani wrote:
> On Wed, Apr 29, 2020 at 5:38 PM Daniil Lunev <dlunev@chromium.org> wrote:
>>
>> [to make it appear on the mailing list as I didn't realize I was in
>> hypertext sending mode]
>>
>> On Thu, Apr 30, 2020 at 10:11 AM Daniil Lunev <dlunev@chromium.org> wrote:
>>>
>>> Hi Enric.
>>> I encountered the issue on a Hatch device when trying running 5.4 kernel on that. After talking to Prashant it seems that any device with coreboot built before a certain point (a particular fix for device hierarchy in ACPI tables of Chrome devices which happened in mid-April) will not be able to correctly initialize the driver and will get a kernel panic trying to do so.
> 
> A clarifying detail here: This should not be seen in any current
> *production* device. No prod device firmware will carry the erroneous
> ACPI device entry.
> 

Thanks for the clarification. Then, I don't think we need to upstream this. This
kind of "defensive-programming" it's not something that should matter to upstream.

Thanks,
 Enric


>>> Thanks,
>>> Daniil
>>>
>>> On Thu, Apr 30, 2020 at 7:58 AM Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote:
>>>>
>>>> Hi Daniil,
>>>>
>>>> Thank you for the patch.
>>>>
>>>> On 28/4/20 3:02, Daniil Lunev wrote:
>>>>> Missing EC in device hierarchy causes NULL pointer to be returned to the
>>>>> probe function which leads to NULL pointer dereference when trying to
>>>>> send a command to the EC. This can be the case if the device is missing
>>>>> or incorrectly configured in the firmware blob. Even if the situation
>>>>
>>>> There is any production device with a buggy firmware outside? Or this is just
>>>> for defensive programming while developing the firmware? Which device is
>>>> affected for this issue?
>>>>
>>>> Thanks,
>>>>  Enric
>>>>
>>>>> occures, the driver shall not cause a kernel panic as the condition is
>>>>> not critical for the system functions.
>>>>>
>>>>> Signed-off-by: Daniil Lunev <dlunev@chromium.org>
>>>>> ---
>>>>>
>>>>>  drivers/platform/chrome/cros_ec_typec.c | 5 +++++
>>>>>  1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
>>>>> index 874269c07073..30d99c930445 100644
>>>>> --- a/drivers/platform/chrome/cros_ec_typec.c
>>>>> +++ b/drivers/platform/chrome/cros_ec_typec.c
>>>>> @@ -301,6 +301,11 @@ 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, "Failed to get Cros EC data\n");
>>>>> +             return -EINVAL;
>>>>> +     }
>>>>> +
>>>>>       platform_set_drvdata(pdev, typec);
>>>>>
>>>>>       ret = cros_typec_get_cmd_version(typec);
>>>>>

Re: [PATCH] platform/chrome: cros_ec_typec: Handle NULL EC pointer during probe.

[Prashant Malani]

Hi Enric,

On Thu, Apr 30, 2020 at 8:26 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Prashant,
>
> On 30/4/20 2:43, Prashant Malani wrote:
> > On Wed, Apr 29, 2020 at 5:38 PM Daniil Lunev <dlunev@chromium.org> wrote:
> >>
> >> [to make it appear on the mailing list as I didn't realize I was in
> >> hypertext sending mode]
> >>
> >> On Thu, Apr 30, 2020 at 10:11 AM Daniil Lunev <dlunev@chromium.org> wrote:
> >>>
> >>> Hi Enric.
> >>> I encountered the issue on a Hatch device when trying running 5.4 kernel on that. After talking to Prashant it seems that any device with coreboot built before a certain point (a particular fix for device hierarchy in ACPI tables of Chrome devices which happened in mid-April) will not be able to correctly initialize the driver and will get a kernel panic trying to do so.
> >
> > A clarifying detail here: This should not be seen in any current
> > *production* device. No prod device firmware will carry the erroneous
> > ACPI device entry.
> >
>
> Thanks for the clarification. Then, I don't think we need to upstream this. This
> kind of "defensive-programming" it's not something that should matter to upstream.

Actually, on second thought, I am not 100% sure about this:
Daniil, is the erroneous ACPI device on a *production* firmware for
this device (I'm not sure about the vintage of that device's BIOS)?

My apologies for the confusion, Enric and Daniil; but would be good to
get clarification from Daniil.

Best regards,
>
> Thanks,
>  Enric
>
>
> >>> Thanks,
> >>> Daniil
> >>>
> >>> On Thu, Apr 30, 2020 at 7:58 AM Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote:
> >>>>
> >>>> Hi Daniil,
> >>>>
> >>>> Thank you for the patch.
> >>>>
> >>>> On 28/4/20 3:02, Daniil Lunev wrote:
> >>>>> Missing EC in device hierarchy causes NULL pointer to be returned to the
> >>>>> probe function which leads to NULL pointer dereference when trying to
> >>>>> send a command to the EC. This can be the case if the device is missing
> >>>>> or incorrectly configured in the firmware blob. Even if the situation
> >>>>
> >>>> There is any production device with a buggy firmware outside? Or this is just
> >>>> for defensive programming while developing the firmware? Which device is
> >>>> affected for this issue?
> >>>>
> >>>> Thanks,
> >>>>  Enric
> >>>>
> >>>>> occures, the driver shall not cause a kernel panic as the condition is
> >>>>> not critical for the system functions.
> >>>>>
> >>>>> Signed-off-by: Daniil Lunev <dlunev@chromium.org>
> >>>>> ---
> >>>>>
> >>>>>  drivers/platform/chrome/cros_ec_typec.c | 5 +++++
> >>>>>  1 file changed, 5 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> >>>>> index 874269c07073..30d99c930445 100644
> >>>>> --- a/drivers/platform/chrome/cros_ec_typec.c
> >>>>> +++ b/drivers/platform/chrome/cros_ec_typec.c
> >>>>> @@ -301,6 +301,11 @@ 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, "Failed to get Cros EC data\n");
> >>>>> +             return -EINVAL;
> >>>>> +     }
> >>>>> +
> >>>>>       platform_set_drvdata(pdev, typec);
> >>>>>
> >>>>>       ret = cros_typec_get_cmd_version(typec);
> >>>>>

Re: [PATCH] platform/chrome: cros_ec_typec: Handle NULL EC pointer during probe.

[Daniil Lunev]

On the official revision of coreboot for hatch it doesn't even try to
load Type C. However it gives some warning messages from
cros-usbpd-notify-acpi about EC, So I wonder why the check of the same
type is not appropriate in the typec driver?

../chrome/cros_usbpd_notify.c

/* Get the EC device pointer needed to talk to the EC. */
ec_dev = dev_get_drvdata(dev->parent);
if (!ec_dev) {
/*
* We continue even for older devices which don't have the
* correct device heirarchy, namely, GOOG0003 is a child
* of GOOG0004.
*/
dev_warn(dev, "Couldn't get Chrome EC device pointer.\n");
}


# dmesg
...
[    8.513351] cros-ec-spi spi-PRP0001:02: EC failed to respond in time
[    8.722072] cros-ec-spi spi-PRP0001:02: EC failed to respond in time
[    8.729271] cros-ec-spi spi-PRP0001:02: Cannot identify the EC: error -110
[    8.736966] cros-ec-spi spi-PRP0001:02: cannot register EC,
fallback to spidev
[    8.767017] cros_ec_lpcs GOOG0004:00: Chrome EC device registered
[    8.807537] cros-usbpd-notify-acpi GOOG0003:00: Couldn't get Chrome
EC device pointer.
...

On Fri, May 1, 2020 at 2:17 AM Prashant Malani <pmalani@chromium.org> wrote:
>
> Hi Enric,
>
> On Thu, Apr 30, 2020 at 8:26 AM Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
> >
> > Hi Prashant,
> >
> > On 30/4/20 2:43, Prashant Malani wrote:
> > > On Wed, Apr 29, 2020 at 5:38 PM Daniil Lunev <dlunev@chromium.org> wrote:
> > >>
> > >> [to make it appear on the mailing list as I didn't realize I was in
> > >> hypertext sending mode]
> > >>
> > >> On Thu, Apr 30, 2020 at 10:11 AM Daniil Lunev <dlunev@chromium.org> wrote:
> > >>>
> > >>> Hi Enric.
> > >>> I encountered the issue on a Hatch device when trying running 5.4 kernel on that. After talking to Prashant it seems that any device with coreboot built before a certain point (a particular fix for device hierarchy in ACPI tables of Chrome devices which happened in mid-April) will not be able to correctly initialize the driver and will get a kernel panic trying to do so.
> > >
> > > A clarifying detail here: This should not be seen in any current
> > > *production* device. No prod device firmware will carry the erroneous
> > > ACPI device entry.
> > >
> >
> > Thanks for the clarification. Then, I don't think we need to upstream this. This
> > kind of "defensive-programming" it's not something that should matter to upstream.
>
> Actually, on second thought, I am not 100% sure about this:
> Daniil, is the erroneous ACPI device on a *production* firmware for
> this device (I'm not sure about the vintage of that device's BIOS)?
>
> My apologies for the confusion, Enric and Daniil; but would be good to
> get clarification from Daniil.
>
> Best regards,
> >
> > Thanks,
> >  Enric
> >
> >
> > >>> Thanks,
> > >>> Daniil
> > >>>
> > >>> On Thu, Apr 30, 2020 at 7:58 AM Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote:
> > >>>>
> > >>>> Hi Daniil,
> > >>>>
> > >>>> Thank you for the patch.
> > >>>>
> > >>>> On 28/4/20 3:02, Daniil Lunev wrote:
> > >>>>> Missing EC in device hierarchy causes NULL pointer to be returned to the
> > >>>>> probe function which leads to NULL pointer dereference when trying to
> > >>>>> send a command to the EC. This can be the case if the device is missing
> > >>>>> or incorrectly configured in the firmware blob. Even if the situation
> > >>>>
> > >>>> There is any production device with a buggy firmware outside? Or this is just
> > >>>> for defensive programming while developing the firmware? Which device is
> > >>>> affected for this issue?
> > >>>>
> > >>>> Thanks,
> > >>>>  Enric
> > >>>>
> > >>>>> occures, the driver shall not cause a kernel panic as the condition is
> > >>>>> not critical for the system functions.
> > >>>>>
> > >>>>> Signed-off-by: Daniil Lunev <dlunev@chromium.org>
> > >>>>> ---
> > >>>>>
> > >>>>>  drivers/platform/chrome/cros_ec_typec.c | 5 +++++
> > >>>>>  1 file changed, 5 insertions(+)
> > >>>>>
> > >>>>> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > >>>>> index 874269c07073..30d99c930445 100644
> > >>>>> --- a/drivers/platform/chrome/cros_ec_typec.c
> > >>>>> +++ b/drivers/platform/chrome/cros_ec_typec.c
> > >>>>> @@ -301,6 +301,11 @@ 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, "Failed to get Cros EC data\n");
> > >>>>> +             return -EINVAL;
> > >>>>> +     }
> > >>>>> +
> > >>>>>       platform_set_drvdata(pdev, typec);
> > >>>>>
> > >>>>>       ret = cros_typec_get_cmd_version(typec);
> > >>>>>

Re: [PATCH] platform/chrome: cros_ec_typec: Handle NULL EC pointer during probe.

[Prashant Malani]

Hi Daniil,

On Fri, May 01, 2020 at 10:15:18AM +1000, Daniil Lunev wrote:
> On the official revision of coreboot for hatch it doesn't even try to
> load Type C. However it gives some warning messages from
> cros-usbpd-notify-acpi about EC, So I wonder why the check of the same
> type is not appropriate in the typec driver?

I think the difference is that GOOG0003 is already present on shipped /
official versions of coreboot (so not having that check can cause
existing release images/devices to crash), whereas for GOOG0014 that is / isn't the case.

Is GOOG0014 present on the official release coreboot image for this
device? If so, what's its path (/sys/bus/acpi/devices/<HID>/path) ?

Best regards,

-Prashant
> 
> ../chrome/cros_usbpd_notify.c
> 
> /* Get the EC device pointer needed to talk to the EC. */
> ec_dev = dev_get_drvdata(dev->parent);
> if (!ec_dev) {
> /*
> * We continue even for older devices which don't have the
> * correct device heirarchy, namely, GOOG0003 is a child
> * of GOOG0004.
> */
> dev_warn(dev, "Couldn't get Chrome EC device pointer.\n");
> }
> 
> 
> # dmesg
> ...
> [    8.513351] cros-ec-spi spi-PRP0001:02: EC failed to respond in time
> [    8.722072] cros-ec-spi spi-PRP0001:02: EC failed to respond in time
> [    8.729271] cros-ec-spi spi-PRP0001:02: Cannot identify the EC: error -110
> [    8.736966] cros-ec-spi spi-PRP0001:02: cannot register EC,
> fallback to spidev
> [    8.767017] cros_ec_lpcs GOOG0004:00: Chrome EC device registered
> [    8.807537] cros-usbpd-notify-acpi GOOG0003:00: Couldn't get Chrome
> EC device pointer.
> ...
> 
> On Fri, May 1, 2020 at 2:17 AM Prashant Malani <pmalani@chromium.org> wrote:
> >
> > Hi Enric,
> >
> > On Thu, Apr 30, 2020 at 8:26 AM Enric Balletbo i Serra
> > <enric.balletbo@collabora.com> wrote:
> > >
> > > Hi Prashant,
> > >
> > > On 30/4/20 2:43, Prashant Malani wrote:
> > > > On Wed, Apr 29, 2020 at 5:38 PM Daniil Lunev <dlunev@chromium.org> wrote:
> > > >>
> > > >> [to make it appear on the mailing list as I didn't realize I was in
> > > >> hypertext sending mode]
> > > >>
> > > >> On Thu, Apr 30, 2020 at 10:11 AM Daniil Lunev <dlunev@chromium.org> wrote:
> > > >>>
> > > >>> Hi Enric.
> > > >>> I encountered the issue on a Hatch device when trying running 5.4 kernel on that. After talking to Prashant it seems that any device with coreboot built before a certain point (a particular fix for device hierarchy in ACPI tables of Chrome devices which happened in mid-April) will not be able to correctly initialize the driver and will get a kernel panic trying to do so.
> > > >
> > > > A clarifying detail here: This should not be seen in any current
> > > > *production* device. No prod device firmware will carry the erroneous
> > > > ACPI device entry.
> > > >
> > >
> > > Thanks for the clarification. Then, I don't think we need to upstream this. This
> > > kind of "defensive-programming" it's not something that should matter to upstream.
> >
> > Actually, on second thought, I am not 100% sure about this:
> > Daniil, is the erroneous ACPI device on a *production* firmware for
> > this device (I'm not sure about the vintage of that device's BIOS)?
> >
> > My apologies for the confusion, Enric and Daniil; but would be good to
> > get clarification from Daniil.
> >
> > Best regards,
> > >
> > > Thanks,
> > >  Enric
> > >
> > >
> > > >>> Thanks,
> > > >>> Daniil
> > > >>>
> > > >>> On Thu, Apr 30, 2020 at 7:58 AM Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote:
> > > >>>>
> > > >>>> Hi Daniil,
> > > >>>>
> > > >>>> Thank you for the patch.
> > > >>>>
> > > >>>> On 28/4/20 3:02, Daniil Lunev wrote:
> > > >>>>> Missing EC in device hierarchy causes NULL pointer to be returned to the
> > > >>>>> probe function which leads to NULL pointer dereference when trying to
> > > >>>>> send a command to the EC. This can be the case if the device is missing
> > > >>>>> or incorrectly configured in the firmware blob. Even if the situation
> > > >>>>
> > > >>>> There is any production device with a buggy firmware outside? Or this is just
> > > >>>> for defensive programming while developing the firmware? Which device is
> > > >>>> affected for this issue?
> > > >>>>
> > > >>>> Thanks,
> > > >>>>  Enric
> > > >>>>
> > > >>>>> occures, the driver shall not cause a kernel panic as the condition is
> > > >>>>> not critical for the system functions.
> > > >>>>>
> > > >>>>> Signed-off-by: Daniil Lunev <dlunev@chromium.org>
> > > >>>>> ---
> > > >>>>>
> > > >>>>>  drivers/platform/chrome/cros_ec_typec.c | 5 +++++
> > > >>>>>  1 file changed, 5 insertions(+)
> > > >>>>>
> > > >>>>> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > > >>>>> index 874269c07073..30d99c930445 100644
> > > >>>>> --- a/drivers/platform/chrome/cros_ec_typec.c
> > > >>>>> +++ b/drivers/platform/chrome/cros_ec_typec.c
> > > >>>>> @@ -301,6 +301,11 @@ 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, "Failed to get Cros EC data\n");
> > > >>>>> +             return -EINVAL;
> > > >>>>> +     }
> > > >>>>> +
> > > >>>>>       platform_set_drvdata(pdev, typec);
> > > >>>>>
> > > >>>>>       ret = cros_typec_get_cmd_version(typec);
> > > >>>>>

Re: [PATCH] platform/chrome: cros_ec_typec: Handle NULL EC pointer during probe.

[Daniil Lunev]

Hi Prashant,
I do not think it is present. Thinking about it, I do not think it
shall be an issue on any released device as it will have either a
firmware which wouldn't even trigger the typec probe or the one after
the hierarchy fix. Likely I just got a firmware which was somewhere in
between those two (As I did some unrelated FW testing). So, yes,
probably putting this upstream is not necessary, though IMO more
sanity checks - especially on non-critical run-once paths - are always
better than having a kernel panic lingering around the corner, not
like I am insisting on pushing the patch though with all the info, up
to Enric.
Cheers,
Daniil

On Fri, May 1, 2020 at 10:56 AM Prashant Malani <pmalani@chromium.org> wrote:
>
> Hi Daniil,
>
> On Fri, May 01, 2020 at 10:15:18AM +1000, Daniil Lunev wrote:
> > On the official revision of coreboot for hatch it doesn't even try to
> > load Type C. However it gives some warning messages from
> > cros-usbpd-notify-acpi about EC, So I wonder why the check of the same
> > type is not appropriate in the typec driver?
>
> I think the difference is that GOOG0003 is already present on shipped /
> official versions of coreboot (so not having that check can cause
> existing release images/devices to crash), whereas for GOOG0014 that is / isn't the case.
>
> Is GOOG0014 present on the official release coreboot image for this
> device? If so, what's its path (/sys/bus/acpi/devices/<HID>/path) ?
>
> Best regards,
>
> -Prashant
> >
> > ../chrome/cros_usbpd_notify.c
> >
> > /* Get the EC device pointer needed to talk to the EC. */
> > ec_dev = dev_get_drvdata(dev->parent);
> > if (!ec_dev) {
> > /*
> > * We continue even for older devices which don't have the
> > * correct device heirarchy, namely, GOOG0003 is a child
> > * of GOOG0004.
> > */
> > dev_warn(dev, "Couldn't get Chrome EC device pointer.\n");
> > }
> >
> >
> > # dmesg
> > ...
> > [    8.513351] cros-ec-spi spi-PRP0001:02: EC failed to respond in time
> > [    8.722072] cros-ec-spi spi-PRP0001:02: EC failed to respond in time
> > [    8.729271] cros-ec-spi spi-PRP0001:02: Cannot identify the EC: error -110
> > [    8.736966] cros-ec-spi spi-PRP0001:02: cannot register EC,
> > fallback to spidev
> > [    8.767017] cros_ec_lpcs GOOG0004:00: Chrome EC device registered
> > [    8.807537] cros-usbpd-notify-acpi GOOG0003:00: Couldn't get Chrome
> > EC device pointer.
> > ...
> >
> > On Fri, May 1, 2020 at 2:17 AM Prashant Malani <pmalani@chromium.org> wrote:
> > >
> > > Hi Enric,
> > >
> > > On Thu, Apr 30, 2020 at 8:26 AM Enric Balletbo i Serra
> > > <enric.balletbo@collabora.com> wrote:
> > > >
> > > > Hi Prashant,
> > > >
> > > > On 30/4/20 2:43, Prashant Malani wrote:
> > > > > On Wed, Apr 29, 2020 at 5:38 PM Daniil Lunev <dlunev@chromium.org> wrote:
> > > > >>
> > > > >> [to make it appear on the mailing list as I didn't realize I was in
> > > > >> hypertext sending mode]
> > > > >>
> > > > >> On Thu, Apr 30, 2020 at 10:11 AM Daniil Lunev <dlunev@chromium.org> wrote:
> > > > >>>
> > > > >>> Hi Enric.
> > > > >>> I encountered the issue on a Hatch device when trying running 5.4 kernel on that. After talking to Prashant it seems that any device with coreboot built before a certain point (a particular fix for device hierarchy in ACPI tables of Chrome devices which happened in mid-April) will not be able to correctly initialize the driver and will get a kernel panic trying to do so.
> > > > >
> > > > > A clarifying detail here: This should not be seen in any current
> > > > > *production* device. No prod device firmware will carry the erroneous
> > > > > ACPI device entry.
> > > > >
> > > >
> > > > Thanks for the clarification. Then, I don't think we need to upstream this. This
> > > > kind of "defensive-programming" it's not something that should matter to upstream.
> > >
> > > Actually, on second thought, I am not 100% sure about this:
> > > Daniil, is the erroneous ACPI device on a *production* firmware for
> > > this device (I'm not sure about the vintage of that device's BIOS)?
> > >
> > > My apologies for the confusion, Enric and Daniil; but would be good to
> > > get clarification from Daniil.
> > >
> > > Best regards,
> > > >
> > > > Thanks,
> > > >  Enric
> > > >
> > > >
> > > > >>> Thanks,
> > > > >>> Daniil
> > > > >>>
> > > > >>> On Thu, Apr 30, 2020 at 7:58 AM Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote:
> > > > >>>>
> > > > >>>> Hi Daniil,
> > > > >>>>
> > > > >>>> Thank you for the patch.
> > > > >>>>
> > > > >>>> On 28/4/20 3:02, Daniil Lunev wrote:
> > > > >>>>> Missing EC in device hierarchy causes NULL pointer to be returned to the
> > > > >>>>> probe function which leads to NULL pointer dereference when trying to
> > > > >>>>> send a command to the EC. This can be the case if the device is missing
> > > > >>>>> or incorrectly configured in the firmware blob. Even if the situation
> > > > >>>>
> > > > >>>> There is any production device with a buggy firmware outside? Or this is just
> > > > >>>> for defensive programming while developing the firmware? Which device is
> > > > >>>> affected for this issue?
> > > > >>>>
> > > > >>>> Thanks,
> > > > >>>>  Enric
> > > > >>>>
> > > > >>>>> occures, the driver shall not cause a kernel panic as the condition is
> > > > >>>>> not critical for the system functions.
> > > > >>>>>
> > > > >>>>> Signed-off-by: Daniil Lunev <dlunev@chromium.org>
> > > > >>>>> ---
> > > > >>>>>
> > > > >>>>>  drivers/platform/chrome/cros_ec_typec.c | 5 +++++
> > > > >>>>>  1 file changed, 5 insertions(+)
> > > > >>>>>
> > > > >>>>> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > > > >>>>> index 874269c07073..30d99c930445 100644
> > > > >>>>> --- a/drivers/platform/chrome/cros_ec_typec.c
> > > > >>>>> +++ b/drivers/platform/chrome/cros_ec_typec.c
> > > > >>>>> @@ -301,6 +301,11 @@ 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, "Failed to get Cros EC data\n");
> > > > >>>>> +             return -EINVAL;
> > > > >>>>> +     }
> > > > >>>>> +
> > > > >>>>>       platform_set_drvdata(pdev, typec);
> > > > >>>>>
> > > > >>>>>       ret = cros_typec_get_cmd_version(typec);
> > > > >>>>>

Re: [PATCH] platform/chrome: cros_ec_typec: Handle NULL EC pointer during probe.

[Enric Balletbo i Serra]

Hi.

On 1/5/20 5:22, Daniil Lunev wrote:
> Hi Prashant,
> I do not think it is present. Thinking about it, I do not think it
> shall be an issue on any released device as it will have either a
> firmware which wouldn't even trigger the typec probe or the one after
> the hierarchy fix. Likely I just got a firmware which was somewhere in
> between those two (As I did some unrelated FW testing). So, yes,
> probably putting this upstream is not necessary, though IMO more
> sanity checks - especially on non-critical run-once paths - are always
> better than having a kernel panic lingering around the corner, not
> like I am insisting on pushing the patch though with all the info, up
> to Enric.

I'd prefer to not push the patch. If at some point this is starts of being
possible we will catch soon.

Thank you,
 Enric

> Cheers,
> Daniil
> 
> On Fri, May 1, 2020 at 10:56 AM Prashant Malani <pmalani@chromium.org> wrote:
>>
>> Hi Daniil,
>>
>> On Fri, May 01, 2020 at 10:15:18AM +1000, Daniil Lunev wrote:
>>> On the official revision of coreboot for hatch it doesn't even try to
>>> load Type C. However it gives some warning messages from
>>> cros-usbpd-notify-acpi about EC, So I wonder why the check of the same
>>> type is not appropriate in the typec driver?
>>
>> I think the difference is that GOOG0003 is already present on shipped /
>> official versions of coreboot (so not having that check can cause
>> existing release images/devices to crash), whereas for GOOG0014 that is / isn't the case.
>>
>> Is GOOG0014 present on the official release coreboot image for this
>> device? If so, what's its path (/sys/bus/acpi/devices/<HID>/path) ?
>>
>> Best regards,
>>
>> -Prashant
>>>
>>> ../chrome/cros_usbpd_notify.c
>>>
>>> /* Get the EC device pointer needed to talk to the EC. */
>>> ec_dev = dev_get_drvdata(dev->parent);
>>> if (!ec_dev) {
>>> /*
>>> * We continue even for older devices which don't have the
>>> * correct device heirarchy, namely, GOOG0003 is a child
>>> * of GOOG0004.
>>> */
>>> dev_warn(dev, "Couldn't get Chrome EC device pointer.\n");
>>> }
>>>
>>>
>>> # dmesg
>>> ...
>>> [    8.513351] cros-ec-spi spi-PRP0001:02: EC failed to respond in time
>>> [    8.722072] cros-ec-spi spi-PRP0001:02: EC failed to respond in time
>>> [    8.729271] cros-ec-spi spi-PRP0001:02: Cannot identify the EC: error -110
>>> [    8.736966] cros-ec-spi spi-PRP0001:02: cannot register EC,
>>> fallback to spidev
>>> [    8.767017] cros_ec_lpcs GOOG0004:00: Chrome EC device registered
>>> [    8.807537] cros-usbpd-notify-acpi GOOG0003:00: Couldn't get Chrome
>>> EC device pointer.
>>> ...
>>>
>>> On Fri, May 1, 2020 at 2:17 AM Prashant Malani <pmalani@chromium.org> wrote:
>>>>
>>>> Hi Enric,
>>>>
>>>> On Thu, Apr 30, 2020 at 8:26 AM Enric Balletbo i Serra
>>>> <enric.balletbo@collabora.com> wrote:
>>>>>
>>>>> Hi Prashant,
>>>>>
>>>>> On 30/4/20 2:43, Prashant Malani wrote:
>>>>>> On Wed, Apr 29, 2020 at 5:38 PM Daniil Lunev <dlunev@chromium.org> wrote:
>>>>>>>
>>>>>>> [to make it appear on the mailing list as I didn't realize I was in
>>>>>>> hypertext sending mode]
>>>>>>>
>>>>>>> On Thu, Apr 30, 2020 at 10:11 AM Daniil Lunev <dlunev@chromium.org> wrote:
>>>>>>>>
>>>>>>>> Hi Enric.
>>>>>>>> I encountered the issue on a Hatch device when trying running 5.4 kernel on that. After talking to Prashant it seems that any device with coreboot built before a certain point (a particular fix for device hierarchy in ACPI tables of Chrome devices which happened in mid-April) will not be able to correctly initialize the driver and will get a kernel panic trying to do so.
>>>>>>
>>>>>> A clarifying detail here: This should not be seen in any current
>>>>>> *production* device. No prod device firmware will carry the erroneous
>>>>>> ACPI device entry.
>>>>>>
>>>>>
>>>>> Thanks for the clarification. Then, I don't think we need to upstream this. This
>>>>> kind of "defensive-programming" it's not something that should matter to upstream.
>>>>
>>>> Actually, on second thought, I am not 100% sure about this:
>>>> Daniil, is the erroneous ACPI device on a *production* firmware for
>>>> this device (I'm not sure about the vintage of that device's BIOS)?
>>>>
>>>> My apologies for the confusion, Enric and Daniil; but would be good to
>>>> get clarification from Daniil.
>>>>
>>>> Best regards,
>>>>>
>>>>> Thanks,
>>>>>  Enric
>>>>>
>>>>>
>>>>>>>> Thanks,
>>>>>>>> Daniil
>>>>>>>>
>>>>>>>> On Thu, Apr 30, 2020 at 7:58 AM Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote:
>>>>>>>>>
>>>>>>>>> Hi Daniil,
>>>>>>>>>
>>>>>>>>> Thank you for the patch.
>>>>>>>>>
>>>>>>>>> On 28/4/20 3:02, Daniil Lunev wrote:
>>>>>>>>>> Missing EC in device hierarchy causes NULL pointer to be returned to the
>>>>>>>>>> probe function which leads to NULL pointer dereference when trying to
>>>>>>>>>> send a command to the EC. This can be the case if the device is missing
>>>>>>>>>> or incorrectly configured in the firmware blob. Even if the situation
>>>>>>>>>
>>>>>>>>> There is any production device with a buggy firmware outside? Or this is just
>>>>>>>>> for defensive programming while developing the firmware? Which device is
>>>>>>>>> affected for this issue?
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>  Enric
>>>>>>>>>
>>>>>>>>>> occures, the driver shall not cause a kernel panic as the condition is
>>>>>>>>>> not critical for the system functions.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Daniil Lunev <dlunev@chromium.org>
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>>  drivers/platform/chrome/cros_ec_typec.c | 5 +++++
>>>>>>>>>>  1 file changed, 5 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
>>>>>>>>>> index 874269c07073..30d99c930445 100644
>>>>>>>>>> --- a/drivers/platform/chrome/cros_ec_typec.c
>>>>>>>>>> +++ b/drivers/platform/chrome/cros_ec_typec.c
>>>>>>>>>> @@ -301,6 +301,11 @@ 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, "Failed to get Cros EC data\n");
>>>>>>>>>> +             return -EINVAL;
>>>>>>>>>> +     }
>>>>>>>>>> +
>>>>>>>>>>       platform_set_drvdata(pdev, typec);
>>>>>>>>>>
>>>>>>>>>>       ret = cros_typec_get_cmd_version(typec);
>>>>>>>>>>