Messages in this thread | | | Date | Mon, 9 Oct 2023 17:29:40 +0530 | Subject | Re: [REBASE PATCH v5 10/17] pstore: Add pstore_region_defined() helper and export it | From | 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@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..
| |