print: require struct drm_device for drm_err() and friends

Bill Wendling Fri, 16 May 2025 00:07:21 -0700

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 attemptingto get

'device->init_name'. The compiler sees this as undefined behavior, which may

lead to unexpected outcomes, especially with Clang where pathsdetermined to be

undefined are removed entirely under certain conditions.

(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

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

Reply via email to