All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/3] dt-bindings: reserved-memory: ramoops: Update the binding
@ 2023-01-13 11:58 Mukesh Ojha
  2023-01-13 11:58 ` [PATCH v2 2/3] Documentation: admin-guide: ramoops.rst: Update the ramoops document Mukesh Ojha
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Mukesh Ojha @ 2023-01-13 11:58 UTC (permalink / raw)
  To: keescook, gpiccoli, corbet, tony.luck, robh+dt, krzysztof.kozlowski+dt
  Cc: linux-hardening, linux-doc, linux-kernel, devicetree, Mukesh Ojha

Update the ramoops region binding document with details
like region can also be reserved dynamically apart from
reserving it statically.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
Change in v2:
  - Added this patch as per changes going to be done in patch 3/3

 .../bindings/reserved-memory/ramoops.yaml          | 34 ++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
index 0391871..54e46e8 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]
@@ -142,3 +149,26 @@ examples:
             };
         };
     };
+
+  - |
+    / {
+        compatible = "foo";
+        model = "foo";
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        reserved-memory {
+            #address-cells = <2>;
+            #size-cells = <2>;
+            ranges;
+
+            ramoops: ramoops_region {
+                compatible = "ramoops";
+                alloc-ranges = <0x0 0x00000000 0xffffffff 0xffffffff>;
+                size = <0x0 0x10000>;       /* 64kB */
+                console-size = <0x8000>;    /* 32kB */
+                record-size = <0x400>;      /*  1kB */
+                ecc-size = <16>;
+            };
+        };
+    };
-- 
2.7.4


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

* [PATCH v2 2/3] Documentation: admin-guide: ramoops.rst: Update the ramoops document
  2023-01-13 11:58 [PATCH v2 1/3] dt-bindings: reserved-memory: ramoops: Update the binding Mukesh Ojha
@ 2023-01-13 11:58 ` Mukesh Ojha
  2023-01-13 13:24   ` Mukesh Ojha
  2023-01-13 11:58 ` [PATCH v3 3/3] pstore/ram: Rework logic for detecting ramoops Mukesh Ojha
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Mukesh Ojha @ 2023-01-13 11:58 UTC (permalink / raw)
  To: keescook, gpiccoli, corbet, tony.luck, robh+dt, krzysztof.kozlowski+dt
  Cc: linux-hardening, linux-doc, linux-kernel, devicetree, 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 environments where it is preferred the region to be dynamically
allocated at runtime. So, update the document while adding the
support in the driver.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
Change in v2:
  - Added this patch as per changes going to be done in patch 3/3

 Documentation/admin-guide/ramoops.rst | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/Documentation/admin-guide/ramoops.rst b/Documentation/admin-guide/ramoops.rst
index e9f8514..88884b2 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 = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		ramoops@8f000000 {
+			compatible = "ramoops";
+			alloc-ranges = <0x0 0x00000000 0xffffffff 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:
 
-- 
2.7.4


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

* [PATCH v3 3/3] pstore/ram: Rework logic for detecting ramoops
  2023-01-13 11:58 [PATCH v2 1/3] dt-bindings: reserved-memory: ramoops: Update the binding Mukesh Ojha
  2023-01-13 11:58 ` [PATCH v2 2/3] Documentation: admin-guide: ramoops.rst: Update the ramoops document Mukesh Ojha
@ 2023-01-13 11:58 ` Mukesh Ojha
  2023-01-13 12:04 ` [PATCH v2 1/3] dt-bindings: reserved-memory: ramoops: Update the binding Krzysztof Kozlowski
  2023-01-13 15:26 ` Rob Herring
  3 siblings, 0 replies; 9+ messages in thread
From: Mukesh Ojha @ 2023-01-13 11:58 UTC (permalink / raw)
  To: keescook, gpiccoli, corbet, tony.luck, robh+dt, krzysztof.kozlowski+dt
  Cc: linux-hardening, linux-doc, linux-kernel, devicetree, 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 environments 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() when platform_get_resource() failed.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
Changes in v2:
 - Addressed the comments made by kees and Guilherme in v1.

 fs/pstore/ram.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

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] 9+ messages in thread

* Re: [PATCH v2 1/3] dt-bindings: reserved-memory: ramoops: Update the binding
  2023-01-13 11:58 [PATCH v2 1/3] dt-bindings: reserved-memory: ramoops: Update the binding Mukesh Ojha
  2023-01-13 11:58 ` [PATCH v2 2/3] Documentation: admin-guide: ramoops.rst: Update the ramoops document Mukesh Ojha
  2023-01-13 11:58 ` [PATCH v3 3/3] pstore/ram: Rework logic for detecting ramoops Mukesh Ojha
@ 2023-01-13 12:04 ` Krzysztof Kozlowski
  2023-01-13 13:22   ` Mukesh Ojha
  2023-01-27 12:29   ` Mukesh Ojha
  2023-01-13 15:26 ` Rob Herring
  3 siblings, 2 replies; 9+ messages in thread
From: Krzysztof Kozlowski @ 2023-01-13 12:04 UTC (permalink / raw)
  To: Mukesh Ojha, keescook, gpiccoli, corbet, tony.luck, robh+dt,
	krzysztof.kozlowski+dt
  Cc: linux-hardening, linux-doc, linux-kernel, devicetree

Subject: drop second/last, redundant "binding". The "dt-bindings" prefix
is already stating that these are bindings.

Your subject says nothing. Everything is "update".

On 13/01/2023 12:58, Mukesh Ojha wrote:
> Update the ramoops region binding document with details
> like region can also be reserved dynamically apart from
> reserving it statically.

So what exactly can be here reserved dynamically? And what does it mean
'dynamically'? By whom? How is this property of hardware (not OS)?

> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
> Change in v2:
>   - Added this patch as per changes going to be done in patch 3/3
> 
>  .../bindings/reserved-memory/ramoops.yaml          | 34 ++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
> index 0391871..54e46e8 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

There is no such property. You cannot require it.

>  
>  anyOf:
>    - required: [record-size]
> @@ -142,3 +149,26 @@ examples:
>              };
>          };
>      };
> +
> +  - |
> +    / {
> +        compatible = "foo";
> +        model = "foo";
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        reserved-memory {
> +            #address-cells = <2>;
> +            #size-cells = <2>;
> +            ranges;
> +
> +            ramoops: ramoops_region {

Node names should be generic, no underscores in node names.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Any reason in naming it differently then existing one? You have there
example.

> +                compatible = "ramoops";
> +                alloc-ranges = <0x0 0x00000000 0xffffffff 0xffffffff>;
> +                size = <0x0 0x10000>;       /* 64kB */
> +                console-size = <0x8000>;    /* 32kB */
> +                record-size = <0x400>;      /*  1kB */
> +                ecc-size = <16>;
> +            };
> +        };
> +    };

Best regards,
Krzysztof


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

* Re: [PATCH v2 1/3] dt-bindings: reserved-memory: ramoops: Update the binding
  2023-01-13 12:04 ` [PATCH v2 1/3] dt-bindings: reserved-memory: ramoops: Update the binding Krzysztof Kozlowski
@ 2023-01-13 13:22   ` Mukesh Ojha
  2023-01-27 12:29   ` Mukesh Ojha
  1 sibling, 0 replies; 9+ messages in thread
From: Mukesh Ojha @ 2023-01-13 13:22 UTC (permalink / raw)
  To: Krzysztof Kozlowski, keescook, gpiccoli, corbet, tony.luck,
	robh+dt, krzysztof.kozlowski+dt
  Cc: linux-hardening, linux-doc, linux-kernel, devicetree

Hi Krzysztof,

On 1/13/2023 5:34 PM, Krzysztof Kozlowski wrote:
> Subject: drop second/last, redundant "binding". The "dt-bindings" prefix
> is already stating that these are bindings.
> 
> Your subject says nothing. Everything is "update".
> 

I will fix this.

> On 13/01/2023 12:58, Mukesh Ojha wrote:
>> Update the ramoops region binding document with details
>> like region can also be reserved dynamically apart from
>> reserving it statically.
>  > So what exactly can be here reserved dynamically? And what does it mean
> 'dynamically'? By whom? How is this property of hardware (not OS)?
> 
Normally, for ramoops, already known physical address range from DDR are 
mentioned in Device tree which gets reserved from reserved-memory kernel 
framework(example 1 in this file).

By being dynamic, I meant, with this we are letting the framework to get 
the size from a range of allowable address (<0x0 0x00000000 0xffffffff 
0xffffffff>).

Let me know, if you need more detail.

>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>> Change in v2:
>>    - Added this patch as per changes going to be done in patch 3/3
>>
>>   .../bindings/reserved-memory/ramoops.yaml          | 34 ++++++++++++++++++++--
>>   1 file changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
>> index 0391871..54e46e8 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
> 
> There is no such property. You cannot require it.

Forgot to mention this.

--- a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
+++ b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
@@ -37,6 +37,20 @@ properties:
    reg:
      description: region of memory that is preserved between reboots

+  size:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    maxItems: 2
+    description: >
+      Length based on parent's \#size-cells. Size in bytes of memory to
+      reserve.
+
+  alloc-ranges:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description: >
+      Address and Length pairs. Specifies regions of memory that are
+      acceptable to allocate from.
+
    ecc-size:
      $ref: /schemas/types.yaml#/definitions/uint32
      description: enables ECC support and specifies ECC buffer size in 
bytes
@@ -119,6 +133,7 @@ oneOf:
        - reg

    - required:
+      - alloc-ranges
        - size
> 
>>   
>>   anyOf:
>>     - required: [record-size]
>> @@ -142,3 +149,26 @@ examples:
>>               };
>>           };
>>       };
>> +
>> +  - |
>> +    / {
>> +        compatible = "foo";
>> +        model = "foo";
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
>> +        reserved-memory {
>> +            #address-cells = <2>;
>> +            #size-cells = <2>;
>> +            ranges;
>> +
>> +            ramoops: ramoops_region {
> 
> Node names should be generic, no underscores in node names.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 

Thanks.

> Any reason in naming it differently then existing one? You have there
> example.

ramoops@bfdf0000 is not valid after dynamic allocation of memory.
Probably, will mention it as

ramoops_region: ramoops { ??

-Mukesh
> 
>> +                compatible = "ramoops";
>> +                alloc-ranges = <0x0 0x00000000 0xffffffff 0xffffffff>;
>> +                size = <0x0 0x10000>;       /* 64kB */
>> +                console-size = <0x8000>;    /* 32kB */
>> +                record-size = <0x400>;      /*  1kB */
>> +                ecc-size = <16>;
>> +            };
>> +        };
>> +    };
> 
> Best regards,
> Krzysztof
> 

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

* Re: [PATCH v2 2/3] Documentation: admin-guide: ramoops.rst: Update the ramoops document
  2023-01-13 11:58 ` [PATCH v2 2/3] Documentation: admin-guide: ramoops.rst: Update the ramoops document Mukesh Ojha
@ 2023-01-13 13:24   ` Mukesh Ojha
  0 siblings, 0 replies; 9+ messages in thread
From: Mukesh Ojha @ 2023-01-13 13:24 UTC (permalink / raw)
  To: keescook, gpiccoli, corbet, tony.luck, robh+dt, krzysztof.kozlowski+dt
  Cc: linux-hardening, linux-doc, linux-kernel, devicetree

Hi,

On 1/13/2023 5:28 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 environments where it is preferred the region to be dynamically
> allocated at runtime. So, update the document while adding the
> support in the driver.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
> Change in v2:
>    - Added this patch as per changes going to be done in patch 3/3
> 
>   Documentation/admin-guide/ramoops.rst | 25 ++++++++++++++++++++++---
>   1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/admin-guide/ramoops.rst b/Documentation/admin-guide/ramoops.rst
> index e9f8514..88884b2 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 = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		ramoops@8f000000 {

Will fix it as ramoops_region : ramoops ?

> +			compatible = "ramoops";
> +			alloc-ranges = <0x0 0x00000000 0xffffffff 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:
>   

-Mukesh

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

* Re: [PATCH v2 1/3] dt-bindings: reserved-memory: ramoops: Update the binding
  2023-01-13 11:58 [PATCH v2 1/3] dt-bindings: reserved-memory: ramoops: Update the binding Mukesh Ojha
                   ` (2 preceding siblings ...)
  2023-01-13 12:04 ` [PATCH v2 1/3] dt-bindings: reserved-memory: ramoops: Update the binding Krzysztof Kozlowski
@ 2023-01-13 15:26 ` Rob Herring
  3 siblings, 0 replies; 9+ messages in thread
From: Rob Herring @ 2023-01-13 15:26 UTC (permalink / raw)
  To: Mukesh Ojha
  Cc: tony.luck, keescook, linux-hardening, linux-doc, corbet,
	krzysztof.kozlowski+dt, devicetree, gpiccoli, robh+dt,
	linux-kernel


On Fri, 13 Jan 2023 17:28:44 +0530, Mukesh Ojha wrote:
> Update the ramoops region binding document with details
> like region can also be reserved dynamically apart from
> reserving it statically.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
> Change in v2:
>   - Added this patch as per changes going to be done in patch 3/3
> 
>  .../bindings/reserved-memory/ramoops.yaml          | 34 ++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/reserved-memory/ramoops.example.dts:17.13-40: Warning (reg_format): /reserved-memory/ramoops@bfdf0000:reg: property has invalid length (8 bytes) (#address-cells == 2, #size-cells == 2)
Documentation/devicetree/bindings/reserved-memory/ramoops.example.dtb: Warning (pci_device_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/reserved-memory/ramoops.example.dtb: Warning (pci_device_bus_num): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/reserved-memory/ramoops.example.dtb: Warning (simple_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/reserved-memory/ramoops.example.dtb: Warning (i2c_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/reserved-memory/ramoops.example.dtb: Warning (spi_bus_reg): Failed prerequisite 'reg_format'
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/reserved-memory/ramoops.example.dtb: reserved-memory: ramoops@bfdf0000:reg:0: [3219062784, 65536] is too short
	From schema: /usr/local/lib/python3.10/dist-packages/dtschema/schemas/reg.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1673611126-13803-1-git-send-email-quic_mojha@quicinc.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.


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

* Re: [PATCH v2 1/3] dt-bindings: reserved-memory: ramoops: Update the binding
  2023-01-13 12:04 ` [PATCH v2 1/3] dt-bindings: reserved-memory: ramoops: Update the binding Krzysztof Kozlowski
  2023-01-13 13:22   ` Mukesh Ojha
@ 2023-01-27 12:29   ` Mukesh Ojha
  2023-01-27 12:52     ` Krzysztof Kozlowski
  1 sibling, 1 reply; 9+ messages in thread
From: Mukesh Ojha @ 2023-01-27 12:29 UTC (permalink / raw)
  To: Krzysztof Kozlowski, keescook, gpiccoli, corbet, tony.luck,
	robh+dt, krzysztof.kozlowski+dt
  Cc: linux-hardening, linux-doc, linux-kernel, devicetree

Hi,

On 1/13/2023 5:34 PM, Krzysztof Kozlowski wrote:
> Subject: drop second/last, redundant "binding". The "dt-bindings" prefix
> is already stating that these are bindings.
> 
> Your subject says nothing. Everything is "update".
> 
> On 13/01/2023 12:58, Mukesh Ojha wrote:
>> Update the ramoops region binding document with details
>> like region can also be reserved dynamically apart from
>> reserving it statically.
> 
> So what exactly can be here reserved dynamically? And what does it mean
> 'dynamically'? By whom? How is this property of hardware (not OS)?
> 
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>> Change in v2:
>>    - Added this patch as per changes going to be done in patch 3/3
>>
>>   .../bindings/reserved-memory/ramoops.yaml          | 34 ++++++++++++++++++++--
>>   1 file changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
>> index 0391871..54e46e8 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
> 
> There is no such property. You cannot require it.

I was thinking, since this size is part reserved-memory.yaml and
we have

allOf:
   - $ref: "reserved-memory.yaml"

Is your comment still applies?

-Mukesh
> 
>>   
>>   anyOf:
>>     - required: [record-size]
>> @@ -142,3 +149,26 @@ examples:
>>               };
>>           };
>>       };
>> +
>> +  - |
>> +    / {
>> +        compatible = "foo";
>> +        model = "foo";
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
>> +        reserved-memory {
>> +            #address-cells = <2>;
>> +            #size-cells = <2>;
>> +            ranges;
>> +
>> +            ramoops: ramoops_region {
> 
> Node names should be generic, no underscores in node names.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 
> Any reason in naming it differently then existing one? You have there
> example.
> 
>> +                compatible = "ramoops";
>> +                alloc-ranges = <0x0 0x00000000 0xffffffff 0xffffffff>;
>> +                size = <0x0 0x10000>;       /* 64kB */
>> +                console-size = <0x8000>;    /* 32kB */
>> +                record-size = <0x400>;      /*  1kB */
>> +                ecc-size = <16>;
>> +            };
>> +        };
>> +    };
> 
> Best regards,
> Krzysztof
> 

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

* Re: [PATCH v2 1/3] dt-bindings: reserved-memory: ramoops: Update the binding
  2023-01-27 12:29   ` Mukesh Ojha
@ 2023-01-27 12:52     ` Krzysztof Kozlowski
  0 siblings, 0 replies; 9+ messages in thread
From: Krzysztof Kozlowski @ 2023-01-27 12:52 UTC (permalink / raw)
  To: Mukesh Ojha, keescook, gpiccoli, corbet, tony.luck, robh+dt,
	krzysztof.kozlowski+dt
  Cc: linux-hardening, linux-doc, linux-kernel, devicetree

On 27/01/2023 13:29, Mukesh Ojha wrote:
>>> +oneOf:
>>> +  - required:
>>> +      - reg
>>> +
>>> +  - required:
>>> +      - size
>>
>> There is no such property. You cannot require it.
> 
> I was thinking, since this size is part reserved-memory.yaml and
> we have
> 
> allOf:
>    - $ref: "reserved-memory.yaml"
> 
> Is your comment still applies?

Ah, you are right. It's ok.


Best regards,
Krzysztof


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

end of thread, other threads:[~2023-01-27 12:53 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-13 11:58 [PATCH v2 1/3] dt-bindings: reserved-memory: ramoops: Update the binding Mukesh Ojha
2023-01-13 11:58 ` [PATCH v2 2/3] Documentation: admin-guide: ramoops.rst: Update the ramoops document Mukesh Ojha
2023-01-13 13:24   ` Mukesh Ojha
2023-01-13 11:58 ` [PATCH v3 3/3] pstore/ram: Rework logic for detecting ramoops Mukesh Ojha
2023-01-13 12:04 ` [PATCH v2 1/3] dt-bindings: reserved-memory: ramoops: Update the binding Krzysztof Kozlowski
2023-01-13 13:22   ` Mukesh Ojha
2023-01-27 12:29   ` Mukesh Ojha
2023-01-27 12:52     ` Krzysztof Kozlowski
2023-01-13 15:26 ` Rob Herring

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.