>-----Original Message----- >From: Kees Cook <keescook@xxxxxxxxxxxx> >Sent: Thursday, December 3, 2020 1:02 AM >To: Bhaskara Budiredla <bbudiredla@xxxxxxxxxxx> >Cc: ulf.hansson@xxxxxxxxxx; ccross@xxxxxxxxxxx; tony.luck@xxxxxxxxx; Sunil >Kovvuri Goutham <sgoutham@xxxxxxxxxxx>; linux-mmc@xxxxxxxxxxxxxxx; >linux-kernel@xxxxxxxxxxxxxxx; outgoing2/0000-cover-letter.patch@mx0b- >0016f401.pphosted.com >Subject: Re: [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@xxxxxxxxxxxx> 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: > Yes, it's a tri-state but not compiled as a separate driver. >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. > No, this will cause problems for MMC_PSTORE=m and MMC_BLOCK=y MMC_PSTORE automatically have to be selected as module or static based on MMC_BLOCK selection. There were couple of function calls from the code in MMC_BLOCK to MMC_PSTORE. Uffe prefers them to be unconditional calls (as per discussion in v1). >> >> + 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.) > Would it be sufficient to add corresponding notes to the commit message? >-- >Kees Cook
