-------- Forwarded Message --------The prefix field is officially supposed to be a const char *. There is no documentation to say that this is intended to be used as a private data field by random printer wrappers. So overloading it like this feels very hacky and dangerous. Also, you are mixing types - uintptr_t then uint. So an arch with 64-bit pointers but only 32-bit ints would hit a truncated compiler warning?
Subject: [RFC] drm/print: Introduce drm_line_printer Date: Tue, 14 May 2024 16:56:31 +0200 From: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx> To: dri-devel@xxxxxxxxxxxxxxxxxxxxx
This drm printer wrapper can be used to increase the robustness of
the captured output generated by any other drm_printer to make sure
we didn't lost any intermediate lines of the output by adding line
numbers to each output line. Helpful for capturing some crash data.
Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
---
drivers/gpu/drm/drm_print.c | 9 +++++++++
include/drm/drm_print.h | 37 +++++++++++++++++++++++++++++++++++++
2 files changed, 46 insertions(+)
diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index cf2efb44722c..d6fb50d3407a 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -214,6 +214,15 @@ void __drm_printfn_err(struct drm_printer *p, struct va_format *vaf)
}
EXPORT_SYMBOL(__drm_printfn_err);
+void __drm_printfn_line(struct drm_printer *p, struct va_format *vaf)
+{
+ unsigned int line = (uintptr_t)(++p->prefix);
This is insufficient. As previously commented, there needs to be a global counter as well as a local line counter. The global count must be global to at least whatever entity is generating a specific set of prints. Being global to a higher level, e.g. kernel global, is fine. But without that, two concurrent dumps that get interleaved can be impossible to separate resulting in a useless bug report/log.+ struct drm_printer *dp = p->arg;
+
+ drm_printf(dp, "%u: %pV", line, vaf);
The prefix field could potentially be split into a 16:16 global:local index with the global master just being a static u16 inside that function. With the first print call to a given drm_printer object being defined by the global value being zero. And it then sets the global value to the next increment skipping over zero on a 16-bit wrap around. But see above about prefix not being intended for such purposes. So now you are just piling hacks upon hacks.
Plus it would be much nicer output to have the ability to put an arbitrary prefix in front of the G.L number, as per the original implementation. The whole point of this is to aid identification of otherwise uniform data such as hexdumps. So anything that makes it less clear is bad.
John.
+}
+EXPORT_SYMBOL(__drm_printfn_line);
+
/**
* drm_puts - print a const string to a &drm_printer stream
* @p: the &drm printer
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 089950ad8681..58cc73c53853 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -186,6 +186,7 @@ void __drm_puts_seq_file(struct drm_printer *p, const char *str);
void __drm_printfn_info(struct drm_printer *p, struct va_format *vaf);
void __drm_printfn_dbg(struct drm_printer *p, struct va_format *vaf);
void __drm_printfn_err(struct drm_printer *p, struct va_format *vaf);
+void __drm_printfn_line(struct drm_printer *p, struct va_format *vaf);
__printf(2, 3)
void drm_printf(struct drm_printer *p, const char *f, ...);
@@ -357,6 +358,42 @@ static inline struct drm_printer drm_err_printer(struct drm_device *drm,
return p;
}
+/**
+ * drm_line_printer - construct a &drm_printer that prefixes outputs with line numbers
+ * @dp: the &struct drm_printer which actually generates the output
+ *
+ * This printer can be used to increase the robustness of the captured output
+ * to make sure we didn't lost any intermediate lines of the output. Helpful
+ * while capturing some crash data.
+ *
+ * For example::
+ *
+ * void crash_dump(struct drm_device *drm)
+ * {
+ * struct drm_printer dp = drm_err_printer(drm, "crash");
+ * struct drm_printer lp = drm_line_printer(&dp);
+ *
+ * drm_printf(&lp, "foo");
+ * drm_printf(&lp, "bar");
+ * }
+ *
+ * Above code will print into the dmesg something like::
+ *
+ * [ ] 0000:00:00.0: [drm] *ERROR* crash 1: foo
+ * [ ] 0000:00:00.0: [drm] *ERROR* crash 2: bar
+ *
+ * RETURNS:
+ * The &drm_printer object
+ */
+static inline struct drm_printer drm_line_printer(struct drm_printer *dp)
+{
+ struct drm_printer lp = {
+ .printfn = __drm_printfn_line,
+ .arg = dp,
+ };
+ return lp;
+}
+
/*
* struct device based logging
*
-- 2.43.0
