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