On Mon, 2012-07-23 at 09:07 -0400, Jason Baron wrote: > On Thu, Jul 19, 2012 at 01:46:20PM -0600, Jim Cromie wrote: > > commit c4e00daaa96d3a0786f1f4fe6456281c60ef9a16 changed __dev_printk > > in a way that broke dynamic-debug's ability to control the dynamic > > prefix of dev_dbg(dev,..), but not dev_dbg(NULL,..) or pr_debug(..), > > which is why it wasnt noticed sooner. > > > > When dev==NULL, __dev_printk() just calls printk(), which just works. > > But otherwise, it assumed that level was always a string like "<L>" > > and just plucked out the 'L', ignoring the rest. However, > > dynamic_emit_prefix() adds "[tid] module:func:line:" to the string, > > those additions all got lost. > >
I think this is not a really good approach. 3 depths of %pV can consume quite a lot of stack and avoiding this is useful. I'd much rather improve/centralize the mechanism in dynamic_debug.c so that the extra recursion depth is avoided. Something like this: o Remove KERN_DEBUG from dynamic_emit_prefix o Make a function of create_syslog_header to format the header for printk_emit. o Call printk_emit in dynamic_dev_dbg and dynamic_netdev_dbg o Call printk_emit in __dev_printk o Remove now unused EXPORT_SYMBOL(__netdev_printk) o Neatening Thoughts? --- drivers/base/core.c | 55 +++++++++++++++++++++++++---------------- include/linux/device.h | 6 +++- lib/dynamic_debug.c | 63 ++++++++++++++++++++++++++++++++++++++--------- net/core/dev.c | 5 ++- 4 files changed, 91 insertions(+), 38 deletions(-) diff --git a/drivers/base/core.c b/drivers/base/core.c index 346be8b..94f3803 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -1844,25 +1844,19 @@ void device_shutdown(void) */ #ifdef CONFIG_PRINTK -int __dev_printk(const char *level, const struct device *dev, - struct va_format *vaf) +int create_syslog_header(const struct device *dev, char *hdr, size_t hdrlen) { - char dict[128]; - size_t dictlen = 0; const char *subsys; - - if (!dev) - return printk("%s(NULL device *): %pV", level, vaf); + size_t pos = 0; if (dev->class) subsys = dev->class->name; else if (dev->bus) subsys = dev->bus->name; else - goto skip; + return 0; - dictlen += snprintf(dict + dictlen, sizeof(dict) - dictlen, - "SUBSYSTEM=%s", subsys); + pos += snprintf(hdr + pos, hdrlen - pos, "SUBSYSTEM=%s", subsys); /* * Add device identifier DEVICE=: @@ -1878,24 +1872,39 @@ int __dev_printk(const char *level, const struct device *dev, c = 'b'; else c = 'c'; - dictlen++; - dictlen += snprintf(dict + dictlen, sizeof(dict) - dictlen, - "DEVICE=%c%u:%u", - c, MAJOR(dev->devt), MINOR(dev->devt)); + pos++; + pos += snprintf(hdr + pos, hdrlen - pos, + "DEVICE=%c%u:%u", + c, MAJOR(dev->devt), MINOR(dev->devt)); } else if (strcmp(subsys, "net") == 0) { struct net_device *net = to_net_dev(dev); - dictlen++; - dictlen += snprintf(dict + dictlen, sizeof(dict) - dictlen, - "DEVICE=n%u", net->ifindex); + pos++; + pos += snprintf(hdr + pos, hdrlen - pos, + "DEVICE=n%u", net->ifindex); } else { - dictlen++; - dictlen += snprintf(dict + dictlen, sizeof(dict) - dictlen, - "DEVICE=+%s:%s", subsys, dev_name(dev)); + pos++; + pos += snprintf(hdr + pos, hdrlen - pos, + "DEVICE=+%s:%s", subsys, dev_name(dev)); } -skip: + + return pos; +} +EXPORT_SYMBOL(create_syslog_header); + +int __dev_printk(const char *level, const struct device *dev, + struct va_format *vaf) +{ + char hdr[128]; + size_t hdrlen; + + if (!dev) + return printk("%s(NULL device *): %pV", level, vaf); + + hdrlen = create_syslog_header(dev, hdr, sizeof(hdr)); + return printk_emit(0, level[1] - '0', - dictlen ? dict : NULL, dictlen, + hdrlen ? hdr : NULL, hdrlen, "%s %s: %pV", dev_driver_string(dev), dev_name(dev), vaf); } @@ -1914,6 +1923,7 @@ int dev_printk(const char *level, const struct device *dev, vaf.va = &args; r = __dev_printk(level, dev, &vaf); + va_end(args); return r; @@ -1933,6 +1943,7 @@ int func(const struct device *dev, const char *fmt, ...) \ vaf.va = &args; \ \ r = __dev_printk(kern_level, dev, &vaf); \ + \ va_end(args); \ \ return r; \ diff --git a/include/linux/device.h b/include/linux/device.h index 6de9415..74863f0 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -884,12 +884,14 @@ extern const char *dev_driver_string(const struct device *dev); #ifdef CONFIG_PRINTK +extern int create_syslog_header(const struct device *dev, + char *hdr, size_t hdrlen); + extern int __dev_printk(const char *level, const struct device *dev, struct va_format *vaf); extern __printf(3, 4) int dev_printk(const char *level, const struct device *dev, - const char *fmt, ...) - ; + const char *fmt, ...); extern __printf(2, 3) int dev_emerg(const struct device *dev, const char *fmt, ...); extern __printf(2, 3) diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c index 7ca29a0..08442e7 100644 --- a/lib/dynamic_debug.c +++ b/lib/dynamic_debug.c @@ -521,25 +521,23 @@ static char *dynamic_emit_prefix(const struct _ddebug *desc, char *buf) int pos_after_tid; int pos = 0; - pos += snprintf(buf + pos, remaining(pos), "%s", KERN_DEBUG); if (desc->flags & _DPRINTK_FLAGS_INCL_TID) { if (in_interrupt()) - pos += snprintf(buf + pos, remaining(pos), "%s ", - "<intr>"); + pos += snprintf(buf + pos, remaining(pos), "<intr> "); else pos += snprintf(buf + pos, remaining(pos), "[%d] ", - task_pid_vnr(current)); + task_pid_vnr(current)); } pos_after_tid = pos; if (desc->flags & _DPRINTK_FLAGS_INCL_MODNAME) pos += snprintf(buf + pos, remaining(pos), "%s:", - desc->modname); + desc->modname); if (desc->flags & _DPRINTK_FLAGS_INCL_FUNCNAME) pos += snprintf(buf + pos, remaining(pos), "%s:", - desc->function); + desc->function); if (desc->flags & _DPRINTK_FLAGS_INCL_LINENO) pos += snprintf(buf + pos, remaining(pos), "%d:", - desc->lineno); + desc->lineno); if (pos - pos_after_tid) pos += snprintf(buf + pos, remaining(pos), " "); if (pos >= PREFIX_SIZE) @@ -559,9 +557,13 @@ int __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...) BUG_ON(!fmt); va_start(args, fmt); + vaf.fmt = fmt; vaf.va = &args; - res = printk("%s%pV", dynamic_emit_prefix(descriptor, buf), &vaf); + + res = printk(KERN_DEBUG "%s%pV", + dynamic_emit_prefix(descriptor, buf), &vaf); + va_end(args); return res; @@ -580,9 +582,25 @@ int __dynamic_dev_dbg(struct _ddebug *descriptor, BUG_ON(!fmt); va_start(args, fmt); + vaf.fmt = fmt; vaf.va = &args; - res = __dev_printk(dynamic_emit_prefix(descriptor, buf), dev, &vaf); + + if (!dev) { + res = printk(KERN_DEBUG "(NULL device *): %pV", &vaf); + } else { + char dict[128]; + size_t dictlen; + + dictlen = create_syslog_header(dev, dict, sizeof(dict)); + + res = printk_emit(0, 7, + dictlen ? dict : NULL, dictlen, + "%s%s %s: %pV", + dynamic_emit_prefix(descriptor, buf), + dev_driver_string(dev), dev_name(dev), &vaf); + } + va_end(args); return res; @@ -592,20 +610,41 @@ EXPORT_SYMBOL(__dynamic_dev_dbg); #ifdef CONFIG_NET int __dynamic_netdev_dbg(struct _ddebug *descriptor, - const struct net_device *dev, const char *fmt, ...) + const struct net_device *dev, const char *fmt, ...) { struct va_format vaf; va_list args; int res; - char buf[PREFIX_SIZE]; BUG_ON(!descriptor); BUG_ON(!fmt); va_start(args, fmt); + vaf.fmt = fmt; vaf.va = &args; - res = __netdev_printk(dynamic_emit_prefix(descriptor, buf), dev, &vaf); + + if (dev && dev->dev.parent) { + char buf[PREFIX_SIZE]; + char dict[128]; + size_t dictlen; + + dictlen = create_syslog_header(dev->dev.parent, + dict, sizeof(dict)); + + res = printk_emit(0, 7, + dictlen ? dict : NULL, dictlen, + "%s%s %s: %pV", + dynamic_emit_prefix(descriptor, buf), + dev_driver_string(dev->dev.parent), + netdev_name(dev), &vaf); + + } else if (dev) { + res = printk(KERN_DEBUG "%s: %pV", netdev_name(dev), &vaf); + } else { + res = printk(KERN_DEBUG "(NULL net_device): %pV", &vaf); + } + va_end(args); return res; diff --git a/net/core/dev.c b/net/core/dev.c index 1cb0d8a..1651065 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -6324,7 +6324,7 @@ const char *netdev_drivername(const struct net_device *dev) return empty; } -int __netdev_printk(const char *level, const struct net_device *dev, +static int __netdev_printk(const char *level, const struct net_device *dev, struct va_format *vaf) { int r; @@ -6339,7 +6339,6 @@ int __netdev_printk(const char *level, const struct net_device *dev, return r; } -EXPORT_SYMBOL(__netdev_printk); int netdev_printk(const char *level, const struct net_device *dev, const char *format, ...) @@ -6354,6 +6353,7 @@ int netdev_printk(const char *level, const struct net_device *dev, vaf.va = &args; r = __netdev_printk(level, dev, &vaf); + va_end(args); return r; @@ -6373,6 +6373,7 @@ int func(const struct net_device *dev, const char *fmt, ...) \ vaf.va = &args; \ \ r = __netdev_printk(level, dev, &vaf); \ + \ va_end(args); \ \ return r; \ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/