* Re: [PATCH 5/5] drm/print: require struct drm_device for drm_err() and friends
[not found] ` <dfe6e774883e6ef93cfaa2b6fe92b804061ab9d9.1737644530.git.jani.nikula@intel.com>
@ 2025-05-15 20:18 ` Bill Wendling
2025-05-15 23:52 ` Bill Wendling
2025-05-16 9:48 ` Jani Nikula
0 siblings, 2 replies; 4+ messages in thread
From: Bill Wendling @ 2025-05-15 20:18 UTC (permalink / raw)
To: Jani Nikula, dri-devel
Cc: intel-gfx, gustavo.sousa, maarten.lankhorst, mripard,
tzimmermann, airlied, simona, linux-kernel, kees
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.
(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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 5/5] drm/print: require struct drm_device for drm_err() and friends
2025-05-15 20:18 ` [PATCH 5/5] drm/print: require struct drm_device for drm_err() and friends Bill Wendling
@ 2025-05-15 23:52 ` Bill Wendling
2025-05-16 9:48 ` Jani Nikula
1 sibling, 0 replies; 4+ messages in thread
From: Bill Wendling @ 2025-05-15 23:52 UTC (permalink / raw)
To: Jani Nikula, dri-devel
Cc: intel-gfx, gustavo.sousa, maarten.lankhorst, mripard,
tzimmermann, airlied, simona, linux-kernel, kees
On Thu, May 15, 2025 at 1:18 PM 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.
>
> (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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 5/5] drm/print: require struct drm_device for drm_err() and friends
2025-05-15 20:18 ` [PATCH 5/5] drm/print: require struct drm_device for drm_err() and friends Bill Wendling
2025-05-15 23:52 ` Bill Wendling
@ 2025-05-16 9:48 ` Jani Nikula
2025-05-16 20:41 ` Bill Wendling
1 sibling, 1 reply; 4+ messages in thread
From: Jani Nikula @ 2025-05-16 9:48 UTC (permalink / raw)
To: Bill Wendling, dri-devel
Cc: intel-gfx, gustavo.sousa, maarten.lankhorst, mripard,
tzimmermann, airlied, simona, linux-kernel, kees
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 5/5] drm/print: require struct drm_device for drm_err() and friends
2025-05-16 9:48 ` Jani Nikula
@ 2025-05-16 20:41 ` Bill Wendling
0 siblings, 0 replies; 4+ messages in thread
From: Bill Wendling @ 2025-05-16 20:41 UTC (permalink / raw)
To: Jani Nikula
Cc: dri-devel, intel-gfx, gustavo.sousa, maarten.lankhorst, mripard,
tzimmermann, airlied, simona, linux-kernel, kees
On Fri, May 16, 2025 at 2:48 AM Jani Nikula <jani.nikula@intel.com> wrote:
> 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.
>
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-05-16 20:42 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <cover.1737644530.git.jani.nikula@intel.com>
[not found] ` <dfe6e774883e6ef93cfaa2b6fe92b804061ab9d9.1737644530.git.jani.nikula@intel.com>
2025-05-15 20:18 ` [PATCH 5/5] drm/print: require struct drm_device for drm_err() and friends Bill Wendling
2025-05-15 23:52 ` Bill Wendling
2025-05-16 9:48 ` Jani Nikula
2025-05-16 20:41 ` Bill Wendling
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®