Re: [PATCH v8 1/4] drm: Introduce device wedged event

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Oct 25, 2024 at 12:08:50PM +0300, Jani Nikula wrote:
> On Fri, 25 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:

...

> > +/*
> > + * Available recovery methods for wedged device. To be sent along with device
> > + * wedged uevent.
> > + */
> > +static const char *const drm_wedge_recovery_opts[] = {
> > +	[ffs(DRM_WEDGE_RECOVERY_REBIND) - 1]	= "rebind",
> > +	[ffs(DRM_WEDGE_RECOVERY_BUS_RESET) - 1]	= "bus-reset",
> > +};
> > +static_assert(ARRAY_SIZE(drm_wedge_recovery_opts) == ffs(DRM_WEDGE_RECOVERY_BUS_RESET));
> 
> This might work in most cases, but you also might end up finding that
> there's an arch and compiler combo out there that just won't be able to
> figure out ffs() at compile time, and the array initialization fails.

We have ilog2() macro for such cases, but it is rather fls() and not ffs(),
and I have no idea why ffs() even being used here, especially in the index
part of the array assignments. It's unreadable.

> If that happens, you'd have to either convert back to an enum (and call
> the wedge event function with BIT(DRM_WEDGE_RECOVERY_REBIND) etc.), or
> make this a array of structs mapping the macro values to strings and
> loop over it.
> 
> Also, the main point of the static assert was to ensure the array is
> updated when a new recovery option is added, and there's no out of
> bounds access. That no longer holds, and the static assert is pretty
> much useless. You still have to manually find and update this.

-- 
With Best Regards,
Andy Shevchenko