On Thu, May 15, 2025 at 1:18 PM 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.
>
> (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.)
>
I potential fix for this would be something like this (untested). I'm
not familiar with how 'dev_name' is used to know whether or not this
could cause issues:
diff --git a/include/linux/device.h b/include/linux/device.h
index 79e49fe494b7..ea20d439fe8e 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -778,6 +778,9 @@ static inline bool device_iommu_mapped(struct device *dev)
*/
static inline const char *dev_name(const struct device *dev)
{
+ if (!dev)
+ return "default";
+
/* Use the init name until the kobject becomes available */
if (dev->init_name)
return dev->init_name;
-bw
