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

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

[D. Manresa]

Hello,

v4 addresses Sakari's review of v3. Both commit messages are much
shorter, the in-code comments are trimmed, and the data moved in 1/2
now lives in struct ipu_sensor as asked:

- the "lens-focus" name goes into struct ipu_property_names, next to
  clock_noncontinuous, so it is copied per sensor like every other
  property name and ipu_bridge_init() needs no strscpy() for it;
- link_freqs becomes a per-sensor array, which also drops the
  sensor - bridge->sensors indexing v3 needed.

No functional change against v3.

Testing: unchanged from v1/v2, on a Surface Pro 7+ (IPU6, OV5693 +
OV8865 + OV7251) running the equivalent change on 6.19 - the PCI remove
-> module unload -> rescan -> modprobe sequence, fatal today (-EEXIST),
completes cleanly with the series, with all three cameras streaming
after each rebind. The patches as posted are compile-tested only; that
machine is not on a media/next kernel and is leaving my hands, so I
cannot promise a fresh run on it.

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, and the fwnode_handle_put()
  comment now says why the reference is dropped rather than what the
  call does.

Changes since v2:
- rebased onto media/next (1/2 rewritten for the new property
  indexing).

Changes since v1:
- shortened the 2/2 commit message;
- dev_info() -> dev_dbg() on the reuse path;
- Assisted-by: tag per Documentation/process/coding-assistants.rst.

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 | 29 +++++++++++++++++++++++++---
 include/media/ipu-bridge.h           |  6 ++++++
 2 files changed, 32 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH v4 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")
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 v4 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.

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 | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index aea4699..a77ea60 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,22 @@ 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);
+		set_secondary_fwnode(dev, fwnode);
+		/* The node is never unregistered, no reference is needed. */
+		fwnode_handle_put(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 v4 1/2] media: ipu-bridge: don't reference the module image from software nodes

[Fernando Rimoli]

Hi,

You said the Surface Pro 7+ is leaving your hands, so here is a run on
other hardware: a Surface Pro 9, IPU6 Alder Lake-P (8086:465d), with
three sensors on the bridge (OVTI5693, OVTID858, SMO55F0).

To be clear, both of the failures you fixed need the IPU driver
unloaded, by module unload or by PCI remove and rescan, and I cannot do
that on this machine. A warm reload of intel-ipu6 leaves its CSE
firmware refusing authentication, and the cameras stay dead until a cold
boot. So this is a no-regression run of the normal paths, not a
reproduction of the bug.

What I ran: your v4 1/2 and 2/2 on the 6.19-based tree this machine
boots, on top of a20c843c9bc4 ("media: ipu-bridge: Keep the
clock-noncontinuous property name out of rodata"), which your 1/2 builds
on. Both applied with offsets only. 1/2 changes struct ipu_sensor, so I
rebuilt intel-ipu6 against the new header as well. intel-ipu6-isys was
not rebuilt, and all 24 symbols it imports still match.

After a cold boot:

  intel-ipu6 0000:00:05.0: Found supported sensor OVTI5693:00
  intel-ipu6 0000:00:05.0: Found supported sensor OVTID858:00
  intel-ipu6 0000:00:05.0: Found supported sensor SMO55F0:00
  intel-ipu6 0000:00:05.0: Connected 3 cameras

The two sensors whose drivers parse link-frequencies from the endpoint
get it from the per-sensor copy:

  ov5693   419200000
  vd55g    396000000

(ov13858 never parses the property; it reports its own menu, 540000000.)
Unbinding and rebinding those two sensor drivers, three times in all,
parses the same nodes again, and the values come back intact each time.
The front sensor needs clock-noncontinuous on this machine (without it,
it delivers no frames), and it streams at 28.63 fps, so the per-sensor
property names reach it too. Rear 29.94 fps, unchanged. No warnings in
the log from the bridge.

Tested-by: Fernando Rimoli <fernandorimoli11@gmail.com> # Surface Pro 9, IPU6 (8086:465d); boot and sensor rebind, not unload or rescan

I have not put a tag on 2/2. Its new path only runs when the IPU node is
already registered at init, which never happens on a normal boot, so
nothing I could run here reaches it.

Fernando