Re: [PATCH v1 06/11] Documentation: pstore/blk: blkoops: create document for pstore_blk

Randy Dunlap
2020-01-21T06:37:06Z

On 1/20/20 9:23 PM, liaoweixiong wrote:
> hi Randy Dunlap,
>
> On 2020/1/21 PM12:13, Randy Dunlap wrote:
>> Hi,
>>
>> I have some documentation comments for you:
>>
>>
>> On 1/19/20 5:03 PM, WeiXiong Liao wrote:
>>> The document, at Documentation/admin-guide/pstore-block.rst, tells us
>>> how to use pstore/blk and blkoops.
>>>
>>> Signed-off-by: WeiXiong Liao <liaoweixiong@allwinnertech.com>
>>> ---
>>> Documentation/admin-guide/pstore-block.rst | 278 +++++++++++++++++++++++++++++
>>> MAINTAINERS | 1 +
>>> fs/pstore/Kconfig | 2 +
>>> 3 files changed, 281 insertions(+)
>>> create mode 100644 Documentation/admin-guide/pstore-block.rst
>>>
>>> diff --git a/Documentation/admin-guide/pstore-block.rst b/Documentation/admin-guide/pstore-block.rst
>>> new file mode 100644
>>> index 000000000000..58418d429c55
>>> --- /dev/null
>>> +++ b/Documentation/admin-guide/pstore-block.rst
>>> +
>>> +
>>> +dmesg_size
>>> +~~~~~~~~~~
>>> +
>>> +The chunk size in bytes for dmesg(oops/panic). It **MUST** be a multiple of
>>> +4096. If you don't need it, safely set it 0 or ignore it.
>>
>> set it to 0 or ignore it.
>>
>
> I will fix it, thank you.
>
>> The example above is: blkoops.dmesg_size=64
>> where 64 is not a multiple of 4096. (?)
>>
>
> The module parameter dmesg_size is in unit KB.

I didn't see that documented anywhere.

>>> +Normally the number of bytes written should be returned, while for error,
>>> +negative number should be returned.
>>> +
>>> +panic_write (for block device)
>>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> +
>>> +It's much similar to panic_write for non-block device, but panic_write for
>>> +block device writes alignment to SECTOR_SIZE, that's why the parameters are
>>
>> writes only aligned sectors of SECTOR_SIZE (??)
>>
>
> How about this?
>
> It's much similar to panic_write for non-block device, but the position and
> data size of panic_write for block device must be aligned to SECTOR_SIZE,
> that's why the parameters are @sects and @start_sect. Block device driver
> should register it by ``blkoops_register_blkdev``.

OK.

>>> +@sects and @start_sect. Block device driver should register it by
>>> +``blkoops_register_blkdev``.
>>> +
>>> +The parameter @start_sect is the relative position of the block device and
>>> +partition. If block driver requires absolute position for panic_write,
>>> +``blkoops_blkdev_info`` will be helpful, which can provide the absolute
>>> +position of the block device (or partition) on the whole disk/flash.
>>> +
>>> +Normally zero should be returned, otherwise it indicates an error.
>>> +
>>> +Compression and header
>>> +----------------------
>>> +
>>> +Block device is large enough for uncompressed dmesg data. Actually we do not
>>> +recommend data compression because pstore/blk will insert some information into
>>> +the first line of dmesg data. For example::
>>> +
>>> + Panic: Total 16 times
>>> +
>>> +It means that it's the 16th times panic log since the first booting. Sometimes
>>
>> time of a panic log since ...
>>
>
> Should it be like this?
> It means the time of a panic log since the first booting.

That sounds like clock time, not the number of instances or occurrences.

>
>>> +the oops|panic occurs since burning is very important for embedded device to
>>
>> ^^^^^^^ huh??
>>
>
> How about this?
>
> Sometimes the number of occurrences of oops|panic since the first
> booting is important
> to judge whether the system is stable.

OK.

>>> +judge whether the system is stable.
>>> +
>>> +The following line is inserted by pstore filesystem. For example::
>>> +
>>> + Oops#2 Part1
>>> +
>>> +It means that it's the 2nd times oops log on last booting.
>>
>> 2nd time of an oops log on the last boot. (?)
>>
>
> How about this?
>
> It means that it's OOPS for the 2nd time on the last boot.

OK. It's an oops counter.

>>> +#. Just use CPU to transfer.
>>> + Do not use DMA to transfer unless you are sure that DMA will not keep lock.
>>> +#. Operate register directly.
>>
>> Don't know what that means.
>>
>
> How about this?
>
> #. Control registers directly.
> Please control registers directly rather than use Linux kernel
> resources.

OK.

> Do I/O map while initializing rather than wait until a panic occurs.
>
>>> + Try not to use Linux kernel resources. Do I/O map while initializing rather
>>> + than waiting until the panic.

--
~Randy