Re: Repeating "unregister_netdevice: waiting for lo to become free" caused by upstream 76da0704507bb ("ipv6: only call ip6_route_dev_notify() once for NETDEV_UNREGISTER")

[Rafał Miłecki]

On 25 April 2018 at 16:44, Rafał Miłecki <zaj...@gmail.com> wrote:
> On 25.04.2018 16:30, Konstantin Khlebnikov wrote:
>>
>> On 25.04.2018 17:16, Rafał Miłecki wrote:
>>>
>>> On 23.04.2018 15:08, Rafał Miłecki wrote:
>>>>
>>>> I've just updated my kernel 4.4.x and noticed a regression. Bisecting
>>>> pointed me to the commit 2417da3f4d6bc ("ipv6: only call
>>>> ip6_route_dev_notify() once for NETDEV_UNREGISTER") [0] which is
>>>> backport of upstream 76da0704507bb. That backported commit has
>>>> appeared in a 4.4.103.
>>>>
>>>> I use OpenWrt/LEDE [1] distribution and LXC [2] 1.1.5. After stopping
>>>> a container I start getting these messages:
>>>> [  229.419188] unregister_netdevice: waiting for lo to become free.
>>>> Usage count = 1
>>>> [  239.660408] unregister_netdevice: waiting for lo to become free.
>>>> Usage count = 1
>>>> [  249.839189] unregister_netdevice: waiting for lo to become free.
>>>> Usage count = 1
>>>> (...)
>>>>
>>>> Trying to start LXC nevertheless results in lxc-start command hang
>>>> around network configuration. Trying to query LXC state afterwards
>>>> results in a lxc-info command hang too.
>>>>
>>>> I tried Googling for this issue and found similar reports:
>>>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1729637
>>>> https://github.com/fnproject/fn/issues/686
>>>>
>>>> https://lime-technology.com/forums/topic/66863-kernelunregister_netdevice-waiting-for-lo-to-become-free-usage-count-1/
>>>> all of them related to the Docker, which is probably a similar use
>>>> case to the LXC.
>>>>
>>>> I couldn't find any reference to commit 76da0704507bb that could
>>>> suggest fixing the problem I'm seeing.
>>>>
>>>> Does anyone have an idea what is the issue I'm seeing about? Or even
>>>> better, how to fix it? Can I provide any additional info that would
>>>> help?
>>>>
>>>>
>>>> [0]
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?h=linux-4.4.y=2417da3f4d6bc4fc6c77f613f0e2264090892aa5
>>>> [1] https://openwrt.org/
>>>> [2] https://linuxcontainers.org/
>>>
>>>
>>> Today I tried 4.14.34 to see if that helps. Unfortunately it doesn't. I
>>> still experience the same problem.
>>>
>>>  From reading various reports regarding that "unregister_netdevice:
>>> waiting for lo to become free" message it appears the problem is caused
>>> by a leaking dst refcnt somewhere in the kernel code.
>>>
>>> I found links to few commit fixing leaks at various places:
>>> 4a31a6b19f9dd ("sctp: fix dst refcnt leak in sctp_v4_get_dst")
>>> 957d761cf91cd ("sctp: fix dst refcnt leak in sctp_v6_get_dst()")
>>> 4ee806d51176b ("net: tcp: close sock if net namespace is exiting")
>>> d747a7a51b009 ("tcp: reset sk_rx_dst in tcp_disconnect()")
>>> 751eb6b6042a5 ("ipv6: addrconf: fix dev refcont leak when DAD failed")
>>>
>>> All above patches are present in the linux-v4.4.y and are part of kernel
>>> 4.4.124 I use. So it seems I'm facing yet another dst refcnt leak.
>>>
>>> Could commit 2417da3f4d6bc ("ipv6: only call ip6_route_dev_notify() once
>>> for NETDEV_UNREGISTER") introduce a new dst refcnt leak? Or does it only
>>> expost existing one?
>>
>>
>> Mathias Tillman reported this as "4.4.103 linux kernel regression".
>> Last message in that thread (which I couldn't find in mailing list
>> archives) had:
>> | As it turns out, it's due to a patch in the Turris Omnia/OpenWRT code
>> that adds a in6_dev_get call without calling in6_dev_put.
>
>
> Wow, this is very helpful, thank you!
>
> Somehow I didn't even think about OpenWrt downstream patches. Too bad
> this wasn't reported to the OpenWrt community, I spent 2 days on this.
> There is indeed:
> target/linux/generic/patches-4.4/670-ipv6-allow-rejecting-with-source-address-failed-policy.patch
> [PATCH 1/2] ipv6: allow rejecting with "source address failed policy"
>
> I'll move this issue discussion to the OpenWrt/LEDE now, I hope we can
> sort it out.

For a reference it has been fixed in OpenWrt/LEDE by Felix in:

1) master branch:
https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=58f7b5b96c301176d639540df4723c798af2a999

2) lede-17.01 branch
https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=999bb66b20b03c753801ecebf1ec2a03c6a63c96

-- 
Rafał

Re: Repeating "unregister_netdevice: waiting for lo to become free" caused by upstream 76da0704507bb ("ipv6: only call ip6_route_dev_notify() once for NETDEV_UNREGISTER")

[Rafał Miłecki]

On 25.04.2018 16:30, Konstantin Khlebnikov wrote:

On 25.04.2018 17:16, Rafał Miłecki wrote:

On 23.04.2018 15:08, Rafał Miłecki wrote:

I've just updated my kernel 4.4.x and noticed a regression. Bisecting
pointed me to the commit 2417da3f4d6bc ("ipv6: only call
ip6_route_dev_notify() once for NETDEV_UNREGISTER") [0] which is
backport of upstream 76da0704507bb. That backported commit has
appeared in a 4.4.103.

I use OpenWrt/LEDE [1] distribution and LXC [2] 1.1.5. After stopping
a container I start getting these messages:
[  229.419188] unregister_netdevice: waiting for lo to become free. Usage count 
= 1
[  239.660408] unregister_netdevice: waiting for lo to become free. Usage count 
= 1
[  249.839189] unregister_netdevice: waiting for lo to become free. Usage count 
= 1
(...)

Trying to start LXC nevertheless results in lxc-start command hang
around network configuration. Trying to query LXC state afterwards
results in a lxc-info command hang too.

I tried Googling for this issue and found similar reports:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1729637
https://github.com/fnproject/fn/issues/686
https://lime-technology.com/forums/topic/66863-kernelunregister_netdevice-waiting-for-lo-to-become-free-usage-count-1/
all of them related to the Docker, which is probably a similar use
case to the LXC.

I couldn't find any reference to commit 76da0704507bb that could
suggest fixing the problem I'm seeing.

Does anyone have an idea what is the issue I'm seeing about? Or even
better, how to fix it? Can I provide any additional info that would
help?


[0] 
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?h=linux-4.4.y=2417da3f4d6bc4fc6c77f613f0e2264090892aa5
[1] https://openwrt.org/
[2] https://linuxcontainers.org/


Today I tried 4.14.34 to see if that helps. Unfortunately it doesn't. I
still experience the same problem.

 From reading various reports regarding that "unregister_netdevice:
waiting for lo to become free" message it appears the problem is caused
by a leaking dst refcnt somewhere in the kernel code.

I found links to few commit fixing leaks at various places:
4a31a6b19f9dd ("sctp: fix dst refcnt leak in sctp_v4_get_dst")
957d761cf91cd ("sctp: fix dst refcnt leak in sctp_v6_get_dst()")
4ee806d51176b ("net: tcp: close sock if net namespace is exiting")
d747a7a51b009 ("tcp: reset sk_rx_dst in tcp_disconnect()")
751eb6b6042a5 ("ipv6: addrconf: fix dev refcont leak when DAD failed")

All above patches are present in the linux-v4.4.y and are part of kernel
4.4.124 I use. So it seems I'm facing yet another dst refcnt leak.

Could commit 2417da3f4d6bc ("ipv6: only call ip6_route_dev_notify() once
for NETDEV_UNREGISTER") introduce a new dst refcnt leak? Or does it only
expost existing one?


Mathias Tillman reported this as "4.4.103 linux kernel regression".
Last message in that thread (which I couldn't find in mailing list archives) 
had:
| As it turns out, it's due to a patch in the Turris Omnia/OpenWRT code that 
adds a in6_dev_get call without calling in6_dev_put.


Wow, this is very helpful, thank you!

Somehow I didn't even think about OpenWrt downstream patches. Too bad
this wasn't reported to the OpenWrt community, I spent 2 days on this.
There is indeed:
target/linux/generic/patches-4.4/670-ipv6-allow-rejecting-with-source-address-failed-policy.patch
[PATCH 1/2] ipv6: allow rejecting with "source address failed policy"

I'll move this issue discussion to the OpenWrt/LEDE now, I hope we can
sort it out.

Re: Repeating "unregister_netdevice: waiting for lo to become free" caused by upstream 76da0704507bb ("ipv6: only call ip6_route_dev_notify() once for NETDEV_UNREGISTER")

[Rafał Miłecki]

On 23.04.2018 15:08, Rafał Miłecki wrote:

I've just updated my kernel 4.4.x and noticed a regression. Bisecting
pointed me to the commit 2417da3f4d6bc ("ipv6: only call
ip6_route_dev_notify() once for NETDEV_UNREGISTER") [0] which is
backport of upstream 76da0704507bb. That backported commit has
appeared in a 4.4.103.

I use OpenWrt/LEDE [1] distribution and LXC [2] 1.1.5. After stopping
a container I start getting these messages:
[  229.419188] unregister_netdevice: waiting for lo to become free. Usage count 
= 1
[  239.660408] unregister_netdevice: waiting for lo to become free. Usage count 
= 1
[  249.839189] unregister_netdevice: waiting for lo to become free. Usage count 
= 1
(...)

Trying to start LXC nevertheless results in lxc-start command hang
around network configuration. Trying to query LXC state afterwards
results in a lxc-info command hang too.

I tried Googling for this issue and found similar reports:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1729637
https://github.com/fnproject/fn/issues/686
https://lime-technology.com/forums/topic/66863-kernelunregister_netdevice-waiting-for-lo-to-become-free-usage-count-1/
all of them related to the Docker, which is probably a similar use
case to the LXC.

I couldn't find any reference to commit 76da0704507bb that could
suggest fixing the problem I'm seeing.

Does anyone have an idea what is the issue I'm seeing about? Or even
better, how to fix it? Can I provide any additional info that would
help?


[0] 
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?h=linux-4.4.y=2417da3f4d6bc4fc6c77f613f0e2264090892aa5
[1] https://openwrt.org/
[2] https://linuxcontainers.org/


Today I tried 4.14.34 to see if that helps. Unfortunately it doesn't. I
still experience the same problem.

From reading various reports regarding that "unregister_netdevice:
waiting for lo to become free" message it appears the problem is caused
by a leaking dst refcnt somewhere in the kernel code.

I found links to few commit fixing leaks at various places:
4a31a6b19f9dd ("sctp: fix dst refcnt leak in sctp_v4_get_dst")
957d761cf91cd ("sctp: fix dst refcnt leak in sctp_v6_get_dst()")
4ee806d51176b ("net: tcp: close sock if net namespace is exiting")
d747a7a51b009 ("tcp: reset sk_rx_dst in tcp_disconnect()")
751eb6b6042a5 ("ipv6: addrconf: fix dev refcont leak when DAD failed")

All above patches are present in the linux-v4.4.y and are part of kernel
4.4.124 I use. So it seems I'm facing yet another dst refcnt leak.

Could commit 2417da3f4d6bc ("ipv6: only call ip6_route_dev_notify() once
for NETDEV_UNREGISTER") introduce a new dst refcnt leak? Or does it only
expost existing one?

Repeating "unregister_netdevice: waiting for lo to become free" caused by upstream 76da0704507bb ("ipv6: only call ip6_route_dev_notify() once for NETDEV_UNREGISTER")

[Rafał Miłecki]

Hi,

I've just updated my kernel 4.4.x and noticed a regression. Bisecting
pointed me to the commit 2417da3f4d6bc ("ipv6: only call
ip6_route_dev_notify() once for NETDEV_UNREGISTER") [0] which is
backport of upstream 76da0704507bb. That backported commit has
appeared in a 4.4.103.

I use OpenWrt/LEDE [1] distribution and LXC [2] 1.1.5. After stopping
a container I start getting these messages:
[  229.419188] unregister_netdevice: waiting for lo to become free.
Usage count = 1
[  239.660408] unregister_netdevice: waiting for lo to become free.
Usage count = 1
[  249.839189] unregister_netdevice: waiting for lo to become free.
Usage count = 1
(...)

Trying to start LXC nevertheless results in lxc-start command hang
around network configuration. Trying to query LXC state afterwards
results in a lxc-info command hang too.

I tried Googling for this issue and found similar reports:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1729637
https://github.com/fnproject/fn/issues/686
https://lime-technology.com/forums/topic/66863-kernelunregister_netdevice-waiting-for-lo-to-become-free-usage-count-1/
all of them related to the Docker, which is probably a similar use
case to the LXC.

I couldn't find any reference to commit 76da0704507bb that could
suggest fixing the problem I'm seeing.

Does anyone have an idea what is the issue I'm seeing about? Or even
better, how to fix it? Can I provide any additional info that would
help?


[0] 
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?h=linux-4.4.y=2417da3f4d6bc4fc6c77f613f0e2264090892aa5
[1] https://openwrt.org/
[2] https://linuxcontainers.org/

-- 
Rafał

Re: [PATCH 00/47] Netfilter/IPVS updates for net-next

[Rafał Miłecki]

Hi Pablo,

> The following patchset contains Netfilter/IPVS updates for your net-next
> tree. This batch comes with more input sanitization for xtables to
> address bug reports from fuzzers, preparation works to the flowtable
> infrastructure and assorted updates. In no particular order, they are:
>
> 1) Make sure userspace provides a valid standard target verdict, from
>Florian Westphal.
>
> 2) Sanitize error target size, also from Florian.
>
> 3) Validate that last rule in basechain matches underflow/policy since
>userspace assumes this when decoding the ruleset blob that comes
>from the kernel, from Florian.
>
> 4) Consolidate hook entry checks through xt_check_table_hooks(),
>patch from Florian.
>
> 5) Cap ruleset allocations at 512 mbytes, 134217728 rules and reject
>very large compat offset arrays, so we have a reasonable upper limit
>and fuzzers don't exercise the oom-killer. Patches from Florian.
>
> 6) Several WARN_ON checks on xtables mutex helper, from Florian.
>
> 7) xt_rateest now has a hashtable per net, from Cong Wang.
>
> 8) Consolidate counter allocation in xt_counters_alloc(), from Florian.
>
> 9) Earlier xt_table_unlock() call in {ip,ip6,arp,eb}tables, patch
>from Xin Long.
>
> 10) Set FLOW_OFFLOAD_DIR_* to IP_CT_DIR_* definitions, patch from
> Felix Fietkau.
>
> 11) Consolidate code through flow_offload_fill_dir(), also from Felix.
>
> 12) Inline ip6_dst_mtu_forward() just like ip_dst_mtu_maybe_forward()
> to remove a dependency with flowtable and ipv6.ko, from Felix.
>
> 13) Cache mtu size in flow_offload_tuple object, this is safe for
> forwarding as f87c10a8aa1e describes, from Felix.
>
> 14) Rename nf_flow_table.c to nf_flow_table_core.o, to simplify too
> modular infrastructure, from Felix.

I see you mentioned changes from Felix in the pull request but:
1) I don't see any commits from Felix listed below
2) I don't think you sent any of these patches

Can you take a look at what has happened to them, please?

Re: [PATCH] wcn36xx: Disable 5GHz for wcn3610

[Rafał Miłecki]

On 03/29/2018 08:20 AM, Ramon Fried wrote:

wcn3610 can only operate on 2.4GHz band due to RF limitation.
If wcn36xx digital block is associated with an external IRIS
RF module, retrieve the id and disable 5GHz band in case of
wcn3610 id.

Signed-off-by: Ramon Fried 
---
 drivers/net/wireless/ath/wcn36xx/main.c| 4 +++-
 drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/main.c 
b/drivers/net/wireless/ath/wcn36xx/main.c
index ab5be6d2c691..833531a68c95 100644
--- a/drivers/net/wireless/ath/wcn36xx/main.c
+++ b/drivers/net/wireless/ath/wcn36xx/main.c
@@ -1146,7 +1146,7 @@ static int wcn36xx_init_ieee80211(struct wcn36xx *wcn)
BIT(NL80211_IFTYPE_MESH_POINT);

wcn->hw->wiphy->bands[NL80211_BAND_2GHZ] = _band_2ghz;
-   if (wcn->rf_id != RF_IRIS_WCN3620)
+   if (wcn->rf_id != RF_IRIS_WCN3610 && wcn->rf_id != RF_IRIS_WCN3620)
wcn->hw->wiphy->bands[NL80211_BAND_5GHZ] = _band_5ghz;

wcn->hw->wiphy->max_scan_ssids = WCN36XX_MAX_SCAN_SSIDS;
@@ -1248,6 +1248,8 @@ static int wcn36xx_platform_get_resources(struct wcn36xx 
*wcn,
if (iris_node) {
if (of_device_is_compatible(iris_node, "qcom,wcn3620"))
wcn->rf_id = RF_IRIS_WCN3620;
+   else if (of_device_is_compatible(iris_node, "qcom,wcn3610"))
+   wcn->rf_id = RF_IRIS_WCN3620;


RF_IRIS_WCN3610 ?

Re: [QUESTION] Mainline support for B43_PHY_AC wifi cards

[Rafał Miłecki]

On 23 March 2018 at 15:09, Juri Lelli <juri.le...@gmail.com> wrote:
> On 23/03/18 14:43, Rafał Miłecki wrote:
>> Hi,
>>
>> On 23 March 2018 at 10:47, Juri Lelli <juri.le...@gmail.com> wrote:
>> > I've got a Dell XPS 13 9343/0TM99H (BIOS A15 01/23/2018) mounting a
>> > BCM4352 802.11ac (rev 03) wireless card and so far I've been using it on
>> > Fedora with broadcom-wl package (which I believe installs Broadcom's STA
>> > driver?). It works good apart from occasional hiccups after suspend.
>> >
>> > I'd like to get rid of that dependency (you can understand that it's
>> > particularly annoying when testing mainline kernels), but I found out
>> > that support for my card is BROKEN in mainline [1]. Just to see what
>> > happens, I forcibly enabled it witnessing that it indeed crashes like
>> > below as Kconfig warns. :)
>> >
>> >  bcma: bus0: Found chip with id 0x4352, rev 0x03 and package 0x00
>> >  bcma: bus0: Core 0 found: ChipCommon (manuf 0x4BF, id 0x800, rev 0x2B, 
>> > class 0x0)
>> >  bcma: bus0: Core 1 found: IEEE 802.11 (manuf 0x4BF, id 0x812, rev 0x2A, 
>> > class 0x0)
>> >  bcma: bus0: Core 2 found: ARM CR4 (manuf 0x4BF, id 0x83E, rev 0x02, class 
>> > 0x0)
>> >  bcma: bus0: Core 3 found: PCIe Gen2 (manuf 0x4BF, id 0x83C, rev 0x01, 
>> > class 0x0)
>> >  bcma: bus0: Core 4 found: USB 2.0 Device (manuf 0x4BF, id 0x81A, rev 
>> > 0x11, class 0x0)
>> >  bcma: Unsupported SPROM revision: 11
>> >  bcma: bus0: Invalid SPROM read from the PCIe card, trying to use fallback 
>> > SPROM
>> >  bcma: bus0: Using fallback SPROM failed (err -2)
>> >  bcma: bus0: No SPROM available
>> >  bcma: bus0: Bus registered
>> >  b43-phy0: Broadcom 4352 WLAN found (core revision 42)
>> >  b43-phy0: Found PHY: Analog 12, Type 11 (AC), Revision 1
>> >  b43-phy0: Found Radio: Manuf 0x17F, ID 0x2069, Revision 4, Version 0
>> >  BUG: unable to handle kernel NULL pointer dereference at 
>>
>> This isn't really useful without a full backtrace.
>
> Sure. I cut it here because I didn't expect people to debug what is
> already known to be broken (but still it seemed to carry useful
> information about the hw). :)

Please paste the remaining part if you still got it.


>> > So, question: is replacing my card the only way I can get rid of this
>> > downstream dependency? :(
>>
>> It's definitely the cheapest way. Getting AC PHY into anything usable
>> (proper setup that will allow Tx & Rx anything) would probably take
>> weeks or months of development. I'm not even going to estimate cost of
>> adding support for 802.11n and 802.11ac features. I was the last
>> person actively working on b43, right now I spend my free time on
>> other hobby projects. Few people were planning to help but it seems it
>> never worked out for them.
>
> I see. Just wondering why even if Broadcom's STA solution seems to work
> fine, it is not mainline. Maybe a maintenance problem? But Fedora ships
> with very recent kernels, so I'd expect the driver to work with mainline
> (I tried compiling that against mainline, but I got errors that I didn't
> spend time figuring out how to fix).
>
> Do you know what's the deal w.r.t. the STA driver?

Driver being closed source and company not willing to open source it
is usually a big problem getting it mainline...

-- 
Rafał

Re: [QUESTION] Mainline support for B43_PHY_AC wifi cards

[Rafał Miłecki]

Hi,

On 23 March 2018 at 10:47, Juri Lelli  wrote:
> I've got a Dell XPS 13 9343/0TM99H (BIOS A15 01/23/2018) mounting a
> BCM4352 802.11ac (rev 03) wireless card and so far I've been using it on
> Fedora with broadcom-wl package (which I believe installs Broadcom's STA
> driver?). It works good apart from occasional hiccups after suspend.
>
> I'd like to get rid of that dependency (you can understand that it's
> particularly annoying when testing mainline kernels), but I found out
> that support for my card is BROKEN in mainline [1]. Just to see what
> happens, I forcibly enabled it witnessing that it indeed crashes like
> below as Kconfig warns. :)
>
>  bcma: bus0: Found chip with id 0x4352, rev 0x03 and package 0x00
>  bcma: bus0: Core 0 found: ChipCommon (manuf 0x4BF, id 0x800, rev 0x2B, class 
> 0x0)
>  bcma: bus0: Core 1 found: IEEE 802.11 (manuf 0x4BF, id 0x812, rev 0x2A, 
> class 0x0)
>  bcma: bus0: Core 2 found: ARM CR4 (manuf 0x4BF, id 0x83E, rev 0x02, class 
> 0x0)
>  bcma: bus0: Core 3 found: PCIe Gen2 (manuf 0x4BF, id 0x83C, rev 0x01, class 
> 0x0)
>  bcma: bus0: Core 4 found: USB 2.0 Device (manuf 0x4BF, id 0x81A, rev 0x11, 
> class 0x0)
>  bcma: Unsupported SPROM revision: 11
>  bcma: bus0: Invalid SPROM read from the PCIe card, trying to use fallback 
> SPROM
>  bcma: bus0: Using fallback SPROM failed (err -2)
>  bcma: bus0: No SPROM available
>  bcma: bus0: Bus registered
>  b43-phy0: Broadcom 4352 WLAN found (core revision 42)
>  b43-phy0: Found PHY: Analog 12, Type 11 (AC), Revision 1
>  b43-phy0: Found Radio: Manuf 0x17F, ID 0x2069, Revision 4, Version 0
>  BUG: unable to handle kernel NULL pointer dereference at 

This isn't really useful without a full backtrace.


> So, question: is replacing my card the only way I can get rid of this
> downstream dependency? :(

It's definitely the cheapest way. Getting AC PHY into anything usable
(proper setup that will allow Tx & Rx anything) would probably take
weeks or months of development. I'm not even going to estimate cost of
adding support for 802.11n and 802.11ac features. I was the last
person actively working on b43, right now I spend my free time on
other hobby projects. Few people were planning to help but it seems it
never worked out for them.

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

[Rafał Miłecki]

On 15 March 2018 at 08:29, Rafał Miłecki <zaj...@gmail.com> wrote:
> From: Rafał Miłecki <ra...@milecki.pl>
>
> Testing brcmfmac with more recent firmwares resulted in AP interfaces
> not working in some specific setups. Debugging resulted in discovering
> support for IAPP in Broadcom's firmwares.
>
> Older firmwares were only generating 802.11f frames. Newer ones like:
> 1) 10.10 (TOB) (r663589)
> 2) 10.10.122.20 (r683106)
> for 4366b1 and 4366c0 respectively seem to also /respect/ 802.11f frames
> in the Tx path by performing a STA disassociation.
>
> This obsoleted standard and its implementation is something that:
> 1) Most people don't need / want to use
> 2) Can allow local DoS attacks
> 3) Breaks AP interfaces in some specific bridge setups
>
> To solve issues it can cause this commit modifies brcmfmac to drop IAPP
> packets. If affects:
> 1) Rx path: driver won't be sending these unwanted packets up.
> 2) Tx path: driver will reject packets that would trigger STA
>disassociation perfromed by a firmware (possible local DoS attack).
>
> It appears there are some Broadcom's clients/users who care about this
> feature despite the drawbacks. They can switch it on using a new module
> param.
>
> This change results in only two more comparisons (check for module param
> and check for Ethernet packet length) for 99.9% of packets. Its overhead
> should be very minimal.
>
> Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
> ---

I forgot to include the changelog, sorry.

V2: Use module param to don't /abuse/ Kconfig
Slightly optimize brcmf_skb_is_iapp
Move some description from Kconfig to the code
Update commit description: specify affected fws & mention impact

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

[Rafał Miłecki]

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

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

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

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

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

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

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

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

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index 70ef9835b647..5532ef39439f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
@@ -75,6 +75,10 @@ static int brcmf_roamoff;
 module_param_named(roamoff, brcmf_roamoff, int, S_IRUSR);
 MODULE_PARM_DESC(roamoff, "Do not use internal roaming engine");
 
+static int brcmf_iapp_enable;
+module_param_named(iapp, brcmf_iapp_enable, int, 0);
+MODULE_PARM_DESC(iapp, "Enable partial support for the obsoleted Inter-Access 
Point Protocol");
+
 #ifdef DEBUG
 /* always succeed brcmf_bus_started() */
 static int brcmf_ignore_probe_fail;
@@ -438,6 +442,7 @@ struct brcmf_mp_device *brcmf_get_module_param(struct 
device *dev,
settings->feature_disable = brcmf_feature_disable;
settings->fcmode = brcmf_fcmode;
settings->roamoff = !!brcmf_roamoff;
+   settings->iapp = !!brcmf_iapp_enable;
 #ifdef DEBUG
settings->ignore_probe_fail = !!brcmf_ignore_probe_fail;
 #endif
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h
index a62f8e70b320..ef914619e8e1 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h
@@ -58,6 +58,7 @@ struct brcmf_mp_device {
unsigned intfeature_disable;
int fcmode;
boolroamoff;
+   booliapp;
boolignore_probe_fail;
struct brcmfmac_pd_cc *country_codes;
union {
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 19048526b4af..ca97a8b4c59f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -230,6 +230,37 @@ static void brcmf_netdev_set_multicast_list(struct 
net_device *ndev)
schedule_work(>multicast_work);
 }
 
+/**
+ * brcmf_skb_is_iapp - checks if skb is an IAPP packet
+ *
+ * @skb: skb to check
+ */
+static bool brcmf_skb_is_iapp(struct sk_buff *skb)
+{
+   static const u8 iapp_l2_update_packet[6] __aligned(2) = {
+   0x00, 0x01, 0xaf, 0x81, 0x01, 0x00,
+   };
+   unsigned char *eth_data;
+#if !defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
+   const u16 *a, *b;
+#endif
+
+   if (skb->len - skb->mac_len != 6 ||
+   !is_multicast_ether_addr(eth_hdr(skb)->h_dest))
+   return false;
+
+   eth_data = skb_mac_header(skb) + ETH_HLEN;
+#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
+   return !(((*(const u32 *)eth_data) ^ (*(const u32 
*)iapp_l2_update_packet)) |
+((*(const u16 *)(eth_data + 4)) ^ (*(const u16 
*)(iapp_l2_update_packet + 4;
+#else
+   a = (const u16 *)eth_data;
+   b = (const u16 *)iapp_l2_update_packet;
+
+   return !((a[0] ^ b[0]) | (a[1] ^ b[1]) | (a[2] ^ b[2]));
+#endif
+}
+
 static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
   struct net_device *ndev)
 {
@@ -250,6 +281,23 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff 
*skb,
goto do

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

[Rafał Miłecki]

On 2018-03-14 16:08, Kalle Valo wrote:

Arend van Spriel  writes:


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

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

strange feature, there should be stronger reasons (size savings etc)
before adding a Kconfig option.

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


Hi Kalle,

Good to be wary about Kconfig option.


I think Linus doesn't like pointless Kconfig options, me neither for
that matter, so I try to make sure the justifications are really there
before adding anything new.


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


I'm no cpu profile expert but is really one (or two?) if checks of a
cached variable in the datapath really measurable? My guess is that 
it's

just noise in the results.

But I'm not going to argue about it, if you think it's still needed I'm
fine with that. Just mention in the commit log the justification the 
new

Kconfig option.


I think you should be right and that's also why I put
skb->len - skb->mac_len != 6
as the first check in that function. That simple (quick?) check should
reject 99.9% of packets.

I could move skb_mac_header() call a bit further which should optimize
this function even more and maybe then we could switch to the module
parameter?

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

[Rafał Miłecki]

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

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

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

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

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

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

To solve issues it can cause this commit modifies brcmfmac to drop 
IAPP

packets. If affects:
1) Rx path: driver won't be sending these unwanted packets up.
2) Tx path: driver will reject packets that would trigger STA
disassociation perfromed by a firmware (possible local DoS 
attack).


It appears there are some Broadcom's clients/users who care about 
this

feature despite the drawbacks. They can switch it on by a newly added
Kconfig option.


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


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

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
  drivers/net/wireless/broadcom/brcm80211/Kconfig| 20 +++
  .../wireless/broadcom/brcm80211/brcmfmac/core.c| 39 
++

  2 files changed, 59 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/Kconfig 
b/drivers/net/wireless/broadcom/brcm80211/Kconfig

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

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


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


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

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

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


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

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


[...]


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

goto done;
}

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


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

 * netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb,
 *   struct net_device *dev);
 *  Called when a packet needs to be transmitted.
 *	Returns NETDEV_TX_OK.  Can return NETDEV_TX_BUSY, but you should 
stop
 *	the queue before that can happen; it's for obsolete devices and 
weird

 *  corner cases, but the stack really does a non-trivial amount
 *  of useless work if you return NETDEV_TX_BUSY.
 *  Required; cannot be NULL.


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



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


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


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

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

[Rafał Miłecki]

On 2018-03-14 15:24, Kalle Valo wrote:

Rafał Miłecki <zaj...@gmail.com> writes:


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

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

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

To solve issues it can cause this commit modifies brcmfmac to drop 
IAPP

packets. If affects:
1) Rx path: driver won't be sending these unwanted packets up.
2) Tx path: driver will reject packets that would trigger STA
   disassociation perfromed by a firmware (possible local DoS attack).

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

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>


[...]


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

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


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

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


+/**
+ * brcmf_skb_is_iapp - checks if skb is an IAPP packet
+ *
+ * @skb: skb to check
+ */
+static bool brcmf_skb_is_iapp(struct sk_buff *skb)
+{
+   const u8 iapp_l2_update_packet[6] __aligned(2) = {
+   0x00, 0x01, 0xaf, 0x81, 0x01, 0x00,
+   };


static?


Sure



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


#ifndef?


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



+   const u16 *a = (const u16 *)eth_data;
+   const u16 *b = (const u16 *)iapp_l2_update_packet;
+#endif
+
+   if (skb->len - skb->mac_len != 6 ||
+   !is_multicast_ether_addr(eth_hdr(skb)->h_dest))
+   return false;
+
+#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)


#ifdef?

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

[Rafał Miłecki]

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

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

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

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

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

To solve issues it can cause this commit modifies brcmfmac to drop 
IAPP

packets. If affects:
1) Rx path: driver won't be sending these unwanted packets up.
2) Tx path: driver will reject packets that would trigger STA
disassociation perfromed by a firmware (possible local DoS 
attack).


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


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

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

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
  drivers/net/wireless/broadcom/brcm80211/Kconfig| 20 +++
  .../wireless/broadcom/brcm80211/brcmfmac/core.c| 39 
++

  2 files changed, 59 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/Kconfig 
b/drivers/net/wireless/broadcom/brcm80211/Kconfig

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

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


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


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

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

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


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

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


[...]


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

goto done;
}

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


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

 * netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb,
 *   struct net_device *dev);
 *  Called when a packet needs to be transmitted.
 *	Returns NETDEV_TX_OK.  Can return NETDEV_TX_BUSY, but you should 
stop
 *	the queue before that can happen; it's for obsolete devices and 
weird

 *  corner cases, but the stack really does a non-trivial amount
 *  of useless work if you return NETDEV_TX_BUSY.
 *  Required; cannot be NULL.


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



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


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

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

[Rafał Miłecki]

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

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

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

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

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

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

To solve issues it can cause this commit modifies brcmfmac to drop 
IAPP

packets. If affects:
1) Rx path: driver won't be sending these unwanted packets up.
2) Tx path: driver will reject packets that would trigger STA
disassociation perfromed by a firmware (possible local DoS 
attack).


It appears there are some Broadcom's clients/users who care about 
this

feature despite the drawbacks. They can switch it on by a newly added
Kconfig option.


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


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

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
  drivers/net/wireless/broadcom/brcm80211/Kconfig| 20 +++
  .../wireless/broadcom/brcm80211/brcmfmac/core.c| 39 
++

  2 files changed, 59 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/Kconfig 
b/drivers/net/wireless/broadcom/brcm80211/Kconfig

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

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


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


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

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

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


Please also take a look at my original patch
[PATCH] brcmfmac: detect & reject faked packet generated by a firmware
https://patchwork.kernel.org/patch/10191451/

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

[Rafał Miłecki]

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

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

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

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

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

diff --git a/drivers/net/wireless/broadcom/brcm80211/Kconfig 
b/drivers/net/wireless/broadcom/brcm80211/Kconfig
index 9d99eb42d917..876787ef991a 100644
--- a/drivers/net/wireless/broadcom/brcm80211/Kconfig
+++ b/drivers/net/wireless/broadcom/brcm80211/Kconfig
@@ -68,6 +68,26 @@ config BRCMFMAC_PCIE
  IEEE802.11ac embedded FullMAC WLAN driver. Say Y if you want to
  use the driver for an PCIE wireless card.
 
+config BRCMFMAC_IAPP
+   bool "Partial support for obsoleted Inter-Access Point Protocol"
+   depends on BRCMFMAC
+   ---help---
+ Most of Broadcom's firmwares can send 802.11f ADD frame every
+ time new STA connects to the AP interface. Some recent ones
+ can also disassociate STA when they receive such a frame.
+
+ It's important to understand this behavior can lead to a local
+ DoS security issue. Attacker may trigger disassociation of any
+ STA by sending a proper Ethernet frame to the wireless
+ interface.
+
+ Moreover this feature may break AP interfaces in some specific
+ setups. This applies e.g. to the bridge with hairpin mode
+ enabled and IFLA_BRPORT_MCAST_TO_UCAST set. IAPP packet
+ generated by a firmware will get passed back to the wireless
+ interface and cause immediate disassociation of just-connected
+ STA.
+
 config BRCM_TRACING
bool "Broadcom device tracing"
depends on BRCMSMAC || BRCMFMAC
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 19048526b4af..db6987015fb1 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -230,6 +230,34 @@ static void brcmf_netdev_set_multicast_list(struct 
net_device *ndev)
schedule_work(>multicast_work);
 }
 
+/**
+ * brcmf_skb_is_iapp - checks if skb is an IAPP packet
+ *
+ * @skb: skb to check
+ */
+static bool brcmf_skb_is_iapp(struct sk_buff *skb)
+{
+   const u8 iapp_l2_update_packet[6] __aligned(2) = {
+   0x00, 0x01, 0xaf, 0x81, 0x01, 0x00,
+   };
+   unsigned char *eth_data = skb_mac_header(skb) + ETH_HLEN;
+#if !defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
+   const u16 *a = (const u16 *)eth_data;
+   const u16 *b = (const u16 *)iapp_l2_update_packet;
+#endif
+
+   if (skb->len - skb->mac_len != 6 ||
+   !is_multicast_ether_addr(eth_hdr(skb)->h_dest))
+   return false;
+
+#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
+   return !(((*(const u32 *)eth_data) ^ (*(const u32 
*)iapp_l2_update_packet)) |
+((*(const u16 *)(eth_data + 4)) ^ (*(const u16 
*)(iapp_l2_update_packet + 4;
+#else
+   return !((a[0] ^ b[0]) | (a[1] ^ b[1]) | (a[2] ^ b[2]));
+#endif
+}
+
 static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
   struct net_device *ndev)
 {
@@ -250,6 +278,12 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff 
*skb,
goto done;
}
 
+   if (!IS_ENABLED(CONFIG_BRCMFMAC_IAPP) && brcmf_skb_is_iapp(skb)) {
+   dev_kfree_skb(skb);
+   ret = -EINVAL;
+   goto done;
+   }
+
/* Make sure there's enough writeable headroom */
if (skb_headroom(skb) < drvr->hdrlen || skb_header_cloned(skb)) {
head_delta = max_t(int, drvr->hdrlen - skb_headroom(skb), 0);
@@ -325,6 +359,11 @@ void brcmf_txflowblock_if(struct brcmf_if *ifp,
 
 void brcmf_netif_rx(struct brcmf_if *ifp, struct sk_buff *skb)
 {
+   if (!IS_ENABLED(CONFIG_BRCMFMAC_IAPP) && brcmf_skb_is_iapp(skb)) {
+   brcmu_pkt_buf_free_skb(skb);
+   return;
+   }

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

[Rafał Miłecki]

On 03/13/2018 12:01 AM, Stephen Hemminger wrote:

On Mon, 12 Mar 2018 23:42:48 +0100
Rafał Miłecki <zaj...@gmail.com> wrote:


2) Blame bridge + mcast-to-ucast + hairpin for 802.11f incompatibility

If we agree that 802.11f support in FullMAC firmware is acceptable, then
we have to make sure Linux's bridge doesn't break it by passing 802.11f
(broadcast) frames back to the source interface. That would require a
check like in below diff + proper code for handling such packets. I'm
afraid I'm not familiar with bridge code enough to complete that.

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index edae702..9e5d6ea 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -126,6 +126,27 @@ static void br_do_proxy_arp(struct sk_buff *skb, struct 
net_bridge *br,
}
  }

+static bool br_skb_is_iapp_add_packet(struct sk_buff *skb)
+{
+   const u8 iapp_add_packet[6] __aligned(2) = {
+   0x00, 0x01, 0xaf, 0x81, 0x01, 0x00,
+   };
+#if !defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
+   const u16 *a = (const u16 *)skb->data;
+   const u16 *b = (const u16 *)iapp_add_packet;
+#endif
+
+   if (skb->len != 6)
+   return false;
+
+#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
+   return !(((*(const u32 *)skb->data) ^ (*(const u32 *)iapp_add_packet)) |
+((*(const u16 *)(skb->data + 4)) ^ (*(const u16 
*)(iapp_add_packet + 4;
+#else
+   return !((a[0] ^ b[0]) | (a[1] ^ b[1]) | (a[2] ^ b[2]));
+#endif
+}
+
  /* note: already called with rcu_read_lock */
  int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff 
*skb)
  {
@@ -155,6 +176,8 @@ int br_handle_frame_finish(struct net *net, struct sock 
*sk, struct sk_buff *skb
if (is_multicast_ether_addr(dest)) {
/* by definition the broadcast is also a multicast address */
if (is_broadcast_ether_addr(dest)) {
+   if (br_skb_is_iapp_add_packet(skb))
+   pr_warn("This packet should not be passed back to 
the source interface!\n");
pkt_type = BR_PKT_BROADCAST;
local_rcv = true;
} else {



Don't like bridge doing special case code for magic received values directly in 
input path.
Really needs to be generic which is why I suggested ebtables.


We need in-bridge solution only if we decide to support FullMAC
firmwares with 802.11f implementation.

In that case is this possible to use ebtables as a workaround at all?
Can I really use ebtables to set switch to don't pass 802.11f ADD frames
back to the original interface?

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

[Rafał Miłecki]

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

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


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

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

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


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

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

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


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

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


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


I kept thinking about this for a bit more. We probably have to start
with deciding what to blame for this problem. I see two options I'll
describe below.

Please share your opinions on this.


1) Blame brcmfmac fw for implementing 802.11f

In that case we should add workaround to brcmfmac driver to drop/ignore
802.11f packets. A slightly improved version of my
[PATCH] brcmfmac: detect & reject faked packet generated by a firmware
https://patchwork.kernel.org/patch/10191451/
should work.


2) Blame bridge + mcast-to-ucast + hairpin for 802.11f incompatibility

If we agree that 802.11f support in FullMAC firmware is acceptable, then
we have to make sure Linux's bridge doesn't break it by passing 802.11f
(broadcast) frames back to the source interface. That would require a
check like in below diff + proper code for handling such packets. I'm
afraid I'm not familiar with bridge code enough to complete that.

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index edae702..9e5d6ea 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -126,6 +126,27 @@ static void br_do_proxy_arp(struct sk_buff *skb, struct 
net_bridge *br,
}
 }

+static bool br_skb_is_iapp_add_packet(struct sk_buff *skb)
+{
+   const u8 iapp_add_packet[6] __aligned(2) = {
+   0x00, 0x01, 0xaf, 0x81, 0x01, 0x00,
+   };
+#if !defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
+   const u16 *a = (const u16 *)skb->data;
+   const u16 *b = (const u16 *)iapp_add_packet;
+#endif
+
+   if (skb->len != 6)
+   return false;
+
+#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
+   return !(((*(const u32 *)skb->data) ^ (*(const u32 *)iapp_add_packet)) |
+((*(const u16 *)(skb->data + 4)) ^ (*(const u16 
*)(iapp_add_packet + 4;
+#else
+   return !((a[0] ^ b[0]) | (a[1] ^ b[1]) | (a[2] ^ b[2]));
+#endif
+}
+
 /* note: already called with rcu_read_lock */
 int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff 
*skb)
 {
@@ -155,6 +176,8 @@ int br_handle_frame_finish(struct net *net, struct sock 
*sk, struct sk_buff *skb
if (is_multicast_ether_addr(dest)) {
/* by definition the broadcast is also a multicast address */
if (is_broadcast_ether_addr(dest)) {
+   if (br_skb_is_iapp_add_packet(skb))
+   pr_warn("This packet should not be passed back to 
the source interface!\n");
pkt_type = BR_PKT_BROADCAST;
local_rcv = true;
} else {

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

[Rafał Miłecki]

On 12 March 2018 at 12:48, Linus Lüssing <linus.luess...@c0d3.blue> wrote:
> On Mon, Mar 12, 2018 at 12:08:56PM +0100, Linus Lüssing wrote:
>> On Tue, Feb 27, 2018 at 11:08:20AM +0100, Rafał Miłecki wrote:
>> > I've problem when using OpenWrt/LEDE on a home router with Broadcom's
>> > FullMAC WiFi chipset.
>>
>> Hi Rafał,
>>
>> Thanks for reporting this issue!
>>
>> > Can you see any solution for this problem? Is that an option to stop
>> > multicast-to-unicast from touching 802.11f packets? Some other ideas?
>> > Obviously I can't modify Broadcom's firmware and drop that obsoleted
>> > standard.
>>
>> Just to avoid some potential confusion: This is more an issue of
>> hairpinning than it is an issue of multicast-to-unicast per se,
>> right?
>>
>> That is, if you set this to 0 manually:
>> /sys/class/net//brport/multicast_to_unicast
>> Then the issue still occurs, right?
>
> Btw., if in OpenWRT/LEDE you set 'option multicast_to_unicast 0'
> in /etc/config/network for the bridge (and not bridge port)
> then netifd should refrain from setting the bridge hairpinning and
> AP isolation on wireless devices, too, if I remember correctly.
>
> Can you confirm that the issue disappears for you then?

Yes, absolutely. This reverts OpenWrt/LEDE to the old setup (no
mcast-to-ucast + hairpin) and it works. I was hoping we can make
mcast-to-ucast + hairpin work with Broadcom's 802.11f however instead
of moving OpenWrt/LEDE back to the old setup.

-- 
Rafał

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

[Rafał Miłecki]

On 12 March 2018 at 12:08, Linus Lüssing <linus.luess...@c0d3.blue> wrote:
> On Tue, Feb 27, 2018 at 11:08:20AM +0100, Rafał Miłecki wrote:
>> I've problem when using OpenWrt/LEDE on a home router with Broadcom's
>> FullMAC WiFi chipset.
>
> Hi Rafał,
>
> Thanks for reporting this issue!
>
>> Can you see any solution for this problem? Is that an option to stop
>> multicast-to-unicast from touching 802.11f packets? Some other ideas?
>> Obviously I can't modify Broadcom's firmware and drop that obsoleted
>> standard.
>
> Just to avoid some potential confusion: This is more an issue of
> hairpinning than it is an issue of multicast-to-unicast per se,
> right?
>
> That is, if you set this to 0 manually:
> /sys/class/net//brport/multicast_to_unicast
> Then the issue still occurs, right?

I can't really tell, I didn't go into details on bridge +
mcast-to-unicast + hairpin.

By default OpenWrt/LEDE sets
/sys/class/net//brport/multicast_to_unicast
to 1. Changing it to 0 (with a simple echo) doesn't /fix/ the problem.

-- 
Rafał

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

[Rafał Miłecki]

On 28 February 2018 at 12:31, Arend van Spriel
<arend.vanspr...@broadcom.com> wrote:
> On 2/27/2018 11:14 AM, Rafał Miłecki wrote:
>>
>> Sending with a fixed linux-wireless ML address. Please kindly send your
>> replies using linux-wireless@
>>
>> On 02/27/2018 11:08 AM, Rafał Miłecki wrote:
>>>
>>> I've problem when using OpenWrt/LEDE on a home router with Broadcom's
>>> FullMAC WiFi chipset.
>>>
>>>
>>> First of all OpenWrt/LEDE uses bridge interface for LAN network with:
>>> 1) IFLA_BRPORT_MCAST_TO_UCAST
>>> 2) Clients isolation in hostapd
>>> 3) Hairpin mode enabled
>>>
>>> For more details please see Linus's patch description:
>>> https://patchwork.kernel.org/patch/9530669/
>>> and maybe hairpin mode patch:
>>> https://lwn.net/Articles/347344/
>>>
>>> Short version: in that setup packets received from a bridged wireless
>>> interface can be handled back to it for transmission.
>>>
>>>
>>> Now, Broadcom's firmware for their FullMAC chipsets in AP mode
>>> supports an obsoleted 802.11f AKA IAPP standard. It's a roaming
>>> standard that was replaced by 802.11r.
>>>
>>> Whenever a new station associates, firmware generates a packet like:
>>> ff ff ff ff  ff ff ec 10  7b 5f ?? ??  00 06 00 01  af 81 01 00
>>> (just masked 2 bytes of my MAC)
>>>
>>> For mode details you can see discussion in my brcmfmac patch thread:
>>> https://patchwork.kernel.org/patch/10191451/
>>>
>>>
>>> The problem is that bridge (in setup as above) handles such a packet
>>> back to the device.
>
>
> From reading the referenced links I understand the hairpin mode is causing
> the packet to be sent back to the device, and the hairpin mode is required
> for MCAST_TO_UCAST, right?
>
>>> That makes Broadcom's FullMAC firmware believe that a given station
>>> just connected to another AP in a network (which doesn't even exist).
>>> As a result firmware immediately disassociates that station. It's
>>> simply impossible to connect to the router. Every association is
>>> followed by immediate disassociation.
>>>
>>>
>>> Can you see any solution for this problem? Is that an option to stop
>>> multicast-to-unicast from touching 802.11f packets? Some other ideas?
>>> Obviously I can't modify Broadcom's firmware and drop that obsoleted
>>> standard.
>
>
> As far as I can tell you are correct that the 802.11f amendment was never
> adopted into the 802.11 standard. I will ask internally if we still have a
> reason for carrying it in our firmware.

Thanks! Having at least a way to disable it would be nice.

That said I think we still should look for a solution for existing
firmwares. I guess it may takes months to years to never to release
new firmwares for all supported chipsets.

-- 
Rafał

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

[Rafał Miłecki]

On 27 February 2018 at 18:05, Stephen Hemminger
<step...@networkplumber.org> wrote:
> On Tue, 27 Feb 2018 11:08:20 +0100
> Rafał Miłecki <zaj...@gmail.com> wrote:
>
>> I've problem when using OpenWrt/LEDE on a home router with Broadcom's
>> FullMAC WiFi chipset.
>>
>>
>> First of all OpenWrt/LEDE uses bridge interface for LAN network with:
>> 1) IFLA_BRPORT_MCAST_TO_UCAST
>> 2) Clients isolation in hostapd
>> 3) Hairpin mode enabled
>>
>> For more details please see Linus's patch description:
>> https://patchwork.kernel.org/patch/9530669/
>> and maybe hairpin mode patch:
>> https://lwn.net/Articles/347344/
>>
>> Short version: in that setup packets received from a bridged wireless
>> interface can be handled back to it for transmission.
>>
>>
>> Now, Broadcom's firmware for their FullMAC chipsets in AP mode
>> supports an obsoleted 802.11f AKA IAPP standard. It's a roaming
>> standard that was replaced by 802.11r.
>>
>> Whenever a new station associates, firmware generates a packet like:
>> ff ff ff ff  ff ff ec 10  7b 5f ?? ??  00 06 00 01  af 81 01 00
>> (just masked 2 bytes of my MAC)
>>
>> For mode details you can see discussion in my brcmfmac patch thread:
>> https://patchwork.kernel.org/patch/10191451/
>>
>>
>> The problem is that bridge (in setup as above) handles such a packet
>> back to the device.
>>
>> That makes Broadcom's FullMAC firmware believe that a given station
>> just connected to another AP in a network (which doesn't even exist).
>> As a result firmware immediately disassociates that station. It's
>> simply impossible to connect to the router. Every association is
>> followed by immediate disassociation.
>>
>>
>> Can you see any solution for this problem? Is that an option to stop
>> multicast-to-unicast from touching 802.11f packets? Some other ideas?
>> Obviously I can't modify Broadcom's firmware and drop that obsoleted
>> standard.
>>
>
> ebtables is your friend in dealing with weird and broken devices.

It may be weird, not sure if actually broken. Anyway I'd like to have
some generic solution instead of telling every user to use ebtables to
workaround the problem.

-- 
Rafał

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

[Rafał Miłecki]

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

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

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


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

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

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


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

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

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


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

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


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

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

[Rafał Miłecki]

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


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

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

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


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

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

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


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

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


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

-- 
Rafał

[PATCH 1/2] net: phy: broadcom: support new device flag for setting master mode

[Rafał Miłecki]

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

Some of Broadcom's PHYs run by default in slave mode with Automatic
Slave/Master configuration disabled. It stops them from working properly
with some devices.

So far it has been verified for BCM54210E and BCM50212E which don't
work well with Intel's I217-LM and I218-LM:
http://ark.intel.com/products/60019/Intel-Ethernet-Connection-I217-LM
http://ark.intel.com/products/71307/Intel-Ethernet-Connection-I218-LM
I was told there is massive ping loss.

This commit adds support for a new flag which can be set by an ethernet
driver to fixup PHY setup.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
 drivers/net/phy/broadcom.c | 6 ++
 include/linux/brcmphy.h| 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 1e9ad30a35c8..d7ed69deabfb 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -43,6 +43,12 @@ static int bcm54210e_config_init(struct phy_device *phydev)
val &= ~BCM54810_SHD_CLK_CTL_GTXCLK_EN;
bcm_phy_write_shadow(phydev, BCM54810_SHD_CLK_CTL, val);
 
+   if (phydev->dev_flags & PHY_BRCM_EN_MASTER_MODE) {
+   val = phy_read(phydev, MII_CTRL1000);
+   val |= CTL1000_AS_MASTER | CTL1000_ENABLE_MASTER;
+   phy_write(phydev, MII_CTRL1000, val);
+   }
+
return 0;
 }
 
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index abcda9b458ab..9ac9e3e3d1e5 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -63,6 +63,7 @@
 #define PHY_BRCM_EXT_IBND_TX_ENABLE0x2000
 #define PHY_BRCM_CLEAR_RGMII_MODE  0x4000
 #define PHY_BRCM_DIS_TXCRXC_NOENRGY0x8000
+#define PHY_BRCM_EN_MASTER_MODE0x0001
 
 /* Broadcom BCM7xxx specific workarounds */
 #define PHY_BRCM_7XXX_REV(x)   (((x) >> 8) & 0xff)
-- 
2.11.0

[PATCH 2/2] net: bgmac: enable master mode for BCM54210E and B50212E PHYs

[Rafał Miłecki]

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

There are 4 very similar PHYs:
0x600d84a1: BCM54210E (rev B0)
0x600d84a2: BCM54210E (rev B1)
0x600d84a5: B50212E (rev B0)
0x600d84a6: B50212E (rev B1)
that need setting master mode manually. It's because they run in slave
mode by default with Automatic Slave/Master configuration disabled which
can lead to unreliable connection with massive ping loss.

So far it was reported for a board with BCM47189 SoC and B50212E B1 PHY
connected to the bgmac supported ethernet device. Telling PHY driver to
setup PHY properly solves this issue.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
 drivers/net/ethernet/broadcom/bgmac-bcma.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma.c 
b/drivers/net/ethernet/broadcom/bgmac-bcma.c
index 6322594ab260..6fe074c1588b 100644
--- a/drivers/net/ethernet/broadcom/bgmac-bcma.c
+++ b/drivers/net/ethernet/broadcom/bgmac-bcma.c
@@ -184,13 +184,19 @@ static int bgmac_probe(struct bcma_device *core)
 
if (!bgmac_is_bcm4707_family(core) &&
!(ci->id == BCMA_CHIP_ID_BCM53573 && core->core_unit == 1)) {
+   struct phy_device *phydev;
+
mii_bus = bcma_mdio_mii_register(bgmac);
if (IS_ERR(mii_bus)) {
err = PTR_ERR(mii_bus);
goto err;
}
-
bgmac->mii_bus = mii_bus;
+
+   phydev = mdiobus_get_phy(bgmac->mii_bus, bgmac->phyaddr);
+   if (ci->id == BCMA_CHIP_ID_BCM53573 && phydev &&
+   (phydev->drv->phy_id & phydev->drv->phy_id_mask) == 
PHY_ID_BCM54210E)
+   phydev->dev_flags |= PHY_BRCM_EN_MASTER_MODE;
}
 
if (core->bus->hosttype == BCMA_HOSTTYPE_PCI) {
-- 
2.11.0

[PATCH 0/2] net: support bgmac with B50212E B1 PHY

[Rafał Miłecki]

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

I got a report that a board with BCM47189 SoC and B50212E B1 PHY doesn't
work well some devices as there is massive ping loss. After analyzing
PHY state it has appeared that is runs in slave mode and doesn't auto
switch to master properly when needed.

This patchset fixes this by:
1) Adding new flag support to the PHY driver for setting master mode
2) Modifying bgmac to request master mode for reported hardware

Rafał Miłecki (2):
  net: phy: broadcom: support new device flag for setting master mode
  net: bgmac: enable master mode for BCM54210E and B50212E PHYs

 drivers/net/ethernet/broadcom/bgmac-bcma.c | 8 +++-
 drivers/net/phy/broadcom.c | 6 ++
 include/linux/brcmphy.h| 1 +
 3 files changed, 14 insertions(+), 1 deletion(-)

-- 
2.11.0

[PATCH] net: phy: broadcom: force master mode for BCM54210E and B50212E

[Rafał Miłecki]

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

First of all let me explain that the code we use for BCM54210E is also
executed for the B50212E. They are very similar so it probably makes
sense but it may be worth noting. The IDs are:
0x600d84a1: BCM54210E (rev B0)
0x600d84a2: BCM54210E (rev B1)
0x600d84a5: B50212E (rev B0)
0x600d84a6: B50212E (rev B1)

I got a report that a board with BCM47189 SoC and B50212E B1 PHY doesn't
work well with Intel's I217-LM and I218-LM:
http://ark.intel.com/products/60019/Intel-Ethernet-Connection-I217-LM
http://ark.intel.com/products/71307/Intel-Ethernet-Connection-I218-LM
I was told there are massive ping loss.

A solution to this problem is setting master mode in the 1000BASE-T
register. I noticed a similar fix is present in the tg3 driver. One
thing I'm not sure if this is needed for BCM54210E. It shouldn't hurt
however since both are so similar.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
David: I'm not 100% sure if this is the best fix, so let's give others
(Florian?) a moment to look at it / review it, please.
---
 drivers/net/phy/broadcom.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 1e9ad30a35c8..2569db0923b0 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -43,6 +43,10 @@ static int bcm54210e_config_init(struct phy_device *phydev)
val &= ~BCM54810_SHD_CLK_CTL_GTXCLK_EN;
bcm_phy_write_shadow(phydev, BCM54810_SHD_CLK_CTL, val);
 
+   val = phy_read(phydev, MII_CTRL1000);
+   val |= CTL1000_AS_MASTER | CTL1000_ENABLE_MASTER;
+   phy_write(phydev, MII_CTRL1000, val);
+
return 0;
 }
 
-- 
2.11.0

[PATCH V5 1/2] firmware: add more flexible request_firmware_async function

[Rafał Miłecki]

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

So far we got only one function for loading firmware asynchronously:
request_firmware_nowait. It didn't allow much customization of firmware
loading process - there is only one bool uevent argument. Moreover this
bool also controls user helper in an unclear way.

Some drivers need more flexible helper providing more options. This
includes control over uevent or warning for the missing firmware. Of
course this list may grow up in the future.

To handle this easily this patch adds a generic request_firmware_async
function. It takes struct with options as an argument which will allow
extending it in the future without massive changes.

This is a really cheap change (no new independent API) which works
nicely with the existing code. The old request_firmware_nowait is kept
as a simple helper calling new helper.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
V3: Don't expose all FW_OPT_* flags.
As Luis noted we want a struct so add struct firmware_opts for real
flexibility.
Thank you Luis for your review!
V5: Rebase, update commit message, resend after drvdata discussion
---
 drivers/base/firmware_class.c | 43 ++-
 include/linux/firmware.h  | 15 ++-
 2 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index b9f907eedbf7..cde85b05e4cb 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1375,7 +1375,7 @@ static void request_firmware_work_func(struct work_struct 
*work)
_request_firmware(, fw_work->name, fw_work->device, NULL, 0,
  fw_work->opt_flags);
fw_work->cont(fw, fw_work->context);
-   put_device(fw_work->device); /* taken in request_firmware_nowait() */
+   put_device(fw_work->device); /* taken in __request_firmware_nowait() */
 
module_put(fw_work->module);
kfree_const(fw_work->name);
@@ -1383,10 +1383,9 @@ static void request_firmware_work_func(struct 
work_struct *work)
 }
 
 /**
- * request_firmware_nowait - asynchronous version of request_firmware
+ * __request_firmware_nowait - asynchronous version of request_firmware
  * @module: module requesting the firmware
- * @uevent: sends uevent to copy the firmware image if this flag
- * is non-zero else the firmware copy must be done manually.
+ * @opt_flags: flags that control firmware loading process, see FW_OPT_*
  * @name: name of firmware file
  * @device: device for which firmware is being loaded
  * @gfp: allocation flags
@@ -1405,9 +1404,9 @@ static void request_firmware_work_func(struct work_struct 
*work)
  *
  * - can't sleep at all if @gfp is GFP_ATOMIC.
  **/
-int
-request_firmware_nowait(
-   struct module *module, bool uevent,
+static int
+__request_firmware_nowait(
+   struct module *module, unsigned int opt_flags,
const char *name, struct device *device, gfp_t gfp, void *context,
void (*cont)(const struct firmware *fw, void *context))
 {
@@ -1426,8 +1425,7 @@ request_firmware_nowait(
fw_work->device = device;
fw_work->context = context;
fw_work->cont = cont;
-   fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
-   (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
+   fw_work->opt_flags = FW_OPT_NOWAIT | opt_flags;
 
if (!try_module_get(module)) {
kfree_const(fw_work->name);
@@ -1440,8 +1438,35 @@ request_firmware_nowait(
schedule_work(_work->work);
return 0;
 }
+
+int request_firmware_nowait(struct module *module, bool uevent,
+   const char *name, struct device *device, gfp_t gfp,
+   void *context,
+   void (*cont)(const struct firmware *fw, void 
*context))
+{
+   unsigned int opt_flags = FW_OPT_FALLBACK |
+   (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
+
+   return __request_firmware_nowait(module, opt_flags, name, device, gfp,
+context, cont);
+}
 EXPORT_SYMBOL(request_firmware_nowait);
 
+int __request_firmware_async(struct module *module, const char *name,
+struct firmware_opts *fw_opts, struct device *dev,
+void *context,
+void (*cont)(const struct firmware *fw, void 
*context))
+{
+   unsigned int opt_flags = FW_OPT_UEVENT;
+
+   if (fw_opts->optional)
+   opt_flags |= FW_OPT_NO_WARN;
+
+   return __request_firmware_nowait(module, opt_flags, name, dev,
+GFP_KERNEL, context, cont);
+}
+EXPORT_SYMBOL(__request_firmware_async);
+
 #ifdef CONFIG_PM_SLEEP
 static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
 
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index b1f9f0ccb8ac..a32b6e6

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

[Rafał Miłecki]

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

Failing to load NVRAM *file* isn't critical if we manage to get platform
NVRAM in the fallback path. It means warnings like:
[   10.801506] brcmfmac :01:00.0: Direct firmware load for 
brcm/brcmfmac43602-pcie.txt failed with error -2
are unnecessary & disturbing for people with *platform* NVRAM as they
are not expected to have NVRAM file. This is a very common case for
Broadcom home routers.

Instead of printing warning immediately within the firmware subsystem
let's try our fallback code first. If that fails as well, then it's a
right moment to print an error.

This should reduce amount of false reports from users seeing this
warning while having wireless working perfectly fine with the platform
NVRAM.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
V2: Update commit message as it wasn't clear enough (thanks Andy) & add extra
messages to the firmware.c.
V3: Set FW_OPT_UEVENT to don't change behavior
V4: Switch to the new request_firmware_async syntax
V5: Rebase, update commit message, resend after drvdata discussion
---
 .../wireless/broadcom/brcm80211/brcmfmac/firmware.c| 18 +-
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
index d231042f19d6..524442b3870f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
@@ -462,8 +462,14 @@ static void brcmf_fw_request_nvram_done(const struct 
firmware *fw, void *ctx)
raw_nvram = false;
} else {
data = bcm47xx_nvram_get_contents(_len);
-   if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
-   goto fail;
+   if (!data) {
+   brcmf_dbg(TRACE, "Failed to get platform NVRAM\n");
+   if (!(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) {
+   brcmf_err("Loading NVRAM from %s and using 
platform one both failed\n",
+ fwctx->nvram_name);
+   goto fail;
+   }
+   }
raw_nvram = true;
}
 
@@ -491,6 +497,9 @@ static void brcmf_fw_request_nvram_done(const struct 
firmware *fw, void *ctx)
 static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
 {
struct brcmf_fw *fwctx = ctx;
+   struct firmware_opts fw_opts = {
+   .optional = true,
+   };
int ret = 0;
 
brcmf_dbg(TRACE, "enter: dev=%s\n", dev_name(fwctx->dev));
@@ -503,9 +512,8 @@ static void brcmf_fw_request_code_done(const struct 
firmware *fw, void *ctx)
goto done;
 
fwctx->code = fw;
-   ret = request_firmware_nowait(THIS_MODULE, true, fwctx->nvram_name,
- fwctx->dev, GFP_KERNEL, fwctx,
- brcmf_fw_request_nvram_done);
+   ret = request_firmware_async(fwctx->nvram_name, _opts, fwctx->dev,
+fwctx, brcmf_fw_request_nvram_done);
 
/* pass NULL to nvram callback for bcm47xx fallback */
if (ret)
-- 
2.11.0

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

[Rafał Miłecki]

On 11 July 2017 at 17:01, Russell Joyce  wrote:
> Thanks for your comments.
>
>> What I think Rafał is saying is that it would be better to have this
>> code in cfg80211 so other drivers including mac80211 could use it.
>
>
> While I agree that moving all wireless LED triggers to cfg80211 would be an
> ideal situation, it seems a bit out of scope for what I was trying to achieve.
> This would probably also require removing the mac80211 LED triggers (and any
> other similar triggers that might be created by specific wireless drivers not
> using mac80211), in order to consolidate them in one place.
>
> Besides this, I'm not sure where exactly in cfg80211 this functionality would
> go (I assume it was originally put in mac80211 instead for a reason?),
> although I'm certainly no expert in this area of the kernel.

I don't expect you to rewrite all mac80211 drivers at this point. Just
focus on the generic cfg80211 helper and use it in brcmfmac. Other
cfg80211 drivers and mac80211 may follow in the future.

I'm not sure what's the best place in cfg80211 for this. Try
something, or try to get some comments from cfg80211 guys.


>> Indeed. However, the LED subsystem could/should(?) take care of mapping
>> "rx" and "tx" triggers to the same LED.
>
> In terms of the LED triggers, the only alternative I can see is to create a
> single complex trigger that exposes "rx" and "tx" parameters that can be
> individually enabled or disabled. This would reduce the number of triggers 
> from
> three to one, but also makes things slightly more awkward for the user, and
> deviates from the convention set by mac80211.

Ideally we should have "rx" and "tx". LED subsystem should allow
assigning *both* (at the same time) to the LED.

I'll try to discuss with with LED guys this week.

Sorry, I was busy last week.

-- 
Rafał

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

[Rafał Miłecki]

On 7 July 2017 at 16:09, Russell Joyce  wrote:
> Add three basic LED triggers to brcmfmac, based on those in mac80211: one
> for transmit, one for receive, and one for combined transmit/receive.
>
> Signed-off-by: Russell Joyce 

1) I think most of it should be some cfg80211 shareable code.
2) This "rxtx" while surely present in other places sounds like a
workaround for LED subsystem limitation. Maybe it's time to finally
rework LED triggers.

Re: [PATCH 5/7] ath9k: of: Use the clk API to get the reference clock rate

[Rafał Miłecki]

On 03/13/2017 10:05 PM, Alban wrote:

@@ -573,6 +575,12 @@ static int ath9k_of_init(struct ath_softc *sc)

ath_dbg(common, CONFIG, "parsing configuration from OF node\n");

+   clk = clk_get(sc->dev, "ref");
+   if (!IS_ERR(clk)) {
+   ah->is_clk_25mhz = (clk_get_rate(clk) == 2500);


One trivial thing: you don't need these extra braces.



+   clk_put(clk);
+   }

Re: [PATCH 3/7] ath9k: Add support for reading the EEPROM data using the nvmem API

[Rafał Miłecki]

On 03/13/2017 10:05 PM, Alban wrote:

@@ -654,6 +656,25 @@ static int ath9k_init_softc(u16 devid, struct ath_softc 
*sc,
if (ret)
return ret;

+   /* If the EEPROM hasn't been retrieved via firmware request
+* use the nvmem API insted.
+*/
+   if (!ah->eeprom_blob) {
+   struct nvmem_cell *eeprom_cell;
+
+   eeprom_cell = nvmem_cell_get(ah->dev, "eeprom");
+   if (!IS_ERR(eeprom_cell)) {
+   ah->eeprom_data = nvmem_cell_read(
+   eeprom_cell, >eeprom_size);
+   nvmem_cell_put(eeprom_cell);
+
+   if (IS_ERR(ah->eeprom_data)) {
+   dev_err(ah->dev, "failed to read eeprom");


One trivial thing: missing line break.



+   return PTR_ERR(ah->eeprom_data);
+   }
+   }
+   }
+
if (ath9k_led_active_high != -1)
ah->config.led_active_high = ath9k_led_active_high == 1;

Re: [PATCH 2/3] net: ethernet: bgmac: unify code of the same family

[Rafał Miłecki]

On 2017-02-03 22:39, Jon Mason wrote:

BCM471X and BCM535X are of the same family (from what I can derive from
internal documents).  Group them into the case statement together, 
which

results in more code reuse.

Also, use existing helper variables to make the code a little more
readable too.

Signed-off-by: Jon Mason 


I'd like to review it / test it on few devices. Please give me weekend 
for

that.

Re: [PATCH v2 2/2] net: ethernet: bgmac: mac address change bug

[Rafał Miłecki]

On 02/03/2017 10:08 PM, Jon Mason wrote:

From: Hari Vyas 

ndo_set_mac_address() passes struct sockaddr * as 2nd parameter to
bgmac_set_mac_address() but code assumed u8 *.  This caused two bytes
chopping and the wrong mac address was configured.

Signed-off-by: Hari Vyas 
Signed-off-by: Jon Mason 
Fixes: 4e209001b86 ("bgmac: write mac address to hardware in 
ndo_set_mac_address")


I think you were going to Cc stable?

Re: [PATCH v2 1/2] net: ethernet: bgmac: init sequence bug

[Rafał Miłecki]

On 02/03/2017 10:08 PM, Jon Mason wrote:

@@ -61,15 +60,20 @@ static bool platform_bgmac_clk_enabled(struct bgmac *bgmac)

 static void platform_bgmac_clk_enable(struct bgmac *bgmac, u32 flags)
 {
-   bgmac_idm_write(bgmac, BCMA_IOCTL,
-   (BCMA_IOCTL_CLK | BCMA_IOCTL_FGC | flags));
+   u32 val;
+
+   val = bgmac_idm_read(bgmac, BCMA_IOCTL);
+   /* Some bits of BCMA_IOCTL set by HW/ATF and should not change */
+   val |= flags & ~(BGMAC_AWCACHE | BGMAC_ARCACHE | BGMAC_AWUSER |
+BGMAC_ARUSER);
+   val |= BGMAC_CLK_EN;
bgmac_idm_read(bgmac, BCMA_IOCTL);


This read was previously following write op most likely to flush it or
something. I don't think it makes any sense to read after read.

Re: [PATCH 1/2] net: ethernet: bgmac: init sequence bug

[Rafał Miłecki]

On 2017-02-02 01:31, Zac Schroff wrote:

How about BCMA_IOCTL_PRESERVE_ACROSS_INIT?


I think wireless drivers may still set some these bits during init.

I've a simpler idea: make it bgmac specific. Call it sth like
BGMAC_BCMA_IOCTL_PRESERVE
BGMAC_BCMA_IOCTL_RESERVED
BGMAC_BCMA_IOCTL_DONT_TOUCH

Re: [PATCH 2/2] net: ethernet: bgmac: mac address change bug

[Rafał Miłecki]

On 02/01/2017 11:39 PM, Jon Mason wrote:

From: Hari Vyas 

ndo_set_mac_address() passes struct sockaddr * as 2nd parameter to
bgmac_set_mac_address() but code assumed u8 *.  This caused two bytes
chopping and the wrong mac address was configured.

Signed-off-by: Hari Vyas 
Signed-off-by: Jon Mason 
Fixes: 4e209001b86 ("bgmac: write mac address to hardware in 
ndo_set_mac_address")


Sounds OK, would it make sense to Cc stable?

Re: [PATCH v2 3/4] phy: Add USB3 PHY support for Broadcom NSP SoC

[Rafał Miłecki]

[Resending with fixed/complete Cc-s]

On Tue, 17 Jan 2017 11:14:29 -0500, Yendapally Reddy Dhananjaya Reddy 
 wrote:> This patch adds support for Broadcom 
NSP USB3 PHY
>
> Signed-off-by: Yendapally Reddy Dhananjaya Reddy 


Seriously?! I really dislike what you did there.

NACK.

You are aware this block is common for both: Northstar and Northstar Plus and
we already have phy-bcm-ns-usb3.c! In fact Jon told me to rewrite my initial
driver to make is possible to reuse it on NSP and I did that!

This is old comment from Jon:

In 30 March 2016 at 23:31, Jon Mason  wrote:
> On Mon, Mar 28, 2016 at 9:46 PM, Florian Fainelli 
> wrote:
>>
>> CC: bcm-kernel-feedback-list, Jon
>
>
> This is a common IP block with NSP.  I believe with some minor changes it
> can support both.  Please allow me 1-2 days to look at these in more detail
> and see if I can get these patches working on NSP.

Please start using existing code instead of inventing everything from the
scratch internally at Broadcom. You did the same thing with (Q)SPI driver.


This driver duplicates phy-bcm-ns-usb3.c and should have not been accepted. I
strongly suggest *reverting* it and adjusting existing driver if needed.

Re: [PATCH 1/2] net: ethernet: bgmac: init sequence bug

[Rafał Miłecki]

On 02/01/2017 11:39 PM, Jon Mason wrote:

From: Zac Schroff 

Fix a bug in the 'bgmac' driver init sequence that blind writes for init
sequence where it should preserve most bits other than the ones it is
deliberately manipulating.

Signed-off-by: Zac Schroff 
Signed-off-by: Jon Mason 
Fixes: f6a95a24957 ("net: ethernet: bgmac: Add platform device support")
---
 drivers/net/ethernet/broadcom/bgmac-platform.c | 10 +++---
 include/linux/bcma/bcma_regs.h |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c 
b/drivers/net/ethernet/broadcom/bgmac-platform.c
index 6f736c1..9bbe05c 100644
--- a/drivers/net/ethernet/broadcom/bgmac-platform.c
+++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
@@ -61,15 +61,19 @@ static bool platform_bgmac_clk_enabled(struct bgmac *bgmac)

 static void platform_bgmac_clk_enable(struct bgmac *bgmac, u32 flags)
 {
-   bgmac_idm_write(bgmac, BCMA_IOCTL,
-   (BCMA_IOCTL_CLK | BCMA_IOCTL_FGC | flags));
+   u32 regval;
+
+   /* Some bits of BCMA_IOCTL set by HW/ATF & should not change */
+   regval = bgmac_idm_read(bgmac, BCMA_IOCTL) & BCMA_IOCTL_DO_NOT_MODIFY;
+   regval |= ((flags & (~BCMA_IOCTL_DO_NOT_MODIFY)) | BCMA_IOCTL_CLK);


You don't need these braces around whole calculation.
This should work the same:
(flags & (~BCMA_IOCTL_DO_NOT_MODIFY)) | BCMA_IOCTL_CLK



+   bgmac_idm_write(bgmac, BCMA_IOCTL, regval | BCMA_IOCTL_FGC);
bgmac_idm_read(bgmac, BCMA_IOCTL);

bgmac_idm_write(bgmac, BCMA_RESET_CTL, 0);
bgmac_idm_read(bgmac, BCMA_RESET_CTL);
udelay(1);

-   bgmac_idm_write(bgmac, BCMA_IOCTL, (BCMA_IOCTL_CLK | flags));
+   bgmac_idm_write(bgmac, BCMA_IOCTL, regval);
bgmac_idm_read(bgmac, BCMA_IOCTL);
udelay(1);
 }
diff --git a/include/linux/bcma/bcma_regs.h b/include/linux/bcma/bcma_regs.h
index 9986f82..41d7404 100644
--- a/include/linux/bcma/bcma_regs.h
+++ b/include/linux/bcma/bcma_regs.h
@@ -31,6 +31,7 @@
 #define  BCMA_IOCTL_CORE_BITS  0x3FFC
 #define  BCMA_IOCTL_PME_EN 0x4000
 #define  BCMA_IOCTL_BIST_EN0x8000
+#define  BCMA_IOCTL_DO_NOT_MODIFY  0x7F80


This sounds like a pretty bad name.

Take a look at brcmsmac and SICF_*:
http://lxr.free-electrons.com/source/drivers/net/wireless/broadcom/brcm80211/brcmsmac/d11.h?v=4.9#L1737

Or b43 and B43_BCMA_IOCTL_*:
http://lxr.free-electrons.com/source/drivers/net/wireless/broadcom/b43/b43.h?v=4.9#L494

Both drives modify bits you marked as DO_NOT_MODIFY and they are OK.

Re: [PATCH 4.10-rc3 05/13] net: bgmac: fix build errors when linux/phy*.h is removed from net/dsa.h

[Rafał Miłecki]

On 01/31/2017 08:18 PM, Russell King wrote:

drivers/net/ethernet/broadcom/bgmac.c:1015:17: error: dereferencing pointer to 
incomplete type 'struct mii_bus'
drivers/net/ethernet/broadcom/bgmac.c:1185:2: error: implicit declaration of 
function 'phy_start' [-Werror=implicit-function-declaration]
drivers/net/ethernet/broadcom/bgmac.c:1198:2: error: implicit declaration of 
function 'phy_stop' [-Werror=implicit-function-declaration]
drivers/net/ethernet/broadcom/bgmac.c:1239:9: error: implicit declaration of 
function 'phy_mii_ioctl' [-Werror=implicit-function-declaration]
drivers/net/ethernet/broadcom/bgmac.c:1389:28: error: 
'phy_ethtool_get_link_ksettings' undeclared here (not in a function)
drivers/net/ethernet/broadcom/bgmac.c:1390:28: error: 
'phy_ethtool_set_link_ksettings' undeclared here (not in a function)
drivers/net/ethernet/broadcom/bgmac.c:1403:13: error: dereferencing pointer to 
incomplete type 'struct phy_device'
drivers/net/ethernet/broadcom/bgmac.c:1417:3: error: implicit declaration of 
function 'phy_print_status' [-Werror=implicit-function-declaration]
drivers/net/ethernet/broadcom/bgmac.c:1424:26: error: storage size of 
'fphy_status' isn't known
drivers/net/ethernet/broadcom/bgmac.c:1424:9: error: variable 'fphy_status' has 
initializer but incomplete type
drivers/net/ethernet/broadcom/bgmac.c:1425:11: warning: excess elements in 
struct initializer
drivers/net/ethernet/broadcom/bgmac.c:1425:3: error: unknown field 'link' 
specified in initializer
drivers/net/ethernet/broadcom/bgmac.c:1426:12: note: in expansion of macro 
'SPEED_1000'
drivers/net/ethernet/broadcom/bgmac.c:1426:3: error: unknown field 'speed' 
specified in initializer
drivers/net/ethernet/broadcom/bgmac.c:1427:13: note: in expansion of macro 
'DUPLEX_FULL'
drivers/net/ethernet/broadcom/bgmac.c:1427:3: error: unknown field 'duplex' 
specified in initializer
drivers/net/ethernet/broadcom/bgmac.c:1432:12: error: implicit declaration of 
function 'fixed_phy_register' [-Werror=implicit-function-declaration]
drivers/net/ethernet/broadcom/bgmac.c:1432:31: error: 'PHY_POLL' undeclared 
(first use in this function)
drivers/net/ethernet/broadcom/bgmac.c:1438:8: error: implicit declaration of 
function 'phy_connect_direct' [-Werror=implicit-function-declaration]
drivers/net/ethernet/broadcom/bgmac.c:1439:6: error: 'PHY_INTERFACE_MODE_MII' 
undeclared (first use in this function)
drivers/net/ethernet/broadcom/bgmac.c:1521:2: error: implicit declaration of 
function 'phy_disconnect' [-Werror=implicit-function-declaration]
drivers/net/ethernet/broadcom/bgmac.c:1541:15: error: expected declaration 
specifiers or '...' before string constant

Add linux/phy.h to bgmac.c

Signed-off-by: Russell King <rmk+ker...@armlinux.org.uk>


Acked-by: Rafał Miłecki <ra...@milecki.pl>

[PATCH] net: phy: broadcom: rehook BCM54612E specific init

[Rafał Miłecki]

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

This extra BCM54612E code in PHY driver isn't really aneg specific. Even
without it aneg works OK but the problem is no packets pass through PHY.

Moreover putting this code inside config_aneg callback didn't allow
resuming PHY correctly. When driver called phy_stop and phy_start it was
putting PHY machine into RESUMING state. After that machine was
switching into AN and NOLINK without ever calling phy_start_aneg. This
prevented this extra setup from being called and PHY didn't work.

This change has been verified to fix network on BCM47186B0 SoC device
with BCM54612E.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
 drivers/net/phy/broadcom.c | 67 +++---
 1 file changed, 33 insertions(+), 34 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 794b9ec81ba5..9cd8b27d1292 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -46,6 +46,34 @@ static int bcm54210e_config_init(struct phy_device *phydev)
return 0;
 }
 
+static int bcm54612e_config_init(struct phy_device *phydev)
+{
+   /* Clear TX internal delay unless requested. */
+   if ((phydev->interface != PHY_INTERFACE_MODE_RGMII_ID) &&
+   (phydev->interface != PHY_INTERFACE_MODE_RGMII_TXID)) {
+   /* Disable TXD to GTXCLK clock delay (default set) */
+   /* Bit 9 is the only field in shadow register 00011 */
+   bcm_phy_write_shadow(phydev, 0x03, 0);
+   }
+
+   /* Clear RX internal delay unless requested. */
+   if ((phydev->interface != PHY_INTERFACE_MODE_RGMII_ID) &&
+   (phydev->interface != PHY_INTERFACE_MODE_RGMII_RXID)) {
+   u16 reg;
+
+   reg = bcm54xx_auxctl_read(phydev,
+ MII_BCM54XX_AUXCTL_SHDWSEL_MISC);
+   /* Disable RXD to RXC delay (default set) */
+   reg &= ~MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_SKEW_EN;
+   /* Clear shadow selector field */
+   reg &= ~MII_BCM54XX_AUXCTL_SHDWSEL_MASK;
+   bcm54xx_auxctl_write(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
+MII_BCM54XX_AUXCTL_MISC_WREN | reg);
+   }
+
+   return 0;
+}
+
 static int bcm54810_config(struct phy_device *phydev)
 {
int rc, val;
@@ -250,6 +278,10 @@ static int bcm54xx_config_init(struct phy_device *phydev)
err = bcm54210e_config_init(phydev);
if (err)
return err;
+   } else if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54612E) {
+   err = bcm54612e_config_init(phydev);
+   if (err)
+   return err;
} else if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810) {
err = bcm54810_config(phydev);
if (err)
@@ -395,39 +427,6 @@ static int bcm5481_config_aneg(struct phy_device *phydev)
return ret;
 }
 
-static int bcm54612e_config_aneg(struct phy_device *phydev)
-{
-   int ret;
-
-   /* First, auto-negotiate. */
-   ret = genphy_config_aneg(phydev);
-
-   /* Clear TX internal delay unless requested. */
-   if ((phydev->interface != PHY_INTERFACE_MODE_RGMII_ID) &&
-   (phydev->interface != PHY_INTERFACE_MODE_RGMII_TXID)) {
-   /* Disable TXD to GTXCLK clock delay (default set) */
-   /* Bit 9 is the only field in shadow register 00011 */
-   bcm_phy_write_shadow(phydev, 0x03, 0);
-   }
-
-   /* Clear RX internal delay unless requested. */
-   if ((phydev->interface != PHY_INTERFACE_MODE_RGMII_ID) &&
-   (phydev->interface != PHY_INTERFACE_MODE_RGMII_RXID)) {
-   u16 reg;
-
-   reg = bcm54xx_auxctl_read(phydev,
- MII_BCM54XX_AUXCTL_SHDWSEL_MISC);
-   /* Disable RXD to RXC delay (default set) */
-   reg &= ~MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_SKEW_EN;
-   /* Clear shadow selector field */
-   reg &= ~MII_BCM54XX_AUXCTL_SHDWSEL_MASK;
-   bcm54xx_auxctl_write(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
-MII_BCM54XX_AUXCTL_MISC_WREN | reg);
-   }
-
-   return ret;
-}
-
 static int brcm_phy_setbits(struct phy_device *phydev, int reg, int set)
 {
int val;
@@ -590,7 +589,7 @@ static struct phy_driver broadcom_drivers[] = {
.features   = PHY_GBIT_FEATURES,
.flags  = PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
.config_init= bcm54xx_config_init,
-   .config_aneg= bcm54612e_config_aneg,
+   .config_aneg= genphy_config_aneg,
.read_status= genphy_read_status,
.ack_interrupt  = bcm_phy_ack_intr,
.config_intr= bcm_phy_config_intr,
-- 
2.11.0

[PATCH V3 1/3] net: bgmac: allocate struct bgmac just once & don't copy it

[Rafał Miłecki]

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

So far were were allocating struct bgmac in 3 places: platform code,
bcma code and shared bgmac_enet_probe function. The reason for this was
bgmac_enet_probe:
1) Requiring early-filled struct bgmac
2) Calling alloc_etherdev on its own in order to use netdev_priv later

This solution got few drawbacks:
1) Was duplicating allocating code
2) Required copying early-filled struct
3) Resulted in platform/bcma code having access only to unused struct

Solve this situation by simply extracting some probe code into the new
bgmac_alloc function.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
Reviewed-by: Florian Fainelli <f.faine...@gmail.com>
---
V2: Add bgmac_alloc function instead of hacking alloc_etherdev and netdev_priv
V3: Add missing EXPORT_SYMBOL_GPL to allow compiling as modules
---
 drivers/net/ethernet/broadcom/bgmac-bcma.c |  4 +---
 drivers/net/ethernet/broadcom/bgmac-platform.c |  2 +-
 drivers/net/ethernet/broadcom/bgmac.c  | 25 +
 drivers/net/ethernet/broadcom/bgmac.h  |  3 ++-
 4 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma.c 
b/drivers/net/ethernet/broadcom/bgmac-bcma.c
index 4a4ffc0c4c65..9281abda4026 100644
--- a/drivers/net/ethernet/broadcom/bgmac-bcma.c
+++ b/drivers/net/ethernet/broadcom/bgmac-bcma.c
@@ -117,12 +117,11 @@ static int bgmac_probe(struct bcma_device *core)
u8 *mac;
int err;
 
-   bgmac = kzalloc(sizeof(*bgmac), GFP_KERNEL);
+   bgmac = bgmac_alloc(>dev);
if (!bgmac)
return -ENOMEM;
 
bgmac->bcma.core = core;
-   bgmac->dev = >dev;
bgmac->dma_dev = core->dma_dev;
bgmac->irq = core->irq;
 
@@ -307,7 +306,6 @@ static int bgmac_probe(struct bcma_device *core)
 err1:
bcma_mdio_mii_unregister(bgmac->mii_bus);
 err:
-   kfree(bgmac);
bcma_set_drvdata(core, NULL);
 
return err;
diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c 
b/drivers/net/ethernet/broadcom/bgmac-platform.c
index 6f736c19872f..805e6ed6c390 100644
--- a/drivers/net/ethernet/broadcom/bgmac-platform.c
+++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
@@ -151,7 +151,7 @@ static int bgmac_probe(struct platform_device *pdev)
struct resource *regs;
const u8 *mac_addr;
 
-   bgmac = devm_kzalloc(>dev, sizeof(*bgmac), GFP_KERNEL);
+   bgmac = bgmac_alloc(>dev);
if (!bgmac)
return -ENOMEM;
 
diff --git a/drivers/net/ethernet/broadcom/bgmac.c 
b/drivers/net/ethernet/broadcom/bgmac.c
index 9122608df16e..fe88126b1e0c 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1446,22 +1446,32 @@ int bgmac_phy_connect_direct(struct bgmac *bgmac)
 }
 EXPORT_SYMBOL_GPL(bgmac_phy_connect_direct);
 
-int bgmac_enet_probe(struct bgmac *info)
+struct bgmac *bgmac_alloc(struct device *dev)
 {
struct net_device *net_dev;
struct bgmac *bgmac;
-   int err;
 
/* Allocation and references */
-   net_dev = alloc_etherdev(sizeof(*bgmac));
+   net_dev = devm_alloc_etherdev(dev, sizeof(*bgmac));
if (!net_dev)
-   return -ENOMEM;
+   return NULL;
 
net_dev->netdev_ops = _netdev_ops;
net_dev->ethtool_ops = _ethtool_ops;
+
bgmac = netdev_priv(net_dev);
-   memcpy(bgmac, info, sizeof(*bgmac));
+   bgmac->dev = dev;
bgmac->net_dev = net_dev;
+
+   return bgmac;
+}
+EXPORT_SYMBOL_GPL(bgmac_alloc);
+
+int bgmac_enet_probe(struct bgmac *bgmac)
+{
+   struct net_device *net_dev = bgmac->net_dev;
+   int err;
+
net_dev->irq = bgmac->irq;
SET_NETDEV_DEV(net_dev, bgmac->dev);
 
@@ -1488,7 +1498,7 @@ int bgmac_enet_probe(struct bgmac *info)
err = bgmac_dma_alloc(bgmac);
if (err) {
dev_err(bgmac->dev, "Unable to alloc memory for DMA\n");
-   goto err_netdev_free;
+   goto err_out;
}
 
bgmac->int_mask = BGMAC_IS_ERRMASK | BGMAC_IS_RX | BGMAC_IS_TX_MASK;
@@ -1521,8 +1531,7 @@ int bgmac_enet_probe(struct bgmac *info)
phy_disconnect(net_dev->phydev);
 err_dma_free:
bgmac_dma_free(bgmac);
-err_netdev_free:
-   free_netdev(net_dev);
+err_out:
 
return err;
 }
diff --git a/drivers/net/ethernet/broadcom/bgmac.h 
b/drivers/net/ethernet/broadcom/bgmac.h
index 71f493f2451f..dfebaded3b52 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -517,7 +517,8 @@ struct bgmac {
int (*phy_connect)(struct bgmac *bgmac);
 };
 
-int bgmac_enet_probe(struct bgmac *info);
+struct bgmac *bgmac_alloc(struct device *dev);
+int bgmac_enet_probe(struct bgmac *bgmac);
 void bgmac_enet_remove(struct bgmac *bgmac);
 void bgmac_adjust_link(struct net_device *net_dev);
 int bgmac_phy_connect_direct(struct bgmac *bgmac);
-- 
2.11.0

[PATCH V3 2/3] net: bgmac: drop struct bcma_mdio we don't need anymore

[Rafał Miłecki]

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

Adding struct bcma_mdio was a workaround for bcma code not having access
to the struct bgmac used in the core code. Now we don't duplicate this
struct we can just use it internally in bcma code.

This simplifies code & allows access to all bgmac driver details from
all places in bcma code.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
Reviewed-by: Florian Fainelli <f.faine...@gmail.com>
---
 drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c | 98 ++---
 drivers/net/ethernet/broadcom/bgmac-bcma.c  |  2 +-
 drivers/net/ethernet/broadcom/bgmac.h   |  2 +-
 3 files changed, 42 insertions(+), 60 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c 
b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
index 7c19c8e2bf91..9d9984999dce 100644
--- a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
+++ b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
@@ -12,11 +12,6 @@
 #include 
 #include "bgmac.h"
 
-struct bcma_mdio {
-   struct bcma_device *core;
-   u8 phyaddr;
-};
-
 static bool bcma_mdio_wait_value(struct bcma_device *core, u16 reg, u32 mask,
 u32 value, int timeout)
 {
@@ -37,7 +32,7 @@ static bool bcma_mdio_wait_value(struct bcma_device *core, 
u16 reg, u32 mask,
  * PHY ops
  **/
 
-static u16 bcma_mdio_phy_read(struct bcma_mdio *bcma_mdio, u8 phyaddr, u8 reg)
+static u16 bcma_mdio_phy_read(struct bgmac *bgmac, u8 phyaddr, u8 reg)
 {
struct bcma_device *core;
u16 phy_access_addr;
@@ -56,12 +51,12 @@ static u16 bcma_mdio_phy_read(struct bcma_mdio *bcma_mdio, 
u8 phyaddr, u8 reg)
BUILD_BUG_ON(BGMAC_PC_MCT_SHIFT != BCMA_GMAC_CMN_PC_MCT_SHIFT);
BUILD_BUG_ON(BGMAC_PC_MTE != BCMA_GMAC_CMN_PC_MTE);
 
-   if (bcma_mdio->core->id.id == BCMA_CORE_4706_MAC_GBIT) {
-   core = bcma_mdio->core->bus->drv_gmac_cmn.core;
+   if (bgmac->bcma.core->id.id == BCMA_CORE_4706_MAC_GBIT) {
+   core = bgmac->bcma.core->bus->drv_gmac_cmn.core;
phy_access_addr = BCMA_GMAC_CMN_PHY_ACCESS;
phy_ctl_addr = BCMA_GMAC_CMN_PHY_CTL;
} else {
-   core = bcma_mdio->core;
+   core = bgmac->bcma.core;
phy_access_addr = BGMAC_PHY_ACCESS;
phy_ctl_addr = BGMAC_PHY_CNTL;
}
@@ -87,7 +82,7 @@ static u16 bcma_mdio_phy_read(struct bcma_mdio *bcma_mdio, u8 
phyaddr, u8 reg)
 }
 
 /* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipphywr */
-static int bcma_mdio_phy_write(struct bcma_mdio *bcma_mdio, u8 phyaddr, u8 reg,
+static int bcma_mdio_phy_write(struct bgmac *bgmac, u8 phyaddr, u8 reg,
   u16 value)
 {
struct bcma_device *core;
@@ -95,12 +90,12 @@ static int bcma_mdio_phy_write(struct bcma_mdio *bcma_mdio, 
u8 phyaddr, u8 reg,
u16 phy_ctl_addr;
u32 tmp;
 
-   if (bcma_mdio->core->id.id == BCMA_CORE_4706_MAC_GBIT) {
-   core = bcma_mdio->core->bus->drv_gmac_cmn.core;
+   if (bgmac->bcma.core->id.id == BCMA_CORE_4706_MAC_GBIT) {
+   core = bgmac->bcma.core->bus->drv_gmac_cmn.core;
phy_access_addr = BCMA_GMAC_CMN_PHY_ACCESS;
phy_ctl_addr = BCMA_GMAC_CMN_PHY_CTL;
} else {
-   core = bcma_mdio->core;
+   core = bgmac->bcma.core;
phy_access_addr = BGMAC_PHY_ACCESS;
phy_ctl_addr = BGMAC_PHY_CNTL;
}
@@ -110,8 +105,8 @@ static int bcma_mdio_phy_write(struct bcma_mdio *bcma_mdio, 
u8 phyaddr, u8 reg,
tmp |= phyaddr;
bcma_write32(core, phy_ctl_addr, tmp);
 
-   bcma_write32(bcma_mdio->core, BGMAC_INT_STATUS, BGMAC_IS_MDIO);
-   if (bcma_read32(bcma_mdio->core, BGMAC_INT_STATUS) & BGMAC_IS_MDIO)
+   bcma_write32(bgmac->bcma.core, BGMAC_INT_STATUS, BGMAC_IS_MDIO);
+   if (bcma_read32(bgmac->bcma.core, BGMAC_INT_STATUS) & BGMAC_IS_MDIO)
dev_warn(>dev, "Error setting MDIO int\n");
 
tmp = BGMAC_PA_START;
@@ -132,39 +127,39 @@ static int bcma_mdio_phy_write(struct bcma_mdio 
*bcma_mdio, u8 phyaddr, u8 reg,
 }
 
 /* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipphyinit */
-static void bcma_mdio_phy_init(struct bcma_mdio *bcma_mdio)
+static void bcma_mdio_phy_init(struct bgmac *bgmac)
 {
-   struct bcma_chipinfo *ci = _mdio->core->bus->chipinfo;
+   struct bcma_chipinfo *ci = >bcma.core->bus->chipinfo;
u8 i;
 
if (ci->id == BCMA_CHIP_ID_BCM5356) {
for (i = 0; i < 5; i++) {
-   bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x008b);
-   bcma_mdio_phy_write(bcma_mdio, i, 0x15, 0x0100);
-   bcma_mdio_phy_write(bcma_mdio, 

[PATCH V3 3/3] net: bgmac: use PHY subsystem for initializing PHY

[Rafał Miłecki]

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

This adds support for using bgmac with PHYs supported by standalone PHY
drivers. Having any PHY initialization in bgmac is hacky and shouldn't
be extended but rather removed if anyone has hardware to test it.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
Reviewed-by: Florian Fainelli <f.faine...@gmail.com>
---
 drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c 
b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
index 9d9984999dce..6ce80cbcb48e 100644
--- a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
+++ b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
@@ -132,6 +132,10 @@ static void bcma_mdio_phy_init(struct bgmac *bgmac)
struct bcma_chipinfo *ci = >bcma.core->bus->chipinfo;
u8 i;
 
+   /* For some legacy hardware we do chipset-based PHY initialization here
+* without even detecting PHY ID. It's hacky and should be cleaned as
+* soon as someone can test it.
+*/
if (ci->id == BCMA_CHIP_ID_BCM5356) {
for (i = 0; i < 5; i++) {
bcma_mdio_phy_write(bgmac, i, 0x1f, 0x008b);
@@ -140,6 +144,7 @@ static void bcma_mdio_phy_init(struct bgmac *bgmac)
bcma_mdio_phy_write(bgmac, i, 0x12, 0x2aaa);
bcma_mdio_phy_write(bgmac, i, 0x1f, 0x000b);
}
+   return;
}
if ((ci->id == BCMA_CHIP_ID_BCM5357 && ci->pkg != 10) ||
(ci->id == BCMA_CHIP_ID_BCM4749 && ci->pkg != 10) ||
@@ -161,7 +166,12 @@ static void bcma_mdio_phy_init(struct bgmac *bgmac)
bcma_mdio_phy_write(bgmac, i, 0x17, 0x9273);
bcma_mdio_phy_write(bgmac, i, 0x1f, 0x000b);
}
+   return;
}
+
+   /* For all other hw do initialization using PHY subsystem. */
+   if (bgmac->net_dev && bgmac->net_dev->phydev)
+   phy_init_hw(bgmac->net_dev->phydev);
 }
 
 /* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipphyreset */
-- 
2.11.0

[PATCH V3 0/3] net-next: use one struct bgmac & add PHY support

[Rafał Miłecki]

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

This patchset adds support for initializing PHY using PHY subsystem.
It's required e.g. for wireless access point devices that use bgmac
supported Ethernet device connected to some external PHY.

Implementing this required accessing phydev in bcma specific code which
wasn't possible with core code allocating struct bgmac on its own. This
is why I needed to modify alloc_etherdev usage first.

Rafał Miłecki (3):
  net: bgmac: allocate struct bgmac just once & don't copy it
  net: bgmac: drop struct bcma_mdio we don't need anymore
  net: bgmac: use PHY subsystem for initializing PHY

 drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c | 108 +++-
 drivers/net/ethernet/broadcom/bgmac-bcma.c  |   6 +-
 drivers/net/ethernet/broadcom/bgmac-platform.c  |   2 +-
 drivers/net/ethernet/broadcom/bgmac.c   |  25 --
 drivers/net/ethernet/broadcom/bgmac.h   |   5 +-
 5 files changed, 73 insertions(+), 73 deletions(-)

-- 
2.11.0

Re: [PATCH V2 3/3] net: bgmac: use PHY subsystem for initializing PHY

[Rafał Miłecki]

On 31 January 2017 at 19:16, David Miller  wrote:
> From: David Miller 
> Date: Tue, 31 Jan 2017 13:14:52 -0500 (EST)
>
>> From: Florian Fainelli 
>> Date: Tue, 31 Jan 2017 10:06:34 -0800
>>
>>> On 01/31/2017 10:04 AM, David Miller wrote:

 This entire series is being held up because there is still questions about
 whether this is programming the internal PHYs and therefore whether the
 code should be placed elsewhere.

 Can we please move the discussion forward?
>>>
>>> The patches are good as-is, we can resolve the PHY programming later on
>>> when we have figured out the details of the 4749 chip we derailed on.
>>
>> Ok great, series applied, thanks.
>
> Actually, reverted, I get build failures:
>
> [davem@dhcp-10-15-49-210 net-next]$ make -s -j8
>   DESCEND  objtool
> Setup is 17084 bytes (padded to 17408 bytes).
> System is 10211 kB
> CRC 68863231
> Kernel: arch/x86/boot/bzImage is ready  (#139)
> ERROR: "bgmac_alloc" [drivers/net/ethernet/broadcom/bgmac-platform.ko] 
> undefined!
> ERROR: "bgmac_alloc" [drivers/net/ethernet/broadcom/bgmac-bcma.ko] undefined!
> scripts/Makefile.modpost:91: recipe for target '__modpost' failed
> make[1]: *** [__modpost] Error 1
> Makefile:1196: recipe for target 'modules' failed
> make: *** [modules] Error 2

Sorry! :( I'm working on this. Shit, I should have resent this after
adding devm_alloc_etherdev to let kbuild test it.

-- 
Rafał

Re: [PATCH V2 3/3] net: bgmac: use PHY subsystem for initializing PHY

[Rafał Miłecki]

On 29 January 2017 at 23:36, Florian Fainelli <f.faine...@gmail.com> wrote:
> Le 01/29/17 à 13:31, Rafał Miłecki a écrit :
>> On 29 January 2017 at 21:22, Florian Fainelli <f.faine...@gmail.com> wrote:
>>> On 01/29/2017 12:14 PM, Rafał Miłecki wrote:
>>>> On 01/29/2017 04:08 AM, Florian Fainelli wrote:
>>>>> On 01/28/2017 01:08 PM, Rafał Miłecki wrote:
>>>>>> From: Rafał Miłecki <ra...@milecki.pl>
>>>>>>
>>>>>> This adds support for using bgmac with PHYs supported by standalone PHY
>>>>>> drivers. Having any PHY initialization in bgmac is hacky and shouldn't
>>>>>> be extended but rather removed if anyone has hardware to test it.
>>>>>>
>>>>>> Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
>>>>>> ---
>>>>>>  drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c | 10 ++
>>>>>>  1 file changed, 10 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
>>>>>> b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
>>>>>> index 9d9984999dce..6ce80cbcb48e 100644
>>>>>> --- a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
>>>>>> +++ b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
>>>>>> @@ -132,6 +132,10 @@ static void bcma_mdio_phy_init(struct bgmac *bgmac)
>>>>>>  struct bcma_chipinfo *ci = >bcma.core->bus->chipinfo;
>>>>>>  u8 i;
>>>>>>
>>>>>> +/* For some legacy hardware we do chipset-based PHY
>>>>>> initialization here
>>>>>> + * without even detecting PHY ID. It's hacky and should be
>>>>>> cleaned as
>>>>>> + * soon as someone can test it.
>>>>>> + */
>>>>>>  if (ci->id == BCMA_CHIP_ID_BCM5356) {
>>>>>>  for (i = 0; i < 5; i++) {
>>>>>>  bcma_mdio_phy_write(bgmac, i, 0x1f, 0x008b);
>>>>>> @@ -140,6 +144,7 @@ static void bcma_mdio_phy_init(struct bgmac *bgmac)
>>>>>>  bcma_mdio_phy_write(bgmac, i, 0x12, 0x2aaa);
>>>>>>  bcma_mdio_phy_write(bgmac, i, 0x1f, 0x000b);
>>>>>>  }
>>>>>> +return;
>>>>>
>>>>> That part is clearly initializing the built-in Ethernet switch's PHYs,
>>>>> and so the natural place for that would be to stick these init values
>>>>> into the Broadcom PHY driver. When b53-srab/b53_common attaches the
>>>>> switch, it will scan all of these port's builtin PHYs and bind to an
>>>>> appropriate PHY driver which could have this initialization as part of
>>>>> the config_init routine for instance. Right now, we are most likely
>>>>> using the Generic PHY.
>>>>
>>>> I don't think this code is for switch's PHYs. I believe this code is for
>>>> wireless access points that have no switch and have Ethernet interface
>>>> connected
>>>> directly to some single-port PHY. I saw 2 or 3 devices like this. They
>>>> often
>>>> also use PoE.
>>>
>>> Humm, built-in PHYs would typically appear as 0-5 on the MDIO bus,
>>> whereas external PHYs would have a different (and non conflicting)
>>> address, also, there are restrictions on the Roboswitch devices as to
>>> where you could wire these external PHYs (to port 5, or 7, 8).
>>
>> From BCM47186B0:
>> [0.942419] bgmac_bcma bcma0:2: Found PHY addr: 25
>>
>> From BCM47189:
>> [1.758079] bgmac_bcma bcma0:5: Found PHY addr: 0
>
> Address 0 is usually a broadcast address, although with most Broadcom
> switches, it just maps to the first port's built-in PHY.
>
>>
>> I got one more AP device but I bricked it by corrupting NVRAM
>> (bootloader doesn't start due to that).
>>
>>
>> My BCM47186B0 seems to not have any switch. It isn't that clear in
>> BCM47189 case. You may be right, some sources say BCM47189 has
>> built-in switch so maybe it's indeed BCM54210E connected to switch's
>> port. On the other hand I'm not using any switch driver on my BCM47189
>> AP board, so how does it work? Just an accidentally working setup left
>> by the bootloader?
>
> That's what I suspect is happening yes. Do you have the different FCC
> IDs of these routers so maybe looking at pictures of the inside would
> tell us what kind of internal vs. external PHYs are populated?

I don't. I read all chipset IDs I could see:
1) 2 Winbond RAM chips
2) MXIC MX25L25635F flash
3) BCM47189 SoC
4) B50212E PHY
5) BCM43217 WiFi

There clearly isn't external switch (like BCM43125 or so) but we can't
say (even from the photos) where that B50212E is connected to. It's
most likely built-in switch's port as you said.

-- 
Rafał

Re: [PATCH V2 3/3] net: bgmac: use PHY subsystem for initializing PHY

[Rafał Miłecki]

On 29 January 2017 at 21:22, Florian Fainelli <f.faine...@gmail.com> wrote:
> On 01/29/2017 12:14 PM, Rafał Miłecki wrote:
>> On 01/29/2017 04:08 AM, Florian Fainelli wrote:
>>> On 01/28/2017 01:08 PM, Rafał Miłecki wrote:
>>>> From: Rafał Miłecki <ra...@milecki.pl>
>>>>
>>>> This adds support for using bgmac with PHYs supported by standalone PHY
>>>> drivers. Having any PHY initialization in bgmac is hacky and shouldn't
>>>> be extended but rather removed if anyone has hardware to test it.
>>>>
>>>> Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
>>>> ---
>>>>  drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c | 10 ++
>>>>  1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
>>>> b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
>>>> index 9d9984999dce..6ce80cbcb48e 100644
>>>> --- a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
>>>> +++ b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
>>>> @@ -132,6 +132,10 @@ static void bcma_mdio_phy_init(struct bgmac *bgmac)
>>>>  struct bcma_chipinfo *ci = >bcma.core->bus->chipinfo;
>>>>  u8 i;
>>>>
>>>> +/* For some legacy hardware we do chipset-based PHY
>>>> initialization here
>>>> + * without even detecting PHY ID. It's hacky and should be
>>>> cleaned as
>>>> + * soon as someone can test it.
>>>> + */
>>>>  if (ci->id == BCMA_CHIP_ID_BCM5356) {
>>>>  for (i = 0; i < 5; i++) {
>>>>  bcma_mdio_phy_write(bgmac, i, 0x1f, 0x008b);
>>>> @@ -140,6 +144,7 @@ static void bcma_mdio_phy_init(struct bgmac *bgmac)
>>>>  bcma_mdio_phy_write(bgmac, i, 0x12, 0x2aaa);
>>>>  bcma_mdio_phy_write(bgmac, i, 0x1f, 0x000b);
>>>>  }
>>>> +return;
>>>
>>> That part is clearly initializing the built-in Ethernet switch's PHYs,
>>> and so the natural place for that would be to stick these init values
>>> into the Broadcom PHY driver. When b53-srab/b53_common attaches the
>>> switch, it will scan all of these port's builtin PHYs and bind to an
>>> appropriate PHY driver which could have this initialization as part of
>>> the config_init routine for instance. Right now, we are most likely
>>> using the Generic PHY.
>>
>> I don't think this code is for switch's PHYs. I believe this code is for
>> wireless access points that have no switch and have Ethernet interface
>> connected
>> directly to some single-port PHY. I saw 2 or 3 devices like this. They
>> often
>> also use PoE.
>
> Humm, built-in PHYs would typically appear as 0-5 on the MDIO bus,
> whereas external PHYs would have a different (and non conflicting)
> address, also, there are restrictions on the Roboswitch devices as to
> where you could wire these external PHYs (to port 5, or 7, 8).

>From BCM47186B0:
[0.942419] bgmac_bcma bcma0:2: Found PHY addr: 25

>From BCM47189:
[1.758079] bgmac_bcma bcma0:5: Found PHY addr: 0

I got one more AP device but I bricked it by corrupting NVRAM
(bootloader doesn't start due to that).


My BCM47186B0 seems to not have any switch. It isn't that clear in
BCM47189 case. You may be right, some sources say BCM47189 has
built-in switch so maybe it's indeed BCM54210E connected to switch's
port. On the other hand I'm not using any switch driver on my BCM47189
AP board, so how does it work? Just an accidentally working setup left
by the bootloader?

-- 
Rafał

Re: [PATCH V2 3/3] net: bgmac: use PHY subsystem for initializing PHY

[Rafał Miłecki]

On 01/29/2017 04:08 AM, Florian Fainelli wrote:

On 01/28/2017 01:08 PM, Rafał Miłecki wrote:

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

This adds support for using bgmac with PHYs supported by standalone PHY
drivers. Having any PHY initialization in bgmac is hacky and shouldn't
be extended but rather removed if anyone has hardware to test it.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
 drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c 
b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
index 9d9984999dce..6ce80cbcb48e 100644
--- a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
+++ b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
@@ -132,6 +132,10 @@ static void bcma_mdio_phy_init(struct bgmac *bgmac)
struct bcma_chipinfo *ci = >bcma.core->bus->chipinfo;
u8 i;

+   /* For some legacy hardware we do chipset-based PHY initialization here
+* without even detecting PHY ID. It's hacky and should be cleaned as
+* soon as someone can test it.
+*/
if (ci->id == BCMA_CHIP_ID_BCM5356) {
for (i = 0; i < 5; i++) {
bcma_mdio_phy_write(bgmac, i, 0x1f, 0x008b);
@@ -140,6 +144,7 @@ static void bcma_mdio_phy_init(struct bgmac *bgmac)
bcma_mdio_phy_write(bgmac, i, 0x12, 0x2aaa);
bcma_mdio_phy_write(bgmac, i, 0x1f, 0x000b);
}
+   return;


That part is clearly initializing the built-in Ethernet switch's PHYs,
and so the natural place for that would be to stick these init values
into the Broadcom PHY driver. When b53-srab/b53_common attaches the
switch, it will scan all of these port's builtin PHYs and bind to an
appropriate PHY driver which could have this initialization as part of
the config_init routine for instance. Right now, we are most likely
using the Generic PHY.


I don't think this code is for switch's PHYs. I believe this code is for
wireless access points that have no switch and have Ethernet interface connected
directly to some single-port PHY. I saw 2 or 3 devices like this. They often
also use PoE.



Here are the different PHY IDs you should read from these models if you
want to make a subsequent patch that moves this initialization down to
the Broadcom PHY driver:

5356: 0x03625DA0
5357/53572: 0x03625F00
4749: could either be 0x600D85F0 or the same as 53010 (0x600D8760),
unclear where that product came from... Jon, would you know by chance?


This is very valuable info, thank you! I'll definitely work on this. So far I
tried using bgmac.ko + broadcom.ko on AP device with BCM47186B0 but it doesn't
work for some reason, I'll work on this & keep moving PHY code out of bgmac.

Re: [PATCH V2 1/3] net: bgmac: allocate struct bgmac just once & don't copy it

[Rafał Miłecki]

On 01/29/2017 12:40 AM, kbuild test robot wrote:

[auto build test ERROR on net-next/master]
[also build test ERROR on v4.10-rc5 next-20170125]
[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/Rafa-Mi-ecki/net-next-use-one-struct-bgmac-add-PHY-support/20170129-062241
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386

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

   drivers/net/ethernet/broadcom/bgmac.c: In function 'bgmac_alloc':

drivers/net/ethernet/broadcom/bgmac.c:1455:12: error: implicit declaration of 
function 'devm_alloc_etherdev' [-Werror=implicit-function-declaration]

 net_dev = devm_alloc_etherdev(dev, sizeof(*bgmac));
   ^~~


Thanks, this is expected as this patch depends on
[PATCH net-next] net: add devm version of alloc_etherdev_mqs function

Let's see if net guys will find my devm_alloc_etherdev implementation
acceptable.

[PATCH V2 2/3] net: bgmac: drop struct bcma_mdio we don't need anymore

[Rafał Miłecki]

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

Adding struct bcma_mdio was a workaround for bcma code not having access
to the struct bgmac used in the core code. Now we don't duplicate this
struct we can just use it internally in bcma code.

This simplifies code & allows access to all bgmac driver details from
all places in bcma code.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
 drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c | 98 ++---
 drivers/net/ethernet/broadcom/bgmac-bcma.c  |  2 +-
 drivers/net/ethernet/broadcom/bgmac.h   |  2 +-
 3 files changed, 42 insertions(+), 60 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c 
b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
index 7c19c8e2bf91..9d9984999dce 100644
--- a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
+++ b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
@@ -12,11 +12,6 @@
 #include 
 #include "bgmac.h"
 
-struct bcma_mdio {
-   struct bcma_device *core;
-   u8 phyaddr;
-};
-
 static bool bcma_mdio_wait_value(struct bcma_device *core, u16 reg, u32 mask,
 u32 value, int timeout)
 {
@@ -37,7 +32,7 @@ static bool bcma_mdio_wait_value(struct bcma_device *core, 
u16 reg, u32 mask,
  * PHY ops
  **/
 
-static u16 bcma_mdio_phy_read(struct bcma_mdio *bcma_mdio, u8 phyaddr, u8 reg)
+static u16 bcma_mdio_phy_read(struct bgmac *bgmac, u8 phyaddr, u8 reg)
 {
struct bcma_device *core;
u16 phy_access_addr;
@@ -56,12 +51,12 @@ static u16 bcma_mdio_phy_read(struct bcma_mdio *bcma_mdio, 
u8 phyaddr, u8 reg)
BUILD_BUG_ON(BGMAC_PC_MCT_SHIFT != BCMA_GMAC_CMN_PC_MCT_SHIFT);
BUILD_BUG_ON(BGMAC_PC_MTE != BCMA_GMAC_CMN_PC_MTE);
 
-   if (bcma_mdio->core->id.id == BCMA_CORE_4706_MAC_GBIT) {
-   core = bcma_mdio->core->bus->drv_gmac_cmn.core;
+   if (bgmac->bcma.core->id.id == BCMA_CORE_4706_MAC_GBIT) {
+   core = bgmac->bcma.core->bus->drv_gmac_cmn.core;
phy_access_addr = BCMA_GMAC_CMN_PHY_ACCESS;
phy_ctl_addr = BCMA_GMAC_CMN_PHY_CTL;
} else {
-   core = bcma_mdio->core;
+   core = bgmac->bcma.core;
phy_access_addr = BGMAC_PHY_ACCESS;
phy_ctl_addr = BGMAC_PHY_CNTL;
}
@@ -87,7 +82,7 @@ static u16 bcma_mdio_phy_read(struct bcma_mdio *bcma_mdio, u8 
phyaddr, u8 reg)
 }
 
 /* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipphywr */
-static int bcma_mdio_phy_write(struct bcma_mdio *bcma_mdio, u8 phyaddr, u8 reg,
+static int bcma_mdio_phy_write(struct bgmac *bgmac, u8 phyaddr, u8 reg,
   u16 value)
 {
struct bcma_device *core;
@@ -95,12 +90,12 @@ static int bcma_mdio_phy_write(struct bcma_mdio *bcma_mdio, 
u8 phyaddr, u8 reg,
u16 phy_ctl_addr;
u32 tmp;
 
-   if (bcma_mdio->core->id.id == BCMA_CORE_4706_MAC_GBIT) {
-   core = bcma_mdio->core->bus->drv_gmac_cmn.core;
+   if (bgmac->bcma.core->id.id == BCMA_CORE_4706_MAC_GBIT) {
+   core = bgmac->bcma.core->bus->drv_gmac_cmn.core;
phy_access_addr = BCMA_GMAC_CMN_PHY_ACCESS;
phy_ctl_addr = BCMA_GMAC_CMN_PHY_CTL;
} else {
-   core = bcma_mdio->core;
+   core = bgmac->bcma.core;
phy_access_addr = BGMAC_PHY_ACCESS;
phy_ctl_addr = BGMAC_PHY_CNTL;
}
@@ -110,8 +105,8 @@ static int bcma_mdio_phy_write(struct bcma_mdio *bcma_mdio, 
u8 phyaddr, u8 reg,
tmp |= phyaddr;
bcma_write32(core, phy_ctl_addr, tmp);
 
-   bcma_write32(bcma_mdio->core, BGMAC_INT_STATUS, BGMAC_IS_MDIO);
-   if (bcma_read32(bcma_mdio->core, BGMAC_INT_STATUS) & BGMAC_IS_MDIO)
+   bcma_write32(bgmac->bcma.core, BGMAC_INT_STATUS, BGMAC_IS_MDIO);
+   if (bcma_read32(bgmac->bcma.core, BGMAC_INT_STATUS) & BGMAC_IS_MDIO)
dev_warn(>dev, "Error setting MDIO int\n");
 
tmp = BGMAC_PA_START;
@@ -132,39 +127,39 @@ static int bcma_mdio_phy_write(struct bcma_mdio 
*bcma_mdio, u8 phyaddr, u8 reg,
 }
 
 /* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipphyinit */
-static void bcma_mdio_phy_init(struct bcma_mdio *bcma_mdio)
+static void bcma_mdio_phy_init(struct bgmac *bgmac)
 {
-   struct bcma_chipinfo *ci = _mdio->core->bus->chipinfo;
+   struct bcma_chipinfo *ci = >bcma.core->bus->chipinfo;
u8 i;
 
if (ci->id == BCMA_CHIP_ID_BCM5356) {
for (i = 0; i < 5; i++) {
-   bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x008b);
-   bcma_mdio_phy_write(bcma_mdio, i, 0x15, 0x0100);
-   bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000f);
-   bcma_mdio_phy_write(bcma_mdio, i,

[PATCH V2 1/3] net: bgmac: allocate struct bgmac just once & don't copy it

[Rafał Miłecki]

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

So far were were allocating struct bgmac in 3 places: platform code,
bcma code and shared bgmac_enet_probe function. The reason for this was
bgmac_enet_probe:
1) Requiring early-filled struct bgmac
2) Calling alloc_etherdev on its own in order to use netdev_priv later

This solution got few drawbacks:
1) Was duplicating allocating code
2) Required copying early-filled struct
3) Resulted in platform/bcma code having access only to unused struct

Solve this situation by simply extracting some probe code into the new
bgmac_alloc function.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
V2: Add bgmac_alloc function instead of hacking alloc_etherdev and netdev_priv

Important: this patch depends on:
[PATCH net-next] net: add devm version of alloc_etherdev_mqs function
and is the first user of devm_alloc_etherdev.
---
 drivers/net/ethernet/broadcom/bgmac-bcma.c |  4 +---
 drivers/net/ethernet/broadcom/bgmac-platform.c |  2 +-
 drivers/net/ethernet/broadcom/bgmac.c  | 24 
 drivers/net/ethernet/broadcom/bgmac.h  |  3 ++-
 4 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma.c 
b/drivers/net/ethernet/broadcom/bgmac-bcma.c
index 4a4ffc0c4c65..9281abda4026 100644
--- a/drivers/net/ethernet/broadcom/bgmac-bcma.c
+++ b/drivers/net/ethernet/broadcom/bgmac-bcma.c
@@ -117,12 +117,11 @@ static int bgmac_probe(struct bcma_device *core)
u8 *mac;
int err;
 
-   bgmac = kzalloc(sizeof(*bgmac), GFP_KERNEL);
+   bgmac = bgmac_alloc(>dev);
if (!bgmac)
return -ENOMEM;
 
bgmac->bcma.core = core;
-   bgmac->dev = >dev;
bgmac->dma_dev = core->dma_dev;
bgmac->irq = core->irq;
 
@@ -307,7 +306,6 @@ static int bgmac_probe(struct bcma_device *core)
 err1:
bcma_mdio_mii_unregister(bgmac->mii_bus);
 err:
-   kfree(bgmac);
bcma_set_drvdata(core, NULL);
 
return err;
diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c 
b/drivers/net/ethernet/broadcom/bgmac-platform.c
index 6f736c19872f..805e6ed6c390 100644
--- a/drivers/net/ethernet/broadcom/bgmac-platform.c
+++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
@@ -151,7 +151,7 @@ static int bgmac_probe(struct platform_device *pdev)
struct resource *regs;
const u8 *mac_addr;
 
-   bgmac = devm_kzalloc(>dev, sizeof(*bgmac), GFP_KERNEL);
+   bgmac = bgmac_alloc(>dev);
if (!bgmac)
return -ENOMEM;
 
diff --git a/drivers/net/ethernet/broadcom/bgmac.c 
b/drivers/net/ethernet/broadcom/bgmac.c
index 0e066dc6b8cc..632d4d7b5a5b 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1446,22 +1446,31 @@ int bgmac_phy_connect_direct(struct bgmac *bgmac)
 }
 EXPORT_SYMBOL_GPL(bgmac_phy_connect_direct);
 
-int bgmac_enet_probe(struct bgmac *info)
+struct bgmac *bgmac_alloc(struct device *dev)
 {
struct net_device *net_dev;
struct bgmac *bgmac;
-   int err;
 
/* Allocation and references */
-   net_dev = alloc_etherdev(sizeof(*bgmac));
+   net_dev = devm_alloc_etherdev(dev, sizeof(*bgmac));
if (!net_dev)
-   return -ENOMEM;
+   return NULL;
 
net_dev->netdev_ops = _netdev_ops;
net_dev->ethtool_ops = _ethtool_ops;
+
bgmac = netdev_priv(net_dev);
-   memcpy(bgmac, info, sizeof(*bgmac));
+   bgmac->dev = dev;
bgmac->net_dev = net_dev;
+
+   return bgmac;
+}
+
+int bgmac_enet_probe(struct bgmac *bgmac)
+{
+   struct net_device *net_dev = bgmac->net_dev;
+   int err;
+
net_dev->irq = bgmac->irq;
SET_NETDEV_DEV(net_dev, bgmac->dev);
 
@@ -1488,7 +1497,7 @@ int bgmac_enet_probe(struct bgmac *info)
err = bgmac_dma_alloc(bgmac);
if (err) {
dev_err(bgmac->dev, "Unable to alloc memory for DMA\n");
-   goto err_netdev_free;
+   goto err_out;
}
 
bgmac->int_mask = BGMAC_IS_ERRMASK | BGMAC_IS_RX | BGMAC_IS_TX_MASK;
@@ -1521,8 +1530,7 @@ int bgmac_enet_probe(struct bgmac *info)
phy_disconnect(net_dev->phydev);
 err_dma_free:
bgmac_dma_free(bgmac);
-err_netdev_free:
-   free_netdev(net_dev);
+err_out:
 
return err;
 }
diff --git a/drivers/net/ethernet/broadcom/bgmac.h 
b/drivers/net/ethernet/broadcom/bgmac.h
index 71f493f2451f..dfebaded3b52 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -517,7 +517,8 @@ struct bgmac {
int (*phy_connect)(struct bgmac *bgmac);
 };
 
-int bgmac_enet_probe(struct bgmac *info);
+struct bgmac *bgmac_alloc(struct device *dev);
+int bgmac_enet_probe(struct bgmac *bgmac);
 void bgmac_enet_remove(struct bgmac *bgmac);
 void bgmac_adjust_link(struct net_device *net_dev);
 int bgmac_phy_connect_direct(struct bgmac *bgmac);
-- 
2.11.0

[PATCH V2 0/3] net-next: use one struct bgmac & add PHY support

[Rafał Miłecki]

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

This patchset adds support for initializing PHY using PHY subsystem.
It's required e.g. for wireless access point devices that use bgmac
supported Ethernet device connected to some external PHY.

Implementing this required accessing phydev in bcma specific code which
wasn't possible with core code allocating struct bgmac on its own. This
is why I needed to modify alloc_etherdev usage first.

Rafał Miłecki (3):
  net: bgmac: allocate struct bgmac just once & don't copy it
  net: bgmac: drop struct bcma_mdio we don't need anymore
  net: bgmac: use PHY subsystem for initializing PHY

 drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c | 108 +++-
 drivers/net/ethernet/broadcom/bgmac-bcma.c  |   6 +-
 drivers/net/ethernet/broadcom/bgmac-platform.c  |   2 +-
 drivers/net/ethernet/broadcom/bgmac.c   |  24 --
 drivers/net/ethernet/broadcom/bgmac.h   |   5 +-
 5 files changed, 72 insertions(+), 73 deletions(-)

-- 
2.11.0

[PATCH V2 3/3] net: bgmac: use PHY subsystem for initializing PHY

[Rafał Miłecki]

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

This adds support for using bgmac with PHYs supported by standalone PHY
drivers. Having any PHY initialization in bgmac is hacky and shouldn't
be extended but rather removed if anyone has hardware to test it.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
 drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c 
b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
index 9d9984999dce..6ce80cbcb48e 100644
--- a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
+++ b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
@@ -132,6 +132,10 @@ static void bcma_mdio_phy_init(struct bgmac *bgmac)
struct bcma_chipinfo *ci = >bcma.core->bus->chipinfo;
u8 i;
 
+   /* For some legacy hardware we do chipset-based PHY initialization here
+* without even detecting PHY ID. It's hacky and should be cleaned as
+* soon as someone can test it.
+*/
if (ci->id == BCMA_CHIP_ID_BCM5356) {
for (i = 0; i < 5; i++) {
bcma_mdio_phy_write(bgmac, i, 0x1f, 0x008b);
@@ -140,6 +144,7 @@ static void bcma_mdio_phy_init(struct bgmac *bgmac)
bcma_mdio_phy_write(bgmac, i, 0x12, 0x2aaa);
bcma_mdio_phy_write(bgmac, i, 0x1f, 0x000b);
}
+   return;
}
if ((ci->id == BCMA_CHIP_ID_BCM5357 && ci->pkg != 10) ||
(ci->id == BCMA_CHIP_ID_BCM4749 && ci->pkg != 10) ||
@@ -161,7 +166,12 @@ static void bcma_mdio_phy_init(struct bgmac *bgmac)
bcma_mdio_phy_write(bgmac, i, 0x17, 0x9273);
bcma_mdio_phy_write(bgmac, i, 0x1f, 0x000b);
}
+   return;
}
+
+   /* For all other hw do initialization using PHY subsystem. */
+   if (bgmac->net_dev && bgmac->net_dev->phydev)
+   phy_init_hw(bgmac->net_dev->phydev);
 }
 
 /* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipphyreset */
-- 
2.11.0

[PATCH net-next] net: add devm version of alloc_etherdev_mqs function

[Rafał Miłecki]

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

This patch adds devm_alloc_etherdev_mqs function and devm_alloc_etherdev
macro. These can be used for simpler netdev allocation without having to
care about calling free_netdev.

Thanks to this change drivers, their error paths and removal paths may
get simpler by a bit.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
I'm working on V2 of:
[PATCH 0/3] net-next: use one struct bgmac & add PHY support
and I just realized I could get a way simpler error path in bgmac with a
devm_alloc_etherdev helper. So there is my suggestion for adding it.
---
 include/linux/etherdevice.h |  5 +
 net/ethernet/eth.c  | 28 
 2 files changed, 33 insertions(+)

diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index 42add77ae47d..c62b709b1ce0 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h
@@ -54,6 +54,11 @@ struct net_device *alloc_etherdev_mqs(int sizeof_priv, 
unsigned int txqs,
 #define alloc_etherdev(sizeof_priv) alloc_etherdev_mq(sizeof_priv, 1)
 #define alloc_etherdev_mq(sizeof_priv, count) alloc_etherdev_mqs(sizeof_priv, 
count, count)
 
+struct net_device *devm_alloc_etherdev_mqs(struct device *dev, int sizeof_priv,
+  unsigned int txqs,
+  unsigned int rxqs);
+#define devm_alloc_etherdev(dev, sizeof_priv) devm_alloc_etherdev_mqs(dev, 
sizeof_priv, 1, 1)
+
 struct sk_buff **eth_gro_receive(struct sk_buff **head,
 struct sk_buff *skb);
 int eth_gro_complete(struct sk_buff *skb, int nhoff);
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 8c5a479681ca..efdaaab735fc 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -392,6 +392,34 @@ struct net_device *alloc_etherdev_mqs(int sizeof_priv, 
unsigned int txqs,
 }
 EXPORT_SYMBOL(alloc_etherdev_mqs);
 
+static void devm_free_netdev(struct device *dev, void *res)
+{
+   free_netdev(*(struct net_device **)res);
+}
+
+struct net_device *devm_alloc_etherdev_mqs(struct device *dev, int sizeof_priv,
+  unsigned int txqs, unsigned int rxqs)
+{
+   struct net_device **dr;
+   struct net_device *netdev;
+
+   dr = devres_alloc(devm_free_netdev, sizeof(*dr), GFP_KERNEL);
+   if (!dr)
+   return NULL;
+
+   netdev = alloc_etherdev_mqs(sizeof_priv, txqs, rxqs);
+   if (!netdev) {
+   devres_free(dr);
+   return NULL;
+   }
+
+   *dr = netdev;
+   devres_add(dev, dr);
+
+   return netdev;
+}
+EXPORT_SYMBOL(devm_alloc_etherdev_mqs);
+
 ssize_t sysfs_format_mac(char *buf, const unsigned char *addr, int len)
 {
return scnprintf(buf, PAGE_SIZE, "%*phC\n", len, addr);
-- 
2.11.0

Re: [PATCH 1/3] net: bgmac: allocate struct bgmac just once & don't copy it

[Rafał Miłecki]

On 27 January 2017 at 17:14, David Miller <da...@davemloft.net> wrote:
> From: Felix Fietkau <n...@nbd.name>
> Date: Fri, 27 Jan 2017 17:02:33 +0100
>
>> On 2017-01-27 10:20, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <ra...@milecki.pl>
>>>
>>> To share as much code as possible in bgmac we call alloc_etherdev from
>>> bgmac.c which is used by both: platform and bcma code. The easiest
>>> solution was to use it for allocating whole struct bgmac but it doesn't
>>> work well as we already get early-filled struct bgmac as an argument.
>>>
>>> So far we were solving this by copying received struct into newly
>>> allocated one. The problem is it means storing 2 allocated structs,
>>> using only 1 of them and non-shared code not having access to it.
>>>
>>> This patch solves it by using alloc_etherdev to allocate *pointer* for
>>> the already allocated struct. The only downside of this is we have to be
>>> careful when using netdev_priv.
>>>
>>> Another solution was to call alloc_etherdev in platform/bcma specific
>>> code but Jon advised against it due to sharing less code that way.
>> How does that lead to sharing less code?
>> I find this pointer indirection rather ugly and uncommon, and I think it
>> would be much cleaner to split the probe into bgmac_enet_alloc and
>> bgmac_enet_probe (with bgmac_enet_alloc calling alloc_etherdev and doing
>> basic setup).
>
> I agree, it would be so much better if bgmac_probe() and friends
> initialized a real bgmac object which was the private of a netdev
> struct, then passed that down into bgmac_enet_probe().

I'll work on V2, thanks.

-- 
Rafał

[PATCH V2 net-next] net: phy: broadcom: add support for BCM54210E

[Rafał Miłecki]

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

It's Broadcom PHY simply described as single-port
RGMII 10/100/1000BASE-T PHY. It requires disabling delay skew and GTXCLK
bits.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
V2: Include include/linux/brcmphy.h change I forgot to commit
---
 drivers/net/phy/broadcom.c | 34 +-
 include/linux/brcmphy.h|  1 +
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 97d1c057c0a1..794b9ec81ba5 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -30,6 +30,22 @@ MODULE_DESCRIPTION("Broadcom PHY driver");
 MODULE_AUTHOR("Maciej W. Rozycki");
 MODULE_LICENSE("GPL");
 
+static int bcm54210e_config_init(struct phy_device *phydev)
+{
+   int val;
+
+   val = bcm54xx_auxctl_read(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC);
+   val &= ~MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_SKEW_EN;
+   val |= MII_BCM54XX_AUXCTL_MISC_WREN;
+   bcm54xx_auxctl_write(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC, val);
+
+   val = bcm_phy_read_shadow(phydev, BCM54810_SHD_CLK_CTL);
+   val &= ~BCM54810_SHD_CLK_CTL_GTXCLK_EN;
+   bcm_phy_write_shadow(phydev, BCM54810_SHD_CLK_CTL, val);
+
+   return 0;
+}
+
 static int bcm54810_config(struct phy_device *phydev)
 {
int rc, val;
@@ -230,7 +246,11 @@ static int bcm54xx_config_init(struct phy_device *phydev)
(phydev->dev_flags & PHY_BRCM_AUTO_PWRDWN_ENABLE))
bcm54xx_adjust_rxrefclk(phydev);
 
-   if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810) {
+   if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54210E) {
+   err = bcm54210e_config_init(phydev);
+   if (err)
+   return err;
+   } else if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810) {
err = bcm54810_config(phydev);
if (err)
return err;
@@ -542,6 +562,17 @@ static struct phy_driver broadcom_drivers[] = {
.ack_interrupt  = bcm_phy_ack_intr,
.config_intr= bcm_phy_config_intr,
 }, {
+   .phy_id = PHY_ID_BCM54210E,
+   .phy_id_mask= 0xfff0,
+   .name   = "Broadcom BCM54210E",
+   .features   = PHY_GBIT_FEATURES,
+   .flags  = PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
+   .config_init= bcm54xx_config_init,
+   .config_aneg= genphy_config_aneg,
+   .read_status= genphy_read_status,
+   .ack_interrupt  = bcm_phy_ack_intr,
+   .config_intr= bcm_phy_config_intr,
+}, {
.phy_id = PHY_ID_BCM5461,
.phy_id_mask= 0xfff0,
.name   = "Broadcom BCM5461",
@@ -680,6 +711,7 @@ module_phy_driver(broadcom_drivers);
 static struct mdio_device_id __maybe_unused broadcom_tbl[] = {
{ PHY_ID_BCM5411, 0xfff0 },
{ PHY_ID_BCM5421, 0xfff0 },
+   { PHY_ID_BCM54210E, 0xfff0 },
{ PHY_ID_BCM5461, 0xfff0 },
{ PHY_ID_BCM54612E, 0xfff0 },
{ PHY_ID_BCM54616S, 0xfff0 },
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index cf93f1399d3e..5881d1fdc1fb 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -17,6 +17,7 @@
 #define PHY_ID_BCM5482 0x0143bcb0
 #define PHY_ID_BCM5411 0x00206070
 #define PHY_ID_BCM5421 0x002060e0
+#define PHY_ID_BCM54210E   0x600d84a0
 #define PHY_ID_BCM5464 0x002060b0
 #define PHY_ID_BCM5461 0x002060c0
 #define PHY_ID_BCM54612E   0x03625e60
-- 
2.11.0

[PATCH] net: phy: broadcom: add support for BCM54210E

[Rafał Miłecki]

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

It's Broadcom PHY simply described as single-port
RGMII 10/100/1000BASE-T PHY. It requires disabling delay skew and GTXCLK
bits.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
 drivers/net/phy/broadcom.c | 34 +-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 97d1c057c0a1..794b9ec81ba5 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -30,6 +30,22 @@ MODULE_DESCRIPTION("Broadcom PHY driver");
 MODULE_AUTHOR("Maciej W. Rozycki");
 MODULE_LICENSE("GPL");
 
+static int bcm54210e_config_init(struct phy_device *phydev)
+{
+   int val;
+
+   val = bcm54xx_auxctl_read(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC);
+   val &= ~MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_SKEW_EN;
+   val |= MII_BCM54XX_AUXCTL_MISC_WREN;
+   bcm54xx_auxctl_write(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC, val);
+
+   val = bcm_phy_read_shadow(phydev, BCM54810_SHD_CLK_CTL);
+   val &= ~BCM54810_SHD_CLK_CTL_GTXCLK_EN;
+   bcm_phy_write_shadow(phydev, BCM54810_SHD_CLK_CTL, val);
+
+   return 0;
+}
+
 static int bcm54810_config(struct phy_device *phydev)
 {
int rc, val;
@@ -230,7 +246,11 @@ static int bcm54xx_config_init(struct phy_device *phydev)
(phydev->dev_flags & PHY_BRCM_AUTO_PWRDWN_ENABLE))
bcm54xx_adjust_rxrefclk(phydev);
 
-   if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810) {
+   if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54210E) {
+   err = bcm54210e_config_init(phydev);
+   if (err)
+   return err;
+   } else if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810) {
err = bcm54810_config(phydev);
if (err)
return err;
@@ -542,6 +562,17 @@ static struct phy_driver broadcom_drivers[] = {
.ack_interrupt  = bcm_phy_ack_intr,
.config_intr= bcm_phy_config_intr,
 }, {
+   .phy_id = PHY_ID_BCM54210E,
+   .phy_id_mask= 0xfff0,
+   .name   = "Broadcom BCM54210E",
+   .features   = PHY_GBIT_FEATURES,
+   .flags  = PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
+   .config_init= bcm54xx_config_init,
+   .config_aneg= genphy_config_aneg,
+   .read_status= genphy_read_status,
+   .ack_interrupt  = bcm_phy_ack_intr,
+   .config_intr= bcm_phy_config_intr,
+}, {
.phy_id = PHY_ID_BCM5461,
.phy_id_mask= 0xfff0,
.name   = "Broadcom BCM5461",
@@ -680,6 +711,7 @@ module_phy_driver(broadcom_drivers);
 static struct mdio_device_id __maybe_unused broadcom_tbl[] = {
{ PHY_ID_BCM5411, 0xfff0 },
{ PHY_ID_BCM5421, 0xfff0 },
+   { PHY_ID_BCM54210E, 0xfff0 },
{ PHY_ID_BCM5461, 0xfff0 },
{ PHY_ID_BCM54612E, 0xfff0 },
{ PHY_ID_BCM54616S, 0xfff0 },
-- 
2.11.0

[PATCH 2/3] net: bgmac: drop struct bcma_mdio we don't need anymore

[Rafał Miłecki]

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

Adding struct bcma_mdio was a workaround for bcma code not having access
to the struct bgmac used in the core code. Now we don't duplicate this
struct we can just use it internally in bcma code.

This simplifies code & allows access to all bgmac driver details from
all places in bcma code.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
 drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c | 98 ++---
 drivers/net/ethernet/broadcom/bgmac-bcma.c  |  2 +-
 drivers/net/ethernet/broadcom/bgmac.h   |  2 +-
 3 files changed, 42 insertions(+), 60 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c 
b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
index 7c19c8e2bf91..9d9984999dce 100644
--- a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
+++ b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
@@ -12,11 +12,6 @@
 #include 
 #include "bgmac.h"
 
-struct bcma_mdio {
-   struct bcma_device *core;
-   u8 phyaddr;
-};
-
 static bool bcma_mdio_wait_value(struct bcma_device *core, u16 reg, u32 mask,
 u32 value, int timeout)
 {
@@ -37,7 +32,7 @@ static bool bcma_mdio_wait_value(struct bcma_device *core, 
u16 reg, u32 mask,
  * PHY ops
  **/
 
-static u16 bcma_mdio_phy_read(struct bcma_mdio *bcma_mdio, u8 phyaddr, u8 reg)
+static u16 bcma_mdio_phy_read(struct bgmac *bgmac, u8 phyaddr, u8 reg)
 {
struct bcma_device *core;
u16 phy_access_addr;
@@ -56,12 +51,12 @@ static u16 bcma_mdio_phy_read(struct bcma_mdio *bcma_mdio, 
u8 phyaddr, u8 reg)
BUILD_BUG_ON(BGMAC_PC_MCT_SHIFT != BCMA_GMAC_CMN_PC_MCT_SHIFT);
BUILD_BUG_ON(BGMAC_PC_MTE != BCMA_GMAC_CMN_PC_MTE);
 
-   if (bcma_mdio->core->id.id == BCMA_CORE_4706_MAC_GBIT) {
-   core = bcma_mdio->core->bus->drv_gmac_cmn.core;
+   if (bgmac->bcma.core->id.id == BCMA_CORE_4706_MAC_GBIT) {
+   core = bgmac->bcma.core->bus->drv_gmac_cmn.core;
phy_access_addr = BCMA_GMAC_CMN_PHY_ACCESS;
phy_ctl_addr = BCMA_GMAC_CMN_PHY_CTL;
} else {
-   core = bcma_mdio->core;
+   core = bgmac->bcma.core;
phy_access_addr = BGMAC_PHY_ACCESS;
phy_ctl_addr = BGMAC_PHY_CNTL;
}
@@ -87,7 +82,7 @@ static u16 bcma_mdio_phy_read(struct bcma_mdio *bcma_mdio, u8 
phyaddr, u8 reg)
 }
 
 /* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipphywr */
-static int bcma_mdio_phy_write(struct bcma_mdio *bcma_mdio, u8 phyaddr, u8 reg,
+static int bcma_mdio_phy_write(struct bgmac *bgmac, u8 phyaddr, u8 reg,
   u16 value)
 {
struct bcma_device *core;
@@ -95,12 +90,12 @@ static int bcma_mdio_phy_write(struct bcma_mdio *bcma_mdio, 
u8 phyaddr, u8 reg,
u16 phy_ctl_addr;
u32 tmp;
 
-   if (bcma_mdio->core->id.id == BCMA_CORE_4706_MAC_GBIT) {
-   core = bcma_mdio->core->bus->drv_gmac_cmn.core;
+   if (bgmac->bcma.core->id.id == BCMA_CORE_4706_MAC_GBIT) {
+   core = bgmac->bcma.core->bus->drv_gmac_cmn.core;
phy_access_addr = BCMA_GMAC_CMN_PHY_ACCESS;
phy_ctl_addr = BCMA_GMAC_CMN_PHY_CTL;
} else {
-   core = bcma_mdio->core;
+   core = bgmac->bcma.core;
phy_access_addr = BGMAC_PHY_ACCESS;
phy_ctl_addr = BGMAC_PHY_CNTL;
}
@@ -110,8 +105,8 @@ static int bcma_mdio_phy_write(struct bcma_mdio *bcma_mdio, 
u8 phyaddr, u8 reg,
tmp |= phyaddr;
bcma_write32(core, phy_ctl_addr, tmp);
 
-   bcma_write32(bcma_mdio->core, BGMAC_INT_STATUS, BGMAC_IS_MDIO);
-   if (bcma_read32(bcma_mdio->core, BGMAC_INT_STATUS) & BGMAC_IS_MDIO)
+   bcma_write32(bgmac->bcma.core, BGMAC_INT_STATUS, BGMAC_IS_MDIO);
+   if (bcma_read32(bgmac->bcma.core, BGMAC_INT_STATUS) & BGMAC_IS_MDIO)
dev_warn(>dev, "Error setting MDIO int\n");
 
tmp = BGMAC_PA_START;
@@ -132,39 +127,39 @@ static int bcma_mdio_phy_write(struct bcma_mdio 
*bcma_mdio, u8 phyaddr, u8 reg,
 }
 
 /* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipphyinit */
-static void bcma_mdio_phy_init(struct bcma_mdio *bcma_mdio)
+static void bcma_mdio_phy_init(struct bgmac *bgmac)
 {
-   struct bcma_chipinfo *ci = _mdio->core->bus->chipinfo;
+   struct bcma_chipinfo *ci = >bcma.core->bus->chipinfo;
u8 i;
 
if (ci->id == BCMA_CHIP_ID_BCM5356) {
for (i = 0; i < 5; i++) {
-   bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x008b);
-   bcma_mdio_phy_write(bcma_mdio, i, 0x15, 0x0100);
-   bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000f);
-   bcma_mdio_phy_write(bcma_mdio, i,

[PATCH 1/3] net: bgmac: allocate struct bgmac just once & don't copy it

[Rafał Miłecki]

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

To share as much code as possible in bgmac we call alloc_etherdev from
bgmac.c which is used by both: platform and bcma code. The easiest
solution was to use it for allocating whole struct bgmac but it doesn't
work well as we already get early-filled struct bgmac as an argument.

So far we were solving this by copying received struct into newly
allocated one. The problem is it means storing 2 allocated structs,
using only 1 of them and non-shared code not having access to it.

This patch solves it by using alloc_etherdev to allocate *pointer* for
the already allocated struct. The only downside of this is we have to be
careful when using netdev_priv.

Another solution was to call alloc_etherdev in platform/bcma specific
code but Jon advised against it due to sharing less code that way.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
 drivers/net/ethernet/broadcom/bgmac-platform.c |  2 +-
 drivers/net/ethernet/broadcom/bgmac.c  | 24 +++-
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c 
b/drivers/net/ethernet/broadcom/bgmac-platform.c
index 6f736c19872f..4fefd1a74fcb 100644
--- a/drivers/net/ethernet/broadcom/bgmac-platform.c
+++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
@@ -98,7 +98,7 @@ static void platform_bgmac_cmn_maskset32(struct bgmac *bgmac, 
u16 offset,
 
 static void bgmac_nicpm_speed_set(struct net_device *net_dev)
 {
-   struct bgmac *bgmac = netdev_priv(net_dev);
+   struct bgmac *bgmac = *(struct bgmac **)netdev_priv(net_dev);
u32 val;
 
if (!bgmac->plat.nicpm_base)
diff --git a/drivers/net/ethernet/broadcom/bgmac.c 
b/drivers/net/ethernet/broadcom/bgmac.c
index 0e066dc6b8cc..73d679337903 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -777,7 +777,7 @@ static void bgmac_write_mac_address(struct bgmac *bgmac, u8 
*addr)
 
 static void bgmac_set_rx_mode(struct net_device *net_dev)
 {
-   struct bgmac *bgmac = netdev_priv(net_dev);
+   struct bgmac *bgmac = *(struct bgmac **)netdev_priv(net_dev);
 
if (net_dev->flags & IFF_PROMISC)
bgmac_cmdcfg_maskset(bgmac, ~0, BGMAC_CMDCFG_PROM, true);
@@ -1112,7 +1112,7 @@ static void bgmac_chip_init(struct bgmac *bgmac)
 
 static irqreturn_t bgmac_interrupt(int irq, void *dev_id)
 {
-   struct bgmac *bgmac = netdev_priv(dev_id);
+   struct bgmac *bgmac = *(struct bgmac **)netdev_priv(dev_id);
 
u32 int_status = bgmac_read(bgmac, BGMAC_INT_STATUS);
int_status &= bgmac->int_mask;
@@ -1161,7 +1161,7 @@ static int bgmac_poll(struct napi_struct *napi, int 
weight)
 
 static int bgmac_open(struct net_device *net_dev)
 {
-   struct bgmac *bgmac = netdev_priv(net_dev);
+   struct bgmac *bgmac = *(struct bgmac **)netdev_priv(net_dev);
int err = 0;
 
bgmac_chip_reset(bgmac);
@@ -1191,7 +1191,7 @@ static int bgmac_open(struct net_device *net_dev)
 
 static int bgmac_stop(struct net_device *net_dev)
 {
-   struct bgmac *bgmac = netdev_priv(net_dev);
+   struct bgmac *bgmac = *(struct bgmac **)netdev_priv(net_dev);
 
netif_carrier_off(net_dev);
 
@@ -1210,7 +1210,7 @@ static int bgmac_stop(struct net_device *net_dev)
 static netdev_tx_t bgmac_start_xmit(struct sk_buff *skb,
struct net_device *net_dev)
 {
-   struct bgmac *bgmac = netdev_priv(net_dev);
+   struct bgmac *bgmac = *(struct bgmac **)netdev_priv(net_dev);
struct bgmac_dma_ring *ring;
 
/* No QOS support yet */
@@ -1220,7 +1220,7 @@ static netdev_tx_t bgmac_start_xmit(struct sk_buff *skb,
 
 static int bgmac_set_mac_address(struct net_device *net_dev, void *addr)
 {
-   struct bgmac *bgmac = netdev_priv(net_dev);
+   struct bgmac *bgmac = *(struct bgmac **)netdev_priv(net_dev);
int ret;
 
ret = eth_prepare_mac_addr_change(net_dev, addr);
@@ -1356,7 +1356,7 @@ static void bgmac_get_strings(struct net_device *dev, u32 
stringset,
 static void bgmac_get_ethtool_stats(struct net_device *dev,
struct ethtool_stats *ss, uint64_t *data)
 {
-   struct bgmac *bgmac = netdev_priv(dev);
+   struct bgmac *bgmac = *(struct bgmac **)netdev_priv(dev);
const struct bgmac_stat *s;
unsigned int i;
u64 val;
@@ -1396,7 +1396,7 @@ static const struct ethtool_ops bgmac_ethtool_ops = {
 
 void bgmac_adjust_link(struct net_device *net_dev)
 {
-   struct bgmac *bgmac = netdev_priv(net_dev);
+   struct bgmac *bgmac = *(struct bgmac **)netdev_priv(net_dev);
struct phy_device *phy_dev = net_dev->phydev;
bool update = false;
 
@@ -1446,21 +1446,19 @@ int bgmac_phy_connect_direct(struct bgmac *bgmac)
 }
 EXPORT_SYMBOL_GPL(bgmac_phy_connect_direct);
 
-int bgmac_enet_probe(struct bgmac *info)
+int bgmac_enet_probe(struct

[PATCH 3/3] net: bgmac: use PHY subsystem for initializing PHY

[Rafał Miłecki]

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

This adds support for using bgmac with PHYs supported by standalone PHY
drivers. Having any PHY initialization in bgmac is hacky and shouldn't
be extended but rather removed if anyone has hardware to test it.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
 drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c 
b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
index 9d9984999dce..6ce80cbcb48e 100644
--- a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
+++ b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
@@ -132,6 +132,10 @@ static void bcma_mdio_phy_init(struct bgmac *bgmac)
struct bcma_chipinfo *ci = >bcma.core->bus->chipinfo;
u8 i;
 
+   /* For some legacy hardware we do chipset-based PHY initialization here
+* without even detecting PHY ID. It's hacky and should be cleaned as
+* soon as someone can test it.
+*/
if (ci->id == BCMA_CHIP_ID_BCM5356) {
for (i = 0; i < 5; i++) {
bcma_mdio_phy_write(bgmac, i, 0x1f, 0x008b);
@@ -140,6 +144,7 @@ static void bcma_mdio_phy_init(struct bgmac *bgmac)
bcma_mdio_phy_write(bgmac, i, 0x12, 0x2aaa);
bcma_mdio_phy_write(bgmac, i, 0x1f, 0x000b);
}
+   return;
}
if ((ci->id == BCMA_CHIP_ID_BCM5357 && ci->pkg != 10) ||
(ci->id == BCMA_CHIP_ID_BCM4749 && ci->pkg != 10) ||
@@ -161,7 +166,12 @@ static void bcma_mdio_phy_init(struct bgmac *bgmac)
bcma_mdio_phy_write(bgmac, i, 0x17, 0x9273);
bcma_mdio_phy_write(bgmac, i, 0x1f, 0x000b);
}
+   return;
}
+
+   /* For all other hw do initialization using PHY subsystem. */
+   if (bgmac->net_dev && bgmac->net_dev->phydev)
+   phy_init_hw(bgmac->net_dev->phydev);
 }
 
 /* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipphyreset */
-- 
2.11.0

[PATCH 0/3] net-next: use one struct bgmac & add PHY support

[Rafał Miłecki]

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

This patchset adds support for initializing PHY using PHY subsystem.
It's required e.g. for wireless access point devices that use bgmac
supported Ethernet device connected to some external PHY.

Implementing this required accessing phydev in bcma specific code which
wasn't possible with core code allocating struct bgmac on its own. This
is why I needed to modify alloc_etherdev usage first.

Rafał Miłecki (3):
  net: bgmac: allocate struct bgmac just once & don't copy it
  net: bgmac: drop struct bcma_mdio we don't need anymore
  net: bgmac: use PHY subsystem for initializing PHY

 drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c | 108 +++-
 drivers/net/ethernet/broadcom/bgmac-bcma.c  |   2 +-
 drivers/net/ethernet/broadcom/bgmac-platform.c  |   2 +-
 drivers/net/ethernet/broadcom/bgmac.c   |  24 +++---
 drivers/net/ethernet/broadcom/bgmac.h   |   2 +-
 5 files changed, 64 insertions(+), 74 deletions(-)

-- 
2.11.0

[PATCH V2 1/3] net: phy: broadcom: use auxctl reading helper in BCM54612E code

[Rafał Miłecki]

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

Starting with commit 5b4e29005123 ("net: phy: broadcom: add
bcm54xx_auxctl_read") we have a reading helper so use it and avoid code
duplication.
It also means we don't need MII_BCM54XX_AUXCTL_SHDWSEL_MISC define as
it's the same as MII_BCM54XX_AUXCTL_SHDWSEL_MISC just for reading needs
(same value shifted by 12 bits).

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
Reviewed-by: Florian Fainelli <f.faine...@gmail.com>
---
 drivers/net/phy/broadcom.c | 6 ++
 include/linux/brcmphy.h| 1 -
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 4223e35490b0..25c6e6cea2dc 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -395,10 +395,8 @@ static int bcm54612e_config_aneg(struct phy_device *phydev)
(phydev->interface != PHY_INTERFACE_MODE_RGMII_RXID)) {
u16 reg;
 
-   /* Errata: reads require filling in the write selector field */
-   bcm54xx_auxctl_write(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
-MII_BCM54XX_AUXCTL_MISC_RDSEL_MISC);
-   reg = phy_read(phydev, MII_BCM54XX_AUX_CTL);
+   reg = bcm54xx_auxctl_read(phydev,
+ MII_BCM54XX_AUXCTL_SHDWSEL_MISC);
/* Disable RXD to RXC delay (default set) */
reg &= ~MII_BCM54XX_AUXCTL_MISC_RXD_RXC_SKEW;
/* Clear shadow selector field */
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index 295fb3e73de5..34e61004b9dc 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -111,7 +111,6 @@
 #define MII_BCM54XX_AUXCTL_MISC_WREN   0x8000
 #define MII_BCM54XX_AUXCTL_MISC_RXD_RXC_SKEW   0x0100
 #define MII_BCM54XX_AUXCTL_MISC_FORCE_AMDIX0x0200
-#define MII_BCM54XX_AUXCTL_MISC_RDSEL_MISC 0x7000
 #define MII_BCM54XX_AUXCTL_SHDWSEL_MISC0x0007
 #define MII_BCM54XX_AUXCTL_SHDWSEL_READ_SHIFT  12
 #define MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_SKEW_EN  (1 << 8)
-- 
2.11.0

[PATCH V2 3/3] net: phy: bcm-phy-lib: clean up remaining AUXCTL register defines

[Rafał Miłecki]

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

1) Use 0x%02x format for register number. This follows some other
   defines and makes it easier to distinct register from values.
2) Put register define above values and sort the values. It makes
   reading header code easier.
3) Use 0x%04x format for all values. It's about consistency with other
   values (and most of the header) not a personal preference.
4) Separate define for reading shift value with an extre empty line.
   It's user for all AUXCTL registers in a bcm54xx_auxctl_read.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
V2: Don't drop SHDWSEL_ as it seems to be used in datasheet. Thanks Florian.
---
 include/linux/brcmphy.h | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index f9cb73df127e..cf93f1399d3e 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -104,17 +104,17 @@
 /*
  * AUXILIARY CONTROL SHADOW ACCESS REGISTERS.  (PHY REG 0x18)
  */
-#define MII_BCM54XX_AUXCTL_SHDWSEL_AUXCTL  0x
+#define MII_BCM54XX_AUXCTL_SHDWSEL_AUXCTL  0x00
 #define MII_BCM54XX_AUXCTL_ACTL_TX_6DB 0x0400
 #define MII_BCM54XX_AUXCTL_ACTL_SMDSP_ENA  0x0800
 
-#define MII_BCM54XX_AUXCTL_MISC_WREN   0x8000
-#define MII_BCM54XX_AUXCTL_MISC_FORCE_AMDIX0x0200
-#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC0x0007
-#define MII_BCM54XX_AUXCTL_SHDWSEL_READ_SHIFT  12
-#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_SKEW_EN  (1 << 8)
-#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC_WIRESPEED_EN   (1 << 4)
+#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC0x07
+#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC_WIRESPEED_EN   0x0010
+#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_SKEW_EN  0x0100
+#define MII_BCM54XX_AUXCTL_MISC_FORCE_AMDIX0x0200
+#define MII_BCM54XX_AUXCTL_MISC_WREN   0x8000
 
+#define MII_BCM54XX_AUXCTL_SHDWSEL_READ_SHIFT  12
 #define MII_BCM54XX_AUXCTL_SHDWSEL_MASK0x0007
 
 /*
-- 
2.11.0

[PATCH V2 0/3] net-next: Broadcom PHY driver cleanup

[Rafał Miłecki]

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

I will probably need to use broadcom.ko for PHY connected to interface
of bgmac supported device so I started looking at it willing to
understand it better.

I found AUXCTL part of the driver / lib a bit confusing and hard to read
so I'm trying to clean it up a bit. I hope this patchset makes following
AUXCTL operations much easier making it clear which defines are for
registers and which for values.

There is no functional change in this pachset.

Rafał Miłecki (3):
  net: phy: broadcom: use auxctl reading helper in BCM54612E code
  net: phy: broadcom: drop duplicated define for RGMII SKEW delay
  net: phy: bcm-phy-lib: clean up remaining AUXCTL register defines

 drivers/net/phy/broadcom.c |  8 +++-
 include/linux/brcmphy.h| 16 +++-
 2 files changed, 10 insertions(+), 14 deletions(-)

-- 
2.11.0

[PATCH V2 2/3] net: phy: broadcom: drop duplicated define for RGMII SKEW delay

[Rafał Miłecki]

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

We had two defines for the same bit (both were used with the
MII_BCM54XX_AUXCTL_SHDWSEL_MISC register).

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
V2: Drop the other define to match datasheet. Thanks Florian.
---
 drivers/net/phy/broadcom.c | 2 +-
 include/linux/brcmphy.h| 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 25c6e6cea2dc..97d1c057c0a1 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -398,7 +398,7 @@ static int bcm54612e_config_aneg(struct phy_device *phydev)
reg = bcm54xx_auxctl_read(phydev,
  MII_BCM54XX_AUXCTL_SHDWSEL_MISC);
/* Disable RXD to RXC delay (default set) */
-   reg &= ~MII_BCM54XX_AUXCTL_MISC_RXD_RXC_SKEW;
+   reg &= ~MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_SKEW_EN;
/* Clear shadow selector field */
reg &= ~MII_BCM54XX_AUXCTL_SHDWSEL_MASK;
bcm54xx_auxctl_write(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index 34e61004b9dc..f9cb73df127e 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -109,7 +109,6 @@
 #define MII_BCM54XX_AUXCTL_ACTL_SMDSP_ENA  0x0800
 
 #define MII_BCM54XX_AUXCTL_MISC_WREN   0x8000
-#define MII_BCM54XX_AUXCTL_MISC_RXD_RXC_SKEW   0x0100
 #define MII_BCM54XX_AUXCTL_MISC_FORCE_AMDIX0x0200
 #define MII_BCM54XX_AUXCTL_SHDWSEL_MISC0x0007
 #define MII_BCM54XX_AUXCTL_SHDWSEL_READ_SHIFT  12
-- 
2.11.0

[PATCH 3/3] net: phy: bcm-phy-lib: clean up remaining AUXCTL register defines

[Rafał Miłecki]

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

1) Use 0x%02x format for register number. This follows some other
   defines and makes it easier to distinct register from values.
2) Put register define above values and sort the values. It makes
   reading header code easier.
3) Drop SHDWSEL_ name part from the only value define using it. For all
   other values we just start with MISC_.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
 drivers/net/phy/bcm-phy-lib.c | 6 +++---
 include/linux/brcmphy.h   | 8 
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index ab9ad689617c..8d7e21a9fa77 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -241,7 +241,7 @@ int bcm_phy_downshift_get(struct phy_device *phydev, u8 
*count)
return val;
 
/* Check if wirespeed is enabled or not */
-   if (!(val & MII_BCM54XX_AUXCTL_SHDWSEL_MISC_WIRESPEED_EN)) {
+   if (!(val & MII_BCM54XX_AUXCTL_MISC_WIRESPEED_EN)) {
*count = DOWNSHIFT_DEV_DISABLE;
return 0;
}
@@ -283,12 +283,12 @@ int bcm_phy_downshift_set(struct phy_device *phydev, u8 
count)
val |= MII_BCM54XX_AUXCTL_MISC_WREN;
 
if (count == DOWNSHIFT_DEV_DISABLE) {
-   val &= ~MII_BCM54XX_AUXCTL_SHDWSEL_MISC_WIRESPEED_EN;
+   val &= ~MII_BCM54XX_AUXCTL_MISC_WIRESPEED_EN;
return bcm54xx_auxctl_write(phydev,
MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
val);
} else {
-   val |= MII_BCM54XX_AUXCTL_SHDWSEL_MISC_WIRESPEED_EN;
+   val |= MII_BCM54XX_AUXCTL_MISC_WIRESPEED_EN;
ret = bcm54xx_auxctl_write(phydev,
   MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
   val);
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index bff53da82b58..a79d57caf7f1 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -104,16 +104,16 @@
 /*
  * AUXILIARY CONTROL SHADOW ACCESS REGISTERS.  (PHY REG 0x18)
  */
-#define MII_BCM54XX_AUXCTL_SHDWSEL_AUXCTL  0x
+#define MII_BCM54XX_AUXCTL_SHDWSEL_AUXCTL  0x00
 #define MII_BCM54XX_AUXCTL_ACTL_TX_6DB 0x0400
 #define MII_BCM54XX_AUXCTL_ACTL_SMDSP_ENA  0x0800
 
-#define MII_BCM54XX_AUXCTL_MISC_WREN   0x8000
+#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC0x07
+#define MII_BCM54XX_AUXCTL_MISC_WIRESPEED_EN   0x0010
 #define MII_BCM54XX_AUXCTL_MISC_RXD_RXC_SKEW   0x0100
 #define MII_BCM54XX_AUXCTL_MISC_FORCE_AMDIX0x0200
-#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC0x0007
+#define MII_BCM54XX_AUXCTL_MISC_WREN   0x8000
 #define MII_BCM54XX_AUXCTL_SHDWSEL_READ_SHIFT  12
-#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC_WIRESPEED_EN   (1 << 4)
 
 #define MII_BCM54XX_AUXCTL_SHDWSEL_MASK0x0007
 
-- 
2.11.0

[PATCH 2/3] net: phy: broadcom: drop duplicated define for RXD to RXC delay

[Rafał Miłecki]

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

We had two defines for the same bit (both were used with the
MII_BCM54XX_AUXCTL_SHDWSEL_MISC register).

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
 drivers/net/phy/broadcom.c | 2 +-
 include/linux/brcmphy.h| 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 25c6e6cea2dc..9b7c2d57ca92 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -42,7 +42,7 @@ static int bcm54810_config(struct phy_device *phydev)
return rc;
 
val = bcm54xx_auxctl_read(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC);
-   val &= ~MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_SKEW_EN;
+   val &= ~MII_BCM54XX_AUXCTL_MISC_RXD_RXC_SKEW;
val |= MII_BCM54XX_AUXCTL_MISC_WREN;
rc = bcm54xx_auxctl_write(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
  val);
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index 34e61004b9dc..bff53da82b58 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -113,7 +113,6 @@
 #define MII_BCM54XX_AUXCTL_MISC_FORCE_AMDIX0x0200
 #define MII_BCM54XX_AUXCTL_SHDWSEL_MISC0x0007
 #define MII_BCM54XX_AUXCTL_SHDWSEL_READ_SHIFT  12
-#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_SKEW_EN  (1 << 8)
 #define MII_BCM54XX_AUXCTL_SHDWSEL_MISC_WIRESPEED_EN   (1 << 4)
 
 #define MII_BCM54XX_AUXCTL_SHDWSEL_MASK0x0007
-- 
2.11.0

[PATCH 1/3] net: phy: broadcom: use auxctl reading helper in BCM54612E code

[Rafał Miłecki]

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

Starting with commit 5b4e29005123 ("net: phy: broadcom: add
bcm54xx_auxctl_read") we have a reading helper so use it and avoid code
duplication.
It also means we don't need MII_BCM54XX_AUXCTL_SHDWSEL_MISC define as
it's the same as MII_BCM54XX_AUXCTL_SHDWSEL_MISC just for reading needs
(same value shifted by 12 bits).

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
 drivers/net/phy/broadcom.c | 6 ++
 include/linux/brcmphy.h| 1 -
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 4223e35490b0..25c6e6cea2dc 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -395,10 +395,8 @@ static int bcm54612e_config_aneg(struct phy_device *phydev)
(phydev->interface != PHY_INTERFACE_MODE_RGMII_RXID)) {
u16 reg;
 
-   /* Errata: reads require filling in the write selector field */
-   bcm54xx_auxctl_write(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
-MII_BCM54XX_AUXCTL_MISC_RDSEL_MISC);
-   reg = phy_read(phydev, MII_BCM54XX_AUX_CTL);
+   reg = bcm54xx_auxctl_read(phydev,
+ MII_BCM54XX_AUXCTL_SHDWSEL_MISC);
/* Disable RXD to RXC delay (default set) */
reg &= ~MII_BCM54XX_AUXCTL_MISC_RXD_RXC_SKEW;
/* Clear shadow selector field */
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index 295fb3e73de5..34e61004b9dc 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -111,7 +111,6 @@
 #define MII_BCM54XX_AUXCTL_MISC_WREN   0x8000
 #define MII_BCM54XX_AUXCTL_MISC_RXD_RXC_SKEW   0x0100
 #define MII_BCM54XX_AUXCTL_MISC_FORCE_AMDIX0x0200
-#define MII_BCM54XX_AUXCTL_MISC_RDSEL_MISC 0x7000
 #define MII_BCM54XX_AUXCTL_SHDWSEL_MISC0x0007
 #define MII_BCM54XX_AUXCTL_SHDWSEL_READ_SHIFT  12
 #define MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_SKEW_EN  (1 << 8)
-- 
2.11.0

[PATCH net] net: bgmac: fix reversed checks for clock control flag

[Rafał Miłecki]

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

This fixes regression introduced by patch adding feature flags. It was
already reported and patch followed (it got accepted) but it appears it
was incorrect. Instead of fixing reversed condition it broke a good one.

This patch was verified to actually fix SoC hanges caused by bgmac on
BCM47186B0.

Fixes: db791eb2970b ("net: ethernet: bgmac: convert to feature flags")
Fixes: 4af1474e6198 ("net: bgmac: Fix errant feature flag check")
Cc: Jon Mason <jon.ma...@broadcom.com>
Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
It seems 4af1474e6198 was never backported to the 4.8, so I'm not Cc-ing
stable. I'll send separated patch for 4.8 once this one gets accepted.
---
 drivers/net/ethernet/broadcom/bgmac.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c 
b/drivers/net/ethernet/broadcom/bgmac.c
index 91cbf92..49f4cafe 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1049,9 +1049,9 @@ static void bgmac_enable(struct bgmac *bgmac)
 
mode = (bgmac_read(bgmac, BGMAC_DEV_STATUS) & BGMAC_DS_MM_MASK) >>
BGMAC_DS_MM_SHIFT;
-   if (!(bgmac->feature_flags & BGMAC_FEAT_CLKCTLST) || mode != 0)
+   if (bgmac->feature_flags & BGMAC_FEAT_CLKCTLST || mode != 0)
bgmac_set(bgmac, BCMA_CLKCTLST, BCMA_CLKCTLST_FORCEHT);
-   if (bgmac->feature_flags & BGMAC_FEAT_CLKCTLST && mode == 2)
+   if (!(bgmac->feature_flags & BGMAC_FEAT_CLKCTLST) && mode == 2)
bgmac_cco_ctl_maskset(bgmac, 1, ~0,
  BGMAC_CHIPCTL_1_RXC_DLL_BYPASS);
 
-- 
2.10.1

Re: [PATCH v6 5/7] net: ethernet: bgmac: device tree phy enablement

[Rafał Miłecki]

On 2016-11-04 06:11, Jon Mason wrote:

Change the bgmac driver to allow for phy's defined by the device tree

Signed-off-by: Jon Mason <jon.ma...@broadcom.com>


[Resending from Roundcube due to "Wrong MIME labeling on 8-bit character 
texts."

vger.kernel.org reject when using Thunderbird]

Checked for possible regressions on Northstar, looks OK.

Acked-by: Rafał Miłecki <ra...@milecki.pl>

Re: [PATCH v6 6/7] net: ethernet: bgmac: add NS2 support

[Rafał Miłecki]

On 2016-11-04 06:11, Jon Mason wrote:

Add support for the variant of amac hardware present in the Broadcom
Northstar2 based SoCs.  Northstar2 requires an additional register to 
be

configured with the port speed/duplexity (NICPM).  This can be added to
the link callback to hide it from the instances that do not use this.
Also, clearing of the pending interrupts on init is required due to
observed issues on some platforms.

Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
Reviewed-by: Florian Fainelli <f.faine...@gmail.com>


[Resending from Roundcube due to "Wrong MIME labeling on 8-bit character 
texts."

vger.kernel.org reject when using Thunderbird]

Checked for possible regressions on Northstar, looks OK.

Acked-by: Rafał Miłecki <ra...@milecki.pl>

Re: [PATCH v2] rtl8xxxu: Fix for agressive power saving by rtl8723bu wireless IC

[Rafał Miłecki]

On 1 November 2016 at 08:24, John Heenan  wrote:
> @@ -5779,6 +5779,12 @@ static int rtl8xxxu_start(struct ieee80211_hw *hw)
>
> ret = 0;
>
> +   if(priv->fops == _fops) {

OK, let me be the first. Documentation/CodingStyle also says to use
space between "if" and "(" ;)


> @@ -6080,9 +6086,11 @@ static int rtl8xxxu_probe(struct usb_interface 
> *interface,
> goto exit;
> }
>
> -   ret = rtl8xxxu_init_device(hw);
> -   if (ret)
> -   goto exit;
> +   if(priv->fops != _fops) {

Same here.

I reviewed style only.

-- 
Rafał

[PATCH] brcmfmac: print name of connect status event

[Rafał Miłecki]

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

This simplifies debugging. Format %s (%u) comes from similar debugging
message in brcmf_fweh_event_worker.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 3 ++-
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c | 4 ++--
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.h | 2 ++
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index b777e1b..1e7c6f0 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -5506,7 +5506,8 @@ brcmf_notify_connect_status_ap(struct brcmf_cfg80211_info 
*cfg,
u32 reason = e->reason;
struct station_info sinfo;
 
-   brcmf_dbg(CONN, "event %d, reason %d\n", event, reason);
+   brcmf_dbg(CONN, "event %s (%u), reason %d\n",
+ brcmf_fweh_event_name(event), event, reason);
if (event == BRCMF_E_LINK && reason == BRCMF_E_REASON_LINK_BSSCFG_DIS &&
ndev != cfg_to_ndev(cfg)) {
brcmf_dbg(CONN, "AP mode link down\n");
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
index 79c081f..c79306b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
@@ -69,7 +69,7 @@ static struct brcmf_fweh_event_name fweh_event_names[] = {
  *
  * @code: code to lookup.
  */
-static const char *brcmf_fweh_event_name(enum brcmf_fweh_event_code code)
+const char *brcmf_fweh_event_name(enum brcmf_fweh_event_code code)
 {
int i;
for (i = 0; i < ARRAY_SIZE(fweh_event_names); i++) {
@@ -79,7 +79,7 @@ static const char *brcmf_fweh_event_name(enum 
brcmf_fweh_event_code code)
return "unknown";
 }
 #else
-static const char *brcmf_fweh_event_name(enum brcmf_fweh_event_code code)
+const char *brcmf_fweh_event_name(enum brcmf_fweh_event_code code)
 {
return "nodebug";
 }
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.h 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.h
index 26ff5a9..5fba4b4 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.h
@@ -287,6 +287,8 @@ struct brcmf_fweh_info {
 void *data);
 };
 
+const char *brcmf_fweh_event_name(enum brcmf_fweh_event_code code);
+
 void brcmf_fweh_attach(struct brcmf_pub *drvr);
 void brcmf_fweh_detach(struct brcmf_pub *drvr);
 int brcmf_fweh_register(struct brcmf_pub *drvr, enum brcmf_fweh_event_code 
code,
-- 
2.9.3

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

[Rafał Miłecki]

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

I believe I get easily irritated due to my communication experience I
got so far :(


Over a year ago I reported brcmfmac can't recover from failed
register_netdev(ice). This bug remains unfixed.

In 2014 I reported problem with 80 MHz support. I didn't have hardware
to fix & test it on my own (you weren't able/allowed to send me one of
your PCIe cards). In remained broken until I fixed it year later.

You missed my crash bug report about caused by missing eth_type_trans
and came with patch on your own a month later.

Earlier this year I reported you problem with BCM4366 and multiple
interfaces. I didn't get much help. 3 months later I came with patch
to workaround the problem but you said there's a better way to do
this. It took me 2 weeks to figure out a new wlioctl API for that
while all I needed was a simple hint on "interface_remove".

Right now I'm waiting to get any answer from you about 4366c0
firmware. It's still less than 2 weeks since I asked for it, but a
simple ETA would be nice. I'm actually not sure if I should report
more problems to you to don't distract you from pending things.

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


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

-- 
Rafał

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

[Rafał Miłecki]

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

Flowrings contain skbs waiting for transmission that were passed to us
by netif. It means we checked every one of them looking for 802.1x
Ethernet type. When deleting flowring we have to use freeing function
that will check for 802.1x type as well.

Freeing skbs without a proper check was leading to counter not being
properly decreased. This was triggering a WARNING every time
brcmf_netdev_wait_pend8021x was called.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
Acked-by: Arend van Spriel <ar...@broadcom.com>
Cc: sta...@vger.kernel.org # 4.5+
---
V2: Add Cc for stable 4.5+. It doesn't apply cleanly to 4.4 and is not
possible for 4.3- due to missing brcmf_get_ifp.
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c
index b16b367..d0b738d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c
@@ -234,13 +234,20 @@ static void brcmf_flowring_block(struct brcmf_flowring 
*flow, u16 flowid,
 
 void brcmf_flowring_delete(struct brcmf_flowring *flow, u16 flowid)
 {
+   struct brcmf_bus *bus_if = dev_get_drvdata(flow->dev);
struct brcmf_flowring_ring *ring;
+   struct brcmf_if *ifp;
u16 hash_idx;
+   u8 ifidx;
struct sk_buff *skb;
 
ring = flow->rings[flowid];
if (!ring)
return;
+
+   ifidx = brcmf_flowring_ifidx_get(flow, flowid);
+   ifp = brcmf_get_ifp(bus_if->drvr, ifidx);
+
brcmf_flowring_block(flow, flowid, false);
hash_idx = ring->hash_id;
flow->hash[hash_idx].ifidx = BRCMF_FLOWRING_INVALID_IFIDX;
@@ -249,7 +256,7 @@ void brcmf_flowring_delete(struct brcmf_flowring *flow, u16 
flowid)
 
skb = skb_dequeue(>skblist);
while (skb) {
-   brcmu_pkt_buf_free_skb(skb);
+   brcmf_txfinalize(ifp, skb, false);
skb = skb_dequeue(>skblist);
}
 
-- 
2.9.3

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

[Rafał Miłecki]

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

Some projects may work on their own stable kernels, e.g. Ubuntu, see:
https://wiki.ubuntu.com/Kernel/Dev/ExtendedStable

That's why I don't always look strictly at upstream stable releases only.

-- 
Rafał

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

[Rafał Miłecki]

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

Let me see if patchwork with pick Cc tag as it does for others.

Cc: sta...@vger.kernel.org # 4.5+

This may be worth backporting to 4.4 as well (as it's longterm), but
I'll do it separately due to patch not applying cleanly.

-- 
Rafał

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

[Rafał Miłecki]

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

I was analyzing this.
1) This patch uses brcmf_get_ifp which is available in 4.4+ only.
2) It applies cleanly to 4.5+ only due to 32f90caa7debd ("brcmfmac:
Increase nr of supported flowrings.")
3) 4.4 would also require applying to the patch without broadcom/ subdir

That said I suggest 4.5+. Any objections?

-- 
Rafał

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

[Rafał Miłecki]

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

Even with timeout increased to 950 ms we get WARNINGs from time to time.
It mostly happens on A-MPDU stalls (e.g. when station goes out of
range). It may take up to 5-10 secods for the firmware to recover and
for that time it doesn't process packets.

It's still useful to have a message on time out as it may indicate some
firmware problem and incorrect key update. Raising a WARNING however
wasn't really that necessary, it doesn't point to any driver bug anymore
and backtrace wasn't much useful.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 6d046ba..9e6f60a 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -1161,7 +1161,8 @@ int brcmf_netdev_wait_pend8021x(struct brcmf_if *ifp)
 !brcmf_get_pend_8021x_cnt(ifp),
 MAX_WAIT_FOR_8021X_TX);
 
-   WARN_ON(!err);
+   if (!err)
+   brcmf_err("Timed out waiting for no pending 802.1x packets\n");
 
return !err;
 }
-- 
2.9.3

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

[Rafał Miłecki]

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

Flowrings contain skbs waiting for transmission that were passed to us
by netif. It means we checked every one of them looking for 802.1x
Ethernet type. When deleting flowring we have to use freeing function
that will check for 802.1x type as well.

Freeing skbs without a proper check was leading to counter not being
properly decreased. This was triggering a WARNING every time
brcmf_netdev_wait_pend8021x was called.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
Kalle: this isn't important enough for 4.8 as it's too late for that.

I'd like to get it for 4.9 however, as this fixes bug that could lead
to WARNING on every add_key/del_key call. We was struggling with these
WARNINGs for some time and this fixes one of two problems causing them.
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c
index b16b367..d0b738d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c
@@ -234,13 +234,20 @@ static void brcmf_flowring_block(struct brcmf_flowring 
*flow, u16 flowid,
 
 void brcmf_flowring_delete(struct brcmf_flowring *flow, u16 flowid)
 {
+   struct brcmf_bus *bus_if = dev_get_drvdata(flow->dev);
struct brcmf_flowring_ring *ring;
+   struct brcmf_if *ifp;
u16 hash_idx;
+   u8 ifidx;
struct sk_buff *skb;
 
ring = flow->rings[flowid];
if (!ring)
return;
+
+   ifidx = brcmf_flowring_ifidx_get(flow, flowid);
+   ifp = brcmf_get_ifp(bus_if->drvr, ifidx);
+
brcmf_flowring_block(flow, flowid, false);
hash_idx = ring->hash_id;
flow->hash[hash_idx].ifidx = BRCMF_FLOWRING_INVALID_IFIDX;
@@ -249,7 +256,7 @@ void brcmf_flowring_delete(struct brcmf_flowring *flow, u16 
flowid)
 
skb = skb_dequeue(>skblist);
while (skb) {
-   brcmu_pkt_buf_free_skb(skb);
+   brcmf_txfinalize(ifp, skb, false);
skb = skb_dequeue(>skblist);
}
 
-- 
2.9.3

[PATCH] brcmfmac: proto: add callback for queuing TX data

[Rafał Miłecki]

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

So far our core code was calling brcmf_fws_process_skb which wasn't
a proper thing to do. If case of devices using msgbuf protocol fwsignal
shouldn't be used. It was an unnecessary extra layer simply calling
a protocol specifix txdata function.

Please note we already have txdata callback, but it's used for calls
between bcdc and fwsignal so it couldn't be simply used there.

This makes core code more generic (instead of bcdc/fwsignal specific).

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
There is one 1 CHECK report from checkpatch.pl in this patch:
CHECK: Comparison to NULL could be written "!proto->hdrpull"
#157: FILE: drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.c:54:
+   if (!proto->tx_queue_data || (proto->hdrpull == NULL) ||

It's caused by a code that was already there and that should be fixed
in a separated patch. This shouldn't stop this patch from being applied.
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c   | 12 
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c   |  8 +++-
 .../net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c   | 15 +--
 .../net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.h   |  1 +
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c |  6 +++---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.c  |  2 +-
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h  |  9 +
 7 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
index 038a960..384b187 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
@@ -326,6 +326,17 @@ brcmf_proto_bcdc_hdrpull(struct brcmf_pub *drvr, bool 
do_fws,
return 0;
 }
 
+static int brcmf_proto_bcdc_tx_queue_data(struct brcmf_pub *drvr, int ifidx,
+ struct sk_buff *skb)
+{
+   struct brcmf_if *ifp = brcmf_get_ifp(drvr, ifidx);
+
+   if (!brcmf_fws_queue_skbs(drvr->fws))
+   return brcmf_proto_txdata(drvr, ifidx, 0, skb);
+
+   return brcmf_fws_process_skb(ifp, skb);
+}
+
 static int
 brcmf_proto_bcdc_txdata(struct brcmf_pub *drvr, int ifidx, u8 offset,
struct sk_buff *pktbuf)
@@ -375,6 +386,7 @@ int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr)
drvr->proto->hdrpull = brcmf_proto_bcdc_hdrpull;
drvr->proto->query_dcmd = brcmf_proto_bcdc_query_dcmd;
drvr->proto->set_dcmd = brcmf_proto_bcdc_set_dcmd;
+   drvr->proto->tx_queue_data = brcmf_proto_bcdc_tx_queue_data;
drvr->proto->txdata = brcmf_proto_bcdc_txdata;
drvr->proto->configure_addr_mode = brcmf_proto_bcdc_configure_addr_mode;
drvr->proto->delete_peer = brcmf_proto_bcdc_delete_peer;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 1715280..6d046ba 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -239,7 +239,13 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff 
*skb,
if (eh->h_proto == htons(ETH_P_PAE))
atomic_inc(>pend_8021x_cnt);
 
-   ret = brcmf_fws_process_skb(ifp, skb);
+   /* determine the priority */
+   if ((skb->priority == 0) || (skb->priority > 7))
+   skb->priority = cfg80211_classify8021d(skb, NULL);
+
+   ret = brcmf_proto_tx_queue_data(drvr, ifp->ifidx, skb);
+   if (ret < 0)
+   brcmf_txfinalize(ifp, skb, false);
 
 done:
if (ret) {
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
index a190f53..5f1a592 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
@@ -2100,16 +2100,6 @@ int brcmf_fws_process_skb(struct brcmf_if *ifp, struct 
sk_buff *skb)
int rc = 0;
 
brcmf_dbg(DATA, "tx proto=0x%X\n", ntohs(eh->h_proto));
-   /* determine the priority */
-   if ((skb->priority == 0) || (skb->priority > 7))
-   skb->priority = cfg80211_classify8021d(skb, NULL);
-
-   if (fws->avoid_queueing) {
-   rc = brcmf_proto_txdata(drvr, ifp->ifidx, 0, skb);
-   if (rc < 0)
-   brcmf_txfinalize(ifp, skb, false);
-   return rc;
-   }
 
/* set control buffer information */
skcb->if_flags = 0;
@@ -2442,6 +2432,11 @@ void brcmf_fws_deinit(struct brcmf_pub *drvr)
kfree(fws);
 }
 
+bool brcmf_fws_queue_skbs(struct brcmf_fws_info *fws)
+{
+   re

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

[Rafał Miłecki]

On 26 September 2016 at 14:13, Rafał Miłecki <zaj...@gmail.com> wrote:
> On 26 September 2016 at 13:46, Arend Van Spriel
> <arend.vanspr...@broadcom.com> wrote:
>> On 26-9-2016 12:23, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <ra...@milecki.pl>
>>>
>>> We need to track 802.1x packets to know if there are any pending ones
>>> for transmission. This is required for performing key update in the
>>> firmware.
>>
>> The problem we are trying to solve is a pretty old one. The problem is
>> that wpa_supplicant uses two separate code paths: EAPOL messaging
>> through data path and key configuration though nl80211.
>
> Can I find it described/reported somewhere?
>
>
>>> Unfortunately our old tracking code wasn't very accurate. It was
>>> treating skb as pending as soon as it was passed by the netif. Actual
>>> handling packet to the firmware was happening later as brcmfmac
>>> internally queues them and uses its own worker(s).
>>
>> That does not seem right. As soon as we get a 1x packet we need to wait
>> with key configuration regardless whether it is still in the driver or
>> handed over to firmware already.
>
> OK, thanks.

Actually, it's not OK. I was trying to report/describe/discuss this
problem for over a week. I couldn't get much of answer from you.

I had to come with a patch I worked on for quite some time. Only then
you decided to react and reply with a reason for a nack. I see this
patch may be wrong (but it's still hard to know what's going wrong
without a proper hostapd bug report). I'd expect you to somehow work &
communicate with open source community.

-- 
Rafał

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

[Rafał Miłecki]

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

Can I find it described/reported somewhere?


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

OK, thanks.

[PATCH] brcmfmac: implement more accurate skb tracking

[Rafał Miłecki]

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

We need to track 802.1x packets to know if there are any pending ones
for transmission. This is required for performing key update in the
firmware.

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

Other than that it was hard to handle freeing packets. Everytime we had
to determine (in more generic funcions) if packet was counted as pending
802.1x one or not. It was causing some problems, e.g. it wasn't clear if
brcmf_flowring_delete should free skb directly or not.

This patch introduces 2 separated functions for tracking skbs. This
simplifies logic, fixes brcmf_flowring_delete (maybe other hidden bugs
as well) and allows further simplifications. Thanks to better accuracy
is also increases time window for key update (and lowers timeout risk).

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
This was successfully tested with 4366b1. Can someone give it a try with
some USB/SDIO device, please?
---
 .../wireless/broadcom/brcm80211/brcmfmac/bcdc.c| 11 +++
 .../wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c  | 12 +++-
 .../wireless/broadcom/brcm80211/brcmfmac/core.c| 36 --
 .../wireless/broadcom/brcm80211/brcmfmac/core.h|  2 ++
 .../wireless/broadcom/brcm80211/brcmfmac/msgbuf.c  | 14 +++--
 .../wireless/broadcom/brcm80211/brcmfmac/proto.h   | 11 +++
 .../wireless/broadcom/brcm80211/brcmfmac/sdio.c|  8 +
 .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 10 ++
 8 files changed, 91 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
index d1bc51f..3e40244 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
@@ -326,6 +326,16 @@ brcmf_proto_bcdc_hdrpull(struct brcmf_pub *drvr, bool 
do_fws,
return 0;
 }
 
+static int brcmf_proto_bcdc_hdr_get_ifidx(struct brcmf_pub *drvr,
+ struct sk_buff *skb)
+{
+   struct brcmf_proto_bcdc_header *h;
+
+   h = (struct brcmf_proto_bcdc_header *)(skb->data);
+
+   return BCDC_GET_IF_IDX(h);
+}
+
 static int
 brcmf_proto_bcdc_txdata(struct brcmf_pub *drvr, int ifidx, u8 offset,
struct sk_buff *pktbuf)
@@ -373,6 +383,7 @@ int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr)
}
 
drvr->proto->hdrpull = brcmf_proto_bcdc_hdrpull;
+   drvr->proto->hdr_get_ifidx = brcmf_proto_bcdc_hdr_get_ifidx;
drvr->proto->query_dcmd = brcmf_proto_bcdc_query_dcmd;
drvr->proto->set_dcmd = brcmf_proto_bcdc_set_dcmd;
drvr->proto->txdata = brcmf_proto_bcdc_txdata;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index 03404cb..fef9d02 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -43,6 +43,7 @@
 #include "chip.h"
 #include "bus.h"
 #include "debug.h"
+#include "proto.h"
 #include "sdio.h"
 #include "core.h"
 #include "common.h"
@@ -772,6 +773,7 @@ int brcmf_sdiod_send_buf(struct brcmf_sdio_dev *sdiodev, u8 
*buf, uint nbytes)
 int brcmf_sdiod_send_pkt(struct brcmf_sdio_dev *sdiodev,
 struct sk_buff_head *pktq)
 {
+   struct brcmf_pub *pub = sdiodev->bus_if->drvr;
struct sk_buff *skb;
u32 addr = sdiodev->sbwad;
int err;
@@ -784,10 +786,18 @@ int brcmf_sdiod_send_pkt(struct brcmf_sdio_dev *sdiodev,
 
if (pktq->qlen == 1 || !sdiodev->sg_support)
skb_queue_walk(pktq, skb) {
+   struct brcmf_if *ifp;
+   int ifidx;
+
+   ifidx = brcmf_proto_hdr_get_ifidx(pub, skb);
+   ifp = brcmf_get_ifp(pub, ifidx);
+   brcmf_tx_passing_skb(ifp, skb);
err = brcmf_sdiod_buffrw(sdiodev, SDIO_FUNC_2, true,
 addr, skb);
-   if (err)
+   if (err) {
+   brcmf_tx_regained_skb(ifp, skb);
break;
+   }
}
else
err = brcmf_sdiod_sglist_rw(sdiodev, SDIO_FUNC_2, true, addr,
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index bc3d8ab..7cdc1f6 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80

[PATCH 2/2] brcmfmac: compile fws(ignal) code only with BCDC support enabled

[Rafał Miłecki]

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

It's not needed by the other (msgbuf) protocol, so let's save some size
and compile it conditionally.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
 .../wireless/broadcom/brcm80211/brcmfmac/Makefile  |  4 +-
 .../broadcom/brcm80211/brcmfmac/fwsignal.h | 59 ++
 2 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile
index 9e4b505..ad3b06e 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile
@@ -27,7 +27,6 @@ brcmfmac-objs += \
chip.o \
fwil.o \
fweh.o \
-   fwsignal.o \
p2p.o \
proto.o \
common.o \
@@ -37,7 +36,8 @@ brcmfmac-objs += \
btcoex.o \
vendor.o
 brcmfmac-$(CONFIG_BRCMFMAC_PROTO_BCDC) += \
-   bcdc.o
+   bcdc.o \
+   fwsignal.o
 brcmfmac-$(CONFIG_BRCMFMAC_PROTO_MSGBUF) += \
commonring.o \
flowring.o \
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.h 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.h
index 8f7c1d7..ba0c1bc 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.h
@@ -18,6 +18,7 @@
 #ifndef FWSIGNAL_H_
 #define FWSIGNAL_H_
 
+#ifdef CONFIG_BRCMFMAC_PROTO_BCDC
 int brcmf_fws_init(struct brcmf_pub *drvr);
 void brcmf_fws_deinit(struct brcmf_pub *drvr);
 bool brcmf_fws_skbs_queueing(struct brcmf_fws_info *fws);
@@ -31,5 +32,63 @@ void brcmf_fws_del_interface(struct brcmf_if *ifp);
 void brcmf_fws_bustxfail(struct brcmf_fws_info *fws, struct sk_buff *skb);
 void brcmf_fws_bus_blocked(struct brcmf_pub *drvr, bool flow_blocked);
 void brcmf_fws_rxreorder(struct brcmf_if *ifp, struct sk_buff *skb);
+#else
+static inline int brcmf_fws_init(struct brcmf_pub *drvr)
+{
+   return -ENOTSUPP;
+}
+
+static inline void brcmf_fws_deinit(struct brcmf_pub *drvr)
+{
+}
+
+static inline bool brcmf_fws_skbs_queueing(struct brcmf_fws_info *fws)
+{
+   return false;
+}
+
+static inline bool brcmf_fws_fc_active(struct brcmf_fws_info *fws)
+{
+   return false;
+}
+
+static inline void brcmf_fws_hdrpull(struct brcmf_if *ifp, s16 siglen,
+struct sk_buff *skb)
+{
+}
+
+static inline int brcmf_fws_process_skb(struct brcmf_if *ifp,
+   struct sk_buff *skb)
+{
+   return -ENOTSUPP;
+}
+
+static inline void brcmf_fws_reset_interface(struct brcmf_if *ifp)
+{
+}
+
+static inline void brcmf_fws_add_interface(struct brcmf_if *ifp)
+{
+}
+
+static inline void brcmf_fws_del_interface(struct brcmf_if *ifp)
+{
+}
+
+static inline void brcmf_fws_bustxfail(struct brcmf_fws_info *fws,
+  struct sk_buff *skb)
+{
+}
+
+static inline void brcmf_fws_bus_blocked(struct brcmf_pub *drvr,
+bool flow_blocked)
+{
+}
+
+static inline void brcmf_fws_rxreorder(struct brcmf_if *ifp,
+  struct sk_buff *skb)
+{
+}
+#endif
 
 #endif /* FWSIGNAL_H_ */
-- 
2.9.3

[PATCH 1/2] brcmfmac: initialize fws(ignal) for BCDC protocol only

[Rafał Miłecki]

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

There are two protocols used by Broadcom FullMAC devices: BCDC and
msgbuf. They use different ways for (some part of) communication with
the firmware. Firmware Signaling is required for the first one only
(BCDC).

So far we were always initializing fws and always calling it's skb
processing function. It was fws that was passing skb processing to the
protocol specific function. It was redundant for the msgbuf case.

Simply taking few lines of code out of fws allows us to totally avoid
using it. This simplifies code flow, saves some memory & will allow
further optimizations like not compiling fwsignal.c.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
 .../wireless/broadcom/brcm80211/brcmfmac/core.c| 24 --
 .../broadcom/brcm80211/brcmfmac/fwsignal.c | 17 ++-
 .../broadcom/brcm80211/brcmfmac/fwsignal.h |  1 +
 3 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 27cd50a..bc3d8ab 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -250,7 +250,17 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff 
*skb,
if (eh->h_proto == htons(ETH_P_PAE))
atomic_inc(>pend_8021x_cnt);
 
-   ret = brcmf_fws_process_skb(ifp, skb);
+   /* determine the priority */
+   if (skb->priority == 0 || skb->priority > 7)
+   skb->priority = cfg80211_classify8021d(skb, NULL);
+
+   if (drvr->fws && brcmf_fws_skbs_queueing(drvr->fws)) {
+   ret = brcmf_fws_process_skb(ifp, skb);
+   } else {
+   ret = brcmf_proto_txdata(drvr, ifp->ifidx, 0, skb);
+   if (ret < 0)
+   brcmf_txfinalize(ifp, skb, false);
+   }
 
 done:
if (ret) {
@@ -405,7 +415,7 @@ void brcmf_txcomplete(struct device *dev, struct sk_buff 
*txp, bool success)
struct brcmf_if *ifp;
 
/* await txstatus signal for firmware if active */
-   if (brcmf_fws_fc_active(drvr->fws)) {
+   if (drvr->fws && brcmf_fws_fc_active(drvr->fws)) {
if (!success)
brcmf_fws_bustxfail(drvr->fws, txp);
} else {
@@ -1006,11 +1016,13 @@ int brcmf_bus_start(struct device *dev)
}
brcmf_feat_attach(drvr);
 
-   ret = brcmf_fws_init(drvr);
-   if (ret < 0)
-   goto fail;
+   if (bus_if->proto_type == BRCMF_PROTO_BCDC) {
+   ret = brcmf_fws_init(drvr);
+   if (ret < 0)
+   goto fail;
 
-   brcmf_fws_add_interface(ifp);
+   brcmf_fws_add_interface(ifp);
+   }
 
drvr->config = brcmf_cfg80211_attach(drvr, bus_if->dev,
 drvr->settings->p2p_enable);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
index a190f53..495eaf8 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
@@ -2100,16 +2100,6 @@ int brcmf_fws_process_skb(struct brcmf_if *ifp, struct 
sk_buff *skb)
int rc = 0;
 
brcmf_dbg(DATA, "tx proto=0x%X\n", ntohs(eh->h_proto));
-   /* determine the priority */
-   if ((skb->priority == 0) || (skb->priority > 7))
-   skb->priority = cfg80211_classify8021d(skb, NULL);
-
-   if (fws->avoid_queueing) {
-   rc = brcmf_proto_txdata(drvr, ifp->ifidx, 0, skb);
-   if (rc < 0)
-   brcmf_txfinalize(ifp, skb, false);
-   return rc;
-   }
 
/* set control buffer information */
skcb->if_flags = 0;
@@ -2155,7 +2145,7 @@ void brcmf_fws_add_interface(struct brcmf_if *ifp)
struct brcmf_fws_info *fws = ifp->drvr->fws;
struct brcmf_fws_mac_descriptor *entry;
 
-   if (!ifp->ndev)
+   if (!fws || !ifp->ndev)
return;
 
entry = >desc.iface[ifp->ifidx];
@@ -2442,6 +2432,11 @@ void brcmf_fws_deinit(struct brcmf_pub *drvr)
kfree(fws);
 }
 
+bool brcmf_fws_skbs_queueing(struct brcmf_fws_info *fws)
+{
+   return !fws->avoid_queueing;
+}
+
 bool brcmf_fws_fc_active(struct brcmf_fws_info *fws)
 {
if (!fws->creditmap_received)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.h 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.h
index ef0ad85..8f7c1d7 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.h
@@ -20,6 +20,7 @@
 
 int brcmf_fws_init(struct b

[PATCH] brcmfmac: drop unused fields from struct brcmf_pub

[Rafał Miłecki]

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

They seem to be there from the first day. We calculate these values but
never use them.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 3 ---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h | 4 
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c | 2 --
 3 files changed, 9 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 65e8c87..27cd50a 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -519,9 +519,6 @@ int brcmf_net_attach(struct brcmf_if *ifp, bool rtnl_locked)
ndev->needed_headroom += drvr->hdrlen;
ndev->ethtool_ops = _ethtool_ops;
 
-   drvr->rxsz = ndev->mtu + ndev->hard_header_len +
- drvr->hdrlen;
-
/* set the mac address */
memcpy(ndev->dev_addr, ifp->mac_addr, ETH_ALEN);
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
index 8fa34ca..f16cfc9 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
@@ -112,15 +112,11 @@ struct brcmf_pub {
 
/* Internal brcmf items */
uint hdrlen;/* Total BRCMF header length (proto + bus) */
-   uint rxsz;  /* Rx buffer size bus module should use */
 
/* Dongle media info */
char fwver[BRCMF_DRIVER_FIRMWARE_VERSION_LEN];
u8 mac[ETH_ALEN];   /* MAC address obtained from dongle */
 
-   /* Multicast data packets sent to dongle */
-   unsigned long tx_multicast;
-
struct mac_address addresses[BRCMF_MAX_IFS];
 
struct brcmf_if *iflist[BRCMF_MAX_IFS];
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
index 9f9024a..a190f53 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
@@ -2104,8 +2104,6 @@ int brcmf_fws_process_skb(struct brcmf_if *ifp, struct 
sk_buff *skb)
if ((skb->priority == 0) || (skb->priority > 7))
skb->priority = cfg80211_classify8021d(skb, NULL);
 
-   drvr->tx_multicast += !!multicast;
-
if (fws->avoid_queueing) {
rc = brcmf_proto_txdata(drvr, ifp->ifidx, 0, skb);
if (rc < 0)
-- 
2.9.3

[PATCH] brcmfmac: fix memory leak in brcmf_fill_bss_param

[Rafał Miłecki]

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

This function is called from get_station callback which means that every
time user space was getting/dumping station(s) we were leaking 2 KiB.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
Fixes: 1f0dc59a6de ("brcmfmac: rework .get_station() callback")
Cc: sta...@vger.kernel.org # 4.2+
---
Kalle, ideally this should go as 4.8 fix, but I'm aware it's quite late.
If you are not planning to send another pull request, just get it for
the next release and let's let stable guys backport it.
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index b8aec5e5..62a7675 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -2533,7 +2533,7 @@ static void brcmf_fill_bss_param(struct brcmf_if *ifp, 
struct station_info *si)
 WL_BSS_INFO_MAX);
if (err) {
brcmf_err("Failed to get bss info (%d)\n", err);
-   return;
+   goto out_kfree;
}
si->filled |= BIT(NL80211_STA_INFO_BSS_PARAM);
si->bss_param.beacon_interval = le16_to_cpu(buf->bss_le.beacon_period);
@@ -2545,6 +2545,9 @@ static void brcmf_fill_bss_param(struct brcmf_if *ifp, 
struct station_info *si)
si->bss_param.flags |= BSS_PARAM_FLAGS_SHORT_PREAMBLE;
if (capability & WLAN_CAPABILITY_SHORT_SLOT_TIME)
si->bss_param.flags |= BSS_PARAM_FLAGS_SHORT_SLOT_TIME;
+
+out_kfree:
+   kfree(buf);
 }
 
 static s32
-- 
2.9.3

[PATCH RFC] brcmfmac: stop netif queue when waiting for packets transmission

[Rafał Miłecki]

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

Sending a new key to the firmware should be done without any 802.1x
packets pending. Currently brcmfmac has very trivial code waiting for
that condition and it doesn't seem to be enough.

We should stop netif from sending any extra packets in order to:
1) Make sure new 802.1x packets won't be coming over and over
2) Avoid a race with netif providing a new packet right after our
   waiting code

Another solution would be to accept only non-802.1x packets. This would
require enqueuing all packets and hacking brcmf_fws_dequeue_worker to
dequeue only non-802.1x ones but that would most likely result in too
hacky code.
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 201a980..1791060 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -471,11 +471,14 @@ send_key_to_dongle(struct brcmf_if *ifp, struct 
brcmf_wsec_key *key)
 
convert_key_from_CPU(key, _le);
 
+   netif_stop_queue(ifp->ndev);
brcmf_netdev_wait_pend8021x(ifp);
 
err = brcmf_fil_bsscfg_data_set(ifp, "wsec_key", _le,
sizeof(key_le));
 
+   netif_start_queue(ifp->ndev);
+
if (err)
brcmf_err("wsec_key error (%d)\n", err);
return err;
-- 
2.9.3

Re: [PATCH v2 4/6] net: ethernet: bgmac: convert to feature flags

[Rafał Miłecki]

On 17 August 2016 at 13:34, Rafał Miłecki <zaj...@gmail.com> wrote:
> On 8 July 2016 at 01:08, Jon Mason <jon.ma...@broadcom.com> wrote:
>> mode = (bgmac_read(bgmac, BGMAC_DEV_STATUS) & BGMAC_DS_MM_MASK) >>
>> BGMAC_DS_MM_SHIFT;
>> -   if (ci->id != BCMA_CHIP_ID_BCM47162 || mode != 0)
>> +   if (bgmac->feature_flags & BGMAC_FEAT_CLKCTLST || mode != 0)
>> bgmac_set(bgmac, BCMA_CLKCTLST, BCMA_CLKCTLST_FORCEHT);
>> -   if (ci->id == BCMA_CHIP_ID_BCM47162 && mode == 2)
>> +   if (bgmac->feature_flags & BGMAC_FEAT_CLKCTLST && mode == 2)
>> bcma_chipco_chipctl_maskset(>core->bus->drv_cc, 1, ~0,
>> BGMAC_CHIPCTL_1_RXC_DLL_BYPASS);
>
> Jon, it looks to me you translated two following conditions:
> ci->id != BCMA_CHIP_ID_BCM47162
> and
> ci->id == BCMA_CHIP_ID_BCM47162
> into the same flag check:
> bgmac->feature_flags & BGMAC_FEAT_CLKCTLST
>
> I don't think it's intentional, is it? Do you have a moment to fix this?

Ping

-- 
Rafał

Re: [PATCH v4] brcmfmac: add missing header dependencies

[Rafał Miłecki]

On 29 August 2016 at 17:42, Baoyou Xie <baoyou@linaro.org> wrote:
> On 29 August 2016 at 23:31, Rafał Miłecki <zaj...@gmail.com> wrote:
>>
>> On 29 August 2016 at 14:39, Baoyou Xie <baoyou@linaro.org> wrote:
>> > We get 1 warning when build kernel with W=1:
>> > drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c:23:6:
>> > warning: no previous prototype for '__brcmf_err' [-Wmissing-prototypes]
>>
>> building? I'm not native English, but I think so.
>>
>>
>> > In fact, this function is declared in brcmfmac/debug.h, so this patch
>> > add missing header dependencies.
>>
>> adds
>>
>>
>> > Signed-off-by: Baoyou Xie <baoyou@linaro.org>
>> > Acked-by: Arnd Bergmann <a...@arndb.de>
>>
>> Please don't resend patches just to add tags like that. This only
>> increases a noise and patchwork handles this just fine, see:
>> https://patchwork.kernel.org/patch/9303285/
>> https://patchwork.kernel.org/patch/9303285/mbox/
>
>
> it modifies a typo biuld/build.

Please send your replies to all, not privately.

OK, so you should also include a changelog in the comments part of diff, e.g.:
V4: Fix typo in "biuld"

Re: [PATCH v4] brcmfmac: add missing header dependencies

[Rafał Miłecki]

On 29 August 2016 at 14:39, Baoyou Xie  wrote:
> We get 1 warning when build kernel with W=1:
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c:23:6: warning: 
> no previous prototype for '__brcmf_err' [-Wmissing-prototypes]

building? I'm not native English, but I think so.


> In fact, this function is declared in brcmfmac/debug.h, so this patch
> add missing header dependencies.

adds


> Signed-off-by: Baoyou Xie 
> Acked-by: Arnd Bergmann 

Please don't resend patches just to add tags like that. This only
increases a noise and patchwork handles this just fine, see:
https://patchwork.kernel.org/patch/9303285/
https://patchwork.kernel.org/patch/9303285/mbox/

Re: [PATCH 1/1] brcmfmac: fix pmksa->bssid usage

[Rafał Miłecki]

On 22 August 2016 at 15:03, Nicolas Iooss  wrote:
> After I sent the following patch a few weeks ago, I have not received
> any feedback. Could you please review it and tell me what I may have
> done wrong?

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#checking_state_of_patches_from_patchwork
https://patchwork.kernel.org/patch/9265733/

[PATCH net-next] net: bgmac: make it clear when setting interface type to RMII

[Rafał Miłecki]

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

It doesn't really change anything as BGMAC_CHIPCTL_1_IF_TYPE_RMII is
equal to 0. It make code a bit clener, so far when reading it one could
think we forgot to set a proper mode. It also keeps this mode code in
sync with other ones.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
 drivers/net/ethernet/broadcom/bgmac.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c 
b/drivers/net/ethernet/broadcom/bgmac.c
index 63ef7235..6ea0e5f 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -932,7 +932,8 @@ static void bgmac_chip_reset(struct bgmac *bgmac)
et_swtype <<= 4;
sw_type = et_swtype;
} else if (bgmac->feature_flags & BGMAC_FEAT_SW_TYPE_EPHYRMII) {
-   sw_type = BGMAC_CHIPCTL_1_SW_TYPE_EPHYRMII;
+   sw_type = BGMAC_CHIPCTL_1_IF_TYPE_RMII |
+ BGMAC_CHIPCTL_1_SW_TYPE_EPHYRMII;
} else if (bgmac->feature_flags & BGMAC_FEAT_SW_TYPE_RGMII) {
sw_type = BGMAC_CHIPCTL_1_IF_TYPE_RGMII |
  BGMAC_CHIPCTL_1_SW_TYPE_RGMII;
-- 
1.8.4.5

[PATCH net-next] net: bgmac: support Ethernet core on BCM53573 SoCs

[Rafał Miłecki]

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

BCM53573 is a new series of Broadcom's SoCs. It's based on ARM and can
be found in two packages (versions): BCM53573 and BCM47189. It shares
some code with the Northstar family, but also requires some new quirks.

First of all there can be up to 2 Ethernet cores on this SoC. If that is
the case, they are connected to two different switch ports allowing some
more complex/optimized setups. It seems the second unit doesn't come
fully configured and requires some IRQ quirk.

Other than that only the first core is connected to the PHY. For the
second one we have to register fixed PHY (similarly to the Northstar),
otherwise generic PHY driver would get some invalid info.

This has been successfully tested on Tenda AC9 (BCM47189B0).

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
---
 drivers/net/ethernet/broadcom/bgmac-bcma.c | 19 ++-
 drivers/net/ethernet/broadcom/bgmac.c  | 25 +
 drivers/net/ethernet/broadcom/bgmac.h  | 19 +++
 include/linux/bcma/bcma.h  |  3 +++
 include/linux/bcma/bcma_regs.h |  1 +
 5 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma.c 
b/drivers/net/ethernet/broadcom/bgmac-bcma.c
index 9a9745c4..3bc0a04 100644
--- a/drivers/net/ethernet/broadcom/bgmac-bcma.c
+++ b/drivers/net/ethernet/broadcom/bgmac-bcma.c
@@ -92,6 +92,7 @@ MODULE_DEVICE_TABLE(bcma, bgmac_bcma_tbl);
 /* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipattach */
 static int bgmac_probe(struct bcma_device *core)
 {
+   struct bcma_chipinfo *ci = >bus->chipinfo;
struct ssb_sprom *sprom = >bus->sprom;
struct mii_bus *mii_bus;
struct bgmac *bgmac;
@@ -157,7 +158,8 @@ static int bgmac_probe(struct bcma_device *core)
dev_info(bgmac->dev, "Found PHY addr: %d%s\n", bgmac->phyaddr,
 bgmac->phyaddr == BGMAC_PHY_NOREGS ? " (NOREGS)" : "");
 
-   if (!bgmac_is_bcm4707_family(core)) {
+   if (!bgmac_is_bcm4707_family(core) &&
+   !(ci->id == BCMA_CHIP_ID_BCM53573 && core->core_unit == 1)) {
mii_bus = bcma_mdio_mii_register(core, bgmac->phyaddr);
if (!IS_ERR(mii_bus)) {
err = PTR_ERR(mii_bus);
@@ -230,6 +232,21 @@ static int bgmac_probe(struct bcma_device *core)
bgmac->feature_flags |= BGMAC_FEAT_NO_RESET;
bgmac->feature_flags |= BGMAC_FEAT_FORCE_SPEED_2500;
break;
+   case BCMA_CHIP_ID_BCM53573:
+   bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
+   bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
+   if (ci->pkg == BCMA_PKG_ID_BCM47189)
+   bgmac->feature_flags |= BGMAC_FEAT_IOST_ATTACHED;
+   if (core->core_unit == 0) {
+   bgmac->feature_flags |= BGMAC_FEAT_CC4_IF_SW_TYPE;
+   if (ci->pkg == BCMA_PKG_ID_BCM47189)
+   bgmac->feature_flags |=
+   BGMAC_FEAT_CC4_IF_SW_TYPE_RGMII;
+   } else if (core->core_unit == 1) {
+   bgmac->feature_flags |= BGMAC_FEAT_IRQ_ID_OOB_6;
+   bgmac->feature_flags |= BGMAC_FEAT_CC7_IF_TYPE_RGMII;
+   }
+   break;
default:
bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
diff --git a/drivers/net/ethernet/broadcom/bgmac.c 
b/drivers/net/ethernet/broadcom/bgmac.c
index c4751ec..63ef7235 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -940,6 +940,27 @@ static void bgmac_chip_reset(struct bgmac *bgmac)
bgmac_cco_ctl_maskset(bgmac, 1, ~(BGMAC_CHIPCTL_1_IF_TYPE_MASK |
  BGMAC_CHIPCTL_1_SW_TYPE_MASK),
  sw_type);
+   } else if (bgmac->feature_flags & BGMAC_FEAT_CC4_IF_SW_TYPE) {
+   u32 sw_type = BGMAC_CHIPCTL_4_IF_TYPE_MII |
+ BGMAC_CHIPCTL_4_SW_TYPE_EPHY;
+   u8 et_swtype = 0;
+   char buf[4];
+
+   if (bcm47xx_nvram_getenv("et_swtype", buf, sizeof(buf)) > 0) {
+   if (kstrtou8(buf, 0, _swtype))
+   dev_err(bgmac->dev, "Failed to parse et_swtype 
(%s)\n",
+   buf);
+   sw_type = (et_swtype & 0x0f) << 12;
+   } else if (bgmac->feature_flags & 
BGMAC_FEAT_CC4_IF_SW_TYPE_RGMII) {
+   sw_type = BGMAC_CHIPCTL_4_IF_TYPE_RGMII |
+ BGMAC_CHIPCTL_4_SW_TYPE_R

[PATCH net] net: bgmac: fix reversed check for MII registration error

[Rafał Miłecki]

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

It was failing on successful registration returning meaningless errors.

Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
Fixes: 55954f3bfdac ("net: ethernet: bgmac: move BCMA MDIO Phy code into a 
separate file")
---
This fix is intendent for net repository (4.8 release).
---
 drivers/net/ethernet/broadcom/bgmac-bcma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma.c 
b/drivers/net/ethernet/broadcom/bgmac-bcma.c
index 9a9745c4..625235d 100644
--- a/drivers/net/ethernet/broadcom/bgmac-bcma.c
+++ b/drivers/net/ethernet/broadcom/bgmac-bcma.c
@@ -159,7 +159,7 @@ static int bgmac_probe(struct bcma_device *core)
 
if (!bgmac_is_bcm4707_family(core)) {
mii_bus = bcma_mdio_mii_register(core, bgmac->phyaddr);
-   if (!IS_ERR(mii_bus)) {
+   if (IS_ERR(mii_bus)) {
err = PTR_ERR(mii_bus);
goto err;
}
-- 
1.8.4.5

Re: [PATCH v2 4/6] net: ethernet: bgmac: convert to feature flags

[Rafał Miłecki]

On 8 July 2016 at 01:08, Jon Mason  wrote:
> mode = (bgmac_read(bgmac, BGMAC_DEV_STATUS) & BGMAC_DS_MM_MASK) >>
> BGMAC_DS_MM_SHIFT;
> -   if (ci->id != BCMA_CHIP_ID_BCM47162 || mode != 0)
> +   if (bgmac->feature_flags & BGMAC_FEAT_CLKCTLST || mode != 0)
> bgmac_set(bgmac, BCMA_CLKCTLST, BCMA_CLKCTLST_FORCEHT);
> -   if (ci->id == BCMA_CHIP_ID_BCM47162 && mode == 2)
> +   if (bgmac->feature_flags & BGMAC_FEAT_CLKCTLST && mode == 2)
> bcma_chipco_chipctl_maskset(>core->bus->drv_cc, 1, ~0,
> BGMAC_CHIPCTL_1_RXC_DLL_BYPASS);

Jon, it looks to me you translated two following conditions:
ci->id != BCMA_CHIP_ID_BCM47162
and
ci->id == BCMA_CHIP_ID_BCM47162
into the same flag check:
bgmac->feature_flags & BGMAC_FEAT_CLKCTLST

I don't think it's intentional, is it? Do you have a moment to fix this?