Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220404041101.6276-1-akihiko.odaki@gmail.com> <Yk4DGZfpYbK8dofL@chromium.org>
In-Reply-To: <Yk4DGZfpYbK8dofL@chromium.org>
From:   Guenter Roeck <groeck@google.com>
Date:   Wed, 6 Apr 2022 14:32:26 -0700
Message-ID: <CABXOdTcY3w56hc7kWsDLxKU-c6fCLYt_jigK13tKjjm9OHi2+w@mail.gmail.com>
Subject: Re: [PATCH] platform/chrome: cros_ec_typec: Check for EC driver
To:     Prashant Malani <pmalani@chromium.org>
Cc:     Akihiko Odaki <akihiko.odaki@gmail.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        chrome-platform@lists.linux.dev,
        Benson Leung <bleung@chromium.org>,
        Guenter Roeck <groeck@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Apr 6, 2022 at 2:16 PM Prashant Malani <pmalani@chromium.org> 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@gmail.com>
> > ---
> >  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] ?
>

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.

Guenter

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