Re: [PATCH 12/34] brcmfmac: Replace old IO functions with simpler ones.

[Ian Molton]

On 07/08/17 18:55, Ian Molton wrote:
> On 07/08/17 12:26, Arend van Spriel wrote:
>> On 7/26/2017 10:25 PM, Ian Molton wrote:
>>> Primarily this patch removes:
>>>
>>> brcmf_sdiod_f0_writeb()
>>> brcmf_sdiod_reg_write()
>>> brcmf_sdiod_reg_read()
>>
>> Having [patch 30/34] "brcmfmac: Correctly handle accesses to SDIO func0"
>> before this patch could make this look cleaner.
> 
> This is an artifact of how I came to the realisation the code was using
> the obsoleted functions - it could be reordered, but it'd probably get
> messy...

I gave it a try, and it got messy - I'd prefer to leave these patches in
their current ordering if thats OK.

-Ian

Re: [PATCH 11/34] brcmfmac: Split brcmf_sdiod_buffrw function up.

[Ian Molton]

On 07/08/17 12:26, Arend van Spriel wrote:

>> +err = brcmf_sdiod_buff_write(sdiodev, SDIO_FUNC_2, addr, mypkt);
>>
>>   brcmu_pkt_buf_free_skb(mypkt);
>> +
> 
> You are keen on sprinkling whitespace here and there. Could you please
> use separate patches for that as much as possible. Not sure why you
> added this one...
> 
>>   return err;
>>
> 
> ...and kept this one.
> 
>>   }
> 

Good catch. There's a couple of others - I'll break them out. That
latter one is clearly an error on my part.

-Ian

Re: [PATCH 08/34] brcmfmac: Fix uninitialised variable

[Ian Molton]

On 07/08/17 12:26, Arend van Spriel wrote:
> On 7/26/2017 10:25 PM, Ian Molton wrote:
>> Unlikely to be a problem, but brcmf_sdiod_regrb() is
>> not symmetric with brcmf_sdiod_regrl() in this regard.
> 
> You are pretty keen on symmetry, but you could also remove the
> initialization in brcmf_sdiod_regrl(). As long as no -Wunitialized pops
> up that would have my preference.

Whilst the compiler does not complain, there are paths through
brcmf_sdiod_reg_read() that do not set *data, so I think it best to
initialise the value, but its not really very important. Code that isn't
checking the return value is asking for it anyway :)

In the later patches we could remove the initialisation, as IIRC the
Linux MMC IO functions do it for us and we don't avoid calling them in
that version of the code.

-Ian

Re: [PATCH 06/34] brcmfmac: Remove bandaid for SleepCSR

[Ian Molton]

On 07/08/17 12:25, Arend van Spriel wrote:
> On 26-07-17 22:25, Ian Molton wrote:
>> Register access code is not the place for band-aid fixes like this.
>> If this is a genuine problem, it should be fixed further up in the driver
>> stack.
> 
> So lets make it a SDIO debug message for all register accesses getting
> rid of the error message.

Not quite sure I follow here - but as the code is completely gone in
later patches in the series, does it matter?

Perhaps address this if it in future, if it crops up as a problem, since
it wont be fatal even if it does...

-Ian

Re: [PATCH 34/34] brcmfmac: Reduce the noise from repeatedly dereferencing common pointers

[Ian Molton]

On 08/08/17 13:29, Arend van Spriel wrote:
>>  int brcmf_sdio_sleep(struct brcmf_sdio *bus, bool sleep)
>>  {
>> +struct brcmf_sdio_dev *sdiodev = bus->sdiodev;
>> +struct sdio_func *func1 = sdiodev->func1;
>
> Actually only wanted to explicitly mention this one. Probably the
> compiler is smart enough to keep sdiodev from the stack, but I would say
> it is a variable you do not really need and also in terms of readability
> it seems clear enough to do it in one assignment.

Yeah, it could be - but as you say, the compiler *will* be smart enough,
and doing it in this manner makes it consistent with all the other code :-)

I'm looking at your other comments atm. :)

-Ian

>>  int ret;
>>  
>> -sdio_claim_host(bus->sdiodev->func1);
>> +sdio_claim_host(func1);
>>  ret = brcmf_sdio_bus_sleep(bus, sleep, false);
>> -sdio_release_host(bus->sdiodev->func1);
>> +sdio_release_host(func1);
>>  

Re: [PATCH 31/34] brcmfmac: Remove func0 from function array

[Ian Molton]

On 08/08/17 12:19, Arend van Spriel wrote:
>> -sdio_f0_readb((sdiodev)->func[0], (addr), (r))
>> +sdio_f0_readb((sdiodev)->func[1], (addr), (r))
> 
> There is no reason to keep these any longer as these do not provide any
> functionality over the core sdio function unless you consider the
> sdiodev dereference.

I'm happy to submit an incremental patch to these that gets us right
down to the linux mmc core functions. Just seemed like too big a change
to do in one hit.

-Ian

Re: [PATCH 29/34] brcmfmac: stabilise the value of ->sbwad in use for some xfer routines.

[Ian Molton]

On 07/08/17 13:32, Arend van Spriel wrote:
> 
> We actually just need the chipcommon base address so why not have that
> here, ie.:
> +u32 cc_base;

I see no advantage to that - the u32 is the same size as (or not much
bigger than the pointer to the struct brcmf_core, and my approach makes
it clear where the value came from rather than making another copy of it.

> Another option is to simple use SI_ENUM_BASE as the chipcommon base
> address will always be 0x1800 for the SDIO chips.

I don't like this approach. Why bother probing the core if we then dont
use the values returned? May as well hard code everything... Also not
futureproof.

-Ian

Re: [PATCH 14/34] brcmfmac: Remove brcmf_sdiod_addrprep()

[Ian Molton]

On 07/08/17 12:27, Arend van Spriel wrote:
> On 7/26/2017 10:25 PM, Ian Molton wrote:
>> This function has become trivial enough that it may as well be pushed
>> into
>> its callers, which has the side-benefit of clarifying what's going on.
>>
>> Remove it, and rename brcmf_sdiod_set_sbaddr_window() to
>> brcmf_sdiod_set_backplane_window() as it's easier to understand.
> 
> Reviewed-by: Arend van Spriel 
>> Signed-off-by: Ian Molton 
> 
> comments below...
> 

>> -if (sdiodev->state == BRCMF_SDIOD_NOMEDIUM)
>> -return -ENOMEDIUM;
> 
> So now you are dropping the state check here, which seems significant
> enough to mention in the commit log. We need to discuss that. The idea
> of it was to refrain from using IO function of the MMC stack when we no
> there is no longer a device, ie. when stack has previously returned a
> -ENOMEDIUM.

Yeah, that code is broken (see earlier email) AFAICT anyway, and we
should probably handle losing our card a lot more gracefully in general.

>> +if (bar0 == sdiodev->sbwad)
>> +return 0;
>>
>> -addr = (address & SBSDIO_SBWINDOW_MASK) >> 8;
>> +v = bar0 >> 8;
> 
> why introducing a new variable on the stack. You actually don't need any
> and just mask and shift the addr variable passed to the function.

Linux code *generally* doesn't do this. Its stylistic anyway, since the
compiler certainly won't be dumb enough to allocate another register (or
stack space) in this instance.

>>   if (!retval)
>>   data = sdio_readl(sdiodev->func[1], addr, );
> 
> The if-statement is now redundant here...

Good catch! :)

-Ian

Re: [PATCH 09/34] brcmfmac: Remove noisy debugging.

[Ian Molton]

On 07/08/17 12:26, Arend van Spriel wrote:
> 
> Needing this debugging does not necessarily means you are doing
> something wrong. You may be dealing with hardware that is doing
> something wrong and when that happens this debug can be useful. I
> frankly hardly ever enable SDIO debug level unless I am in that
> scenario. Maybe adding a debug level for low-level access would be
> useful to reduce the noise for SDIO debug level.

Perhaps, but its only actually called from a half dozen or so places in
the code + the buscore_*32 entrypoints.

All it actually does now is ensure the address window is right and call
the Linux SDIO core readl method. if we can't trust that, we're kinda
screwed anyway. If we later bring the SDIO code in line with the PCIe
code, it wont even do the address window checking (it'll just assume its
already correct).

We can always enable the SDIO core lowlevel debug if we really want to
see register level acceses.

-Ian

Re: [PATCH 07/34] brcmfmac: Remove brcmf_sdiod_request_data()

[Ian Molton]

On 07/08/17 18:51, Ian Molton wrote:
> On 07/08/17 12:25, Arend van Spriel wrote:
>>> Handling of -ENOMEDIUM is altered, but as that's pretty much broken
>>> anyway
>>> we can ignore that.
>>
>> Please explain why you think it is broken.
> 
> Not got the code to hand right now, but from memory, theres a trapdoor
> case where the state can wind up set to something that prevents it ever
> being changed again. I'll dig it up when I get back from holiday (this
> next few days).

Hi,

Here is the function I had in mind:


void brcmf_sdiod_change_state(struct brcmf_sdio_dev *sdiodev,
  enum brcmf_sdiod_state state)
{
if (sdiodev->state == BRCMF_SDIOD_NOMEDIUM ||
state == sdiodev->state)
return;

brcmf_dbg(TRACE, "%d -> %d\n", sdiodev->state, state);
switch (sdiodev->state) {
case BRCMF_SDIOD_DATA:
/* any other state means bus interface is down */
brcmf_bus_change_state(sdiodev->bus_if, BRCMF_BUS_DOWN);
break;
case BRCMF_SDIOD_DOWN:
/* transition from DOWN to DATA means bus interface is up */
if (state == BRCMF_SDIOD_DATA)
brcmf_bus_change_state(sdiodev->bus_if,
BRCMF_BUS_UP);
break;
default:
break;
}
sdiodev->state = state;
}


If it's *ever*  called with state = BRCMF_SDIOD_NOMEDIUM it will
eventually (last line) set sdiodev->state to the same value.

If its ever called again, the first if() statement will make it return
before ever changing sdiodev->state again, no matter what value is
passed for state.

This has to be a bug, surely?

-Ian

Re: [PATCH 01/34] brcmfmac: Fix parameter order in brcmf_sdiod_f0_writeb()

[Ian Molton]

On 07/08/17 12:25, Arend van Spriel wrote:
>> All the other IO functions are the other way round in this
>> driver. Make this one match.
> 
> Core SDIO functions are indeed the other way around, but the IO
> functions in this file use (addr, data) pattern. So should we aim to get
> it all consistent with core SDIO or just consistent on its own.

This is preparatory for a later patch that removes this abstraction
altogether, so yes, it should match the SDIO order.

-Ian

[PATCH] ath10k: sdio: remove unused struct member

[Erik Stromdahl]

irq_wq in struct ath10k_sdio is a remnant from an earlier
version of the sdio patchset.

It's use was removed as a result of Kalle's review, but somehow
the struct member survived.

It is not used and can therefore safely be removed.

Signed-off-by: Erik Stromdahl 
---
 drivers/net/wireless/ath/ath10k/sdio.c | 4 
 drivers/net/wireless/ath/ath10k/sdio.h | 2 --
 2 files changed, 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/sdio.c 
b/drivers/net/wireless/ath/ath10k/sdio.c
index 48268f0..03a69e5 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -1344,8 +1344,6 @@ static void ath10k_sdio_irq_handler(struct sdio_func 
*func)
 
sdio_claim_host(ar_sdio->func);
 
-   wake_up(_sdio->irq_wq);
-
if (ret && ret != -ECANCELED)
ath10k_warn(ar, "failed to process pending SDIO interrupts: 
%d\n",
ret);
@@ -2000,8 +1998,6 @@ static int ath10k_sdio_probe(struct sdio_func *func,
goto err_free_bmi_buf;
}
 
-   init_waitqueue_head(_sdio->irq_wq);
-
for (i = 0; i < ATH10K_SDIO_BUS_REQUEST_MAX_NUM; i++)
ath10k_sdio_free_bus_req(ar, _sdio->bus_req[i]);
 
diff --git a/drivers/net/wireless/ath/ath10k/sdio.h 
b/drivers/net/wireless/ath/ath10k/sdio.h
index 3f61c67..4ff7b54 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.h
+++ b/drivers/net/wireless/ath/ath10k/sdio.h
@@ -210,8 +210,6 @@ struct ath10k_sdio {
/* temporary buffer for BMI requests */
u8 *bmi_buf;
 
-   wait_queue_head_t irq_wq;
-
bool is_disabled;
 
struct workqueue_struct *workqueue;
-- 
2.7.4

pull-request: iwlwifi-next 2017-08-18

[Luca Coelho]

Hi Kalle,

Here's my third pull-request intended for v4.14.  We continued doing
generic development work, with improvements, bug fixes and cleanups all
around.  More details in the tag description.

My patch "iwlwifi: update channel flags parser" is going to cause a
conflict with net because they both change the same string in debug
message.  Emmanuel's patch "iwlwifi: split the regulatory rules when the
bandwidth flags require it" added "reg_flags" to the string and changed
the Ad-hoc string.  My patch added a few %s's and also changed the Ad-
hoc string.  The result of the conflict resolution should look like
this:

"Ch. %d [%sGHz] %s%s%s%s%s%s%s%s%s%s%s%s(0x%02x) reg_flags 0x%x: %s\n"


I have sent this out before, and kbuildbot reported success.

Please let me know if there are any issues.

Cheers,
Luca.


The following changes since commit 38ef62353acbaa0eea062a9f047b33aebd7d52ce:

  rsi: security enhancements for AP mode (2017-08-17 10:07:00 +0300)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-next.git 
tags/iwlwifi-next-for-kalle-2017-08-18

for you to fetch changes up to 8fe34b060a4b11be35ffd9383acdd0bc7bee72e2:

  iwlwifi: use big-endian for the hw section of the nvm (2017-08-18 17:36:42 
+0300)


Third set of iwlwifi patches for 4.14

* Work for the upcoming A000 device family continues;
* Improvements in debugging;
* A couple of improvements in block-ack sessions;
* Some fixes for channel switch;
* A workaround for a HW data bug;
* Some FW API updates;
* General fixes and cleanups here and there.


Avraham Stern (1):
  iwlwifi: mvm: remove session protection to allow channel switch

Emmanuel Grumbach (7):
  iwlwifi: mvm: remove the corunning support
  iwlwifi: mvm: support new Coex firmware API
  iwlwifi: pcie: support short Tx queues for A000 device family
  iwlwifi: mvm: add command name for FRAME_RELEASE
  iwlwifi: mvm: include more debug data when we get an unexpected baid
  iwlwifi: mvm: update the firmware API in TX
  iwlwifi: mvm: don't send BAR on flushed frames

Gregory Greenman (1):
  iwlwifi: mvm: change open and close criteria of a BA session

Ilan Peer (1):
  iwlwifi: mvm: Fix channel switch in case of count <= 1

João Paulo Rechi Vita (1):
  iwlwifi: Demote messages about fw flags size to info

Luca Coelho (12):
  iwlwifi: mvm: consider RFKILL during INIT as success
  iwlwifi: call iwl_remove_notification from iwl_wait_notification
  iwlwifi: mvm: group all dummy SAR function declarations together
  iwlwifi: mvm: use mvmsta consistently in rs.c
  iwlwifi: move BT_MBOX_PRINT macro to common header
  iwlwifi: pci: add new PCI ID for 7265D
  iwlwifi: update channel flags parser
  iwlwifi: add workaround to disable wide channels in 5GHz
  iwlwifi: fw: fix lar_enabled endian problem in iwl_fw_get_nvm
  iwlwifi: mvm: remove useless argument in iwl_nvm_init()
  iwlwifi: mvm: remove useless check for mvm->cfg in iwl_parse_nvm_section()
  iwlwifi: use big-endian for the hw section of the nvm

Tzipi Peres (1):
  iwlwifi: distinguish different RF modules in A000 devices

 drivers/net/wireless/intel/iwlwifi/cfg/a000.c|  34 ++-
 drivers/net/wireless/intel/iwlwifi/fw/api/coex.h |  50 +++-
 drivers/net/wireless/intel/iwlwifi/fw/api/commands.h |  12 
 drivers/net/wireless/intel/iwlwifi/fw/api/config.h   |   8 ---
 drivers/net/wireless/intel/iwlwifi/fw/api/tx.h   |   9 +--
 drivers/net/wireless/intel/iwlwifi/fw/file.h |   3 +
 drivers/net/wireless/intel/iwlwifi/fw/notif-wait.c   |  25 
 drivers/net/wireless/intel/iwlwifi/fw/nvm.c  |   2 +-
 drivers/net/wireless/intel/iwlwifi/iwl-config.h  |   4 +-
 drivers/net/wireless/intel/iwlwifi/iwl-csr.h |   7 ++-
 drivers/net/wireless/intel/iwlwifi/iwl-drv.c |  12 ++--
 drivers/net/wireless/intel/iwlwifi/iwl-nvm-parse.c   | 118 
+++---
 drivers/net/wireless/intel/iwlwifi/iwl-nvm-parse.h   |   5 +-
 drivers/net/wireless/intel/iwlwifi/mvm/coex.c| 301 
+
 drivers/net/wireless/intel/iwlwifi/mvm/constants.h   |   2 +-
 drivers/net/wireless/intel/iwlwifi/mvm/debugfs.c |  30 --
 drivers/net/wireless/intel/iwlwifi/mvm/fw.c  |  54 ++
 drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c|  16 --
 drivers/net/wireless/intel/iwlwifi/mvm/mvm.h |  31 +++---
 drivers/net/wireless/intel/iwlwifi/mvm/nvm.c | 106 
+-
 drivers/net/wireless/intel/iwlwifi/mvm/ops.c |   6 +-
 drivers/net/wireless/intel/iwlwifi/mvm/rs.c  | 143 
+-