[PATCH 1/1 net-next] mac80211: remove unnecessary null test before debugfs_remove

[PATCH 1/1 net-next] mac80211: remove unnecessary null test before debugfs_remove

[Fabian Frederick]

Fix checkpatch warnings:

    WARNING: debugfs_remove(NULL) is safe this check is probably not required

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 net/mac80211/debugfs_key.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/net/mac80211/debugfs_key.c b/net/mac80211/debugfs_key.c
index 1521cab..ec40f27 100644
--- a/net/mac80211/debugfs_key.c
+++ b/net/mac80211/debugfs_key.c
@@ -299,11 +299,8 @@ void ieee80211_debugfs_key_update_default(struct ieee80211_sub_if_data *sdata)
 		return;
 
 	lockdep_assert_held(&sdata->local->key_mtx);
-
-	if (sdata->debugfs.default_unicast_key) {
-		debugfs_remove(sdata->debugfs.default_unicast_key);
-		sdata->debugfs.default_unicast_key = NULL;
-	}
+	debugfs_remove(sdata->debugfs.default_unicast_key);
+	sdata->debugfs.default_unicast_key = NULL;
 
 	if (sdata->default_unicast_key) {
 		key = key_mtx_dereference(sdata->local,
@@ -314,10 +311,8 @@ void ieee80211_debugfs_key_update_default(struct ieee80211_sub_if_data *sdata)
 					       sdata->vif.debugfs_dir, buf);
 	}
 
-	if (sdata->debugfs.default_multicast_key) {
-		debugfs_remove(sdata->debugfs.default_multicast_key);
-		sdata->debugfs.default_multicast_key = NULL;
-	}
+	debugfs_remove(sdata->debugfs.default_multicast_key);
+	sdata->debugfs.default_multicast_key = NULL;
 
 	if (sdata->default_multicast_key) {
 		key = key_mtx_dereference(sdata->local,
-- 
1.9.3

Re: [PATCH 1/1 net-next] mac80211: remove unnecessary null test before debugfs_remove

[Johannes Berg]

On Tue, 2014-10-21 at 18:20 +0200, Fabian Frederick wrote:
> Fix checkpatch warnings:
> 
>     WARNING: debugfs_remove(NULL) is safe this check is probably not required

I'll apply this; however, I think that checkpatch is a just tool, and
the commit message should reflect why you're changing the code.
Presumably you're not doing it to make the tool happy, but to address an
issue that the tool pointed out, so I think in most cases the commit
message should state the former, not the latter.

Note that in this particular case the NULL check check could be there to
avoid a memory write (which can be significant depending on the context)
so blindly doing what the tool suggested wouldn't always be a good idea.

johannes

Re: [PATCH 1/1 net-next] mac80211: remove unnecessary null test before debugfs_remove

[Fabian Frederick]

> On 21 October 2014 at 21:06 Johannes Berg <johannes@sipsolutions.net> wrote:
>
>
> On Tue, 2014-10-21 at 18:20 +0200, Fabian Frederick wrote:
> > Fix checkpatch warnings:
> >
> >     WARNING: debugfs_remove(NULL) is safe this check is probably not
> >required
>
> I'll apply this; however, I think that checkpatch is a just tool, and
> the commit message should reflect why you're changing the code.
> Presumably you're not doing it to make the tool happy, but to address an
> issue that the tool pointed out, so I think in most cases the commit
> message should state the former, not the latter.
>
> Note that in this particular case the NULL check check could be there to
> avoid a memory write (which can be significant depending on the context)
> so blindly doing what the tool suggested wouldn't always be a good idea.
>
> johannes
>

Thanks Johannes,

Maybe you can replace commit message with:
"
This patch removes NULL check before debugfs_remove.
That function already does that check and is
only called during key management so we can add some memory writes.
"

I can also resubmit patch if necessary.

Regards,
Fabian

Re: [PATCH 1/1 net-next] mac80211: remove unnecessary null test before debugfs_remove

[Johannes Berg]

On Tue, 2014-10-21 at 22:05 +0200, Fabian Frederick wrote:

> I can also resubmit patch if necessary.

No worries, I've already applied the patch (with a modified commit
message).

johannes