[PATCH v3 0/3] soc: qcom: rmtfs: Support dynamic allocation

[PATCH v3 0/3] soc: qcom: rmtfs: Support dynamic allocation

[Bjorn Andersson]

Some platforms have laxed requirements on the placement of the rmtfs
memory region, introduce support for guard pages to allow the DeviceTree
author to rely on the OS/Linux for placement of the region.

Changes since v2:
- Rewrote DeviceTree binding description, to avoid dictating the OS
  behavior.
- Adjusted addr and size before memremap(), to avoid mapping the guard
  pages, and unnecessarily have to adjust the base pointer.

Changes since v1:
- Drop qcom,alloc-size in favour of using reserved-memory/size
- Introduce explicit property to signal that guard pages should be
  carved out from this region (rather than always do it in the dynamic
  case).
- Drop the dma_alloc_coherent() based approach and just add support for
  the guard pages.
- Added handling of failed reserved-memory allocation (patch 3)

---
Bjorn Andersson (3):
      dt-bindings: reserved-memory: rmtfs: Allow guard pages
      soc: qcom: rmtfs: Support discarding guard pages
      soc: qcom: rtmfs: Handle reserved-memory allocation issues

 .../devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml   | 11 +++++++++++
 drivers/soc/qcom/rmtfs_mem.c                                  | 11 ++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)
---
base-commit: 926f75c8a5ab70567eb4c2d82fbc96963313e564
change-id: 20230920-rmtfs-mem-guard-pages-d3d0ed0cb036

Best regards,
-- 
Bjorn Andersson <quic_bjorande@quicinc.com>

[PATCH v3 1/3] dt-bindings: reserved-memory: rmtfs: Allow guard pages

[Bjorn Andersson]

On some Qualcomm platforms the firwmare, or hardware, does not
gracefully handle memory protection of the rmtfs memory region when
placed adjacent to other protected region. Some DeviceTree authors have
worked around this issue by explicitly reserving the space around the
region, but this prevents such author to use rely on the OS to place the
region, through the use of "size" (instead of a fixed location).

Introduce a flag to indicate that guard pages need be carved at the
beginning and end of the memory region. The user shall account for the
two 4k blocks in the defined size.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 .../devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml   | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
index bab982f00485..2d7be508c5a0 100644
--- a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
+++ b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
@@ -26,6 +26,17 @@ properties:
     description: >
       identifier of the client to use this region for buffers
 
+  qcom,use-guard-pages:
+    type: boolean
+    description: >
+      Indicates that the firmware, or hardware, does not gracefully handle
+      memory protection of this region when placed adjacent to other protected
+      memory regions, and that padding around the used portion of the memory
+      region is necessary.
+
+      When this is set, the first and last 4kB should be left unused, and the
+      effective size of the region will thereby shrink with 8kB.
+
   qcom,vmid:
     $ref: /schemas/types.yaml#/definitions/uint32-array
     description: >

-- 
2.25.1

Re: [PATCH v3 1/3] dt-bindings: reserved-memory: rmtfs: Allow guard pages

[Konrad Dybcio]

On 9/21/23 04:37, Bjorn Andersson wrote:
> On some Qualcomm platforms the firwmare, or hardware, does not
> gracefully handle memory protection of the rmtfs memory region when
> placed adjacent to other protected region. Some DeviceTree authors have
> worked around this issue by explicitly reserving the space around the
> region, but this prevents such author to use rely on the OS to place the
> region, through the use of "size" (instead of a fixed location).
> 
> Introduce a flag to indicate that guard pages need be carved at the
> beginning and end of the memory region. The user shall account for the
> two 4k blocks in the defined size.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>   .../devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml   | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
> index bab982f00485..2d7be508c5a0 100644
> --- a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
> +++ b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
> @@ -26,6 +26,17 @@ properties:
>       description: >
>         identifier of the client to use this region for buffers
>   
> +  qcom,use-guard-pages:
> +    type: boolean
> +    description: >
> +      Indicates that the firmware, or hardware, does not gracefully handle
> +      memory protection of this region when placed adjacent to other protected
> +      memory regions, and that padding around the used portion of the memory
> +      region is necessary.
> +
> +      When this is set, the first and last 4kB should be left unused, and the
> +      effective size of the region will thereby shrink with 8kB.
kiB

Konrad

Re: [PATCH v3 1/3] dt-bindings: reserved-memory: rmtfs: Allow guard pages

[Krzysztof Kozlowski]

On 21/09/2023 04:37, Bjorn Andersson wrote:
> On some Qualcomm platforms the firwmare, or hardware, does not
> gracefully handle memory protection of the rmtfs memory region when
> placed adjacent to other protected region. Some DeviceTree authors have
> worked around this issue by explicitly reserving the space around the
> region, but this prevents such author to use rely on the OS to place the
> region, through the use of "size" (instead of a fixed location).
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

> +
> +      When this is set, the first and last 4kB should be left unused, and the
> +      effective size of the region will thereby shrink with 8kB.

Maybe we should not reference the actual size (4 and 8 kB), but rather
page - "the first and last pages in mapping should be left unused ..." etc?



Best regards,
Krzysztof

[PATCH v3 2/3] soc: qcom: rmtfs: Support discarding guard pages

[Bjorn Andersson]

In some configurations, the exact placement of the rmtfs shared memory
region isn't so strict. The DeviceTree author can then choose to use the
"size" property and rely on the OS for placement (in combination with
"alloc-ranges", if desired).

But on some platforms the rmtfs memory region may not be allocated
adjacent to regions allocated by other clients. Add support for
discarding the first and last 4k block in the region, if
qcom,use-guard-pages is specified in DeviceTree.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/soc/qcom/rmtfs_mem.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
index f83811f51175..83bba9321e72 100644
--- a/drivers/soc/qcom/rmtfs_mem.c
+++ b/drivers/soc/qcom/rmtfs_mem.c
@@ -200,6 +200,15 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
 	rmtfs_mem->client_id = client_id;
 	rmtfs_mem->size = rmem->size;
 
+	/*
+	 * If requested, discard the first and last 4k block in order to ensure
+	 * that the rmtfs region isn't adjacent to other protected regions.
+	 */
+	if (of_property_present(node, "qcom,use-guard-pages")) {
+		rmtfs_mem->addr += SZ_4K;
+		rmtfs_mem->size -= 2 * SZ_4K;
+	}
+
 	device_initialize(&rmtfs_mem->dev);
 	rmtfs_mem->dev.parent = &pdev->dev;
 	rmtfs_mem->dev.groups = qcom_rmtfs_mem_groups;

-- 
2.25.1

Re: [PATCH v3 2/3] soc: qcom: rmtfs: Support discarding guard pages

[Konrad Dybcio]

On 9/21/23 04:37, Bjorn Andersson wrote:
> In some configurations, the exact placement of the rmtfs shared memory
> region isn't so strict. The DeviceTree author can then choose to use the
> "size" property and rely on the OS for placement (in combination with
> "alloc-ranges", if desired).
> 
> But on some platforms the rmtfs memory region may not be allocated
> adjacent to regions allocated by other clients. Add support for
> discarding the first and last 4k block in the region, if
> qcom,use-guard-pages is specified in DeviceTree.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
I don't want to block this anymore, but I guess I should ask
the question whether it would be valuable to add a common
reserved-memory property for e.g. low-padding and high-padding

Have we seen cases of that outside rmtfs?

Konrad

Re: [PATCH v3 2/3] soc: qcom: rmtfs: Support discarding guard pages

[Stephan Gerhold]

On Wed, Sep 20, 2023 at 07:37:31PM -0700, Bjorn Andersson wrote:
> In some configurations, the exact placement of the rmtfs shared memory
> region isn't so strict. The DeviceTree author can then choose to use the
> "size" property and rely on the OS for placement (in combination with
> "alloc-ranges", if desired).
> 
> But on some platforms the rmtfs memory region may not be allocated
> adjacent to regions allocated by other clients. Add support for
> discarding the first and last 4k block in the region, if
> qcom,use-guard-pages is specified in DeviceTree.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>  drivers/soc/qcom/rmtfs_mem.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
> index f83811f51175..83bba9321e72 100644
> --- a/drivers/soc/qcom/rmtfs_mem.c
> +++ b/drivers/soc/qcom/rmtfs_mem.c
> @@ -200,6 +200,15 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
>  	rmtfs_mem->client_id = client_id;
>  	rmtfs_mem->size = rmem->size;
>  
> +	/*
> +	 * If requested, discard the first and last 4k block in order to ensure
> +	 * that the rmtfs region isn't adjacent to other protected regions.
> +	 */
> +	if (of_property_present(node, "qcom,use-guard-pages")) {

I think of_property_read_bool() would be more fitting here. Right now
of_property_present() is just a wrapper around of_property_read_bool().
Semantically reading a bool fits better here though. :-)

Feel free to fix that up while applying.

FWIW I don't really have an opinion if "qcom,use-guard-pages" is a good
way to describe this in the DT. For the implementation side feel free to
add my

Reviewed-by: Stephan Gerhold <stephan@gerhold.net>

Thanks,
Stephan

> +		rmtfs_mem->addr += SZ_4K;
> +		rmtfs_mem->size -= 2 * SZ_4K;
> +	}
> +
>  	device_initialize(&rmtfs_mem->dev);
>  	rmtfs_mem->dev.parent = &pdev->dev;
>  	rmtfs_mem->dev.groups = qcom_rmtfs_mem_groups;
> 
> -- 
> 2.25.1
> 

Re: [PATCH v3 2/3] soc: qcom: rmtfs: Support discarding guard pages

[Bjorn Andersson]

On Thu, Sep 21, 2023 at 08:04:06PM +0200, Stephan Gerhold wrote:
> On Wed, Sep 20, 2023 at 07:37:31PM -0700, Bjorn Andersson wrote:
> > In some configurations, the exact placement of the rmtfs shared memory
> > region isn't so strict. The DeviceTree author can then choose to use the
> > "size" property and rely on the OS for placement (in combination with
> > "alloc-ranges", if desired).
> > 
> > But on some platforms the rmtfs memory region may not be allocated
> > adjacent to regions allocated by other clients. Add support for
> > discarding the first and last 4k block in the region, if
> > qcom,use-guard-pages is specified in DeviceTree.
> > 
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > ---
> >  drivers/soc/qcom/rmtfs_mem.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
> > index f83811f51175..83bba9321e72 100644
> > --- a/drivers/soc/qcom/rmtfs_mem.c
> > +++ b/drivers/soc/qcom/rmtfs_mem.c
> > @@ -200,6 +200,15 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
> >  	rmtfs_mem->client_id = client_id;
> >  	rmtfs_mem->size = rmem->size;
> >  
> > +	/*
> > +	 * If requested, discard the first and last 4k block in order to ensure
> > +	 * that the rmtfs region isn't adjacent to other protected regions.
> > +	 */
> > +	if (of_property_present(node, "qcom,use-guard-pages")) {
> 
> I think of_property_read_bool() would be more fitting here. Right now
> of_property_present() is just a wrapper around of_property_read_bool().
> Semantically reading a bool fits better here though. :-)
> 

Are you saying that you would prefer this to be a bool, so hat you can
give it a "false" value? Or you are simply saying "it walks like a
boolean, quacks like a boolean, let's use the boolean accessor"?

> Feel free to fix that up while applying.
> 
> FWIW I don't really have an opinion if "qcom,use-guard-pages" is a good
> way to describe this in the DT. For the implementation side feel free to
> add my
> 

Right, I don't think I commented on your suggestion to make the size of
the guard page configurable. I am not aware of any current or upcoming
reasons for adding such complexity, so I'd simply prefer to stick with a
boolean. Should that need arise, I think this model would allow
extension to express that.

Regards,
Bjorn

> Reviewed-by: Stephan Gerhold <stephan@gerhold.net>
> 
> Thanks,
> Stephan
> 
> > +		rmtfs_mem->addr += SZ_4K;
> > +		rmtfs_mem->size -= 2 * SZ_4K;
> > +	}
> > +
> >  	device_initialize(&rmtfs_mem->dev);
> >  	rmtfs_mem->dev.parent = &pdev->dev;
> >  	rmtfs_mem->dev.groups = qcom_rmtfs_mem_groups;
> > 
> > -- 
> > 2.25.1
> > 

Re: [PATCH v3 2/3] soc: qcom: rmtfs: Support discarding guard pages

[Stephan Gerhold]

eOn Thu, Sep 21, 2023 at 07:51:42PM -0700, Bjorn Andersson wrote:
> On Thu, Sep 21, 2023 at 08:04:06PM +0200, Stephan Gerhold wrote:
> > On Wed, Sep 20, 2023 at 07:37:31PM -0700, Bjorn Andersson wrote:
> > > In some configurations, the exact placement of the rmtfs shared memory
> > > region isn't so strict. The DeviceTree author can then choose to use the
> > > "size" property and rely on the OS for placement (in combination with
> > > "alloc-ranges", if desired).
> > > 
> > > But on some platforms the rmtfs memory region may not be allocated
> > > adjacent to regions allocated by other clients. Add support for
> > > discarding the first and last 4k block in the region, if
> > > qcom,use-guard-pages is specified in DeviceTree.
> > > 
> > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > > ---
> > >  drivers/soc/qcom/rmtfs_mem.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
> > > index f83811f51175..83bba9321e72 100644
> > > --- a/drivers/soc/qcom/rmtfs_mem.c
> > > +++ b/drivers/soc/qcom/rmtfs_mem.c
> > > @@ -200,6 +200,15 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
> > >  	rmtfs_mem->client_id = client_id;
> > >  	rmtfs_mem->size = rmem->size;
> > >  
> > > +	/*
> > > +	 * If requested, discard the first and last 4k block in order to ensure
> > > +	 * that the rmtfs region isn't adjacent to other protected regions.
> > > +	 */
> > > +	if (of_property_present(node, "qcom,use-guard-pages")) {
> > 
> > I think of_property_read_bool() would be more fitting here. Right now
> > of_property_present() is just a wrapper around of_property_read_bool().
> > Semantically reading a bool fits better here though. :-)
> > 
> 
> Are you saying that you would prefer this to be a bool, so hat you can
> give it a "false" value? Or you are simply saying "it walks like a
> boolean, quacks like a boolean, let's use the boolean accessor"?
> 

The latter. I would expect that of_property_present() is used for
properties which usually have a value, while of_property_read_bool()
is used for pure bool values which can be present or not but must not
have a value. I think a "bool" in terms of DT is simply a present or
not-present property without any value?

For example consider

  regulator-min-microvolts = <4200000000>;
  regulator-always-on;

Then I would expect

  - of_property_present(..., "regulator-min-microvolts"), but
  - of_property_read_bool(..., "regulator-always-on")

Does that make sense? :D

> > Feel free to fix that up while applying.
> > 
> > FWIW I don't really have an opinion if "qcom,use-guard-pages" is a good
> > way to describe this in the DT. For the implementation side feel free to
> > add my
> > 
> 
> Right, I don't think I commented on your suggestion to make the size of
> the guard page configurable. I am not aware of any current or upcoming
> reasons for adding such complexity, so I'd simply prefer to stick with a
> boolean. Should that need arise, I think this model would allow
> extension to express that.
> 

I must admit I forgot that I suggested this until now. :')
I don't see a use case for a different "guard size" either so I think
it's fine to have it as a bool.

Thanks,
Stephan

Re: [PATCH v3 2/3] soc: qcom: rmtfs: Support discarding guard pages

[Bjorn Andersson]

On Fri, Sep 22, 2023 at 09:35:00AM +0200, Stephan Gerhold wrote:
> eOn Thu, Sep 21, 2023 at 07:51:42PM -0700, Bjorn Andersson wrote:
> > On Thu, Sep 21, 2023 at 08:04:06PM +0200, Stephan Gerhold wrote:
> > > On Wed, Sep 20, 2023 at 07:37:31PM -0700, Bjorn Andersson wrote:
> > > > In some configurations, the exact placement of the rmtfs shared memory
> > > > region isn't so strict. The DeviceTree author can then choose to use the
> > > > "size" property and rely on the OS for placement (in combination with
> > > > "alloc-ranges", if desired).
> > > > 
> > > > But on some platforms the rmtfs memory region may not be allocated
> > > > adjacent to regions allocated by other clients. Add support for
> > > > discarding the first and last 4k block in the region, if
> > > > qcom,use-guard-pages is specified in DeviceTree.
> > > > 
> > > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > > > ---
> > > >  drivers/soc/qcom/rmtfs_mem.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
> > > > index f83811f51175..83bba9321e72 100644
> > > > --- a/drivers/soc/qcom/rmtfs_mem.c
> > > > +++ b/drivers/soc/qcom/rmtfs_mem.c
> > > > @@ -200,6 +200,15 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
> > > >  	rmtfs_mem->client_id = client_id;
> > > >  	rmtfs_mem->size = rmem->size;
> > > >  
> > > > +	/*
> > > > +	 * If requested, discard the first and last 4k block in order to ensure
> > > > +	 * that the rmtfs region isn't adjacent to other protected regions.
> > > > +	 */
> > > > +	if (of_property_present(node, "qcom,use-guard-pages")) {
> > > 
> > > I think of_property_read_bool() would be more fitting here. Right now
> > > of_property_present() is just a wrapper around of_property_read_bool().
> > > Semantically reading a bool fits better here though. :-)
> > > 
> > 
> > Are you saying that you would prefer this to be a bool, so hat you can
> > give it a "false" value? Or you are simply saying "it walks like a
> > boolean, quacks like a boolean, let's use the boolean accessor"?
> > 
> 
> The latter. I would expect that of_property_present() is used for
> properties which usually have a value, while of_property_read_bool()
> is used for pure bool values which can be present or not but must not
> have a value. I think a "bool" in terms of DT is simply a present or
> not-present property without any value?
> 
> For example consider
> 
>   regulator-min-microvolts = <4200000000>;
>   regulator-always-on;
> 
> Then I would expect
> 
>   - of_property_present(..., "regulator-min-microvolts"), but
>   - of_property_read_bool(..., "regulator-always-on")
> 
> Does that make sense? :D
> 

Certainly, of_property_read_duck() it is.

Thanks,
Bjorn

[PATCH v3 3/3] soc: qcom: rtmfs: Handle reserved-memory allocation issues

[Bjorn Andersson]

In the even that Linux failed to allocate the reserved memory range
specified in the DeviceTree, the size of the reserved_mem will be 0,
which results in a oops when memory remapping is attempted.

Detect this and report that the memory region was not found instead.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/soc/qcom/rmtfs_mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
index 83bba9321e72..13823abd85c2 100644
--- a/drivers/soc/qcom/rmtfs_mem.c
+++ b/drivers/soc/qcom/rmtfs_mem.c
@@ -180,7 +180,7 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
 	int ret, i;
 
 	rmem = of_reserved_mem_lookup(node);
-	if (!rmem) {
+	if (!rmem || !rmem->size) {
 		dev_err(&pdev->dev, "failed to acquire memory region\n");
 		return -EINVAL;
 	}

-- 
2.25.1

Re: [PATCH v3 3/3] soc: qcom: rtmfs: Handle reserved-memory allocation issues

[Konrad Dybcio]

On 9/21/23 04:37, Bjorn Andersson wrote:
> In the even that Linux failed to allocate the reserved memory range
> specified in the DeviceTree, the size of the reserved_mem will be 0,
> which results in a oops when memory remapping is attempted.
> 
> Detect this and report that the memory region was not found instead.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
typo in subject: rtmfs

Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

Re: [PATCH v3 3/3] soc: qcom: rtmfs: Handle reserved-memory allocation issues

[Stephan Gerhold]

On Wed, Sep 20, 2023 at 07:37:32PM -0700, Bjorn Andersson wrote:
> In the even that Linux failed to allocate the reserved memory range
> specified in the DeviceTree, the size of the reserved_mem will be 0,
> which results in a oops when memory remapping is attempted.
> 
> Detect this and report that the memory region was not found instead.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>

I dropped these checks in my remoteproc patches because Caleb suggested
maybe putting this check directly in of_reserved_mem_lookup() (or
similar) given that almost none of the users verify this [1].

Do you have any opinion on that? I asked back then too but you did not
reply yet [2]. :-)

[1]: https://lore.kernel.org/linux-arm-msm/c3f59fb4-4dd8-f27a-d3f5-b1870006a75c@linaro.org/
[2]: https://lore.kernel.org/linux-arm-msm/ZIsld-MAdkKvdzTx@gerhold.net/

> ---
>  drivers/soc/qcom/rmtfs_mem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
> index 83bba9321e72..13823abd85c2 100644
> --- a/drivers/soc/qcom/rmtfs_mem.c
> +++ b/drivers/soc/qcom/rmtfs_mem.c
> @@ -180,7 +180,7 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
>  	int ret, i;
>  
>  	rmem = of_reserved_mem_lookup(node);
> -	if (!rmem) {
> +	if (!rmem || !rmem->size) {
>  		dev_err(&pdev->dev, "failed to acquire memory region\n");
>  		return -EINVAL;
>  	}
> 
> -- 
> 2.25.1
> 

Re: [PATCH v3 3/3] soc: qcom: rtmfs: Handle reserved-memory allocation issues

[Bjorn Andersson]

On Thu, Sep 21, 2023 at 08:11:23PM +0200, Stephan Gerhold wrote:
> On Wed, Sep 20, 2023 at 07:37:32PM -0700, Bjorn Andersson wrote:
> > In the even that Linux failed to allocate the reserved memory range
> > specified in the DeviceTree, the size of the reserved_mem will be 0,
> > which results in a oops when memory remapping is attempted.
> > 
> > Detect this and report that the memory region was not found instead.
> > 
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> 
> I dropped these checks in my remoteproc patches because Caleb suggested
> maybe putting this check directly in of_reserved_mem_lookup() (or
> similar) given that almost none of the users verify this [1].
> 
> Do you have any opinion on that? I asked back then too but you did not
> reply yet [2]. :-)
> 

I'm struggling to come up with a use case where one would like to get
hold of the rmem when it wasn't properly initialized. So, let's make an
attempt at returning NULL from of_reserved_mem_lookup() instead.

Thanks,
Bjorn

> [1]: https://lore.kernel.org/linux-arm-msm/c3f59fb4-4dd8-f27a-d3f5-b1870006a75c@linaro.org/
> [2]: https://lore.kernel.org/linux-arm-msm/ZIsld-MAdkKvdzTx@gerhold.net/
> 
> > ---
> >  drivers/soc/qcom/rmtfs_mem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
> > index 83bba9321e72..13823abd85c2 100644
> > --- a/drivers/soc/qcom/rmtfs_mem.c
> > +++ b/drivers/soc/qcom/rmtfs_mem.c
> > @@ -180,7 +180,7 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
> >  	int ret, i;
> >  
> >  	rmem = of_reserved_mem_lookup(node);
> > -	if (!rmem) {
> > +	if (!rmem || !rmem->size) {
> >  		dev_err(&pdev->dev, "failed to acquire memory region\n");
> >  		return -EINVAL;
> >  	}
> > 
> > -- 
> > 2.25.1
> > 

Re: (subset) [PATCH v3 0/3] soc: qcom: rmtfs: Support dynamic allocation

[Bjorn Andersson]

On Wed, 20 Sep 2023 19:37:29 -0700, Bjorn Andersson wrote:
> Some platforms have laxed requirements on the placement of the rmtfs
> memory region, introduce support for guard pages to allow the DeviceTree
> author to rely on the OS/Linux for placement of the region.
> 
> Changes since v2:
> - Rewrote DeviceTree binding description, to avoid dictating the OS
>   behavior.
> - Adjusted addr and size before memremap(), to avoid mapping the guard
>   pages, and unnecessarily have to adjust the base pointer.
> 
> [...]

Applied, thanks!

[1/3] dt-bindings: reserved-memory: rmtfs: Allow guard pages
      commit: 3ad96787949a96256931ca59aff73ea604bc0e6c
[2/3] soc: qcom: rmtfs: Support discarding guard pages
      commit: 9265bc6bce6919c771970e5a425a66551a1c78a0

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>