| Message ID | 20230921102519.3355538-2-badal.nilawar@intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Add HWMON support for DGFX | expand |
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. > #endif > -- > 2.25.1 >
On 21-09-2023 21:33, 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. This was suggested by Andy, in fact he suggested to reuse existing MISSING_CASE macro from i915. As I couldn't find common place to move it I went with creating new one. I will drop this patch and simply use drm_warn. Regards, Badal > >> #endif >> -- >> 2.25.1 >>
On Thu, 21 Sep 2023, "Nilawar, Badal" <badal.nilawar@intel.com> wrote: > On 21-09-2023 21:33, 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. > This was suggested by Andy, in fact he suggested to reuse existing > MISSING_CASE macro from i915. As I couldn't find common place to move it > I went with creating new one. > > I will drop this patch and simply use drm_warn. Please use drm_WARN() or drm_WARN_ON(). With panic_on_warn=1 in CI, it'll oops the machine, and we'll actually catch these as opposed to just leaving them as lines in dmesg. I guess the main purpose of MISSING_CASE() in i915 was to unify the behaviour, though I think that was also misused. BR, Jani. > > Regards, > Badal >> >>> #endif >>> -- >>> 2.25.1 >>>
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. Andi
> -----Original Message----- > From: Andi Shyti <andi.shyti@linux.intel.com> > Sent: Friday, September 22, 2023 8:46 PM > To: Vivi, Rodrigo <rodrigo.vivi@intel.com> > Cc: Nilawar, Badal <badal.nilawar@intel.com>; intel-xe@lists.freedesktop.org; > linux-hwmon@vger.kernel.org; Gupta, Anshuman > <anshuman.gupta@intel.com>; Dixit, Ashutosh <ashutosh.dixit@intel.com>; > linux@roeck-us.net; andi.shyti@linux.intel.com; Tauro, Riana > <riana.tauro@intel.com>; Brost, Matthew <matthew.brost@intel.com> > Subject: Re: [PATCH v5 1/6] drm/xe: Add XE_MISSING_CASE macro > > 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. IMHO Our CI aborts the on MISSING_CASE, which is not recoverable, drm_warn would Be better alternative here. Thanks, Anshuman Gupta. > > It's so widely used and actually nice to have that I thought many times to > move it to some core kernel libraries. > > Andi
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
On 23-09-2023 00:33, Rodrigo Vivi wrote: > 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. Agreed, for this use case I will also prefer to go with drm_warn. Regards, Badal > >> >> Andi
On Fri, 22 Sep 2023, "Gupta, Anshuman" <anshuman.gupta@intel.com> wrote: >> -----Original Message----- >> From: Andi Shyti <andi.shyti@linux.intel.com> >> Sent: Friday, September 22, 2023 8:46 PM >> To: Vivi, Rodrigo <rodrigo.vivi@intel.com> >> Cc: Nilawar, Badal <badal.nilawar@intel.com>; intel-xe@lists.freedesktop.org; >> linux-hwmon@vger.kernel.org; Gupta, Anshuman >> <anshuman.gupta@intel.com>; Dixit, Ashutosh <ashutosh.dixit@intel.com>; >> linux@roeck-us.net; andi.shyti@linux.intel.com; Tauro, Riana >> <riana.tauro@intel.com>; Brost, Matthew <matthew.brost@intel.com> >> Subject: Re: [PATCH v5 1/6] drm/xe: Add XE_MISSING_CASE macro >> >> 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. > IMHO Our CI aborts the on MISSING_CASE, which is not recoverable, drm_warn would > Be better alternative here. The whole point is that it aborts, so it won't get ignored. It's only for cases like this. BR, Jani. > Thanks, > Anshuman Gupta. >> >> It's so widely used and actually nice to have that I thought many times to >> move it to some core kernel libraries. >> >> Andi
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)) + #endif