Re: [PATCH] drivers:net:wireless: Add mutex locking for function, b43_op_beacon_set_time in main.c

Re: [PATCH] drivers:net:wireless: Add mutex locking for function, b43_op_beacon_set_time in main.c

[Rafał Miłecki]

On 28 November 2014 at 23:16, Nicholas Krause <xerofoify@gmail.com> wrote:
> 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.

For all kind of kernel documentation / descriptions we try to fit 72
chars limit and we really try to avoid exceeding 80 chars.


> @@ -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?

Re: [PATCH] drivers:net:wireless: Add mutex locking for function, b43_op_beacon_set_time in main.c

[Michael Büsch]

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

On Fri, 28 Nov 2014 23:40:46 +0100
Rafał Miłecki <zajec5@gmail.com> wrote:

> > @@ -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?

I think the callback used to be (is?) in atomic context.

> Nicholas: did you test it anyhow?

-- 
Michael

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

Re: [PATCH] drivers:net:wireless: Add mutex locking for function, b43_op_beacon_set_time in main.c

[Rafał Miłecki]

On 29 November 2014 at 04:32, nick <xerofoify@gmail.com> 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.

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!).

Re: [PATCH] drivers:net:wireless: Add mutex locking for function, b43_op_beacon_set_time in main.c

[Michael Büsch]

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

On Fri, 28 Nov 2014 22:32:30 -0500
nick <xerofoify@gmail.com> 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:

>  * @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.

-- 
Michael

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

Re: [PATCH] drivers:net:wireless: Add mutex locking for function, b43_op_beacon_set_time in main.c

[Rafał Miłecki]

On 29 November 2014 at 09:56, Michael Büsch <m@bues.ch> wrote:
> On Fri, 28 Nov 2014 22:32:30 -0500
> nick <xerofoify@gmail.com> 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:
>
>>  * @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.

-- 
Rafał

Re: [PATCH] drivers:net:wireless: Add mutex locking for function, b43_op_beacon_set_time in main.c

[Larry Finger]

On 11/29/2014 12:09 PM, nick wrote:
> 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