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@xxxxx> 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@xxxxx>
> ---
> 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
