[net-next PATCH v5 0/3] Grab IPA IMEM slice through DT

[net-next PATCH v5 0/3] Grab IPA IMEM slice through DT

[Konrad Dybcio]

This adds the necessary driver change to migrate over from
hardcoded-per-IPA-version-but-varying-per-implementation numbers, while
unfortunately keeping them in there for backwards compatibility.

The DT changes will be submitted in a separate series, this one is OK
to merge independently.

Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
Changes in v5:
- Rebase on next-20260223 (NOP)
- Resend after the merge window formally closed..
- Link to v4: https://lore.kernel.org/r/20260219-topic-ipa_imem-v4-0-189d91dbee84@oss.qualcomm.com

Changes in v4:
- Fix a memmap() leak
- Adjust comment style, take Alex's suggestion about very explicit
  DEPRECATION notices
- Pick up tags
- Link to v3: https://lore.kernel.org/r/20260217-topic-ipa_imem-v3-0-d6d8ed1dfb67@oss.qualcomm.com

Changes in v3:
- Pick up tags, rebase (effectively a NOP)
- Add actual binding constraints for modem-tables, drop Alex's r-b
- Better describe the purpose of this region, as much as I can anyway
- Link to v2: https://lore.kernel.org/r/20250527-topic-ipa_imem-v2-0-6d1aad91b841@oss.qualcomm.com

Changes in v2:
- Actually pass the retrieved data to the target function
- Re-wrap comments to match net/ style
- Mention next-next in the mail subjects
- Pick up tags
- Link to v1: https://lore.kernel.org/r/20250523-topic-ipa_imem-v1-0-b5d536291c7f@oss.qualcomm.com

---
Konrad Dybcio (3):
      dt-bindings: sram: qcom,imem: Allow modem-tables subnode
      dt-bindings: net: qcom,ipa: Add sram property for describing IMEM slice
      net: ipa: Grab IMEM slice base/size from DTS

 .../devicetree/bindings/net/qcom,ipa.yaml          |  7 +++++++
 .../devicetree/bindings/sram/qcom,imem.yaml        | 14 ++++++++++++++
 drivers/net/ipa/ipa_data.h                         |  9 +++++++--
 drivers/net/ipa/ipa_mem.c                          | 22 +++++++++++++++++++++-
 4 files changed, 49 insertions(+), 3 deletions(-)
---
base-commit: 779cae956c8316aebc1946ef86ca001f99658270
change-id: 20250523-topic-ipa_imem-def66cca88e5

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

[PATCH net-next v5 1/3] dt-bindings: sram: qcom,imem: Allow modem-tables subnode

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

The IP Accelerator hardware/firmware owns a sizeable region within the
IMEM, named 'modem-tables', containing various packet processing
configuration data.

It's not actually accessed by the OS, although we have to IOMMU-map it
with the IPA device, so that presumably the firmware can act upon it.

Allow it as a subnode of IMEM.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Reviewed-by: Alex Elder <elder@riscstar.com>
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 Documentation/devicetree/bindings/sram/qcom,imem.yaml | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
index 6a627c57ae2f..c63026904061 100644
--- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
+++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
@@ -67,6 +67,20 @@ properties:
     $ref: /schemas/power/reset/syscon-reboot-mode.yaml#
 
 patternProperties:
+  "^modem-tables@[0-9a-f]+$":
+    type: object
+    description:
+      Region containing packet processing configuration for the IP Accelerator.
+
+    properties:
+      reg:
+        maxItems: 1
+
+    required:
+      - reg
+
+    additionalProperties: false
+
   "^pil-reloc@[0-9a-f]+$":
     $ref: /schemas/remoteproc/qcom,pil-info.yaml#
     description: Peripheral image loader relocation region

-- 
2.53.0

[PATCH net-next v5 2/3] dt-bindings: net: qcom,ipa: Add sram property for describing IMEM slice

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

The IPA driver currently grabs a slice of IMEM through hardcoded
addresses. Not only is that ugly and against the principles of DT,
but it also creates a situation where two distinct platforms
implementing the same version of IPA would need to be hardcoded
together and matched at runtime.

Instead, do the sane thing and accept a handle to said region directly.

Don't make it required on purpose, as it's not there on ancient
implementations (currently unsupported) and we're not yet done with
filling the data across al DTs.

Reviewed-by: Alex Elder <elder@riscstar.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 Documentation/devicetree/bindings/net/qcom,ipa.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/qcom,ipa.yaml b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
index c7f5f2ef7452..4237e74041ef 100644
--- a/Documentation/devicetree/bindings/net/qcom,ipa.yaml
+++ b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
@@ -165,6 +165,13 @@ properties:
       initializing IPA hardware.  Optional, and only used when
       Trust Zone performs early initialization.
 
+  sram:
+    maxItems: 1
+    description:
+      A reference to an additional region residing in IMEM (special
+      on-chip SRAM), which is accessed by the IPA firmware and needs
+      to be IOMMU-mapped from the OS.
+
 required:
   - compatible
   - iommus

-- 
2.53.0

[PATCH net-next v5 3/3] net: ipa: Grab IMEM slice base/size from DTS

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

This is a detail that differ per chip, and not per IPA version (and
there are cases of the same IPA versions being implemented across very
very very different SoCs).

This region isn't actually used by the driver, but we most definitely
want to iommu-map it, so that IPA can poke at the data within.

Reviewed-by: Alex Elder <elder@riscstar.com>
Acked-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 drivers/net/ipa/ipa_data.h |  9 +++++++--
 drivers/net/ipa/ipa_mem.c  | 22 +++++++++++++++++++++-
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ipa/ipa_data.h b/drivers/net/ipa/ipa_data.h
index 2fd03f0799b2..f3bdc64cef05 100644
--- a/drivers/net/ipa/ipa_data.h
+++ b/drivers/net/ipa/ipa_data.h
@@ -185,8 +185,13 @@ struct ipa_resource_data {
 struct ipa_mem_data {
 	u32 local_count;
 	const struct ipa_mem *local;
-	u32 imem_addr;
-	u32 imem_size;
+
+	/* These values are now passed via DT, but to support
+	 * older systems we must allow this to be specified here.
+	 */
+	u32 imem_addr; /* DEPRECATED */
+	u32 imem_size; /* DEPRECATED */
+
 	u32 smem_size;
 };
 
diff --git a/drivers/net/ipa/ipa_mem.c b/drivers/net/ipa/ipa_mem.c
index 835a3c9c1fd4..5d3f68bd02d9 100644
--- a/drivers/net/ipa/ipa_mem.c
+++ b/drivers/net/ipa/ipa_mem.c
@@ -7,6 +7,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/io.h>
 #include <linux/iommu.h>
+#include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/types.h>
 
@@ -617,7 +618,9 @@ static void ipa_smem_exit(struct ipa *ipa)
 int ipa_mem_init(struct ipa *ipa, struct platform_device *pdev,
 		 const struct ipa_mem_data *mem_data)
 {
+	struct device_node *ipa_slice_np;
 	struct device *dev = &pdev->dev;
+	u32 imem_base, imem_size;
 	struct resource *res;
 	int ret;
 
@@ -656,7 +659,24 @@ int ipa_mem_init(struct ipa *ipa, struct platform_device *pdev,
 	ipa->mem_addr = res->start;
 	ipa->mem_size = resource_size(res);
 
-	ret = ipa_imem_init(ipa, mem_data->imem_addr, mem_data->imem_size);
+	ipa_slice_np = of_parse_phandle(dev->of_node, "sram", 0);
+	if (ipa_slice_np) {
+		ret = of_address_to_resource(ipa_slice_np, 0, res);
+		of_node_put(ipa_slice_np);
+		if (ret)
+			goto err_unmap;
+
+		imem_base = res->start;
+		imem_size = resource_size(res);
+	} else {
+		/* Backwards compatibility for DTs lacking
+		 * an explicit reference
+		 */
+		imem_base = mem_data->imem_addr;
+		imem_size = mem_data->imem_size;
+	}
+
+	ret = ipa_imem_init(ipa, imem_base, imem_size);
 	if (ret)
 		goto err_unmap;
 

-- 
2.53.0

Re: [net-next,v5,3/3] net: ipa: Grab IMEM slice base/size from DTS

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: ipa: Grab IMEM slice base/size from DTS

This commit changes IPA driver initialization to retrieve IMEM slice
base and size from the device tree instead of hardcoded platform data,
with fallback to the old method for backwards compatibility.

> diff --git a/drivers/net/ipa/ipa_data.h b/drivers/net/ipa/ipa_data.h
> index 2fd03f0799b2..f3bdc64cef05 100644
> --- a/drivers/net/ipa/ipa_data.h
> +++ b/drivers/net/ipa/ipa_data.h
> @@ -185,8 +185,13 @@ struct ipa_resource_data {
>  struct ipa_mem_data {
>  	u32 local_count;
>  	const struct ipa_mem *local;
> -	u32 imem_addr;
> -	u32 imem_size;
> +
> +	/* These values are now passed via DT, but to support
> +	 * older systems we must allow this to be specified here.
> +	 */
> +	u32 imem_addr; /* DEPRECATED */
> +	u32 imem_size; /* DEPRECATED */
> +
>  	u32 smem_size;
>  };
>
> diff --git a/drivers/net/ipa/ipa_mem.c b/drivers/net/ipa/ipa_mem.c
> index 835a3c9c1fd4..5d3f68bd02d9 100644
> --- a/drivers/net/ipa/ipa_mem.c
> +++ b/drivers/net/ipa/ipa_mem.c
> @@ -7,6 +7,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/io.h>
>  #include <linux/iommu.h>
> +#include <linux/of_address.h>
>  #include <linux/platform_device.h>
>  #include <linux/types.h>
>

[ ... ]

> @@ -656,7 +659,24 @@ int ipa_mem_init(struct ipa *ipa, struct platform_device *pdev,
>  	ipa->mem_addr = res->start;
>  	ipa->mem_size = resource_size(res);
>
> -	ret = ipa_imem_init(ipa, mem_data->imem_addr, mem_data->imem_size);
> +	ipa_slice_np = of_parse_phandle(dev->of_node, "sram", 0);
> +	if (ipa_slice_np) {
> +		ret = of_address_to_resource(ipa_slice_np, 0, res);
> +		of_node_put(ipa_slice_np);
> +		if (ret)
> +			goto err_unmap;
> +
> +		imem_base = res->start;
> +		imem_size = resource_size(res);
                     ^^^^

Does this code corrupt the platform device's resource array? The variable
'res' was obtained from platform_get_resource_byname() earlier in this
function, which returns a pointer directly into pdev->resource[]:

drivers/base/platform.c:platform_get_resource_byname()
    return platform_get_resource(dev, type, num);
        => return &dev->resource[i];

Passing this pointer to of_address_to_resource() causes
__of_address_to_resource() to memset the entire structure to zero and then
overwrite all fields:

drivers/of/address.c:__of_address_to_resource()
    memset(r, 0, sizeof(struct resource));
    ...
    r->flags = ...
    r->name = ...
    r->start = ...
    r->end = ...

This permanently corrupts the platform device's "ipa-shared" resource entry.
While ipa->mem_addr and ipa->mem_size were saved before the corruption, the
platform device's resource array is shared kernel infrastructure.

After this corruption:
- /proc/iomem and sysfs will report incorrect resource information
- If the driver is unbound and rebound, platform_get_resource_byname(pdev,
  IORESOURCE_MEM, "ipa-shared") will fail because the resource's name field
  was overwritten with the sram DT node's full_name, causing probe to fail
  with -ENODEV

Would it be better to use a separate local variable for the
of_address_to_resource() call?

struct resource sram_res;
...
ret = of_address_to_resource(ipa_slice_np, 0, &sram_res);
...
imem_base = sram_res.start;
imem_size = resource_size(&sram_res);

> +	} else {
> +		/* Backwards compatibility for DTs lacking
> +		 * an explicit reference
> +		 */
> +		imem_base = mem_data->imem_addr;
> +		imem_size = mem_data->imem_size;
> +	}
> +
> +	ret = ipa_imem_init(ipa, imem_base, imem_size);
>  	if (ret)
>  		goto err_unmap;
-- 
pw-bot: cr

Re: [net-next,v5,3/3] net: ipa: Grab IMEM slice base/size from DTS

[Konrad Dybcio]

On 2/27/26 2:54 AM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> 
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---

[...]

>> @@ -656,7 +659,24 @@ int ipa_mem_init(struct ipa *ipa, struct platform_device *pdev,
>>  	ipa->mem_addr = res->start;
>>  	ipa->mem_size = resource_size(res);
>>
>> -	ret = ipa_imem_init(ipa, mem_data->imem_addr, mem_data->imem_size);
>> +	ipa_slice_np = of_parse_phandle(dev->of_node, "sram", 0);
>> +	if (ipa_slice_np) {
>> +		ret = of_address_to_resource(ipa_slice_np, 0, res);
>> +		of_node_put(ipa_slice_np);
>> +		if (ret)
>> +			goto err_unmap;
>> +
>> +		imem_base = res->start;
>> +		imem_size = resource_size(res);
>                      ^^^^
> 
> Does this code corrupt the platform device's resource array? The variable
> 'res' was obtained from platform_get_resource_byname() earlier in this
> function, which returns a pointer directly into pdev->resource[]:
> 
> drivers/base/platform.c:platform_get_resource_byname()
>     return platform_get_resource(dev, type, num);
>         => return &dev->resource[i];
> 
> Passing this pointer to of_address_to_resource() causes
> __of_address_to_resource() to memset the entire structure to zero and then
> overwrite all fields:
> 
> drivers/of/address.c:__of_address_to_resource()
>     memset(r, 0, sizeof(struct resource));
>     ...
>     r->flags = ...
>     r->name = ...
>     r->start = ...
>     r->end = ...
> 
> This permanently corrupts the platform device's "ipa-shared" resource entry.
> While ipa->mem_addr and ipa->mem_size were saved before the corruption, the
> platform device's resource array is shared kernel infrastructure.
> 
> After this corruption:
> - /proc/iomem and sysfs will report incorrect resource information
> - If the driver is unbound and rebound, platform_get_resource_byname(pdev,
>   IORESOURCE_MEM, "ipa-shared") will fail because the resource's name field
>   was overwritten with the sram DT node's full_name, causing probe to fail
>   with -ENODEV
> 
> Would it be better to use a separate local variable for the
> of_address_to_resource() call?

Yes, the computer proved useful once again - I tunnel-visioned-out the
prior use of 'res'.

Konrad