diff mbox series

[REBASE,v5,10/17] pstore: Add pstore_region_defined() helper and export it

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

Commit Message

Mukesh Ojha Sept. 11, 2023, 10:53 a.m. UTC 
There are users like Qualcomm minidump which is interested in
knowing the pstore frontend addresses and sizes from the backend
(ram) to be able to register it with firmware to finally collect
them during crash for debugging.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 fs/pstore/platform.c   | 15 +++++++++++++++
 fs/pstore/ram.c        | 42 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pstore.h |  6 ++++++
 3 files changed, 63 insertions(+)

Comments

Kees Cook Sept. 13, 2023, 11:24 p.m. UTC | #1 
On Mon, Sep 11, 2023 at 04:23:52PM +0530, Mukesh Ojha wrote:
> There are users like Qualcomm minidump which is interested in
> knowing the pstore frontend addresses and sizes from the backend
> (ram) to be able to register it with firmware to finally collect
> them during crash for debugging.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>  fs/pstore/platform.c   | 15 +++++++++++++++
>  fs/pstore/ram.c        | 42 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pstore.h |  6 ++++++
>  3 files changed, 63 insertions(+)
> 
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index e5bca9a004cc..fdac951059c1 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -139,6 +139,21 @@ enum pstore_type_id pstore_name_to_type(const char *name)
>  }
>  EXPORT_SYMBOL_GPL(pstore_name_to_type);
>  
> +int pstore_region_defined(struct pstore_record *record,
> +			  void **virt, phys_addr_t *phys,
> +			  size_t *size, unsigned int *max_dump_cnt)
> +{
> +	if (!psinfo)
> +		return -EINVAL;
> +
> +	record->psi = psinfo;

Uh, this makes no sense to me. If this is a real pstore_record, you
cannot just assign psi here.

> +
> +	return psinfo->region_info ?
> +	       psinfo->region_info(record, virt, phys, size, max_dump_cnt) :
> +	       -EINVAL;

Common code style for this kind of thing is usually like this:

	if (!psinfo->region_info)
		return -EINVAL;

	return psinfo->region_info(...)

> +}
> +EXPORT_SYMBOL_GPL(pstore_region_defined);
> +
>  static void pstore_timer_kick(void)
>  {
>  	if (pstore_update_ms < 0)
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index ab551caa1d2a..62202f3ddf63 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -437,6 +437,47 @@ static int ramoops_pstore_erase(struct pstore_record *record)
>  	return 0;
>  }
>  
> +static int ramoops_region_info(struct pstore_record *record,
> +			       void **virt, phys_addr_t *phys,
> +			       size_t *size, unsigned int *max_dump_cnt)

But there's a larger problem here -- "virt", "phys" and likely
"max_dump_cnt" are aspects _specific to the ram backend_. This can't be
a generic pstore interface.

I'm not opposed to it being exposed only from ramoops, though.

But I think you'll want a pstore generic way to iterate over the
records...
Mukesh Ojha Oct. 9, 2023, 11:59 a.m. UTC | #2 
Thanks for the review.

On 9/14/2023 4:54 AM, Kees Cook wrote:
> On Mon, Sep 11, 2023 at 04:23:52PM +0530, Mukesh Ojha wrote:
>> There are users like Qualcomm minidump which is interested in
>> knowing the pstore frontend addresses and sizes from the backend
>> (ram) to be able to register it with firmware to finally collect
>> them during crash for debugging.
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>>   fs/pstore/platform.c   | 15 +++++++++++++++
>>   fs/pstore/ram.c        | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/pstore.h |  6 ++++++
>>   3 files changed, 63 insertions(+)
>>
>> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
>> index e5bca9a004cc..fdac951059c1 100644
>> --- a/fs/pstore/platform.c
>> +++ b/fs/pstore/platform.c
>> @@ -139,6 +139,21 @@ enum pstore_type_id pstore_name_to_type(const char *name)
>>   }
>>   EXPORT_SYMBOL_GPL(pstore_name_to_type);
>>   
>> +int pstore_region_defined(struct pstore_record *record,
>> +			  void **virt, phys_addr_t *phys,
>> +			  size_t *size, unsigned int *max_dump_cnt)
>> +{
>> +	if (!psinfo)
>> +		return -EINVAL;
>> +
>> +	record->psi = psinfo;
> 
> Uh, this makes no sense to me. If this is a real pstore_record, you
> cannot just assign psi here.

Ok.

> 
>> +
>> +	return psinfo->region_info ?
>> +	       psinfo->region_info(record, virt, phys, size, max_dump_cnt) :
>> +	       -EINVAL;
> 
> Common code style for this kind of thing is usually like this:
> 
> 	if (!psinfo->region_info)
> 		return -EINVAL;
> 
> 	return psinfo->region_info(...)

Thanks.

-Mukesh
> 
>> +}
>> +EXPORT_SYMBOL_GPL(pstore_region_defined);
>> +
>>   static void pstore_timer_kick(void)
>>   {
>>   	if (pstore_update_ms < 0)
>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> index ab551caa1d2a..62202f3ddf63 100644
>> --- a/fs/pstore/ram.c
>> +++ b/fs/pstore/ram.c
>> @@ -437,6 +437,47 @@ static int ramoops_pstore_erase(struct pstore_record *record)
>>   	return 0;
>>   }
>>   
>> +static int ramoops_region_info(struct pstore_record *record,
>> +			       void **virt, phys_addr_t *phys,
>> +			       size_t *size, unsigned int *max_dump_cnt)
> 
> But there's a larger problem here -- "virt", "phys" and likely
> "max_dump_cnt" are aspects _specific to the ram backend_. This can't be
> a generic pstore interface.
> 
> I'm not opposed to it being exposed only from ramoops, though.
> 
> But I think you'll want a pstore generic way to iterate over the
> records..
diff mbox series

Patch

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index e5bca9a004cc..fdac951059c1 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -139,6 +139,21 @@  enum pstore_type_id pstore_name_to_type(const char *name)
 }
 EXPORT_SYMBOL_GPL(pstore_name_to_type);
 
+int pstore_region_defined(struct pstore_record *record,
+			  void **virt, phys_addr_t *phys,
+			  size_t *size, unsigned int *max_dump_cnt)
+{
+	if (!psinfo)
+		return -EINVAL;
+
+	record->psi = psinfo;
+
+	return psinfo->region_info ?
+	       psinfo->region_info(record, virt, phys, size, max_dump_cnt) :
+	       -EINVAL;
+}
+EXPORT_SYMBOL_GPL(pstore_region_defined);
+
 static void pstore_timer_kick(void)
 {
 	if (pstore_update_ms < 0)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ab551caa1d2a..62202f3ddf63 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -437,6 +437,47 @@  static int ramoops_pstore_erase(struct pstore_record *record)
 	return 0;
 }
 
+static int ramoops_region_info(struct pstore_record *record,
+			       void **virt, phys_addr_t *phys,
+			       size_t *size, unsigned int *max_dump_cnt)
+{
+	struct ramoops_context *cxt = record->psi->data;
+	struct persistent_ram_zone *prz;
+
+	switch (record->type) {
+	case PSTORE_TYPE_DMESG:
+		if (record->id >= cxt->max_dump_cnt)
+			return -EINVAL;
+		prz = cxt->dprzs[record->id];
+		*max_dump_cnt = cxt->max_dump_cnt;
+		break;
+	case PSTORE_TYPE_CONSOLE:
+		if (!cxt->console_size)
+			return -EINVAL;
+		prz = cxt->cprz;
+		break;
+	case PSTORE_TYPE_FTRACE:
+		if (record->id >= cxt->max_ftrace_cnt)
+			return -EINVAL;
+		prz = cxt->fprzs[record->id];
+		*max_dump_cnt = cxt->max_ftrace_cnt;
+		break;
+	case PSTORE_TYPE_PMSG:
+		if (!cxt->pmsg_size)
+			return -EINVAL;
+		prz = cxt->mprz;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	*virt = prz->vaddr;
+	*phys = prz->paddr;
+	*size = prz->size;
+
+	return 0;
+}
+
 static struct ramoops_context oops_cxt = {
 	.pstore = {
 		.owner	= THIS_MODULE,
@@ -446,6 +487,7 @@  static struct ramoops_context oops_cxt = {
 		.write	= ramoops_pstore_write,
 		.write_user	= ramoops_pstore_write_user,
 		.erase	= ramoops_pstore_erase,
+		.region_info = ramoops_region_info,
 	},
 };
 
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 638507a3c8ff..a64d866e8711 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -199,6 +199,9 @@  struct pstore_info {
 	int		(*write_user)(struct pstore_record *record,
 				      const char __user *buf);
 	int		(*erase)(struct pstore_record *record);
+	int		(*region_info)(struct pstore_record *record,
+				       void **virt, phys_addr_t *phys,
+				       size_t *size, unsigned int *max_dump_cnt);
 };
 
 /* Supported frontends */
@@ -230,6 +233,9 @@  struct pstore_ftrace_record {
 #define TS_CPU_SHIFT 8
 #define TS_CPU_MASK (BIT(TS_CPU_SHIFT) - 1)
 
+int pstore_region_defined(struct pstore_record *record,
+			  void **virt, phys_addr_t *phys,
+			  size_t *size, unsigned int *max_dump_cnt);
 /*
  * If CPU number can be stored in IP, store it there, otherwise store it in
  * the time stamp. This means more timestamp resolution is available when