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

[net-next PATCH v3 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 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

 Documentation/devicetree/bindings/net/qcom,ipa.yaml |  7 +++++++
 .../devicetree/bindings/sram/qcom,imem.yaml         | 14 ++++++++++++++
 drivers/net/ipa/ipa_data.h                          |  4 ++++
 drivers/net/ipa/ipa_mem.c                           | 21 ++++++++++++++++++++-
 4 files changed, 45 insertions(+), 1 deletion(-)
---
base-commit: 350adaf7fde9fdbd9aeed6d442a9ae90c6a3ab97
change-id: 20250523-topic-ipa_imem-def66cca88e5

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

[PATCH net-next v3 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.

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

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

[Alex Elder]

On 2/17/26 7:30 AM, Konrad Dybcio wrote:
> 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.

OK so you'll define a "modem-tables@" property in the SRAM node,
whose phandle will then be referred to by the "sram" property
in the IPA node.

That sounds good to me.  Thanks Konrad.

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
> 

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

[Krzysztof Kozlowski]

On Tue, Feb 17, 2026 at 02:30:31PM +0100, Konrad Dybcio wrote:
> 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.

You do not have compatible, so rely on the node name as ABI, which is
fine in general but... I do not see usage of it in the driver. Why do
you need to define modem-tables child then?

> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
>  Documentation/devicetree/bindings/sram/qcom,imem.yaml | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)

Best regards,
Krzysztof

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

[Konrad Dybcio]

On 2/17/26 9:25 PM, Krzysztof Kozlowski wrote:
> On Tue, Feb 17, 2026 at 02:30:31PM +0100, Konrad Dybcio wrote:
>> 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.
> 
> You do not have compatible, so rely on the node name as ABI, which is
> fine in general but... I do not see usage of it in the driver. Why do
> you need to define modem-tables child then?

I don't really *need* the node name to be an ABI. However, the current
binding for IMEM only allows a named "pil-reloc@.." subnode (which is
consumed via of_find_compatible_node() in the remoteproc subsystem) so I
figured the intention was to keep the list of allowed subnodes strictly
moderated

If you'd prefer a blanket pattern declaration with say '^[a-z]@[0-9a-z]+$'
with just a reg requirement inside, I'm fine with that too

Konrad

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

[Krzysztof Kozlowski]

On 18/02/2026 12:05, Konrad Dybcio wrote:
> On 2/17/26 9:25 PM, Krzysztof Kozlowski wrote:
>> On Tue, Feb 17, 2026 at 02:30:31PM +0100, Konrad Dybcio wrote:
>>> 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.
>>
>> You do not have compatible, so rely on the node name as ABI, which is
>> fine in general but... I do not see usage of it in the driver. Why do
>> you need to define modem-tables child then?
> 
> I don't really *need* the node name to be an ABI. However, the current
> binding for IMEM only allows a named "pil-reloc@.." subnode (which is
> consumed via of_find_compatible_node() in the remoteproc subsystem) so I
> figured the intention was to keep the list of allowed subnodes strictly
> moderated
> 
> If you'd prefer a blanket pattern declaration with say '^[a-z]@[0-9a-z]+$'
> with just a reg requirement inside, I'm fine with that too

No, the problem is that you do not use the ABI here at all. Neither
would you use the blanket pattern, so my question stays: why adding ABI
which is not used?

The pil-reloc is being used, as you pointed out.

Best regards,
Krzysztof

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

[Konrad Dybcio]

On 18-Feb-26 12:56, Krzysztof Kozlowski wrote:
> On 18/02/2026 12:05, Konrad Dybcio wrote:
>> On 2/17/26 9:25 PM, Krzysztof Kozlowski wrote:
>>> On Tue, Feb 17, 2026 at 02:30:31PM +0100, Konrad Dybcio wrote:
>>>> 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.
>>>
>>> You do not have compatible, so rely on the node name as ABI, which is
>>> fine in general but... I do not see usage of it in the driver. Why do
>>> you need to define modem-tables child then?
>>
>> I don't really *need* the node name to be an ABI. However, the current
>> binding for IMEM only allows a named "pil-reloc@.." subnode (which is
>> consumed via of_find_compatible_node() in the remoteproc subsystem) so I
>> figured the intention was to keep the list of allowed subnodes strictly
>> moderated
>>
>> If you'd prefer a blanket pattern declaration with say '^[a-z]@[0-9a-z]+$'
>> with just a reg requirement inside, I'm fine with that too
> 
> No, the problem is that you do not use the ABI here at all. Neither
> would you use the blanket pattern, so my question stays: why adding ABI
> which is not used?

The subnode I'm trying to introduce is going to be consumed (via a
phandle reference) from the IPA node, as done by the remaining 2
patches in this series.

I would much prefer not to touch this binding file, but there's an
additionalProperties: false and just adding it as-is results in a:

arch/arm64/boot/dts/qcom/sc7280-idp.dtb: sram@146a5000: 'modem-tables@1234' does not match any of the regexes: '^pil-reloc@[0-9a-f]+$', 'pinctrl-[0-9]+'
        from schema $id: http://devicetree.org/schemas/sram/qcom,imem.yaml#

Konrad

> 
> The pil-reloc is being used, as you pointed out.
> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 18/02/2026 13:26, Konrad Dybcio wrote:
> 
> 
> On 18-Feb-26 12:56, Krzysztof Kozlowski wrote:
>> On 18/02/2026 12:05, Konrad Dybcio wrote:
>>> On 2/17/26 9:25 PM, Krzysztof Kozlowski wrote:
>>>> On Tue, Feb 17, 2026 at 02:30:31PM +0100, Konrad Dybcio wrote:
>>>>> 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.
>>>>
>>>> You do not have compatible, so rely on the node name as ABI, which is
>>>> fine in general but... I do not see usage of it in the driver. Why do
>>>> you need to define modem-tables child then?
>>>
>>> I don't really *need* the node name to be an ABI. However, the current
>>> binding for IMEM only allows a named "pil-reloc@.." subnode (which is
>>> consumed via of_find_compatible_node() in the remoteproc subsystem) so I
>>> figured the intention was to keep the list of allowed subnodes strictly
>>> moderated
>>>
>>> If you'd prefer a blanket pattern declaration with say '^[a-z]@[0-9a-z]+$'
>>> with just a reg requirement inside, I'm fine with that too
>>
>> No, the problem is that you do not use the ABI here at all. Neither
>> would you use the blanket pattern, so my question stays: why adding ABI
>> which is not used?
> 
> The subnode I'm trying to introduce is going to be consumed (via a
> phandle reference) from the IPA node, as done by the remaining 2
> patches in this series.

And that's the problem - I do not see consuming child. I see
of_parse_phandle to sram node, not the child.

> 
> I would much prefer not to touch this binding file, but there's an
> additionalProperties: false and just adding it as-is results in a:
> 
> arch/arm64/boot/dts/qcom/sc7280-idp.dtb: sram@146a5000: 'modem-tables@1234' does not match any of the regexes: '^pil-reloc@[0-9a-f]+$', 'pinctrl-[0-9]+'
>         from schema $id: http://devicetree.org/schemas/sram/qcom,imem.yaml#
> 
> Konrad

Best regards,
Krzysztof

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

[Konrad Dybcio]

On 18-Feb-26 14:21, Krzysztof Kozlowski wrote:
> On 18/02/2026 13:26, Konrad Dybcio wrote:
>>
>>
>> On 18-Feb-26 12:56, Krzysztof Kozlowski wrote:
>>> On 18/02/2026 12:05, Konrad Dybcio wrote:
>>>> On 2/17/26 9:25 PM, Krzysztof Kozlowski wrote:
>>>>> On Tue, Feb 17, 2026 at 02:30:31PM +0100, Konrad Dybcio wrote:
>>>>>> 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.
>>>>>
>>>>> You do not have compatible, so rely on the node name as ABI, which is
>>>>> fine in general but... I do not see usage of it in the driver. Why do
>>>>> you need to define modem-tables child then?
>>>>
>>>> I don't really *need* the node name to be an ABI. However, the current
>>>> binding for IMEM only allows a named "pil-reloc@.." subnode (which is
>>>> consumed via of_find_compatible_node() in the remoteproc subsystem) so I
>>>> figured the intention was to keep the list of allowed subnodes strictly
>>>> moderated
>>>>
>>>> If you'd prefer a blanket pattern declaration with say '^[a-z]@[0-9a-z]+$'
>>>> with just a reg requirement inside, I'm fine with that too
>>>
>>> No, the problem is that you do not use the ABI here at all. Neither
>>> would you use the blanket pattern, so my question stays: why adding ABI
>>> which is not used?
>>
>> The subnode I'm trying to introduce is going to be consumed (via a
>> phandle reference) from the IPA node, as done by the remaining 2
>> patches in this series.
> 
> And that's the problem - I do not see consuming child. I see
> of_parse_phandle to sram node, not the child.

Ah, I just realized this series has no DT examples..

The property I proposed to add into the IPA node&code is indeed
named 'sram', but my intention is to pass a phandle to the *child*
(similarly like we pass a phandle to the child of a nvmem provider
rather than to the provider device itself)

i.e. the design I envisioned is:

imem@foo {
	...

	ipa_modem_tables: modem-tables@1234 {
		reg = <0x1234 0x1234>;
	};
};

...

ipa@foobar {
	...

	sram = <&ipa_modem_tables>;
}

Konrad

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

[Krzysztof Kozlowski]

On 18/02/2026 14:32, Konrad Dybcio wrote:
>>>>>
>>>>> If you'd prefer a blanket pattern declaration with say '^[a-z]@[0-9a-z]+$'
>>>>> with just a reg requirement inside, I'm fine with that too
>>>>
>>>> No, the problem is that you do not use the ABI here at all. Neither
>>>> would you use the blanket pattern, so my question stays: why adding ABI
>>>> which is not used?
>>>
>>> The subnode I'm trying to introduce is going to be consumed (via a
>>> phandle reference) from the IPA node, as done by the remaining 2
>>> patches in this series.
>>
>> And that's the problem - I do not see consuming child. I see
>> of_parse_phandle to sram node, not the child.
> 
> Ah, I just realized this series has no DT examples..
> 
> The property I proposed to add into the IPA node&code is indeed
> named 'sram', but my intention is to pass a phandle to the *child*
> (similarly like we pass a phandle to the child of a nvmem provider
> rather than to the provider device itself)
> 
> i.e. the design I envisioned is:
> 
> imem@foo {
> 	...
> 
> 	ipa_modem_tables: modem-tables@1234 {
> 		reg = <0x1234 0x1234>;
> 	};
> };
> 
> ...
> 
> ipa@foobar {
> 	...
> 
> 	sram = <&ipa_modem_tables>;
> }

OK, this explains my questions.


Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 17/02/2026 14:30, Konrad Dybcio wrote:
> 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.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
>  Documentation/devicetree/bindings/sram/qcom,imem.yaml | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Best regards,
Krzysztof

[PATCH net-next v3 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 v3 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 |  4 ++++
 drivers/net/ipa/ipa_mem.c  | 21 ++++++++++++++++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ipa/ipa_data.h b/drivers/net/ipa/ipa_data.h
index 2fd03f0799b2..5fe164981083 100644
--- a/drivers/net/ipa/ipa_data.h
+++ b/drivers/net/ipa/ipa_data.h
@@ -185,8 +185,12 @@ struct ipa_resource_data {
 struct ipa_mem_data {
 	u32 local_count;
 	const struct ipa_mem *local;
+
+	/* DEPRECATED (now passed via DT) fallback data,
+	 * varies per chip and not per IPA version */
 	u32 imem_addr;
 	u32 imem_size;
+
 	u32 smem_size;
 };
 
diff --git a/drivers/net/ipa/ipa_mem.c b/drivers/net/ipa/ipa_mem.c
index 835a3c9c1fd4..583aea625709 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,23 @@ 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)
+			return ret;
+
+		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: [PATCH net-next v3 3/3] net: ipa: Grab IMEM slice base/size from DTS

[Alex Elder]

On 2/17/26 7:30 AM, Konrad Dybcio wrote:
> 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>

I know I already provided Reviewed-by, but I have two
minor comments.  (You can keep my tag even if you don't
incorporate what I suggest below.)

> ---
>   drivers/net/ipa/ipa_data.h |  4 ++++
>   drivers/net/ipa/ipa_mem.c  | 21 ++++++++++++++++++++-
>   2 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ipa/ipa_data.h b/drivers/net/ipa/ipa_data.h
> index 2fd03f0799b2..5fe164981083 100644
> --- a/drivers/net/ipa/ipa_data.h
> +++ b/drivers/net/ipa/ipa_data.h
> @@ -185,8 +185,12 @@ struct ipa_resource_data {
>   struct ipa_mem_data {
>   	u32 local_count;
>   	const struct ipa_mem *local;
> +
> +	/* DEPRECATED (now passed via DT) fallback data,
> +	 * varies per chip and not per IPA version */
>   	u32 imem_addr;
>   	u32 imem_size;

Both the address and size are deprecated, and although you add
white space, I feel like you could be more explicit about saying
both are deprecated.  For example, maybe more like this?

     /* 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..583aea625709 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,23 @@ 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)
> +			return ret;
> +
> +		imem_base = res->start;
> +		imem_size = resource_size(res);
> +	} else {
> +		/* Backwards compatibility for DTs lacking
> +		 * an explicit reference */

I think netdev style says the end-of-comment should go on
the line below.

					-Alex

> +		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;
>   
> 

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

[Simon Horman]

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 reads the IMEM slice base address and size from a device
tree "sram" phandle, falling back to the existing struct fields when
the DT property is absent.

> diff --git a/drivers/net/ipa/ipa_mem.c b/drivers/net/ipa/ipa_mem.c
> index 835a3c9c1fd4..583aea625709 100644
> --- a/drivers/net/ipa/ipa_mem.c
> +++ b/drivers/net/ipa/ipa_mem.c

[ ... ]

> @@ -656,7 +659,23 @@ 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);

Here, res still points into the platform device's resource array from
the earlier platform_get_resource_byname() call.  Won't
of_address_to_resource() overwrite the platform device's "ipa-shared"
resource entry with the SRAM resource data?

The needed values (mem_addr, mem_size) were already copied out, but
this silently corrupts the platform device's resource table.  Would it
be better to use a local struct resource for this call?

Simon says: I think this is a false positive as I don't see how reusing res
here results in resource table corruption.

> +		of_node_put(ipa_slice_np);
> +		if (ret)
> +			return ret;
                        ^^^^^^^^^^

At this point ipa->mem_virt holds a valid memremap() pointer.  Does
this direct return leak the mapping?  Every other error path after the
memremap() in this function uses goto err_unmap, which calls
memunmap(ipa->mem_virt).  Should this be goto err_unmap instead?

Simon says: This seems to me to be an actual problem.

> +
> +		imem_base = res->start;
> +		imem_size = resource_size(res);

[ ... ]

> +	ret = ipa_imem_init(ipa, imem_base, imem_size);
>  	if (ret)
>  		goto err_unmap;

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

[Konrad Dybcio]

On 2/18/26 11:39 AM, Simon Horman 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
> ---

[...]

>> +		of_node_put(ipa_slice_np);
>> +		if (ret)
>> +			return ret;
>                         ^^^^^^^^^^
> 
> At this point ipa->mem_virt holds a valid memremap() pointer.  Does
> this direct return leak the mapping?  Every other error path after the
> memremap() in this function uses goto err_unmap, which calls
> memunmap(ipa->mem_virt).  Should this be goto err_unmap instead?
> 
> Simon says: This seems to me to be an actual problem.

Yes, the robot proved useful

Konrad