[PATCH 3.16 58/76] nl80211: Sanitize array index in parse_txq_params

[Ben Hutchings]

3.16.56-rc1 review patch.  If anyone has any objections, please let me know.

--

From: Dan Williams 

commit 259d8c1e984318497c84eef547bbb6b1d9f4eb05 upstream.

Wireless drivers rely on parse_txq_params to validate that txq_params->ac
is less than NL80211_NUM_ACS by the time the low-level driver's ->conf_tx()
handler is called. Use a new helper, array_index_nospec(), to sanitize
txq_params->ac with respect to speculation. I.e. ensure that any
speculation into ->conf_tx() handlers is done with a value of
txq_params->ac that is within the bounds of [0, NL80211_NUM_ACS).

Reported-by: Christian Lamparter 
Reported-by: Elena Reshetova 
Signed-off-by: Dan Williams 
Signed-off-by: Thomas Gleixner 
Acked-by: Johannes Berg 
Cc: linux-a...@vger.kernel.org
Cc: kernel-harden...@lists.openwall.com
Cc: gre...@linuxfoundation.org
Cc: linux-wireless@vger.kernel.org
Cc: torva...@linux-foundation.org
Cc: "David S. Miller" 
Cc: a...@linux.intel.com
Link: 
https://lkml.kernel.org/r/151727419584.33451.7700736761686184303.st...@dwillia2-desk3.amr.corp.intel.com
Signed-off-by: Ben Hutchings 
---
 net/wireless/nl80211.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -14,6 +14,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1829,20 +1830,22 @@ static const struct nla_policy txq_param
 static int parse_txq_params(struct nlattr *tb[],
struct ieee80211_txq_params *txq_params)
 {
+   u8 ac;
+
if (!tb[NL80211_TXQ_ATTR_AC] || !tb[NL80211_TXQ_ATTR_TXOP] ||
!tb[NL80211_TXQ_ATTR_CWMIN] || !tb[NL80211_TXQ_ATTR_CWMAX] ||
!tb[NL80211_TXQ_ATTR_AIFS])
return -EINVAL;
 
-   txq_params->ac = nla_get_u8(tb[NL80211_TXQ_ATTR_AC]);
+   ac = nla_get_u8(tb[NL80211_TXQ_ATTR_AC]);
txq_params->txop = nla_get_u16(tb[NL80211_TXQ_ATTR_TXOP]);
txq_params->cwmin = nla_get_u16(tb[NL80211_TXQ_ATTR_CWMIN]);
txq_params->cwmax = nla_get_u16(tb[NL80211_TXQ_ATTR_CWMAX]);
txq_params->aifs = nla_get_u8(tb[NL80211_TXQ_ATTR_AIFS]);
 
-   if (txq_params->ac >= NL80211_NUM_ACS)
+   if (ac >= NL80211_NUM_ACS)
return -EINVAL;
-
+   txq_params->ac = array_index_nospec(ac, NL80211_NUM_ACS);
return 0;
 }
 

Re: [RESEND PATCH] rsi: Remove stack VLA usage

[Larry Finger]

On 03/11/2018 08:43 PM, Tobin C. Harding wrote:

The kernel would like to have all stack VLA usage removed[1].  rsi uses
a VLA based on 'blksize'.  Elsewhere in the SDIO code maximum block size
is defined using a magic number.  We can use a pre-processor defined
constant and declare the array to maximum size.  We add a check before
accessing the array in case of programmer error.

[1]: https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Tobin C. Harding 
---

RESEND: add wireless mailing list to CC's (requested by Kalle)

  drivers/net/wireless/rsi/rsi_91x_hal.c  | 13 +++--
  drivers/net/wireless/rsi/rsi_91x_sdio.c |  9 +++--
  2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_hal.c 
b/drivers/net/wireless/rsi/rsi_91x_hal.c
index 1176de646942..839ebdd602df 100644
--- a/drivers/net/wireless/rsi/rsi_91x_hal.c
+++ b/drivers/net/wireless/rsi/rsi_91x_hal.c
@@ -641,7 +641,7 @@ static int ping_pong_write(struct rsi_hw *adapter, u8 cmd, 
u8 *addr, u32 size)
u32 cmd_addr;
u16 cmd_resp, cmd_req;
u8 *str;
-   int status;
+   int status, ret;
  
  	if (cmd == PING_WRITE) {

cmd_addr = PING_BUFFER_ADDRESS;
@@ -655,12 +655,13 @@ static int ping_pong_write(struct rsi_hw *adapter, u8 
cmd, u8 *addr, u32 size)
str = "PONG_VALID";
}
  
-	status = hif_ops->load_data_master_write(adapter, cmd_addr, size,

+   ret = hif_ops->load_data_master_write(adapter, cmd_addr, size,
block_size, addr);
-   if (status) {
-   rsi_dbg(ERR_ZONE, "%s: Unable to write blk at addr %0x\n",
-   __func__, *addr);
-   return status;
+   if (ret) {
+   if (ret != -EINVAL)
+   rsi_dbg(ERR_ZONE, "%s: Unable to write blk at addr 
%0x\n",
+   __func__, *addr);
+   return ret;
}
  
  	status = bl_cmd(adapter, cmd_req, cmd_resp, str);

diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c 
b/drivers/net/wireless/rsi/rsi_91x_sdio.c
index b0cf41195051..b766578b591a 100644
--- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
+++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
@@ -20,6 +20,8 @@
  #include "rsi_common.h"
  #include "rsi_hal.h"
  
+#define RSI_MAX_BLOCK_SIZE 256

+
  /**
   * rsi_sdio_set_cmd52_arg() - This function prepares cmd 52 read/write arg.
   * @rw: Read/write
@@ -362,7 +364,7 @@ static int rsi_setblocklength(struct rsi_hw *adapter, u32 
length)
rsi_dbg(INIT_ZONE, "%s: Setting the block length\n", __func__);
  
  	status = sdio_set_block_size(dev->pfunction, length);

-   dev->pfunction->max_blksize = 256;
+   dev->pfunction->max_blksize = RSI_MAX_BLOCK_SIZE;
adapter->block_size = dev->pfunction->max_blksize;
  
  	rsi_dbg(INFO_ZONE,

@@ -567,9 +569,12 @@ static int rsi_sdio_load_data_master_write(struct rsi_hw 
*adapter,
  {
u32 num_blocks, offset, i;
u16 msb_address, lsb_address;
-   u8 temp_buf[block_size];
+   u8 temp_buf[RSI_MAX_BLOCK_SIZE];
int status;
  
+	if (block_size > RSI_MAX_BLOCK_SIZE)

+   return -EINVAL;
+
num_blocks = instructions_sz / block_size;
msb_address = base_address >> 16;


I am not giving this patch a negative review, but my solution to the same 
problem has been to change the on-stack array into a u8 pointer, use kmalloc() 
to assign the space, and then free that space at the end. That way large stack 
allocations are avoided, with a minimum of changes.


Larry

  

[RESEND PATCH] rsi: Remove stack VLA usage

[Tobin C. Harding]

The kernel would like to have all stack VLA usage removed[1].  rsi uses
a VLA based on 'blksize'.  Elsewhere in the SDIO code maximum block size
is defined using a magic number.  We can use a pre-processor defined
constant and declare the array to maximum size.  We add a check before
accessing the array in case of programmer error.

[1]: https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Tobin C. Harding 
---

RESEND: add wireless mailing list to CC's (requested by Kalle)

 drivers/net/wireless/rsi/rsi_91x_hal.c  | 13 +++--
 drivers/net/wireless/rsi/rsi_91x_sdio.c |  9 +++--
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_hal.c 
b/drivers/net/wireless/rsi/rsi_91x_hal.c
index 1176de646942..839ebdd602df 100644
--- a/drivers/net/wireless/rsi/rsi_91x_hal.c
+++ b/drivers/net/wireless/rsi/rsi_91x_hal.c
@@ -641,7 +641,7 @@ static int ping_pong_write(struct rsi_hw *adapter, u8 cmd, 
u8 *addr, u32 size)
u32 cmd_addr;
u16 cmd_resp, cmd_req;
u8 *str;
-   int status;
+   int status, ret;
 
if (cmd == PING_WRITE) {
cmd_addr = PING_BUFFER_ADDRESS;
@@ -655,12 +655,13 @@ static int ping_pong_write(struct rsi_hw *adapter, u8 
cmd, u8 *addr, u32 size)
str = "PONG_VALID";
}
 
-   status = hif_ops->load_data_master_write(adapter, cmd_addr, size,
+   ret = hif_ops->load_data_master_write(adapter, cmd_addr, size,
block_size, addr);
-   if (status) {
-   rsi_dbg(ERR_ZONE, "%s: Unable to write blk at addr %0x\n",
-   __func__, *addr);
-   return status;
+   if (ret) {
+   if (ret != -EINVAL)
+   rsi_dbg(ERR_ZONE, "%s: Unable to write blk at addr 
%0x\n",
+   __func__, *addr);
+   return ret;
}
 
status = bl_cmd(adapter, cmd_req, cmd_resp, str);
diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c 
b/drivers/net/wireless/rsi/rsi_91x_sdio.c
index b0cf41195051..b766578b591a 100644
--- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
+++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
@@ -20,6 +20,8 @@
 #include "rsi_common.h"
 #include "rsi_hal.h"
 
+#define RSI_MAX_BLOCK_SIZE 256
+
 /**
  * rsi_sdio_set_cmd52_arg() - This function prepares cmd 52 read/write arg.
  * @rw: Read/write
@@ -362,7 +364,7 @@ static int rsi_setblocklength(struct rsi_hw *adapter, u32 
length)
rsi_dbg(INIT_ZONE, "%s: Setting the block length\n", __func__);
 
status = sdio_set_block_size(dev->pfunction, length);
-   dev->pfunction->max_blksize = 256;
+   dev->pfunction->max_blksize = RSI_MAX_BLOCK_SIZE;
adapter->block_size = dev->pfunction->max_blksize;
 
rsi_dbg(INFO_ZONE,
@@ -567,9 +569,12 @@ static int rsi_sdio_load_data_master_write(struct rsi_hw 
*adapter,
 {
u32 num_blocks, offset, i;
u16 msb_address, lsb_address;
-   u8 temp_buf[block_size];
+   u8 temp_buf[RSI_MAX_BLOCK_SIZE];
int status;
 
+   if (block_size > RSI_MAX_BLOCK_SIZE)
+   return -EINVAL;
+
num_blocks = instructions_sz / block_size;
msb_address = base_address >> 16;
 
-- 
2.7.4

Re: [PATCH] firmware: add a function to load optional firmware v2

[Arend van Spriel]

On 3/11/2018 5:05 PM, Andres Rodriguez wrote:

Your patch series then should also have the driver callers who you
want to modify to use this new API. Collect from the 802.11 folks the
other drivers which I think they wanted changed as well.


Arend, Kalle, would love to hear your feedback.


I am not sure if it was ath10k, but Kalle will surely know. The other 
driver firing a whole batch of firmware requests is iwlwifi. These 
basically try to get latest firmware version and if not there try an 
older one.


The brcmfmac driver I maintain is slightly different. It downloads two 
distinct pieces of firmware of which one is optional for certain 
configurations. Currently, my driver does two asynchronous requests for 
it, but I consider changing it and only make the first request 
asynchronous and the second request synchronous. You can look at the 
current code in drivers/net/wireless/broadcom/brcmfmac/firmware.c. 
However, I did quite some restructuring last week. Anyway, I probably 
will end up using the "optional" api where appropriate.


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,

[PATCH] brcmfmac: Add support for getting nvram contents from EFI variables

[Hans de Goede]

Various models Asus laptops with a SDIO attached brcmfmac wifi chip, store
the nvram contents in a special EFI variable. This commit adds support for
getting nvram directly from this EFI variable, without the user needing to
manually copy it.

This makes Wifi / Bluetooth work out of the box on these devices instead of
requiring manual setup.

Note that at least on the Asus T200TA the nvram from the EFI variable
wrongly contains "ccode=ALL" which the firmware does not understand, the
code to fetch the nvram from the EFI variable will fix this up to:
"ccode=XV" which is the correct way to specify the worldwide broadcast
regime.

This has been tested on the following models: Asus T100CHI, Asus T100HA,
Asus T100TA and Asus T200TA

Tested-by: Hans de Goede 
Signed-off-by: Hans de Goede 
---
 .../broadcom/brcm80211/brcmfmac/firmware.c | 68 +-
 1 file changed, 67 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
index 091b52979e03..cbac407ae132 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
@@ -14,6 +14,8 @@
  * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
+#include 
+#include 
 #include 
 #include 
 #include 
@@ -446,6 +448,67 @@ struct brcmf_fw {
 void *nvram_image, u32 nvram_len);
 };
 
+#ifdef CONFIG_EFI
+static u8 *brcmf_fw_nvram_from_efi(size_t *data_len_ret)
+{
+   const u16 name[] = { 'n', 'v', 'r', 'a', 'm', 0 };
+   struct efivar_entry *nvram_efivar;
+   unsigned long data_len = 0;
+   u8 *data = NULL;
+   char *ccode;
+   int err;
+
+   /* So far only Asus devices store the nvram in an EFI var */
+   if (!dmi_name_in_vendors("ASUSTeK COMPUTER INC."))
+   return NULL;
+
+   nvram_efivar = kzalloc(sizeof(*nvram_efivar), GFP_KERNEL);
+   if (!nvram_efivar)
+   return NULL;
+
+   memcpy(&nvram_efivar->var.VariableName, name, sizeof(name));
+   nvram_efivar->var.VendorGuid = EFI_GUID(0x74b00bd9, 0x805a, 0x4d61,
+   0xb5, 0x1f, 0x43, 0x26,
+   0x81, 0x23, 0xd1, 0x13);
+
+   err = efivar_entry_size(nvram_efivar, &data_len);
+   if (err)
+   goto fail;
+
+   data = kmalloc(data_len, GFP_KERNEL);
+   if (!data)
+   goto fail;
+
+   err = efivar_entry_get(nvram_efivar, NULL, &data_len, data);
+   if (err)
+   goto fail;
+
+   /* In some cases the EFI-var stored nvram contains "ccode=ALL" but
+* the firmware does not understand "ALL" change this to "XV" which
+* is the correct way to specify the "worldwide" compatible settings.
+*/
+   ccode = strnstr((char *)data, "ccode=ALL", data_len);
+   if (ccode) {
+   ccode[6] = 'X';
+   ccode[7] = 'V';
+   ccode[8] = '\n';
+   }
+
+   brcmf_info("Using nvram EFI variable\n");
+
+   kfree(nvram_efivar);
+   *data_len_ret = data_len;
+   return data;
+
+fail:
+   kfree(data);
+   kfree(nvram_efivar);
+   return NULL;
+}
+#else
+static u8 *brcmf_fw_nvram_from_efi(size_t *data_len) { return NULL; }
+#endif
+
 static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
 {
struct brcmf_fw *fwctx = ctx;
@@ -462,6 +525,8 @@ static void brcmf_fw_request_nvram_done(const struct 
firmware *fw, void *ctx)
raw_nvram = false;
} else {
data = bcm47xx_nvram_get_contents(&data_len);
+   if (!data)
+   data = brcmf_fw_nvram_from_efi(&data_len);
if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
goto fail;
raw_nvram = true;
@@ -472,7 +537,8 @@ static void brcmf_fw_request_nvram_done(const struct 
firmware *fw, void *ctx)
 fwctx->domain_nr, fwctx->bus_nr);
 
if (raw_nvram)
-   bcm47xx_nvram_release_contents(data);
+   kvfree(data); /* vfree for bcm47xx case / kfree for efi case */
+
release_firmware(fw);
if (!nvram && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
goto fail;
-- 
2.14.3

Re: [PATCH] ath10k: fix recent bandwidth conversion bug

[Rafał Miłecki]

On 11 March 2018 at 08:12, Kalle Valo  wrote:
> Rafał Miłecki  writes:
>
>> On 14 December 2017 at 14:21, Kalle Valo  wrote:
>>> Christian Lamparter  writes:
>>>
 On Monday, November 20, 2017 11:57:21 AM CET Kalle Valo wrote:
> Christian Lamparter  writes:
>
> > On Wednesday, November 1, 2017 9:37:53 PM CET Sebastian Gottschall 
> > wrote:
> >> a additional array bounds check would be good
> >
> > Ah, about that:
> >
> > the bw variable in ath10k_htt_rx_h_rates() is extracted from info2
> > in the following way [0]:
> > |  bw = info2 & 3;
> >
> > the txrate.bw variable in ath10k_update_per_peer_tx_stats() is set by 
> > [1]:
> > |  txrate.bw = ATH10K_HW_BW(peer_stats->flags);
> >
> > ATH10K_HW_BW is a macro defined as [2]:
> > |  #define ATH10K_HW_BW(flags) (((flags) >> 3) & 0x3)
> >
> > In both cases the bandwidth values already are limited to 0-3 by
> > the "and 3" operation.
>
> Until someone changes that part of the code (and the firmware
> interface). IMHO a switch is safer as there we don't have any risk of
> out of bands access.

 The kbuild-bot/CI can catch this too.

 For example, it will look like this:
 drivers/net/wireless/ath/ath10k//htt_rx.c:710:52: warning: invalid
 access past the end of 'ath10k_bw_to_mac80211' (4 4)
>>>
>>> Sure, but after reading about all these security vulnerabilities I have
>>> become even more cautious and try to avoid all tricky stuff.
>>>
 BTW:
 Have you noticed:

 

 Is this really your signed-off-by or not?
>>>
>>> I suspect that patch is taken from my pending branch.
>>>
 In any case, you - as the maintainer - can modify the patch as
 you see fit. So, please do so.
>>>
>>> Ok, we'll send v2.
>>
>> Hi Kalle,
>>
>> I'm trying to figure out the fate of that LEDE's patch. I don't think
>> you ever sent V2.
>>
>> Is that fix still needed? Are you planning to send V2?
>
> Anil now sent v2 (he just forgot to mark it as such):
>
> https://patchwork.kernel.org/patch/10273445/
>
> Thanks for the reminder.

Thanks!

-- 
Rafał

Re: [PATCH] firmware: add a function to load optional firmware v2

[Andres Rodriguez]

Hi Luis,

Thanks for all your feedback, greatly appreciated.

On 2018-03-10 09:40 AM, Luis R. Rodriguez wrote:

On Sat, Mar 10, 2018 at 6:35 AM, Luis R. Rodriguez  wrote:

You also I take it have users in
mind? I'd like to see at least one user of the API or this fixing a
reported issue. Ie, if users have reported this as issues incorrectly,
referring to those incorrect posts as issues and how this created
confusion would help.


The current user I have in mind is amdgpu. I've got some local patches 
for changing it to use request_firmware_optional() for the optional 
firmware files. I will include them in the v3 of this series.


I've also queried some devs from the other DRM drivers in case this 
might be useful to them. So far I've gotten a reply from the nouveau 
devs who are also interested.




Your patch series then should also have the driver callers who you
want to modify to use this new API. Collect from the 802.11 folks the
other drivers which I think they wanted changed as well.


Arend, Kalle, would love to hear your feedback.


The old up on
that front was that the firmware API was in a huge state of flux and
debate about *how* we'd evolve the API, either through a data driven
API or functional driven API, ie whether or not we'd add a flexible
one API call with a set of options, or keep extending functionality
with new exported symbols per use case. The later is how we'd keep
evolving the API as such the way you are doing it is fine. Ie, if
there is a use case for an optional firmware also for the async case a
new API call will have to be made. As stupid as this sounds.



Seems like I got lucky with my timing for this request :)



Also please take a look at lib/test_firmware.c -- I don't think it
makes sense to add a new test case for this API call, so at least
worth documenting why somewhere if you find a suitable place for that.
> Also - I forgot to ask you to extend the
Documentation/driver-api/firmware/ documentation accordingly. Please
do that.



Will do, for these and the feedback in the previous Email.

-Andres


   Luis

Re: [PATCH] wcn36xx: Add support for FTM WLAN

[Ramon Fried]

On 3/10/2018 11:45 AM, Kalle Valo wrote:
> Ramon Fried  writes:
>
>> From: Eyal Ilsar 
>>
>> Introduced infrastructure for supporting FTM WLAN in user space exposing
>> the netlink channel from the kernel WLAN driver.
> What's FTM WLAN? Don't assume that people know all the acronyms.
FTM is factory test mode if I recall correctly, but you're right. I elaborate 
more in the commit message.
>
> This included:
>> 1) Registered wcn36xx driver to testmode callback from netlink
>> 2) Added testmode callback implementation to handle incoming FTM commands
>> 3) Added FTM command packet structure
>> 4) Added handling for GET_BUILD_RELEASE_NUMBER (msgid=0x32A2)
>> 5) Added generic handling for all PTT_MSG packets
> I don't remember the english grammar terminology, but in the commit log
> use the form "register", "add" instead of "registered", "added".
OK. Will fix.
>> +struct wcn36xx_hal_process_ptt_msg_rsp_msg {
>> +struct wcn36xx_hal_msg_header header;
>> +
>> +/* FTM Command response status */
>> +u32   ptt_msg_resp_status;
> Unnecessary whitespace after u32.
Thanks.

>> +static int wcn36xx_smd_process_ptt_msg_rsp(void *buf, size_t len,
>> +void **p_ptt_rsp_msg)
>> +{
>> +struct wcn36xx_hal_process_ptt_msg_rsp_msg *rsp;
>> +int ret = 0;
>> +
>> +ret = wcn36xx_smd_rsp_status_check(buf, len);
>> +if (ret)
>> +return ret;
>> +rsp = (struct wcn36xx_hal_process_ptt_msg_rsp_msg *)buf;
>> +wcn36xx_dbg(WCN36XX_DBG_HAL, "process ptt msg responded with length 
>> %d\n",
>> +rsp->header.len);
>> +wcn36xx_dbg_dump(WCN36XX_DBG_HAL_DUMP, "HAL_PTT_MSG_RSP:", rsp->ptt_msg,
>> +rsp->header.len - sizeof(rsp->ptt_msg_resp_status));
>> +if (rsp->header.len > 0) {
>> +*p_ptt_rsp_msg = kmalloc(rsp->header.len, GFP_ATOMIC);
>> +memcpy(*p_ptt_rsp_msg, rsp->ptt_msg, rsp->header.len);
>> +}
>> +return ret;
>> +}
> Adding few empty lines to the function would make it more readable.
Ok, I will add.
>> @@ -2330,6 +2399,7 @@ int wcn36xx_smd_rsp_process(struct rpmsg_device *rpdev,
>>  struct ieee80211_hw *hw = priv;
>>  struct wcn36xx *wcn = hw->priv;
>>  struct wcn36xx_hal_ind_msg *msg_ind;
>> +
>>  wcn36xx_dbg_dump(WCN36XX_DBG_SMD_DUMP, "SMD <<< ", buf, len);
> Unrelated change.
Nice catch. thanks.
>> +int wcn36xx_tm_cmd(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>> +  void *data, int len)
>> +{
>> +struct wcn36xx *wcn = hw->priv;
>> +struct nlattr *tb[WCN36XX_TM_ATTR_MAX + 1];
>> +int ret = 0;
>> +unsigned short attr;
>> +
>> +wcn36xx_dbg_dump(WCN36XX_DBG_TESTMODE_DUMP, "Data:", data, len);
>> +ret = nla_parse(tb, WCN36XX_TM_ATTR_MAX, data, len,
>> +wcn36xx_tm_policy, NULL);
>> +if (ret)
>> +return ret;
>> +
>> +if (!tb[WCN36XX_TM_ATTR_CMD])
>> +return -EINVAL;
>> +
>> +attr = nla_get_u16(tb[WCN36XX_TM_ATTR_CMD]);
>> +
>> +switch (attr) {
>> +case WCN36XX_TM_CMD_START:
>> +case WCN36XX_TM_CMD_STOP:
>> +// N/A to this driver as it does not need to switch state
>> +break;
> [...]
>
>> +enum wcn36xx_tm_cmd {
>> +/* For backwards compatibility
>> + */
>> +WCN36XX_TM_CMD_START = 1,
>> +
>> +/* For backwards compatibility
>> + */
>> +WCN36XX_TM_CMD_STOP = 2,
> This looks odd. If wcn36xx does not need START and STOP commands why add
> those in the first place?
AFAIK the user-space app sends these commands, but I will check again. in 
WCN36xx unlike ATH10k it's not needed to do any transition or replace the 
firmware
to go in test mode, but I assume that if this command arrives we should return 
success.

>

[PATCH] wcn36xx: calculate DXE default channel values

[Ramon Fried]

DXE channel defaults used hardcoded magic values.
Added bit definitions of the control register and
calculate this values in compilation for clarity.

Signed-off-by: Ramon Fried 
---
 drivers/net/wireless/ath/wcn36xx/dxe.h | 104 +++--
 1 file changed, 99 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.h 
b/drivers/net/wireless/ath/wcn36xx/dxe.h
index 73a14953920d..feb3cb7ee81f 100644
--- a/drivers/net/wireless/ath/wcn36xx/dxe.h
+++ b/drivers/net/wireless/ath/wcn36xx/dxe.h
@@ -140,12 +140,106 @@ H2H_TEST_RX_TX = DMA2
 #define WCN36XX_DXE_WQ_RX_L0xB
 #define WCN36XX_DXE_WQ_RX_H0x4
 
-/* TODO This must calculated properly but not hardcoded */
+/* Channel enable or restart */
+#define WCN36xx_DXE_CH_CTRL_EN BIT(0)
+/* End of packet bit */
+#define WCN36xx_DXE_CH_CTRL_EOPBIT(3)
+/* BD Handling bit */
+#define WCN36xx_DXE_CH_CTRL_BDHBIT(4)
+/* Source is queue */
+#define WCN36xx_DXE_CH_CTRL_SIQBIT(5)
+/* Destination is queue */
+#define WCN36xx_DXE_CH_CTRL_DIQBIT(6)
+/* Pointer descriptor is queue */
+#define WCN36xx_DXE_CH_CTRL_PIQBIT(7)
+/* Relase PDU when done */
+#define WCN36xx_DXE_CH_CTRL_PDU_RELBIT(8)
+/* Stop channel processing */
+#define WCN36xx_DXE_CH_CTRL_STOP   BIT(16)
+/* Enable external descriptor interrupt */
+#define WCN36xx_DXE_CH_CTRL_INE_ED BIT(17)
+/* Enable channel interrupt on errors */
+#define WCN36xx_DXE_CH_CTRL_INE_ERRBIT(18)
+/* Enable Channel interrupt when done */
+#define WCN36xx_DXE_CH_CTRL_INE_DONE   BIT(19)
+/* External descriptor enable */
+#define WCN36xx_DXE_CH_CTRL_EDEN   BIT(20)
+/* Wait for valid bit */
+#define WCN36xx_DXE_CH_CTRL_EDVEN  BIT(21)
+/* Endianness is little endian*/
+#define WCN36xx_DXE_CH_CTRL_ENDIANNESS BIT(26)
+/* Abort transfer */
+#define WCN36xx_DXE_CH_CTRL_ABORT  BIT(27)
+/* Long descriptor format */
+#define WCN36xx_DXE_CH_CTRL_DFMT   BIT(28)
+/* Endian byte swap enable */
+#define WCN36xx_DXE_CH_CTRL_SWAP   BIT(31)
+
+/* Transfer type */
+#define WCN36xx_DXE_CH_CTRL_XTYPE_SHIFT 1
+#define WCN36xx_DXE_CH_CTRL_XTYPE_MASK GENMASK(2, 
WCN36xx_DXE_CH_CTRL_XTYPE_SHIFT)
+#define WCN36xx_DXE_CH_CTRL_XTYPE_SET(x)   ((x) << 
WCN36xx_DXE_CH_CTRL_XTYPE_SHIFT)
+
+/* Channel BMU Threshold select */
+#define WCN36xx_DXE_CH_CTRL_BTHLD_SEL_SHIFT 9
+#define WCN36xx_DXE_CH_CTRL_BTHLD_SEL_MASK GENMASK(12, 
WCN36xx_DXE_CH_CTRL_BTHLD_SEL_SHIFT)
+#define WCN36xx_DXE_CH_CTRL_BTHLD_SEL_SET(x) ((x) << 
WCN36xx_DXE_CH_CTRL_BTHLD_SEL_SHIFT)
+
+/* Channel Priority */
+#define WCN36xx_DXE_CH_CTRL_PRIO_SHIFT 13
+#define WCN36xx_DXE_CH_CTRL_PRIO_MASK GENMASK(15, 
WCN36xx_DXE_CH_CTRL_PRIO_SHIFT)
+#define WCN36xx_DXE_CH_CTRL_PRIO_SET(x) ((x) << WCN36xx_DXE_CH_CTRL_PRIO_SHIFT)
+
+/* Counter select */
+#define WCN36xx_DXE_CH_CTRL_SEL_SHIFT 22
+#define WCN36xx_DXE_CH_CTRL_SEL_MASK GENMASK(25, WCN36xx_DXE_CH_CTRL_SEL_SHIFT)
+#define WCN36xx_DXE_CH_CTRL_SEL_SET(x) ((x) << WCN36xx_DXE_CH_CTRL_SEL_SHIFT)
+
+/* Channel BD template index */
+#define WCN36xx_DXE_CH_CTRL_BDT_IDX_SHIFT 29
+#define WCN36xx_DXE_CH_CTRL_BDT_IDX_MASK GENMASK(30, 
WCN36xx_DXE_CH_CTRL_BDT_IDX_SHIFT)
+#define WCN36xx_DXE_CH_CTRL_BDT_IDX_SET(x) ((x) << 
WCN36xx_DXE_CH_CTRL_BDT_IDX_SHIFT)
+
 /* DXE default control register values */
-#define WCN36XX_DXE_CH_DEFAULT_CTL_RX_L0x847EAD2F
-#define WCN36XX_DXE_CH_DEFAULT_CTL_RX_H0x84FED12F
-#define WCN36XX_DXE_CH_DEFAULT_CTL_TX_H0x853ECF4D
-#define WCN36XX_DXE_CH_DEFAULT_CTL_TX_L0x843e8b4d
+#define WCN36XX_DXE_CH_DEFAULT_CTL_RX_L (WCN36xx_DXE_CH_CTRL_EN | \
+   WCN36xx_DXE_CH_CTRL_XTYPE_SET(WCN36xx_DXE_XTYPE_B2H) | \
+   WCN36xx_DXE_CH_CTRL_EOP | WCN36xx_DXE_CH_CTRL_SIQ | \
+   WCN36xx_DXE_CH_CTRL_PDU_REL | 
WCN36xx_DXE_CH_CTRL_BTHLD_SEL_SET(6) | \
+   WCN36xx_DXE_CH_CTRL_PRIO_SET(5) | WCN36xx_DXE_CH_CTRL_INE_ED | \
+   WCN36xx_DXE_CH_CTRL_INE_ERR | WCN36xx_DXE_CH_CTRL_INE_DONE | \
+   WCN36xx_DXE_CH_CTRL_EDEN | WCN36xx_DXE_CH_CTRL_EDVEN | \
+   WCN36xx_DXE_CH_CTRL_SEL_SET(1) | WCN36xx_DXE_CH_CTRL_ENDIANNESS 
| \
+   WCN36xx_DXE_CH_CTRL_SWAP)
+
+#define WCN36XX_DXE_CH_DEFAULT_CTL_RX_H (WCN36xx_DXE_CH_CTRL_EN | \
+   WCN36xx_DXE_CH_CTRL_XTYPE_SET(WCN36xx_DXE_XTYPE_B2H) | \
+   WCN36xx_DXE_CH_CTRL_EOP | WCN36xx_DXE_CH_CTRL_SIQ | \
+   WCN36xx_DXE_CH_CTRL_PDU_REL | 
WCN36xx_DXE_CH_CTRL_BTHLD_SEL_SET(8) | \
+   WCN36xx_DXE_CH_CTRL_PRIO_SET(6) | WCN36xx_DXE_CH_CTRL_INE_ED | \
+   WCN36xx_DXE_CH_CTRL_INE_ERR | WCN36xx_DXE_CH_CTRL_INE_DONE | \
+   WCN36xx_DXE_CH_CTRL_EDEN | WCN36xx_DXE_CH_CTRL_EDVEN | \
+  

[PATCH] wcn36xx: Check DXE IRQ reason

[Ramon Fried]

IRQ reason was not cheked for errors.
Although error handing is not currently supported, it
will be nice to output an error value to the log if the
DMA operation failed.

Signed-off-by: Ramon Fried 
---
 drivers/net/wireless/ath/wcn36xx/dxe.c | 50 +-
 drivers/net/wireless/ath/wcn36xx/dxe.h |  4 +++
 2 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c 
b/drivers/net/wireless/ath/wcn36xx/dxe.c
index 8e4d6d9ea277..7d5ecaf02288 100644
--- a/drivers/net/wireless/ath/wcn36xx/dxe.c
+++ b/drivers/net/wireless/ath/wcn36xx/dxe.c
@@ -415,14 +415,31 @@ static irqreturn_t wcn36xx_irq_tx_complete(int irq, void 
*dev)
  WCN36XX_DXE_CH_STATUS_REG_ADDR_TX_H,
  &int_reason);
 
-   /* TODO: Check int_reason */
-
wcn36xx_dxe_write_register(wcn,
   WCN36XX_DXE_0_INT_CLR,
   WCN36XX_INT_MASK_CHAN_TX_H);
 
-   wcn36xx_dxe_write_register(wcn, WCN36XX_DXE_0_INT_ED_CLR,
-  WCN36XX_INT_MASK_CHAN_TX_H);
+   if (int_reason & WCN36XX_CH_STAT_INT_ERR_MASK ) {
+   wcn36xx_dxe_write_register(wcn,
+  WCN36XX_DXE_0_INT_ERR_CLR,
+  WCN36XX_INT_MASK_CHAN_TX_H);
+
+   wcn36xx_err("DXE IRQ reported error: 0x%x in high TX 
channel\n",
+   int_src);
+   }
+
+   if (int_reason & WCN36XX_CH_STAT_INT_DONE_MASK) {
+   wcn36xx_dxe_write_register(wcn,
+  WCN36XX_DXE_0_INT_DONE_CLR,
+  WCN36XX_INT_MASK_CHAN_TX_H);
+   }
+
+   if (int_reason & WCN36XX_CH_STAT_INT_ED_MASK) {
+   wcn36xx_dxe_write_register(wcn,
+  WCN36XX_DXE_0_INT_ED_CLR,
+  WCN36XX_INT_MASK_CHAN_TX_H);
+   }
+
wcn36xx_dbg(WCN36XX_DBG_DXE, "dxe tx ready high\n");
reap_tx_dxes(wcn, &wcn->dxe_tx_h_ch);
}
@@ -431,14 +448,33 @@ static irqreturn_t wcn36xx_irq_tx_complete(int irq, void 
*dev)
wcn36xx_dxe_read_register(wcn,
  WCN36XX_DXE_CH_STATUS_REG_ADDR_TX_L,
  &int_reason);
-   /* TODO: Check int_reason */
 
wcn36xx_dxe_write_register(wcn,
   WCN36XX_DXE_0_INT_CLR,
   WCN36XX_INT_MASK_CHAN_TX_L);
 
-   wcn36xx_dxe_write_register(wcn, WCN36XX_DXE_0_INT_ED_CLR,
-  WCN36XX_INT_MASK_CHAN_TX_L);
+
+   if (int_reason & WCN36XX_CH_STAT_INT_ERR_MASK ) {
+   wcn36xx_dxe_write_register(wcn,
+  WCN36XX_DXE_0_INT_ERR_CLR,
+  WCN36XX_INT_MASK_CHAN_TX_L);
+
+   wcn36xx_err("DXE IRQ reported error: 0x%x in low TX 
channel\n",
+   int_src);
+   }
+
+   if (int_reason & WCN36XX_CH_STAT_INT_DONE_MASK) {
+   wcn36xx_dxe_write_register(wcn,
+  WCN36XX_DXE_0_INT_DONE_CLR,
+  WCN36XX_INT_MASK_CHAN_TX_L);
+   }
+
+   if (int_reason & WCN36XX_CH_STAT_INT_ED_MASK) {
+   wcn36xx_dxe_write_register(wcn,
+  WCN36XX_DXE_0_INT_ED_CLR,
+  WCN36XX_INT_MASK_CHAN_TX_L);
+   }
+
wcn36xx_dbg(WCN36XX_DBG_DXE, "dxe tx ready low\n");
reap_tx_dxes(wcn, &wcn->dxe_tx_l_ch);
}
diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.h 
b/drivers/net/wireless/ath/wcn36xx/dxe.h
index feb3cb7ee81f..2bc376c5391b 100644
--- a/drivers/net/wireless/ath/wcn36xx/dxe.h
+++ b/drivers/net/wireless/ath/wcn36xx/dxe.h
@@ -262,6 +262,10 @@ H2H_TEST_RX_TX = DMA2
 #define WCN36XX_DXE_0_INT_DONE_CLR (WCN36XX_DXE_MEM_REG + 0x38)
 #define WCN36XX_DXE_0_INT_ERR_CLR  (WCN36XX_DXE_MEM_REG + 0x3C)
 
+#define WCN36XX_CH_STAT_INT_DONE_MASK   0x8000
+#define WCN36XX_CH_STAT_INT_ERR_MASK0x4000
+#define WCN36XX_CH_STAT_INT_ED_MASK 0x2000
+
 #define WCN36XX_DXE_0_CH0_STATUS   (WCN36XX_DXE_MEM_REG + 0x404)
 #define WCN36XX_DXE_0_CH1_STATUS   (WCN36XX_DXE_MEM_REG + 0x444)
 #define WCN36XX_DXE_0_CH2_STATUS   (WCN36X