| Message ID | 20180709134858.12446-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 09, 2018 at 02:48:58PM +0100, Chris Wilson wrote: > Error messages are intended to be addressed to the user; be clear, > succinct, instructive and unambiguous. Adding the function name to > that message does not add any information the user requires and in > the process makes the message less clear. > > E.g. > > [ 245.539711] i915 0000:00:02.0: [drm:i915_gem_init [i915]] Failed to initialize GPU, declaring it wedged! Overall I like the idea... The down side is that for us when debugging we would need to always trust grep like searches and many debug messages are constructed out of variables what makes it a bit hard to find sometimes. Ok, nothing that we couldn't figure out... > > becomes > > [ 245.539711] i915 0000:00:02.0: Failed to initialize GPU, declaring it wedged! What about adding an "ERROR:" ? [ 245.539711] i915 0000:00:02.0: ERROR: Failed to initialize GPU, declaring it wedged! > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_drv.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 2959c88a37a5..c2b9a4a0ee49 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -104,8 +104,13 @@ __i915_printk(struct drm_i915_private *dev_priv, const char *level, > vaf.fmt = fmt; > vaf.va = &args; > > - dev_printk(level, kdev, "[" DRM_NAME ":%ps] %pV", > - __builtin_return_address(0), &vaf); > + if (is_error) > + dev_printk(level, kdev, "%pV", &vaf); > + else > + dev_printk(level, kdev, "[" DRM_NAME ":%ps] %pV", > + __builtin_return_address(0), &vaf); > + > + va_end(args); > > if (is_error && !shown_bug_once) { > /* > @@ -117,8 +122,6 @@ __i915_printk(struct drm_i915_private *dev_priv, const char *level, > dev_notice(kdev, "%s", FDO_BUG_MSG); > shown_bug_once = true; > } > - > - va_end(args); > } > > /* Map PCH device id to PCH type, or PCH_NONE if unknown. */ > -- > 2.18.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Rodrigo Vivi (2018-07-09 18:51:02) > On Mon, Jul 09, 2018 at 02:48:58PM +0100, Chris Wilson wrote: > > Error messages are intended to be addressed to the user; be clear, > > succinct, instructive and unambiguous. Adding the function name to > > that message does not add any information the user requires and in > > the process makes the message less clear. > > > > E.g. > > > > [ 245.539711] i915 0000:00:02.0: [drm:i915_gem_init [i915]] Failed to initialize GPU, declaring it wedged! > > Overall I like the idea... > > The down side is that for us when debugging we would need to always trust grep like > searches and many debug messages are constructed out of variables what makes it a bit > hard to find sometimes. Ok, nothing that we couldn't figure out... A big difference is that error messages are targeted at the user, and as such should be succinct and not require them to dig into the source code to understand what it means and what action they need to take. Usually such error messages are accompanied by a lot of debug output for developers to pour over, but for the average user, imo we just need to say what broke and no longer works, and how they can file a bug (any information we need for that bug should be captured automatically and read for them to attach). > > becomes > > > > [ 245.539711] i915 0000:00:02.0: Failed to initialize GPU, declaring it wedged! > > What about adding an "ERROR:" ? The KERN_ERR is recorded in the output for the userspace application to decide how to colourize and highlight when it presents the kmsg records. -Chris
On Mon, Jul 09, 2018 at 09:14:05PM +0100, Chris Wilson wrote: > Quoting Rodrigo Vivi (2018-07-09 18:51:02) > > On Mon, Jul 09, 2018 at 02:48:58PM +0100, Chris Wilson wrote: > > > Error messages are intended to be addressed to the user; be clear, > > > succinct, instructive and unambiguous. Adding the function name to > > > that message does not add any information the user requires and in > > > the process makes the message less clear. > > > > > > E.g. > > > > > > [ 245.539711] i915 0000:00:02.0: [drm:i915_gem_init [i915]] Failed to initialize GPU, declaring it wedged! > > > > Overall I like the idea... > > > > The down side is that for us when debugging we would need to always trust grep like > > searches and many debug messages are constructed out of variables what makes it a bit > > hard to find sometimes. Ok, nothing that we couldn't figure out... > > A big difference is that error messages are targeted at the user, and as > such should be succinct and not require them to dig into the source code > to understand what it means and what action they need to take. Usually > such error messages are accompanied by a lot of debug output for > developers to pour over, but for the average user, imo we just need to > say what broke and no longer works, and how they can file a bug (any > information we need for that bug should be captured automatically and > read for them to attach). it makes sense. > > > > becomes > > > > > > [ 245.539711] i915 0000:00:02.0: Failed to initialize GPU, declaring it wedged! > > > > What about adding an "ERROR:" ? > > The KERN_ERR is recorded in the output for the userspace application to > decide how to colourize and highlight when it presents the kmsg records. hmm... good point. Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > -Chris > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 2959c88a37a5..c2b9a4a0ee49 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -104,8 +104,13 @@ __i915_printk(struct drm_i915_private *dev_priv, const char *level, vaf.fmt = fmt; vaf.va = &args; - dev_printk(level, kdev, "[" DRM_NAME ":%ps] %pV", - __builtin_return_address(0), &vaf); + if (is_error) + dev_printk(level, kdev, "%pV", &vaf); + else + dev_printk(level, kdev, "[" DRM_NAME ":%ps] %pV", + __builtin_return_address(0), &vaf); + + va_end(args); if (is_error && !shown_bug_once) { /* @@ -117,8 +122,6 @@ __i915_printk(struct drm_i915_private *dev_priv, const char *level, dev_notice(kdev, "%s", FDO_BUG_MSG); shown_bug_once = true; } - - va_end(args); } /* Map PCH device id to PCH type, or PCH_NONE if unknown. */
Error messages are intended to be addressed to the user; be clear, succinct, instructive and unambiguous. Adding the function name to that message does not add any information the user requires and in the process makes the message less clear. E.g. [ 245.539711] i915 0000:00:02.0: [drm:i915_gem_init [i915]] Failed to initialize GPU, declaring it wedged! becomes [ 245.539711] i915 0000:00:02.0: Failed to initialize GPU, declaring it wedged! Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_drv.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)