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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Aug 05, 2016 at 11:54:19AM +0300, Jani Nikula wrote:
> 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.

Added full blown docs for it, and pushed the lot.

/**
 * DEBUGSTRING:
 * @result: character string where to write
 * @len: maximum number of bytes to write
 * @reg: register offset
 * @val: register value
 * @devid: PCI device ID
 *
 * Print the decoded representation of the register value into @result.
 *
 * Returns: Number of bytes written to @result
 */

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx