Re: [PATCH] drm/i915: Remove WARN_ON and WARN_ON_ONCE overrides.

From: Jani Nikula <jani.nikula@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@linux.ie>,
	Pankaj Bharadiya <pankaj.laxminarayan.bharadiya@intel.com>,
	intel-gfx <intel-gfx@lists.freedesktop.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>
Subject: Re: [PATCH] drm/i915: Remove WARN_ON and WARN_ON_ONCE overrides.
Date: Thu, 02 Apr 2020 11:09:21 +0300	[thread overview]
Message-ID: <87h7y2mhqm.fsf@intel.com> (raw)
In-Reply-To: <CAKMK7uHNEXffFOdoD5W=KVCCP-1Gi-epp77FoPswPWUvPSNDXA@mail.gmail.com>

On Thu, 02 Apr 2020, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Apr 2, 2020 at 8:23 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>
>> On Thu, 02 Apr 2020, Pankaj Bharadiya <pankaj.laxminarayan.bharadiya@intel.com> wrote:
>> > Now we have new struct drm_device based drm_WARN* macros. These are
>> > preferred over the regular WARN* macros.
>> >
>> > Remove WARN_ON and WARN_ON_ONCE overriedes to avoid any temptations to
>> > use them in the future.
>>
>> Well, since they are overrides of macros in bug.h, the users are still
>> there. There are still 100+ users in i915. You just don't get as much
>> information with them after this patch.
>>
>> I'm not opposed to this patch, but at the same time I'd like to see more
>> converted to the drm_WARN* alternatives.
>
> Yeah our overrides are super useful, I think better to try to move
> them to the core version. So many times when I didn't know which
> warning was hit in a bug report (warnings in drm core already have
> this problem), and the additional output here would have helped. I'd
> at least keep this for now.

The drm_WARN_ON, drm_WARN_ON_ONCE macros in drm_print.h are comparable
replacements, with the device info as well as the condition.

BR,
Jani.

> -Daniel
>
>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>>
>>
>> BR,
>> Jani.
>>
>> >
>> > Suggested-by: Jani Nikula <jani.nikula@intel.com>
>> > Signed-off-by: Pankaj Bharadiya <pankaj.laxminarayan.bharadiya@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_utils.h | 15 ---------------
>> >  1 file changed, 15 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
>> > index 03a73d2bd50d..c666a64375d9 100644
>> > --- a/drivers/gpu/drm/i915/i915_utils.h
>> > +++ b/drivers/gpu/drm/i915/i915_utils.h
>> > @@ -36,21 +36,6 @@ struct timer_list;
>> >
>> >  #define FDO_BUG_URL "https://gitlab.freedesktop.org/drm/intel/-/wikis/How-to-file-i915-bugs"
>> >
>> > -#undef WARN_ON
>> > -/* Many gcc seem to no see through this and fall over :( */
>> > -#if 0
>> > -#define WARN_ON(x) ({ \
>> > -     bool __i915_warn_cond = (x); \
>> > -     if (__builtin_constant_p(__i915_warn_cond)) \
>> > -             BUILD_BUG_ON(__i915_warn_cond); \
>> > -     WARN(__i915_warn_cond, "WARN_ON(" #x ")"); })
>> > -#else
>> > -#define WARN_ON(x) WARN((x), "%s", "WARN_ON(" __stringify(x) ")")
>> > -#endif
>> > -
>> > -#undef WARN_ON_ONCE
>> > -#define WARN_ON_ONCE(x) WARN_ONCE((x), "%s", "WARN_ON_ONCE(" __stringify(x) ")")
>> > -
>> >  #define MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \
>> >                            __stringify(x), (long)(x))
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel