Re: [PATCH v1 11/18] driver core: Redefine the meaning of fwnode_operations.add_links()

[Saravana Kannan]

On Mon, Nov 16, 2020 at 8:16 AM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
> >
> > Change the meaning of fwnode_operations.add_links() to just create
> > fwnode links by parsing the properties of a given fwnode.
> >
> > This patch doesn't actually make any code changes. To keeps things more
> > digestable, the actual functional changes come in later patches in this
> > series.
> >
> > Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>
> > ---
> >  include/linux/fwnode.h | 42 +++---------------------------------------
> >  1 file changed, 3 insertions(+), 39 deletions(-)
> >
> > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> > index afde643f37a2..ec02e1e939cc 100644
> > --- a/include/linux/fwnode.h
> > +++ b/include/linux/fwnode.h
> > @@ -78,44 +78,8 @@ struct fwnode_reference_args {
> >   *                            endpoint node.
> >   * @graph_get_port_parent: Return the parent node of a port node.
> >   * @graph_parse_endpoint: Parse endpoint for port and endpoint id.
> > - * @add_links: Called after the device corresponding to the fwnode is added
> > - *             using device_add(). The function is expected to create device
> > - *             links to all the suppliers of the device that are available at
> > - *             the time this function is called.  The function must NOT stop
> > - *             at the first failed device link if other unlinked supplier
> > - *             devices are present in the system.  This is necessary for the
> > - *             driver/bus sync_state() callbacks to work correctly.
> > - *
> > - *             For example, say Device-C depends on suppliers Device-S1 and
> > - *             Device-S2 and the dependency is listed in that order in the
> > - *             firmware.  Say, S1 gets populated from the firmware after
> > - *             late_initcall_sync().  Say S2 is populated and probed way
> > - *             before that in device_initcall(). When C is populated, if this
> > - *             add_links() function doesn't continue past a "failed linking to
> > - *             S1" and continue linking C to S2, then S2 will get a
> > - *             sync_state() callback before C is probed. This is because from
> > - *             the perspective of S2, C was never a consumer when its
> > - *             sync_state() evaluation is done. To avoid this, the add_links()
> > - *             function has to go through all available suppliers of the
> > - *             device (that corresponds to this fwnode) and link to them
> > - *             before returning.
> > - *
> > - *             If some suppliers are not yet available (indicated by an error
> > - *             return value), this function will be called again when other
> > - *             devices are added to allow creating device links to any newly
> > - *             available suppliers.
> > - *
> > - *             Return 0 if device links have been successfully created to all
> > - *             the known suppliers of this device or if the supplier
> > - *             information is not known.
> > - *
> > - *             Return -ENODEV if the suppliers needed for probing this device
> > - *             have not been registered yet (because device links can only be
> > - *             created to devices registered with the driver core).
> > - *
> > - *             Return -EAGAIN if some of the suppliers of this device have not
> > - *             been registered yet, but none of those suppliers are necessary
> > - *             for probing the device.
> > + * @add_links: Create fwnode links to all the suppliers of the fwnode. Return
> > + *             zero on success, a negative error code otherwise.
>
> I'd say something like "Create fwnode links to all nodes that
> represent devices supplying resources to the device represented by the
> current fwnode.  Return ..., or a negative ... on failure."

I don't have a strong opinion about this, but want to clarify that I'm
intentionally choosing not to say "device" because not all fwnodes
will have devices created for them. Do you still want me to make this
change?


>
> >   */
> >  struct fwnode_operations {
> >         struct fwnode_handle *(*get)(struct fwnode_handle *fwnode);
> > @@ -155,7 +119,7 @@ struct fwnode_operations {
> >         (*graph_get_port_parent)(struct fwnode_handle *fwnode);
> >         int (*graph_parse_endpoint)(const struct fwnode_handle *fwnode,
> >                                     struct fwnode_endpoint *endpoint);
> > -       int (*add_links)(const struct fwnode_handle *fwnode,
> > +       int (*add_links)(struct fwnode_handle *fwnode,
> >                          struct device *dev);
> >  };
> >
> > --
> > 2.29.1.341.ge80a0c044ae-goog
> >