* [PATCH] staging: pi433: add rf69_dbg_hex function
@ 2022-02-11 8:07 Paulo Miguel Almeida
2022-02-11 8:14 ` Greg KH
2022-02-11 14:25 ` Dan Carpenter
0 siblings, 2 replies; 6+ messages in thread
From: Paulo Miguel Almeida @ 2022-02-11 8:07 UTC (permalink / raw)
To: gregkh, paulo.miguel.almeida.rodenas, realwakka
Cc: linux-staging, linux-kernel
dev_<level> functions don't support printing hex dumps and the
alternative available (print_hex_dump_debug) doesn't print the device
information such as device's driver name and device name. That type of
information which comes in handy for situations in which you can more
than 1 device attached at the same type.
this patch adds a utility function that can obtain the same result as
print_hex_dump_debug while being able to honour all possible flags that
one may be interested in when dynamic debug is used.
Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
Meta-comments:
the initial discussion to use print_hex_dump_debug started in this patch
but the original idea got merged into the brach.
https://lore.kernel.org/lkml/a630d8381cee0f543e0d77614052e1d04ab162a5.camel@perches.com/#t
---
drivers/staging/pi433/rf69.c | 39 ++++++++++++++++++++++++++++--------
1 file changed, 31 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index 901f8db3e3ce..82d4ba24c35f 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -822,9 +822,37 @@ int rf69_set_dagc(struct spi_device *spi, enum dagc dagc)
/*-------------------------------------------------------------------------*/
+static void rf69_dbg_hex(struct spi_device *spi, u8 *buf, unsigned int size,
+ const char *fmt, ...)
+{
+ va_list args;
+ char textbuf[512] = {};
+ char *text = textbuf;
+ int text_pos;
+
+ int rowsize = 16;
+ int i, linelen, remaining = size;
+
+ va_start(args, fmt);
+ text_pos = vscnprintf(text, sizeof(textbuf), fmt, args);
+ text += text_pos;
+ va_end(args);
+
+ for (i = 0; i < size; i += rowsize) {
+ linelen = min(remaining, rowsize);
+ remaining -= rowsize;
+
+ hex_dump_to_buffer(buf + i, linelen, rowsize, 1,
+ text, sizeof(textbuf) - text_pos, false);
+
+ dev_dbg(&spi->dev, "%s\n", textbuf);
+
+ memset(text, 0, sizeof(textbuf) - text_pos);
+ }
+}
+
int rf69_read_fifo(struct spi_device *spi, u8 *buffer, unsigned int size)
{
- int i;
struct spi_transfer transfer;
u8 local_buffer[FIFO_SIZE + 1];
int retval;
@@ -844,9 +872,7 @@ int rf69_read_fifo(struct spi_device *spi, u8 *buffer, unsigned int size)
retval = spi_sync_transfer(spi, &transfer, 1);
- /* print content read from fifo for debugging purposes */
- for (i = 0; i < size; i++)
- dev_dbg(&spi->dev, "%d - 0x%x\n", i, local_buffer[i + 1]);
+ rf69_dbg_hex(spi, local_buffer + 1, size, "%s - ", __func__);
memcpy(buffer, &local_buffer[1], size);
@@ -855,7 +881,6 @@ int rf69_read_fifo(struct spi_device *spi, u8 *buffer, unsigned int size)
int rf69_write_fifo(struct spi_device *spi, u8 *buffer, unsigned int size)
{
- int i;
u8 local_buffer[FIFO_SIZE + 1];
if (size > FIFO_SIZE) {
@@ -867,9 +892,7 @@ int rf69_write_fifo(struct spi_device *spi, u8 *buffer, unsigned int size)
local_buffer[0] = REG_FIFO | WRITE_BIT;
memcpy(&local_buffer[1], buffer, size);
- /* print content written from fifo for debugging purposes */
- for (i = 0; i < size; i++)
- dev_dbg(&spi->dev, "0x%x\n", buffer[i]);
+ rf69_dbg_hex(spi, local_buffer + 1, size, "%s - ", __func__);
return spi_write(spi, local_buffer, size + 1);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: pi433: add rf69_dbg_hex function
2022-02-11 8:07 [PATCH] staging: pi433: add rf69_dbg_hex function Paulo Miguel Almeida
@ 2022-02-11 8:14 ` Greg KH
2022-02-11 19:39 ` Paulo Miguel Almeida
2022-02-11 14:25 ` Dan Carpenter
1 sibling, 1 reply; 6+ messages in thread
From: Greg KH @ 2022-02-11 8:14 UTC (permalink / raw)
To: Paulo Miguel Almeida; +Cc: realwakka, linux-staging, linux-kernel
On Fri, Feb 11, 2022 at 09:07:32PM +1300, Paulo Miguel Almeida wrote:
> dev_<level> functions don't support printing hex dumps and the
> alternative available (print_hex_dump_debug) doesn't print the device
> information such as device's driver name and device name. That type of
> information which comes in handy for situations in which you can more
> than 1 device attached at the same type.
>
> this patch adds a utility function that can obtain the same result as
> print_hex_dump_debug while being able to honour all possible flags that
> one may be interested in when dynamic debug is used.
>
> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> ---
> Meta-comments:
>
> the initial discussion to use print_hex_dump_debug started in this patch
> but the original idea got merged into the brach.
>
> https://lore.kernel.org/lkml/a630d8381cee0f543e0d77614052e1d04ab162a5.camel@perches.com/#t
>
> ---
> drivers/staging/pi433/rf69.c | 39 ++++++++++++++++++++++++++++--------
> 1 file changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index 901f8db3e3ce..82d4ba24c35f 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -822,9 +822,37 @@ int rf69_set_dagc(struct spi_device *spi, enum dagc dagc)
>
> /*-------------------------------------------------------------------------*/
>
> +static void rf69_dbg_hex(struct spi_device *spi, u8 *buf, unsigned int size,
> + const char *fmt, ...)
> +{
> + va_list args;
> + char textbuf[512] = {};
> + char *text = textbuf;
> + int text_pos;
> +
> + int rowsize = 16;
> + int i, linelen, remaining = size;
> +
> + va_start(args, fmt);
> + text_pos = vscnprintf(text, sizeof(textbuf), fmt, args);
> + text += text_pos;
> + va_end(args);
> +
> + for (i = 0; i < size; i += rowsize) {
> + linelen = min(remaining, rowsize);
> + remaining -= rowsize;
> +
> + hex_dump_to_buffer(buf + i, linelen, rowsize, 1,
> + text, sizeof(textbuf) - text_pos, false);
> +
> + dev_dbg(&spi->dev, "%s\n", textbuf);
> +
> + memset(text, 0, sizeof(textbuf) - text_pos);
> + }
> +}
This is a lot of additional complexity for almost no real benefit.
> +
> int rf69_read_fifo(struct spi_device *spi, u8 *buffer, unsigned int size)
> {
> - int i;
> struct spi_transfer transfer;
> u8 local_buffer[FIFO_SIZE + 1];
> int retval;
> @@ -844,9 +872,7 @@ int rf69_read_fifo(struct spi_device *spi, u8 *buffer, unsigned int size)
>
> retval = spi_sync_transfer(spi, &transfer, 1);
>
> - /* print content read from fifo for debugging purposes */
> - for (i = 0; i < size; i++)
> - dev_dbg(&spi->dev, "%d - 0x%x\n", i, local_buffer[i + 1]);
What is wrong with this simple line?
> + rf69_dbg_hex(spi, local_buffer + 1, size, "%s - ", __func__);
>
> memcpy(buffer, &local_buffer[1], size);
>
> @@ -855,7 +881,6 @@ int rf69_read_fifo(struct spi_device *spi, u8 *buffer, unsigned int size)
>
> int rf69_write_fifo(struct spi_device *spi, u8 *buffer, unsigned int size)
> {
> - int i;
> u8 local_buffer[FIFO_SIZE + 1];
>
> if (size > FIFO_SIZE) {
> @@ -867,9 +892,7 @@ int rf69_write_fifo(struct spi_device *spi, u8 *buffer, unsigned int size)
> local_buffer[0] = REG_FIFO | WRITE_BIT;
> memcpy(&local_buffer[1], buffer, size);
>
> - /* print content written from fifo for debugging purposes */
> - for (i = 0; i < size; i++)
> - dev_dbg(&spi->dev, "0x%x\n", buffer[i]);
> + rf69_dbg_hex(spi, local_buffer + 1, size, "%s - ", __func__);
Again, the original is fine here, why make this so complex?
Also, you are using local_buffer here, not buffer, why?
I think the original is just fine, no need to polish something as tiny
as a hex dump for debugging only.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: pi433: add rf69_dbg_hex function
2022-02-11 8:07 [PATCH] staging: pi433: add rf69_dbg_hex function Paulo Miguel Almeida
2022-02-11 8:14 ` Greg KH
@ 2022-02-11 14:25 ` Dan Carpenter
2022-02-11 19:22 ` Paulo Miguel Almeida
1 sibling, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2022-02-11 14:25 UTC (permalink / raw)
To: Paulo Miguel Almeida; +Cc: gregkh, realwakka, linux-staging, linux-kernel
On Fri, Feb 11, 2022 at 09:07:32PM +1300, Paulo Miguel Almeida wrote:
> dev_<level> functions don't support printing hex dumps and the
> alternative available (print_hex_dump_debug) doesn't print the device
> information such as device's driver name and device name. That type of
> information which comes in handy for situations in which you can more
> than 1 device attached at the same type.
>
> this patch adds a utility function that can obtain the same result as
> print_hex_dump_debug while being able to honour all possible flags that
> one may be interested in when dynamic debug is used.
>
> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> ---
I feel like this patch is over engineering your debug code. Is this
really worthwhile? If you really prefer the new format that's fine but
it seems like not necessarily a good use of energy.
> Meta-comments:
>
> the initial discussion to use print_hex_dump_debug started in this patch
> but the original idea got merged into the brach.
>
> https://lore.kernel.org/lkml/a630d8381cee0f543e0d77614052e1d04ab162a5.camel@perches.com/#t
>
> ---
> drivers/staging/pi433/rf69.c | 39 ++++++++++++++++++++++++++++--------
> 1 file changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index 901f8db3e3ce..82d4ba24c35f 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -822,9 +822,37 @@ int rf69_set_dagc(struct spi_device *spi, enum dagc dagc)
>
> /*-------------------------------------------------------------------------*/
>
> +static void rf69_dbg_hex(struct spi_device *spi, u8 *buf, unsigned int size,
> + const char *fmt, ...)
> +{
> + va_list args;
> + char textbuf[512] = {};
No need to initialize this.
> + char *text = textbuf;
> + int text_pos;
> +
Don't put a blank line in the middle of the declaration block.
> + int rowsize = 16;
> + int i, linelen, remaining = size;
> +
> + va_start(args, fmt);
> + text_pos = vscnprintf(text, sizeof(textbuf), fmt, args);
> + text += text_pos;
When you're printing to a buffer like this then I find it's easier to
leave the start of the buffer constant. So get rid of the text pointer
and just use "textbuf + prefix_len" instead of having a "text" pointer.
> + va_end(args);
> +
> + for (i = 0; i < size; i += rowsize) {
> + linelen = min(remaining, rowsize);
> + remaining -= rowsize;
> +
> + hex_dump_to_buffer(buf + i, linelen, rowsize, 1,
> + text, sizeof(textbuf) - text_pos, false);
> +
> + dev_dbg(&spi->dev, "%s\n", textbuf);
> +
> + memset(text, 0, sizeof(textbuf) - text_pos);
No need for this.
> + }
Instead of printing lineline at a time in a loop, what happens if you
just print size bytes? (I honestly don't know because I have never used
this function before).
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: pi433: add rf69_dbg_hex function
2022-02-11 14:25 ` Dan Carpenter
@ 2022-02-11 19:22 ` Paulo Miguel Almeida
0 siblings, 0 replies; 6+ messages in thread
From: Paulo Miguel Almeida @ 2022-02-11 19:22 UTC (permalink / raw)
To: Dan Carpenter; +Cc: gregkh, realwakka, linux-staging, linux-kernel
thanks for taking the time to review my patch
On Fri, Feb 11, 2022 at 05:25:28PM +0300, Dan Carpenter wrote:
>
> I feel like this patch is over engineering your debug code. Is this
> really worthwhile? If you really prefer the new format that's fine but
> it seems like not necessarily a good use of energy.
>
After reflecting on it for while, I think that you and Greg are right
not to like the approach taken.
> > + }
>
> Instead of printing lineline at a time in a loop, what happens if you
> just print size bytes? (I honestly don't know because I have never used
> this function before).
>
It gets truncated as hex_dump_to_buffer prints at most 32 bytes.
thanks,
Paulo Almeida
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: pi433: add rf69_dbg_hex function
2022-02-11 8:14 ` Greg KH
@ 2022-02-11 19:39 ` Paulo Miguel Almeida
2022-02-12 2:59 ` Joe Perches
0 siblings, 1 reply; 6+ messages in thread
From: Paulo Miguel Almeida @ 2022-02-11 19:39 UTC (permalink / raw)
To: Greg KH; +Cc: realwakka, linux-staging, linux-kernel
On Fri, Feb 11, 2022 at 09:14:40AM +0100, Greg KH wrote:
>
> This is a lot of additional complexity for almost no real benefit.
>
you're right. I will no longer pursue this approach.
> > - /* print content read from fifo for debugging purposes */
> > - for (i = 0; i < size; i++)
> > - dev_dbg(&spi->dev, "%d - 0x%x\n", i, local_buffer[i + 1]);
>
> What is wrong with this simple line?
>
to be honest, I think that 1 register per line isn't the easiest way to
read them. Given that print_hex_dump_debug existed and had this
horizontal-style priting format, I thought that it would be a better
way of visualizing the fifo data.
the only problems with print_hex_dump_debug was the absense of device
name and string format... so I saw a couple of drivers implementing
alternative hex_dump-like functions and thought that pi433 would benefit
from similar approach.
> > - /* print content written from fifo for debugging purposes */
> > - for (i = 0; i < size; i++)
> > - dev_dbg(&spi->dev, "0x%x\n", buffer[i]);
if we are keeping this format, I may need to add the register idx to
dev_dbg:
dev_dbg(&spi->dev, "%d - 0x%x\n", i, buffer[i]);
> Again, the original is fine here, why make this so complex?
[thinking out loud/brainstorm]
I do agree that, for just a single driver, having a method like that
seemed unnecessary but do you think it would be a good idea having
something like dev_dbg_hex_dump or similar?
print_hex_dump_debug has the following limitation:
1) lacks string format
2) doesn't honor dynamic debug flags (other then 'p')
3) doesn't print device driver name and device name
> > + rf69_dbg_hex(spi, local_buffer + 1, size, "%s - ", __func__);
>
> Also, you are using local_buffer here, not buffer, why?
>
That was a mistake, good catch.
> I think the original is just fine, no need to polish something as tiny
> as a hex dump for debugging only.
>
> thanks,
>
> greg k-h
thanks for taking the time to review the patch, I won't pursue this
approach anymore.
thanks,
Paulo Almeida
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: pi433: add rf69_dbg_hex function
2022-02-11 19:39 ` Paulo Miguel Almeida
@ 2022-02-12 2:59 ` Joe Perches
0 siblings, 0 replies; 6+ messages in thread
From: Joe Perches @ 2022-02-12 2:59 UTC (permalink / raw)
To: Paulo Miguel Almeida, Greg KH; +Cc: realwakka, linux-staging, linux-kernel
On Sat, 2022-02-12 at 08:39 +1300, Paulo Miguel Almeida wrote:
> On Fri, Feb 11, 2022 at 09:14:40AM +0100, Greg KH wrote:
> >
> > This is a lot of additional complexity for almost no real benefit.
> >
>
> you're right. I will no longer pursue this approach.
>
> > > - /* print content read from fifo for debugging purposes */
> > > - for (i = 0; i < size; i++)
> > > - dev_dbg(&spi->dev, "%d - 0x%x\n", i, local_buffer[i + 1]);
> >
> > What is wrong with this simple line?
> >
>
> to be honest, I think that 1 register per line isn't the easiest way to
> read them. Given that print_hex_dump_debug existed and had this
> horizontal-style priting format, I thought that it would be a better
> way of visualizing the fifo data.
>
> the only problems with print_hex_dump_debug was the absense of device
> name and string format... so I saw a couple of drivers implementing
> alternative hex_dump-like functions and thought that pi433 would benefit
> from similar approach.
>
> > > - /* print content written from fifo for debugging purposes */
> > > - for (i = 0; i < size; i++)
> > > - dev_dbg(&spi->dev, "0x%x\n", buffer[i]);
>
> if we are keeping this format, I may need to add the register idx to
> dev_dbg:
> dev_dbg(&spi->dev, "%d - 0x%x\n", i, buffer[i]);
You could use %*ph with a buffer length up to 64 bytes.
Multiple times if necessary.
Something like:
for (i = 0; i < size; i += 64)
dev_dbg(&spi->dev, "FIFO buffer: %d: %*ph\n",
i, min_t(int, 64, size - i), buffer + i);
Or for a more realistic length like 16
for (i = 0; i < size; i += 16)
dev_dbg(&spi->dev, "FIFO buffer: %d: %*ph\n",
i, min_t(int, 16, size - i), buffer + i);
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-02-12 3:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-11 8:07 [PATCH] staging: pi433: add rf69_dbg_hex function Paulo Miguel Almeida
2022-02-11 8:14 ` Greg KH
2022-02-11 19:39 ` Paulo Miguel Almeida
2022-02-12 2:59 ` Joe Perches
2022-02-11 14:25 ` Dan Carpenter
2022-02-11 19:22 ` Paulo Miguel Almeida
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).