* [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()
@ 2015-12-08 16:07 Masahiro Yamada
2015-12-08 16:16 ` Joe Perches
0 siblings, 1 reply; 26+ messages in thread
From: Masahiro Yamada @ 2015-12-08 16:07 UTC (permalink / raw)
To: devicetree
Cc: Masahiro Yamada, Frank Rowand, Rob Herring, linux-kernel, Grant Likely
Trivial changes suggested by checkpatch.pl.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2:
- Fix printk(KERN_DEBUG ...) too
drivers/of/address.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 9582c57..65fafbb 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -23,7 +23,7 @@ static int __of_address_to_resource(struct device_node *dev,
#ifdef DEBUG
static void of_dump_addr(const char *s, const __be32 *addr, int na)
{
- printk(KERN_DEBUG "%s", s);
+ pr_debug("%s", s);
while (na--)
printk(" %08x", be32_to_cpu(*(addr++)));
printk("\n");
@@ -597,7 +597,7 @@ static u64 __of_translate_address(struct device_node *dev,
pbus = of_match_bus(parent);
pbus->count_cells(dev, &pna, &pns);
if (!OF_CHECK_COUNTS(pna, pns)) {
- printk(KERN_ERR "prom_parse: Bad cell count for %s\n",
+ pr_err("prom_parse: Bad cell count for %s\n",
of_node_full_name(dev));
break;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()
2015-12-08 16:07 [PATCH v2] of/address: replace printk() with pr_debug() / pr_err() Masahiro Yamada
@ 2015-12-08 16:16 ` Joe Perches
2015-12-08 17:03 ` Joe Perches
0 siblings, 1 reply; 26+ messages in thread
From: Joe Perches @ 2015-12-08 16:16 UTC (permalink / raw)
To: Masahiro Yamada, devicetree
Cc: Frank Rowand, Rob Herring, linux-kernel, Grant Likely
On Wed, 2015-12-09 at 01:07 +0900, Masahiro Yamada wrote:
> Trivial changes suggested by checkpatch.pl.
[]
> diff --git a/drivers/of/address.c b/drivers/of/address.c
[]
> @@ -23,7 +23,7 @@ static int __of_address_to_resource(struct device_node *dev,
> #ifdef DEBUG
> static void of_dump_addr(const char *s, const __be32 *addr, int na)
> {
> - printk(KERN_DEBUG "%s", s);
> + pr_debug("%s", s);
> while (na--)
> printk(" %08x", be32_to_cpu(*(addr++)));
> printk("\n");
mixing pr_debug and printk doesn't make much sense.
It might be nicer to use
static void of_dumpaddr(const char *s, const __be32 *addr, int na)
{
#ifdef DEBUG
...
#endif
}
to avoid the other static declaration
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()
2015-12-08 16:16 ` Joe Perches
@ 2015-12-08 17:03 ` Joe Perches
2015-12-08 21:28 ` Rob Herring
0 siblings, 1 reply; 26+ messages in thread
From: Joe Perches @ 2015-12-08 17:03 UTC (permalink / raw)
To: Masahiro Yamada, devicetree
Cc: Frank Rowand, Rob Herring, linux-kernel, Grant Likely
On Tue, 2015-12-08 at 08:16 -0800, Joe Perches wrote:
> On Wed, 2015-12-09 at 01:07 +0900, Masahiro Yamada wrote:
> > Trivial changes suggested by checkpatch.pl.
> []
> > diff --git a/drivers/of/address.c b/drivers/of/address.c
> []
> > @@ -23,7 +23,7 @@ static int __of_address_to_resource(struct device_node *dev,
> > #ifdef DEBUG
> > static void of_dump_addr(const char *s, const __be32 *addr, int na)
> > {
> > - printk(KERN_DEBUG "%s", s);
> > + pr_debug("%s", s);
> > while (na--)
> > printk(" %08x", be32_to_cpu(*(addr++)));
> > printk("\n");
>
> mixing pr_debug and printk doesn't make much sense.
>
> It might be nicer to use
>
> static void of_dumpaddr(const char *s, const __be32 *addr, int na)
> {
> #ifdef DEBUG
> ...
> #endif
> }
>
> to avoid the other static declaration
Or more simply:
static void of_dumpaddr(const char *s, const __be32 *addr, int na)
{
print_hex_dump_debug(s, DUMP_PREFIX_NONE, 16, 1,
addr, na * sizeof(__be32), false);
}
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()
2015-12-08 17:03 ` Joe Perches
@ 2015-12-08 21:28 ` Rob Herring
2015-12-09 0:03 ` Joe Perches
2015-12-09 21:59 ` [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions Joe Perches
0 siblings, 2 replies; 26+ messages in thread
From: Rob Herring @ 2015-12-08 21:28 UTC (permalink / raw)
To: Joe Perches
Cc: Masahiro Yamada, devicetree, Frank Rowand, linux-kernel, Grant Likely
On Tue, Dec 8, 2015 at 11:03 AM, Joe Perches <joe@perches.com> wrote:
> On Tue, 2015-12-08 at 08:16 -0800, Joe Perches wrote:
>> On Wed, 2015-12-09 at 01:07 +0900, Masahiro Yamada wrote:
>> > Trivial changes suggested by checkpatch.pl.
>> []
>> > diff --git a/drivers/of/address.c b/drivers/of/address.c
>> []
>> > @@ -23,7 +23,7 @@ static int __of_address_to_resource(struct device_node *dev,
>> > #ifdef DEBUG
>> > static void of_dump_addr(const char *s, const __be32 *addr, int na)
>> > {
>> > - printk(KERN_DEBUG "%s", s);
>> > + pr_debug("%s", s);
>> > while (na--)
>> > printk(" %08x", be32_to_cpu(*(addr++)));
>> > printk("\n");
>>
>> mixing pr_debug and printk doesn't make much sense.
>>
>> It might be nicer to use
>>
>> static void of_dumpaddr(const char *s, const __be32 *addr, int na)
>> {
>> #ifdef DEBUG
>> ...
>> #endif
>> }
>>
>> to avoid the other static declaration
>
> Or more simply:
>
> static void of_dumpaddr(const char *s, const __be32 *addr, int na)
> {
> print_hex_dump_debug(s, DUMP_PREFIX_NONE, 16, 1,
> addr, na * sizeof(__be32), false);
> }
Except that it doesn't do the endian swapping. Looking closer at this,
we should just drop this hunk. So I will take v1.
Rob
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()
2015-12-08 21:28 ` Rob Herring
@ 2015-12-09 0:03 ` Joe Perches
2015-12-09 12:03 ` Andy Shevchenko
2015-12-09 21:59 ` [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions Joe Perches
1 sibling, 1 reply; 26+ messages in thread
From: Joe Perches @ 2015-12-09 0:03 UTC (permalink / raw)
To: Rob Herring, Andrew Morton
Cc: Masahiro Yamada, devicetree, Frank Rowand, linux-kernel, Grant Likely
On Tue, 2015-12-08 at 15:28 -0600, Rob Herring wrote:
> On Tue, Dec 8, 2015 at 11:03 AM, Joe Perches <joe@perches.com> wrote:
> > On Tue, 2015-12-08 at 08:16 -0800, Joe Perches wrote:
> > > On Wed, 2015-12-09 at 01:07 +0900, Masahiro Yamada wrote:
> > > > Trivial changes suggested by checkpatch.pl.
> > > []
> > > > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > > []
> > > > @@ -23,7 +23,7 @@ static int __of_address_to_resource(struct device_node *dev,
> > > > #ifdef DEBUG
> > > > static void of_dump_addr(const char *s, const __be32 *addr, int na)
> > > > {
> > > > - printk(KERN_DEBUG "%s", s);
> > > > + pr_debug("%s", s);
> > > > while (na--)
> > > > printk(" %08x", be32_to_cpu(*(addr++)));
> > > > printk("\n");
[]
> > static void of_dumpaddr(const char *s, const __be32 *addr, int na)
> > {
> > print_hex_dump_debug(s, DUMP_PREFIX_NONE, 16, 1,
> > addr, na * sizeof(__be32), false);
> > }
>
> Except that it doesn't do the endian swapping. Looking closer at this,
> we should just drop this hunk. So I will take v1.
Maybe endian conversions should be added to hex_dump_debug like:
(probably doesn't apply. Evolution 3.18 is horrible)
---
include/linux/printk.h | 7 +++++++
lib/hexdump.c | 56 +++++++++++++++++++++++++++++++++++---------------
2 files changed, 47 insertions(+), 16 deletions(-)
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 9729565..4be190c 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -424,6 +424,13 @@ enum {
DUMP_PREFIX_ADDRESS,
DUMP_PREFIX_OFFSET
};
+
+enum {
+ DUMP_TYPE_CPU = 0,
+ DUMP_TYPE_LE = BIT(30),
+ DUMP_TYPE_BE = BIT(31)
+};
+
extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
int groupsize, char *linebuf, size_t linebuflen,
bool ascii);
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 992457b..49113aa 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -81,6 +81,7 @@ EXPORT_SYMBOL(bin2hex);
* @len: number of bytes in the @buf
* @rowsize: number of bytes to print per line; must be 16 or 32
* @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * OR'd with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
* @linebuf: where to put the converted data
* @linebuflen: total size of @linebuf, including space for terminating NUL
* @ascii: include ASCII after the hex output
@@ -114,19 +115,20 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
int j, lx = 0;
int ascii_column;
int ret;
+ int actual_groupsize = groupsize & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
if (rowsize != 16 && rowsize != 32)
rowsize = 16;
if (len > rowsize) /* limit to one line at a time */
len = rowsize;
- if (!is_power_of_2(groupsize) || groupsize > 8)
- groupsize = 1;
- if ((len % groupsize) != 0) /* no mixed size output */
- groupsize = 1;
+ if (!is_power_of_2(actual_groupsize) || actual_groupsize > 8)
+ actual_groupsize = 1;
+ if ((len % actual_groupsize) != 0) /* no mixed size output */
+ actual_groupsize = 1;
- ngroups = len / groupsize;
- ascii_column = rowsize * 2 + rowsize / groupsize + 1;
+ ngroups = len / actual_groupsize;
+ ascii_column = rowsize * 2 + rowsize / actual_groupsize + 1;
if (!linebuflen)
goto overflow1;
@@ -134,35 +136,56 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
if (!len)
goto nil;
- if (groupsize == 8) {
+ if (actual_groupsize == 8) {
const u64 *ptr8 = buf;
for (j = 0; j < ngroups; j++) {
+ u64 val;
+
+ if (groupsize & DUMP_TYPE_LE)
+ val = get_unaligned_le64(ptr8 + j);
+ else if (groupsize & DUMP_TYPE_BE)
+ val = get_unaligned_be64(ptr8 + j);
+ else
+ val = get_unaligned(ptr8 + j);
ret = snprintf(linebuf + lx, linebuflen - lx,
- "%s%16.16llx", j ? " " : "",
- get_unaligned(ptr8 + j));
+ "%s%16.16llx", j ? " " : "", val);
if (ret >= linebuflen - lx)
goto overflow1;
lx += ret;
}
- } else if (groupsize == 4) {
+ } else if (actual_groupsize == 4) {
const u32 *ptr4 = buf;
for (j = 0; j < ngroups; j++) {
+ u32 val;
+
+ if (groupsize & DUMP_TYPE_LE)
+ val = get_unaligned_le32(ptr4 + j);
+ else if (groupsize & DUMP_TYPE_BE)
+ val = get_unaligned_be32(ptr4 + j);
+ else
+ val = get_unaligned(ptr4 + j);
ret = snprintf(linebuf + lx, linebuflen - lx,
- "%s%8.8x", j ? " " : "",
- get_unaligned(ptr4 + j));
+ "%s%8.8x", j ? " " : "", val);
if (ret >= linebuflen - lx)
goto overflow1;
lx += ret;
}
- } else if (groupsize == 2) {
+ } else if (actual_groupsize == 2) {
const u16 *ptr2 = buf;
for (j = 0; j < ngroups; j++) {
+ u16 val;
+
+ if (groupsize & DUMP_TYPE_LE)
+ val = get_unaligned_le16(ptr2 + j);
+ else if (groupsize & DUMP_TYPE_BE)
+ val = get_unaligned_be16(ptr2 + j);
+ else
+ val = get_unaligned(ptr2 + j);
ret = snprintf(linebuf + lx, linebuflen - lx,
- "%s%4.4x", j ? " " : "",
- get_unaligned(ptr2 + j));
+ "%s%4.4x", j ? " " : "", val);
if (ret >= linebuflen - lx)
goto overflow1;
lx += ret;
@@ -203,7 +226,8 @@ nil:
overflow2:
linebuf[lx++] = '\0';
overflow1:
- return ascii ? ascii_column + len : (groupsize * 2 + 1) * ngroups - 1;
+ return ascii ? ascii_column + len
+ : (actual_groupsize * 2 + 1) * ngroups - 1;
}
EXPORT_SYMBOL(hex_dump_to_buffer);
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()
@ 2015-12-09 12:03 ` Andy Shevchenko
0 siblings, 0 replies; 26+ messages in thread
From: Andy Shevchenko @ 2015-12-09 12:03 UTC (permalink / raw)
To: Joe Perches
Cc: Rob Herring, Andrew Morton, Masahiro Yamada, devicetree,
Frank Rowand, linux-kernel, Grant Likely
On Wed, Dec 9, 2015 at 2:03 AM, Joe Perches <joe@perches.com> wrote:
> On Tue, 2015-12-08 at 15:28 -0600, Rob Herring wrote:
>> On Tue, Dec 8, 2015 at 11:03 AM, Joe Perches <joe@perches.com> wrote:
>> > On Tue, 2015-12-08 at 08:16 -0800, Joe Perches wrote:
>> > > On Wed, 2015-12-09 at 01:07 +0900, Masahiro Yamada wrote:
>> > > > Trivial changes suggested by checkpatch.pl.
>> > > []
>> > > > diff --git a/drivers/of/address.c b/drivers/of/address.c
>> > > []
>> > > > @@ -23,7 +23,7 @@ static int __of_address_to_resource(struct device_node *dev,
>> > > > #ifdef DEBUG
>> > > > static void of_dump_addr(const char *s, const __be32 *addr, int na)
>> > > > {
>> > > > - printk(KERN_DEBUG "%s", s);
>> > > > + pr_debug("%s", s);
>> > > > while (na--)
>> > > > printk(" %08x", be32_to_cpu(*(addr++)));
>> > > > printk("\n");
> []
>> > static void of_dumpaddr(const char *s, const __be32 *addr, int na)
>> > {
>> > print_hex_dump_debug(s, DUMP_PREFIX_NONE, 16, 1,
>> > addr, na * sizeof(__be32), false);
>> > }
>>
>> Except that it doesn't do the endian swapping. Looking closer at this,
>> we should just drop this hunk. So I will take v1.
>
> Maybe endian conversions should be added to hex_dump_debug like:
> (probably doesn't apply. Evolution 3.18 is horrible)
Overall the idea is not bad I think, though few comments below.
Besides that, please, update test_hexdump (wrt my update to the test
suite [1]) to go through BE test cases.
[1] http://www.spinics.net/lists/kernel/msg2139337.html
> ---
> include/linux/printk.h | 7 +++++++
> lib/hexdump.c | 56 +++++++++++++++++++++++++++++++++++---------------
> 2 files changed, 47 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 9729565..4be190c 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -424,6 +424,13 @@ enum {
> DUMP_PREFIX_ADDRESS,
> DUMP_PREFIX_OFFSET
> };
> +
> +enum {
> + DUMP_TYPE_CPU = 0,
Do we need explicit enum for that? Documentation update anyway is required.
> + DUMP_TYPE_LE = BIT(30),
> + DUMP_TYPE_BE = BIT(31)
> +};
> +
> extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
> int groupsize, char *linebuf, size_t linebuflen,
> bool ascii);
> diff --git a/lib/hexdump.c b/lib/hexdump.c
> index 992457b..49113aa 100644
> --- a/lib/hexdump.c
> +++ b/lib/hexdump.c
> @@ -81,6 +81,7 @@ EXPORT_SYMBOL(bin2hex);
> * @len: number of bytes in the @buf
> * @rowsize: number of bytes to print per line; must be 16 or 32
> * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> + * OR'd with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
> * @linebuf: where to put the converted data
> * @linebuflen: total size of @linebuf, including space for terminating NUL
> * @ascii: include ASCII after the hex output
> @@ -114,19 +115,20 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> int j, lx = 0;
> int ascii_column;
> int ret;
> + int actual_groupsize = groupsize & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
I would rather prefer to have function parameter to be renamed.
E.g. gsflags ?
>
> if (rowsize != 16 && rowsize != 32)
> rowsize = 16;
>
> if (len > rowsize) /* limit to one line at a time */
> len = rowsize;
> - if (!is_power_of_2(groupsize) || groupsize > 8)
> - groupsize = 1;
> - if ((len % groupsize) != 0) /* no mixed size output */
> - groupsize = 1;
> + if (!is_power_of_2(actual_groupsize) || actual_groupsize > 8)
> + actual_groupsize = 1;
> + if ((len % actual_groupsize) != 0) /* no mixed size output */
> + actual_groupsize = 1;
>
> - ngroups = len / groupsize;
> - ascii_column = rowsize * 2 + rowsize / groupsize + 1;
> + ngroups = len / actual_groupsize;
> + ascii_column = rowsize * 2 + rowsize / actual_groupsize + 1;
>
> if (!linebuflen)
> goto overflow1;
> @@ -134,35 +136,56 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> if (!len)
> goto nil;
>
> - if (groupsize == 8) {
> + if (actual_groupsize == 8) {
> const u64 *ptr8 = buf;
>
> for (j = 0; j < ngroups; j++) {
> + u64 val;
> +
> + if (groupsize & DUMP_TYPE_LE)
> + val = get_unaligned_le64(ptr8 + j);
> + else if (groupsize & DUMP_TYPE_BE)
> + val = get_unaligned_be64(ptr8 + j);
> + else
> + val = get_unaligned(ptr8 + j);
> ret = snprintf(linebuf + lx, linebuflen - lx,
> - "%s%16.16llx", j ? " " : "",
> - get_unaligned(ptr8 + j));
> + "%s%16.16llx", j ? " " : "", val);
> if (ret >= linebuflen - lx)
> goto overflow1;
> lx += ret;
> }
> - } else if (groupsize == 4) {
> + } else if (actual_groupsize == 4) {
> const u32 *ptr4 = buf;
>
> for (j = 0; j < ngroups; j++) {
> + u32 val;
> +
> + if (groupsize & DUMP_TYPE_LE)
> + val = get_unaligned_le32(ptr4 + j);
> + else if (groupsize & DUMP_TYPE_BE)
> + val = get_unaligned_be32(ptr4 + j);
> + else
> + val = get_unaligned(ptr4 + j);
> ret = snprintf(linebuf + lx, linebuflen - lx,
> - "%s%8.8x", j ? " " : "",
> - get_unaligned(ptr4 + j));
> + "%s%8.8x", j ? " " : "", val);
> if (ret >= linebuflen - lx)
> goto overflow1;
> lx += ret;
> }
> - } else if (groupsize == 2) {
> + } else if (actual_groupsize == 2) {
> const u16 *ptr2 = buf;
>
> for (j = 0; j < ngroups; j++) {
> + u16 val;
> +
> + if (groupsize & DUMP_TYPE_LE)
> + val = get_unaligned_le16(ptr2 + j);
> + else if (groupsize & DUMP_TYPE_BE)
> + val = get_unaligned_be16(ptr2 + j);
> + else
> + val = get_unaligned(ptr2 + j);
> ret = snprintf(linebuf + lx, linebuflen - lx,
> - "%s%4.4x", j ? " " : "",
> - get_unaligned(ptr2 + j));
> + "%s%4.4x", j ? " " : "", val);
> if (ret >= linebuflen - lx)
> goto overflow1;
> lx += ret;
> @@ -203,7 +226,8 @@ nil:
> overflow2:
> linebuf[lx++] = '\0';
> overflow1:
> - return ascii ? ascii_column + len : (groupsize * 2 + 1) * ngroups - 1;
> + return ascii ? ascii_column + len
> + : (actual_groupsize * 2 + 1) * ngroups - 1;
> }
> EXPORT_SYMBOL(hex_dump_to_buffer);
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()
@ 2015-12-09 12:03 ` Andy Shevchenko
0 siblings, 0 replies; 26+ messages in thread
From: Andy Shevchenko @ 2015-12-09 12:03 UTC (permalink / raw)
To: Joe Perches
Cc: Rob Herring, Andrew Morton, Masahiro Yamada,
devicetree-u79uwXL29TY76Z2rM5mHXA, Frank Rowand,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Grant Likely
On Wed, Dec 9, 2015 at 2:03 AM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> On Tue, 2015-12-08 at 15:28 -0600, Rob Herring wrote:
>> On Tue, Dec 8, 2015 at 11:03 AM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
>> > On Tue, 2015-12-08 at 08:16 -0800, Joe Perches wrote:
>> > > On Wed, 2015-12-09 at 01:07 +0900, Masahiro Yamada wrote:
>> > > > Trivial changes suggested by checkpatch.pl.
>> > > []
>> > > > diff --git a/drivers/of/address.c b/drivers/of/address.c
>> > > []
>> > > > @@ -23,7 +23,7 @@ static int __of_address_to_resource(struct device_node *dev,
>> > > > #ifdef DEBUG
>> > > > static void of_dump_addr(const char *s, const __be32 *addr, int na)
>> > > > {
>> > > > - printk(KERN_DEBUG "%s", s);
>> > > > + pr_debug("%s", s);
>> > > > while (na--)
>> > > > printk(" %08x", be32_to_cpu(*(addr++)));
>> > > > printk("\n");
> []
>> > static void of_dumpaddr(const char *s, const __be32 *addr, int na)
>> > {
>> > print_hex_dump_debug(s, DUMP_PREFIX_NONE, 16, 1,
>> > addr, na * sizeof(__be32), false);
>> > }
>>
>> Except that it doesn't do the endian swapping. Looking closer at this,
>> we should just drop this hunk. So I will take v1.
>
> Maybe endian conversions should be added to hex_dump_debug like:
> (probably doesn't apply. Evolution 3.18 is horrible)
Overall the idea is not bad I think, though few comments below.
Besides that, please, update test_hexdump (wrt my update to the test
suite [1]) to go through BE test cases.
[1] http://www.spinics.net/lists/kernel/msg2139337.html
> ---
> include/linux/printk.h | 7 +++++++
> lib/hexdump.c | 56 +++++++++++++++++++++++++++++++++++---------------
> 2 files changed, 47 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 9729565..4be190c 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -424,6 +424,13 @@ enum {
> DUMP_PREFIX_ADDRESS,
> DUMP_PREFIX_OFFSET
> };
> +
> +enum {
> + DUMP_TYPE_CPU = 0,
Do we need explicit enum for that? Documentation update anyway is required.
> + DUMP_TYPE_LE = BIT(30),
> + DUMP_TYPE_BE = BIT(31)
> +};
> +
> extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
> int groupsize, char *linebuf, size_t linebuflen,
> bool ascii);
> diff --git a/lib/hexdump.c b/lib/hexdump.c
> index 992457b..49113aa 100644
> --- a/lib/hexdump.c
> +++ b/lib/hexdump.c
> @@ -81,6 +81,7 @@ EXPORT_SYMBOL(bin2hex);
> * @len: number of bytes in the @buf
> * @rowsize: number of bytes to print per line; must be 16 or 32
> * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> + * OR'd with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
> * @linebuf: where to put the converted data
> * @linebuflen: total size of @linebuf, including space for terminating NUL
> * @ascii: include ASCII after the hex output
> @@ -114,19 +115,20 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> int j, lx = 0;
> int ascii_column;
> int ret;
> + int actual_groupsize = groupsize & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
I would rather prefer to have function parameter to be renamed.
E.g. gsflags ?
>
> if (rowsize != 16 && rowsize != 32)
> rowsize = 16;
>
> if (len > rowsize) /* limit to one line at a time */
> len = rowsize;
> - if (!is_power_of_2(groupsize) || groupsize > 8)
> - groupsize = 1;
> - if ((len % groupsize) != 0) /* no mixed size output */
> - groupsize = 1;
> + if (!is_power_of_2(actual_groupsize) || actual_groupsize > 8)
> + actual_groupsize = 1;
> + if ((len % actual_groupsize) != 0) /* no mixed size output */
> + actual_groupsize = 1;
>
> - ngroups = len / groupsize;
> - ascii_column = rowsize * 2 + rowsize / groupsize + 1;
> + ngroups = len / actual_groupsize;
> + ascii_column = rowsize * 2 + rowsize / actual_groupsize + 1;
>
> if (!linebuflen)
> goto overflow1;
> @@ -134,35 +136,56 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> if (!len)
> goto nil;
>
> - if (groupsize == 8) {
> + if (actual_groupsize == 8) {
> const u64 *ptr8 = buf;
>
> for (j = 0; j < ngroups; j++) {
> + u64 val;
> +
> + if (groupsize & DUMP_TYPE_LE)
> + val = get_unaligned_le64(ptr8 + j);
> + else if (groupsize & DUMP_TYPE_BE)
> + val = get_unaligned_be64(ptr8 + j);
> + else
> + val = get_unaligned(ptr8 + j);
> ret = snprintf(linebuf + lx, linebuflen - lx,
> - "%s%16.16llx", j ? " " : "",
> - get_unaligned(ptr8 + j));
> + "%s%16.16llx", j ? " " : "", val);
> if (ret >= linebuflen - lx)
> goto overflow1;
> lx += ret;
> }
> - } else if (groupsize == 4) {
> + } else if (actual_groupsize == 4) {
> const u32 *ptr4 = buf;
>
> for (j = 0; j < ngroups; j++) {
> + u32 val;
> +
> + if (groupsize & DUMP_TYPE_LE)
> + val = get_unaligned_le32(ptr4 + j);
> + else if (groupsize & DUMP_TYPE_BE)
> + val = get_unaligned_be32(ptr4 + j);
> + else
> + val = get_unaligned(ptr4 + j);
> ret = snprintf(linebuf + lx, linebuflen - lx,
> - "%s%8.8x", j ? " " : "",
> - get_unaligned(ptr4 + j));
> + "%s%8.8x", j ? " " : "", val);
> if (ret >= linebuflen - lx)
> goto overflow1;
> lx += ret;
> }
> - } else if (groupsize == 2) {
> + } else if (actual_groupsize == 2) {
> const u16 *ptr2 = buf;
>
> for (j = 0; j < ngroups; j++) {
> + u16 val;
> +
> + if (groupsize & DUMP_TYPE_LE)
> + val = get_unaligned_le16(ptr2 + j);
> + else if (groupsize & DUMP_TYPE_BE)
> + val = get_unaligned_be16(ptr2 + j);
> + else
> + val = get_unaligned(ptr2 + j);
> ret = snprintf(linebuf + lx, linebuflen - lx,
> - "%s%4.4x", j ? " " : "",
> - get_unaligned(ptr2 + j));
> + "%s%4.4x", j ? " " : "", val);
> if (ret >= linebuflen - lx)
> goto overflow1;
> lx += ret;
> @@ -203,7 +226,8 @@ nil:
> overflow2:
> linebuf[lx++] = '\0';
> overflow1:
> - return ascii ? ascii_column + len : (groupsize * 2 + 1) * ngroups - 1;
> + return ascii ? ascii_column + len
> + : (actual_groupsize * 2 + 1) * ngroups - 1;
> }
> EXPORT_SYMBOL(hex_dump_to_buffer);
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()
2015-12-09 12:03 ` Andy Shevchenko
(?)
@ 2015-12-09 19:28 ` Joe Perches
2015-12-09 20:02 ` Andy Shevchenko
-1 siblings, 1 reply; 26+ messages in thread
From: Joe Perches @ 2015-12-09 19:28 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Rob Herring, Andrew Morton, Masahiro Yamada, devicetree,
Frank Rowand, linux-kernel, Grant Likely, Rasmus Villemoes
On Wed, 2015-12-09 at 14:03 +0200, Andy Shevchenko wrote:
> On Wed, Dec 9, 2015 at 2:03 AM, Joe Perches <joe@perches.com> wrote:
> > On Tue, 2015-12-08 at 15:28 -0600, Rob Herring wrote:
> > > On Tue, Dec 8, 2015 at 11:03 AM, Joe Perches <joe@perches.com> wrote:
> > > > On Tue, 2015-12-08 at 08:16 -0800, Joe Perches wrote:
> > > > > On Wed, 2015-12-09 at 01:07 +0900, Masahiro Yamada wrote:
> > > > > > Trivial changes suggested by checkpatch.pl.
> > > > > []
> > > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > > > > []
> > > > > > @@ -23,7 +23,7 @@ static int __of_address_to_resource(struct device_node *dev,
> > > > > > #ifdef DEBUG
> > > > > > static void of_dump_addr(const char *s, const __be32 *addr, int na)
> > > > > > {
> > > > > > - printk(KERN_DEBUG "%s", s);
> > > > > > + pr_debug("%s", s);
> > > > > > while (na--)
> > > > > > printk(" %08x", be32_to_cpu(*(addr++)));
> > > > > > printk("\n");
> > []
> > > > static void of_dumpaddr(const char *s, const __be32 *addr, int na)
> > > > {
> > > > print_hex_dump_debug(s, DUMP_PREFIX_NONE, 16, 1,
> > > > addr, na * sizeof(__be32), false);
> > > > }
> > >
> > > Except that it doesn't do the endian swapping. Looking closer at this,
> > > we should just drop this hunk. So I will take v1.
> >
> > Maybe endian conversions should be added to hex_dump_debug like:
> > (probably doesn't apply. Evolution 3.18 is horrible)
>
> Overall the idea is not bad I think, though few comments below.
> Besides that, please, update test_hexdump (wrt my update to the test
> suite [1]) to go through BE test cases.
>
> [1] http://www.spinics.net/lists/kernel/msg2139337.html
I don't want to have to learn that code as it seems a bit tricky.
Maybe you could do it.
> > diff --git a/lib/hexdump.c b/lib/hexdump.c
> > index 992457b..49113aa 100644
> > --- a/lib/hexdump.c
> > +++ b/lib/hexdump.c
> > @@ -81,6 +81,7 @@ EXPORT_SYMBOL(bin2hex);
> > * @len: number of bytes in the @buf
> > * @rowsize: number of bytes to print per line; must be 16 or 32
> > * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> > + * OR'd with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
> > * @linebuf: where to put the converted data
> > * @linebuflen: total size of @linebuf, including space for terminating NUL
> > * @ascii: include ASCII after the hex output
> > @@ -114,19 +115,20 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> > int j, lx = 0;
> > int ascii_column;
> > int ret;
> > + int actual_groupsize = groupsize & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
>
> I would rather prefer to have function parameter to be renamed.
>
> E.g. gsflags ?
>
Well, it's a bit simpler changelog.
---
include/linux/printk.h | 7 +++++++
lib/hexdump.c | 39 +++++++++++++++++++++++++++++++--------
2 files changed, 38 insertions(+), 8 deletions(-)
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 9729565..4be190c 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -424,6 +424,13 @@ enum {
DUMP_PREFIX_ADDRESS,
DUMP_PREFIX_OFFSET
};
+
+enum {
+ DUMP_TYPE_CPU = 0,
+ DUMP_TYPE_LE = BIT(30),
+ DUMP_TYPE_BE = BIT(31)
+};
+
extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
int groupsize, char *linebuf, size_t linebuflen,
bool ascii);
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 992457b..5b1eda70 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -80,7 +80,8 @@ EXPORT_SYMBOL(bin2hex);
* @buf: data blob to dump
* @len: number of bytes in the @buf
* @rowsize: number of bytes to print per line; must be 16 or 32
- * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * OR with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
* @linebuf: where to put the converted data
* @linebuflen: total size of @linebuf, including space for terminating NUL
* @ascii: include ASCII after the hex output
@@ -105,7 +106,7 @@ EXPORT_SYMBOL(bin2hex);
* (excluding the terminating NUL) which would have been written to the final
* string if enough space had been available.
*/
-int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
+int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupflags,
char *linebuf, size_t linebuflen, bool ascii)
{
const u8 *ptr = buf;
@@ -114,6 +115,7 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
int j, lx = 0;
int ascii_column;
int ret;
+ int groupsize = groupflags & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
if (rowsize != 16 && rowsize != 32)
rowsize = 16;
@@ -138,9 +140,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
const u64 *ptr8 = buf;
for (j = 0; j < ngroups; j++) {
+ u64 val;
+
+ if (groupflags & DUMP_TYPE_LE)
+ val = get_unaligned_le64(ptr8 + j);
+ else if (groupflags & DUMP_TYPE_BE)
+ val = get_unaligned_be64(ptr8 + j);
+ else
+ val = get_unaligned(ptr8 + j);
ret = snprintf(linebuf + lx, linebuflen - lx,
- "%s%16.16llx", j ? " " : "",
- get_unaligned(ptr8 + j));
+ "%s%16.16llx", j ? " " : "", val);
if (ret >= linebuflen - lx)
goto overflow1;
lx += ret;
@@ -149,9 +158,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
const u32 *ptr4 = buf;
for (j = 0; j < ngroups; j++) {
+ u32 val;
+
+ if (groupflags & DUMP_TYPE_LE)
+ val = get_unaligned_le32(ptr4 + j);
+ else if (groupflags & DUMP_TYPE_BE)
+ val = get_unaligned_be32(ptr4 + j);
+ else
+ val = get_unaligned(ptr4 + j);
ret = snprintf(linebuf + lx, linebuflen - lx,
- "%s%8.8x", j ? " " : "",
- get_unaligned(ptr4 + j));
+ "%s%8.8x", j ? " " : "", val);
if (ret >= linebuflen - lx)
goto overflow1;
lx += ret;
@@ -160,9 +176,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
const u16 *ptr2 = buf;
for (j = 0; j < ngroups; j++) {
+ u16 val;
+
+ if (groupflags & DUMP_TYPE_LE)
+ val = get_unaligned_le16(ptr2 + j);
+ else if (groupflags & DUMP_TYPE_BE)
+ val = get_unaligned_be16(ptr2 + j);
+ else
+ val = get_unaligned(ptr2 + j);
ret = snprintf(linebuf + lx, linebuflen - lx,
- "%s%4.4x", j ? " " : "",
- get_unaligned(ptr2 + j));
+ "%s%4.4x", j ? " " : "", val);
if (ret >= linebuflen - lx)
goto overflow1;
lx += ret;
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()
@ 2015-12-09 20:02 ` Andy Shevchenko
0 siblings, 0 replies; 26+ messages in thread
From: Andy Shevchenko @ 2015-12-09 20:02 UTC (permalink / raw)
To: Joe Perches
Cc: Rob Herring, Andrew Morton, Masahiro Yamada, devicetree,
Frank Rowand, linux-kernel, Grant Likely, Rasmus Villemoes
On Wed, Dec 9, 2015 at 9:28 PM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2015-12-09 at 14:03 +0200, Andy Shevchenko wrote:
>> On Wed, Dec 9, 2015 at 2:03 AM, Joe Perches <joe@perches.com> wrote:
>> > On Tue, 2015-12-08 at 15:28 -0600, Rob Herring wrote:
>> > > On Tue, Dec 8, 2015 at 11:03 AM, Joe Perches <joe@perches.com> wrote:
>> > > > On Tue, 2015-12-08 at 08:16 -0800, Joe Perches wrote:
>> > > > > On Wed, 2015-12-09 at 01:07 +0900, Masahiro Yamada wrote:
>> > > > > > Trivial changes suggested by checkpatch.pl.
>> > > > > []
>> > > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c
>> > > > > []
>> > > > > > @@ -23,7 +23,7 @@ static int __of_address_to_resource(struct device_node *dev,
>> > > > > > #ifdef DEBUG
>> > > > > > static void of_dump_addr(const char *s, const __be32 *addr, int na)
>> > > > > > {
>> > > > > > - printk(KERN_DEBUG "%s", s);
>> > > > > > + pr_debug("%s", s);
>> > > > > > while (na--)
>> > > > > > printk(" %08x", be32_to_cpu(*(addr++)));
>> > > > > > printk("\n");
>> > []
>> > > > static void of_dumpaddr(const char *s, const __be32 *addr, int na)
>> > > > {
>> > > > print_hex_dump_debug(s, DUMP_PREFIX_NONE, 16, 1,
>> > > > addr, na * sizeof(__be32), false);
>> > > > }
>> > >
>> > > Except that it doesn't do the endian swapping. Looking closer at this,
>> > > we should just drop this hunk. So I will take v1.
>> >
>> > Maybe endian conversions should be added to hex_dump_debug like:
>> > (probably doesn't apply. Evolution 3.18 is horrible)
>>
>> Overall the idea is not bad I think, though few comments below.
>> Besides that, please, update test_hexdump (wrt my update to the test
>> suite [1]) to go through BE test cases.
>>
>> [1] http://www.spinics.net/lists/kernel/msg2139337.html
>
> I don't want to have to learn that code as it seems a bit tricky.
> Maybe you could do it.
Okay, I'm fine if it at least passes existing test cases. Good to
mention this in commit message then.
>
>> > diff --git a/lib/hexdump.c b/lib/hexdump.c
>> > index 992457b..49113aa 100644
>> > --- a/lib/hexdump.c
>> > +++ b/lib/hexdump.c
>> > @@ -81,6 +81,7 @@ EXPORT_SYMBOL(bin2hex);
>> > * @len: number of bytes in the @buf
>> > * @rowsize: number of bytes to print per line; must be 16 or 32
>> > * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
>> > + * OR'd with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
>> > * @linebuf: where to put the converted data
>> > * @linebuflen: total size of @linebuf, including space for terminating NUL
>> > * @ascii: include ASCII after the hex output
>> > @@ -114,19 +115,20 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>> > int j, lx = 0;
>> > int ascii_column;
>> > int ret;
>> > + int actual_groupsize = groupsize & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
>>
>> I would rather prefer to have function parameter to be renamed.
>>
>> E.g. gsflags ?
>>
>
> Well, it's a bit simpler changelog.
Generally I'm fine with this version, though take into account above and below.
> ---
> include/linux/printk.h | 7 +++++++
> lib/hexdump.c | 39 +++++++++++++++++++++++++++++++--------
> 2 files changed, 38 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 9729565..4be190c 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -424,6 +424,13 @@ enum {
> DUMP_PREFIX_ADDRESS,
> DUMP_PREFIX_OFFSET
> };
> +
> +enum {
> + DUMP_TYPE_CPU = 0,
And still open this, do we need it? I think you may just mention in
the documentation that default behaviour is CPU like.
> + DUMP_TYPE_LE = BIT(30),
> + DUMP_TYPE_BE = BIT(31)
> +};
> +
> extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
> int groupsize, char *linebuf, size_t linebuflen,
Here as well to change.
> bool ascii);
> diff --git a/lib/hexdump.c b/lib/hexdump.c
> index 992457b..5b1eda70 100644
> --- a/lib/hexdump.c
> +++ b/lib/hexdump.c
> @@ -80,7 +80,8 @@ EXPORT_SYMBOL(bin2hex);
> * @buf: data blob to dump
> * @len: number of bytes in the @buf
> * @rowsize: number of bytes to print per line; must be 16 or 32
> - * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> + * OR with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
Maybe specify "bitwise OR with.." ?
%DUMP_…
> * @linebuf: where to put the converted data
> * @linebuflen: total size of @linebuf, including space for terminating NUL
> * @ascii: include ASCII after the hex output
> @@ -105,7 +106,7 @@ EXPORT_SYMBOL(bin2hex);
> * (excluding the terminating NUL) which would have been written to the final
> * string if enough space had been available.
> */
> -int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> +int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupflags,
> char *linebuf, size_t linebuflen, bool ascii)
> {
> const u8 *ptr = buf;
> @@ -114,6 +115,7 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> int j, lx = 0;
> int ascii_column;
> int ret;
> + int groupsize = groupflags & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
>
> if (rowsize != 16 && rowsize != 32)
> rowsize = 16;
> @@ -138,9 +140,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> const u64 *ptr8 = buf;
>
> for (j = 0; j < ngroups; j++) {
> + u64 val;
> +
> + if (groupflags & DUMP_TYPE_LE)
> + val = get_unaligned_le64(ptr8 + j);
> + else if (groupflags & DUMP_TYPE_BE)
> + val = get_unaligned_be64(ptr8 + j);
> + else
> + val = get_unaligned(ptr8 + j);
> ret = snprintf(linebuf + lx, linebuflen - lx,
> - "%s%16.16llx", j ? " " : "",
> - get_unaligned(ptr8 + j));
> + "%s%16.16llx", j ? " " : "", val);
> if (ret >= linebuflen - lx)
> goto overflow1;
> lx += ret;
> @@ -149,9 +158,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> const u32 *ptr4 = buf;
>
> for (j = 0; j < ngroups; j++) {
> + u32 val;
> +
> + if (groupflags & DUMP_TYPE_LE)
> + val = get_unaligned_le32(ptr4 + j);
> + else if (groupflags & DUMP_TYPE_BE)
> + val = get_unaligned_be32(ptr4 + j);
> + else
> + val = get_unaligned(ptr4 + j);
> ret = snprintf(linebuf + lx, linebuflen - lx,
> - "%s%8.8x", j ? " " : "",
> - get_unaligned(ptr4 + j));
> + "%s%8.8x", j ? " " : "", val);
> if (ret >= linebuflen - lx)
> goto overflow1;
> lx += ret;
> @@ -160,9 +176,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> const u16 *ptr2 = buf;
>
> for (j = 0; j < ngroups; j++) {
> + u16 val;
> +
> + if (groupflags & DUMP_TYPE_LE)
> + val = get_unaligned_le16(ptr2 + j);
> + else if (groupflags & DUMP_TYPE_BE)
> + val = get_unaligned_be16(ptr2 + j);
> + else
> + val = get_unaligned(ptr2 + j);
> ret = snprintf(linebuf + lx, linebuflen - lx,
> - "%s%4.4x", j ? " " : "",
> - get_unaligned(ptr2 + j));
> + "%s%4.4x", j ? " " : "", val);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()
@ 2015-12-09 20:02 ` Andy Shevchenko
0 siblings, 0 replies; 26+ messages in thread
From: Andy Shevchenko @ 2015-12-09 20:02 UTC (permalink / raw)
To: Joe Perches
Cc: Rob Herring, Andrew Morton, Masahiro Yamada,
devicetree-u79uwXL29TY76Z2rM5mHXA, Frank Rowand,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Grant Likely,
Rasmus Villemoes
On Wed, Dec 9, 2015 at 9:28 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> On Wed, 2015-12-09 at 14:03 +0200, Andy Shevchenko wrote:
>> On Wed, Dec 9, 2015 at 2:03 AM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
>> > On Tue, 2015-12-08 at 15:28 -0600, Rob Herring wrote:
>> > > On Tue, Dec 8, 2015 at 11:03 AM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
>> > > > On Tue, 2015-12-08 at 08:16 -0800, Joe Perches wrote:
>> > > > > On Wed, 2015-12-09 at 01:07 +0900, Masahiro Yamada wrote:
>> > > > > > Trivial changes suggested by checkpatch.pl.
>> > > > > []
>> > > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c
>> > > > > []
>> > > > > > @@ -23,7 +23,7 @@ static int __of_address_to_resource(struct device_node *dev,
>> > > > > > #ifdef DEBUG
>> > > > > > static void of_dump_addr(const char *s, const __be32 *addr, int na)
>> > > > > > {
>> > > > > > - printk(KERN_DEBUG "%s", s);
>> > > > > > + pr_debug("%s", s);
>> > > > > > while (na--)
>> > > > > > printk(" %08x", be32_to_cpu(*(addr++)));
>> > > > > > printk("\n");
>> > []
>> > > > static void of_dumpaddr(const char *s, const __be32 *addr, int na)
>> > > > {
>> > > > print_hex_dump_debug(s, DUMP_PREFIX_NONE, 16, 1,
>> > > > addr, na * sizeof(__be32), false);
>> > > > }
>> > >
>> > > Except that it doesn't do the endian swapping. Looking closer at this,
>> > > we should just drop this hunk. So I will take v1.
>> >
>> > Maybe endian conversions should be added to hex_dump_debug like:
>> > (probably doesn't apply. Evolution 3.18 is horrible)
>>
>> Overall the idea is not bad I think, though few comments below.
>> Besides that, please, update test_hexdump (wrt my update to the test
>> suite [1]) to go through BE test cases.
>>
>> [1] http://www.spinics.net/lists/kernel/msg2139337.html
>
> I don't want to have to learn that code as it seems a bit tricky.
> Maybe you could do it.
Okay, I'm fine if it at least passes existing test cases. Good to
mention this in commit message then.
>
>> > diff --git a/lib/hexdump.c b/lib/hexdump.c
>> > index 992457b..49113aa 100644
>> > --- a/lib/hexdump.c
>> > +++ b/lib/hexdump.c
>> > @@ -81,6 +81,7 @@ EXPORT_SYMBOL(bin2hex);
>> > * @len: number of bytes in the @buf
>> > * @rowsize: number of bytes to print per line; must be 16 or 32
>> > * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
>> > + * OR'd with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
>> > * @linebuf: where to put the converted data
>> > * @linebuflen: total size of @linebuf, including space for terminating NUL
>> > * @ascii: include ASCII after the hex output
>> > @@ -114,19 +115,20 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>> > int j, lx = 0;
>> > int ascii_column;
>> > int ret;
>> > + int actual_groupsize = groupsize & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
>>
>> I would rather prefer to have function parameter to be renamed.
>>
>> E.g. gsflags ?
>>
>
> Well, it's a bit simpler changelog.
Generally I'm fine with this version, though take into account above and below.
> ---
> include/linux/printk.h | 7 +++++++
> lib/hexdump.c | 39 +++++++++++++++++++++++++++++++--------
> 2 files changed, 38 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 9729565..4be190c 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -424,6 +424,13 @@ enum {
> DUMP_PREFIX_ADDRESS,
> DUMP_PREFIX_OFFSET
> };
> +
> +enum {
> + DUMP_TYPE_CPU = 0,
And still open this, do we need it? I think you may just mention in
the documentation that default behaviour is CPU like.
> + DUMP_TYPE_LE = BIT(30),
> + DUMP_TYPE_BE = BIT(31)
> +};
> +
> extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
> int groupsize, char *linebuf, size_t linebuflen,
Here as well to change.
> bool ascii);
> diff --git a/lib/hexdump.c b/lib/hexdump.c
> index 992457b..5b1eda70 100644
> --- a/lib/hexdump.c
> +++ b/lib/hexdump.c
> @@ -80,7 +80,8 @@ EXPORT_SYMBOL(bin2hex);
> * @buf: data blob to dump
> * @len: number of bytes in the @buf
> * @rowsize: number of bytes to print per line; must be 16 or 32
> - * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> + * OR with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
Maybe specify "bitwise OR with.." ?
%DUMP_…
> * @linebuf: where to put the converted data
> * @linebuflen: total size of @linebuf, including space for terminating NUL
> * @ascii: include ASCII after the hex output
> @@ -105,7 +106,7 @@ EXPORT_SYMBOL(bin2hex);
> * (excluding the terminating NUL) which would have been written to the final
> * string if enough space had been available.
> */
> -int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> +int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupflags,
> char *linebuf, size_t linebuflen, bool ascii)
> {
> const u8 *ptr = buf;
> @@ -114,6 +115,7 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> int j, lx = 0;
> int ascii_column;
> int ret;
> + int groupsize = groupflags & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
>
> if (rowsize != 16 && rowsize != 32)
> rowsize = 16;
> @@ -138,9 +140,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> const u64 *ptr8 = buf;
>
> for (j = 0; j < ngroups; j++) {
> + u64 val;
> +
> + if (groupflags & DUMP_TYPE_LE)
> + val = get_unaligned_le64(ptr8 + j);
> + else if (groupflags & DUMP_TYPE_BE)
> + val = get_unaligned_be64(ptr8 + j);
> + else
> + val = get_unaligned(ptr8 + j);
> ret = snprintf(linebuf + lx, linebuflen - lx,
> - "%s%16.16llx", j ? " " : "",
> - get_unaligned(ptr8 + j));
> + "%s%16.16llx", j ? " " : "", val);
> if (ret >= linebuflen - lx)
> goto overflow1;
> lx += ret;
> @@ -149,9 +158,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> const u32 *ptr4 = buf;
>
> for (j = 0; j < ngroups; j++) {
> + u32 val;
> +
> + if (groupflags & DUMP_TYPE_LE)
> + val = get_unaligned_le32(ptr4 + j);
> + else if (groupflags & DUMP_TYPE_BE)
> + val = get_unaligned_be32(ptr4 + j);
> + else
> + val = get_unaligned(ptr4 + j);
> ret = snprintf(linebuf + lx, linebuflen - lx,
> - "%s%8.8x", j ? " " : "",
> - get_unaligned(ptr4 + j));
> + "%s%8.8x", j ? " " : "", val);
> if (ret >= linebuflen - lx)
> goto overflow1;
> lx += ret;
> @@ -160,9 +176,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> const u16 *ptr2 = buf;
>
> for (j = 0; j < ngroups; j++) {
> + u16 val;
> +
> + if (groupflags & DUMP_TYPE_LE)
> + val = get_unaligned_le16(ptr2 + j);
> + else if (groupflags & DUMP_TYPE_BE)
> + val = get_unaligned_be16(ptr2 + j);
> + else
> + val = get_unaligned(ptr2 + j);
> ret = snprintf(linebuf + lx, linebuflen - lx,
> - "%s%4.4x", j ? " " : "",
> - get_unaligned(ptr2 + j));
> + "%s%4.4x", j ? " " : "", val);
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()
2015-12-09 20:02 ` Andy Shevchenko
(?)
@ 2015-12-09 20:09 ` Joe Perches
-1 siblings, 0 replies; 26+ messages in thread
From: Joe Perches @ 2015-12-09 20:09 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Rob Herring, Andrew Morton, Masahiro Yamada, devicetree,
Frank Rowand, linux-kernel, Grant Likely, Rasmus Villemoes
On Wed, 2015-12-09 at 22:02 +0200, Andy Shevchenko wrote:
> On Wed, Dec 9, 2015 at 9:28 PM, Joe Perches <joe@perches.com> wrote:
[]
> > > > diff --git a/lib/hexdump.c b/lib/hexdump.c
> > > > index 992457b..49113aa 100644
> > > > --- a/lib/hexdump.c
> > > > +++ b/lib/hexdump.c
> > > > @@ -81,6 +81,7 @@ EXPORT_SYMBOL(bin2hex);
> > > > * @len: number of bytes in the @buf
> > > > * @rowsize: number of bytes to print per line; must be 16 or 32
> > > > * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> > > > + * OR'd with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
> > > > * @linebuf: where to put the converted data
> > > > * @linebuflen: total size of @linebuf, including space for terminating NUL
> > > > * @ascii: include ASCII after the hex output
> > > > @@ -114,19 +115,20 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> > > > int j, lx = 0;
> > > > int ascii_column;
> > > > int ret;
> > > > + int actual_groupsize = groupsize & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
> > >
> > > I would rather prefer to have function parameter to be renamed.
> > >
> > > E.g. gsflags ?
> > >
> >
> > Well, it's a bit simpler changelog.
>
> Generally I'm fine with this version, though take into account above and below.
>
> > ---
> > include/linux/printk.h | 7 +++++++
> > lib/hexdump.c | 39 +++++++++++++++++++++++++++++++--------
> > 2 files changed, 38 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/linux/printk.h b/include/linux/printk.h
> > index 9729565..4be190c 100644
> > --- a/include/linux/printk.h
> > +++ b/include/linux/printk.h
> > @@ -424,6 +424,13 @@ enum {
> > DUMP_PREFIX_ADDRESS,
> > DUMP_PREFIX_OFFSET
> > };
> > +
> > +enum {
> > + DUMP_TYPE_CPU = 0,
>
> And still open this, do we need it? I think you may just mention in
> the documentation that default behaviour is CPU like.
The only documentation I'm aware of is the kernel-doc
> > + DUMP_TYPE_LE = BIT(30),
> > + DUMP_TYPE_BE = BIT(31)
> > +};
> > +
> > extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
> > int groupsize, char *linebuf, size_t linebuflen,
>
> Here as well to change.
Right, thanks.
> > bool ascii);
> > diff --git a/lib/hexdump.c b/lib/hexdump.c
> > index 992457b..5b1eda70 100644
> > --- a/lib/hexdump.c
> > +++ b/lib/hexdump.c
> > @@ -80,7 +80,8 @@ EXPORT_SYMBOL(bin2hex);
> > * @buf: data blob to dump
> > * @len: number of bytes in the @buf
> > * @rowsize: number of bytes to print per line; must be 16 or 32
> > - * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> > + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> > + * OR with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
>
> Maybe specify "bitwise OR with.." ?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()
@ 2015-12-09 20:17 ` Rasmus Villemoes
0 siblings, 0 replies; 26+ messages in thread
From: Rasmus Villemoes @ 2015-12-09 20:17 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Joe Perches, Rob Herring, Andrew Morton, Masahiro Yamada,
devicetree, Frank Rowand, linux-kernel, Grant Likely
On Wed, Dec 09 2015, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Wed, Dec 9, 2015 at 9:28 PM, Joe Perches <joe@perches.com> wrote:
>> ---
>> include/linux/printk.h | 7 +++++++
>> lib/hexdump.c | 39 +++++++++++++++++++++++++++++++--------
>> 2 files changed, 38 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/linux/printk.h b/include/linux/printk.h
>> index 9729565..4be190c 100644
>> --- a/include/linux/printk.h
>> +++ b/include/linux/printk.h
>> @@ -424,6 +424,13 @@ enum {
>> DUMP_PREFIX_ADDRESS,
>> DUMP_PREFIX_OFFSET
>> };
>> +
>> +enum {
>> + DUMP_TYPE_CPU = 0,
>
> And still open this, do we need it? I think you may just mention in
> the documentation that default behaviour is CPU like.
I think it's best to have a name for the default. In this case it's
unlikely to ever be relevant, but in general one could imagine stuff
like
#ifdef THIS_OR_THAT
#define MY_DUMP_TYPE DUMP_TYPE_LE
#else
#define MY_DUMP_TYPE DUMP_TYPE_CPU
#endif
which is a lot more readable than if the latter was a naked 0.
The feature seems fine to me. It's always been somewhat annoying that
grouping changes the order of the byte values on little-endian machines
(so grouping is actually a wrong name for it; it's more like "treat the
bytes an array of u16/u32/u64"), and one can now get around that by
specifying DUMP_TYPE_BE.
Rasmus
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()
@ 2015-12-09 20:17 ` Rasmus Villemoes
0 siblings, 0 replies; 26+ messages in thread
From: Rasmus Villemoes @ 2015-12-09 20:17 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Joe Perches, Rob Herring, Andrew Morton, Masahiro Yamada,
devicetree@vger.kernel.org, Frank Rowand,
linux-kernel@vger.kernel.org, Grant Likely
On Wed, Dec 09 2015, Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, Dec 9, 2015 at 9:28 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
>> ---
>> include/linux/printk.h | 7 +++++++
>> lib/hexdump.c | 39 +++++++++++++++++++++++++++++++--------
>> 2 files changed, 38 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/linux/printk.h b/include/linux/printk.h
>> index 9729565..4be190c 100644
>> --- a/include/linux/printk.h
>> +++ b/include/linux/printk.h
>> @@ -424,6 +424,13 @@ enum {
>> DUMP_PREFIX_ADDRESS,
>> DUMP_PREFIX_OFFSET
>> };
>> +
>> +enum {
>> + DUMP_TYPE_CPU = 0,
>
> And still open this, do we need it? I think you may just mention in
> the documentation that default behaviour is CPU like.
I think it's best to have a name for the default. In this case it's
unlikely to ever be relevant, but in general one could imagine stuff
like
#ifdef THIS_OR_THAT
#define MY_DUMP_TYPE DUMP_TYPE_LE
#else
#define MY_DUMP_TYPE DUMP_TYPE_CPU
#endif
which is a lot more readable than if the latter was a naked 0.
The feature seems fine to me. It's always been somewhat annoying that
grouping changes the order of the byte values on little-endian machines
(so grouping is actually a wrong name for it; it's more like "treat the
bytes an array of u16/u32/u64"), and one can now get around that by
specifying DUMP_TYPE_BE.
Rasmus
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()
2015-12-09 20:17 ` Rasmus Villemoes
(?)
@ 2015-12-09 20:36 ` Andy Shevchenko
2015-12-09 20:42 ` Joe Perches
-1 siblings, 1 reply; 26+ messages in thread
From: Andy Shevchenko @ 2015-12-09 20:36 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Joe Perches, Rob Herring, Andrew Morton, Masahiro Yamada,
devicetree, Frank Rowand, linux-kernel, Grant Likely
On Wed, Dec 9, 2015 at 10:17 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On Wed, Dec 09 2015, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> On Wed, Dec 9, 2015 at 9:28 PM, Joe Perches <joe@perches.com> wrote:
>>> +enum {
>>> + DUMP_TYPE_CPU = 0,
>>
>> And still open this, do we need it? I think you may just mention in
>> the documentation that default behaviour is CPU like.
>
> I think it's best to have a name for the default. In this case it's
> unlikely to ever be relevant, but in general one could imagine stuff
> like
>
> #ifdef THIS_OR_THAT
> #define MY_DUMP_TYPE DUMP_TYPE_LE
> #else
> #define MY_DUMP_TYPE DUMP_TYPE_CPU
> #endif
>
> which is a lot more readable than if the latter was a naked 0.
Point taken.
Though _CPU suggests user to think if it's equivalent to BE or LE. I'm
wondering if _NATIVE is better?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()
2015-12-09 20:36 ` Andy Shevchenko
@ 2015-12-09 20:42 ` Joe Perches
0 siblings, 0 replies; 26+ messages in thread
From: Joe Perches @ 2015-12-09 20:42 UTC (permalink / raw)
To: Andy Shevchenko, Rasmus Villemoes
Cc: Rob Herring, Andrew Morton, Masahiro Yamada, devicetree,
Frank Rowand, linux-kernel, Grant Likely
On Wed, 2015-12-09 at 22:36 +0200, Andy Shevchenko wrote:
[]
> wondering if _NATIVE is better?
I think CPU is better myself and it's already
in use like:
include/linux/iio/iio.h-enum iio_endian {
include/linux/iio/iio.h: IIO_CPU,
include/linux/iio/iio.h- IIO_BE,
include/linux/iio/iio.h- IIO_LE,
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions
2015-12-08 21:28 ` Rob Herring
2015-12-09 0:03 ` Joe Perches
@ 2015-12-09 21:59 ` Joe Perches
2015-12-09 22:09 ` Andrew Morton
` (2 more replies)
1 sibling, 3 replies; 26+ messages in thread
From: Joe Perches @ 2015-12-09 21:59 UTC (permalink / raw)
To: Andrew Morton, linux-kernel
Cc: Andy Shevchenko, Rasmus Villemoes, Rob Herring
There are use cases for dumping buffers with specific endian types
for 2, 4, and 8 byte variables in arrays.
Add an enum for DUMP_TYPE_(LE|BE|CPU) to enable emitting them as such.
Rename groupsize to groupflags in the functions and add documentation
to the kernel-doc to describe the use of the DUMP_TYPE_ enum.
Signed-off-by: Joe Perches <joe@perches.com>
---
include/linux/printk.h | 23 +++++++++++++++--------
lib/hexdump.c | 49 ++++++++++++++++++++++++++++++++++++++-----------
2 files changed, 53 insertions(+), 19 deletions(-)
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 9729565..aac2943 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -424,12 +424,19 @@ enum {
DUMP_PREFIX_ADDRESS,
DUMP_PREFIX_OFFSET
};
+
+enum {
+ DUMP_TYPE_CPU = 0,
+ DUMP_TYPE_LE = BIT(30),
+ DUMP_TYPE_BE = BIT(31)
+};
+
extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
- int groupsize, char *linebuf, size_t linebuflen,
+ int groupflags, char *linebuf, size_t linebuflen,
bool ascii);
#ifdef CONFIG_PRINTK
extern void print_hex_dump(const char *level, const char *prefix_str,
- int prefix_type, int rowsize, int groupsize,
+ int prefix_type, int rowsize, int groupflags,
const void *buf, size_t len, bool ascii);
#if defined(CONFIG_DYNAMIC_DEBUG)
#define print_hex_dump_bytes(prefix_str, prefix_type, buf, len) \
@@ -440,7 +447,7 @@ extern void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
#endif /* defined(CONFIG_DYNAMIC_DEBUG) */
#else
static inline void print_hex_dump(const char *level, const char *prefix_str,
- int prefix_type, int rowsize, int groupsize,
+ int prefix_type, int rowsize, int groupflags,
const void *buf, size_t len, bool ascii)
{
}
@@ -453,17 +460,17 @@ static inline void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
#if defined(CONFIG_DYNAMIC_DEBUG)
#define print_hex_dump_debug(prefix_str, prefix_type, rowsize, \
- groupsize, buf, len, ascii) \
+ groupflags, buf, len, ascii) \
dynamic_hex_dump(prefix_str, prefix_type, rowsize, \
- groupsize, buf, len, ascii)
+ groupflags, buf, len, ascii)
#elif defined(DEBUG)
#define print_hex_dump_debug(prefix_str, prefix_type, rowsize, \
- groupsize, buf, len, ascii) \
+ groupflags, buf, len, ascii) \
print_hex_dump(KERN_DEBUG, prefix_str, prefix_type, rowsize, \
- groupsize, buf, len, ascii)
+ groupflags, buf, len, ascii)
#else
static inline void print_hex_dump_debug(const char *prefix_str, int prefix_type,
- int rowsize, int groupsize,
+ int rowsize, int groupflags,
const void *buf, size_t len, bool ascii)
{
}
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 992457b..f44608e 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -80,7 +80,9 @@ EXPORT_SYMBOL(bin2hex);
* @buf: data blob to dump
* @len: number of bytes in the @buf
* @rowsize: number of bytes to print per line; must be 16 or 32
- * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * use a bitwise OR with %DUMP_TYPE_BE or %DUMP_TYPE_LE for endian conversions
+ * of @buf, otherwise %DUMP_TYPE_CPU is used
* @linebuf: where to put the converted data
* @linebuflen: total size of @linebuf, including space for terminating NUL
* @ascii: include ASCII after the hex output
@@ -105,7 +107,7 @@ EXPORT_SYMBOL(bin2hex);
* (excluding the terminating NUL) which would have been written to the final
* string if enough space had been available.
*/
-int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
+int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupflags,
char *linebuf, size_t linebuflen, bool ascii)
{
const u8 *ptr = buf;
@@ -114,6 +116,7 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
int j, lx = 0;
int ascii_column;
int ret;
+ int groupsize = groupflags & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
if (rowsize != 16 && rowsize != 32)
rowsize = 16;
@@ -138,9 +141,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
const u64 *ptr8 = buf;
for (j = 0; j < ngroups; j++) {
+ u64 val;
+
+ if (groupflags & DUMP_TYPE_LE)
+ val = get_unaligned_le64(ptr8 + j);
+ else if (groupflags & DUMP_TYPE_BE)
+ val = get_unaligned_be64(ptr8 + j);
+ else
+ val = get_unaligned(ptr8 + j);
ret = snprintf(linebuf + lx, linebuflen - lx,
- "%s%16.16llx", j ? " " : "",
- get_unaligned(ptr8 + j));
+ "%s%16.16llx", j ? " " : "", val);
if (ret >= linebuflen - lx)
goto overflow1;
lx += ret;
@@ -149,9 +159,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
const u32 *ptr4 = buf;
for (j = 0; j < ngroups; j++) {
+ u32 val;
+
+ if (groupflags & DUMP_TYPE_LE)
+ val = get_unaligned_le32(ptr4 + j);
+ else if (groupflags & DUMP_TYPE_BE)
+ val = get_unaligned_be32(ptr4 + j);
+ else
+ val = get_unaligned(ptr4 + j);
ret = snprintf(linebuf + lx, linebuflen - lx,
- "%s%8.8x", j ? " " : "",
- get_unaligned(ptr4 + j));
+ "%s%8.8x", j ? " " : "", val);
if (ret >= linebuflen - lx)
goto overflow1;
lx += ret;
@@ -160,9 +177,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
const u16 *ptr2 = buf;
for (j = 0; j < ngroups; j++) {
+ u16 val;
+
+ if (groupflags & DUMP_TYPE_LE)
+ val = get_unaligned_le16(ptr2 + j);
+ else if (groupflags & DUMP_TYPE_BE)
+ val = get_unaligned_be16(ptr2 + j);
+ else
+ val = get_unaligned(ptr2 + j);
ret = snprintf(linebuf + lx, linebuflen - lx,
- "%s%4.4x", j ? " " : "",
- get_unaligned(ptr2 + j));
+ "%s%4.4x", j ? " " : "", val);
if (ret >= linebuflen - lx)
goto overflow1;
lx += ret;
@@ -216,7 +240,10 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
* @prefix_type: controls whether prefix of an offset, address, or none
* is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
* @rowsize: number of bytes to print per line; must be 16 or 32
- * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * use a bitwise OR with %DUMP_TYPE_BE or %DUMP_TYPE_LE for endian conversions
+ * of @buf, otherwise %DUMP_TYPE_CPU is used
* @buf: data blob to dump
* @len: number of bytes in the @buf
* @ascii: include ASCII after the hex output
@@ -240,7 +267,7 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
* ffffffff88089af0: 73727170 77767574 7b7a7978 7f7e7d7c pqrstuvwxyz{|}~.
*/
void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
- int rowsize, int groupsize,
+ int rowsize, int groupflags,
const void *buf, size_t len, bool ascii)
{
const u8 *ptr = buf;
@@ -254,7 +281,7 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
linelen = min(remaining, rowsize);
remaining -= rowsize;
- hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
+ hex_dump_to_buffer(ptr + i, linelen, rowsize, groupflags,
linebuf, sizeof(linebuf), ascii);
switch (prefix_type) {
--
2.6.3.368.gf34be46
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions
2015-12-09 21:59 ` [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions Joe Perches
@ 2015-12-09 22:09 ` Andrew Morton
2015-12-09 22:16 ` Joe Perches
2015-12-09 22:11 ` Andy Shevchenko
2015-12-10 5:55 ` kbuild test robot
2 siblings, 1 reply; 26+ messages in thread
From: Andrew Morton @ 2015-12-09 22:09 UTC (permalink / raw)
To: Joe Perches; +Cc: linux-kernel, Andy Shevchenko, Rasmus Villemoes, Rob Herring
On Wed, 9 Dec 2015 13:59:30 -0800 Joe Perches <joe@perches.com> wrote:
> There are use cases for dumping buffers with specific endian types
> for 2, 4, and 8 byte variables in arrays.
>
> Add an enum for DUMP_TYPE_(LE|BE|CPU) to enable emitting them as such.
>
> Rename groupsize to groupflags in the functions and add documentation
> to the kernel-doc to describe the use of the DUMP_TYPE_ enum.
What are these use cases? Will patches be forthcoming to convert
them?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions
2015-12-09 21:59 ` [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions Joe Perches
2015-12-09 22:09 ` Andrew Morton
@ 2015-12-09 22:11 ` Andy Shevchenko
2015-12-09 22:22 ` Joe Perches
2015-12-09 23:09 ` Andy Shevchenko
2015-12-10 5:55 ` kbuild test robot
2 siblings, 2 replies; 26+ messages in thread
From: Andy Shevchenko @ 2015-12-09 22:11 UTC (permalink / raw)
To: Joe Perches; +Cc: Andrew Morton, linux-kernel, Rasmus Villemoes, Rob Herring
On Wed, Dec 9, 2015 at 11:59 PM, Joe Perches <joe@perches.com> wrote:
> There are use cases for dumping buffers with specific endian types
> for 2, 4, and 8 byte variables in arrays.
>
> Add an enum for DUMP_TYPE_(LE|BE|CPU) to enable emitting them as such.
>
> Rename groupsize to groupflags in the functions and add documentation
> to the kernel-doc to describe the use of the DUMP_TYPE_ enum.
One comment below and my tag
Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
> include/linux/printk.h | 23 +++++++++++++++--------
> lib/hexdump.c | 49 ++++++++++++++++++++++++++++++++++++++-----------
> 2 files changed, 53 insertions(+), 19 deletions(-)
>
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 9729565..aac2943 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -424,12 +424,19 @@ enum {
> DUMP_PREFIX_ADDRESS,
> DUMP_PREFIX_OFFSET
> };
> +
> +enum {
> + DUMP_TYPE_CPU = 0,
> + DUMP_TYPE_LE = BIT(30),
> + DUMP_TYPE_BE = BIT(31)
> +};
> +
> extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
> - int groupsize, char *linebuf, size_t linebuflen,
> + int groupflags, char *linebuf, size_t linebuflen,
> bool ascii);
> #ifdef CONFIG_PRINTK
> extern void print_hex_dump(const char *level, const char *prefix_str,
> - int prefix_type, int rowsize, int groupsize,
> + int prefix_type, int rowsize, int groupflags,
> const void *buf, size_t len, bool ascii);
> #if defined(CONFIG_DYNAMIC_DEBUG)
> #define print_hex_dump_bytes(prefix_str, prefix_type, buf, len) \
> @@ -440,7 +447,7 @@ extern void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
> #endif /* defined(CONFIG_DYNAMIC_DEBUG) */
> #else
> static inline void print_hex_dump(const char *level, const char *prefix_str,
> - int prefix_type, int rowsize, int groupsize,
> + int prefix_type, int rowsize, int groupflags,
> const void *buf, size_t len, bool ascii)
> {
> }
> @@ -453,17 +460,17 @@ static inline void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
>
> #if defined(CONFIG_DYNAMIC_DEBUG)
> #define print_hex_dump_debug(prefix_str, prefix_type, rowsize, \
> - groupsize, buf, len, ascii) \
> + groupflags, buf, len, ascii) \
> dynamic_hex_dump(prefix_str, prefix_type, rowsize, \
> - groupsize, buf, len, ascii)
> + groupflags, buf, len, ascii)
> #elif defined(DEBUG)
> #define print_hex_dump_debug(prefix_str, prefix_type, rowsize, \
> - groupsize, buf, len, ascii) \
> + groupflags, buf, len, ascii) \
> print_hex_dump(KERN_DEBUG, prefix_str, prefix_type, rowsize, \
> - groupsize, buf, len, ascii)
> + groupflags, buf, len, ascii)
> #else
> static inline void print_hex_dump_debug(const char *prefix_str, int prefix_type,
> - int rowsize, int groupsize,
> + int rowsize, int groupflags,
> const void *buf, size_t len, bool ascii)
> {
> }
> diff --git a/lib/hexdump.c b/lib/hexdump.c
> index 992457b..f44608e 100644
> --- a/lib/hexdump.c
> +++ b/lib/hexdump.c
> @@ -80,7 +80,9 @@ EXPORT_SYMBOL(bin2hex);
> * @buf: data blob to dump
> * @len: number of bytes in the @buf
> * @rowsize: number of bytes to print per line; must be 16 or 32
> - * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> + * use a bitwise OR with %DUMP_TYPE_BE or %DUMP_TYPE_LE for endian conversions
> + * of @buf, otherwise %DUMP_TYPE_CPU is used
> * @linebuf: where to put the converted data
> * @linebuflen: total size of @linebuf, including space for terminating NUL
> * @ascii: include ASCII after the hex output
> @@ -105,7 +107,7 @@ EXPORT_SYMBOL(bin2hex);
> * (excluding the terminating NUL) which would have been written to the final
> * string if enough space had been available.
> */
> -int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> +int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupflags,
> char *linebuf, size_t linebuflen, bool ascii)
> {
> const u8 *ptr = buf;
> @@ -114,6 +116,7 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> int j, lx = 0;
> int ascii_column;
> int ret;
> + int groupsize = groupflags & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
>
> if (rowsize != 16 && rowsize != 32)
> rowsize = 16;
> @@ -138,9 +141,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> const u64 *ptr8 = buf;
>
> for (j = 0; j < ngroups; j++) {
> + u64 val;
> +
> + if (groupflags & DUMP_TYPE_LE)
> + val = get_unaligned_le64(ptr8 + j);
> + else if (groupflags & DUMP_TYPE_BE)
> + val = get_unaligned_be64(ptr8 + j);
> + else
> + val = get_unaligned(ptr8 + j);
> ret = snprintf(linebuf + lx, linebuflen - lx,
> - "%s%16.16llx", j ? " " : "",
> - get_unaligned(ptr8 + j));
> + "%s%16.16llx", j ? " " : "", val);
> if (ret >= linebuflen - lx)
> goto overflow1;
> lx += ret;
> @@ -149,9 +159,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> const u32 *ptr4 = buf;
>
> for (j = 0; j < ngroups; j++) {
> + u32 val;
> +
> + if (groupflags & DUMP_TYPE_LE)
> + val = get_unaligned_le32(ptr4 + j);
> + else if (groupflags & DUMP_TYPE_BE)
> + val = get_unaligned_be32(ptr4 + j);
> + else
> + val = get_unaligned(ptr4 + j);
> ret = snprintf(linebuf + lx, linebuflen - lx,
> - "%s%8.8x", j ? " " : "",
> - get_unaligned(ptr4 + j));
> + "%s%8.8x", j ? " " : "", val);
> if (ret >= linebuflen - lx)
> goto overflow1;
> lx += ret;
> @@ -160,9 +177,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> const u16 *ptr2 = buf;
>
> for (j = 0; j < ngroups; j++) {
> + u16 val;
> +
> + if (groupflags & DUMP_TYPE_LE)
> + val = get_unaligned_le16(ptr2 + j);
> + else if (groupflags & DUMP_TYPE_BE)
> + val = get_unaligned_be16(ptr2 + j);
> + else
> + val = get_unaligned(ptr2 + j);
> ret = snprintf(linebuf + lx, linebuflen - lx,
> - "%s%4.4x", j ? " " : "",
> - get_unaligned(ptr2 + j));
> + "%s%4.4x", j ? " " : "", val);
> if (ret >= linebuflen - lx)
> goto overflow1;
> lx += ret;
> @@ -216,7 +240,10 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
> * @prefix_type: controls whether prefix of an offset, address, or none
> * is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
> * @rowsize: number of bytes to print per line; must be 16 or 32
> - * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
Duplicate.
> + * use a bitwise OR with %DUMP_TYPE_BE or %DUMP_TYPE_LE for endian conversions
> + * of @buf, otherwise %DUMP_TYPE_CPU is used
> * @buf: data blob to dump
> * @len: number of bytes in the @buf
> * @ascii: include ASCII after the hex output
> @@ -240,7 +267,7 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
> * ffffffff88089af0: 73727170 77767574 7b7a7978 7f7e7d7c pqrstuvwxyz{|}~.
> */
> void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
> - int rowsize, int groupsize,
> + int rowsize, int groupflags,
> const void *buf, size_t len, bool ascii)
> {
> const u8 *ptr = buf;
> @@ -254,7 +281,7 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
> linelen = min(remaining, rowsize);
> remaining -= rowsize;
>
> - hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
> + hex_dump_to_buffer(ptr + i, linelen, rowsize, groupflags,
> linebuf, sizeof(linebuf), ascii);
>
> switch (prefix_type) {
> --
> 2.6.3.368.gf34be46
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions
2015-12-09 22:09 ` Andrew Morton
@ 2015-12-09 22:16 ` Joe Perches
2015-12-09 22:45 ` Andrew Morton
0 siblings, 1 reply; 26+ messages in thread
From: Joe Perches @ 2015-12-09 22:16 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, Andy Shevchenko, Rasmus Villemoes, Rob Herring
On Wed, 2015-12-09 at 14:09 -0800, Andrew Morton wrote:
> On Wed, 9 Dec 2015 13:59:30 -0800 Joe Perches <joe@perches.com> wrote:
>
> > There are use cases for dumping buffers with specific endian types
> > for 2, 4, and 8 byte variables in arrays.
> >
> > Add an enum for DUMP_TYPE_(LE|BE|CPU) to enable emitting them as such.
> >
> > Rename groupsize to groupflags in the functions and add documentation
> > to the kernel-doc to describe the use of the DUMP_TYPE_ enum.
>
> What are these use cases?
Buffers that contain LE or BE arrays that want
to be emitted by print_hex_dump.
> Will patches be forthcoming to convert them?
Sure, after a settling period for Masahiro's patch
to be applied upstream.
https://lkml.org/lkml/2015/12/8/480
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions
2015-12-09 22:11 ` Andy Shevchenko
@ 2015-12-09 22:22 ` Joe Perches
2015-12-09 23:09 ` Andy Shevchenko
1 sibling, 0 replies; 26+ messages in thread
From: Joe Perches @ 2015-12-09 22:22 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andrew Morton, linux-kernel, Rasmus Villemoes, Rob Herring
On Thu, 2015-12-10 at 00:11 +0200, Andy Shevchenko wrote:
> On Wed, Dec 9, 2015 at 11:59 PM, Joe Perches <joe@perches.com> wrote:
[]
> > diff --git a/lib/hexdump.c b/lib/hexdump.c
[]
> > @@ -216,7 +240,10 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
> > * @prefix_type: controls whether prefix of an offset, address, or none
> > * is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
> > * @rowsize: number of bytes to print per line; must be 16 or 32
> > - * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> > + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> > + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
>
> Duplicate.
Ick. Andrew, can you delete one line please or do you want a resend?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions
2015-12-09 22:16 ` Joe Perches
@ 2015-12-09 22:45 ` Andrew Morton
2015-12-09 23:02 ` Joe Perches
0 siblings, 1 reply; 26+ messages in thread
From: Andrew Morton @ 2015-12-09 22:45 UTC (permalink / raw)
To: Joe Perches; +Cc: linux-kernel, Andy Shevchenko, Rasmus Villemoes, Rob Herring
On Wed, 09 Dec 2015 14:16:36 -0800 Joe Perches <joe@perches.com> wrote:
> On Wed, 2015-12-09 at 14:09 -0800, Andrew Morton wrote:
> > On Wed,____9 Dec 2015 13:59:30 -0800 Joe Perches <joe@perches.com> wrote:
> >
> > > There are use cases for dumping buffers with specific endian types
> > > for 2, 4, and 8 byte variables in arrays.
> > >
> > > Add an enum for DUMP_TYPE_(LE|BE|CPU) to enable emitting them as such.
> > >
> > > Rename groupsize to groupflags in the functions and add documentation
> > > to the kernel-doc to describe the use of the DUMP_TYPE_ enum.
> >
> > What are these use cases?
>
> Buffers that contain LE or BE arrays that want
> to be emitted by print_hex_dump.
Make that "where are".
> > Will patches be forthcoming to convert them?
>
> Sure, after a settling period for Masahiro's patch
> to be applied upstream.
>
> https://lkml.org/lkml/2015/12/8/480
One single callsite under #ifdef DEBUG? Doesn't seem to warrant adding
code and complexity to core library functions.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions
2015-12-09 22:45 ` Andrew Morton
@ 2015-12-09 23:02 ` Joe Perches
0 siblings, 0 replies; 26+ messages in thread
From: Joe Perches @ 2015-12-09 23:02 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, Andy Shevchenko, Rasmus Villemoes, Rob Herring
On Wed, 2015-12-09 at 14:45 -0800, Andrew Morton wrote:
> On Wed, 09 Dec 2015 14:16:36 -0800 Joe Perches <joe@perches.com> wrote:
> > > Will patches be forthcoming to convert them?
> > Sure, after a settling period for Masahiro's patch
> > to be applied upstream.
> >
> > https://lkml.org/lkml/2015/12/8/480
>
> One single callsite under #ifdef DEBUG? Doesn't seem to warrant adding
> code and complexity to core library functions.
It was just a for-instance.
It's pretty trivial code and it's quite likely there
are other buffers that could be emitted that way.
e.g.: look at code like:
drivers/message/fusion/mptdebug.h
drivers/infiniband/hw/mthca/mthca_mr.c
Dunno, I didn't look very hard.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions
2015-12-09 22:11 ` Andy Shevchenko
2015-12-09 22:22 ` Joe Perches
@ 2015-12-09 23:09 ` Andy Shevchenko
2015-12-09 23:15 ` Joe Perches
1 sibling, 1 reply; 26+ messages in thread
From: Andy Shevchenko @ 2015-12-09 23:09 UTC (permalink / raw)
To: Joe Perches; +Cc: Andrew Morton, linux-kernel, Rasmus Villemoes, Rob Herring
On Thu, Dec 10, 2015 at 12:11 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Dec 9, 2015 at 11:59 PM, Joe Perches <joe@perches.com> wrote:
>> There are use cases for dumping buffers with specific endian types
>> for 2, 4, and 8 byte variables in arrays.
>>
>> Add an enum for DUMP_TYPE_(LE|BE|CPU) to enable emitting them as such.
>>
>> Rename groupsize to groupflags in the functions and add documentation
>> to the kernel-doc to describe the use of the DUMP_TYPE_ enum.
>
> One comment below and my tag
> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Couple of additional notes. (Tag still in power)
1. There is similar to print_hex_dump() function in seq_file. Separate patch?
2. What to do with ASCII part? Shall we print it as byte stream?
And one more style comment below.
>
>>
>> Signed-off-by: Joe Perches <joe@perches.com>
>> ---
>> include/linux/printk.h | 23 +++++++++++++++--------
>> lib/hexdump.c | 49 ++++++++++++++++++++++++++++++++++++++-----------
>> 2 files changed, 53 insertions(+), 19 deletions(-)
>>
>> diff --git a/include/linux/printk.h b/include/linux/printk.h
>
>> index 9729565..aac2943 100644
>> --- a/include/linux/printk.h
>> +++ b/include/linux/printk.h
>> @@ -424,12 +424,19 @@ enum {
>> DUMP_PREFIX_ADDRESS,
>> DUMP_PREFIX_OFFSET
>> };
>> +
>> +enum {
>> + DUMP_TYPE_CPU = 0,
>> + DUMP_TYPE_LE = BIT(30),
>> + DUMP_TYPE_BE = BIT(31)
>> +};
>> +
>> extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
>> - int groupsize, char *linebuf, size_t linebuflen,
>> + int groupflags, char *linebuf, size_t linebuflen,
>> bool ascii);
>> #ifdef CONFIG_PRINTK
>> extern void print_hex_dump(const char *level, const char *prefix_str,
>> - int prefix_type, int rowsize, int groupsize,
>> + int prefix_type, int rowsize, int groupflags,
>> const void *buf, size_t len, bool ascii);
>> #if defined(CONFIG_DYNAMIC_DEBUG)
>> #define print_hex_dump_bytes(prefix_str, prefix_type, buf, len) \
>> @@ -440,7 +447,7 @@ extern void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
>> #endif /* defined(CONFIG_DYNAMIC_DEBUG) */
>> #else
>> static inline void print_hex_dump(const char *level, const char *prefix_str,
>> - int prefix_type, int rowsize, int groupsize,
>> + int prefix_type, int rowsize, int groupflags,
>> const void *buf, size_t len, bool ascii)
>> {
>> }
>> @@ -453,17 +460,17 @@ static inline void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
>>
>> #if defined(CONFIG_DYNAMIC_DEBUG)
>> #define print_hex_dump_debug(prefix_str, prefix_type, rowsize, \
>> - groupsize, buf, len, ascii) \
>> + groupflags, buf, len, ascii) \
>> dynamic_hex_dump(prefix_str, prefix_type, rowsize, \
>> - groupsize, buf, len, ascii)
>> + groupflags, buf, len, ascii)
>> #elif defined(DEBUG)
>> #define print_hex_dump_debug(prefix_str, prefix_type, rowsize, \
>> - groupsize, buf, len, ascii) \
>> + groupflags, buf, len, ascii) \
>> print_hex_dump(KERN_DEBUG, prefix_str, prefix_type, rowsize, \
>> - groupsize, buf, len, ascii)
>> + groupflags, buf, len, ascii)
>> #else
>> static inline void print_hex_dump_debug(const char *prefix_str, int prefix_type,
>> - int rowsize, int groupsize,
>> + int rowsize, int groupflags,
>> const void *buf, size_t len, bool ascii)
>> {
>> }
>> diff --git a/lib/hexdump.c b/lib/hexdump.c
>> index 992457b..f44608e 100644
>> --- a/lib/hexdump.c
>> +++ b/lib/hexdump.c
>> @@ -80,7 +80,9 @@ EXPORT_SYMBOL(bin2hex);
>> * @buf: data blob to dump
>> * @len: number of bytes in the @buf
>> * @rowsize: number of bytes to print per line; must be 16 or 32
>> - * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
>> + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
>> + * use a bitwise OR with %DUMP_TYPE_BE or %DUMP_TYPE_LE for endian conversions
>> + * of @buf, otherwise %DUMP_TYPE_CPU is used
>> * @linebuf: where to put the converted data
>> * @linebuflen: total size of @linebuf, including space for terminating NUL
>> * @ascii: include ASCII after the hex output
>> @@ -105,7 +107,7 @@ EXPORT_SYMBOL(bin2hex);
>> * (excluding the terminating NUL) which would have been written to the final
>> * string if enough space had been available.
>> */
>> -int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>> +int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupflags,
>> char *linebuf, size_t linebuflen, bool ascii)
>> {
>> const u8 *ptr = buf;
>> @@ -114,6 +116,7 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>> int j, lx = 0;
>> int ascii_column;
>> int ret;
>> + int groupsize = groupflags & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
Can you move this upper to first or second position in the definition block?
>>
>> if (rowsize != 16 && rowsize != 32)
>> rowsize = 16;
>> @@ -138,9 +141,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>> const u64 *ptr8 = buf;
>>
>> for (j = 0; j < ngroups; j++) {
>> + u64 val;
>> +
>> + if (groupflags & DUMP_TYPE_LE)
>> + val = get_unaligned_le64(ptr8 + j);
>> + else if (groupflags & DUMP_TYPE_BE)
>> + val = get_unaligned_be64(ptr8 + j);
>> + else
>> + val = get_unaligned(ptr8 + j);
>> ret = snprintf(linebuf + lx, linebuflen - lx,
>> - "%s%16.16llx", j ? " " : "",
>> - get_unaligned(ptr8 + j));
>> + "%s%16.16llx", j ? " " : "", val);
>> if (ret >= linebuflen - lx)
>> goto overflow1;
>> lx += ret;
>> @@ -149,9 +159,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>> const u32 *ptr4 = buf;
>>
>> for (j = 0; j < ngroups; j++) {
>> + u32 val;
>> +
>> + if (groupflags & DUMP_TYPE_LE)
>> + val = get_unaligned_le32(ptr4 + j);
>> + else if (groupflags & DUMP_TYPE_BE)
>> + val = get_unaligned_be32(ptr4 + j);
>> + else
>> + val = get_unaligned(ptr4 + j);
>> ret = snprintf(linebuf + lx, linebuflen - lx,
>> - "%s%8.8x", j ? " " : "",
>> - get_unaligned(ptr4 + j));
>> + "%s%8.8x", j ? " " : "", val);
>> if (ret >= linebuflen - lx)
>> goto overflow1;
>> lx += ret;
>> @@ -160,9 +177,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>> const u16 *ptr2 = buf;
>>
>> for (j = 0; j < ngroups; j++) {
>> + u16 val;
>> +
>> + if (groupflags & DUMP_TYPE_LE)
>> + val = get_unaligned_le16(ptr2 + j);
>> + else if (groupflags & DUMP_TYPE_BE)
>> + val = get_unaligned_be16(ptr2 + j);
>> + else
>> + val = get_unaligned(ptr2 + j);
>> ret = snprintf(linebuf + lx, linebuflen - lx,
>> - "%s%4.4x", j ? " " : "",
>> - get_unaligned(ptr2 + j));
>> + "%s%4.4x", j ? " " : "", val);
>> if (ret >= linebuflen - lx)
>> goto overflow1;
>> lx += ret;
>> @@ -216,7 +240,10 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
>> * @prefix_type: controls whether prefix of an offset, address, or none
>> * is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
>> * @rowsize: number of bytes to print per line; must be 16 or 32
>> - * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
>> + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
>
>> + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
>
> Duplicate.
>
>> + * use a bitwise OR with %DUMP_TYPE_BE or %DUMP_TYPE_LE for endian conversions
>> + * of @buf, otherwise %DUMP_TYPE_CPU is used
>> * @buf: data blob to dump
>> * @len: number of bytes in the @buf
>> * @ascii: include ASCII after the hex output
>> @@ -240,7 +267,7 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
>> * ffffffff88089af0: 73727170 77767574 7b7a7978 7f7e7d7c pqrstuvwxyz{|}~.
>> */
>> void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
>> - int rowsize, int groupsize,
>> + int rowsize, int groupflags,
>> const void *buf, size_t len, bool ascii)
>> {
>> const u8 *ptr = buf;
>> @@ -254,7 +281,7 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
>> linelen = min(remaining, rowsize);
>> remaining -= rowsize;
>>
>> - hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
>> + hex_dump_to_buffer(ptr + i, linelen, rowsize, groupflags,
>> linebuf, sizeof(linebuf), ascii);
>>
>> switch (prefix_type) {
>> --
>> 2.6.3.368.gf34be46
>>
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions
2015-12-09 23:09 ` Andy Shevchenko
@ 2015-12-09 23:15 ` Joe Perches
0 siblings, 0 replies; 26+ messages in thread
From: Joe Perches @ 2015-12-09 23:15 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andrew Morton, linux-kernel, Rasmus Villemoes, Rob Herring
On Thu, 2015-12-10 at 01:09 +0200, Andy Shevchenko wrote:
> On Thu, Dec 10, 2015 at 12:11 AM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Wed, Dec 9, 2015 at 11:59 PM, Joe Perches <joe@perches.com> wrote:
> > > There are use cases for dumping buffers with specific endian types
> > > for 2, 4, and 8 byte variables in arrays.
> > >
> > > Add an enum for DUMP_TYPE_(LE|BE|CPU) to enable emitting them as such.
> > >
> > > Rename groupsize to groupflags in the functions and add documentation
> > > to the kernel-doc to describe the use of the DUMP_TYPE_ enum.
> >
> > One comment below and my tag
> > Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
> Couple of additional notes. (Tag still in power)
>
> 1. There is similar to print_hex_dump() function in seq_file. Separate patch?
Let's see if anyone wants this one first.
> 2. What to do with ASCII part? Shall we print it as byte stream?
It's still emitted as a byte stream.
> And one more style comment below.
That one's a nit I don't care about.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions
2015-12-09 21:59 ` [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions Joe Perches
2015-12-09 22:09 ` Andrew Morton
2015-12-09 22:11 ` Andy Shevchenko
@ 2015-12-10 5:55 ` kbuild test robot
2015-12-10 6:20 ` Joe Perches
2 siblings, 1 reply; 26+ messages in thread
From: kbuild test robot @ 2015-12-10 5:55 UTC (permalink / raw)
To: Joe Perches
Cc: kbuild-all, Andrew Morton, linux-kernel, Andy Shevchenko,
Rasmus Villemoes, Rob Herring
Hi Joe,
[auto build test WARNING on v4.4-rc4]
[also build test WARNING on next-20151209]
url: https://github.com/0day-ci/linux/commits/Joe-Perches/hexdump-Add-ability-to-do-endian-conversions-in-print_hex_dump-functions/20151210-060244
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
include/linux/compiler.h:228:8: sparse: attribute 'no_sanitize_address': unknown attribute
include/linux/printk.h:430:24: sparse: undefined identifier 'BIT'
>> include/linux/printk.h:430:27: sparse: bad constant expression type
include/linux/printk.h:431:24: sparse: undefined identifier 'BIT'
include/linux/printk.h:431:27: sparse: bad constant expression type
In file included from lib/decompress.c:19:0:
include/linux/printk.h:430:17: error: implicit declaration of function 'BIT' [-Werror=implicit-function-declaration]
DUMP_TYPE_LE = BIT(30),
^
include/linux/printk.h:430:2: error: enumerator value for 'DUMP_TYPE_LE' is not an integer constant
DUMP_TYPE_LE = BIT(30),
^
include/linux/printk.h:432:1: error: enumerator value for 'DUMP_TYPE_BE' is not an integer constant
};
^
cc1: some warnings being treated as errors
vim +430 include/linux/printk.h
414 printk_ratelimited(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
415 #else
416 #define pr_debug_ratelimited(fmt, ...) \
417 no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
418 #endif
419
420 extern const struct file_operations kmsg_fops;
421
422 enum {
423 DUMP_PREFIX_NONE,
424 DUMP_PREFIX_ADDRESS,
425 DUMP_PREFIX_OFFSET
426 };
427
428 enum {
429 DUMP_TYPE_CPU = 0,
> 430 DUMP_TYPE_LE = BIT(30),
431 DUMP_TYPE_BE = BIT(31)
432 };
433
434 extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
435 int groupflags, char *linebuf, size_t linebuflen,
436 bool ascii);
437 #ifdef CONFIG_PRINTK
438 extern void print_hex_dump(const char *level, const char *prefix_str,
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions
2015-12-10 5:55 ` kbuild test robot
@ 2015-12-10 6:20 ` Joe Perches
0 siblings, 0 replies; 26+ messages in thread
From: Joe Perches @ 2015-12-10 6:20 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all, Andrew Morton, linux-kernel, Andy Shevchenko,
Rasmus Villemoes, Rob Herring
On Thu, 2015-12-10 at 13:55 +0800, kbuild test robot wrote:
> Hi Joe,
Hello Fengguang.
Thanks for the report. I'll fix and resubmit.
> > > include/linux/printk.h:430:24: sparse: undefined identifier 'BIT'
> > > include/linux/printk.h:430:27: sparse: bad constant expression type
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2015-12-10 6:20 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-08 16:07 [PATCH v2] of/address: replace printk() with pr_debug() / pr_err() Masahiro Yamada
2015-12-08 16:16 ` Joe Perches
2015-12-08 17:03 ` Joe Perches
2015-12-08 21:28 ` Rob Herring
2015-12-09 0:03 ` Joe Perches
2015-12-09 12:03 ` Andy Shevchenko
2015-12-09 12:03 ` Andy Shevchenko
2015-12-09 19:28 ` Joe Perches
2015-12-09 20:02 ` Andy Shevchenko
2015-12-09 20:02 ` Andy Shevchenko
2015-12-09 20:09 ` Joe Perches
2015-12-09 20:17 ` Rasmus Villemoes
2015-12-09 20:17 ` Rasmus Villemoes
2015-12-09 20:36 ` Andy Shevchenko
2015-12-09 20:42 ` Joe Perches
2015-12-09 21:59 ` [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions Joe Perches
2015-12-09 22:09 ` Andrew Morton
2015-12-09 22:16 ` Joe Perches
2015-12-09 22:45 ` Andrew Morton
2015-12-09 23:02 ` Joe Perches
2015-12-09 22:11 ` Andy Shevchenko
2015-12-09 22:22 ` Joe Perches
2015-12-09 23:09 ` Andy Shevchenko
2015-12-09 23:15 ` Joe Perches
2015-12-10 5:55 ` kbuild test robot
2015-12-10 6:20 ` Joe Perches
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.