lkml.org 
[lkml]   [2020]   [Dec]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [EXT] Re: [PATCH v2 1/2] mmc: Support kmsg dumper based on pstore/blk
On Wed, Dec 02, 2020 at 06:36:21AM +0000, Bhaskara Budiredla wrote:
> >From: Kees Cook <keescook@chromium.org>
> >On Mon, Nov 23, 2020 at 04:49:24PM +0530, Bhaskara Budiredla wrote:
> >Why isn't this just written as:
> >
> >config MMC_PSTORE
> > bool "Log panic/oops to a MMC buffer"
> > depends on MMC_BLOCK
> > select PSTORE_BLK
> > help
> > This option will let you create platform backend to store kmsg
> > crash dumps to a user specified MMC device. This is primarily
> > based on pstore/blk.
> >
>
> The idea was to compile MMC_PSTORE as part of MMC_BLOCK driver,
> provided it is optionally enabled.
> The above arrangement compiles MMC_PSTORE
> as module: if (CONFIG_MMC_PSTORE_BACKEND == y && CONFIG_MMC_BLOCK == m)
> as static: if (CONFIG_MMC_PSTORE_BACKEND == y && CONFIG_MMC_BLOCK == y)

Ah, okay. If it's a tri-state, wouldn't it track CONFIG_MMC_BLOCK's
state? As in, does this work:

config MMC_PSTORE
tristate "Log panic/oops to a MMC buffer"
depends on MMC_BLOCK
select PSTORE_BLK
help
This option will let you create platform backend to store kmsg
crash dumps to a user specified MMC device. This is primarily
based on pstore/blk.

> >> + if (strncmp(cxt->dev_name, disk_name, strlen(disk_name)))
> >> + return;
> >
> >Why isn't this just strcmp()?
>
> The mmc disk name (disk_name) doesn't include the partition number.
> strncmp is restricted to something like /dev/mmcblk0, it doesn't cover full /dev/mmcblk0pn.
> The partition number check is carried out in the next statement.

Okay, gotcha; thanks!

> >> + dev->flags = PSTORE_FLAGS_DMESG;
> >
> >Can't this support more than just DMESG? I don't see anything specific to that.
> >This is using pstore/zone ultimately, which can support whatever frontends it
> >needs to.
>
> Yes, as of now the support is only for DMESG. We will extend this to other frontends
> on need basis.

Okay -- I assume this has mostly to do with not having erasure (below).

> >> + dev->erase = NULL;
> >
> >No way to remove the records?
>
> Yes, at this time, no removal of records.

Okay. (I think this might be worth mentioning in docs somewhere.)

--
Kees Cook

\
 
 \ /
  Last update: 2020-12-02 20:35    [W:0.108 / U:1.412 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site