drivers:net:wireless: Add mutex locking for function, b43_op_beacon_set_time in main.c
Subsystems (3)
PATCH 1/1 drivers:net:wireless: Add mutex locking for function, b43_op_beacon_set_time in main.c
Adds needed mutex lockng of wl->mutex in order to prevent issues with separate threads executing on the b43_update_templates function call in the function, b43_op_beacon_set_time at the same time.
--- drivers/net/wireless/b43/main.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/wireless/b43/main.c +2 -1
@@ -5094,8 +5094,9 @@ static int b43_op_beacon_set_tim(struct ieee80211_hw *hw, {struct b43_wl *wl =hw_to_b43_wl (hw); -/* FIXME: add locking */ +mutex_lock (&wl->mutex);b43_update_templates (wl); +mutex_unlock (&wl->mutex);return 0 ; }
Signature · 1 line
2.1.0
Quoted 2 lines · Nicholas Krause
Adds needed mutex lockng of
wl->mutexin order to prevent issues with separate threads executing on theb43_update_templatesfunction call in the function,b43_op_beacon_set_timeat the same time.
For all kind of kernel documentation / descriptions we try to fit 72 chars limit and we really try to avoid exceeding 80 chars.
drivers/net/wireless/b43/main.c static int b43_op_beacon_set_tim(struct ieee80211_hw *hw, · line 5102
Quoted 11 lines
drivers/net/wireless/b43/main.c +2
-1@@ -5094,8 +5094,9 @@ static int b43_op_beacon_set_tim(struct ieee80211_hw *hw, {struct b43_wl *wl =hw_to_b43_wl (hw); -/* FIXME: add locking */ +mutex_lock (&wl->mutex);b43_update_templates (wl); +mutex_unlock (&wl->mutex);return 0 ; }
Does anyone remember why this simple solution wasn't implemented earlier? Michael?
Nicholas: did you test it anyhow?
Quoted 9 lines · On Fri, 28 Nov 2014 23:40:46 +0100 Rafa? Mi?ecki <[email protected]> wrote:
Quoted 9 lines
drivers/net/wireless/b43/main.c +2
-1@@ -5094,8 +5094,9 @@ static int b43_op_beacon_set_tim(struct ieee80211_hw *hw, {struct b43_wl *wl =hw_to_b43_wl (hw); -/* FIXME: add locking */ +mutex_lock (&wl->mutex);b43_update_templates (wl); +mutex_unlock (&wl->mutex);
Quoted 5 lines
drivers/net/wireless/b43/main.c static int b43_op_beacon_set_tim(struct ieee80211_hw *hw, · line 5102
Quoted 2 lines
drivers/net/wireless/b43/main.c +0
-0return 0 ; }Does anyone remember why this simple solution wasn't implemented earlier? Michael?
I think the callback used to be (is?) in atomic context.
Quoted 1 line
Nicholas: did you test it anyhow?
Signature · 8 lines
Michael -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/b43-dev/attachments/20141129/76b5d0f6/attachment.sig>
Michael,
I don't have hardware for this driver on me, so I didn't test it. However this seems to be correct from my reading of the code around this function and other locking related to this driver.
Cheers Nick
Quoted 22 lines · Michael B?sch
Quoted 9 lines · On Fri, 28 Nov 2014 23:40:46 +0100 Rafa? Mi?ecki <[email protected]> wrote:
Quoted 9 lines
drivers/net/wireless/b43/main.c +2
-1@@ -5094,8 +5094,9 @@ static int b43_op_beacon_set_tim(struct ieee80211_hw *hw, {struct b43_wl *wl =hw_to_b43_wl (hw); -/* FIXME: add locking */ +mutex_lock (&wl->mutex);b43_update_templates (wl); +mutex_unlock (&wl->mutex);Quoted 5 lines
drivers/net/wireless/b43/main.c static int b43_op_beacon_set_tim(struct ieee80211_hw *hw, · line 5102
Quoted 2 lines
drivers/net/wireless/b43/main.c +0
-0return 0 ; }Does anyone remember why this simple solution wasn't implemented earlier? Michael?
I think the callback used to be (is?) in atomic context.
Quoted 1 line
Nicholas: did you test it anyhow?
Quoted 3 lines · nick
I don't have hardware for this driver on me, so I didn't test it. However this seems to be correct from my reading of the code around this function and other locking related to this driver.
So do you say it's not executed in an atomic?
From *early* look it seems b43_update_templates is called from b43_op_beacon_set_tim and b43_op_bss_info_changed, both or them are mac80211 callbacks. That also should be safe, however I didn't check if it may conflict with some in-kernel code (still, assuming it's not atomic code, which I don't know!).
Quoted 3 lines · On Fri, 28 Nov 2014 22:32:30 -0500 nick <[email protected]> wrote:
I don't have hardware for this driver on me, so I didn't test it. However this seems to be correct from my reading of the code around this function and other locking related to this driver.
From the current docs:
Quoted 2 lines
* @set_tim: Set TIM bit. mac80211 calls this function when a TIM bit
* must be set or cleared for a given STA. Must be atomic.
So it is not allowed to lock a mutex here.
Signature · 8 lines
Michael -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/b43-dev/attachments/20141129/9e9fdd70/attachment.sig>
Quoted 7 lines · Michael B?sch
Quoted 3 lines · On Fri, 28 Nov 2014 22:32:30 -0500 nick <[email protected]> wrote:
I don't have hardware for this driver on me, so I didn't test it. However this seems to be correct from my reading of the code around this function and other locking related to this driver.
Quoted 6 lines
From the current docs:
Quoted 2 lines
* @set_tim: Set TIM bit. mac80211 calls this function when a TIM bit
* must be set or cleared for a given STA. Must be atomic.So it is not allowed to lock a mutex here.
Nicholas please be more careful with your patches. It seems your patch is wrong, you didn't check the docs and you didn't even say in the first place that it wasn't tested. You could at least submit with with RFT tag or sth.
Signature · 1 line
Rafa?
A message that is not in the archive (sent off-list or not yet received): CACna6ryjZ-LY06wAMzoL77Wca3McUUT+VYsO4EPO87NqjBnaNw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Sorry about that, next time I will be more careful. Nick
Quoted 20 lines · Rafa? Mi?ecki
Quoted 7 lines · Michael B?sch
Quoted 3 lines · On Fri, 28 Nov 2014 22:32:30 -0500 nick <[email protected]> wrote:
I don't have hardware for this driver on me, so I didn't test it. However this seems to be correct from my reading of the code around this function and other locking related to this driver.
Quoted 6 lines
From the current docs:
Quoted 2 lines
* @set_tim: Set TIM bit. mac80211 calls this function when a TIM bit
* must be set or cleared for a given STA. Must be atomic.So it is not allowed to lock a mutex here.
Nicholas please be more careful with your patches. It seems your patch is wrong, you didn't check the docs and you didn't even say in the first place that it wasn't tested. You could at least submit with with RFT tag or sth.
Quoted 1 line · nick
Sorry about that, next time I will be more careful.
One other thing would be to change the initial part of the subject. Yes, the device is in drivers/net/wireless/, but it is much more common to not include those in patches that are sent to Linville. I would have used "b43: Add ...". That catches the eye more quickly. As they say in the newspaper business, "Don't bury the lead."
Larry