Re: [PATCH 3/4] remoteproc: add support for a new 64-bit trace version

[Suman Anna]

On 5/21/20 2:42 PM, Suman Anna wrote:
> Hi Bjorn,
>
> On 5/21/20 1:04 PM, Bjorn Andersson wrote:
> > On Wed 25 Mar 13:47 PDT 2020, Suman Anna wrote:
> >
> > > Introduce a new trace entry resource structure that accommodates
> > > a 64-bit device address to support 64-bit processors. This is to
> > > be used using an overloaded version value of 1 in the upper 32-bits
> > > of the previous resource type field. The new resource still uses
> > > 32-bits for the length field (followed by a 32-bit reserved field,
> > > so can be updated in the future), which is a sufficiently large
> > > trace buffer size. A 32-bit padding field also had to be added
> > > to align the device address on a 64-bit boundary, and match the
> > > usage on the firmware side.
> > >
> > > The remoteproc debugfs logic also has been adjusted accordingly.
> > >
> > > Signed-off-by: Suman Anna <s-anna@xxxxxx>
> > > ---
> > > Â drivers/remoteproc/remoteproc_core.cÂÂÂ | 40 ++++++++++++++++++++-----
> > > Â drivers/remoteproc/remoteproc_debugfs.c | 37 ++++++++++++++++++-----
> > > Â include/linux/remoteproc.hÂÂÂÂÂÂÂÂÂÂÂÂÂ | 26 ++++++++++++++++
> > > Â 3 files changed, 87 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/remoteproc_core.c 
> > > b/drivers/remoteproc/remoteproc_core.c
> > > index 53bc37c508c6..b9a097990862 100644
> > > --- a/drivers/remoteproc/remoteproc_core.c
> > > +++ b/drivers/remoteproc/remoteproc_core.c
> > > @@ -609,21 +609,45 @@ void rproc_vdev_release(struct kref *ref)
> > > ÂÂ *
> > > ÂÂ * Returns 0 on success, or an appropriate error code otherwise
> > > ÂÂ */
> > > -static int rproc_handle_trace(struct rproc *rproc, struct 
> > > fw_rsc_trace *rsc,
> > > +static int rproc_handle_trace(struct rproc *rproc, void *rsc,
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ int offset, int avail, u16 ver)
> > > Â {
> > > ÂÂÂÂÂ struct rproc_debug_trace *trace;
> > > ÂÂÂÂÂ struct device *dev = &rproc->dev;
> > > +ÂÂÂ struct fw_rsc_trace *rsc1;
> > > +ÂÂÂ struct fw_rsc_trace2 *rsc2;
> > > ÂÂÂÂÂ char name[15];
> > > +ÂÂÂ size_t rsc_size;
> > > +ÂÂÂ u32 reserved;
> > > +ÂÂÂ u64 da;
> > > +ÂÂÂ u32 len;
> > > +
> > > +ÂÂÂ if (!ver) {
> >
> > This looks like a switch to me, but I also do think this looks rather
> > crude, if you spin off the tail of this function and call it from a
> > rproc_handle_trace() and rproc_handle_trace64() I believe this would be
> > cleaner.
>
> Yeah, ok. Will refactor for this in v2.
>
> > > +ÂÂÂÂÂÂÂ rsc1 = (struct fw_rsc_trace *)rsc;
> > > +ÂÂÂÂÂÂÂ rsc_size = sizeof(*rsc1);
> > > +ÂÂÂÂÂÂÂ reserved = rsc1->reserved;
> > > +ÂÂÂÂÂÂÂ da = rsc1->da;
> > > +ÂÂÂÂÂÂÂ len = rsc1->len;
> > > +ÂÂÂ } else if (ver == 1) {
> > > +ÂÂÂÂÂÂÂ rsc2 = (struct fw_rsc_trace2 *)rsc;
> > > +ÂÂÂÂÂÂÂ rsc_size = sizeof(*rsc2);
> > > +ÂÂÂÂÂÂÂ reserved = rsc2->reserved;
> > > +ÂÂÂÂÂÂÂ da = rsc2->da;
> > > +ÂÂÂÂÂÂÂ len = rsc2->len;
> > > +ÂÂÂ } else {
> > > +ÂÂÂÂÂÂÂ dev_err(dev, "unsupported trace rsc version %d\n", ver);
> >
> > If we use "type" to describe your 64-bit-da-trace then this sanity check
> > would have been taken care of by the core.
> >
> > > +ÂÂÂÂÂÂÂ return -EINVAL;
> > > +ÂÂÂ }
> > > -ÂÂÂ if (sizeof(*rsc) > avail) {
> > > +ÂÂÂ if (rsc_size > avail) {
> > > ÂÂÂÂÂÂÂÂÂ dev_err(dev, "trace rsc is truncated\n");
> > > ÂÂÂÂÂÂÂÂÂ return -EINVAL;
> > > ÂÂÂÂÂ }
> > > ÂÂÂÂÂ /* make sure reserved bytes are zeroes */
> > > -ÂÂÂ if (rsc->reserved) {
> > > -ÂÂÂÂÂÂÂ dev_err(dev, "trace rsc has non zero reserved bytes\n");
> > > +ÂÂÂ if (reserved) {
> > > +ÂÂÂÂÂÂÂ dev_err(dev, "trace rsc has non zero reserved bytes, value = 
> > > 0x%x\n",
> > > +ÂÂÂÂÂÂÂÂÂÂÂ reserved);
> > > ÂÂÂÂÂÂÂÂÂ return -EINVAL;
> > > ÂÂÂÂÂ }
> > > @@ -632,8 +656,8 @@ static int rproc_handle_trace(struct rproc 
> > > *rproc, struct fw_rsc_trace *rsc,
> > > ÂÂÂÂÂÂÂÂÂ return -ENOMEM;
> > > ÂÂÂÂÂ /* set the trace buffer dma properties */
> > > -ÂÂÂ trace->trace_mem.len = rsc->len;
> > > -ÂÂÂ trace->trace_mem.da = rsc->da;
> > > +ÂÂÂ trace->trace_mem.len = len;
> > > +ÂÂÂ trace->trace_mem.da = da;
> > > ÂÂÂÂÂ /* set pointer on rproc device */
> > > ÂÂÂÂÂ trace->rproc = rproc;
> > > @@ -652,8 +676,8 @@ static int rproc_handle_trace(struct rproc 
> > > *rproc, struct fw_rsc_trace *rsc,
> > > ÂÂÂÂÂ rproc->num_traces++;
> > > -ÂÂÂ dev_dbg(dev, "%s added: da 0x%x, len 0x%x\n",
> > > -ÂÂÂÂÂÂÂ name, rsc->da, rsc->len);
> > > +ÂÂÂ dev_dbg(dev, "%s added: da 0x%llx, len 0x%x\n",
> > > +ÂÂÂÂÂÂÂ name, da, len);
> > > ÂÂÂÂÂ return 0;
> > > Â }
> > > diff --git a/drivers/remoteproc/remoteproc_debugfs.c 
> > > b/drivers/remoteproc/remoteproc_debugfs.c
> > > index 3560eed7a360..ff43736db45a 100644
> > > --- a/drivers/remoteproc/remoteproc_debugfs.c
> > > +++ b/drivers/remoteproc/remoteproc_debugfs.c
> > > @@ -192,7 +192,8 @@ static int rproc_rsc_table_show(struct seq_file 
> > > *seq, void *p)
> > > ÂÂÂÂÂ struct resource_table *table = rproc->table_ptr;
> > > ÂÂÂÂÂ struct fw_rsc_carveout *c;
> > > ÂÂÂÂÂ struct fw_rsc_devmem *d;
> > > -ÂÂÂ struct fw_rsc_trace *t;
> > > +ÂÂÂ struct fw_rsc_trace *t1;
> > > +ÂÂÂ struct fw_rsc_trace2 *t2;
> > > ÂÂÂÂÂ struct fw_rsc_vdev *v;
> > > ÂÂÂÂÂ int i, j;
> > > @@ -205,6 +206,7 @@ static int rproc_rsc_table_show(struct seq_file 
> > > *seq, void *p)
> > > ÂÂÂÂÂÂÂÂÂ int offset = table->offset[i];
> > > ÂÂÂÂÂÂÂÂÂ struct fw_rsc_hdr *hdr = (void *)table + offset;
> > > ÂÂÂÂÂÂÂÂÂ void *rsc = (void *)hdr + sizeof(*hdr);
> > > +ÂÂÂÂÂÂÂ u16 ver = hdr->st.v;
> > > ÂÂÂÂÂÂÂÂÂ switch (hdr->st.t) {
> > > ÂÂÂÂÂÂÂÂÂ case RSC_CARVEOUT:
> > > @@ -230,13 +232,32 @@ static int rproc_rsc_table_show(struct seq_file 
> > > *seq, void *p)
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂ seq_printf(seq, "Â Name %s\n\n", d->name);
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂ break;
> > > ÂÂÂÂÂÂÂÂÂ case RSC_TRACE:
> > > -ÂÂÂÂÂÂÂÂÂÂÂ t = rsc;
> > > -ÂÂÂÂÂÂÂÂÂÂÂ seq_printf(seq, "Entry %d is of type %s\n",
> > > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ i, types[hdr->st.t]);
> > > -ÂÂÂÂÂÂÂÂÂÂÂ seq_printf(seq, "Â Device Address 0x%x\n", t->da);
> > > -ÂÂÂÂÂÂÂÂÂÂÂ seq_printf(seq, "Â Length 0x%x Bytes\n", t->len);
> > > -ÂÂÂÂÂÂÂÂÂÂÂ seq_printf(seq, "Â Reserved (should be zero) [%d]\n", 
> > > t->reserved);
> > > -ÂÂÂÂÂÂÂÂÂÂÂ seq_printf(seq, "Â Name %s\n\n", t->name);
> > > +ÂÂÂÂÂÂÂÂÂÂÂ if (ver == 0) {
> >
> > Again, this is a switch, here in a switch. Just defining a new
> > RSC_TRACE64 type would reduce the amount of code here...
>
> OK.
>
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ t1 = rsc;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ seq_printf(seq, "Entry %d is version %d of type %s\n",
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ i, ver, types[hdr->st.t]);
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ seq_printf(seq, "Â Device Address 0x%x\n",
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ t1->da);
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ seq_printf(seq, "Â Length 0x%x Bytes\n",
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ t1->len);
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ seq_printf(seq, "Â Reserved (should be zero) [%d]\n",
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ t1->reserved);
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ seq_printf(seq, "Â Name %s\n\n", t1->name);
> > > +ÂÂÂÂÂÂÂÂÂÂÂ } else if (ver == 1) {
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ t2 = rsc;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ seq_printf(seq, "Entry %d is version %d of type %s\n",
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ i, ver, types[hdr->st.t]);
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ seq_printf(seq, "Â Device Address 0x%llx\n",
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ t2->da);
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ seq_printf(seq, "Â Length 0x%x Bytes\n",
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ t2->len);
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ seq_printf(seq, "Â Reserved (should be zero) [%d]\n",
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ t2->reserved);
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ seq_printf(seq, "Â Name %s\n\n", t2->name);
> > > +ÂÂÂÂÂÂÂÂÂÂÂ } else {
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ seq_printf(seq, "Entry %d is an unsupported version 
> > > %d of type %s\n",
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ i, ver, types[hdr->st.t]);
> > > +ÂÂÂÂÂÂÂÂÂÂÂ }
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂ break;
> > > ÂÂÂÂÂÂÂÂÂ case RSC_VDEV:
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂ v = rsc;
> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > > index 526d3cb45e37..3b3bea42f8b1 100644
> > > --- a/include/linux/remoteproc.h
> > > +++ b/include/linux/remoteproc.h
> > > @@ -243,6 +243,32 @@ struct fw_rsc_trace {
> > > ÂÂÂÂÂ u8 name[32];
> > > Â } __packed;
> > > +/**
> > > + * struct fw_rsc_trace2 - trace buffer declaration supporting 64-bits
> > > + * @padding: initial padding after type field for aligned 64-bit access
> > > + * @da: device address (64-bit)
> > > + * @len: length (in bytes)
> > > + * @reserved: reserved (must be zero)
> > > + * @name: human-readable name of the trace buffer
> > > + *
> > > + * This resource entry is an enhanced version of the fw_rsc_trace 
> > > resourec entry
> > > + * and the provides equivalent functionality but designed for 64-bit 
> > > remote
> > > + * processors.
> > > + *
> > > + * @da specifies the device address of the buffer, @len specifies
> > > + * its size, and @name may contain a human readable name of the 
> > > trace buffer.
> > > + *
> > > + * After booting the remote processor, the trace buffers are exposed 
> > > to the
> > > + * user via debugfs entries (called trace0, trace1, etc..).
> > > + */
> > > +struct fw_rsc_trace2 {
> >
> > Sounds more like fw_rsc_trace64 to me - in particular since the version
> > of trace2 is 1...
>
> Yeah, will rename this.
>
> > > +ÂÂÂ u32 padding;
> > > +ÂÂÂ u64 da;
> > > +ÂÂÂ u32 len;
> > > +ÂÂÂ u32 reserved;
> >
> > What's the purpose of this reserved field?
>
> Partly to make sure the entire resource is aligned on an 8-byte, and 
> partly copied over from fw_rsc_trace entry. I guess 32-bits is already 
> large enough of a size for trace entries irrespective of 32-bit or 
> 64-bit traces, so I doubt if we want to make the len field also a u64.

Looking at this again, I can drop both padding and reserved fields, if I 
move the len field before da. Any preferences/comments?

regards
Suman

> regards
> Suman
>
> > Regards,
> > Bjorn
> >
> > > +ÂÂÂ u8 name[32];
> > > +} __packed;
> > > +
> > > Â /**
> > > ÂÂ * struct fw_rsc_vdev_vring - vring descriptor entry
> > > ÂÂ * @da: device address
> > > --
> > > 2.23.0