diff mbox series

[RFC,v9,2/5] dt-bindings: pstore-block: new support for blkoops

Message ID 1550577170-18761-3-git-send-email-liaoweixiong@allwinnertech.com
State Changes Requested, archived
Headers show
Series pstore/block: new support logger for block devices | expand

Checks

Context Check Description
robh/checkpatch success

Commit Message

WeiXiong Liao Feb. 19, 2019, 11:52 a.m. UTC
Create DT binding document for blkoops.

Signed-off-by: liaoweixiong <liaoweixiong@allwinnertech.com>
---
 .../devicetree/bindings/pstore/blkoops.txt         | 53 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pstore/blkoops.txt

Comments

Rob Herring (Arm) Feb. 22, 2019, 3:36 p.m. UTC | #1
+boot-architecture list

On Tue, Feb 19, 2019 at 07:52:47PM +0800, liaoweixiong wrote:
> Create DT binding document for blkoops.
> 
> Signed-off-by: liaoweixiong <liaoweixiong@allwinnertech.com>
> ---
>  .../devicetree/bindings/pstore/blkoops.txt         | 53 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pstore/blkoops.txt
> 
> diff --git a/Documentation/devicetree/bindings/pstore/blkoops.txt b/Documentation/devicetree/bindings/pstore/blkoops.txt
> new file mode 100644
> index 0000000..5462915
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pstore/blkoops.txt
> @@ -0,0 +1,53 @@
> +Blkoops oops logger
> +===================
> +
> +Blkoops provides a block partition for oops, excluding panics now, so they can
> +be recovered after a reboot.
> +
> +Any space of block device will be used for a circular buffer of oops records.
> +These records have a configurable size, with a size of 0 indicating that they
> +should be disabled.
> +
> +At least one of "block-device" and "total_size" must be set.
> +
> +At least one of "dmesg-size" or "pmsg-size" must be set non-zero.
> +
> +Required properties:
> +
> +- compatible: must be "blkoops".
> +
> +Optional properties:
> +
> +- block-device: The block device to use. Most of the time, it is a partition of
> +		device. If block-device is NULL, no block device is effective
> +		and the data will be lost after rebooting.
> +		It accept the following variants:
> +		1) <hex_major><hex_minor> device number in hexadecimal
> +		   represents itself no leading 0x, for example b302.
> +		2) /dev/<disk_name> represents the device number of disk
> +		3) /dev/<disk_name><decimal> represents the device number of
> +		   partition - device number of disk plus the partition number
> +		4) /dev/<disk_name>p<decimal> - same as the above, that form is
> +		   used when disk name of partitioned disk ends on a digit.
> +		5) PARTUUID=00112233-4455-6677-8899-AABBCCDDEEFF representing
> +		   the unique id of a partition if the partition table provides
> +		   it. The UUID may be either an EFI/GPT UUID, or refer to an
> +		   MSDOS partition using the format SSSSSSSS-PP, where SSSSSSSS
> +		   is a zero-filled hex representation of the 32-bit
> +		   "NT disk signature", and PP is a zero-filled hex
> +		   representation of the 1-based partition number.
> +		6) PARTUUID=<UUID>/PARTNROFF=<int> to select a partition in
> +		   relation to a partition with a known unique id.
> +		7) <major>:<minor> major and minor number of the device
> +		   separated by a colon.

No.

I didn't suggest to go look at PARTUUID to copy it into the binding, but 
rather to point out that the kernel can already mount by UUID. 
Specifying the UUID in DT is also not what I suggested. My suggestion is 
to define a known UUID so that the kernel (and bootloaders, userspace, 
the world) can just know the UUID. Just like the EFI system partition. 
Now this means you have to get it defined in the UEFI specification 
(or maybe EBBR[1]). If you want help with how to do that, the 
boot-architecture list is a good place to start.

major/minor numbers are a Linux thing, so they don't go in DT.
/dev/* is Linux thing, so it doesn't go in DT.

You can always define all these parameters as kernel command line 
options and avoid DT. That would also make this work on *all* systems, 
not just DT based systems. (Though I still believe that the partition 
should be discoverable.)

Rob

[1] https://github.com/ARM-software/ebbr
WeiXiong Liao Feb. 25, 2019, 2:20 p.m. UTC | #2
On 2019-02-22 23:36, Rob Herring wrote:
> +boot-architecture list
> 
> On Tue, Feb 19, 2019 at 07:52:47PM +0800, liaoweixiong wrote:
>> Create DT binding document for blkoops.
>>
>> Signed-off-by: liaoweixiong <liaoweixiong@allwinnertech.com>
>> ---
>>  .../devicetree/bindings/pstore/blkoops.txt         | 53 ++++++++++++++++++++++
>>  MAINTAINERS                                        |  1 +
>>  2 files changed, 54 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pstore/blkoops.txt
>>
>> diff --git a/Documentation/devicetree/bindings/pstore/blkoops.txt b/Documentation/devicetree/bindings/pstore/blkoops.txt
>> new file mode 100644
>> index 0000000..5462915
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pstore/blkoops.txt
>> @@ -0,0 +1,53 @@
>> +Blkoops oops logger
>> +===================
>> +
>> +Blkoops provides a block partition for oops, excluding panics now, so they can
>> +be recovered after a reboot.
>> +
>> +Any space of block device will be used for a circular buffer of oops records.
>> +These records have a configurable size, with a size of 0 indicating that they
>> +should be disabled.
>> +
>> +At least one of "block-device" and "total_size" must be set.
>> +
>> +At least one of "dmesg-size" or "pmsg-size" must be set non-zero.
>> +
>> +Required properties:
>> +
>> +- compatible: must be "blkoops".
>> +
>> +Optional properties:
>> +
>> +- block-device: The block device to use. Most of the time, it is a partition of
>> +		device. If block-device is NULL, no block device is effective
>> +		and the data will be lost after rebooting.
>> +		It accept the following variants:
>> +		1) <hex_major><hex_minor> device number in hexadecimal
>> +		   represents itself no leading 0x, for example b302.
>> +		2) /dev/<disk_name> represents the device number of disk
>> +		3) /dev/<disk_name><decimal> represents the device number of
>> +		   partition - device number of disk plus the partition number
>> +		4) /dev/<disk_name>p<decimal> - same as the above, that form is
>> +		   used when disk name of partitioned disk ends on a digit.
>> +		5) PARTUUID=00112233-4455-6677-8899-AABBCCDDEEFF representing
>> +		   the unique id of a partition if the partition table provides
>> +		   it. The UUID may be either an EFI/GPT UUID, or refer to an
>> +		   MSDOS partition using the format SSSSSSSS-PP, where SSSSSSSS
>> +		   is a zero-filled hex representation of the 32-bit
>> +		   "NT disk signature", and PP is a zero-filled hex
>> +		   representation of the 1-based partition number.
>> +		6) PARTUUID=<UUID>/PARTNROFF=<int> to select a partition in
>> +		   relation to a partition with a known unique id.
>> +		7) <major>:<minor> major and minor number of the device
>> +		   separated by a colon.
> 
> No.
> 
> I didn't suggest to go look at PARTUUID to copy it into the binding, but 
> rather to point out that the kernel can already mount by UUID. 
> Specifying the UUID in DT is also not what I suggested. My suggestion is 
> to define a known UUID so that the kernel (and bootloaders, userspace, 
> the world) can just know the UUID. Just like the EFI system partition. 
> Now this means you have to get it defined in the UEFI specification 
> (or maybe EBBR[1]). If you want help with how to do that, the 
> boot-architecture list is a good place to start.
> 

Thanks for your suggestion. I don't know whether it is a good idea to
define a known UUID for pstore/blk in the UEFI specification. This
property is only used for pstore/blk to know which block device it can
use. It only works on linux. I think more thorough and rigorous
consideration is needed.
Besides that, mbr partition table has no partition UUID, how can it to
be compatible with mbr?

> major/minor numbers are a Linux thing, so they don't go in DT.
> /dev/* is Linux thing, so it doesn't go in DT.
> 
> You can always define all these parameters as kernel command line 
> options and avoid DT. That would also make this work on *all* systems, 
> not just DT based systems. (Though I still believe that the partition 
> should be discoverable.)
> 

The pstore/blk has already support command line. It now has 3
configuration methods, they are kconfig, DT and module parameters. I
will cancel DT support on next version until we discuss a viable
approach by this mail and than i will submit other patches to implement
DT. Is this ok?

> Rob
> 
> [1] https://github.com/ARM-software/ebbr
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pstore/blkoops.txt b/Documentation/devicetree/bindings/pstore/blkoops.txt
new file mode 100644
index 0000000..5462915
--- /dev/null
+++ b/Documentation/devicetree/bindings/pstore/blkoops.txt
@@ -0,0 +1,53 @@ 
+Blkoops oops logger
+===================
+
+Blkoops provides a block partition for oops, excluding panics now, so they can
+be recovered after a reboot.
+
+Any space of block device will be used for a circular buffer of oops records.
+These records have a configurable size, with a size of 0 indicating that they
+should be disabled.
+
+At least one of "block-device" and "total_size" must be set.
+
+At least one of "dmesg-size" or "pmsg-size" must be set non-zero.
+
+Required properties:
+
+- compatible: must be "blkoops".
+
+Optional properties:
+
+- block-device: The block device to use. Most of the time, it is a partition of
+		device. If block-device is NULL, no block device is effective
+		and the data will be lost after rebooting.
+		It accept the following variants:
+		1) <hex_major><hex_minor> device number in hexadecimal
+		   represents itself no leading 0x, for example b302.
+		2) /dev/<disk_name> represents the device number of disk
+		3) /dev/<disk_name><decimal> represents the device number of
+		   partition - device number of disk plus the partition number
+		4) /dev/<disk_name>p<decimal> - same as the above, that form is
+		   used when disk name of partitioned disk ends on a digit.
+		5) PARTUUID=00112233-4455-6677-8899-AABBCCDDEEFF representing
+		   the unique id of a partition if the partition table provides
+		   it. The UUID may be either an EFI/GPT UUID, or refer to an
+		   MSDOS partition using the format SSSSSSSS-PP, where SSSSSSSS
+		   is a zero-filled hex representation of the 32-bit
+		   "NT disk signature", and PP is a zero-filled hex
+		   representation of the 1-based partition number.
+		6) PARTUUID=<UUID>/PARTNROFF=<int> to select a partition in
+		   relation to a partition with a known unique id.
+		7) <major>:<minor> major and minor number of the device
+		   separated by a colon.
+
+- total-size: The total size in kbytes pstore/blk can use. It must be a multiple
+	      of 4. It must be less than or equal to size of block-device. If
+	      total-size is zero with block-devce valid, it will be set to equal
+	      to size of block-device.
+
+- dmesg-size: maximum size in kbytes of each dump done on oops, which must be a
+	      multiple of 4.
+
+- pmsg-size: maximum size in kbytes for userspace messages, which must be a
+	     multiple of 4.
diff --git a/MAINTAINERS b/MAINTAINERS
index 51029a4..f49dd37 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12318,6 +12318,7 @@  F:	drivers/firmware/efi/efi-pstore.c
 F:	drivers/acpi/apei/erst.c
 F:	Documentation/admin-guide/ramoops.rst
 F:	Documentation/devicetree/bindings/reserved-memory/ramoops.txt
+F:	Documentation/devicetree/bindings/pstore-block/
 K:	\b(pstore|ramoops)
 
 PTP HARDWARE CLOCK SUPPORT