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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi D.,

Thanks for the update.

On Thu, Oct 08, 2026 at 03:26:13PM +0200, D. Manresa wrote:
> The software nodes registered by ipu_bridge_init() are deliberately
> never unregistered: sensor drivers and the fwnode graph keep references
> to them, so they are left registered when the ipu-bridge module is
> unloaded and a later rebind is intended to reuse the already registered
> nodes.
> 
> For that to work, nothing reachable from the registered nodes may point
> into the ipu-bridge module image. Most of the data already lives in the
> dedicated, never freed, struct ipu_bridge allocation: the property name
> strings in struct ipu_property_names are character arrays copied into
> the per-sensor struct, the node name strings are likewise character
> arrays inside the struct, and the data-lanes array is a struct
> ipu_bridge member precisely so that "it survives if the module is
> unloaded along with the rest of the struct". Commit a20c843c9bc4
> ("media: ipu-bridge: Keep the clock-noncontinuous property name out of
> rodata") moved the "clock-noncontinuous" property name there as well.
> 
> Two references into the module image remain, though:
> 
> 1. The values of the "link-frequencies" endpoint property point at
>    cfg->link_freqs inside the const ipu_supported_sensors[] table in
>    module rodata.
> 
> 2. The name of the "lens-focus" device property is a string literal in
>    module rodata.
> 
> Both dangle as soon as the module is unloaded, while the properties
> that carry them stay registered and readable. In practice, after
> unloading and reloading the IPU modules on a Surface Pro 7+ (IPU6,
> ov8865 + ov5693 + ov7251), re-probing sensor drivers read poisoned
> link-frequencies from the surviving nodes and fail to probe:
> 
>   ov8865: failed to find 360000000 clk rate in endpoint link-frequencies
>   ov5693: supported link freq 419200000 not found
> 
> where 419200000/360000000 are exactly the values the bridge had
> originally published for those sensors, i.e. the properties no longer
> return their original contents. Depending on what happens to the freed
> module mapping, reading the properties can also fault. Similarly, a VCM
> lookup through the "lens-focus" reference can no longer match (or
> faults) once the property's name pointer is dangling.
> 
> Copy the link frequencies and the "lens-focus" property name into
> struct ipu_bridge, next to the data-lanes array kept there for the same
> reason, and make the registered properties point at those copies, so
> the nodes survive module unload intact. These were the only remaining
> references from the registered nodes into the module image (the
> sensor->vcm_type pointer into ipu_vcm_types[] is only dereferenced
> during ipu_bridge_init() itself and is not reachable from the nodes).

This commit message is exceedingly long considering what the patch does.
Please remove non-essential information in it.

> 
> 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 | 14 +++++++++++---
>  include/media/ipu-bridge.h           |  9 +++++++++
>  2 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index 3739c4a..b9779c3 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -623,7 +623,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(bridge->lens_focus,
> +						 sensor->vcm_ref);
>  	}
>  
>  	sensor->ep_properties[IPU_BRIDGE_NEXT_PROPERTY(i, EP_BUS_TYPE)] =
> @@ -636,11 +637,17 @@ 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) {
> +		u64 *link_freqs = bridge->link_freqs[sensor - bridge->sensors];
> +
> +		memcpy(link_freqs, cfg->link_freqs,
> +		       cfg->nr_link_freqs * sizeof(*link_freqs));
> +
>  		sensor->ep_properties[IPU_BRIDGE_NEXT_PROPERTY(i, EP_LINK_FREQUENCIES)] =
>  			PROPERTY_ENTRY_U64_ARRAY_LEN(names->link_frequencies,
> -						     cfg->link_freqs,
> +						     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)] =
> @@ -1112,6 +1119,7 @@ int ipu_bridge_init(struct device *dev,
>  
>  	strscpy(bridge->ipu_node_name, IPU_HID,
>  		sizeof(bridge->ipu_node_name));
> +	strscpy(bridge->lens_focus, "lens-focus", sizeof(bridge->lens_focus));
>  	bridge->ipu_hid_node.name = bridge->ipu_node_name;
>  	bridge->dev = dev;
>  	bridge->pci_id = dev_is_pci(dev) ? to_pci_dev(dev)->device : 0;
> diff --git a/include/media/ipu-bridge.h b/include/media/ipu-bridge.h
> index 3ef94c2..a49f37f 100644
> --- a/include/media/ipu-bridge.h
> +++ b/include/media/ipu-bridge.h
> @@ -203,6 +203,15 @@ struct ipu_bridge {
>  	char ipu_node_name[ACPI_ID_LEN];
>  	struct software_node ipu_hid_node;
>  	u32 data_lanes[4];
> +	/*
> +	 * The software nodes registered by the bridge are deliberately never
> +	 * unregistered (see ipu_bridge_init()), so every string and array
> +	 * they reference must live in this never freed struct rather than in
> +	 * the module image, so that the nodes stay intact if the module is
> +	 * unloaded.
> +	 */
> +	char lens_focus[sizeof("lens-focus")];
> +	u64 link_freqs[IPU_MAX_PORTS][MAX_NUM_LINK_FREQS];

Can you move this to struct ipu_sensor?

>  	unsigned int n_sensors;
>  	struct ipu_sensor sensors[IPU_MAX_PORTS];
>  };

-- 
Regards,

Sakari Ailus