linux-doc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 1/2] dt-bindings: ramoops: Add support to get the region dynamically
@ 2023-02-02  9:28 Mukesh Ojha
  2023-02-02  9:28 ` [PATCH v5 2/2] pstore/ram: Rework logic for detecting ramoops Mukesh Ojha
  2023-02-02 23:59 ` [PATCH v5 1/2] dt-bindings: ramoops: Add support to get the region dynamically Rob Herring
  0 siblings, 2 replies; 6+ messages in thread
From: Mukesh Ojha @ 2023-02-02  9:28 UTC (permalink / raw)
  To: linux-hardening, linux-doc, linux-kernel, devicetree
  Cc: keescook, tony.luck, gpiccoli, robh+dt, krzysztof.kozlowski+dt,
	corbet, Mukesh Ojha

The reserved memory region for ramoops is assumed to be at a
fixed and known location when read from the devicetree. This
is not desirable in an environment where it is preferred the
region to be dynamically allocated at runtime, as opposed to
being fixed at compile time.

So, update the ramoops binding by using some reserve memory
property to allocate the ramoops region dynamically.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
Changes in v5:
 - Updated the commit description.
 - Removed example from yaml file.

Changes in v4:
 - Addressed comment made by Krzysztof on ramoops node name.

Changes in v3:
 - Fixed yaml error and updated commit text as per comment.

Change in v2:
  - Added this patch as per changes going to be done in patch 3/3

 .../devicetree/bindings/reserved-memory/ramoops.yaml          | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
index 0391871..51b6003 100644
--- a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
+++ b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
@@ -10,7 +10,8 @@ description: |
   ramoops provides persistent RAM storage for oops and panics, so they can be
   recovered after a reboot. This is a child-node of "/reserved-memory", and
   is named "ramoops" after the backend, rather than "pstore" which is the
-  subsystem.
+  subsystem. This region can be reserved both statically or dynamically by
+  using appropriate property in device tree.
 
   Parts of this storage may be set aside for other persistent log buffers, such
   as kernel log messages, or for optional ECC error-correction data.  The total
@@ -112,7 +113,13 @@ unevaluatedProperties: false
 
 required:
   - compatible
-  - reg
+
+oneOf:
+  - required:
+      - reg
+
+  - required:
+      - size
 
 anyOf:
   - required: [record-size]
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v5 2/2] pstore/ram: Rework logic for detecting ramoops
  2023-02-02  9:28 [PATCH v5 1/2] dt-bindings: ramoops: Add support to get the region dynamically Mukesh Ojha
@ 2023-02-02  9:28 ` Mukesh Ojha
  2023-02-14 13:44   ` Mukesh Ojha
  2023-02-02 23:59 ` [PATCH v5 1/2] dt-bindings: ramoops: Add support to get the region dynamically Rob Herring
  1 sibling, 1 reply; 6+ messages in thread
From: Mukesh Ojha @ 2023-02-02  9:28 UTC (permalink / raw)
  To: linux-hardening, linux-doc, linux-kernel, devicetree
  Cc: keescook, tony.luck, gpiccoli, robh+dt, krzysztof.kozlowski+dt,
	corbet, Mukesh Ojha

The reserved memory region for ramoops is assumed to be at a fixed
and known location when read from the devicetree. This is not desirable
in an environment where it is preferred the region to be dynamically
allocated at runtime, as opposed to being fixed at compile time.

Also, some of the platforms might be still expecting dedicated
memory region for ramoops node where the region is known beforehand
and platform_get_resource() is used in that case.

So, add logic to detect the start and size of the ramoops memory
region by looking up reserved memory region with of_reserved_mem_lookup()
api when platform_get_resource() fails also update the ramoops
documentation,

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
Changes in v5:
 - Removed the CC list from the commit text.

Changes in v4:
 - Updated the minor change in documentation.

Changes in v3:
 - Merged 2/3 and 3/3 into one.
   https://lore.kernel.org/lkml/1673611126-13803-2-git-send-email-quic_mojha@quicinc.com/
   https://lore.kernel.org/lkml/1673611126-13803-3-git-send-email-quic_mojha@quicinc.com/

Changes in v2:
 - Addressed the comments made by kees and Guilherme in v1.


 Documentation/admin-guide/ramoops.rst | 25 ++++++++++++++++++++++---
 fs/pstore/ram.c                       | 18 +++++++++++++-----
 2 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/Documentation/admin-guide/ramoops.rst b/Documentation/admin-guide/ramoops.rst
index e9f8514..3586d15 100644
--- a/Documentation/admin-guide/ramoops.rst
+++ b/Documentation/admin-guide/ramoops.rst
@@ -16,8 +16,9 @@ survive after a restart.
 Ramoops concepts
 ----------------
 
-Ramoops uses a predefined memory area to store the dump. The start and size
-and type of the memory area are set using three variables:
+Ramoops uses both predefined and dynamically memory area to store the dump.
+The start and size and type of the memory area are set using three
+variables:
 
   * ``mem_address`` for the start
   * ``mem_size`` for the size. The memory size will be rounded down to a
@@ -70,7 +71,8 @@ Setting the ramoops parameters can be done in several different manners:
 
  B. Use Device Tree bindings, as described in
  ``Documentation/devicetree/bindings/reserved-memory/ramoops.yaml``.
- For example::
+
+ Example of statically reserved ramoops region::
 
 	reserved-memory {
 		#address-cells = <2>;
@@ -85,6 +87,23 @@ Setting the ramoops parameters can be done in several different manners:
 		};
 	};
 
+ Example of dynamically reserved ramoops region::
+
+	reserved-memory {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+
+		ramoops_region: ramoops {
+			compatible = "ramoops";
+			alloc-ranges = <0x00000000 0xffffffff>;
+			size = <0 0x100000>;
+			record-size = <0x4000>;
+			console-size = <0x4000>;
+		};
+	};
+
+
  C. Use a platform device and set the platform data. The parameters can then
  be set through that platform data. An example of doing that is:
 
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ade66db..17c9f46 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -20,6 +20,7 @@
 #include <linux/compiler.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_reserved_mem.h>
 
 #include "internal.h"
 #include "ram_internal.h"
@@ -643,6 +644,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
 {
 	struct device_node *of_node = pdev->dev.of_node;
 	struct device_node *parent_node;
+	struct reserved_mem *rmem;
 	struct resource *res;
 	u32 value;
 	int ret;
@@ -651,13 +653,19 @@ static int ramoops_parse_dt(struct platform_device *pdev,
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
-		dev_err(&pdev->dev,
-			"failed to locate DT /reserved-memory resource\n");
-		return -EINVAL;
+		rmem = of_reserved_mem_lookup(of_node);
+		if (!rmem) {
+			dev_err(&pdev->dev,
+				"failed to locate DT /reserved-memory resource\n");
+			return -EINVAL;
+		}
+		pdata->mem_size = rmem->size;
+		pdata->mem_address = rmem->base;
+	} else {
+		pdata->mem_size = resource_size(res);
+		pdata->mem_address = res->start;
 	}
 
-	pdata->mem_size = resource_size(res);
-	pdata->mem_address = res->start;
 	/*
 	 * Setting "unbuffered" is deprecated and will be ignored if
 	 * "mem_type" is also specified.
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v5 1/2] dt-bindings: ramoops: Add support to get the region dynamically
  2023-02-02  9:28 [PATCH v5 1/2] dt-bindings: ramoops: Add support to get the region dynamically Mukesh Ojha
  2023-02-02  9:28 ` [PATCH v5 2/2] pstore/ram: Rework logic for detecting ramoops Mukesh Ojha
@ 2023-02-02 23:59 ` Rob Herring
  2023-02-03  8:20   ` Mukesh Ojha
  1 sibling, 1 reply; 6+ messages in thread
From: Rob Herring @ 2023-02-02 23:59 UTC (permalink / raw)
  To: Mukesh Ojha
  Cc: linux-hardening, linux-doc, linux-kernel, devicetree, keescook,
	tony.luck, gpiccoli, krzysztof.kozlowski+dt, corbet

On Thu, Feb 02, 2023 at 02:58:00PM +0530, Mukesh Ojha wrote:
> The reserved memory region for ramoops is assumed to be at a
> fixed and known location when read from the devicetree. This
> is not desirable in an environment where it is preferred the
> region to be dynamically allocated at runtime, as opposed to
> being fixed at compile time.
> 
> So, update the ramoops binding by using some reserve memory
> property to allocate the ramoops region dynamically.

Sorry, but I still don't think this belongs in DT as I commented on v4.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v5 1/2] dt-bindings: ramoops: Add support to get the region dynamically
  2023-02-02 23:59 ` [PATCH v5 1/2] dt-bindings: ramoops: Add support to get the region dynamically Rob Herring
@ 2023-02-03  8:20   ` Mukesh Ojha
  2023-02-03 14:59     ` Rob Herring
  0 siblings, 1 reply; 6+ messages in thread
From: Mukesh Ojha @ 2023-02-03  8:20 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-hardening, linux-doc, linux-kernel, devicetree, keescook,
	tony.luck, gpiccoli, krzysztof.kozlowski+dt, corbet



On 2/3/2023 5:29 AM, Rob Herring wrote:
> On Thu, Feb 02, 2023 at 02:58:00PM +0530, Mukesh Ojha wrote:
>> The reserved memory region for ramoops is assumed to be at a
>> fixed and known location when read from the devicetree. This
>> is not desirable in an environment where it is preferred the
>> region to be dynamically allocated at runtime, as opposed to
>> being fixed at compile time.
>>
>> So, update the ramoops binding by using some reserve memory
>> property to allocate the ramoops region dynamically.
> 
> Sorry, but I still don't think this belongs in DT as I commented on v4
Do you mean, we should not even document this here ? or are you against 
the size property mentioned in this patch.

-Mukesh

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v5 1/2] dt-bindings: ramoops: Add support to get the region dynamically
  2023-02-03  8:20   ` Mukesh Ojha
@ 2023-02-03 14:59     ` Rob Herring
  0 siblings, 0 replies; 6+ messages in thread
From: Rob Herring @ 2023-02-03 14:59 UTC (permalink / raw)
  To: Mukesh Ojha
  Cc: linux-hardening, linux-doc, linux-kernel, devicetree, keescook,
	tony.luck, gpiccoli, krzysztof.kozlowski+dt, corbet

On Fri, Feb 3, 2023 at 2:20 AM Mukesh Ojha <quic_mojha@quicinc.com> wrote:
>
>
>
> On 2/3/2023 5:29 AM, Rob Herring wrote:
> > On Thu, Feb 02, 2023 at 02:58:00PM +0530, Mukesh Ojha wrote:
> >> The reserved memory region for ramoops is assumed to be at a
> >> fixed and known location when read from the devicetree. This
> >> is not desirable in an environment where it is preferred the
> >> region to be dynamically allocated at runtime, as opposed to
> >> being fixed at compile time.
> >>
> >> So, update the ramoops binding by using some reserve memory
> >> property to allocate the ramoops region dynamically.
> >
> > Sorry, but I still don't think this belongs in DT as I commented on v4
> Do you mean, we should not even document this here ? or are you against
> the size property mentioned in this patch.

I don't think dynamic ramoops location should be defined in DT.

Rob

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v5 2/2] pstore/ram: Rework logic for detecting ramoops
  2023-02-02  9:28 ` [PATCH v5 2/2] pstore/ram: Rework logic for detecting ramoops Mukesh Ojha
@ 2023-02-14 13:44   ` Mukesh Ojha
  0 siblings, 0 replies; 6+ messages in thread
From: Mukesh Ojha @ 2023-02-14 13:44 UTC (permalink / raw)
  To: linux-hardening, linux-doc, linux-kernel, devicetree
  Cc: keescook, tony.luck, gpiccoli, robh+dt, krzysztof.kozlowski+dt, corbet

Hi Kees/Rob,

Since, we are not agreeing to put dynamics ramoops region support in 
device tree.

In Qualcomm SoC, during reset the pstore static region did not get 
preserved across boots and we have our own mechanism to collect regions
if physical address and size is somehow passed to boot-firmware by 
writing to some shared memory. So, we wanted to reuse the region
supported by pstore(dmesg/console/ etc.) for that we wanted to this
flexibility to put this region dynamically anywhere in the ram.

This patch will help achieve the same . Can you suggest if this gets
allowed.

-Mukesh

On 2/2/2023 2:58 PM, Mukesh Ojha wrote:
> The reserved memory region for ramoops is assumed to be at a fixed
> and known location when read from the devicetree. This is not desirable
> in an environment where it is preferred the region to be dynamically
> allocated at runtime, as opposed to being fixed at compile time.
> 
> Also, some of the platforms might be still expecting dedicated
> memory region for ramoops node where the region is known beforehand
> and platform_get_resource() is used in that case.
> 
> So, add logic to detect the start and size of the ramoops memory
> region by looking up reserved memory region with of_reserved_mem_lookup()
> api when platform_get_resource() fails also update the ramoops
> documentation.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > ---
> Changes in v5:
>   - Removed the CC list from the commit text.
> 
> Changes in v4:
>   - Updated the minor change in documentation.
> 
> Changes in v3:
>   - Merged 2/3 and 3/3 into one.
>     https://lore.kernel.org/lkml/1673611126-13803-2-git-send-email-quic_mojha@quicinc.com/
>     https://lore.kernel.org/lkml/1673611126-13803-3-git-send-email-quic_mojha@quicinc.com/
> 
> Changes in v2:
>   - Addressed the comments made by kees and Guilherme in v1.
> 
> 
>   Documentation/admin-guide/ramoops.rst | 25 ++++++++++++++++++++++---
>   fs/pstore/ram.c                       | 18 +++++++++++++-----
>   2 files changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/admin-guide/ramoops.rst b/Documentation/admin-guide/ramoops.rst
> index e9f8514..3586d15 100644
> --- a/Documentation/admin-guide/ramoops.rst
> +++ b/Documentation/admin-guide/ramoops.rst
> @@ -16,8 +16,9 @@ survive after a restart.
>   Ramoops concepts
>   ----------------
>   
> -Ramoops uses a predefined memory area to store the dump. The start and size
> -and type of the memory area are set using three variables:
> +Ramoops uses both predefined and dynamically memory area to store the dump.
> +The start and size and type of the memory area are set using three
> +variables:
>   
>     * ``mem_address`` for the start
>     * ``mem_size`` for the size. The memory size will be rounded down to a
> @@ -70,7 +71,8 @@ Setting the ramoops parameters can be done in several different manners:
>   
>    B. Use Device Tree bindings, as described in
>    ``Documentation/devicetree/bindings/reserved-memory/ramoops.yaml``.
> - For example::
> +
> + Example of statically reserved ramoops region::
>   
>   	reserved-memory {
>   		#address-cells = <2>;
> @@ -85,6 +87,23 @@ Setting the ramoops parameters can be done in several different manners:
>   		};
>   	};
>   
> + Example of dynamically reserved ramoops region::
> +
> +	reserved-memory {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +
> +		ramoops_region: ramoops {
> +			compatible = "ramoops";
> +			alloc-ranges = <0x00000000 0xffffffff>;
> +			size = <0 0x100000>;
> +			record-size = <0x4000>;
> +			console-size = <0x4000>;
> +		};
> +	};
> +
> +
>    C. Use a platform device and set the platform data. The parameters can then
>    be set through that platform data. An example of doing that is:
>   
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index ade66db..17c9f46 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -20,6 +20,7 @@
>   #include <linux/compiler.h>
>   #include <linux/of.h>
>   #include <linux/of_address.h>
> +#include <linux/of_reserved_mem.h>
>   
>   #include "internal.h"
>   #include "ram_internal.h"
> @@ -643,6 +644,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
>   {
>   	struct device_node *of_node = pdev->dev.of_node;
>   	struct device_node *parent_node;
> +	struct reserved_mem *rmem;
>   	struct resource *res;
>   	u32 value;
>   	int ret;
> @@ -651,13 +653,19 @@ static int ramoops_parse_dt(struct platform_device *pdev,
>   
>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   	if (!res) {
> -		dev_err(&pdev->dev,
> -			"failed to locate DT /reserved-memory resource\n");
> -		return -EINVAL;
> +		rmem = of_reserved_mem_lookup(of_node);
> +		if (!rmem) {
> +			dev_err(&pdev->dev,
> +				"failed to locate DT /reserved-memory resource\n");
> +			return -EINVAL;
> +		}
> +		pdata->mem_size = rmem->size;
> +		pdata->mem_address = rmem->base;
> +	} else {
> +		pdata->mem_size = resource_size(res);
> +		pdata->mem_address = res->start;
>   	}
>   
> -	pdata->mem_size = resource_size(res);
> -	pdata->mem_address = res->start;
>   	/*
>   	 * Setting "unbuffered" is deprecated and will be ignored if
>   	 * "mem_type" is also specified.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-02-14 13:46 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-02  9:28 [PATCH v5 1/2] dt-bindings: ramoops: Add support to get the region dynamically Mukesh Ojha
2023-02-02  9:28 ` [PATCH v5 2/2] pstore/ram: Rework logic for detecting ramoops Mukesh Ojha
2023-02-14 13:44   ` Mukesh Ojha
2023-02-02 23:59 ` [PATCH v5 1/2] dt-bindings: ramoops: Add support to get the region dynamically Rob Herring
2023-02-03  8:20   ` Mukesh Ojha
2023-02-03 14:59     ` Rob Herring

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).