[PATCH] staging: pi433: remove need to recompile code to debug fifo content

[PATCH] staging: pi433: remove need to recompile code to debug fifo content

[Paulo Miguel Almeida]

Debugging content present in the FIFO register is tricky as when we read
the FIFO register that changes the content of fifo struct which reduces
number of possible ways of debugging it. Rf69 uC has the possibility of
triggering certain IRQs depending on how many items are in the FIFO
queue, so being able to know what's in there is an important way to
troubleshoot certain problems.

This patch removes the requirement of having to compile pi433 driver
with DEBUG_FIFO_ACCESS set and let that be driven by printk verbositity
level and/or dynamic debug config instead.

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
Meta-comments:

#1
In my mind, I didn't like the idea of having to change the code and then 
echo "module pi433 +p" > <debugfs>/dynamic_debug/control to only then
be able to read stuff being sent/retrieved from fifo. It felt somewhat
redundant at a certain level. On the other hand, I understand that
removing the conditional compilation will force a for-loop to iterate
for no real reason most of the time (max 66 iterations)... so I made a 
trade-off and in case anyone disagrees with that, just let me know and I
will be happy to change to a different approach.

#2
In the past, it's been pointed out to me during code review that I tend
to add code comments which could be omitted. In this case, the for-loop
seemed a bit odd without explaining why it's in there. Let me know if
you think I should keep/remove it.

Patch dependency:

This patch depends on the following patches being applied first:
- https://lore.kernel.org/lkml/Yf9ivRB5qpmA5rY2@mail.google.com/
- https://lore.kernel.org/lkml/YgA4XHU4uf6YkOk5@mail.google.com/

---
 drivers/staging/pi433/rf69.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index a8def70808d6..901f8db3e3ce 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -6,11 +6,6 @@
  *	Marcus Wolf <linux@wolf-entwicklungen.de>
  */
 
-/* enable prosa debug info */
-#undef DEBUG
-/* enable print of values on fifo access */
-#undef DEBUG_FIFO_ACCESS
-
 #include <linux/types.h>
 #include <linux/spi/spi.h>
 
@@ -829,9 +824,7 @@ int rf69_set_dagc(struct spi_device *spi, enum dagc dagc)
 
 int rf69_read_fifo(struct spi_device *spi, u8 *buffer, unsigned int size)
 {
-#ifdef DEBUG_FIFO_ACCESS
 	int i;
-#endif
 	struct spi_transfer transfer;
 	u8 local_buffer[FIFO_SIZE + 1];
 	int retval;
@@ -851,10 +844,9 @@ int rf69_read_fifo(struct spi_device *spi, u8 *buffer, unsigned int size)
 
 	retval = spi_sync_transfer(spi, &transfer, 1);
 
-#ifdef DEBUG_FIFO_ACCESS
+	/* 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]);
-#endif
 
 	memcpy(buffer, &local_buffer[1], size);
 
@@ -863,9 +855,7 @@ 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)
 {
-#ifdef DEBUG_FIFO_ACCESS
 	int i;
-#endif
 	u8 local_buffer[FIFO_SIZE + 1];
 
 	if (size > FIFO_SIZE) {
@@ -877,10 +867,9 @@ 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);
 
-#ifdef DEBUG_FIFO_ACCESS
+	/* print content written from fifo for debugging purposes */
 	for (i = 0; i < size; i++)
 		dev_dbg(&spi->dev, "0x%x\n", buffer[i]);
-#endif
 
 	return spi_write(spi, local_buffer, size + 1);
 }
-- 
2.34.1

Re: [PATCH] staging: pi433: remove need to recompile code to debug fifo content

[Dan Carpenter]

On Mon, Feb 07, 2022 at 05:45:12PM +1300, Paulo Miguel Almeida wrote:
> Debugging content present in the FIFO register is tricky as when we read
> the FIFO register that changes the content of fifo struct which reduces
> number of possible ways of debugging it. Rf69 uC has the possibility of
> triggering certain IRQs depending on how many items are in the FIFO
> queue, so being able to know what's in there is an important way to
> troubleshoot certain problems.
> 
> This patch removes the requirement of having to compile pi433 driver
> with DEBUG_FIFO_ACCESS set and let that be driven by printk verbositity
> level and/or dynamic debug config instead.
> 
> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> ---
> Meta-comments:
> 
> #1
> In my mind, I didn't like the idea of having to change the code and then 
> echo "module pi433 +p" > <debugfs>/dynamic_debug/control to only then
> be able to read stuff being sent/retrieved from fifo. It felt somewhat
> redundant at a certain level. On the other hand, I understand that
> removing the conditional compilation will force a for-loop to iterate
> for no real reason most of the time (max 66 iterations)... so I made a 
> trade-off and in case anyone disagrees with that, just let me know and I
> will be happy to change to a different approach.
> 

This is fine.  It's useful information to you.  It's makes the code
nicer by removing ifdefs.  It's not going to show up in benchmarking.

> #2
> In the past, it's been pointed out to me during code review that I tend
> to add code comments which could be omitted. In this case, the for-loop
> seemed a bit odd without explaining why it's in there. Let me know if
> you think I should keep/remove it.

Remove.  Everyone knows what dev_dbg() does and the "read from fifo"
vs "written from[sic] fifo" is built into the function name.

>  int rf69_read_fifo(struct spi_device *spi, u8 *buffer, unsigned int size)
>  {
> -#ifdef DEBUG_FIFO_ACCESS
>  	int i;
> -#endif
>  	struct spi_transfer transfer;
>  	u8 local_buffer[FIFO_SIZE + 1];

You did not introduce this but we are potentially printing out
uninitialized data if spi_sync_transfer() fails.  Please initialize this
with:

	u8 local_buffer[FIFO_SIZE + 1] = {};

Do that in a separate patch, though.

>  	int retval;
> @@ -851,10 +844,9 @@ int rf69_read_fifo(struct spi_device *spi, u8 *buffer, unsigned int size)
>  
>  	retval = spi_sync_transfer(spi, &transfer, 1);
>  
> -#ifdef DEBUG_FIFO_ACCESS
> +	/* 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]);
> -#endif
>  
>  	memcpy(buffer, &local_buffer[1], size);
>  

regards,
dan carpenter

Re: [PATCH] staging: pi433: remove need to recompile code to debug fifo content

[Joe Perches]

On Mon, 2022-02-07 at 13:06 +0300, Dan Carpenter wrote:
> On Mon, Feb 07, 2022 at 05:45:12PM +1300, Paulo Miguel Almeida wrote:
> > Debugging content present in the FIFO register is tricky as when we read
> > the FIFO register that changes the content of fifo struct which reduces
> > number of possible ways of debugging it. Rf69 uC has the possibility of
> > triggering certain IRQs depending on how many items are in the FIFO
> > queue, so being able to know what's in there is an important way to
> > troubleshoot certain problems.
> > 
> > This patch removes the requirement of having to compile pi433 driver
> > with DEBUG_FIFO_ACCESS set and let that be driven by printk verbositity
> > level and/or dynamic debug config instead.
> > 
> > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> > ---
> > Meta-comments:
> > 
> > #1
> > In my mind, I didn't like the idea of having to change the code and then 
> > echo "module pi433 +p" > <debugfs>/dynamic_debug/control to only then
> > be able to read stuff being sent/retrieved from fifo. It felt somewhat
> > redundant at a certain level. On the other hand, I understand that
> > removing the conditional compilation will force a for-loop to iterate
> > for no real reason most of the time (max 66 iterations)... so I made a 
> > trade-off and in case anyone disagrees with that, just let me know and I
> > will be happy to change to a different approach.
> > 
> 
> This is fine.  It's useful information to you.  It's makes the code
> nicer by removing ifdefs.  It's not going to show up in benchmarking.
> 
> > #2
> > In the past, it's been pointed out to me during code review that I tend
> > to add code comments which could be omitted. In this case, the for-loop
> > seemed a bit odd without explaining why it's in there. Let me know if
> > you think I should keep/remove it.
> 
> Remove.  Everyone knows what dev_dbg() does and the "read from fifo"
> vs "written from[sic] fifo" is built into the function name.
> 
> >  int rf69_read_fifo(struct spi_device *spi, u8 *buffer, unsigned int size)
> >  {
[]
> > @@ -851,10 +844,9 @@ int rf69_read_fifo(struct spi_device *spi, u8 *buffer, unsigned int size)
> >  
> >  	retval = spi_sync_transfer(spi, &transfer, 1);
> >  
> > -#ifdef DEBUG_FIFO_ACCESS
> > +	/* 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]);
> > -#endif

If you use

	print_hex_dump_debug

perhaps the DEBUG_FIFO_ACCESS could be removed.

Re: [PATCH] staging: pi433: remove need to recompile code to debug fifo content

[Paulo Miguel Almeida]

On Mon, Feb 07, 2022 at 02:15:47AM -0800, Joe Perches wrote:
> > > -#ifdef DEBUG_FIFO_ACCESS
> > > +	/* 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]);
> > > -#endif
> 
> If you use
> 
> 	print_hex_dump_debug
> 
> perhaps the DEBUG_FIFO_ACCESS could be removed.
> 

Hi Joe, thanks for taking the time to review my patch.

print_hex_dump_debug is pretty convenient and straight to the point
which I do like. The only thing that I "didn't like" is the fact that 
when configuring dynamic debugging, print_hex_dump_debug only cares 
about 'p' flag and ignores all of the other flags that can be helpful
for tracking things down. 

So essentially I lose track of which function and device instance 
the message belongs to (users can have more than 1 at the same time).

But, because of your suggestion, I came across hex_dump_to_buffer()
which bridged the gap. thanks a lot :)

Would you be comfortable with the following implementation? (Dan, feel
free to chip in)

	char linebuf[FIFO_SIZE * 3] = {0};
	hex_dump_to_buffer(local_buffer + 1, size, 16, 1, linebuf,
			   ARRAY_SIZE(linebuf), false);
	dev_dbg(&spi->dev, "%s\n", linebuf);

thanks,

Paulo Almeida

Re: [PATCH] staging: pi433: remove need to recompile code to debug fifo content

[Paulo Miguel Almeida]

On Mon, Feb 07, 2022 at 01:06:01PM +0300, Dan Carpenter wrote:
> > #2
> > In the past, it's been pointed out to me during code review that I tend
> > to add code comments which could be omitted. In this case, the for-loop
> > seemed a bit odd without explaining why it's in there. Let me know if
> > you think I should keep/remove it.
> 
> Remove.  Everyone knows what dev_dbg() does and the "read from fifo"
> vs "written from[sic] fifo" is built into the function name.
> 

fair enough

> >  int rf69_read_fifo(struct spi_device *spi, u8 *buffer, unsigned int size)
> >  {
> > -#ifdef DEBUG_FIFO_ACCESS
> >  	int i;
> > -#endif
> >  	struct spi_transfer transfer;
> >  	u8 local_buffer[FIFO_SIZE + 1];
> 
> You did not introduce this but we are potentially printing out
> uninitialized data if spi_sync_transfer() fails.  Please initialize this
> with:
> 
> 	u8 local_buffer[FIFO_SIZE + 1] = {};
> 
> Do that in a separate patch, though.
> 

good point, will do. Thanks a lot :)

thanks,

Paulo Almeida