Re: [PATCH v9 4/4] drm/panthor: show device-wide list of DRM GEM objects over DebugFS

[Boris Brezillon <boris.brezillon@collabora.com>]

On Fri, 18 Apr 2025 03:27:07 +0100
Adrián Larumbe <adrian.larumbe@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.

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;
        }
}