[PATCH v2 0/2] remoteproc: qcom: Use of_reserved_mem_lookup()

[PATCH v2 0/2] remoteproc: qcom: Use of_reserved_mem_lookup()

[Stephan Gerhold]

Use of_reserved_mem_lookup() instead of of_address_to_resource() inside
the Qualcomm remoteproc drivers. This has the advantage that it ensures
that the referenced memory region was really reserved and is not e.g.
status = "disabled".

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Changes in v2:
- Add missing check for rmem (Bjorn)
- Add checks for rmem->size to ensure dynamic reserved memory was 
  really allocated
- Link to v1: https://lore.kernel.org/r/20230529-rproc-of-rmem-v1-1-5b1e38880aba@gerhold.net

---
Stephan Gerhold (2):
      remoteproc: qcom: Handle reserved-memory allocation issues
      remoteproc: qcom: Use of_reserved_mem_lookup()

 drivers/remoteproc/qcom_q6v5_adsp.c | 24 ++++++++---------
 drivers/remoteproc/qcom_q6v5_mss.c  | 35 +++++++++++++++----------
 drivers/remoteproc/qcom_q6v5_pas.c  | 51 ++++++++++++++++++++-----------------
 drivers/remoteproc/qcom_q6v5_wcss.c |  2 +-
 drivers/remoteproc/qcom_wcnss.c     | 24 ++++++++---------
 5 files changed, 71 insertions(+), 65 deletions(-)
---
base-commit: 1ca04f21b204e99dd704146231adfb79ea2fb366
change-id: 20230529-rproc-of-rmem-7d931f61f64e

Best regards,
-- 
Stephan Gerhold <stephan@gerhold.net>

[PATCH v2 1/2] remoteproc: qcom: Handle reserved-memory allocation issues

[Stephan Gerhold]

If Linux fails to allocate the dynamic reserved memory specified in the
device tree, the size of the reserved_mem will be 0. Add a check for
this to avoid using an invalid reservation.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
New patch in v2, I wasn't aware of this until Bjorn posted a similar
patch for rmtfs:
https://lore.kernel.org/linux-arm-msm/20230530233643.4044823-4-quic_bjorande@quicinc.com/
---
 drivers/remoteproc/qcom_q6v5_mss.c  | 2 +-
 drivers/remoteproc/qcom_q6v5_wcss.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index 70bffc9f33f6..a35ab6e860f3 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -1932,7 +1932,7 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
 		return 0;
 
 	rmem = of_reserved_mem_lookup(node);
-	if (!rmem) {
+	if (!rmem || !rmem->size) {
 		dev_err(qproc->dev, "unable to resolve metadata region\n");
 		return -EINVAL;
 	}
diff --git a/drivers/remoteproc/qcom_q6v5_wcss.c b/drivers/remoteproc/qcom_q6v5_wcss.c
index b437044aa126..9edab9d60c21 100644
--- a/drivers/remoteproc/qcom_q6v5_wcss.c
+++ b/drivers/remoteproc/qcom_q6v5_wcss.c
@@ -882,7 +882,7 @@ static int q6v5_alloc_memory_region(struct q6v5_wcss *wcss)
 		rmem = of_reserved_mem_lookup(node);
 	of_node_put(node);
 
-	if (!rmem) {
+	if (!rmem || !rmem->size) {
 		dev_err(dev, "unable to acquire memory-region\n");
 		return -EINVAL;
 	}

-- 
2.40.1

[PATCH v2 2/2] remoteproc: qcom: Use of_reserved_mem_lookup()

[Stephan Gerhold]

Reserved memory can be either looked up using the generic function
of_address_to_resource() or using the special of_reserved_mem_lookup().
The latter has the advantage that it ensures that the referenced memory
region was really reserved and is not e.g. status = "disabled".

of_reserved_mem also supports allocating reserved memory dynamically at
boot time. This works only when using of_reserved_mem_lookup() since
there won't be a fixed address in the device tree.

Switch the code to use of_reserved_mem_lookup(), similar to
qcom_q6v5_wcss.c which is using it already. There is no functional
difference for static reserved memory allocations.

While at it this also adds two missing of_node_put() calls in
qcom_q6v5_pas.c.

Link: https://lore.kernel.org/r/20230529-rproc-of-rmem-v1-1-5b1e38880aba@gerhold.net
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 drivers/remoteproc/qcom_q6v5_adsp.c | 24 ++++++++---------
 drivers/remoteproc/qcom_q6v5_mss.c  | 33 ++++++++++++++----------
 drivers/remoteproc/qcom_q6v5_pas.c  | 51 ++++++++++++++++++++-----------------
 drivers/remoteproc/qcom_wcnss.c     | 24 ++++++++---------
 4 files changed, 69 insertions(+), 63 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
index 6777a3bd6226..c87ab77fec16 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -14,8 +14,8 @@
 #include <linux/kernel.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
-#include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
@@ -637,28 +637,26 @@ static int adsp_init_mmio(struct qcom_adsp *adsp,
 
 static int adsp_alloc_memory_region(struct qcom_adsp *adsp)
 {
+	struct reserved_mem *rmem = NULL;
 	struct device_node *node;
-	struct resource r;
-	int ret;
 
 	node = of_parse_phandle(adsp->dev->of_node, "memory-region", 0);
-	if (!node) {
-		dev_err(adsp->dev, "no memory-region specified\n");
+	if (node)
+		rmem = of_reserved_mem_lookup(node);
+	of_node_put(node);
+
+	if (!rmem || !rmem->size) {
+		dev_err(adsp->dev, "unable to resolve memory-region\n");
 		return -EINVAL;
 	}
 
-	ret = of_address_to_resource(node, 0, &r);
-	of_node_put(node);
-	if (ret)
-		return ret;
-
-	adsp->mem_phys = adsp->mem_reloc = r.start;
-	adsp->mem_size = resource_size(&r);
+	adsp->mem_phys = adsp->mem_reloc = rmem->base;
+	adsp->mem_size = rmem->size;
 	adsp->mem_region = devm_ioremap_wc(adsp->dev,
 				adsp->mem_phys, adsp->mem_size);
 	if (!adsp->mem_region) {
 		dev_err(adsp->dev, "unable to map memory region: %pa+%zx\n",
-			&r.start, adsp->mem_size);
+			&rmem->base, adsp->mem_size);
 		return -EBUSY;
 	}
 
diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index a35ab6e860f3..dbd8bc53d0c9 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -15,7 +15,6 @@
 #include <linux/kernel.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
-#include <linux/of_address.h>
 #include <linux/of_device.h>
 #include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
@@ -1875,8 +1874,6 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
 	struct device_node *child;
 	struct reserved_mem *rmem;
 	struct device_node *node;
-	struct resource r;
-	int ret;
 
 	/*
 	 * In the absence of mba/mpss sub-child, extract the mba and mpss
@@ -1891,15 +1888,20 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
 		of_node_put(child);
 	}
 
-	ret = of_address_to_resource(node, 0, &r);
+	if (!node) {
+		dev_err(qproc->dev, "no mba memory-region specified\n");
+		return -EINVAL;
+	}
+
+	rmem = of_reserved_mem_lookup(node);
 	of_node_put(node);
-	if (ret) {
+	if (!rmem || !rmem->size) {
 		dev_err(qproc->dev, "unable to resolve mba region\n");
-		return ret;
+		return -EINVAL;
 	}
 
-	qproc->mba_phys = r.start;
-	qproc->mba_size = resource_size(&r);
+	qproc->mba_phys = rmem->base;
+	qproc->mba_size = rmem->size;
 
 	if (!child) {
 		node = of_parse_phandle(qproc->dev->of_node,
@@ -1910,15 +1912,20 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
 		of_node_put(child);
 	}
 
-	ret = of_address_to_resource(node, 0, &r);
+	if (!node) {
+		dev_err(qproc->dev, "no mpss memory-region specified\n");
+		return -EINVAL;
+	}
+
+	rmem = of_reserved_mem_lookup(node);
 	of_node_put(node);
-	if (ret) {
+	if (!rmem || !rmem->size) {
 		dev_err(qproc->dev, "unable to resolve mpss region\n");
-		return ret;
+		return -EINVAL;
 	}
 
-	qproc->mpss_phys = qproc->mpss_reloc = r.start;
-	qproc->mpss_size = resource_size(&r);
+	qproc->mpss_phys = qproc->mpss_reloc = rmem->base;
+	qproc->mpss_size = rmem->size;
 
 	if (!child) {
 		node = of_parse_phandle(qproc->dev->of_node, "memory-region", 2);
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index ca0155f41dac..dd3fba4dae33 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -13,8 +13,8 @@
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
@@ -533,9 +533,8 @@ static void adsp_pds_detach(struct qcom_adsp *adsp, struct device **pds,
 
 static int adsp_alloc_memory_region(struct qcom_adsp *adsp)
 {
+	struct reserved_mem *rmem;
 	struct device_node *node;
-	struct resource r;
-	int ret;
 
 	node = of_parse_phandle(adsp->dev->of_node, "memory-region", 0);
 	if (!node) {
@@ -543,17 +542,19 @@ static int adsp_alloc_memory_region(struct qcom_adsp *adsp)
 		return -EINVAL;
 	}
 
-	ret = of_address_to_resource(node, 0, &r);
+	rmem = of_reserved_mem_lookup(node);
 	of_node_put(node);
-	if (ret)
-		return ret;
+	if (!rmem || !rmem->size) {
+		dev_err(adsp->dev, "unable to resolve memory-region\n");
+		return -EINVAL;
+	}
 
-	adsp->mem_phys = adsp->mem_reloc = r.start;
-	adsp->mem_size = resource_size(&r);
+	adsp->mem_phys = adsp->mem_reloc = rmem->base;
+	adsp->mem_size = rmem->size;
 	adsp->mem_region = devm_ioremap_wc(adsp->dev, adsp->mem_phys, adsp->mem_size);
 	if (!adsp->mem_region) {
 		dev_err(adsp->dev, "unable to map memory region: %pa+%zx\n",
-			&r.start, adsp->mem_size);
+			&rmem->base, adsp->mem_size);
 		return -EBUSY;
 	}
 
@@ -566,16 +567,19 @@ static int adsp_alloc_memory_region(struct qcom_adsp *adsp)
 		return -EINVAL;
 	}
 
-	ret = of_address_to_resource(node, 0, &r);
-	if (ret)
-		return ret;
+	rmem = of_reserved_mem_lookup(node);
+	of_node_put(node);
+	if (!rmem || !rmem->size) {
+		dev_err(adsp->dev, "unable to resolve dtb memory-region\n");
+		return -EINVAL;
+	}
 
-	adsp->dtb_mem_phys = adsp->dtb_mem_reloc = r.start;
-	adsp->dtb_mem_size = resource_size(&r);
+	adsp->dtb_mem_phys = adsp->dtb_mem_reloc = rmem->base;
+	adsp->dtb_mem_size = rmem->size;
 	adsp->dtb_mem_region = devm_ioremap_wc(adsp->dev, adsp->dtb_mem_phys, adsp->dtb_mem_size);
 	if (!adsp->dtb_mem_region) {
 		dev_err(adsp->dev, "unable to map dtb memory region: %pa+%zx\n",
-			&r.start, adsp->dtb_mem_size);
+			&rmem->base, adsp->dtb_mem_size);
 		return -EBUSY;
 	}
 
@@ -584,29 +588,28 @@ static int adsp_alloc_memory_region(struct qcom_adsp *adsp)
 
 static int adsp_assign_memory_region(struct qcom_adsp *adsp)
 {
+	struct reserved_mem *rmem = NULL;
 	struct qcom_scm_vmperm perm;
 	struct device_node *node;
-	struct resource r;
 	int ret;
 
 	if (!adsp->region_assign_idx)
 		return 0;
 
 	node = of_parse_phandle(adsp->dev->of_node, "memory-region", adsp->region_assign_idx);
-	if (!node) {
-		dev_err(adsp->dev, "missing shareable memory-region\n");
+	if (node)
+		rmem = of_reserved_mem_lookup(node);
+	of_node_put(node);
+	if (!rmem || !rmem->size) {
+		dev_err(adsp->dev, "unable to resolve shareable memory-region\n");
 		return -EINVAL;
 	}
 
-	ret = of_address_to_resource(node, 0, &r);
-	if (ret)
-		return ret;
-
 	perm.vmid = QCOM_SCM_VMID_MSS_MSA;
 	perm.perm = QCOM_SCM_PERM_RW;
 
-	adsp->region_assign_phys = r.start;
-	adsp->region_assign_size = resource_size(&r);
+	adsp->region_assign_phys = rmem->base;
+	adsp->region_assign_size = rmem->size;
 	adsp->region_assign_perms = BIT(QCOM_SCM_VMID_HLOS);
 
 	ret = qcom_scm_assign_mem(adsp->region_assign_phys,
diff --git a/drivers/remoteproc/qcom_wcnss.c b/drivers/remoteproc/qcom_wcnss.c
index 1ed0647bc962..8bb0c8ef0771 100644
--- a/drivers/remoteproc/qcom_wcnss.c
+++ b/drivers/remoteproc/qcom_wcnss.c
@@ -14,8 +14,8 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/io.h>
-#include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
@@ -506,27 +506,25 @@ static int wcnss_request_irq(struct qcom_wcnss *wcnss,
 
 static int wcnss_alloc_memory_region(struct qcom_wcnss *wcnss)
 {
+	struct reserved_mem *rmem = NULL;
 	struct device_node *node;
-	struct resource r;
-	int ret;
 
 	node = of_parse_phandle(wcnss->dev->of_node, "memory-region", 0);
-	if (!node) {
-		dev_err(wcnss->dev, "no memory-region specified\n");
+	if (node)
+		rmem = of_reserved_mem_lookup(node);
+	of_node_put(node);
+
+	if (!rmem || !rmem->size) {
+		dev_err(wcnss->dev, "unable to resolve memory-region\n");
 		return -EINVAL;
 	}
 
-	ret = of_address_to_resource(node, 0, &r);
-	of_node_put(node);
-	if (ret)
-		return ret;
-
-	wcnss->mem_phys = wcnss->mem_reloc = r.start;
-	wcnss->mem_size = resource_size(&r);
+	wcnss->mem_phys = wcnss->mem_reloc = rmem->base;
+	wcnss->mem_size = rmem->size;
 	wcnss->mem_region = devm_ioremap_wc(wcnss->dev, wcnss->mem_phys, wcnss->mem_size);
 	if (!wcnss->mem_region) {
 		dev_err(wcnss->dev, "unable to map memory region: %pa+%zx\n",
-			&r.start, wcnss->mem_size);
+			&rmem->base, wcnss->mem_size);
 		return -EBUSY;
 	}
 

-- 
2.40.1

Re: [PATCH v2 1/2] remoteproc: qcom: Handle reserved-memory allocation issues

[Caleb Connolly]

On 6/14/23 17:31, Stephan Gerhold wrote:
> If Linux fails to allocate the dynamic reserved memory specified in the
> device tree, the size of the reserved_mem will be 0. Add a check for
> this to avoid using an invalid reservation.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

Other uses of of_reserved_mem_lookup() also have unchecked uses of rmem 
[1], or check different things [2].

Does it make sense to put this check in the function itself?

I can't think of any obvious scenarios where it makes sense to 
differentiate between rmem being NULL vs having a size of zero at the 
time where a driver is fetching it.

As Bjorn described in the rmtfs patch, the memory allocation is 
essentially ignored, wouldn't it be better to print an error and 
invalidate the rmem in [3]?

[1]: 
https://elixir.bootlin.com/linux/v6.4-rc6/source/drivers/net/ethernet/mediatek/mtk_wed.c#L818
[2]: 
https://elixir.bootlin.com/linux/v6.4-rc6/source/drivers/remoteproc/rcar_rproc.c#L71
[3]: 
https://elixir.bootlin.com/linux/v6.4-rc6/source/drivers/of/of_reserved_mem.c#L276

// Caleb (they/them)
> ---
> New patch in v2, I wasn't aware of this until Bjorn posted a similar
> patch for rmtfs:
> https://lore.kernel.org/linux-arm-msm/20230530233643.4044823-4-quic_bjorande@quicinc.com/
> ---
>   drivers/remoteproc/qcom_q6v5_mss.c  | 2 +-
>   drivers/remoteproc/qcom_q6v5_wcss.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index 70bffc9f33f6..a35ab6e860f3 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -1932,7 +1932,7 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
>   		return 0;
>   
>   	rmem = of_reserved_mem_lookup(node);
> -	if (!rmem) {
> +	if (!rmem || !rmem->size) {
>   		dev_err(qproc->dev, "unable to resolve metadata region\n");
>   		return -EINVAL;
>   	}
> diff --git a/drivers/remoteproc/qcom_q6v5_wcss.c b/drivers/remoteproc/qcom_q6v5_wcss.c
> index b437044aa126..9edab9d60c21 100644
> --- a/drivers/remoteproc/qcom_q6v5_wcss.c
> +++ b/drivers/remoteproc/qcom_q6v5_wcss.c
> @@ -882,7 +882,7 @@ static int q6v5_alloc_memory_region(struct q6v5_wcss *wcss)
>   		rmem = of_reserved_mem_lookup(node);
>   	of_node_put(node);
>   
> -	if (!rmem) {
> +	if (!rmem || !rmem->size) {
>   		dev_err(dev, "unable to acquire memory-region\n");
>   		return -EINVAL;
>   	}
> 

Re: [PATCH v2 1/2] remoteproc: qcom: Handle reserved-memory allocation issues

[Stephan Gerhold]

On Thu, Jun 15, 2023 at 11:44:06AM +0100, Caleb Connolly wrote:
> On 6/14/23 17:31, Stephan Gerhold wrote:
> > If Linux fails to allocate the dynamic reserved memory specified in the
> > device tree, the size of the reserved_mem will be 0. Add a check for
> > this to avoid using an invalid reservation.
> > 
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> 
> Other uses of of_reserved_mem_lookup() also have unchecked uses of rmem [1],
> or check different things [2].
> 
> Does it make sense to put this check in the function itself?
> 
> I can't think of any obvious scenarios where it makes sense to differentiate
> between rmem being NULL vs having a size of zero at the time where a driver
> is fetching it.
> 
> As Bjorn described in the rmtfs patch, the memory allocation is essentially
> ignored, wouldn't it be better to print an error and invalidate the rmem in
> [3]?
> 

"Invalidating" isn't that easy because the reserved_mem is currently
stored in a simple array. Removing an entry would require shifting all
following values. But I suppose it would be easy to add the rmem->size
!= 0 check in of_reserved_mem_lookup() so it doesn't have to be checked
on all usages.

Given that no one seems to check for this at the moment I'm inclined to
agree with you that it would be better to handle this directly in
of_reserved_mem. Bjorn, what do you think?

Thanks,
Stephan

> [1]: https://elixir.bootlin.com/linux/v6.4-rc6/source/drivers/net/ethernet/mediatek/mtk_wed.c#L818
> [2]: https://elixir.bootlin.com/linux/v6.4-rc6/source/drivers/remoteproc/rcar_rproc.c#L71
> [3]: https://elixir.bootlin.com/linux/v6.4-rc6/source/drivers/of/of_reserved_mem.c#L276
> 
> // Caleb (they/them)
> > ---
> > New patch in v2, I wasn't aware of this until Bjorn posted a similar
> > patch for rmtfs:
> > https://lore.kernel.org/linux-arm-msm/20230530233643.4044823-4-quic_bjorande@quicinc.com/
> > ---
> >   drivers/remoteproc/qcom_q6v5_mss.c  | 2 +-
> >   drivers/remoteproc/qcom_q6v5_wcss.c | 2 +-
> >   2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> > index 70bffc9f33f6..a35ab6e860f3 100644
> > --- a/drivers/remoteproc/qcom_q6v5_mss.c
> > +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> > @@ -1932,7 +1932,7 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
> >   		return 0;
> >   	rmem = of_reserved_mem_lookup(node);
> > -	if (!rmem) {
> > +	if (!rmem || !rmem->size) {
> >   		dev_err(qproc->dev, "unable to resolve metadata region\n");
> >   		return -EINVAL;
> >   	}
> > diff --git a/drivers/remoteproc/qcom_q6v5_wcss.c b/drivers/remoteproc/qcom_q6v5_wcss.c
> > index b437044aa126..9edab9d60c21 100644
> > --- a/drivers/remoteproc/qcom_q6v5_wcss.c
> > +++ b/drivers/remoteproc/qcom_q6v5_wcss.c
> > @@ -882,7 +882,7 @@ static int q6v5_alloc_memory_region(struct q6v5_wcss *wcss)
> >   		rmem = of_reserved_mem_lookup(node);
> >   	of_node_put(node);
> > -	if (!rmem) {
> > +	if (!rmem || !rmem->size) {
> >   		dev_err(dev, "unable to acquire memory-region\n");
> >   		return -EINVAL;
> >   	}
> > 

Re: [PATCH v2 1/2] remoteproc: qcom: Handle reserved-memory allocation issues

[Stephan Gerhold]

On Thu, Jun 15, 2023 at 04:51:44PM +0200, Stephan Gerhold wrote:
> On Thu, Jun 15, 2023 at 11:44:06AM +0100, Caleb Connolly wrote:
> > On 6/14/23 17:31, Stephan Gerhold wrote:
> > > If Linux fails to allocate the dynamic reserved memory specified in the
> > > device tree, the size of the reserved_mem will be 0. Add a check for
> > > this to avoid using an invalid reservation.
> > > 
> > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > 
> > Other uses of of_reserved_mem_lookup() also have unchecked uses of rmem [1],
> > or check different things [2].
> > 
> > Does it make sense to put this check in the function itself?
> > 
> > I can't think of any obvious scenarios where it makes sense to differentiate
> > between rmem being NULL vs having a size of zero at the time where a driver
> > is fetching it.
> > 
> > As Bjorn described in the rmtfs patch, the memory allocation is essentially
> > ignored, wouldn't it be better to print an error and invalidate the rmem in
> > [3]?
> > 
> 
> "Invalidating" isn't that easy because the reserved_mem is currently
> stored in a simple array. Removing an entry would require shifting all
> following values. But I suppose it would be easy to add the rmem->size
> != 0 check in of_reserved_mem_lookup() so it doesn't have to be checked
> on all usages.
> 
> Given that no one seems to check for this at the moment I'm inclined to
> agree with you that it would be better to handle this directly in
> of_reserved_mem. Bjorn, what do you think?
> 

I sent a v3 with the additional checks reverted. I'll work on a separate
patch series to improve this independently of this one for all users
(directly in of_reserved_mem).

Thanks,
Stephan