Re: [PATCH v1 1/4] driver core: fw_devlink: Use driver to determine probe ability

[Saravana Kannan <saravanak@google.com>]

Hi Michael,

Thanks for reporting this and being willing to work on a fix.

On Mon, Jan 22, 2024 at 5:46 PM Michael Pratt <mcpratt@pm.me> wrote:
>
> The function __fw_devlink_pickup_dangling_consumers()
> intends to ignore suppliers that are already capable of probing,

fw_devlink isn't trying to figure out if a fwnode is "already capable"
of probing. It's trying to figure out if a fwnode will NEVER probe. If
it's just looking at "right now" or "already capable", it becomes
kinda useless.

> but uses whether or not a bus struct is defined in the device struct.

Because if you don't need a class of devices to probe, you add them to
a "class" not a "bus".

> There are some cases where a firmware child node
> can be address translatable but not able to probe
> (e.g. the use of of_platform_populate() for MTD partitions),
> so whether or not a driver is present is a more accurate way
> to guess whether a fwnode represents a real probing device here.

No, checking for the driver is not a "more accurate way" for the
reasons mentioned above.

> This also serves as a preparation step for further changes
> to fw_devlink including making the contents of this function
> less strict in order to compensate for more cases being passed into
> the rest of the function because the return case is now more strict.

This change itself is a definite Nack, but I'll look at your other
patches to see what you are trying to do.

> "Hey! Who's driving the bus?"

The driver isn't here yet. He'll be here in a while. But at least this
is a mode of transportation and not a football stadium :)

See more below.

> Signed-off-by: Michael Pratt <mcpratt@pm.me>
> ---
>  drivers/base/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 14d46af40f9a..c05a5f6b0641 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -214,7 +214,7 @@ static void __fwnode_links_move_consumers(struct fwnode_handle *from,
>   * @new_sup: fwnode of new supplier
>   *
>   * If the @fwnode has a corresponding struct device and the device supports
> - * probing (that is, added to a bus), then we want to let fw_devlink create
> + * probing (that is, bound to a driver), then we want to let fw_devlink create
>   * MANAGED device links to this device, so leave @fwnode and its descendant's
>   * fwnode links alone.
>   *
> @@ -225,7 +225,7 @@ static void __fw_devlink_pickup_dangling_consumers(struct fwnode_handle *fwnode,
>  {
>         struct fwnode_handle *child;
>
> -       if (fwnode->dev && fwnode->dev->bus)
> +       if (fwnode->dev && fwnode->dev->driver)

This will completely break fw_devlink when modules are enabled. Which
is where fw_devlink is also very much needed. And if modules are
loaded using udev events, this is guaranteed to break for those cases.
Also, the driver gets set AFTER a device is probed. Not before. So, I
think you are just deleting/incorrectly moving a whole bunch of device
links that would have been created.

A first level sanity test for any fw_devlink change is to take a
sufficiently complicated board/system and then compare the output of
this command before and after your changes:
ls -1 /sys/class/devlink

The diff you see should be exactly what you expect/want to happen. If
there are other unexpected diffs it's generally a bug. I've caught so
many bugs in my changes (before I send them) this way.

Also, if a device is never supposed to probe, it should not be added
to a bus anyway. That's what a "class" is for. It's for a class of
devices. Adding a device to a bus and then never probing it is such a
waste. And this device (at least for nvmem-cells) is never even
referenced -- which is an even bigger waste of memory.

I'd really prefer if someone with nvmem cells experience (hint hint
Michael hint hint :) ) can clean up the framework to not create
devices unnecessarily or at least make it a device that's added to a
class instead of a bus.

-Saravana