[PATCH 0/3] dynamic_hex_dump cleanup

[PATCH 0/3] dynamic_hex_dump cleanup

[Dan Williams]

More than one driver has worked around the fact that
print_hex_dump_debug() requires CONFIG_DYNAMIC_DEBUG=y to build. Provide
a dynamic_hex_dump() so that drivers that want the extra debugging to be
turned off in the CONFIG_DYNAMIC_DEBUG=n can use dynamic_hex_dump()
directly.

I'm fine to carry this whole set through nvdimm.git if no one sees any
issues.

---

Dan Williams (3):
      printk: provide generic dynamic_hex_dump fallback
      virtio, rpmsg: switch to dynamic_hex_dump()
      acpi, nfit: kill ACPI_NFIT_DEBUG, switch to dynamic_hex_dump()


 drivers/acpi/nfit/Kconfig        |   12 ------------
 drivers/acpi/nfit/core.c         |   21 ++++++++-------------
 drivers/rpmsg/virtio_rpmsg_bus.c |    6 ------
 include/linux/dynamic_debug.h    |    6 ++++++
 4 files changed, 14 insertions(+), 31 deletions(-)

[PATCH 1/3] printk: provide generic dynamic_hex_dump fallback

[Dan Williams]

The lack of a silent fallback for this routine has caused the nfit
driver and the virtio_rpmsg_bus to develop local workarounds. Define a
nop version of dynamic_hex_dump() in the CONFIG_DYNAMIC_DEBUG=n case so
we can clean up those local workarounds.

Cc: Jason Baron <jbaron-JqFfY2XvxFXQT0dZR+AlfA@public.gmane.org>
Cc: Ohad Ben-Cohen <ohad-Ix1uc/W3ht7QT0dZR+AlfA@public.gmane.org>
Cc: Bjorn Andersson <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 include/linux/dynamic_debug.h |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 546d68057e3b..c26302e1e387 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -180,6 +180,12 @@ static inline int ddebug_dyndbg_module_param_cb(char *param, char *val,
 	do { if (0) printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); } while (0)
 #define dynamic_dev_dbg(dev, fmt, ...)					\
 	do { if (0) dev_printk(KERN_DEBUG, dev, fmt, ##__VA_ARGS__); } while (0)
+
+static inline void dynamic_hex_dump(const char *prefix_str, int prefix_type,
+		int rowsize, int groupsize, const void *buf, size_t len,
+		bool ascii)
+{
+}
 #endif
 
 #endif

[PATCH 2/3] virtio, rpmsg: switch to dynamic_hex_dump()

[Dan Williams]

Now that dynamic_hex_dump() has a fallback in the CONFIG_DYNAMIC_DEBUG=n
case we can kill the unnecessary ifdefs in the rpmsg driver.

Cc: Ohad Ben-Cohen <ohad-Ix1uc/W3ht7QT0dZR+AlfA@public.gmane.org>
Cc: Bjorn Andersson <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/rpmsg/virtio_rpmsg_bus.c |    6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 5e66e081027e..00d92b399ddd 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -599,10 +599,8 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
 
 	dev_dbg(dev, "TX From 0x%x, To 0x%x, Len %d, Flags %d, Reserved %d\n",
 		msg->src, msg->dst, msg->len, msg->flags, msg->reserved);
-#if defined(CONFIG_DYNAMIC_DEBUG)
 	dynamic_hex_dump("rpmsg_virtio TX: ", DUMP_PREFIX_NONE, 16, 1,
 			 msg, sizeof(*msg) + msg->len, true);
-#endif
 
 	sg_init_one(&sg, msg, sizeof(*msg) + len);
 
@@ -687,10 +685,8 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
 
 	dev_dbg(dev, "From: 0x%x, To: 0x%x, Len: %d, Flags: %d, Reserved: %d\n",
 		msg->src, msg->dst, msg->len, msg->flags, msg->reserved);
-#if defined(CONFIG_DYNAMIC_DEBUG)
 	dynamic_hex_dump("rpmsg_virtio RX: ", DUMP_PREFIX_NONE, 16, 1,
 			 msg, sizeof(*msg) + msg->len, true);
-#endif
 
 	/*
 	 * We currently use fixed-sized buffers, so trivially sanitize
@@ -801,10 +797,8 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
 	struct device *dev = &vrp->vdev->dev;
 	int ret;
 
-#if defined(CONFIG_DYNAMIC_DEBUG)
 	dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16, 1,
 			 data, len, true);
-#endif
 
 	if (len != sizeof(*msg)) {
 		dev_err(dev, "malformed ns msg (%d)\n", len);

Re: [PATCH 2/3] virtio, rpmsg: switch to dynamic_hex_dump()

[kbuild test robot]

Hi Dan,

[auto build test ERROR on pm/linux-next]
[also build test ERROR on v4.11-rc8 next-20170428]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Dan-Williams/dynamic_hex_dump-cleanup/20170429-135503
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: arm-multi_v7_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers//rpmsg/virtio_rpmsg_bus.c: In function 'rpmsg_send_offchannel_raw':
>> drivers//rpmsg/virtio_rpmsg_bus.c:602:2: error: implicit declaration of function 'dynamic_hex_dump' [-Werror=implicit-function-declaration]
     dynamic_hex_dump("rpmsg_virtio TX: ", DUMP_PREFIX_NONE, 16, 1,
     ^~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/dynamic_hex_dump +602 drivers//rpmsg/virtio_rpmsg_bus.c

bcabbcca Ohad Ben-Cohen 2011-10-20  596  	msg->dst = dst;
bcabbcca Ohad Ben-Cohen 2011-10-20  597  	msg->reserved = 0;
bcabbcca Ohad Ben-Cohen 2011-10-20  598  	memcpy(msg->data, data, len);
bcabbcca Ohad Ben-Cohen 2011-10-20  599  
bcabbcca Ohad Ben-Cohen 2011-10-20  600  	dev_dbg(dev, "TX From 0x%x, To 0x%x, Len %d, Flags %d, Reserved %d\n",
0963679c Anna, Suman    2016-08-12  601  		msg->src, msg->dst, msg->len, msg->flags, msg->reserved);
211e3a93 Anna, Suman    2016-08-12 @602  	dynamic_hex_dump("rpmsg_virtio TX: ", DUMP_PREFIX_NONE, 16, 1,
bcabbcca Ohad Ben-Cohen 2011-10-20  603  			 msg, sizeof(*msg) + msg->len, true);
bcabbcca Ohad Ben-Cohen 2011-10-20  604  
bcabbcca Ohad Ben-Cohen 2011-10-20  605  	sg_init_one(&sg, msg, sizeof(*msg) + len);

:::::: The code at line 602 was first introduced by commit
:::::: 211e3a93e5b5933e64ddfb299eee462ac7c7d500 rpmsg: use dynamic_hex_dump for hex dump traces

:::::: TO: Anna, Suman <s-anna-l0cyMroinI0@public.gmane.org>
:::::: CC: Bjorn Andersson <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[PATCH 3/3] acpi, nfit: kill ACPI_NFIT_DEBUG, switch to dynamic_hex_dump()

[Dan Williams]

Inevitably when one actually needs to debug a DSM issue it's on a
distribution kernel that has CONFIG_ACPI_NFIT_DEBUG=n. The config symbol
is only there to avoid the compile error due to the missing fallback for
print_hex_dump_debug in the CONFIG_DYNAMIC_DEBUG=n case. Now that the
compile error is fixed the nfit driver can call dynamic_hex_dump()
directly, and enable debug of unmodified distro kernels.

Signed-off-by: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/acpi/nfit/Kconfig |   12 ------------
 drivers/acpi/nfit/core.c  |   21 ++++++++-------------
 2 files changed, 8 insertions(+), 25 deletions(-)

diff --git a/drivers/acpi/nfit/Kconfig b/drivers/acpi/nfit/Kconfig
index dd0d53c52552..6d3351452ea2 100644
--- a/drivers/acpi/nfit/Kconfig
+++ b/drivers/acpi/nfit/Kconfig
@@ -12,15 +12,3 @@ config ACPI_NFIT
 
 	  To compile this driver as a module, choose M here:
 	  the module will be called nfit.
-
-config ACPI_NFIT_DEBUG
-	bool "NFIT DSM debug"
-	depends on ACPI_NFIT
-	depends on DYNAMIC_DEBUG
-	default n
-	help
-	  Enabling this option causes the nfit driver to dump the
-	  input and output buffers of _DSM operations on the ACPI0012
-	  device and its children.  This can be very verbose, so leave
-	  it disabled unless you are debugging a hardware / firmware
-	  issue.
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 261eea1d2906..ca391a2cbe63 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -268,14 +268,11 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
 		in_buf.buffer.length = call_pkg->nd_size_in;
 	}
 
-	if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG)) {
-		dev_dbg(dev, "%s:%s cmd: %d: func: %d input length: %d\n",
-				__func__, dimm_name, cmd, func,
-				in_buf.buffer.length);
-		print_hex_dump_debug("nvdimm in  ", DUMP_PREFIX_OFFSET, 4, 4,
+	dev_dbg(dev, "%s:%s cmd: %d: func: %d input length: %d\n",
+			__func__, dimm_name, cmd, func, in_buf.buffer.length);
+	dynamic_hex_dump("nvdimm in  ", DUMP_PREFIX_OFFSET, 4, 4,
 			in_buf.buffer.pointer,
 			min_t(u32, 256, in_buf.buffer.length), true);
-	}
 
 	out_obj = acpi_evaluate_dsm(handle, uuid, 1, func, &in_obj);
 	if (!out_obj) {
@@ -307,13 +304,11 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
 		goto out;
 	}
 
-	if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG)) {
-		dev_dbg(dev, "%s:%s cmd: %s output length: %d\n", __func__,
-				dimm_name, cmd_name, out_obj->buffer.length);
-		print_hex_dump_debug(cmd_name, DUMP_PREFIX_OFFSET, 4,
-				4, out_obj->buffer.pointer, min_t(u32, 128,
-					out_obj->buffer.length), true);
-	}
+	dev_dbg(dev, "%s:%s cmd: %s output length: %d\n", __func__, dimm_name,
+			cmd_name, out_obj->buffer.length);
+	dynamic_hex_dump(cmd_name, DUMP_PREFIX_OFFSET, 4, 4,
+			out_obj->buffer.pointer,
+			min_t(u32, 128, out_obj->buffer.length), true);
 
 	for (i = 0, offset = 0; i < desc->out_num; i++) {
 		u32 out_size = nd_cmd_out_size(nvdimm, cmd, desc, i, buf,

[PATCH v2] acpi, nfit: kill ACPI_NFIT_DEBUG

[Dan Williams]

Inevitably when one actually needs to debug a DSM issue it's on a
distribution kernel that has CONFIG_ACPI_NFIT_DEBUG=n. The config symbol
was only there to avoid the compile error due to the missing fallback for
print_hex_dump_debug in the CONFIG_DYNAMIC_DEBUG=n case. That was fixed
with commit cdf17449af1d "hexdump: do not print debug dumps for
!CONFIG_DEBUG", so the config symbol can just be dropped.

Cc: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
Changes in v2:
* Thanks to Joe for pointing out the misunderstanding about
  print_hex_dump_debug() compilation being broken in the
  CONFIG_DYNAMIC_DEBUG=n case. Since commit cdf17449af1d we can rely on
  print_hex_dump_debug() to do the right thing by default.

 drivers/acpi/nfit/Kconfig |   12 ------------
 drivers/acpi/nfit/core.c  |   21 ++++++++-------------
 2 files changed, 8 insertions(+), 25 deletions(-)

diff --git a/drivers/acpi/nfit/Kconfig b/drivers/acpi/nfit/Kconfig
index dd0d53c52552..6d3351452ea2 100644
--- a/drivers/acpi/nfit/Kconfig
+++ b/drivers/acpi/nfit/Kconfig
@@ -12,15 +12,3 @@ config ACPI_NFIT
 
 	  To compile this driver as a module, choose M here:
 	  the module will be called nfit.
-
-config ACPI_NFIT_DEBUG
-	bool "NFIT DSM debug"
-	depends on ACPI_NFIT
-	depends on DYNAMIC_DEBUG
-	default n
-	help
-	  Enabling this option causes the nfit driver to dump the
-	  input and output buffers of _DSM operations on the ACPI0012
-	  device and its children.  This can be very verbose, so leave
-	  it disabled unless you are debugging a hardware / firmware
-	  issue.
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 261eea1d2906..ced95a79c1a1 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -268,14 +268,11 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
 		in_buf.buffer.length = call_pkg->nd_size_in;
 	}
 
-	if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG)) {
-		dev_dbg(dev, "%s:%s cmd: %d: func: %d input length: %d\n",
-				__func__, dimm_name, cmd, func,
-				in_buf.buffer.length);
-		print_hex_dump_debug("nvdimm in  ", DUMP_PREFIX_OFFSET, 4, 4,
+	dev_dbg(dev, "%s:%s cmd: %d: func: %d input length: %d\n",
+			__func__, dimm_name, cmd, func, in_buf.buffer.length);
+	print_hex_dump_debug("nvdimm in  ", DUMP_PREFIX_OFFSET, 4, 4,
 			in_buf.buffer.pointer,
 			min_t(u32, 256, in_buf.buffer.length), true);
-	}
 
 	out_obj = acpi_evaluate_dsm(handle, uuid, 1, func, &in_obj);
 	if (!out_obj) {
@@ -307,13 +304,11 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
 		goto out;
 	}
 
-	if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG)) {
-		dev_dbg(dev, "%s:%s cmd: %s output length: %d\n", __func__,
-				dimm_name, cmd_name, out_obj->buffer.length);
-		print_hex_dump_debug(cmd_name, DUMP_PREFIX_OFFSET, 4,
-				4, out_obj->buffer.pointer, min_t(u32, 128,
-					out_obj->buffer.length), true);
-	}
+	dev_dbg(dev, "%s:%s cmd: %s output length: %d\n", __func__, dimm_name,
+			cmd_name, out_obj->buffer.length);
+	print_hex_dump_debug(cmd_name, DUMP_PREFIX_OFFSET, 4, 4,
+			out_obj->buffer.pointer,
+			min_t(u32, 128, out_obj->buffer.length), true);
 
 	for (i = 0, offset = 0; i < desc->out_num; i++) {
 		u32 out_size = nd_cmd_out_size(nvdimm, cmd, desc, i, buf,

Re: [PATCH 0/3] dynamic_hex_dump cleanup

[Joe Perches]

On Fri, 2017-04-28 at 14:28 -0700, Dan Williams wrote:
> More than one driver has worked around the fact that
> print_hex_dump_debug() requires CONFIG_DYNAMIC_DEBUG=y to build.

No it doesn't.  builds work fine.  Output is restricted.

> Provide a dynamic_hex_dump() so that drivers that want the extra debugging to be
> turned off in the CONFIG_DYNAMIC_DEBUG=n can use dynamic_hex_dump()
> directly.

I think the concept is unnecessary
.
Just use print_hex_dump with KERN_DEBUG.

Re: [PATCH 0/3] dynamic_hex_dump cleanup

[Dan Williams]

On Fri, Apr 28, 2017 at 2:49 PM, Joe Perches <joe@perches.com> wrote:
> On Fri, 2017-04-28 at 14:28 -0700, Dan Williams wrote:
>> More than one driver has worked around the fact that
>> print_hex_dump_debug() requires CONFIG_DYNAMIC_DEBUG=y to build.
>
> No it doesn't.  builds work fine.  Output is restricted.
>
>> Provide a dynamic_hex_dump() so that drivers that want the extra debugging to be
>> turned off in the CONFIG_DYNAMIC_DEBUG=n can use dynamic_hex_dump()
>> directly.
>
> I think the concept is unnecessary
> .
> Just use print_hex_dump with KERN_DEBUG.

No, we don't want any possibility of output in the
CONFIG_DYNAMIC_DEBUG=n case. This is extra debug that only makes sense
in the CONFIG_DYNAMIC_DEBUG case.

Re: [PATCH 0/3] dynamic_hex_dump cleanup

[Joe Perches]

On Fri, 2017-04-28 at 14:52 -0700, Dan Williams wrote:
> On Fri, Apr 28, 2017 at 2:49 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> > On Fri, 2017-04-28 at 14:28 -0700, Dan Williams wrote:
> > > More than one driver has worked around the fact that
> > > print_hex_dump_debug() requires CONFIG_DYNAMIC_DEBUG=y to build.
> > 
> > No it doesn't.  builds work fine.  Output is restricted.
> > 
> > > Provide a dynamic_hex_dump() so that drivers that want the extra debugging to be
> > > turned off in the CONFIG_DYNAMIC_DEBUG=n can use dynamic_hex_dump()
> > > directly.
> > 
> > I think the concept is unnecessary
> > .
> > Just use print_hex_dump with KERN_DEBUG.
> 
> No, we don't want any possibility of output in the
> CONFIG_DYNAMIC_DEBUG=n case. This is extra debug that only makes sense
> in the CONFIG_DYNAMIC_DEBUG case.

No, that doesn't work the same.

Look at your conversion of drivers/acpi/nfit/core.c

dev_dbg outputs always when DEBUG is defined and
optionally outputs when CONFIG_DYNAMIC_DEBUG is enabled.

Re: [PATCH 0/3] dynamic_hex_dump cleanup

[Dan Williams]

On Fri, Apr 28, 2017 at 3:07 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> On Fri, 2017-04-28 at 14:52 -0700, Dan Williams wrote:
>> On Fri, Apr 28, 2017 at 2:49 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
>> > On Fri, 2017-04-28 at 14:28 -0700, Dan Williams wrote:
>> > > More than one driver has worked around the fact that
>> > > print_hex_dump_debug() requires CONFIG_DYNAMIC_DEBUG=y to build.
>> >
>> > No it doesn't.  builds work fine.  Output is restricted.
>> >
>> > > Provide a dynamic_hex_dump() so that drivers that want the extra debugging to be
>> > > turned off in the CONFIG_DYNAMIC_DEBUG=n can use dynamic_hex_dump()
>> > > directly.
>> >
>> > I think the concept is unnecessary
>> > .
>> > Just use print_hex_dump with KERN_DEBUG.
>>
>> No, we don't want any possibility of output in the
>> CONFIG_DYNAMIC_DEBUG=n case. This is extra debug that only makes sense
>> in the CONFIG_DYNAMIC_DEBUG case.
>
> No, that doesn't work the same.
>
> Look at your conversion of drivers/acpi/nfit/core.c
>
> dev_dbg outputs always when DEBUG is defined and
> optionally outputs when CONFIG_DYNAMIC_DEBUG is enabled.

Right, that's what I want dev_dbg() to output at KERN_DEBUG level
always and the hexdump only in the dynamic case.

Re: [PATCH 0/3] dynamic_hex_dump cleanup

[Dan Williams]

On Fri, Apr 28, 2017 at 3:14 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Fri, Apr 28, 2017 at 3:07 PM, Joe Perches <joe@perches.com> wrote:
>> On Fri, 2017-04-28 at 14:52 -0700, Dan Williams wrote:
>>> On Fri, Apr 28, 2017 at 2:49 PM, Joe Perches <joe@perches.com> wrote:
>>> > On Fri, 2017-04-28 at 14:28 -0700, Dan Williams wrote:
>>> > > More than one driver has worked around the fact that
>>> > > print_hex_dump_debug() requires CONFIG_DYNAMIC_DEBUG=y to build.
>>> >
>>> > No it doesn't.  builds work fine.  Output is restricted.
>>> >
>>> > > Provide a dynamic_hex_dump() so that drivers that want the extra debugging to be
>>> > > turned off in the CONFIG_DYNAMIC_DEBUG=n can use dynamic_hex_dump()
>>> > > directly.
>>> >
>>> > I think the concept is unnecessary
>>> > .
>>> > Just use print_hex_dump with KERN_DEBUG.
>>>
>>> No, we don't want any possibility of output in the
>>> CONFIG_DYNAMIC_DEBUG=n case. This is extra debug that only makes sense
>>> in the CONFIG_DYNAMIC_DEBUG case.
>>
>> No, that doesn't work the same.
>>
>> Look at your conversion of drivers/acpi/nfit/core.c
>>
>> dev_dbg outputs always when DEBUG is defined and
>> optionally outputs when CONFIG_DYNAMIC_DEBUG is enabled.
>
> Right, that's what I want dev_dbg() to output at KERN_DEBUG level
> always and the hexdump only in the dynamic case.

Joe, I'm trying to understand your objection. What you are proposing
is that the debug output is present at the KERN_DEBUG level in the
CONFIG_DYNAMIC_DEBUG=n case and that's not what either use case wants.
What breaks by letting users call dynamic_hex_dump() directly?

Re: [PATCH 0/3] dynamic_hex_dump cleanup

[Joe Perches]

On Fri, 2017-04-28 at 15:19 -0700, Dan Williams wrote:
> On Fri, Apr 28, 2017 at 3:14 PM, Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> > On Fri, Apr 28, 2017 at 3:07 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> > > On Fri, 2017-04-28 at 14:52 -0700, Dan Williams wrote:
> > > > On Fri, Apr 28, 2017 at 2:49 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> > > > > On Fri, 2017-04-28 at 14:28 -0700, Dan Williams wrote:
> > > > > > More than one driver has worked around the fact that
> > > > > > print_hex_dump_debug() requires CONFIG_DYNAMIC_DEBUG=y to build.
> > > > > 
> > > > > No it doesn't.  builds work fine.  Output is restricted.
> > > > > 
> > > > > > Provide a dynamic_hex_dump() so that drivers that want the extra debugging to be
> > > > > > turned off in the CONFIG_DYNAMIC_DEBUG=n can use dynamic_hex_dump()
> > > > > > directly.
> > > > > 
> > > > > I think the concept is unnecessary
> > > > > .
> > > > > Just use print_hex_dump with KERN_DEBUG.
> > > > 
> > > > No, we don't want any possibility of output in the
> > > > CONFIG_DYNAMIC_DEBUG=n case. This is extra debug that only makes sense
> > > > in the CONFIG_DYNAMIC_DEBUG case.
> > > 
> > > No, that doesn't work the same.
> > > 
> > > Look at your conversion of drivers/acpi/nfit/core.c
> > > 
> > > dev_dbg outputs always when DEBUG is defined and
> > > optionally outputs when CONFIG_DYNAMIC_DEBUG is enabled.
> > 
> > Right, that's what I want dev_dbg() to output at KERN_DEBUG level
> > always and the hexdump only in the dynamic case.
> 
> Joe, I'm trying to understand your objection. What you are proposing
> is that the debug output is present at the KERN_DEBUG level in the
> CONFIG_DYNAMIC_DEBUG=n case and that's not what either use case wants.
> What breaks by letting users call dynamic_hex_dump() directly?

Call it hex_dump_dbg and I have no objections.

The "dynamicism" is a side effect of not having DEBUG
defined.  dev_dbg will emit if DEBUG is declared regardless
of dynamic_debug.

This new function should have the same behavior.

Re: [PATCH 0/3] dynamic_hex_dump cleanup

[Dan Williams]

On Fri, Apr 28, 2017 at 3:31 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> On Fri, 2017-04-28 at 15:19 -0700, Dan Williams wrote:
>> On Fri, Apr 28, 2017 at 3:14 PM, Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>> > On Fri, Apr 28, 2017 at 3:07 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
>> > > On Fri, 2017-04-28 at 14:52 -0700, Dan Williams wrote:
>> > > > On Fri, Apr 28, 2017 at 2:49 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
>> > > > > On Fri, 2017-04-28 at 14:28 -0700, Dan Williams wrote:
>> > > > > > More than one driver has worked around the fact that
>> > > > > > print_hex_dump_debug() requires CONFIG_DYNAMIC_DEBUG=y to build.
>> > > > >
>> > > > > No it doesn't.  builds work fine.  Output is restricted.
>> > > > >
>> > > > > > Provide a dynamic_hex_dump() so that drivers that want the extra debugging to be
>> > > > > > turned off in the CONFIG_DYNAMIC_DEBUG=n can use dynamic_hex_dump()
>> > > > > > directly.
>> > > > >
>> > > > > I think the concept is unnecessary
>> > > > > .
>> > > > > Just use print_hex_dump with KERN_DEBUG.
>> > > >
>> > > > No, we don't want any possibility of output in the
>> > > > CONFIG_DYNAMIC_DEBUG=n case. This is extra debug that only makes sense
>> > > > in the CONFIG_DYNAMIC_DEBUG case.
>> > >
>> > > No, that doesn't work the same.
>> > >
>> > > Look at your conversion of drivers/acpi/nfit/core.c
>> > >
>> > > dev_dbg outputs always when DEBUG is defined and
>> > > optionally outputs when CONFIG_DYNAMIC_DEBUG is enabled.
>> >
>> > Right, that's what I want dev_dbg() to output at KERN_DEBUG level
>> > always and the hexdump only in the dynamic case.
>>
>> Joe, I'm trying to understand your objection. What you are proposing
>> is that the debug output is present at the KERN_DEBUG level in the
>> CONFIG_DYNAMIC_DEBUG=n case and that's not what either use case wants.
>> What breaks by letting users call dynamic_hex_dump() directly?
>
> Call it hex_dump_dbg and I have no objections.

...but again, hex_dump_dbg() implies to me dev_dbg() behavior, i.e.
that the hexdump happens in the KERN_DEBUG case which I don't want. I
want the call to be compiled out completely when it can't be
enabled/disabled dynamically.

> The "dynamicism" is a side effect of not having DEBUG
> defined.  dev_dbg will emit if DEBUG is declared regardless
> of dynamic_debug.
>
> This new function should have the same behavior.

I think we do need a hex_dump_dbg(), but that does not eliminate the
utility of / need for dynamic_hex_dump().

Re: [PATCH 0/3] dynamic_hex_dump cleanup

[Joe Perches]

On Fri, 2017-04-28 at 15:59 -0700, Dan Williams wrote:
> ...but again, hex_dump_dbg() implies to me dev_dbg() behavior, i.e.
> that the hexdump happens in the KERN_DEBUG case

I think you are confusing KERN_DEBUG, a logging level, with a
preprocessor #define of DEBUG.

>  which I don't want. I
> want the call to be compiled out completely when it can't be
> enabled/disabled dynamically.

dev_dbg is compiled away completely when DEBUG is
not defined and when CONFIG_DYNAMIC_DEBUG is not
defined.

This hex_dump facility should behave the same way.

> I think we do need a hex_dump_dbg(), but that does not eliminate the
> utility of / need for dynamic_hex_dump().

dynamic_hex_dump should only exist when CONFIG_DYNAMIC_DEBUG
is enabled and should otherwise be invisible to driver code.

Re: [PATCH 0/3] dynamic_hex_dump cleanup

[Dan Williams]

On Fri, Apr 28, 2017 at 4:04 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> On Fri, 2017-04-28 at 15:59 -0700, Dan Williams wrote:
>> ...but again, hex_dump_dbg() implies to me dev_dbg() behavior, i.e.
>> that the hexdump happens in the KERN_DEBUG case
>
> I think you are confusing KERN_DEBUG, a logging level, with a
> preprocessor #define of DEBUG.

Indeed I am.

>
>>  which I don't want. I
>> want the call to be compiled out completely when it can't be
>> enabled/disabled dynamically.
>
> dev_dbg is compiled away completely when DEBUG is
> not defined and when CONFIG_DYNAMIC_DEBUG is not
> defined.
>
> This hex_dump facility should behave the same way.
>
>> I think we do need a hex_dump_dbg(), but that does not eliminate the
>> utility of / need for dynamic_hex_dump().
>
> dynamic_hex_dump should only exist when CONFIG_DYNAMIC_DEBUG
> is enabled and should otherwise be invisible to driver code.
>

So, I overlooked that Linus fixed this compile problem a while back with:

cdf17449af1d hexdump: do not print debug dumps for !CONFIG_DEBUG

Which predated the introduction of ACPI_NFIT_DEBUG. So I can just call
print_hex_dump_debug() and call it a day.

Sorry for the noise, Joe!