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 <[email protected]>
> ---
> 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...
--
Kees Cook
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 <[email protected]>
>> ---
>> 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..