On Fri, Aug 05, 2016 at 11:54:19AM +0300, Jani Nikula wrote:
> On Fri, 05 Aug 2016, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
