[PATCH 0/5] brcmfmac: network namespace support

[PATCH 0/5] brcmfmac: network namespace support

[Arend van Spriel]

This series intended for 4.12 includes:
- support for network namespace
- rework fwsignal module
- small fix for suspend error code path

Arend van Spriel (3):
  brcmfmac: add support to move wiphy instance into network namespace
  brcmfmac: restore bus state when enter_D3 fails
  brcmfmac: no need for d11inf instance in brcmf_pno_start_sched_scan()

Franky Lin (2):
  brcmfmac: wrap brcmf_fws_init into bcdc layer
  brcmfmac: move brcmf_fws_deinit to bcdc layer

 drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c     |  8 ++++++++
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c |  3 ++-
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c     | 12 +++---------
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c     |  1 +
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/pno.c      |  2 --
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h    |  9 +++++++++
 6 files changed, 23 insertions(+), 12 deletions(-)

-- 
1.9.1

[PATCH 1/5] brcmfmac: wrap brcmf_fws_init into bcdc layer

[Arend van Spriel]

From: Franky Lin <franky.lin@broadcom.com>

Create a new protocol layer interface brcmf_proto_init_cb for protocol
layer to finish initialzation after core module components(fweh and
etc.) are initialized.

Signed-off-by: Franky Lin <franky.lin@broadcom.com>
Change-Id: I560d2478a7c09766cf07b20d74b31dff5ca6ac7b
Reviewed-on: http://hnd-swgit.sj.broadcom.com:8080/8156
Reviewed-by: brcm80211 ci <brcm80211-ci@broadcom.com>
Reviewed-by: Arend Van Spriel <arend.vanspriel@broadcom.com>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c  | 7 +++++++
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c  | 2 +-
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h | 9 +++++++++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
index 92eafcc..bc24b00 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
@@ -417,6 +417,12 @@ static void brcmf_proto_bcdc_rxreorder(struct brcmf_if *ifp,
 	brcmf_fws_reset_interface(ifp);
 }
 
+static int
+brcmf_proto_bcdc_init_done(struct brcmf_pub *drvr)
+{
+	return brcmf_fws_init(drvr);
+}
+
 int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr)
 {
 	struct brcmf_bcdc *bcdc;
@@ -443,6 +449,7 @@ int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr)
 	drvr->proto->add_if = brcmf_proto_bcdc_add_if;
 	drvr->proto->del_if = brcmf_proto_bcdc_del_if;
 	drvr->proto->reset_if = brcmf_proto_bcdc_reset_if;
+	drvr->proto->init_done = brcmf_proto_bcdc_init_done;
 	drvr->proto->pd = bcdc;
 
 	drvr->hdrlen += BCDC_HEADER_LEN + BRCMF_PROT_FW_SIGNAL_MAX_TXBYTES;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 60c6c78..9886280 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -986,7 +986,7 @@ int brcmf_bus_started(struct device *dev)
 	}
 	brcmf_feat_attach(drvr);
 
-	ret = brcmf_fws_init(drvr);
+	ret = brcmf_proto_init_done(drvr);
 	if (ret < 0)
 		goto fail;
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h
index 3048ed5..600fd33 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h
@@ -47,6 +47,7 @@ struct brcmf_proto {
 	void (*add_if)(struct brcmf_if *ifp);
 	void (*del_if)(struct brcmf_if *ifp);
 	void (*reset_if)(struct brcmf_if *ifp);
+	int (*init_done)(struct brcmf_pub *drvr);
 	void *pd;
 };
 
@@ -145,4 +146,12 @@ static inline bool brcmf_proto_is_reorder_skb(struct sk_buff *skb)
 	drvr->proto->reset_if(ifp);
 }
 
+static inline int
+brcmf_proto_init_done(struct brcmf_pub *drvr)
+{
+	if (!drvr->proto->init_done)
+		return 0;
+	return drvr->proto->init_done(drvr);
+}
+
 #endif /* BRCMFMAC_PROTO_H */
-- 
1.9.1

[PATCH 2/5] brcmfmac: move brcmf_fws_deinit to bcdc layer

[Arend van Spriel]

From: Franky Lin <franky.lin@broadcom.com>

Move brcmf_fws_deinit into brcmf_proto_bcdc_detach since it is a bcdc
exclusive feature.

Signed-off-by: Franky Lin <franky.lin@broadcom.com>
Change-Id: Iefa5db632b92185a49d538b1cd25b7d8be618ce0
Reviewed-on: http://hnd-swgit.sj.broadcom.com:8080/8157
Reviewed-by: brcm80211 ci <brcm80211-ci@broadcom.com>
Reviewed-by: Arend Van Spriel <arend.vanspriel@broadcom.com>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c | 1 +
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 7 -------
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
index bc24b00..67a132c1 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
@@ -464,6 +464,7 @@ int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr)
 
 void brcmf_proto_bcdc_detach(struct brcmf_pub *drvr)
 {
+	brcmf_fws_deinit(drvr);
 	kfree(drvr->proto->pd);
 	drvr->proto->pd = NULL;
 }
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 9886280..ba4da48 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -32,7 +32,6 @@
 #include "p2p.h"
 #include "cfg80211.h"
 #include "fwil.h"
-#include "fwsignal.h"
 #include "feature.h"
 #include "proto.h"
 #include "pcie.h"
@@ -1034,10 +1033,6 @@ int brcmf_bus_started(struct device *dev)
 		brcmf_cfg80211_detach(drvr->config);
 		drvr->config = NULL;
 	}
-	if (drvr->fws) {
-		brcmf_proto_del_if(ifp->drvr, ifp);
-		brcmf_fws_deinit(drvr);
-	}
 	brcmf_net_detach(ifp->ndev, false);
 	if (p2p_ifp)
 		brcmf_net_detach(p2p_ifp->ndev, false);
@@ -1103,8 +1098,6 @@ void brcmf_detach(struct device *dev)
 
 	brcmf_cfg80211_detach(drvr->config);
 
-	brcmf_fws_deinit(drvr);
-
 	brcmf_bus_stop(drvr->bus_if);
 
 	brcmf_proto_detach(drvr);
-- 
1.9.1

[PATCH 3/5] brcmfmac: add support to move wiphy instance into network namespace

[Arend van Spriel]

To support network namespace the driver must assure all created
network interfaces are in the same namespace as the wiphy instance
and flag the support using WIPHY_FLAG_NETNS_OK.

Verified using two terminals:

 Terminal 1			Terminal 2
 --------------------------	---------------------------------
 # ip netns add brcm-wifi	# iw dev
				phy#0
					Interface wlan3
						ifindex 11
						wdev 0x1
 # ip netns exec brcm-wifi bash
 # iw dev
 # echo $$
 20337			# iw phy0 set netns 20337
 # iw dev
 phy#0
	Interface wlan3
		ifindex 11
		wdev 0x2
 # iw phy0 interface add wl3.ap type __ap
 # iw dev
 phy#0
	Interface wl3.ap
		ifindex 2
		wdev 0x3
	Interface wlan3
		ifindex 11
		wdev 0x2
				# iw dev
 # iw phy0 set netns 1
 # iw dev
				# iw dev
				phy#0
					Interface wl3.ap
						ifindex 2
						wdev 0x5
					Interface wlan3
						ifindex 11
						wdev 0x4

Note:
	increasing wdev identifier above indicates issue in
	cfg80211 which is addressed separately.

Tested-by: Mark Asselstine <mark.asselstine@windriver.com>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
Change-Id: I533ad998aec13b42b2605d8eae19a9c8fa540ffb
Reviewed-on: http://hnd-swgit.sj.broadcom.com:8080/8161
Reviewed-by: brcm80211 ci <brcm80211-ci@broadcom.com>
Reviewed-by: Franky Lin <franky.lin@broadcom.com>
Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 3 ++-
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c     | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 4e1ca69..89ac124 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -6453,7 +6453,8 @@ static int brcmf_setup_wiphy(struct wiphy *wiphy, struct brcmf_if *ifp)
 				    BIT(NL80211_BSS_SELECT_ATTR_BAND_PREF) |
 				    BIT(NL80211_BSS_SELECT_ATTR_RSSI_ADJUST);
 
-	wiphy->flags |= WIPHY_FLAG_PS_ON_BY_DEFAULT |
+	wiphy->flags |= WIPHY_FLAG_NETNS_OK |
+			WIPHY_FLAG_PS_ON_BY_DEFAULT |
 			WIPHY_FLAG_OFFCHAN_TX |
 			WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL;
 	if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_TDLS))
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index ba4da48..a5ffdfe 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -475,8 +475,9 @@ int brcmf_net_attach(struct brcmf_if *ifp, bool rtnl_locked)
 	ndev->needed_headroom += drvr->hdrlen;
 	ndev->ethtool_ops = &brcmf_ethtool_ops;
 
-	/* set the mac address */
+	/* set the mac address & netns */
 	memcpy(ndev->dev_addr, ifp->mac_addr, ETH_ALEN);
+	dev_net_set(ndev, wiphy_net(cfg_to_wiphy(drvr->config)));
 
 	INIT_WORK(&ifp->multicast_work, _brcmf_set_multicast_list);
 	INIT_WORK(&ifp->ndoffload_work, _brcmf_update_ndtable);
-- 
1.9.1

[PATCH 4/5] brcmfmac: restore bus state when enter_D3 fails

[Arend van Spriel]

In brcmf_pcie_suspend() we inform the firmware on the device that
it will enter in D3 state. Before this is done we already bring down
the bus state. However, When entering D3 fails we abort the suspend
and the bus state need to be restored.

Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
Reviewed-by: Franky Lin <franky.lin@broadcom.com>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
Change-Id: I837664ac9e06303b3a9596096f0f7c2bf7636b9a
Reviewed-on: http://hnd-swgit.sj.broadcom.com:8080/8160
Reviewed-by: brcm80211 ci <brcm80211-ci@broadcom.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index 6fae4cf..f36b96d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -1877,6 +1877,7 @@ static int brcmf_pcie_pm_enter_D3(struct device *dev)
 			   BRCMF_PCIE_MBDATA_TIMEOUT);
 	if (!devinfo->mbdata_completed) {
 		brcmf_err("Timeout on response for entering D3 substate\n");
+		brcmf_bus_change_state(bus, BRCMF_BUS_UP);
 		return -EIO;
 	}
 
-- 
1.9.1

[PATCH 5/5] brcmfmac: no need for d11inf instance in brcmf_pno_start_sched_scan()

[Arend van Spriel]

In brcmf_pno_start_sched_scan() a local variable is declared and
assigned for struct brcmu_d11inf. However, there is no other reference
to it so it is unnecessary.

Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
Reviewed-by: Franky Lin <franky.lin@broadcom.com>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/pno.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pno.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pno.c
index 9a25e79..6c3bde8 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pno.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pno.c
@@ -182,7 +182,6 @@ int brcmf_pno_clean(struct brcmf_if *ifp)
 int brcmf_pno_start_sched_scan(struct brcmf_if *ifp,
 			       struct cfg80211_sched_scan_request *req)
 {
-	struct brcmu_d11inf *d11inf;
 	struct brcmf_pno_config_le pno_cfg;
 	struct cfg80211_ssid *ssid;
 	u16 chan;
@@ -209,7 +208,6 @@ int brcmf_pno_start_sched_scan(struct brcmf_if *ifp,
 	}
 
 	/* configure channels to use */
-	d11inf = &ifp->drvr->config->d11inf;
 	for (i = 0; i < req->n_channels; i++) {
 		chan = req->channels[i]->hw_value;
 		pno_cfg.channel_list[i] = cpu_to_le16(chan);
-- 
1.9.1

Re: [PATCH 0/5] brcmfmac: network namespace support

[Rafał Miłecki]

On 03/28/2017 12:43 PM, Arend van Spriel wrote:
> This series intended for 4.12 includes:
> - support for network namespace
> - rework fwsignal module
> - small fix for suspend error code path
>
> Arend van Spriel (3):
>   brcmfmac: add support to move wiphy instance into network namespace
>   brcmfmac: restore bus state when enter_D3 fails
>   brcmfmac: no need for d11inf instance in brcmf_pno_start_sched_scan()
>
> Franky Lin (2):
>   brcmfmac: wrap brcmf_fws_init into bcdc layer
>   brcmfmac: move brcmf_fws_deinit to bcdc layer

It seems only 1 patch matches your patchset title. Maybe try picking them more
carefully ;)

Re: [PATCH 1/5] brcmfmac: wrap brcmf_fws_init into bcdc layer

[Rafał Miłecki]

On 03/28/2017 12:43 PM, Arend van Spriel wrote:
> From: Franky Lin <franky.lin@broadcom.com>
>
> Create a new protocol layer interface brcmf_proto_init_cb for protocol
> layer to finish initialzation after core module components(fweh and
> etc.) are initialized.
>
> Signed-off-by: Franky Lin <franky.lin@broadcom.com>
> Change-Id: I560d2478a7c09766cf07b20d74b31dff5ca6ac7b
> Reviewed-on: http://hnd-swgit.sj.broadcom.com:8080/8156

These 2 lines are rather useless.


> Reviewed-by: brcm80211 ci <brcm80211-ci@broadcom.com>

Please use full names only.


> Reviewed-by: Arend Van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c  | 7 +++++++
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c  | 2 +-
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h | 9 +++++++++
>  3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> index 92eafcc..bc24b00 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> @@ -417,6 +417,12 @@ static void brcmf_proto_bcdc_rxreorder(struct brcmf_if *ifp,
>  	brcmf_fws_reset_interface(ifp);
>  }
>
> +static int
> +brcmf_proto_bcdc_init_done(struct brcmf_pub *drvr)
> +{
> +	return brcmf_fws_init(drvr);
> +}
> +
>  int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr)
>  {
>  	struct brcmf_bcdc *bcdc;
> @@ -443,6 +449,7 @@ int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr)
>  	drvr->proto->add_if = brcmf_proto_bcdc_add_if;
>  	drvr->proto->del_if = brcmf_proto_bcdc_del_if;
>  	drvr->proto->reset_if = brcmf_proto_bcdc_reset_if;
> +	drvr->proto->init_done = brcmf_proto_bcdc_init_done;
>  	drvr->proto->pd = bcdc;
>
>  	drvr->hdrlen += BCDC_HEADER_LEN + BRCMF_PROT_FW_SIGNAL_MAX_TXBYTES;
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index 60c6c78..9886280 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -986,7 +986,7 @@ int brcmf_bus_started(struct device *dev)
>  	}
>  	brcmf_feat_attach(drvr);
>
> -	ret = brcmf_fws_init(drvr);
> +	ret = brcmf_proto_init_done(drvr);
>  	if (ret < 0)
>  		goto fail;
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h
> index 3048ed5..600fd33 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h
> @@ -47,6 +47,7 @@ struct brcmf_proto {
>  	void (*add_if)(struct brcmf_if *ifp);
>  	void (*del_if)(struct brcmf_if *ifp);
>  	void (*reset_if)(struct brcmf_if *ifp);
> +	int (*init_done)(struct brcmf_pub *drvr);
>  	void *pd;
>  };
>
> @@ -145,4 +146,12 @@ static inline bool brcmf_proto_is_reorder_skb(struct sk_buff *skb)
>  	drvr->proto->reset_if(ifp);
>  }
>
> +static inline int
> +brcmf_proto_init_done(struct brcmf_pub *drvr)
> +{
> +	if (!drvr->proto->init_done)
> +		return 0;
> +	return drvr->proto->init_done(drvr);
> +}
> +
>  #endif /* BRCMFMAC_PROTO_H */

So how is it any different from change in my:
[PATCH] brcmfmac: wrap brcmf_fws_(de)init into bcdc layer
? Is it only about replacing "init" with "init_done"?

I don't see why you couldn't rebase your changes on top of my patch.

Re: [PATCH 2/5] brcmfmac: move brcmf_fws_deinit to bcdc layer

[Rafał Miłecki]

On 03/28/2017 12:43 PM, Arend van Spriel wrote:
> From: Franky Lin <franky.lin@broadcom.com>
>
> Move brcmf_fws_deinit into brcmf_proto_bcdc_detach since it is a bcdc
> exclusive feature.
>
> Signed-off-by: Franky Lin <franky.lin@broadcom.com>
> Change-Id: Iefa5db632b92185a49d538b1cd25b7d8be618ce0
> Reviewed-on: http://hnd-swgit.sj.broadcom.com:8080/8157
> Reviewed-by: brcm80211 ci <brcm80211-ci@broadcom.com>
> Reviewed-by: Arend Van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c | 1 +
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 7 -------
>  2 files changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> index bc24b00..67a132c1 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> @@ -464,6 +464,7 @@ int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr)
>
>  void brcmf_proto_bcdc_detach(struct brcmf_pub *drvr)
>  {
> +	brcmf_fws_deinit(drvr);
>  	kfree(drvr->proto->pd);
>  	drvr->proto->pd = NULL;
>  }

I'd appreciate you commenting on someones work. I wouldn't mind "Move deinit to
brcmf_proto_bcdc_detach" comment so I could update my
[PATCH] brcmfmac: wrap brcmf_fws_(de)init into bcdc layer
if that is so important.


> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index 9886280..ba4da48 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -32,7 +32,6 @@
>  #include "p2p.h"
>  #include "cfg80211.h"
>  #include "fwil.h"
> -#include "fwsignal.h"
>  #include "feature.h"
>  #include "proto.h"
>  #include "pcie.h"
> @@ -1034,10 +1033,6 @@ int brcmf_bus_started(struct device *dev)
>  		brcmf_cfg80211_detach(drvr->config);
>  		drvr->config = NULL;
>  	}
> -	if (drvr->fws) {
> -		brcmf_proto_del_if(ifp->drvr, ifp);
> -		brcmf_fws_deinit(drvr);
> -	}
>  	brcmf_net_detach(ifp->ndev, false);
>  	if (p2p_ifp)
>  		brcmf_net_detach(p2p_ifp->ndev, false);

I don't like this. By removing del_if from error path you assume add_if doesn't
allocate any memory and it never will.

It's quite hacky for me. If you init something, then you should also deinit it.
Otherwise it's too easy to introduce an invalid state or add a memory leak.
Please keep code simple to follow.

Re: [PATCH 1/5] brcmfmac: wrap brcmf_fws_init into bcdc layer

[Arend Van Spriel]

On 29-3-2017 13:18, Rafał Miłecki wrote:
> On 03/28/2017 12:43 PM, Arend van Spriel wrote:
>> From: Franky Lin <franky.lin@broadcom.com>
>>
>> Create a new protocol layer interface brcmf_proto_init_cb for protocol
>> layer to finish initialzation after core module components(fweh and
>> etc.) are initialized.
>>
>> Signed-off-by: Franky Lin <franky.lin@broadcom.com>
>> Change-Id: I560d2478a7c09766cf07b20d74b31dff5ca6ac7b
>> Reviewed-on: http://hnd-swgit.sj.broadcom.com:8080/8156
> 
> These 2 lines are rather useless.

Sorry. Our Gerrit server adds these and I make an effort to remove this
before submitting, but every now and then a few slip through.

>> Reviewed-by: brcm80211 ci <brcm80211-ci@broadcom.com>
> 
> Please use full names only.

Sure thing. This is actually Jenkins account we use for building and
smoketesting the driver.

>> Reviewed-by: Arend Van Spriel <arend.vanspriel@broadcom.com>
>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>> ---
>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c  | 7 +++++++
>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c  | 2 +-
>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h | 9 +++++++++
>>  3 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
>> index 92eafcc..bc24b00 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
>> @@ -417,6 +417,12 @@ static void brcmf_proto_bcdc_rxreorder(struct
>> brcmf_if *ifp,
>>      brcmf_fws_reset_interface(ifp);
>>  }
>>
>> +static int
>> +brcmf_proto_bcdc_init_done(struct brcmf_pub *drvr)
>> +{
>> +    return brcmf_fws_init(drvr);
>> +}
>> +
>>  int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr)
>>  {
>>      struct brcmf_bcdc *bcdc;
>> @@ -443,6 +449,7 @@ int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr)
>>      drvr->proto->add_if = brcmf_proto_bcdc_add_if;
>>      drvr->proto->del_if = brcmf_proto_bcdc_del_if;
>>      drvr->proto->reset_if = brcmf_proto_bcdc_reset_if;
>> +    drvr->proto->init_done = brcmf_proto_bcdc_init_done;
>>      drvr->proto->pd = bcdc;
>>
>>      drvr->hdrlen += BCDC_HEADER_LEN + BRCMF_PROT_FW_SIGNAL_MAX_TXBYTES;
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> index 60c6c78..9886280 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> @@ -986,7 +986,7 @@ int brcmf_bus_started(struct device *dev)
>>      }
>>      brcmf_feat_attach(drvr);
>>
>> -    ret = brcmf_fws_init(drvr);
>> +    ret = brcmf_proto_init_done(drvr);
>>      if (ret < 0)
>>          goto fail;
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h
>> index 3048ed5..600fd33 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h
>> @@ -47,6 +47,7 @@ struct brcmf_proto {
>>      void (*add_if)(struct brcmf_if *ifp);
>>      void (*del_if)(struct brcmf_if *ifp);
>>      void (*reset_if)(struct brcmf_if *ifp);
>> +    int (*init_done)(struct brcmf_pub *drvr);
>>      void *pd;
>>  };
>>
>> @@ -145,4 +146,12 @@ static inline bool
>> brcmf_proto_is_reorder_skb(struct sk_buff *skb)
>>      drvr->proto->reset_if(ifp);
>>  }
>>
>> +static inline int
>> +brcmf_proto_init_done(struct brcmf_pub *drvr)
>> +{
>> +    if (!drvr->proto->init_done)
>> +        return 0;
>> +    return drvr->proto->init_done(drvr);
>> +}
>> +
>>  #endif /* BRCMFMAC_PROTO_H */
> 
> So how is it any different from change in my:
> [PATCH] brcmfmac: wrap brcmf_fws_(de)init into bcdc layer
> ? Is it only about replacing "init" with "init_done"?
> 
> I don't see why you couldn't rebase your changes on top of my patch.

What would be the benefit? These changes were under review/testing when
you submitted your patch so I felt it made sense to go with that.

Regards,
Arend

Re: [PATCH 2/5] brcmfmac: move brcmf_fws_deinit to bcdc layer

[Arend Van Spriel]

On 29-3-2017 13:23, Rafał Miłecki wrote:
> On 03/28/2017 12:43 PM, Arend van Spriel wrote:
>> From: Franky Lin <franky.lin@broadcom.com>
>>
>> Move brcmf_fws_deinit into brcmf_proto_bcdc_detach since it is a bcdc
>> exclusive feature.
>>
>> Signed-off-by: Franky Lin <franky.lin@broadcom.com>
>> Change-Id: Iefa5db632b92185a49d538b1cd25b7d8be618ce0
>> Reviewed-on: http://hnd-swgit.sj.broadcom.com:8080/8157
>> Reviewed-by: brcm80211 ci <brcm80211-ci@broadcom.com>
>> Reviewed-by: Arend Van Spriel <arend.vanspriel@broadcom.com>
>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>> ---
>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c | 1 +
>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 7 -------
>>  2 files changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
>> index bc24b00..67a132c1 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
>> @@ -464,6 +464,7 @@ int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr)
>>
>>  void brcmf_proto_bcdc_detach(struct brcmf_pub *drvr)
>>  {
>> +    brcmf_fws_deinit(drvr);
>>      kfree(drvr->proto->pd);
>>      drvr->proto->pd = NULL;
>>  }
> 
> I'd appreciate you commenting on someones work. I wouldn't mind "Move
> deinit to
> brcmf_proto_bcdc_detach" comment so I could update my
> [PATCH] brcmfmac: wrap brcmf_fws_(de)init into bcdc layer
> if that is so important.

The naming of the functions in fwsignal were poorly chosen (by me). The
common pattern throughout the driver is to use brcmf_<mod>_attach/detach
If fwsignal would have followed that pattern you probably would have
done so.

>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> index 9886280..ba4da48 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> @@ -32,7 +32,6 @@
>>  #include "p2p.h"
>>  #include "cfg80211.h"
>>  #include "fwil.h"
>> -#include "fwsignal.h"
>>  #include "feature.h"
>>  #include "proto.h"
>>  #include "pcie.h"
>> @@ -1034,10 +1033,6 @@ int brcmf_bus_started(struct device *dev)
>>          brcmf_cfg80211_detach(drvr->config);
>>          drvr->config = NULL;
>>      }
>> -    if (drvr->fws) {
>> -        brcmf_proto_del_if(ifp->drvr, ifp);
>> -        brcmf_fws_deinit(drvr);
>> -    }
>>      brcmf_net_detach(ifp->ndev, false);
>>      if (p2p_ifp)
>>          brcmf_net_detach(p2p_ifp->ndev, false);
> 
> I don't like this. By removing del_if from error path you assume add_if
> doesn't
> allocate any memory and it never will.

This removal was actually the reason this patch was not submitted
earlier as I asked for more testing.

> It's quite hacky for me. If you init something, then you should also
> deinit it.
> Otherwise it's too easy to introduce an invalid state or add a memory leak.
> Please keep code simple to follow.

The removal here is safe as all interfaces are deleted in brcmf_detach()
using brcmf_remove_interface() which is called when brcmf_bus_started()
fails.

Regards,
Arend

Re: [PATCH 1/5] brcmfmac: wrap brcmf_fws_init into bcdc layer

[Kalle Valo]

Arend Van Spriel <arend.vanspriel@broadcom.com> writes:

> On 29-3-2017 13:18, Rafa=C5=82 Mi=C5=82ecki wrote:
>> On 03/28/2017 12:43 PM, Arend van Spriel wrote:
>>> From: Franky Lin <franky.lin@broadcom.com>
>>>
>>> Create a new protocol layer interface brcmf_proto_init_cb for protocol
>>> layer to finish initialzation after core module components(fweh and
>>> etc.) are initialized.
>>>
>>> Signed-off-by: Franky Lin <franky.lin@broadcom.com>
>>> Change-Id: I560d2478a7c09766cf07b20d74b31dff5ca6ac7b
>>> Reviewed-on: http://hnd-swgit.sj.broadcom.com:8080/8156
>>=20
>> These 2 lines are rather useless.
>
> Sorry. Our Gerrit server adds these and I make an effort to remove this
> before submitting, but every now and then a few slip through.
>
>>> Reviewed-by: brcm80211 ci <brcm80211-ci@broadcom.com>
>>=20
>> Please use full names only.
>
> Sure thing. This is actually Jenkins account we use for building and
> smoketesting the driver.

I'll remove these from patches 1-4 during commit. (patch 5 seems to be
ok)

--=20
Kalle Valo

Re: [1/5] brcmfmac: wrap brcmf_fws_init into bcdc layer

[Kalle Valo]

Arend Van Spriel <arend.vanspriel@broadcom.com> wrote:
> From: Franky Lin <franky.lin@broadcom.com>
> 
> Create a new protocol layer interface brcmf_proto_init_cb for protocol
> layer to finish initialzation after core module components(fweh and
> etc.) are initialized.
> 
> Signed-off-by: Franky Lin <franky.lin@broadcom.com>
> Reviewed-by: Arend Van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>

5 patches applied to wireless-drivers-next.git, thanks.

62c50a34883c brcmfmac: wrap brcmf_fws_init into bcdc layer
8f9dd1a97438 brcmfmac: move brcmf_fws_deinit to bcdc layer
0cc0236cf713 brcmfmac: add support to move wiphy instance into network namespace
49fe9b59f0e9 brcmfmac: restore bus state when enter_D3 fails
78b9ccb81377 brcmfmac: no need for d11inf instance in brcmf_pno_start_sched_scan()

-- 
https://patchwork.kernel.org/patch/9648789/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches