[PATCH 5/5] drm/print: require struct drm_device for drm_err() and friends
Bill Wendling
isanbard at gmail.com
Fri May 16 20:41:56 UTC 2025
On Fri, May 16, 2025 at 2:48 AM Jani Nikula <jani.nikula at intel.com> wrote:
> On Thu, 15 May 2025, Bill Wendling <isanbard at 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 at 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.
>
I did a quick check and vmlinux size changed by only about 0.0078%.
Most modules didn't change size, some did increase, but typically less
than 1%, and a few actually shrank in size (??). The largest change
was 6.5840% : counter.ko.
Reverting the patches would probably work, but that relies upon 'drm'
never being NULL. Indeed, it looks like 'dev_driver_string' is wary of
a NULL 'drm', though it still accesses the argument as if it couldn't
be NULL... This all seems like a disaster waiting to happen, to be
honest. There should either be no way for 'drm' to be NULL or handling
for when it is. What happened with this series of patches is adding
only partial handling for when it is NULL.
-bw
More information about the Intel-gfx
mailing list