Re: [Patch v6 11/12] pstore/ram: Add ramoops ready notifier support

[Mukesh Ojha <quic_mojha@quicinc.com>]

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
>