lkml.org 
[lkml]   [2018]   [Jan]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH v2 4/4] drivers: firmware: xilinx: Add debugfs interface
    Date
    Thanks for review Greg,

    > -----Original Message-----
    > From: Greg KH [mailto:gregkh@linuxfoundation.org]
    > Sent: Tuesday, January 23, 2018 12:41 AM
    > To: Jolly Shah <JOLLYS@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; Rajan Vaja
    > <RAJANV@xilinx.com>; Jolly Shah <JOLLYS@xilinx.com>
    > Subject: Re: [PATCH v2 4/4] drivers: firmware: xilinx: Add debugfs interface
    >
    > On Wed, Jan 17, 2018 at 12:20:34PM -0800, Jolly Shah wrote:
    > > +/* Setup debugfs fops */
    > > +static const struct file_operations fops_zynqmp_pm_dbgfs = {
    > > + .owner = THIS_MODULE,
    > > + .write = zynqmp_pm_debugfs_api_write,
    > > + .read = zynqmp_pm_debugfs_api_version_read,
    > > +};
    > > +
    > > +/**
    > > + * zynqmp_pm_api_debugfs_init - Initialize debugfs interface
    > > + *
    > > + * Return: Returns 0 on success
    > > + * Corresponding error code otherwise
    > > + */
    > > +int zynqmp_pm_api_debugfs_init(void)
    > > +{
    > > + int err;
    > > +
    > > + /* Initialize debugfs interface */
    > > + zynqmp_pm_debugfs_dir = debugfs_create_dir(DRIVER_NAME, NULL);
    > > + if (!zynqmp_pm_debugfs_dir) {
    > > + pr_err("debugfs_create_dir failed\n");
    > > + return -ENODEV;
    > > + }
    >
    > No, you should NEVER care if a debugfs call returned an error or not, no need to
    > check it at all. Your code path should not change based on the return value as
    > no code should depened on the functionality of debugfs.
    >
    > Any error returned by a debugfs call can be passed right back into it with no
    > problems, so again, no need to check this.
    >

    Fixed in v3 patch series. Not saving dentries anymore but added check to show warning message instead of error.

    > > +
    > > + zynqmp_pm_debugfs_power =
    > > + debugfs_create_file("pm", 0220,
    > > + zynqmp_pm_debugfs_dir, NULL,
    > > + &fops_zynqmp_pm_dbgfs);
    > > + if (!zynqmp_pm_debugfs_power) {
    > > + pr_err("debugfs_create_file power failed\n");
    > > + err = -ENODEV;
    > > + goto err_dbgfs;
    > > + }
    > > +
    > > + zynqmp_pm_debugfs_api_version =
    > > + debugfs_create_file("api_version", 0444,
    > > + zynqmp_pm_debugfs_dir, NULL,
    > > + &fops_zynqmp_pm_dbgfs);
    > > + if (!zynqmp_pm_debugfs_api_version) {
    > > + pr_err("debugfs_create_file api_version failed\n");
    > > + err = -ENODEV;
    > > + goto err_dbgfs;
    > > + }
    >
    > Why do you save these dentries at all anyway? You never do anything with
    > them, just create the files and away you go, no need to worry about anything.
    >
    > Remember, debugfs was created to be very simple to use, don't make it more
    > complex than it has to be please.
    >
    > thanks,
    >
    > greg k-h

    Fixed in v3 patch series.

    Thanks,
    Jolly Shah

    \
     
     \ /
      Last update: 2018-01-25 00:34    [W:4.740 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site