Re: [PATCH i-g-t 1/3] tools/intel_reg: Don't reuse stale decoded results for later registers

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 05 Aug 2016, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> In case the ->debug_output() function skips decoding the register it
> just returns, which means the caller will reuse whatever it already has
> in the tmp buffer as the decoded result for this result. What it usually
> has in there is the decoded result of some previous register.
>
> Showing incorrect decoded results is no good, so let's allow
> ->debug_output() to actually return how many bytes it wrote, and the
> caller can then skip showing the decoded results if zero bytes
> were produced.
>
> We'll make a variant of snprintf() that's safe to call without having to
> check the return value for the case when it didn't have enough space to
> do its work, that is, make it return 0 in case no bytes were written.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  tools/intel_reg_decode.c | 195 ++++++++++++++++++++++++++---------------------
>  1 file changed, 109 insertions(+), 86 deletions(-)
>
> diff --git a/tools/intel_reg_decode.c b/tools/intel_reg_decode.c
> index 71f3ead6177f..0dbabe81ca3d 100644
> --- a/tools/intel_reg_decode.c
> +++ b/tools/intel_reg_decode.c
> @@ -26,6 +26,7 @@
>   */
>  
>  #include <errno.h>
> +#include <stdarg.h>
>  #include <stdbool.h>
>  #include <stdint.h>
>  #include <stdio.h>
> @@ -39,12 +40,32 @@
>  #include "intel_reg_spec.h"
>  
>  #define _DEBUGSTRING(func) \
> -	void func(char *result, int len, int reg, uint32_t val, uint32_t devid)
> +	int func(char *result, int len, int reg, uint32_t val, uint32_t devid)
>  #define DEBUGSTRING(func) static _DEBUGSTRING(func)

This could use a brief comment describing the return value.

[snip]

> @@ -2631,13 +2652,15 @@ int intel_reg_spec_decode(char *buf, size_t bufsize, const struct reg *reg,
>  			if (reg->addr != r->reg)
>  				continue;
>  
> -			if (r->debug_output)
> -				r->debug_output(tmp, sizeof(tmp), r->reg,
> -						val, devid);
> -			else if (devid)
> +			if (r->debug_output) {
> +				if (r->debug_output(tmp, sizeof(tmp), r->reg,
> +						    val, devid) == 0)
> +					continue;
> +			} else if (devid) {
>  				return 0;
> -			else
> +			} else {
>  				continue;
> +			}

Bah, I've included too much trickery between the devid == 0 vs != 0
cases to make this clear. No fault of this patch.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

No need to resend, please just add the comment on the return value.


>  
>  			if (devid) {
>  				strncpy(buf, tmp, bufsize);

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx