[PATCH] net: mvpp2: Fix error checking

[PATCH] net: mvpp2: Fix error checking

[Yeqi Fu]

The function debugfs_create_dir returns ERR_PTR if an error occurs,
and the appropriate way to verify for errors is to use the inline
function IS_ERR. The patch will substitute the null-comparison with
IS_ERR.

Suggested-by: Ivan Orlov <ivan.orlov0322@gmail.com>
Signed-off-by: Yeqi Fu <asuk4.q@gmail.com>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c
index 75e83ea2a926..9c53f378edda 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c
@@ -593,7 +593,7 @@ static int mvpp2_dbgfs_c2_entry_init(struct dentry *parent,
 	sprintf(c2_entry_name, "%03d", id);
 
 	c2_entry_dir = debugfs_create_dir(c2_entry_name, parent);
-	if (!c2_entry_dir)
+	if (IS_ERR(c2_entry_dir))
 		return -ENOMEM;
 
 	entry = &priv->dbgfs_entries->c2_entries[id];
@@ -626,7 +626,7 @@ static int mvpp2_dbgfs_flow_tbl_entry_init(struct dentry *parent,
 	sprintf(flow_tbl_entry_name, "%03d", id);
 
 	flow_tbl_entry_dir = debugfs_create_dir(flow_tbl_entry_name, parent);
-	if (!flow_tbl_entry_dir)
+	if (IS_ERR(flow_tbl_entry_dir))
 		return -ENOMEM;
 
 	entry = &priv->dbgfs_entries->flt_entries[id];
@@ -646,11 +646,11 @@ static int mvpp2_dbgfs_cls_init(struct dentry *parent, struct mvpp2 *priv)
 	int i, ret;
 
 	cls_dir = debugfs_create_dir("classifier", parent);
-	if (!cls_dir)
+	if (IS_ERR(cls_dir))
 		return -ENOMEM;
 
 	c2_dir = debugfs_create_dir("c2", cls_dir);
-	if (!c2_dir)
+	if (IS_ERR(c2_dir))
 		return -ENOMEM;
 
 	for (i = 0; i < MVPP22_CLS_C2_N_ENTRIES; i++) {
@@ -660,7 +660,7 @@ static int mvpp2_dbgfs_cls_init(struct dentry *parent, struct mvpp2 *priv)
 	}
 
 	flow_tbl_dir = debugfs_create_dir("flow_table", cls_dir);
-	if (!flow_tbl_dir)
+	if (IS_ERR(flow_tbl_dir))
 		return -ENOMEM;
 
 	for (i = 0; i < MVPP2_CLS_FLOWS_TBL_SIZE; i++) {
-- 
2.37.2

Re: [PATCH] net: mvpp2: Fix error checking

[Russell King (Oracle)]

On Thu, May 18, 2023 at 03:08:11AM +0800, Yeqi Fu wrote:
> The function debugfs_create_dir returns ERR_PTR if an error occurs,
> and the appropriate way to verify for errors is to use the inline
> function IS_ERR. The patch will substitute the null-comparison with
> IS_ERR.

Exactly as I said to a very similar patch received a few days ago
from SikkiLadho:

"The modern wisdom for debugfs is not to check for any errors, so if
we're going to touch this, that's the route that any patch should be
taking.

Thanks."

Your patch seems to have the same Suggested-by: which suggests to me
that you probably know SikkiLadho and are working together with the
person who suggested the change, so it would be good that when a
patch from one of you is commented upon, those comments are taken
into account rather than someone else sending an identical patch to
the first.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: mvpp2: Fix error checking

[Andrew Lunn]

On Wed, May 17, 2023 at 09:11:10PM +0100, Russell King (Oracle) wrote:
> On Thu, May 18, 2023 at 03:08:11AM +0800, Yeqi Fu wrote:
> > The function debugfs_create_dir returns ERR_PTR if an error occurs,
> > and the appropriate way to verify for errors is to use the inline
> > function IS_ERR. The patch will substitute the null-comparison with
> > IS_ERR.
> 
> Exactly as I said to a very similar patch received a few days ago
> from SikkiLadho:
> 
> "The modern wisdom for debugfs is not to check for any errors, so if
> we're going to touch this, that's the route that any patch should be
> taking.
> 
> Thanks."
> 
> Your patch seems to have the same Suggested-by: which suggests to me
> that you probably know SikkiLadho and are working together with the
> person who suggested the change, so it would be good that when a
> patch from one of you is commented upon, those comments are taken
> into account rather than someone else sending an identical patch to
> the first.

Hi Yeqi

Even better would be you write a patch for the bot you are using to
find these issues and teach it that nothing is actually wrong here, or
suggest to remove all checks.

	Andrew

Re: [PATCH] net: mvpp2: Fix error checking

[Simon Horman]

On Thu, May 18, 2023 at 03:08:11AM +0800, Yeqi Fu wrote:
> The function debugfs_create_dir returns ERR_PTR if an error occurs,
> and the appropriate way to verify for errors is to use the inline
> function IS_ERR. The patch will substitute the null-comparison with
> IS_ERR.

The comment above debugfs_create_dir includes the following text.

 * NOTE: it's expected that most callers should _ignore_ the errors returned
 * by this function. Other debugfs functions handle the fact that the "dentry"
 * passed to them could be an error and they don't crash in that case.
 * Drivers should generally work fine even if debugfs fails to init anyway.

And I notice that in this same file there are calls to debugfs_create_dir()
where that advice is followed: the return value is ignored.

So I think the correct approach here is to simply remove the error
checking.

And, to answer my own question while reviewing this. I don't think
Fixes tags are warranted, as debugfs_create_dir() is not expected
to return errors, so there shouldn't be a but in practice. At least
that is my reasoning.

--
pw-bot: cr