hi Jason ( Kay, I previously mis-addressed you at vrfy.com, so you havent received the thread directly until now. sorry about that)
On Fri, Jul 20, 2012 at 2:38 PM, Jason Baron <[email protected]> wrote: > On Thu, Jul 19, 2012 at 01:46:19PM -0600, Jim Cromie wrote: >> 3 patches here, 1st is bugfix, others are trivial. >> >> 1- fix __dev_printk, which broke dev_dbg() prefix under CONFIG_DYNAMIC_DEBUG. >> > > Patch looks good, and would be really nice to get into 3.5. Kay, are you > ok with this patch? > >> 2- change dyndbg prefix interfield separator from ':' to '.' >> >> for example (output from test-code, not submitted): >> r8169 0000:02:00.0: r8169.rtl_init_one: set-drvdata pdev:ffff880223041000 >> dev:ffff880220d6a000 >> hwmon hwmon1: k10temp.k10temp_probe.180: set-drvdata pdev:ffff88022303d000 >> dev:ffff8801dfd2a000 >> >> This improves usability of cut -d: <logfile> for pr_debug() messages, >> as field position is less volatile with various uses of dyndbg flags. >> Its not perfect: >> - dev_dbg on net-devices adds several more colons, >> but this doesnt vary with dyndbg flags. >> - dyndbg=+pfmlt still adds a field vs dyndbg==p (ie no prefix) >> - pr_fmt() commonly adds another colon (unchanged with this patch) > > As you suggest in the patch, changing the delimiter to a non-colon > character such as ',' would resolve these cases? yes mostly - depending upon what "these" are. Changing the prefix delimiter (I chose dot since its used in nested names everywhere) definitely improves the situation, with this patch the field-count variation is 0 or 1 for dyndbg==p or dyndbg==p[fmlt]+ respectively It obviously doesnt address colons which may be added elsewhere; pr_fmt() KBUILD_MODNAME ":" __func__ adds another, and dev_printk adds several, depending on the device type Its reasonable to change all the pr_fmt() delimiters to dot also, but thats a lot of touches to a lot of modules (ie churn), and probably isnt worth doing unless its done fully, which would need a solid consensus that its worth the trouble. changing delimiters inside dev_printk() / dev_driver_string() is harder; it means changing values in low level fields (dev->bus ? dev->bus->name : (dev->class ? dev->class->name : "")); which are also exposed to user-space, forex by lspci etc ]$ lspci | cut -c 1-20 00:00.0 Host bridge: 00:07.0 PCI bridge: 00:11.0 SATA control 00:12.0 USB Controll 00:14.0 SMBus: ATI T 00:14.1 IDE interfac 00:14.2 Audio device 00:14.3 ISA bridge: 00:14.4 PCI bridge: That looks like a can of worms best left closed. So this patch only addresses dyndbg prefix, and is 95% resolved. the remaining 0-1 variation isnt really important - almost all dyndbg users will want some prefix - at least module, so the missing field in dyndbg==p is pretty inconsequential, and its typically provided by pr_fmt() anyway. In any case, I think ths patch does what is practical now, and no more. If you agree, the patch needs your Ackd-by before Greg will take it. Going forward, the proper priorities for dyndbg are IMO (CMIIW) - get jumplabels working Ive coded this, and it looks ok, but Im stuck on the circular include problem seen when I add #include <linux/jumplabel.h> needed for the ddebug.key field. Ive tried a few permutations, but I dont think thats gonna illuminate a working fix. I think I'll post a minimal RF-Help patch to demonstrate the problem .. - propose a pr_debug_flags("flagchars", "fmt", ...) "flagchars" would be uppercase chars defined for each module DBG_DEFINE_FLAG( mod_debug_str, "A", "alpha flag controls .. blah blah.."); I think this could be made to work for both dyndbg and non-dyndbg: $ echo A > /sys/module/modname/parameters/dbg_str $ echo module modname +pA > /dbg/dynamic_debug/control This needs more thought, and perhaps simplification, but it seems appropriate to see if dyndbg and non-dyndbg can be made to fit together cogently from a user perspective. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
