Date:   Mon, 20 Jan 2020 22:36:58 -0800
From:   Randy Dunlap <rdunlap@...radead.org>
To:     liaoweixiong <liaoweixiong@...winnertech.com>,
        Kees Cook <keescook@...omium.org>,
        Anton Vorontsov <anton@...msg.org>,
        Colin Cross <ccross@...roid.com>,
        Tony Luck <tony.luck@...el.com>,
        Jonathan Corbet <corbet@....net>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Rob Herring <robh@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc:     linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-mtd@...ts.infradead.org
Subject: Re: [PATCH v1 06/11] Documentation: pstore/blk: blkoops: create
 document for pstore_blk

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@...winnertech.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.

>>> +@...ts 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

Powered by blists - more mailing lists