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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3337f687-a668-c058-178b-a1438641c519@allwinnertech.com>
Date:   Tue, 21 Jan 2020 16:19:26 +0800
From:   liaoweixiong <liaoweixiong@...winnertech.com>
To:     Randy Dunlap <rdunlap@...radead.org>,
        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

hi Randy Dunlap,

On 2020/1/21 2:36 PM, Randy Dunlap wrote:
> 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.
> 

Oh, sorry, that is my oversight. It seems that not only the other size 
introductions but also introductions on Kconfig should be corrected. 
Thank you very much and is the following modification OK?

The chunk size in KB for dmesg(oops/panic). It **MUST** be a multiple of 4.

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

It is an oops/panic counter too. How about this?

It means that it's OOPS/PANIC for the 16th time since the first booting.

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