Re: [PATCH] platform/chrome: cros_ec_typec: Handle NULL EC pointer during probe.
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 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//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 wrote: Hi Enric, On Thu, Apr 30, 2020 at 8:26 AM Enric Balletbo i Serra wrote: > > Hi Prashant, > > On 30/4/20 2:43, Prashant Malani wrote: >> On Wed, Apr 29, 2020 at 5:38 PM Daniil Lunev 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 >>> 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 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
Re: [PATCH] platform/chrome: cros_ec_typec: Handle NULL EC pointer during probe.
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 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//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 wrote: > > > > > > Hi Enric, > > > > > > On Thu, Apr 30, 2020 at 8:26 AM Enric Balletbo i Serra > > > wrote: > > > > > > > > Hi Prashant, > > > > > > > > On 30/4/20 2:43, Prashant Malani wrote: > > > > > On Wed, Apr 29, 2020 at 5:38 PM Daniil Lunev > > > > > 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 > > > > >> 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 > > > > >>> 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
Re: [PATCH] platform/chrome: cros_ec_typec: Handle NULL EC pointer during probe.
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//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 wrote: > > > > Hi Enric, > > > > On Thu, Apr 30, 2020 at 8:26 AM Enric Balletbo i Serra > > wrote: > > > > > > Hi Prashant, > > > > > > On 30/4/20 2:43, Prashant Malani wrote: > > > > On Wed, Apr 29, 2020 at 5:38 PM Daniil Lunev > > > > 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 > > > >> 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 > > > >>> 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 > > > > --- > > > > > > > > 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
Re: [PATCH] platform/chrome: cros_ec_typec: Handle NULL EC pointer during probe.
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 wrote: > > Hi Enric, > > On Thu, Apr 30, 2020 at 8:26 AM Enric Balletbo i Serra > wrote: > > > > Hi Prashant, > > > > On 30/4/20 2:43, Prashant Malani wrote: > > > On Wed, Apr 29, 2020 at 5:38 PM Daniil Lunev 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 > > >> 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 > > >>> 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 > > > --- > > > > > > 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.
Hi Enric, On Thu, Apr 30, 2020 at 8:26 AM Enric Balletbo i Serra wrote: > > Hi Prashant, > > On 30/4/20 2:43, Prashant Malani wrote: > > On Wed, Apr 29, 2020 at 5:38 PM Daniil Lunev 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 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 > >>> 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 > > --- > > > > 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.
Hi Prashant, On 30/4/20 2:43, Prashant Malani wrote: > On Wed, Apr 29, 2020 at 5:38 PM Daniil Lunev 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 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 >>> 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 > --- > > 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.
On Wed, Apr 29, 2020 at 5:38 PM Daniil Lunev 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 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 > > 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 > >> > --- > >> > > >> > 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.
[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 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 > 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 >> > --- >> > >> > 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.
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 > --- > > 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); >