| Message ID | 01f976d5ab93c985756fc1b2e83656fb0a2a28c8.1474856262.git.joe@perches.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Sep 25, 2016 at 07:18:34PM -0700, Joe Perches wrote: > Remove function name and special " *ERROR*" from argument list > > $ size drivers/gpu/drm/built-in.o* (x86-32 defconfig, most drm selected) > text data bss dec hex filename > 5635366 182579 14328 5832273 58fe51 drivers/gpu/drm/built-in.o.new > 5779552 182579 14328 5976459 5b318b drivers/gpu/drm/built-in.o.old > > Using "%ps", __builtin_return_address(0) is the same as "%s", __func__ > except for static inlines, but it's more or less the same output. And for static inlines we more often want to know which of the many callers triggered the error. We would have to play it by ear to see which error messages need to be improved in cases where it not obvious without the context of the static inline function name. Lgtm, reducing the call size for DRM_DEBUG is a definite improvement. Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > Miscellanea: > > o Convert args... to ##__VA_ARGS__ > o The equivalent DRM_DEV_<FOO> macros are rarely used and not > worth conversion Today. I would rather see us to migrate to DRM_DEV (i.e. dev_printk) so that the drm subsystem is consistent with the rest of the kernel and providing the necessary information for syslog filtering. -Chris
On Mon, Sep 26, 2016 at 4:36 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Sun, Sep 25, 2016 at 07:18:34PM -0700, Joe Perches wrote: >> Remove function name and special " *ERROR*" from argument list >> >> $ size drivers/gpu/drm/built-in.o* (x86-32 defconfig, most drm selected) >> text data bss dec hex filename >> 5635366 182579 14328 5832273 58fe51 drivers/gpu/drm/built-in.o.new >> 5779552 182579 14328 5976459 5b318b drivers/gpu/drm/built-in.o.old >> >> Using "%ps", __builtin_return_address(0) is the same as "%s", __func__ >> except for static inlines, but it's more or less the same output. > > And for static inlines we more often want to know which of the many > callers triggered the error. We would have to play it by ear to see > which error messages need to be improved in cases where it not obvious > without the context of the static inline function name. > > Lgtm, reducing the call size for DRM_DEBUG is a definite improvement. > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > Agreed on the static inlines, hopefully we won't break anyone's log grep workflow. I will push this to -misc in a couple days if no one objects. Reviewed-by: Sean Paul <seanpaul@chromium.org> >> >> Miscellanea: >> >> o Convert args... to ##__VA_ARGS__ >> o The equivalent DRM_DEV_<FOO> macros are rarely used and not >> worth conversion > > Today. I would rather see us to migrate to DRM_DEV (i.e. dev_printk) so > that the drm subsystem is consistent with the rest of the kernel and > providing the necessary information for syslog filtering. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Sep 27, 2016 at 11:54 AM, Sean Paul <seanpaul@chromium.org> wrote: > On Mon, Sep 26, 2016 at 4:36 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> On Sun, Sep 25, 2016 at 07:18:34PM -0700, Joe Perches wrote: >>> Remove function name and special " *ERROR*" from argument list >>> >>> $ size drivers/gpu/drm/built-in.o* (x86-32 defconfig, most drm selected) >>> text data bss dec hex filename >>> 5635366 182579 14328 5832273 58fe51 drivers/gpu/drm/built-in.o.new >>> 5779552 182579 14328 5976459 5b318b drivers/gpu/drm/built-in.o.old >>> >>> Using "%ps", __builtin_return_address(0) is the same as "%s", __func__ >>> except for static inlines, but it's more or less the same output. >> >> And for static inlines we more often want to know which of the many >> callers triggered the error. We would have to play it by ear to see >> which error messages need to be improved in cases where it not obvious >> without the context of the static inline function name. >> >> Lgtm, reducing the call size for DRM_DEBUG is a definite improvement. >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> >> > > Agreed on the static inlines, hopefully we won't break anyone's log > grep workflow. > > I will push this to -misc in a couple days if no one objects. Applied, thanks! Sean > > Reviewed-by: Sean Paul <seanpaul@chromium.org> > >>> >>> Miscellanea: >>> >>> o Convert args... to ##__VA_ARGS__ >>> o The equivalent DRM_DEV_<FOO> macros are rarely used and not >>> worth conversion >> >> Today. I would rather see us to migrate to DRM_DEV (i.e. dev_printk) so >> that the drm subsystem is consistent with the rest of the kernel and >> providing the necessary information for syslog filtering. >> -Chris >> >> -- >> Chris Wilson, Intel Open Source Technology Centre >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 80c7f25b5b74..6efdba4993fc 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -89,7 +89,6 @@ void drm_dev_printk(const struct device *dev, const char *level, EXPORT_SYMBOL(drm_dev_printk); void drm_printk(const char *level, unsigned int category, - const char *function_name, const char *prefix, const char *format, ...) { struct va_format vaf; @@ -102,7 +101,9 @@ void drm_printk(const char *level, unsigned int category, vaf.fmt = format; vaf.va = &args; - printk("%s" DRM_PRINTK_FMT, level, function_name, prefix, &vaf); + printk("%s" "[" DRM_NAME ":%ps]%s %pV", + level, __builtin_return_address(0), + strcmp(level, KERN_ERR) == 0 ? " *ERROR*" : "", &vaf); va_end(args); } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 95cd04aa9bf7..4729c543d877 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -140,9 +140,8 @@ void drm_dev_printk(const struct device *dev, const char *level, unsigned int category, const char *function_name, const char *prefix, const char *format, ...); -extern __printf(5, 6) +extern __printf(3, 4) void drm_printk(const char *level, unsigned int category, - const char *function_name, const char *prefix, const char *format, ...); /***********************************************************************/ @@ -198,8 +197,7 @@ void drm_printk(const char *level, unsigned int category, drm_dev_printk(dev, KERN_ERR, DRM_UT_NONE, __func__, " *ERROR*",\ fmt, ##__VA_ARGS__) #define DRM_ERROR(fmt, ...) \ - drm_printk(KERN_ERR, DRM_UT_NONE, __func__, " *ERROR*", fmt, \ - ##__VA_ARGS__) + drm_printk(KERN_ERR, DRM_UT_NONE, fmt, ##__VA_ARGS__) /** * Rate limited error output. Like DRM_ERROR() but won't flood the log. @@ -241,38 +239,38 @@ void drm_printk(const char *level, unsigned int category, #define DRM_DEV_DEBUG(dev, fmt, args...) \ drm_dev_printk(dev, KERN_DEBUG, DRM_UT_CORE, __func__, "", fmt, \ ##args) -#define DRM_DEBUG(fmt, args...) \ - drm_printk(KERN_DEBUG, DRM_UT_CORE, __func__, "", fmt, ##args) +#define DRM_DEBUG(fmt, ...) \ + drm_printk(KERN_DEBUG, DRM_UT_CORE, fmt, ##__VA_ARGS__) #define DRM_DEV_DEBUG_DRIVER(dev, fmt, args...) \ drm_dev_printk(dev, KERN_DEBUG, DRM_UT_DRIVER, __func__, "", \ fmt, ##args) -#define DRM_DEBUG_DRIVER(fmt, args...) \ - drm_printk(KERN_DEBUG, DRM_UT_DRIVER, __func__, "", fmt, ##args) +#define DRM_DEBUG_DRIVER(fmt, ...) \ + drm_printk(KERN_DEBUG, DRM_UT_DRIVER, fmt, ##__VA_ARGS__) #define DRM_DEV_DEBUG_KMS(dev, fmt, args...) \ drm_dev_printk(dev, KERN_DEBUG, DRM_UT_KMS, __func__, "", fmt, \ ##args) -#define DRM_DEBUG_KMS(fmt, args...) \ - drm_printk(KERN_DEBUG, DRM_UT_KMS, __func__, "", fmt, ##args) +#define DRM_DEBUG_KMS(fmt, ...) \ + drm_printk(KERN_DEBUG, DRM_UT_KMS, fmt, ##__VA_ARGS__) #define DRM_DEV_DEBUG_PRIME(dev, fmt, args...) \ drm_dev_printk(dev, KERN_DEBUG, DRM_UT_PRIME, __func__, "", \ fmt, ##args) -#define DRM_DEBUG_PRIME(fmt, args...) \ - drm_printk(KERN_DEBUG, DRM_UT_PRIME, __func__, "", fmt, ##args) +#define DRM_DEBUG_PRIME(fmt, ...) \ + drm_printk(KERN_DEBUG, DRM_UT_PRIME, fmt, ##__VA_ARGS__) #define DRM_DEV_DEBUG_ATOMIC(dev, fmt, args...) \ drm_dev_printk(dev, KERN_DEBUG, DRM_UT_ATOMIC, __func__, "", \ fmt, ##args) -#define DRM_DEBUG_ATOMIC(fmt, args...) \ - drm_printk(KERN_DEBUG, DRM_UT_ATOMIC, __func__, "", fmt, ##args) +#define DRM_DEBUG_ATOMIC(fmt, ...) \ + drm_printk(KERN_DEBUG, DRM_UT_ATOMIC, fmt, ##__VA_ARGS__) #define DRM_DEV_DEBUG_VBL(dev, fmt, args...) \ drm_dev_printk(dev, KERN_DEBUG, DRM_UT_VBL, __func__, "", fmt, \ ##args) -#define DRM_DEBUG_VBL(fmt, args...) \ - drm_printk(KERN_DEBUG, DRM_UT_VBL, __func__, "", fmt, ##args) +#define DRM_DEBUG_VBL(fmt, ...) \ + drm_printk(KERN_DEBUG, DRM_UT_VBL, fmt, ##__VA_ARGS__) #define _DRM_DEV_DEFINE_DEBUG_RATELIMITED(dev, level, fmt, args...) \ ({ \
Remove function name and special " *ERROR*" from argument list $ size drivers/gpu/drm/built-in.o* (x86-32 defconfig, most drm selected) text data bss dec hex filename 5635366 182579 14328 5832273 58fe51 drivers/gpu/drm/built-in.o.new 5779552 182579 14328 5976459 5b318b drivers/gpu/drm/built-in.o.old Using "%ps", __builtin_return_address(0) is the same as "%s", __func__ except for static inlines, but it's more or less the same output. Miscellanea: o Convert args... to ##__VA_ARGS__ o The equivalent DRM_DEV_<FOO> macros are rarely used and not worth conversion Signed-off-by: Joe Perches <joe@perches.com> --- drivers/gpu/drm/drm_drv.c | 5 +++-- include/drm/drmP.h | 30 ++++++++++++++---------------- 2 files changed, 17 insertions(+), 18 deletions(-)