| Message ID | 20240314201217.2112644-1-alison.schofield@intel.com |
|---|---|
| State | Accepted |
| Commit | 6c871260965255a1c142fb77ccee58b172d1690b |
| Headers | show |
| Series | [v2] cxl/trace: Properly initialize cxl_poison region name | expand |
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
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>
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 > >
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)
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)
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)
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)
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.
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
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, "");