Re: [PATCH v6 17/28] drm/atomic-state-helper: Provide comparison macros

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] drm/atomic: incorrect field name stringified in STATE_CHECK static_asserts
- [Low] drm/atomic: format string mismatch in STATE_CHECK_U32_16_16 macro
--

commit 82f5bb4389d99e46549406b2a6e1384ec96ce6cc
Author: Maxime Ripard <mripard@kernel.org>

drm/atomic-state-helper: Provide comparison macros

In order to make the implementation of atomic_sro_compare_state easier
and reduce the boilerplate, introduce a set of STATE_CHECK_* macros to
compare state field values depending on their type.

> diff --git a/include/drm/drm_atomic_sro_helper.h b/include/drm/drm_atomic_sro_helper.h
> index a42ed8136a499..71340978ce7c4 100644
> --- a/include/drm/drm_atomic_sro_helper.h
> +++ b/include/drm/drm_atomic_sro_helper.h
[ ... ]
> +#define STATE_CHECK_BOOL(r, p, n, sa, sb, f)				\
> +	do {								\
> +		static_assert(__same_type(sa->f, sb->f),		\
> +			      __stringify(f) " field types don't match"); \
> +		static_assert(__same_type(sa->f, bool),			\
> +			      __stringify(n) " is not a bool");		\
                              ^^^^^^^^^^^^^^

[Severity: Low]
Is this static_assert using the wrong parameter for the stringified message?

Since 'n' is the object's human-readable name and 'f' is the field name,
if a type mismatch occurs, the compiler will emit a message like "plane is
not a bool" instead of the intended field name.

Should this use __stringify(f) instead of __stringify(n)? This pattern
appears to be repeated across all the STATE_CHECK_* macros.

[ ... ]
> +#define STATE_CHECK_U32_16_16(r, p, n, sa, sb, f)			\
> +	do {								\
> +		static_assert(__same_type(sa->f, sb->f),		\
> +			      __stringify(f) " field types don't match"); \
> +		static_assert(__same_type(sa->f, (u32)0),		\
> +			      __stringify(n) " is not a u32");		\
> +		if (sa->f != sb->f) {					\
> +			drm_atomic_helper_print_state_mismatch(p,	\
> +							       n,	\
> +							       __stringify(f), \
> +							       "expected %d.%06u, got %d.%06u", \
                                                                        ^^              ^^

[Severity: Low]
Does this format string mismatch the argument types?

The expression 'sa->f >> 16' produces an unsigned int (since sa->f is u32),
but the format string uses '%d' instead of '%u'. This can trigger a -Wformat
compiler warning.

> +							       sa->f >> 16, ((sa->f & 0xffff) * 15625) >> 10, \
> +							       sb->f >> 16, ((sb->f & 0xffff) * 15625) >> 10); \
> +			r = false;					\
> +		}							\
> +	} while (0)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-drm-state-readout-v6-0-6cc02cd10998@kernel.org?part=17