[PATCH v7 1/5] drm: Introduce device wedged event

[Andy Shevchenko]

On Tue, Oct 01, 2024 at 08:08:18AM +0300, Raag Jadav wrote:
> On Mon, Sep 30, 2024 at 03:59:59PM +0300, Andy Shevchenko wrote:
> > On Mon, Sep 30, 2024 at 01:08:41PM +0530, Raag Jadav wrote:

...

> > > +static const char *const drm_wedge_recovery_opts[] = {
> > > +	[DRM_WEDGE_RECOVERY_REBIND] = "rebind",
> > > +	[DRM_WEDGE_RECOVERY_BUS_RESET] = "bus-reset",
> > > +	[DRM_WEDGE_RECOVERY_REBOOT] = "reboot",
> > > +};
> > 
> > Place for static_assert() is here, as it closer to the actual data we test...
> 
> Shouldn't it be at the point of access?

No, the idea of static_assert() is in word 'static', meaning it's allowed to be
used in the global space.

> If no, why do we care about the data when it's not being used?

What does this suppose to mean? The assertion is for enforcing the boundaries
that are defined by different means (constant of the size and real size of
an array).

...

> > > +static bool drm_wedge_recovery_is_valid(enum drm_wedge_recovery method)
> > > +{
> > > +	static_assert(ARRAY_SIZE(drm_wedge_recovery_opts) == DRM_WEDGE_RECOVERY_MAX);
> > 
> > ...it doesn't fully belong to this function (or only to this function).
> 
> The purpose of having a helper is to have a single point of access, no?

What single access you are talking about? It seems you are trying to solve
non-existing issue. There is a function that is being used exactly once
and it's a one-liner. There is no point to have it being separated (at least
right now).

> Side note: It also goes well with is_valid() semantic IMHO.

It doesn't matter at all, it's unrelated to the point.

> > > +	return method >= DRM_WEDGE_RECOVERY_REBIND && method < DRM_WEDGE_RECOVERY_MAX;
> > > +}
> > 
> > Why do we need this one-liner (after above comment being addressed) as a
> > separate function?
> 
> I'm not sure if I'm following you. Method is not a constant here, we'll get it
> on the stack.

I elaborated above.

-- 
With Best Regards,
Andy Shevchenko