Re: [PATCH v5 0/2] media: ipu-bridge: survive module unload and reuse the software nodes on rebind

["D. Manresa" <dmanresa@gmail.com>]

Hello,

Eduardo, Andy, Bart: an apology, and a caveat on this series that comes
out of your thread.

The cover letter above says that the asymmetry underneath 2/2 is in
drivers/base rather than in the bridge, and ends by offering to send a
fix for software_node_notify() "if it should be balanced there instead".
I wrote that without noticing that Eduardo had already reported exactly
this on 11 October, with a reproducer and a splat:

  https://lore.kernel.org/linux-media/20261011131518.48838-1-toorandom@gmail.com/

that Andy had pointed it at the core two days later, and that Bart had
answered on the 15th that it looks like a bug in swnode.c and that he
would send a fix proposal. That was on the list I was posting to, eleven
days before I posted. I should have cited it instead of presenting the
observation as mine, and the offer at the end of my cover was redundant
with Bart's. Sorry for the noise.

The analysis in that thread still holds on current mainline, for what it
is worth: the code it quotes predates c393ef0741d4 ("software node:
provide wrappers around kobject_get/put()"), so the calls now read
swnode_get()/swnode_put(), but both are plain passthroughs to
kobject_get()/kobject_put() and nothing else moved. The put in
software_node_notify_remove() is still outside the "if (swnode->managed)"
block, and dev_to_swnode() still resolves dev_fwnode(dev) and then its
->secondary, which is what makes the two sides unbalanced: at device_add()
time a device that has no secondary fwnode yet gives NULL and
software_node_notify() returns before its swnode_get(), while at
device_del() the secondary is set, dev_to_swnode() resolves, and the
swnode_put() runs.

Now the caveat, which is the part that actually matters for this series.

2/2 depends on that imbalance on purpose. The reuse path keeps the
reference that software_node_find_by_name() returns, instead of dropping
it, precisely so that there is something for the unmatched swnode_put()
to consume on the next device_del(). That is what makes the count flat
across a PCI remove/rescan cycle (4 -> 3 on remove, back to 4 on bind,
over 16 cycles) rather than losing one per cycle.

So if the core put becomes balanced, 2/2 as posted leaks one reference
per remove/rescan cycle and the IPU node is never freed. Eduardo's patch
is in the same position for the same reason: its fwnode_handle_get()
before set_secondary_fwnode() has no counterpart once the core stops
putting what it never got. Both are compensations for one core bug, at
two different sites, and whichever core fix lands should take both of
them with it.

They do not collide textually: Eduardo's hunk is in
ipu_bridge_instantiate_ivsc(), on the mei-csi device, and mine is in
ipu_bridge_init(), on the IPU PCI device.

Sakari, Hans: given the above, I would rather you did not apply 2/2 as
it stands. 1/2 is independent of all this and can go on its own. I am
happy to rework 2/2 on top of whatever lands in swnode.c, or to drop it
if the core fix makes it unnecessary.

Bart, if you have a proposal in a tree somewhere I can run it on the
Surface Pro 7+ that produced the numbers above: three sensors on IPU6
across PCI remove/rescan cycles, with a module that reads the node's
kobject reference count directly. The machine is being given away in a
few days, so that offer has a short fuse; after that I have no IPU6
hardware and someone else would have to.

Assisted-by: LLM