Date:   Tue, 5 Nov 2019 16:00:18 -0800
From:   Saravana Kannan <saravanak@...gle.com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.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 3:07 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
>
> 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. :-)

I've definitely seen such comments before. So I'll leave fw_ret as is.
If you and Greg both want to change it to the way you mentioned, I'm
happy to do it.

> > >
> > > > +             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.

Ok, I'll rephrase my comment:
"suppliers it needs to create device links to" is a subset of listed
suppliers. There's no requirement that fw needs to create links to ALL
listed suppliers. For example, I can't think of any reason for
sync_state() to be necessary for an interrupt controller driver. So,
fw doesn't need to create device links from consumer to interrupt
supplier. So I'm being more explicit and saying "the suppliers this
device needs to create device links to" instead of "the listed
suppliers of this device".

Long story short, I wrote the comment this way intentionally and
changing it to what you suggest makes it inaccurate IMHO. But I'm open
to other wording suggestions to improve the clarity of this comment.

>
> > > > + *
> > > > + *           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.

Agree, the latest suggestion sounds better.

> > > > + *
> > > > + *           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).

I don't mind sending updates at all. Just trying to make sure I follow
the maintainers' preference in case they don't want trivial (because
my current ones aren't terrible :)) comment update patches.

Once we agree on all the discussion here, I can send an update patch.

Thanks again for your review.

-Saravana

Powered by blists - more mailing lists