| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 31 Jan 2018 19:48:00 +0000
From: Jolly Shah <JOLLYS@...inx.com>
To: Greg KH <gregkh@...uxfoundation.org>
CC: "ard.biesheuvel@...aro.org" <ard.biesheuvel@...aro.org>,
"mingo@...nel.org" <mingo@...nel.org>,
"matt@...eblueprint.co.uk" <matt@...eblueprint.co.uk>,
"sudeep.holla@....com" <sudeep.holla@....com>,
"hkallweit1@...il.com" <hkallweit1@...il.com>,
"keescook@...omium.org" <keescook@...omium.org>,
"dmitry.torokhov@...il.com" <dmitry.torokhov@...il.com>,
"michal.simek@...inx.com" <michal.simek@...inx.com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"mark.rutland@....com" <mark.rutland@....com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
Rajan Vaja <RAJANV@...inx.com>
Subject: RE: [PATCH v3 4/4] drivers: firmware: xilinx: Add debugfs interface
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@...uxfoundation.org]
> Sent: Thursday, January 25, 2018 1:31 AM
> To: Jolly Shah <JOLLYS@...inx.com>
> Cc: ard.biesheuvel@...aro.org; mingo@...nel.org; matt@...eblueprint.co.uk;
> sudeep.holla@....com; hkallweit1@...il.com; keescook@...omium.org;
> dmitry.torokhov@...il.com; michal.simek@...inx.com; robh+dt@...nel.org;
> mark.rutland@....com; linux-arm-kernel@...ts.infradead.org; linux-
> kernel@...r.kernel.org; devicetree@...r.kernel.org; Jolly Shah
> <JOLLYS@...inx.com>; Rajan Vaja <RAJANV@...inx.com>
> 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@...inx.com>
> > + * Davorin Mista <davorin.mista@...ios.com>
> > + * Jolly Shah <jollys@...inx.com>
> > + * Rajan Vaja <rajanv@...inx.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
Powered by blists - more mailing lists