[PATCH] drivers:net:wireless: Add proper locking for the function, b43_op_beacon_set_tim in main.c

[PATCH] drivers:net:wireless: Add proper locking for the function, b43_op_beacon_set_tim in main.c

[Nicholas Krause]

This adds proper locking for the function, b43_op_beacon_set_tim in main.c by using the mutex lock
in the structure pointer wl, as embedded into this pointer as a mutex in order to protect against
multiple access to the pointer wl when updating the templates for this pointer in the function,
b43_update_templates internally in the function, b43_op_beacon_set_tim.

Signed-off-by: Nicholas Krause <xerofoify-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/net/wireless/b43/main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
index 47731cb..d568fc8 100644
--- a/drivers/net/wireless/b43/main.c
+++ b/drivers/net/wireless/b43/main.c
@@ -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;
 }
-- 
2.1.0

Re: [PATCH] drivers:net:wireless: Add proper locking for the function, b43_op_beacon_set_tim in main.c

[Michael Büsch]

[-- Attachment #1: Type: text/plain, Size: 1387 bytes --]

On Fri,  2 Jan 2015 02:34:01 -0500
Nicholas Krause <xerofoify@gmail.com> wrote:

> This adds proper locking for the function, b43_op_beacon_set_tim in main.c by using the mutex lock
> in the structure pointer wl, as embedded into this pointer as a mutex in order to protect against
> multiple access to the pointer wl when updating the templates for this pointer in the function,
> b43_update_templates internally in the function, b43_op_beacon_set_tim.
> 
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  drivers/net/wireless/b43/main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
> index 47731cb..d568fc8 100644
> --- a/drivers/net/wireless/b43/main.c
> +++ b/drivers/net/wireless/b43/main.c
> @@ -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;
>  }

Thanks for the patch.

However, this does not work. We are in atomic context here.
Please see the b43-dev mailing list archives for a recent thread about that.
I'm also pretty sure that this is safe without lock, due to the higher level locks in mac80211.

-- 
Michael

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] drivers:net:wireless: Add proper locking for the function, b43_op_beacon_set_tim in main.c

[Rafał Miłecki]

On 2 January 2015 at 10:27, Michael Büsch <m@bues.ch> wrote:
> On Fri,  2 Jan 2015 02:34:01 -0500
> Nicholas Krause <xerofoify@gmail.com> wrote:
>
>> This adds proper locking for the function, b43_op_beacon_set_tim in main.c by using the mutex lock
>> in the structure pointer wl, as embedded into this pointer as a mutex in order to protect against
>> multiple access to the pointer wl when updating the templates for this pointer in the function,
>> b43_update_templates internally in the function, b43_op_beacon_set_tim.
>>
>> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
>> ---
>>  drivers/net/wireless/b43/main.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
>> index 47731cb..d568fc8 100644
>> --- a/drivers/net/wireless/b43/main.c
>> +++ b/drivers/net/wireless/b43/main.c
>> @@ -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;
>>  }
>
> Thanks for the patch.
>
> However, this does not work. We are in atomic context here.
> Please see the b43-dev mailing list archives for a recent thread about that.

Michael: guess who it was who sent the patch doing the same back in November.

Yes, the same troll.

-- 
Rafał

Re: [PATCH] drivers:net:wireless: Add proper locking for the function, b43_op_beacon_set_tim in main.c

[Michael Büsch]

[-- Attachment #1: Type: text/plain, Size: 1703 bytes --]

On Fri, 2 Jan 2015 12:19:12 +0100
Rafał Miłecki <zajec5@gmail.com> wrote:

> On 2 January 2015 at 10:27, Michael Büsch <m@bues.ch> wrote:
> > On Fri,  2 Jan 2015 02:34:01 -0500
> > Nicholas Krause <xerofoify@gmail.com> wrote:
> >
> >> This adds proper locking for the function, b43_op_beacon_set_tim in main.c by using the mutex lock
> >> in the structure pointer wl, as embedded into this pointer as a mutex in order to protect against
> >> multiple access to the pointer wl when updating the templates for this pointer in the function,
> >> b43_update_templates internally in the function, b43_op_beacon_set_tim.
> >>
> >> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> >> ---
> >>  drivers/net/wireless/b43/main.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
> >> index 47731cb..d568fc8 100644
> >> --- a/drivers/net/wireless/b43/main.c
> >> +++ b/drivers/net/wireless/b43/main.c
> >> @@ -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;
> >>  }
> >
> > Thanks for the patch.
> >
> > However, this does not work. We are in atomic context here.
> > Please see the b43-dev mailing list archives for a recent thread about that.
> 
> Michael: guess who it was who sent the patch doing the same back in November.
> 
> Yes, the same troll.

D'oh.
Thanks for the notice.

-- 
Michael

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] drivers:net:wireless: Add proper locking for the function, b43_op_beacon_set_tim in main.c

[nick]

Michael,
My fault I wasn't aware we were in atomic context, however if we are then why
not just remove the comment. If you want I can send in a patch explaining why
this comment is no longer needed.
Regards,
Nick

On 2015-01-02 04:27 AM, Michael Büsch wrote:
> On Fri,  2 Jan 2015 02:34:01 -0500
> Nicholas Krause <xerofoify-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
>> This adds proper locking for the function, b43_op_beacon_set_tim in main.c by using the mutex lock
>> in the structure pointer wl, as embedded into this pointer as a mutex in order to protect against
>> multiple access to the pointer wl when updating the templates for this pointer in the function,
>> b43_update_templates internally in the function, b43_op_beacon_set_tim.
>>
>> Signed-off-by: Nicholas Krause <xerofoify-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/net/wireless/b43/main.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
>> index 47731cb..d568fc8 100644
>> --- a/drivers/net/wireless/b43/main.c
>> +++ b/drivers/net/wireless/b43/main.c
>> @@ -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;
>>  }
> 
> Thanks for the patch.
> 
> However, this does not work. We are in atomic context here.
> Please see the b43-dev mailing list archives for a recent thread about that.
> I'm also pretty sure that this is safe without lock, due to the higher level locks in mac80211.
> 

Re: [PATCH] drivers:net:wireless: Add proper locking for the function, b43_op_beacon_set_tim in main.c

[Larry Finger]

On 01/02/2015 01:34 AM, Nicholas Krause wrote:
> This adds proper locking for the function, b43_op_beacon_set_tim in main.c by using the mutex lock
> in the structure pointer wl, as embedded into this pointer as a mutex in order to protect against
> multiple access to the pointer wl when updating the templates for this pointer in the function,
> b43_update_templates internally in the function, b43_op_beacon_set_tim.
>
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>

Although this patch has already been rejected, I want to comment on the subject 
you have chosen. As you are submitting it to the maintainer of 
drivers/net/wireless/, there is no need to place those entities in the subject. 
A better subject would have been "b43: Add proper locking for 
b43_op_beacon_set_tim()".

Larry

Re: [PATCH] drivers:net:wireless: Add proper locking for the function, b43_op_beacon_set_tim in main.c

[Kalle Valo]

Nicholas Krause <xerofoify@gmail.com> writes:

> This adds proper locking for the function, b43_op_beacon_set_tim in
> main.c by using the mutex lock in the structure pointer wl, as
> embedded into this pointer as a mutex in order to protect against
> multiple access to the pointer wl when updating the templates for this
> pointer in the function, b43_update_templates internally in the
> function, b43_op_beacon_set_tim.
>
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>

Based on the discussion I have dropped this patch.

-- 
Kalle Valo

Re: [PATCH] drivers:net:wireless: Add proper locking for the function, b43_op_beacon_set_tim in main.c

[nick]

Kalle,
That's fine. On the other hand I am interested in the
reasons why this patch was dropped so I can send in a
version 2 of this patch.
Regards Nick 

On 2015-01-05 04:29 AM, Kalle Valo wrote:
> Nicholas Krause <xerofoify-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
> 
>> This adds proper locking for the function, b43_op_beacon_set_tim in
>> main.c by using the mutex lock in the structure pointer wl, as
>> embedded into this pointer as a mutex in order to protect against
>> multiple access to the pointer wl when updating the templates for this
>> pointer in the function, b43_update_templates internally in the
>> function, b43_op_beacon_set_tim.
>>
>> Signed-off-by: Nicholas Krause <xerofoify-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> Based on the discussion I have dropped this patch.
> 

Re: [PATCH] drivers:net:wireless: Add proper locking for the function, b43_op_beacon_set_tim in main.c

[Larry Finger]

On 01/05/2015 11:16 AM, nick wrote:
> Kalle,
> That's fine. On the other hand I am interested in the
> reasons why this patch was dropped so I can send in a
> version 2 of this patch.
> Regards Nick

Apparently you missed Michael Büsch's comment:

"Thanks for the patch.

However, this does not work. We are in atomic context here.
Please see the b43-dev mailing list archives for a recent thread about that.
I'm also pretty sure that this is safe without lock, due to the higher level 
locks in mac80211."

Did you actually test the patch? If Michael is right, and I trust him on this 
matter, your patch should lock the system.

Larry

Re: [PATCH] drivers:net:wireless: Add proper locking for the function, b43_op_beacon_set_tim in main.c

[nick]

Larry,
Your right I build tested it. Unfortunately I don't have
the hardware.
Regards Nick
On 2015-01-05 12:46 PM, Larry Finger wrote:
> On 01/05/2015 11:16 AM, nick wrote:
>> Kalle,
>> That's fine. On the other hand I am interested in the
>> reasons why this patch was dropped so I can send in a
>> version 2 of this patch.
>> Regards Nick
> 
> Apparently you missed Michael Büsch's comment:
> 
> "Thanks for the patch.
> 
> However, this does not work. We are in atomic context here.
> Please see the b43-dev mailing list archives for a recent thread about that.
> I'm also pretty sure that this is safe without lock, due to the higher level locks in mac80211."
> 
> Did you actually test the patch? If Michael is right, and I trust him on this matter, your patch should lock the system.
> 
> Larry
>