RFC: brcmfmac bus level addressing issues.

RFC: brcmfmac bus level addressing issues.

[Ian Molton]

Hi folks,

Its come to my attention that there is a substantial disparity between
the PCIe and SDIO variants of the driver when it comes to handlign
writes via the backplane.

The SDIO bus code checks, upon every (32 bit) access, wether the
backplane window is in the right place, and only updates it if it has
actually changed.

The PCIe code sets the window *regardless* of wether its changed, on
*every single* write.

The SDIO code has no explicit selection of the window address based on
the core selected.

The PCIe code uses brcmf_pcie_select_core(), which, ultimately, appears
to be totally redundant, due to the above mentioned 32 bit access code
setting the window register regardless of its current value.

------------------------------------

Can we standardise how this is supposed to work? Its ugly, and its going
to cause bugs, ultimately. I suspect its probably the cause of the code
I mentioned in my recent patch titled "brcmfmac: HACK - stabilise the
value of ->sbwad in use for some xfer routines."

We really *dont* want to call brcmf_pcie_select_core() all over the
place. Its inefficient, traversing a list as it does, when all it does
is return a pointer that never actually changes, to the core structures
that contain addressing info.

I'd propose we do what I've done in my SDIO patch set - we call
brcmf_chip_get_core() *once* after the chip has been probed, and store
the pointer returned.

The window register setting can be hidden in the read32/write32 buscore
ops, and will never be incorrect from that point, and the code can
simply use a flat address space model. A single if() has got to be less
costly than writing the register on overy single read32/write32...

Anyhow, whatever we decide to do, can we do the same thing in both bus
drivers?

-Ian

RE: brcmfmac bus level addressing issues.

[Hante Meuleman]

Hi Ian,

I've really no idea what you mean. Brcmf_pcie_select_core is redundant?
Care to try to boot a device without this function? Called all over the
place? Hell no, it is default pointing to PCIE2 and functions which need
to map the window to another core will do so, temporarily, but move it
back to PCIE2, at least that is the idea, may be you found a bug? We are
for sure not going to hide the selecting of the window in the read/write
routines, that would result in a giant amount of overhead. Currently PCIE
devices reach 1.5Gpbs, we need to go faster than that in the near future.
We don't want just change that to make it bit nicer..... Why do you need
to see the same in the SDIO and PCIE drivers? SDIO and PCIE differ in many
aspects. Sure some things can be improved in or the other, but they sure
don't need to look alike.

It may be ugly, but thusfar it has not caused bugs (and there won't be
large changes in the near future where it will cause bugs). The concept in
pcie bus part is simple. The main core to select is PCIE2 (once you have
booted and established initial communication with firmware) and every
routine which needs to access another core will change the window
temporarily and set it back once done. Please don't mess with this, it
works, it is clear and it is fast.

Regards,
Hante

-----Original Message-----
From: Ian Molton [mailto:ian@mnementh.co.uk]
Sent: Tuesday, July 18, 2017 11:45 AM
To: linux-wireless@vger.kernel.org
Cc: Arend Van Spriel; Franky Lin; Hante Meuleman
Subject: RFC: brcmfmac bus level addressing issues.

Hi folks,

Its come to my attention that there is a substantial disparity between the
PCIe and SDIO variants of the driver when it comes to handlign writes via
the backplane.

The SDIO bus code checks, upon every (32 bit) access, wether the backplane
window is in the right place, and only updates it if it has actually
changed.

The PCIe code sets the window *regardless* of wether its changed, on
*every single* write.

The SDIO code has no explicit selection of the window address based on the
core selected.

The PCIe code uses brcmf_pcie_select_core(), which, ultimately, appears to
be totally redundant, due to the above mentioned 32 bit access code
setting the window register regardless of its current value.

------------------------------------

Can we standardise how this is supposed to work? Its ugly, and its going
to cause bugs, ultimately. I suspect its probably the cause of the code I
mentioned in my recent patch titled "brcmfmac: HACK - stabilise the value
of ->sbwad in use for some xfer routines."

We really *dont* want to call brcmf_pcie_select_core() all over the place.
Its inefficient, traversing a list as it does, when all it does is return
a pointer that never actually changes, to the core structures that contain
addressing info.

I'd propose we do what I've done in my SDIO patch set - we call
brcmf_chip_get_core() *once* after the chip has been probed, and store the
pointer returned.

The window register setting can be hidden in the read32/write32 buscore
ops, and will never be incorrect from that point, and the code can simply
use a flat address space model. A single if() has got to be less costly
than writing the register on overy single read32/write32...

Anyhow, whatever we decide to do, can we do the same thing in both bus
drivers?

-Ian

Re: brcmfmac bus level addressing issues.

[Ian Molton]

On 18/07/17 11:35, Hante Meuleman wrote:
> Hi Ian,
> 
> I've really no idea what you mean.

You should look at the code...

> brcmf_pcie_select_core is redundant?

Essentially yes - there may be a couple of corner cases where the IO
accesses are not performed via brcmf_pcie_buscore_{read,write}32() - but
other than that, brcmf_pcie_buscore_prep_addr() sets the IO window
unconditionally on every access.

> Care to try to boot a device without this function?

I strongly suspect it would work. Perhaps try it? Give me a device and
I'll try it.

> Called all over the  place? Hell no, it is default pointing to PCIE2
> and functions which need to map the window to another core will do
> so, temporarily, but move it back to PCIE2, at least that is the
> idea, may be you found a bug?

brcmf_pcie_select_core() looks up the core structure from the core id.

it then sets BRCMF_PCIE_BAR0_WINDOW according to the core base address.

it actually goes to the length of reading it back and trying again if
its not set, even, which is at least a little bit horrifying.

------------

brcmf_pcie_buscore_{read,write}32() both call brcmf_pcie_buscore_prep_addr()

brcmf_pcie_buscore_prep_addr() *unconditionally* programs
BRCMF_PCIE_BAR0_WINDOW on *every single* IO access.

If you want inefficient - its right there.


The SDIO version of the code is actually considerably more efficient on
this point - it at least only programs the window register only when it
changes, not on every single IO access.

> We are
> for sure not going to hide the selecting of the window in the read/write
> routines, that would result in a giant amount of overhead.

Actually it would result in *considerably* less overhead than the
current code, that blindly sets the window on every access.

> Currently PCIE
> devices reach 1.5Gpbs, we need to go faster than that in the near future.

I dont need a lesson on writing efficient code, thanks.

> We don't want just change that to make it bit nicer..... Why do you need
> to see the same in the SDIO and PCIE drivers? SDIO and PCIE differ in many
> aspects. Sure some things can be improved in or the other, but they sure
> don't need to look alike.

I dont "need" to see the same in both drivers. Not where it isnt
appropriate.

but every part of the drivers that can share code without noticeably
impacting performance *should* do so. You should be justifying to me why
the code has to be different, not the other way around. Are you
sreiously arguing that sharing common code is a bad idea?

> It may be ugly, but thusfar it has not caused bugs

Oh, I bet you it has... try reading the SDIO version (note the reliance
on the dangling ->sbwad pointer) and tell me again that this hasnt
caused bugs.

Right now, the bulk of the driver code is sat on top of at least two bus
drivers with differing IO models, and is working via good luck alone.

> The concept in pcie bus part is simple.

And differs completely from the SDIO part.

> The main core to select is PCIE2 (once you have
> booted and established initial communication with firmware) and every
> routine which needs to access another core will change the window
> temporarily and set it back once done. Please don't mess with this, it
> works, it is clear and it is fast.

If is anything but fast. changing the window involves traversiong the
list of cores. Every. Single. Time. It doesnt *have* to - but thats what
brcmf_chip_get_core() does, and brcmf_pcie_select_core() calls it.

Re: brcmfmac bus level addressing issues.

[Ian Molton]

Reposting as CC's got reset (not by me).

On 18/07/17 15:36, Ian Molton wrote:
> On 18/07/17 13:50, Hante Meuleman wrote:
>> Hi Ian,
>>
>> - Ok, I may not have been reading all the code that well, but I'm pretty
>> sure, you cannot do without brcmf_pcie_select_core, at least that is how I
>> "intended" it when I wrote it. Try buying a macbook with a 43602, remove
>> the function and all calls (8 in total) and try booting it. I might be
>> mistaken but I'm 99% sure that it won't, give it a try I would say.
> 
> I'm not buying a macbook just to make a point.
> 
>> - brcmf_pcie_buscore_{read,write}32() are not the main functions, they are
>> the corner cases. They exist to satisfy chip.c which is a common module to
>> support different bus types (SDIO and PCIE). Chip.c just identifies the
>> cores on the chip and does some magic reset stuff. The main functions to
>> access memory (during normal operation) are brcmf_pcie_read/write_{all
>> variants} functions. They matter when it comes to performance, and I was
>> under the impression you were familiar with the code and wanted to add
>> window selection functionality to those functions, and that would have
>> been bad.
> 
> Did I mention them even once?
> 
>> Chip.c is just init, we don't care about performance there.
> 
> Then you invalidated your whole argument for not cleaning it up.
> 
>> - I'm unfamiliar with sbwad, perhaps it has caused issues in the past. You
>> wrote : " Can we standardise how this is supposed to work? Its ugly, and
>> its going to cause bugs" and then you talk about a lot of changes in the
>> PCIE code.
> 
> ->swbad is used in the SDIO code to store the last value the window
> register was programmed with, so as to avoid programming it twice.
> 
> Im talking about making some attempt to unify the PCIE and SDIO bus
> code, since they BOTH have the same constraint - an IO window that needs
> to be moved about depending on the addresses being accessed.
> 
> Whhat is really needed is for brcmf_pcie_select_core and a variant of it
> for SDIO (and presumably USB, but I havent looked) to share common code.
> 
> When you pull the code apart, its really not THAT dissimilar, but it
> *looks* it and thats not a good thing.
> 
> The major differences are:
> 
> The PCIe code sets the window address in ALL the _{read,write}32 calls,
> wheras the SDIO code does not.
> The SDIO code does not have an analogue to brcmf_pcie_select_core() -
> but it should, since it has similar restrictions. It works by good luck.
> 
>> I don't see a need to cleanup just because we might change
>> something in the future which could result in a bug because it was
>> programmed perfectly pretty.
> 
> -EPARSE
> 
>> As far as I can see (and I can see a long way)
> 
> Oh please.
> 
>> there are not going to be a lot of changes in the pcie code in the
>> near future.
> 
> Maybe so - its a lot better than the SDIO code.
> 
>>  - I don't need to justify to you why the code is different. I wrote the
>> code and I'll be the one adding new functionality.
> 
> You realise that this is Linux, right? its public code, and worked on
> publicly, by many people. You might be a maintainer of a driver, but
> that does not make you "king of Linux".
> 
>> You need to justify
>> prettying up the code while I'm familiar with the code after working on it
>> for the last 3 years.
> 
> "Its illegible shite (the SDIO code certainly is) and it should never
> have got into mainline" sounds like pretty damn good justification to me.
> 
> If you've been working on this crap for 3 YEARS then *hang your damn
> head in shame* - I've been working on it for about 5 DAYS now, and I've
> found 2 complete breakages (one admittedly patched now), a possible
> firmware bug, and undefined behaviour in the SDIO bus code.
> 
> But its NOT a pissing contest. I've sent patches. It doesn't matter how
> long you've been working on the code - if the patches make it better,
> then take the damn patches. Or at least explain why you aren't going to.
> 
>> No offence,
> 
> Too late for that.
> 
>> but "Im going to be one to get familiarized with your prettifying
>> changes while you just hop on to the next driver.....
> 
> You cheeky bastard. "hop onto the next driver". As if that means anything.
> 
> Just because you're familiar with the stinking pile of crap doesn't mean
> it should not be touched for fear of breaking it.
> 
> You're the one ploughing ahead adding new features on top of a garbage
> driver core.
> 
> Stop. Listen. Fix it.
> 
> Then add the new features.
> 
> You could at the very least give some feedback on the 29 patches I sent
> cleaning it up.
> 
> For reference, I've noticed a couple of minor nits in my patchset ( one
> spelling error in a commit message, one dangling variable, and some
> minor shenannigans around f0 writes which needs a trivial re-work for
> compliance with the linux mmc framework (I should use a QUIRK for the
> func0 accesses - although the original driver code was worse and
> actively worked around the quirk instead). I will re-spin and post them.
> 
> In the mean time, I'd appreciate some review on the rest. Since you're
> clearly full time on this driver, bloody well get on with it.
> 
> -Ian
> 
>>
>> Regards, Hante
>>
>> -----Original Message-----
>> From: Ian Molton [mailto:ian@mnementh.co.uk]
>> Sent: Tuesday, July 18, 2017 1:28 PM
>> To: Hante Meuleman; linux-wireless@vger.kernel.org
>> Cc: Arend Van Spriel; Franky Lin
>> Subject: Re: brcmfmac bus level addressing issues.
>>
>> On 18/07/17 11:35, Hante Meuleman wrote:
>>> Hi Ian,
>>>
>>> I've really no idea what you mean.
>>
>> You should look at the code...
>>
>>> brcmf_pcie_select_core is redundant?
>>
>> Essentially yes - there may be a couple of corner cases where the IO
>> accesses are not performed via brcmf_pcie_buscore_{read,write}32() - but
>> other than that, brcmf_pcie_buscore_prep_addr() sets the IO window
>> unconditionally on every access.
>>
>>> Care to try to boot a device without this function?
>>
>> I strongly suspect it would work. Perhaps try it? Give me a device and
>> I'll try it.
>>
>>> Called all over the  place? Hell no, it is default pointing to PCIE2
>>> and functions which need to map the window to another core will do so,
>>> temporarily, but move it back to PCIE2, at least that is the idea, may
>>> be you found a bug?
>>
>> brcmf_pcie_select_core() looks up the core structure from the core id.
>>
>> it then sets BRCMF_PCIE_BAR0_WINDOW according to the core base address.
>>
>> it actually goes to the length of reading it back and trying again if its
>> not set, even, which is at least a little bit horrifying.
>>
>> ------------
>>
>> brcmf_pcie_buscore_{read,write}32() both call
>> brcmf_pcie_buscore_prep_addr()
>>
>> brcmf_pcie_buscore_prep_addr() *unconditionally* programs
>> BRCMF_PCIE_BAR0_WINDOW on *every single* IO access.
>>
>> If you want inefficient - its right there.
>>
>>
>> The SDIO version of the code is actually considerably more efficient on
>> this point - it at least only programs the window register only when it
>> changes, not on every single IO access.
>>
>>> We are
>>> for sure not going to hide the selecting of the window in the
>>> read/write routines, that would result in a giant amount of overhead.
>>
>> Actually it would result in *considerably* less overhead than the current
>> code, that blindly sets the window on every access.
>>
>>> Currently PCIE
>>> devices reach 1.5Gpbs, we need to go faster than that in the near
>> future.
>>
>> I dont need a lesson on writing efficient code, thanks.
>>
>>> We don't want just change that to make it bit nicer..... Why do you
>>> need to see the same in the SDIO and PCIE drivers? SDIO and PCIE
>>> differ in many aspects. Sure some things can be improved in or the
>>> other, but they sure don't need to look alike.
>>
>> I dont "need" to see the same in both drivers. Not where it isnt
>> appropriate.
>>
>> but every part of the drivers that can share code without noticeably
>> impacting performance *should* do so. You should be justifying to me why
>> the code has to be different, not the other way around. Are you sreiously
>> arguing that sharing common code is a bad idea?
>>
>>> It may be ugly, but thusfar it has not caused bugs
>>
>> Oh, I bet you it has... try reading the SDIO version (note the reliance on
>> the dangling ->sbwad pointer) and tell me again that this hasnt caused
>> bugs.
>>
>> Right now, the bulk of the driver code is sat on top of at least two bus
>> drivers with differing IO models, and is working via good luck alone.
>>
>>> The concept in pcie bus part is simple.
>>
>> And differs completely from the SDIO part.
>>
>>> The main core to select is PCIE2 (once you have booted and established
>>> initial communication with firmware) and every routine which needs to
>>> access another core will change the window temporarily and set it back
>>> once done. Please don't mess with this, it works, it is clear and it
>>> is fast.
>>
>> If is anything but fast. changing the window involves traversiong the list
>> of cores. Every. Single. Time. It doesnt *have* to - but thats what
>> brcmf_chip_get_core() does, and brcmf_pcie_select_core() calls it.
>>
> 

Re: brcmfmac bus level addressing issues.

[Arend van Spriel]

On 18-07-17 17:14, Ian Molton wrote:
> Reposting as CC's got reset (not by me).
> 
> On 18/07/17 15:36, Ian Molton wrote:
>> On 18/07/17 13:50, Hante Meuleman wrote:
>>> Hi Ian,
>>>
>>> - Ok, I may not have been reading all the code that well, but I'm pretty
>>> sure, you cannot do without brcmf_pcie_select_core, at least that is how I
>>> "intended" it when I wrote it. Try buying a macbook with a 43602, remove
>>> the function and all calls (8 in total) and try booting it. I might be
>>> mistaken but I'm 99% sure that it won't, give it a try I would say.
>>
>> I'm not buying a macbook just to make a point.
>>
>>> - brcmf_pcie_buscore_{read,write}32() are not the main functions, they are
>>> the corner cases. They exist to satisfy chip.c which is a common module to
>>> support different bus types (SDIO and PCIE). Chip.c just identifies the
>>> cores on the chip and does some magic reset stuff. The main functions to
>>> access memory (during normal operation) are brcmf_pcie_read/write_{all
>>> variants} functions. They matter when it comes to performance, and I was
>>> under the impression you were familiar with the code and wanted to add
>>> window selection functionality to those functions, and that would have
>>> been bad.
>>
>> Did I mention them even once?
>>
>>> Chip.c is just init, we don't care about performance there.
>>
>> Then you invalidated your whole argument for not cleaning it up.
>>
>>> - I'm unfamiliar with sbwad, perhaps it has caused issues in the past. You
>>> wrote : " Can we standardise how this is supposed to work? Its ugly, and
>>> its going to cause bugs" and then you talk about a lot of changes in the
>>> PCIE code.
>>
>> ->swbad is used in the SDIO code to store the last value the window
>> register was programmed with, so as to avoid programming it twice.
>>
>> Im talking about making some attempt to unify the PCIE and SDIO bus
>> code, since they BOTH have the same constraint - an IO window that needs
>> to be moved about depending on the addresses being accessed.
>>
>> Whhat is really needed is for brcmf_pcie_select_core and a variant of it
>> for SDIO (and presumably USB, but I havent looked) to share common code.
>>
>> When you pull the code apart, its really not THAT dissimilar, but it
>> *looks* it and thats not a good thing.
>>
>> The major differences are:
>>
>> The PCIe code sets the window address in ALL the _{read,write}32 calls,
>> wheras the SDIO code does not.
>> The SDIO code does not have an analogue to brcmf_pcie_select_core() -
>> but it should, since it has similar restrictions. It works by good luck.
>>
>>> I don't see a need to cleanup just because we might change
>>> something in the future which could result in a bug because it was
>>> programmed perfectly pretty.
>>
>> -EPARSE
>>
>>> As far as I can see (and I can see a long way)
>>
>> Oh please.
>>
>>> there are not going to be a lot of changes in the pcie code in the
>>> near future.
>>
>> Maybe so - its a lot better than the SDIO code.
>>
>>>  - I don't need to justify to you why the code is different. I wrote the
>>> code and I'll be the one adding new functionality.
>>
>> You realise that this is Linux, right? its public code, and worked on
>> publicly, by many people. You might be a maintainer of a driver, but
>> that does not make you "king of Linux".
>>
>>> You need to justify
>>> prettying up the code while I'm familiar with the code after working on it
>>> for the last 3 years.
>>
>> "Its illegible shite (the SDIO code certainly is) and it should never
>> have got into mainline" sounds like pretty damn good justification to me.
>>
>> If you've been working on this crap for 3 YEARS then *hang your damn
>> head in shame* - I've been working on it for about 5 DAYS now, and I've
>> found 2 complete breakages (one admittedly patched now), a possible
>> firmware bug, and undefined behaviour in the SDIO bus code.
>>
>> But its NOT a pissing contest. I've sent patches. It doesn't matter how
>> long you've been working on the code - if the patches make it better,
>> then take the damn patches. Or at least explain why you aren't going to.
>>
>>> No offence,
>>
>> Too late for that.
>>
>>> but "Im going to be one to get familiarized with your prettifying
>>> changes while you just hop on to the next driver.....
>>
>> You cheeky bastard. "hop onto the next driver". As if that means anything.
>>
>> Just because you're familiar with the stinking pile of crap doesn't mean
>> it should not be touched for fear of breaking it.
>>
>> You're the one ploughing ahead adding new features on top of a garbage
>> driver core.
>>
>> Stop. Listen. Fix it.

Hi Ian,

Now stop yourself and listen. This is no collaboration in any sense. You
mentioned you don't know the maintainers, but you seem to easily
disregard us. Also you said it yourself all your patches are cleanup.
What needs fixing is the gscan feature detection and that one is on me.
So I am chasing that although I am actually on vacation. As long as my
wife does not notice we are ok :-p

>> Then add the new features.
>>
>> You could at the very least give some feedback on the 29 patches I sent
>> cleaning it up.

That really has to wait until I return as well as running some tests on
older 4329 and some newer chipsets.

>> For reference, I've noticed a couple of minor nits in my patchset ( one
>> spelling error in a commit message, one dangling variable, and some
>> minor shenannigans around f0 writes which needs a trivial re-work for
>> compliance with the linux mmc framework (I should use a QUIRK for the
>> func0 accesses - although the original driver code was worse and
>> actively worked around the quirk instead). I will re-spin and post them.
>>
>> In the mean time, I'd appreciate some review on the rest. Since you're
>> clearly full time on this driver, bloody well get on with it.

Actually, Hante is not working full-time on this driver. Neither am I.

Now please stop the insults when you hit some push back or in generalf
for that matter. It is awfully counterproductive for both you and others.

Regards,
Arend

>> -Ian
>>
>>>
>>> Regards, Hante
>>>
>>> -----Original Message-----
>>> From: Ian Molton [mailto:ian@mnementh.co.uk]
>>> Sent: Tuesday, July 18, 2017 1:28 PM
>>> To: Hante Meuleman; linux-wireless@vger.kernel.org
>>> Cc: Arend Van Spriel; Franky Lin
>>> Subject: Re: brcmfmac bus level addressing issues.
>>>
>>> On 18/07/17 11:35, Hante Meuleman wrote:
>>>> Hi Ian,
>>>>
>>>> I've really no idea what you mean.
>>>
>>> You should look at the code...
>>>
>>>> brcmf_pcie_select_core is redundant?
>>>
>>> Essentially yes - there may be a couple of corner cases where the IO
>>> accesses are not performed via brcmf_pcie_buscore_{read,write}32() - but
>>> other than that, brcmf_pcie_buscore_prep_addr() sets the IO window
>>> unconditionally on every access.
>>>
>>>> Care to try to boot a device without this function?
>>>
>>> I strongly suspect it would work. Perhaps try it? Give me a device and
>>> I'll try it.
>>>
>>>> Called all over the  place? Hell no, it is default pointing to PCIE2
>>>> and functions which need to map the window to another core will do so,
>>>> temporarily, but move it back to PCIE2, at least that is the idea, may
>>>> be you found a bug?
>>>
>>> brcmf_pcie_select_core() looks up the core structure from the core id.
>>>
>>> it then sets BRCMF_PCIE_BAR0_WINDOW according to the core base address.
>>>
>>> it actually goes to the length of reading it back and trying again if its
>>> not set, even, which is at least a little bit horrifying.
>>>
>>> ------------
>>>
>>> brcmf_pcie_buscore_{read,write}32() both call
>>> brcmf_pcie_buscore_prep_addr()
>>>
>>> brcmf_pcie_buscore_prep_addr() *unconditionally* programs
>>> BRCMF_PCIE_BAR0_WINDOW on *every single* IO access.
>>>
>>> If you want inefficient - its right there.
>>>
>>>
>>> The SDIO version of the code is actually considerably more efficient on
>>> this point - it at least only programs the window register only when it
>>> changes, not on every single IO access.
>>>
>>>> We are
>>>> for sure not going to hide the selecting of the window in the
>>>> read/write routines, that would result in a giant amount of overhead.
>>>
>>> Actually it would result in *considerably* less overhead than the current
>>> code, that blindly sets the window on every access.
>>>
>>>> Currently PCIE
>>>> devices reach 1.5Gpbs, we need to go faster than that in the near
>>> future.
>>>
>>> I dont need a lesson on writing efficient code, thanks.
>>>
>>>> We don't want just change that to make it bit nicer..... Why do you
>>>> need to see the same in the SDIO and PCIE drivers? SDIO and PCIE
>>>> differ in many aspects. Sure some things can be improved in or the
>>>> other, but they sure don't need to look alike.
>>>
>>> I dont "need" to see the same in both drivers. Not where it isnt
>>> appropriate.
>>>
>>> but every part of the drivers that can share code without noticeably
>>> impacting performance *should* do so. You should be justifying to me why
>>> the code has to be different, not the other way around. Are you sreiously
>>> arguing that sharing common code is a bad idea?
>>>
>>>> It may be ugly, but thusfar it has not caused bugs
>>>
>>> Oh, I bet you it has... try reading the SDIO version (note the reliance on
>>> the dangling ->sbwad pointer) and tell me again that this hasnt caused
>>> bugs.
>>>
>>> Right now, the bulk of the driver code is sat on top of at least two bus
>>> drivers with differing IO models, and is working via good luck alone.
>>>
>>>> The concept in pcie bus part is simple.
>>>
>>> And differs completely from the SDIO part.
>>>
>>>> The main core to select is PCIE2 (once you have booted and established
>>>> initial communication with firmware) and every routine which needs to
>>>> access another core will change the window temporarily and set it back
>>>> once done. Please don't mess with this, it works, it is clear and it
>>>> is fast.
>>>
>>> If is anything but fast. changing the window involves traversiong the list
>>> of cores. Every. Single. Time. It doesnt *have* to - but thats what
>>> brcmf_chip_get_core() does, and brcmf_pcie_select_core() calls it.
>>>
>>
> 

Re: brcmfmac bus level addressing issues.

[Ian Molton]

On 18/07/17 21:44, Arend van Spriel wrote:
> 
> 
>>> Stop. Listen. Fix it.
> 
> Hi Ian,
> 
> Now stop yourself and listen. This is no collaboration in any sense.

Arend - This is not directed at you - you've been polite, and I've no
issue with you.

I had felt that the initial conflict over this was over, but then Hante
wrote his last post. I may have been a bit blunt about the code
initially, but I made *no personal attacks* up until Hante's post (I've
just read *all* my posts on this thread and checked to be sure). I hop
you can understand that his comments earlier were well out of line. I've
actually chosen to take the day off today, before I replied to him with
something I'd really regret posting. I was furious.

> but you seem to easily disregard us.

Actually, no. You yourself have been helpful and responsive.

> Also you said it yourself all your patches are
> cleanup. What needs fixing is the gscan feature detection and that
> one is on me.

Fair enough - I guess now that its in the kernel it needs fixing before
4.13 - but I would argue that no new features should go into the driver
for the short term once that is done. Lets give it a good spring clean.

I am, actually, able to put some time into this, and *want* to help - MY
current employer would benefit greatly from this driver becoming stable.

(as it stands, back to back wpasuplpicant runs make it keel over, as
does module unload and reload).

> So I am chasing that although I am actually on vacation.

Dude - enjoy your vacation. Seriously.

> As long as my wife does not notice we are ok :-p

Shhh :)

>>> You could at the very least give some feedback on the 29 patches I sent
>>> cleaning it up.
> 
> That really has to wait until I return as well as running some tests on
> older 4329 and some newer chipsets.

Fair. When do you return? No pressure - it just means I know I can hold
off and do something else on my project until then.

> Actually, Hante is not working full-time on this driver. Neither am I.

Fair enough. He did wax lyrical about his "seniority" on the driver
though. Pulling rank does not sit well with me.

> Now please stop the insults when you hit some push back or in generalf
> for that matter.

I'd recommend re-reading these threads. Whilst I'd grant my initial
comments in the first cut of the patchset were a little harsh, they were
NOT personally directed.

Hante crossed that line, but I've said my piece on the matter now. I
have nothing further to say to Hante on the matter and consider it over,
if he will do the same.

> It is awfully counterproductive for both you and others.

It certainly is - it cost me a days work, as I couldnt think straight
after this morning.

So - Lets let this be the end of it.

I propose that in the near future we sit down and plan some cleanup work
for this driver (*after* your vacation). Get it stable, make it a
shining example of how to do WiFi.

Deal?

-Ian

> 
> Regards,
> Arend
> 
>>> -Ian
>>>
>>>>
>>>> Regards, Hante
>>>>
>>>> -----Original Message-----
>>>> From: Ian Molton [mailto:ian@mnementh.co.uk]
>>>> Sent: Tuesday, July 18, 2017 1:28 PM
>>>> To: Hante Meuleman; linux-wireless@vger.kernel.org
>>>> Cc: Arend Van Spriel; Franky Lin
>>>> Subject: Re: brcmfmac bus level addressing issues.
>>>>
>>>> On 18/07/17 11:35, Hante Meuleman wrote:
>>>>> Hi Ian,
>>>>>
>>>>> I've really no idea what you mean.
>>>>
>>>> You should look at the code...
>>>>
>>>>> brcmf_pcie_select_core is redundant?
>>>>
>>>> Essentially yes - there may be a couple of corner cases where the IO
>>>> accesses are not performed via brcmf_pcie_buscore_{read,write}32() - but
>>>> other than that, brcmf_pcie_buscore_prep_addr() sets the IO window
>>>> unconditionally on every access.
>>>>
>>>>> Care to try to boot a device without this function?
>>>>
>>>> I strongly suspect it would work. Perhaps try it? Give me a device and
>>>> I'll try it.
>>>>
>>>>> Called all over the  place? Hell no, it is default pointing to PCIE2
>>>>> and functions which need to map the window to another core will do so,
>>>>> temporarily, but move it back to PCIE2, at least that is the idea, may
>>>>> be you found a bug?
>>>>
>>>> brcmf_pcie_select_core() looks up the core structure from the core id.
>>>>
>>>> it then sets BRCMF_PCIE_BAR0_WINDOW according to the core base address.
>>>>
>>>> it actually goes to the length of reading it back and trying again if its
>>>> not set, even, which is at least a little bit horrifying.
>>>>
>>>> ------------
>>>>
>>>> brcmf_pcie_buscore_{read,write}32() both call
>>>> brcmf_pcie_buscore_prep_addr()
>>>>
>>>> brcmf_pcie_buscore_prep_addr() *unconditionally* programs
>>>> BRCMF_PCIE_BAR0_WINDOW on *every single* IO access.
>>>>
>>>> If you want inefficient - its right there.
>>>>
>>>>
>>>> The SDIO version of the code is actually considerably more efficient on
>>>> this point - it at least only programs the window register only when it
>>>> changes, not on every single IO access.
>>>>
>>>>> We are
>>>>> for sure not going to hide the selecting of the window in the
>>>>> read/write routines, that would result in a giant amount of overhead.
>>>>
>>>> Actually it would result in *considerably* less overhead than the current
>>>> code, that blindly sets the window on every access.
>>>>
>>>>> Currently PCIE
>>>>> devices reach 1.5Gpbs, we need to go faster than that in the near
>>>> future.
>>>>
>>>> I dont need a lesson on writing efficient code, thanks.
>>>>
>>>>> We don't want just change that to make it bit nicer..... Why do you
>>>>> need to see the same in the SDIO and PCIE drivers? SDIO and PCIE
>>>>> differ in many aspects. Sure some things can be improved in or the
>>>>> other, but they sure don't need to look alike.
>>>>
>>>> I dont "need" to see the same in both drivers. Not where it isnt
>>>> appropriate.
>>>>
>>>> but every part of the drivers that can share code without noticeably
>>>> impacting performance *should* do so. You should be justifying to me why
>>>> the code has to be different, not the other way around. Are you sreiously
>>>> arguing that sharing common code is a bad idea?
>>>>
>>>>> It may be ugly, but thusfar it has not caused bugs
>>>>
>>>> Oh, I bet you it has... try reading the SDIO version (note the reliance on
>>>> the dangling ->sbwad pointer) and tell me again that this hasnt caused
>>>> bugs.
>>>>
>>>> Right now, the bulk of the driver code is sat on top of at least two bus
>>>> drivers with differing IO models, and is working via good luck alone.
>>>>
>>>>> The concept in pcie bus part is simple.
>>>>
>>>> And differs completely from the SDIO part.
>>>>
>>>>> The main core to select is PCIE2 (once you have booted and established
>>>>> initial communication with firmware) and every routine which needs to
>>>>> access another core will change the window temporarily and set it back
>>>>> once done. Please don't mess with this, it works, it is clear and it
>>>>> is fast.
>>>>
>>>> If is anything but fast. changing the window involves traversiong the list
>>>> of cores. Every. Single. Time. It doesnt *have* to - but thats what
>>>> brcmf_chip_get_core() does, and brcmf_pcie_select_core() calls it.
>>>>
>>>
>>

RE: brcmfmac bus level addressing issues.

[Hante Meuleman]

Dear Ian,

Yes, I was intentionally provocative. Sure, you made no personal attacks,
but this is how you entered my space; I've been working (partrly) for the
last 5 years on brcmfmac and to be honest, I don't work much on it
anymore. But here it is; my screen filled with mails from you with a
number of patches on brcmfmac and some of them were on code I wrote, and
then you use words like:

- horrid
- crap
- awfully
- hideous
- spaghetti mess

No matter what, how impersonal code is supposed to be, I took it personal,
and I got agitated. Now I never directly reply on patches, so I let it be,
but then you start with "brcmfac bus level addressing issues" and you come
with claims I really didn't understand.

Stuff like " The PCIe code sets the window *regardless* of wether its
changed, on *every single* write." Is totally incorrect. Sure if you limit
yourself to the function brcmf_pcie_buscore_{read,write}32(). But you
talked about performance, and msgbuf prototocol is where performance
counts and that don't use those functions. You wrote: " The PCIe code uses
brcmf_pcie_select_core(), which, ultimately, appears to be totally
redundant" and that is simply not true. So I decided to answer that mail
and provoke you a bit. I'm sorry for that, I shouldn't have done that. But
really you may not be directly insulting persons, but when you write: "
Can we standardise how this is supposed to work? Its ugly, and its going
to cause bugs, ultimately" then I just read another negative strong word
(in this case ugly) and it is partly about code I wrote.

You obviously spent some time on creating all these patches, but why
provoke/agitate people? Why use such strong words? You may not consider
them personally, but I just explained why I do. Can you at least
understand that? Just some word of advice and then I hope we can leave it
to that. You can achieve much more without those negative strong words,
the words I listed above are what pop up with me every mail you wrote, not
the code.

Regards,
Hante


-----Original Message-----
From: Ian Molton [mailto:ian@mnementh.co.uk]
Sent: Wednesday, July 19, 2017 12:45 AM
To: Arend van Spriel; linux-wireless@vger.kernel.org
Cc: Franky Lin; brcm80211-dev-list@broadcom.com; Hante Meuleman
Subject: Re: brcmfmac bus level addressing issues.

On 18/07/17 21:44, Arend van Spriel wrote:
>
>
>>> Stop. Listen. Fix it.
>
> Hi Ian,
>
> Now stop yourself and listen. This is no collaboration in any sense.

Arend - This is not directed at you - you've been polite, and I've no
issue with you.

I had felt that the initial conflict over this was over, but then Hante
wrote his last post. I may have been a bit blunt about the code initially,
but I made *no personal attacks* up until Hante's post (I've just read
*all* my posts on this thread and checked to be sure). I hop you can
understand that his comments earlier were well out of line. I've actually
chosen to take the day off today, before I replied to him with something
I'd really regret posting. I was furious.

> but you seem to easily disregard us.

Actually, no. You yourself have been helpful and responsive.

> Also you said it yourself all your patches are cleanup. What needs
> fixing is the gscan feature detection and that one is on me.

Fair enough - I guess now that its in the kernel it needs fixing before
4.13 - but I would argue that no new features should go into the driver
for the short term once that is done. Lets give it a good spring clean.

I am, actually, able to put some time into this, and *want* to help - MY
current employer would benefit greatly from this driver becoming stable.

(as it stands, back to back wpasuplpicant runs make it keel over, as does
module unload and reload).

> So I am chasing that although I am actually on vacation.

Dude - enjoy your vacation. Seriously.

> As long as my wife does not notice we are ok :-p

Shhh :)

>>> You could at the very least give some feedback on the 29 patches I
>>> sent cleaning it up.
>
> That really has to wait until I return as well as running some tests
> on older 4329 and some newer chipsets.

Fair. When do you return? No pressure - it just means I know I can hold
off and do something else on my project until then.

> Actually, Hante is not working full-time on this driver. Neither am I.

Fair enough. He did wax lyrical about his "seniority" on the driver
though. Pulling rank does not sit well with me.

> Now please stop the insults when you hit some push back or in generalf
> for that matter.

I'd recommend re-reading these threads. Whilst I'd grant my initial
comments in the first cut of the patchset were a little harsh, they were
NOT personally directed.

Hante crossed that line, but I've said my piece on the matter now. I have
nothing further to say to Hante on the matter and consider it over, if he
will do the same.

> It is awfully counterproductive for both you and others.

It certainly is - it cost me a days work, as I couldnt think straight
after this morning.

So - Lets let this be the end of it.

I propose that in the near future we sit down and plan some cleanup work
for this driver (*after* your vacation). Get it stable, make it a shining
example of how to do WiFi.

Deal?

-Ian

>
> Regards,
> Arend
>
>>> -Ian
>>>
>>>>
>>>> Regards, Hante
>>>>
>>>> -----Original Message-----
>>>> From: Ian Molton [mailto:ian@mnementh.co.uk]
>>>> Sent: Tuesday, July 18, 2017 1:28 PM
>>>> To: Hante Meuleman; linux-wireless@vger.kernel.org
>>>> Cc: Arend Van Spriel; Franky Lin
>>>> Subject: Re: brcmfmac bus level addressing issues.
>>>>
>>>> On 18/07/17 11:35, Hante Meuleman wrote:
>>>>> Hi Ian,
>>>>>
>>>>> I've really no idea what you mean.
>>>>
>>>> You should look at the code...
>>>>
>>>>> brcmf_pcie_select_core is redundant?
>>>>
>>>> Essentially yes - there may be a couple of corner cases where the
>>>> IO accesses are not performed via
>>>> brcmf_pcie_buscore_{read,write}32() - but other than that,
>>>> brcmf_pcie_buscore_prep_addr() sets the IO window unconditionally on
every access.
>>>>
>>>>> Care to try to boot a device without this function?
>>>>
>>>> I strongly suspect it would work. Perhaps try it? Give me a device
>>>> and I'll try it.
>>>>
>>>>> Called all over the  place? Hell no, it is default pointing to
>>>>> PCIE2 and functions which need to map the window to another core
>>>>> will do so, temporarily, but move it back to PCIE2, at least that
>>>>> is the idea, may be you found a bug?
>>>>
>>>> brcmf_pcie_select_core() looks up the core structure from the core
id.
>>>>
>>>> it then sets BRCMF_PCIE_BAR0_WINDOW according to the core base
address.
>>>>
>>>> it actually goes to the length of reading it back and trying again
>>>> if its not set, even, which is at least a little bit horrifying.
>>>>
>>>> ------------
>>>>
>>>> brcmf_pcie_buscore_{read,write}32() both call
>>>> brcmf_pcie_buscore_prep_addr()
>>>>
>>>> brcmf_pcie_buscore_prep_addr() *unconditionally* programs
>>>> BRCMF_PCIE_BAR0_WINDOW on *every single* IO access.
>>>>
>>>> If you want inefficient - its right there.
>>>>
>>>>
>>>> The SDIO version of the code is actually considerably more
>>>> efficient on this point - it at least only programs the window
>>>> register only when it changes, not on every single IO access.
>>>>
>>>>> We are
>>>>> for sure not going to hide the selecting of the window in the
>>>>> read/write routines, that would result in a giant amount of
overhead.
>>>>
>>>> Actually it would result in *considerably* less overhead than the
>>>> current code, that blindly sets the window on every access.
>>>>
>>>>> Currently PCIE
>>>>> devices reach 1.5Gpbs, we need to go faster than that in the near
>>>> future.
>>>>
>>>> I dont need a lesson on writing efficient code, thanks.
>>>>
>>>>> We don't want just change that to make it bit nicer..... Why do
>>>>> you need to see the same in the SDIO and PCIE drivers? SDIO and
>>>>> PCIE differ in many aspects. Sure some things can be improved in
>>>>> or the other, but they sure don't need to look alike.
>>>>
>>>> I dont "need" to see the same in both drivers. Not where it isnt
>>>> appropriate.
>>>>
>>>> but every part of the drivers that can share code without
>>>> noticeably impacting performance *should* do so. You should be
>>>> justifying to me why the code has to be different, not the other
>>>> way around. Are you sreiously arguing that sharing common code is a
bad idea?
>>>>
>>>>> It may be ugly, but thusfar it has not caused bugs
>>>>
>>>> Oh, I bet you it has... try reading the SDIO version (note the
>>>> reliance on the dangling ->sbwad pointer) and tell me again that
>>>> this hasnt caused bugs.
>>>>
>>>> Right now, the bulk of the driver code is sat on top of at least
>>>> two bus drivers with differing IO models, and is working via good
luck alone.
>>>>
>>>>> The concept in pcie bus part is simple.
>>>>
>>>> And differs completely from the SDIO part.
>>>>
>>>>> The main core to select is PCIE2 (once you have booted and
>>>>> established initial communication with firmware) and every routine
>>>>> which needs to access another core will change the window
>>>>> temporarily and set it back once done. Please don't mess with
>>>>> this, it works, it is clear and it is fast.
>>>>
>>>> If is anything but fast. changing the window involves traversiong
>>>> the list of cores. Every. Single. Time. It doesnt *have* to - but
>>>> thats what
>>>> brcmf_chip_get_core() does, and brcmf_pcie_select_core() calls it.
>>>>
>>>
>>

Re: brcmfmac bus level addressing issues.

[Ian Molton]

On 19/07/17 09:39, Hante Meuleman wrote:
> Dear Ian,

Hi,

> Stuff like " The PCIe code sets the window *regardless* of wether its
> changed, on *every single* write." Is totally incorrect. Sure if you limit
> yourself to the function brcmf_pcie_buscore_{read,write}32(). But you
> talked about performance, and msgbuf prototocol is where performance
> counts and that don't use those functions. You wrote: " The PCIe code uses
> brcmf_pcie_select_core(), which, ultimately, appears to be totally
> redundant" and that is simply not true.

This actually kinda underlines my point - at the time I was, as you say,
referring to the _{read,write}32() functions (which is where I'd been
cleaning up in the sdio code too.

> So I decided to answer that mail
> and provoke you a bit. I'm sorry for that, I shouldn't have done that.

Fair enough. I lost my cool in my reply to that too, so lets just start
again. Fair?

> You obviously spent some time on creating all these patches, but why
> provoke/agitate people? Why use such strong words? You may not consider
> them personally, but I just explained why I do. Can you at least
> understand that?

Sorry it came over as personal. Its not. And yes, it sometimes hurts
when people are critical about code you've written - and I've been on
the receiving end of it before now - and they were correct, and I was
wrong at the time. It has to be said though - that code is not pretty
(sdio side in particular - as mentioned before, the PCIe side is a fair
bit better).

> Just some word of advice and then I hope we can leave it to that.
Yes, lets.

-Ian

Re: brcmfmac bus level addressing issues.

[Arend van Spriel]

On 19-07-17 00:45, Ian Molton wrote:

[...]

> Fair enough - I guess now that its in the kernel it needs fixing before
> 4.13 - but I would argue that no new features should go into the driver
> for the short term once that is done. Lets give it a good spring clean.
> 
> I am, actually, able to put some time into this, and *want* to help - MY
> current employer would benefit greatly from this driver becoming stable.
> 
> (as it stands, back to back wpasuplpicant runs make it keel over, as
> does module unload and reload).

It is good to get such feedback. We should get in details about this.

>> So I am chasing that although I am actually on vacation.
> 
> Dude - enjoy your vacation. Seriously.
> 

[...]

>> That really has to wait until I return as well as running some tests on
>> older 4329 and some newer chipsets.
> 
> Fair. When do you return? No pressure - it just means I know I can hold
> off and do something else on my project until then.

Vacation is only this week.

Regards,
Arend

Re: brcmfmac bus level addressing issues.

[Ian Molton]

On 19/07/17 12:47, Arend van Spriel wrote:
> On 19-07-17 00:45, Ian Molton wrote:

>> (as it stands, back to back wpasuplpicant runs make it keel over, as
>> does module unload and reload).
> 
> It is good to get such feedback. We should get in details about this.

I'll reply with a couple of logs on here. I was going to just post the
one, but of course, typically, it did not fail the same way this time...

# rmmod brcmfmac

brcmfmac: brcmf_proto_bcdc_query_dcmd: brcmf_proto_bcdc_msg failed
w/status -5
brcmfmac: brcmf_cfg80211_get_tx_power: error (-5)
brcmfmac: brcmf_fil_cmd_data: bus is down. we have nothing to do.
brcmfmac: brcmf_link_down: WLC_DISASSOC failed (-5)
brcmfmac: brcmf_fil_cmd_data: bus is down. we have nothing to do.
brcmfmac: brcmf_fil_cmd_data: bus is down. we have nothing to do.
brcmfmac: brcmf_fil_cmd_data: bus is down. we have nothing to do.
brcmfmac: brcmf_cfg80211_get_channel: chanspec failed (-5)
Enter

# modprobe brcmfmac
 [1 ] core 0x800:49 base 0x18000000 wrap 0x18100000
 [2 ] core 0x812:39 base 0x18001000 wrap 0x18101000
 [3 ] core 0x829:21 base 0x18002000 wrap 0x18102000
 [4 ] core 0x82a:9  base 0x18003000 wrap 0x18103000
 [5 ] core 0x80e:22 base 0x18004000 wrap 0x18104000
 [6 ] core 0x135:0  base 0x00000000 wrap 0x18105000
 [7 ] core 0x240:0  base 0x00000000 wrap 0x18106000
Enter
RAM: base=0x0 size=524288 (0x80000) sr=65536 (0x10000)
Found BCM43430, AXI backplane, rev=1
ccrev=49, pmurev=24, pmucaps=0x39d25f18
Enter
Enter
Enter
Enter
brcmfmac: brcmf_c_preinit_dcmds: Firmware version = wl0: Aug 29 2016
20:48:16 version 7.45.41.26 (r640327) FWID 01-4527cfab
IPv6: ADDRCONF(NETDEV_UP): wlan0: link is not ready
IPv6: ADDRCONF(NETDEV_CHANGE): wlan0: link becomes ready

#
# wpa_supplicant -B -i wlan0 -c <(wpa_passphrase vortex24 password) &&
dhclient wlan0

Successfully initialized wpa_supplicant
rfkill: Cannot open RFKILL control device
brcmfmac: brcmf_link_down: WLC_DISASSOC failed (-11)
rfkill: Cannot open RFKILL control device
brcmfmac: brcmf_cfg80211_escan: Connecting: status (3)
brcmfmac: brcmf_cfg80211_scan: scan error (-11)

# wget --no-check-certificate
https://cdn.kernel.org/pub/linux/kernel/v4.x/linux-4.12.tar.xz
--2017-07-19 19:13:22--
https://cdn.kernel.org/pub/linux/kernel/v4.x/linux-4.12.tar.xz
Resolving cdn.kernel.org (cdn.kernel.org)... 151.101.17.176,
2a04:4e42:4::432
Connecting to cdn.kernel.org (cdn.kernel.org)|151.101.17.176|:443...
connected.
WARNING: The certificate of ‘cdn.kernel.org’ is not trusted.
WARNING: The certificate of ‘cdn.kernel.org’ hasn't got a known issuer.
HTTP request sent, awaiting response... 200 OK
Length: 99186576 (95M) [application/x-xz]
Saving to: ‘linux-4.12.tar.xz.30’

linux-4.12.tar.xz.3   1%[                    ]   1.81M  1.08MB/s
      ^C
root@martel:~# brcmfmac: brcmf_cfg80211_escan: Connecting: status (3)
brcmfmac: brcmf_cfg80211_scan: scan error (-11)

# wget --no-check-certificate
https://cdn.kernel.org/pub/linux/kernel/v4.x/linux-4.12.tar.xz
--2017-07-19 19:15:01--
https://cdn.kernel.org/pub/linux/kernel/v4.x/linux-4.12.tar.xz
Resolving cdn.kernel.org (cdn.kernel.org)...
<never completed>

brcmfmac: brcmf_cfg80211_escan: Connecting: status (3)
brcmfmac: brcmf_cfg80211_scan: scan error (-11)
brcmfmac: brcmf_cfg80211_escan: Scanning already: status (1)
brcmfmac: brcmf_cfg80211_scan: scan error (-11)
brcmfmac: brcmf_cfg80211_escan: Connecting: status (3)
brcmfmac: brcmf_cfg80211_scan: scan error (-11)
brcmfmac: brcmf_cfg80211_escan: Connecting: status (3)
brcmfmac: brcmf_cfg80211_scan: scan error (-11)