[Intel-gfx] [PATCH] drm/i915/sdvo: Fix up debug output to not split lines

[Daniel Vetter]

On Wed, Nov 27, 2013 at 3:09 PM, Mika Kuoppala
<mika.kuoppala at linux.intel.com> wrote:
> Daniel Vetter <daniel.vetter at 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 at 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch