RE: [PATCH v3 4/4] drivers: firmware: xilinx: Add debugfs interface

[Jolly Shah]

Hi Greg,
Thanks for review comments. All will be taken care in next version patch.

Thanks,
Jolly Shah

> -----Original Message-----
> From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx]
> Sent: Thursday, January 25, 2018 1:31 AM
> To: Jolly Shah <JOLLYS@xxxxxxxxxx>
> Cc: ard.biesheuvel@xxxxxxxxxx; mingo@xxxxxxxxxx; matt@xxxxxxxxxxxxxxxxxxx;
> sudeep.holla@xxxxxxx; hkallweit1@xxxxxxxxx; keescook@xxxxxxxxxxxx;
> dmitry.torokhov@xxxxxxxxx; michal.simek@xxxxxxxxxx; robh+dt@xxxxxxxxxx;
> mark.rutland@xxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Jolly Shah
> <JOLLYS@xxxxxxxxxx>; Rajan Vaja <RAJANV@xxxxxxxxxx>
> Subject: Re: [PATCH v3 4/4] drivers: firmware: xilinx: Add debugfs interface
> 
> On Wed, Jan 24, 2018 at 03:21:14PM -0800, Jolly Shah wrote:
> > Firmware-debug provides debugfs interface to all APIs.
> 
> I don't understand this changelog comment, care to make it more
> informational?  At least describe the debugfs files you are adding so that people
> have a chance to understand what is going on here :)
> 
> > diff --git a/drivers/firmware/xilinx/zynqmp/Kconfig
> > b/drivers/firmware/xilinx/zynqmp/Kconfig
> > index 8f7709d..bdd0188 100644
> > --- a/drivers/firmware/xilinx/zynqmp/Kconfig
> > +++ b/drivers/firmware/xilinx/zynqmp/Kconfig
> > @@ -13,4 +13,11 @@ config ZYNQMP_FIRMWARE
> >  	  Say yes to enable zynqmp firmware interface driver.
> >  	  In doubt, say N
> >
> > +config ZYNQMP_FIRMWARE_DEBUG
> > +	bool "Enable Xilinx Zynq MPSoC firmware debug APIs"
> > +	default ARCH_ZYNQMP && ZYNQMP_FIRMWARE && DEBUG_FS
> > +	help
> > +	  Say yes to enable zynqmp firmware interface debug APIs.
> > +	  In doubt, say N
> 
> So your default is going to be Y if the driver is selected?  That's not good, just
> leave the default alone please.
> 
> > +
> >  endmenu
> > diff --git a/drivers/firmware/xilinx/zynqmp/Makefile
> > b/drivers/firmware/xilinx/zynqmp/Makefile
> > index 6629781..02f0c9a 100644
> > --- a/drivers/firmware/xilinx/zynqmp/Makefile
> > +++ b/drivers/firmware/xilinx/zynqmp/Makefile
> > @@ -2,3 +2,4 @@
> >  # Makefile for Xilinx firmwares
> >
> >  obj-$(CONFIG_ZYNQMP_FIRMWARE) += firmware.o firmware-ggs.o
> > +obj-$(CONFIG_ZYNQMP_FIRMWARE_DEBUG) += firmware-debug.o
> > diff --git a/drivers/firmware/xilinx/zynqmp/firmware-debug.c
> > b/drivers/firmware/xilinx/zynqmp/firmware-debug.c
> > new file mode 100644
> > index 0000000..daefc62
> > --- /dev/null
> > +++ b/drivers/firmware/xilinx/zynqmp/firmware-debug.c
> > @@ -0,0 +1,511 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Xilinx Zynq MPSoC Firmware layer for debugfs APIs
> > + *
> > + *  Copyright (C) 2014-2018 Xilinx, Inc.
> > + *
> > + *  Michal Simek <michal.simek@xxxxxxxxxx>
> > + *  Davorin Mista <davorin.mista@xxxxxxxxxx>
> > + *  Jolly Shah <jollys@xxxxxxxxxx>
> > + *  Rajan Vaja <rajanv@xxxxxxxxxx>
> > + */
> > +
> > +#include <linux/compiler.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/firmware/xilinx/zynqmp/firmware.h>
> > +#include <linux/firmware/xilinx/zynqmp/firmware-debug.h>
> > +
> > +#define DRIVER_NAME	"zynqmp-firmware"
> 
> You define this in 2 places, but only use it in one, you don't need this at all,
> please remove all instances.
> 
> > +static int process_api_request(u32 pm_id, u64 *pm_api_arg, u32
> > +*pm_api_ret) {
> > +	const struct zynqmp_eemi_ops *eemi_ops = get_eemi_ops();
> > +	u32 pm_api_version;
> > +	int ret;
> > +
> > +	if (!eemi_ops)
> > +		return -ENXIO;
> > +
> > +	switch (pm_id) {
> > +	case PM_GET_API_VERSION:
> > +		eemi_ops->get_api_version(&pm_api_version);
> > +		pr_info("%s PM-API Version = %d.%d\n", __func__,
> > +			pm_api_version >> 16, pm_api_version & 0xffff);
> 
> This is a _very_ noisy function, dumping a lot of stuff to the kernel log.  Why?
> Why not send it back to the caller of the debugfs file reader instead?
> 
> > +	case PM_GET_NODE_STATUS:
> > +		ret = eemi_ops->get_node_status(pm_api_arg[0],
> > +						&pm_api_ret[0],
> > +						&pm_api_ret[1],
> > +						&pm_api_ret[2]);
> > +		if (!ret)
> > +			pr_info("GET_NODE_STATUS:\n\tNodeId:
> %llu\n\tStatus: %u\n\tRequirements: %u\n\tUsage: %u\n",
> > +				pm_api_arg[0], pm_api_ret[0],
> > +				pm_api_ret[1], pm_api_ret[2]);
> 
> Multi-line dmesg messages?  Not good, you just lost the logging level for the
> multiple lines :(
> 
> Again, don't do this at all, just put it in file output instead.  That way tools/users
> can actually use it, instead of having to dig through kernel log messages.
> 
> > +/**
> > + * zynqmp_pm_api_debugfs_init - Initialize debugfs interface
> > + *
> > + * Return:      None
> > + */
> > +void zynqmp_pm_api_debugfs_init(void) {
> > +	struct dentry *root_dir;
> > +	struct dentry *d;
> > +
> > +	/* Initialize debugfs interface */
> > +	root_dir = debugfs_create_dir(DRIVER_NAME, NULL);
> > +	if (!root_dir) {
> > +		pr_warn("debugfs_create_dir failed\n");
> 
> Why warn?  What can a user do about this?  Just return and move on.
> 
> > +		return;
> > +	}
> > +
> > +	d = debugfs_create_file("pm", 0220, root_dir, NULL,
> > +				&fops_zynqmp_pm_dbgfs);
> > +	if (!d) {
> > +		pr_warn("debugfs_create_file power failed\n");
> > +		goto err_dbgfs;
> > +	}
> > +
> > +	d = debugfs_create_file("api_version", 0444, root_dir,
> > +				NULL, &fops_zynqmp_pm_dbgfs);
> > +	if (!d) {
> > +		pr_warn("debugfs_create_file api_version failed\n");
> > +		goto err_dbgfs;
> 
> Same for these files, who cares if they were not created or not at all?
> No need to even check here at all, this whole function can be reduced to just 3
> lines:
> 	debugfs_create_dir(...);
> 	debugfs_create_file(...);
> 	debugfs_create_file(...);
> 
> There, nice and simple.  Remember, debugfs doesn't matter, and it should be
> _really_ easy to use.  Don't make it more complex than it has to be please.
> 
> thanks,
> 
> greg k-h