Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210713073946.102501-1-martin.kepplinger@puri.sm> <YO1VowaJ1Impm1U7@kuha.fi.intel.com>
In-Reply-To: <YO1VowaJ1Impm1U7@kuha.fi.intel.com>
From:   Saravana Kannan <saravanak@google.com>
Date:   Tue, 13 Jul 2021 19:16:46 -0700
Message-ID: <CAGETcx83TDPLwnm+63Wk_bo-mZfkA+XnBZC2GU=WQT60EgnozQ@mail.gmail.com>
Subject: Re: [PATCH] usb: typec: tipd: Don't block probing of consumer of
 "connector" nodes
To:     Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc:     Martin Kepplinger <martin.kepplinger@puri.sm>,
        "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        gregkh@linuxfoundation.org, grandmaster@al2klimov.de,
        kernel@puri.sm, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Jul 13, 2021 at 1:58 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> +Rafael, Saravana
>
> On Tue, Jul 13, 2021 at 09:39:46AM +0200, Martin Kepplinger wrote:
> > Similar as with tcpm this patch lets fw_devlink know not to wait on the
> > fwnode to be populated as a struct device.
> >
> > Without this patch, USB functionality can be broken on some previously
> > supported boards.
> >
> > Fixes: 28ec344bb891 ("usb: typec: tcpm: Don't block probing of consumers of "connector" nodes")
>
> That patch has gone under my radar.
>
> > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> > ---
> > hi,
> >
> > at least the boards based on imx8mq-librem5.dtsi suffer from this, possibly
> > more.
> >
> > thanks,
> >                            martin
> >
> >
> >  drivers/usb/typec/tipd/core.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> > index 938219bc1b4b..b31aaf57eb3b 100644
> > --- a/drivers/usb/typec/tipd/core.c
> > +++ b/drivers/usb/typec/tipd/core.c
> > @@ -629,6 +629,8 @@ static int tps6598x_probe(struct i2c_client *client)
> >       if (!fwnode)
> >               return -ENODEV;
> >
> > +     fw_devlink_purge_absent_suppliers(fwnode);

Martin,

Please include the comment from 28ec344bb891 that explains why the
function is called.

>
> Why do we have to care about this kind of stuff in the drivers? It
> sounds like something that only affects DT platforms, so why isn't
> this being fixed in the DT core code?
>
> I didn't have time to study this fw_deflink thing yet,

To give some context, fw_devlink (it's NOT limited to DT, but
currently only implemented for DT) parses the firmware and infers the
inter-device dependencies. And it creates device links between
supplier and consumer devices and that has a lot of benefits: No
initcall chicken between drivers, no/lot less deferred probing,
allowing more drivers to be modules, support for sync_state() ops in
drivers, allowing massively asynchronous probing, etc.

There's a lot of nuance and corner cases on how the dependencies are
inferred (without any help from drivers), but simplifying the
explanation to talk about the stuff relevant to this patch: fw_devlink
expects all nodes with "compatible" property to be probed by a driver.
That's what the "compatible" property is used by Linux for.

In this specific case, instead of probing the "connector" with a
driver, the USB framework/this driver directly parses the node based
on the name (why even have the "compatible" property then?) and uses
the info from the node and never probes this DT node. Since this
driver/DT node doesn't follow the norms, this specific driver needs to
inform fw_devlink that this is the case (that it's not going to probe
this DT node). That's what this one line call is doing.

> it is
> completely new to me and unfortunately I'm going on vacation now, but
> it looks like something that has been added to the driver core in a
> haste without enough thinking, and which is now causing problems. Not
> cool.

I'm sorry that you didn't get a chance to look into fw_devlink before,
but it certainly wasn't done in haste :) It took close to 2 years to
get this in and I talked about this in at least 2 of the LPCs. It also
had a trial run to catch issues from 5.12-rc1 (or something early like
that) till 5.12-rc7 and then reverted before 5.12 release. And then
finally enabled again in 5.13-rc1 and merged in 5.13.

-Saravana