Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1546862462-19640-1-git-send-email-liaoweixiong@allwinnertech.com>
In-Reply-To: <1546862462-19640-1-git-send-email-liaoweixiong@allwinnertech.com>
From:   Kees Cook <keescook@chromium.org>
Date:   Mon, 7 Jan 2019 13:47:24 -0800
Message-ID: <CAGXu5jKrEHkogWsNW0rui8fBbdGin0syWzdfhVhw1V9tTRy0+A@mail.gmail.com>
Subject: Re: [RFC v5 0/4] pstore/block: new support logger for block devices
To:     liaoweixiong <liaoweixiong@allwinnertech.com>
Cc:     Anton Vorontsov <anton@enomsg.org>,
        Colin Cross <ccross@android.com>,
        Tony Luck <tony.luck@intel.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "David S. Miller" <davem@davemloft.net>,
        Andrew Morton <akpm@linux-foundation.org>,
        Nicolas Ferre <nicolas.ferre@microchip.com>,
        Arnd Bergmann <arnd@arndb.de>,
        "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jan 7, 2019 at 4:01 AM liaoweixiong
<liaoweixiong@allwinnertech.com> wrote:
>
> Why should we need pstore_block?
> 1. Most embedded intelligent equipment have no persistent ram, which
> increases costs. We perfer to cheaper solutions, like block devices.
> In fast, there is already a sample for block device logger in driver
> MTD (drivers/mtd/mtdoops.c).
> 2. Do not any equipment have battery, which means that it lost all data
> on general ram if power failure. Pstore has little to do for these
> equipments.

I'm catching up from being off over the holidays, but I just wanted to
say that I like the general idea here.

> [PATCH v5]
> On patch 1:
> 1. rename pstore/rom to pstore/blk

Thanks! The earlier name seemed wrong. :)

> 2. Do not allocate any memory in the write path of panic. So, use local
> array instead in function romz_recover_dmesg_meta.
> 3. Add C header file "linux/fs.h" to fix implicit declaration of function
>    'filp_open','kernel_read'...
> On patch 3:
> 1. If panic, do not recover pmsg but flush if it is dirty.
> 2. Fix erase pmsg failed.
> On patch 4:
> 1. Create a document for pstore/blk

Overall, I wonder if more of the ram backend code could get reused for
the blk backend? I'd much rather be able to share that, since much of
the logic is the same.

I'll get to a more detailed review in the coming days. Thanks for sending this!

-Kees

>
> [PATCH v4]
> On patch 1:
> 1. Fix always true condition '(--i >= 0) => (0-u32max >= 0)' in function
>    romz_init_zones by defining variable i to 'int' rahter than
>    'unsigned int'.
> 2. To make codes more easily to read, we use macro READ_NEXT_ZONE for
>    return value of romz_dmesg_read if it need to read next zone.
>    Moveover, we assign READ_NEXT_ZONE -1024 rather than 0.
> 3. Add 'FLUSH_META' to 'enum romz_flush_mode' and rename 'NOT_FLUSH' to
>    'FLUSH_NONE'
> 4. Function romz_zone_write work badly with FLUSH_PART mode as badly
>    address and offset to write.
> On patch 3:
> NEW SUPPORT psmg for pstore_rom.
>
> [PATCH v3]
> On patch 1:
> Fix build as module error for undefined 'vfs_read' and 'vfs_write'
> Both of 'vfs_read' and 'vfs_write' haven't be exproted yet, so we use
> 'kernel_read' and 'kernel_write' instead.
>
> [PATCH v2]
> On patch 1:
> Fix build as module error for redefinition of 'romz_unregister' and
> 'romz_register'
>
> [PATCH v1]
> On patch 1:
> Core codes of pstore_rom, which works well on allwinner(sunxi) platform.
> On patch 2:
> A sample for pstore_rom, using general ram rather than block device.
>
> liaoweixiong (4):
>   pstore/blk: new support logger for block devices
>   pstore/blk: add sample for pstore_blk
>   pstore/blk: support pmsg for pstore block
>   Documentation: pstore/blk: create document for pstore_blk
>
>  Documentation/admin-guide/pstore-block.rst |  226 ++++++
>  MAINTAINERS                                |    1 +
>  fs/pstore/Kconfig                          |   20 +
>  fs/pstore/Makefile                         |    5 +
>  fs/pstore/blkbuf.c                         |   46 ++
>  fs/pstore/blkzone.c                        | 1168 ++++++++++++++++++++++++++++
>  include/linux/pstore_blk.h                 |   62 ++
>  7 files changed, 1528 insertions(+)
>  create mode 100644 Documentation/admin-guide/pstore-block.rst
>  create mode 100644 fs/pstore/blkbuf.c
>  create mode 100644 fs/pstore/blkzone.c
>  create mode 100644 include/linux/pstore_blk.h
>
> --
> 1.9.1
>


-- 
Kees Cook