Re: [Intel-xe] [PATCH v5 1/6] drm/xe: Add XE_MISSING_CASE macro

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Sep 22, 2023 at 05:16:23PM +0200, Andi Shyti wrote:
> Hi Rodrigo,
> 
> On Thu, Sep 21, 2023 at 12:03:26PM -0400, Rodrigo Vivi wrote:
> > On Thu, Sep 21, 2023 at 03:55:14PM +0530, Badal Nilawar wrote:
> > > Add XE_MISSING_CASE macro to handle missing switch case
> > > 
> > > v2: Add comment about macro usage (Himal)
> > > 
> > > Cc: Andi Shyti <andi.shyti@linux.intel.com>
> > > Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> > > Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_macros.h | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_macros.h b/drivers/gpu/drm/xe/xe_macros.h
> > > index daf56c846d03..6c74c69920ed 100644
> > > --- a/drivers/gpu/drm/xe/xe_macros.h
> > > +++ b/drivers/gpu/drm/xe/xe_macros.h
> > > @@ -15,4 +15,8 @@
> > >  			    "Ioctl argument check failed at %s:%d: %s", \
> > >  			    __FILE__, __LINE__, #cond), 1))
> > >  
> > > +/* Parameter to macro should be a variable name */
> > > +#define XE_MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \
> > > +				__stringify(x), (long)(x))
> > > +
> > 
> > No, please! Let's not add unnecessary macros.
> 
> it's not a bad idea, actually... in i915 we have the MISSING_CASE
> for switch cases with a defined number of cases and print
> warnings in case none if them is the one inside the switch.
> 
> It's so widely used and actually nice to have that I thought many
> times to move it to some core kernel libraries.

to be really honest, I also liked the MISSING_CASE in many occasions.
It is useful for platform enabling so once we add a new hardware we
ensure to get some splats on the first power-on and can easily
identify the missing cases.

But even in i915 I have already seen some miss-usages of that.
And i915 is full of i915isms that we believe it is good but we
never tried to move to core kernel or when we tried we got some push
backs showing that that is not very common and desired way.

I know, just looking around Xe is also starting to get Xeisms,
but I'd like to avoid that as much as we can. in this case here
it is only 2 lines that could be a standard drm_warn or similar call.

I don't believe that it is worth adding a macro for this. So, maybe
if we really want this the right approach is to move the i915's one
to core kernel and then make Xe to start using it.

> 
> Andi