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

From: Akihiko Odaki
Date: Wed Apr 06 2022 - 21:31:49 EST


On 2022/04/07 6:32, Guenter Roeck wrote:
On Wed, Apr 6, 2022 at 2:16 PM Prashant Malani <pmalani@xxxxxxxxxxxx> wrote:

Hi Akihiko,

Thanks for the patch.

On Apr 04 13:11, Akihiko Odaki wrote:
The EC driver may not be initialized when cros_typec_probe is called,
particulary when CONFIG_CROS_EC_CHARDEV=m.

Signed-off-by: Akihiko Odaki <akihiko.odaki@xxxxxxxxx>
---
drivers/platform/chrome/cros_ec_typec.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 4bd2752c0823..7cb2e35c4ded 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -1084,6 +1084,9 @@ static int cros_typec_probe(struct platform_device *pdev)
}

ec_dev = dev_get_drvdata(&typec->ec->ec->dev);
+ if (!ec_dev)
+ return -EPROBE_DEFER;
+

Just a quick check: are you still seeing this issue with 5.18-rc1, which
contains a null check for the parent EC device [1] ?

Yes, I'm seeing this problem with the check.



I may be missing something, but from the context I suspect this may
make the problem worse. My understanding was that the problem was seen
specifically if CONFIG_CROS_EC_CHARDEV=m. In that situation, it
appears that the parent EC device does _not yet_ exist. If the driver
returns -ENODEV in that situation, it will never be instantiated. The
big question for me is why the type C device is instantiated in the
first place if the parent EC device does not [yet] exist. I have not
been able to identify the code path where this happens. >
There is a similar problem with other EC child devices which are also
sometimes instantiated even though the parent EC device does not exist
(ie dev_get_drvdata(pdev->dev.parent) returns NULL). That can happen
if the parent EC device instantiation fails because of EC
communication errors (see https://b.corp.google.com/issues/228118385
for examples [sorry, internal only, I can't make it public]). I think
we need to track down why that happens and prevent child devices from
being instantiated in the first place instead of trying to work around
the problem in the child drivers.

Well, I think you have two misunderstanding.

1. The parent exists and dev_get_drvdata(pdev->dev.parent) returns non-NULL value. However, dev_get_drvdata(&typec->ec->ec->dev) returns NULL. (Yes, that is confusing.) I'm wondering dev_get_drvdata(pdev->dev.parent) returned NULL in the following crash log but it would be a problem distinct from what is handled with my patch:
https://lore.kernel.org/lkml/CABXOdTe9u_DW=NZM1-J120Gu1gibDy8SsgHP3bJwwLsE_iuLAQ@xxxxxxxxxxxxxx/

2. My patch returns -EPROBE_DEFER instead of -ENODEV and I confirmed it will eventually be instantiated.

Regards,
Akihiko Odaki


Guenter

Thanks,

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/platform/chrome?id=ffebd90532728086007038986900426544e3df4e