[v7,1/7] drm/print: Fix and add support for NULL as first argument in drm_* macros

  Comments say macros DRM_DEBUG_* are deprecated in favor of
drm_dbg_*(NULL, ...), but they have broken support for it,
as the macro will result in `(NULL) ? (NULL)->dev : NULL`.

Thus, fix them by separating logic to get dev ptr in a new
function, which will return the dev ptr if arg is not NULL.
Use it in drm_dbg_*, and also in __DRM_DEFINE_DBG_RATELIMITED,
where a similar (but correct) NULL check was in place.

Also, add support for NULL in __drm_printk, so that all the
drm_* macros will hence support NULL as the first argument.
This also means that deprecation comments mentioning pr_()*
can now be changed to the drm equivalents.

There is a need to support device pointers, as in some cases,
we may not have drm_device but just the device ptr, such as
when dealing with struct mipi_dsi_host. Before this change,
passing just mipi_dsi_host would have worked, since due to
preprocessing, the resultant would be "host->dev", but now
due to NULL check that cannot happen.

Signed-off-by: Siddh Raman Pant <code@siddh.me>
---
 include/drm/drm_print.h | 114 ++++++++++++++++++++++++++++++----------
 1 file changed, 87 insertions(+), 27 deletions(-)
  

On Sun, 26 Feb 2023, Siddh Raman Pant <code@siddh.me> wrote:
> Comments say macros DRM_DEBUG_* are deprecated in favor of
> drm_dbg_*(NULL, ...), but they have broken support for it,
> as the macro will result in `(NULL) ? (NULL)->dev : NULL`.
>
> Thus, fix them by separating logic to get dev ptr in a new
> function, which will return the dev ptr if arg is not NULL.
> Use it in drm_dbg_*, and also in __DRM_DEFINE_DBG_RATELIMITED,
> where a similar (but correct) NULL check was in place.
>
> Also, add support for NULL in __drm_printk, so that all the
> drm_* macros will hence support NULL as the first argument.
> This also means that deprecation comments mentioning pr_()*
> can now be changed to the drm equivalents.
>
> There is a need to support device pointers, as in some cases,
> we may not have drm_device but just the device ptr, such as
> when dealing with struct mipi_dsi_host. Before this change,
> passing just mipi_dsi_host would have worked, since due to
> preprocessing, the resultant would be "host->dev", but now
> due to NULL check that cannot happen.

First of all, that's two distinct changes in one patch. The subject says
one thing, but it's really two.

But the main question is, do we *really* want to let callers pass either
struct drm_device * or struct device *? It will be type safe with
generics, but if it's okay to use either, people *will* use either. The
call sites will end up being a mixture of both. You can't control it. It
will be very tedious if you ever want to revert that decision.

Do we want to promote a style where you can pass either? To me, in C
context, it seems awfully sloppy and confusing rather than convenient.

I'd argue the struct mipi_dsi_host stuff should use dev_* calls
directly, as it's more of a special case, rather than allow struct
device * in drm_* logging macros.


BR,
Jani.


>
> Signed-off-by: Siddh Raman Pant <code@siddh.me>
> ---
>  include/drm/drm_print.h | 114 ++++++++++++++++++++++++++++++----------
>  1 file changed, 87 insertions(+), 27 deletions(-)
>
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index a93a387f8a1a..1a7d4064d120 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -34,6 +34,7 @@
>  #include <linux/dynamic_debug.h>
>  
>  #include <drm/drm.h>
> +#include <drm/drm_device.h>
>  
>  /* Do *not* use outside of drm_print.[ch]! */
>  extern unsigned long __drm_debug;
> @@ -445,15 +446,73 @@ void __drm_dev_dbg(struct _ddebug *desc, const struct device *dev,
>  #define DRM_DEV_DEBUG_KMS(dev, fmt, ...)				\
>  	drm_dev_dbg(dev, DRM_UT_KMS, fmt, ##__VA_ARGS__)
>  
> +/* Helpers for struct drm_device based logging. */
> +
> +/**
> + * __drm_dev_ptr - Helper function to get drm->dev pointer.
> + * @drm: struct drm_device pointer.
> + *
> + * RETURNS:
> + * The struct device pointer (NULL if @drm is NULL).
> + */
> +static inline struct device *__drm_dev_ptr(const struct drm_device *drm)
> +{
> +	if (drm)
> +		return drm->dev;
> +
> +	return NULL;
> +}
> +
> +/**
> + * __dev_ptr - Helper function to get the same device pointer.
> + * @dev: struct device pointer, or NULL.
> + *
> + * RETURNS:
> + * The struct device pointer (NULL if @dev is NULL).
> + */
> +static inline struct device *__dev_ptr(const struct device *dev)
> +{
> +	return (struct device *)dev;
> +}
> +
> +/**
> + * __get_dev_ptr - Helper to get device pointer given struct drm_device *,
> + *		   or struct device *, or NULL.
> + *
> + * Primarily for use in drm_*() print macros, since they need to handle NULL
> + * as the first argument passed.
> + *
> + * struct device pointers are supported for edge cases (such as mipi_dsi_host),
> + * the default (and recommended) way is to pass struct drm_device pointer.
> + */
> +#define  __get_dev_ptr(drm)			\
> +	_Generic((drm),				\
> +		struct device * :		\
> +			__dev_ptr,		\
> +		const struct device * :		\
> +			__dev_ptr,		\
> +		default :			\
> +			__drm_dev_ptr		\
> +	)(drm)
> +
>  /*
>   * struct drm_device based logging
>   *
>   * Prefer drm_device based logging over device or prink based logging.
> + *
> + * The below macros support device pointers to cope for edge cases where
> + * we only have device (like in mipi_dsi_host) but not drm_device.
>   */
>  
>  /* Helper for struct drm_device based logging. */
>  #define __drm_printk(drm, level, type, fmt, ...)			\
> -	dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__)
> +({									\
> +	struct device *__dev_ = __get_dev_ptr(drm);			\
> +	if (__dev_)							\
> +		dev_##level##type(__dev_, "[drm] " fmt, ##__VA_ARGS__);	\
> +	else								\
> +		pr_##level##type("[drm] " fmt, ##__VA_ARGS__);		\
> +})
>  
>  
>  #define drm_info(drm, fmt, ...)					\
> @@ -487,25 +546,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(__get_dev_ptr(drm), DRM_UT_CORE, fmt, ##__VA_ARGS__)
> +#define drm_dbg_driver(drm, fmt, ...)					\
> +	drm_dev_dbg(__get_dev_ptr(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(__get_dev_ptr(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(__get_dev_ptr(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(__get_dev_ptr(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(__get_dev_ptr(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(__get_dev_ptr(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(__get_dev_ptr(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(__get_dev_ptr(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(__get_dev_ptr(drm), DRM_UT_DRMRES, fmt, ##__VA_ARGS__)
>  
>  #define drm_dbg(drm, fmt, ...)	drm_dbg_driver(drm, fmt, ##__VA_ARGS__)
>  
> @@ -533,31 +592,31 @@ void __drm_err(const char *format, ...);
>  #define _DRM_PRINTK(once, level, fmt, ...)				\
>  	printk##once(KERN_##level "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
>  
> -/* NOTE: this is deprecated in favor of pr_info(). */
> +/* NOTE: this is deprecated in favor of drm_info(NULL, ...). */
>  #define DRM_INFO(fmt, ...)						\
>  	_DRM_PRINTK(, INFO, fmt, ##__VA_ARGS__)
> -/* NOTE: this is deprecated in favor of pr_notice(). */
> +/* NOTE: this is deprecated in favor of drm_notice(NULL, ...). */
>  #define DRM_NOTE(fmt, ...)						\
>  	_DRM_PRINTK(, NOTICE, fmt, ##__VA_ARGS__)
> -/* NOTE: this is deprecated in favor of pr_warn(). */
> +/* NOTE: this is deprecated in favor of drm_warn(NULL, ...). */
>  #define DRM_WARN(fmt, ...)						\
>  	_DRM_PRINTK(, WARNING, fmt, ##__VA_ARGS__)
>  
> -/* NOTE: this is deprecated in favor of pr_info_once(). */
> +/* NOTE: this is deprecated in favor of drm_info_once(NULL, ...). */
>  #define DRM_INFO_ONCE(fmt, ...)						\
>  	_DRM_PRINTK(_once, INFO, fmt, ##__VA_ARGS__)
> -/* NOTE: this is deprecated in favor of pr_notice_once(). */
> +/* NOTE: this is deprecated in favor of drm_notice_once(NULL, ...). */
>  #define DRM_NOTE_ONCE(fmt, ...)						\
>  	_DRM_PRINTK(_once, NOTICE, fmt, ##__VA_ARGS__)
> -/* NOTE: this is deprecated in favor of pr_warn_once(). */
> +/* NOTE: this is deprecated in favor of drm_warn_once(NULL, ...). */
>  #define DRM_WARN_ONCE(fmt, ...)						\
>  	_DRM_PRINTK(_once, WARNING, fmt, ##__VA_ARGS__)
>  
> -/* NOTE: this is deprecated in favor of pr_err(). */
> +/* NOTE: this is deprecated in favor of drm_err(NULL, ...). */
>  #define DRM_ERROR(fmt, ...)						\
>  	__drm_err(fmt, ##__VA_ARGS__)
>  
> -/* NOTE: this is deprecated in favor of pr_err_ratelimited(). */
> +/* NOTE: this is deprecated in favor of drm_err_ratelimited(NULL, ...). */
>  #define DRM_ERROR_RATELIMITED(fmt, ...)					\
>  	DRM_DEV_ERROR_RATELIMITED(NULL, fmt, ##__VA_ARGS__)
>  
> @@ -593,13 +652,14 @@ void __drm_err(const char *format, ...);
>  #define DRM_DEBUG_DP(fmt, ...)						\
>  	__drm_dbg(DRM_UT_DP, fmt, ## __VA_ARGS__)
>  
> -#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__);	\
> +#define __DRM_DEFINE_DBG_RATELIMITED(category, drm, fmt, ...)		\
> +({									\
> +	static DEFINE_RATELIMIT_STATE(rs_, DEFAULT_RATELIMIT_INTERVAL,	\
> +				      DEFAULT_RATELIMIT_BURST);		\
> +									\
> +	if (drm_debug_enabled(DRM_UT_ ## category) && __ratelimit(&rs_))\
> +		drm_dev_printk(__get_dev_ptr(drm), KERN_DEBUG,		\
> +			       fmt, ## __VA_ARGS__);			\
>  })
>  
>  #define drm_dbg_kms_ratelimited(drm, fmt, ...) \
  

On Mon, 27 Feb 2023 15:13:21 +0530, Jani Nikula wrote:
> First of all, that's two distinct changes in one patch. The subject says
> one thing, but it's really two.

Sorry, my bad.

> But the main question is, do we *really* want to let callers pass either
> struct drm_device * or struct device *? It will be type safe with
> generics, but if it's okay to use either, people *will* use either. The
> call sites will end up being a mixture of both. You can't control it. It
> will be very tedious if you ever want to revert that decision.
> 
> Do we want to promote a style where you can pass either? To me, in C
> context, it seems awfully sloppy and confusing rather than convenient.
> 
> I'd argue the struct mipi_dsi_host stuff should use dev_* calls
> directly, as it's more of a special case, rather than allow struct
> device * in drm_* logging macros.

I agree. I thought direct dev_* calls would not be ideal, as there is a
TODO to move away from that, and also incorrectly expected to have more
such dev ptr problems. But on a second thought, you are correct.

Should I post a new patch, with using __drm_dev_ptr instead and
removing the __get_dev_ptr generic macro, and using dev_* in
drm_mipi_dsi.c as `dev_err(dev, "*ERROR* [drm] <msg>", ...);`?

> BR,
> Jani.

Thanks,
Siddh
  

On Mon, 27 Feb 2023, Siddh Raman Pant <code@siddh.me> wrote:
> On Mon, 27 Feb 2023 15:13:21 +0530, Jani Nikula wrote:
>> First of all, that's two distinct changes in one patch. The subject says
>> one thing, but it's really two.
>
> Sorry, my bad.
>
>> But the main question is, do we *really* want to let callers pass either
>> struct drm_device * or struct device *? It will be type safe with
>> generics, but if it's okay to use either, people *will* use either. The
>> call sites will end up being a mixture of both. You can't control it. It
>> will be very tedious if you ever want to revert that decision.
>> 
>> Do we want to promote a style where you can pass either? To me, in C
>> context, it seems awfully sloppy and confusing rather than convenient.
>> 
>> I'd argue the struct mipi_dsi_host stuff should use dev_* calls
>> directly, as it's more of a special case, rather than allow struct
>> device * in drm_* logging macros.
>
> I agree. I thought direct dev_* calls would not be ideal, as there is a
> TODO to move away from that, and also incorrectly expected to have more
> such dev ptr problems. But on a second thought, you are correct.
>
> Should I post a new patch, with using __drm_dev_ptr instead and
> removing the __get_dev_ptr generic macro, and using dev_* in
> drm_mipi_dsi.c as `dev_err(dev, "*ERROR* [drm] <msg>", ...);`?

I think commit 1040e424353f ("drm: mipi-dsi: Convert logging to drm_*
functions.") and any similar ones should just be reverted. It worked by
accident. You're supposed to pass struct drm_device * to the drm_*
logging functions, and that passes struct mipi_dsi_host *.

BR,
Jani.



>
>> BR,
>> Jani.
>
> Thanks,
> Siddh