On Fri, 18 Apr 2025 21:15:32 +0100
Adrián Larumbe <adrian.laru...@collabora.com> wrote:
> Hi Boris,
>
> On 18.04.2025 10:04, Boris Brezillon wrote:
> > On Fri, 18 Apr 2025 03:27:07 +0100
> > Adrián Larumbe <adrian.laru...@collabora.com> wrote:
> >
> > > +#ifdef CONFIG_DEBUG_FS
> > > +static void
> > > +panthor_gem_debugfs_format_flags(char flags_str[], int flags_len,
> > > + const char * const names[], u32 name_count,
> > > + u32 flags)
> > > +{
> > > + bool first = true;
> > > + int offset = 0;
> > > +
> > > +#define ACC_FLAGS(...) \
> > > + ({ \
> > > + offset += snprintf(flags_str + offset, flags_len - offset,
> > > ##__VA_ARGS__); \
> > > + if (offset == flags_len) \
> > > + return; \
> > > + })
> >
> > I tried applying, but checkpatch complains with:
> >
> > -:225: WARNING:MACRO_WITH_FLOW_CONTROL: Macros with flow control statements
> > should be avoided
> > #225: FILE: drivers/gpu/drm/panthor/panthor_gem.c:359:
> > +#define ACC_FLAGS(...) \
> > + ({ \
> > + offset += snprintf(flags_str + offset, flags_len - offset,
> > ##__VA_ARGS__); \
> > + if (offset == flags_len) \
> > + return; \
> > + })
> >
> >
> > and now I'm wondering why we even need to return early? Oh, and the
> > check looks suspicious to: snprintf() returns the number of chars
> > that would have been written if the destination was big enough, so
> > the offset will actually be greater than flags_len if the formatted
> > string doesn't fit.
>
> I noticed this warning when running checkpatch myself, and while a return
> isn't strictly
> necessary, I thought there was no point in going down the function when no
> more bytes
> could be written into the format string because it's already full.
>
> I did check the code for other locations where flow control is happening
> inside a macro
> and found this:
>
> #define group_queue_work(group, wname) \
> do { \
> group_get(group); \
> if (!queue_work((group)->ptdev->scheduler->wq, &(group)->wname
> ## _work)) \
> group_put(group); \
> } while (0)
>
> although I'm not sure whether the do {} while (0) is right there to make the
> warning go away.
No, the main difference is that it doesn't return behind the callers'
back which is what checkscript is complaining about, though using do {}
while(0) is usually preferred over ({}) when
you have nothing to return.
>
> > snprintf() returns the number of chars
> > that would have been written if the destination was big enough, so
> > the offset will actually be greater than flags_len if the formatted
> > string doesn't fit.
>
> Good catch, I don't know why I thought snprintf() would print at most
> 'flags_len
> - offset' bytes, and would return exactly that count at most too, rather than
> throwing a hypothetical max value. Then maybe checking whether 'if (offset >=
> flags_len)' would be enough ?
It should.
>
>
> > There are a few other formatting issues reported by checkpatch
> > --strict BTW.
> >
> > Unfortunately this led me to have a second look at these bits
> > and I have a few more comments, sorry about that :-/.
>
> > +
> > + ACC_FLAGS("%c", '(');
>
> > Now that flags have their own columns, I'm not sure it makes sense
> > surround them with parenthesis. That's even weird if we start running
> > out of space, because there would be an open '(', a few flags,
> > the last one being truncated, and no closing ')'. The other thing
> > that's bothering me is the fact we don't know when not all flags
> > have been displayed because of lack of space.
> >
> > > +
> > > + if (!flags)
> > > + ACC_FLAGS("%s", "none");
> > > +
> > > + while (flags) {
> > > + u32 bit = fls(flags) - 1;
> >
> > I would probably print flags in bit position order, so ffs()
> > instead of fls().
> >
> > > + u32 idx = bit + 1;
> >
> > Why do you have a "+ 1" here? Feels like an off-by-one error to
> > me.
> >
> > > +
> > > + if (!first)
> > > + ACC_FLAGS("%s", ",");
> > > +
> > > + if (idx < name_count && names[idx])
> > > + ACC_FLAGS("%s", names[idx]);
> > > +
> > > + first = false;
> > > + flags &= ~BIT(bit);
> > > + }
> > > +
> > > + ACC_FLAGS("%c", ')');
> > > +
> > > +#undef ACC_FLAGS
> > > +}
> >
> > How about something like that:
> >
> > static void
> > panthor_gem_debugfs_format_flags(char *flags_str, u32 flags_str_len,
> > const char * const flag_names[],
> > u32 flag_name_count, u32 flags)
> > {
> > int offset = 0;
> >
> > if (!flags) {
> > snprintf(flags_str, flags_str_len, "%s", "none");
> > return;
> > }
> >
> > while (flags) {
> > const char *flag_name = "?";
> > u32 flag = ffs(flags) - 1;
> > int new_offset = offset;
> >
> > flags &= ~BIT(flag);
> >
> > if (flag < flag_name_count && flag_names[flag])
> > flag_name = flag_names[flag];
> >
> > /* Print as much as we can. */
> > new_offset += snprintf(flags_str + offset, flags_str_len -
> > offset,
> > "%s%s", offset ? "," : "", flag_name);
> >
> > /* If we have flags remaining, check that we have enough space
> > for
> > * the "...".
> > */
> > if (flags)
> > new_offset += strlen(",...");
> >
> > /* If we overflowed, replace what we've written by "..." and
> > * bail out.
> > */
> > if (new_offset > flags_str_len) {
> > snprintf(flags_str + offset, flags_str_len - offset,
> > "%s...", offset ? "," : "");
> > return;
> > }
> >
> > offset = new_offset;
> > }
> > }
>
> This looks good to me. However, I'm starting to wonder whether it makes sense
> to
> try to come up with a very elaborate flag formatting scheme, because of two
> reasons:
>
> - It messes up with the output because we need to provide enough headroom in
> case the flag set will increase in the future. This is not a big deal because
> the debugfs file is meant to be parsed by UM tools, but ...
>
> - In case we go over the space available to print flags, not printing the
> remaining ones feels even less informative than printing let's say a
> hexadecimal
> mask, because parsing tools would rather deal with no missing data than the
> absence of human-readable flag names.
>
> On top of that, I think, while these flags could be mostly of interest to
> parsing
> tools, they'd be less so to someone casually peeking into the raw textual
> output. I think they'd be mostly interested in the process which triggered
> their creation, their size, virtual address in the VM, and above all their
> name
> (potentially a very long one).
>
> With all these things born in mind, I'd say we'd best just print a 32 bit mask
> for both flag fields, for which we'd always know the exact span in bytes, and
> then print all the available flag names in the debugfs file prelude for
> parsing
> tools to pick up on.
Yeah, I think I agree with you. The flag printing is messy as it is,
and if we're going to use a tool to parse the output, we're probably
better off with an hexadecimal value here.
Regards,
Boris
