Re: pull-request: wireless-drivers-next 2016-07-13

[kbuild test robot]

Hi,

[auto build test ERROR on wireless-drivers-next/master]
[cannot apply to v4.7-rc7 next-20160714]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Kalle-Valo/pull-request-wireless-drivers-next-2016-07-13/20160714-023750
base:   
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git 
master
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 5.3.1-8) 5.3.1 20160205
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc64 

All error/warnings (new ones prefixed by >>):

   drivers/net/wireless/marvell/mwifiex/scan.c: In function 
'mwifiex_cancel_scan':
>> drivers/net/wireless/marvell/mwifiex/scan.c:2024:12: error: variable 'info' 
>> has initializer but incomplete type
struct cfg80211_scan_info info = {
   ^
>> drivers/net/wireless/marvell/mwifiex/scan.c:2025:6: error: unknown field 
>> 'aborted' specified in initializer
 .aborted = true,
 ^
>> drivers/net/wireless/marvell/mwifiex/scan.c:2025:17: warning: excess 
>> elements in struct initializer
 .aborted = true,
^
   drivers/net/wireless/marvell/mwifiex/scan.c:2025:17: note: (near 
initialization for 'info')
>> drivers/net/wireless/marvell/mwifiex/scan.c:2024:31: error: storage size of 
>> 'info' isn't known
struct cfg80211_scan_info info = {
  ^
>> drivers/net/wireless/marvell/mwifiex/scan.c:2024:31: warning: unused 
>> variable 'info' [-Wunused-variable]

vim +/info +2024 drivers/net/wireless/marvell/mwifiex/scan.c

  2018  spin_unlock_irqrestore(>mwifiex_cmd_lock, 
cmd_flags);
  2019  for (i = 0; i < adapter->priv_num; i++) {
  2020  priv = adapter->priv[i];
  2021  if (!priv)
  2022  continue;
  2023  if (priv->scan_request) {
> 2024  struct cfg80211_scan_info info = {
> 2025  .aborted = true,
  2026  };
  2027  
  2028  mwifiex_dbg(adapter, INFO,

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: pull-request: wireless-drivers-next 2016-07-13

[David Miller]

From: Kalle Valo 
Date: Wed, 13 Jul 2016 21:29:13 +0300

> here's a pull request for net-next. This time there are few conflicts
> due to the cfg80211 scan API changes, and one of them is easy to miss,
> so please pay extra attention to them. Otherwise there's not nothing
> really out of ordinary. Please note that I also pulled wireless-drivers
> to wireless-drivers-next to reduce the amount of conflicts.
> 
> So about the conflicts, the obvious are notified by git:
 ...
> I have attached the output from git diff as an example how to resolve
> this, hopefully that helps. Please let me know if there are any problems
> or if you want to handle these differently.

Thanks for all of the conflict resolution info, it really helped.

I pushed this all out, please double check my work.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] brcmfmac: Decrease 8021x_cnt for dropped packets

[Per Förlin]

2016-07-14 10:57 GMT+02:00 Arend Van Spriel :
> On 13-7-2016 20:52, Per Förlin wrote:
>> 2016-07-13 13:20 GMT+02:00 Arend Van Spriel :
>>> On 12-7-2016 12:23, Per Förlin wrote:
 2016-07-12 11:48 GMT+02:00 Arend Van Spriel :
>
>
> On 12-7-2016 10:35, Per Förlin wrote:
>> 2016-07-06 11:53 GMT+02:00 Per Förlin :
>>> I have now verified this patch on backports 4.4.
>>>
>>> 2016-04-12 23:55 GMT+02:00  :
 From: Per Forlin 

 This patch resolves an issue where pend_8021x_cnt was not decreased
 on txfinalize. This caused brcmf_netdev_wait_pend8021x to timeout
 because the counter indicated pending packets.

 WARNING: at .../brcmfmac/core.c:1289 brcmf_netdev_wait_pend8021x
   (warn_slowpath_common)
   (warn_slowpath_null)
   (brcmf_netdev_wait_pend8021x [brcmfmac])
   (send_key_to_dongle [brcmfmac])
   (brcmf_cfg80211_del_key [brcmfmac])
   (nl80211_del_key [cfg80211])
   (genl_rcv_msg)
   (netlink_rcv_skb)
   (genl_rcv)
   (netlink_unicast)
   (netlink_sendmsg)
   (sock_sendmsg)
   (___sys_sendmsg)
   (__sys_sendmsg)
   (SyS_sendmsg)

 The solution is to pull back the header offset in case
 of an error in txdata(), which may happen in case of
>> Clarification:
>>
>> txdata=brcmf_proto_bcdc_txdata()
>> brcmf_proto_bcdc_txdata(): Calls brcmf_proto_bcdc_hdrpush()
>>
>> The header needs to be pulled back in case of error otherwise
>> the error handling and cleanup up will fail to decrease the counter
>> of pending packets.
>
> Yes, this part of the patch is clear to me.
>
 Thanks, I wasn't sure.

 packet overload in brcmf_sdio_bus_txdata.

 Overloading an WLAN interface is not an unlikely scenario.
>
> So here is where things start to look suspicious and I have mentioned
> this before. My thought here was "How the hell can you end up with a
> 2048 packets on the sdio queue", which I mentioned to you before. There
> is a high watermark on the queue upon which we do a netif_stop_queue()
> so network layer does not keep pushing tx packets our way. Looking
> further into this I found that we introduced a bug with commit
> 9cd18359d31e ("brcmfmac: Make FWS queueing configurable.") so we ended
> up doing nothing except increasing as statistics debug counter :-(
>
 Is there a fix available for the high watermark issue or is it
 something you will look into?

 To produce a load on the wlan interface I run
 iperf -c 239.255.1.3 -u -b 10m -f m -i 60 -t 3000
 and this is enough in my case to fill up the 2048 queue.

 In case of packet overload the error print "out of bus->txq"
 is very verbose. To reduce SPAM degrade it to a debug print.

 Signed-off-by: Per Forlin 
 ---
 Change log:
  v2 - Add variable to know whether the counter is increased or not
  v3 - txfinalize should decrease the counter. Adjust skb header offset
  v4 - Fix build error

 drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 4 
  drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c | 4 +++-
  drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
  3 files changed, 8 insertions(+), 2 deletions(-)

 diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c 
 b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
 index ed9998b..f342f7c 100644
 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
 +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
 @@ -541,6 +541,9 @@ void brcmf_txfinalize(struct brcmf_if *ifp, struct 
 sk_buff *txp, bool success)
 struct ethhdr *eh;
 u16 type;

 +   if (!ifp)
 +   goto free;
 +
>
> This may not be needed.
>
 This is not strictly needed. I can remove it.

 eh = (struct ethhdr *)(txp->data);
 type = ntohs(eh->h_proto);

 @@ -553,6 +556,7 @@ void brcmf_txfinalize(struct brcmf_if *ifp, struct 
 sk_buff *txp, bool success)
 if (!success)
 ifp->stats.tx_errors++;

 +free:
 brcmu_pkt_buf_free_skb(txp);
  }

 diff --git 
 a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c 
 b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
 index f82c9ab..98cb83f 100644

Re: [PATCH v4 1/3] Documentation: dt: net: add ath9k wireless device binding

[Arnd Bergmann]

On Monday, July 11, 2016 11:21:26 PM CEST Martin Blumenstingl wrote:
> On Mon, Jul 11, 2016 at 12:01 AM, Arnd Bergmann  wrote:
> >> ath9k reads the data from the EEPROM into memory. With that property
> >> disabled ath9k simply assumes that the endianness of the values in the
> >> EEPROM are having the correct endianness for the host system (in other
> >> words: no swap is being applied).
> >> I am not sure I understand you correctly, but isn't what you are
> >> explaining an issue in the ath9k code, rather than in this
> >> documentation?
> >
> > I looked at the code more to find that out now, but I'm more confused
> > now, as the eeprom seems to be read as a byte stream, and the endianess
> > conversion that the driver performs is not on the data values in it,
> > but seems to instead swap the bytes in each 16-bit word, regardless
> > of the contents (the values inside of the byte stream are always
> > interpreted as big-endian). Is that a correct observation?
> that seems to be the case for the ar9003 eeprom. Other implementations
> are doing it different, look at ath9k_hw_ar9287_check_eeprom for
> example: first ath9k_hw_nvram_swap_data checks the two magic bytes at
> the beginning of the data and swaps the bytes in each 16-bit word if
> the magic bytes don't match the magic bytes for the "native system
> endianness" (see AR5416_EEPROM_MAGIC). Then more swapping is applied.
> I asked for more details about the EEPROM format (specifically the
> endianness part) here [0] as I don't have access to the datasheets
> (all I have is the ath9k code)

Ok.

> > What I see in ath_pci_eeprom_read() is that the 16-bit words are taken
> > from the lower 16 bit of the little-endian AR_EEPROM_STATUS_DATA
> > register. As this is coming from a PCI register, it must have a device
> > specific endianess that is identical on all CPUs, so in the description
> > above, mentioning CPU endianess is a bit confusing. I could not find
> > the code that does the conditional byteswap, instead this function
> >
> > static bool ar9300_eeprom_read_byte(struct ath_hw *ah, int address,
> > u8 *buffer)
> > {
> > u16 val;
> >
> > if (unlikely(!ath9k_hw_nvram_read(ah, address / 2, )))
> > return false;
> >
> > *buffer = (val >> (8 * (address % 2))) & 0xff;
> > return true;
> > }
> >
> > evidently assumes that the lower 8 bit of the 16-bit data from PCI
> > come first, i.e. it byteswaps on big-endian CPUs to get the bytestream
> > back into the order in which it is stored in the EEPROM.
> Please have a look at the ath9k_hw_nvram_swap_data function and
> eeprom_ops.check_eeprom (for example ath9k_hw_ar9287_check_eeprom).
> These are performing the conditional swapping (in addition to whatever
> previous swapping there was).
> The basic code works like this: read the full EEPROM data into memory
> (either from PCI bus, ath9k_platform_data or request_firmware), then
> eeprom_ops.check_eeprom will call ath9k_hw_nvram_swap_data for 16-bit
> word swapping and afterwards the check_eeprom implementation will doe
> further swapping.
> Apart from that your findings seem correct (at least this is identical
> to how I would interpret the code).

Ok, so my interpretation of what this is done for is that the
swap in ath9k_hw_nvram_swap_data() is done to compensate for
the data that is read byte-reversed from the PCI bus and it
is does not swap when the data is read from a file. The result
is a structure with big-endian 16-bit and 32-bit members but
all fields in the right place.

The swapping in ath9k_hw_ar9287_check_eeprom() then turns the
big-endian fields into little-endian fields so it can be used
on little-endian CPUs without going through le16_to_cpu().

However, the whole thing still looks fragile to me as it
doesn't seem to handle the case where we want to swap the
values but not the bus.

My guess is that we still want to fix the driver to handle
this more consistently in order to decide whether a DT property
is needed or not.

> > Interestingly, this also seems to happen for ath_ahb_eeprom_read()
> > even though on that one the value does not get swapped by the bus
> > accessor, so presumably big-endian machines with a ahb based ath9k
> > store their eeprom byte-reversed?
> on AHB the eeprom data has to be provided via ath9k_platform_data /
> request_firmware mechanism. Thus there is no bus specific swapping,
> only the ath9k_hw_nvram_swap_data / eeprom_ops.check_eeprom swapping
> is applied in this case.

I guess the header then indicates that none of the swapping is
performed.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] wlcore/wl18xx: mesh: added initial mesh support for wl8

[Kalle Valo]

"Machani, Yaniv"  writes:

> On Tue, Jun 28, 2016 at 13:41:35, Machani, Yaniv wrote:
>> Guy; Johannes Berg; Arik Nemtsov; linux-wireless@vger.kernel.org; 
>> net...@vger.kernel.org
>> Subject: [PATCH] wlcore/wl18xx: mesh: added initial mesh support for 
>> wl8
>> 
>> From: Maital Hahn 
>> 
>> 1. Added support for interface and role of mesh type.
>> 2. Enabled enable/start of mesh-point role,
>>and opening and closing a connection with a mesh peer.
>> 3. Added multirole combination of mesh and ap
>>under the same limits of dual ap mode.
>> 4. Add support for 'sta_rc_update' opcode for mesh IF.
>>The 'sta_rc_update' opcode is being used in mesh_plink.c.
>> Add support in wlcore to handle this opcode correctly for mesh (as 
>> opposed to current implementation that handles STA only).
>> 5. Bumped the firmware version to support new Mesh functionality
>> 
>> Signed-off-by: Maital Hahn 
>> Signed-off-by: Yaniv Machani 
>> ---
>
> Any comments on this patch ? Can this be pulled ?

I'm away this week, will look at it next week. But it's on my queue:

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

-- 
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] staging: rtl8723au: hal: check BT_Active and BT_State with correct bit pattern

[Colin King]

From: Colin Ian King 

BT_Active and BT_State are being masked with 0x00ff so it the subsequent
comparisons with 0x are therefore a buggy check.  Instead, check them
against 0x00ff.

Unfortunately I couldn't find a datasheet or hardware to see if 0x
is an expected invalid bit pattern that should be checked before BT_Active and
BT_State are masked with 0x00ff, so for now, this fix seems like the least
risky approach.

Signed-off-by: Colin Ian King 
---
 drivers/staging/rtl8723au/hal/rtl8723a_bt-coexist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8723au/hal/rtl8723a_bt-coexist.c 
b/drivers/staging/rtl8723au/hal/rtl8723a_bt-coexist.c
index bfcbd7a..6989580 100644
--- a/drivers/staging/rtl8723au/hal/rtl8723a_bt-coexist.c
+++ b/drivers/staging/rtl8723au/hal/rtl8723a_bt-coexist.c
@@ -9824,7 +9824,7 @@ void BTDM_CheckBTIdleChange1Ant(struct rtw_adapter 
*padapter)
BT_Polling = rtl8723au_read32(padapter, regBTPolling);
RTPRINT(FBT, BT_TRACE, ("[DM][BT], BT_Polling(0x%x) =%x\n", 
regBTPolling, BT_Polling));
 
-   if (BT_Active == 0x && BT_State == 0x && BT_Polling == 
0x)
+   if (BT_Active == 0x00ff && BT_State == 0x00ff && BT_Polling == 
0x)
return;
if (BT_Polling == 0)
return;
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: iwlwifi + wpa2-leap + multiple AP's = call trace

[Emmanuel Grumbach]

On Wed, Jul 13, 2016 at 9:33 PM, Ismael Farfán  wrote:
> Hi
>
> I'm using wicd, so changed from wext to nl80211 and I don't see the
> trace anymore... it still refuses to connect though, so I'll check
> what's wrong with my configuration.
>
> Do you want me to collect some information regarding wpa_supplicant
> and wext from my system?
> If so, how?

I am very busy right now, so I doubt  I will have the time to look at
it. So let's drop it unless it still doesn't work for you.

>
>
> 2016-07-13 11:31 GMT-05:00 Emmanuel Grumbach :
>> On Jul 13, 2016 9:23 AM, "Arend Van Spriel" 
>> wrote:
>>>
>>>
>>>
>>> On 13-7-2016 2:29, Ismael Farfán wrote:
>>> > Hello list
>>> >
>>> > I searched this error around and didn't find anything, so here it goes.
>>> >
>>> > Today I tried to connect to an enterprise network, which means,
>>> > literally, tens of AP's sharing the same name... the network requieres
>>> > user/password authentication (wpa2-leap).
>>> >
>>> > I'm using Arch
>>> > $ uname -a
>>> > Linux 4.6.3-1-ARCH #1 SMP PREEMPT Fri Jun 24 21:19:13 CEST 2016 x86_64
>>> > GNU/Linux
>>> >
>>> > Since the thing just didn't connect, I checked dmesg and found this:
>>> >
>>> > Any ideas?
>>>
>>> From the stack trace it seems wpa_supplicant on Arch is using WEXT API.
>>> You could try and change it to use NL80211 API. Personally, I have not
>>> used Arch Linux so no idea where to change that.
>>>
>>> The warning is here [1]:
>>>
>>> 503 IWL_DEBUG_TE(mvm, "Add new TE, duration %d TU\n",
>>> 504  le32_to_cpu(te_cmd->duration));
>>> 505
>>> 506 spin_lock_bh(>time_event_lock);
>>> 507 if (WARN_ON(te_data->id != TE_MAX)) {
>>> 508 spin_unlock_bh(>time_event_lock);
>>> 509 return -EIO;
>>> 510 }
>>>
>>> It seems this function is called with te_data that is already in use,
>>> but that is my uneducated guess so I may be wrong.
>>>
>>
>> This is right :)
>>
>> Can you please record tracing of this?
>>
>> Thank you.
>>
>>> Regards,
>>> Arend
>>>
>>> [1]
>>>
>>> http://lxr.free-electrons.com/source/drivers/net/wireless/intel/iwlwifi/mvm/time-event.c#L507
>>> >
>>> > [   83.030072] wifi0: aborting authentication with xx:xx:xx:xx:xx:xx
>>> > by local choice (Reason: 3=DEAUTH_LEAVING)
>>> > [   83.030073] [ cut here ]
>>> > [   83.030082] WARNING: CPU: 2 PID: 1087 at
>>> > drivers/net/wireless/intel/iwlwifi/mvm/time-event.c:507 iwl_mvm_tim
>>> > e_event_send_add+0x1c6/0x200 [iwlmvm]
>>> > [   83.030157] CPU: 2 PID: 1087 Comm: wpa_supplicant Tainted: G
>>> >O4.6.3-1-ARCH #1
>>> > [   83.030158] Hardware name: Notebook
>>> > P65_P67SA   /P65_P67SA
>>> > , BIOS 1.03.01 07/22/2015
>>> > [   83.030160]  0286 09e998bc 880403a5b940
>>> > 812e54c2
>>> > [   83.030162]    880403a5b980
>>> > 8107a6bb
>>> > [   83.030164]  01fb81a8a180 88041b443580 88041c329548
>>> > fffb
>>> > [   83.030167] Call Trace:
>>> > [   83.030172]  [] dump_stack+0x63/0x81
>>> > [   83.030174]  [] __warn+0xcb/0xf0
>>> > [   83.030176]  [] warn_slowpath_null+0x1d/0x20
>>> > [   83.030181]  []
>>> > iwl_mvm_time_event_send_add+0x1c6/0x200 [iwlmvm]
>>> > [   83.030184]  [] ? up+0x32/0x50
>>> > [   83.030187]  [] ? wake_up_klogd+0x3b/0x50
>>> > [   83.030189]  [] ? console_unlock+0x4e9/0x590
>>> > [   83.030193]  []
>>> > iwl_mvm_protect_session+0x220/0x280 [iwlmvm]
>>> > [   83.030196]  [] ? iwl_mvm_ref_sync+0x2e/0x140
>>> > [iwlmvm]
>>> > [   83.030199]  [] ? vprintk_default+0x1f/0x30
>>> > [   83.030202]  []
>>> > iwl_mvm_mac_mgd_prepare_tx+0x5a/0xa0 [iwlmvm]
>>> > [   83.030216]  [] ieee80211_mgd_deauth+0x338/0x4d0
>>> > [mac80211]
>>> > [   83.030219]  [] ? enqueue_entity+0x323/0xd70
>>> > [   83.030228]  [] ieee80211_deauth+0x18/0x20
>>> > [mac80211]
>>> > [   83.030237]  [] cfg80211_mlme_deauth+0x9f/0x1a0
>>> > [cfg80211]
>>> > [   83.030242]  [] cfg80211_disconnect+0x9a/0x200
>>> > [cfg80211]
>>> > [   83.030248]  []
>>> > cfg80211_mgd_wext_siwessid+0xa9/0x170 [cfg80211]
>>> > [   83.030255]  [] cfg80211_wext_siwessid+0x22/0x40
>>> > [cfg80211]
>>> > [   83.030258]  [] ioctl_standard_iw_point+0x133/0x350
>>> > [   83.030264]  [] ?
>>> > cfg80211_wext_giwessid+0x50/0x50 [cfg80211]
>>> > [   83.030266]  [] ? wake_up_q+0x32/0x70
>>> > [   83.030268]  [] ? iw_handler_get_private+0x60/0x60
>>> > [   83.030271]  [] ioctl_standard_call+0x87/0xd0
>>> > [   83.030273]  [] ?
>>> > call_commit_handler.part.4+0x30/0x30
>>> > [   83.030275]  [] wireless_process_ioctl+0x1f0/0x230
>>> > [   83.030278]  [] ? dev_get_by_name_rcu+0x5e/0x80
>>> > [   83.030280]  [] wext_handle_ioctl+0x78/0xd0
>>> > [   83.030283]  [] dev_ioctl+0x2a7/0x5a0
>>> > [   83.030285]  [] sock_ioctl+0x126/0x290
>>> > [   83.030287]  [] do_vfs_ioctl+0xa3/0x5d0
>>> > [   83.030290]  [] ? 

RE: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Grumbach, Emmanuel]

> 
> On Mon, Jul 11, 2016 at 06:27:30PM +, Grumbach, Emmanuel wrote:
> > I guess that works, but it seems wrong to me. Usually, registration
> > should happen only upon INIT, and yes, at that time the firmware is
> > not ready to provide the information yet.
> 
> > >
> > > As can be seen in the current code base, iwl_mvm_tzone_get_temp()
> > > will return -EIO 100% of the time when the firmware doesn't support
> > > reading the
> 
> If I understad correctly this error happen 100% of the time, not only during
> init. Hence seems there is an issue here, i.e. cur_ucode is not marked
> correctly as IWL_UCODE_REGULAR or iwl_mvm_get_temp() fail 100% of the
> time (iwl_mvm_is_tt_in_fw() incorrecly return true on Prarit device ? ).

Cur_ucode will not be IWL_UCODE_REGULAR until you load the firmware which
will happen upon ifup.

> 
> BTW, you implement thermal_zone device, but do you also need hwmon
> device? Perhaps using theramal_zone_params no_hwmon option would be
> proper here?

That's an interesting direction. I'd have to check, but TBH, I am not familiar 
with
that code. Luca was very involved during the development but he is not available
right now. I will be back more the less when the merge window will close :)

> 
> Stanislaw
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Stanislaw Gruszka]

On Mon, Jul 11, 2016 at 06:27:30PM +, Grumbach, Emmanuel wrote:
> I guess that works, but it seems wrong to me. Usually, registration
> should happen only upon INIT, and yes, at that time the firmware is not
> ready to provide the information yet.

> > 
> > As can be seen in the current code base, iwl_mvm_tzone_get_temp()
> > will return
> > -EIO 100% of the time when the firmware doesn't support reading the

If I understad correctly this error happen 100% of the time, not only
during init. Hence seems there is an issue here, i.e. cur_ucode is not
marked correctly as IWL_UCODE_REGULAR or iwl_mvm_get_temp() fail
100% of the time (iwl_mvm_is_tt_in_fw() incorrecly return true on
Prarit device ? ).

BTW, you implement thermal_zone device, but do you also need hwmon
device? Perhaps using theramal_zone_params no_hwmon option would be
proper here?

Stanislaw
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] brcmfmac: Decrease 8021x_cnt for dropped packets

[Arend Van Spriel]

On 13-7-2016 20:52, Per Förlin wrote:
> 2016-07-13 13:20 GMT+02:00 Arend Van Spriel :
>> On 12-7-2016 12:23, Per Förlin wrote:
>>> 2016-07-12 11:48 GMT+02:00 Arend Van Spriel :


 On 12-7-2016 10:35, Per Förlin wrote:
> 2016-07-06 11:53 GMT+02:00 Per Förlin :
>> I have now verified this patch on backports 4.4.
>>
>> 2016-04-12 23:55 GMT+02:00  :
>>> From: Per Forlin 
>>>
>>> This patch resolves an issue where pend_8021x_cnt was not decreased
>>> on txfinalize. This caused brcmf_netdev_wait_pend8021x to timeout
>>> because the counter indicated pending packets.
>>>
>>> WARNING: at .../brcmfmac/core.c:1289 brcmf_netdev_wait_pend8021x
>>>   (warn_slowpath_common)
>>>   (warn_slowpath_null)
>>>   (brcmf_netdev_wait_pend8021x [brcmfmac])
>>>   (send_key_to_dongle [brcmfmac])
>>>   (brcmf_cfg80211_del_key [brcmfmac])
>>>   (nl80211_del_key [cfg80211])
>>>   (genl_rcv_msg)
>>>   (netlink_rcv_skb)
>>>   (genl_rcv)
>>>   (netlink_unicast)
>>>   (netlink_sendmsg)
>>>   (sock_sendmsg)
>>>   (___sys_sendmsg)
>>>   (__sys_sendmsg)
>>>   (SyS_sendmsg)
>>>
>>> The solution is to pull back the header offset in case
>>> of an error in txdata(), which may happen in case of
> Clarification:
>
> txdata=brcmf_proto_bcdc_txdata()
> brcmf_proto_bcdc_txdata(): Calls brcmf_proto_bcdc_hdrpush()
>
> The header needs to be pulled back in case of error otherwise
> the error handling and cleanup up will fail to decrease the counter
> of pending packets.

 Yes, this part of the patch is clear to me.

>>> Thanks, I wasn't sure.
>>>
>>> packet overload in brcmf_sdio_bus_txdata.
>>>
>>> Overloading an WLAN interface is not an unlikely scenario.

 So here is where things start to look suspicious and I have mentioned
 this before. My thought here was "How the hell can you end up with a
 2048 packets on the sdio queue", which I mentioned to you before. There
 is a high watermark on the queue upon which we do a netif_stop_queue()
 so network layer does not keep pushing tx packets our way. Looking
 further into this I found that we introduced a bug with commit
 9cd18359d31e ("brcmfmac: Make FWS queueing configurable.") so we ended
 up doing nothing except increasing as statistics debug counter :-(

>>> Is there a fix available for the high watermark issue or is it
>>> something you will look into?
>>>
>>> To produce a load on the wlan interface I run
>>> iperf -c 239.255.1.3 -u -b 10m -f m -i 60 -t 3000
>>> and this is enough in my case to fill up the 2048 queue.
>>>
>>> In case of packet overload the error print "out of bus->txq"
>>> is very verbose. To reduce SPAM degrade it to a debug print.
>>>
>>> Signed-off-by: Per Forlin 
>>> ---
>>> Change log:
>>>  v2 - Add variable to know whether the counter is increased or not
>>>  v3 - txfinalize should decrease the counter. Adjust skb header offset
>>>  v4 - Fix build error
>>>
>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 4 
>>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c | 4 +++-
>>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
>>>  3 files changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c 
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>> index ed9998b..f342f7c 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>> @@ -541,6 +541,9 @@ void brcmf_txfinalize(struct brcmf_if *ifp, struct 
>>> sk_buff *txp, bool success)
>>> struct ethhdr *eh;
>>> u16 type;
>>>
>>> +   if (!ifp)
>>> +   goto free;
>>> +

 This may not be needed.

>>> This is not strictly needed. I can remove it.
>>>
>>> eh = (struct ethhdr *)(txp->data);
>>> type = ntohs(eh->h_proto);
>>>
>>> @@ -553,6 +556,7 @@ void brcmf_txfinalize(struct brcmf_if *ifp, struct 
>>> sk_buff *txp, bool success)
>>> if (!success)
>>> ifp->stats.tx_errors++;
>>>
>>> +free:
>>> brcmu_pkt_buf_free_skb(txp);
>>>  }
>>>
>>> diff --git 
>>> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c 
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>>> index f82c9ab..98cb83f 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>>> @@ -1899,8 +1899,10 @@ int 

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Kalle Valo]

Prarit Bhargava  writes:

> On 07/13/2016 03:24 AM, Luca Coelho wrote:
>
>> I totally agree with Emmanuel and Kalle. We should not change this.
>> It is a design decision to return an error when the interface is
>> down, this is very common with other subsystems as well.
>
> Please show me another subsystem or driver that does this.  I've looked around
> the kernel but cannot find one that updates the firmware and implements new
> features on the fly like this.  I have come across several drivers that allow
> for an update, but they do not implement new features based on the
> firmware.
>
> Additionally, what happens when someone back revs firmware versions (which
> happens far more than you and I would expect)?  Does that mean I now go from a
> functional system to a non-functional system wrt to userspace?

I'm not following, what do you mean exactly? Why are you talking
updating the firmware?

So when we talk about "loading firmware" we mean that the driver pushes
the firmware image to to the chipset. And then the interface is down the
chipset is powered down and the RAM on it will be erased. That's the
general idea anyway, I haven't checked how iwlwifi exactly works in this
case but Luca or Emmanuel can correct me.

>> The userspace should be able to handle errors and report something
>> like "unavailable" when this kind of error is returned.
>
> I myself have made the same arguments wrt to cpufreq code & bad userspace
> choices.  I just went through this a few months back with what went from a
> simple patch and turned out to be a hideous patch in cpufreq.  You cannot 
> break
> userspace like this.

Don't get me wrong, I'm a strong supporter of stable user space
interfaces and I always try to adher to that. But there's a limit for
everything. If I'm understanding correctly, what you mean is that the
kernel should never return an error because an application doesn't
handle errors gracefully. Sorry, but that doesn't make sense to me.

> See commit 51443fbf3d2c ("cpufreq: intel_pstate: Fix intel_pstate powersave
> min_perf_pct value").  What should have been a trivial change resulted in a
> massive change because of broken userspace.

In that cpufreq case I understand, it was about a combination of
configuration values which broke the user space. But here we are just
dealing with a simple error value, nothing fancy.

>> I'm not sure EIO is the best we can have, but for me that's exactly
>> what it is.  The thermal zone *is* there, but cannot be accessed
>> because the firmware is not available.  I'm okay to change it to EBUSY,
>> if that would help userspace, but I think that's a bit misleading.  The
>> device is not busy, on the contrary, it's not even running at all.
>> 
>
> I understand that, but by returning -EIO we end up with an error.
>
>> Furthermore, I don't think this is "breaking userspace" in the sense of
>> being a regression.  
>
> I run (let's say 4.5 kernel).  sensors works.  I update to 4.7.  sensors 
> doesn't
> work.  How is that not a regression?  That's _exactly_ what it should be
> reported as.

Sure, it's a regression in a way. But that's how the user space app you
are using is implemented, the same problem would happen with any driver
returning errors.

>> The userspace API has always been implemented with
>> the possibility of returning errors.  It's not a good design if a
>> single device returning an error causes all the other devices to also
>> fail.
>> 
>
> If that were the case we would never have to worry about "breaking userspace"?
> For any kernel change I could just say that the userspace design was bad and 
> be
> done with it.  Why fix anything then?

Because we are talking about a simple error value.

> I don't see any harm in waiting to register the sysfs files for hwmon until 
> the
> firmware has been validated.

I'm against of that because it's bad software design. It's standard
practise in Linux that drivers register their capabilities during driver
probe time so that user space can query them whenever needed. I assume a
properly behaving user space app would want to know about all the
available sensors once the driver is initialised and your suggestion
would break that.

> IIUC, the up/down'ing of the device doesn't happen that often (during
> initial boot, and suspend/resume, switching wifi connections,
> shutdown?).

Basically it can happen anytime, this is fully controlled by user space.
There's no point of trying to make any assumptions as they won't hold
anyway.

> This would make the iwlwifi community happy (IMO) and
> sensors would still work. At the same time I could write a patch for
> lm-sensors to fix this issue if it comes up in future versions.
> [Aside: I'm going to have the reproducing system available today and
> will test this out. It looks like just moving some code around.]

Another option, but still a bad one I don't like, is that you change the
kernel interface to ignore all errors from drivers (like iwlwifi). This
way 

Re: [PATCH 0/4 v1] Refactoring ieee80211_iface_work

[Arend Van Spriel]

On 13-7-2016 22:19, Alex Briskin wrote:
> Hi All,
> This is my first patch(s).

Hi Alex,

As these patches are touching mac80211 it would be good to use
'mac80211: ' prefix.

> I've decided to refactor ieee80211_iface_work function and break it down
> to smaller better defined function.
> 
> I think these changes make the code much more readable and do not impose
> no overhead.
> 
> I've tested these patches with sparse and checkpatch.pl 
> 
> Function names might not be descriptive enough.
> Hope you find this useful.
> 
> Alex Briskin (4):
>   0) [28e464b19aaaba90c8946fb979b58709d55dffcf] 
>   Added new function ieee80211_is_skb_handled_by_pkt_type and moved
>   some code from ieee80211_iface_work to reduce complexity and 
>   improve readability
> 
>   1) [486e3d5abb4dc6361cdd923254a2b68d43dcdaba]
>   Refactored code in ieee80211_is_skb_handled_by_pkt_type.
>   "if () {} else if ()" replaced by switch case. 

I would collapse these two patches in one patch.

>   2) [9ef2eab8e831420bc6748a4466ffa6b7a99bf447]
>   Added new function ieee80211_is_handled_by_frame_control and moved
>   some code from ieee80211_iface_work to it.
> 
>   3) [1de8cdf9a0c05c6a21d9e43e5b55862f6efcf450] 
>   Added new function ieee80211_handle_by_vif_type with code from
>   ieee80211_iface_work.
> 
>   At this point ieee80211_iface_work seems to me much more readable
>   and better understood. 

You are allowed to have an opinion :-) The function naming of the three
functions could be more consistent as you seem to drop a bit in every
patch, ie. is_skb_handled_by -> is_handled_by -> handle_by.

Regards,
Arend

>  net/mac80211/iface.c | 264 
> +--
>  1 file changed, 150 insertions(+), 114 deletions(-)
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Kalle Valo]

Prarit Bhargava  writes:

> On 07/13/2016 02:50 AM, Kalle Valo wrote:
>> Prarit Bhargava  writes:
>> 
 We implement thermal zone because we do support it, but the problem is
 that we need the firmware to be loaded for that. So you can argue that
 we should register *later* when the firmware is loaded. But this is
 really not helping all that much because the firmware can also be
 stopped at any time. So you'd want us to register / unregister the
 thermal zone anytime the firmware is loaded / unloaded?
>>>
>>> You might have to do that.  I think that if the firmware enables a feature 
>>> then
>>> the act of loading the firmware should run the code that enables the 
>>> feature.
>>> IMO of course.
>> 
>> But I suspect that the iwlwifi firmware is loaded during interface up
>> (and unloaded during interface down) and in that case
>> register/unregister would be happening all the time. 
>
> You make it sound like the interface is coming and going a 1000 times a 
> second.
>  Maybe this happens once during runtime & during suspend/resume
>  cycles?

Of course it doesn't happen 1000 times a second but it depends on user
space behaviour. In some cases, when the wlan interface is always up,
the firmware is loaded only once. But in some cases the user space might
change the interface state more frequently.

More so registering services like thermal zone should happen during
driver probe time, not during interface up event.

> What about the cases when the firmware isn't present (and that's what
> lead me to this bug)?

In that case the kernel could return a predefined error value like
-EGAIN or -ENOTDOWN so that the user space knows that a value is not
available at this time (but might be available later).

-- 
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html