Re: [PATCH] brcmfmac: stop watchdog before detach and free everything

[Arend van Spriel]

On 5/28/2018 9:50 AM, Michael Trimarchi wrote:

Watchdog need to be stopped in brcmf_sdio_remove to avoid
i
The system is going down NOW!
[ 1348.110759] Unable to handle kernel NULL pointer dereference at virtual 
address 02f8
Sent SIGTERM to all processes
[ 1348.121412] Mem abort info:
[ 1348.126962]   ESR = 0x9604
[ 1348.130023]   Exception class = DABT (current EL), IL = 32 bits
[ 1348.135948]   SET = 0, FnV = 0
[ 1348.138997]   EA = 0, S1PTW = 0
[ 1348.142154] Data abort info:
[ 1348.145045]   ISV = 0, ISS = 0x0004
[ 1348.148884]   CM = 0, WnR = 0
[ 1348.151861] user pgtable: 4k pages, 48-bit VAs, pgdp = (ptrval)
[ 1348.158475] [02f8] pgd=
[ 1348.163364] Internal error: Oops: 9604 [#1] PREEMPT SMP
[ 1348.168927] Modules linked in: ipv6
[ 1348.172421] CPU: 3 PID: 1421 Comm: brcmf_wdog/mmc0 Not tainted 
4.17.0-rc5-next-20180517 #18
[ 1348.180757] Hardware name: Amarula A64-Relic (DT)
[ 1348.185455] pstate: 6005 (nZCv daif -PAN -UAO)
[ 1348.190251] pc : brcmf_sdiod_freezer_count+0x0/0x20
[ 1348.195124] lr : brcmf_sdio_watchdog_thread+0x64/0x290


Hi Michael,

Thanks for the patch. In normal scenario the callstack looks like this:

brcmf_sdio_remove()
-> brcmf_detach()
-> brcmf_bus_stop()
-> brcmf_sdio_bus_stop()

In brcmf_sdio_bus_stop() the watchdog is terminated. So in what scenario 
did you encounter this null pointer deref?


Regards,
Arend

Re: [PATCH net-next v2 2/2] dt: bindings: add new dt entries for brcmfmac

[Arend van Spriel]

On 4/5/2018 3:10 PM, Kalle Valo wrote:

Ulf Hansson <ulf.hans...@linaro.org> writes:


On 20 March 2018 at 10:55, Kalle Valo <kv...@codeaurora.org> wrote:

Arend van Spriel <arend.vanspr...@broadcom.com> writes:


If I get it right, you mean something like this:

mmc3: mmc@1c12000 {
...
  broken-sg-support;
  sd-head-align = 4;
  sd-sgentry-align = 512;

  brcmf: wifi@1 {
  ...
  };
};

Where dt: bindings documentation for these entries should reside?
In generic MMC bindings? Well, this is the very special case and
mmc-linux maintainer will unlikely to accept these changes.
Also, extra kernel code modification might be required. It could make
quite trivial change much more complex.


If the MMC maintainers are not copied on this patch series, it will
likely be hard for them to identify this patch series and chime in...


The main question is whether this is indeed a "very special case" as
Alexey claims it to be or that it is likely to be applicable to other
device and host combinations as you are suggesting.

If these properties are imposed by the host or host controller it
would make sense to have these in the mmc bindings.


BTW, last year we were discussing something similar (I mean related to
alignment requirements) with ath10k SDIO patches and at the time the
patch submitter was proposing to have a bounce buffer in ath10k to
workaround that. I don't remember the details anymore, they are on the
ath10k mailing list archive if anyone is curious to know, but I would
not be surprised if they are similar as here. So there might be a need
to solve this in a generic way (but not sure of course as I haven't
checked the details).


I re-call something about these as well, here are the patches. Perhaps
I should pick some of them up...

https://patchwork.kernel.org/patch/10123137/
https://patchwork.kernel.org/patch/10123139/
https://patchwork.kernel.org/patch/10123141/
https://patchwork.kernel.org/patch/10123143/


Actually I was talking about a different patch, found it now:

ath10k_sdio: DMA bounce buffers for read write

https://patchwork.kernel.org/patch/9979543/


The patches Uffe mentions are related to this. In brcmfmac we simply 
implemented functionality to create a scatter list and send it through 
the MMC stack using SDIO CMD53. It is in the creation of the scatter 
list that these alignment properties are needed. Moving the whole 
functionality to the MMC stack will also move those properties into 
their right context, ie. SDIO host controller.


Our broker_sg_support property is basically doing the bounce buffer 
thing if I understand it correctly.


Regards,
Arend

Re: [PATCH 07/12] brcmfmac: Convert ALLFFMAC to ether_broadcast_addr

[Arend van Spriel]

On 3/31/2018 9:05 AM, Joe Perches wrote:

Remove the local ALLFFMAC extern array and use the new global instead.


I stumbled upon this one couple of weeks ago. I moved the definition to 
flowring.c although I considered for a moment to pick up the task you 
took here valiantly.



Miscellanea:

o Convert char *mac to const char *mac as it can't be modified


The real reason is off course that your new global is const and thus mac 
variable need to be const as well to avoid compiler warning.


I have to agree with Kalle regarding the upstream logistics, but for 
what it is worth...


Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>

Signed-off-by: Joe Perches <j...@perches.com>
---
  drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c   | 2 --
  drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h   | 2 --
  drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c | 8 
  3 files changed, 4 insertions(+), 8 deletions(-)

Re: Passing uninitialised local variable

[Arend van Spriel]

On 3/28/2018 1:20 PM, Himanshu Jha wrote:

Hello everyone,


You added everyone, but me :-(

Not really a problem, but it would help if the driver name was mentioned 
in the subject.



I recently found that a local variable in passed uninitialised to the
function at

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:2950

 u32 var;
 err = brcmf_fil_iovar_int_get(ifp, "dtim_assoc", );
 if (err) {
 brcmf_err("wl dtim_assoc failed (%d)\n", err);
 goto update_bss_info_out;
 }
 dtim_period = (u8)var;


Now, the brcmf_fil_iovar_int_get() is defined as:

s32
brcmf_fil_iovar_int_get(struct brcmf_if *ifp, char *name, u32 *data)
{
 __le32 data_le = cpu_to_le32(*data);
 s32 err;

 err = brcmf_fil_iovar_data_get(ifp, name, _le, sizeof(data_le));
 if (err == 0)
 *data = le32_to_cpu(data_le);
 return err;
}

We can cleary see that 'var' in used uninitialised in the very first line
which is an undefined behavior.


Why undefined? We copy some stack data and we do transfer that to the 
device. However in this case the device does nothing with it and it is 
simply overwritten by the response.



So, what could be a possible fix for the above ?

I'm not sure initialising 'var' to 0 would be the correct solution.


Coverity flagged this and probably still does. For this particular 
instance setting var to '0' is fine. However, there are quite a few 
other places. For some instances the data contains a selector value for 
obtaining info from the device, which is what we copy in 
brcmf_fil_iovar_int_get(). So maybe it would be best to have a separate 
function for those, eg. brcmf_fil_iovar_int_selget() or so.


Regards,
Arend

Re: [PATCH net-next v2 2/2] dt: bindings: add new dt entries for brcmfmac

[Arend van Spriel]

On 3/20/2018 8:58 AM, Alexey Roslyakov wrote:

Arend,


Also I am not sure if the broken-sg-support is still needed. We added that for 
omap_hsmmc, but that has since changed to scatter-gather emulation so it might 
not be needed anymore.

I can confirm it doesn't impact wifi performance in case of rk3288+ap6335.

But I still have to set settings->bus.sdio.sd_head_align = 4 and
settings->bus.sdio.sd_sgentry_align = 512, otherwise
brcmf_sdiod_sglist_rw fails:

   974.888452] net_ratelimit: 8 callbacks suppressed
[  974.888457] brcmfmac: brcmf_sdiod_sglist_rw: CMD53 sg block read failed -84
[  974.901527] brcmfmac: brcmf_sdio_rxglom: glom read of 6144 bytes failed: -5
[  974.910248] brcmfmac: brcmf_sdio_rxfail: abort command, terminate frame
[  975.018041] brcmfmac: brcmf_sdiod_sglist_rw: CMD53 sg block read failed -84
[  975.025833] brcmfmac: brcmf_sdio_rxglom: glom read of 6144 bytes failed: -5
[  975.033634] brcmfmac: brcmf_sdio_rxfail: abort command, terminate frame
[  975.033924] dwmmc_rockchip ff0d.dwmmc: Unexpected command
timeout, state 0
[  975.049209] brcmfmac: brcmf_sdio_readframes: RXHEADER FAILED: -84
[  975.056034] brcmfmac: brcmf_sdio_rxfail: abort command, terminate
frame, send NAK
[  975.068367] brcmfmac: brcmf_sdio_hdparse: HW header length too long
[  975.075379] brcmfmac: brcmf_sdio_rxfail: terminate frame


Hi Alex,

Thanks for checking. In your case I think you do not need sd_head_align 
as it will default to either 4 or 8:


#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
#define ALIGNMENT  8
#else
#define ALIGNMENT  4
#endif

I am not saying you should not be needing this. When it comes to DT 
people are often tempted to accommodate a driver solution especially 
when such a solution is already in place. However, DT is a hardware 
description and these do not describe the wifi device. They are 
applicable to the wifi device because it is a limitation of the host 
controller and as such should be described in the DT binding of the host 
controller.


Regards,
Arend


Regards,
   Alex

On 20 March 2018 at 06:16, Arend van Spriel
<arend.vanspr...@broadcom.com> wrote:

+ Uffe

On 3/19/2018 6:55 PM, Florian Fainelli wrote:


On 03/19/2018 07:10 AM, Alexey Roslyakov wrote:


Hi Arend,
I appreciate your response. In my opinion, it has nothing to do with
SDIO host, because it defines "quirks" in the driver itself.



It is not clear to me from your patch series whether the problem is that:

- the SDIO device has a specific alignment requirements, which would be
either a SDIO device driver limitation/issue or maybe the underlying
hardware device/firmware requiring that

- the SDIO host controller used is not capable of coping nicely with
these said limitations

It seems to me like what you are doing here is a) applicable to possibly
more SDIO devices and host combinations, and b) should likely be done at
the layer between the host and device, such that it is available to more
combinations.



Indeed. That was my thought exactly and I can not imagine Uffe would push
back on that reasoning.


If I get it right, you mean something like this:

mmc3: mmc@1c12000 {
...
  broken-sg-support;
  sd-head-align = 4;
  sd-sgentry-align = 512;

  brcmf: wifi@1 {
  ...
  };
};

Where dt: bindings documentation for these entries should reside?
In generic MMC bindings? Well, this is the very special case and
mmc-linux maintainer will unlikely to accept these changes.
Also, extra kernel code modification might be required. It could make
quite trivial change much more complex.



If the MMC maintainers are not copied on this patch series, it will
likely be hard for them to identify this patch series and chime in...



The main question is whether this is indeed a "very special case" as Alexey
claims it to be or that it is likely to be applicable to other device and
host combinations as you are suggesting.

If these properties are imposed by the host or host controller it would make
sense to have these in the mmc bindings.




Also I am not sure if the broken-sg-support is still needed. We added
that for omap_hsmmc, but that has since changed to scatter-gather emulation
so it might not be needed anymore.



I've experienced the problem with rk3288 (dw-mmc host) and sdio
settings like above solved it.
Frankly, I haven't investigated any deeper which one of the settings
helped in my case yet...
I will try to get rid of broken-sg-support first and let you know if
it does make any difference.



Are you using some chromebook. I have some lying around here so I could also
look into it. What broadcom chipset do you have?

Regards,
Arend



All the best,
Alex.

On 19 March 2018 at 16:31, Arend van Spriel
<arend.vanspr...@broadcom.com> wrote:


On 3/19/2018 2:40 AM, Alexey Roslyakov wrote:



In case if the host has higher align requirements for SG items, allow
setting device-specific aligns for scatterlist items.

Signed-o

Re: [PATCH net-next v2 2/2] dt: bindings: add new dt entries for brcmfmac

[Arend van Spriel]

+ Uffe

On 3/19/2018 6:55 PM, Florian Fainelli wrote:

On 03/19/2018 07:10 AM, Alexey Roslyakov wrote:

Hi Arend,
I appreciate your response. In my opinion, it has nothing to do with
SDIO host, because it defines "quirks" in the driver itself.


It is not clear to me from your patch series whether the problem is that:

- the SDIO device has a specific alignment requirements, which would be
either a SDIO device driver limitation/issue or maybe the underlying
hardware device/firmware requiring that

- the SDIO host controller used is not capable of coping nicely with
these said limitations

It seems to me like what you are doing here is a) applicable to possibly
more SDIO devices and host combinations, and b) should likely be done at
the layer between the host and device, such that it is available to more
combinations.


Indeed. That was my thought exactly and I can not imagine Uffe would 
push back on that reasoning.



If I get it right, you mean something like this:

mmc3: mmc@1c12000 {
...
 broken-sg-support;
 sd-head-align = 4;
 sd-sgentry-align = 512;

 brcmf: wifi@1 {
 ...
 };
};

Where dt: bindings documentation for these entries should reside?
In generic MMC bindings? Well, this is the very special case and
mmc-linux maintainer will unlikely to accept these changes.
Also, extra kernel code modification might be required. It could make
quite trivial change much more complex.


If the MMC maintainers are not copied on this patch series, it will
likely be hard for them to identify this patch series and chime in...


The main question is whether this is indeed a "very special case" as 
Alexey claims it to be or that it is likely to be applicable to other 
device and host combinations as you are suggesting.


If these properties are imposed by the host or host controller it would 
make sense to have these in the mmc bindings.





Also I am not sure if the broken-sg-support is still needed. We added that for 
omap_hsmmc, but that has since changed to scatter-gather emulation so it might 
not be needed anymore.


I've experienced the problem with rk3288 (dw-mmc host) and sdio
settings like above solved it.
Frankly, I haven't investigated any deeper which one of the settings
helped in my case yet...
I will try to get rid of broken-sg-support first and let you know if
it does make any difference.


Are you using some chromebook. I have some lying around here so I could 
also look into it. What broadcom chipset do you have?


Regards,
Arend


All the best,
   Alex.

On 19 March 2018 at 16:31, Arend van Spriel
<arend.vanspr...@broadcom.com> wrote:

On 3/19/2018 2:40 AM, Alexey Roslyakov wrote:


In case if the host has higher align requirements for SG items, allow
setting device-specific aligns for scatterlist items.

Signed-off-by: Alexey Roslyakov <alexey.roslya...@gmail.com>
---
   Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt | 5
+
   1 file changed, 5 insertions(+)

diff --git
a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
index 86602f264dce..187b8c1b52a7 100644
--- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
+++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
@@ -17,6 +17,11 @@ Optional properties:
 When not specified the device will use in-band SDIO interrupts.
- interrupt-names : name of the out-of-band interrupt, which must be
set
 to "host-wake".
+ - brcm,broken-sg-support : boolean flag to indicate that the SDIO host
+   controller has higher align requirement than 32 bytes for each
+   scatterlist item.
+ - brcm,sd-head-align : alignment requirement for start of data buffer.
+ - brcm,sd-sgentry-align : length alignment requirement for each sg
entry.



Hi Alexey,

Thanks for the patch. However, the problem with these is that they are
characterizing the host controller and not the wireless device. So from
device tree perspective , which is to describe the hardware, these
properties should be SDIO host controller properties. Also I am not sure if
the broken-sg-support is still needed. We added that for omap_hsmmc, but
that has since changed to scatter-gather emulation so it might not be needed
anymore.

Regards,
Arend

Re: [PATCH net-next v2 2/2] dt: bindings: add new dt entries for brcmfmac

[Arend van Spriel]

On 3/19/2018 2:40 AM, Alexey Roslyakov wrote:

In case if the host has higher align requirements for SG items, allow
setting device-specific aligns for scatterlist items.

Signed-off-by: Alexey Roslyakov 
---
  Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt | 5 +
  1 file changed, 5 insertions(+)

diff --git 
a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt 
b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
index 86602f264dce..187b8c1b52a7 100644
--- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
+++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
@@ -17,6 +17,11 @@ Optional properties:
When not specified the device will use in-band SDIO interrupts.
   - interrupt-names : name of the out-of-band interrupt, which must be set
to "host-wake".
+ - brcm,broken-sg-support : boolean flag to indicate that the SDIO host
+   controller has higher align requirement than 32 bytes for each
+   scatterlist item.
+ - brcm,sd-head-align : alignment requirement for start of data buffer.
+ - brcm,sd-sgentry-align : length alignment requirement for each sg entry.


Hi Alexey,

Thanks for the patch. However, the problem with these is that they are 
characterizing the host controller and not the wireless device. So from 
device tree perspective , which is to describe the hardware, these 
properties should be SDIO host controller properties. Also I am not sure 
if the broken-sg-support is still needed. We added that for omap_hsmmc, 
but that has since changed to scatter-gather emulation so it might not 
be needed anymore.


Regards,
Arend

Re: [PATCH V2] brcmfmac: drop Inter-Access Point Protocol packets by default

[Arend van Spriel]

On 3/15/2018 8:29 AM, Rafał Miłecki wrote:

From: Rafał Miłecki <ra...@milecki.pl>

Testing brcmfmac with more recent firmwares resulted in AP interfaces
not working in some specific setups. Debugging resulted in discovering
support for IAPP in Broadcom's firmwares.

Older firmwares were only generating 802.11f frames. Newer ones like:
1) 10.10 (TOB) (r663589)
2) 10.10.122.20 (r683106)
for 4366b1 and 4366c0 respectively seem to also /respect/ 802.11f frames
in the Tx path by performing a STA disassociation.

This obsoleted standard and its implementation is something that:
1) Most people don't need / want to use
2) Can allow local DoS attacks
3) Breaks AP interfaces in some specific bridge setups

To solve issues it can cause this commit modifies brcmfmac to drop IAPP
packets. If affects:
1) Rx path: driver won't be sending these unwanted packets up.
2) Tx path: driver will reject packets that would trigger STA
disassociation perfromed by a firmware (possible local DoS attack).

It appears there are some Broadcom's clients/users who care about this
feature despite the drawbacks. They can switch it on using a new module
param.

This change results in only two more comparisons (check for module param
and check for Ethernet packet length) for 99.9% of packets. Its overhead
should be very minimal.


Hi Rafał,

Thanks for this patch.

Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
  .../wireless/broadcom/brcm80211/brcmfmac/common.c  |  5 ++
  .../wireless/broadcom/brcm80211/brcmfmac/common.h  |  1 +
  .../wireless/broadcom/brcm80211/brcmfmac/core.c| 57 ++
  3 files changed, 63 insertions(+)

Re: [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default

[Arend van Spriel]

On 3/14/2018 5:10 PM, Kalle Valo wrote:

Rafał Miłecki  writes:


+   unsigned char *eth_data = skb_mac_header(skb) + ETH_HLEN;
+#if !defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)


#ifndef?


I followed what is used in the include/linux/etherdevice.h. Is that a
good exceuse? Could it be there any some good reason for #if defined()?


Don't know, maybe just a matter of taste? But it would be nice to know
the background behind #ifdef vs #if defined(), never figured it out why
two different forms.


Well. In this case you could use either one, but if you have more 
conditions #if defined() is bit more efficient:


#ifdef A
#ifdef B
#endif
#endif

vs.

#if defined(A) && defined(B)

Regards,
Arend

Re: [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default

[Arend van Spriel]

On 3/14/2018 4:57 PM, Rafał Miłecki wrote:

On 2018-03-14 16:39, Rafał Miłecki wrote:

On 2018-03-14 13:58, Arend van Spriel wrote:

On 3/14/2018 12:01 PM, Rafał Miłecki wrote:

From: Rafał Miłecki <ra...@milecki.pl>

Testing brcmfmac with more recent firmwares resulted in AP interfaces
not working in some specific setups. Debugging resulted in discovering
support for IAPP in Broadcom's firmwares. This is an obsoleted standard
and its implementation is something that:
1) Most people don't need / want to use
2) Can allow local DoS attacks
3) Breaks AP interfaces in some specific bridge setups

To solve issues it can cause this commit modifies brcmfmac to drop IAPP
packets. If affects:
1) Rx path: driver won't be sending these unwanted packets up.
2) Tx path: driver will reject packets that would trigger STA
disassociation perfromed by a firmware (possible local DoS attack).

It appears there are some Broadcom's clients/users who care about this
feature despite the drawbacks. They can switch it on by a newly added
Kconfig option.


Thanks for taking this approach. Looks fine except for  (see below)

Reviewed-by: Arend van Spriel <arend.vanspr...@broadcom.com>

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
  drivers/net/wireless/broadcom/brcm80211/Kconfig| 20 +++
  .../wireless/broadcom/brcm80211/brcmfmac/core.c| 39
++
  2 files changed, 59 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/Kconfig
b/drivers/net/wireless/broadcom/brcm80211/Kconfig
index 9d99eb42d917..876787ef991a 100644
--- a/drivers/net/wireless/broadcom/brcm80211/Kconfig
+++ b/drivers/net/wireless/broadcom/brcm80211/Kconfig
@@ -68,6 +68,26 @@ config BRCMFMAC_PCIE
IEEE802.11ac embedded FullMAC WLAN driver. Say Y if you want to
use the driver for an PCIE wireless card.

+config BRCMFMAC_IAPP
+bool "Partial support for obsoleted Inter-Access Point Protocol"
+depends on BRCMFMAC
+---help---
+  Most of Broadcom's firmwares can send 802.11f ADD frame every
+  time new STA connects to the AP interface. Some recent ones
+  can also disassociate STA when they receive such a frame.


I do not see any evidence that this would occur only for recent
firmware. That stuff is old and not touched recently.


My evidence is comparing firmwares for 4366b1: 10.10.69.3309 (r610991)
vs. 10.10 (TOB) (r663589).

The first one is from linux-firmware.git and it doesn't implement IAPP
in the TX path. The later one is what I got from you privately and it
implements it.

Also a firmware for 4366c0: 10.10.122.20 (r683106) which is relatively
new implements IAPP in the TX path.



diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 19048526b4af..db6987015fb1 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c


[...]


  static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
 struct net_device *ndev)
  {
@@ -250,6 +278,12 @@ static netdev_tx_t
brcmf_netdev_start_xmit(struct sk_buff *skb,
  goto done;
  }

+if (!IS_ENABLED(CONFIG_BRCMFMAC_IAPP) && brcmf_skb_is_iapp(skb)) {
+dev_kfree_skb(skb);
+ret = -EINVAL;
+goto done;
+}


This is not right. The function must return netdev_tx_t type. Here is
kerneldoc of .start_xmit():

 * netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb,
 *   struct net_device *dev);
 *Called when a packet needs to be transmitted.
 *Returns NETDEV_TX_OK.  Can return NETDEV_TX_BUSY, but you
should stop
 *the queue before that can happen; it's for obsolete devices and
weird
 *corner cases, but the stack really does a non-trivial amount
 *of useless work if you return NETDEV_TX_BUSY.
 *Required; cannot be NULL.


Please take a closer look at how brcmf_netdev_start_xmit() works. Above
 code *will* return netdev_tx_t.



You may want to increase dropped netstat or add driver internal
statistic counter so there is visibility of IAPP packets being
dropped.


OK, I'll try to find a stat to increase.


So after checking brcmf_netdev_start_xmit() again, I realized I actually
*do* that. Doing:
ret = -EINVAL;
goto done;
results in increasing tx_dropped.


Okay, okay. Admittedly I only looked at the patch. Feel free to remove 
the Reviewed-by.


Regards,
Arend

Re: [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default

[Arend van Spriel]

On 3/14/2018 3:24 PM, Kalle Valo wrote:

+config BRCMFMAC_IAPP
>+   bool "Partial support for obsoleted Inter-Access Point Protocol"
>+   depends on BRCMFMAC
>+   ---help---
>+ Most of Broadcom's firmwares can send 802.11f ADD frame every
>+ time new STA connects to the AP interface. Some recent ones
>+ can also disassociate STA when they receive such a frame.
>+
>+ It's important to understand this behavior can lead to a local
>+ DoS security issue. Attacker may trigger disassociation of any
>+ STA by sending a proper Ethernet frame to the wireless
>+ interface.
>+
>+ Moreover this feature may break AP interfaces in some specific
>+ setups. This applies e.g. to the bridge with hairpin mode
>+ enabled and IFLA_BRPORT_MCAST_TO_UCAST set. IAPP packet
>+ generated by a firmware will get passed back to the wireless
>+ interface and cause immediate disassociation of just-connected
>+ STA.

Sorry for jumping late, but does it really make sense to have a Kconfig
option for this? I don't think we should add a Kconfig option for every
strange feature, there should be stronger reasons (size savings etc)
before adding a Kconfig option.

And in this case the size savings can't be much. Wouldn't a module
parameter be simpler for a functionality change like this?


Hi Kalle,

Good to be wary about Kconfig option. So my reason for asking a Kconfig 
option is that this is directly in the datapaths (tx and rx) so I prefer 
to disable/enable it compile time rather then runtime.


Regards,
Arend

Re: [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default

[Arend van Spriel]

On 3/14/2018 12:01 PM, Rafał Miłecki wrote:

From: Rafał Miłecki <ra...@milecki.pl>

Testing brcmfmac with more recent firmwares resulted in AP interfaces
not working in some specific setups. Debugging resulted in discovering
support for IAPP in Broadcom's firmwares. This is an obsoleted standard
and its implementation is something that:
1) Most people don't need / want to use
2) Can allow local DoS attacks
3) Breaks AP interfaces in some specific bridge setups

To solve issues it can cause this commit modifies brcmfmac to drop IAPP
packets. If affects:
1) Rx path: driver won't be sending these unwanted packets up.
2) Tx path: driver will reject packets that would trigger STA
disassociation perfromed by a firmware (possible local DoS attack).

It appears there are some Broadcom's clients/users who care about this
feature despite the drawbacks. They can switch it on by a newly added
Kconfig option.


Thanks for taking this approach. Looks fine except for  (see below)

Reviewed-by: Arend van Spriel <arend.vanspr...@broadcom.com>

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
  drivers/net/wireless/broadcom/brcm80211/Kconfig| 20 +++
  .../wireless/broadcom/brcm80211/brcmfmac/core.c| 39 ++
  2 files changed, 59 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/Kconfig 
b/drivers/net/wireless/broadcom/brcm80211/Kconfig
index 9d99eb42d917..876787ef991a 100644
--- a/drivers/net/wireless/broadcom/brcm80211/Kconfig
+++ b/drivers/net/wireless/broadcom/brcm80211/Kconfig
@@ -68,6 +68,26 @@ config BRCMFMAC_PCIE
  IEEE802.11ac embedded FullMAC WLAN driver. Say Y if you want to
  use the driver for an PCIE wireless card.

+config BRCMFMAC_IAPP
+   bool "Partial support for obsoleted Inter-Access Point Protocol"
+   depends on BRCMFMAC
+   ---help---
+ Most of Broadcom's firmwares can send 802.11f ADD frame every
+ time new STA connects to the AP interface. Some recent ones
+ can also disassociate STA when they receive such a frame.


I do not see any evidence that this would occur only for recent 
firmware. That stuff is old and not touched recently.



diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 19048526b4af..db6987015fb1 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c


[...]


  static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
   struct net_device *ndev)
  {
@@ -250,6 +278,12 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff 
*skb,
goto done;
}

+   if (!IS_ENABLED(CONFIG_BRCMFMAC_IAPP) && brcmf_skb_is_iapp(skb)) {
+   dev_kfree_skb(skb);
+   ret = -EINVAL;
+   goto done;
+   }


This is not right. The function must return netdev_tx_t type. Here is 
kerneldoc of .start_xmit():


 * netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb,
 *   struct net_device *dev);
 *  Called when a packet needs to be transmitted.
 *  Returns NETDEV_TX_OK.  Can return NETDEV_TX_BUSY, but you should stop
 *  the queue before that can happen; it's for obsolete devices and weird
 *  corner cases, but the stack really does a non-trivial amount
 *  of useless work if you return NETDEV_TX_BUSY.
 *  Required; cannot be NULL.

You may want to increase dropped netstat or add driver internal 
statistic counter so there is visibility of IAPP packets being dropped.


Regards,
Arend

Re: Problem with bridge (mcast-to-ucast + hairpin) and Broadcom's 802.11f in their FullMAC fw

[Arend van Spriel]

On 3/13/2018 8:20 AM, Felix Fietkau wrote:

[resent with fixed typo in linux-wireless address]

On 2018-02-27 11:08, Rafał Miłecki wrote:

I've problem when using OpenWrt/LEDE on a home router with Broadcom's
FullMAC WiFi chipset.


First of all OpenWrt/LEDE uses bridge interface for LAN network with:
1) IFLA_BRPORT_MCAST_TO_UCAST
2) Clients isolation in hostapd
3) Hairpin mode enabled

For more details please see Linus's patch description:
https://patchwork.kernel.org/patch/9530669/
and maybe hairpin mode patch:
https://lwn.net/Articles/347344/

Short version: in that setup packets received from a bridged wireless
interface can be handled back to it for transmission.


Now, Broadcom's firmware for their FullMAC chipsets in AP mode
supports an obsoleted 802.11f AKA IAPP standard. It's a roaming
standard that was replaced by 802.11r.

Whenever a new station associates, firmware generates a packet like:
ff ff ff ff  ff ff ec 10  7b 5f ?? ??  00 06 00 01  af 81 01 00
(just masked 2 bytes of my MAC)

For mode details you can see discussion in my brcmfmac patch thread:
https://patchwork.kernel.org/patch/10191451/


The problem is that bridge (in setup as above) handles such a packet
back to the device.

That makes Broadcom's FullMAC firmware believe that a given station
just connected to another AP in a network (which doesn't even exist).
As a result firmware immediately disassociates that station. It's
simply impossible to connect to the router. Every association is
followed by immediate disassociation.


Can you see any solution for this problem? Is that an option to stop
multicast-to-unicast from touching 802.11f packets? Some other ideas?
Obviously I can't modify Broadcom's firmware and drop that obsoleted
standard.

Let's look at it from a different angle: Since these packets are
forwarded as normal packets by the bridge, and the Broadcom firmware
reacts to them in this nasty way, that's basically local DoS security
issue. In my opinion that matters a lot more than having support for an
obsolete feature that almost nobody will ever want to use.

I think the right approach to deal with this issue is to drop these
garbage packets in both the receive and transmit path of brcmfmac.


My approach was to get rid of it in firmware as this never made it into 
the 802.11 spec. So I asked internally whether it was still used. Turns 
out that we still rely on it for some customers. I am fine with dropping 
these "garbage" packets, but given that there is still use for it I 
would like to see that under a Kconfig flag. Dropping it may be the default.


Regards,
Arend

Re: [PATCH 3/3] wlcore: Use common error handling code in wl1271_acx_sta_rate_policies()

[Arend van Spriel]

On 3/10/2018 10:33 PM, SF Markus Elfring wrote:

From: Markus Elfring 
Date: Sat, 10 Mar 2018 22:18:45 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.


You call this an issue?


Signed-off-by: Markus Elfring 
---
  drivers/net/wireless/ti/wlcore/acx.c | 24 +++-
  1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/acx.c 
b/drivers/net/wireless/ti/wlcore/acx.c
index 1cc5bba670e1..7d37a417c756 100644
--- a/drivers/net/wireless/ti/wlcore/acx.c
+++ b/drivers/net/wireless/ti/wlcore/acx.c


[...]


ret = wl1271_cmd_configure(wl, ACX_RATE_POLICY, acx, sizeof(*acx));
-   if (ret < 0) {
-   wl1271_warning("Setting of rate policies failed: %d", ret);
-   goto out;
-   }
+   if (ret < 0)
+   goto report_failure;

-out:
+free_acx:
kfree(acx);
return ret;
+
+report_failure:
+   wl1271_warning("Setting of rate policies failed: %d", ret);
+   goto free_acx;


In my opinion you are introducing a new issue. I don't call this 
"common" in any way. It is leaning more towards "spaghetti code" [1]. 
Jumping over a label to return to it with another jump. They are not 
long jumps, but it sure does not make thing more readable. Always aim 
for simple top-to-bottom.


Regards,
Arend

[1] https://en.wikipedia.org/wiki/Spaghetti_code


  }

  int wl1271_acx_ap_rate_policy(struct wl1271 *wl, struct conf_tx_rate_class *c,

Re: [PATCH] drivers: net: wireless: ath: ath9: dfs: remove VLA usage

[Arend van Spriel]

On 3/9/2018 1:30 PM, Andreas Christoforou wrote:

The kernel would like to have all stack VLA usage removed.


I think there was a remark made earlier to give more explanation here. 
It should explain why we want "VLA on stack" removed.



Signed-off-by: Andreas Christoforou 
---
  drivers/net/wireless/ath/ath9k/dfs.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/dfs.c 
b/drivers/net/wireless/ath/ath9k/dfs.c
index 6fee9a4..cfb0f84 100644
--- a/drivers/net/wireless/ath/ath9k/dfs.c
+++ b/drivers/net/wireless/ath/ath9k/dfs.c
@@ -41,7 +41,6 @@ static const int BIN_DELTA_MAX= 10;

  /* we need at least 3 deltas / 4 samples for a reliable chirp detection */
  #define NUM_DIFFS 3
-static const int FFT_NUM_SAMPLES   = (NUM_DIFFS + 1);

  /* Threshold for difference of delta peaks */
  static const int MAX_DIFF = 2;
@@ -101,7 +100,7 @@ static bool ath9k_check_chirping(struct ath_softc *sc, u8 
*data,
 int datalen, bool is_ctl, bool is_ext)
  {
int i;
-   int max_bin[FFT_NUM_SAMPLES];
+   int max_bin[NUM_DIFFS + 1];


Just wondering. Is this actually a VLA. FFT_NUM_SAMPLES was static const 
so not really going to show a lot of variation. This array will always 
have the same size on the stack.


Regards,
Arend

Re: Problem with bridge (mcast-to-ucast + hairpin) and Broadcom's 802.11f in their FullMAC fw

[Arend van Spriel]

On 2/27/2018 11:14 AM, Rafał Miłecki wrote:

Sending with a fixed linux-wireless ML address. Please kindly send your
replies using linux-wireless@

On 02/27/2018 11:08 AM, Rafał Miłecki wrote:

I've problem when using OpenWrt/LEDE on a home router with Broadcom's
FullMAC WiFi chipset.


First of all OpenWrt/LEDE uses bridge interface for LAN network with:
1) IFLA_BRPORT_MCAST_TO_UCAST
2) Clients isolation in hostapd
3) Hairpin mode enabled

For more details please see Linus's patch description:
https://patchwork.kernel.org/patch/9530669/
and maybe hairpin mode patch:
https://lwn.net/Articles/347344/

Short version: in that setup packets received from a bridged wireless
interface can be handled back to it for transmission.


Now, Broadcom's firmware for their FullMAC chipsets in AP mode
supports an obsoleted 802.11f AKA IAPP standard. It's a roaming
standard that was replaced by 802.11r.

Whenever a new station associates, firmware generates a packet like:
ff ff ff ff  ff ff ec 10  7b 5f ?? ??  00 06 00 01  af 81 01 00
(just masked 2 bytes of my MAC)

For mode details you can see discussion in my brcmfmac patch thread:
https://patchwork.kernel.org/patch/10191451/


The problem is that bridge (in setup as above) handles such a packet
back to the device.


From reading the referenced links I understand the hairpin mode is 
causing the packet to be sent back to the device, and the hairpin mode 
is required for MCAST_TO_UCAST, right?



That makes Broadcom's FullMAC firmware believe that a given station
just connected to another AP in a network (which doesn't even exist).
As a result firmware immediately disassociates that station. It's
simply impossible to connect to the router. Every association is
followed by immediate disassociation.


Can you see any solution for this problem? Is that an option to stop
multicast-to-unicast from touching 802.11f packets? Some other ideas?
Obviously I can't modify Broadcom's firmware and drop that obsoleted
standard.


As far as I can tell you are correct that the 802.11f amendment was 
never adopted into the 802.11 standard. I will ask internally if we 
still have a reason for carrying it in our firmware.


Regards,
Arend

Re: [PATCH] brcmfmac: Make sure CLM downloading is optional

[Arend van Spriel]

On 1/15/2018 6:10 PM, Bjorn Andersson wrote:

The presence of a CLM file is described as optional, but missing the clm
blob causes the preinit to return unsuccessfully. Fix this by ignoring
the return value of the brcmf_c_process_clm_blob().

Also remove the extra debug print, as brcmf_c_process_clm_blob() already
did print a useful error message before returning.

Fixes: fdd0bd88ceae ("brcmfmac: add CLM download support")
Cc: sta...@vger.kernel.org
Signed-off-by: Bjorn Andersson 
---

This regression was introduced in v4.15-rc1, but I unfortunately didn't test
WiFi until now. Included a Cc to stable@ in case you choose to pick this up
after v4.15.


Hi Bjorn,

Thanks for looking into this. Actually there already have been a couple 
of fixes posted on the linux-wireless list. [1] was rejected, [2] is 
being discussed, and [2] was posted by me and I was awaiting response 
from the guy reporting it.


Now the thing is that for old (Broadcom) firmware this is optional. 
Those firmwares don't even have CLM support and those that do have the 
CLM data embedded in firmware. However, Cypress wants to provide their 
customers with firmware without CLM data. For those devices it is not 
optional. I still prefer we add a mechanism to the driver to detect 
that, but we do not have that yet.


Now regarding your patch some comments below ...


  drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c | 8 ++--
  1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index 6a59d0609d30..0c67ba6ae135 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
@@ -278,12 +278,8 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp)
}
ri->result = err;

-   /* Do any CLM downloading */
-   err = brcmf_c_process_clm_blob(ifp);
-   if (err < 0) {
-   brcmf_err("download CLM blob file failed, %d\n", err);
-   goto done;
-   }
+   /* Do any optional CLM downloading */
+   brcmf_c_process_clm_blob(ifp);


The error print is indeed redundant and should be removed here. However, 
brcmf_c_process_clm_blob() also returns -ENOMEM upon allocation failure 
and I would still fail the driver probe for that.


Regards,
Arend

[1] https://patchwork.kernel.org/patch/10106571/
[2] https://patchwork.kernel.org/patch/10159731/
[3] https://patchwork.kernel.org/patch/10154595/

Re: [PATCH v2] b43: Replace mdelay with usleep_range in b43_radio_2057_init_post

[Arend van Spriel]

On 1/9/2018 10:47 AM, Jia-Ju Bai wrote:



On 2018/1/9 17:07, Arend van Spriel wrote:

On 1/9/2018 9:39 AM, Jia-Ju Bai wrote:



On 2018/1/9 16:35, Greg KH wrote:

On Tue, Jan 09, 2018 at 09:40:06AM +0800, Jia-Ju Bai wrote:

b43_radio_2057_init_post is not called in an interrupt handler
nor holding a spinlock.
The function mdelay in it can be replaced with usleep_range,
to reduce busy wait.

Signed-off-by: Jia-Ju Bai <baijiaju1...@gmail.com>
---
v2:
* Replace mdelay with usleep_range, instead of msleep in v1.
   Thank Larry for good advice.
---
  drivers/net/wireless/broadcom/b43/phy_n.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/b43/phy_n.c
b/drivers/net/wireless/broadcom/b43/phy_n.c
index a5557d7..f2a2f41 100644
--- a/drivers/net/wireless/broadcom/b43/phy_n.c
+++ b/drivers/net/wireless/broadcom/b43/phy_n.c
@@ -1031,7 +1031,7 @@ static void b43_radio_2057_init_post(struct
b43_wldev *dev)
  b43_radio_set(dev, R2057_RFPLL_MISC_CAL_RESETN, 0x78);
  b43_radio_set(dev, R2057_XTAL_CONFIG2, 0x80);
-mdelay(2);
+usleep_range(2000, 3000);

Where did 3000 come from?  Are you sure about that?


I am not very sure, and I use it according to Larry's message:


Hi Jia-Ju Bai,

The duration here is for settling the registers so hardware can pick
it up. Right after this they are written again. Now this is during
initialization of the radio so not time critical, but probably
anything in the range of 2000..3000 would also have been fine.


Hi Arend,

Thanks for your detailed explanation :)
So I think usleep_range(2000, 3000) is okay.


Sure.

Regards,
Arend

Re: [PATCH v2] b43: Replace mdelay with usleep_range in b43_radio_2057_init_post

[Arend van Spriel]

On 1/9/2018 9:39 AM, Jia-Ju Bai wrote:



On 2018/1/9 16:35, Greg KH wrote:

On Tue, Jan 09, 2018 at 09:40:06AM +0800, Jia-Ju Bai wrote:

b43_radio_2057_init_post is not called in an interrupt handler
nor holding a spinlock.
The function mdelay in it can be replaced with usleep_range,
to reduce busy wait.

Signed-off-by: Jia-Ju Bai 
---
v2:
* Replace mdelay with usleep_range, instead of msleep in v1.
   Thank Larry for good advice.
---
  drivers/net/wireless/broadcom/b43/phy_n.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/b43/phy_n.c
b/drivers/net/wireless/broadcom/b43/phy_n.c
index a5557d7..f2a2f41 100644
--- a/drivers/net/wireless/broadcom/b43/phy_n.c
+++ b/drivers/net/wireless/broadcom/b43/phy_n.c
@@ -1031,7 +1031,7 @@ static void b43_radio_2057_init_post(struct
b43_wldev *dev)
  b43_radio_set(dev, R2057_RFPLL_MISC_CAL_RESETN, 0x78);
  b43_radio_set(dev, R2057_XTAL_CONFIG2, 0x80);
-mdelay(2);
+usleep_range(2000, 3000);

Where did 3000 come from?  Are you sure about that?


I am not very sure, and I use it according to Larry's message:


Hi Jia-Ju Bai,

The duration here is for settling the registers so hardware can pick it 
up. Right after this they are written again. Now this is during 
initialization of the radio so not time critical, but probably anything 
in the range of 2000..3000 would also have been fine.


Regards,
Arend

Re: [PATCH] brcmsmac: use ARRAY_SIZE on rfseq_updategainu_events

[Arend van Spriel]

On 12/7/2017 11:20 AM, Colin King wrote:

From: Colin Ian King <colin.k...@canonical.com>

Use the ARRAY_SIZE macro on rfseq_updategainu_events to determine
size of the array. Improvement suggested by coccinelle.


Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>

Signed-off-by: Colin Ian King <colin.k...@canonical.com>
---
  drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

Re: [PATCH v1] brcmfmac: Avoid build error with make W=1

[Arend van Spriel]

On 23-11-17 16:57, Andy Shevchenko wrote:

When I run make W=1 on gcc (Debian 7.2.0-16) 7.2.0 I got an error for
the first run, all next ones are okay.

   CC [M]  drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.o
drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c:2078: error: Cannot 
parse struct or union!
scripts/Makefile.build:310: recipe for target 
'drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.o' failed

Seems like something happened with W=1 and wrong kernel doc format.
As a quick fix remove dubious /** in the code.


Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>

Signed-off-by: Andy Shevchenko <andriy.shevche...@linux.intel.com>
---
  drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Re: [PATCH] dsa: dsa2: fix compile error for !CONFIG_OF

[Arend van Spriel]

On 11/24/2017 3:28 AM, Andrew Lunn wrote:

On Thu, Nov 23, 2017 at 08:27:48PM +0100, Arend Van Spriel wrote:

+ Arnd

On Thu, Nov 23, 2017 at 8:12 PM, Arend Van Spriel
<arend.vanspr...@broadcom.com> wrote:

On Thu, Nov 23, 2017 at 3:04 PM, Andrew Lunn <and...@lunn.ch> wrote:

On Thu, Nov 23, 2017 at 01:00:51PM +0100, Arend van Spriel wrote:

Compilation fails building on x86_64 platform which does not
have CONFIG_OF enabled.

Signed-off-by: Arend van Spriel <arend.vanspr...@broadcom.com>
---
After rebasing my branch to v4.14 I attempted to build the kernel and hit
the following compile issue:

net/dsa/dsa2.c: In function \u2018dsa_switch_parse_member_of\u2019:
net/dsa/dsa2.c:678:2: error: implicit declaration of function
'of_property_read_variable_u32_array'


Hi Arend

https://lkml.org/lkml/2017/11/6/493


So my email/patch did get through initially. Sorry for the noise and
thanks for the info.


Hi Andrew,

Getting back to this. It seems that this patch did not get in. At
least I searched for it in v4.14.1 but no luck.


Hi Arned

The use of of_property_read_variable_u32_array was added in
975e6e32215e ("net: dsa: rework switch parsing"). This patch is not in
v4.14. It is in linus/master, so v4.15-rc1 should have it. And the fix
is also in linus/master.

So there does not appear to be anything wrong. I just built v4.14.1
for x86_64 with DSA without problems.


Thanks, Andrew

I am actually using wireless-testing tree which it based on 4.14 and 
throws in net-next and the wireless trees. I assume the fix did not go 
through net-next. Sorry for the confusion.


Regards,
Arend

Re: [PATCH] dsa: dsa2: fix compile error for !CONFIG_OF

[Arend Van Spriel]

+ Arnd

On Thu, Nov 23, 2017 at 8:12 PM, Arend Van Spriel
<arend.vanspr...@broadcom.com> wrote:
> On Thu, Nov 23, 2017 at 3:04 PM, Andrew Lunn <and...@lunn.ch> wrote:
>> On Thu, Nov 23, 2017 at 01:00:51PM +0100, Arend van Spriel wrote:
>>> Compilation fails building on x86_64 platform which does not
>>> have CONFIG_OF enabled.
>>>
>>> Signed-off-by: Arend van Spriel <arend.vanspr...@broadcom.com>
>>> ---
>>> After rebasing my branch to v4.14 I attempted to build the kernel and hit
>>> the following compile issue:
>>>
>>> net/dsa/dsa2.c: In function \u2018dsa_switch_parse_member_of\u2019:
>>> net/dsa/dsa2.c:678:2: error: implicit declaration of function
>>> 'of_property_read_variable_u32_array'
>>
>> Hi Arend
>>
>> https://lkml.org/lkml/2017/11/6/493
>
> So my email/patch did get through initially. Sorry for the noise and
> thanks for the info.

Hi Andrew,

Getting back to this. It seems that this patch did not get in. At
least I searched for it in v4.14.1 but no luck.

Regards,
Arend

Re: [PATCH] dsa: dsa2: fix compile error for !CONFIG_OF

[Arend Van Spriel]

On Thu, Nov 23, 2017 at 3:04 PM, Andrew Lunn <and...@lunn.ch> wrote:
> On Thu, Nov 23, 2017 at 01:00:51PM +0100, Arend van Spriel wrote:
>> Compilation fails building on x86_64 platform which does not
>> have CONFIG_OF enabled.
>>
>> Signed-off-by: Arend van Spriel <arend.vanspr...@broadcom.com>
>> ---
>> After rebasing my branch to v4.14 I attempted to build the kernel and hit
>> the following compile issue:
>>
>> net/dsa/dsa2.c: In function \u2018dsa_switch_parse_member_of\u2019:
>> net/dsa/dsa2.c:678:2: error: implicit declaration of function
>> 'of_property_read_variable_u32_array'
>
> Hi Arend
>
> https://lkml.org/lkml/2017/11/6/493

So my email/patch did get through initially. Sorry for the noise and
thanks for the info.

Regards,
Arend

Re: kernel BUG at crypto/asymmetric_keys/public_key.c:80

[Arend van Spriel]

+ Johannes

On 22-11-17 18:43, Florian Fainelli wrote:

Hi,

(sorry for the cross post)

I am at v4.14-12995-g0c86a6bd85ff and just met the following, attached
is my .config file. Is this a known problem? Thanks!

[1.798714] cfg80211: Loading compiled-in X.509 certificates for
regulatory database
[1.809390] [ cut here ]
[1.814020] kernel BUG at crypto/asymmetric_keys/public_key.c:80!
[1.820123] Internal error: Oops - BUG: 0 [#1] SMP ARM
[1.825273] Modules linked in:
[1.828341] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
4.14.0-12995-g0c86a6bd85ff #15
[1.836096] Hardware name: Broadcom STB (Flattened Device Tree)
[1.842025] task: ee0a task.stack: ee096000
[1.846576] PC is at public_key_verify_signature+0x21c/0x260
[1.852248] LR is at x509_check_for_self_signed+0xb0/0x10c
[1.857743] pc : []lr : []psr: 6013
[1.864019] sp : ee097cf8  ip : c0a7a3ae  fp : 
[1.869252] r10: c248e9d8  r9 : c0b401e0  r8 : ee374040
[1.874487] r7 : c0a7a340  r6 : ee374200  r5 : c2404c48  r4 : edac8880
[1.881024] r3 :   r2 : c0b40480  r1 : ee3741c0  r0 : ee374040
[1.887563] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM
Segment user
[1.894709] Control: 30c5387d  Table: 3000  DAC: fffd
[1.900465] Process swapper/0 (pid: 1, stack limit = 0xee096210)
[1.906481] Stack: (0xee097cf8 to 0xee098000)
[1.910845] 7ce0:
   6013 
[1.919037] 7d00: 014080c0 c052149c 11a0 c248e9d8 
c033dcb8 ee097d98 014000c0
[1.927229] 7d20:  0001 ed58622e c0a7a634 0001
ed58622e c0a7a634 c0521530
[1.935421] 7d40: c052143c  ed586200  
 c0518bc0 c248e9d8
[1.943612] 7d60:  c02416f8 c244bc28  ed586200
ed586388 ed586384 6013
[1.951804] 7d80:  c0b400d4 c0518bc0 c248e9d8 
c025de10  
[1.959995] 7da0:  fffe fffe  
c0b400d4 c0518bc0 c0518c34
[1.968187] 7dc0: c0a23498 ee096000  00040e00 ee374302
edac8880 edac8880 ee374200
[1.976378] 7de0: c0a7a340 02a8 c0b401e0 c248e9d8 
c0526ee8  edac8880
[1.984569] 7e00: ee374200 c0525f24 c244d210 ee097e80 c248e9d8
ee097e80 c244d1ac c0526b74
[1.992760] 7e20: c244d210 c244b988 c248e9d8 ee097e80 c244d1ac
c0b401e0 c248e9d8 c0524f20
[2.000952] 7e40: c2404c48 c244b988 c0a7a340 edac8801 ee02c180
edac8800  c0511148
[2.009143] 7e60: c24b7eda 0048 6013  
c244d1b4  
[2.017335] 7e80:     
c0a7a340 02a8 
[2.025527] 7ea0: 7fff 00040e00 c0a7a340 c24df7b4 02a8
c0a7a5e8 c0bb10d8 c0b18088
[2.033718] 7ec0: 1f03 c0e47898 02a8 1f03 000e
  c2404c48
[2.041910] 7ee0: e000 c0e4777c  c0e6583c c0e74f98
0008  c0201bd8
[2.050101] 7f00: 6013 c025dda4  c0c05a00 c0e005d8
  0007
[2.058292] 7f20: 0007 00040e00  c240d790 
c2404c48 c0e65818 
[2.066483] 7f40:  00040e00  00040e00 c24a3100
c24a3100 c24a3100 0109
[2.074675] 7f60: c0e65838 c0e6583c c0e74f98 c0e00e6c 0007
0007  c0e005d8
[2.082866] 7f80: c09b47f8  c09b47f8  
  
[2.091057] 7fa0:  c09b4800  c0208920 
  
[2.099248] 7fc0:     
  
[2.107440] 7fe0:     0013
 60bd36df 5ae9d652
[2.115645] [] (public_key_verify_signature) from
[] (x509_check_for_self_signed+0xb0/0x10c)
[2.125842] [] (x509_check_for_self_signed) from
[] (x509_cert_parse+0x14c/0x1a8)
[2.135080] [] (x509_cert_parse) from []
(x509_key_preparse+0x14/0x18c)
[2.143449] [] (x509_key_preparse) from []
(asymmetric_key_preparse+0x54/0xd4)
[2.152430] [] (asymmetric_key_preparse) from []
(key_create_or_update+0x120/0x3c4)
[2.161846] [] (key_create_or_update) from []
(regulatory_init_db+0x11c/0x1e4)
[2.170828] [] (regulatory_init_db) from []
(do_one_initcall+0x54/0x18c)
[2.179293] [] (do_one_initcall) from []
(kernel_init_freeable+0x140/0x1cc)
[2.188011] [] (kernel_init_freeable) from []
(kernel_init+0x8/0x110)
[2.196210] [] (kernel_init) from []
(ret_from_fork+0x14/0x34)
[2.203796] Code: ebf8636b eaab e7f001f2 e7f001f2 (e7f001f2)
[2.209901] ---[ end trace 4ec242c4e6a05178 ]---
[2.214553] Kernel panic - not syncing: Attempted to kill init!
exitcode=0x000b



This is the regulatory database stuff that Johannes added. The BUG() 
that triggers is here:


int public_key_verify_signature(const struct public_key *pkey,
const struct public_key_signature *sig)
{
struct crypto_wait cwait;
struct 

Re: [PATCH v3 1/2] dt-bindings: add device tree binding for Allwinner XR819 SDIO Wi-Fi

[Arend van Spriel]

On 10/4/2017 11:03 AM, Icenowy Zheng wrote:



于 2017年10月4日 GMT+08:00 下午5:02:17, Kalle Valo  写到:

Icenowy Zheng  writes:


Allwinner XR819 is a SDIO Wi-Fi chip, which has the functionality to

use

an out-of-band interrupt pin instead of SDIO in-band interrupt.

Add the device tree binding of this chip, in order to make it

possible

to add this interrupt pin to device trees.

Signed-off-by: Icenowy Zheng 
Acked-by: Rob Herring 
---
Changes in v3:
- Renames the node name.
- Adds ACK from Rob.
Changes in v2:
- Removed status property in example.
- Added required property reg.

  .../bindings/net/wireless/allwinner,xr819.txt  | 38

++

  1 file changed, 38 insertions(+)
  create mode 100644

Documentation/devicetree/bindings/net/wireless/allwinner,xr819.txt

Like I asked already last time, AFAICS there is no upstream xr819
wireless driver in drivers/net/wireless directory. Do we still accept
bindings like this for out-of-tree drivers?


See esp8089.

There's also no in-tree driver for it.


The question is whether we should. The above might be a precedent, but 
it may not necessarily be the way to go. The commit message for esp8089 
seems to hint that there is intent to have an in-tree driver:


"""
Note that at this point there only is an out of tree driver for this
hardware, there is no clear timeline / path for merging this. Still
I believe it would be good to specify the binding for this in tree
now, so that any future migration to an in tree driver will not cause
compatiblity issues.

Cc: Icenowy Zheng 
Signed-off-by: Hans de Goede 
Signed-off-by: Rob Herring 
"""

Regardless the bindings are in principle independent of the kernel and 
just describing hardware. I think there have been discussions to move 
the bindings to their own repository, but apparently it was decided 
otherwise.


Regards,
Arend

Re: [PATCH] brcm80211: make const array ucode_ofdm_rates static, reduces object code size

[Arend van Spriel]

Please use 'brcmsmac:' as prefix instead of 'brcm80211:'.

On 22-09-17 16:03, Colin King wrote:

From: Colin Ian King <colin.k...@canonical.com>

Don't populate const array ucode_ofdm_rates on the stack, instead make it
static. Makes the object code smaller by 100 bytes:

Before:
text   data bss dec hex filename
   39482564   0   400469c6e phy_cmn.o

After
text   data bss dec hex filename
   39326620   0   399469c0a phy_cmn.o

(gcc 6.3.0, x86-64)


Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>

Signed-off-by: Colin Ian King <colin.k...@canonical.com>
---
  drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_cmn.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_cmn.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_cmn.c
index 1c4e9dd57960..3a13d176b221 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_cmn.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_cmn.c
@@ -1916,7 +1916,7 @@ void wlc_phy_txpower_update_shm(struct brcms_phy *pi)
 pi->hwpwr_txcur);
  
  		for (j = TXP_FIRST_OFDM; j <= TXP_LAST_OFDM; j++) {

-   const u8 ucode_ofdm_rates[] = {
+   static const u8 ucode_ofdm_rates[] = {
0x0c, 0x12, 0x18, 0x24, 0x30, 0x48, 0x60, 0x6c
};
offset = wlapi_bmac_rate_shm_offset(

Re: Regression: Bug 196547 - Since 4.12 - bonding module not working with wireless drivers

[Arend van Spriel]

On 10-08-17 07:39, Kalle Valo wrote:

Hi Mahesh and Andy,

James Feeney reported that there's a serious regression in bonding
module since v4.12, it doesn't work with wireless drivers anymore as
wireless drivers don't report the link speed via ethtool:

https://bugzilla.kernel.org/show_bug.cgi?id=196547

In the bug report it's said that this commit is the culprit:

3f3c278c94dd bonding: fix active-backup transition


This commit references another one. ie. commit c4adfc822bf5 ("bonding: 
make speed, duplex setting consistent with link state"). Before this 
commit the result of __ethtool_get_link_ksettings() was simply ignored.


--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -365,9 +365,10 @@ int bond_set_carrier(struct bonding *bond)
 /* Get link speed and duplex from the slave's base driver
  * using ethtool. If for some reason the call fails or the
  * values are invalid, set speed and duplex to -1,
- * and return.
+ * and return. Return 1 if speed or duplex settings are
+ * UNKNOWN; 0 otherwise.
  */
-static void bond_update_speed_duplex(struct slave *slave)
+static int bond_update_speed_duplex(struct slave *slave)
 {
struct net_device *slave_dev = slave->dev;
struct ethtool_link_ksettings ecmd;
@@ -377,24 +378,27 @@ static void bond_update_speed_duplex(struct slave 
*slave)

slave->duplex = DUPLEX_UNKNOWN;

res = __ethtool_get_link_ksettings(slave_dev, );
-   if (res < 0)
-   return;
-
-   if (ecmd.base.speed == 0 || ecmd.base.speed == ((__u32)-1))
-   return;
-
+   if (res < 0) {
+   slave->link = BOND_LINK_DOWN;
+   return 1;
+   }
+   if (ecmd.base.speed == 0 || ecmd.base.speed == ((__u32)-1)) {
+   slave->link = BOND_LINK_DOWN;
+   return 1;
+   }

Commit 3f3c278c94dd ("bonding: fix active-backup transition") moves 
setting the link state to the call sites of bond_update_speed_duplex(), 
just not all call sites.



Is there a fix for this or should that commit be reverted? This seems to
be a serious regression as there are multiple reports already and we
should get it fixed for v4.13, and the fix backported to v4.12 stable
release.


The ethtool callbacks really seem optional. At least in brcmfmac, the 
wireless driver I maintain, I only provide get_drvinfo callback and 
there is no warning triggered upon registering the netdev. The changes 
above now require each netdev to implement the get_link_ksettings 
callback (get_settings is deprecated) or the link is marked as DOWN 
ruling it out to be used as active bond slave. To the end-users who were 
using bonding this is simply a regression. So to fix that both changes 
should be reverted in my opinion.


Now specifically for wireless interfaces we could implement 
get_link_ksettings callback although most of the fields requested are 
meaningless in wireless context. Regarding the speed and half-duplex 
values we raised some concerns in an earlier discussion with James. 
Wireless is always half-duplex as there can be only one (unintended ref 
to [1]). If the reported speed in wifi is difficult. In wifi we have 
txrate and rxrate which are inherently asynchronous and it is a 
per-packet value so it is going to change a lot. Seeing only 4 call 
sites in the bonding code tells me that is not taken into account. All 
in all this shenanigan seems netconf material to me.


Regards,
Arend

[1] https://en.wikipedia.org/wiki/Highlander_(film)

Re: [PATCH V5 2/2] brcmfmac: don't warn user about NVRAM if fallback to the platform one succeeds

[Arend van Spriel]

On 31-07-17 17:09, Rafał Miłecki wrote:
> From: Rafał Miłecki <ra...@milecki.pl>
> 
> Failing to load NVRAM *file* isn't critical if we manage to get platform
> NVRAM in the fallback path. It means warnings like:
> [   10.801506] brcmfmac :01:00.0: Direct firmware load for 
> brcm/brcmfmac43602-pcie.txt failed with error -2
> are unnecessary & disturbing for people with *platform* NVRAM as they
> are not expected to have NVRAM file. This is a very common case for
> Broadcom home routers.
> 
> Instead of printing warning immediately within the firmware subsystem
> let's try our fallback code first. If that fails as well, then it's a
> right moment to print an error.
> 
> This should reduce amount of false reports from users seeing this
> warning while having wireless working perfectly fine with the platform
> NVRAM.

Reviewed-by: Arend van Spriel <arend.vanspr...@broadcom.com>
> Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
> ---
> V2: Update commit message as it wasn't clear enough (thanks Andy) & add extra
> messages to the firmware.c.
> V3: Set FW_OPT_UEVENT to don't change behavior
> V4: Switch to the new request_firmware_async syntax
> V5: Rebase, update commit message, resend after drvdata discussion
> ---
>  .../wireless/broadcom/brcm80211/brcmfmac/firmware.c| 18 
> +-
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> index d231042f19d6..524442b3870f 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> @@ -462,8 +462,14 @@ static void brcmf_fw_request_nvram_done(const struct 
> firmware *fw, void *ctx)
>   raw_nvram = false;
>   } else {
>   data = bcm47xx_nvram_get_contents(_len);
> - if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
> - goto fail;
> + if (!data) {
> + brcmf_dbg(TRACE, "Failed to get platform NVRAM\n");

Better make this INFO level instead of TRACE. The intent of TRACE level
is for entry/exit points in functions.

Regards,
Arend

Re: [PATCH 11/11] net: brcmfmac: constify pci_device_id.

[Arend van Spriel]

On 17-07-17 20:16, Arvind Yadav wrote:
> pci_device_id are not supposed to change at runtime. All functions
> working with pci_device_id provided by  work with
> const pci_device_id. So mark the non-const structs as const.

Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>
> Signed-off-by: Arvind Yadav <arvind.yadav...@gmail.com>
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> index f878706..e6e9b00 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> @@ -1951,7 +1951,7 @@ static const struct dev_pm_ops brcmf_pciedrvr_pm = {
>   BRCM_PCIE_VENDOR_ID_BROADCOM, dev_id,\
>   subvend, subdev, PCI_CLASS_NETWORK_OTHER << 8, 0x00, 0 }
>  
> -static struct pci_device_id brcmf_pcie_devid_table[] = {
> +static const struct pci_device_id brcmf_pcie_devid_table[] = {
>   BRCMF_PCIE_DEVICE(BRCM_PCIE_4350_DEVICE_ID),
>   BRCMF_PCIE_DEVICE(BRCM_PCIE_4356_DEVICE_ID),
>   BRCMF_PCIE_DEVICE(BRCM_PCIE_43567_DEVICE_ID),
> 

Re: [PATCH V2] brcmfmac: fix possible buffer overflow in brcmf_cfg80211_mgmt_tx()

[Arend van Spriel]

On 7/7/2017 10:09 PM, Arend van Spriel wrote:

The lower level nl80211 code in cfg80211 ensures that "len" is between
25 and NL80211_ATTR_FRAME (2304).  We subtract DOT11_MGMT_HDR_LEN (24) from
"len" so thats's max of 2280.  However, the action_frame->data[] buffer is
only BRCMF_FIL_ACTION_FRAME_SIZE (1800) bytes long so this memcpy() can
overflow.

memcpy(action_frame->data, [DOT11_MGMT_HDR_LEN],
   le16_to_cpu(action_frame->len));

Cc: sta...@vger.kernel.org # 3.9.x
Fixes: 18e2f61db3b70 ("brcmfmac: P2P action frame tx.")
Reported-by: "freenerguo(郭大兴)" <freener...@tencent.com>
Signed-off-by: Arend van Spriel <arend.vanspr...@broadcom.com>
---
  V2:
   - added Fixes: tag and Cc: for stable kernels.
   - Cc: patch to netdev list.
---
Hi David,

Here is the patch as Linus send it to us and secur...@kernel.org. I
removed the lower bound check as that is already done in cfg80211.
Now I signed off on the patch although formally I suppose Linus should
sign it off. Putting it out there so people can respond as deemed
necessary.

The reason for submitting it to your tree is the fact that Kalle is
on vacation for next 10 days or so which was indicated to me by Johannes.
The patch applies to the master branch of your net repository. For
reference V1 of this patch can be found here [1].


Hi Dave,

Not sure if you missed this one. It is addressing a reported security 
issue and intended for the net repository, not net-next which is 
obviously closed [2].


Regards,
Arend

[2] http://vger.kernel.org/~davem/net-next.html


Regards,
Arend

[1] https://patchwork.kernel.org/patch/9829977/
---
  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index cd1d673..d182a00 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -4851,6 +4851,11 @@ static int brcmf_cfg80211_stop_ap(struct wiphy *wiphy, 
struct net_device *ndev)
cfg80211_mgmt_tx_status(wdev, *cookie, buf, len, true,
GFP_KERNEL);
} else if (ieee80211_is_action(mgmt->frame_control)) {
+   if (len > BRCMF_FIL_ACTION_FRAME_SIZE + DOT11_MGMT_HDR_LEN) {
+   brcmf_err("invalid action frame length\n");
+   err = -EINVAL;
+   goto exit;
+   }
af_params = kzalloc(sizeof(*af_params), GFP_KERNEL);
if (af_params == NULL) {
brcmf_err("unable to allocate frame\n");

Re: [PATCH] brcmfmac: added LED triggers for transmit/receive

[Arend van Spriel]

On 10-07-17 19:02, Russell Joyce wrote:
>> 1) I think most of it should be some cfg80211 shareable code.
> 
> I’m not sure exactly what you mean by this, could you please clarify?

What I think Rafał is saying is that it would be better to have this
code in cfg80211 so other drivers including mac80211 could use it.

>> 2) This "rxtx" while surely present in other places sounds like a
>> workaround for LED subsystem limitation. Maybe it's time to finally
>> rework LED triggers.
> 
> I agree that it’s not an ideal way to do things, but I couldn’t think of a
> better alternative. I think that having a combined trigger is useful though, 
> for
> situations like using the single LED on a Raspberry Pi to show Wi-Fi activity.

Indeed. However, the LED subsystem could/should(?) take care of mapping
"rx" and "tx" triggers to the same LED.

I am happy to comment on your patch, but maybe you can first take a look
at the suggestion Rafał made above.

Regards,
Arend

[PATCH V2] brcmfmac: fix possible buffer overflow in brcmf_cfg80211_mgmt_tx()

[Arend van Spriel]

The lower level nl80211 code in cfg80211 ensures that "len" is between
25 and NL80211_ATTR_FRAME (2304).  We subtract DOT11_MGMT_HDR_LEN (24) from
"len" so thats's max of 2280.  However, the action_frame->data[] buffer is
only BRCMF_FIL_ACTION_FRAME_SIZE (1800) bytes long so this memcpy() can
overflow.

memcpy(action_frame->data, [DOT11_MGMT_HDR_LEN],
   le16_to_cpu(action_frame->len));

Cc: sta...@vger.kernel.org # 3.9.x
Fixes: 18e2f61db3b70 ("brcmfmac: P2P action frame tx.")
Reported-by: "freenerguo(郭大兴)" <freener...@tencent.com>
Signed-off-by: Arend van Spriel <arend.vanspr...@broadcom.com>
---
 V2:
  - added Fixes: tag and Cc: for stable kernels.
  - Cc: patch to netdev list.
---
Hi David,

Here is the patch as Linus send it to us and secur...@kernel.org. I
removed the lower bound check as that is already done in cfg80211.
Now I signed off on the patch although formally I suppose Linus should
sign it off. Putting it out there so people can respond as deemed
necessary.

The reason for submitting it to your tree is the fact that Kalle is
on vacation for next 10 days or so which was indicated to me by Johannes.
The patch applies to the master branch of your net repository. For
reference V1 of this patch can be found here [1].

Regards,
Arend

[1] https://patchwork.kernel.org/patch/9829977/
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index cd1d673..d182a00 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -4851,6 +4851,11 @@ static int brcmf_cfg80211_stop_ap(struct wiphy *wiphy, 
struct net_device *ndev)
cfg80211_mgmt_tx_status(wdev, *cookie, buf, len, true,
GFP_KERNEL);
} else if (ieee80211_is_action(mgmt->frame_control)) {
+   if (len > BRCMF_FIL_ACTION_FRAME_SIZE + DOT11_MGMT_HDR_LEN) {
+   brcmf_err("invalid action frame length\n");
+   err = -EINVAL;
+   goto exit;
+   }
af_params = kzalloc(sizeof(*af_params), GFP_KERNEL);
if (af_params == NULL) {
brcmf_err("unable to allocate frame\n");
-- 
1.9.1

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Arend van Spriel]

On 23-06-17 23:53, Luis R. Rodriguez wrote:
> On Tue, May 16, 2017 at 10:41:08AM +0200, Arend Van Spriel wrote:
>> On 16-5-2017 1:13, Luis R. Rodriguez wrote:
>>> Since no upstream delta is needed for firmwared I'd like to first encourage
>>> evaluating the above. While distributions don't carry it yet that may be 
>>> seen as
>>> an issue but since what we are looking for are corner cases, only folks 
>>> needing
>>> to deploy a specific solution would need it or a custom proprietary 
>>> solution.
>>
>> Ok. I will go try and run firmwared in OpenWrt on a router platform.
>> Have to steal one from a colleague :-p Will study firmwared.
> 
> Arend, curious how this effort goes. Its important to me as we know then that
> if this works its a good approach to recommend moving forward which should 
> also
> prove less complex than that soup we had with the custom fallback stuff.

Hi Luis,

So I looked at firmwared and we basically need to extend it. For our
router platform we need to obtain nvram calibration data from an MTD
partition which contains the raw data, ie. no file-system on it. So
firmwared would need some sort of configuration to map a particular
firmware file to some action obtaining the data like kicking off some
mtd-utils in my case.

Regards,
Arend

Re: [PATCH] cfg80211: Fix a memory leak in error handling path in 'brcmf_cfg80211_attach'

[Arend van Spriel]

On 20-06-17 08:22, Christophe JAILLET wrote:
> If 'wiphy_new()' fails, we leak 'ops'. Add a new label in the error
> handling path to free it in such a case.

Thanks. Please add the following tags:

Cc: sta...@vger.kernel.org
> Fixes: 5c22fb85102a7 ("brcmfmac: add wowl gtk rekeying offload support")
Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>
> Signed-off-by: Christophe JAILLET <christophe.jail...@wanadoo.fr>
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Re: brcmfmac: Fix kernel oops on resume when request firmware fails.

[Arend van Spriel]

On 13-06-17 12:23, Enric Balletbo Serra wrote:
> Hello Kalle,
> 
> 2017-06-13 7:54 GMT+02:00 Kalle Valo :
>> Enric Balletbo i Serra  wrote:
>>
>>> When request firmware fails, brcmf_ops_sdio_remove is being called and
>>> brcmf_bus freed. In such circumstancies if you do a suspend/resume cycle
>>> the kernel hangs on resume due a NULL pointer dereference in resume
>>> function.
>>>
>>> Steps to reproduce the problem:
>>>  - modprobe brcmfmac without the firmware
>>>  brcmfmac mmc1:0001:1: Direct firmware load for 
>>> brcm/brcmfmac4354-sdio.bin
>>>  failed with error -2
>>>  - do a suspend/resume cycle (echo mem > /sys/power/state)
>>>
>>> Protect against the NULL pointer derefence by checking if dev_get_drvdata
>>> returned a valid pointer.
>>>
>>> Signed-off-by: Enric Balletbo i Serra 
>>
>> My understanding is that there's a new version of this patch which fixes
>> the issue. If not, let me know.
>>
>> Patch set to Superseded.
>>
> 
> Yes there are these patch series [1] that fixes the issue, I guess
> Arend is working on a v2 to fix a small issue we found.
> 
> [1] https://www.spinics.net/lists/linux-wireless/msg162762.html

That series was actually RFT so not a formal submit. I send out a series
yesterday, which indeed has the small issue fixed [2].

Regards,
Arend

[2] https://patchwork.kernel.org/patch/9780793/

>> --
>> https://patchwork.kernel.org/patch/9743159/
>>
>> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
>>
> 
> Regards,
>  Enric
> 

Re: [PATCH 0/6] Constant Time Memory Comparisons Are Important

[Arend van Spriel]

On 6/11/2017 11:30 PM, Emil Lenngren wrote:

2017-06-11 22:48 GMT+02:00 Emmanuel Grumbach :

On Sun, Jun 11, 2017 at 4:36 PM, Kees Cook  wrote:


On Sun, Jun 11, 2017 at 1:13 AM, Kalle Valo  wrote:

"Jason A. Donenfeld"  writes:


Whenever you're comparing two MACs, it's important to do this using
crypto_memneq instead of memcmp. With memcmp, you leak timing information,
which could then be used to iteratively forge a MAC.


Do you have any pointers where I could learn more about this?


While not using C specifically, this talks about the problem generally:
https://www.chosenplaintext.ca/articles/beginners-guide-constant-time-cryptography.html



Sorry for the stupid question, but the MAC address is in plaintext in
the air anyway or easily accessible via user space tools. I fail to
see what it is so secret about a MAC address in that code where that
same MAC address is accessible via myriads of ways.


I think you're mixing up Media Access Control (MAC) addresses with
Message Authentication Code (MAC). The second one is a cryptographic
signature of a message.


While this may be obvious to those who are in the know this mixup is 
easily made outside the crypto domain and especially in the (wireless) 
networking domain (my mind wandered towards the same error path). As 
this series is touching stuff outside crypto it is good to be explicit 
and not use such abbreviations that can be misinterpreted. The article 
Kees referred to is also useful to get into the proper context here and 
at least worth mentioning this or other useful references in the cover 
letter.


Regards,
Arend

Re: [PATCH v2] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain

[Arend van Spriel]

On 11-06-17 02:18, Peter Housel wrote:
> 
>> On Jun 10, 2017, at 12:27 PM, Arend van Spriel 
>> <arend.vanspr...@broadcom.com> wrote:
>>
>> On 03-06-17 17:36, Andy Shevchenko wrote:
>>> On Sat, Jun 3, 2017 at 1:29 AM, Peter S. Housel <hou...@acm.org> wrote:
>>>> An earlier change to this function (3bdae810721b) fixed a leak in the
>>>> case of an unsuccessful call to brcmf_sdiod_buffrw(). However, the
>>>> glom_skb buffer, used for emulating a scattering read, is never used
>>>> or referenced after its contents are copied into the destination
>>>> buffers, and therefore always needs to be freed by the end of the
>>>> function.
>>
>> [snip]
>>
>>>> +   skb_queue_walk(pktq, skb) {
>>>> +   memcpy(skb->data, glom_skb->data, 
>>>> skb->len);
>>>> +   skb_pull(glom_skb, skb->len);
>>>> +   }
>>>>}
>>>
>>>> +   brcmu_pkt_buf_free_skb(glom_skb);
>>>
>>> Can we just add this one line instead or I'm missing something?
>>
>> I guess. We don't want to walk the packet queue if glom_skb is not
>> carrying data due to brcmf_sdiod_buffrw() failure.
>>
>> So I would go with the patch below as brcmu_pkt_buf_free_skb() simply
>> ignores null pointer.
> 
> I’m fine with this, or indeed most of the other proposed solutions. The 
> important thing is that the leak is fixed; in the driver's current state I 
> was able to run our wearable device out of memory in just over 20 seconds 
> running iperf.

Sure. The reason behind the suggestion from Franky was to get rid of the
label inside branch and I agree with that. To address Andy's comment I
think my proposal should tackle that.

Just out of curiosity, we added the broken-sg-support thing for OMAP
platform. So what platform/mmc-host are you using. I try to keep an
overview where this workaround is needed.

Regards,
Arend

Re: [PATCH v2] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain

[Arend van Spriel]

On 03-06-17 17:36, Andy Shevchenko wrote:
> On Sat, Jun 3, 2017 at 1:29 AM, Peter S. Housel  wrote:
>> An earlier change to this function (3bdae810721b) fixed a leak in the
>> case of an unsuccessful call to brcmf_sdiod_buffrw(). However, the
>> glom_skb buffer, used for emulating a scattering read, is never used
>> or referenced after its contents are copied into the destination
>> buffers, and therefore always needs to be freed by the end of the
>> function.

[snip]

>> +   skb_queue_walk(pktq, skb) {
>> +   memcpy(skb->data, glom_skb->data, skb->len);
>> +   skb_pull(glom_skb, skb->len);
>> +   }
>> }
> 
>> +   brcmu_pkt_buf_free_skb(glom_skb);
> 
> Can we just add this one line instead or I'm missing something?

I guess. We don't want to walk the packet queue if glom_skb is not
carrying data due to brcmf_sdiod_buffrw() failure.

So I would go with the patch below as brcmu_pkt_buf_free_skb() simply
ignores null pointer.

Regards,
Arend
---
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index 5bc2ba2..3722f23 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -705,7 +705,7 @@ int brcmf_sdiod_recv_pkt(struct brcmf_sdio_dev
*sdiodev, struct sk_buff *pkt)
 int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev *sdiodev,
   struct sk_buff_head *pktq, uint totlen)
 {
-   struct sk_buff *glom_skb;
+   struct sk_buff *glom_skb = NULL;
struct sk_buff *skb;
u32 addr = sdiodev->sbwad;
int err = 0;
@@ -726,10 +726,8 @@ int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev
*sdiodev,
return -ENOMEM;
err = brcmf_sdiod_buffrw(sdiodev, SDIO_FUNC_2, false, addr,
 glom_skb);
-   if (err) {
-   brcmu_pkt_buf_free_skb(glom_skb);
+   if (err)
goto done;
-   }

skb_queue_walk(pktq, skb) {
memcpy(skb->data, glom_skb->data, skb->len);
@@ -740,6 +738,7 @@ int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev
*sdiodev,
pktq);

 done:
+   brcmu_pkt_buf_free_skb(glom_skb);
return err;
 }

Re: BRCMFMAC OOB interrupt problem

[Arend van Spriel]

On 01-06-17 09:44, Hegr, Jiri wrote:
> Dears,
> We use small WiFi evaluation board WM-BN-BM-04_EVB_V1.2 with BCM43362 chip 
> (Broadcom).
> This board is connected to OMAP-L138 via SDIO interface with Linux 4.9.10 
> containing WiFi driver brcmfmac.
> Our problem is in OOB interrupt. The driver (and the WiFi chip) works fine if 
> the interrupt is not set in a device tree (SDIO interface is used instead).
> But if the host-wake interrupt is used, the initialization phase does not 
> pass. We are sure that the OOB interrupt is triggered. We found out that it 
> waits for a control frame (probably) but it gets some event frame only. After 
> about 2.5 seconds the timeout occurs and the initialization fails.
> Could you help us to solve the problem?
> There is our device tree settings and terminal output bellow (the "DBG" lines 
> are our own).
> 
> There is also problem with WARN_ON(nents > sdiodev->max_segment_count); in 
> bcmsdh.c file, but it seems that it does not influence the driver work 
> because the problem occurs without OOB interrupt setting also and everything 
> works fine.
> 
> Regards,
> 
> Jiri Hegr
> 
> mmc1: mmc@21b000 {
>  #address-cells = <1>;
>  #size-cells = <0>;
>  max-frequency = <2500>;
>  bus-width = <4>;
>  pinctrl-names = "default";
>  pinctrl-0 = <_pins>;
>  status = "okay";
>  non-removable;
> 
>  brcmf: bcrmf@1 {
> reg = <1>;
> compatible = "brcm,bcm4329-fmac";
> interrupt-parent = <>;
> interrupts = <95 0x02>;
> interrupt-names = "host-wake";
>  };
> };
> 
> [   24.314527] brcmfmac: *** DBG: starting datawork worker thread...
> [   24.354778] brcmfmac: *** DBG: brcmf_sdio_readframes...
> [   24.360088] brcmfmac: *** DBG: rd->len check: 0
> [   24.404232] brcmfmac: *** DBG: rd->channel check: 1
> [   24.435047] brcmfmac: *** DBG: rd->len check: 0
> [   24.454977] brcmfmac: *** DBG: brcmf_sdio_readframes - loop break
> [   27.013900] brcmfmac: brcmf_sdio_bus_rxctl: resumed on timeout
> [   27.019813] brcmfmac: brcmf_c_preinit_dcmds: Retreiving cur_etheraddr 
> failed, -110
> [   27.031282] brcmfmac: brcmf_bus_start: failed: -110
> [   27.046164] brcmfmac: brcmf_sdio_firmware_callback: dongle is not 
> responding

Can you build the driver with CONFIG_BRCMDBG and load the driver with
debugging:

$ sudo insmod brcmfmac.ko debug=0x31516

Regards,
Arend

>   Honeywell  *
> Jiri Hegr - R Scientist III.
> 
> Honeywell International s.r.o. - Aerospace Advanced Techonologies
> V Parku 2325/16
> Praha 148 00, Czech Republic
> Office: Turanka 100
> 627 00 Brno, Czech Republic
> 
> Tel: +420 532 115 564
> E-mail:jiri.he...@honeywell.com
> 
> 

Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_op_bss_info_changed

[Arend van Spriel]

On 31-05-17 12:26, Kalle Valo wrote:
> Jia-Ju Bai  writes:
> 
>> The driver may sleep under a spin lock, and the function call path is:
>> b43legacy_op_bss_info_changed (acquire the lock by spin_lock_irqsave)
>>   b43legacy_synchronize_irq
>> synchronize_irq --> may sleep
>>
>> To fix it, the lock is released before b43legacy_synchronize_irq, and the 
>> lock is acquired again after this function.
>>
>> Signed-off-by: Jia-Ju Bai 
>> ---
>>  drivers/net/wireless/broadcom/b43legacy/main.c |2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/wireless/broadcom/b43legacy/main.c 
>> b/drivers/net/wireless/broadcom/b43legacy/main.c
>> index f1e3dad..31ead21 100644
>> --- a/drivers/net/wireless/broadcom/b43legacy/main.c
>> +++ b/drivers/net/wireless/broadcom/b43legacy/main.c
>> @@ -2859,7 +2859,9 @@ static void b43legacy_op_bss_info_changed(struct 
>> ieee80211_hw *hw,
>>  b43legacy_write32(dev, B43legacy_MMIO_GEN_IRQ_MASK, 0);
>>  
>>  if (changed & BSS_CHANGED_BSSID) {
>> +spin_unlock_irqrestore(>irq_lock, flags);
>>  b43legacy_synchronize_irq(dev);
>> +spin_lock_irqsave(>irq_lock, flags);
> 
> To me this looks like a fragile workaround and not a real fix. You can
> easily add new race conditions with releasing the lock like this.

Hi Jia-Ju,

Agree with Kalle as I was about to say the same thing. You really need
to determine what is protected by the irq_lock. Here you are using the
lock because you are about to change wl->bssid a bit further down. Did
not check the entire function but it seems the lock perimeter is too wide.

Regards,
Arend

Re: 4.12-RC2 BUG: scheduling while atomic: irq/47-iwlwifi

[Arend Van Spriel]

On 23-5-2017 9:22, Johannes Berg wrote:
> On Tue, 2017-05-23 at 09:19 +0200, Arend Van Spriel wrote:
>>
>>> WARN_ON_ONCE(!rcu_read_lock_held() && !lockdep_rtnl_is_held());
>>
>> Thought about something like this after sending the email. So there
>> are two call sites. One for scheduled scan results notification and
>> one in scheduled scan stop scenario. So for the latter it is not
>> needed to use the rcu_read_lock() as it should have RTNL lock hence
>> the two checks above?
> 
> Right. The latter can't even really use rcu_read_lock() since it also
> wants to modify the list, and that's not sufficient protection for
> modifying.

Hence the name ;-)

Regards,
Arend

Re: 4.12-RC2 BUG: scheduling while atomic: irq/47-iwlwifi

[Arend Van Spriel]

On 22-5-2017 23:04, Johannes Berg wrote:
> Hi Arend,
> 
> Sorry, I forgot that the original message wasn't Cc'ed to the wireless
> list, only netdev.

That explains. Not subscribed to that.

>> +++ b/net/wireless/scan.c
>> @@ -322,9 +322,7 @@ static void cfg80211_del_sched_scan_req(struct
>> cfg80211_regi
>>  {
>> struct cfg80211_sched_scan_request *pos;
>>
>> -   ASSERT_RTNL();
>> -
>> -   list_for_each_entry(pos, >sched_scan_req_list, list) {
>> +   list_for_each_entry_rcu(pos, >sched_scan_req_list,
>> list) {
> 
> [snip]
> 
> This looks fine, but perhaps in the above we should have some kind of
> locking assertion, e.g.
> 
>   WARN_ON_ONCE(!rcu_read_lock_held() && !lockdep_rtnl_is_held());

Thought about something like this after sending the email. So there are
two call sites. One for scheduled scan results notification and one in
scheduled scan stop scenario. So for the latter it is not needed to use
the rcu_read_lock() as it should have RTNL lock hence the two checks above?

Will create a formal patch.

Regards,
Arend

Re: 4.12-RC2 BUG: scheduling while atomic: irq/47-iwlwifi

[Arend Van Spriel]

On 22-5-2017 14:09, Arend van Spriel wrote:
> On 5/22/2017 12:57 PM, Johannes Berg wrote:
>> On Mon, 2017-05-22 at 12:36 +0200, Sander Eikelenboom wrote:
>>> Hi,
>>>
>>> I encountered this splat with 4.12-RC2.
>>
>> Ugh, yeah, I should've seen that in the review.
>>
>> Arend, please take a look at this. cfg80211_sched_scan_results() cannot
>> sleep, so you can't rtnl_lock() in there. Looks like you can just rely
>> on RCU though?
> 
> I see. I think you are right on RCU. Don't have the code in front of me
> now, but I think the lookup has an ASSERT_RTNL. Will look into it after
> my monday meeting :-p

I realized I have a laptop lying around with intel 3160 wifi chip and
tried to reproduce the issue. Did not run into the splat running
4.12-rc1 from wireless-drivers-next repo. I did not get the email from
Sander so I don't know any details.

Here is what I changed based on the info Johannes provided. Can you
please check if this get rid of the splat and let me know.

Regards,
Arend
---
diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index 14d5f0c..04833bb 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -322,9 +322,7 @@ static void cfg80211_del_sched_scan_req(struct
cfg80211_regi
 {
struct cfg80211_sched_scan_request *pos;

-   ASSERT_RTNL();
-
-   list_for_each_entry(pos, >sched_scan_req_list, list) {
+   list_for_each_entry_rcu(pos, >sched_scan_req_list, list) {
if (pos->reqid == reqid)
return pos;
}
@@ -398,13 +396,13 @@ void cfg80211_sched_scan_results(struct wiphy
*wiphy, u64
trace_cfg80211_sched_scan_results(wiphy, reqid);
/* ignore if we're not scanning */

-   rtnl_lock();
+   rcu_read_lock();
request = cfg80211_find_sched_scan_req(rdev, reqid);
if (request) {
request->report_results = true;
queue_work(cfg80211_wq, >sched_scan_res_wk);
}
-   rtnl_unlock();
+   rcu_read_unlock();
 }
 EXPORT_SYMBOL(cfg80211_sched_scan_results);

Re: 4.12-RC2 BUG: scheduling while atomic: irq/47-iwlwifi

[Arend van Spriel]

On 5/22/2017 12:57 PM, Johannes Berg wrote:

On Mon, 2017-05-22 at 12:36 +0200, Sander Eikelenboom wrote:

Hi,

I encountered this splat with 4.12-RC2.


Ugh, yeah, I should've seen that in the review.

Arend, please take a look at this. cfg80211_sched_scan_results() cannot
sleep, so you can't rtnl_lock() in there. Looks like you can just rely
on RCU though?


I see. I think you are right on RCU. Don't have the code in front of me 
now, but I think the lookup has an ASSERT_RTNL. Will look into it after 
my monday meeting :-p


Regards,
Arend

Re: [PATCH 1/1] dt-binding: net: wireless: fix node name in the BCM43xx example

[Arend van Spriel]

On 5/21/2017 4:19 PM, Andreas Färber wrote:

Hi,

Am 16.05.2017 um 21:56 schrieb Martin Blumenstingl:

On Tue, May 16, 2017 at 12:05 AM, Arend Van Spriel
<arend.vanspr...@broadcom.com> wrote:

On 15-5-2017 22:13, Martin Blumenstingl wrote:

The example in the BCM43xx documentation uses "brcmf" as node name.
However, wireless devices should be named "wifi" instead. Fix this to


Since when is that a rule. I never got the memo and the DTC did not ever
complain to me about the naming.


How do you expect it to? Maintain a blacklist of every device model
someone might use, including all typo variations?


Not really why I was asking it. Just saying the node name is trivial as 
I don't think there is different kernel behaviour depending on the node 
name.



That being said I do not really care

and I suppose it is for the sake of consistency only.

I'm not sure if it's actually a rule or (as you already noted) just
for consistency. back when I added devicetree support to ath9k Rob
pointed out that the node should be named "wifi" (instead of "ath9k"),
see [0]


The general rule is that the node name should be the type of the device,
not duplicate its compatible string.

For consistency Rob was asking we use "wifi" as node name.


Fine with that. Not sure how long ago it was that I added this binding, 
but DT folks were involved back than. I never looked back so I should 
not be surprised with new consistency rules. I was just curious about 
the story behind it.


Thanks,
Arend

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Arend Van Spriel]

On 16-5-2017 1:13, Luis R. Rodriguez wrote:
> On Fri, May 12, 2017 at 11:02:26PM +0200, Arend Van Spriel wrote:
>> try again.. replacing email address from Michał
>> On 12-5-2017 22:55, Arend Van Spriel wrote:
>>> Let me explain the idea to refresh your memory (and mine). It started
>>> when we were working on adding driver support for OpenWrt in brcmfmac.
>>> The driver requests for firmware calibration data, but on routers it is
>>> stored in flash. So after failing on the firmware request we now call a
>>> platform specific API. That was my itch, but it was not bad enough to go
>>> and scratch. Now for N900 case there is a similar scenario alhtough it
>>> has additional requirement to go to user-space due to need to use a
>>> proprietary library to obtain the NVS calibration data. My thought: Why
>>> should firmware_class care?
> 
> Agreed.
> 
>>> So the idea is that firmware_class provides
>>> a registry for modules that can produce a certain firmware "file". Those
>>> modules can do whatever is needed. If they need to use umh so be it.
>>> They would only register themselves with firmware_class on platforms
>>> that need them. It would basically be replacing the fallback mechanism
>>> and only be effective on certain platforms.
> 
> Sure, so it sounds like the work that Daniel Wagner and Tom Gundersen worked
> [0] on which provides a firmwared with two modes: best-effort, and final-mode,
> would address what you are looking for but without requiring any upstream
> changes, *and* it also helps solve the rootfs race remote-proc folks had
> concerns over.
> 
> The other added gain over this solution is if folks need their own proprietary
> concoction they can just fork firmwared and have that do whatever it needs
> for the specific device on the specific rootfs. That is, firmwared can be the
> upstream solution if folks need it, but if folks need something custom they 
> can
> just mimic the implementation: best-effort, and and final-mode.
> 
> Yet another added gain over this solution we can do *not* support the
> custom fallback mechanism as its not needed, the udev event should suffice
> to let userspace do what it needs.
> 
> Lastly, if we did not want to deal with timeouts for the way the driver data
> API implements it I think we might be able to do away with them for for async
> requests if we assume there will be a daemon that spawns in final-mode 
> eventually,
> and since it *knows* when the rootfs is ready it should be able to do a final
> lookup, if it returns -ENOENT; then indeed we know we can give up. Now, 
> perhaps
> how and if we want to deal with timeouts when using the driver data API for
> the fallback mechanism is worth considering given it does not have a fallback
> mechanism support yet. If we *add* them it would seem this would also put an
> implicit race against userspace finishing initialization and running firmwared
> in final-mode.

Just to be clear. When you are saying "rootfs" in this story, you mean
any (mounted) file-system which may hold the firmware. At least that was
one of the arguments. In kernel space we can not know how the system is
setup in terms of mount points, let alone on which mounted file-system
the firmware resides.

> Johannes, do you recall the corner cases we spoke about regarding timeouts?
> Does this match what we spoke about?
> 
>>> Let me know if this idea is still of interest and I will rebase what I
>>> have for an RFC round.
> 
> Since no upstream delta is needed for firmwared I'd like to first encourage
> evaluating the above. While distributions don't carry it yet that may be seen 
> as
> an issue but since what we are looking for are corner cases, only folks 
> needing
> to deploy a specific solution would need it or a custom proprietary solution.

Ok. I will go try and run firmwared in OpenWrt on a router platform.
Have to steal one from a colleague :-p Will study firmwared.

> [0] https://github.com/teg/firmwared.git
> 
> PS.
> 
> Note that firmware signing will require an additional file, the detached
> signature. The driver data API does not currently support the fallback
> mechanism so we would not have to worry about that yet but once we add
> fallback support we'd need to consider this.

Do you have references to the firmware signing design. Is the idea to
have one signature and all "firmware files" need to be signed with it?

Thanks,
Arend

Re: [PATCH 1/1] dt-binding: net: wireless: fix node name in the BCM43xx example

[Arend Van Spriel]

On 15-5-2017 22:13, Martin Blumenstingl wrote:
> The example in the BCM43xx documentation uses "brcmf" as node name.
> However, wireless devices should be named "wifi" instead. Fix this to

Hi Martin,

Since when is that a rule. I never got the memo and the DTC did not ever
complain to me about the naming. That being said I do not really care
and I suppose it is for the sake of consistency only.

> make sure that .dts authors can simply use the documentation as
> reference (or simply copy the node from the documentation and then
> adjust only the board specific bits).

Please feel free to add my...

Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>
> Signed-off-by: Martin Blumenstingl <martin.blumensti...@googlemail.com>
> ---
>  Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git 
> a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt 
> b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> index 5dbf169cd81c..590f622188de 100644
> --- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> +++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> @@ -31,7 +31,7 @@ mmc3: mmc@01c12000 {
>   non-removable;
>   status = "okay";
>  
> - brcmf: bcrmf@1 {
> + brcmf: wifi@1 {
>   reg = <1>;
>   compatible = "brcm,bcm4329-fmac";
>   interrupt-parent = <>;
> 

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Arend Van Spriel]

On 4-5-2017 4:28, Luis R. Rodriguez wrote:
> On Wed, May 03, 2017 at 09:02:20PM +0200, Arend Van Spriel wrote:
>> On 3-1-2017 18:59, Luis R. Rodriguez wrote:
>>> On Mon, Dec 26, 2016 at 05:35:59PM +0100, Pavel Machek wrote:
>>>>
>>>> Right question is "should we solve it without user-space help"?
>>>>
>>>> Answer is no, too. Way too complex. Yes, it would be nice if hardware
>>>> was designed in such a way that getting calibration data from kernel
>>>> is easy, and if you design hardware, please design it like that. But
>>>> N900 is not designed like that and getting the calibration through
>>>> userspace looks like only reasonable solution.
>>>
>>> Arend seems to have a better alternative in mind possible for other
>>> devices which *can* probably pull of doing this easily and nicely,
>>> given the nasty history of the usermode helper crap we should not
>>> in any way discourage such efforts.
>>>
>>> Arend -- please look at the firmware cache, it not a hash but a hash
>>> table for an O(1) lookups would be a welcomed change, then it could
>>> be repurposed for what you describe, I think the only difference is
>>> you'd perhaps want a custom driver hook to fetch the calibration data
>>> so the driver does whatever it needs.
>>
>> Hi Luis,
>>
>> I let my idea catch dust on the shelf for a while. 
> 
> :) BTW did you get to check out Daniel Wagner and Tom Gundersen's firmware 
> work
> [0] ? This can provide a proper clear fallback mechanism which *also* helps
> address the race between "critical mount points ready" problem we had 
> discussed
> long ago. IIRC it did this by having two modes of operation a best effort-mode
> and a final-mode. The final-mode would only be used once all the real rootfs 
> is
> ready. Userspace decides when to kick / signal firmwared to switch to 
> final-mode
> as only userspace will know for sure when that is ready. The best-effort mode
> would be used in initramfs. There is also no need for a "custom fallback", the
> uevent fallback mechanism can be relied upon alone. Custom tools can just fork
> off and do something similar to firmward then in terms of architecture. This 
> is
> a form of fallback mechanism I think I'd be happy to see enabled on the new
> driver data API. But of course, I recall also liking what you had in mind as 
> well
> so would be happy to see more alternatives! The cleaner the better !
> 
> [0] https://github.com/teg/firmwared
> 
>> Actually had a couple
>> of patches ready, but did get around testing them. So I wanted to rebase
>> them on your linux-next tree. I bumped into the umh lock thing and was
>> wondering why direct filesystem access was under that lock as well.
> 
> Indeed, it was an odd thing.
> 
>> In your tree I noticed a fix for that. 
> 
> Yup!
> 
> It took a lot of git archeology to reach a sound approach forward which makes
> me feel comfortable without regressing the kernel, this should not regress
> the kernel.
> 
>> The more reason to base my work on
>> top of your firmware_class changes. Now my question is what is the best
>> branch to choose, because you have a "few" in that repo to choose from ;-)
> 
> I have a series of topical changes, and I rebase onto the latest linux-next
> regularly before posting patches, if 0-day finds issues, I dish out a next
> try2 or tryX iteration until all issues are fixed. So in this case you'd
> just go for the latest driver-data-$(next_date) and if there is a try
> postfix use the latest tryX.
> 
> In this case 20170501-driver-data-try2, this is based on linux-next tag
> next-20170501. If you have issues booting on that next tag though you
> could instead try the v4.11-rc8-driver-data-try3 branch based on v4.11-rc8.
> But naturally patches ultimately should be based on the latest linux-next
> tag for actual submission.
> 
> PS. after my changes the fallback mechanism can easily be shoved into its
> own file, not sure if that helps with how clean of a solution your work
> will be.

Let me explain the idea to refresh your memory (and mine). It started
when we were working on adding driver support for OpenWrt in brcmfmac.
The driver requests for firmware calibration data, but on routers it is
stored in flash. So after failing on the firmware request we now call a
platform specific API. That was my itch, but it was not bad enough to go
and scratch. Now for N900 case there is a similar scenario alhtough it
has additional requirement to go to user-space due to need to use a
proprietary library to obtain the NVS calibration data. My tho

Re: [PATCH] brcmfmac: btcoex: replace init_timer with setup_timer

[Arend van Spriel]

On 5/12/2017 10:54 AM, Kalle Valo wrote:

Arend van Spriel <arend.vanspr...@broadcom.com> writes:


On 5/12/2017 10:19 AM, Kalle Valo wrote:

Xie Qirong <cheerx1...@gmail.com> writes:


The combination of init_timer and setting up the data and function field
manually is equivalent to calling setup_timer(). This is an api
consolidation only and improves readability.

Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>
Signed-off-by: Xie Qirong <cheerx1...@gmail.com>
---

   setup_timer.cocci suggested the following improvement:
   drivers/net/wireless/broadcom/brcm80211/brcmfmac/btcoex.c:383:1-11: Use
   setup_timer function for function on line 384.

   Patch was compile checked with: x86_64_defconfig + CONFIG_BRCMFMAC=y +
   CONFIG_BRCMFMAC_USB=y + CONFIG_BRCMFMAC_PCIE=y + CONFIG_BRCM_TRACING=y +
   CONFIG_BRCMDBG=y

   Kernel version: 4.11.0 (localversion-next is next-20170512)


How is this different from the first version?

https://patchwork.kernel.org/patch/9709467/


Hi Kalle,

This is actually the third version. You are referring to the
not-specifically-named "v2" here, but how are you to know ;-)


Exactly :)


This third version is the same as v1 on which I commented to put the
coccinelle output in the commit message. So I would still keep v2 if
nothing else changed in v3 apart from my Acked-by: tag.


Ok, but I can easily take v3 (ie. this one) so that you get credit ;)


If you add the coccinelle output in the commit message, ie. above the 
'---' that would be great. So for both you have to do additional stuff 
provided you find it useful to have the coccinelle output. :-p


Regards,
Arend

Re: [PATCH] brcmfmac: btcoex: replace init_timer with setup_timer

[Arend van Spriel]

On 5/12/2017 10:19 AM, Kalle Valo wrote:

Xie Qirong <cheerx1...@gmail.com> writes:


The combination of init_timer and setting up the data and function field
manually is equivalent to calling setup_timer(). This is an api
consolidation only and improves readability.

Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>
Signed-off-by: Xie Qirong <cheerx1...@gmail.com>
---

  setup_timer.cocci suggested the following improvement:
  drivers/net/wireless/broadcom/brcm80211/brcmfmac/btcoex.c:383:1-11: Use
  setup_timer function for function on line 384.

  Patch was compile checked with: x86_64_defconfig + CONFIG_BRCMFMAC=y +
  CONFIG_BRCMFMAC_USB=y + CONFIG_BRCMFMAC_PCIE=y + CONFIG_BRCM_TRACING=y +
  CONFIG_BRCMDBG=y

  Kernel version: 4.11.0 (localversion-next is next-20170512)


How is this different from the first version?

https://patchwork.kernel.org/patch/9709467/


Hi Kalle,

This is actually the third version. You are referring to the 
not-specifically-named "v2" here, but how are you to know ;-) This third 
version is the same as v1 on which I commented to put the coccinelle 
output in the commit message. So I would still keep v2 if nothing else 
changed in v3 apart from my Acked-by: tag.


Regards,
Arend


Always add patch version "[PATCH v2]" and a changelog. I added a section
"Frequent problems" to the wiki to mention about these common problems:

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

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

Re: [PATCH] wcn36xx: Close SMD channel on device removal

[Arend van Spriel]

On 5/10/2017 1:03 AM, Bjorn Andersson wrote:

On Mon 08 May 23:17 PDT 2017, Kalle Valo wrote:


Bjorn Andersson  writes:


The SMD channel is not the primary WCNSS channel and must explicitly be
closed as the device is removed, or the channel will already by open on
a subsequent probe call in e.g. the case of reloading the kernel module.

This issue was introduced because I simplified the underlying SMD
implementation while the SMD adaptions of the driver sat on the mailing
list, but missed to update these patches. The patch does however only
apply back to the transition to rpmsg, hence the limited Fixes.

Fixes: 5052de8deff5 ("soc: qcom: smd: Transition client drivers from smd to 
rpmsg")
Reported-by: Eyal Ilsar 
Signed-off-by: Bjorn Andersson 


As this is a regression I'll queue this to 4.12.



Thanks.


But if this is an older bug (didn't quite understand your description
though) should there be a separate patch for stable releases?



AFAICT this never worked, as it seems I did the rework in SMD while we
tried to figure out the dependency issues we had with moving to SMD. So
v4.9 through v4.11 has SMD support - with this bug.

How do I proceed, do you want me to write up a fix for stable@? Do I
send that out as an ordinary patch?


If the patch applies cleanly on branches linux-4.9.y through 
linux-4.11.y in the stable repository you can go for '--- Option 1 ---' 
as described in /Documentation/stable_kernel_rules.txt.


Regards,
Arend

Re: [PATCH] net: wireless: ath: ath10k: remove unnecessary code

[Arend Van Spriel]

On 9-5-2017 7:33, Kalle Valo wrote:
> "Gustavo A. R. Silva"  writes:
> 
>> The name of an array used by itself will always return the array's address.
>> So these tests will always evaluate as false and therefore the _return_
>> will never be executed.
>>
>> Signed-off-by: Gustavo A. R. Silva 
> 
> I don't understand the commit log, especially what does "The name of an
> array used by itself" mean?

The array fields in struct wmi_start_scan_arg that are checked here are
fixed size arrays so they can never be NULL.

Maybe that helps rephrasing this commit message.

Regards,
Arend

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Arend Van Spriel]

On 3-1-2017 18:59, Luis R. Rodriguez wrote:
> On Mon, Dec 26, 2016 at 05:35:59PM +0100, Pavel Machek wrote:
>>
>> Right question is "should we solve it without user-space help"?
>>
>> Answer is no, too. Way too complex. Yes, it would be nice if hardware
>> was designed in such a way that getting calibration data from kernel
>> is easy, and if you design hardware, please design it like that. But
>> N900 is not designed like that and getting the calibration through
>> userspace looks like only reasonable solution.
> 
> Arend seems to have a better alternative in mind possible for other
> devices which *can* probably pull of doing this easily and nicely,
> given the nasty history of the usermode helper crap we should not
> in any way discourage such efforts.
> 
> Arend -- please look at the firmware cache, it not a hash but a hash
> table for an O(1) lookups would be a welcomed change, then it could
> be repurposed for what you describe, I think the only difference is
> you'd perhaps want a custom driver hook to fetch the calibration data
> so the driver does whatever it needs.

Hi Luis,

I let my idea catch dust on the shelf for a while. Actually had a couple
of patches ready, but did get around testing them. So I wanted to rebase
them on your linux-next tree. I bumped into the umh lock thing and was
wondering why direct filesystem access was under that lock as well. In
your tree I noticed a fix for that. The more reason to base my work on
top of your firmware_class changes. Now my question is what is the best
branch to choose, because you have a "few" in that repo to choose from ;-)

Regards,
Arend

Re: [PATCH] brcmfmac: btcoex: replace init_timer with setup_timer

[Arend van Spriel]

On 5/3/2017 9:35 AM, Xie Qirong wrote:

Signed-off-by: Xie Qirong <cheerx1...@gmail.com>
---

  setup_timer.cocci suggested the following improvement:
  drivers/net/wireless/broadcom/brcm80211/brcmfmac/btcoex.c:383:1-11: Use
  setup_timer function for function on line 384.


Move the text above before your sign-off so it will end up in the git 
commit message.


When done you may also add my acknowledgement, ie.:

Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>

Regards,
Arend

Re: [PATCH v2] brcmfmac: Ensure pointer correctly set if skb data location changes

[Arend Van Spriel]

On 24-4-2017 13:40, James Hughes wrote:
> The incoming skb header may be resized if header space is
> insufficient, which might change the data adddress in the skb.
> Ensure that a cached pointer to that data is correctly set by
> moving assignment to after any possible changes.

Hi Kalle,

This one should go on 4.12 queue as well.

Thanks,
Arend

> Signed-off-by: James Hughes <james.hug...@raspberrypi.org>
> 
> Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>
> ---

Re: [PATCH v2] brcmfmac: Make skb header writable before use

[Arend Van Spriel]

On 25-4-2017 9:38, Arend Van Spriel wrote:
> On 24-4-2017 15:03, James Hughes wrote:
>> The driver was making changes to the skb_header without
>> ensuring it was writable (i.e. uncloned).
>> This patch also removes some boiler plate header size
>> checking/adjustment code as that is also handled by the
>> skb_cow_header function used to make header writable.
>>
>> This patch depends on
>> brcmfmac: Ensure pointer correctly set if skb data location changes
>>
>> Signed-off-by: James Hughes <james.hug...@raspberrypi.org>
>> ---
>> Changes in v2
>>   Makes the _cow_ call at the entry point of the skb in to the
>>   stack, means only needs to be done once, and error handling
>>   is easier.
>>   Split a separate minor bug fix off to a separate patch (which
>>   this patch depends on)
> 
> Hi James,
> 
> Can you please indicate in this section that you want it applied for
> 4.12 as it is an important fix. Probably will have to wait until after
> the merge window before it can get in the wireless-drivers repo.

Hi Kalle,

I should have checked kernel mailing list first as Linus added another
rc cycle. So can this patch still go in wireless-drivers-next for 4.12
kernel. I want it for stable as well, but I will look at that when it
lands upstream.

Regards,
Arend

> Regards,
> Arend
> 
>>  .../net/wireless/broadcom/brcm80211/brcmfmac/core.c   | 19 
>> ---
>>  1 file changed, 4 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c 
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> index 9b7c19a508ac..88f8675a94c2 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> @@ -211,22 +211,11 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct 
>> sk_buff *skb,
>>  goto done;
>>  }
>>  
>> -/* Make sure there's enough room for any header */
>> -if (skb_headroom(skb) < drvr->hdrlen) {
>> -struct sk_buff *skb2;
>> -
>> -brcmf_dbg(INFO, "%s: insufficient headroom\n",
>> -  brcmf_ifname(ifp));
>> -drvr->bus_if->tx_realloc++;
>> -skb2 = skb_realloc_headroom(skb, drvr->hdrlen);
>> +/* Make sure there's enough room for any header, and make it writable */
>> +if (skb_cow_head(skb, drvr->hdrlen)) {
>>  dev_kfree_skb(skb);
>> -skb = skb2;
>> -if (skb == NULL) {
>> -brcmf_err("%s: skb_realloc_headroom failed\n",
>> -  brcmf_ifname(ifp));
>> -ret = -ENOMEM;
>> -goto done;
>> -}
>> +ret = -ENOMEM;
>> +goto done;
>>  }
>>  
>>  /* validate length for ether packet */
>>

Re: [PATCH v2] brcmfmac: Make skb header writable before use

[Arend Van Spriel]

On 24-4-2017 15:03, James Hughes wrote:
> The driver was making changes to the skb_header without
> ensuring it was writable (i.e. uncloned).
> This patch also removes some boiler plate header size
> checking/adjustment code as that is also handled by the
> skb_cow_header function used to make header writable.
> 
> This patch depends on
> brcmfmac: Ensure pointer correctly set if skb data location changes
> 
> Signed-off-by: James Hughes 
> ---
> Changes in v2
>   Makes the _cow_ call at the entry point of the skb in to the
>   stack, means only needs to be done once, and error handling
>   is easier.
>   Split a separate minor bug fix off to a separate patch (which
>   this patch depends on)

Hi James,

Can you please indicate in this section that you want it applied for
4.12 as it is an important fix. Probably will have to wait until after
the merge window before it can get in the wireless-drivers repo.

Regards,
Arend

>  .../net/wireless/broadcom/brcm80211/brcmfmac/core.c   | 19 
> ---
>  1 file changed, 4 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index 9b7c19a508ac..88f8675a94c2 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -211,22 +211,11 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct 
> sk_buff *skb,
>   goto done;
>   }
>  
> - /* Make sure there's enough room for any header */
> - if (skb_headroom(skb) < drvr->hdrlen) {
> - struct sk_buff *skb2;
> -
> - brcmf_dbg(INFO, "%s: insufficient headroom\n",
> -   brcmf_ifname(ifp));
> - drvr->bus_if->tx_realloc++;
> - skb2 = skb_realloc_headroom(skb, drvr->hdrlen);
> + /* Make sure there's enough room for any header, and make it writable */
> + if (skb_cow_head(skb, drvr->hdrlen)) {
>   dev_kfree_skb(skb);
> - skb = skb2;
> - if (skb == NULL) {
> - brcmf_err("%s: skb_realloc_headroom failed\n",
> -   brcmf_ifname(ifp));
> - ret = -ENOMEM;
> - goto done;
> - }
> + ret = -ENOMEM;
> + goto done;
>   }
>  
>   /* validate length for ether packet */
> 

Re: [PATCH v2] brcmfmac: Make skb header writable before use

[Arend Van Spriel]

On 24-4-2017 15:03, James Hughes wrote:
> The driver was making changes to the skb_header without
> ensuring it was writable (i.e. uncloned).
> This patch also removes some boiler plate header size
> checking/adjustment code as that is also handled by the
> skb_cow_header function used to make header writable.
> 
> This patch depends on
> brcmfmac: Ensure pointer correctly set if skb data location changes

Hi James,

I actually thought I was going to tackle this so I spend some time
working on it today, but I will submit that as a subsequent patch. Below
some comments but apart from that:

Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>
> Signed-off-by: James Hughes <james.hug...@raspberrypi.org>
> ---
> Changes in v2
>   Makes the _cow_ call at the entry point of the skb in to the
>   stack, means only needs to be done once, and error handling
>   is easier.
>   Split a separate minor bug fix off to a separate patch (which
>   this patch depends on)
> 
> 
> 
>  .../net/wireless/broadcom/brcm80211/brcmfmac/core.c   | 19 
> ---
>  1 file changed, 4 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index 9b7c19a508ac..88f8675a94c2 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -211,22 +211,11 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct 
> sk_buff *skb,
>   goto done;
>   }
>  
> - /* Make sure there's enough room for any header */
> - if (skb_headroom(skb) < drvr->hdrlen) {
> - struct sk_buff *skb2;
> -
> - brcmf_dbg(INFO, "%s: insufficient headroom\n",
> -   brcmf_ifname(ifp));
> - drvr->bus_if->tx_realloc++;
> - skb2 = skb_realloc_headroom(skb, drvr->hdrlen);
> + /* Make sure there's enough room for any header, and make it writable */
Please rephrase: /* Make sure there is enough writable headroom */

Just do:
ret = skb_cow_head(skb, drvr->hdrlen);
if (ret < 0) {
brcmf_err("%s: skb_cow_head failed\n",
  brcmf_ifname(ifp));
> + if (skb_cow_head(skb, drvr->hdrlen)) {
>   dev_kfree_skb(skb);
> - skb = skb2;
> - if (skb == NULL) {
> - brcmf_err("%s: skb_realloc_headroom failed\n",
> -   brcmf_ifname(ifp));
> - ret = -ENOMEM;
> - goto done;
> - }
> + ret = -ENOMEM;
... and drop this assignment.

> + goto done;
>   }
>  
>   /* validate length for ether packet */
> 

Re: [PATCH v2] brcmfmac: Make skb header writable before use

[Arend Van Spriel]

On 24-4-2017 20:09, Eric Dumazet wrote:
> On Mon, 2017-04-24 at 14:03 +0100, James Hughes wrote:
>> The driver was making changes to the skb_header without
>> ensuring it was writable (i.e. uncloned).
>> This patch also removes some boiler plate header size
>> checking/adjustment code as that is also handled by the
>> skb_cow_header function used to make header writable.
>>
>> This patch depends on
>> brcmfmac: Ensure pointer correctly set if skb data location changes
>>
>> Signed-off-by: James Hughes 
>> ---
>> Changes in v2
>>   Makes the _cow_ call at the entry point of the skb in to the
>>   stack, means only needs to be done once, and error handling
>>   is easier.
>>   Split a separate minor bug fix off to a separate patch (which
>>   this patch depends on)
> 
> Best way to handle dependencies is to send a patch series.
> 
> 1/2 brcmfmac: Ensure pointer correctly set if skb data location changes
> 2/2 brcmfmac: Make skb header writable before use

There is actually no real dependency. Both are valid fixes of course but
either one applies to wireless-drivers-next/master on its own.

Regards,
Arend

Re: [PATCH] brcmfmac: Ensure pointer correctly set if skb data location changes

[Arend van Spriel]

On 4/24/2017 12:52 PM, James Hughes wrote:

The incoming skb header may be resized if header space is
insufficient, which might change the data adddress in the skb.
Ensure that a cached pointer to that data is correctly set by
moving assignment to after any possible changes.


Thanks, James

Minor nit below...

You may add my acknowledgement:

Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>

Signed-off-by: James Hughes <james.hug...@raspberrypi.org>
---
  drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c


[...]


@@ -229,6 +229,8 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff 
*skb,
}
}
  
+	eh = (struct ethhdr *)(skb->data);

+


Please move after the length validation below.

Regards,
Arend


/* validate length for ether packet */
if (skb->len < sizeof(*eh)) {
ret = -EINVAL;

Re: [PATCH] brcm80211: brcmfmac: Ensure that incoming skb's are writable

[Arend Van Spriel]

On 21-4-2017 11:22, James Hughes wrote:
> On 20 April 2017 at 20:48, Arend van Spriel
> <arend.vanspr...@broadcom.com> wrote:
>> + linux-wireless
>>
>> On 4/20/2017 1:16 PM, James Hughes wrote:
>>>
>>> The driver was adding header information to incoming skb
>>> without ensuring the head was uncloned and hence writable.
>>>
>>> skb_cow_head has been used to ensure they are writable, however,
>>> this required some changes to error handling to ensure that
>>> if skb_cow_head failed it was not ignored.
>>>
>>> This really needs to be reviewed by someone who is more familiar
>>> with this code base to ensure any deallocation of skb's is
>>> still correct.
>>>
>>> Signed-off-by: James Hughes <james.hug...@raspberrypi.org>
>>> ---
>>>   .../wireless/broadcom/brcm80211/brcmfmac/bcdc.c| 15 --
>>>   .../wireless/broadcom/brcm80211/brcmfmac/core.c| 23 +---
>>>   .../broadcom/brcm80211/brcmfmac/fwsignal.c | 32
>>> +-
>>>   .../wireless/broadcom/brcm80211/brcmfmac/sdio.c|  7 -
>>>   4 files changed, 51 insertions(+), 26 deletions(-)
>>>
>>
>> [...]
>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>> index 5eaac13..08272e8 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>> @@ -198,7 +198,7 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct
>>> sk_buff *skb,
>>> int ret;
>>> struct brcmf_if *ifp = netdev_priv(ndev);
>>> struct brcmf_pub *drvr = ifp->drvr;
>>> -   struct ethhdr *eh = (struct ethhdr *)(skb->data);
>>> +   struct ethhdr *eh;
>>> brcmf_dbg(DATA, "Enter, bsscfgidx=%d\n", ifp->bsscfgidx);
>>>   @@ -212,23 +212,14 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct
>>> sk_buff *skb,
>>> }
>>> /* Make sure there's enough room for any header */
>>> -   if (skb_headroom(skb) < drvr->hdrlen) {
>>> -   struct sk_buff *skb2;
>>> -
>>> -   brcmf_dbg(INFO, "%s: insufficient headroom\n",
>>> - brcmf_ifname(ifp));
>>> -   drvr->bus_if->tx_realloc++;
>>> -   skb2 = skb_realloc_headroom(skb, drvr->hdrlen);
>>> -   dev_kfree_skb(skb);
>>> -   skb = skb2;
>>> -   if (skb == NULL) {
>>> -   brcmf_err("%s: skb_realloc_headroom failed\n",
>>> - brcmf_ifname(ifp));
>>> -   ret = -ENOMEM;
>>> -   goto done;
>>> -   }
>>
>>
>> What you are throwing away here is code that assures there is sufficient
>> headroom for protocol and bus layer in the tx path, because that is
>> determined by drvr->hdrlen. This is where the skb is handed to the driver so
>> if you could leave the functionality above *and* assure it is writeable that
>> would be the best solution as there is no need for all the other changes
>> down the tx path.
> 
> The skb_cow_head function takes the required headroom as a parameter
> and will ensure that there is enough space, so I don't think this code
> segment is
> required or have I misunderstood what you mean here?

I looked more into what skb_cow_head() and skb_realloc_headroom()
actually do and it seems you are right.

> Is it safe to rely on the _cow_ being done here and not further down
> in the stack?
> Or at least checked further down in the stack. Previous comments from
> another patch
> requested that the _cow_ be done close to the actual addition of the
> header. I presume
> it is unlikely/impossible that the functions that add header
> information  down the stack
> will be called without the above being done first?

It is safe. During probe sequence each layer in the driver stack
increments drvr->hdrlen with headroom it needs. So drvr->hdrlen will
indicate the total headroom needed when .start_xmit() is called. And
yes, the .start_xmit() is the single point of entry in the transmit
path. So the other locations where skb_push() is done are safe.

>>
>>> +   ret = skb_cow_head(skb, drvr->hdrlen);
>>> +   if (ret) {
>>
>>
>> So move the realloc code above here inst

Re: [PATCH] brcm80211: brcmfmac: Ensure that incoming skb's are writable

[Arend van Spriel]

+ linux-wireless

On 4/20/2017 1:16 PM, James Hughes wrote:

The driver was adding header information to incoming skb
without ensuring the head was uncloned and hence writable.

skb_cow_head has been used to ensure they are writable, however,
this required some changes to error handling to ensure that
if skb_cow_head failed it was not ignored.

This really needs to be reviewed by someone who is more familiar
with this code base to ensure any deallocation of skb's is
still correct.

Signed-off-by: James Hughes 
---
  .../wireless/broadcom/brcm80211/brcmfmac/bcdc.c| 15 --
  .../wireless/broadcom/brcm80211/brcmfmac/core.c| 23 +---
  .../broadcom/brcm80211/brcmfmac/fwsignal.c | 32 +-
  .../wireless/broadcom/brcm80211/brcmfmac/sdio.c|  7 -
  4 files changed, 51 insertions(+), 26 deletions(-)



[...]


diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 5eaac13..08272e8 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -198,7 +198,7 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff 
*skb,
int ret;
struct brcmf_if *ifp = netdev_priv(ndev);
struct brcmf_pub *drvr = ifp->drvr;
-   struct ethhdr *eh = (struct ethhdr *)(skb->data);
+   struct ethhdr *eh;
  
  	brcmf_dbg(DATA, "Enter, bsscfgidx=%d\n", ifp->bsscfgidx);
  
@@ -212,23 +212,14 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,

}
  
  	/* Make sure there's enough room for any header */

-   if (skb_headroom(skb) < drvr->hdrlen) {
-   struct sk_buff *skb2;
-
-   brcmf_dbg(INFO, "%s: insufficient headroom\n",
- brcmf_ifname(ifp));
-   drvr->bus_if->tx_realloc++;
-   skb2 = skb_realloc_headroom(skb, drvr->hdrlen);
-   dev_kfree_skb(skb);
-   skb = skb2;
-   if (skb == NULL) {
-   brcmf_err("%s: skb_realloc_headroom failed\n",
- brcmf_ifname(ifp));
-   ret = -ENOMEM;
-   goto done;
-   }


What you are throwing away here is code that assures there is sufficient 
headroom for protocol and bus layer in the tx path, because that is 
determined by drvr->hdrlen. This is where the skb is handed to the 
driver so if you could leave the functionality above *and* assure it is 
writeable that would be the best solution as there is no need for all 
the other changes down the tx path.



+   ret = skb_cow_head(skb, drvr->hdrlen);
+   if (ret) {


So move the realloc code above here instead of simply freeing the skb.


+   dev_kfree_skb_any(skb);
+   goto done;
}
  
+	eh = (struct ethhdr *)(skb->data);


Now this is actually a separate fix so I would like a separate patch for it.

I have a RPi3 sitting on my desk so how can I replicate the issue. It 
was something about broadcast/multicast traffic when using AP mode and a 
bridge, right?


Regards,
Arend

Re: [PATCH] brcm80211: brcmfmac: Ensure that incoming skb's are writable

[Arend van Spriel]

On 4/20/2017 2:09 PM, James Hughes wrote:

On 20 April 2017 at 12:31, Kalle Valo  wrote:

+ linux-wireless

James Hughes  writes:


The driver was adding header information to incoming skb
without ensuring the head was uncloned and hence writable.

skb_cow_head has been used to ensure they are writable, however,
this required some changes to error handling to ensure that
if skb_cow_head failed it was not ignored.

This really needs to be reviewed by someone who is more familiar
with this code base to ensure any deallocation of skb's is
still correct.

Signed-off-by: James Hughes 


You should also CC linux-wireless, otherwise patchwork won't see it.

--
Kalle Valo


Thanks Kalle, I wasn't subscribed to wireless, but have now done so. I
also failed to read the MAINTAINERS list correctly..

With regard to this particular patch, this is related to the recent
patches to use skb_cow_head in a number of USB net drivers to ensure
they can write headers correctly, and I suspect the same fault is
possible/likely in other drivers outside the USB net realm, as this
patch shows.

I'm not overly happy with the error handling in this patch, but that
said, the error handling over this entire driver does strike me as
suspect. Quite a few places where return codes are ignored, just in my
quick examination. So not really sure how to proceed past this patch,
if at all.


I would appreciate it if you can provide details about the code you 
consider suspect. I will respond on the patches soon.


Regards,
Arend

Re: [4.9.13] brcmf use-after-free on resume

[Arend Van Spriel]

On 27-3-2017 11:24, Arend Van Spriel wrote:
> + Johannes
> 
> On 6-3-2017 11:48, Arend Van Spriel wrote:
>> + linux-wireless
>>
>> On 6-3-2017 8:04, Daniel J Blueman wrote:
>>> When resuming from suspend with a BCM43602 on Ubuntu 16.04 with
>>> 4.9.13, we see use after free [1].
>>>
>>> We see the struct cfg80211_ops is accessed in the resume path, after
>>> it was previously freed:
>>>
>>> (gdb) list *(brcmf_cfg80211_attach+0x10b)
>>> 0x1d77b is in brcmf_cfg80211_attach
>>> (drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:6861).
>>> 6856brcmf_err("ndev is invalid\n");
>>> 6857return NULL;
>>> 6858}
>>> 6859
>>> 6860ops = kmemdup(_cfg80211_ops, sizeof(*ops), GFP_KERNEL);
>>> 6861if (!ops)
>>> 6862return NULL;
>>> 6863
>>> 6864ifp = netdev_priv(ndev);
>>> 6865#ifdef CONFIG_PM
>>>
>>> (gdb) list *(wiphy_resume+0x591)
>>> 0xb751 is in wiphy_resume (net/wireless/sysfs.c:133).
>>> 128int ret = 0;
>>> 129
>>> 130/* Age scan results with time spent in suspend */
>>> 131cfg80211_bss_age(rdev, get_seconds() - rdev->suspend_at);
>>> 132
>>> 133if (rdev->ops->resume) {
>>> 134rtnl_lock();
>>> 135if (rdev->wiphy.registered)
>>> 136ret = rdev_resume(rdev);
>>> 137rtnl_unlock();
>>>
>>> I'm unsure if this relates to the ordering of callbacks processed by
>>> dpm_run_callback.
>>
>> The problem is that our driver can not access the device as it has been
>> powered off during suspend. So in the resume we cleanup everything
>> calling wiphy_unregister() and wiphy_free(). This means the rdev in
>> wiphy_resume() above is already freed. Not sure how to handle this
>> properly. Probably we should do a proper rebind.
>>
>> Regards,
>> Arend
>>
>>> Thanks,
>>>   Daniel
>>>
>>> -- [1]
>>>
>>> BUG: KASAN: use-after-free in wiphy_resume+0x591/0x5a0 [cfg80211] at
>>> addr 8803fefebb30
>>> Read of size 8 by task kworker/u16:15/3066
>>> CPU: 0 PID: 3066 Comm: kworker/u16:15 Not tainted 4.9.13-debug+ #7
>>> Hardware name: Dell Inc. XPS 15 9550/0N7TVV, BIOS 1.2.19 12/22/2016
>>> Workqueue: events_unbound async_run_entry_fn
>>>  8803bffdf9d8 880db6e1 88042740ef00 8803fefebb28
>>>  8803bffdfa00 87a4d941 8803bffdfa98 8803fefebb20
>>>  88042740ef00 8803bffdfa88 87a4dbda 8803fb132360
>>> Call Trace:
>>>  [] dump_stack+0x85/0xc4
>>>  [] kasan_object_err+0x21/0x70
>>>  [] kasan_report_error+0x1fa/0x500
>>>  [] ? trace_hardirqs_on_caller+0x3fe/0x580
>>>  [] ? cfg80211_bss_age+0x9a/0xc0 [cfg80211]
>>>  [] ? trace_hardirqs_on+0xd/0x10
>>>  [] ? wiphy_suspend+0xc70/0xc70 [cfg80211]
>>>  [] __asan_report_load8_noabort+0x61/0x70
>>>  [] ? wiphy_suspend+0xbb0/0xc70 [cfg80211]
>>>  [] ? wiphy_resume+0x591/0x5a0 [cfg80211]
>>>  [] wiphy_resume+0x591/0x5a0 [cfg80211]
>>>  [] ? wiphy_suspend+0xc70/0xc70 [cfg80211]
>>>  [] dpm_run_callback+0x6e/0x4f0
>>>  [] device_resume+0x1c2/0x670
>>>  [] async_resume+0x1d/0x50
>>>  [] async_run_entry_fn+0xfe/0x610
>>>  [] process_one_work+0x716/0x1a50
>>>  [] ? process_one_work+0x679/0x1a50
>>>  [] ? _raw_spin_unlock_irq+0x3d/0x60
>>>  [] ? pwq_dec_nr_in_flight+0x2b0/0x2b0
>>>  [] worker_thread+0xe0/0x1460
>>>  [] ? process_one_work+0x1a50/0x1a50
>>>  [] kthread+0x222/0x2e0
>>>  [] ? kthread_park+0x80/0x80
>>>  [] ? kthread_park+0x80/0x80
>>>  [] ? kthread_park+0x80/0x80
>>>  [] ret_from_fork+0x2a/0x40
> 
> So the wiphy instance is unregistered *and* freed in our
> brcmf_pcie_resume() callback. However, the wiphy_resume() for this
> instance still comes right after that. I tried several fixes using API
> calls into driver core, but to no avail. So I came up with the fix
> below.
> 
> Regards,
> Arend
> ---
resending with proper indentation (I hope).
---
diff --git a/net/wireless/core.c b/net/wireless/core.c
index d888613..3af9c2e 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -1096,6 +1096,17 @@ void cfg80211_stop_iface(struct wiphy *wiphy,
struct wireless_dev *wdev,
 }
 EXPORT_SYMBOL(cfg80211_stop_iface);

+bool cfg80211_dev_in_list_rtnl(struct cfg8

Re: [4.9.13] brcmf use-after-free on resume

[Arend Van Spriel]

+ Johannes

On 6-3-2017 11:48, Arend Van Spriel wrote:
> + linux-wireless
>
> On 6-3-2017 8:04, Daniel J Blueman wrote:
>> When resuming from suspend with a BCM43602 on Ubuntu 16.04 with
>> 4.9.13, we see use after free [1].
>>
>> We see the struct cfg80211_ops is accessed in the resume path, after
>> it was previously freed:
>>
>> (gdb) list *(brcmf_cfg80211_attach+0x10b)
>> 0x1d77b is in brcmf_cfg80211_attach
>> (drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:6861).
>> 6856brcmf_err("ndev is invalid\n");
>> 6857return NULL;
>> 6858}
>> 6859
>> 6860ops = kmemdup(_cfg80211_ops, sizeof(*ops), GFP_KERNEL);
>> 6861if (!ops)
>> 6862return NULL;
>> 6863
>> 6864ifp = netdev_priv(ndev);
>> 6865#ifdef CONFIG_PM
>>
>> (gdb) list *(wiphy_resume+0x591)
>> 0xb751 is in wiphy_resume (net/wireless/sysfs.c:133).
>> 128int ret = 0;
>> 129
>> 130/* Age scan results with time spent in suspend */
>> 131cfg80211_bss_age(rdev, get_seconds() - rdev->suspend_at);
>> 132
>> 133if (rdev->ops->resume) {
>> 134rtnl_lock();
>> 135if (rdev->wiphy.registered)
>> 136ret = rdev_resume(rdev);
>> 137rtnl_unlock();
>>
>> I'm unsure if this relates to the ordering of callbacks processed by
>> dpm_run_callback.
>
> The problem is that our driver can not access the device as it has been
> powered off during suspend. So in the resume we cleanup everything
> calling wiphy_unregister() and wiphy_free(). This means the rdev in
> wiphy_resume() above is already freed. Not sure how to handle this
> properly. Probably we should do a proper rebind.
>
> Regards,
> Arend
>
>> Thanks,
>>   Daniel
>>
>> -- [1]
>>
>> BUG: KASAN: use-after-free in wiphy_resume+0x591/0x5a0 [cfg80211] at
>> addr 8803fefebb30
>> Read of size 8 by task kworker/u16:15/3066
>> CPU: 0 PID: 3066 Comm: kworker/u16:15 Not tainted 4.9.13-debug+ #7
>> Hardware name: Dell Inc. XPS 15 9550/0N7TVV, BIOS 1.2.19 12/22/2016
>> Workqueue: events_unbound async_run_entry_fn
>>  8803bffdf9d8 880db6e1 88042740ef00 8803fefebb28
>>  8803bffdfa00 87a4d941 8803bffdfa98 8803fefebb20
>>  88042740ef00 8803bffdfa88 87a4dbda 8803fb132360
>> Call Trace:
>>  [] dump_stack+0x85/0xc4
>>  [] kasan_object_err+0x21/0x70
>>  [] kasan_report_error+0x1fa/0x500
>>  [] ? trace_hardirqs_on_caller+0x3fe/0x580
>>  [] ? cfg80211_bss_age+0x9a/0xc0 [cfg80211]
>>  [] ? trace_hardirqs_on+0xd/0x10
>>  [] ? wiphy_suspend+0xc70/0xc70 [cfg80211]
>>  [] __asan_report_load8_noabort+0x61/0x70
>>  [] ? wiphy_suspend+0xbb0/0xc70 [cfg80211]
>>  [] ? wiphy_resume+0x591/0x5a0 [cfg80211]
>>  [] wiphy_resume+0x591/0x5a0 [cfg80211]
>>  [] ? wiphy_suspend+0xc70/0xc70 [cfg80211]
>>  [] dpm_run_callback+0x6e/0x4f0
>>  [] device_resume+0x1c2/0x670
>>  [] async_resume+0x1d/0x50
>>  [] async_run_entry_fn+0xfe/0x610
>>  [] process_one_work+0x716/0x1a50
>>  [] ? process_one_work+0x679/0x1a50
>>  [] ? _raw_spin_unlock_irq+0x3d/0x60
>>  [] ? pwq_dec_nr_in_flight+0x2b0/0x2b0
>>  [] worker_thread+0xe0/0x1460
>>  [] ? process_one_work+0x1a50/0x1a50
>>  [] kthread+0x222/0x2e0
>>  [] ? kthread_park+0x80/0x80
>>  [] ? kthread_park+0x80/0x80
>>  [] ? kthread_park+0x80/0x80
>>  [] ret_from_fork+0x2a/0x40

So the wiphy instance is unregistered *and* freed in our
brcmf_pcie_resume() callback. However, the wiphy_resume() for this
instance still comes right after that. I tried several fixes using API
calls into driver core, but to no avail. So I came up with the fix
below.

Regards,
Arend
---
Sorry about the whitespace issues below (if any). My favorite mail
client went haywire and I had to use gmail web interface and it looks
like tabs are gone.
---
diff --git a/net/wireless/core.c b/net/wireless/core.c
index d888613..3af9c2e 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -1096,6 +1096,17 @@ void cfg80211_stop_iface(struct wiphy *wiphy,
struct wireless_dev *wdev,
 }
 EXPORT_SYMBOL(cfg80211_stop_iface);

+bool cfg80211_dev_in_list_rtnl(struct cfg80211_registered_device *rdev)
+{
+ struct cfg80211_registered_device *item;
+
+ list_for_each_entry(item, _rdev_list, list) {
+ if (item == rdev)
+ return true;
+ }
+ return false;
+}
+
 static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
  unsigned long state, void *ptr)
 {
diff --git a/net/wireless/core.h b/net/wireless/core.h
index e9afb

Re: [PATCH] 4.9.13 brcmfmac: fix use-after-free on resume

[Arend Van Spriel]

On 13-3-2017 10:33, Arend Van Spriel wrote:
> On 6-3-2017 15:50, Daniel J Blueman wrote:
>> On 6 March 2017 at 21:00, Arend Van Spriel <arend.vanspr...@broadcom.com> 
>> wrote:
>>> + linux-wireless
>>>
>>> On 6-3-2017 8:14, Daniel J Blueman wrote:
>>>> KASAN reported 'struct wireless_dev wdev' was read after being freed.
>>>> Fix by freeing after the access.
>>>
>>> I would rather like to see the KASAN report, because something is off
>>> here. This function is called with wdev as a parameter so how can it be
>>> accessed after free here? brcmf_remove_interface() does not free the
>>> wdev nor the brcmf_cfg80211_vif instance which contains the wdev.
>>>
>>> Regards,
>>> Arend
>>>
>>>> Signed-off-by: Daniel J Blueman <dan...@quora.org>
>>>>
>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
>>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
>>>> index de19c7c..aa0f470 100644
>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
>>>> @@ -2288,12 +2288,13 @@ int brcmf_p2p_del_vif(struct wiphy *wiphy,
>>>> struct wireless_dev *wdev)
>>>> else
>>>> err = 0;
>>>> }
>>>> -   brcmf_remove_interface(vif->ifp, true);
>>>>
>>>> -   brcmf_cfg80211_arm_vif_event(cfg, NULL);
>>>> if (vif->wdev.iftype != NL80211_IFTYPE_P2P_DEVICE)
>>>> p2p->bss_idx[P2PAPI_BSSCFG_CONNECTION].vif = NULL;
>>>>
>>>> +   brcmf_remove_interface(vif->ifp, true);
>>>> +   brcmf_cfg80211_arm_vif_event(cfg, NULL);
>>>> +
>>>> return err;
>>>>  }
>>
>> Sure, https://quora.org/kernel/brcmfmac/dmesg.txt
> 
> So this dmesg looks to be about a separate issue during resume, which
> you sent an email about earlier [1] which is about using wiphy struct
> after free. However, back to this I further checked the code and it
> seems the vif is indeed freed through the netdev destructor callback
> upon calling brcmf_remove_inteface(). However, I would prefer to store
> vif->wdev.iftype in a local variable and avoid changing the order of the
> calls in the code.

Forgot to mention some things about the commit message. Please drop the
the kernel version from the subject. You should create the patch against
the wireless-drivers tree and indicate the patch is for 4.11, ie.
"[PATCH for-4.11] brcmfmac: fix use-after-free of wireless_dev
instance". Also add as much useful information in the commit message as
possible like the proper KASAN report or at least mention that
brcmf_remove_interface() does an unregister_netdev() which results in
freeing the wireless_dev through netdev destructor callback.

Regards,
Arend

> Regards,
> Arend
> 
> [1] http://marc.info/?l=linux-netdev=148878437808750=2
> 
>> vmlinux, cfg80211.o, brcmfmac.o and config are in the same path; this
>> is against v4.9.13 stock.
> 

Re: [PATCH] 4.9.13 brcmfmac: fix use-after-free on resume

[Arend Van Spriel]

On 6-3-2017 15:50, Daniel J Blueman wrote:
> On 6 March 2017 at 21:00, Arend Van Spriel <arend.vanspr...@broadcom.com> 
> wrote:
>> + linux-wireless
>>
>> On 6-3-2017 8:14, Daniel J Blueman wrote:
>>> KASAN reported 'struct wireless_dev wdev' was read after being freed.
>>> Fix by freeing after the access.
>>
>> I would rather like to see the KASAN report, because something is off
>> here. This function is called with wdev as a parameter so how can it be
>> accessed after free here? brcmf_remove_interface() does not free the
>> wdev nor the brcmf_cfg80211_vif instance which contains the wdev.
>>
>> Regards,
>> Arend
>>
>>> Signed-off-by: Daniel J Blueman <dan...@quora.org>
>>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
>>> index de19c7c..aa0f470 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
>>> @@ -2288,12 +2288,13 @@ int brcmf_p2p_del_vif(struct wiphy *wiphy,
>>> struct wireless_dev *wdev)
>>> else
>>> err = 0;
>>> }
>>> -   brcmf_remove_interface(vif->ifp, true);
>>>
>>> -   brcmf_cfg80211_arm_vif_event(cfg, NULL);
>>> if (vif->wdev.iftype != NL80211_IFTYPE_P2P_DEVICE)
>>> p2p->bss_idx[P2PAPI_BSSCFG_CONNECTION].vif = NULL;
>>>
>>> +   brcmf_remove_interface(vif->ifp, true);
>>> +   brcmf_cfg80211_arm_vif_event(cfg, NULL);
>>> +
>>> return err;
>>>  }
> 
> Sure, https://quora.org/kernel/brcmfmac/dmesg.txt

So this dmesg looks to be about a separate issue during resume, which
you sent an email about earlier [1] which is about using wiphy struct
after free. However, back to this I further checked the code and it
seems the vif is indeed freed through the netdev destructor callback
upon calling brcmf_remove_inteface(). However, I would prefer to store
vif->wdev.iftype in a local variable and avoid changing the order of the
calls in the code.

Regards,
Arend

[1] http://marc.info/?l=linux-netdev=148878437808750=2

> vmlinux, cfg80211.o, brcmfmac.o and config are in the same path; this
> is against v4.9.13 stock.

Re: [PATCH 08/26] brcmsmac: make some local variables 'static const' to reduce stack size

[Arend Van Spriel]

On 7-3-2017 10:44, Kalle Valo wrote:
> Arnd Bergmann <a...@arndb.de> writes:
> 
>> On Mon, Mar 6, 2017 at 5:19 PM, Kalle Valo <kv...@codeaurora.org> wrote:
>>> Arend Van Spriel <arend.vanspr...@broadcom.com> writes:
>>>
>>>> On 2-3-2017 17:38, Arnd Bergmann wrote:
>>>>> With KASAN and a couple of other patches applied, this driver is one
>>>>> of the few remaining ones that actually use more than 2048 bytes of
>>>>> kernel stack:
>>>>>
>>>>> broadcom/brcm80211/brcmsmac/phy/phy_n.c: In function
>>>>> 'wlc_phy_workarounds_nphy_gainctrl':
>>>>> broadcom/brcm80211/brcmsmac/phy/phy_n.c:16065:1: warning: the
>>>>> frame size of 3264 bytes is larger than 2048 bytes
>>>>> [-Wframe-larger-than=]
>>>>> broadcom/brcm80211/brcmsmac/phy/phy_n.c: In function 
>>>>> 'wlc_phy_workarounds_nphy':
>>>>> broadcom/brcm80211/brcmsmac/phy/phy_n.c:17138:1: warning: the
>>>>> frame size of 2864 bytes is larger than 2048 bytes
>>>>> [-Wframe-larger-than=]
>>>>>
>>>>> Here, I'm reducing the stack size by marking as many local variables as
>>>>> 'static const' as I can without changing the actual code.
>>>>
>>>> Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>
>>>
>>> Arnd, via which tree are you planning to submit these? I'm not sure
>>> what I should do with the wireless drivers patches from this series.
>>
>> I'm not quite sure myself yet. I'd probably want the first few patches that
>> do most of the work get merged through Andrew's linux-mm tree once
>> we have come to agreement on them. The driver specific patches like
>> the brcmsmac ones depend on the introduction of noinline_for_kasan
>> or noinline_if_stackbloat and could either go in along with the first
>> set, or as a follow-up through the normal maintainer trees.
> 
> Either way is fine for me. Just mark clearly if you want the wireless
> drivers patches to go through via my tree, otherwise I'll ignore them.

That (dreaded) phy code does not get a lot of changes so I think it does
not matter which tree is will go through in terms of risk for conflicts.
So going through linux-mm is fine for me as well.

Regards,
Arend

Re: [PATCH] 4.9.13 brcmfmac: fix use-after-free on resume

[Arend Van Spriel]

+ linux-wireless

On 6-3-2017 8:14, Daniel J Blueman wrote:
> KASAN reported 'struct wireless_dev wdev' was read after being freed.
> Fix by freeing after the access.

I would rather like to see the KASAN report, because something is off
here. This function is called with wdev as a parameter so how can it be
accessed after free here? brcmf_remove_interface() does not free the
wdev nor the brcmf_cfg80211_vif instance which contains the wdev.

Regards,
Arend

> Signed-off-by: Daniel J Blueman 
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
> index de19c7c..aa0f470 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
> @@ -2288,12 +2288,13 @@ int brcmf_p2p_del_vif(struct wiphy *wiphy,
> struct wireless_dev *wdev)
> else
> err = 0;
> }
> -   brcmf_remove_interface(vif->ifp, true);
> 
> -   brcmf_cfg80211_arm_vif_event(cfg, NULL);
> if (vif->wdev.iftype != NL80211_IFTYPE_P2P_DEVICE)
> p2p->bss_idx[P2PAPI_BSSCFG_CONNECTION].vif = NULL;
> 
> +   brcmf_remove_interface(vif->ifp, true);
> +   brcmf_cfg80211_arm_vif_event(cfg, NULL);
> +
> return err;
>  }
> 

Re: [4.9.13] brcmf use-after-free on resume

[Arend Van Spriel]

+ linux-wireless

On 6-3-2017 8:04, Daniel J Blueman wrote:
> When resuming from suspend with a BCM43602 on Ubuntu 16.04 with
> 4.9.13, we see use after free [1].
> 
> We see the struct cfg80211_ops is accessed in the resume path, after
> it was previously freed:
> 
> (gdb) list *(brcmf_cfg80211_attach+0x10b)
> 0x1d77b is in brcmf_cfg80211_attach
> (drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:6861).
> 6856brcmf_err("ndev is invalid\n");
> 6857return NULL;
> 6858}
> 6859
> 6860ops = kmemdup(_cfg80211_ops, sizeof(*ops), GFP_KERNEL);
> 6861if (!ops)
> 6862return NULL;
> 6863
> 6864ifp = netdev_priv(ndev);
> 6865#ifdef CONFIG_PM
> 
> (gdb) list *(wiphy_resume+0x591)
> 0xb751 is in wiphy_resume (net/wireless/sysfs.c:133).
> 128int ret = 0;
> 129
> 130/* Age scan results with time spent in suspend */
> 131cfg80211_bss_age(rdev, get_seconds() - rdev->suspend_at);
> 132
> 133if (rdev->ops->resume) {
> 134rtnl_lock();
> 135if (rdev->wiphy.registered)
> 136ret = rdev_resume(rdev);
> 137rtnl_unlock();
> 
> I'm unsure if this relates to the ordering of callbacks processed by
> dpm_run_callback.

The problem is that our driver can not access the device as it has been
powered off during suspend. So in the resume we cleanup everything
calling wiphy_unregister() and wiphy_free(). This means the rdev in
wiphy_resume() above is already freed. Not sure how to handle this
properly. Probably we should do a proper rebind.

Regards,
Arend

> Thanks,
>   Daniel
> 
> -- [1]
> 
> BUG: KASAN: use-after-free in wiphy_resume+0x591/0x5a0 [cfg80211] at
> addr 8803fefebb30
> Read of size 8 by task kworker/u16:15/3066
> CPU: 0 PID: 3066 Comm: kworker/u16:15 Not tainted 4.9.13-debug+ #7
> Hardware name: Dell Inc. XPS 15 9550/0N7TVV, BIOS 1.2.19 12/22/2016
> Workqueue: events_unbound async_run_entry_fn
>  8803bffdf9d8 880db6e1 88042740ef00 8803fefebb28
>  8803bffdfa00 87a4d941 8803bffdfa98 8803fefebb20
>  88042740ef00 8803bffdfa88 87a4dbda 8803fb132360
> Call Trace:
>  [] dump_stack+0x85/0xc4
>  [] kasan_object_err+0x21/0x70
>  [] kasan_report_error+0x1fa/0x500
>  [] ? trace_hardirqs_on_caller+0x3fe/0x580
>  [] ? cfg80211_bss_age+0x9a/0xc0 [cfg80211]
>  [] ? trace_hardirqs_on+0xd/0x10
>  [] ? wiphy_suspend+0xc70/0xc70 [cfg80211]
>  [] __asan_report_load8_noabort+0x61/0x70
>  [] ? wiphy_suspend+0xbb0/0xc70 [cfg80211]
>  [] ? wiphy_resume+0x591/0x5a0 [cfg80211]
>  [] wiphy_resume+0x591/0x5a0 [cfg80211]
>  [] ? wiphy_suspend+0xc70/0xc70 [cfg80211]
>  [] dpm_run_callback+0x6e/0x4f0
>  [] device_resume+0x1c2/0x670
>  [] async_resume+0x1d/0x50
>  [] async_run_entry_fn+0xfe/0x610
>  [] process_one_work+0x716/0x1a50
>  [] ? process_one_work+0x679/0x1a50
>  [] ? _raw_spin_unlock_irq+0x3d/0x60
>  [] ? pwq_dec_nr_in_flight+0x2b0/0x2b0
>  [] worker_thread+0xe0/0x1460
>  [] ? process_one_work+0x1a50/0x1a50
>  [] kthread+0x222/0x2e0
>  [] ? kthread_park+0x80/0x80
>  [] ? kthread_park+0x80/0x80
>  [] ? kthread_park+0x80/0x80
>  [] ret_from_fork+0x2a/0x40
> Object at 8803fefebb28, in cache kmalloc-1024 size: 1024
> Allocated:
> PID = 431
>  save_stack_trace+0x1b/0x20
>  save_stack+0x46/0xd0
>  kasan_kmalloc+0xad/0xe0
>  kasan_slab_alloc+0x12/0x20
>  __kmalloc_track_caller+0x134/0x360
>  kmemdup+0x20/0x50
>  brcmf_cfg80211_attach+0x10b/0x3a90 [brcmfmac]
>  brcmf_bus_start+0x19a/0x9a0 [brcmfmac]
>  brcmf_pcie_setup+0x1f1a/0x3680 [brcmfmac]
>  brcmf_fw_request_nvram_done+0x44c/0x11b0 [brcmfmac]
>  request_firmware_work_func+0x135/0x280
>  process_one_work+0x716/0x1a50
>  worker_thread+0xe0/0x1460
>  kthread+0x222/0x2e0
>  ret_from_fork+0x2a/0x40
> Freed:
> PID = 3101
>  save_stack_trace+0x1b/0x20
>  save_stack+0x46/0xd0
>  kasan_slab_free+0x71/0xb0
>  kfree+0xe8/0x2e0
>  brcmf_cfg80211_detach+0x62/0xf0 [brcmfmac]
>  brcmf_detach+0x14a/0x2b0 [brcmfmac]
>  brcmf_pcie_remove+0x140/0x5d0 [brcmfmac]
>  brcmf_pcie_pm_leave_D3+0x198/0x2e0 [brcmfmac]
>  pci_pm_resume+0x186/0x220
>  dpm_run_callback+0x6e/0x4f0
>  device_resume+0x1c2/0x670
>  async_resume+0x1d/0x50
>  async_run_entry_fn+0xfe/0x610
>  process_one_work+0x716/0x1a50
>  worker_thread+0xe0/0x1460
>  kthread+0x222/0x2e0
>  ret_from_fork+0x2a/0x40
> Memory state around the buggy address:
>  8803fefeba00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>  8803fefeba80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>> 8803fefebb00: fc fc fc fc fc fb fb fb fb fb fb fb fb fb fb fb
>^
>  8803fefebb80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>  8803fefebc00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> 

Re: [PATCH 07/26] brcmsmac: reduce stack size with KASAN

[Arend Van Spriel]

On 6-3-2017 11:38, Arnd Bergmann wrote:
> On Mon, Mar 6, 2017 at 10:16 AM, Arend Van Spriel
> <arend.vanspr...@broadcom.com> wrote:
>> On 2-3-2017 17:38, Arnd Bergmann wrote:
>>> The wlc_phy_table_write_nphy/wlc_phy_table_read_nphy functions always put 
>>> an object
>>> on the stack, which will each require a redzone with KASAN and lead to 
>>> possible
>>> stack overflow:
>>>
>>> drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c: In function 
>>> 'wlc_phy_workarounds_nphy':
>>> drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c:17135:1: 
>>> warning: the frame size of 6312 bytes is larger than 1000 bytes 
>>> [-Wframe-larger-than=]
>>
>> Looks like this warning text ended up in the wrong commit message. Got
>> me confused for a sec :-p
> 
> What's wrong about the warning?

The warning is about the function 'wlc_phy_workarounds_nphy' (see PATCH
9/26) and not about wlc_phy_table_write_nphy/wlc_phy_table_read_nphy
functions.

>>> This marks the two functions as noinline_for_kasan, avoiding the problem 
>>> entirely.
>>
>> Frankly I seriously dislike annotating code for the sake of some
>> (dynamic) memory analyzer. To me the whole thing seems rather
>> unnecessary. If the code passes the 2048 stack limit without KASAN it
>> would seem the limit with KASAN should be such that no warning is given.
>> I suspect that it is rather difficult to predict the additional size of
>> the instrumentation code and on some systems there might be a real issue
>> with increased stack usage.
> 
> The frame sizes don't normally change that much. There are a couple of
> drivers like brcmsmac that repeatedly call an inline function which has
> a local variable that it passes by reference to an extern function.
> 
> While normally those variables share a stack location, KASAN forces
> each instance to its own location and adds (in this case) 80 bytes of
> redzone around it to detect out-of-bounds access.
> 
> While most drivers are fine with a 1500 byte warning limit, increasing
> the limit to 7kb would silence brcmsmac (unless more registers
> are accessed from wlc_phy_workarounds_nphy) but also risk a
> stack overflow to go unnoticed.

Given the amount of local variables maybe just tag the functions with
noinline instead.

Regards,
Arend

Re: [PATCH 10/26] brcmsmac: reindent split functions

[Arend Van Spriel]

On 2-3-2017 17:38, Arnd Bergmann wrote:
> In the previous commit I left the indentation alone to help reviewing
> the patch, this one now runs the three new functions through 'indent -kr -8'
> with some manual fixups to avoid silliness.
> 
> No changes other than whitespace are intended here.

Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>
> Signed-off-by: Arnd Bergmann <a...@arndb.de>
> ---
>  .../broadcom/brcm80211/brcmsmac/phy/phy_n.c| 1507 
> +---
>  1 file changed, 697 insertions(+), 810 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
> index d76c092bb6b4..9b39789c673d 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
> @@ -16074,7 +16074,8 @@ static void wlc_phy_workarounds_nphy_rev7(struct 
> brcms_phy *pi)
>   NPHY_REV3_RFSEQ_CMD_INT_PA_PU,
>   NPHY_REV3_RFSEQ_CMD_END
>   };
> - static const u8 rfseq_rx2tx_dlys_rev3_ipa[] = { 8, 6, 6, 4, 4, 16, 43, 
> 1, 1 };
> + static const u8 rfseq_rx2tx_dlys_rev3_ipa[] =
> + { 8, 6, 6, 4, 4, 16, 43, 1, 1 };
>   static const u16 rfseq_rx2tx_dacbufpu_rev7[] = { 0x10f, 0x10f };
>   u32 leg_data_weights;
>   u8 chan_freq_range = 0;
> @@ -16114,526 +16115,452 @@ static void wlc_phy_workarounds_nphy_rev7(struct 
> brcms_phy *pi)
>   int coreNum;
>  
>  
> - if (NREV_IS(pi->pubpi.phy_rev, 7)) {
> - mod_phy_reg(pi, 0x221, (0x1 << 4), (1 << 4));
> -
> - mod_phy_reg(pi, 0x160, (0x7f << 0), (32 << 0));
> - mod_phy_reg(pi, 0x160, (0x7f << 8), (39 << 8));
> - mod_phy_reg(pi, 0x161, (0x7f << 0), (46 << 0));
> - mod_phy_reg(pi, 0x161, (0x7f << 8), (51 << 8));
> - mod_phy_reg(pi, 0x162, (0x7f << 0), (55 << 0));
> - mod_phy_reg(pi, 0x162, (0x7f << 8), (58 << 8));
> - mod_phy_reg(pi, 0x163, (0x7f << 0), (60 << 0));
> - mod_phy_reg(pi, 0x163, (0x7f << 8), (62 << 8));
> - mod_phy_reg(pi, 0x164, (0x7f << 0), (62 << 0));
> - mod_phy_reg(pi, 0x164, (0x7f << 8), (63 << 8));
> - mod_phy_reg(pi, 0x165, (0x7f << 0), (63 << 0));
> - mod_phy_reg(pi, 0x165, (0x7f << 8), (64 << 8));
> - mod_phy_reg(pi, 0x166, (0x7f << 0), (64 << 0));
> - mod_phy_reg(pi, 0x166, (0x7f << 8), (64 << 8));
> - mod_phy_reg(pi, 0x167, (0x7f << 0), (64 << 0));
> - mod_phy_reg(pi, 0x167, (0x7f << 8), (64 << 8));
> - }
> -
> - if (NREV_LE(pi->pubpi.phy_rev, 8)) {
> - write_phy_reg(pi, 0x23f, 0x1b0);
> - write_phy_reg(pi, 0x240, 0x1b0);
> - }
> + if (NREV_IS(pi->pubpi.phy_rev, 7)) {
> + mod_phy_reg(pi, 0x221, (0x1 << 4), (1 << 4));
> +
> + mod_phy_reg(pi, 0x160, (0x7f << 0), (32 << 0));
> + mod_phy_reg(pi, 0x160, (0x7f << 8), (39 << 8));
> + mod_phy_reg(pi, 0x161, (0x7f << 0), (46 << 0));
> + mod_phy_reg(pi, 0x161, (0x7f << 8), (51 << 8));
> + mod_phy_reg(pi, 0x162, (0x7f << 0), (55 << 0));
> + mod_phy_reg(pi, 0x162, (0x7f << 8), (58 << 8));
> + mod_phy_reg(pi, 0x163, (0x7f << 0), (60 << 0));
> + mod_phy_reg(pi, 0x163, (0x7f << 8), (62 << 8));
> + mod_phy_reg(pi, 0x164, (0x7f << 0), (62 << 0));
> + mod_phy_reg(pi, 0x164, (0x7f << 8), (63 << 8));
> + mod_phy_reg(pi, 0x165, (0x7f << 0), (63 << 0));
> + mod_phy_reg(pi, 0x165, (0x7f << 8), (64 << 8));
> + mod_phy_reg(pi, 0x166, (0x7f << 0), (64 << 0));
> + mod_phy_reg(pi, 0x166, (0x7f << 8), (64 << 8));
> + mod_phy_reg(pi, 0x167, (0x7f << 0), (64 << 0));
> + mod_phy_reg(pi, 0x167, (0x7f << 8), (64 << 8));
> + }
>  
> - if (NREV_GE(pi->pubpi.phy_rev, 8))
> - mod_phy_reg(pi, 0xbd, (0xff << 0), (114 << 0));
> + if (NREV_L

Re: [PATCH 08/26] brcmsmac: make some local variables 'static const' to reduce stack size

[Arend Van Spriel]

On 2-3-2017 17:38, Arnd Bergmann wrote:
> With KASAN and a couple of other patches applied, this driver is one
> of the few remaining ones that actually use more than 2048 bytes of
> kernel stack:
> 
> broadcom/brcm80211/brcmsmac/phy/phy_n.c: In function 
> 'wlc_phy_workarounds_nphy_gainctrl':
> broadcom/brcm80211/brcmsmac/phy/phy_n.c:16065:1: warning: the frame size of 
> 3264 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> broadcom/brcm80211/brcmsmac/phy/phy_n.c: In function 
> 'wlc_phy_workarounds_nphy':
> broadcom/brcm80211/brcmsmac/phy/phy_n.c:17138:1: warning: the frame size of 
> 2864 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> 
> Here, I'm reducing the stack size by marking as many local variables as
> 'static const' as I can without changing the actual code.

Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>
> Signed-off-by: Arnd Bergmann <a...@arndb.de>
> ---
>  .../broadcom/brcm80211/brcmsmac/phy/phy_n.c| 197 
> ++---
>  1 file changed, 97 insertions(+), 100 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
> index 42dc8e1f483d..48a4df488d75 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
> @@ -14764,8 +14764,8 @@ static void 
> wlc_phy_ipa_restore_tx_digi_filts_nphy(struct brcms_phy *pi)
>  }
>  
>  static void
> -wlc_phy_set_rfseq_nphy(struct brcms_phy *pi, u8 cmd, u8 *events, u8 *dlys,
> -u8 len)
> +wlc_phy_set_rfseq_nphy(struct brcms_phy *pi, u8 cmd, const u8 *events,
> +const u8 *dlys, u8 len)
>  {
>   u32 t1_offset, t2_offset;
>   u8 ctr;
> @@ -15240,16 +15240,16 @@ static void 
> wlc_phy_workarounds_nphy_gainctrl_2057_rev5(struct brcms_phy *pi)
>  static void wlc_phy_workarounds_nphy_gainctrl_2057_rev6(struct brcms_phy *pi)
>  {
>   u16 currband;
> - s8 lna1G_gain_db_rev7[] = { 9, 14, 19, 24 };
> - s8 *lna1_gain_db = NULL;
> - s8 *lna1_gain_db_2 = NULL;
> - s8 *lna2_gain_db = NULL;
> - s8 tiaA_gain_db_rev7[] = { -9, -6, -3, 0, 3, 3, 3, 3, 3, 3 };
> - s8 *tia_gain_db;
> - s8 tiaA_gainbits_rev7[] = { 0, 1, 2, 3, 4, 4, 4, 4, 4, 4 };
> - s8 *tia_gainbits;
> - u16 rfseqA_init_gain_rev7[] = { 0x624f, 0x624f };
> - u16 *rfseq_init_gain;
> + static const s8 lna1G_gain_db_rev7[] = { 9, 14, 19, 24 };
> + const s8 *lna1_gain_db = NULL;
> + const s8 *lna1_gain_db_2 = NULL;
> + const s8 *lna2_gain_db = NULL;
> + static const s8 tiaA_gain_db_rev7[] = { -9, -6, -3, 0, 3, 3, 3, 3, 3, 3 
> };
> + const s8 *tia_gain_db;
> + static const s8 tiaA_gainbits_rev7[] = { 0, 1, 2, 3, 4, 4, 4, 4, 4, 4 };
> + const s8 *tia_gainbits;
> + static const u16 rfseqA_init_gain_rev7[] = { 0x624f, 0x624f };
> + const u16 *rfseq_init_gain;
>   u16 init_gaincode;
>   u16 clip1hi_gaincode;
>   u16 clip1md_gaincode = 0;
> @@ -15310,10 +15310,9 @@ static void 
> wlc_phy_workarounds_nphy_gainctrl_2057_rev6(struct brcms_phy *pi)
>  
>   if ((freq <= 5080) || (freq == 5825)) {
>  
> - s8 lna1A_gain_db_rev7[] = { 11, 16, 20, 24 };
> - s8 lna1A_gain_db_2_rev7[] = {
> - 11, 17, 22, 25};
> - s8 lna2A_gain_db_rev7[] = { -1, 6, 10, 14 };
> + static const s8 lna1A_gain_db_rev7[] = { 11, 
> 16, 20, 24 };
> + static const s8 lna1A_gain_db_2_rev7[] = { 11, 
> 17, 22, 25};
> + static const s8 lna2A_gain_db_rev7[] = { -1, 6, 
> 10, 14 };
>  
>   crsminu_th = 0x3e;
>   lna1_gain_db = lna1A_gain_db_rev7;
> @@ -15321,10 +15320,9 @@ static void 
> wlc_phy_workarounds_nphy_gainctrl_2057_rev6(struct brcms_phy *pi)
>   lna2_gain_db = lna2A_gain_db_rev7;
>   } else if ((freq >= 5500) && (freq <= 5700)) {
>  
> - s8 lna1A_gain_db_rev7[] = { 11, 17, 21, 25 };
> - s8 lna1A_gain_db_2_rev7[] = {
> - 12, 18, 22, 26};
> - s8 lna2A_gain_db_rev7[] = { 1, 8, 12, 16 };
> + static const s8 lna1A_gain_db_rev7[] = { 11, 
> 17, 21, 25 };
> + static const s8 lna1A_gain_db_2_rev7[] = { 12, 
> 18, 22, 26};
> + static const s8 lna2A

Re: [PATCH 09/26] brcmsmac: split up wlc_phy_workarounds_nphy

[Arend Van Spriel]

On 2-3-2017 17:38, Arnd Bergmann wrote:
> The stack consumption in this driver is still relatively high, with one
> remaining warning if the warning level is lowered to 1536 bytes:
> 
> drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c:17135:1: error: 
> the frame size of 1880 bytes is larger than 1536 bytes 
> [-Werror=frame-larger-than=]
> 
> The affected function is actually a collection of three separate 
> implementations,
> and each of them is fairly large by itself. Splitting them up is done easily
> and improves readability at the same time.
> 
> I'm leaving the original indentation to make the review easier.

Thanks ;-)

Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>
> Signed-off-by: Arnd Bergmann <a...@arndb.de>
> ---
>  .../broadcom/brcm80211/brcmsmac/phy/phy_n.c| 178 
> -
>  1 file changed, 104 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
> index 48a4df488d75..d76c092bb6b4 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
> @@ -16061,52 +16061,8 @@ static void wlc_phy_workarounds_nphy_gainctrl(struct 
> brcms_phy *pi)
>   }
>  }
>  
> -static void wlc_phy_workarounds_nphy(struct brcms_phy *pi)
> +static void wlc_phy_workarounds_nphy_rev7(struct brcms_phy *pi)
>  {
> - static const u8 rfseq_rx2tx_events[] = {
> - NPHY_RFSEQ_CMD_NOP,
> - NPHY_RFSEQ_CMD_RXG_FBW,
> - NPHY_RFSEQ_CMD_TR_SWITCH,
> - NPHY_RFSEQ_CMD_CLR_HIQ_DIS,
> - NPHY_RFSEQ_CMD_RXPD_TXPD,
> - NPHY_RFSEQ_CMD_TX_GAIN,
> - NPHY_RFSEQ_CMD_EXT_PA
> - };
> - u8 rfseq_rx2tx_dlys[] = { 8, 6, 6, 2, 4, 60, 1 };
> - static const u8 rfseq_tx2rx_events[] = {
> - NPHY_RFSEQ_CMD_NOP,
> - NPHY_RFSEQ_CMD_EXT_PA,
> - NPHY_RFSEQ_CMD_TX_GAIN,
> - NPHY_RFSEQ_CMD_RXPD_TXPD,
> - NPHY_RFSEQ_CMD_TR_SWITCH,
> - NPHY_RFSEQ_CMD_RXG_FBW,
> - NPHY_RFSEQ_CMD_CLR_HIQ_DIS
> - };
> - static const u8 rfseq_tx2rx_dlys[] = { 8, 6, 2, 4, 4, 6, 1 };
> - static const u8 rfseq_tx2rx_events_rev3[] = {
> - NPHY_REV3_RFSEQ_CMD_EXT_PA,
> - NPHY_REV3_RFSEQ_CMD_INT_PA_PU,
> - NPHY_REV3_RFSEQ_CMD_TX_GAIN,
> - NPHY_REV3_RFSEQ_CMD_RXPD_TXPD,
> - NPHY_REV3_RFSEQ_CMD_TR_SWITCH,
> - NPHY_REV3_RFSEQ_CMD_RXG_FBW,
> - NPHY_REV3_RFSEQ_CMD_CLR_HIQ_DIS,
> - NPHY_REV3_RFSEQ_CMD_END
> - };
> - static const u8 rfseq_tx2rx_dlys_rev3[] = { 8, 4, 2, 2, 4, 4, 6, 1 };
> - u8 rfseq_rx2tx_events_rev3[] = {
> - NPHY_REV3_RFSEQ_CMD_NOP,
> - NPHY_REV3_RFSEQ_CMD_RXG_FBW,
> - NPHY_REV3_RFSEQ_CMD_TR_SWITCH,
> - NPHY_REV3_RFSEQ_CMD_CLR_HIQ_DIS,
> - NPHY_REV3_RFSEQ_CMD_RXPD_TXPD,
> - NPHY_REV3_RFSEQ_CMD_TX_GAIN,
> - NPHY_REV3_RFSEQ_CMD_INT_PA_PU,
> - NPHY_REV3_RFSEQ_CMD_EXT_PA,
> - NPHY_REV3_RFSEQ_CMD_END
> - };
> - u8 rfseq_rx2tx_dlys_rev3[] = { 8, 6, 6, 4, 4, 18, 42, 1, 1 };
> -
>   static const u8 rfseq_rx2tx_events_rev3_ipa[] = {
>   NPHY_REV3_RFSEQ_CMD_NOP,
>   NPHY_REV3_RFSEQ_CMD_RXG_FBW,
> @@ -16120,29 +16076,15 @@ static void wlc_phy_workarounds_nphy(struct 
> brcms_phy *pi)
>   };
>   static const u8 rfseq_rx2tx_dlys_rev3_ipa[] = { 8, 6, 6, 4, 4, 16, 43, 
> 1, 1 };
>   static const u16 rfseq_rx2tx_dacbufpu_rev7[] = { 0x10f, 0x10f };
> -
> - s16 alpha0, alpha1, alpha2;
> - s16 beta0, beta1, beta2;
> - u32 leg_data_weights, ht_data_weights, nss1_data_weights,
> - stbc_data_weights;
> + u32 leg_data_weights;
>   u8 chan_freq_range = 0;
>   static const u16 dac_control = 0x0002;
>   u16 aux_adc_vmid_rev7_core0[] = { 0x8e, 0x96, 0x96, 0x96 };
>   u16 aux_adc_vmid_rev7_core1[] = { 0x8f, 0x9f, 0x9f, 0x96 };
> - u16 aux_adc_vmid_rev4[] = { 0xa2, 0xb4, 0xb4, 0x89 };
> - u16 aux_adc_vmid_rev3[] = { 0xa2, 0xb4, 0xb4, 0x89 };
> - u16 *aux_adc_vmid;
>   u16 aux_adc_gain_rev7[] = { 0x02, 0x02, 0x02, 0x02 };
> - u16 aux_adc_gain_rev4[] = { 0x02, 0x02, 0x02, 0x00 };
> - u16 aux_adc_gain_rev3[] = { 0x02, 0x02, 0x02, 0x00 };
> - u16 *aux_adc_gain;
> - static const u16 sk_adc_vmid[] = { 0xb4, 0xb4, 0xb4, 0x24 };
> - static const u16 sk_adc_gain[] = { 0x02, 0x02, 0x02, 0x02 };
>   s

Re: [PATCH 07/26] brcmsmac: reduce stack size with KASAN

[Arend Van Spriel]

On 2-3-2017 17:38, Arnd Bergmann wrote:
> The wlc_phy_table_write_nphy/wlc_phy_table_read_nphy functions always put an 
> object
> on the stack, which will each require a redzone with KASAN and lead to 
> possible
> stack overflow:
> 
> drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c: In function 
> 'wlc_phy_workarounds_nphy':
> drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c:17135:1: 
> warning: the frame size of 6312 bytes is larger than 1000 bytes 
> [-Wframe-larger-than=]

Looks like this warning text ended up in the wrong commit message. Got
me confused for a sec :-p

> This marks the two functions as noinline_for_kasan, avoiding the problem 
> entirely.

Frankly I seriously dislike annotating code for the sake of some
(dynamic) memory analyzer. To me the whole thing seems rather
unnecessary. If the code passes the 2048 stack limit without KASAN it
would seem the limit with KASAN should be such that no warning is given.
I suspect that it is rather difficult to predict the additional size of
the instrumentation code and on some systems there might be a real issue
with increased stack usage.

Regards,
Arend

> Signed-off-by: Arnd Bergmann 
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
> index b3aab2fe96eb..42dc8e1f483d 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
> @@ -14157,7 +14157,7 @@ static void wlc_phy_bphy_init_nphy(struct brcms_phy 
> *pi)
>   write_phy_reg(pi, NPHY_TO_BPHY_OFF + BPHY_STEP, 0x668);
>  }
>  
> -void
> +noinline_for_kasan void
>  wlc_phy_table_write_nphy(struct brcms_phy *pi, u32 id, u32 len, u32 offset,
>u32 width, const void *data)
>  {
> @@ -14171,7 +14171,7 @@ wlc_phy_table_write_nphy(struct brcms_phy *pi, u32 
> id, u32 len, u32 offset,
>   wlc_phy_write_table_nphy(pi, );
>  }
>  
> -void
> +noinline_for_kasan void
>  wlc_phy_table_read_nphy(struct brcms_phy *pi, u32 id, u32 len, u32 offset,
>   u32 width, void *data)
>  {
> 

Re: [PATCH] brcmfmac: Use net_device_stats from struct net_device

[Arend Van Spriel]

On 13-2-2017 11:14, Tobias Klauser wrote:
> Instead of using a private copy of struct net_device_stats in struct
> brcm_if, use stats from struct net_device.  Also remove the now
> unnecessary .ndo_get_stats function.

Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>
> Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
> ---
>  .../wireless/broadcom/brcm80211/brcmfmac/core.c| 26 
> +++---
>  .../wireless/broadcom/brcm80211/brcmfmac/core.h|  2 --
>  2 files changed, 8 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index b73a55b00fa7..60da86a8d95b 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -249,10 +249,10 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct 
> sk_buff *skb,
>  
>  done:
>   if (ret) {
> - ifp->stats.tx_dropped++;
> + ndev->stats.tx_dropped++;
>   } else {
> - ifp->stats.tx_packets++;
> - ifp->stats.tx_bytes += skb->len;
> + ndev->stats.tx_packets++;
> + ndev->stats.tx_bytes += skb->len;
>   }
>  
>   /* Return ok: we always eat the packet */
> @@ -296,15 +296,15 @@ void brcmf_txflowblock(struct device *dev, bool state)
>  void brcmf_netif_rx(struct brcmf_if *ifp, struct sk_buff *skb)
>  {
>   if (skb->pkt_type == PACKET_MULTICAST)
> - ifp->stats.multicast++;
> + ifp->ndev->stats.multicast++;
>  
>   if (!(ifp->ndev->flags & IFF_UP)) {
>   brcmu_pkt_buf_free_skb(skb);
>   return;
>   }
>  
> - ifp->stats.rx_bytes += skb->len;
> - ifp->stats.rx_packets++;
> + ifp->ndev->stats.rx_bytes += skb->len;
> + ifp->ndev->stats.rx_packets++;
>  
>   brcmf_dbg(DATA, "rx proto=0x%X\n", ntohs(skb->protocol));
>   if (in_interrupt())
> @@ -327,7 +327,7 @@ static int brcmf_rx_hdrpull(struct brcmf_pub *drvr, 
> struct sk_buff *skb,
>  
>   if (ret || !(*ifp) || !(*ifp)->ndev) {
>   if (ret != -ENODATA && *ifp)
> - (*ifp)->stats.rx_errors++;
> + (*ifp)->ndev->stats.rx_errors++;
>   brcmu_pkt_buf_free_skb(skb);
>   return -ENODATA;
>   }
> @@ -388,7 +388,7 @@ void brcmf_txfinalize(struct brcmf_if *ifp, struct 
> sk_buff *txp, bool success)
>   }
>  
>   if (!success)
> - ifp->stats.tx_errors++;
> + ifp->ndev->stats.tx_errors++;
>  
>   brcmu_pkt_buf_free_skb(txp);
>  }
> @@ -411,15 +411,6 @@ void brcmf_txcomplete(struct device *dev, struct sk_buff 
> *txp, bool success)
>   }
>  }
>  
> -static struct net_device_stats *brcmf_netdev_get_stats(struct net_device 
> *ndev)
> -{
> - struct brcmf_if *ifp = netdev_priv(ndev);
> -
> - brcmf_dbg(TRACE, "Enter, bsscfgidx=%d\n", ifp->bsscfgidx);
> -
> - return >stats;
> -}
> -
>  static void brcmf_ethtool_get_drvinfo(struct net_device *ndev,
>   struct ethtool_drvinfo *info)
>  {
> @@ -492,7 +483,6 @@ static int brcmf_netdev_open(struct net_device *ndev)
>  static const struct net_device_ops brcmf_netdev_ops_pri = {
>   .ndo_open = brcmf_netdev_open,
>   .ndo_stop = brcmf_netdev_stop,
> - .ndo_get_stats = brcmf_netdev_get_stats,
>   .ndo_start_xmit = brcmf_netdev_start_xmit,
>   .ndo_set_mac_address = brcmf_netdev_set_mac_address,
>   .ndo_set_rx_mode = brcmf_netdev_set_multicast_list
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
> index de3197be5491..6aecd8dfd824 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
> @@ -171,7 +171,6 @@ enum brcmf_netif_stop_reason {
>   * @drvr: points to device related information.
>   * @vif: points to cfg80211 specific interface information.
>   * @ndev: associated network device.
> - * @stats: interface specific network statistics.
>   * @multicast_work: worker object for multicast provisioning.
>   * @ndoffload_work: worker object for neighbor discovery offload 
> configuration.
>   * @fws_desc: interface specific firmware-signalling descriptor.
> @@ -187,7 +186,6 @@ struct brcmf_if {
>   struct brcmf_pub *drvr;
>   struct brcmf_cfg80211_vif *vif;
>   struct net_device *ndev;
> - struct net_device_stats stats;
>   struct work_struct multicast_work;
>   struct work_struct ndoffload_work;
>   struct brcmf_fws_mac_descriptor *fws_desc;
> 

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Arend Van Spriel]

On 27-1-2017 8:33, Kalle Valo wrote:
> Pali Rohár  writes:
> 
>> NVS calibration data for wl1251 are model specific. Every one device with
>> wl1251 chip has different and calibrated in factory.
>>
>> Not all wl1251 chips have own EEPROM where are calibration data stored. And
>> in that case there is no "standard" place. Every device has stored them on
>> different place (some in rootfs file, some in dedicated nand partition,
>> some in another proprietary structure).
>>
>> Kernel wl1251 driver cannot support every one different storage decided by
>> device manufacture so it will use request_firmware_prefer_user() call for
>> loading NVS calibration data and userspace helper will be responsible to
>> prepare correct data.
>>
>> In case userspace helper fails request_firmware_prefer_user() still try to
>> load data file directly from VFS as fallback mechanism.
>>
>> On Nokia N900 device which has wl1251 chip, NVS calibration data are stored
>> in CAL nand partition. CAL is proprietary Nokia key/value format for nand
>> devices.
>>
>> With this patch it is finally possible to load correct model specific NVS
>> calibration data for Nokia N900.
>>
>> Signed-off-by: Pali Rohár 
>> ---
>>  drivers/net/wireless/ti/wl1251/Kconfig |1 +
>>  drivers/net/wireless/ti/wl1251/main.c  |2 +-
>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/ti/wl1251/Kconfig 
>> b/drivers/net/wireless/ti/wl1251/Kconfig
>> index 7142ccf..affe154 100644
>> --- a/drivers/net/wireless/ti/wl1251/Kconfig
>> +++ b/drivers/net/wireless/ti/wl1251/Kconfig
>> @@ -2,6 +2,7 @@ config WL1251
>>  tristate "TI wl1251 driver support"
>>  depends on MAC80211
>>  select FW_LOADER
>> +select FW_LOADER_USER_HELPER
>>  select CRC7
>>  ---help---
>>This will enable TI wl1251 driver support. The drivers make
>> diff --git a/drivers/net/wireless/ti/wl1251/main.c 
>> b/drivers/net/wireless/ti/wl1251/main.c
>> index 208f062..24f8866 100644
>> --- a/drivers/net/wireless/ti/wl1251/main.c
>> +++ b/drivers/net/wireless/ti/wl1251/main.c
>> @@ -110,7 +110,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
>>  struct device *dev = wiphy_dev(wl->hw->wiphy);
>>  int ret;
>>  
>> -ret = request_firmware(, WL1251_NVS_NAME, dev);
>> +ret = request_firmware_prefer_user(, WL1251_NVS_NAME, dev);
> 
> I don't see the need for this. Just remove the default nvs file from
> filesystem and the fallback user helper will be always used, right?

Indeed. The only remaining issue would be that an error message is
logged. Also note the fallback is only used if selected in Kconfig.

> Like we discussed earlier, the default nvs file should not be used by
> normal users.

Yup.

Regards,
Arend

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Arend Van Spriel]

On 27-1-2017 13:26, Kalle Valo wrote:
> Pali Rohár  writes:
> 
>> On Friday 27 January 2017 13:49:03 Kalle Valo wrote:
>>> Pali Rohár  writes:
>>>
> So
> for those other platforms there will be a delay waiting for user-mode
> helper to fail, before trying to get nvs file from /lib/firmware.

 Yes, there will be. But there is no easy way to fix this problem that
 kernel is trying to use default/example NVS data...
>>>
>>> Kernel is doing correctly and requesting NVS data as expected, the
>>> problem here is that linux-firmware claims that the example NVS data is
>>> real calibration data (which it is not). Distros should not use that,
>>> only developers for testing purposes. We should not courage users using
>>> example calibration data.
>>>
>>> The simple fix is to rename the NVS file in linux-firmware to something
>>> like wl1251-nvs.bin.example, no need to workaround this in kernel. If
>>> you send a patch to linux-firmware I'm happy to ack that.
>>
>> I agree with rename and fact that default/example data should not be
>> used.
>>
>> But...
>>
>> 1) Kernel should not read device/model specific data from VFS where
>> are stored not-device-specific files preinstalled by linux
>> distributions.
>>
>> And linux distributions are already putting files into VFS and kernel
>> cannot enforce userspace to not do that (as they are already doing it).
> 
> I'm having problems to understand what you are saying here.

This is a personal opinion. I read it as: /lib/firmware can only contain
files for from linux-firmware.

At least the device-specific vs. non-device-specific does not seem to
hold. The firmware files that we have in the linux-firmware repository
are very device-specific. Unless you mean the 'platform' when talking
about 'device'.

Regards,
Arend

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Arend Van Spriel]

On 27-1-2017 11:10, Pali Rohár wrote:
> On Friday 27 January 2017 11:05:32 Arend Van Spriel wrote:
>> On 27-1-2017 10:43, Pali Rohár wrote:
>>> On Friday 27 January 2017 09:33:40 Kalle Valo wrote:
>>>> Pali Rohár <pali.ro...@gmail.com> writes:
>>>>
>>>>> NVS calibration data for wl1251 are model specific. Every one device with
>>>>> wl1251 chip has different and calibrated in factory.
>>>>>
>>>>> Not all wl1251 chips have own EEPROM where are calibration data stored. 
>>>>> And
>>>>> in that case there is no "standard" place. Every device has stored them on
>>>>> different place (some in rootfs file, some in dedicated nand partition,
>>>>> some in another proprietary structure).
>>>>>
>>>>> Kernel wl1251 driver cannot support every one different storage decided by
>>>>> device manufacture so it will use request_firmware_prefer_user() call for
>>>>> loading NVS calibration data and userspace helper will be responsible to
>>>>> prepare correct data.
>>>>>
>>>>> In case userspace helper fails request_firmware_prefer_user() still try to
>>>>> load data file directly from VFS as fallback mechanism.
>>>>>
>>>>> On Nokia N900 device which has wl1251 chip, NVS calibration data are 
>>>>> stored
>>>>> in CAL nand partition. CAL is proprietary Nokia key/value format for nand
>>>>> devices.
>>>>>
>>>>> With this patch it is finally possible to load correct model specific NVS
>>>>> calibration data for Nokia N900.
>>>>>
>>>>> Signed-off-by: Pali Rohár <pali.ro...@gmail.com>
>>>>> ---
>>>>>  drivers/net/wireless/ti/wl1251/Kconfig |1 +
>>>>>  drivers/net/wireless/ti/wl1251/main.c  |2 +-
>>>>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/wireless/ti/wl1251/Kconfig 
>>>>> b/drivers/net/wireless/ti/wl1251/Kconfig
>>>>> index 7142ccf..affe154 100644
>>>>> --- a/drivers/net/wireless/ti/wl1251/Kconfig
>>>>> +++ b/drivers/net/wireless/ti/wl1251/Kconfig
>>>>> @@ -2,6 +2,7 @@ config WL1251
>>>>>   tristate "TI wl1251 driver support"
>>>>>   depends on MAC80211
>>>>>   select FW_LOADER
>>>>> + select FW_LOADER_USER_HELPER
>>>>>   select CRC7
>>>>>   ---help---
>>>>> This will enable TI wl1251 driver support. The drivers make
>>>>> diff --git a/drivers/net/wireless/ti/wl1251/main.c 
>>>>> b/drivers/net/wireless/ti/wl1251/main.c
>>>>> index 208f062..24f8866 100644
>>>>> --- a/drivers/net/wireless/ti/wl1251/main.c
>>>>> +++ b/drivers/net/wireless/ti/wl1251/main.c
>>>>> @@ -110,7 +110,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
>>>>>   struct device *dev = wiphy_dev(wl->hw->wiphy);
>>>>>   int ret;
>>>>>  
>>>>> - ret = request_firmware(, WL1251_NVS_NAME, dev);
>>>>> + ret = request_firmware_prefer_user(, WL1251_NVS_NAME, dev);
>>>>
>>>> I don't see the need for this. Just remove the default nvs file from
>>>> filesystem and the fallback user helper will be always used, right?
>>>
>>> It is part of linux-firmware repository. And already part of all
>>> previous versions of linux-firmware packages in lot of linux
>>> distributions. So removing it is not possible...
>>
>> You are probably saying that on your platform you can not remove
>> anything from /lib/firmware, right? I don't see how you come from "it is
>> part of firmware package" to "removing is not possible". Trying to
>> understand this and it makes no sense.
> 
> It is already in linux distribution packages. If I remove that file from
> file system it will be placed there again by package management or it it
> will throw error message about system integrity (missing file, etc...).
> 
> Also that file is already in linux-firmware git and so is propagated to
> /lib/firmware by anybody who is using linux-firmware.
> 
>>>> Like we discussed earlier, the default nvs file should not be used by
>>>> normal users.
>>>
>>> But already is and we need to deal with this fact.
>>
>> Why?
> 
> Because everybody has already installed it.
> 
>> Are there other platforms that use the default nvs file and have a
>> working wifi.
> 
> I do not know.
> 
>> So your "removing is not possible" would be about
>> regression for those?
> 
> Yes, that is possible.
> 
> Also you can use wifi on Nokia N900 with this default file. Yes it is
> not recommended and probably has performance problems... but more people
> use it for SSH and it is working. Pavel could confirm this.
> 
> So yes, if you remove that file *now* there is regression for Nokia N900
> when you are using SSH over wifi.

So you are changing the behavior for all platforms using wl1251, but the
user-helper preference is (probably) only applicable for N900, right? So
for those other platforms there will be a delay waiting for user-mode
helper to fail, before trying to get nvs file from /lib/firmware.

Regards,
Arend

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Arend Van Spriel]

On 27-1-2017 10:43, Pali Rohár wrote:
> On Friday 27 January 2017 09:33:40 Kalle Valo wrote:
>> Pali Rohár  writes:
>>
>>> NVS calibration data for wl1251 are model specific. Every one device with
>>> wl1251 chip has different and calibrated in factory.
>>>
>>> Not all wl1251 chips have own EEPROM where are calibration data stored. And
>>> in that case there is no "standard" place. Every device has stored them on
>>> different place (some in rootfs file, some in dedicated nand partition,
>>> some in another proprietary structure).
>>>
>>> Kernel wl1251 driver cannot support every one different storage decided by
>>> device manufacture so it will use request_firmware_prefer_user() call for
>>> loading NVS calibration data and userspace helper will be responsible to
>>> prepare correct data.
>>>
>>> In case userspace helper fails request_firmware_prefer_user() still try to
>>> load data file directly from VFS as fallback mechanism.
>>>
>>> On Nokia N900 device which has wl1251 chip, NVS calibration data are stored
>>> in CAL nand partition. CAL is proprietary Nokia key/value format for nand
>>> devices.
>>>
>>> With this patch it is finally possible to load correct model specific NVS
>>> calibration data for Nokia N900.
>>>
>>> Signed-off-by: Pali Rohár 
>>> ---
>>>  drivers/net/wireless/ti/wl1251/Kconfig |1 +
>>>  drivers/net/wireless/ti/wl1251/main.c  |2 +-
>>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/ti/wl1251/Kconfig 
>>> b/drivers/net/wireless/ti/wl1251/Kconfig
>>> index 7142ccf..affe154 100644
>>> --- a/drivers/net/wireless/ti/wl1251/Kconfig
>>> +++ b/drivers/net/wireless/ti/wl1251/Kconfig
>>> @@ -2,6 +2,7 @@ config WL1251
>>> tristate "TI wl1251 driver support"
>>> depends on MAC80211
>>> select FW_LOADER
>>> +   select FW_LOADER_USER_HELPER
>>> select CRC7
>>> ---help---
>>>   This will enable TI wl1251 driver support. The drivers make
>>> diff --git a/drivers/net/wireless/ti/wl1251/main.c 
>>> b/drivers/net/wireless/ti/wl1251/main.c
>>> index 208f062..24f8866 100644
>>> --- a/drivers/net/wireless/ti/wl1251/main.c
>>> +++ b/drivers/net/wireless/ti/wl1251/main.c
>>> @@ -110,7 +110,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
>>> struct device *dev = wiphy_dev(wl->hw->wiphy);
>>> int ret;
>>>  
>>> -   ret = request_firmware(, WL1251_NVS_NAME, dev);
>>> +   ret = request_firmware_prefer_user(, WL1251_NVS_NAME, dev);
>>
>> I don't see the need for this. Just remove the default nvs file from
>> filesystem and the fallback user helper will be always used, right?
> 
> It is part of linux-firmware repository. And already part of all
> previous versions of linux-firmware packages in lot of linux
> distributions. So removing it is not possible...

You are probably saying that on your platform you can not remove
anything from /lib/firmware, right? I don't see how you come from "it is
part of firmware package" to "removing is not possible". Trying to
understand this and it makes no sense.

>> Like we discussed earlier, the default nvs file should not be used by
>> normal users.
> 
> But already is and we need to deal with this fact.

Why? Are there other platforms that use the default nvs file and have a
working wifi. So your "removing is not possible" would be about
regression for those?

Regards,
Arend

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Arend Van Spriel]

On 24-12-2016 17:52, Pali Rohár wrote:
> NVS calibration data for wl1251 are model specific. Every one device with
> wl1251 chip has different and calibrated in factory.
> 
> Not all wl1251 chips have own EEPROM where are calibration data stored. And
> in that case there is no "standard" place. Every device has stored them on
> different place (some in rootfs file, some in dedicated nand partition,
> some in another proprietary structure).
> 
> Kernel wl1251 driver cannot support every one different storage decided by
> device manufacture so it will use request_firmware_prefer_user() call for
> loading NVS calibration data and userspace helper will be responsible to
> prepare correct data.

Responding to this patch as it provides a lot of context to discuss. As
you might have gathered from earlier discussions I am not a fan of using
user-space helper. I can agree that the kernel driver, wl1251 in this
case, should be agnostic to platform specific details regarding storage
solutions and the firmware api should hide that. However, it seems your
only solution is adding user-space to the mix and changing the api
towards that. Can we solve it without user-space help?

The firmware_class already supports a number of path prefixes it
traverses looking for the requested firmware. So I was thinking about
adding a hashtable in which a platform driver can add firmware which are
stored in the hashtable using the hashed firmware name. Upon a firmware
request from the driver we could check the hashtable before traversing
the path prefixes on VFS. The obvious problem is that the request may
come before the firmware is added to the hashtable. Just wanted to pitch
the idea first and hear what others think about it and maybe someone has
a nice solution for this problem. Fingers crossed :-p

> In case userspace helper fails request_firmware_prefer_user() still try to
> load data file directly from VFS as fallback mechanism.
> 
> On Nokia N900 device which has wl1251 chip, NVS calibration data are stored
> in CAL nand partition. CAL is proprietary Nokia key/value format for nand
> devices.

With the firmware hashtable api on N900 a platform driver could
interpret the CAL data in the nand partition and provide it through the
firmware_class.

> With this patch it is finally possible to load correct model specific NVS
> calibration data for Nokia N900.

But on other devices that use wl1251, but for instance have no userspace
helper the request to userspace will fail (after 60 sec?) and try VFS
after that. Maybe not so nice. You should consider other device
configurations. Not just N900.

Regards,
Arend

> Signed-off-by: Pali Rohár 
> ---
>  drivers/net/wireless/ti/wl1251/Kconfig |1 +
>  drivers/net/wireless/ti/wl1251/main.c  |2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ti/wl1251/Kconfig 
> b/drivers/net/wireless/ti/wl1251/Kconfig
> index 7142ccf..affe154 100644
> --- a/drivers/net/wireless/ti/wl1251/Kconfig
> +++ b/drivers/net/wireless/ti/wl1251/Kconfig
> @@ -2,6 +2,7 @@ config WL1251
>   tristate "TI wl1251 driver support"
>   depends on MAC80211
>   select FW_LOADER
> + select FW_LOADER_USER_HELPER
>   select CRC7
>   ---help---
> This will enable TI wl1251 driver support. The drivers make
> diff --git a/drivers/net/wireless/ti/wl1251/main.c 
> b/drivers/net/wireless/ti/wl1251/main.c
> index 208f062..24f8866 100644
> --- a/drivers/net/wireless/ti/wl1251/main.c
> +++ b/drivers/net/wireless/ti/wl1251/main.c
> @@ -110,7 +110,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
>   struct device *dev = wiphy_dev(wl->hw->wiphy);
>   int ret;
>  
> - ret = request_firmware(, WL1251_NVS_NAME, dev);
> + ret = request_firmware_prefer_user(, WL1251_NVS_NAME, dev);
>  
>   if (ret < 0) {
>   wl1251_error("could not get nvs file: %d", ret);
> 

Re: [PATCH] brcmfmac: fix spelling mistakes on "Ivalid"

[Arend Van Spriel]

On 23-12-2016 1:43, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
> 
> Trivial fixes to spelling mistake "Ivalid" to "Invalid" in
> brcmf_err error messages.

Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index 7ffc4ab..15eaf72 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -3971,7 +3971,7 @@ brcmf_configure_wpaie(struct brcmf_if *ifp,
>   pval |= AES_ENABLED;
>   break;
>   default:
> - brcmf_err("Ivalid unicast security info\n");
> + brcmf_err("Invalid unicast security info\n");
>   }
>   offset++;
>   }
> @@ -4015,7 +4015,7 @@ brcmf_configure_wpaie(struct brcmf_if *ifp,
>   wpa_auth |= WPA2_AUTH_1X_SHA256;
>   break;
>   default:
> - brcmf_err("Ivalid key mgmt info\n");
> + brcmf_err("Invalid key mgmt info\n");
>   }
>   offset++;
>   }
> 

Re: [PATCH 2/2] net: wireless: fix to uses struct

[Arend Van Spriel]

On 21-12-2016 23:23, Ozgur Karatas wrote:
> 
> The patch fixed to struct uses in reg.c, I think doesn't need to be use to 
> "struct". 
> There is dataype not have to logical link and each is different definitons.
> 
> I'm undecided on this patch. I compiled and didn't to errors.

There must be something wrong in the way you build stuff, but still just
looking at your patch it is fundamentally wrong, which is what makes
people say "do a basic C course". Let me try and explain below.

> Signed-off-by: Ozgur Karatas 
> ---
>  net/wireless/reg.c  | 10 +-
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
> index 5dbac37..5b70970 100644
> --- a/net/wireless/reg.c
> +++ b/net/wireless/reg.c
> @@ -490,7 +490,7 @@ static int reg_query_builtin(const char *alpha2)
>   if (!regdom)
>   return -ENODATA;
>  
> - request = kzalloc(sizeof(struct reg_regdb_apply_request), GFP_KERNEL);
> + request = kzalloc(sizeof(*reg_regdb_apply_request), GFP_KERNEL);

Making it more abstract to explain what you are doing:

x = foo(sizeof(T), GFP_KERNEL); where T is "struct Y".

which you change to:

x = foo(sizeof(*Y), GFP_KERNEL);

Y has no meaning for the sizeof operator and the compiler will yell at
it being an unknown identifier. In a lot of kernel code you will find:

x = foo(sizeof(*x), GFP_KERNEL);

which is probably the coding style fix you are attempting to make, but
miserably fail to do so. There is nothing linux kernel specific about
this. It is really fundamental knowledge of the C language. The correct
change for this instance is:

-   request = kzalloc(sizeof(struct reg_regdb_apply_request), GFP_KERNEL);
+   request = kzalloc(sizeof(*request), GFP_KERNEL);

Hope this helps to come up with a working V2 of this patch.

Regards,
Arend

Re: wl1251 & mac address & calibration data

[Arend Van Spriel]

On 18-12-2016 13:09, Pali Rohár wrote:
> On Sunday 18 December 2016 12:54:00 Arend Van Spriel wrote:
>> On 18-12-2016 12:04, Pali Rohár wrote:
>>> On Sunday 18 December 2016 11:49:53 Arend Van Spriel wrote:
>>>> On 16-12-2016 11:40, Pali Rohár wrote:
>>>>> On Friday 16 December 2016 08:25:44 Daniel Wagner wrote:
>>>>>> On 12/16/2016 03:03 AM, Luis R. Rodriguez wrote:
>>>>>>> For the new API a solution for "fallback mechanisms" should be
>>>>>>> clean though and I am looking to stay as far as possible from
>>>>>>> the existing mess. A solution to help both the old API and new
>>>>>>> API is possible for the "fallback mechanism" though -- but for
>>>>>>> that I can only refer you at this point to some of Daniel
>>>>>>> Wagner and Tom Gunderson's firmwared deamon prospect. It
>>>>>>> should help pave the way for a clean solution and help address
>>>>>>> other stupid issues.
>>>>>>
>>>>>> The firmwared project is hosted here
>>>>>>
>>>>>> https://github.com/teg/firmwared
>>>>>>
>>>>>> As Luis pointed out, firmwared relies on
>>>>>> FW_LOADER_USER_HELPER_FALLBACK, which is not enabled by default.
>>>>>
>>>>> I know. But it does not mean that I cannot enable this option at
>>>>> kernel compile time.
>>>>>
>>>>> Bigger problem is that currently request_firmware() first try to
>>>>> load firmware directly from VFS and after that (if fails)
>>>>> fallback to user helper.
>>>>>
>>>>> So I would need to extend kernel firmware code with new function
>>>>> (or flag) to not use VFS and try only user mode helper.
>>>>
>>>> Why do you need the user-mode helper anyway. This is all static
>>>> data, right?
>>>
>>> Those are static data, but device specific!
>>
>> So what?
>>
>>>> So why not cook up a firmware file in user-space once and put
>>>> it in /lib/firmware for the driver to request directly.
>>>
>>> 1. Violates FHS
>>
>> How?
>>
>>> 2. Does not work for readonly /, readonly /lib, readonly
>>> /lib/firmware
>>
>> Que?
>>
>>> 3. Backup & restore of rootfs between same devices does not work
>>> (as rootfs now contains device specific data).
>>
>> True.
>>
>>> 4. Sharing one rootfs (either via nfs or other technology) does not
>>> work for more devices (even in state when rootfs is used only by
>>> one device at one time).
>>
>> Indeed.
>>
>>> And it is common that N900 developers have rootfs in laptop and via
>>> usb (cdc_ether) exports it over nfs to N900 device and boot
>>> system. It basically break booting from one nfs-exported rootfs,
>>> as that export become model specific...
>>
>> These are all you choices and more a logistic issue. If your take is
>> that udev is the way to solve those, fine by me.
>>
>>>> Seems a bit
>>>> overkill to have a {e,}udev or whatever daemon running if the
>>>> result is always the same. Just my 2 cents.
>>>
>>> No it is not. It will break couple of other things in Linux and
>>> device
>>
>> Now I am curious. What "couple of other things" will be broken.
>>
>>> and model specific calibration data should not be in /lib/firmware!
>>> That directory is used for firmware files, not calibration.
>>
>> What is "firmware"? Really. These are binary blobs required to make
>> the device work. And guess what, your device needs calibration data.
>> Why make the distinction.
>>
>> Regards,
>> Arend
> 
> File wl1251-nvs.bin is provided by linux-firmware package and contains 
> default data which should be overriden by model specific calibrated 
> data.

Ah. Someone thought it was a good idea to provide the "one ring to rule
them all". Nice.

> But overwriting that one file is not possible as it next update of 
> linux-firmware package will overwrite it back. It break any normal usage 
> of package management.
> 
> Also it is ridiculously broken by design if some "boot" files needs to 
> be overwritten to initialize hardware properly. To not break booting you 
> need to overwrite that file before first boot. But without booting 
> device you cannot read calibration data. So some hack with autoreboot 
> after boot is needed. And how to detect that we have real overwritten 
> calibration data and not default one from linux-firmware? Any heuristic 
> or checks will be broken here. And no, nothing like you need to reboot 
> your device now (and similar concept) from windows world is not 
> accepted.

Well. After reading and creating calibration data you could just rebind
the driver to the device to have it probed again. But yeah, the default
one from linux-firmware should never have been there in the first place.

> "firmware" is one for chip. Any N900 device with wl1251 chip needs 
> exactly same firmware "wl1251-fw.bin". But every N900 needs different 
> calibration data which is not firmware.

Ok. This is exactly why Luis is giving the new API different name just
calling it "data".

Regards,
Arend

Re: wl1251 & mac address & calibration data

[Arend Van Spriel]

On 18-12-2016 12:04, Pali Rohár wrote:
> On Sunday 18 December 2016 11:49:53 Arend Van Spriel wrote:
>> On 16-12-2016 11:40, Pali Rohár wrote:
>>> On Friday 16 December 2016 08:25:44 Daniel Wagner wrote:
>>>> On 12/16/2016 03:03 AM, Luis R. Rodriguez wrote:
>>>>> For the new API a solution for "fallback mechanisms" should be
>>>>> clean though and I am looking to stay as far as possible from the
>>>>> existing mess. A solution to help both the old API and new API is
>>>>> possible for the "fallback mechanism" though -- but for that I
>>>>> can only refer you at this point to some of Daniel Wagner and
>>>>> Tom Gunderson's firmwared deamon prospect. It should help pave
>>>>> the way for a clean solution and help address other stupid
>>>>> issues.
>>>>
>>>> The firmwared project is hosted here
>>>>
>>>> https://github.com/teg/firmwared
>>>>
>>>> As Luis pointed out, firmwared relies on
>>>> FW_LOADER_USER_HELPER_FALLBACK, which is not enabled by default.
>>>
>>> I know. But it does not mean that I cannot enable this option at
>>> kernel compile time.
>>>
>>> Bigger problem is that currently request_firmware() first try to
>>> load firmware directly from VFS and after that (if fails) fallback
>>> to user helper.
>>>
>>> So I would need to extend kernel firmware code with new function
>>> (or flag) to not use VFS and try only user mode helper.
>>
>> Why do you need the user-mode helper anyway. This is all static data,
>> right?
> 
> Those are static data, but device specific!

So what?

>> So why not cook up a firmware file in user-space once and put
>> it in /lib/firmware for the driver to request directly.
> 
> 1. Violates FHS

How?

> 2. Does not work for readonly /, readonly /lib, readonly /lib/firmware

Que?

> 3. Backup & restore of rootfs between same devices does not work (as 
> rootfs now contains device specific data).

True.

> 4. Sharing one rootfs (either via nfs or other technology) does not work 
> for more devices (even in state when rootfs is used only by one device 
> at one time).

Indeed.

> And it is common that N900 developers have rootfs in laptop and via usb 
> (cdc_ether) exports it over nfs to N900 device and boot system. It 
> basically break booting from one nfs-exported rootfs, as that export 
> become model specific...

These are all you choices and more a logistic issue. If your take is
that udev is the way to solve those, fine by me.

>> Seems a bit
>> overkill to have a {e,}udev or whatever daemon running if the result
>> is always the same. Just my 2 cents.
> 
> No it is not. It will break couple of other things in Linux and device 

Now I am curious. What "couple of other things" will be broken.

> and model specific calibration data should not be in /lib/firmware! That 
> directory is used for firmware files, not calibration.

What is "firmware"? Really. These are binary blobs required to make the
device work. And guess what, your device needs calibration data. Why
make the distinction.

Regards,
Arend

Re: wl1251 & mac address & calibration data

[Arend Van Spriel]

On 16-12-2016 11:40, Pali Rohár wrote:
> On Friday 16 December 2016 08:25:44 Daniel Wagner wrote:
>> On 12/16/2016 03:03 AM, Luis R. Rodriguez wrote:
>>> For the new API a solution for "fallback mechanisms" should be
>>> clean though and I am looking to stay as far as possible from the
>>> existing mess. A solution to help both the old API and new API is
>>> possible for the "fallback mechanism" though -- but for that I can
>>> only refer you at this point to some of Daniel Wagner and Tom
>>> Gunderson's firmwared deamon prospect. It should help pave the way
>>> for a clean solution and help address other stupid issues.
>>
>> The firmwared project is hosted here
>>
>> https://github.com/teg/firmwared
>>
>> As Luis pointed out, firmwared relies on
>> FW_LOADER_USER_HELPER_FALLBACK, which is not enabled by default.
> 
> I know. But it does not mean that I cannot enable this option at kernel 
> compile time.
> 
> Bigger problem is that currently request_firmware() first try to load 
> firmware directly from VFS and after that (if fails) fallback to user 
> helper.
> 
> So I would need to extend kernel firmware code with new function (or 
> flag) to not use VFS and try only user mode helper.

Why do you need the user-mode helper anyway. This is all static data,
right? So why not cook up a firmware file in user-space once and put it
in /lib/firmware for the driver to request directly. Seems a bit
overkill to have a {e,}udev or whatever daemon running if the result is
always the same. Just my 2 cents.

Regards,
Arend

Re: [PATCH 8/8] Makefile: drop -D__CHECK_ENDIAN__ from cflags

[Arend Van Spriel]

On 15-12-2016 6:15, Michael S. Tsirkin wrote:
> That's the default now, no need for makefiles to set it.
> 
> Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
> ---
>  drivers/bluetooth/Makefile| 2 --
>  drivers/net/can/Makefile  | 1 -
>  drivers/net/ethernet/altera/Makefile  | 1 -
>  drivers/net/ethernet/atheros/alx/Makefile | 1 -
>  drivers/net/ethernet/freescale/Makefile   | 2 --
>  drivers/net/wireless/ath/Makefile | 2 --
>  drivers/net/wireless/ath/wil6210/Makefile | 2 --
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile | 2 --
>  drivers/net/wireless/broadcom/brcm80211/brcmsmac/Makefile | 1 -

For brcm80211 drivers:

Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>

Regards,
Arend

Re: wl1251 & mac address & calibration data

[Arend Van Spriel]

On 15-12-2016 16:33, Pali Rohár wrote:
> On Thu Dec 15 09:18:44 2016 Kalle Valo  wrote:
>> (Adding Luis because he has been working on request_firmware() lately)
>>
>> Pali Rohár  writes:
>>
> So no, there is no argument against... request_firmware() in
> fallback mode with userspace helper is by design blocking and
> waiting for userspace. But waiting for some change in DTS in
> kernel is just nonsense.

 I would just mark the wlan device with status = "disabled" and
 enable it in the overlay together with adding the NVS & MAC info.
>>>
>>> So if you think that this solution make sense, we can wait what net 
>>> wireless maintainers say about it...
>>>
>>> For me it looks like that solution can be:
>>>
>>> extending request_firmware() to use only userspace helper
>>
>> I haven't followed the discussion very closely but this is my preference
>> what drivers should do:
>>
>> 1) First the driver should do try to get the calibration data and mac
>>address from the device tree.
>>
> 
> Ok, but there is no (dynamic, device specific) data in DTS for N900. So 1) is 
> noop.

Uhm. What do you mean? You can propose a patch to the DT bindings [1] to
get it in there and create your N900 DTB or am I missing something here.
Are there hardware restrictions that do not allow you to boot with your
own DTB.

>> 2) If they are not in DT the driver should retrieve the calibration data
>>with request_firmware(). BUT with an option for user space to
>>implement that with a helper script so that the data can be created
>>dynamically, which I believe openwrt does with ath10k calibration
>>data right now.
> 
> Currently there is flag for request_firmware() that it should fallback to 
> user helper if direct VFS access not find needed firmware.
> 
> But this flag is not suitable as /lib/firmware already provides default (not 
> device specific) calibration data.
> 
> So I would suggest to add another flag/function which will primary use user 
> helper.

I recall Luis saying that user-mode helper (fallback) should be
discouraged, because there is no assurance that there is a user-mode
helper so you might just be pissing in the wind. The idea was to have a
dedicated API call that explicitly does the request towards user-space.

By the way, are we talking here about wl1251 device or driver as you
also mentioned wl12xx? I did not read the entire thread.

Regards,
Arend

Re: [PATCH 1/1] orinoco: fix improper return value

[Arend Van Spriel]

On 8-12-2016 1:40, Pan Bian wrote:
> Function orinoco_ioctl_commit() returns 0 (indicates success) when the
> call to orinoco_lock() fails. Thus, the return value is inconsistent with
> the execution status. It may be better to return "-EBUSY" when the call 
> to orinoco_lock() fails.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=188671
> 
> Signed-off-by: Pan Bian 
> ---
>  drivers/net/wireless/intersil/orinoco/wext.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/intersil/orinoco/wext.c 
> b/drivers/net/wireless/intersil/orinoco/wext.c
> index 1d4dae4..fee57ea 100644
> --- a/drivers/net/wireless/intersil/orinoco/wext.c
> +++ b/drivers/net/wireless/intersil/orinoco/wext.c
> @@ -1314,7 +1314,7 @@ static int orinoco_ioctl_commit(struct net_device *dev,
>   return 0;
>  
>   if (orinoco_lock(priv, ) != 0)
> - return err;
> + return -EBUSY;

Actually, orinoco_lock will return either -EBUSY or 0 so maybe better to
just do:

err = orinoco_lock(priv, );
if (err < 0)
return err;

Regards,
Arend

>   err = orinoco_commit(priv);
>  
> 

Re: wl1251 & mac address & calibration data

[Arend Van Spriel]

On 22-11-2016 18:05, Pali Rohár wrote:
> On Tuesday 22 November 2016 17:14:28 Michal Kazior wrote:
>> On 22 November 2016 at 16:31, Pali Rohár  wrote:
>>> On Tuesday 22 November 2016 16:22:57 Michal Kazior wrote:
 On 21 November 2016 at 16:51, Pali Rohár 
 wrote:
> On Friday 11 November 2016 18:20:50 Pali Rohár wrote:
>> Hi! I will open discussion about mac address and calibration
>> data for wl1251 wireless chip again...
>>
>> Problem: Mac address & calibration data for wl1251 chip on
>> Nokia N900 are stored on second nand partition (mtd1) in
>> special proprietary format which is used only for Nokia N900
>> (probably on N8x0 and N9 too). Wireless driver wl1251.ko
>> cannot work without mac address and calibration data.

 Same problem applies to some ath9k/ath10k supported routers. Some
 even carry mac address as implicit offset from ethernet mac
 address. As far as I understand OpenWRT cooks cal blobs on first
 boot prior to loading modules.
>>>
>>> So... wl1251 on Nokia N900 is not alone and this problem is there
>>> for more drivers and devices. Which means we should come up with
>>> some generic solution.
>>
>> This isn't particularly a problem for ath9k/ath10k.
>>
>> Let me give you more background on ath10k.
>>
>> ath10k devices can come with caldata and macaddr stored in their
>> OTP/EEPROM. In that case a generic "template" board file is used.
>> Userspace doesn't need to do anything special.
>>
>> Some vendors however decide to use flash partition to store caldata.
>> In that case ath10k expects userspace to prepare
>> cal-$bus-$devname.bin files, each for a different radio (you can
>> have multiple radios on a system).
>>
>> Now translating this for wl1251 I would expect it should also use
>> something like wl1251-nvs-sdio-0x0001.bin for devices like N900 that
>> have caldata on flash partition (instead of the generic
>> wl1251-nvs.bin). I'm not sure if wl1251-nvs.bin is something
>> comparable to (the generic) board.bin ath10k has though. Maybe the
>> entire idea behind wl1251-nvs.bin is flawed as it's supposed to be
>> device specific and is oblivious to possibility of having multiple
>> wl1251 radios on one system (probably sane assumption from practical
>> standpoint but still).
> 
> Basically nvs data are device specific, in ideal case they should be 
> generated in factory by some calibration process (or so).

For brcmfmac we have what we call nvram data, which is determined during
manufacturing. We use the firmware_class API to obtain that file, but on
router it may be stored in flash. So an API was created for that router
architecture and brcmfmac calls that API [1]. Not a generic solution but
it gets the job done. Personally, I would have liked this to be handled
behind the firmware_class API to hide the storage details from the driver.

Regards,
Arend

[1]
http://lxr.free-electrons.com/source/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c#L449

Re: [PATCH] brcmfmac: implement more accurate skb tracking

[Arend Van Spriel]

On 29-9-2016 23:57, Rafał Miłecki wrote:
> On 27 September 2016 at 11:24, Arend Van Spriel
> <arend.vanspr...@broadcom.com> wrote:
>> On 26-9-2016 14:38, Rafał Miłecki wrote:
>>> On 26 September 2016 at 14:13, Rafał Miłecki <zaj...@gmail.com> wrote:
>>>> On 26 September 2016 at 13:46, Arend Van Spriel
>>>> <arend.vanspr...@broadcom.com> wrote:
>>>>> On 26-9-2016 12:23, Rafał Miłecki wrote:
>>>>>> From: Rafał Miłecki <ra...@milecki.pl>
>>>>>>
>>>>>> We need to track 802.1x packets to know if there are any pending ones
>>>>>> for transmission. This is required for performing key update in the
>>>>>> firmware.
>>>>>
>>>>> The problem we are trying to solve is a pretty old one. The problem is
>>>>> that wpa_supplicant uses two separate code paths: EAPOL messaging
>>>>> through data path and key configuration though nl80211.
>>>>
>>>> Can I find it described/reported somewhere?
>>>>
>>>>
>>>>>> Unfortunately our old tracking code wasn't very accurate. It was
>>>>>> treating skb as pending as soon as it was passed by the netif. Actual
>>>>>> handling packet to the firmware was happening later as brcmfmac
>>>>>> internally queues them and uses its own worker(s).
>>>>>
>>>>> That does not seem right. As soon as we get a 1x packet we need to wait
>>>>> with key configuration regardless whether it is still in the driver or
>>>>> handed over to firmware already.
>>>>
>>>> OK, thanks.
>>>
>>> Actually, it's not OK. I was trying to report/describe/discuss this
>>> problem for over a week. I couldn't get much of answer from you.
>>>
>>> I had to come with a patch I worked on for quite some time. Only then
>>> you decided to react and reply with a reason for a nack. I see this
>>> patch may be wrong (but it's still hard to know what's going wrong
>>> without a proper hostapd bug report). I'd expect you to somehow work &
>>> communicate with open source community.
>>
>> We do or at least make an honest attempt, but there is more on our plate
>> so responses may be delayed. It also does not help when you get anal and
>> preachy when we do respond. Also not OK. In this case the delay is
>> caused because I had to pick up the thread(s) as Hante is on vacation
>> (he needed a break :-p ). However, you started sending patches so I
>> decided to look at and respond to those. Sorry if you felt like we left
>> you hanging to dry.
> 
> I believe I get easily irritated due to my communication experience I
> got so far :(
> 
> 
> Over a year ago I reported brcmfmac can't recover from failed
> register_netdev(ice). This bug remains unfixed.
> 
> In 2014 I reported problem with 80 MHz support. I didn't have hardware
> to fix & test it on my own (you weren't able/allowed to send me one of
> your PCIe cards). In remained broken until I fixed it year later.
> 
> You missed my crash bug report about caused by missing eth_type_trans
> and came with patch on your own a month later.
> 
> Earlier this year I reported you problem with BCM4366 and multiple
> interfaces. I didn't get much help. 3 months later I came with patch
> to workaround the problem but you said there's a better way to do
> this. It took me 2 weeks to figure out a new wlioctl API for that
> while all I needed was a simple hint on "interface_remove".
> 
> Right now I'm waiting to get any answer from you about 4366c0
> firmware. It's still less than 2 weeks since I asked for it, but a
> simple ETA would be nice. I'm actually not sure if I should report
> more problems to you to don't distract you from pending things.

This is a difficult question. All upstream firmware releases for router
chips are put on hold until further notice. Some decisions have been
made, but I have not seen a detailed plan to give an ETA.

> Problems with brcmf_netdev_wait_pend8021x were reported multiples
> times for last few months. When I finally got time for that it took me
> a week to debug them.

For the pend8021x you were sending a number of messages showing debug
progress so not sure whether you wanted our feedback on that. If so a
ping might have done it.

> As you can see, it takes me months to get help on some things. And in
> few cases I never got much help at all. Yes, I was hoping to have you
> more involved into brcmfmac development and problems solving. I guess
> things didn't meet my expectations and I got grumpy & preachy.

Thanks for listing all our failures. Somehow we are very good at getting
each other grumpy. When we provide a patch and you break it up and
submit that to Kalle, we get grumpy and it all piles up to the point
where we have this kind of conversation. As long as it helps to get
things of our chest I can live with that. Hope you can too. We strive to
give support to the community, but the priority is low as it is not
full-time activity.

Regards,
Arend

Re: [PATCH 4.9] brcmfmac: use correct skb freeing helper when deleting flowring

[Arend Van Spriel]

On 27-9-2016 13:58, Rafał Miłecki wrote:
> On 27 September 2016 at 13:44, Rafał Miłecki <zaj...@gmail.com> wrote:
>> On 27 September 2016 at 13:27, Kalle Valo <kv...@codeaurora.org> wrote:
>>> Arend Van Spriel <arend.vanspr...@broadcom.com> writes:
>>>
>>>> On 27-9-2016 11:14, Rafał Miłecki wrote:
>>>>> From: Rafał Miłecki <ra...@milecki.pl>
>>>>>
>>>>> Flowrings contain skbs waiting for transmission that were passed to us
>>>>> by netif. It means we checked every one of them looking for 802.1x
>>>>> Ethernet type. When deleting flowring we have to use freeing function
>>>>> that will check for 802.1x type as well.
>>>>>
>>>>> Freeing skbs without a proper check was leading to counter not being
>>>>> properly decreased. This was triggering a WARNING every time
>>>>> brcmf_netdev_wait_pend8021x was called.
>>>>
>>>> Acked-by: Arend van Spriel <ar...@broadcom.com>
>>>>> Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
>>>>> ---
>>>>> Kalle: this isn't important enough for 4.8 as it's too late for that.
>>>>>
>>>>> I'd like to get it for 4.9 however, as this fixes bug that could lead
>>>>> to WARNING on every add_key/del_key call. We was struggling with these
>>>>> WARNINGs for some time and this fixes one of two problems causing them.
>>>
>>> Ok, I'll queue this for 4.9.
>>>
>>>> Please mark it for stable as well.
>>>
>>> I can add that. Any ideas how old releases stable releases should this
>>> go to?
>>
>> I was analyzing this.
>> 1) This patch uses brcmf_get_ifp which is available in 4.4+ only.
>> 2) It applies cleanly to 4.5+ only due to 32f90caa7debd ("brcmfmac:
>> Increase nr of supported flowrings.")
>> 3) 4.4 would also require applying to the patch without broadcom/ subdir
>>
>> That said I suggest 4.5+. Any objections?

No objections. Just a tip. I tend to look at kernel.org main page to see
the stable and long-term kernel listed. So 4.7+ and 4.5+ have same
meaning as 4.5 and 4.6 are not stable/long-term kernels.

Regards,
Arend

> Let me see if patchwork with pick Cc tag as it does for others.
> 
> Cc: sta...@vger.kernel.org # 4.5+
> 
> This may be worth backporting to 4.4 as well (as it's longterm), but
> I'll do it separately due to patch not applying cleanly.

Re: [PATCH 4.9] brcmfmac: use correct skb freeing helper when deleting flowring

[Arend Van Spriel]

On 27-9-2016 13:27, Kalle Valo wrote:
> Arend Van Spriel <arend.vanspr...@broadcom.com> writes:
> 
>> On 27-9-2016 11:14, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <ra...@milecki.pl>
>>>
>>> Flowrings contain skbs waiting for transmission that were passed to us
>>> by netif. It means we checked every one of them looking for 802.1x
>>> Ethernet type. When deleting flowring we have to use freeing function
>>> that will check for 802.1x type as well.
>>>
>>> Freeing skbs without a proper check was leading to counter not being
>>> properly decreased. This was triggering a WARNING every time
>>> brcmf_netdev_wait_pend8021x was called.
>>
>> Acked-by: Arend van Spriel <ar...@broadcom.com>
>>> Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
>>> ---
>>> Kalle: this isn't important enough for 4.8 as it's too late for that.
>>>
>>> I'd like to get it for 4.9 however, as this fixes bug that could lead
>>> to WARNING on every add_key/del_key call. We was struggling with these
>>> WARNINGs for some time and this fixes one of two problems causing them.
> 
> Ok, I'll queue this for 4.9.
> 
>> Please mark it for stable as well.
> 
> I can add that. Any ideas how old releases stable releases should this
> go to?

Not sure if the vendor directory move causes issues as stable can not
fallback to three-way merge. I assumed it would so my last stable tag
was only for 4.7 and I took care of older kernels at later time with
backported patch. I can do that for this one as well.

Regards,
Arend

Re: [PATCH] brcmfmac: replace WARNING on timeout with a simple error message

[Arend Van Spriel]

On 27-9-2016 12:12, Rafał Miłecki wrote:
> From: Rafał Miłecki <ra...@milecki.pl>
> 
> Even with timeout increased to 950 ms we get WARNINGs from time to time.
> It mostly happens on A-MPDU stalls (e.g. when station goes out of
> range). It may take up to 5-10 secods for the firmware to recover and
> for that time it doesn't process packets.
> 
> It's still useful to have a message on time out as it may indicate some
> firmware problem and incorrect key update. Raising a WARNING however
> wasn't really that necessary, it doesn't point to any driver bug anymore
> and backtrace wasn't much useful.

Indeed the interesting part would be in another context. So:

Acked-by: Arend van Spriel <arend.vanspr...@broadcom.com>
> Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index 6d046ba..9e6f60a 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -1161,7 +1161,8 @@ int brcmf_netdev_wait_pend8021x(struct brcmf_if *ifp)
>!brcmf_get_pend_8021x_cnt(ifp),
>MAX_WAIT_FOR_8021X_TX);
>  
> - WARN_ON(!err);
> + if (!err)
> + brcmf_err("Timed out waiting for no pending 802.1x packets\n");
>  
>   return !err;
>  }
> 

Re: [PATCH 4.9] brcmfmac: use correct skb freeing helper when deleting flowring

[Arend Van Spriel]

On 27-9-2016 11:14, Rafał Miłecki wrote:
> From: Rafał Miłecki <ra...@milecki.pl>
> 
> Flowrings contain skbs waiting for transmission that were passed to us
> by netif. It means we checked every one of them looking for 802.1x
> Ethernet type. When deleting flowring we have to use freeing function
> that will check for 802.1x type as well.
> 
> Freeing skbs without a proper check was leading to counter not being
> properly decreased. This was triggering a WARNING every time
> brcmf_netdev_wait_pend8021x was called.

Acked-by: Arend van Spriel <ar...@broadcom.com>
> Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
> ---
> Kalle: this isn't important enough for 4.8 as it's too late for that.
> 
> I'd like to get it for 4.9 however, as this fixes bug that could lead
> to WARNING on every add_key/del_key call. We was struggling with these
> WARNINGs for some time and this fixes one of two problems causing them.

Please mark it for stable as well.

> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c
> index b16b367..d0b738d 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c
> @@ -234,13 +234,20 @@ static void brcmf_flowring_block(struct brcmf_flowring 
> *flow, u16 flowid,
>  
>  void brcmf_flowring_delete(struct brcmf_flowring *flow, u16 flowid)
>  {
> + struct brcmf_bus *bus_if = dev_get_drvdata(flow->dev);
>   struct brcmf_flowring_ring *ring;
> + struct brcmf_if *ifp;
>   u16 hash_idx;
> + u8 ifidx;
>   struct sk_buff *skb;
>  
>   ring = flow->rings[flowid];
>   if (!ring)
>   return;
> +
> + ifidx = brcmf_flowring_ifidx_get(flow, flowid);
> + ifp = brcmf_get_ifp(bus_if->drvr, ifidx);
> +
>   brcmf_flowring_block(flow, flowid, false);
>   hash_idx = ring->hash_id;
>   flow->hash[hash_idx].ifidx = BRCMF_FLOWRING_INVALID_IFIDX;

I am not very familiar with flowring code, but I suppose this is just
initializing the entry for later use, right?

> @@ -249,7 +256,7 @@ void brcmf_flowring_delete(struct brcmf_flowring *flow, 
> u16 flowid)
>  
>   skb = skb_dequeue(>skblist);
>   while (skb) {
> - brcmu_pkt_buf_free_skb(skb);
> + brcmf_txfinalize(ifp, skb, false);
>   skb = skb_dequeue(>skblist);
>   }
>  
> 

Re: [PATCH] brcmfmac: implement more accurate skb tracking

[Arend Van Spriel]

On 26-9-2016 14:38, Rafał Miłecki wrote:
> On 26 September 2016 at 14:13, Rafał Miłecki <zaj...@gmail.com> wrote:
>> On 26 September 2016 at 13:46, Arend Van Spriel
>> <arend.vanspr...@broadcom.com> wrote:
>>> On 26-9-2016 12:23, Rafał Miłecki wrote:
>>>> From: Rafał Miłecki <ra...@milecki.pl>
>>>>
>>>> We need to track 802.1x packets to know if there are any pending ones
>>>> for transmission. This is required for performing key update in the
>>>> firmware.
>>>
>>> The problem we are trying to solve is a pretty old one. The problem is
>>> that wpa_supplicant uses two separate code paths: EAPOL messaging
>>> through data path and key configuration though nl80211.
>>
>> Can I find it described/reported somewhere?
>>
>>
>>>> Unfortunately our old tracking code wasn't very accurate. It was
>>>> treating skb as pending as soon as it was passed by the netif. Actual
>>>> handling packet to the firmware was happening later as brcmfmac
>>>> internally queues them and uses its own worker(s).
>>>
>>> That does not seem right. As soon as we get a 1x packet we need to wait
>>> with key configuration regardless whether it is still in the driver or
>>> handed over to firmware already.
>>
>> OK, thanks.
> 
> Actually, it's not OK. I was trying to report/describe/discuss this
> problem for over a week. I couldn't get much of answer from you.
> 
> I had to come with a patch I worked on for quite some time. Only then
> you decided to react and reply with a reason for a nack. I see this
> patch may be wrong (but it's still hard to know what's going wrong
> without a proper hostapd bug report). I'd expect you to somehow work &
> communicate with open source community.

We do or at least make an honest attempt, but there is more on our plate
so responses may be delayed. It also does not help when you get anal and
preachy when we do respond. Also not OK. In this case the delay is
caused because I had to pick up the thread(s) as Hante is on vacation
(he needed a break :-p ). However, you started sending patches so I
decided to look at and respond to those. Sorry if you felt like we left
you hanging to dry.

Regards,
Arend

Re: [PATCH] brcmfmac: implement more accurate skb tracking

[Arend Van Spriel]

On 26-9-2016 16:59, Dan Williams wrote:
> On Mon, 2016-09-26 at 14:13 +0200, Rafał Miłecki wrote:
>> On 26 September 2016 at 13:46, Arend Van Spriel
>> <arend.vanspr...@broadcom.com> wrote:
>>>
>>> On 26-9-2016 12:23, Rafał Miłecki wrote:
>>>>
>>>> From: Rafał Miłecki <ra...@milecki.pl>
>>>>
>>>> We need to track 802.1x packets to know if there are any pending
>>>> ones
>>>> for transmission. This is required for performing key update in
>>>> the
>>>> firmware.
>>>
>>> The problem we are trying to solve is a pretty old one. The problem
>>> is
>>> that wpa_supplicant uses two separate code paths: EAPOL messaging
>>> through data path and key configuration though nl80211.
>>
>> Can I find it described/reported somewhere?
> 
> If I understand the issue correctly, you can find all this in the
> supplicant code.  Once the supplicant has done whatever it wants to do
> with the data frames that just happen to be EAPOL it then sends the
> keys down to the driver with nl80211.

Indeed. EAPOL packets are simply data packets as far as the 802.11 stack
is concerned. The arrival of those in the driver is not predictable
hence we hold off the key configuration until those have been passed
over to firmware.

> But it sounds like, instead of sniffing EAPOL frames in the driver skb
> tracking and sniffing ETH_P_PAE, you should probably implement support
> for NL80211_CMD_CRIT_PROTOCOL_START/NL80211_CMD_CRIT_PROTOCOL_STOP and
> key off the passed-in NL80211_CRIT_PROTO_EAPOL.  At least at the
> beginning of connection setup only EAPOL packets will be allowed
> anyway.
> 
> It doesn't seem like the supplicant uses NL80211_CRIT_PROTO_EAPOL yet,
> but that should also be fixed in the supplicant itself.  You should
> probably get some comments from Jouni on how he'd like to see all this
> work.  But generally the less specific sniffing of frames in drivers,
> likely the better.

Indeed. That was the main motivation to introduce the CRIT_PROTO api. If
I recall correctly it was considered the task of the network manager to
issue the START/STOP. Recently noticed the use of CRIT_PROTO_DHCP on
some target system, which we already support in brcmfmac. From your
response I guess you consider CRIT_PROTO_EAPOL to be issued by the
supplicant.

Regards,
Arend

> Dan
> 
>>
>>>
>>>>
>>>> Unfortunately our old tracking code wasn't very accurate. It was
>>>> treating skb as pending as soon as it was passed by the netif.
>>>> Actual
>>>> handling packet to the firmware was happening later as brcmfmac
>>>> internally queues them and uses its own worker(s).
>>>
>>> That does not seem right. As soon as we get a 1x packet we need to
>>> wait
>>> with key configuration regardless whether it is still in the driver
>>> or
>>> handed over to firmware already.
>>
>> OK, thanks.

Re: [PATCH] brcmfmac: implement more accurate skb tracking

[Arend Van Spriel]

On 26-9-2016 14:13, Rafał Miłecki wrote:
> On 26 September 2016 at 13:46, Arend Van Spriel
> <arend.vanspr...@broadcom.com> wrote:
>> On 26-9-2016 12:23, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <ra...@milecki.pl>
>>>
>>> We need to track 802.1x packets to know if there are any pending ones
>>> for transmission. This is required for performing key update in the
>>> firmware.
>>
>> The problem we are trying to solve is a pretty old one. The problem is
>> that wpa_supplicant uses two separate code paths: EAPOL messaging
>> through data path and key configuration though nl80211.
> 
> Can I find it described/reported somewhere?

Not sure. It is something that I recall from working at Intersil so back
in the prism days.

Regards,
Arend

>>> Unfortunately our old tracking code wasn't very accurate. It was
>>> treating skb as pending as soon as it was passed by the netif. Actual
>>> handling packet to the firmware was happening later as brcmfmac
>>> internally queues them and uses its own worker(s).
>>
>> That does not seem right. As soon as we get a 1x packet we need to wait
>> with key configuration regardless whether it is still in the driver or
>> handed over to firmware already.
> 
> OK, thanks.
> 

Re: [PATCH] brcmfmac: implement more accurate skb tracking

[Arend Van Spriel]

On 26-9-2016 12:23, Rafał Miłecki wrote:
> From: Rafał Miłecki 
> 
> We need to track 802.1x packets to know if there are any pending ones
> for transmission. This is required for performing key update in the
> firmware.

The problem we are trying to solve is a pretty old one. The problem is
that wpa_supplicant uses two separate code paths: EAPOL messaging
through data path and key configuration though nl80211.

> Unfortunately our old tracking code wasn't very accurate. It was
> treating skb as pending as soon as it was passed by the netif. Actual
> handling packet to the firmware was happening later as brcmfmac
> internally queues them and uses its own worker(s).

That does not seem right. As soon as we get a 1x packet we need to wait
with key configuration regardless whether it is still in the driver or
handed over to firmware already.

Regards,
Arend

> Other than that it was hard to handle freeing packets. Everytime we had
> to determine (in more generic funcions) if packet was counted as pending
> 802.1x one or not. It was causing some problems, e.g. it wasn't clear if
> brcmf_flowring_delete should free skb directly or not.
> 
> This patch introduces 2 separated functions for tracking skbs. This
> simplifies logic, fixes brcmf_flowring_delete (maybe other hidden bugs
> as well) and allows further simplifications. Thanks to better accuracy
> is also increases time window for key update (and lowers timeout risk).
> 
> Signed-off-by: Rafał Miłecki 
> ---
> This was successfully tested with 4366b1. Can someone give it a try with
> some USB/SDIO device, please?
> ---
>  .../wireless/broadcom/brcm80211/brcmfmac/bcdc.c| 11 +++
>  .../wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c  | 12 +++-
>  .../wireless/broadcom/brcm80211/brcmfmac/core.c| 36 
> --
>  .../wireless/broadcom/brcm80211/brcmfmac/core.h|  2 ++
>  .../wireless/broadcom/brcm80211/brcmfmac/msgbuf.c  | 14 +++--
>  .../wireless/broadcom/brcm80211/brcmfmac/proto.h   | 11 +++
>  .../wireless/broadcom/brcm80211/brcmfmac/sdio.c|  8 +
>  .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 10 ++
>  8 files changed, 91 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> index d1bc51f..3e40244 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> @@ -326,6 +326,16 @@ brcmf_proto_bcdc_hdrpull(struct brcmf_pub *drvr, bool 
> do_fws,
>   return 0;
>  }
>  
> +static int brcmf_proto_bcdc_hdr_get_ifidx(struct brcmf_pub *drvr,
> +   struct sk_buff *skb)
> +{
> + struct brcmf_proto_bcdc_header *h;
> +
> + h = (struct brcmf_proto_bcdc_header *)(skb->data);
> +
> + return BCDC_GET_IF_IDX(h);
> +}
> +
>  static int
>  brcmf_proto_bcdc_txdata(struct brcmf_pub *drvr, int ifidx, u8 offset,
>   struct sk_buff *pktbuf)
> @@ -373,6 +383,7 @@ int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr)
>   }
>  
>   drvr->proto->hdrpull = brcmf_proto_bcdc_hdrpull;
> + drvr->proto->hdr_get_ifidx = brcmf_proto_bcdc_hdr_get_ifidx;
>   drvr->proto->query_dcmd = brcmf_proto_bcdc_query_dcmd;
>   drvr->proto->set_dcmd = brcmf_proto_bcdc_set_dcmd;
>   drvr->proto->txdata = brcmf_proto_bcdc_txdata;
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> index 03404cb..fef9d02 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> @@ -43,6 +43,7 @@
>  #include "chip.h"
>  #include "bus.h"
>  #include "debug.h"
> +#include "proto.h"
>  #include "sdio.h"
>  #include "core.h"
>  #include "common.h"
> @@ -772,6 +773,7 @@ int brcmf_sdiod_send_buf(struct brcmf_sdio_dev *sdiodev, 
> u8 *buf, uint nbytes)
>  int brcmf_sdiod_send_pkt(struct brcmf_sdio_dev *sdiodev,
>struct sk_buff_head *pktq)
>  {
> + struct brcmf_pub *pub = sdiodev->bus_if->drvr;
>   struct sk_buff *skb;
>   u32 addr = sdiodev->sbwad;
>   int err;
> @@ -784,10 +786,18 @@ int brcmf_sdiod_send_pkt(struct brcmf_sdio_dev *sdiodev,
>  
>   if (pktq->qlen == 1 || !sdiodev->sg_support)
>   skb_queue_walk(pktq, skb) {
> + struct brcmf_if *ifp;
> + int ifidx;
> +
> + ifidx = brcmf_proto_hdr_get_ifidx(pub, skb);
> + ifp = brcmf_get_ifp(pub, ifidx);
> + brcmf_tx_passing_skb(ifp, skb);
>   err = brcmf_sdiod_buffrw(sdiodev, SDIO_FUNC_2, true,
>addr, skb);
> - if (err)
> + if (err) {
> +