* [PATCH v2] cxl/trace: Properly initialize cxl_poison region name
@ 2024-03-14 20:12 alison.schofield
2024-03-14 20:41 ` Steven Rostedt
2024-03-14 20:51 ` Ira Weiny
0 siblings, 2 replies; 10+ messages in thread
From: alison.schofield @ 2024-03-14 20:12 UTC (permalink / raw)
To: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma,
Ira Weiny, Dan Williams, Steve Rostedt
Cc: Alison Schofield, linux-cxl
From: Alison Schofield <alison.schofield@intel.com>
The TP_STRUCT__entry that gets assigned the region name, or an
empty string if no region is present, is erroneously initialized
to the cxl_region pointer. It needs to be properly initialized
otherwise it's length is wrong and garbage chars can appear in
the kernel trace output: /sys/kernel/tracing/trace
The bad initialization was due in part to a naming conflict with
the parameter: struct cxl_region *region. The field 'region' is
already exposed externally as the region name, so changing that
to something logical, like 'region_name' is not an option. Instead
rename the internal only struct cxl_region to the commonly used
'cxlr'.
Impact is that tooling depending on that trace data can miss
picking up a valid event when searching by region name. The
TP_printk() output, if enabled, does emit the correct region
names in the dmesg log.
This was found during testing of the cxl-list option to report
media-errors for a region.
Fixes: ddf49d57b841 ("cxl/trace: Add TRACE support for CXL media-error records")
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
Changes in v2:
- Initialize the region name with an assignment (Steve)
- Rename struct cxl_region 'cxlr' instead of overusing 'region' identifier
- Update commit message & log
drivers/cxl/core/trace.h | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
index bdf117a33744..e5f13260fc52 100644
--- a/drivers/cxl/core/trace.h
+++ b/drivers/cxl/core/trace.h
@@ -646,18 +646,18 @@ u64 cxl_trace_hpa(struct cxl_region *cxlr, struct cxl_memdev *memdev, u64 dpa);
TRACE_EVENT(cxl_poison,
- TP_PROTO(struct cxl_memdev *cxlmd, struct cxl_region *region,
+ TP_PROTO(struct cxl_memdev *cxlmd, struct cxl_region *cxlr,
const struct cxl_poison_record *record, u8 flags,
__le64 overflow_ts, enum cxl_poison_trace_type trace_type),
- TP_ARGS(cxlmd, region, record, flags, overflow_ts, trace_type),
+ TP_ARGS(cxlmd, cxlr, record, flags, overflow_ts, trace_type),
TP_STRUCT__entry(
__string(memdev, dev_name(&cxlmd->dev))
__string(host, dev_name(cxlmd->dev.parent))
__field(u64, serial)
__field(u8, trace_type)
- __string(region, region)
+ __string(region, cxlr ? dev_name(&cxlr->dev) : "")
__field(u64, overflow_ts)
__field(u64, hpa)
__field(u64, dpa)
@@ -677,10 +677,10 @@ TRACE_EVENT(cxl_poison,
__entry->source = cxl_poison_record_source(record);
__entry->trace_type = trace_type;
__entry->flags = flags;
- if (region) {
- __assign_str(region, dev_name(®ion->dev));
- memcpy(__entry->uuid, ®ion->params.uuid, 16);
- __entry->hpa = cxl_trace_hpa(region, cxlmd,
+ if (cxlr) {
+ __assign_str(region, dev_name(&cxlr->dev));
+ memcpy(__entry->uuid, &cxlr->params.uuid, 16);
+ __entry->hpa = cxl_trace_hpa(cxlr, cxlmd,
__entry->dpa);
} else {
__assign_str(region, "");
base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
--
2.37.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] cxl/trace: Properly initialize cxl_poison region name
2024-03-14 20:12 [PATCH v2] cxl/trace: Properly initialize cxl_poison region name alison.schofield
@ 2024-03-14 20:41 ` Steven Rostedt
2024-03-14 21:05 ` Alison Schofield
2024-03-14 20:51 ` Ira Weiny
1 sibling, 1 reply; 10+ messages in thread
From: Steven Rostedt @ 2024-03-14 20:41 UTC (permalink / raw)
To: alison.schofield
Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma,
Ira Weiny, Dan Williams, linux-cxl
On Thu, 14 Mar 2024 13:12:17 -0700
alison.schofield@intel.com wrote:
> From: Alison Schofield <alison.schofield@intel.com>
>
> The TP_STRUCT__entry that gets assigned the region name, or an
> empty string if no region is present, is erroneously initialized
> to the cxl_region pointer. It needs to be properly initialized
> otherwise it's length is wrong and garbage chars can appear in
> the kernel trace output: /sys/kernel/tracing/trace
>
> The bad initialization was due in part to a naming conflict with
> the parameter: struct cxl_region *region. The field 'region' is
> already exposed externally as the region name, so changing that
> to something logical, like 'region_name' is not an option. Instead
> rename the internal only struct cxl_region to the commonly used
> 'cxlr'.
>
> Impact is that tooling depending on that trace data can miss
> picking up a valid event when searching by region name. The
> TP_printk() output, if enabled, does emit the correct region
> names in the dmesg log.
>
> This was found during testing of the cxl-list option to report
> media-errors for a region.
>
> Fixes: ddf49d57b841 ("cxl/trace: Add TRACE support for CXL media-error records")
Probably should add Cc: stable, as passing in region as a string was a real
bug.
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>
> Changes in v2:
> - Initialize the region name with an assignment (Steve)
> - Rename struct cxl_region 'cxlr' instead of overusing 'region' identifier
> - Update commit message & log
>
>
> drivers/cxl/core/trace.h | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> index bdf117a33744..e5f13260fc52 100644
> --- a/drivers/cxl/core/trace.h
> +++ b/drivers/cxl/core/trace.h
> @@ -646,18 +646,18 @@ u64 cxl_trace_hpa(struct cxl_region *cxlr, struct cxl_memdev *memdev, u64 dpa);
>
> TRACE_EVENT(cxl_poison,
>
> - TP_PROTO(struct cxl_memdev *cxlmd, struct cxl_region *region,
> + TP_PROTO(struct cxl_memdev *cxlmd, struct cxl_region *cxlr,
> const struct cxl_poison_record *record, u8 flags,
> __le64 overflow_ts, enum cxl_poison_trace_type trace_type),
>
> - TP_ARGS(cxlmd, region, record, flags, overflow_ts, trace_type),
> + TP_ARGS(cxlmd, cxlr, record, flags, overflow_ts, trace_type),
>
> TP_STRUCT__entry(
> __string(memdev, dev_name(&cxlmd->dev))
> __string(host, dev_name(cxlmd->dev.parent))
> __field(u64, serial)
> __field(u8, trace_type)
> - __string(region, region)
> + __string(region, cxlr ? dev_name(&cxlr->dev) : "")
I'm still curious to why NULL didn't work. I guess it may never have as I
noticed there's nothing else doing that. There are cases that a variable
returns NULL and the __string() handles it. But I guess the compiler gets
confused if the NULL is a possible return to the condition in __string().
Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
-- Steve
> __field(u64, overflow_ts)
> __field(u64, hpa)
> __field(u64, dpa)
> @@ -677,10 +677,10 @@ TRACE_EVENT(cxl_poison,
> __entry->source = cxl_poison_record_source(record);
> __entry->trace_type = trace_type;
> __entry->flags = flags;
> - if (region) {
> - __assign_str(region, dev_name(®ion->dev));
> - memcpy(__entry->uuid, ®ion->params.uuid, 16);
> - __entry->hpa = cxl_trace_hpa(region, cxlmd,
> + if (cxlr) {
> + __assign_str(region, dev_name(&cxlr->dev));
> + memcpy(__entry->uuid, &cxlr->params.uuid, 16);
> + __entry->hpa = cxl_trace_hpa(cxlr, cxlmd,
> __entry->dpa);
> } else {
> __assign_str(region, "");
>
> base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] cxl/trace: Properly initialize cxl_poison region name
2024-03-14 20:12 [PATCH v2] cxl/trace: Properly initialize cxl_poison region name alison.schofield
2024-03-14 20:41 ` Steven Rostedt
@ 2024-03-14 20:51 ` Ira Weiny
1 sibling, 0 replies; 10+ messages in thread
From: Ira Weiny @ 2024-03-14 20:51 UTC (permalink / raw)
To: alison.schofield, Davidlohr Bueso, Jonathan Cameron, Dave Jiang,
Vishal Verma, Ira Weiny, Dan Williams, Steve Rostedt
Cc: Alison Schofield, linux-cxl
alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
>
> The TP_STRUCT__entry that gets assigned the region name, or an
> empty string if no region is present, is erroneously initialized
> to the cxl_region pointer. It needs to be properly initialized
> otherwise it's length is wrong and garbage chars can appear in
> the kernel trace output: /sys/kernel/tracing/trace
>
> The bad initialization was due in part to a naming conflict with
> the parameter: struct cxl_region *region. The field 'region' is
> already exposed externally as the region name, so changing that
> to something logical, like 'region_name' is not an option. Instead
> rename the internal only struct cxl_region to the commonly used
> 'cxlr'.
>
> Impact is that tooling depending on that trace data can miss
> picking up a valid event when searching by region name. The
> TP_printk() output, if enabled, does emit the correct region
> names in the dmesg log.
>
> This was found during testing of the cxl-list option to report
> media-errors for a region.
>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] cxl/trace: Properly initialize cxl_poison region name
2024-03-14 20:41 ` Steven Rostedt
@ 2024-03-14 21:05 ` Alison Schofield
2024-03-14 21:17 ` Steven Rostedt
0 siblings, 1 reply; 10+ messages in thread
From: Alison Schofield @ 2024-03-14 21:05 UTC (permalink / raw)
To: Steven Rostedt
Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma,
Ira Weiny, Dan Williams, linux-cxl
On Thu, Mar 14, 2024 at 04:41:36PM -0400, Steven Rostedt wrote:
> On Thu, 14 Mar 2024 13:12:17 -0700
> alison.schofield@intel.com wrote:
>
> > From: Alison Schofield <alison.schofield@intel.com>
> >
> > The TP_STRUCT__entry that gets assigned the region name, or an
> > empty string if no region is present, is erroneously initialized
> > to the cxl_region pointer. It needs to be properly initialized
> > otherwise it's length is wrong and garbage chars can appear in
> > the kernel trace output: /sys/kernel/tracing/trace
> >
> > The bad initialization was due in part to a naming conflict with
> > the parameter: struct cxl_region *region. The field 'region' is
> > already exposed externally as the region name, so changing that
> > to something logical, like 'region_name' is not an option. Instead
> > rename the internal only struct cxl_region to the commonly used
> > 'cxlr'.
> >
> > Impact is that tooling depending on that trace data can miss
> > picking up a valid event when searching by region name. The
> > TP_printk() output, if enabled, does emit the correct region
> > names in the dmesg log.
> >
> > This was found during testing of the cxl-list option to report
> > media-errors for a region.
> >
> > Fixes: ddf49d57b841 ("cxl/trace: Add TRACE support for CXL media-error records")
>
> Probably should add Cc: stable, as passing in region as a string was a real
> bug.
>
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >
> > Changes in v2:
> > - Initialize the region name with an assignment (Steve)
> > - Rename struct cxl_region 'cxlr' instead of overusing 'region' identifier
> > - Update commit message & log
> >
> >
> > drivers/cxl/core/trace.h | 14 +++++++-------
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> > index bdf117a33744..e5f13260fc52 100644
> > --- a/drivers/cxl/core/trace.h
> > +++ b/drivers/cxl/core/trace.h
> > @@ -646,18 +646,18 @@ u64 cxl_trace_hpa(struct cxl_region *cxlr, struct cxl_memdev *memdev, u64 dpa);
> >
> > TRACE_EVENT(cxl_poison,
> >
> > - TP_PROTO(struct cxl_memdev *cxlmd, struct cxl_region *region,
> > + TP_PROTO(struct cxl_memdev *cxlmd, struct cxl_region *cxlr,
> > const struct cxl_poison_record *record, u8 flags,
> > __le64 overflow_ts, enum cxl_poison_trace_type trace_type),
> >
> > - TP_ARGS(cxlmd, region, record, flags, overflow_ts, trace_type),
> > + TP_ARGS(cxlmd, cxlr, record, flags, overflow_ts, trace_type),
> >
> > TP_STRUCT__entry(
> > __string(memdev, dev_name(&cxlmd->dev))
> > __string(host, dev_name(cxlmd->dev.parent))
> > __field(u64, serial)
> > __field(u8, trace_type)
> > - __string(region, region)
> > + __string(region, cxlr ? dev_name(&cxlr->dev) : "")
>
> I'm still curious to why NULL didn't work. I guess it may never have as I
> noticed there's nothing else doing that. There are cases that a variable
> returns NULL and the __string() handles it. But I guess the compiler gets
> confused if the NULL is a possible return to the condition in __string().
Here's the full warning spew:
In file included from ./include/trace/define_trace.h:102,
from drivers/cxl/core/trace.h:713,
from drivers/cxl/core/trace.c:8:
drivers/cxl/core/./trace.h: In function ‘trace_event_get_offsets_cxl_poison’:
./include/trace/stages/stage5_get_offsets.h:50:21: warning: argument 1 null where non-null expected [-Wnonnull]
50 | strlen((src) ? (const char *)(src) : "(null)") + 1)
| ^~~~~~
./include/trace/trace_events.h:263:9: note: in definition of macro ‘DECLARE_EVENT_CLASS’
263 | tstruct; \
| ^~~~~~~
./include/trace/trace_events.h:43:30: note: in expansion of macro ‘PARAMS’
43 | PARAMS(tstruct), \
| ^~~~~~
drivers/cxl/core/./trace.h:647:1: note: in expansion of macro ‘TRACE_EVENT’
647 | TRACE_EVENT(cxl_poison,
| ^~~~~~~~~~~
drivers/cxl/core/./trace.h:655:9: note: in expansion of macro ‘TP_STRUCT__entry’
655 | TP_STRUCT__entry(
| ^~~~~~~~~~~~~~~~
./include/trace/stages/stage5_get_offsets.h:49:29: note: in expansion of macro ‘__dynamic_array’
49 | #define __string(item, src) __dynamic_array(char, item, \
| ^~~~~~~~~~~~~~~
drivers/cxl/core/./trace.h:660:17: note: in expansion of macro ‘__string’
660 | __string(region, cxlr ? dev_name(&cxlr->dev) : NULL)
| ^~~~~~~~
In file included from ./include/linux/uuid.h:11,
from ./include/linux/libnvdimm.h:12,
from ./drivers/cxl/cxl.h:7,
from drivers/cxl/core/trace.c:4:
./include/linux/string.h:126:24: note: in a call to function ‘strlen’ declared ‘nonnull’
126 | extern __kernel_size_t strlen(const char *);
| ^~~~~~
>
> Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
>
> -- Steve
>
>
>
> > __field(u64, overflow_ts)
> > __field(u64, hpa)
> > __field(u64, dpa)
> > @@ -677,10 +677,10 @@ TRACE_EVENT(cxl_poison,
> > __entry->source = cxl_poison_record_source(record);
> > __entry->trace_type = trace_type;
> > __entry->flags = flags;
> > - if (region) {
> > - __assign_str(region, dev_name(®ion->dev));
> > - memcpy(__entry->uuid, ®ion->params.uuid, 16);
> > - __entry->hpa = cxl_trace_hpa(region, cxlmd,
> > + if (cxlr) {
> > + __assign_str(region, dev_name(&cxlr->dev));
> > + memcpy(__entry->uuid, &cxlr->params.uuid, 16);
> > + __entry->hpa = cxl_trace_hpa(cxlr, cxlmd,
> > __entry->dpa);
> > } else {
> > __assign_str(region, "");
> >
> > base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] cxl/trace: Properly initialize cxl_poison region name
2024-03-14 21:05 ` Alison Schofield
@ 2024-03-14 21:17 ` Steven Rostedt
2024-03-14 22:36 ` Alison Schofield
0 siblings, 1 reply; 10+ messages in thread
From: Steven Rostedt @ 2024-03-14 21:17 UTC (permalink / raw)
To: Alison Schofield
Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma,
Ira Weiny, Dan Williams, linux-cxl
On Thu, 14 Mar 2024 14:05:00 -0700
Alison Schofield <alison.schofield@intel.com> wrote:
> > I'm still curious to why NULL didn't work. I guess it may never have as I
> > noticed there's nothing else doing that. There are cases that a variable
> > returns NULL and the __string() handles it. But I guess the compiler gets
> > confused if the NULL is a possible return to the condition in __string().
>
> Here's the full warning spew:
>
> In file included from ./include/trace/define_trace.h:102,
> from drivers/cxl/core/trace.h:713,
> from drivers/cxl/core/trace.c:8:
> drivers/cxl/core/./trace.h: In function ‘trace_event_get_offsets_cxl_poison’:
> ./include/trace/stages/stage5_get_offsets.h:50:21: warning: argument 1 null where non-null expected [-Wnonnull]
> 50 | strlen((src) ? (const char *)(src) : "(null)") + 1)
> | ^~~~~~
> ./include/trace/trace_events.h:263:9: n
The full warning didn't add any new information. The above was all I
needed. But I'm curious if you applied this patch if the warning will go
away. (Note I didn't test nor compile this).
-- Steve
diff --git a/include/trace/stages/stage5_get_offsets.h b/include/trace/stages/stage5_get_offsets.h
index e30a13be46ba..dfae18d8f4df 100644
--- a/include/trace/stages/stage5_get_offsets.h
+++ b/include/trace/stages/stage5_get_offsets.h
@@ -9,6 +9,13 @@
#undef __entry
#define __entry entry
+static inline const char *__string_src(const char *str)
+{
+ if (!str)
+ return "(null)";
+ return str;
+}
+
/*
* Fields should never declare an array: i.e. __field(int, arr[5])
* If they do, it will cause issues in parsing and possibly corrupt the
@@ -47,7 +54,7 @@
#undef __string
#define __string(item, src) __dynamic_array(char, item, \
- strlen((src) ? (const char *)(src) : "(null)") + 1)
+ strlen(__string_src(src)) + 1)
#undef __string_len
#define __string_len(item, src, len) __dynamic_array(char, item, (len) + 1)
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] cxl/trace: Properly initialize cxl_poison region name
2024-03-14 21:17 ` Steven Rostedt
@ 2024-03-14 22:36 ` Alison Schofield
2024-03-14 22:50 ` Steven Rostedt
2024-03-15 10:47 ` Steven Rostedt
0 siblings, 2 replies; 10+ messages in thread
From: Alison Schofield @ 2024-03-14 22:36 UTC (permalink / raw)
To: Steven Rostedt
Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma,
Ira Weiny, Dan Williams, linux-cxl
On Thu, Mar 14, 2024 at 05:17:37PM -0400, Steven Rostedt wrote:
> On Thu, 14 Mar 2024 14:05:00 -0700
> Alison Schofield <alison.schofield@intel.com> wrote:
>
> > > I'm still curious to why NULL didn't work. I guess it may never have as I
> > > noticed there's nothing else doing that. There are cases that a variable
> > > returns NULL and the __string() handles it. But I guess the compiler gets
> > > confused if the NULL is a possible return to the condition in __string().
> >
> > Here's the full warning spew:
> >
> > In file included from ./include/trace/define_trace.h:102,
> > from drivers/cxl/core/trace.h:713,
> > from drivers/cxl/core/trace.c:8:
> > drivers/cxl/core/./trace.h: In function ‘trace_event_get_offsets_cxl_poison’:
> > ./include/trace/stages/stage5_get_offsets.h:50:21: warning: argument 1 null where non-null expected [-Wnonnull]
> > 50 | strlen((src) ? (const char *)(src) : "(null)") + 1)
> > | ^~~~~~
> > ./include/trace/trace_events.h:263:9: n
>
> The full warning didn't add any new information. The above was all I
> needed. But I'm curious if you applied this patch if the warning will go
> away. (Note I didn't test nor compile this).
>
> -- Steve
>
> diff --git a/include/trace/stages/stage5_get_offsets.h b/include/trace/stages/stage5_get_offsets.h
> index e30a13be46ba..dfae18d8f4df 100644
> --- a/include/trace/stages/stage5_get_offsets.h
> +++ b/include/trace/stages/stage5_get_offsets.h
> @@ -9,6 +9,13 @@
> #undef __entry
> #define __entry entry
>
+#ifndef STAGE5_H_INCLUDED
+#define STAGE5_H_INCLUDED
> +static inline const char *__string_src(const char *str)
> +{
> + if (!str)
> + return "(null)";
> + return str;
> +}
+#endif /* STAGE5_H_INCLUDED */
Happy to try it out...
Your diff, with the ifdef above, makes the compiler happy and it functions
as wanted - no garbage in the fields.
> +
> /*
> * Fields should never declare an array: i.e. __field(int, arr[5])
> * If they do, it will cause issues in parsing and possibly corrupt the
> @@ -47,7 +54,7 @@
>
> #undef __string
> #define __string(item, src) __dynamic_array(char, item, \
> - strlen((src) ? (const char *)(src) : "(null)") + 1)
> + strlen(__string_src(src)) + 1)
>
> #undef __string_len
> #define __string_len(item, src, len) __dynamic_array(char, item, (len) + 1)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] cxl/trace: Properly initialize cxl_poison region name
2024-03-14 22:36 ` Alison Schofield
@ 2024-03-14 22:50 ` Steven Rostedt
2024-03-15 10:47 ` Steven Rostedt
1 sibling, 0 replies; 10+ messages in thread
From: Steven Rostedt @ 2024-03-14 22:50 UTC (permalink / raw)
To: Alison Schofield
Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma,
Ira Weiny, Dan Williams, linux-cxl
On Thu, 14 Mar 2024 15:36:48 -0700
Alison Schofield <alison.schofield@intel.com> wrote:
> > diff --git a/include/trace/stages/stage5_get_offsets.h b/include/trace/stages/stage5_get_offsets.h
> > index e30a13be46ba..dfae18d8f4df 100644
> > --- a/include/trace/stages/stage5_get_offsets.h
> > +++ b/include/trace/stages/stage5_get_offsets.h
> > @@ -9,6 +9,13 @@
> > #undef __entry
> > #define __entry entry
> >
>
> +#ifndef STAGE5_H_INCLUDED
> +#define STAGE5_H_INCLUDED
> > +static inline const char *__string_src(const char *str)
> > +{
> > + if (!str)
> > + return "(null)";
> > + return str;
> > +}
> +#endif /* STAGE5_H_INCLUDED */
>
> Happy to try it out...
>
> Your diff, with the ifdef above, makes the compiler happy and it functions
> as wanted - no garbage in the fields.
Hmm, I guess the:
strlen((src ? (const char *)(src) : NULL) ? (src ? (const char *) src : NULL)) ? "(null)")) + 1
was too complex for the compiler, where as:
strlen(__string_str((str ? (const char *)(src) : NULL))) + 1
is not.
Perhaps I should add this change. I would still submit your change with the
"" as that needs to be backported, but this probably does not.
Thanks for testing it out.
-- Steve
>
> > +
> > /*
> > * Fields should never declare an array: i.e. __field(int, arr[5])
> > * If they do, it will cause issues in parsing and possibly corrupt the
> > @@ -47,7 +54,7 @@
> >
> > #undef __string
> > #define __string(item, src) __dynamic_array(char, item, \
> > - strlen((src) ? (const char *)(src) : "(null)") + 1)
> > + strlen(__string_src(src)) + 1)
> >
> > #undef __string_len
> > #define __string_len(item, src, len) __dynamic_array(char, item, (len) + 1)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] cxl/trace: Properly initialize cxl_poison region name
2024-03-14 22:36 ` Alison Schofield
2024-03-14 22:50 ` Steven Rostedt
@ 2024-03-15 10:47 ` Steven Rostedt
2024-03-15 16:18 ` Dan Williams
1 sibling, 1 reply; 10+ messages in thread
From: Steven Rostedt @ 2024-03-15 10:47 UTC (permalink / raw)
To: Alison Schofield
Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma,
Ira Weiny, Dan Williams, linux-cxl
On Thu, 14 Mar 2024 15:36:48 -0700
Alison Schofield <alison.schofield@intel.com> wrote:
> On Thu, Mar 14, 2024 at 05:17:37PM -0400, Steven Rostedt wrote:
> > On Thu, 14 Mar 2024 14:05:00 -0700
> > Alison Schofield <alison.schofield@intel.com> wrote:
> >
> > > > I'm still curious to why NULL didn't work. I guess it may never have as I
> > > > noticed there's nothing else doing that. There are cases that a variable
> > > > returns NULL and the __string() handles it. But I guess the compiler gets
> > > > confused if the NULL is a possible return to the condition in __string().
> > >
> > > Here's the full warning spew:
> > >
> > > In file included from ./include/trace/define_trace.h:102,
> > > from drivers/cxl/core/trace.h:713,
> > > from drivers/cxl/core/trace.c:8:
> > > drivers/cxl/core/./trace.h: In function ‘trace_event_get_offsets_cxl_poison’:
> > > ./include/trace/stages/stage5_get_offsets.h:50:21: warning: argument 1 null where non-null expected [-Wnonnull]
> > > 50 | strlen((src) ? (const char *)(src) : "(null)") + 1)
> > > | ^~~~~~
> > > ./include/trace/trace_events.h:263:9: n
> >
> > The full warning didn't add any new information. The above was all I
> > needed. But I'm curious if you applied this patch if the warning will go
> > away. (Note I didn't test nor compile this).
> >
> > -- Steve
> >
> > diff --git a/include/trace/stages/stage5_get_offsets.h b/include/trace/stages/stage5_get_offsets.h
> > index e30a13be46ba..dfae18d8f4df 100644
> > --- a/include/trace/stages/stage5_get_offsets.h
> > +++ b/include/trace/stages/stage5_get_offsets.h
> > @@ -9,6 +9,13 @@
> > #undef __entry
> > #define __entry entry
> >
>
> +#ifndef STAGE5_H_INCLUDED
> +#define STAGE5_H_INCLUDED
> > +static inline const char *__string_src(const char *str)
> > +{
> > + if (!str)
> > + return "(null)";
> > + return str;
> > +}
> +#endif /* STAGE5_H_INCLUDED */
>
> Happy to try it out...
>
> Your diff, with the ifdef above, makes the compiler happy and it functions
> as wanted - no garbage in the fields.
I created the patch:
https://lore.kernel.org/linux-trace-kernel/20240314232754.345cea82@rorschach.local.home/
That adds this. But now the kernel will not build because it requires the
fix of this patch. Otherwise it fails with:
In file included from /work/build/trace/nobackup/linux-test.git/include/trace/define_trace.h:102,
from /work/build/trace/nobackup/linux-test.git/drivers/cxl/core/trace.h:713,
from /work/build/trace/nobackup/linux-test.git/drivers/cxl/core/trace.c:8:
/work/build/trace/nobackup/linux-test.git/drivers/cxl/core/./trace.h: In function ‘trace_event_get_offsets_cxl_poison’:
/work/build/trace/nobackup/linux-test.git/drivers/cxl/core/./trace.h:660:34: error: passing argument 1 of ‘__string_src’ from incompatible pointer type [-Werror=incompatible-pointer-types]
660 | __string(region, region)
| ^~~~~~
| |
| struct cxl_region *
I like that the patch also catches other bugs where non strings are passed
to __string(). But I can't send this to Linus if it breaks the build.
I'm breaking my pull request up as there's some other places that break the
build with my new checks. I'll do a pull of all my updates first, and after
Linus pulls in my change I will base the next pull off of that merge commit.
But this requires that this fix is in Linus's tree before that. If not, can
I take this fix and add it to that later pull request with all the tests. I
want to make sure that the kernel always builds.
-- Steve
>
> > +
> > /*
> > * Fields should never declare an array: i.e. __field(int, arr[5])
> > * If they do, it will cause issues in parsing and possibly corrupt the
> > @@ -47,7 +54,7 @@
> >
> > #undef __string
> > #define __string(item, src) __dynamic_array(char, item, \
> > - strlen((src) ? (const char *)(src) : "(null)") + 1)
> > + strlen(__string_src(src)) + 1)
> >
> > #undef __string_len
> > #define __string_len(item, src, len) __dynamic_array(char, item, (len) + 1)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] cxl/trace: Properly initialize cxl_poison region name
2024-03-15 10:47 ` Steven Rostedt
@ 2024-03-15 16:18 ` Dan Williams
2024-03-15 16:30 ` Steven Rostedt
0 siblings, 1 reply; 10+ messages in thread
From: Dan Williams @ 2024-03-15 16:18 UTC (permalink / raw)
To: Steven Rostedt, Alison Schofield
Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma,
Ira Weiny, Dan Williams, linux-cxl
Steven Rostedt wrote:
> On Thu, 14 Mar 2024 15:36:48 -0700
> Alison Schofield <alison.schofield@intel.com> wrote:
>
> > On Thu, Mar 14, 2024 at 05:17:37PM -0400, Steven Rostedt wrote:
> > > On Thu, 14 Mar 2024 14:05:00 -0700
> > > Alison Schofield <alison.schofield@intel.com> wrote:
> > >
> > > > > I'm still curious to why NULL didn't work. I guess it may never have as I
> > > > > noticed there's nothing else doing that. There are cases that a variable
> > > > > returns NULL and the __string() handles it. But I guess the compiler gets
> > > > > confused if the NULL is a possible return to the condition in __string().
> > > >
> > > > Here's the full warning spew:
> > > >
> > > > In file included from ./include/trace/define_trace.h:102,
> > > > from drivers/cxl/core/trace.h:713,
> > > > from drivers/cxl/core/trace.c:8:
> > > > drivers/cxl/core/./trace.h: In function ‘trace_event_get_offsets_cxl_poison’:
> > > > ./include/trace/stages/stage5_get_offsets.h:50:21: warning: argument 1 null where non-null expected [-Wnonnull]
> > > > 50 | strlen((src) ? (const char *)(src) : "(null)") + 1)
> > > > | ^~~~~~
> > > > ./include/trace/trace_events.h:263:9: n
> > >
> > > The full warning didn't add any new information. The above was all I
> > > needed. But I'm curious if you applied this patch if the warning will go
> > > away. (Note I didn't test nor compile this).
> > >
> > > -- Steve
> > >
> > > diff --git a/include/trace/stages/stage5_get_offsets.h b/include/trace/stages/stage5_get_offsets.h
> > > index e30a13be46ba..dfae18d8f4df 100644
> > > --- a/include/trace/stages/stage5_get_offsets.h
> > > +++ b/include/trace/stages/stage5_get_offsets.h
> > > @@ -9,6 +9,13 @@
> > > #undef __entry
> > > #define __entry entry
> > >
> >
> > +#ifndef STAGE5_H_INCLUDED
> > +#define STAGE5_H_INCLUDED
> > > +static inline const char *__string_src(const char *str)
> > > +{
> > > + if (!str)
> > > + return "(null)";
> > > + return str;
> > > +}
> > +#endif /* STAGE5_H_INCLUDED */
> >
> > Happy to try it out...
> >
> > Your diff, with the ifdef above, makes the compiler happy and it functions
> > as wanted - no garbage in the fields.
>
> I created the patch:
>
> https://lore.kernel.org/linux-trace-kernel/20240314232754.345cea82@rorschach.local.home/
>
> That adds this. But now the kernel will not build because it requires the
> fix of this patch. Otherwise it fails with:
>
> In file included from /work/build/trace/nobackup/linux-test.git/include/trace/define_trace.h:102,
> from /work/build/trace/nobackup/linux-test.git/drivers/cxl/core/trace.h:713,
> from /work/build/trace/nobackup/linux-test.git/drivers/cxl/core/trace.c:8:
> /work/build/trace/nobackup/linux-test.git/drivers/cxl/core/./trace.h: In function ‘trace_event_get_offsets_cxl_poison’:
> /work/build/trace/nobackup/linux-test.git/drivers/cxl/core/./trace.h:660:34: error: passing argument 1 of ‘__string_src’ from incompatible pointer type [-Werror=incompatible-pointer-types]
> 660 | __string(region, region)
> | ^~~~~~
> | |
> | struct cxl_region *
>
> I like that the patch also catches other bugs where non strings are passed
> to __string(). But I can't send this to Linus if it breaks the build.
>
> I'm breaking my pull request up as there's some other places that break the
> build with my new checks. I'll do a pull of all my updates first, and after
> Linus pulls in my change I will base the next pull off of that merge commit.
>
> But this requires that this fix is in Linus's tree before that. If not, can
> I take this fix and add it to that later pull request with all the tests. I
> want to make sure that the kernel always builds.
Acked-by: Dan Williams <dan.j.williams@intel.com>
...for you taking the fix through your tree, and yes, please Cc stable
on it as well.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] cxl/trace: Properly initialize cxl_poison region name
2024-03-15 16:18 ` Dan Williams
@ 2024-03-15 16:30 ` Steven Rostedt
0 siblings, 0 replies; 10+ messages in thread
From: Steven Rostedt @ 2024-03-15 16:30 UTC (permalink / raw)
To: Dan Williams
Cc: Alison Schofield, Davidlohr Bueso, Jonathan Cameron, Dave Jiang,
Vishal Verma, Ira Weiny, linux-cxl
On Fri, 15 Mar 2024 09:18:04 -0700
Dan Williams <dan.j.williams@intel.com> wrote:
> Acked-by: Dan Williams <dan.j.williams@intel.com>
>
> ...for you taking the fix through your tree, and yes, please Cc stable
> on it as well.
Thanks, will do!
I'll pull it in and start running my tests on it.
-- Steve
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-03-15 16:28 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-14 20:12 [PATCH v2] cxl/trace: Properly initialize cxl_poison region name alison.schofield
2024-03-14 20:41 ` Steven Rostedt
2024-03-14 21:05 ` Alison Schofield
2024-03-14 21:17 ` Steven Rostedt
2024-03-14 22:36 ` Alison Schofield
2024-03-14 22:50 ` Steven Rostedt
2024-03-15 10:47 ` Steven Rostedt
2024-03-15 16:18 ` Dan Williams
2024-03-15 16:30 ` Steven Rostedt
2024-03-14 20:51 ` Ira Weiny
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.