Re: [PATCH v2 01/11] pstore/blk: new support logger for block devices

From: WeiXiong Liao <liaoweixiong@allwinnertech.com>
To: Kees Cook <keescook@chromium.org>
Cc: Rob Herring <robh@kernel.org>, Tony Luck <tony.luck@intel.com>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Richard Weinberger <richard@nod.at>,
	Anton Vorontsov <anton@enomsg.org>,
	linux-doc@vger.kernel.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-kernel@vger.kernel.org, Colin Cross <ccross@android.com>,
	linux-mtd@lists.infradead.org,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
	"David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH v2 01/11] pstore/blk: new support logger for block devices
Date: Sun, 22 Mar 2020 18:28:22 +0800	[thread overview]
Message-ID: <c49f1d24-b818-aeda-7447-b89d8eddb1c6@allwinnertech.com> (raw)
In-Reply-To: <202003201111.BE5EAB9A@keescook>

hi Kees Cook,

On 2020/3/21 上午2:20, Kees Cook wrote:
> On Fri, Mar 20, 2020 at 09:50:36AM +0800, WeiXiong Liao wrote:
>> On 2020/3/19 AM 1:23, Kees Cook wrote:
>>> On Thu, Feb 27, 2020 at 04:21:51PM +0800, liaoweixiong wrote:
>>>> On 2020/2/26 AM 8:52, Kees Cook wrote:
>>>>> On Fri, Feb 07, 2020 at 08:25:45PM +0800, WeiXiong Liao wrote:
>>>>>> +obj-$(CONFIG_PSTORE_BLK) += pstore_blk.o
>>>>>> +pstore_blk-y += blkzone.o
>>>>>
>>>>> Why this dance with files? I would just expect:
>>>>>
>>>>> obj-$(CONFIG_PSTORE_BLK)     += blkzone.o
>>>>>
>>>>
>>>> This makes the built module named blkzone.ko rather than
>>>> pstore_blk.ko.
>>>
>>> You can just do a regular build rule:
>>>
>>> obj-$(CONFIG_PSTORE_BLK) += blkzone.o
>>>
>>
>> I don't get it. If make it as your words, the built module will be
>> blkzone.ko.
>> The module is named pstore/blk, however it built out blkzone.ko. I think
>> it's confusing.
> 
> I mean just pick whatever filename you want it to be named. The Makefile
> case for ramoops was that ramoops got renamed but we wanted to keep the
> old API name.
> 
> So, if you want it named pstore-blk.ko, just rename blkzone.c to
> pstore-blk.c.
> 

How about rename blkzone.c to psotre_zone.c and blkoops.c to pstore_blk.c?

Please refer to my reply email for patch 2.

>>>>> If you're expecting concurrent writers (use of atomic_set(), I would
>>>>> expect the whole write to be locked instead. (i.e. what happens if
>>>>> multiple callers call blkz_zone_write()?)
>>>>>
>>>>
>>>> I don't agree with it. The datalen will be updated everywhere. It's useless
>>>> to lock here.
>>>
>>> But there could be multiple writers; locking should be needed.
>>>
>>
>> All the recorders such as dmesg, pmsg, console and ftrace have been
>> locked on
>> pstore and upper layers. So, a recorder will not write in parallel and
>> different
>> recorders operate privately zone. They don't have any influence on each
>> other.
> 
> Yes, sorry, I was confusing myself about pmsg, and I forgot it had a
> global lock. Each are locked or split by CPU.
> 
>> The only parallel case I think is that recorder writes while dirty-flush
>> thread is
>> working. And the dirty-flusher will flush the whole zone rather than
>> part of it, so,
>> it is OK to call in parallel.
> 
> Okay, thanks for clarifying.
> 
>> Based on these reasons, I don't think locking should be needed.
> 
> Agreed.
> 

-- 
WeiXiong Liao

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/