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

[Mukesh Ojha <quic_mojha@xxxxxxxxxxx>]

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@xxxxxxxxxxx>
> > ---
> >   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