Re: [PATCH v2 01/11] driver core: fw_devlink: Don't purge child fwnode's consumer links

[Saravana Kannan <saravanak@google.com>]

On Fri, Jan 27, 2023 at 1:22 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Jan 26, 2023 at 04:11:28PM -0800, Saravana Kannan wrote:
> > When a device X is bound successfully to a driver, if it has a child
> > firmware node Y that doesn't have a struct device created by then, we
> > delete fwnode links where the child firmware node Y is the supplier. We
> > did this to avoid blocking the consumers of the child firmware node Y
> > from deferring probe indefinitely.
> >
> > While that a step in the right direction, it's better to make the
> > consumers of the child firmware node Y to be consumers of the device X
> > because device X is probably implementing whatever functionality is
> > represented by child firmware node Y. By doing this, we capture the
> > device dependencies more accurately and ensure better
> > probe/suspend/resume ordering.
>
> ...
>
> >  static unsigned int defer_sync_state_count = 1;
> >  static DEFINE_MUTEX(fwnode_link_lock);
> >  static bool fw_devlink_is_permissive(void);
> > +static void __fw_devlink_link_to_consumers(struct device *dev);
> >  static bool fw_devlink_drv_reg_done;
> >  static bool fw_devlink_best_effort;
>
> I'm wondering if may avoid adding more forward declarations...
>
> Perhaps it's a sign that devlink code should be split to its own
> module?

I've thought about that before, but I'm not there yet. Maybe once my
remaining refactors and TODOs are done, it'd be a good time to revisit
this question.

But I don't think it should be done for the reason of forward
declaration as we'd just end up moving these into base.h and we can do
that even today.

>
> ...
>
> > -int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup)
> > +static int __fwnode_link_add(struct fwnode_handle *con,
> > +                          struct fwnode_handle *sup)
>
> I believe we tolerate a bit longer lines, so you may still have it on a single
> line.

That'd make it >80 cols. I'm going to leave it as is.

>
> ...
>
> > +int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup)
> > +{
>
> > +     int ret = 0;
>
> Redundant assignment.

Thanks. Will fix in v3.

>
> > +     mutex_lock(&fwnode_link_lock);
> > +     ret = __fwnode_link_add(con, sup);
> > +     mutex_unlock(&fwnode_link_lock);
> >       return ret;
> >  }
>
> ...
>
> >       if (dev->fwnode && dev->fwnode->dev == dev) {
>
> You may have above something like
>
>
>         fwnode = dev_fwnode(dev);

I'll leave it as-is for now. I see dev->fwnode vs dev_fwnode() don't
always give the same results. I need to re-examine other places I use
dev->fwnode in fw_devlink code before I start using that function. But
in general it seems like a good idea. I'll add this to my TODOs.

>         if (fwnode && fwnode->dev == dev) {
>
> >               struct fwnode_handle *child;
> >               fwnode_links_purge_suppliers(dev->fwnode);
> > +             mutex_lock(&fwnode_link_lock);
> >               fwnode_for_each_available_child_node(dev->fwnode, child)
> > -                     fw_devlink_purge_absent_suppliers(child);
> > +                     __fw_devlink_pickup_dangling_consumers(child,
> > +                                                            dev->fwnode);
>
>                         __fw_devlink_pickup_dangling_consumers(child, fwnode);

I like the dev->fwnode->dev == dev check. It makes it super clear that
I'm checking "The device's fwnode points back to the device". If I
just use fwnode->dev == dev, then one will have to go back and read
what fwnode is set to, etc. Also, when reading all these function
calls it's easier to see that I'm working on the dev's fwnode (where
dev is the device that was just bound to a driver) instead of some
other fwnode.

So I find it more readable as is and the compiler would optimize it
anyway. If you feel strongly about this, I can change to use fwnode
instead of dev->fwnode.

Thanks,
Saravana