| Message ID | 20240123014517.5787-2-mcpratt@pm.me |
|---|---|
| State | New |
| Headers | show |
| Series | fw_devlink: generically handle bad links to child fwnodes | expand |
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
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) return; fwnode->flags |= FWNODE_FLAG_NOT_DEVICE;
The function __fw_devlink_pickup_dangling_consumers() intends to ignore suppliers that are already capable of probing, but uses whether or not a bus struct is defined in the device struct. 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. 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. "Hey! Who's driving the bus?" Signed-off-by: Michael Pratt <mcpratt@pm.me> --- drivers/base/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)