diff mbox series

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

Message ID 1700864395-1479-12-git-send-email-quic_mojha@quicinc.com (mailing list archive)
State Handled Elsewhere
Headers show
Series Add Qualcomm Minidump driver related support | expand

Commit Message

Mukesh Ojha Nov. 24, 2023, 10:19 p.m. UTC
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(+)

Comments

Pavan Kondeti Nov. 27, 2023, 10:10 a.m. UTC | #1
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.

Thanks,
Pavan
Mukesh Ojha Nov. 28, 2023, 11:10 a.m. UTC | #2
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
>
diff mbox series

Patch

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);
+
+void unregister_ramoops_ready_notifier(int (*fn)(const char *, int,
+				     void *, phys_addr_t, size_t))
+{
+	struct ramoops_context *cxt = &oops_cxt;
+
+	mutex_lock(&cxt->lock);
+	WARN_ON_ONCE(cxt->callback != fn);
+	cxt->callback = NULL;
+	mutex_unlock(&cxt->lock);
+}
+EXPORT_SYMBOL_GPL(unregister_ramoops_ready_notifier);
+
 /* Read a u32 from a dt property and make sure it's safe for an int. */
 static int ramoops_parse_dt_u32(struct platform_device *pdev,
 				const char *propname,
@@ -911,6 +983,11 @@  static int ramoops_probe(struct platform_device *pdev)
 	ramoops_pmsg_size = pdata->pmsg_size;
 	ramoops_ftrace_size = pdata->ftrace_size;
 
+	mutex_lock(&cxt->lock);
+	ramoops_ready_notifier(cxt);
+	cxt->ramoops_ready = true;
+	mutex_unlock(&cxt->lock);
+
 	pr_info("using 0x%lx@0x%llx, ecc: %d\n",
 		cxt->size, (unsigned long long)cxt->phys_addr,
 		cxt->ecc_info.ecc_size);
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index b3537336c4e1..9745d48ba59e 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -39,6 +39,12 @@  struct ramoops_platform_data {
 	struct persistent_ram_ecc_info ecc_info;
 };
 
+int register_ramoops_ready_notifier(int (*fn)(const char *name, int id,
+				    void *vaddr, phys_addr_t paddr,
+				    size_t size));
+void unregister_ramoops_ready_notifier(int (*fn)(const char *name, int id,
+				       void *vaddr, phys_addr_t paddr,
+				       size_t size));
 #ifdef CONFIG_PSTORE_DYNAMIC_RAMOOPS_REGION_RESERVATION
 void __init setup_dynamic_ramoops(void);
 #else