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

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

[D. Manresa]

Hello,

v5 fixes a reference counting bug that 2/2 had in v4, and carries
Fernando Rimoli's Tested-by on 1/2.

A correction before anything else: in v4 I wrote that the Surface Pro 7+
was leaving my hands and that the patches were compile-tested only. It
is still here for a few more days, so this version is tested on it.

2/2 as posted in v4 dropped the reference software_node_find_by_name()
takes, on the grounds that the node is never unregistered. That is the
wrong half of the story. software_node_notify() resolves a device's
software node through dev_fwnode(dev)->secondary, which the IPU does not
have yet when device_add() calls it, so it takes no reference;
software_node_notify_remove() does resolve it on the next device_del()
and calls swnode_put(). The reuse path has to hand its reference to that
put.

Measured on the Pro 7+ with a module that reads the kobject reference
count of the INT343E node directly:

  v4 2/2 as posted   4 -> 3 -> 2 -> 1, and the node is gone from
                     /sys/kernel/software_nodes on the fourth
                     remove/rescan cycle. Reproduced twice.
  v5 2/2             4 -> 3 on each PCI remove, back to 4 on each
                     bind, over 16 cycles, children intact.

The decrement happens at the PCI remove, not at the module unload, and
only the IPU node is affected: the sensors' own nodes (INT33BE-4,
INT347A-1, INT347E-5 here) hold steady, since those devices are never
deleted. A remove that follows a failed bind costs nothing, because
there is no secondary fwnode left for software_node_notify_remove() to
resolve.

The series itself, on a Surface Pro 7+ (IPU6 Tiger Lake, 8086:9a19 at
0000:00:05.0, OV5693 + OV8865 + OV7251) running 6.19 with the equivalent
change:

 - without the series, the PCI remove -> modprobe -r -> rescan ->
   modprobe sequence fails exactly as the 2/2 commit message describes,
   with -EEXIST, and the cameras stay dead until a reboot;
 - with it, four cycles complete and all three sensors stream after
   each one.

Two things that cost me time, in case they would cost a reviewer the
same:

 - modprobe -r intel_ipu6_isys intel_ipu6 leaves ipu_bridge resident at
   reference count 0. Without an explicit modprobe -r ipu_bridge the
   module image is never freed and neither failure reproduces.
 - the reuse path logs at dev_dbg(), so it is invisible without
   dyndbg=+p.

The asymmetry underneath this is in drivers/base, not here: a software
node that becomes a device's secondary fwnode after device_add() gets a
swnode_put() at device_del() that no swnode_get() ever matched. I am
accounting for it in the bridge because that is where this series is,
but if it should be balanced in software_node_notify() instead, say so
and I will send that.

Unrelated but worth flagging: Sergey Lebedev's "Add the Surface Pro 11
infrared illuminator" series touches the same two lines 1/2 does
(dev_properties[3] and the "lens-focus" name), so whichever of the two
lands second needs a trivial rebase.

Changes since v4:
 - 2/2: keep the reference from software_node_find_by_name() instead of
   dropping it, and say in the comment what consumes it;
 - 2/2: commit message notes the reference counting;
 - 1/2: Fernando Rimoli's Tested-by (Surface Pro 9);
 - tested on hardware, where v4 was compile-tested only.

Changes since v3:
 - 1/2 and 2/2: commit messages cut down;
 - 1/2: lens_focus moved into struct ipu_property_names and link_freqs
   into struct ipu_sensor;
 - 2/2: reuse-path comment shortened.

v4: https://lore.kernel.org/linux-media/20261010155626.2766298-1-dmanresa@gmail.com/
v3: https://lore.kernel.org/linux-media/20261008132614.2456716-1-dmanresa@gmail.com/

Thanks,
D. Manresa

D. Manresa (2):
  media: ipu-bridge: don't reference the module image from software
    nodes
  media: ipu-bridge: reuse the software nodes on rebind

 drivers/media/pci/intel/ipu-bridge.c | 34 +++++++++++++++++++++++++++++++---
 include/media/ipu-bridge.h           |  6 ++++++
 2 files changed, 37 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH v5 1/2] media: ipu-bridge: don't reference the module image from software nodes

[D. Manresa]

The software nodes the bridge registers are deliberately never
unregistered, so that a later rebind can reuse them. Nothing reachable
from them may therefore point into the module image, but two properties
still do: the "link-frequencies" values point at cfg->link_freqs in
ipu_supported_sensors[], and the "lens-focus" property name is a string
literal.

Both dangle once the module is unloaded. Re-probing sensor drivers then
read poisoned link frequencies from the surviving nodes and fail:

  ov5693: supported link freq 419200000 not found

Copy both into struct ipu_sensor, where the other property names and
values already live.

Assisted-by: LLM
Fixes: 803abec64ef9 ("media: ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver")
Fixes: 68b9bcc8a534 ("media: ipu3-cio2: Add support for instantiating i2c-clients for VCMs")
Tested-by: Fernando Rimoli <fernandorimoli11@gmail.com> # Surface Pro 9, IPU6 (8086:465d); boot and sensor rebind, not unload or rescan
Signed-off-by: D. Manresa <dmanresa@gmail.com>
---
 drivers/media/pci/intel/ipu-bridge.c | 12 +++++++++---
 include/media/ipu-bridge.h           |  6 ++++++
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 3739c4a..aea4699 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -290,6 +290,7 @@ static const struct ipu_property_names prop_names = {
 	.remote_endpoint = "remote-endpoint",
 	.link_frequencies = "link-frequencies",
 	.clock_noncontinuous = "clock-noncontinuous",
+	.lens_focus = "lens-focus",
 };
 
 static const char * const ipu_vcm_types[] = {
@@ -623,7 +624,8 @@ static void ipu_bridge_create_fwnode_properties(
 		sensor->vcm_ref[0] =
 			SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_VCM]);
 		sensor->dev_properties[3] =
-			PROPERTY_ENTRY_REF_ARRAY("lens-focus", sensor->vcm_ref);
+			PROPERTY_ENTRY_REF_ARRAY(names->lens_focus,
+						 sensor->vcm_ref);
 	}
 
 	sensor->ep_properties[IPU_BRIDGE_NEXT_PROPERTY(i, EP_BUS_TYPE)] =
@@ -636,11 +638,15 @@ static void ipu_bridge_create_fwnode_properties(
 		PROPERTY_ENTRY_REF_ARRAY(names->remote_endpoint,
 					 sensor->local_ref);
 
-	if (cfg->nr_link_freqs > 0)
+	if (cfg->nr_link_freqs > 0) {
+		memcpy(sensor->link_freqs, cfg->link_freqs,
+		       cfg->nr_link_freqs * sizeof(*sensor->link_freqs));
+
 		sensor->ep_properties[IPU_BRIDGE_NEXT_PROPERTY(i, EP_LINK_FREQUENCIES)] =
 			PROPERTY_ENTRY_U64_ARRAY_LEN(names->link_frequencies,
-						     cfg->link_freqs,
+						     sensor->link_freqs,
 						     cfg->nr_link_freqs);
+	}
 
 	if (cfg->flags & IPU_BR_FL_CSI2_CLK_NONCONTINUOUS)
 		sensor->ep_properties[IPU_BRIDGE_NEXT_PROPERTY(i, EP_CLOCK_NONCONTINUOUS)] =
diff --git a/include/media/ipu-bridge.h b/include/media/ipu-bridge.h
index 3ef94c2..7c89fb6 100644
--- a/include/media/ipu-bridge.h
+++ b/include/media/ipu-bridge.h
@@ -136,6 +136,7 @@ struct ipu_property_names {
 	char remote_endpoint[16];
 	char link_frequencies[17];
 	char clock_noncontinuous[20];
+	char lens_focus[11];
 };
 
 struct ipu_node_names {
@@ -178,6 +179,11 @@ struct ipu_sensor {
 	const char *vcm_type;
 
 	struct ipu_property_names prop_names;
+	/*
+	 * The registered software nodes outlive the module, so the property
+	 * values they point at must not live in it.
+	 */
+	u64 link_freqs[MAX_NUM_LINK_FREQS];
 	struct property_entry ep_properties[IPU_BRIDGE_EP_NUM_ENTRIES];
 	struct property_entry dev_properties[5];
 	struct property_entry ipu_properties[3];
-- 
2.43.0

[PATCH v5 2/2] media: ipu-bridge: reuse the software nodes on rebind

[D. Manresa]

The software nodes are deliberately never unregistered so that a rebind
can reuse them, but ipu_bridge_init() only takes that path when the IPU
kept its secondary fwnode link. Removing and re-scanning the IPU PCI
device clears that link, so the next probe tries to register the IPU HID
node again and fails:

  sysfs: cannot create duplicate filename '/kernel/software_nodes/INT343E'
  intel-ipu6 0000:00:05.0: Failed to register the IPU HID node
  intel-ipu6: probe of 0000:00:05.0 failed with error -17

after which the cameras are unusable until reboot.

Look the registered node up instead and restore the device's secondary
fwnode, keeping the reference the lookup took. software_node_notify()
resolves a device's software node through its secondary fwnode, which is
not set yet when device_add() calls it, so it takes no reference, while
the device_del() of the next remove does resolve it and calls
swnode_put(). Dropping the lookup reference as well would cost the node
one reference per remove/rescan cycle: measured on a Surface Pro 7+, it
was gone from /sys/kernel/software_nodes by the fourth.

Assisted-by: LLM
Fixes: 803abec64ef9 ("media: ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver")
Signed-off-by: D. Manresa <dmanresa@gmail.com>
---
 drivers/media/pci/intel/ipu-bridge.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index aea4699..45a304c 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -1098,6 +1098,7 @@ static DEFINE_MUTEX(ipu_bridge_mutex);
 int ipu_bridge_init(struct device *dev,
 		    ipu_parse_sensor_fwnode_t parse_sensor_fwnode)
 {
+	const struct software_node *ipu_node;
 	struct fwnode_handle *fwnode;
 	struct ipu_bridge *bridge;
 	unsigned int i;
@@ -1108,6 +1109,27 @@ int ipu_bridge_init(struct device *dev,
 	if (!ipu_bridge_check_fwnode_graph(dev_fwnode(dev)))
 		return 0;
 
+	/*
+	 * The nodes registered by a previous bind are still there, but the
+	 * PCI remove/rescan cycle dropped the IPU's secondary fwnode link to
+	 * them. Registering them again would fail with -EEXIST, so reuse
+	 * them and restore just that link.
+	 */
+	ipu_node = software_node_find_by_name(NULL, IPU_HID);
+	if (ipu_node) {
+		fwnode = software_node_fwnode(ipu_node);
+		/*
+		 * The node only becomes the device's secondary fwnode here,
+		 * after device_add() has run, so software_node_notify() took
+		 * no reference on it. Keep the one from the lookup for the
+		 * swnode_put() that software_node_notify_remove() will do on
+		 * the next device_del().
+		 */
+		set_secondary_fwnode(dev, fwnode);
+		dev_dbg(dev, "Reusing the previously registered software nodes\n");
+		return 0;
+	}
+
 	if (!ipu_bridge_ivsc_is_ready())
 		return dev_err_probe(dev, -EPROBE_DEFER,
 				     "waiting for IVSC to become ready\n");
-- 
2.43.0

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

[Sergey Lebedev]

Hi Dario,

You asked for a run with KASAN on IPU hardware. Here is one, with a
result that is partly about my own patch.

Setup: a Surface Pro 11 (Lunar Lake, IPU7 8086:645d, run by
intel-ipu6), media next d7e7bca7169 with my VD55G0 v1 series, generic
inline KASAN with KASAN_VMALLOC, which KASAN needs to see module memory
at all, and lockdep. KASAN reports only once per boot, so each variant
had a boot of its own. The sequence: load the IPU stack, capture, unload
intel_ipu6_isys, intel_ipu6 and ipu_bridge, then unbind and bind the
VD55G0 with the bridge gone.

  variant                                   bind after the unload
  my v1 series as posted                    froze, 2 of 2 boots
  + your v5                                 froze, 1 of 1
  + your v5 + my 4/4 fixed (st,leds name    clean
    and GPIOs in the bridge's structs)

The freezes were total. The persistent journal stops before the bind,
with no KASAN report and no Oops, and kdump did not fire even with
panic_on_oops, the soft and hard lockup panics and hung_task_panic
armed; only the power button brought the machine back. With the bridge
still loaded, the same rebind is clean. So on this machine the dangling
pointer that freezes it is my st,leds, which Fernando pointed out on
25/10. Every run without that fix froze at the first read, so I cannot
tell what link-frequencies alone would have done. KASAN reported nothing
even where it should have, so the evidence for v5 below is that what
froze before no longer does, not KASAN's silence.

With v5 and the fix, the run went on, all clean (no KASAN, lockdep or
Oops lines, debug_locks still 1), with a capture of 10 of 10 frames
after each step:

- the rebind above;
- a reload of the stack;
- four PCI remove/rescan cycles of the IPU, each logging "Reusing the
  previously registered software nodes", none hitting a duplicate
  INT343E node, and the IPU7 back each time without a cold boot;
- one remove -> unload -> rescan -> reload cycle, as in your cover
  letter.

Only the VD55G0 was bound on that kernel (the stock OV13858 driver fails
probe on this machine without a patch of mine), so that is the sensor
the run covers. The only WARNING was the known one at
ipu6-isys-queue.c:203, once per stream start, in every variant.

So both patches ran here and held. The failure they guard against is
not a quiet stale read: for an st,leds-style property, it is a machine
that has to be power-cycled. My v2 4/4 keeps st,leds in the bridge's
structs, as your 1/2 does for lens-focus and link-frequencies.

Tested-by: Sergey Lebedev <lsa.uz@pm.me> # Surface Pro 11, IPU7, KASAN

Sergey

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

[D. Manresa]

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

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

[Bartosz Golaszewski]

On Wed, 28 Oct 2026 15:41:45 +0100, "D. Manresa" <dmanresa@gmail.com> said:
> 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.

Yes, I've sent a series[1]. So far no reviews but please leave your Tested-by
under this thread if you could.

And yes: with the driver core change merged, the code in media should not be
changed as it will lead to a reference leak instead.

>
> Assisted-by: LLM
>

Does this mean you had an agent write this email? This would explain why it's
way too long for what it needs to say. :( I have nothing against using LLMs
for writing code and reviews but would appreciate it if we could keep human
interaction, well... human.

Thanks,
Bartosz

[1] https://lore.kernel.org/all/20261023-swnode-dev-notify-balance-v2-0-948d920a79f6@oss.qualcomm.com/