Message-ID: <WfZQTVQwRumG8L5YEf32cJoQCFo_5pAh7QH6Giq1CGI8J-PTcwXV-aWtWQqQCdkI2ZVQbbCVdVXxLhorucDAeKDlJoXW0A2Xfm0-cVvdhSo=@pm.me>
Date: Sun, 25 Feb 2024 20:37:42 +0000
From: Michael Pratt <mcpratt@...me>
To: Saravana Kannan <saravanak@...gle.com>
Cc: devicetree@...r.kernel.org, gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org, linux-mtd@...ts.infradead.org, abel.vesa@...aro.org, alexander.stein@...tq-group.com, andriy.shevchenko@...ux.intel.com, bigunclemax@...il.com, brgl@...ev.pl, colin.foster@...advantage.com, djrscally@...il.com, dmitry.baryshkov@...aro.org, festevam@...il.com, fido_max@...ox.ru, frowand.list@...il.com, geert@...ux-m68k.org, heikki.krogerus@...ux.intel.com, kernel@...gutronix.de, linus.walleij@...aro.org, linux@...ck-us.net, luca.weiss@...rphone.com, magnus.damm@...il.com, martin.kepplinger@...i.sm, miquel.raynal@...tlin.com, rafal@...ecki.pl, ansuelsmth@...il.com, richard@....at, sakari.ailus@...ux.intel.com, sudeep.holla@....com, tglx@...utronix.de, tony@...mide.com, vigneshr@...com, dianders@...omium.org, jpb@...nel.org, rafael@...nel.org, Android Kernel Team <kernel-team@...roid.com>
Subject: Re: [PATCH v1 2/4] driver core: fw_devlink: Link to supplier ancestor if no device

Hi Saravana,

Thanks for taking a look at the patch series.
I'm going to write a longer reply to the rest of what you said later.

On Monday, February 5th, 2024 at 14:25, Saravana Kannan <saravanak@...gle.com> wrote:

> 
> 
> On Mon, Jan 22, 2024 at 5:46 PM Michael Pratt mcpratt@...me wrote:
> 
> > 
> > Signed-off-by: Michael Pratt mcpratt@...me
> 
> 
> NACK for a bunch of reasons.
> 
> 1. This logic should not be in fwnode_link_add(). That's supposed to
> do the exact linking that the firmware meant. This logic should be
> where the fwnode links are converted to device links or deferred
> probing decision is done.

Yeah, I can definitely make this change without any problems.

> 
> 2. If we handle one parent above correctly, it should be easy to
> handle multiple parents above too. So why not fix it where we handle
> multiple parents above?

I can do this too, but since going one parent up would have to occur
multiple times, it would still end up being a recursive search for the flag.
Without finding the right fwnode right away, the new fixup function here
would have to recursively call itself 3 or 4 times instead of once or twice,
and each call would result in going through the entire loop of 
fw_devlink_link_device() for the consumer which would overall take a little more time.


> 
> Btw, I'm happy to fix this or help you fix this once I understand the
> issue. Just wanted to give a quick NACK to avoid people spending
> cycles on this patch in its current state.


I totally understand. I am still new to kernel things, so I imagine that
whenever I think that I'm finished with something,
that I'm actually just halfway done with it, even though it took forever
to get to this point, haha.


> 
> -Saravana
> 
> > ---
> > drivers/base/core.c | 49 ++++++++++++++++++++++++++++++++++++------
> > include/linux/fwnode.h | 4 ++++
> > 2 files changed, 47 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index c05a5f6b0641..7f2652cf5edc 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -92,13 +92,45 @@ static int __fwnode_link_add(struct fwnode_handle *con,
> > return 0;
> > }
> > 
> > +static int __fwnode_link_add_parents(struct fwnode_handle *con,
> > + struct fwnode_handle *sup,
> > + u8 flags, bool loop)
> > +{
> > + int ret = -EINVAL;
> > + struct fwnode_handle parent;
> > +
> > + fwnode_for_each_parent_node(sup, parent) {
> > + / Ignore the root node */
> > + if (fwnode_count_parents(parent) &&
> > + fwnode_device_is_available(parent) &&
> > + !(parent->flags & FWNODE_FLAG_NOT_DEVICE) &&
> > + !(parent->flags & FWNODE_FLAG_PARENT_IS_DEV)) {
> > + ret = __fwnode_link_add(con, parent, flags);
> > + }
> > +
> > + if (!loop && !ret) {
> > + fwnode_handle_put(parent);
> > + return 0;
> > + }
> > + }
> > +
> > + return ret;
> > +}
> > +
> > int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup)
> > {
> > int ret;
> > 
> > mutex_lock(&fwnode_link_lock);
> > - ret = __fwnode_link_add(con, sup, 0);
> > +
> > + if ((sup->flags & FWNODE_FLAG_NOT_DEVICE) &&
> > + (sup->flags & FWNODE_FLAG_PARENT_IS_DEV))
> > + ret = __fwnode_link_add_parents(con, sup, 0, false);
> > + else
> > + ret = __fwnode_link_add(con, sup, 0);
> > +
> > mutex_unlock(&fwnode_link_lock);
> > +
> > return ret;
> > }
> > 
> > @@ -218,7 +250,8 @@ static void __fwnode_links_move_consumers(struct fwnode_handle *from,
> > * MANAGED device links to this device, so leave @fwnode and its descendant's
> > * fwnode links alone.
> > *
> > - * Otherwise, move its consumers to the new supplier @new_sup.
> > + * Otherwise, flag descendants of @fwnode as having a parent fwnode for a device
> > + * that has probed and move bad fwnode consumer links from @fwnode to @new_sup.
> > */
> > static void __fw_devlink_pickup_dangling_consumers(struct fwnode_handle *fwnode,
> > struct fwnode_handle *new_sup)
> > @@ -228,8 +261,11 @@ static void __fw_devlink_pickup_dangling_consumers(struct fwnode_handle *fwnode,
> > if (fwnode->dev && fwnode->dev->driver)
> > return;
> > 
> > - fwnode->flags |= FWNODE_FLAG_NOT_DEVICE;
> > - __fwnode_links_move_consumers(fwnode, new_sup);
> > + if (fwnode->flags & FWNODE_FLAG_NOT_DEVICE)
> > + __fwnode_links_move_consumers(fwnode, new_sup);
> > +
> > + fwnode->flags |= FWNODE_FLAG_PARENT_IS_DEV;
> > + new_sup->flags &= ~(FWNODE_FLAG_PARENT_IS_DEV);
> > 
> > fwnode_for_each_available_child_node(fwnode, child)
> > __fw_devlink_pickup_dangling_consumers(child, new_sup);
> > @@ -2071,8 +2107,9 @@ static int fw_devlink_create_devlink(struct device *con,
> > device_links_write_unlock();
> > }
> > 
> > - if (sup_handle->flags & FWNODE_FLAG_NOT_DEVICE)
> > - sup_dev = fwnode_get_next_parent_dev(sup_handle);
> > + if ((sup_handle->flags & FWNODE_FLAG_NOT_DEVICE) &&
> > + !(sup_handle->flags & FWNODE_FLAG_PARENT_IS_DEV))
> > + return -EINVAL;
> > else
> > sup_dev = get_dev_from_fwnode(sup_handle);
> > 
> > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> > index 2a72f55d26eb..3ed8ba503062 100644
> > --- a/include/linux/fwnode.h
> > +++ b/include/linux/fwnode.h
> > @@ -30,6 +30,9 @@ struct device;
> > * BEST_EFFORT: The fwnode/device needs to probe early and might be missing some
> > * suppliers. Only enforce ordering with suppliers that have
> > * drivers.
> > + * PARENT_IS_DEV: The fwnode is a child of a fwnode that is or will be populated as a
> > + * struct device, which is more suitable for device links
> > + * than the fwnode child which may never have a struct device.
> > */
> > #define FWNODE_FLAG_LINKS_ADDED BIT(0)
> > #define FWNODE_FLAG_NOT_DEVICE BIT(1)
> > @@ -37,6 +40,7 @@ struct device;
> > #define FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD BIT(3)
> > #define FWNODE_FLAG_BEST_EFFORT BIT(4)
> > #define FWNODE_FLAG_VISITED BIT(5)
> > +#define FWNODE_FLAG_PARENT_IS_DEV BIT(6)
> > 
> > struct fwnode_handle {
> > struct fwnode_handle *secondary;
> > --
> > 2.30.2

--
MCP

Powered by blists - more mailing lists