[v3,13/17] mvpp2: no need to check return value of debugfs_create functions

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.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Maxime Chevallier <maxime.chevallier@bootlin.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Nathan Huckleberry <nhuck@google.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 .../ethernet/marvell/mvpp2/mvpp2_debugfs.c    | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

On Sat, Aug 10, 2019 at 3:17 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> 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.

Maybe adding this recommendation to the comment block above the
definition of debugfs_create_dir() in fs/debugfs/inode.c would help
prevent this issue in the future?  What failure means, and how to
proceed can be tricky; more documentation can only help in this
regard.

>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Nathan Huckleberry <nhuck@google.com>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  .../ethernet/marvell/mvpp2/mvpp2_debugfs.c    | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c
> index 274fb07362cb..4a3baa7e0142 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c
> @@ -452,8 +452,6 @@ static int mvpp2_dbgfs_flow_port_init(struct dentry *parent,
>         struct dentry *port_dir;
>
>         port_dir = debugfs_create_dir(port->dev->name, parent);
> -       if (IS_ERR(port_dir))
> -               return PTR_ERR(port_dir);
>
>         port_entry = &port->priv->dbgfs_entries->port_flow_entries[port->id];
>
> @@ -480,8 +478,6 @@ static int mvpp2_dbgfs_flow_entry_init(struct dentry *parent,
>         sprintf(flow_entry_name, "%02d", flow);
>
>         flow_entry_dir = debugfs_create_dir(flow_entry_name, parent);
> -       if (!flow_entry_dir)
> -               return -ENOMEM;
>
>         entry = &priv->dbgfs_entries->flow_entries[flow];
>
> @@ -514,8 +510,6 @@ static int mvpp2_dbgfs_flow_init(struct dentry *parent, struct mvpp2 *priv)
>         int i, ret;
>
>         flow_dir = debugfs_create_dir("flows", parent);
> -       if (!flow_dir)
> -               return -ENOMEM;
>
>         for (i = 0; i < MVPP2_N_PRS_FLOWS; i++) {
>                 ret = mvpp2_dbgfs_flow_entry_init(flow_dir, priv, i);
> @@ -539,8 +533,6 @@ static int mvpp2_dbgfs_prs_entry_init(struct dentry *parent,
>         sprintf(prs_entry_name, "%03d", tid);
>
>         prs_entry_dir = debugfs_create_dir(prs_entry_name, parent);
> -       if (!prs_entry_dir)
> -               return -ENOMEM;
>
>         entry = &priv->dbgfs_entries->prs_entries[tid];
>
> @@ -578,8 +570,6 @@ static int mvpp2_dbgfs_prs_init(struct dentry *parent, struct mvpp2 *priv)
>         int i, ret;
>
>         prs_dir = debugfs_create_dir("parser", parent);
> -       if (!prs_dir)
> -               return -ENOMEM;
>
>         for (i = 0; i < MVPP2_PRS_TCAM_SRAM_SIZE; i++) {
>                 ret = mvpp2_dbgfs_prs_entry_init(prs_dir, priv, i);
> @@ -688,8 +678,6 @@ static int mvpp2_dbgfs_port_init(struct dentry *parent,
>         struct dentry *port_dir;
>
>         port_dir = debugfs_create_dir(port->dev->name, parent);
> -       if (IS_ERR(port_dir))
> -               return PTR_ERR(port_dir);
>
>         debugfs_create_file("parser_entries", 0444, port_dir, port,
>                             &mvpp2_dbgfs_port_parser_fops);
> @@ -716,15 +704,10 @@ void mvpp2_dbgfs_init(struct mvpp2 *priv, const char *name)
>         int ret, i;
>
>         mvpp2_root = debugfs_lookup(MVPP2_DRIVER_NAME, NULL);
> -       if (!mvpp2_root) {
> +       if (!mvpp2_root)
>                 mvpp2_root = debugfs_create_dir(MVPP2_DRIVER_NAME, NULL);
> -               if (IS_ERR(mvpp2_root))
> -                       return;
> -       }
>
>         mvpp2_dir = debugfs_create_dir(name, mvpp2_root);
> -       if (IS_ERR(mvpp2_dir))
> -               return;
>
>         priv->dbgfs_dir = mvpp2_dir;
>         priv->dbgfs_entries = kzalloc(sizeof(*priv->dbgfs_entries), GFP_KERNEL);
> --
> 2.22.0
>

On Mon, Aug 12, 2019 at 10:55:51AM -0700, Nick Desaulniers wrote:
> On Sat, Aug 10, 2019 at 3:17 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> 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.
> 
> Maybe adding this recommendation to the comment block above the
> definition of debugfs_create_dir() in fs/debugfs/inode.c would help
> prevent this issue in the future?  What failure means, and how to
> proceed can be tricky; more documentation can only help in this
> regard.

If it was there, would you have read it?  :)

I'll add it to the list for when I revamp the debugfs documentation that
is already in the kernel, that very few people actually read...

thanks,

greg k-h

On Mon, Aug 12, 2019 at 12:01 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Aug 12, 2019 at 10:55:51AM -0700, Nick Desaulniers wrote:
> > On Sat, Aug 10, 2019 at 3:17 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> 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.
> >
> > Maybe adding this recommendation to the comment block above the
> > definition of debugfs_create_dir() in fs/debugfs/inode.c would help
> > prevent this issue in the future?  What failure means, and how to
> > proceed can be tricky; more documentation can only help in this
> > regard.
>
> If it was there, would you have read it?  :)

Absolutely; I went looking for it, which is why I haven't added my
reviewed by tag, because it's not clear from the existing comment
block how callers should handle the return value, particularly as you
describe in this commit's commit message.

>
> I'll add it to the list for when I revamp the debugfs documentation that
> is already in the kernel, that very few people actually read...
>
> thanks,
>
> greg k-h

On Mon, Aug 12, 2019 at 12:44:36PM -0700, Nick Desaulniers wrote:
> On Mon, Aug 12, 2019 at 12:01 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Aug 12, 2019 at 10:55:51AM -0700, Nick Desaulniers wrote:
> > > On Sat, Aug 10, 2019 at 3:17 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> 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.
> > >
> > > Maybe adding this recommendation to the comment block above the
> > > definition of debugfs_create_dir() in fs/debugfs/inode.c would help
> > > prevent this issue in the future?  What failure means, and how to
> > > proceed can be tricky; more documentation can only help in this
> > > regard.
> >
> > If it was there, would you have read it?  :)
> 
> Absolutely; I went looking for it, which is why I haven't added my
> reviewed by tag, because it's not clear from the existing comment
> block how callers should handle the return value, particularly as you
> describe in this commit's commit message.

Ok, fair enough, I'll update the documentation soon, thanks.

greg k-h