Date:   Wed, 6 Nov 2019 00:07:40 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Saravana Kannan <saravanak@...gle.com>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Len Brown <lenb@...nel.org>,
        Android Kernel Team <kernel-team@...roid.com>,
        LKML <linux-kernel@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>
Subject: Re: [PATCH v1 3/5] driver core: Allow fwnode_operations.add_links to
 differentiate errors

On Tue, Nov 5, 2019 at 11:52 PM Saravana Kannan <saravanak@...gle.com> wrote:
>
> Hi Rafael,
>
> Thanks for the review.
>
> On Tue, Nov 5, 2019 at 2:43 PM Rafael J. Wysocki <rjw@...ysocki.net> wrote:
> >
> > On Monday, October 28, 2019 11:00:24 PM CET Saravana Kannan wrote:
> > > When add_links() still has suppliers that it needs to link to in the
> > > future, this patch allows it to differentiate between suppliers that are
> > > needed for probing vs suppliers that are needed for sync_state()
> > > correctness.
> >
> > I guess you mean that it will return different error codes in the different
> > cases.
>
> Yes.
>
> >
> > > Signed-off-by: Saravana Kannan <saravanak@...gle.com>
> > > ---
> > >  drivers/base/core.c    | 12 ++++++++----
> > >  include/linux/fwnode.h | 13 +++++++++----
> > >  2 files changed, 17 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index 48cd43a91ce6..e6d3e6d485da 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -2297,7 +2297,7 @@ int device_add(struct device *dev)
> > >       struct device *parent;
> > >       struct kobject *kobj;
> > >       struct class_interface *class_intf;
> > > -     int error = -EINVAL;
> > > +     int error = -EINVAL, fw_ret;
> > >       struct kobject *glue_dir = NULL;
> > >
> > >       dev = get_device(dev);
> > > @@ -2413,9 +2413,13 @@ int device_add(struct device *dev)
> > >        */
> > >       device_link_add_missing_supplier_links();
> > >
> > > -     if (fwnode_has_op(dev->fwnode, add_links)
> > > -         && fwnode_call_int_op(dev->fwnode, add_links, dev))
> > > -             device_link_wait_for_mandatory_supplier(dev, true);
> > > +     if (fwnode_has_op(dev->fwnode, add_links)) {
> >
> > fw_ret can be defined here and I'd just call it "ret".
>
> I thought that style of variable declaration is frowned up in the
> kernel coding style.

Well, I'm not aware of that. :-)

> >
> > > +             fw_ret = fwnode_call_int_op(dev->fwnode, add_links, dev);
> > > +             if (fw_ret == -ENODEV)
> > > +                     device_link_wait_for_mandatory_supplier(dev);
> > > +             else if (fw_ret)
> > > +                     device_link_wait_for_optional_supplier(dev);
> > > +     }
> > >
> > >       bus_probe_device(dev);
> > >       if (parent)
> > > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> > > index 25bb81f8ded8..a19134eae5a5 100644
> > > --- a/include/linux/fwnode.h
> > > +++ b/include/linux/fwnode.h
> > > @@ -96,10 +96,15 @@ struct fwnode_reference_args {
> > >   *           available suppliers.
> > >   *
> > >   *           Return 0 if device links have been successfully created to all
> > > - *           the suppliers of this device or if the supplier information is
> > > - *           not known. Return an error if and only if the supplier
> > > - *           information is known but some of the suppliers are not yet
> > > - *           available to create device links to.
> > > + *           the suppliers this device needs to create device links to or if
> > > + *           the supplier information is not known.
> >
> > "the known suppliers of this device or if the supplier information is not known."
>
> "suppliers it needs to create device links to" is a subset of known
> suppliers. There's no requirement that fw needs to create links to ALL
> known suppliers. Just a minor distinction.

That depends on what exactly you mean by "known suppliers".  The
suppliers that are not listed by the firmware are not known at this
point.

> > > + *
> > > + *           Return -ENODEV if and only if the suppliers needed for probing
> > > + *           the device are not yet available to create device links to.
> >
> > It would be more precise to say something like this:
> >
> > "Return -ENODEV if an attempt to create a device link to one of the device's
> > suppliers needed for probing it fails."
>
> "attempt to create a device link to one of the device's suppliers
> needed for probing it fails" to me means device_link_add() fails.
> But I'm trying to say that it should return an error if the struct
> device isn't even there yet.

OK, so it should be something like "if the supplier device has not
been registered yet".

My point is that "not yet available" is kind of ambiguous.

> > > + *
> > > + *           Return -EAGAIN if there are suppliers that need to be linked to
> > > + *           that are not yet available but none of those suppliers are
> > > + *           necessary for probing this device.
> >
> > "Return -EAGAIN if attempts to create device links to some of the device's
> > suppliers have failed, but those suppliers are not necessary for probing the
> > device."
>
> Same comment as before. The distinction I'm making here is that
> -EAGAIN is needed when the struct device itself isn't there.
>
> Btw, Greg already pulled these into driver-core-next. Let me know if
> you want me to send a delta patch to fix any of these comments.

Well, it's a Greg's call if he has taken the patches, but it also
depends on you (if you agree with the comments, it would be prudent to
send updates).

Powered by blists - more mailing lists