| Message ID | 20180727152003.11663-2-sibis@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add coredump support for Q6v5 Modem remoteproc | expand |
Hi Sibi, On 27-07-18, 20:49, Sibi Sankar wrote: > Introduce custom dump function per remoteproc segment. It is responsible > for filling the device memory segment associated with coredump > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > --- > drivers/remoteproc/remoteproc_core.c | 15 ++++++++++----- > include/linux/remoteproc.h | 3 +++ > 2 files changed, 13 insertions(+), 5 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 283b258f5e0f..ec56cd822b26 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1183,13 +1183,18 @@ static void rproc_coredump(struct rproc *rproc) > phdr->p_align = 0; > > ptr = rproc_da_to_va(rproc, segment->da, segment->size); > - if (!ptr) { > - dev_err(&rproc->dev, > + > + if (segment->dump) { > + segment->dump(rproc, ptr, segment->size, data + offset); Am not sure I follow, you are calling this w/o checking if ptr is valid, so you maybe passing null to segment->dump() ? > + } else { > + if (!ptr) { > + dev_err(&rproc->dev, > "invalid coredump segment (%pad, %zu)\n", > &segment->da, segment->size); > - memset(data + offset, 0xff, segment->size); > - } else { > - memcpy(data + offset, ptr, segment->size); > + memset(data + offset, 0xff, segment->size); > + } else { > + memcpy(data + offset, ptr, segment->size); > + }
Hi Vinod, Thanks for the review, On 08/07/2018 11:45 AM, Vinod wrote: > Hi Sibi, > > On 27-07-18, 20:49, Sibi Sankar wrote: >> Introduce custom dump function per remoteproc segment. It is responsible >> for filling the device memory segment associated with coredump >> >> Signed-off-by: Sibi Sankar <sibis@codeaurora.org> >> --- >> drivers/remoteproc/remoteproc_core.c | 15 ++++++++++----- >> include/linux/remoteproc.h | 3 +++ >> 2 files changed, 13 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c >> index 283b258f5e0f..ec56cd822b26 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -1183,13 +1183,18 @@ static void rproc_coredump(struct rproc *rproc) >> phdr->p_align = 0; >> >> ptr = rproc_da_to_va(rproc, segment->da, segment->size); >> - if (!ptr) { >> - dev_err(&rproc->dev, >> + >> + if (segment->dump) { >> + segment->dump(rproc, ptr, segment->size, data + offset); > > Am not sure I follow, you are calling this w/o checking if ptr is valid, > so you maybe passing null to segment->dump() ? > the rationale behind passing ptr directly to dump_fn is that it will help in tracking the segments being core dumped (q6v5_pil in particular requires to unlock mba before dumping and cleanup after all the segments are dumped which is currently decided based on a mask that is maintained). It also allows the remoteproc driver to fill the memory as needed (instead of the default 0xff). This is applicable to drivers that implement dump_fn, for others the default behavior is maintained. >> + } else { >> + if (!ptr) { >> + dev_err(&rproc->dev, >> "invalid coredump segment (%pad, %zu)\n", >> &segment->da, segment->size); >> - memset(data + offset, 0xff, segment->size); >> - } else { >> - memcpy(data + offset, ptr, segment->size); >> + memset(data + offset, 0xff, segment->size); >> + } else { >> + memcpy(data + offset, ptr, segment->size); >> + } >
On Fri 27 Jul 08:19 PDT 2018, Sibi Sankar wrote: > Introduce custom dump function per remoteproc segment. It is responsible > for filling the device memory segment associated with coredump > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > --- > drivers/remoteproc/remoteproc_core.c | 15 ++++++++++----- > include/linux/remoteproc.h | 3 +++ > 2 files changed, 13 insertions(+), 5 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 283b258f5e0f..ec56cd822b26 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1183,13 +1183,18 @@ static void rproc_coredump(struct rproc *rproc) > phdr->p_align = 0; > > ptr = rproc_da_to_va(rproc, segment->da, segment->size); > - if (!ptr) { > - dev_err(&rproc->dev, > + > + if (segment->dump) { > + segment->dump(rproc, ptr, segment->size, data + offset); rproc_da_to_va() is an exported symbol, so if you pass segment to the dump function the driver can, if it needs to, call the function itself. A typical use case, that I see, is to use the custom dump function to write out CPU or hardware state to the dump file, in which case the "da" won't be valid. So please make this call dump(rproc, segment, data + offset) and move the rproc_da_to_va() into the else block. > + } else { > + if (!ptr) { > + dev_err(&rproc->dev, > "invalid coredump segment (%pad, %zu)\n", > &segment->da, segment->size); > - memset(data + offset, 0xff, segment->size); > - } else { > - memcpy(data + offset, ptr, segment->size); > + memset(data + offset, 0xff, segment->size); > + } else { > + memcpy(data + offset, ptr, segment->size); > + } > } > > offset += phdr->p_filesz; > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > index e3c5d856b6da..0fbb01a9955c 100644 > --- a/include/linux/remoteproc.h > +++ b/include/linux/remoteproc.h > @@ -399,6 +399,8 @@ enum rproc_crash_type { > * @node: list node related to the rproc segment list > * @da: device address of the segment > * @size: size of the segment > + * @dump: custom dump function to fill device memory segment associated > + * with coredump > */ > struct rproc_dump_segment { > struct list_head node; > @@ -406,6 +408,7 @@ struct rproc_dump_segment { > dma_addr_t da; > size_t size; > > + void (*dump)(struct rproc *rproc, void *ptr, size_t len, void *priv); "priv" isn't the best name to represent the memory to which you expect dump to write to. Please call it "dest". > loff_t offset; > }; Regards, Bjorn
Hi Bjorn, Thanks for the review :) On 2018-10-08 11:53, Bjorn Andersson wrote: > On Fri 27 Jul 08:19 PDT 2018, Sibi Sankar wrote: > >> Introduce custom dump function per remoteproc segment. It is >> responsible >> for filling the device memory segment associated with coredump >> >> Signed-off-by: Sibi Sankar <sibis@codeaurora.org> >> --- >> drivers/remoteproc/remoteproc_core.c | 15 ++++++++++----- >> include/linux/remoteproc.h | 3 +++ >> 2 files changed, 13 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c >> b/drivers/remoteproc/remoteproc_core.c >> index 283b258f5e0f..ec56cd822b26 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -1183,13 +1183,18 @@ static void rproc_coredump(struct rproc >> *rproc) >> phdr->p_align = 0; >> >> ptr = rproc_da_to_va(rproc, segment->da, segment->size); >> - if (!ptr) { >> - dev_err(&rproc->dev, >> + >> + if (segment->dump) { >> + segment->dump(rproc, ptr, segment->size, data + offset); > > rproc_da_to_va() is an exported symbol, so if you pass segment to the > dump function the driver can, if it needs to, call the function itself. > > A typical use case, that I see, is to use the custom dump function to > write out CPU or hardware state to the dump file, in which case the > "da" > won't be valid. > > > So please make this call dump(rproc, segment, data + offset) and move > the rproc_da_to_va() into the else block. > yup will redefine it. >> + } else { >> + if (!ptr) { >> + dev_err(&rproc->dev, >> "invalid coredump segment (%pad, %zu)\n", >> &segment->da, segment->size); >> - memset(data + offset, 0xff, segment->size); >> - } else { >> - memcpy(data + offset, ptr, segment->size); >> + memset(data + offset, 0xff, segment->size); >> + } else { >> + memcpy(data + offset, ptr, segment->size); >> + } >> } >> >> offset += phdr->p_filesz; >> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h >> index e3c5d856b6da..0fbb01a9955c 100644 >> --- a/include/linux/remoteproc.h >> +++ b/include/linux/remoteproc.h >> @@ -399,6 +399,8 @@ enum rproc_crash_type { >> * @node: list node related to the rproc segment list >> * @da: device address of the segment >> * @size: size of the segment >> + * @dump: custom dump function to fill device memory segment >> associated >> + * with coredump >> */ >> struct rproc_dump_segment { >> struct list_head node; >> @@ -406,6 +408,7 @@ struct rproc_dump_segment { >> dma_addr_t da; >> size_t size; >> >> + void (*dump)(struct rproc *rproc, void *ptr, size_t len, void >> *priv); > > "priv" isn't the best name to represent the memory to which you expect > dump to write to. Please call it "dest". > will rename it >> loff_t offset; >> }; > > Regards, > Bjorn
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 283b258f5e0f..ec56cd822b26 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -1183,13 +1183,18 @@ static void rproc_coredump(struct rproc *rproc) phdr->p_align = 0; ptr = rproc_da_to_va(rproc, segment->da, segment->size); - if (!ptr) { - dev_err(&rproc->dev, + + if (segment->dump) { + segment->dump(rproc, ptr, segment->size, data + offset); + } else { + if (!ptr) { + dev_err(&rproc->dev, "invalid coredump segment (%pad, %zu)\n", &segment->da, segment->size); - memset(data + offset, 0xff, segment->size); - } else { - memcpy(data + offset, ptr, segment->size); + memset(data + offset, 0xff, segment->size); + } else { + memcpy(data + offset, ptr, segment->size); + } } offset += phdr->p_filesz; diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h index e3c5d856b6da..0fbb01a9955c 100644 --- a/include/linux/remoteproc.h +++ b/include/linux/remoteproc.h @@ -399,6 +399,8 @@ enum rproc_crash_type { * @node: list node related to the rproc segment list * @da: device address of the segment * @size: size of the segment + * @dump: custom dump function to fill device memory segment associated + * with coredump */ struct rproc_dump_segment { struct list_head node; @@ -406,6 +408,7 @@ struct rproc_dump_segment { dma_addr_t da; size_t size; + void (*dump)(struct rproc *rproc, void *ptr, size_t len, void *priv); loff_t offset; };
Introduce custom dump function per remoteproc segment. It is responsible for filling the device memory segment associated with coredump Signed-off-by: Sibi Sankar <sibis@codeaurora.org> --- drivers/remoteproc/remoteproc_core.c | 15 ++++++++++----- include/linux/remoteproc.h | 3 +++ 2 files changed, 13 insertions(+), 5 deletions(-)