[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