Re: [PATCH] drm/i915: Introduce i915_dbg macro

[Dave Gordon <david.s.gordon@intel.com>]

On 26/01/16 09:44, Joonas Lahtinen wrote:
> On ma, 2016-01-25 at 18:57 +0000, Dave Gordon wrote:
>> On 25/01/16 18:17, Daniel Vetter wrote:
>>> On Fri, Jan 22, 2016 at 05:54:15PM +0530, akash.goel@intel.com
>>> wrote:
>>>> From: Akash Goel <akash.goel@intel.com>
>>>>
>>>> Added a new macro i915_dbg, which is a wrapper over dev_dbg
>>>> macro.
>>>> dev_dbg allows use of dynamic debug framework, so offers a number
>>>> of advantages over DRM_DEBUG to debug user space startup issues.
>>>> Like provides more fine grain control by allowing to
>>>> enable/disable
>>>> certain debug messages of interest on the fly, also allows
>>>> filtering
>>>> of debug messages based on pid.
>>>>
>>>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Signed-off-by: Akash Goel <akash.goel@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_drv.h | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>>> b/drivers/gpu/drm/i915/i915_drv.h
>>>> index bc7164f..749513f 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -2456,6 +2456,7 @@ struct drm_i915_cmd_table {
>>>>    		BUILD_BUG(); \
>>>>    	__p; \
>>>>    })
>>>> +#define i915_dbg(DEV, args...)	dev_dbg(__I915__(DEV)-
>>>>> dev->dev, ##args)
>>
>> I915_DBG(...) ?
>>
>> It's conventional that macros should be UPPERCASE.
>>
>> Especially when some config options may mean that the code
>> disappears
>> entirely, so you have to be sure not to use arguments with side-
>> effects!
>
> Slight correction here (for future), from Kernel Coding Style
> documentation;
>
> "CAPITALIZED macro names are appreciated but macros resembling
> functions may be named in lower case."
>
> And looking at "include/linux/device.h", dev_dbg definition is a macro
> too, like almost all the printing functions. I'd rather see it as
> i915_dbg. Arguments with side effects can be handled nicely as can be
> seen.
>
> We really should increase the priority of modernizing the debugging
> infrastructure for i915 (and as a dependency for DRM as Daniel hoped).
>
> Regards, Joonas
>
>> .Dave.

The fact that the upstream definitions are not great doesn't mean we 
should copy the flaws:

#if defined(CONFIG_DYNAMIC_DEBUG)
#define dev_dbg(dev, format, ...)                    \
do {                                                 \
         dynamic_dev_dbg(dev, format, ##__VA_ARGS__); \
} while (0)
#elif defined(DEBUG)
#define dev_dbg(dev, format, arg...)            \
         dev_printk(KERN_DEBUG, dev, format, ##arg)
#else
#define dev_dbg(dev, format, arg...)                            \
({                                                              \
         if (0)                                                  \
                 dev_printk(KERN_DEBUG, dev, format, ##arg);     \
})
#endif

So what's wrong with the above?

Firstly, the CONFIG_DYNAMIC_DEBUG version is wrapped in a do-while(0) 
but the others aren't; this makes them different syntactically - it's a 
statement body, whereas the others are (void) expressions. In either 
case, writing
	x = dev_dbg(...);
will give an error (different errors, though!). But the following:
	x = 1, dev_dbg(...);
compiles if not CONFIG_DYNAMIC_DEBUG. You probably wouldn't write the 
above, but it could itself be the result of a macro expansion, and it 
would work (x is assigned 1, dev_dbg() is called) ... until you try to 
enable dynamic debug.

(IMHO they should all be wrapped, which ensures you can't get away with 
using it in any other way than as a statement.)

Secondly, the CONFIG_DYNAMIC_DEBUG version uses the C99 __VA_ARGS__ 
syntax, whereas the others use the GCC-specific "arg..." method. This 
*probably* won't matter but it's an unnecessary inconsistency.

Thirdly, the non-DEBUG version doesn't evaluate its arguments, whereas 
the other two obviously do. So code that includes a side-effect inside 
the parameters to the call will behave differently; and there'll be no 
clue at all that something that looks like a regular function call:

	dev_dbg(mydev, "Been here %d times now", ++i);

... may or may not increment i, depending on the compile-time definition 
above. This is just laying traps for the developer; calling it DEV_DBG() 
might at least make people *notice* that it's a macro not a function!

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx