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

[Mukesh Ojha]

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