devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Greg KH <gregkh@linuxfoundation.org>
To: Jolly Shah <jolly.shah@xilinx.com>
Cc: ard.biesheuvel@linaro.org, mingo@kernel.org,
	matt@codeblueprint.co.uk, sudeep.holla@arm.com,
	hkallweit1@gmail.com, keescook@chromium.org,
	dmitry.torokhov@gmail.com, michal.simek@xilinx.com,
	robh+dt@kernel.org, mark.rutland@arm.com,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	Jolly Shah <jollys@xilinx.com>, Rajan Vaja <rajanv@xilinx.com>
Subject: Re: [PATCH v3 4/4] drivers: firmware: xilinx: Add debugfs interface
Date: Thu, 25 Jan 2018 10:30:57 +0100	[thread overview]
Message-ID: <20180125093057.GA1936@kroah.com> (raw)
In-Reply-To: <1516836074-4149-5-git-send-email-jollys@xilinx.com>

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@xilinx.com>
> + *  Davorin Mista <davorin.mista@aggios.com>
> + *  Jolly Shah <jollys@xilinx.com>
> + *  Rajan Vaja <rajanv@xilinx.com>
> + */
> +
> +#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

  reply	other threads:[~2018-01-25  9:30 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-24 23:21 [PATCH v3 0/4] drivers: firmware: xilinx: Add firmware driver support Jolly Shah
     [not found] ` <1516836074-4149-1-git-send-email-jollys-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
2018-01-24 23:21   ` [PATCH v3 1/4] dt-bindings: firmware: Add bindings for ZynqMP firmware Jolly Shah
2018-01-30 17:08     ` Rob Herring
2018-01-31 18:03       ` Jolly Shah
2018-02-01 15:11         ` Rob Herring
2018-01-31 18:03     ` Mark Rutland
     [not found]       ` <20180131180354.mqf4gvvprdtycbn5-agMKViyK24J5pKCnmE3YQBJ8xKzm50AiAL8bYrjMMd8@public.gmane.org>
2018-02-01  1:04         ` Jolly Shah
     [not found]           ` <CY1PR0201MB076454E915A6E6947A39F04BB8FA0-w5HSbVcoX6AJQdFqRSmStRrHTHEw16jenBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
2018-02-01 10:27             ` Mark Rutland
2018-02-02 19:50               ` Jolly Shah
2018-01-24 23:21 ` [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware driver Jolly Shah
     [not found]   ` <1516836074-4149-3-git-send-email-jollys-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
2018-01-30  5:05     ` Shubhrajyoti Datta
2018-01-31 19:46       ` Jolly Shah
2018-01-31 18:20     ` Mark Rutland
     [not found]       ` <20180131182012.oshjmvahetaizlbu-agMKViyK24J5pKCnmE3YQBJ8xKzm50AiAL8bYrjMMd8@public.gmane.org>
2018-02-01  1:23         ` Jolly Shah
     [not found]           ` <CY1PR0201MB0764D88D409A35B8379323B4B8FA0-w5HSbVcoX6AJQdFqRSmStRrHTHEw16jenBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
2018-02-01 10:33             ` Mark Rutland
     [not found]               ` <20180201103321.fftn37mgzbk6oltl-agMKViyK24J5pKCnmE3YQBJ8xKzm50AiAL8bYrjMMd8@public.gmane.org>
2018-02-01 23:54                 ` Jolly Shah
2018-01-24 23:21 ` [PATCH v3 3/4] drivers: firmware: xilinx: Add sysfs interface Jolly Shah
2018-01-24 23:21 ` [PATCH v3 4/4] drivers: firmware: xilinx: Add debugfs interface Jolly Shah
2018-01-25  9:30   ` Greg KH [this message]
     [not found]     ` <20180125093057.GA1936-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
2018-01-31 19:48       ` Jolly Shah

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180125093057.GA1936@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=ard.biesheuvel@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dmitry.torokhov@gmail.com \
    --cc=hkallweit1@gmail.com \
    --cc=jolly.shah@xilinx.com \
    --cc=jollys@xilinx.com \
    --cc=keescook@chromium.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=matt@codeblueprint.co.uk \
    --cc=michal.simek@xilinx.com \
    --cc=mingo@kernel.org \
    --cc=rajanv@xilinx.com \
    --cc=robh+dt@kernel.org \
    --cc=sudeep.holla@arm.com \
    --subject='Re: [PATCH v3 4/4] drivers: firmware: xilinx: Add debugfs interface' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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