Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Bhaskara Budiredla <bbudiredla@marvell.com>
To:     Kees Cook <keescook@chromium.org>
CC:     "ulf.hansson@linaro.org" <ulf.hansson@linaro.org>,
        "ccross@android.com" <ccross@android.com>,
        "tony.luck@intel.com" <tony.luck@intel.com>,
        "Sunil Kovvuri Goutham" <sgoutham@marvell.com>,
        "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "outgoing2/0000-cover-letter.patch@mx0b-0016f401.pphosted.com" 
        <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
Thread-Topic: [EXT] Re: [PATCH v2 1/2] mmc: Support kmsg dumper based on
 pstore/blk
Thread-Index: AQHWwYqKGp95SRIfpkeLwxPInxlbBqnivWIAgACfeiCAAOO4AIAAn9QA
Date:   Thu, 3 Dec 2020 05:41:08 +0000
Message-ID: <CY4PR1801MB20702A51343093FD43A53A50DEF20@CY4PR1801MB2070.namprd18.prod.outlook.com>
References: <20201123111925.28999-1-bbudiredla@marvell.com>
 <202012011218.3B6566C5@keescook>
 <CY4PR1801MB207097391BD8DE37A0F49102DEF30@CY4PR1801MB2070.namprd18.prod.outlook.com>
 <202012021126.20ACBBD@keescook>
In-Reply-To: <202012021126.20ACBBD@keescook>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: d7b7400f-0ed7-40a8-62a4-08d8974e08bd
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Dec 2020 05:41:08.1138
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: oezJLdecWlwE14IITIznC3hKKGA8EY+L3In5v1Oe59K+ErR4Wws+bNbkfm6EKheIdDYwtWNDlwP6w41B4LpWlg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR18MB1190
X-OriginatorOrg: marvell.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.312,18.0.737
 definitions=2020-12-03_01:2020-11-30,2020-12-03 signatures=0
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


>-----Original Message-----
>From: Kees Cook <keescook@chromium.org>
>Sent: Thursday, December 3, 2020 1:02 AM
>To: Bhaskara Budiredla <bbudiredla@marvell.com>
>Cc: ulf.hansson@linaro.org; ccross@android.com; tony.luck@intel.com; Sunil
>Kovvuri Goutham <sgoutham@marvell.com>; linux-mmc@vger.kernel.org;
>linux-kernel@vger.kernel.org; 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@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 =3D=3D y && CONFIG_MMC_BLOCK =3D=3D m)
>> as static:     if (CONFIG_MMC_PSTORE_BACKEND =3D=3D y &&
>CONFIG_MMC_BLOCK =3D=3D 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.=20

>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=3Dm and MMC_BLOCK=3Dy
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). =20

>> >> +	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 f=
ull
>/dev/mmcblk0pn.
>> The partition number check is carried out in the next statement.
>
>Okay, gotcha; thanks!
>
>> >> +	dev->flags =3D 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 =3D 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?=20

>--
>Kees Cook