[PATCH] staging: pi433: add rf69_dbg_hex function

[PATCH] staging: pi433: add rf69_dbg_hex function

[Paulo Miguel Almeida]

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

Re: [PATCH] staging: pi433: add rf69_dbg_hex function

[Greg KH]

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

Re: [PATCH] staging: pi433: add rf69_dbg_hex function

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2807 bytes --]

Hi Paulo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]

url:    https://github.com/0day-ci/linux/commits/Paulo-Miguel-Almeida/staging-pi433-add-rf69_dbg_hex-function/20220211-160900
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git f2c461536226eb0852a241e72b9125685d6741b4
config: arc-randconfig-r043-20220211 (https://download.01.org/0day-ci/archive/20220211/202202111821.UaNmElDF-lkp(a)intel.com/config)
compiler: arc-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/a7ea3ecd6bd7905c6ee495a1053108c2bb922ec2
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Paulo-Miguel-Almeida/staging-pi433-add-rf69_dbg_hex-function/20220211-160900
        git checkout a7ea3ecd6bd7905c6ee495a1053108c2bb922ec2
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash drivers/staging/pi433/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/staging/pi433/rf69.c: In function 'rf69_dbg_hex':
>> drivers/staging/pi433/rf69.c:837:9: warning: function 'rf69_dbg_hex' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
     837 |         text_pos = vscnprintf(text, sizeof(textbuf), fmt, args);
         |         ^~~~~~~~


vim +837 drivers/staging/pi433/rf69.c

   824	
   825	static void rf69_dbg_hex(struct spi_device *spi, u8 *buf, unsigned int size,
   826				 const char *fmt, ...)
   827	{
   828		va_list args;
   829		char textbuf[512] = {};
   830		char *text = textbuf;
   831		int text_pos;
   832	
   833		int rowsize = 16;
   834		int i, linelen, remaining = size;
   835	
   836		va_start(args, fmt);
 > 837		text_pos = vscnprintf(text, sizeof(textbuf), fmt, args);
   838		text += text_pos;
   839		va_end(args);
   840	
   841		for (i = 0; i < size; i += rowsize) {
   842			linelen = min(remaining, rowsize);
   843			remaining -= rowsize;
   844	
   845			hex_dump_to_buffer(buf + i, linelen, rowsize, 1,
   846					   text, sizeof(textbuf) - text_pos, false);
   847	
   848			dev_dbg(&spi->dev, "%s\n", textbuf);
   849	
   850			memset(text, 0, sizeof(textbuf) - text_pos);
   851		}
   852	}
   853	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH] staging: pi433: add rf69_dbg_hex function

[Dan Carpenter]

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
 

Re: [PATCH] staging: pi433: add rf69_dbg_hex function

[Paulo Miguel Almeida]

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

Re: [PATCH] staging: pi433: add rf69_dbg_hex function

[Paulo Miguel Almeida]

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

Re: [PATCH] staging: pi433: add rf69_dbg_hex function

[Joe Perches]

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);