Re: [PATCH v4 1/3] mwifiex: reset card->adapter during device unregister

[Brian Norris]

+ Dmitry

Hi Amit,

On Mon, Oct 10, 2016 at 01:53:32PM -0700, Brian Norris wrote:
> On Thu, Oct 06, 2016 at 11:36:24PM +0530, Amitkumar Karwar wrote:
> > From: Xinming Hu 
> > 
> > card->adapter gets initialized during device registration.
> > As it's not cleared, we may end up accessing invalid memory
> > in some corner cases. This patch fixes the problem.
> > 
> > Signed-off-by: Xinming Hu 
> > Signed-off-by: Amitkumar Karwar 
> > ---
> > v4: Same as v1, v2, v3
> > ---
> >  drivers/net/wireless/marvell/mwifiex/pcie.c | 1 +
> >  drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
> >  2 files changed, 2 insertions(+)
> > 
> > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
> > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > index f1eeb73..ba9e068 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct 
> > mwifiex_adapter *adapter)
> > pci_disable_msi(pdev);
> >}
> > }
> > +   card->adapter = NULL;
> >  }
> >  
> >  /* This function initializes the PCI-E host memory space, WCB rings, etc.
> > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
> > b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > index 8718950..4cad1c2 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > @@ -2066,6 +2066,7 @@ mwifiex_unregister_dev(struct mwifiex_adapter 
> > *adapter)
> > struct sdio_mmc_card *card = adapter->card;
> >  
> > if (adapter->card) {
> > +   card->adapter = NULL;
> > sdio_claim_host(card->func);
> > sdio_disable_func(card->func);
> > sdio_release_host(card->func);
> 
> As discussed on v1, I had qualms about the raciness between reads/writes
> of card->adapter, but I believe we:
> (a) can't have any command activity while writing the ->adapter field
> (either we're just init'ing the device, or we've disabled interrupts and
> are tearing it down) and
> (b) can't have a race between suspend()/resume() and unregister_dev(),
> since unregister_dev() is called from device remove() (which should not
> be concurrent with suspend()).
> 
> Also, I thought you had the same problem in usb.c, but in fact, you
> fixed that ages ago here:
> 
> 353d2a69ea26 mwifiex: fix issues in driver unload path for USB chipsets
> 
> Would be nice if fixes were bettery synchronized across the three
> interface drivers you support. We seem to be discovering unnecessary
> divergence on a few points recently.
> 
> At any rate:
> 
> Reviewed-by: Brian Norris 
> Tested-by: Brian Norris 

Dmitry helped me re-realize my original qualms:

mwifiex_unregister_dev() is called in the failure path for your async FW
request, and so it may race with suspend(). So I retract my Reviewed-by.
Sorry.

I'm going to look into converting to asynchronous device probing, which
might remove the need for async FW request, and would therefore resolve
both patch 1 and 3's races without any additional complicated hacks. But
I'm not sure if that will satisfy all mwifiex users well enough. I'll
have to give it a little more thought. Any thoughts from your side,
Amit?

Brian

Re: [PATCH v2 1/2] mwifiex: reset card->adapter during device unregister

[Brian Norris]

(I think Dmitry noticed the same while I wrote this.)

On Mon, Oct 10, 2016 at 04:47:08PM -0700, Brian Norris wrote:
> [*] The other cases are in error handling cases. I guess I should make
> sure those didn't race too...

Ah, well I think I missed one case:

For the async FW request code path, the callback mwifiex_fw_dpc() can
fail to load FW, and so it unwinds with:

if (adapter->if_ops.unregister_dev)
adapter->if_ops.unregister_dev(adapter);

This all happens after probe() is finished, so we can have a race on
card->adapter being read in suspend() and written in the $subject patch.

Can I revoke my Reviewed-by? (Or make it Reviewed-and-found-wanting-by?)

Brian

Re: [PATCH v2 1/2] mwifiex: reset card->adapter during device unregister

[Dmitry Torokhov]

Hi Brian,

On Mon, Oct 10, 2016 at 4:47 PM, Brian Norris  wrote:
> Hi Dmitry,
>
> On Mon, Oct 10, 2016 at 04:43:06PM -0700, Dmitry Torokhov wrote:
>> On Thu, Oct 6, 2016 at 6:03 AM, Amitkumar Karwar  wrote:
>> >> From: linux-wireless-ow...@vger.kernel.org [mailto:linux-wireless-
>> >> ow...@vger.kernel.org] On Behalf Of Brian Norris
>> >> Sent: Wednesday, October 05, 2016 10:00 PM
>> >> To: Amitkumar Karwar
>> >> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
>> >> raja...@google.com; Xinming Hu
>> >> Subject: Re: [PATCH v2 1/2] mwifiex: reset card->adapter during device
>> >> unregister
>> >>
>> >> On Wed, Oct 05, 2016 at 02:04:53PM +, Amitkumar Karwar wrote:
>> >> > > From: Brian Norris [mailto:briannor...@chromium.org]
>> >> > > Sent: Wednesday, October 05, 2016 3:28 AM
>> >> > > To: Amitkumar Karwar
>> >> > > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
>> >> > > raja...@google.com; briannor...@google.com; Xinming Hu
>> >> > > Subject: Re: [PATCH v2 1/2] mwifiex: reset card->adapter during
>> >> > > device unregister
>> >> > >
>> >> > > On Tue, Oct 04, 2016 at 10:38:24PM +0530, Amitkumar Karwar wrote:
>> >>
>> >> > > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
>> >> > > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
>> >> > > > @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct
>> >> > > mwifiex_adapter *adapter)
>> >> > > > pci_disable_msi(pdev);
>> >> > > >}
>> >> > > > }
>> >> > > > +   card->adapter = NULL;
>> >> > >
>> >> > > I think you have a similar problem here as in patch 2; there is no
>> >> > > locking to protect fields in struct pcie_service_card or struct
>> >> > > sdio_mmc_card below. That problem kind of already exists, except
>> >> > > that you only write the value of card->adapter once at registration
>> >> > > time, so it's not actually unsafe. But now that you're introducing a
>> >> > > second write, you have a problem.
>> >> > >
>> >> > > Brian
>> >> > >
>> >> >
>> >> > We have a global "add_remove_card_sem" semaphore in our code for
>> >> > synchronizing initialization and teardown threads. Ideally "init +
>> >> > teardown/reboot" should not have a race issue with this logic
>> >> >
>> >> > Later there was a loophole introduced in this after using async
>> >> > firmware download API. During initialization, firmware downloads
>> >> > asynchronously in a separate thread where might have released the
>> >> > semaphore. I am working on a patch to fix this.
>> >> >
>> >> > So "card->adapter" doesn't need to have locking. Even if we have two
>> >> > write operations, those two threads can't run simultaneously due to
>> >> > above mentioned logic.
>> >>
>> >> What about writes racing with reads? You have lots of unsynchronized
>> >> cases that read this, although most of them should be halted by now
>> >> (e.g., cmd processing). I was looking at suspend() in particular, which
>> >> I thought you were looking at in this patch series.
>> >
>> > Please note that "card->adapter" is used only in pcie.c/sdio.c/usb.c files
>> >
>> > Writes won't have race with reads.
>> >
>> > 1) write 1  --- "card->adapter = adapter;" in mwifiex_register_dev()
>> > This place is at the beginning of initialization.
>> > mwifiex_pcie_probe() -> mwifiex_add_card() -> 
>> > adapter->if_ops.register_dev()
>> > There is no chance that "card->adapter" is read anywhere at this 
>> > point. FW is not yet downloaded
>> >
>> > 2) write 2  "card->adapter = NULL;" in mwifiex_unregister_dev()
>> > This place the end of teardown phase.
>> > Interrupts are disabled and all cleanup is done. We have 
>> > "card->adapter" NULL checks at entry point of suspend/remove/resume, if 
>> > they get called after this.
>>
>> How exactly are you preventing ->suspend() from being called *while*
>> you are running mwifiex_unregister_dev()? I.e. user slamming the lid
>> of a laptop and throwing it in their backpack?
>
> As I already commented, isn't this primarily [*] called from the driver
> remove() callback? remove() doesn't race with suspend(), does it?

Nope, there is request_firmware_nowait() path that is asynchronous
with device registration/unregistration and power management. Maybe
the right way is to move the driver to async probing and switch that
request_firmware_nowait() into plain request_firmware(). I haven't
looked that closely.

The wording "we may end up accessing invalid memory in some corner
cases" which is a synonym for "the code is racy" caught my eye, thus
the response on the patch.

Thanks.

-- 
Dmitry

Re: [PATCH v2 1/2] mwifiex: reset card->adapter during device unregister

[Brian Norris]

Hi Dmitry,

On Mon, Oct 10, 2016 at 04:43:06PM -0700, Dmitry Torokhov wrote:
> On Thu, Oct 6, 2016 at 6:03 AM, Amitkumar Karwar  wrote:
> >> From: linux-wireless-ow...@vger.kernel.org [mailto:linux-wireless-
> >> ow...@vger.kernel.org] On Behalf Of Brian Norris
> >> Sent: Wednesday, October 05, 2016 10:00 PM
> >> To: Amitkumar Karwar
> >> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> >> raja...@google.com; Xinming Hu
> >> Subject: Re: [PATCH v2 1/2] mwifiex: reset card->adapter during device
> >> unregister
> >>
> >> On Wed, Oct 05, 2016 at 02:04:53PM +, Amitkumar Karwar wrote:
> >> > > From: Brian Norris [mailto:briannor...@chromium.org]
> >> > > Sent: Wednesday, October 05, 2016 3:28 AM
> >> > > To: Amitkumar Karwar
> >> > > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> >> > > raja...@google.com; briannor...@google.com; Xinming Hu
> >> > > Subject: Re: [PATCH v2 1/2] mwifiex: reset card->adapter during
> >> > > device unregister
> >> > >
> >> > > On Tue, Oct 04, 2016 at 10:38:24PM +0530, Amitkumar Karwar wrote:
> >>
> >> > > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> >> > > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> >> > > > @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct
> >> > > mwifiex_adapter *adapter)
> >> > > > pci_disable_msi(pdev);
> >> > > >}
> >> > > > }
> >> > > > +   card->adapter = NULL;
> >> > >
> >> > > I think you have a similar problem here as in patch 2; there is no
> >> > > locking to protect fields in struct pcie_service_card or struct
> >> > > sdio_mmc_card below. That problem kind of already exists, except
> >> > > that you only write the value of card->adapter once at registration
> >> > > time, so it's not actually unsafe. But now that you're introducing a
> >> > > second write, you have a problem.
> >> > >
> >> > > Brian
> >> > >
> >> >
> >> > We have a global "add_remove_card_sem" semaphore in our code for
> >> > synchronizing initialization and teardown threads. Ideally "init +
> >> > teardown/reboot" should not have a race issue with this logic
> >> >
> >> > Later there was a loophole introduced in this after using async
> >> > firmware download API. During initialization, firmware downloads
> >> > asynchronously in a separate thread where might have released the
> >> > semaphore. I am working on a patch to fix this.
> >> >
> >> > So "card->adapter" doesn't need to have locking. Even if we have two
> >> > write operations, those two threads can't run simultaneously due to
> >> > above mentioned logic.
> >>
> >> What about writes racing with reads? You have lots of unsynchronized
> >> cases that read this, although most of them should be halted by now
> >> (e.g., cmd processing). I was looking at suspend() in particular, which
> >> I thought you were looking at in this patch series.
> >
> > Please note that "card->adapter" is used only in pcie.c/sdio.c/usb.c files
> >
> > Writes won't have race with reads.
> >
> > 1) write 1  --- "card->adapter = adapter;" in mwifiex_register_dev()
> > This place is at the beginning of initialization.
> > mwifiex_pcie_probe() -> mwifiex_add_card() -> 
> > adapter->if_ops.register_dev()
> > There is no chance that "card->adapter" is read anywhere at this 
> > point. FW is not yet downloaded
> >
> > 2) write 2  "card->adapter = NULL;" in mwifiex_unregister_dev()
> > This place the end of teardown phase.
> > Interrupts are disabled and all cleanup is done. We have 
> > "card->adapter" NULL checks at entry point of suspend/remove/resume, if 
> > they get called after this.
> 
> How exactly are you preventing ->suspend() from being called *while*
> you are running mwifiex_unregister_dev()? I.e. user slamming the lid
> of a laptop and throwing it in their backpack?

As I already commented, isn't this primarily [*] called from the driver
remove() callback? remove() doesn't race with suspend(), does it?

[*] The other cases are in error handling cases. I guess I should make
sure those didn't race too...

Brian

Re: [PATCH v2 1/2] mwifiex: reset card->adapter during device unregister

[Dmitry Torokhov]

On Thu, Oct 6, 2016 at 6:03 AM, Amitkumar Karwar  wrote:
> Hi Brian,
>
>> From: linux-wireless-ow...@vger.kernel.org [mailto:linux-wireless-
>> ow...@vger.kernel.org] On Behalf Of Brian Norris
>> Sent: Wednesday, October 05, 2016 10:00 PM
>> To: Amitkumar Karwar
>> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
>> raja...@google.com; Xinming Hu
>> Subject: Re: [PATCH v2 1/2] mwifiex: reset card->adapter during device
>> unregister
>>
>> Hi,
>>
>> On Wed, Oct 05, 2016 at 02:04:53PM +, Amitkumar Karwar wrote:
>> > > From: Brian Norris [mailto:briannor...@chromium.org]
>> > > Sent: Wednesday, October 05, 2016 3:28 AM
>> > > To: Amitkumar Karwar
>> > > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
>> > > raja...@google.com; briannor...@google.com; Xinming Hu
>> > > Subject: Re: [PATCH v2 1/2] mwifiex: reset card->adapter during
>> > > device unregister
>> > >
>> > > Hi,
>> > >
>> > > On Tue, Oct 04, 2016 at 10:38:24PM +0530, Amitkumar Karwar wrote:
>>
>> > > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
>> > > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
>> > > > @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct
>> > > mwifiex_adapter *adapter)
>> > > > pci_disable_msi(pdev);
>> > > >}
>> > > > }
>> > > > +   card->adapter = NULL;
>> > >
>> > > I think you have a similar problem here as in patch 2; there is no
>> > > locking to protect fields in struct pcie_service_card or struct
>> > > sdio_mmc_card below. That problem kind of already exists, except
>> > > that you only write the value of card->adapter once at registration
>> > > time, so it's not actually unsafe. But now that you're introducing a
>> > > second write, you have a problem.
>> > >
>> > > Brian
>> > >
>> >
>> > We have a global "add_remove_card_sem" semaphore in our code for
>> > synchronizing initialization and teardown threads. Ideally "init +
>> > teardown/reboot" should not have a race issue with this logic
>> >
>> > Later there was a loophole introduced in this after using async
>> > firmware download API. During initialization, firmware downloads
>> > asynchronously in a separate thread where might have released the
>> > semaphore. I am working on a patch to fix this.
>> >
>> > So "card->adapter" doesn't need to have locking. Even if we have two
>> > write operations, those two threads can't run simultaneously due to
>> > above mentioned logic.
>>
>> What about writes racing with reads? You have lots of unsynchronized
>> cases that read this, although most of them should be halted by now
>> (e.g., cmd processing). I was looking at suspend() in particular, which
>> I thought you were looking at in this patch series.
>
> Please note that "card->adapter" is used only in pcie.c/sdio.c/usb.c files
>
> Writes won't have race with reads.
>
> 1) write 1  --- "card->adapter = adapter;" in mwifiex_register_dev()
> This place is at the beginning of initialization.
> mwifiex_pcie_probe() -> mwifiex_add_card() -> 
> adapter->if_ops.register_dev()
> There is no chance that "card->adapter" is read anywhere at this 
> point. FW is not yet downloaded
>
> 2) write 2  "card->adapter = NULL;" in mwifiex_unregister_dev()
> This place the end of teardown phase.
> Interrupts are disabled and all cleanup is done. We have 
> "card->adapter" NULL checks at entry point of suspend/remove/resume, if they 
> get called after this.

How exactly are you preventing ->suspend() from being called *while*
you are running mwifiex_unregister_dev()? I.e. user slamming the lid
of a laptop and throwing it in their backpack?

Thanks.

-- 
Dmitry

[PATCH] rtlwifi: Fix regression caused by commit d86e64768859

[Larry Finger]

In commit d86e64768859 ("rtlwifi: rtl818x: constify local structures"),
the configuration struct for most of the drivers was changed to be
constant. The problem is that five of the modified drivers need to be
able to update the firmware name based on the exact model of the card.
As the file names were stored in one of the members of that struct,
these drivers would fail with a kernel BUG splat when they tried to
update the firmware name.

Rather than reverting the previous commit, I used a suggestion by
Johannes Berg and made the firmware file name pointers be local to
the routines that update the software variables.

The configuration struct of rtl8192cu, which was not touched in the
previous patch, is now constantfied.

Fixes: d86e64768859 ("rtlwifi: rtl818x: constify local structures")
Suggested-by: Johannes Berg 
Signed-off-by: Larry Finger 
Cc: Stable [4.8+] 
Cc: Julia Lawall 
---
Kalle,

My apologies for letting these bugs to get by my review and testing. As
they affect kernel 4.8, please push this patch as soon as possible. To
reiterate, this patch replaces the one entitled 'Revert "rtlwifi: rtl818x:
constify local structures"'

Thanks,

Larry
 drivers/net/wireless/realtek/rtlwifi/core.c |  2 +-
 drivers/net/wireless/realtek/rtlwifi/rtl8188ee/sw.c |  8 
 drivers/net/wireless/realtek/rtlwifi/rtl8192ce/sw.c | 10 +-
 drivers/net/wireless/realtek/rtlwifi/rtl8192cu/sw.c | 12 ++--
 drivers/net/wireless/realtek/rtlwifi/rtl8192de/sw.c |  6 +++---
 drivers/net/wireless/realtek/rtlwifi/rtl8192ee/sw.c |  8 
 drivers/net/wireless/realtek/rtlwifi/rtl8192se/sw.c |  9 +
 drivers/net/wireless/realtek/rtlwifi/rtl8723ae/sw.c | 10 +-
 drivers/net/wireless/realtek/rtlwifi/rtl8723be/sw.c |  6 +++---
 drivers/net/wireless/realtek/rtlwifi/rtl8821ae/sw.c | 18 +-
 drivers/net/wireless/realtek/rtlwifi/wifi.h |  2 --
 11 files changed, 45 insertions(+), 46 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c 
b/drivers/net/wireless/realtek/rtlwifi/core.c
index f95760c..8e7f23c 100644
--- a/drivers/net/wireless/realtek/rtlwifi/core.c
+++ b/drivers/net/wireless/realtek/rtlwifi/core.c
@@ -111,7 +111,7 @@ static void rtl_fw_do_work(const struct firmware *firmware, 
void *context,
if (!err)
goto found_alt;
}
-   pr_err("Firmware %s not available\n", rtlpriv->cfg->fw_name);
+   pr_err("Selected firmware is not available\n");
rtlpriv->max_fw_size = 0;
return;
}
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/sw.c 
b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/sw.c
index e7b11b4..f361808 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/sw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/sw.c
@@ -86,6 +86,7 @@ int rtl88e_init_sw_vars(struct ieee80211_hw *hw)
struct rtl_priv *rtlpriv = rtl_priv(hw);
struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
u8 tid;
+   char *fw_name;
 
rtl8188ee_bt_reg_init(hw);
rtlpriv->dm.dm_initialgain_enable = 1;
@@ -169,10 +170,10 @@ int rtl88e_init_sw_vars(struct ieee80211_hw *hw)
return 1;
}
 
-   rtlpriv->cfg->fw_name = "rtlwifi/rtl8188efw.bin";
+   fw_name = "rtlwifi/rtl8188efw.bin";
rtlpriv->max_fw_size = 0x8000;
-   pr_info("Using firmware %s\n", rtlpriv->cfg->fw_name);
-   err = request_firmware_nowait(THIS_MODULE, 1, rtlpriv->cfg->fw_name,
+   pr_info("Using firmware %s\n", fw_name);
+   err = request_firmware_nowait(THIS_MODULE, 1, fw_name,
  rtlpriv->io.dev, GFP_KERNEL, hw,
  rtl_fw_cb);
if (err) {
@@ -284,7 +285,6 @@ static const struct rtl_hal_cfg rtl88ee_hal_cfg = {
.bar_id = 2,
.write_readback = true,
.name = "rtl88e_pci",
-   .fw_name = "rtlwifi/rtl8188efw.bin",
.ops = _hal_ops,
.mod_params = _mod_params,
 
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/sw.c 
b/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/sw.c
index 5c46a98..25cbd68 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/sw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/sw.c
@@ -92,6 +92,7 @@ int rtl92c_init_sw_vars(struct ieee80211_hw *hw)
struct rtl_priv *rtlpriv = rtl_priv(hw);
struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
struct rtl_hal *rtlhal = rtl_hal(rtl_priv(hw));
+   char *fw_name;
 
rtl8192ce_bt_reg_init(hw);
 
@@ -165,13 +166,13 @@ int rtl92c_init_sw_vars(struct ieee80211_hw *hw)
/* request fw */
if (IS_VENDOR_UMC_A_CUT(rtlhal->version) &&
!IS_92C_SERIAL(rtlhal->version))
-   rtlpriv->cfg->fw_name = 

Re: [PATCH v4 2/3] mwifiex: remove redundant pdev check in suspend/resume handlers

[Brian Norris]

On Thu, Oct 06, 2016 at 11:36:25PM +0530, Amitkumar Karwar wrote:
> to_pci_dev() would just do struct offset arithmetic on struct
> device to get 'pdev' pointer. We never get NULL pdev pointer
> 
> Signed-off-by: Amitkumar Karwar 
> ---
> New patch introduced in v3 as per inputs from Brian Norris.
> v4: Same as v3
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 22 ++
>  1 file changed, 6 insertions(+), 16 deletions(-)

Reviewed-by: Brian Norris 

Re: [PATCH v4 1/3] mwifiex: reset card->adapter during device unregister

[Brian Norris]

Hi Amit,

On Thu, Oct 06, 2016 at 11:36:24PM +0530, Amitkumar Karwar wrote:
> From: Xinming Hu 
> 
> card->adapter gets initialized during device registration.
> As it's not cleared, we may end up accessing invalid memory
> in some corner cases. This patch fixes the problem.
> 
> Signed-off-by: Xinming Hu 
> Signed-off-by: Amitkumar Karwar 
> ---
> v4: Same as v1, v2, v3
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 1 +
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
> b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index f1eeb73..ba9e068 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct 
> mwifiex_adapter *adapter)
>   pci_disable_msi(pdev);
>  }
>   }
> + card->adapter = NULL;
>  }
>  
>  /* This function initializes the PCI-E host memory space, WCB rings, etc.
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
> b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index 8718950..4cad1c2 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -2066,6 +2066,7 @@ mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
>   struct sdio_mmc_card *card = adapter->card;
>  
>   if (adapter->card) {
> + card->adapter = NULL;
>   sdio_claim_host(card->func);
>   sdio_disable_func(card->func);
>   sdio_release_host(card->func);

As discussed on v1, I had qualms about the raciness between reads/writes
of card->adapter, but I believe we:
(a) can't have any command activity while writing the ->adapter field
(either we're just init'ing the device, or we've disabled interrupts and
are tearing it down) and
(b) can't have a race between suspend()/resume() and unregister_dev(),
since unregister_dev() is called from device remove() (which should not
be concurrent with suspend()).

Also, I thought you had the same problem in usb.c, but in fact, you
fixed that ages ago here:

353d2a69ea26 mwifiex: fix issues in driver unload path for USB chipsets

Would be nice if fixes were bettery synchronized across the three
interface drivers you support. We seem to be discovering unnecessary
divergence on a few points recently.

At any rate:

Reviewed-by: Brian Norris 
Tested-by: Brian Norris 

Thanks.

Re: [PATCH v2 1/2] mwifiex: reset card->adapter during device unregister

[Brian Norris]

Hi Amit,

On Thu, Oct 06, 2016 at 01:03:02PM +, Amitkumar Karwar wrote:
> > From: linux-wireless-ow...@vger.kernel.org [mailto:linux-wireless-
> > ow...@vger.kernel.org] On Behalf Of Brian Norris
> > Sent: Wednesday, October 05, 2016 10:00 PM
> > To: Amitkumar Karwar
> > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > raja...@google.com; Xinming Hu
> > Subject: Re: [PATCH v2 1/2] mwifiex: reset card->adapter during device
> > unregister
> > 
> > On Wed, Oct 05, 2016 at 02:04:53PM +, Amitkumar Karwar wrote:
> > > > From: Brian Norris [mailto:briannor...@chromium.org]
> > > > Sent: Wednesday, October 05, 2016 3:28 AM
> > > > To: Amitkumar Karwar
> > > > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > > > raja...@google.com; briannor...@google.com; Xinming Hu
> > > > Subject: Re: [PATCH v2 1/2] mwifiex: reset card->adapter during
> > > > device unregister
> > > >
> > > > On Tue, Oct 04, 2016 at 10:38:24PM +0530, Amitkumar Karwar wrote:
> > 
> > > > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct
> > > > mwifiex_adapter *adapter)
> > > > >   pci_disable_msi(pdev);
> > > > >  }
> > > > >   }
> > > > > + card->adapter = NULL;

...

> > What about writes racing with reads? You have lots of unsynchronized
> > cases that read this, although most of them should be halted by now
> > (e.g., cmd processing). I was looking at suspend() in particular, which
> > I thought you were looking at in this patch series.
> 
> Please note that "card->adapter" is used only in pcie.c/sdio.c/usb.c files
> 
> Writes won't have race with reads.
> 
> 1) write 1  --- "card->adapter = adapter;" in mwifiex_register_dev()
>   This place is at the beginning of initialization. 
>   mwifiex_pcie_probe() -> mwifiex_add_card() -> 
> adapter->if_ops.register_dev()
>   There is no chance that "card->adapter" is read anywhere at this point. 
> FW is not yet downloaded

Sure.

> 2) write 2  "card->adapter = NULL;" in mwifiex_unregister_dev()
>   This place the end of teardown phase.
>   Interrupts are disabled and all cleanup is done. We have
>   "card->adapter" NULL checks at entry point of
>   suspend/remove/resume, if they get called after this.

I guess the question boils down to: can driver suspend() race with
mwifiex_unregister_dev() here, then?

And I guess the answer is "no", because unregistration only happens via

  PCIe driver remove() -> mwifiex_remove_card() -> 
adapter->if_ops.unregister_dev()

and I think the device driver core guarantees that suspend() and
remove() won't race.

In that case:

Reviewed-by: Brian Norris 

I'll reply to the latest revision too, since it's identical.

Thanks for the explanations.

Regards,
Brian

Re: [PATCH] Revert "rtlwifi: rtl818x: constify local structures"

[Julia Lawall]

On Mon, 10 Oct 2016, Larry Finger wrote:

> On 10/10/2016 11:56 AM, Johannes Berg wrote:
> > On Mon, 2016-10-10 at 10:25 -0500, Larry Finger wrote:
> > > This reverts commit d86e64768859fca82c78e52877ceeba04e25d27a.
> > >
> > > For drivers rtl8188ee, rtl8192ce, rtl8192ee, rtl8723ae, and
> > > rtl8821ae,
> > > the Coccinelle script missed the fact that the code changes the
> > > firmware
> > > name. When that happens, the kernel issues a BUG splat because
> > > it is unable to overwrite the old name.
> >
> > Hmm. That seems somewhat problematic, for example if you have multiple
> > devices that use the same driver but need different firmware?
> >
> > Not that I really know what's going on, but changing static variables
> > based on runtime seems like it could cause issues in such cases.
>
> I think the situation is OK, but I have created a patch that converts all the
> firmware names into local strings in the routines that initiate firmware
> loading. That way the affected structs can be constified without problem.

Great, thanks :)

julia

>
> @Kalle: Please drop the patch with this subject.
>
> Thanks,
>
> Larry
>

Re: [PATCH] Revert "rtlwifi: rtl818x: constify local structures"

[Larry Finger]

On 10/10/2016 11:56 AM, Johannes Berg wrote:

On Mon, 2016-10-10 at 10:25 -0500, Larry Finger wrote:

This reverts commit d86e64768859fca82c78e52877ceeba04e25d27a.

For drivers rtl8188ee, rtl8192ce, rtl8192ee, rtl8723ae, and
rtl8821ae,
the Coccinelle script missed the fact that the code changes the
firmware
name. When that happens, the kernel issues a BUG splat because
it is unable to overwrite the old name.


Hmm. That seems somewhat problematic, for example if you have multiple
devices that use the same driver but need different firmware?

Not that I really know what's going on, but changing static variables
based on runtime seems like it could cause issues in such cases.


I think the situation is OK, but I have created a patch that converts all the 
firmware names into local strings in the routines that initiate firmware 
loading. That way the affected structs can be constified without problem.


@Kalle: Please drop the patch with this subject.

Thanks,

Larry

[PATCH v6 4/4] mac80211: multicast to unicast conversion

[Michael Braun]

This patch adds support for sending multicast data packets with ARP, IPv4
and IPv6 payload (possible 802.1q tagged) as 802.11 unicast frames to all
stations.

IEEE 802.11 multicast has well known issues, among them:
 1. packets are not acked and hence not retransmitted, resulting in
decreased reliablity
 2. packets are send at low rate, increasing time required on air

When used with AP_VLAN, there is another disadvantage:
 3. all stations in the BSS are woken up, regardsless of their AP_VLAN
assignment.

By doing multicast to unicast conversion, all three issus are solved.

IEEE802.11-2012 proposes directed multicast service (DMS) using A-MSDU
frames and a station initiated control protocol. It has the advantage that
the station can recover the destination multicast mac address, but it is
not backward compatible with non QOS stations and does not enable the
administrator of a BSS to force this mode of operation within a BSS.
Additionally, it would require both the ap and the station to implement
the control protocol, which is optional on both ends. Furthermore, I've
seen a few mobile phone stations locally that indicate qos support but
won't complete DHCP if their broadcasts are encapsulated as A-MSDU. Though
they work fine with this series approach.

This patch therefore does not opt to implement DMS but instead just
replicates the packet and changes the destination address. As this works
fine with ARP, IPv4 and IPv6, it is limited to these protocols and normal
802.11 multicast frames are send out for all other payload protocols.

There is a runtime toggle to enable multicast conversion in a per-bss
fashion.

When there is only a single station assigned to the AP_VLAN interface, no
packet replication will occur. 4addr mode of operation is unchanged.

This change opts for iterating all BSS stations for finding the stations
assigned to this AP/AP_VLAN interface, as there currently is no per
AP_VLAN list to iterate and multicast packets are expected to be few.
If needed, such a list could be added later.

Signed-off-by: Michael Braun 

--
v5:
  - rename bss->unicast to bss->multicast_to_unicast
  - access sdata->bss only after checking iftype
v4:
  - rename MULTICAST_TO_UNICAST to MULTICAST_TO_UNICAST
v3: fix compile error for trace.h
v2: add nl80211 toggle
rename tx_dnat to change_da
change int to bool unicast
---
 net/mac80211/cfg.c|  15 +++
 net/mac80211/debugfs_netdev.c |   3 ++
 net/mac80211/ieee80211_i.h|   1 +
 net/mac80211/tx.c | 101 ++
 4 files changed, 120 insertions(+)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 1edb017..1db4c3e 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2242,6 +2242,20 @@ static int ieee80211_set_wds_peer(struct wiphy *wiphy, 
struct net_device *dev,
return 0;
 }
 
+static int ieee80211_set_multicast_to_unicast(struct wiphy *wiphy,
+ struct net_device *dev,
+ const bool enabled)
+{
+   struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+
+   if (sdata->vif.type != NL80211_IFTYPE_AP)
+   return -1;
+
+   sdata->u.ap.multicast_to_unicast = enabled;
+
+   return 0;
+}
+
 static void ieee80211_rfkill_poll(struct wiphy *wiphy)
 {
struct ieee80211_local *local = wiphy_priv(wiphy);
@@ -3400,6 +3414,7 @@ const struct cfg80211_ops mac80211_config_ops = {
.set_tx_power = ieee80211_set_tx_power,
.get_tx_power = ieee80211_get_tx_power,
.set_wds_peer = ieee80211_set_wds_peer,
+   .set_multicast_to_unicast = ieee80211_set_multicast_to_unicast,
.rfkill_poll = ieee80211_rfkill_poll,
CFG80211_TESTMODE_CMD(ieee80211_testmode_cmd)
CFG80211_TESTMODE_DUMP(ieee80211_testmode_dump)
diff --git a/net/mac80211/debugfs_netdev.c b/net/mac80211/debugfs_netdev.c
index ed7bff4..509c6c3 100644
--- a/net/mac80211/debugfs_netdev.c
+++ b/net/mac80211/debugfs_netdev.c
@@ -487,6 +487,8 @@ static ssize_t ieee80211_if_fmt_num_buffered_multicast(
 }
 IEEE80211_IF_FILE_R(num_buffered_multicast);
 
+IEEE80211_IF_FILE(multicast_to_unicast, u.ap.multicast_to_unicast, HEX);
+
 /* IBSS attributes */
 static ssize_t ieee80211_if_fmt_tsf(
const struct ieee80211_sub_if_data *sdata, char *buf, int buflen)
@@ -642,6 +644,7 @@ static void add_ap_files(struct ieee80211_sub_if_data 
*sdata)
DEBUGFS_ADD(dtim_count);
DEBUGFS_ADD(num_buffered_multicast);
DEBUGFS_ADD_MODE(tkip_mic_test, 0200);
+   DEBUGFS_ADD_MODE(multicast_to_unicast, 0600);
 }
 
 static void add_vlan_files(struct ieee80211_sub_if_data *sdata)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 70c0963..84374ed 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -293,6 +293,7 @@ struct ieee80211_if_ap {
 

[PATCH v6 2/4] mac80211: filter multicast data packets on AP / AP_VLAN

[Michael Braun]

This patch adds filtering for multicast data packets on AP_VLAN interfaces
that have no authorized station connected and changes filtering on AP
interfaces to not count stations assigned to AP_VLAN interfaces.

This saves airtime and avoids waking up other stations currently authorized
in this BSS. When using WPA, the packets dropped could not be decrypted by
any station.

The behaviour when there are no AP_VLAN interfaces is left unchanged.
When there are AP_VLAN interfaces, this patch
1. adds filtering multicast data packets sent on AP_VLAN interfaces that
   have no authorized station connected.
   No filtering happens on 4addr AP_VLAN interfaces.
2. makes filtering of multicast data packets sent on AP interfaces depend
   on the number of authorized stations in this bss not assigned to an
   AP_VLAN interface.

Therefore, a new num_mcast_sta counter is added for AP_VLAN interfaces.
The existing one for AP interfaces is altered to not track stations
assigned to an AP_VLAN interface.

The new counter is exposed in debugfs.

Signed-off-by: Michael Braun 

--
v4:
 - update description
v3:
 - reuse existing num_mcast_sta
v2:
 - use separate function to inc/dec mcast_sta counters
 - do not filter in 4addr mode
 - change description
 - change filtering on AP interface (do not count AP_VLAN sta)
 - use new counters regardless of 4addr or not
 - simplify cfg.c:change_station
 - remove no-op change in __cleanup_single_sta
---
 net/mac80211/cfg.c| 20 ++--
 net/mac80211/debugfs_netdev.c | 11 +++
 net/mac80211/ieee80211_i.h| 33 +
 net/mac80211/rx.c |  5 +++--
 net/mac80211/sta_info.c   | 10 ++
 net/mac80211/tx.c |  5 ++---
 6 files changed, 57 insertions(+), 27 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 24133f5..1edb017 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1357,9 +1357,6 @@ static int ieee80211_change_station(struct wiphy *wiphy,
goto out_err;
 
if (params->vlan && params->vlan != sta->sdata->dev) {
-   bool prev_4addr = false;
-   bool new_4addr = false;
-
vlansdata = IEEE80211_DEV_TO_SUB_IF(params->vlan);
 
if (params->vlan->ieee80211_ptr->use_4addr) {
@@ -1369,26 +1366,21 @@ static int ieee80211_change_station(struct wiphy *wiphy,
}
 
rcu_assign_pointer(vlansdata->u.vlan.sta, sta);
-   new_4addr = true;
__ieee80211_check_fast_rx_iface(vlansdata);
}
 
if (sta->sdata->vif.type == NL80211_IFTYPE_AP_VLAN &&
-   sta->sdata->u.vlan.sta) {
+   sta->sdata->u.vlan.sta)
RCU_INIT_POINTER(sta->sdata->u.vlan.sta, NULL);
-   prev_4addr = true;
-   }
+
+   if (test_sta_flag(sta, WLAN_STA_AUTHORIZED))
+   ieee80211_vif_dec_num_mcast(sta->sdata);
 
sta->sdata = vlansdata;
ieee80211_check_fast_xmit(sta);
 
-   if (sta->sta_state == IEEE80211_STA_AUTHORIZED &&
-   prev_4addr != new_4addr) {
-   if (new_4addr)
-   atomic_dec(>sdata->bss->num_mcast_sta);
-   else
-   atomic_inc(>sdata->bss->num_mcast_sta);
-   }
+   if (test_sta_flag(sta, WLAN_STA_AUTHORIZED))
+   ieee80211_vif_inc_num_mcast(sta->sdata);
 
ieee80211_send_layer2_update(sta);
}
diff --git a/net/mac80211/debugfs_netdev.c b/net/mac80211/debugfs_netdev.c
index a5ba739..ed7bff4 100644
--- a/net/mac80211/debugfs_netdev.c
+++ b/net/mac80211/debugfs_netdev.c
@@ -477,6 +477,7 @@ IEEE80211_IF_FILE_RW(tdls_wider_bw);
 IEEE80211_IF_FILE(num_mcast_sta, u.ap.num_mcast_sta, ATOMIC);
 IEEE80211_IF_FILE(num_sta_ps, u.ap.ps.num_sta_ps, ATOMIC);
 IEEE80211_IF_FILE(dtim_count, u.ap.ps.dtim_count, DEC);
+IEEE80211_IF_FILE(num_mcast_sta_vlan, u.vlan.num_mcast_sta, ATOMIC);
 
 static ssize_t ieee80211_if_fmt_num_buffered_multicast(
const struct ieee80211_sub_if_data *sdata, char *buf, int buflen)
@@ -643,6 +644,13 @@ static void add_ap_files(struct ieee80211_sub_if_data 
*sdata)
DEBUGFS_ADD_MODE(tkip_mic_test, 0200);
 }
 
+static void add_vlan_files(struct ieee80211_sub_if_data *sdata)
+{
+   /* add num_mcast_sta_vlan using name num_mcast_sta */
+   debugfs_create_file("num_mcast_sta", 0400, sdata->vif.debugfs_dir,
+   sdata, _mcast_sta_vlan_ops);
+}
+
 static void add_ibss_files(struct ieee80211_sub_if_data *sdata)
 {
DEBUGFS_ADD_MODE(tsf, 0600);
@@ -746,6 +754,9 @@ static void add_files(struct ieee80211_sub_if_data *sdata)
case NL80211_IFTYPE_AP:
add_ap_files(sdata);

[PATCH v6 1/4] mac80211: remove unnecessary num_mcast_sta user

[Michael Braun]

Checking for num_mcast_sta in __ieee80211_request_smps_ap() is unnecessary,
as sta list will be empty in this case anyway, so list_for_each_entry(sta,
...) will exit immediately.

Signed-off-by: Michael Braun 
---
 net/mac80211/cfg.c | 7 ---
 1 file changed, 7 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 543b1d4..24133f5 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2313,13 +2313,6 @@ int __ieee80211_request_smps_ap(struct 
ieee80211_sub_if_data *sdata,
smps_mode == IEEE80211_SMPS_AUTOMATIC)
return 0;
 
-/* If no associated stations, there's no need to do anything */
-   if (!atomic_read(>u.ap.num_mcast_sta)) {
-   sdata->smps_mode = smps_mode;
-   ieee80211_queue_work(>local->hw, >recalc_smps);
-   return 0;
-   }
-
ht_dbg(sdata,
   "SMPS %d requested in AP mode, sending Action frame to %d 
stations\n",
   smps_mode, atomic_read(>u.ap.num_mcast_sta));
-- 
2.1.4

[PATCH v6 3/4] cfg80211: configure multicast to unicast for AP interfaces

[Michael Braun]

This add a userspace toggle to configure multicast to unicast.

Signed-off-by: Michael Braun 

--
v6:
 - clarify documentation
 - fix policy for NL80211_ATTR_MULTICAST_TO_UNICAST_ENABLED
---
 include/net/cfg80211.h   |  6 ++
 include/uapi/linux/nl80211.h | 18 ++
 net/wireless/nl80211.c   | 36 
 net/wireless/rdev-ops.h  | 12 
 net/wireless/trace.h | 19 +++
 5 files changed, 91 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 7ce6223..7b0941d 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2460,6 +2460,8 @@ struct cfg80211_qos_map {
  *
  * @set_wds_peer: set the WDS peer for a WDS interface
  *
+ * @set_multicast_to_unicast: configure multicast to unicast conversion for BSS
+ *
  * @rfkill_poll: polls the hw rfkill line, use cfg80211 reporting
  * functions to adjust rfkill hw state
  *
@@ -2722,6 +2724,10 @@ struct cfg80211_ops {
int (*set_wds_peer)(struct wiphy *wiphy, struct net_device *dev,
const u8 *addr);
 
+   int (*set_multicast_to_unicast)(struct wiphy *wiphy,
+   struct net_device *dev,
+   const bool enabled);
+
void(*rfkill_poll)(struct wiphy *wiphy);
 
 #ifdef CONFIG_NL80211_TESTMODE
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 2206941..327bbb8 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -599,6 +599,17 @@
  *
  * @NL80211_CMD_SET_WDS_PEER: Set the MAC address of the peer on a WDS 
interface.
  *
+ * @NL80211_CMD_SET_MULTICAST_TO_UNICAST: Configure if this AP should perform
+ *  multicast to unicast conversion. When enabled, all multicast packets
+ *  with ethertype ARP, IPv4 or IPv6 (possibly within an 802.1q header)
+ *  will be sent out to each station once with the destination (multicast)
+ *  mac address replaced by the stations mac address.
+ *  This can only be toggled per BSS. Configure this on an interface of
+ *  type %NL80211_IFTYPE_AP. It applies to all its vlans interfaces
+ *  (%NL80211_IFTYPE_AP_VLAN), except for those in 4addr (WDS) mode.
+ *  If %NL80211_ATTR_MULTICAST_TO_UNICAST_ENABLED is not present with this
+ *  command, the feature is disabled.
+ *
  * @NL80211_CMD_JOIN_MESH: Join a mesh. The mesh ID must be given, and initial
  * mesh config parameters may be given.
  * @NL80211_CMD_LEAVE_MESH: Leave the mesh network -- no special arguments, the
@@ -1026,6 +1037,8 @@ enum nl80211_commands {
 
NL80211_CMD_ABORT_SCAN,
 
+   NL80211_CMD_SET_MULTICAST_TO_UNICAST,
+
/* add new commands above here */
 
/* used to define NL80211_CMD_MAX below */
@@ -1867,6 +1880,9 @@ enum nl80211_commands {
  * @NL80211_ATTR_MESH_PEER_AID: Association ID for the mesh peer (u16). This is
  * used to pull the stored data for mesh peer in power save state.
  *
+ * @NL80211_ATTR_MULTICAST_TO_UNICAST_ENABLED: Multicast packets should be
+ *  send out as unicast to all stations.
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2261,6 +2277,8 @@ enum nl80211_attrs {
 
NL80211_ATTR_MESH_PEER_AID,
 
+   NL80211_ATTR_MULTICAST_TO_UNICAST_ENABLED,
+
/* add attributes here, update the policy in nl80211.c */
 
__NL80211_ATTR_AFTER_LAST,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index f02653a..3684e28 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -409,6 +409,7 @@ static const struct nla_policy 
nl80211_policy[NUM_NL80211_ATTR] = {
.len = VHT_MUMIMO_GROUPS_DATA_LEN
},
[NL80211_ATTR_MU_MIMO_FOLLOW_MAC_ADDR] = { .len = ETH_ALEN },
+   [NL80211_ATTR_MULTICAST_TO_UNICAST_ENABLED] = { .type = NLA_FLAG, },
 };
 
 /* policy for the key attributes */
@@ -1538,6 +1539,7 @@ static int nl80211_send_wiphy(struct 
cfg80211_registered_device *rdev,
goto nla_put_failure;
}
CMD(set_wds_peer, SET_WDS_PEER);
+   CMD(set_multicast_to_unicast, SET_MULTICAST_TO_UNICAST);
if (rdev->wiphy.flags & WIPHY_FLAG_SUPPORTS_TDLS) {
CMD(tdls_mgmt, TDLS_MGMT);
CMD(tdls_oper, TDLS_OPER);
@@ -2164,6 +2166,32 @@ static int nl80211_set_wds_peer(struct sk_buff *skb, 
struct genl_info *info)
return rdev_set_wds_peer(rdev, dev, bssid);
 }
 
+static int nl80211_set_multicast_to_unicast(struct sk_buff *skb,
+   struct genl_info *info)
+{
+   struct cfg80211_registered_device *rdev = info->user_ptr[0];
+   struct net_device *dev = info->user_ptr[1];
+  

Re: [PATCH] Revert "rtlwifi: rtl818x: constify local structures"

[Johannes Berg]

On Mon, 2016-10-10 at 10:25 -0500, Larry Finger wrote:
> This reverts commit d86e64768859fca82c78e52877ceeba04e25d27a.
> 
> For drivers rtl8188ee, rtl8192ce, rtl8192ee, rtl8723ae, and
> rtl8821ae,
> the Coccinelle script missed the fact that the code changes the
> firmware
> name. When that happens, the kernel issues a BUG splat because
> it is unable to overwrite the old name.

Hmm. That seems somewhat problematic, for example if you have multiple
devices that use the same driver but need different firmware?

Not that I really know what's going on, but changing static variables
based on runtime seems like it could cause issues in such cases.

johannes

[PATCH] mac80211: fix A-MSDU outer SA/DA

[Michael Braun]

According to IEEE 802.11-2012 section 8.3.2 table 8-19, the outer SA/DA
of A-MSDU frames need to be changed depending on FromDS/ToDS values.

Signed-off-by: Michael Braun 
---
 net/mac80211/tx.c | 24 +++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 5023966..acd334c 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3050,7 +3050,7 @@ static bool ieee80211_amsdu_prepare_head(struct 
ieee80211_sub_if_data *sdata,
int hdr_len = fast_tx->hdr_len - sizeof(rfc1042_header);
int subframe_len = skb->len - hdr_len;
void *data;
-   u8 *qc;
+   u8 *qc, *bssid;
 
if (info->flags & IEEE80211_TX_CTL_RATE_CTRL_PROBE)
return false;
@@ -3062,10 +3062,32 @@ static bool ieee80211_amsdu_prepare_head(struct 
ieee80211_sub_if_data *sdata,
 _len))
return false;
 
+   switch (sdata->vif.type) {
+   case NL80211_IFTYPE_STATION:
+   bssid = sdata->u.mgd.bssid;
+   break;
+   case NL80211_IFTYPE_AP:
+   case NL80211_IFTYPE_AP_VLAN:
+   bssid = sdata->vif.addr;
+   break;
+   default:
+   bssid = NULL;
+   }
+
amsdu_hdr.h_proto = cpu_to_be16(subframe_len);
memcpy(amsdu_hdr.h_source, skb->data + fast_tx->sa_offs, ETH_ALEN);
memcpy(amsdu_hdr.h_dest, skb->data + fast_tx->da_offs, ETH_ALEN);
 
+   /* according to IEEE 802.11-2012 8.3.2 table 8-19, the outer SA/DA
+* fields needs to be changed to BSSID for A-MSDU frames depending
+* on FromDS/ToDS values.
+*/
+   hdr = data;
+   if (bssid && ieee80211_has_fromds(hdr->frame_control))
+   memcpy(amsdu_hdr.h_source, bssid, ETH_ALEN);
+   if (bssid && ieee80211_has_tods(hdr->frame_control))
+   memcpy(amsdu_hdr.h_dest, bssid, ETH_ALEN);
+
data = skb_push(skb, sizeof(amsdu_hdr));
memmove(data, data + sizeof(amsdu_hdr), hdr_len);
memcpy(data + hdr_len, _hdr, sizeof(amsdu_hdr));
-- 
2.1.4

[PATCH v2] mac80211: enable to inject a-msdu frames using monitor interface

[Michael Braun]

Problem: When injecting an A-MSDU using a PF_PACKET socket, the qos flag
IEEE80211_QOS_CTL_A_MSDU_PRESENT is cleared.

How to reproduce: Inject a frame on a mac80211 hwsim monitor interface and
have tshark sniffing on this monitor interface.
You'll see the packet twice: Once with correct flag and once with flag
cleared. On hwsim0, you'll only see the packet with a cleared flag.

Signed-off-by: Michael Braun 
---
 net/mac80211/wme.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/net/mac80211/wme.c b/net/mac80211/wme.c
index 9eb0aee..f6a708c 100644
--- a/net/mac80211/wme.c
+++ b/net/mac80211/wme.c
@@ -248,6 +248,11 @@ void ieee80211_set_qos_hdr(struct ieee80211_sub_if_data 
*sdata,
/* preserve EOSP bit */
ack_policy = *p & IEEE80211_QOS_CTL_EOSP;
 
+   /* preserve A-MSDU bit for MONITOR interfaces to allow injecting
+* A-MSDU frames
+*/
+   ack_policy |= *p & IEEE80211_QOS_CTL_A_MSDU_PRESENT;
+
if (is_multicast_ether_addr(hdr->addr1) ||
sdata->noack_map & BIT(tid)) {
ack_policy |= IEEE80211_QOS_CTL_ACK_POLICY_NOACK;
-- 
2.1.4

Re: [PATCH v2] ath10k: cache calibration data when the core is stopped

[Valo, Kalle]

Marty Faltesek  writes:

> ath10k: cache calibration data when the core is stopped
>
> Caching calibration data allows it to be accessed when the
> device is not active.
>
> Signed-off-by: Marty Faltesek 

Thanks, I'll now send v3.

-- 
Kalle Valo

[PATCH v3] ath10k: cache calibration data when the core is stopped

[Kalle Valo]

From: Marty Faltesek 

Commit 0b8e3c4ca29f ("ath10k: move cal data len to hw_params") broke retrieving
the calibration data from cal_data debugfs file. The length of file was always
zero. The reason is:

static ssize_t ath10k_debug_cal_data_read(struct file *file,
  char __user *user_buf,
  size_t count, loff_t *ppos)
{
struct ath10k *ar = file->private_data;
void *buf = file->private_data;


This is obviously bogus, private_data cannot contain both struct ath10k and the
buffer. Fix it by caching calibration data to ar->debug.cal_data. This also
allows it to be accessed when the device is not active (interface is down).

The cal_data buffer is fixed size because during the first firmware probe we
don't yet know what will be the lenght of the calibration data. It was simplest
just to use a fixed length. There's a WARN_ON() in
ath10k_debug_cal_data_fetch() if the buffer is too small.

Tested with qca988x and firmware 10.2.4.70.56.

Reported-by: Nikolay Martynov 
Fixes: 0b8e3c4ca29f ("ath10k: move cal data len to hw_params")
Cc: sta...@vger.kernel.org # 4.7+
Signed-off-by: Marty Faltesek 
[kv...@qca.qualcomm.com: improve commit log and minor other changes]
Signed-off-by: Kalle Valo 
---
 drivers/net/wireless/ath/ath10k/core.h  |1 +
 drivers/net/wireless/ath/ath10k/debug.c |   79 ---
 2 files changed, 42 insertions(+), 38 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h 
b/drivers/net/wireless/ath/ath10k/core.h
index 6e5aa2d09699..b7067cc9328d 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -450,6 +450,7 @@ struct ath10k_debug {
u32 pktlog_filter;
u32 reg_addr;
u32 nf_cal_period;
+   void *cal_data;
 
struct ath10k_fw_crash_data *fw_crash_data;
 };
diff --git a/drivers/net/wireless/ath/ath10k/debug.c 
b/drivers/net/wireless/ath/ath10k/debug.c
index 832da6ed9f13..5d508efa0b31 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -30,6 +30,8 @@
 /* ms */
 #define ATH10K_DEBUG_HTT_STATS_INTERVAL 1000
 
+#define ATH10K_DEBUG_CAL_DATA_LEN 12064
+
 #define ATH10K_FW_CRASH_DUMP_VERSION 1
 
 /**
@@ -1451,75 +1453,68 @@ static const struct file_operations fops_fw_dbglog = {
.llseek = default_llseek,
 };
 
-static int ath10k_debug_cal_data_open(struct inode *inode, struct file *file)
+static int ath10k_debug_cal_data_fetch(struct ath10k *ar)
 {
-   struct ath10k *ar = inode->i_private;
-   void *buf;
u32 hi_addr;
__le32 addr;
int ret;
 
-   mutex_lock(>conf_mutex);
-
-   if (ar->state != ATH10K_STATE_ON &&
-   ar->state != ATH10K_STATE_UTF) {
-   ret = -ENETDOWN;
-   goto err;
-   }
+   lockdep_assert_held(>conf_mutex);
 
-   buf = vmalloc(ar->hw_params.cal_data_len);
-   if (!buf) {
-   ret = -ENOMEM;
-   goto err;
-   }
+   if (WARN_ON(ar->hw_params.cal_data_len > ATH10K_DEBUG_CAL_DATA_LEN))
+   return -EINVAL;
 
hi_addr = host_interest_item_address(HI_ITEM(hi_board_data));
 
ret = ath10k_hif_diag_read(ar, hi_addr, , sizeof(addr));
if (ret) {
-   ath10k_warn(ar, "failed to read hi_board_data address: %d\n", 
ret);
-   goto err_vfree;
+   ath10k_warn(ar, "failed to read hi_board_data address: %d\n",
+   ret);
+   return ret;
}
 
-   ret = ath10k_hif_diag_read(ar, le32_to_cpu(addr), buf,
+   ret = ath10k_hif_diag_read(ar, le32_to_cpu(addr), ar->debug.cal_data,
   ar->hw_params.cal_data_len);
if (ret) {
ath10k_warn(ar, "failed to read calibration data: %d\n", ret);
-   goto err_vfree;
+   return ret;
}
 
-   file->private_data = buf;
+   return 0;
+}
 
-   mutex_unlock(>conf_mutex);
+static int ath10k_debug_cal_data_open(struct inode *inode, struct file *file)
+{
+   struct ath10k *ar = inode->i_private;
 
-   return 0;
+   mutex_lock(>conf_mutex);
 
-err_vfree:
-   vfree(buf);
+   if (ar->state == ATH10K_STATE_ON ||
+   ar->state == ATH10K_STATE_UTF) {
+   ath10k_debug_cal_data_fetch(ar);
+   }
 
-err:
+   file->private_data = ar;
mutex_unlock(>conf_mutex);
 
-   return ret;
+   return 0;
 }
 
 static ssize_t ath10k_debug_cal_data_read(struct file *file,
- char __user *user_buf,
- size_t count, loff_t *ppos)
+  char __user *user_buf,
+  size_t count, loff_t *ppos)
 {
struct 

Re: [mac80211] BUG_ON with current -git (4.8.0-11417-g24532f7)

[Sergey Senozhatsky]

Cc Andy

Andy, can this be related to CONFIG_VMAP_STACK?

On (10/11/16 00:03), Sergey Senozhatsky wrote:
> Hello,
> 
> current -git kills my system. adding
> 
>   if (!virt_addr_valid([2])) {
>   WARN_ON(1);
>   return -EINVAL;
>   }
> 
> to ieee80211_aes_ccm_decrypt() given the following backtrace
> 
>  WARNING: CPU: 5 PID: 252 at net/mac80211/aes_ccm.c:77 
> ieee80211_aes_ccm_decrypt+0xc8/0x197
>  CPU: 5 PID: 252 Comm: irq/29-iwlwifi Tainted: GW   
> 4.8.0-next-20161010-dbg-7-g79797e9-dirty #88
>   c9413638 811ff0e3  
>   c9413678 8103fe91 004d01c8 192826d3
>   88040fc526d8 0008 c9413978 c941397a
>  Call Trace:
>   [] dump_stack+0x4f/0x65
>   [] __warn+0xc2/0xdd
>   [] warn_slowpath_null+0x1d/0x1f
>   [] ieee80211_aes_ccm_decrypt+0xc8/0x197
>   [] ? __put_page+0x3c/0x3f
>   [] ? put_page+0x4a/0x62
>   [] ? __pskb_pull_tail+0x1e8/0x279
>   [] ? ccmp_special_blocks.isra.5+0x51/0x12d
>   [] ieee80211_crypto_ccmp_decrypt+0x1ba/0x221
>   [] ieee80211_rx_handlers+0x52a/0x19c2
>   [] ? start_dl_timer+0xa8/0xb4
>   [] ? put_lock_stats.isra.24+0xe/0x20
>   [] ? del_timer+0x57/0x61
>   [] ieee80211_prepare_and_rx_handle+0xcd6/0xd2a
>   [] ? local_clock+0x10/0x12
>   [] ? __lock_acquire.isra.31+0x202/0x57e
>   [] ? rcu_read_unlock+0x23/0x23
>   [] ? sched_clock_cpu+0x17/0xc6
>   [] ieee80211_rx_napi+0x5af/0x698
>   [] ? get_lock_stats+0x19/0x50
>   [] ? put_lock_stats.isra.24+0xe/0x20
>   [] iwl_mvm_rx_rx_mpdu+0x5ab/0x60c [iwlmvm]
>   [] ? get_lock_stats+0x19/0x50
>   [] iwl_mvm_rx+0x45/0x69 [iwlmvm]
>   [] iwl_pcie_rx_handle+0x478/0x584 [iwlwifi]
>   [] iwl_pcie_irq_handler+0x39c/0x52d [iwlwifi]
>   [] ? irq_finalize_oneshot+0xa7/0xa7
>   [] irq_thread_fn+0x1d/0x34
>   [] irq_thread+0xe6/0x1bb
>   [] ? wake_threads_waitq+0x2c/0x2c
>   [] ? irq_thread_dtor+0x95/0x95
>   [] kthread+0xc6/0xce
>   [] ? put_lock_stats.isra.24+0xe/0x20
>   [] ? __list_del_entry+0x22/0x22
>   [] ret_from_fork+0x22/0x30
>  ---[ end trace 94da6d4698b938b2 ]---

-ss

[PATCH] Revert "rtlwifi: rtl818x: constify local structures"

[Larry Finger]

This reverts commit d86e64768859fca82c78e52877ceeba04e25d27a.

For drivers rtl8188ee, rtl8192ce, rtl8192ee, rtl8723ae, and rtl8821ae,
the Coccinelle script missed the fact that the code changes the firmware
name. When that happens, the kernel issues a BUG splat because
it is unable to overwrite the old name.

Although this bug only affects 5 of the 8 drivers it touched, I decided to
revert the entire patch. Continuing to constantify the other three could
too easily lead to introduction of future bugs.

Fixes: d86e64768859 ("rtlwifi: rtl818x: constify local structures")
Signed-off-by: Larry Finger 
Cc: Stable  
Cc: Julia Lawall 
---

Kalle,

My apologies for letting these bugs to get by my review and testing. As
they affect kernel 4.8, please push this patch as soon as possible.

Thanks,

Larry
---
 drivers/net/wireless/realtek/rtlwifi/rtl8188ee/sw.c | 2 +-
 drivers/net/wireless/realtek/rtlwifi/rtl8192ce/sw.c | 2 +-
 drivers/net/wireless/realtek/rtlwifi/rtl8192de/sw.c | 2 +-
 drivers/net/wireless/realtek/rtlwifi/rtl8192ee/sw.c | 2 +-
 drivers/net/wireless/realtek/rtlwifi/rtl8192se/sw.c | 2 +-
 drivers/net/wireless/realtek/rtlwifi/rtl8723ae/sw.c | 2 +-
 drivers/net/wireless/realtek/rtlwifi/rtl8723be/sw.c | 2 +-
 drivers/net/wireless/realtek/rtlwifi/rtl8821ae/sw.c | 2 +-
 8 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/sw.c 
b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/sw.c
index e7b11b4..47e32cb 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/sw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/sw.c
@@ -280,7 +280,7 @@ static struct rtl_mod_params rtl88ee_mod_params = {
.debug = DBG_EMERG,
 };
 
-static const struct rtl_hal_cfg rtl88ee_hal_cfg = {
+static struct rtl_hal_cfg rtl88ee_hal_cfg = {
.bar_id = 2,
.write_readback = true,
.name = "rtl88e_pci",
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/sw.c 
b/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/sw.c
index 5c46a98..5400a96 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/sw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/sw.c
@@ -254,7 +254,7 @@ static struct rtl_mod_params rtl92ce_mod_params = {
.debug = DBG_EMERG,
 };
 
-static const struct rtl_hal_cfg rtl92ce_hal_cfg = {
+static struct rtl_hal_cfg rtl92ce_hal_cfg = {
.bar_id = 2,
.write_readback = true,
.name = "rtl92c_pci",
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192de/sw.c 
b/drivers/net/wireless/realtek/rtlwifi/rtl8192de/sw.c
index a9c39eb..60f42ae 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192de/sw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192de/sw.c
@@ -258,7 +258,7 @@ static struct rtl_mod_params rtl92de_mod_params = {
.debug = DBG_EMERG,
 };
 
-static const struct rtl_hal_cfg rtl92de_hal_cfg = {
+static struct rtl_hal_cfg rtl92de_hal_cfg = {
.bar_id = 2,
.write_readback = true,
.name = "rtl8192de",
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/sw.c 
b/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/sw.c
index ac299cb..c31c6bf 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/sw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/sw.c
@@ -262,7 +262,7 @@ static struct rtl_mod_params rtl92ee_mod_params = {
.debug = DBG_EMERG,
 };
 
-static const struct rtl_hal_cfg rtl92ee_hal_cfg = {
+static struct rtl_hal_cfg rtl92ee_hal_cfg = {
.bar_id = 2,
.write_readback = true,
.name = "rtl92ee_pci",
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192se/sw.c 
b/drivers/net/wireless/realtek/rtlwifi/rtl8192se/sw.c
index a652d45..9378b0d 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192se/sw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192se/sw.c
@@ -302,7 +302,7 @@ static struct rtl_mod_params rtl92se_mod_params = {
 
 /* Because memory R/W bursting will cause system hang/crash
  * for 92se, so we don't read back after every write action */
-static const struct rtl_hal_cfg rtl92se_hal_cfg = {
+static struct rtl_hal_cfg rtl92se_hal_cfg = {
.bar_id = 1,
.write_readback = false,
.name = "rtl92s_pci",
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/sw.c 
b/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/sw.c
index 89c828a..ff49a8c 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/sw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/sw.c
@@ -276,7 +276,7 @@ static struct rtl_mod_params rtl8723e_mod_params = {
.disable_watchdog = false,
 };
 
-static const struct rtl_hal_cfg rtl8723e_hal_cfg = {
+static struct rtl_hal_cfg rtl8723e_hal_cfg = {
.bar_id = 2,
.write_readback = true,
.name = "rtl8723e_pci",
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/sw.c 
b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/sw.c
index 20b53f0..2101793 100644

[mac80211] BUG_ON with current -git (4.8.0-11417-g24532f7)

[Sergey Senozhatsky]

Hello,

current -git kills my system. adding

if (!virt_addr_valid([2])) {
WARN_ON(1);
return -EINVAL;
}

to ieee80211_aes_ccm_decrypt() given the following backtrace

 WARNING: CPU: 5 PID: 252 at net/mac80211/aes_ccm.c:77 
ieee80211_aes_ccm_decrypt+0xc8/0x197
 CPU: 5 PID: 252 Comm: irq/29-iwlwifi Tainted: GW   
4.8.0-next-20161010-dbg-7-g79797e9-dirty #88
  c9413638 811ff0e3  
  c9413678 8103fe91 004d01c8 192826d3
  88040fc526d8 0008 c9413978 c941397a
 Call Trace:
  [] dump_stack+0x4f/0x65
  [] __warn+0xc2/0xdd
  [] warn_slowpath_null+0x1d/0x1f
  [] ieee80211_aes_ccm_decrypt+0xc8/0x197
  [] ? __put_page+0x3c/0x3f
  [] ? put_page+0x4a/0x62
  [] ? __pskb_pull_tail+0x1e8/0x279
  [] ? ccmp_special_blocks.isra.5+0x51/0x12d
  [] ieee80211_crypto_ccmp_decrypt+0x1ba/0x221
  [] ieee80211_rx_handlers+0x52a/0x19c2
  [] ? start_dl_timer+0xa8/0xb4
  [] ? put_lock_stats.isra.24+0xe/0x20
  [] ? del_timer+0x57/0x61
  [] ieee80211_prepare_and_rx_handle+0xcd6/0xd2a
  [] ? local_clock+0x10/0x12
  [] ? __lock_acquire.isra.31+0x202/0x57e
  [] ? rcu_read_unlock+0x23/0x23
  [] ? sched_clock_cpu+0x17/0xc6
  [] ieee80211_rx_napi+0x5af/0x698
  [] ? get_lock_stats+0x19/0x50
  [] ? put_lock_stats.isra.24+0xe/0x20
  [] iwl_mvm_rx_rx_mpdu+0x5ab/0x60c [iwlmvm]
  [] ? get_lock_stats+0x19/0x50
  [] iwl_mvm_rx+0x45/0x69 [iwlmvm]
  [] iwl_pcie_rx_handle+0x478/0x584 [iwlwifi]
  [] iwl_pcie_irq_handler+0x39c/0x52d [iwlwifi]
  [] ? irq_finalize_oneshot+0xa7/0xa7
  [] irq_thread_fn+0x1d/0x34
  [] irq_thread+0xe6/0x1bb
  [] ? wake_threads_waitq+0x2c/0x2c
  [] ? irq_thread_dtor+0x95/0x95
  [] kthread+0xc6/0xce
  [] ? put_lock_stats.isra.24+0xe/0x20
  [] ? __list_del_entry+0x22/0x22
  [] ret_from_fork+0x22/0x30
 ---[ end trace 94da6d4698b938b2 ]---

-ss

Re: [PATCH v2] ath10k: cache calibration data when the core is stopped

[Valo, Kalle]

Marty Faltesek  writes:

> Caching calibration data allows it to be accessed when the
> device is not active.
>
> ---

Signed-off-by missing. Can you send it as a reply to this message and
I'll add it to v3?

>  drivers/net/wireless/ath/ath10k/core.h  |  1 +
>  drivers/net/wireless/ath/ath10k/debug.c | 72 
> ++---
>  2 files changed, 31 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.h 
> b/drivers/net/wireless/ath/ath10k/core.h
> index 6e5aa2d..7274ebe 100644
> --- a/drivers/net/wireless/ath/ath10k/core.h
> +++ b/drivers/net/wireless/ath/ath10k/core.h
> @@ -452,6 +452,7 @@ struct ath10k_debug {
>   u32 nf_cal_period;
>  
>   struct ath10k_fw_crash_data *fw_crash_data;
> + void *cal_data;

To properly document locking I'll move this under the "protected by
conf_mutex" comment.

> -static int ath10k_debug_cal_data_open(struct inode *inode, struct file *file)
> +static int ath10k_debug_cal_data_fetch(struct ath10k *ar)
>  {
> - struct ath10k *ar = inode->i_private;
> - void *buf;
>   u32 hi_addr;
>   __le32 addr;
>   int ret;
>  
> - mutex_lock(>conf_mutex);

I'll add lockdep_assert_held() here.

>  static ssize_t ath10k_debug_cal_data_read(struct file *file,
> -   char __user *user_buf,
> -   size_t count, loff_t *ppos)
> +  char __user *user_buf,
> +  size_t count, loff_t *ppos)
>  {
>   struct ath10k *ar = file->private_data;
> - void *buf = file->private_data;
>  
>   return simple_read_from_buffer(user_buf, count, ppos,
> -buf, ar->hw_params.cal_data_len);
> -}

I'll add locking to this function.

> @@ -2343,7 +2327,9 @@ int ath10k_debug_create(struct ath10k *ar)
>   ar->debug.fw_crash_data = vzalloc(sizeof(*ar->debug.fw_crash_data));
>   if (!ar->debug.fw_crash_data)
>   return -ENOMEM;
> -
> + ar->debug.cal_data = vzalloc(ar->hw_params.cal_data_len);
> + if (!ar->debug.cal_data)
> + return -ENOMEM;
>   INIT_LIST_HEAD(>debug.fw_stats.pdevs);
>   INIT_LIST_HEAD(>debug.fw_stats.vdevs);
>   INIT_LIST_HEAD(>debug.fw_stats.peers);
> @@ -2355,7 +2341,9 @@ int ath10k_debug_create(struct ath10k *ar)
>  void ath10k_debug_destroy(struct ath10k *ar)
>  {
>   vfree(ar->debug.fw_crash_data);
> + vfree(ar->debug.cal_data);
>   ar->debug.fw_crash_data = NULL;
> + ar->debug.cal_data = NULL;

Actually I gave you a bad advice, this won't work as
ar->hw_params.cal_data_len is not yet initialised, we initialise it only
after we have probed the capabilities during the first firmware boot. I
changed the allocation to a fixed length buffer for now, it's only few
kBs virtual memory anyway so it shouldn't matter to anyone.

I have now v3 ready and tested. I'll just need your Signed-off-by and I
can then submit it.

-- 
Kalle Valo

Re: [PATCH v2] ath10k: cache calibration data when the core is stopped

[Marty Faltesek]

ath10k: cache calibration data when the core is stopped

Caching calibration data allows it to be accessed when the
device is not active.

Signed-off-by: Marty Faltesek 

On Mon, Oct 10, 2016 at 10:54 AM, Valo, Kalle  wrote:
> Marty Faltesek  writes:
>
>> Caching calibration data allows it to be accessed when the
>> device is not active.
>>
>> ---
>
> Signed-off-by missing. Can you send it as a reply to this message and
> I'll add it to v3?
>
>>  drivers/net/wireless/ath/ath10k/core.h  |  1 +
>>  drivers/net/wireless/ath/ath10k/debug.c | 72 
>> ++---
>>  2 files changed, 31 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/core.h 
>> b/drivers/net/wireless/ath/ath10k/core.h
>> index 6e5aa2d..7274ebe 100644
>> --- a/drivers/net/wireless/ath/ath10k/core.h
>> +++ b/drivers/net/wireless/ath/ath10k/core.h
>> @@ -452,6 +452,7 @@ struct ath10k_debug {
>>   u32 nf_cal_period;
>>
>>   struct ath10k_fw_crash_data *fw_crash_data;
>> + void *cal_data;
>
> To properly document locking I'll move this under the "protected by
> conf_mutex" comment.
>
>> -static int ath10k_debug_cal_data_open(struct inode *inode, struct file 
>> *file)
>> +static int ath10k_debug_cal_data_fetch(struct ath10k *ar)
>>  {
>> - struct ath10k *ar = inode->i_private;
>> - void *buf;
>>   u32 hi_addr;
>>   __le32 addr;
>>   int ret;
>>
>> - mutex_lock(>conf_mutex);
>
> I'll add lockdep_assert_held() here.
>
>>  static ssize_t ath10k_debug_cal_data_read(struct file *file,
>> -   char __user *user_buf,
>> -   size_t count, loff_t *ppos)
>> +  char __user *user_buf,
>> +  size_t count, loff_t *ppos)
>>  {
>>   struct ath10k *ar = file->private_data;
>> - void *buf = file->private_data;
>>
>>   return simple_read_from_buffer(user_buf, count, ppos,
>> -buf, ar->hw_params.cal_data_len);
>> -}
>
> I'll add locking to this function.
>
>> @@ -2343,7 +2327,9 @@ int ath10k_debug_create(struct ath10k *ar)
>>   ar->debug.fw_crash_data = vzalloc(sizeof(*ar->debug.fw_crash_data));
>>   if (!ar->debug.fw_crash_data)
>>   return -ENOMEM;
>> -
>> + ar->debug.cal_data = vzalloc(ar->hw_params.cal_data_len);
>> + if (!ar->debug.cal_data)
>> + return -ENOMEM;
>>   INIT_LIST_HEAD(>debug.fw_stats.pdevs);
>>   INIT_LIST_HEAD(>debug.fw_stats.vdevs);
>>   INIT_LIST_HEAD(>debug.fw_stats.peers);
>> @@ -2355,7 +2341,9 @@ int ath10k_debug_create(struct ath10k *ar)
>>  void ath10k_debug_destroy(struct ath10k *ar)
>>  {
>>   vfree(ar->debug.fw_crash_data);
>> + vfree(ar->debug.cal_data);
>>   ar->debug.fw_crash_data = NULL;
>> + ar->debug.cal_data = NULL;
>
> Actually I gave you a bad advice, this won't work as
> ar->hw_params.cal_data_len is not yet initialised, we initialise it only
> after we have probed the capabilities during the first firmware boot. I
> changed the allocation to a fixed length buffer for now, it's only few
> kBs virtual memory anyway so it shouldn't matter to anyone.
>
> I have now v3 ready and tested. I'll just need your Signed-off-by and I
> can then submit it.
>
> --
> Kalle Valo

[PATCH] ath10k: Fix kernel panic due to race in accessing arvif list

[Vasanthakumar Thiagarajan]

arvifs list is traversed within data_lock spin_lock in tasklet
context to fill channel information from the corresponding vif.
This means any access to arvifs list for add/del operations
should also be protected with the same spin_lock to avoid the
race. Fix this by performing list add/del on arvfis within the
data_lock. This could fix kernel panic something like the below.

 LR is at ath10k_htt_rx_pktlog_completion_handler+0x100/0xb6c [ath10k_core]
 PC is at ath10k_htt_rx_pktlog_completion_handler+0x1c0/0xb6c [ath10k_core]
 Internal error: Oops: 17 [#1] PREEMPT SMP ARM
 [] (ath10k_htt_rx_pktlog_completion_handler+0x2f4/0xb6c 
[ath10k_core])
 [] (ath10k_htt_txrx_compl_task+0x8b4/0x1188 [ath10k_core])
 [] (tasklet_action+0x8c/0xec)
 [] (__do_softirq+0xdc/0x208)
 [] (irq_exit+0x84/0xe0)
 [] (__handle_domain_irq+0x80/0xa0)
 [] (gic_handle_irq+0x38/0x5c)
 [] (__irq_svc+0x40/0x74)

(gdb) list *(ath10k_htt_rx_pktlog_completion_handler+0x1c0)
0x136c0 is in ath10k_htt_rx_h_channel 
(drivers/net/wireless/ath/ath10k/htt_rx.c:769)
764 struct cfg80211_chan_def def;
765
766 lockdep_assert_held(>data_lock);
767
768 list_for_each_entry(arvif, >arvifs, list) {
769 if (arvif->vdev_id == vdev_id &&
770 ath10k_mac_vif_chan(arvif->vif, ) == 0)
771 return def.chan;
772 }
773

Signed-off-by: Vasanthakumar Thiagarajan 
---
 drivers/net/wireless/ath/ath10k/mac.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c 
b/drivers/net/wireless/ath/ath10k/mac.c
index 2e5d2ca..691b7b5 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -4931,7 +4931,9 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
}
 
ar->free_vdev_map &= ~(1LL << arvif->vdev_id);
+   spin_lock_bh(>data_lock);
list_add(>list, >arvifs);
+   spin_unlock_bh(>data_lock);
 
/* It makes no sense to have firmware do keepalives. mac80211 already
 * takes care of this with idle connection polling.
@@ -5082,7 +5084,9 @@ err_peer_delete:
 err_vdev_delete:
ath10k_wmi_vdev_delete(ar, arvif->vdev_id);
ar->free_vdev_map |= 1LL << arvif->vdev_id;
+   spin_lock_bh(>data_lock);
list_del(>list);
+   spin_unlock_bh(>data_lock);
 
 err:
if (arvif->beacon_buf) {
@@ -5128,7 +5132,9 @@ static void ath10k_remove_interface(struct ieee80211_hw 
*hw,
arvif->vdev_id, ret);
 
ar->free_vdev_map |= 1LL << arvif->vdev_id;
+   spin_lock_bh(>data_lock);
list_del(>list);
+   spin_unlock_bh(>data_lock);
 
if (arvif->vdev_type == WMI_VDEV_TYPE_AP ||
arvif->vdev_type == WMI_VDEV_TYPE_IBSS) {
-- 
1.9.1

Re: [PATCH] iwlwifi: pcie: reduce "unsupported splx" to a warning

[Luca Coelho]

Hi,
On Mon, 2016-10-10 at 02:19 -0500, Chris Rorvick wrote:
> Commit bcb079a14d75 ("iwlwifi: pcie: retrieve and parse ACPI power
> limitations") looks for a specific structure in the ACPI tables for
> setting the default power limit.  The data returned for at least some
> dual band chipsets is not recognized, though.  For example, the AC 8260
> reports the following:

This is not coming from the NIC itself, but from the platform's ACPI
tables.  Can you tell us which platform you are using?


> Name (SPLX, Package (0x04)
> {
> Zero,
> Package (0x03)
> {
> 0,
> 1200,
> 1000
> },
> Package (0x03)
> {
> 0,
> 1200,
> 1000
> },
> Package (0x03)
> {
> 0,
> 1200,
> 1000
> }
> })

This is not the structure that we are expecting.  We expect this:

   Name (SPLX, Package (0x02)
   {
   Zero,
   Package (0x03)
   {
   0x07,
   ,
   
   }
   })

...as you correctly pointed out.  The data in the structure you have is
not for WiFi (actually I don't think 0 is a valid value, but I'll
double-check).


> The current logic expects exactly two elements in the outer package,
> causing the above to be ignored and the power limit unset.
> 
> Despite the interface being fully functional after initialization, the
> above condition is reported as an error.  Knock the message down to a
> warning and provide better context for understanding its consequence.

Reducing this to a warning is an easy way to reduce the verbosity of
the problem, but I think the correct thing to do would be to accept
multiple entries and ignore the ones that don't have the WIFI marker.
 And only type-check the WIFI ones.

There are other things that look a bit inconsistent in this code...
I'll try to find the official ACPI table definitions for this entries
to make sure it's correct.


> Signed-off-by: Chris Rorvick 
> ---
>  drivers/net/wireless/intel/iwlwifi/pcie/drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c 
> b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> index 78cf9a7..19b531f 100644
> --- a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> +++ b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> @@ -540,7 +540,7 @@ static u64 splx_get_pwr_limit(struct iwl_trans *trans, 
> union acpi_object *splx)
>   splx->package.count != 2 ||
>   splx->package.elements[0].type != ACPI_TYPE_INTEGER ||
>   splx->package.elements[0].integer.value != 0) {
> - IWL_ERR(trans, "Unsupported splx structure\n");
> + IWL_WARN(trans, "Unsupported splx structure, not limiting WiFi 
> power\n");
>   return 0;
>   }

If this is really bothering you, I guess I could apply this patch for
now.  But as I said, this is not solving the actual problem.

--
Cheers,
Luca.

[PATCH] cfg80211: Check radar_detect and num_different_channels with beacon interface combinations.

[Purushottam Kushwaha]

This commit enhances the current beacon interval validation to also consider
the "radar_detect" and "num_different_channels".

Rename "cfg80211_validate_beacon_int" to "cfg80211_validate_beacon_combination"
as the validation considers other parameters.

Signed-off-by: Purushottam Kushwaha 
---
 net/wireless/core.h|  7 +--
 net/wireless/mesh.c|  7 +++
 net/wireless/nl80211.c | 32 ++--
 net/wireless/util.c| 49 ++---
 4 files changed, 72 insertions(+), 23 deletions(-)

diff --git a/net/wireless/core.h b/net/wireless/core.h
index 21e3188..e39c8a9 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -474,8 +474,11 @@ int ieee80211_get_ratemask(struct ieee80211_supported_band 
*sband,
   const u8 *rates, unsigned int n_rates,
   u32 *mask);
 
-int cfg80211_validate_beacon_int(struct cfg80211_registered_device *rdev,
-enum nl80211_iftype iftype, u32 beacon_int);
+int
+cfg80211_validate_beacon_combination(struct cfg80211_registered_device *rdev,
+enum nl80211_iftype iftype,
+struct cfg80211_chan_def *chandef,
+u32 beacon_int);
 
 void cfg80211_update_iface_num(struct cfg80211_registered_device *rdev,
   enum nl80211_iftype iftype, int num);
diff --git a/net/wireless/mesh.c b/net/wireless/mesh.c
index fa2066b..1d864b4 100644
--- a/net/wireless/mesh.c
+++ b/net/wireless/mesh.c
@@ -178,6 +178,13 @@ int __cfg80211_join_mesh(struct cfg80211_registered_device 
*rdev,
 NL80211_IFTYPE_MESH_POINT))
return -EINVAL;
 
+   err = cfg80211_validate_beacon_combination(rdev,
+  NL80211_IFTYPE_MESH_POINT,
+  >chandef,
+  setup->beacon_interval);
+   if (err)
+   return err;
+
err = rdev_join_mesh(rdev, dev, conf, setup);
if (!err) {
memcpy(wdev->ssid, setup->mesh_id, setup->mesh_id_len);
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 903cd5a..eb2bfae 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -3807,11 +3807,6 @@ static int nl80211_start_ap(struct sk_buff *skb, struct 
genl_info *info)
params.dtim_period =
nla_get_u32(info->attrs[NL80211_ATTR_DTIM_PERIOD]);
 
-   err = cfg80211_validate_beacon_int(rdev, dev->ieee80211_ptr->iftype,
-  params.beacon_interval);
-   if (err)
-   return err;
-
/*
 * In theory, some of these attributes should be required here
 * but since they were not used when the command was originally
@@ -3899,6 +3894,13 @@ static int nl80211_start_ap(struct sk_buff *skb, struct 
genl_info *info)
   wdev->iftype))
return -EINVAL;
 
+   err = cfg80211_validate_beacon_combination(rdev,
+  dev->ieee80211_ptr->iftype,
+  ,
+  params.beacon_interval);
+   if (err)
+   return err;
+
if (info->attrs[NL80211_ATTR_TX_RATES]) {
err = nl80211_parse_tx_bitrate_mask(info, _rate);
if (err)
@@ -8157,11 +8159,6 @@ static int nl80211_join_ibss(struct sk_buff *skb, struct 
genl_info *info)
ibss.beacon_interval =
nla_get_u32(info->attrs[NL80211_ATTR_BEACON_INTERVAL]);
 
-   err = cfg80211_validate_beacon_int(rdev, NL80211_IFTYPE_ADHOC,
-  ibss.beacon_interval);
-   if (err)
-   return err;
-
if (!rdev->ops->join_ibss)
return -EOPNOTSUPP;
 
@@ -8188,6 +8185,12 @@ static int nl80211_join_ibss(struct sk_buff *skb, struct 
genl_info *info)
if (err)
return err;
 
+   err = cfg80211_validate_beacon_combination(rdev, NL80211_IFTYPE_ADHOC,
+  ,
+  ibss.beacon_interval);
+   if (err)
+   return err;
+
if (!cfg80211_reg_can_beacon(>wiphy, ,
 NL80211_IFTYPE_ADHOC))
return -EINVAL;
@@ -9419,17 +9422,10 @@ static int nl80211_join_mesh(struct sk_buff *skb, 
struct genl_info *info)
nla_get_u32(info->attrs[NL80211_ATTR_MCAST_RATE])))
return -EINVAL;
 
-   if (info->attrs[NL80211_ATTR_BEACON_INTERVAL]) {
+   if (info->attrs[NL80211_ATTR_BEACON_INTERVAL])

[PATCH] cfg80211: Create structure for combination check/iter function parameters

[Purushottam Kushwaha]

Move growing parameter list to a structure for check/iter combination
functions in cfg80211 and mac80211.

Signed-off-by: Purushottam Kushwaha 
---
 .../broadcom/brcm80211/brcmfmac/cfg80211.c | 25 -
 include/net/cfg80211.h | 56 ++--
 net/mac80211/util.c| 42 ---
 net/wireless/util.c| 59 +-
 4 files changed, 86 insertions(+), 96 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 1e73c88..00c5fb2 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -414,24 +414,25 @@ static int brcmf_vif_change_validate(struct 
brcmf_cfg80211_info *cfg,
 struct brcmf_cfg80211_vif *vif,
 enum nl80211_iftype new_type)
 {
-   int iftype_num[NUM_NL80211_IFTYPES];
+   struct iface_combination_params params;
struct brcmf_cfg80211_vif *pos;
bool check_combos = false;
int ret = 0;
 
-   memset(_num[0], 0, sizeof(iftype_num));
+   memset(, 0, sizeof(params));
+   params.num_different_channels = 1;
+
list_for_each_entry(pos, >vif_list, list)
if (pos == vif) {
-   iftype_num[new_type]++;
+   params.iftype_num[new_type]++;
} else {
/* concurrent interfaces so need check combinations */
check_combos = true;
-   iftype_num[pos->wdev.iftype]++;
+   params.iftype_num[pos->wdev.iftype]++;
}
 
if (check_combos)
-   ret = cfg80211_check_combinations(cfg->wiphy, 1, 0,
- iftype_num, 0, false);
+   ret = cfg80211_check_combinations(cfg->wiphy, );
 
return ret;
 }
@@ -439,16 +440,16 @@ static int brcmf_vif_change_validate(struct 
brcmf_cfg80211_info *cfg,
 static int brcmf_vif_add_validate(struct brcmf_cfg80211_info *cfg,
  enum nl80211_iftype new_type)
 {
-   int iftype_num[NUM_NL80211_IFTYPES];
+   struct iface_combination_params params;
struct brcmf_cfg80211_vif *pos;
 
-   memset(_num[0], 0, sizeof(iftype_num));
+   memset(, 0, sizeof(params));
list_for_each_entry(pos, >vif_list, list)
-   iftype_num[pos->wdev.iftype]++;
+   params.iftype_num[pos->wdev.iftype]++;
 
-   iftype_num[new_type]++;
-   return cfg80211_check_combinations(cfg->wiphy, 1, 0,
-  iftype_num, 0, false);
+   params.iftype_num[new_type]++;
+   params.num_different_channels = 1;
+   return cfg80211_check_combinations(cfg->wiphy, );
 }
 
 static void convert_key_from_CPU(struct brcmf_wsec_key *key,
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 49aa6a0..4e509ed 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -772,6 +772,30 @@ struct cfg80211_csa_settings {
 };
 
 /**
+ * struct iface_combination_params - input parameters for interface 
combinations
+ *
+ * Used to pass interface combination parameters
+ *
+ * @num_different_channels: the number of different channels we want
+ * to use for verification
+ * @radar_detect: a bitmap where each bit corresponds to a channel
+ * width where radar detection is needed, as in the definition of
+ *  ieee80211_iface_combination.@radar_detect_widths
+ * @iftype_num: array with the numbers of interfaces of each interface
+ * type.  The index is the interface type as specified in 
+ * nl80211_iftype.
+ * @beacon_gcd: a value specifying GCD of all beaconing interfaces.
+ * @diff_bi: a flag which denotes beacon intervals are different or same.
+ */
+struct iface_combination_params {
+   int num_different_channels;
+   u8 radar_detect;
+   int iftype_num[NUM_NL80211_IFTYPES];
+   u32 beacon_gcd;
+   bool diff_bi;
+};
+
+/**
  * enum station_parameters_apply_mask - station parameter values to apply
  * @STATION_PARAM_APPLY_UAPSD: apply new uAPSD parameters (uapsd_queues, 
max_sp)
  * @STATION_PARAM_APPLY_CAPABILITY: apply new capability
@@ -5583,41 +5607,20 @@ unsigned int 
ieee80211_get_num_supported_channels(struct wiphy *wiphy);
  * cfg80211_check_combinations - check interface combinations
  *
  * @wiphy: the wiphy
- * @num_different_channels: the number of different channels we want
- * to use for verification
- * @radar_detect: a bitmap where each bit corresponds to a channel
- * width where radar detection is needed, as in the definition of
- *  ieee80211_iface_combination.@radar_detect_widths
- * @iftype_num: array with the numbers of interfaces of 

[PATCH v9] cfg80211: Provision to allow the support for different beacon intervals

[Purushottam Kushwaha]

This commit provides a mechanism for the host drivers to advertise the
support for different beacon intervals among the respective interface
combinations in a group, through beacon_int_min_gcd (u32).
This beacon_int_min_gcd will be compared against GCD of all beaconing
interfaces of matching combinations.

Following sets the expectation for beacon_int_min_gcd.

= 0 - all beacon intervals for different interfaces must be same.
> 0 - any beacon interval for the interface part of this combination AND
  *GCD* of all beacon intervals from beaconing interfaces of this
  combination must be greator or equal to this value.

Signed-off-by: Purushottam Kushwaha 
---
 .../broadcom/brcm80211/brcmfmac/cfg80211.c |  6 ++-
 include/net/cfg80211.h | 18 -
 include/uapi/linux/nl80211.h   |  8 +++-
 net/mac80211/util.c|  4 +-
 net/wireless/core.h|  2 +-
 net/wireless/nl80211.c | 14 +--
 net/wireless/util.c| 43 --
 7 files changed, 79 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index b777e1b..1e73c88 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -430,7 +430,8 @@ static int brcmf_vif_change_validate(struct 
brcmf_cfg80211_info *cfg,
}
 
if (check_combos)
-   ret = cfg80211_check_combinations(cfg->wiphy, 1, 0, iftype_num);
+   ret = cfg80211_check_combinations(cfg->wiphy, 1, 0,
+ iftype_num, 0, false);
 
return ret;
 }
@@ -446,7 +447,8 @@ static int brcmf_vif_add_validate(struct 
brcmf_cfg80211_info *cfg,
iftype_num[pos->wdev.iftype]++;
 
iftype_num[new_type]++;
-   return cfg80211_check_combinations(cfg->wiphy, 1, 0, iftype_num);
+   return cfg80211_check_combinations(cfg->wiphy, 1, 0,
+  iftype_num, 0, false);
 }
 
 static void convert_key_from_CPU(struct brcmf_wsec_key *key,
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index fe78f02..49aa6a0 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -3080,6 +3080,12 @@ struct ieee80211_iface_limit {
  * only in special cases.
  * @radar_detect_widths: bitmap of channel widths supported for radar detection
  * @radar_detect_regions: bitmap of regions supported for radar detection
+ * @beacon_int_min_gcd: This interface combination supports different
+ * beacon intervals.
+ * = 0 - all beacon intervals for different interface must be same.
+ * > 0 - any beacon interval for the interface part of this combination AND
+ *   *GCD* of all beacon intervals from beaconing interfaces of this
+ *   combination must be greator or equal to this value.
  *
  * With this structure the driver can describe which interface
  * combinations it supports concurrently.
@@ -3100,7 +3106,7 @@ struct ieee80211_iface_limit {
  *  };
  *
  *
- * 2. Allow #{AP, P2P-GO} <= 8, channels = 1, 8 total:
+ * 2. Allow #{AP, P2P-GO} <= 8, BI min gcd = 10, channels = 1, 8 total:
  *
  *  struct ieee80211_iface_limit limits2[] = {
  * { .max = 8, .types = BIT(NL80211_IFTYPE_AP) |
@@ -3111,6 +3117,7 @@ struct ieee80211_iface_limit {
  * .n_limits = ARRAY_SIZE(limits2),
  * .max_interfaces = 8,
  * .num_different_channels = 1,
+ * .beacon_int_min_gcd = 10,
  *  };
  *
  *
@@ -3138,6 +3145,7 @@ struct ieee80211_iface_combination {
bool beacon_int_infra_match;
u8 radar_detect_widths;
u8 radar_detect_regions;
+   u32 beacon_int_min_gcd;
 };
 
 struct ieee80211_txrx_stypes {
@@ -5583,6 +5591,8 @@ unsigned int ieee80211_get_num_supported_channels(struct 
wiphy *wiphy);
  * @iftype_num: array with the numbers of interfaces of each interface
  * type.  The index is the interface type as specified in 
  * nl80211_iftype.
+ * @beacon_gcd: a value specifying GCD of all beaconing interfaces.
+ * @diff_bi: a flag which denotes beacon intervals are different or same.
  *
  * This function can be called by the driver to check whether a
  * combination of interfaces and their types are allowed according to
@@ -5591,7 +5601,8 @@ unsigned int ieee80211_get_num_supported_channels(struct 
wiphy *wiphy);
 int cfg80211_check_combinations(struct wiphy *wiphy,
const int num_different_channels,
const u8 radar_detect,
-   const int iftype_num[NUM_NL80211_IFTYPES]);
+   const int iftype_num[NUM_NL80211_IFTYPES],
+   const u32 beacon_gcd, bool diff_bi);
 

Re: [RFC] mac80211: fix A-MSDU outer SA/DA

[Johannes Berg]

>  
> + /* according to IEEE 802.11-2012 8.3.2 table 8-19, the outer
> SA/DA
> +  * fields needs to be changed to BSSID for A-MSDU frames
> depending
> +  * on FromDS/ToDS values.
> +  */
> + hdr = data;
> + if (bssid && (hdr->frame_control &
> cpu_to_le16(IEEE80211_FCTL_FROMDS)))
> + memcpy(amsdu_hdr.h_source, bssid, ETH_ALEN);
> + if (bssid && (hdr->frame_control &
> cpu_to_le16(IEEE80211_FCTL_TODS)))
> + memcpy(amsdu_hdr.h_dest, bssid, ETH_ALEN);
> 

You should probably use ieee80211_has_tods() and ieee80211_has_fromds()

johannes

[PATCH] mac80211_hwsim: make multi-channel ops const

[Johannes Berg]

From: Johannes Berg 

Instead of building the multi-channel ops at runtime, declare
the common ops with a macro and build both that way, so that
the multi-channel ops can also be const.

As a side effect, due to the removed code, this decreases the
size of the module (while shifting data from .bss to .text
due to the newly added const).

Signed-off-by: Johannes Berg 
---
 drivers/net/wireless/mac80211_hwsim.c | 79 ++-
 1 file changed, 40 insertions(+), 39 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c 
b/drivers/net/wireless/mac80211_hwsim.c
index 431f13b4faf6..f9ba0772e471 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -2256,35 +2256,51 @@ static void mac80211_hwsim_get_et_stats(struct 
ieee80211_hw *hw,
WARN_ON(i != MAC80211_HWSIM_SSTATS_LEN);
 }
 
+#define HWSIM_COMMON_OPS   \
+   .tx = mac80211_hwsim_tx,\
+   .start = mac80211_hwsim_start,  \
+   .stop = mac80211_hwsim_stop,\
+   .add_interface = mac80211_hwsim_add_interface,  \
+   .change_interface = mac80211_hwsim_change_interface,\
+   .remove_interface = mac80211_hwsim_remove_interface,\
+   .config = mac80211_hwsim_config,\
+   .configure_filter = mac80211_hwsim_configure_filter,\
+   .bss_info_changed = mac80211_hwsim_bss_info_changed,\
+   .sta_add = mac80211_hwsim_sta_add,  \
+   .sta_remove = mac80211_hwsim_sta_remove,\
+   .sta_notify = mac80211_hwsim_sta_notify,\
+   .set_tim = mac80211_hwsim_set_tim,  \
+   .conf_tx = mac80211_hwsim_conf_tx,  \
+   .get_survey = mac80211_hwsim_get_survey,\
+   CFG80211_TESTMODE_CMD(mac80211_hwsim_testmode_cmd)  \
+   .ampdu_action = mac80211_hwsim_ampdu_action,\
+   .flush = mac80211_hwsim_flush,  \
+   .get_tsf = mac80211_hwsim_get_tsf,  \
+   .set_tsf = mac80211_hwsim_set_tsf,  \
+   .get_et_sset_count = mac80211_hwsim_get_et_sset_count,  \
+   .get_et_stats = mac80211_hwsim_get_et_stats,\
+   .get_et_strings = mac80211_hwsim_get_et_strings,
+
 static const struct ieee80211_ops mac80211_hwsim_ops = {
-   .tx = mac80211_hwsim_tx,
-   .start = mac80211_hwsim_start,
-   .stop = mac80211_hwsim_stop,
-   .add_interface = mac80211_hwsim_add_interface,
-   .change_interface = mac80211_hwsim_change_interface,
-   .remove_interface = mac80211_hwsim_remove_interface,
-   .config = mac80211_hwsim_config,
-   .configure_filter = mac80211_hwsim_configure_filter,
-   .bss_info_changed = mac80211_hwsim_bss_info_changed,
-   .sta_add = mac80211_hwsim_sta_add,
-   .sta_remove = mac80211_hwsim_sta_remove,
-   .sta_notify = mac80211_hwsim_sta_notify,
-   .set_tim = mac80211_hwsim_set_tim,
-   .conf_tx = mac80211_hwsim_conf_tx,
-   .get_survey = mac80211_hwsim_get_survey,
-   CFG80211_TESTMODE_CMD(mac80211_hwsim_testmode_cmd)
-   .ampdu_action = mac80211_hwsim_ampdu_action,
+   HWSIM_COMMON_OPS
.sw_scan_start = mac80211_hwsim_sw_scan,
.sw_scan_complete = mac80211_hwsim_sw_scan_complete,
-   .flush = mac80211_hwsim_flush,
-   .get_tsf = mac80211_hwsim_get_tsf,
-   .set_tsf = mac80211_hwsim_set_tsf,
-   .get_et_sset_count = mac80211_hwsim_get_et_sset_count,
-   .get_et_stats = mac80211_hwsim_get_et_stats,
-   .get_et_strings = mac80211_hwsim_get_et_strings,
 };
 
-static struct ieee80211_ops mac80211_hwsim_mchan_ops;
+static const struct ieee80211_ops mac80211_hwsim_mchan_ops = {
+   HWSIM_COMMON_OPS
+   .hw_scan = mac80211_hwsim_hw_scan,
+   .cancel_hw_scan = mac80211_hwsim_cancel_hw_scan,
+   .sw_scan_start = NULL,
+   .sw_scan_complete = NULL,
+   .remain_on_channel = mac80211_hwsim_roc,
+   .cancel_remain_on_channel = mac80211_hwsim_croc,
+   .add_chanctx = mac80211_hwsim_add_chanctx,
+   .remove_chanctx = mac80211_hwsim_remove_chanctx,
+   .change_chanctx = mac80211_hwsim_change_chanctx,
+   .assign_vif_chanctx = mac80211_hwsim_assign_vif_chanctx,
+   .unassign_vif_chanctx = mac80211_hwsim_unassign_vif_chanctx,
+};
 
 struct hwsim_new_radio_params {
unsigned int channels;
@@ -3360,21 +3376,6 @@ static int __init init_mac80211_hwsim(void)
if (channels < 1)
return -EINVAL;
 
-   mac80211_hwsim_mchan_ops = mac80211_hwsim_ops;
-   mac80211_hwsim_mchan_ops.hw_scan = mac80211_hwsim_hw_scan;
-   mac80211_hwsim_mchan_ops.cancel_hw_scan = mac80211_hwsim_cancel_hw_scan;
-   mac80211_hwsim_mchan_ops.sw_scan_start = 

Re: [PATCH] Report scan_abort as part of iw phy

[Johannes Berg]

On Sat, 2016-10-08 at 15:40 +0200, Ola Olsson wrote:
> nl80211: abort_scan has not been reported under the 'Supported
> commands'
> category when calling iw phy. Add that.
> 
> Signed-off-by: Ola Olsson 
> ---
>  net/wireless/nl80211.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 95d55d2..0bbce4f 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -1526,6 +1526,7 @@ static int nl80211_send_wiphy(struct
> cfg80211_registered_device *rdev,
>   CMD(set_bitrate_mask, SET_TX_BITRATE_MASK);
>   CMD(mgmt_tx, FRAME);
>   CMD(mgmt_tx_cancel_wait, FRAME_WAIT_CANCEL);
> + CMD(abort_scan, ABORT_SCAN);
> 

You can't add this here, it has to be inside the "state->split" if.

johannes

Re: [PATCH] Report scan_abort as part of iw phy

[Kalle Valo]

Ola Olsson  writes:

> nl80211: abort_scan has not been reported under the 'Supported commands'
> category when calling iw phy. Add that.
>
> Signed-off-by: Ola Olsson 
> ---
>  net/wireless/nl80211.c | 1 +
>  1 file changed, 1 insertion(+)

You should prefix the title with "cfg80211: " (or "nl80211: ", not
really sure which one is more suitable).

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#subject_name

-- 
Kalle Valo