Re: [PATCH 5/5] drm/print: require struct drm_device for drm_err() and friends

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

On Thu, 15 May 2025, Bill Wendling <isanbard@gmail.com> wrote:
> On 1/23/25 7:09 AM, Jani Nikula wrote:
>> The expectation is that the struct drm_device based logging helpers get
>> passed an actual struct drm_device pointer rather than some random
>> struct pointer where you can dereference the ->dev member.
>> 
>> Add a static inline helper to convert struct drm_device to struct
>> device, with the main benefit being the type checking of the macro
>> argument.
>> 
>> As a side effect, this also reduces macro argument double references.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>   include/drm/drm_print.h | 41 +++++++++++++++++++++++------------------
>>   1 file changed, 23 insertions(+), 18 deletions(-)
>> 
>> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
>> index 9732f514566d..f31eba1c7cab 100644
>> --- a/include/drm/drm_print.h
>> +++ b/include/drm/drm_print.h
>> @@ -584,9 +584,15 @@ void __drm_dev_dbg(struct _ddebug *desc, const struct device *dev,
>>    * Prefer drm_device based logging over device or prink based logging.
>>    */
>>   
>> +/* Helper to enforce struct drm_device type */
>> +static inline struct device *__drm_to_dev(const struct drm_device *drm)
>> +{
>> +	return drm ? drm->dev : NULL;
>> +}
>> +
>>   /* Helper for struct drm_device based logging. */
>>   #define __drm_printk(drm, level, type, fmt, ...)			\
>> -	dev_##level##type((drm) ? (drm)->dev : NULL, "[drm] " fmt, ##__VA_ARGS__)
>> +	dev_##level##type(__drm_to_dev(drm), "[drm] " fmt, ##__VA_ARGS__)
>>   
>>   
>>   #define drm_info(drm, fmt, ...)					\
>> @@ -620,25 +626,25 @@ void __drm_dev_dbg(struct _ddebug *desc, const struct device *dev,
>>   
>>   
>>   #define drm_dbg_core(drm, fmt, ...)					\
>> -	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_CORE, fmt, ##__VA_ARGS__)
>> -#define drm_dbg_driver(drm, fmt, ...)						\
>> -	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
>> +	drm_dev_dbg(__drm_to_dev(drm), DRM_UT_CORE, fmt, ##__VA_ARGS__)
>> +#define drm_dbg_driver(drm, fmt, ...)					\
>> +	drm_dev_dbg(__drm_to_dev(drm), DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
>>   #define drm_dbg_kms(drm, fmt, ...)					\
>> -	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_KMS, fmt, ##__VA_ARGS__)
>> +	drm_dev_dbg(__drm_to_dev(drm), DRM_UT_KMS, fmt, ##__VA_ARGS__)
>>   #define drm_dbg_prime(drm, fmt, ...)					\
>> -	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_PRIME, fmt, ##__VA_ARGS__)
>> +	drm_dev_dbg(__drm_to_dev(drm), DRM_UT_PRIME, fmt, ##__VA_ARGS__)
>>   #define drm_dbg_atomic(drm, fmt, ...)					\
>> -	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
>> +	drm_dev_dbg(__drm_to_dev(drm), DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
>>   #define drm_dbg_vbl(drm, fmt, ...)					\
>> -	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_VBL, fmt, ##__VA_ARGS__)
>> +	drm_dev_dbg(__drm_to_dev(drm), DRM_UT_VBL, fmt, ##__VA_ARGS__)
>>   #define drm_dbg_state(drm, fmt, ...)					\
>> -	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_STATE, fmt, ##__VA_ARGS__)
>> +	drm_dev_dbg(__drm_to_dev(drm), DRM_UT_STATE, fmt, ##__VA_ARGS__)
>>   #define drm_dbg_lease(drm, fmt, ...)					\
>> -	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_LEASE, fmt, ##__VA_ARGS__)
>> +	drm_dev_dbg(__drm_to_dev(drm), DRM_UT_LEASE, fmt, ##__VA_ARGS__)
>>   #define drm_dbg_dp(drm, fmt, ...)					\
>> -	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DP, fmt, ##__VA_ARGS__)
>> +	drm_dev_dbg(__drm_to_dev(drm), DRM_UT_DP, fmt, ##__VA_ARGS__)
>>   #define drm_dbg_drmres(drm, fmt, ...)					\
>> -	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRMRES, fmt, ##__VA_ARGS__)
>> +	drm_dev_dbg(__drm_to_dev(drm), DRM_UT_DRMRES, fmt, ##__VA_ARGS__)
>>   
>>   #define drm_dbg(drm, fmt, ...)	drm_dbg_driver(drm, fmt, ##__VA_ARGS__)
>>   
>> @@ -727,10 +733,9 @@ void __drm_err(const char *format, ...);
>>   #define __DRM_DEFINE_DBG_RATELIMITED(category, drm, fmt, ...)					\
>>   ({												\
>>   	static DEFINE_RATELIMIT_STATE(rs_, DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST);\
>> -	const struct drm_device *drm_ = (drm);							\
>>   												\
>>   	if (drm_debug_enabled(DRM_UT_ ## category) && __ratelimit(&rs_))			\
>> -		drm_dev_printk(drm_ ? drm_->dev : NULL, KERN_DEBUG, fmt, ## __VA_ARGS__);	\
>> +		drm_dev_printk(__drm_to_dev(drm), KERN_DEBUG, fmt, ## __VA_ARGS__);		\
>>   })
>>   
>>   #define drm_dbg_ratelimited(drm, fmt, ...) \
>> @@ -752,13 +757,13 @@ void __drm_err(const char *format, ...);
>>   /* Helper for struct drm_device based WARNs */
>>   #define drm_WARN(drm, condition, format, arg...)			\
>>   	WARN(condition, "%s %s: [drm] " format,				\
>> -			dev_driver_string((drm)->dev),			\
>> -			dev_name((drm)->dev), ## arg)
>> +			dev_driver_string(__drm_to_dev(drm)),		\
>> +			dev_name(__drm_to_dev(drm)), ## arg)
>>   
>>   #define drm_WARN_ONCE(drm, condition, format, arg...)			\
>>   	WARN_ONCE(condition, "%s %s: [drm] " format,			\
>> -			dev_driver_string((drm)->dev),			\
>> -			dev_name((drm)->dev), ## arg)
>> +			dev_driver_string(__drm_to_dev(drm)),		\
>> +			dev_name(__drm_to_dev(drm)), ## arg)
>>
> Hi Jani,
>
> These two changes introduce undefined behavior into these macros. The final
> code generation becomes this (from 'bxt_port_to_phy_channel'):
>
> 	__warn_printk("%s %s: [drm] " "PHY not found for PORT %c",
> 		      dev_driver_string(__drm_to_dev(display->drm)),
> 		      dev_name(__drm_to_dev(display->drm)),
> 		      (port + 'A'));
>
> The issue lies in 'dev_name(__drm_to_dev(display->drm))'. After inlining, it
> becomes this (pseudo code):
>
> 	struct device *device = display->drm ? display->drm->dev : NULL;
> 	const char *name = device->init_name ? device->init_name
> 					     : kobject_name(&device->kobj);
>
> 	__warn_printk("%s %s: [drm] " "PHY not found for PORT %c",
> 		      dev_driver_string(device), name, (port + 'A'));
>
> The issue, of course, is that the 'device' may be NULL when attempting 
> to get
> 'device->init_name'. The compiler sees this as undefined behavior, which may
> lead to unexpected outcomes, especially with Clang where paths 
> determined to be
> undefined are removed entirely under certain conditions.

Would it be better to just revert the drm_WARN() and drm_WARN_ONCE()
macros to use (drm)->dev directly?

It's not ideal, but as the quick fix.

I don't think adding the check in dev_name() would go down well, as
there are roughly 5k users of it, and feels like unnecessary code size
bloat.


BR,
Jani.



>
> (Note, I'm working on making this behavior less draconian by adopting a GCC
> pass, but this will take time to filter out to Linux devs.)
>
> Regards,
> -bw
>

-- 
Jani Nikula, Intel