Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <49c883aa-0f5d-2e5f-adbb-c6793417cb89@quicinc.com>
Date:   Tue, 28 Nov 2023 16:40:25 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [Patch v6 11/12] pstore/ram: Add ramoops ready notifier support
Content-Language: en-US
To:     Pavan Kondeti <quic_pkondeti@quicinc.com>
CC:     <corbet@lwn.net>, <agross@kernel.org>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
        <keescook@chromium.org>, <tony.luck@intel.com>,
        <gpiccoli@igalia.com>, <mathieu.poirier@linaro.org>,
        <vigneshr@ti.com>, <nm@ti.com>, <matthias.bgg@gmail.com>,
        <kgene@kernel.org>, <alim.akhtar@samsung.com>,
        <bmasney@redhat.com>, <linux-doc@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <linux-hardening@vger.kernel.org>,
        <linux-remoteproc@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-mediatek@lists.infradead.org>,
        <linux-samsung-soc@vger.kernel.org>, <kernel@quicinc.com>
References: <1700864395-1479-1-git-send-email-quic_mojha@quicinc.com>
 <1700864395-1479-12-git-send-email-quic_mojha@quicinc.com>
 <3636dc3a-b62b-4ff9-bdc3-fec496a804b7@quicinc.com>
From:   Mukesh Ojha <quic_mojha@quicinc.com>
In-Reply-To: <3636dc3a-b62b-4ff9-bdc3-fec496a804b7@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 11/27/2023 3:40 PM, Pavan Kondeti wrote:
> On Sat, Nov 25, 2023 at 03:49:54AM +0530, Mukesh Ojha wrote:
>> Client like minidump, is only interested in ramoops
>> region addresses/size so that it could register them
>> with its table and also it is only deals with ram
>> backend and does not use pstorefs to read the records.
>> Let's introduce a client notifier in ramoops which
>> gets called when ramoops driver probes successfully
>> and it passes the ramoops region information to the
>> passed callback by the client and If the call for
>> ramoops ready register comes after ramoops probe
>> than call the callback directly.
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>>   fs/pstore/ram.c            | 77 ++++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/pstore_ram.h |  6 ++++
>>   2 files changed, 83 insertions(+)
>>
>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> index a6c0da8cfdd4..72341fd21aec 100644
>> --- a/fs/pstore/ram.c
>> +++ b/fs/pstore/ram.c
>> @@ -22,6 +22,7 @@
>>   #include <linux/of_address.h>
>>   #include <linux/memblock.h>
>>   #include <linux/mm.h>
>> +#include <linux/mutex.h>
>>   
>>   #include "internal.h"
>>   #include "ram_internal.h"
>> @@ -101,6 +102,14 @@ struct ramoops_context {
>>   	unsigned int ftrace_read_cnt;
>>   	unsigned int pmsg_read_cnt;
>>   	struct pstore_info pstore;
>> +	/*
>> +	 * Lock to serialize calls to register_ramoops_ready_notifier,
>> +	 * ramoops_ready_notifier and read/modification of 'ramoops_ready'.
>> +	 */
>> +	struct mutex lock;
>> +	bool ramoops_ready;
>> +	int (*callback)(const char *name, int id, void *vaddr,
>> +			phys_addr_t paddr, size_t size);
>>   };
>>   
>>   static struct platform_device *dummy;
>> @@ -488,6 +497,7 @@ static int ramoops_pstore_erase(struct pstore_record *record)
>>   }
>>   
>>   static struct ramoops_context oops_cxt = {
>> +	.lock   = __MUTEX_INITIALIZER(oops_cxt.lock),
>>   	.pstore = {
>>   		.owner	= THIS_MODULE,
>>   		.name	= "ramoops",
>> @@ -662,6 +672,68 @@ static int ramoops_init_prz(const char *name,
>>   	return 0;
>>   }
>>   
>> +void ramoops_ready_notifier(struct ramoops_context *cxt)
>> +{
>> +	struct persistent_ram_zone *prz;
>> +	int i;
>> +
>> +	if (!cxt->callback)
>> +		return;
>> +
>> +	for (i = 0; i < cxt->max_dump_cnt; i++) {
>> +		prz = cxt->dprzs[i];
>> +		cxt->callback("dmesg", i, prz->vaddr, prz->paddr, prz->size);
>> +	}
>> +
>> +	if (cxt->console_size) {
>> +		prz = cxt->cprz;
>> +		cxt->callback("console", 0, prz->vaddr, prz->paddr, prz->size);
>> +	}
>> +
>> +	for (i = 0; i < cxt->max_ftrace_cnt; i++) {
>> +		prz = cxt->fprzs[i];
>> +		cxt->callback("ftrace", i, prz->vaddr, prz->paddr, prz->size);
>> +	}
>> +
>> +	if (cxt->pmsg_size) {
>> +		prz = cxt->mprz;
>> +		cxt->callback("pmsg", 0, prz->vaddr, prz->paddr, prz->size);
>> +	}
>> +}
>> +
>> +int register_ramoops_ready_notifier(int (*fn)(const char *, int,
>> +				   void *, phys_addr_t, size_t))
>> +{
>> +	struct ramoops_context *cxt = &oops_cxt;
>> +
>> +	mutex_lock(&cxt->lock);
>> +	if (cxt->callback) {
>> +		mutex_unlock(&cxt->lock);
>> +		return -EEXIST;
>> +	}
>> +
>> +	cxt->callback = fn;
>> +	if (cxt->ramoops_ready)
>> +		ramoops_ready_notifier(cxt);
>> +
>> +	mutex_unlock(&cxt->lock);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(register_ramoops_ready_notifier);
>> +
> 
> Can you please elaborate on why do we need this custom notifier logic?
> 
> why would not a standard notifier (include/linux/notifier.h) work here?
> The notifier_call callback can recieve custom data from the
> notifier chain implementer. All we need is to define a custom struct like
> struct pstore_ramoops_zone_data {
> 	const char *name;
> 	int id;
> 	void *vaddr;
> 	phys_addr_t paddr;
> 	size_t size;
> };
> 
> and pass the pointer to array of this struct.
> 
> 
> btw, the current logic only supports just one client and this limitation
> is not highlighted any where.

I could work on it, was not sure if that will be helpful
for other users .

-Mukesh
> 
> Thanks,
> Pavan
>