Skip to contentSkip to conversation

wifi: brcmsmac: use FAM for debug code

PATCH In review Rosen Penev 3 participants 4 messages Mar 9 – Mar 28 Broadcom BRCM80211 IEEE802.11 wireless drivers

lore
Share & tools
lore

Rosen Penev

author
#0 lore raw

PATCH v2 1/1 wifi: brcmsmac: use FAM for debug code

Debug code requires a separate allocation to duplicate a string. A FAM allows properly sized allocation with a single kfree.

Signed-off-by Rosen Penev <[email protected]>

---
v2: rebase and remove debug from member.

 .../broadcom/brcm80211/brcmsmac/mac80211_if.c     | 15 +++++----------
 .../broadcom/brcm80211/brcmsmac/mac80211_if.h     |  4 +---
 2 files changed, 6 insertions(+), 13 deletions(-)
drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c +5 -10
index 6255d673d2d3..7912a999f6f7 100644
@@ -317,9 +317,6 @@ static void brcms_free(struct brcms_info *wl)
 	/* free timers */
 	for (t = wl->timers; t; t = next) {
 		next = t->next;
-#ifdef DEBUG
-		kfree(t->name);
-#endif
 		kfree(t);
 	}
 }

@@ -1499,7 +1496,11 @@ struct brcms_timer *brcms_init_timer(struct brcms_info *wl,
 {
 	struct brcms_timer *t;
 
+#ifdef DEBUG
+	t = kzalloc_flex(*t, name, strlen(name) + 1, GFP_ATOMIC);
+#else
 	t = kzalloc_obj(*t, GFP_ATOMIC);
+#endif
 	if (!t)
 		return NULL;
 

@@ -1511,7 +1512,7 @@ struct brcms_timer *brcms_init_timer(struct brcms_info *wl,
 	wl->timers = t;
 
 #ifdef DEBUG
-	t->name = kstrdup(name, GFP_ATOMIC);
+	strcpy(t->name, name);
 #endif
 
 	return t;

@@ -1574,9 +1575,6 @@ void brcms_free_timer(struct brcms_timer *t)
 
 	if (wl->timers == t) {
 		wl->timers = wl->timers->next;
-#ifdef DEBUG
-		kfree(t->name);
-#endif
 		kfree(t);
 		return;
 

@@ -1586,9 +1584,6 @@ void brcms_free_timer(struct brcms_timer *t)
 	while (tmp) {
 		if (tmp->next == t) {
 			tmp->next = t->next;
-#ifdef DEBUG
-			kfree(t->name);
-#endif
 			kfree(t);
 			return;
 		}
drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.h +1 -3
index eaf926a96a88..3b25a56958b3 100644
@@ -42,9 +42,7 @@ struct brcms_timer {
 	bool periodic;
 	bool set;		/* indicates if timer is active */
 	struct brcms_timer *next;	/* for freeing on unload */
-#ifdef DEBUG
-	char *name;		/* Description of the timer */
-#endif
+	char name[];		/* Description of the timer */
 };
 
 struct brcms_if {

--
2.53.0

#1 lore raw

Hi Arend,

Quoted 3 lines · Rosen Penev

Debug code requires a separate allocation to duplicate a string. A FAM allows properly sized allocation with a single kfree.

Sorry Rosen for hijacking your patch here.

With these changes, does allocating and copying the string really need to be behind a DEBUG ifdef?

The allocation, copying and freeing of the memory isn't in a hot path, so allocating/freeing a couple more bytes shouldn't matter that much, which only leaves the memory footprint, which appears to be less than 10 bytes.

Thanks,

Signature · 4 lines
Julian Calaby

Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
author
#2 lore raw
Quoted 5 lines · Julian Calaby

Hi Arend,

Quoted 1 line · Rosen Penev
Quoted 7 lines
Quoted 2 lines

Debug code requires a separate allocation to duplicate a string. A FAM allows properly sized allocation with a single kfree.

Sorry Rosen for hijacking your patch here.

With these changes, does allocating and copying the string really need to be behind a DEBUG ifdef?

I don't know. I didn't write this code.

Quoted 13 lines

The allocation, copying and freeing of the memory isn't in a hot path, so allocating/freeing a couple more bytes shouldn't matter that much, which only leaves the memory footprint, which appears to be less than 10 bytes.

Thanks,

--
Julian Calaby

Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/

maintainer
#3 lore raw
Quoted 6 lines · Rosen Penev
Quoted 5 lines · Julian Calaby

Hi Arend,

Quoted 1 line · Rosen Penev
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines

Debug code requires a separate allocation to duplicate a string. A FAM allows properly sized allocation with a single kfree.

Sorry Rosen for hijacking your patch here.

With these changes, does allocating and copying the string really need to be behind a DEBUG ifdef?

I don't know. I didn't write this code.

Thanks, Rosen

I did before the concept of FAM landed in the kernel. Whether or not the #ifdef DEBUG is warranted is simply a choice. I prefer to have clean separation of functionality and the related data. If the code using the data is all conditional under DEBUG define then the data must be as well.

I understand that FAM and compiler support for it has its advantages, but this is more churn than gain. The code is functional as is and removing #defines for the sake of changing to a FAM seems not justified for the advantages which in my opinion are pretty limited in this particular case.

Regards,
Arend