[PATCH] remoteproc: debug: remove strlen call in rproc_trace_read

[PATCH] remoteproc: debug: remove strlen call in rproc_trace_read

[Xiang Xiao]

because the trace buffer may contain the binary data

Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
---
 drivers/remoteproc/remoteproc_debugfs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
index e90135c..0808466 100644
--- a/drivers/remoteproc/remoteproc_debugfs.c
+++ b/drivers/remoteproc/remoteproc_debugfs.c
@@ -48,9 +48,8 @@ static ssize_t rproc_trace_read(struct file *filp, char __user *userbuf,
 				size_t count, loff_t *ppos)
 {
 	struct rproc_mem_entry *trace = filp->private_data;
-	int len = strnlen(trace->va, trace->len);
 
-	return simple_read_from_buffer(userbuf, count, ppos, trace->va, len);
+	return simple_read_from_buffer(userbuf, count, ppos, trace->va, trace->len);
 }
 
 static const struct file_operations trace_rproc_ops = {
-- 
2.7.4

Re: [PATCH] remoteproc: debug: remove strlen call in rproc_trace_read

[Bjorn Andersson]

On Wed 07 Nov 07:06 PST 2018, Xiang Xiao wrote:

> because the trace buffer may contain the binary data
> 

I think this patch is good.

@Suman, as I know you're using this and would now read past the first \0
in the buffer, do you have any objections to it?

Regards,
Bjorn

> Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
> ---
>  drivers/remoteproc/remoteproc_debugfs.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
> index e90135c..0808466 100644
> --- a/drivers/remoteproc/remoteproc_debugfs.c
> +++ b/drivers/remoteproc/remoteproc_debugfs.c
> @@ -48,9 +48,8 @@ static ssize_t rproc_trace_read(struct file *filp, char __user *userbuf,
>  				size_t count, loff_t *ppos)
>  {
>  	struct rproc_mem_entry *trace = filp->private_data;
> -	int len = strnlen(trace->va, trace->len);
>  
> -	return simple_read_from_buffer(userbuf, count, ppos, trace->va, len);
> +	return simple_read_from_buffer(userbuf, count, ppos, trace->va, trace->len);
>  }
>  
>  static const struct file_operations trace_rproc_ops = {
> -- 
> 2.7.4
> 

RE: [PATCH] remoteproc: debug: remove strlen call in rproc_trace_read

[Loic PALLARDY]

> -----Original Message-----
> From: linux-remoteproc-owner@vger.kernel.org <linux-remoteproc-
> owner@vger.kernel.org> On Behalf Of Bjorn Andersson
> Sent: jeudi 8 novembre 2018 07:14
> To: Suman Anna <s-anna@ti.com>; Xiang Xiao
> <xiaoxiang781216@gmail.com>
> Cc: linux-remoteproc@vger.kernel.org; Xiang Xiao <xiaoxiang@xiaomi.com>
> Subject: Re: [PATCH] remoteproc: debug: remove strlen call in
> rproc_trace_read
> 
> On Wed 07 Nov 07:06 PST 2018, Xiang Xiao wrote:
> 
> > because the trace buffer may contain the binary data
> >
> 
> I think this patch is good.

It isn't for me as trace buffer is only used for printf like traces. If we read past the first \0 we will display some noise and depending on trace buffer size make trace buffer unreadable on console thanks to a basic 'cat'.
But I recognize binary trace format need.
I would suggest to add a parameter in trace resource to specify the type of trace: text or binary and adapt function behavior according to this.

Regards,
Loic
> 
> @Suman, as I know you're using this and would now read past the first \0
> in the buffer, do you have any objections to it?
> 
> Regards,
> Bjorn
> 
> > Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
> > ---
> >  drivers/remoteproc/remoteproc_debugfs.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_debugfs.c
> b/drivers/remoteproc/remoteproc_debugfs.c
> > index e90135c..0808466 100644
> > --- a/drivers/remoteproc/remoteproc_debugfs.c
> > +++ b/drivers/remoteproc/remoteproc_debugfs.c
> > @@ -48,9 +48,8 @@ static ssize_t rproc_trace_read(struct file *filp, char
> __user *userbuf,
> >  				size_t count, loff_t *ppos)
> >  {
> >  	struct rproc_mem_entry *trace = filp->private_data;
> > -	int len = strnlen(trace->va, trace->len);
> >
> > -	return simple_read_from_buffer(userbuf, count, ppos, trace->va,
> len);
> > +	return simple_read_from_buffer(userbuf, count, ppos, trace->va,
> trace->len);
> >  }
> >
> >  static const struct file_operations trace_rproc_ops = {
> > --
> > 2.7.4
> >

Re: [PATCH] remoteproc: debug: remove strlen call in rproc_trace_read

[xiang xiao]

[-- Attachment #1: Type: text/plain, Size: 3896 bytes --]

Hi Loic,
If the trace buffer is zero out during the remote core initializition,
this patch will work correctly.
Actually, we use trace buffer in two case:
1.A circle buffer to save the log doesn't output yet like yours
2.Record the RTOS schedule event much like ftrace which is in binary format
Here is our usage in the first case:
1.Zero out the circle buffer during initializition
2.But check if the whole buffer just contain ASCII or zero
   a.If so, this is the last time panic/watchdog log
   b.Resend this log to UART/kernel in the second boot
3.And we will zero the segment once we send the log from buffer to UART/kernel
With this, we can cat the trace node to see the last log if the remote
core hang suddenly.
And see the log in the kernel log after the remote or system reboot.
The attached diagram show the workflow in more detail.
Here is the output with this patch in our evb:
banks-evb:/ # /bin/busybox hexdump /d/remoteproc/remoteproc2/trace0
0000000 0000 0000 0000 0000 0000 0000 0000 0000
*
0000060 0000 0000 205b 2020 3020 302e 3530 3532
0000070 5d38 7320 6e65 6f73 3a72 4e20 7475 5874
0000080 7620 7265 6973 6e6f 3720 322e 2036 3632
0000090 6631 3338 2d66 6964 7472 2079 6f4e 2076
00000a0 3820 3220 3130 2038 3631 353a 3a30 3734
00000b0 0a0d 0000 0000 0000 0000 0000 0000 0000
00000c0 0000 0000 0000 0000 0000 0000 0000 0000
*
0001000
banks-evb:/ # cat /d/remoteproc/re
remoteproc0/  remoteproc1/  remoteproc2/
banks-evb:/ # cat /d/remoteproc/remoteproc2/trace0
[    0.005258] sensor: NuttX version 7.26 261f83f-dirty Nov  8 2018 16:50:47
And you can see cat skip all zero smartly.

Thanks
Xiang

On Thu, Nov 8, 2018 at 4:10 PM Loic PALLARDY <loic.pallardy@st.com> wrote:
>
>
>
> > -----Original Message-----
> > From: linux-remoteproc-owner@vger.kernel.org <linux-remoteproc-
> > owner@vger.kernel.org> On Behalf Of Bjorn Andersson
> > Sent: jeudi 8 novembre 2018 07:14
> > To: Suman Anna <s-anna@ti.com>; Xiang Xiao
> > <xiaoxiang781216@gmail.com>
> > Cc: linux-remoteproc@vger.kernel.org; Xiang Xiao <xiaoxiang@xiaomi.com>
> > Subject: Re: [PATCH] remoteproc: debug: remove strlen call in
> > rproc_trace_read
> >
> > On Wed 07 Nov 07:06 PST 2018, Xiang Xiao wrote:
> >
> > > because the trace buffer may contain the binary data
> > >
> >
> > I think this patch is good.
>
> It isn't for me as trace buffer is only used for printf like traces. If we read past the first \0 we will display some noise and depending on trace buffer size make trace buffer unreadable on console thanks to a basic 'cat'.
> But I recognize binary trace format need.
> I would suggest to add a parameter in trace resource to specify the type of trace: text or binary and adapt function behavior according to this.
>
> Regards,
> Loic
> >
> > @Suman, as I know you're using this and would now read past the first \0
> > in the buffer, do you have any objections to it?
> >
> > Regards,
> > Bjorn
> >
> > > Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
> > > ---
> > >  drivers/remoteproc/remoteproc_debugfs.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/remoteproc_debugfs.c
> > b/drivers/remoteproc/remoteproc_debugfs.c
> > > index e90135c..0808466 100644
> > > --- a/drivers/remoteproc/remoteproc_debugfs.c
> > > +++ b/drivers/remoteproc/remoteproc_debugfs.c
> > > @@ -48,9 +48,8 @@ static ssize_t rproc_trace_read(struct file *filp, char
> > __user *userbuf,
> > >                             size_t count, loff_t *ppos)
> > >  {
> > >     struct rproc_mem_entry *trace = filp->private_data;
> > > -   int len = strnlen(trace->va, trace->len);
> > >
> > > -   return simple_read_from_buffer(userbuf, count, ppos, trace->va,
> > len);
> > > +   return simple_read_from_buffer(userbuf, count, ppos, trace->va,
> > trace->len);
> > >  }
> > >
> > >  static const struct file_operations trace_rproc_ops = {
> > > --
> > > 2.7.4
> > >

[-- Attachment #2: syslog.JPG --]
[-- Type: image/jpeg, Size: 85722 bytes --]

RE: [PATCH] remoteproc: debug: remove strlen call in rproc_trace_read

[Loic PALLARDY]

> -----Original Message-----
> From: xiang xiao <xiaoxiang781216@gmail.com>
> Sent: jeudi 8 novembre 2018 11:08
> To: Loic PALLARDY <loic.pallardy@st.com>
> Cc: bjorn.andersson@linaro.org; s-anna@ti.com; linux-
> remoteproc@vger.kernel.org; 肖翔 <xiaoxiang@xiaomi.com>
> Subject: Re: [PATCH] remoteproc: debug: remove strlen call in
> rproc_trace_read
> 
> Hi Loic,
> If the trace buffer is zero out during the remote core initializition,
> this patch will work correctly.

Thanks for this clear clarification.
Indeed you pointed the main point (issue), buffer should be first zero initialized. But as coprocessor should write an initial trace at startup, before user space can access trace buffer, it should be ok.

> Actually, we use trace buffer in two case:
> 1.A circle buffer to save the log doesn't output yet like yours
> 2.Record the RTOS schedule event much like ftrace which is in binary format
> Here is our usage in the first case:
> 1.Zero out the circle buffer during initializition
> 2.But check if the whole buffer just contain ASCII or zero
>    a.If so, this is the last time panic/watchdog log
>    b.Resend this log to UART/kernel in the second boot
> 3.And we will zero the segment once we send the log from buffer to
> UART/kernel
> With this, we can cat the trace node to see the last log if the remote
> core hang suddenly.
> And see the log in the kernel log after the remote or system reboot.
> The attached diagram show the workflow in more detail.

Today one main issue of this trace buffer is circular management which is not supported.
As soon as traces are looping, we are losing part of log.
I saw on your diagram rpmsg-syslog service to manage trace buffer. Any plan to upstream this service ?

> Here is the output with this patch in our evb:
> banks-evb:/ # /bin/busybox hexdump /d/remoteproc/remoteproc2/trace0
> 0000000 0000 0000 0000 0000 0000 0000 0000 0000
> *
> 0000060 0000 0000 205b 2020 3020 302e 3530 3532
> 0000070 5d38 7320 6e65 6f73 3a72 4e20 7475 5874
> 0000080 7620 7265 6973 6e6f 3720 322e 2036 3632
> 0000090 6631 3338 2d66 6964 7472 2079 6f4e 2076
> 00000a0 3820 3220 3130 2038 3631 353a 3a30 3734
> 00000b0 0a0d 0000 0000 0000 0000 0000 0000 0000
> 00000c0 0000 0000 0000 0000 0000 0000 0000 0000
> *
> 0001000
> banks-evb:/ # cat /d/remoteproc/re
> remoteproc0/  remoteproc1/  remoteproc2/
> banks-evb:/ # cat /d/remoteproc/remoteproc2/trace0
> [    0.005258] sensor: NuttX version 7.26 261f83f-dirty Nov  8 2018 16:50:47
> And you can see cat skip all zero smartly.

Yes true 'cat' will do the job.
I'll test tomorrow and let you know.

Regards,
Loic

> 
> Thanks
> Xiang
> 
> On Thu, Nov 8, 2018 at 4:10 PM Loic PALLARDY <loic.pallardy@st.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: linux-remoteproc-owner@vger.kernel.org <linux-remoteproc-
> > > owner@vger.kernel.org> On Behalf Of Bjorn Andersson
> > > Sent: jeudi 8 novembre 2018 07:14
> > > To: Suman Anna <s-anna@ti.com>; Xiang Xiao
> > > <xiaoxiang781216@gmail.com>
> > > Cc: linux-remoteproc@vger.kernel.org; Xiang Xiao
> <xiaoxiang@xiaomi.com>
> > > Subject: Re: [PATCH] remoteproc: debug: remove strlen call in
> > > rproc_trace_read
> > >
> > > On Wed 07 Nov 07:06 PST 2018, Xiang Xiao wrote:
> > >
> > > > because the trace buffer may contain the binary data
> > > >
> > >
> > > I think this patch is good.
> >
> > It isn't for me as trace buffer is only used for printf like traces. If we read
> past the first \0 we will display some noise and depending on trace buffer
> size make trace buffer unreadable on console thanks to a basic 'cat'.
> > But I recognize binary trace format need.
> > I would suggest to add a parameter in trace resource to specify the type of
> trace: text or binary and adapt function behavior according to this.
> >
> > Regards,
> > Loic
> > >
> > > @Suman, as I know you're using this and would now read past the first \0
> > > in the buffer, do you have any objections to it?
> > >
> > > Regards,
> > > Bjorn
> > >
> > > > Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
> > > > ---
> > > >  drivers/remoteproc/remoteproc_debugfs.c | 3 +--
> > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/remoteproc/remoteproc_debugfs.c
> > > b/drivers/remoteproc/remoteproc_debugfs.c
> > > > index e90135c..0808466 100644
> > > > --- a/drivers/remoteproc/remoteproc_debugfs.c
> > > > +++ b/drivers/remoteproc/remoteproc_debugfs.c
> > > > @@ -48,9 +48,8 @@ static ssize_t rproc_trace_read(struct file *filp,
> char
> > > __user *userbuf,
> > > >                             size_t count, loff_t *ppos)
> > > >  {
> > > >     struct rproc_mem_entry *trace = filp->private_data;
> > > > -   int len = strnlen(trace->va, trace->len);
> > > >
> > > > -   return simple_read_from_buffer(userbuf, count, ppos, trace->va,
> > > len);
> > > > +   return simple_read_from_buffer(userbuf, count, ppos, trace->va,
> > > trace->len);
> > > >  }
> > > >
> > > >  static const struct file_operations trace_rproc_ops = {
> > > > --
> > > > 2.7.4
> > > >

Re: [PATCH] remoteproc: debug: remove strlen call in rproc_trace_read

[xiang xiao]

[-- Attachment #1: Type: text/plain, Size: 6108 bytes --]

On Fri, Nov 9, 2018 at 5:01 AM Loic PALLARDY <loic.pallardy@st.com> wrote:
>
>
>
> > -----Original Message-----
> > From: xiang xiao <xiaoxiang781216@gmail.com>
> > Sent: jeudi 8 novembre 2018 11:08
> > To: Loic PALLARDY <loic.pallardy@st.com>
> > Cc: bjorn.andersson@linaro.org; s-anna@ti.com; linux-
> > remoteproc@vger.kernel.org; 肖翔 <xiaoxiang@xiaomi.com>
> > Subject: Re: [PATCH] remoteproc: debug: remove strlen call in
> > rproc_trace_read
> >
> > Hi Loic,
> > If the trace buffer is zero out during the remote core initializition,
> > this patch will work correctly.
>
> Thanks for this clear clarification.
> Indeed you pointed the main point (issue), buffer should be first zero initialized. But as coprocessor should write an initial trace at startup, before user space can access trace buffer, it should be ok.
>
> > Actually, we use trace buffer in two case:
> > 1.A circle buffer to save the log doesn't output yet like yours
> > 2.Record the RTOS schedule event much like ftrace which is in binary format
> > Here is our usage in the first case:
> > 1.Zero out the circle buffer during initializition
> > 2.But check if the whole buffer just contain ASCII or zero
> >    a.If so, this is the last time panic/watchdog log
> >    b.Resend this log to UART/kernel in the second boot
> > 3.And we will zero the segment once we send the log from buffer to
> > UART/kernel
> > With this, we can cat the trace node to see the last log if the remote
> > core hang suddenly.
> > And see the log in the kernel log after the remote or system reboot.
> > The attached diagram show the workflow in more detail.
>
> Today one main issue of this trace buffer is circular management which is not supported.
> As soon as traces are looping, we are losing part of log.
Yes, this is a big problem, we resolve this issue by zero out the
buffer segment once it is sent to kernel/UART,
So the circle buffer always contain one or two zero regions to
identify where is the useful log.

> I saw on your diagram rpmsg-syslog service to manage trace buffer. Any plan to upstream this service ?
Yes, we are planing to upstream it, actually we have prepared several
other general rpmsg services.
Here is the link contain the pathset for these general rpmsg service:
https://github.com/thesofproject/linux/pull/177
which is base on 4.19 SOF kernel, I will rebase to remoteproc base in
the next couple days.
The attached slide describle these general services in detail.

>
> > Here is the output with this patch in our evb:
> > banks-evb:/ # /bin/busybox hexdump /d/remoteproc/remoteproc2/trace0
> > 0000000 0000 0000 0000 0000 0000 0000 0000 0000
> > *
> > 0000060 0000 0000 205b 2020 3020 302e 3530 3532
> > 0000070 5d38 7320 6e65 6f73 3a72 4e20 7475 5874
> > 0000080 7620 7265 6973 6e6f 3720 322e 2036 3632
> > 0000090 6631 3338 2d66 6964 7472 2079 6f4e 2076
> > 00000a0 3820 3220 3130 2038 3631 353a 3a30 3734
> > 00000b0 0a0d 0000 0000 0000 0000 0000 0000 0000
> > 00000c0 0000 0000 0000 0000 0000 0000 0000 0000
> > *
> > 0001000
> > banks-evb:/ # cat /d/remoteproc/re
> > remoteproc0/  remoteproc1/  remoteproc2/
> > banks-evb:/ # cat /d/remoteproc/remoteproc2/trace0
> > [    0.005258] sensor: NuttX version 7.26 261f83f-dirty Nov  8 2018 16:50:47
> > And you can see cat skip all zero smartly.
>
> Yes true 'cat' will do the job.
> I'll test tomorrow and let you know.
>
> Regards,
> Loic
>
> >
> > Thanks
> > Xiang
> >
> > On Thu, Nov 8, 2018 at 4:10 PM Loic PALLARDY <loic.pallardy@st.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: linux-remoteproc-owner@vger.kernel.org <linux-remoteproc-
> > > > owner@vger.kernel.org> On Behalf Of Bjorn Andersson
> > > > Sent: jeudi 8 novembre 2018 07:14
> > > > To: Suman Anna <s-anna@ti.com>; Xiang Xiao
> > > > <xiaoxiang781216@gmail.com>
> > > > Cc: linux-remoteproc@vger.kernel.org; Xiang Xiao
> > <xiaoxiang@xiaomi.com>
> > > > Subject: Re: [PATCH] remoteproc: debug: remove strlen call in
> > > > rproc_trace_read
> > > >
> > > > On Wed 07 Nov 07:06 PST 2018, Xiang Xiao wrote:
> > > >
> > > > > because the trace buffer may contain the binary data
> > > > >
> > > >
> > > > I think this patch is good.
> > >
> > > It isn't for me as trace buffer is only used for printf like traces. If we read
> > past the first \0 we will display some noise and depending on trace buffer
> > size make trace buffer unreadable on console thanks to a basic 'cat'.
> > > But I recognize binary trace format need.
> > > I would suggest to add a parameter in trace resource to specify the type of
> > trace: text or binary and adapt function behavior according to this.
> > >
> > > Regards,
> > > Loic
> > > >
> > > > @Suman, as I know you're using this and would now read past the first \0
> > > > in the buffer, do you have any objections to it?
> > > >
> > > > Regards,
> > > > Bjorn
> > > >
> > > > > Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
> > > > > ---
> > > > >  drivers/remoteproc/remoteproc_debugfs.c | 3 +--
> > > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/remoteproc/remoteproc_debugfs.c
> > > > b/drivers/remoteproc/remoteproc_debugfs.c
> > > > > index e90135c..0808466 100644
> > > > > --- a/drivers/remoteproc/remoteproc_debugfs.c
> > > > > +++ b/drivers/remoteproc/remoteproc_debugfs.c
> > > > > @@ -48,9 +48,8 @@ static ssize_t rproc_trace_read(struct file *filp,
> > char
> > > > __user *userbuf,
> > > > >                             size_t count, loff_t *ppos)
> > > > >  {
> > > > >     struct rproc_mem_entry *trace = filp->private_data;
> > > > > -   int len = strnlen(trace->va, trace->len);
> > > > >
> > > > > -   return simple_read_from_buffer(userbuf, count, ppos, trace->va,
> > > > len);
> > > > > +   return simple_read_from_buffer(userbuf, count, ppos, trace->va,
> > > > trace->len);
> > > > >  }
> > > > >
> > > > >  static const struct file_operations trace_rproc_ops = {
> > > > > --
> > > > > 2.7.4
> > > > >

[-- Attachment #2: rpmsg.pdf --]
[-- Type: application/pdf, Size: 195371 bytes --]