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

[Raag Jadav <raag.jadav@intel.com>]

On Fri, Oct 25, 2024 at 05:45:59PM +0300, Andy Shevchenko wrote:
> 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.

I initially had __builtin_ffs() in mind which is even more ugly.

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

Which would confuse the users since that's not how enums are normally used.

> > make this a array of structs mapping the macro values to strings and
> > loop over it.

Why not just switch() it?

	for_each_set_bit(opt, &method, size) {
		switch (BIT(opt)) {
		case DRM_WEDGE_RECOVERY_REBIND:
			recovery = "rebind";
			break;
		case DRM_WEDGE_RECOVERY_BUS_RESET:
			recovery = "bus-reset";
			break;
		}

		...
	}

I know we'll have to update it with new additions, but it'd be much simpler,
atleast compared to introducing and maintaining a new struct.

> > 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 above in place this won't be needed.

Raag