On Sun, Jan 23, 2022 at 12:20:57PM +0100, Greg KH wrote:
> On Sun, Jan 23, 2022 at 08:40:29PM +1300, Paulo Miguel Almeida wrote:
> > This adds debugfs interface that can be used for debugging possible
> > hardware/software issues.
> >
> > It currently exposes the following debugfs entries for each SPI device
> > probed:
> >
> > /sys/kernel/debug/pi433/<DEVICE>/regs
> > ...
> >
> > The 'regs' file contains all rf69 uC registers values that are useful
> > for troubleshooting misconfigurations between 2 devices. It contains one
> > register per line so it should be easy to use normal filtering tools to
> > find the registers of interest if needed.
> >
> > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@xxxxxxxxx>
> > ---
> > Meta-comments:
> >
> > - I'm not entirely sure if I'm allowed to add additional dependencies to Kconfig
> > the way I did or if I should surround debugfs routines with
> > #ifdef CONFIG_DEBUG_FS. I saw both approaches couldn't put my finger on which
> > one is the 'right' way here. I'm taking suggestions :)
>
> Neither is really needed at all. The debugfs api will work properly if
> the main config option is not enabled, and the code will be compiled
> away properly.
>
> So no need to add any dependancy to your driver at all.
>
> debugfs was designed to be simple to use, and adding dependancies is not
> simple. Same goes for my comments below, the goal is to keep it simple
> and not worry about any error handling.
>
> > - I saw that in some other drivers there is a tendency to have debugfs routines
> > in a separate file such as debugfs.c and in that way this allows for smaller
> > files (which I do like) and Makefile that build files based on selected
> > configs such as:
> >
> > pi433-$(CONFIG_DEBUG_FS) += debugfs.o
>
> Again, not needed.
>
> > The only way I could achieve such thing would be if I moved pi433_device struct
> > to pi433_if.h but I wanted to double check if reviewers would agree with this
> > approach first.
> >
> > ---
> > drivers/staging/pi433/Kconfig | 2 +-
> > drivers/staging/pi433/pi433_if.c | 82 ++++++++++++++++++++++++++++++++
> > drivers/staging/pi433/rf69.c | 2 +-
> > drivers/staging/pi433/rf69.h | 1 +
> > 4 files changed, 85 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/pi433/Kconfig b/drivers/staging/pi433/Kconfig
> > index dd9e4709d1a8..9a8b7ef3e670 100644
> > --- a/drivers/staging/pi433/Kconfig
> > +++ b/drivers/staging/pi433/Kconfig
> > @@ -1,7 +1,7 @@
> > # SPDX-License-Identifier: GPL-2.0
> > config PI433
> > tristate "Pi433 - a 433MHz radio module for Raspberry Pi"
> > - depends on SPI
> > + depends on SPI && DEBUG_FS
>
> You can drop this.
>
> > help
> > This option allows you to enable support for the radio module Pi433.
> >
> > diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> > index 17ff51f6a9da..e3a0d78385c0 100644
> > --- a/drivers/staging/pi433/pi433_if.c
> > +++ b/drivers/staging/pi433/pi433_if.c
> > @@ -41,6 +41,9 @@
> > #ifdef CONFIG_COMPAT
> > #include <linux/compat.h>
> > #endif
> > +#include <linux/debugfs.h>
> > +#include <linux/seq_file.h>
> > +
> >
> > #include "pi433_if.h"
> > #include "rf69.h"
> > @@ -56,6 +59,8 @@ static DEFINE_MUTEX(minor_lock); /* Protect idr accesses */
> >
> > static struct class *pi433_class; /* mainly for udev to create /dev/pi433 */
> >
> > +static struct dentry *dbgfs_root_entry;
>
> There is no need for this dentry. Just look it up if you care about it.
>
> > +
> > /*
> > * tx config is instance specific
> > * so with each open a new tx config struct is needed
> > @@ -103,6 +108,9 @@ struct pi433_device {
> > bool rx_active;
> > bool tx_active;
> > bool interrupt_rx_allowed;
> > +
> > + /* debug fs */
> > + struct dentry *entry;
>
> Again, no need for this, look it up if you need it.
>
> > };
> >
> > struct pi433_instance {
> > @@ -1102,6 +1110,72 @@ static const struct file_operations pi433_fops = {
> > .llseek = no_llseek,
> > };
> >
> > +static int pi433_debugfs_regs_show(struct seq_file *m, void *p)
> > +{
> > + struct pi433_device *dev;
> > + u8 reg_data[114];
> > + size_t i;
> > + char *fmt = "0x%02x, 0x%02x\n";
> > +
> > + // obtain pi433_device reference
> > + dev = m->private;
>
> That is not a "reference", that is just a normal empty pointer. No need
> to call it something else, that's just confusing.
>
> > +
> > + // acquire locks to avoid race conditions
> > + mutex_lock(&dev->tx_fifo_lock);
> > + mutex_lock(&dev->rx_lock);
> > +
> > + // wait for on-going operations to finish
> > + if (dev->tx_active)
> > + wait_event_interruptible(dev->rx_wait_queue, !dev->tx_active);
> > +
> > + if (dev->rx_active)
> > + wait_event_interruptible(dev->tx_wait_queue, !dev->rx_active);
> > +
> > + // read contiguous regs
> > + // skip FIFO register (0x0) otherwise this can affect some of uC ops
> > + for (i = 1; i < 0x50; i++)
> > + reg_data[i] = rf69_read_reg(dev->spi, i);
> > +
> > + // read non-contiguous regs
> > + reg_data[REG_TESTLNA] = rf69_read_reg(dev->spi, REG_TESTLNA);
> > + reg_data[REG_TESTPA1] = rf69_read_reg(dev->spi, REG_TESTPA1);
> > + reg_data[REG_TESTPA2] = rf69_read_reg(dev->spi, REG_TESTPA2);
> > + reg_data[REG_TESTDAGC] = rf69_read_reg(dev->spi, REG_TESTDAGC);
> > + reg_data[REG_TESTAFC] = rf69_read_reg(dev->spi, REG_TESTAFC);
> > +
> > + seq_puts(m, "# reg, val\n");
> > +
> > + // print contiguous regs
> > + for (i = 1; i < 0x50; i++)
> > + seq_printf(m, fmt, i, reg_data[i]);
> > +
> > + // print non-contiguous regs
> > + seq_printf(m, fmt, REG_TESTLNA, reg_data[REG_TESTLNA]);
> > + seq_printf(m, fmt, REG_TESTPA1, reg_data[REG_TESTPA1]);
> > + seq_printf(m, fmt, REG_TESTPA2, reg_data[REG_TESTPA2]);
> > + seq_printf(m, fmt, REG_TESTDAGC, reg_data[REG_TESTDAGC]);
> > + seq_printf(m, fmt, REG_TESTAFC, reg_data[REG_TESTAFC]);
> > +
> > + // release locks
> > + mutex_unlock(&dev->tx_fifo_lock);
> > + mutex_unlock(&dev->rx_lock);
> > +
> > + return 0;
> > +}
> > +
> > +static ssize_t pi433_debugfs_regs_open(struct inode *inode, struct file *filp)
> > +{
> > + return single_open(filp, pi433_debugfs_regs_show, inode->i_private);
> > +}
> > +
> > +static const struct file_operations debugfs_fops = {
> > + .llseek = seq_lseek,
> > + .open = pi433_debugfs_regs_open,
> > + .owner = THIS_MODULE,
> > + .read = seq_read,
> > + .release = single_release
> > +};
> > +
> > /*-------------------------------------------------------------------------*/
> >
> > static int pi433_probe(struct spi_device *spi)
> > @@ -1256,6 +1330,10 @@ static int pi433_probe(struct spi_device *spi)
> > /* spi setup */
> > spi_set_drvdata(spi, device);
> >
> > + /* debugfs setup */
> > + device->entry = debugfs_create_dir(dev_name(device->dev), dbgfs_root_entry);
>
> Make "entry" a local variable, and then pass it to the next call.
>
> And look up dbgfs_root_entry as well. This can be rewritten as:
> entry = debugfs_create_dir(dev_name(device->dev,
> debugfs_lookup("pi433", NULL);
>
> > + debugfs_create_file("regs", 0400, device->entry, device, &debugfs_fops);
>
> When do you ever remove the debugfs entry for the device? I do not see
> that in any release function here. Did you forget about that?
>
> > +
> > return 0;
> >
> > del_cdev:
> > @@ -1353,6 +1431,8 @@ static int __init pi433_init(void)
> > return PTR_ERR(pi433_class);
> > }
> >
> > + dbgfs_root_entry = debugfs_create_dir("pi433", NULL);
>
> Again, no need to keep this around, see above.
>
> > +
> > status = spi_register_driver(&pi433_spi_driver);
> > if (status < 0) {
> > class_destroy(pi433_class);
> > @@ -1370,6 +1450,8 @@ static void __exit pi433_exit(void)
> > spi_unregister_driver(&pi433_spi_driver);
> > class_destroy(pi433_class);
> > unregister_chrdev(MAJOR(pi433_dev), pi433_spi_driver.driver.name);
> > + debugfs_remove_recursive(dbgfs_root_entry);
>
> Can be rewritten as:
> debugfs_remove_recursive(debugfs_lookup("pi433", NULL));
>
> Or better yet:
> debugfs_remove_recursive(debugfs_lookup(KBUILD_MODULE_NAME, NULL));
>
> thanks,
>
> greg k-h
thanks for taking the time to review this patchset.
you are right, I will make the changes you pointed out and submit a new
version of the patchset shortly.
thanks,
Paulo Almeida
