| Message ID | 1385550619-31901-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Daniel, Daniel Vetter <daniel.vetter@ffwll.ch> writes: > It leads to a big mess when stuff interleaves. Especially with the new > patch I've submitted for the drm core to no longer artificially split > up debug messages. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_sdvo.c | 55 ++++++++++++++++++++++++++------------- > 1 file changed, 37 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > index a583e8f718a7..e88ad95df08f 100644 > --- a/drivers/gpu/drm/i915/intel_sdvo.c > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > @@ -413,23 +413,34 @@ static const struct _sdvo_cmd_name { > static void intel_sdvo_debug_write(struct intel_sdvo *intel_sdvo, u8 cmd, > const void *args, int args_len) > { > - int i; > + int i, pos = 0; > +#define BUF_LEN 256 > + char buffer[BUF_LEN]; > + > +#define BUF_PRINT(args...) \ > + pos += snprintf(buffer + pos, max_t(int, BUF_LEN - pos - 1, 0), args) > + You want scnprintf here as it returns the true number of bytes written instead of what would have been written. Also the size argument includes the the trailing null space. Consider: BUG_ON((pos += scnprintf(buffer + pos, max_t(int, BUF_LEN - pos, 0), args)) >= BUF_LEN) > - DRM_DEBUG_KMS("%s: W: %02X ", > - SDVO_NAME(intel_sdvo), cmd); > - for (i = 0; i < args_len; i++) > - DRM_LOG_KMS("%02X ", ((u8 *)args)[i]); > - for (; i < 8; i++) > - DRM_LOG_KMS(" "); > + for (i = 0; i < args_len; i++) { > + BUF_PRINT("%02X ", ((u8 *)args)[i]); > + } > + for (; i < 8; i++) { > + BUF_PRINT(" "); > + } > for (i = 0; i < ARRAY_SIZE(sdvo_cmd_names); i++) { > if (cmd == sdvo_cmd_names[i].cmd) { > - DRM_LOG_KMS("(%s)", sdvo_cmd_names[i].name); > + BUF_PRINT("(%s)", sdvo_cmd_names[i].name); > break; > } > } > - if (i == ARRAY_SIZE(sdvo_cmd_names)) > - DRM_LOG_KMS("(%02X)", cmd); > - DRM_LOG_KMS("\n"); > + if (i == ARRAY_SIZE(sdvo_cmd_names)) { > + BUF_PRINT("(%02X)", cmd); > + } > + BUG_ON(pos >= BUF_LEN - 1); pos >= BUF_LEN is enough if you choose not to BUG_ON on each scnprintf > +#undef BUF_PRINT > +#undef BUF_LEN > + > + DRM_DEBUG_KMS("%s: W: %02X %s\n", SDVO_NAME(intel_sdvo), cmd, buffer); > } > > static const char *cmd_status_names[] = { > @@ -512,9 +523,10 @@ static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo, > { > u8 retry = 15; /* 5 quick checks, followed by 10 long checks */ > u8 status; > - int i; > + int i, pos = 0; > +#define BUF_LEN 256 > + char buffer[BUF_LEN]; > > - DRM_DEBUG_KMS("%s: R: ", SDVO_NAME(intel_sdvo)); > > /* > * The documentation states that all commands will be > @@ -551,10 +563,13 @@ static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo, > goto log_fail; > } > > +#define BUF_PRINT(args...) \ > + pos += snprintf(buffer + pos, max_t(int, BUF_LEN - pos - 1, 0), args) > + ^^ Ditto > if (status <= SDVO_CMD_STATUS_SCALING_NOT_SUPP) > - DRM_LOG_KMS("(%s)", cmd_status_names[status]); > + BUF_PRINT("(%s)", cmd_status_names[status]); > else > - DRM_LOG_KMS("(??? %d)", status); > + BUF_PRINT("(??? %d)", status); > > if (status != SDVO_CMD_STATUS_SUCCESS) > goto log_fail; > @@ -565,13 +580,17 @@ static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo, > SDVO_I2C_RETURN_0 + i, > &((u8 *)response)[i])) > goto log_fail; > - DRM_LOG_KMS(" %02X", ((u8 *)response)[i]); > + BUF_PRINT(" %02X", ((u8 *)response)[i]); > } > - DRM_LOG_KMS("\n"); > + BUG_ON(pos >= BUF_LEN - 1); > +#undef BUF_PRINT > +#undef BUF_LEN > + > + DRM_DEBUG_KMS("%s: R: %s\n", SDVO_NAME(intel_sdvo), buffer); > return true; > > log_fail: > - DRM_LOG_KMS("... failed\n"); > + DRM_DEBUG_KMS("%s: R: ... failed\n", SDVO_NAME(intel_sdvo)); > return false; > } > > -- > 1.8.3.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Nov 27, 2013 at 3:09 PM, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote: > Daniel Vetter <daniel.vetter@ffwll.ch> writes: >> It leads to a big mess when stuff interleaves. Especially with the new >> patch I've submitted for the drm core to no longer artificially split >> up debug messages. >> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> --- >> drivers/gpu/drm/i915/intel_sdvo.c | 55 ++++++++++++++++++++++++++------------- >> 1 file changed, 37 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c >> index a583e8f718a7..e88ad95df08f 100644 >> --- a/drivers/gpu/drm/i915/intel_sdvo.c >> +++ b/drivers/gpu/drm/i915/intel_sdvo.c >> @@ -413,23 +413,34 @@ static const struct _sdvo_cmd_name { >> static void intel_sdvo_debug_write(struct intel_sdvo *intel_sdvo, u8 cmd, >> const void *args, int args_len) >> { >> - int i; >> + int i, pos = 0; >> +#define BUF_LEN 256 >> + char buffer[BUF_LEN]; >> + >> +#define BUF_PRINT(args...) \ >> + pos += snprintf(buffer + pos, max_t(int, BUF_LEN - pos - 1, 0), args) >> + > > You want scnprintf here as it returns the true number of bytes written > instead of what would have been written. Also the size argument includes > the the trailing null space. Consider: Actually the semantics of snprintf which only returns the actual characters written without the terminating 0 at the end is what I want - the next BUF_PRINT should overwrite things. In case of a buffer overrunt the max should make sure that we don't actually overwrite anything, and the BUG_ON below makes sure that the string we feed to printk is still 0-terminated (hence the - 1 there). > BUG_ON((pos += scnprintf(buffer + pos, max_t(int, BUF_LEN - pos, 0), args)) >= BUF_LEN) > >> - DRM_DEBUG_KMS("%s: W: %02X ", >> - SDVO_NAME(intel_sdvo), cmd); >> - for (i = 0; i < args_len; i++) >> - DRM_LOG_KMS("%02X ", ((u8 *)args)[i]); >> - for (; i < 8; i++) >> - DRM_LOG_KMS(" "); >> + for (i = 0; i < args_len; i++) { >> + BUF_PRINT("%02X ", ((u8 *)args)[i]); >> + } >> + for (; i < 8; i++) { >> + BUF_PRINT(" "); >> + } >> for (i = 0; i < ARRAY_SIZE(sdvo_cmd_names); i++) { >> if (cmd == sdvo_cmd_names[i].cmd) { >> - DRM_LOG_KMS("(%s)", sdvo_cmd_names[i].name); >> + BUF_PRINT("(%s)", sdvo_cmd_names[i].name); >> break; >> } >> } >> - if (i == ARRAY_SIZE(sdvo_cmd_names)) >> - DRM_LOG_KMS("(%02X)", cmd); >> - DRM_LOG_KMS("\n"); >> + if (i == ARRAY_SIZE(sdvo_cmd_names)) { >> + BUF_PRINT("(%02X)", cmd); >> + } >> + BUG_ON(pos >= BUF_LEN - 1); > > pos >= BUF_LEN is enough if you choose not to BUG_ON on each scnprintf See above, with snprintf we need to make sure we have a terminating 0. Without that the printk will go rampant. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index a583e8f718a7..e88ad95df08f 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -413,23 +413,34 @@ static const struct _sdvo_cmd_name { static void intel_sdvo_debug_write(struct intel_sdvo *intel_sdvo, u8 cmd, const void *args, int args_len) { - int i; + int i, pos = 0; +#define BUF_LEN 256 + char buffer[BUF_LEN]; + +#define BUF_PRINT(args...) \ + pos += snprintf(buffer + pos, max_t(int, BUF_LEN - pos - 1, 0), args) + - DRM_DEBUG_KMS("%s: W: %02X ", - SDVO_NAME(intel_sdvo), cmd); - for (i = 0; i < args_len; i++) - DRM_LOG_KMS("%02X ", ((u8 *)args)[i]); - for (; i < 8; i++) - DRM_LOG_KMS(" "); + for (i = 0; i < args_len; i++) { + BUF_PRINT("%02X ", ((u8 *)args)[i]); + } + for (; i < 8; i++) { + BUF_PRINT(" "); + } for (i = 0; i < ARRAY_SIZE(sdvo_cmd_names); i++) { if (cmd == sdvo_cmd_names[i].cmd) { - DRM_LOG_KMS("(%s)", sdvo_cmd_names[i].name); + BUF_PRINT("(%s)", sdvo_cmd_names[i].name); break; } } - if (i == ARRAY_SIZE(sdvo_cmd_names)) - DRM_LOG_KMS("(%02X)", cmd); - DRM_LOG_KMS("\n"); + if (i == ARRAY_SIZE(sdvo_cmd_names)) { + BUF_PRINT("(%02X)", cmd); + } + BUG_ON(pos >= BUF_LEN - 1); +#undef BUF_PRINT +#undef BUF_LEN + + DRM_DEBUG_KMS("%s: W: %02X %s\n", SDVO_NAME(intel_sdvo), cmd, buffer); } static const char *cmd_status_names[] = { @@ -512,9 +523,10 @@ static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo, { u8 retry = 15; /* 5 quick checks, followed by 10 long checks */ u8 status; - int i; + int i, pos = 0; +#define BUF_LEN 256 + char buffer[BUF_LEN]; - DRM_DEBUG_KMS("%s: R: ", SDVO_NAME(intel_sdvo)); /* * The documentation states that all commands will be @@ -551,10 +563,13 @@ static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo, goto log_fail; } +#define BUF_PRINT(args...) \ + pos += snprintf(buffer + pos, max_t(int, BUF_LEN - pos - 1, 0), args) + if (status <= SDVO_CMD_STATUS_SCALING_NOT_SUPP) - DRM_LOG_KMS("(%s)", cmd_status_names[status]); + BUF_PRINT("(%s)", cmd_status_names[status]); else - DRM_LOG_KMS("(??? %d)", status); + BUF_PRINT("(??? %d)", status); if (status != SDVO_CMD_STATUS_SUCCESS) goto log_fail; @@ -565,13 +580,17 @@ static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo, SDVO_I2C_RETURN_0 + i, &((u8 *)response)[i])) goto log_fail; - DRM_LOG_KMS(" %02X", ((u8 *)response)[i]); + BUF_PRINT(" %02X", ((u8 *)response)[i]); } - DRM_LOG_KMS("\n"); + BUG_ON(pos >= BUF_LEN - 1); +#undef BUF_PRINT +#undef BUF_LEN + + DRM_DEBUG_KMS("%s: R: %s\n", SDVO_NAME(intel_sdvo), buffer); return true; log_fail: - DRM_LOG_KMS("... failed\n"); + DRM_DEBUG_KMS("%s: R: ... failed\n", SDVO_NAME(intel_sdvo)); return false; }
It leads to a big mess when stuff interleaves. Especially with the new patch I've submitted for the drm core to no longer artificially split up debug messages. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_sdvo.c | 55 ++++++++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 18 deletions(-)