net/mac80211/mesh_plink: Convert timers to use

PATCH In review Kees Cook 2 participants 3 messages Oct 5, 2017

Subsystems (3)

Networking Network drivers mac80211

lore

Share & tools

lore

Kees Cook

author

2017-10-05 00:49 UTC #0 lore raw

PATCH 1/1 net/mac80211/mesh_plink: Convert timers to use

In preparation for unconditionally passing the struct timer_list pointer to all timer callbacks, switch to using the new timer_setup() and from_timer() to pass the timer pointer explicitly. This requires adding a pointer back to the sta_info since container_of() can't resolve the sta_info.

Cc Johannes Berg <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc [email protected]

Cc Thomas Gleixner <[email protected]>

Signed-off-by Kees Cook <[email protected]>

---
This requires commit 686fef928bba ("timer: Prepare to change timer callback argument type") in v4.14-rc3, but should be otherwise stand-alone.

---
 net/mac80211/mesh_plink.c | 9 +++++----
 net/mac80211/sta_info.c   | 2 +-
 net/mac80211/sta_info.h   | 2 ++
 3 files changed, 8 insertions(+), 5 deletions(-)

net/mac80211/mesh_plink.c +5 -4

index f69c6c38ca43..fcc02beaee6d 100644
@@ -604,8 +604,9 @@ void mesh_neighbour_update(struct ieee80211_sub_if_data *sdata,
 	ieee80211_mbss_info_change_notify(sdata, changed);
 }
 
-static void mesh_plink_timer(unsigned long data)
+static void mesh_plink_timer(struct timer_list *t)
 {
+	struct mesh_sta *mesh = from_timer(mesh, t, plink_timer);
 	struct sta_info *sta;
 	u16 reason = 0;
 	struct ieee80211_sub_if_data *sdata;

@@ -617,7 +618,7 @@ static void mesh_plink_timer(unsigned long data)
 	 * del_timer_sync() this timer after having made sure
 	 * it cannot be readded (by deleting the plink.)
 	 */
-	sta = (struct sta_info *) data;
+	sta = mesh->plink_sta;
 
 	if (sta->sdata->local->quiescing)
 		return;

@@ -698,8 +699,8 @@ static void mesh_plink_timer(unsigned long data)
 static inline void mesh_plink_timer_set(struct sta_info *sta, u32 timeout)
 {
 	sta->mesh->plink_timer.expires = jiffies + msecs_to_jiffies(timeout);
-	sta->mesh->plink_timer.data = (unsigned long) sta;
-	sta->mesh->plink_timer.function = mesh_plink_timer;
+	sta->mesh->plink_sta = sta;
+	sta->mesh->plink_timer.function = (TIMER_FUNC_TYPE)mesh_plink_timer;
 	sta->mesh->plink_timeout = timeout;
 	add_timer(&sta->mesh->plink_timer);
 }

net/mac80211/sta_info.c +1 -1

index 69615016d5bf..5e5de9455e4e 100644
@@ -332,7 +332,7 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
 		spin_lock_init(&sta->mesh->plink_lock);
 		if (ieee80211_vif_is_mesh(&sdata->vif) &&
 		    !sdata->u.mesh.user_mpm)
-			init_timer(&sta->mesh->plink_timer);
+			timer_setup(&sta->mesh->plink_timer, NULL, 0);
 		sta->mesh->nonpeer_pm = NL80211_MESH_POWER_ACTIVE;
 	}
 #endif

net/mac80211/sta_info.h +2 -0

index 3acbdfa9f649..21d9760ce5c3 100644
@@ -344,6 +344,7 @@ DECLARE_EWMA(mesh_fail_avg, 20, 8)
  * @plink_state: peer link state
  * @plink_timeout: timeout of peer link
  * @plink_timer: peer link watch timer
+ * @plink_sta: peer link watch timer's sta_info
  * @t_offset: timing offset relative to this host
  * @t_offset_setpoint: reference timing offset of this sta to be used when
  * 	calculating clockdrift

@@ -356,6 +357,7 @@ DECLARE_EWMA(mesh_fail_avg, 20, 8)
  */
 struct mesh_sta {
 	struct timer_list plink_timer;
+	struct sta_info *plink_sta;
 
 	s64 t_offset;
 	s64 t_offset_setpoint;

Signature · 6 lines

2.7.4


-- 
Kees Cook
Pixel Security

Johannes Berg

maintainer

2017-10-05 06:47 UTC #1 lore raw

Quoted 5 lines · Kees Cook

In preparation for unconditionally passing the struct timer_list pointer to all timer callbacks, switch to using the new timer_setup() and from_timer() to pass the timer pointer explicitly. This requires adding a pointer back to the sta_info since container_of() can't resolve the sta_info.

The subject seems to be lacking something ... :-)

Quoted 3 lines

This requires commit 686fef928bba ("timer: Prepare to change timer callback argument type") in v4.14-rc3, but should be otherwise stand-alone.

I still can't apply that because that's not in net-next right now.

net/mac80211/mesh_plink.c static void mesh_plink_timer(unsigned long data) · line 705

Quoted 12 lines

static inline void mesh_plink_timer_set(struct sta_info *sta, u32
timeout)
{
sta->mesh->plink_timer.expires = jiffies +
msecs_to_jiffies(timeout);
net/mac80211/mesh_plink.c +2 -2
-	sta->mesh->plink_timer.data = (unsigned long) sta;
-	sta->mesh->plink_timer.function = mesh_plink_timer;
+	sta->mesh->plink_sta = sta;
+	sta->mesh->plink_timer.function =
(TIMER_FUNC_TYPE)mesh_plink_timer;
net/mac80211/mesh_plink.c +0 -0
 	sta->mesh->plink_timeout = timeout;
 	add_timer(&sta->mesh->plink_timer);

Wouldn't it be better to convert this to timer_setup() now?

That add_timer() should probably also be mod_timer() anyway?

Quoted 13 lines

net/mac80211/sta_info.c +1 -1

index 69615016d5bf..5e5de9455e4e 100644
@@ -332,7 +332,7 @@ struct sta_info *sta_info_alloc(struct
 ieee80211_sub_if_data *sdata,
 		spin_lock_init(&sta->mesh->plink_lock);
 		if (ieee80211_vif_is_mesh(&sdata->vif) &&
 		    !sdata->u.mesh.user_mpm)
-			init_timer(&sta->mesh->plink_timer);
+			timer_setup(&sta->mesh->plink_timer, NULL,
 0);
 		sta->mesh->nonpeer_pm = NL80211_MESH_POWER_ACTIVE;

Quoted 1 line

}

You just have to make mesh_plink_timer() non-static, put a prototype into mesh.h and then you can use the proper timer_setup() here with the function?

Also, the sta->mesh->plink_sta assignment should be here I'd say, no point rewriting it all the time.

I guess you were shooting for minimal, and I suppose we can do the cleanups later too ...

johannes

Kees Cook

author

2017-10-05 17:27 UTC #2 lore raw

Quoted 8 lines · Johannes Berg

Quoted 5 lines · Kees Cook
In preparation for unconditionally passing the struct timer_list pointer to all timer callbacks, switch to using the new timer_setup() and from_timer() to pass the timer pointer explicitly. This requires adding a pointer back to the sta_info since container_of() can't resolve the sta_info.
The subject seems to be lacking something ... :-)

Oh wonderful, all the subjects are cut off. *sigh* I wonder which piece of my workflow broke that...

Quoted 5 lines

Quoted 3 lines
This requires commit 686fef928bba ("timer: Prepare to change timer callback argument type") in v4.14-rc3, but should be otherwise stand-alone.
I still can't apply that because that's not in net-next right now.

Okay, I'll see if Dave brings that into net-next and resend after that.

Quoted 13 lines

Quoted 9 lines

static inline void mesh_plink_timer_set(struct sta_info *sta, u32
timeout)
 {
      sta->mesh->plink_timer.expires = jiffies +
msecs_to_jiffies(timeout);
net/mac80211/mesh_plink.c +2 -2
-     sta->mesh->plink_timer.data = (unsigned long) sta;
-     sta->mesh->plink_timer.function = mesh_plink_timer;
+     sta->mesh->plink_sta = sta;
+     sta->mesh->plink_timer.function =

Quoted 3 lines

(TIMER_FUNC_TYPE)mesh_plink_timer;
      sta->mesh->plink_timeout = timeout;
      add_timer(&sta->mesh->plink_timer);

Quoted 1 line

Wouldn't it be better to convert this to timer_setup() now?

The problem is that plink_timer is used in several places, and it's originally initialized in net/mac80211/sta_info.c. The call to mesh_plink_timer_set() does an update of the function field, so it didn't look like it could get merged with the timer_setup(), but in looking again, it seems that this is the _only_ update to plink_timer.function, so it could probably get collapsed into the timer_setup() call.

Quoted 1 line

That add_timer() should probably also be mod_timer() anyway?

Agreed. I'd avoided making those changes in most places, but I can do it here.

Quoted 19 lines

Quoted 13 lines

net/mac80211/sta_info.c +1 -1

index 69615016d5bf..5e5de9455e4e 100644
@@ -332,7 +332,7 @@ struct sta_info *sta_info_alloc(struct
 ieee80211_sub_if_data *sdata,
              spin_lock_init(&sta->mesh->plink_lock);
              if (ieee80211_vif_is_mesh(&sdata->vif) &&
                  !sdata->u.mesh.user_mpm)
-                     init_timer(&sta->mesh->plink_timer);
+                     timer_setup(&sta->mesh->plink_timer, NULL,
 0);
              sta->mesh->nonpeer_pm = NL80211_MESH_POWER_ACTIVE;

Quoted 1 line

You just have to make mesh_plink_timer() non-static, put a prototype into mesh.h and then you can use the proper timer_setup() here with the function?

Quoted 2 lines

Also, the sta->mesh->plink_sta assignment should be here I'd say, no point rewriting it all the time.

Sounds good. I'll get it cleaned up.

-Kees

Signature · 2 lines

Kees Cook
Pixel Security