[PATCH 0/2] bna: Fix return value check for debugfs create APIs

[PATCH 0/2] bna: Fix return value check for debugfs create APIs

[Zhen Lei]

1. Fix the incorrect return value check for debugfs_create_dir() and
   debugfs_create_file(), which returns ERR_PTR(-ERROR) instead of NULL
   when it fails.
2. Remove field bnad_dentry_files[] in struct bnad. When a directory is
   deleted, files in the directory are automatically deleted. Therefore,
   there is need to record these files.

Zhen Lei (2):
  bna: Fix return value check for debugfs create APIs
  bna: Remove field bnad_dentry_files[] in struct bnad

 drivers/net/ethernet/brocade/bna/bnad.h       |  1 -
 .../net/ethernet/brocade/bna/bnad_debugfs.c   | 34 ++++++++-----------
 2 files changed, 14 insertions(+), 21 deletions(-)

-- 
2.34.1

[PATCH 1/2] bna: Fix return value check for debugfs create APIs

[Zhen Lei]

Fix the incorrect return value check for debugfs_create_dir() and
debugfs_create_file(), which returns ERR_PTR(-ERROR) instead of NULL
when it fails.

Commit 4ad23d2368cc ("bna: Remove error checking for
debugfs_create_dir()") allows the program to continue execution if the
creation of bnad->port_debugfs_root fails, which causes the atomic count
bna_debugfs_port_count to be unbalanced. The corresponding error check
need to be added back.

Fixes: 4ad23d2368cc ("bna: Remove error checking for debugfs_create_dir()")
Fixes: 7afc5dbde091 ("bna: Add debugfs interface.")
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/net/ethernet/brocade/bna/bnad_debugfs.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/brocade/bna/bnad_debugfs.c b/drivers/net/ethernet/brocade/bna/bnad_debugfs.c
index 97291bfbeea589e..ad0b29391f990f3 100644
--- a/drivers/net/ethernet/brocade/bna/bnad_debugfs.c
+++ b/drivers/net/ethernet/brocade/bna/bnad_debugfs.c
@@ -493,25 +493,29 @@ void
 bnad_debugfs_init(struct bnad *bnad)
 {
 	const struct bnad_debugfs_entry *file;
+	struct dentry *de;
 	char name[64];
 	int i;
 
 	/* Setup the BNA debugfs root directory*/
 	if (!bna_debugfs_root) {
-		bna_debugfs_root = debugfs_create_dir("bna", NULL);
+		de = debugfs_create_dir("bna", NULL);
 		atomic_set(&bna_debugfs_port_count, 0);
-		if (!bna_debugfs_root) {
+		if (IS_ERR(de)) {
 			netdev_warn(bnad->netdev,
 				    "debugfs root dir creation failed\n");
 			return;
 		}
+		bna_debugfs_root = de;
 	}
 
 	/* Setup the pci_dev debugfs directory for the port */
 	snprintf(name, sizeof(name), "pci_dev:%s", pci_name(bnad->pcidev));
 	if (!bnad->port_debugfs_root) {
-		bnad->port_debugfs_root =
-			debugfs_create_dir(name, bna_debugfs_root);
+		de = debugfs_create_dir(name, bna_debugfs_root);
+		if (IS_ERR(de))
+			return;
+		bnad->port_debugfs_root = de;
 
 		atomic_inc(&bna_debugfs_port_count);
 
@@ -523,7 +527,7 @@ bnad_debugfs_init(struct bnad *bnad)
 							bnad->port_debugfs_root,
 							bnad,
 							file->fops);
-			if (!bnad->bnad_dentry_files[i]) {
+			if (IS_ERR(bnad->bnad_dentry_files[i])) {
 				netdev_warn(bnad->netdev,
 					    "create %s entry failed\n",
 					    file->name);
-- 
2.34.1

Re: [PATCH 1/2] bna: Fix return value check for debugfs create APIs

[Simon Horman]

On Wed, Oct 23, 2024 at 04:09:20PM +0800, Zhen Lei wrote:
> Fix the incorrect return value check for debugfs_create_dir() and
> debugfs_create_file(), which returns ERR_PTR(-ERROR) instead of NULL
> when it fails.
> 
> Commit 4ad23d2368cc ("bna: Remove error checking for
> debugfs_create_dir()") allows the program to continue execution if the
> creation of bnad->port_debugfs_root fails, which causes the atomic count
> bna_debugfs_port_count to be unbalanced. The corresponding error check
> need to be added back.

Hi Zhen Lei,

The documentation for debugfs_create_dir states:

 * 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.

Which makes me wonder why we are checking the return value of
debugfs_create_dir() at all. Can't we just take advantage of
it not mattering, to debugfs functions, if the return value
is an error or not?

> Fixes: 4ad23d2368cc ("bna: Remove error checking for debugfs_create_dir()")
> Fixes: 7afc5dbde091 ("bna: Add debugfs interface.")
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>

...

Re: [PATCH 1/2] bna: Fix return value check for debugfs create APIs

[Leizhen (ThunderTown)]

On 2024/10/24 20:13, Simon Horman wrote:
> On Wed, Oct 23, 2024 at 04:09:20PM +0800, Zhen Lei wrote:
>> Fix the incorrect return value check for debugfs_create_dir() and
>> debugfs_create_file(), which returns ERR_PTR(-ERROR) instead of NULL
>> when it fails.
>>
>> Commit 4ad23d2368cc ("bna: Remove error checking for
>> debugfs_create_dir()") allows the program to continue execution if the
>> creation of bnad->port_debugfs_root fails, which causes the atomic count
>> bna_debugfs_port_count to be unbalanced. The corresponding error check
>> need to be added back.
> 
> Hi Zhen Lei,
> 
> The documentation for debugfs_create_dir states:
> 
>  * 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.
> 
> Which makes me wonder why we are checking the return value of
> debugfs_create_dir() at all. Can't we just take advantage of
> it not mattering, to debugfs functions, if the return value
> is an error or not?

Do you want to ignore all the return values of debugfs_create_dir() and debugfs_create_file()?
"bna_debugfs_root = debugfs_create_dir("bna", NULL);" and debugfs_create_file() is OK.
I've carefully analyzed the current code, and "bnad->port_debugfs_root = debugfs_create_dir(...);"
is also OK for now.

bnad_debugfs_init():
	bnad->port_debugfs_root = debugfs_create_dir(name, bna_debugfs_root);	//IS_ERR() if fails
(1)
	atomic_inc(&bna_debugfs_port_count);

bnad_debugfs_uninit():
(2)	if (bnad->port_debugfs_root)						//It still works when it's IS_ERR()
		atomic_dec(&bna_debugfs_port_count);

	if (atomic_read(&bna_debugfs_port_count) == 0)
		debugfs_remove(bna_debugfs_root);

If we want the code to be more robust or easier to understand, it is better
to modify (1) and (2) above as follows:
(1) if (IS_ERR(bnad->port_debugfs_root))
	return;
(2) if (!IS_ERR_OR_NULL(bnad->port_debugfs_root))

> 
>> Fixes: 4ad23d2368cc ("bna: Remove error checking for debugfs_create_dir()")
>> Fixes: 7afc5dbde091 ("bna: Add debugfs interface.")
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> 
> ...
> .
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH 1/2] bna: Fix return value check for debugfs create APIs

[Andrew Lunn]

> Do you want to ignore all the return values of debugfs_create_dir() and debugfs_create_file()?

All return values from all debugfs_foo() calls.

debugfs has been designed so that it should be robust if any previous
call to debugfs failed. It will not crash, it will just keep going.

It does not matter if the contents of debugfs are messed up as a
result, it is not an ABI, you cannot trust it to contain anything
useful, and it might be missing all together, since it is optional.

	Andrew

Re: [PATCH 1/2] bna: Fix return value check for debugfs create APIs

[Leizhen (ThunderTown)]

On 2024/10/24 22:04, Andrew Lunn wrote:
>> Do you want to ignore all the return values of debugfs_create_dir() and debugfs_create_file()?
> 
> All return values from all debugfs_foo() calls.

I searched. Currently, bna only involves functions debugfs_create_dir() and
debugfs_create_file(). debugfs_remove() has no return value.


git grep -n "\bdebugfs_" drivers/net/ethernet/brocade/bna/
drivers/net/ethernet/brocade/bna/bnad_debugfs.c:501:            bna_debugfs_root = debugfs_create_dir("bna", NULL);
drivers/net/ethernet/brocade/bna/bnad_debugfs.c:514:                    debugfs_create_dir(name, bna_debugfs_root);
drivers/net/ethernet/brocade/bna/bnad_debugfs.c:521:                                    debugfs_create_file(file->name,
drivers/net/ethernet/brocade/bna/bnad_debugfs.c:544:                    debugfs_remove(bnad->bnad_dentry_files[i]);
drivers/net/ethernet/brocade/bna/bnad_debugfs.c:551:            debugfs_remove(bnad->port_debugfs_root);
drivers/net/ethernet/brocade/bna/bnad_debugfs.c:558:            debugfs_remove(bna_debugfs_root);


> 
> debugfs has been designed so that it should be robust if any previous
> call to debugfs failed. It will not crash, it will just keep going.
> 
> It does not matter if the contents of debugfs are messed up as a
> result, it is not an ABI, you cannot trust it to contain anything
> useful, and it might be missing all together, since it is optional.

Okay, thank you for your detailed explanation.

> 
> 	Andrew
> .
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH 1/2] bna: Fix return value check for debugfs create APIs

[Simon Horman]

On Thu, Oct 24, 2024 at 09:26:30PM +0800, Leizhen (ThunderTown) wrote:
> 
> 
> On 2024/10/24 20:13, Simon Horman wrote:
> > On Wed, Oct 23, 2024 at 04:09:20PM +0800, Zhen Lei wrote:
> >> Fix the incorrect return value check for debugfs_create_dir() and
> >> debugfs_create_file(), which returns ERR_PTR(-ERROR) instead of NULL
> >> when it fails.
> >>
> >> Commit 4ad23d2368cc ("bna: Remove error checking for
> >> debugfs_create_dir()") allows the program to continue execution if the
> >> creation of bnad->port_debugfs_root fails, which causes the atomic count
> >> bna_debugfs_port_count to be unbalanced. The corresponding error check
> >> need to be added back.
> > 
> > Hi Zhen Lei,
> > 
> > The documentation for debugfs_create_dir states:
> > 
> >  * 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.
> > 
> > Which makes me wonder why we are checking the return value of
> > debugfs_create_dir() at all. Can't we just take advantage of
> > it not mattering, to debugfs functions, if the return value
> > is an error or not?
> 
> Do you want to ignore all the return values of debugfs_create_dir() and debugfs_create_file()?
> "bna_debugfs_root = debugfs_create_dir("bna", NULL);" and debugfs_create_file() is OK.
> I've carefully analyzed the current code, and "bnad->port_debugfs_root = debugfs_create_dir(...);"
> is also OK for now.

What I'm saying is that it is unusual to depend on the return value of
debugfs_create_dir() for anything. And it would be best to avoid doing so.

But perhaps that isn't possible for some reason?

> 
> bnad_debugfs_init():
> 	bnad->port_debugfs_root = debugfs_create_dir(name, bna_debugfs_root);	//IS_ERR() if fails
> (1)
> 	atomic_inc(&bna_debugfs_port_count);
> 
> bnad_debugfs_uninit():
> (2)	if (bnad->port_debugfs_root)						//It still works when it's IS_ERR()
> 		atomic_dec(&bna_debugfs_port_count);
> 
> 	if (atomic_read(&bna_debugfs_port_count) == 0)
> 		debugfs_remove(bna_debugfs_root);
> 
> If we want the code to be more robust or easier to understand, it is better
> to modify (1) and (2) above as follows:
> (1) if (IS_ERR(bnad->port_debugfs_root))
> 	return;
> (2) if (!IS_ERR_OR_NULL(bnad->port_debugfs_root))
> 
> > 
> >> Fixes: 4ad23d2368cc ("bna: Remove error checking for debugfs_create_dir()")
> >> Fixes: 7afc5dbde091 ("bna: Add debugfs interface.")
> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> > 
> > ...
> > .
> > 
> 
> -- 
> Regards,
>   Zhen Lei
> 

Re: [PATCH 1/2] bna: Fix return value check for debugfs create APIs

[Leizhen (ThunderTown)]

On 2024/10/24 23:27, Simon Horman wrote:
> On Thu, Oct 24, 2024 at 09:26:30PM +0800, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2024/10/24 20:13, Simon Horman wrote:
>>> On Wed, Oct 23, 2024 at 04:09:20PM +0800, Zhen Lei wrote:
>>>> Fix the incorrect return value check for debugfs_create_dir() and
>>>> debugfs_create_file(), which returns ERR_PTR(-ERROR) instead of NULL
>>>> when it fails.
>>>>
>>>> Commit 4ad23d2368cc ("bna: Remove error checking for
>>>> debugfs_create_dir()") allows the program to continue execution if the
>>>> creation of bnad->port_debugfs_root fails, which causes the atomic count
>>>> bna_debugfs_port_count to be unbalanced. The corresponding error check
>>>> need to be added back.
>>>
>>> Hi Zhen Lei,
>>>
>>> The documentation for debugfs_create_dir states:
>>>
>>>  * 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.
>>>
>>> Which makes me wonder why we are checking the return value of
>>> debugfs_create_dir() at all. Can't we just take advantage of
>>> it not mattering, to debugfs functions, if the return value
>>> is an error or not?
>>
>> Do you want to ignore all the return values of debugfs_create_dir() and debugfs_create_file()?
>> "bna_debugfs_root = debugfs_create_dir("bna", NULL);" and debugfs_create_file() is OK.
>> I've carefully analyzed the current code, and "bnad->port_debugfs_root = debugfs_create_dir(...);"
>> is also OK for now.
> 
> What I'm saying is that it is unusual to depend on the return value of
> debugfs_create_dir() for anything. And it would be best to avoid doing so.
> 
> But perhaps that isn't possible for some reason?

OK, I understand now. Please forgive my poor English. Combine Andrew's reply
and my analysis above. The return value check for the remaining two places
should now be removed.

> 
>>
>> bnad_debugfs_init():
>> 	bnad->port_debugfs_root = debugfs_create_dir(name, bna_debugfs_root);	//IS_ERR() if fails
>> (1)
>> 	atomic_inc(&bna_debugfs_port_count);
>>
>> bnad_debugfs_uninit():
>> (2)	if (bnad->port_debugfs_root)						//It still works when it's IS_ERR()
>> 		atomic_dec(&bna_debugfs_port_count);
>>
>> 	if (atomic_read(&bna_debugfs_port_count) == 0)
>> 		debugfs_remove(bna_debugfs_root);
>>
>> If we want the code to be more robust or easier to understand, it is better
>> to modify (1) and (2) above as follows:
>> (1) if (IS_ERR(bnad->port_debugfs_root))
>> 	return;
>> (2) if (!IS_ERR_OR_NULL(bnad->port_debugfs_root))
>>
>>>
>>>> Fixes: 4ad23d2368cc ("bna: Remove error checking for debugfs_create_dir()")
>>>> Fixes: 7afc5dbde091 ("bna: Add debugfs interface.")
>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>
>>> ...
>>> .
>>>
>>
>> -- 
>> Regards,
>>   Zhen Lei
>>
> .
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH 1/2] bna: Fix return value check for debugfs create APIs

[Simon Horman]

On Fri, Oct 25, 2024 at 12:17:17PM +0800, Leizhen (ThunderTown) wrote:
> 
> 
> On 2024/10/24 23:27, Simon Horman wrote:
> > On Thu, Oct 24, 2024 at 09:26:30PM +0800, Leizhen (ThunderTown) wrote:
> >>
> >>
> >> On 2024/10/24 20:13, Simon Horman wrote:
> >>> On Wed, Oct 23, 2024 at 04:09:20PM +0800, Zhen Lei wrote:
> >>>> Fix the incorrect return value check for debugfs_create_dir() and
> >>>> debugfs_create_file(), which returns ERR_PTR(-ERROR) instead of NULL
> >>>> when it fails.
> >>>>
> >>>> Commit 4ad23d2368cc ("bna: Remove error checking for
> >>>> debugfs_create_dir()") allows the program to continue execution if the
> >>>> creation of bnad->port_debugfs_root fails, which causes the atomic count
> >>>> bna_debugfs_port_count to be unbalanced. The corresponding error check
> >>>> need to be added back.
> >>>
> >>> Hi Zhen Lei,
> >>>
> >>> The documentation for debugfs_create_dir states:
> >>>
> >>>  * 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.
> >>>
> >>> Which makes me wonder why we are checking the return value of
> >>> debugfs_create_dir() at all. Can't we just take advantage of
> >>> it not mattering, to debugfs functions, if the return value
> >>> is an error or not?
> >>
> >> Do you want to ignore all the return values of debugfs_create_dir() and debugfs_create_file()?
> >> "bna_debugfs_root = debugfs_create_dir("bna", NULL);" and debugfs_create_file() is OK.
> >> I've carefully analyzed the current code, and "bnad->port_debugfs_root = debugfs_create_dir(...);"
> >> is also OK for now.
> > 
> > What I'm saying is that it is unusual to depend on the return value of
> > debugfs_create_dir() for anything. And it would be best to avoid doing so.
> > 
> > But perhaps that isn't possible for some reason?
> 
> OK, I understand now. Please forgive my poor English. Combine Andrew's reply
> and my analysis above. The return value check for the remaining two places
> should now be removed.

Thanks. Sorry that I was not clearer.

...

[PATCH 2/2] bna: Remove field bnad_dentry_files[] in struct bnad

[Zhen Lei]

Function debugfs_remove() recursively removes a directory, include all
files created by debugfs_create_file(). Therefore, there is no need to
explicitly record each file with member ->bnad_dentry_files[] and
explicitly delete them at the end. Delete field bnad_dentry_files[] and
its related processing codes for optimization.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/net/ethernet/brocade/bna/bnad.h       |  1 -
 .../net/ethernet/brocade/bna/bnad_debugfs.c   | 22 +++++--------------
 2 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/brocade/bna/bnad.h b/drivers/net/ethernet/brocade/bna/bnad.h
index 10b1e534030e628..4396997c59d041f 100644
--- a/drivers/net/ethernet/brocade/bna/bnad.h
+++ b/drivers/net/ethernet/brocade/bna/bnad.h
@@ -351,7 +351,6 @@ struct bnad {
 	/* debugfs specific data */
 	char	*regdata;
 	u32	reglen;
-	struct dentry *bnad_dentry_files[5];
 	struct dentry *port_debugfs_root;
 };
 
diff --git a/drivers/net/ethernet/brocade/bna/bnad_debugfs.c b/drivers/net/ethernet/brocade/bna/bnad_debugfs.c
index ad0b29391f990f3..317b5c3ffb10251 100644
--- a/drivers/net/ethernet/brocade/bna/bnad_debugfs.c
+++ b/drivers/net/ethernet/brocade/bna/bnad_debugfs.c
@@ -521,13 +521,12 @@ bnad_debugfs_init(struct bnad *bnad)
 
 		for (i = 0; i < ARRAY_SIZE(bnad_debugfs_files); i++) {
 			file = &bnad_debugfs_files[i];
-			bnad->bnad_dentry_files[i] =
-					debugfs_create_file(file->name,
-							file->mode,
-							bnad->port_debugfs_root,
-							bnad,
-							file->fops);
-			if (IS_ERR(bnad->bnad_dentry_files[i])) {
+			de = debugfs_create_file(file->name,
+						 file->mode,
+						 bnad->port_debugfs_root,
+						 bnad,
+						 file->fops);
+			if (IS_ERR(de)) {
 				netdev_warn(bnad->netdev,
 					    "create %s entry failed\n",
 					    file->name);
@@ -541,15 +540,6 @@ bnad_debugfs_init(struct bnad *bnad)
 void
 bnad_debugfs_uninit(struct bnad *bnad)
 {
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(bnad_debugfs_files); i++) {
-		if (bnad->bnad_dentry_files[i]) {
-			debugfs_remove(bnad->bnad_dentry_files[i]);
-			bnad->bnad_dentry_files[i] = NULL;
-		}
-	}
-
 	/* Remove the pci_dev debugfs directory for the port */
 	if (bnad->port_debugfs_root) {
 		debugfs_remove(bnad->port_debugfs_root);
-- 
2.34.1