Re: [PATCH 6/6] powerpc: powernv: no need to check return value of debugfs_create functions

2020-03-03 Thread Michael Ellerman
"Oliver O'Halloran"  writes:
> On Mon, Feb 10, 2020 at 12:12 AM Greg Kroah-Hartman
>  wrote:
>>
>> When calling debugfs functions, there is no need to ever check the
>> return value.  The function can work or not, but the code logic should
>> never do something different based on this.
>
> For memtrace debugfs is the only way to actually use the feature. It'd
> be nice if it still printed out *something* if it failed to create the
> files rather than just being mysteriously absent

That's true, but the current code doesn't actually do that anyway.

>> diff --git a/arch/powerpc/platforms/powernv/memtrace.c 
>> b/arch/powerpc/platforms/powernv/memtrace.c
>> index eb2e75dac369..d6d64f8718e6 100644
>> --- a/arch/powerpc/platforms/powernv/memtrace.c
>> +++ b/arch/powerpc/platforms/powernv/memtrace.c
>> @@ -187,11 +187,6 @@ static int memtrace_init_debugfs(void)
>>
>> snprintf(ent->name, 16, "%08x", ent->nid);
>> dir = debugfs_create_dir(ent->name, memtrace_debugfs_dir);
>> -   if (!dir) {
>> -   pr_err("Failed to create debugfs directory for node 
>> %d\n",
>> -   ent->nid);
>> -   return -1;
>> -   }

debugfs_create_dir() doesn't return NULL on error, it returns
ERR_PTR(-ENOMEM), which will not trigger that pr_err().

So I've merged this and if someone wants to they can send a follow-up to
do proper error checking in memtrace.c

cheers


Re: [PATCH 6/6] powerpc: powernv: no need to check return value of debugfs_create functions

2020-02-10 Thread Greg Kroah-Hartman
On Tue, Feb 11, 2020 at 02:01:53AM +1100, Oliver O'Halloran wrote:
> On Mon, Feb 10, 2020 at 12:12 AM Greg Kroah-Hartman
>  wrote:
> >
> > When calling debugfs functions, there is no need to ever check the
> > return value.  The function can work or not, but the code logic should
> > never do something different based on this.
> 
> For memtrace debugfs is the only way to actually use the feature. It'd
> be nice if it still printed out *something* if it failed to create the
> files rather than just being mysteriously absent, but maybe debugfs
> itself does that. Looks fine otherwise.

No, debugfs will only spit out an error message to the log if a
file/directory is attempted to be created for an already present
file/directory.

For other failures, no error will be printed, other than the normal
lower-level "out of memory" issues that might rarely happen.

thanks,

greg k-h


Re: [PATCH 6/6] powerpc: powernv: no need to check return value of debugfs_create functions

2020-02-10 Thread Oliver O'Halloran
On Mon, Feb 10, 2020 at 12:12 AM Greg Kroah-Hartman
 wrote:
>
> When calling debugfs functions, there is no need to ever check the
> return value.  The function can work or not, but the code logic should
> never do something different based on this.

For memtrace debugfs is the only way to actually use the feature. It'd
be nice if it still printed out *something* if it failed to create the
files rather than just being mysteriously absent, but maybe debugfs
itself does that. Looks fine otherwise.

Reviewed-by: Oliver O'Halloran 

> Cc: Benjamin Herrenschmidt 
> Cc: Paul Mackerras 
> Cc: Michael Ellerman 
> Cc: Sukadev Bhattiprolu 
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Greg Kroah-Hartman 
> ---
>  arch/powerpc/platforms/powernv/memtrace.c  |  7 
>  arch/powerpc/platforms/powernv/opal-imc.c  | 24 --
>  arch/powerpc/platforms/powernv/pci-ioda.c  |  5 ---
>  arch/powerpc/platforms/powernv/vas-debug.c | 37 ++
>  4 files changed, 10 insertions(+), 63 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/memtrace.c 
> b/arch/powerpc/platforms/powernv/memtrace.c
> index eb2e75dac369..d6d64f8718e6 100644
> --- a/arch/powerpc/platforms/powernv/memtrace.c
> +++ b/arch/powerpc/platforms/powernv/memtrace.c
> @@ -187,11 +187,6 @@ static int memtrace_init_debugfs(void)
>
> snprintf(ent->name, 16, "%08x", ent->nid);
> dir = debugfs_create_dir(ent->name, memtrace_debugfs_dir);
> -   if (!dir) {
> -   pr_err("Failed to create debugfs directory for node 
> %d\n",
> -   ent->nid);
> -   return -1;
> -   }
>
> ent->dir = dir;
> debugfs_create_file("trace", 0400, dir, ent, _fops);
> @@ -314,8 +309,6 @@ static int memtrace_init(void)
>  {
> memtrace_debugfs_dir = debugfs_create_dir("memtrace",
>   powerpc_debugfs_root);
> -   if (!memtrace_debugfs_dir)
> -   return -1;
>
> debugfs_create_file("enable", 0600, memtrace_debugfs_dir,
> NULL, _init_fops);
> diff --git a/arch/powerpc/platforms/powernv/opal-imc.c 
> b/arch/powerpc/platforms/powernv/opal-imc.c
> index 000b350d4060..968b9a4d1cd9 100644
> --- a/arch/powerpc/platforms/powernv/opal-imc.c
> +++ b/arch/powerpc/platforms/powernv/opal-imc.c
> @@ -35,11 +35,10 @@ static int imc_mem_set(void *data, u64 val)
>  }
>  DEFINE_DEBUGFS_ATTRIBUTE(fops_imc_x64, imc_mem_get, imc_mem_set, 
> "0x%016llx\n");
>
> -static struct dentry *imc_debugfs_create_x64(const char *name, umode_t mode,
> -struct dentry *parent, u64  
> *value)
> +static void imc_debugfs_create_x64(const char *name, umode_t mode,
> +  struct dentry *parent, u64  *value)
>  {
> -   return debugfs_create_file_unsafe(name, mode, parent,
> - value, _imc_x64);
> +   debugfs_create_file_unsafe(name, mode, parent, value, _imc_x64);
>  }
>
>  /*
> @@ -59,9 +58,6 @@ static void export_imc_mode_and_cmd(struct device_node 
> *node,
>
> imc_debugfs_parent = debugfs_create_dir("imc", powerpc_debugfs_root);
>
> -   if (!imc_debugfs_parent)
> -   return;
> -
> if (of_property_read_u32(node, "cb_offset", _offset))
> cb_offset = IMC_CNTL_BLK_OFFSET;
>
> @@ -69,21 +65,15 @@ static void export_imc_mode_and_cmd(struct device_node 
> *node,
> loc = (u64)(ptr->vbase) + cb_offset;
> imc_mode_addr = (u64 *)(loc + IMC_CNTL_BLK_MODE_OFFSET);
> sprintf(mode, "imc_mode_%d", (u32)(ptr->id));
> -   if (!imc_debugfs_create_x64(mode, 0600, imc_debugfs_parent,
> -   imc_mode_addr))
> -   goto err;
> +   imc_debugfs_create_x64(mode, 0600, imc_debugfs_parent,
> +  imc_mode_addr);
>
> imc_cmd_addr = (u64 *)(loc + IMC_CNTL_BLK_CMD_OFFSET);
> sprintf(cmd, "imc_cmd_%d", (u32)(ptr->id));
> -   if (!imc_debugfs_create_x64(cmd, 0600, imc_debugfs_parent,
> -   imc_cmd_addr))
> -   goto err;
> +   imc_debugfs_create_x64(cmd, 0600, imc_debugfs_parent,
> +  imc_cmd_addr);
> ptr++;
> }
> -   return;
> -
> -err:
> -   debugfs_remove_recursive(imc_debugfs_parent);
>  }
>
>  /*
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c 
> b/arch/powerpc/platforms/powernv/pci-ioda.c
> index 22c22cd7bd82..57d3a6af1d52 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -3174,11 +3174,6 @@ static void pnv_pci_ioda_create_dbgfs(void)
>
> sprintf(name, "PCI%04x",