* [PATCH 0/3] dynamic_hex_dump cleanup
@ 2017-04-28 21:28 Dan Williams
2017-04-28 21:28 ` [PATCH 1/3] printk: provide generic dynamic_hex_dump fallback Dan Williams
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Dan Williams @ 2017-04-28 21:28 UTC (permalink / raw)
To: linux-nvdimm
Cc: Ohad Ben-Cohen, linux-remoteproc, linux-kernel, Bjorn Andersson,
linux-acpi, Jason Baron
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(-)
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] printk: provide generic dynamic_hex_dump fallback
2017-04-28 21:28 [PATCH 0/3] dynamic_hex_dump cleanup Dan Williams
@ 2017-04-28 21:28 ` Dan Williams
2017-04-28 21:29 ` [PATCH 2/3] virtio, rpmsg: switch to dynamic_hex_dump() Dan Williams
` (2 subsequent siblings)
3 siblings, 0 replies; 15+ messages in thread
From: Dan Williams @ 2017-04-28 21:28 UTC (permalink / raw)
To: linux-nvdimm
Cc: Ohad Ben-Cohen, linux-remoteproc, linux-kernel, Bjorn Andersson,
linux-acpi, Jason Baron
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@akamai.com>
Cc: Ohad Ben-Cohen <ohad@wizery.com>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/3] virtio, rpmsg: switch to dynamic_hex_dump()
2017-04-28 21:28 [PATCH 0/3] dynamic_hex_dump cleanup Dan Williams
2017-04-28 21:28 ` [PATCH 1/3] printk: provide generic dynamic_hex_dump fallback Dan Williams
@ 2017-04-28 21:29 ` Dan Williams
2017-04-29 8:46 ` kbuild test robot
2017-04-28 21:29 ` [PATCH 3/3] acpi, nfit: kill ACPI_NFIT_DEBUG, " Dan Williams
2017-04-28 21:49 ` [PATCH 0/3] dynamic_hex_dump cleanup Joe Perches
3 siblings, 1 reply; 15+ messages in thread
From: Dan Williams @ 2017-04-28 21:29 UTC (permalink / raw)
To: linux-nvdimm
Cc: Ohad Ben-Cohen, linux-acpi, linux-remoteproc, linux-kernel,
Bjorn Andersson
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@wizery.com>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
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);
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/3] acpi, nfit: kill ACPI_NFIT_DEBUG, switch to dynamic_hex_dump()
2017-04-28 21:28 [PATCH 0/3] dynamic_hex_dump cleanup Dan Williams
2017-04-28 21:28 ` [PATCH 1/3] printk: provide generic dynamic_hex_dump fallback Dan Williams
2017-04-28 21:29 ` [PATCH 2/3] virtio, rpmsg: switch to dynamic_hex_dump() Dan Williams
@ 2017-04-28 21:29 ` Dan Williams
2017-04-28 23:48 ` [PATCH v2] acpi, nfit: kill ACPI_NFIT_DEBUG Dan Williams
2017-04-28 21:49 ` [PATCH 0/3] dynamic_hex_dump cleanup Joe Perches
3 siblings, 1 reply; 15+ messages in thread
From: Dan Williams @ 2017-04-28 21:29 UTC (permalink / raw)
To: linux-nvdimm; +Cc: linux-acpi, linux-remoteproc, linux-kernel
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@intel.com>
---
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,
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] dynamic_hex_dump cleanup
2017-04-28 21:28 [PATCH 0/3] dynamic_hex_dump cleanup Dan Williams
` (2 preceding siblings ...)
2017-04-28 21:29 ` [PATCH 3/3] acpi, nfit: kill ACPI_NFIT_DEBUG, " Dan Williams
@ 2017-04-28 21:49 ` Joe Perches
2017-04-28 21:52 ` Dan Williams
3 siblings, 1 reply; 15+ messages in thread
From: Joe Perches @ 2017-04-28 21:49 UTC (permalink / raw)
To: Dan Williams, linux-nvdimm
Cc: Ohad Ben-Cohen, linux-remoteproc, linux-kernel, Bjorn Andersson,
linux-acpi, Jason Baron
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] dynamic_hex_dump cleanup
2017-04-28 21:49 ` [PATCH 0/3] dynamic_hex_dump cleanup Joe Perches
@ 2017-04-28 21:52 ` Dan Williams
2017-04-28 22:07 ` Joe Perches
0 siblings, 1 reply; 15+ messages in thread
From: Dan Williams @ 2017-04-28 21:52 UTC (permalink / raw)
To: Joe Perches
Cc: linux-nvdimm@lists.01.org, Ohad Ben-Cohen, linux-remoteproc,
linux-kernel, Bjorn Andersson, Linux ACPI, Jason Baron
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] dynamic_hex_dump cleanup
2017-04-28 21:52 ` Dan Williams
@ 2017-04-28 22:07 ` Joe Perches
2017-04-28 22:14 ` Dan Williams
0 siblings, 1 reply; 15+ messages in thread
From: Joe Perches @ 2017-04-28 22:07 UTC (permalink / raw)
To: Dan Williams
Cc: linux-nvdimm@lists.01.org, Ohad Ben-Cohen, linux-remoteproc,
linux-kernel, Bjorn Andersson, Linux ACPI, Jason Baron
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] dynamic_hex_dump cleanup
2017-04-28 22:07 ` Joe Perches
@ 2017-04-28 22:14 ` Dan Williams
2017-04-28 22:19 ` Dan Williams
0 siblings, 1 reply; 15+ messages in thread
From: Dan Williams @ 2017-04-28 22:14 UTC (permalink / raw)
To: Joe Perches
Cc: linux-nvdimm@lists.01.org, Ohad Ben-Cohen, linux-remoteproc,
linux-kernel, Bjorn Andersson, Linux ACPI, Jason Baron
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] dynamic_hex_dump cleanup
2017-04-28 22:14 ` Dan Williams
@ 2017-04-28 22:19 ` Dan Williams
2017-04-28 22:31 ` Joe Perches
0 siblings, 1 reply; 15+ messages in thread
From: Dan Williams @ 2017-04-28 22:19 UTC (permalink / raw)
To: Joe Perches
Cc: linux-nvdimm@lists.01.org, Ohad Ben-Cohen, linux-remoteproc,
linux-kernel, Bjorn Andersson, Linux ACPI, Jason Baron
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?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] dynamic_hex_dump cleanup
2017-04-28 22:19 ` Dan Williams
@ 2017-04-28 22:31 ` Joe Perches
2017-04-28 22:59 ` Dan Williams
0 siblings, 1 reply; 15+ messages in thread
From: Joe Perches @ 2017-04-28 22:31 UTC (permalink / raw)
To: Dan Williams
Cc: linux-nvdimm@lists.01.org, Ohad Ben-Cohen, linux-remoteproc,
linux-kernel, Bjorn Andersson, Linux ACPI, Jason Baron
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@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?
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] dynamic_hex_dump cleanup
2017-04-28 22:31 ` Joe Perches
@ 2017-04-28 22:59 ` Dan Williams
2017-04-28 23:04 ` Joe Perches
0 siblings, 1 reply; 15+ messages in thread
From: Dan Williams @ 2017-04-28 22:59 UTC (permalink / raw)
To: Joe Perches
Cc: linux-nvdimm@lists.01.org, Ohad Ben-Cohen, linux-remoteproc,
linux-kernel, Bjorn Andersson, Linux ACPI, Jason Baron
On Fri, Apr 28, 2017 at 3:31 PM, Joe Perches <joe@perches.com> 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@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?
>
> 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().
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] dynamic_hex_dump cleanup
2017-04-28 22:59 ` Dan Williams
@ 2017-04-28 23:04 ` Joe Perches
2017-04-28 23:12 ` Dan Williams
0 siblings, 1 reply; 15+ messages in thread
From: Joe Perches @ 2017-04-28 23:04 UTC (permalink / raw)
To: Dan Williams
Cc: linux-nvdimm@lists.01.org, Ohad Ben-Cohen, linux-remoteproc,
linux-kernel, Bjorn Andersson, Linux ACPI, Jason Baron
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] dynamic_hex_dump cleanup
2017-04-28 23:04 ` Joe Perches
@ 2017-04-28 23:12 ` Dan Williams
0 siblings, 0 replies; 15+ messages in thread
From: Dan Williams @ 2017-04-28 23:12 UTC (permalink / raw)
To: Joe Perches
Cc: linux-nvdimm@lists.01.org, Ohad Ben-Cohen, linux-remoteproc,
linux-kernel, Bjorn Andersson, Linux ACPI, Jason Baron
On Fri, Apr 28, 2017 at 4:04 PM, Joe Perches <joe@perches.com> 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!
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2] acpi, nfit: kill ACPI_NFIT_DEBUG
2017-04-28 21:29 ` [PATCH 3/3] acpi, nfit: kill ACPI_NFIT_DEBUG, " Dan Williams
@ 2017-04-28 23:48 ` Dan Williams
0 siblings, 0 replies; 15+ messages in thread
From: Dan Williams @ 2017-04-28 23:48 UTC (permalink / raw)
To: linux-nvdimm; +Cc: Joe Perches, linux-acpi, linux-kernel
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@perches.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
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,
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] virtio, rpmsg: switch to dynamic_hex_dump()
2017-04-28 21:29 ` [PATCH 2/3] virtio, rpmsg: switch to dynamic_hex_dump() Dan Williams
@ 2017-04-29 8:46 ` kbuild test robot
0 siblings, 0 replies; 15+ messages in thread
From: kbuild test robot @ 2017-04-29 8:46 UTC (permalink / raw)
To: Dan Williams
Cc: kbuild-all, linux-nvdimm, Ohad Ben-Cohen, linux-acpi,
linux-remoteproc, linux-kernel, Bjorn Andersson
[-- Attachment #1: Type: text/plain, Size: 2380 bytes --]
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@ti.com>
:::::: CC: Bjorn Andersson <bjorn.andersson@linaro.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40581 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-04-29 8:46 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-28 21:28 [PATCH 0/3] dynamic_hex_dump cleanup Dan Williams
2017-04-28 21:28 ` [PATCH 1/3] printk: provide generic dynamic_hex_dump fallback Dan Williams
2017-04-28 21:29 ` [PATCH 2/3] virtio, rpmsg: switch to dynamic_hex_dump() Dan Williams
2017-04-29 8:46 ` kbuild test robot
2017-04-28 21:29 ` [PATCH 3/3] acpi, nfit: kill ACPI_NFIT_DEBUG, " Dan Williams
2017-04-28 23:48 ` [PATCH v2] acpi, nfit: kill ACPI_NFIT_DEBUG Dan Williams
2017-04-28 21:49 ` [PATCH 0/3] dynamic_hex_dump cleanup Joe Perches
2017-04-28 21:52 ` Dan Williams
2017-04-28 22:07 ` Joe Perches
2017-04-28 22:14 ` Dan Williams
2017-04-28 22:19 ` Dan Williams
2017-04-28 22:31 ` Joe Perches
2017-04-28 22:59 ` Dan Williams
2017-04-28 23:04 ` Joe Perches
2017-04-28 23:12 ` Dan Williams
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).