On Thu, 15 May 2025, Bill Wendling <isanb...@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.nik...@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
