| Message ID | 20220714164330.311734558@goodmis.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
Hi Steven, On Thu, 2022-07-14 at 12:43 -0400, Steven Rostedt wrote: > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > Instead of open coding a __dynamic_array() with a fixed length (which > defeats the purpose of the dynamic array in the first place). Use the > new > __vstring() helper that will use a va_list and only write enough of > the > string into the ring buffer that is needed. > > Link: > https://urldefense.com/v3/__https://lkml.kernel.org/r/20220705224750.354926535@goodmis.org__;!!CTRNKA9wMg0ARbw!w8nx66BKDTtyusp5i2pyzOGNb-QyxIAWjoZwmSQY0zzor_rqvBgUm5__vKK98ApKcDic$ > > > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: linux-usb@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-mediatek@lists.infradead.org > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > --- > drivers/usb/mtu3/mtu3_trace.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/mtu3/mtu3_trace.h > b/drivers/usb/mtu3/mtu3_trace.h > index 1b897636daf2..ef3c17e2f8a6 100644 > --- a/drivers/usb/mtu3/mtu3_trace.h > +++ b/drivers/usb/mtu3/mtu3_trace.h > @@ -25,11 +25,11 @@ TRACE_EVENT(mtu3_log, > TP_ARGS(dev, vaf), > TP_STRUCT__entry( > __string(name, dev_name(dev)) > - __dynamic_array(char, msg, MTU3_MSG_MAX) > + __vstring(msg, vaf->fmt, vaf->va) > ), > TP_fast_assign( > __assign_str(name, dev_name(dev)); > - vsnprintf(__get_str(msg), MTU3_MSG_MAX, vaf->fmt, *vaf- > >va); > + __assign_vstr(msg, vaf->fmt, vaf->va); > ), > TP_printk("%s: %s", __get_str(name), __get_str(msg)) > ); Can you help to remove macro "MTU3_MSG_MAX" and one blank line after it in this file, this macro is not used anymore after apply this patch. Thanks a lot
On Thu, 2022-07-14 at 12:43 -0400, Steven Rostedt wrote: > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > Instead of open coding a __dynamic_array() with a fixed length (which > defeats the purpose of the dynamic array in the first place). Use the > new > __vstring() helper that will use a va_list and only write enough of > the > string into the ring buffer that is needed. > > Link: > https://urldefense.com/v3/__https://lkml.kernel.org/r/20220705224750.354926535@goodmis.org__;!!CTRNKA9wMg0ARbw!w8nx66BKDTtyusp5i2pyzOGNb-QyxIAWjoZwmSQY0zzor_rqvBgUm5__vKK98ApKcDic$ > > > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: linux-usb@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-mediatek@lists.infradead.org > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > --- > drivers/usb/mtu3/mtu3_trace.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/mtu3/mtu3_trace.h > b/drivers/usb/mtu3/mtu3_trace.h > index 1b897636daf2..ef3c17e2f8a6 100644 > --- a/drivers/usb/mtu3/mtu3_trace.h > +++ b/drivers/usb/mtu3/mtu3_trace.h > @@ -25,11 +25,11 @@ TRACE_EVENT(mtu3_log, > TP_ARGS(dev, vaf), > TP_STRUCT__entry( > __string(name, dev_name(dev)) > - __dynamic_array(char, msg, MTU3_MSG_MAX) > + __vstring(msg, vaf->fmt, vaf->va) > ), > TP_fast_assign( > __assign_str(name, dev_name(dev)); > - vsnprintf(__get_str(msg), MTU3_MSG_MAX, vaf->fmt, *vaf- > >va); > + __assign_vstr(msg, vaf->fmt, vaf->va); > ), > TP_printk("%s: %s", __get_str(name), __get_str(msg)) > ); After apply this patch, encounter an issue, please check it irq/254-1120100-137 [000] d..1. 266.549473: mtu3_u2_common_isr: (00000004) RESET irq/254-1120100-137 [000] d..1. 266.629399: mtu3_log: 11201000.usb: link speed super-speed irq/254-1120100-137 [000] d..1. 266.629662: mtu3_log: 11201000.usb: ep0_state SETUPr-speed irq/254-1120100-137 [000] d..1. 266.629668: mtu3_handle_setup: setup - 00 05 001c 0000 0000 irq/254-1120100-137 [000] d..1. 266.629722: mtu3_log: 11201000.usb: ep0_state SETUPr-speed without this patch: irq/254-1120100-135 [000] d..1. 1407.425550: mtu3_u2_common_isr: (00000004) RESET irq/254-1120100-135 [000] d..1. 1407.475295: mtu3_log: 11201000.usb: link speed super-speed irq/254-1120100-135 [000] d..1. 1407.477469: mtu3_log: 11201000.usb: ep0_state SETUP irq/254-1120100-135 [000] d..1. 1407.477476: mtu3_handle_setup: setup - 00 05 001f 0000 0000 irq/254-1120100-135 [000] d..1. 1407.477518: mtu3_log: 11201000.usb: ep0_state SETUP the second and third lines with this patch: irq/254-1120100-137 [000] d..1. 266.629399: mtu3_log: 11201000.usb: link speed super-speed irq/254-1120100-137 [000] d..1. 266.629662: mtu3_log: 11201000.usb: ep0_state SETUPr-speed "r-speed" seems the remain of last log;
On Fri, 15 Jul 2022 18:01:44 +0800 Chunfeng Yun <chunfeng.yun@mediatek.com> wrote: > the second and third lines with this patch: > irq/254-1120100-137 [000] d..1. 266.629399: mtu3_log: > 11201000.usb: link speed super-speed > > irq/254-1120100-137 [000] d..1. 266.629662: mtu3_log: > 11201000.usb: ep0_state SETUPr-speed > > "r-speed" seems the remain of last log; Thanks for testing! I'll investigate. (Another reason I didn't push to linux-next yet). -- Steve
On Fri, 15 Jul 2022 18:01:44 +0800 Chunfeng Yun <chunfeng.yun@mediatek.com> wrote: > irq/254-1120100-137 [000] d..1. 266.629662: mtu3_log: > 11201000.usb: ep0_state SETUPr-speed > > "r-speed" seems the remain of last log; I found an off-by-one bug in the vstring patch. I'll rebase, test and try again. In the mean time, care to add this on top to make sure it's fixed? Thanks! -- Steve diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h index e6f8ba52a958..b18759a673c6 100644 --- a/include/linux/trace_events.h +++ b/include/linux/trace_events.h @@ -922,16 +922,16 @@ perf_trace_buf_submit(void *raw_data, int size, int rctx, u16 type, * gcc warns that you can not use a va_list in an inlined * function. But lets me make it into a macro :-/ */ -#define __trace_event_vstr_len(fmt, va) \ -({ \ - va_list __ap; \ - int __ret; \ - \ - va_copy(__ap, *(va)); \ - __ret = vsnprintf(NULL, 0, fmt, __ap); \ - va_end(__ap); \ - \ - min(__ret, TRACE_EVENT_STR_MAX); \ +#define __trace_event_vstr_len(fmt, va) \ +({ \ + va_list __ap; \ + int __ret; \ + \ + va_copy(__ap, *(va)); \ + __ret = vsnprintf(NULL, 0, fmt, __ap) + 1; \ + va_end(__ap); \ + \ + min(__ret, TRACE_EVENT_STR_MAX); \ }) #endif /* _LINUX_TRACE_EVENT_H */
On Fri, 15 Jul 2022 14:32:05 +0800 Chunfeng Yun <chunfeng.yun@mediatek.com> wrote: > Can you help to remove macro "MTU3_MSG_MAX" and one blank line after it > in this file, this macro is not used anymore after apply this patch. Care to send me a patch, and I'll just include it in my series? -- Steve
On Fri, 2022-07-15 at 17:24 -0400, Steven Rostedt wrote: > On Fri, 15 Jul 2022 18:01:44 +0800 > Chunfeng Yun <chunfeng.yun@mediatek.com> wrote: > > > irq/254-1120100-137 [000] d..1. 266.629662: mtu3_log: > > 11201000.usb: ep0_state SETUPr-speed > > > > "r-speed" seems the remain of last log; > > I found an off-by-one bug in the vstring patch. I'll rebase, test and > try > again. > > In the mean time, care to add this on top to make sure it's fixed? > > Thanks! > > -- Steve > > diff --git a/include/linux/trace_events.h > b/include/linux/trace_events.h > index e6f8ba52a958..b18759a673c6 100644 > --- a/include/linux/trace_events.h > +++ b/include/linux/trace_events.h > @@ -922,16 +922,16 @@ perf_trace_buf_submit(void *raw_data, int size, > int rctx, u16 type, > * gcc warns that you can not use a va_list in an inlined > * function. But lets me make it into a macro :-/ > */ > -#define __trace_event_vstr_len(fmt, va) \ > -({ \ > - va_list __ap; \ > - int __ret; \ > - \ > - va_copy(__ap, *(va)); \ > - __ret = vsnprintf(NULL, 0, fmt, __ap); \ > - va_end(__ap); \ > - \ > - min(__ret, TRACE_EVENT_STR_MAX); \ > +#define __trace_event_vstr_len(fmt, va) \ > +({ \ > + va_list __ap; \ > + int __ret; \ > + \ > + va_copy(__ap, *(va)); \ > + __ret = vsnprintf(NULL, 0, fmt, __ap) + 1; \ It works fine now, thanks a lot > + va_end(__ap); \ > + \ > + min(__ret, TRACE_EVENT_STR_MAX); \ > }) > > #endif /* _LINUX_TRACE_EVENT_H */
On Fri, 2022-07-15 at 17:39 -0400, Steven Rostedt wrote: > On Fri, 15 Jul 2022 14:32:05 +0800 > Chunfeng Yun <chunfeng.yun@mediatek.com> wrote: > > > Can you help to remove macro "MTU3_MSG_MAX" and one blank line > > after it > > in this file, this macro is not used anymore after apply this > > patch. > > Care to send me a patch, and I'll just include it in my series? Seems no need add another patch, just modify this patch as below: diff --git a/drivers/usb/mtu3/mtu3_trace.h b/drivers/usb/mtu3/mtu3_trace.h index a09deae1146f..03d2a9bac27e 100644 --- a/drivers/usb/mtu3/mtu3_trace.h +++ b/drivers/usb/mtu3/mtu3_trace.h @@ -18,18 +18,16 @@ #include "mtu3.h" -#define MTU3_MSG_MAX 256 - TRACE_EVENT(mtu3_log, TP_PROTO(struct device *dev, struct va_format *vaf), TP_ARGS(dev, vaf), TP_STRUCT__entry( __string(name, dev_name(dev)) - __dynamic_array(char, msg, MTU3_MSG_MAX) + __vstring(msg, vaf->fmt, vaf->va) ), TP_fast_assign( __assign_str(name, dev_name(dev)); - vsnprintf(__get_str(msg), MTU3_MSG_MAX, vaf->fmt, *vaf- >va); + __assign_vstr(msg, vaf->fmt, vaf->va); ), TP_printk("%s: %s", __get_str(name), __get_str(msg)) ); > remove below two lines " -#define MTU3_MSG_MAX 256 - " Thanks > -- Steve
On Tue, 19 Jul 2022 13:23:06 +0800 Chunfeng Yun <chunfeng.yun@mediatek.com> wrote: > > Care to send me a patch, and I'll just include it in my series? > Seems no need add another patch, just modify this patch as below: > > diff --git a/drivers/usb/mtu3/mtu3_trace.h > b/drivers/usb/mtu3/mtu3_trace.h > index a09deae1146f..03d2a9bac27e 100644 > --- a/drivers/usb/mtu3/mtu3_trace.h > +++ b/drivers/usb/mtu3/mtu3_trace.h > @@ -18,18 +18,16 @@ > > #include "mtu3.h" > > -#define MTU3_MSG_MAX 256 > - > TRACE_EVENT(mtu3_log, > TP_PROTO(struct device *dev, struct va_format *vaf), > TP_ARGS(dev, vaf), > TP_STRUCT__entry( > __string(name, dev_name(dev)) > - __dynamic_array(char, msg, MTU3_MSG_MAX) > + __vstring(msg, vaf->fmt, vaf->va) > ), > TP_fast_assign( > __assign_str(name, dev_name(dev)); > - vsnprintf(__get_str(msg), MTU3_MSG_MAX, vaf->fmt, *vaf- > >va); > + __assign_vstr(msg, vaf->fmt, vaf->va); > ), > TP_printk("%s: %s", __get_str(name), __get_str(msg)) > ); > > > > remove below two lines > " > -#define MTU3_MSG_MAX 256 > - Fine. Even though I already pushed to linux-next, I did something I seldom do. I rebased my for-next branch and removed this patch. I'll send a v2. -- Steve
diff --git a/drivers/usb/mtu3/mtu3_trace.h b/drivers/usb/mtu3/mtu3_trace.h index 1b897636daf2..ef3c17e2f8a6 100644 --- a/drivers/usb/mtu3/mtu3_trace.h +++ b/drivers/usb/mtu3/mtu3_trace.h @@ -25,11 +25,11 @@ TRACE_EVENT(mtu3_log, TP_ARGS(dev, vaf), TP_STRUCT__entry( __string(name, dev_name(dev)) - __dynamic_array(char, msg, MTU3_MSG_MAX) + __vstring(msg, vaf->fmt, vaf->va) ), TP_fast_assign( __assign_str(name, dev_name(dev)); - vsnprintf(__get_str(msg), MTU3_MSG_MAX, vaf->fmt, *vaf->va); + __assign_vstr(msg, vaf->fmt, vaf->va); ), TP_printk("%s: %s", __get_str(name), __get_str(msg)) );