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

[Akihiko Odaki]

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