Re: [PATCH 2/2] drm/i915: make debug printer shown_bug_once variable to drm_i915_private

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 15 Nov 2019, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Jani Nikula (2019-11-15 10:18:40)
>> Get rid of the module specific static variable.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h   | 2 ++
>>  drivers/gpu/drm/i915/i915_utils.c | 9 ++++-----
>>  2 files changed, 6 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 1779f600fcfb..e11ee3268ae3 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1283,6 +1283,8 @@ struct drm_i915_private {
>>         /* Mutex to protect the above hdcp component related values. */
>>         struct mutex hdcp_comp_mutex;
>>  
>> +       bool shown_bug_once;
>> +
>>         I915_SELFTEST_DECLARE(struct i915_selftest_stash selftest;)
>>  
>>         /*
>> diff --git a/drivers/gpu/drm/i915/i915_utils.c b/drivers/gpu/drm/i915/i915_utils.c
>> index c47261ae86ea..f434274b0b29 100644
>> --- a/drivers/gpu/drm/i915/i915_utils.c
>> +++ b/drivers/gpu/drm/i915/i915_utils.c
>> @@ -13,11 +13,10 @@
>>                     "providing the dmesg log by booting with drm.debug=0xf"
>>  
>>  void
>> -__i915_printk(struct drm_i915_private *dev_priv, const char *level,
>> +__i915_printk(struct drm_i915_private *i915, const char *level,
>>               const char *fmt, ...)
>>  {
>> -       static bool shown_bug_once;
>> -       struct device *kdev = dev_priv->drm.dev;
>> +       struct device *kdev = i915->drm.dev;
>>         bool is_error = level[1] <= KERN_ERR[1];
>>         bool is_debug = level[1] == KERN_DEBUG[1];
>>         struct va_format vaf;
>> @@ -39,7 +38,7 @@ __i915_printk(struct drm_i915_private *dev_priv, const char *level,
>>  
>>         va_end(args);
>>  
>> -       if (is_error && !shown_bug_once) {
>> +       if (is_error && !i915->shown_bug_once) {
>>                 /*
>>                  * Ask the user to file a bug report for the error, except
>>                  * if they may have caused the bug by fiddling with unsafe
>> @@ -47,7 +46,7 @@ __i915_printk(struct drm_i915_private *dev_priv, const char *level,
>>                  */
>>                 if (!test_taint(TAINT_USER))
>>                         dev_notice(kdev, "%s", FDO_BUG_MSG);
>
> I feel this plea to the users to file a bug report should be a one-time
> thing; a true global.

Fair enough.

I am wondering if we should have a convention of naming or commenting
legit globals, both to help automation detecting new accidental ones,
and to help people figure out not to send another conversion patch such
as this.

BR,
Jani.



> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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