[RFC 0/3] brcmfmac: provide band info upon wiphy registration

[RFC 0/3] brcmfmac: provide band info upon wiphy registration

[Arend van Spriel]

This short series rely on a cfg80211 API so making this a RFC. It
applies to master branch of the wireless-next repository.

Arend van Spriel (3):
  brcmfmac: move attach and detach functions in wl_cfg80211.c
  cfg80211: expose cfg80211_disable_40mhz_24ghz module parameter
  brcmfmac: update band and regulatory info before wiphy_register()

 .../net/wireless/brcm80211/brcmfmac/wl_cfg80211.c  | 240 ++++++++++-----------
 include/net/cfg80211.h                             |   9 +
 net/wireless/core.c                                |   6 +
 3 files changed, 130 insertions(+), 125 deletions(-)

-- 
1.9.1

[RFC 1/3] brcmfmac: move attach and detach functions in wl_cfg80211.c

[Arend van Spriel]

Simply moving the functions so subsequent patches are more clear.

Reviewed-by: Hante Meuleman <meuleman@broadcom.com>
Reviewed-by: Daniel (Deognyoun) Kim <dekim@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 .../net/wireless/brcm80211/brcmfmac/wl_cfg80211.c  | 213 +++++++++++----------
 1 file changed, 108 insertions(+), 105 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
index d8fa276..2464e7e 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
@@ -4990,111 +4990,6 @@ static int brcmf_enable_bw40_2g(struct brcmf_if *ifp)
 	return err;
 }
 
-struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr,
-						  struct device *busdev)
-{
-	struct net_device *ndev = drvr->iflist[0]->ndev;
-	struct brcmf_cfg80211_info *cfg;
-	struct wiphy *wiphy;
-	struct brcmf_cfg80211_vif *vif;
-	struct brcmf_if *ifp;
-	s32 err = 0;
-	s32 io_type;
-
-	if (!ndev) {
-		brcmf_err("ndev is invalid\n");
-		return NULL;
-	}
-
-	ifp = netdev_priv(ndev);
-	wiphy = brcmf_setup_wiphy(busdev);
-	if (IS_ERR(wiphy))
-		return NULL;
-
-	cfg = wiphy_priv(wiphy);
-	cfg->wiphy = wiphy;
-	cfg->pub = drvr;
-	init_vif_event(&cfg->vif_event);
-	INIT_LIST_HEAD(&cfg->vif_list);
-
-	vif = brcmf_alloc_vif(cfg, NL80211_IFTYPE_STATION, false);
-	if (IS_ERR(vif)) {
-		wiphy_free(wiphy);
-		return NULL;
-	}
-
-	vif->ifp = ifp;
-	vif->wdev.netdev = ndev;
-	ndev->ieee80211_ptr = &vif->wdev;
-	SET_NETDEV_DEV(ndev, wiphy_dev(cfg->wiphy));
-
-	err = wl_init_priv(cfg);
-	if (err) {
-		brcmf_err("Failed to init iwm_priv (%d)\n", err);
-		goto cfg80211_attach_out;
-	}
-	ifp->vif = vif;
-
-	err = brcmf_p2p_attach(cfg);
-	if (err) {
-		brcmf_err("P2P initilisation failed (%d)\n", err);
-		goto cfg80211_p2p_attach_out;
-	}
-	err = brcmf_btcoex_attach(cfg);
-	if (err) {
-		brcmf_err("BT-coex initialisation failed (%d)\n", err);
-		brcmf_p2p_detach(&cfg->p2p);
-		goto cfg80211_p2p_attach_out;
-	}
-
-	/* If cfg80211 didn't disable 40MHz HT CAP in wiphy_register(),
-	 * setup 40MHz in 2GHz band and enable OBSS scanning.
-	 */
-	if (wiphy->bands[IEEE80211_BAND_2GHZ]->ht_cap.cap &
-	    IEEE80211_HT_CAP_SUP_WIDTH_20_40) {
-		err = brcmf_enable_bw40_2g(ifp);
-		if (!err)
-			err = brcmf_fil_iovar_int_set(ifp, "obss_coex",
-						      BRCMF_OBSS_COEX_AUTO);
-	}
-
-	err = brcmf_fil_iovar_int_set(ifp, "tdls_enable", 1);
-	if (err) {
-		brcmf_dbg(INFO, "TDLS not enabled (%d)\n", err);
-		wiphy->flags &= ~WIPHY_FLAG_SUPPORTS_TDLS;
-	}
-
-	err = brcmf_fil_cmd_int_get(ifp, BRCMF_C_GET_VERSION,
-				    &io_type);
-	if (err) {
-		brcmf_err("Failed to get D11 version (%d)\n", err);
-		goto cfg80211_p2p_attach_out;
-	}
-	cfg->d11inf.io_type = (u8)io_type;
-	brcmu_d11_attach(&cfg->d11inf);
-
-	return cfg;
-
-cfg80211_p2p_attach_out:
-	wl_deinit_priv(cfg);
-
-cfg80211_attach_out:
-	brcmf_free_vif(vif);
-	return NULL;
-}
-
-void brcmf_cfg80211_detach(struct brcmf_cfg80211_info *cfg)
-{
-	if (!cfg)
-		return;
-
-	WARN_ON(!list_empty(&cfg->vif_list));
-	wiphy_unregister(cfg->wiphy);
-	brcmf_btcoex_detach(cfg);
-	wl_deinit_priv(cfg);
-	wiphy_free(cfg->wiphy);
-}
-
 static s32
 brcmf_dongle_roam(struct brcmf_if *ifp, u32 bcn_timeout)
 {
@@ -5679,3 +5574,111 @@ int brcmf_cfg80211_wait_vif_event_timeout(struct brcmf_cfg80211_info *cfg,
 				  vif_event_equals(event, action), timeout);
 }
 
+struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr,
+						  struct device *busdev)
+{
+	struct net_device *ndev = drvr->iflist[0]->ndev;
+	struct brcmf_cfg80211_info *cfg;
+	struct wiphy *wiphy;
+	struct brcmf_cfg80211_vif *vif;
+	struct brcmf_if *ifp;
+	s32 err = 0;
+	s32 io_type;
+
+	if (!ndev) {
+		brcmf_err("ndev is invalid\n");
+		return NULL;
+	}
+
+	ifp = netdev_priv(ndev);
+	wiphy = brcmf_setup_wiphy(busdev);
+	if (IS_ERR(wiphy))
+		return NULL;
+
+	cfg = wiphy_priv(wiphy);
+	cfg->wiphy = wiphy;
+	cfg->pub = drvr;
+	init_vif_event(&cfg->vif_event);
+	INIT_LIST_HEAD(&cfg->vif_list);
+
+	vif = brcmf_alloc_vif(cfg, NL80211_IFTYPE_STATION, false);
+	if (IS_ERR(vif)) {
+		wiphy_free(wiphy);
+		return NULL;
+	}
+
+	vif->ifp = ifp;
+	vif->wdev.netdev = ndev;
+	ndev->ieee80211_ptr = &vif->wdev;
+	SET_NETDEV_DEV(ndev, wiphy_dev(cfg->wiphy));
+
+	err = wl_init_priv(cfg);
+	if (err) {
+		brcmf_err("Failed to init iwm_priv (%d)\n", err);
+		goto cfg80211_attach_out;
+	}
+	ifp->vif = vif;
+
+	err = brcmf_p2p_attach(cfg);
+	if (err) {
+		brcmf_err("P2P initilisation failed (%d)\n", err);
+		goto cfg80211_p2p_attach_out;
+	}
+	err = brcmf_btcoex_attach(cfg);
+	if (err) {
+		brcmf_err("BT-coex initialisation failed (%d)\n", err);
+		brcmf_p2p_detach(&cfg->p2p);
+		goto cfg80211_p2p_attach_out;
+	}
+
+	/* If cfg80211 didn't disable 40MHz HT CAP in wiphy_register(),
+	 * setup 40MHz in 2GHz band and enable OBSS scanning.
+	 */
+	if (wiphy->bands[IEEE80211_BAND_2GHZ]->ht_cap.cap &
+	    IEEE80211_HT_CAP_SUP_WIDTH_20_40) {
+		err = brcmf_enable_bw40_2g(ifp);
+		if (!err)
+			err = brcmf_fil_iovar_int_set(ifp, "obss_coex",
+						      BRCMF_OBSS_COEX_AUTO);
+	}
+	/* clear for now and rely on update later */
+	wiphy->bands[IEEE80211_BAND_2GHZ]->ht_cap.ht_supported = false;
+	wiphy->bands[IEEE80211_BAND_2GHZ]->ht_cap.cap = 0;
+
+	err = brcmf_fil_iovar_int_set(ifp, "tdls_enable", 1);
+	if (err) {
+		brcmf_dbg(INFO, "TDLS not enabled (%d)\n", err);
+		wiphy->flags &= ~WIPHY_FLAG_SUPPORTS_TDLS;
+	}
+
+	err = brcmf_fil_cmd_int_get(ifp, BRCMF_C_GET_VERSION,
+				    &io_type);
+	if (err) {
+		brcmf_err("Failed to get D11 version (%d)\n", err);
+		goto cfg80211_p2p_attach_out;
+	}
+	cfg->d11inf.io_type = (u8)io_type;
+	brcmu_d11_attach(&cfg->d11inf);
+
+	return cfg;
+
+cfg80211_p2p_attach_out:
+	wl_deinit_priv(cfg);
+
+cfg80211_attach_out:
+	brcmf_free_vif(vif);
+	return NULL;
+}
+
+void brcmf_cfg80211_detach(struct brcmf_cfg80211_info *cfg)
+{
+	if (!cfg)
+		return;
+
+	WARN_ON(!list_empty(&cfg->vif_list));
+	wiphy_unregister(cfg->wiphy);
+	brcmf_btcoex_detach(cfg);
+	wl_deinit_priv(cfg);
+	wiphy_free(cfg->wiphy);
+}
+
-- 
1.9.1

[RFC 2/3] cfg80211: expose cfg80211_disable_40mhz_24ghz module parameter

[Arend van Spriel]

Drivers may need to know this variable value for configuring the
device they are controlling. It can be determined indirectly by
setting IEEE80211_HT_CAP_SUP_WIDTH_20_40 flag in the field
ieee80211_sta_htcap::cap of 2G band. During the wiphy_register()
call cfg80211 will clear that bit if the module parameter is true.
However, brcmfmac for one needs to know the value to determine
its custom regulatory domain which must be applied before doing
the wiphy_register().

Reviewed-by: Hante Meuleman <meuleman@broadcom.com>
Reviewed-by: Daniel (Deognyoun) Kim <dekim@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 include/net/cfg80211.h | 9 +++++++++
 net/wireless/core.c    | 6 ++++++
 2 files changed, 15 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index e46c437..47a0310 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -3172,6 +3172,15 @@ static inline const char *wiphy_name(const struct wiphy *wiphy)
 struct wiphy *wiphy_new(const struct cfg80211_ops *ops, int sizeof_priv);
 
 /**
+ * wiphy_is_40mhz_24ghz_disabled - check whether 40MHz bandwidth in 2.4G
+ *
+ * @wiphy: The wiphy to check.
+ *
+ * Return: true is 40MHz is disabled in 2.4G band.
+ */
+bool wiphy_is_40mhz_24ghz_disabled(struct wiphy *wiphy);
+
+/**
  * wiphy_register - register a wiphy with cfg80211
  *
  * @wiphy: The wiphy to register.
diff --git a/net/wireless/core.c b/net/wireless/core.c
index a1c4065..428f370 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -408,6 +408,12 @@ struct wiphy *wiphy_new(const struct cfg80211_ops *ops, int sizeof_priv)
 }
 EXPORT_SYMBOL(wiphy_new);
 
+bool wiphy_is_40mhz_24ghz_disabled(struct wiphy *wiphy)
+{
+	return cfg80211_disable_40mhz_24ghz;
+}
+EXPORT_SYMBOL(wiphy_is_40mhz_24ghz_disabled);
+
 static int wiphy_verify_combinations(struct wiphy *wiphy)
 {
 	const struct ieee80211_iface_combination *c;
-- 
1.9.1

Re: [RFC 2/3] cfg80211: expose cfg80211_disable_40mhz_24ghz module parameter

[Johannes Berg]

On Tue, 2014-06-17 at 11:16 +0200, Arend van Spriel wrote:

>  /**
> + * wiphy_is_40mhz_24ghz_disabled - check whether 40MHz bandwidth in 2.4G

that seems incomplete :)

> + * @wiphy: The wiphy to check.
> + *
> + * Return: true is 40MHz is disabled in 2.4G band.
> + */
> +bool wiphy_is_40mhz_24ghz_disabled(struct wiphy *wiphy);

Why should that have a wiphy argument?

Might also be simpler to just export the variable?

johannes

Re: [RFC 2/3] cfg80211: expose cfg80211_disable_40mhz_24ghz module parameter

[Arend van Spriel]

On 23-06-14 11:34, Johannes Berg wrote:
> On Tue, 2014-06-17 at 11:16 +0200, Arend van Spriel wrote:
>
>>   /**
>> + * wiphy_is_40mhz_24ghz_disabled - check whether 40MHz bandwidth in 2.4G
>
> that seems incomplete :)

No. It *is* incomplete ;-)

>> + * @wiphy: The wiphy to check.
>> + *
>> + * Return: true is 40MHz is disabled in 2.4G band.
>> + */
>> +bool wiphy_is_40mhz_24ghz_disabled(struct wiphy *wiphy);
>
> Why should that have a wiphy argument?

It is up to cfg80211 to determine the logic behind the function. It is 
just context supplied by the caller. The fact that it is determined by a 
module parameter is internal cfg80211 stuff.

> Might also be simpler to just export the variable?

Either way is pretty simple I guess.

Gr. AvS

Re: [RFC 2/3] cfg80211: expose cfg80211_disable_40mhz_24ghz module parameter

[Johannes Berg]

On Tue, 2014-06-24 at 19:21 +0200, Arend van Spriel wrote:

> >> + * @wiphy: The wiphy to check.
> >> + *
> >> + * Return: true is 40MHz is disabled in 2.4G band.
> >> + */
> >> +bool wiphy_is_40mhz_24ghz_disabled(struct wiphy *wiphy);
> >
> > Why should that have a wiphy argument?
> 
> It is up to cfg80211 to determine the logic behind the function. It is 
> just context supplied by the caller. The fact that it is determined by a 
> module parameter is internal cfg80211 stuff.

I just don't really see any way it would ever be per wiphy?

> > Might also be simpler to just export the variable?
> 
> Either way is pretty simple I guess.

Yeah, dunno. I guess exporting the function at least makes sure nobody
will try to set the variable, so that's somewhat useful...

I'd prefer to remove the wiphy argument though (and fix the kernel-doc)

johannes

Re: [RFC 2/3] cfg80211: expose cfg80211_disable_40mhz_24ghz module parameter

[Arend van Spriel]

On 25-06-14 17:57, Johannes Berg wrote:
> On Tue, 2014-06-24 at 19:21 +0200, Arend van Spriel wrote:
>
>>>> + * @wiphy: The wiphy to check.
>>>> + *
>>>> + * Return: true is 40MHz is disabled in 2.4G band.
>>>> + */
>>>> +bool wiphy_is_40mhz_24ghz_disabled(struct wiphy *wiphy);
>>>
>>> Why should that have a wiphy argument?
>>
>> It is up to cfg80211 to determine the logic behind the function. It is
>> just context supplied by the caller. The fact that it is determined by a
>> module parameter is internal cfg80211 stuff.
>
> I just don't really see any way it would ever be per wiphy?

Maybe you are right. By the way, is OBSS support a requirement to do 
40MHz in 2.4G?

>>> Might also be simpler to just export the variable?
>>
>> Either way is pretty simple I guess.
>
> Yeah, dunno. I guess exporting the function at least makes sure nobody
> will try to set the variable, so that's somewhat useful...
>
> I'd prefer to remove the wiphy argument though (and fix the kernel-doc)

No problem. Will do.

Gr. AvS

[RFC 3/3] brcmfmac: update band and regulatory info before wiphy_register()

[Arend van Spriel]

The band information was determined when bringing up the wireless
interface. According description of wiphy_apply_custom_regulatory()
it should be called before wiphy_register(). Currently, the driver
calls the function twice. Once before calling wiphy_register() and
once upon bringing up the interface. This is changed so it will
determine band information and custom regulatory info before the
wiphy_register() call.

Reviewed-by: Hante Meuleman <meuleman@broadcom.com>
Reviewed-by: Daniel (Deognyoun) Kim <dekim@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 .../net/wireless/brcm80211/brcmfmac/wl_cfg80211.c  | 93 ++++++++++------------
 1 file changed, 40 insertions(+), 53 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
index 2464e7e..dbe391c 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
@@ -191,7 +191,6 @@ static struct ieee80211_supported_band __wl_band_2ghz = {
 	.n_channels = ARRAY_SIZE(__wl_2ghz_channels),
 	.bitrates = wl_g_rates,
 	.n_bitrates = wl_g_rates_size,
-	.ht_cap = {IEEE80211_HT_CAP_SUP_WIDTH_20_40, true},
 };
 
 static struct ieee80211_supported_band __wl_band_5ghz_a = {
@@ -4377,7 +4376,6 @@ brcmf_txrx_stypes[NUM_NL80211_IFTYPES] = {
 static struct wiphy *brcmf_setup_wiphy(struct device *phydev)
 {
 	struct wiphy *wiphy;
-	s32 err = 0;
 
 	wiphy = wiphy_new(&wl_cfg80211_ops, sizeof(struct brcmf_cfg80211_info));
 	if (!wiphy) {
@@ -4409,15 +4407,6 @@ static struct wiphy *brcmf_setup_wiphy(struct device *phydev)
 	wiphy->mgmt_stypes = brcmf_txrx_stypes;
 	wiphy->max_remain_on_channel_duration = 5000;
 	brcmf_wiphy_pno_params(wiphy);
-	brcmf_dbg(INFO, "Registering custom regulatory\n");
-	wiphy->regulatory_flags |= REGULATORY_CUSTOM_REG;
-	wiphy_apply_custom_regulatory(wiphy, &brcmf_regdom);
-	err = wiphy_register(wiphy);
-	if (err < 0) {
-		brcmf_err("Could not register wiphy device (%d)\n", err);
-		wiphy_free(wiphy);
-		return ERR_PTR(err);
-	}
 	return wiphy;
 }
 
@@ -5397,17 +5386,11 @@ static s32 brcmf_update_wiphybands(struct brcmf_cfg80211_info *cfg)
 	wiphy = cfg_to_wiphy(cfg);
 	wiphy->bands[IEEE80211_BAND_2GHZ] = bands[IEEE80211_BAND_2GHZ];
 	wiphy->bands[IEEE80211_BAND_5GHZ] = bands[IEEE80211_BAND_5GHZ];
-	wiphy_apply_custom_regulatory(wiphy, &brcmf_regdom);
 
 	return err;
 }
 
 
-static s32 brcmf_dongle_probecap(struct brcmf_cfg80211_info *cfg)
-{
-	return brcmf_update_wiphybands(cfg);
-}
-
 static s32 brcmf_config_dongle(struct brcmf_cfg80211_info *cfg)
 {
 	struct net_device *ndev;
@@ -5443,9 +5426,6 @@ static s32 brcmf_config_dongle(struct brcmf_cfg80211_info *cfg)
 					  NULL, NULL);
 	if (err)
 		goto default_conf_out;
-	err = brcmf_dongle_probecap(cfg);
-	if (err)
-		goto default_conf_out;
 
 	brcmf_configure_arp_offload(ifp, true);
 
@@ -5602,10 +5582,8 @@ struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr,
 	INIT_LIST_HEAD(&cfg->vif_list);
 
 	vif = brcmf_alloc_vif(cfg, NL80211_IFTYPE_STATION, false);
-	if (IS_ERR(vif)) {
-		wiphy_free(wiphy);
-		return NULL;
-	}
+	if (IS_ERR(vif))
+		goto wiphy_out;
 
 	vif->ifp = ifp;
 	vif->wdev.netdev = ndev;
@@ -5615,35 +5593,51 @@ struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr,
 	err = wl_init_priv(cfg);
 	if (err) {
 		brcmf_err("Failed to init iwm_priv (%d)\n", err);
-		goto cfg80211_attach_out;
+		brcmf_free_vif(vif);
+		goto wiphy_out;
 	}
 	ifp->vif = vif;
 
+	if (wiphy_is_40mhz_24ghz_disabled(wiphy) == false) {
+		err = brcmf_enable_bw40_2g(ifp);
+		if (!err)
+			err = brcmf_fil_iovar_int_set(ifp, "obss_coex",
+						      BRCMF_OBSS_COEX_AUTO);
+	}
+
+	/* determine d11 io type before updating bands */
+	err = brcmf_fil_cmd_int_get(ifp, BRCMF_C_GET_VERSION, &io_type);
+	if (err) {
+		brcmf_err("Failed to get D11 version (%d)\n", err);
+		goto priv_out;
+	}
+	cfg->d11inf.io_type = (u8)io_type;
+	brcmu_d11_attach(&cfg->d11inf);
+
+	err = brcmf_update_wiphybands(cfg);
+	if (err)
+		goto priv_out;
+
+	brcmf_dbg(INFO, "Registering custom regulatory\n");
+	wiphy->regulatory_flags |= REGULATORY_CUSTOM_REG;
+	wiphy_apply_custom_regulatory(wiphy, &brcmf_regdom);
+	err = wiphy_register(wiphy);
+	if (err < 0) {
+		brcmf_err("Could not register wiphy device (%d)\n", err);
+		goto priv_out;
+	}
+
 	err = brcmf_p2p_attach(cfg);
 	if (err) {
 		brcmf_err("P2P initilisation failed (%d)\n", err);
-		goto cfg80211_p2p_attach_out;
+		goto unreg_out;
 	}
 	err = brcmf_btcoex_attach(cfg);
 	if (err) {
 		brcmf_err("BT-coex initialisation failed (%d)\n", err);
 		brcmf_p2p_detach(&cfg->p2p);
-		goto cfg80211_p2p_attach_out;
-	}
-
-	/* If cfg80211 didn't disable 40MHz HT CAP in wiphy_register(),
-	 * setup 40MHz in 2GHz band and enable OBSS scanning.
-	 */
-	if (wiphy->bands[IEEE80211_BAND_2GHZ]->ht_cap.cap &
-	    IEEE80211_HT_CAP_SUP_WIDTH_20_40) {
-		err = brcmf_enable_bw40_2g(ifp);
-		if (!err)
-			err = brcmf_fil_iovar_int_set(ifp, "obss_coex",
-						      BRCMF_OBSS_COEX_AUTO);
+		goto unreg_out;
 	}
-	/* clear for now and rely on update later */
-	wiphy->bands[IEEE80211_BAND_2GHZ]->ht_cap.ht_supported = false;
-	wiphy->bands[IEEE80211_BAND_2GHZ]->ht_cap.cap = 0;
 
 	err = brcmf_fil_iovar_int_set(ifp, "tdls_enable", 1);
 	if (err) {
@@ -5651,22 +5645,15 @@ struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr,
 		wiphy->flags &= ~WIPHY_FLAG_SUPPORTS_TDLS;
 	}
 
-	err = brcmf_fil_cmd_int_get(ifp, BRCMF_C_GET_VERSION,
-				    &io_type);
-	if (err) {
-		brcmf_err("Failed to get D11 version (%d)\n", err);
-		goto cfg80211_p2p_attach_out;
-	}
-	cfg->d11inf.io_type = (u8)io_type;
-	brcmu_d11_attach(&cfg->d11inf);
-
 	return cfg;
 
-cfg80211_p2p_attach_out:
+unreg_out:
+	wiphy_unregister(wiphy);
+priv_out:
 	wl_deinit_priv(cfg);
-
-cfg80211_attach_out:
 	brcmf_free_vif(vif);
+wiphy_out:
+	wiphy_free(wiphy);
 	return NULL;
 }
 
-- 
1.9.1

Re: [RFC 3/3] brcmfmac: update band and regulatory info before wiphy_register()

[Johannes Berg]

I'm not really sure why this code requires the previous patch?

> @@ -5615,35 +5593,51 @@ struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr,
>  	err = wl_init_priv(cfg);
>  	if (err) {
>  		brcmf_err("Failed to init iwm_priv (%d)\n", err);
> -		goto cfg80211_attach_out;
> +		brcmf_free_vif(vif);
> +		goto wiphy_out;
>  	}
>  	ifp->vif = vif;
>  
> +	if (wiphy_is_40mhz_24ghz_disabled(wiphy) == false) {
> +		err = brcmf_enable_bw40_2g(ifp);
> +		if (!err)
> +			err = brcmf_fil_iovar_int_set(ifp, "obss_coex",
> +						      BRCMF_OBSS_COEX_AUTO);
> +	}

This seems to have an effect only on the device, so why not do it after
wiphy_register()?

> +	/* determine d11 io type before updating bands */
> +	err = brcmf_fil_cmd_int_get(ifp, BRCMF_C_GET_VERSION, &io_type);
> +	if (err) {
> +		brcmf_err("Failed to get D11 version (%d)\n", err);
> +		goto priv_out;
> +	}
> +	cfg->d11inf.io_type = (u8)io_type;
> +	brcmu_d11_attach(&cfg->d11inf);
> +
> +	err = brcmf_update_wiphybands(cfg);
> +	if (err)
> +		goto priv_out;
> +
> +	brcmf_dbg(INFO, "Registering custom regulatory\n");
> +	wiphy->regulatory_flags |= REGULATORY_CUSTOM_REG;
> +	wiphy_apply_custom_regulatory(wiphy, &brcmf_regdom);

The regdomain is also static const so not affected by it.

johannes

Re: [RFC 3/3] brcmfmac: update band and regulatory info before wiphy_register()

[Arend van Spriel]

On 23-06-14 11:37, Johannes Berg wrote:
> I'm not really sure why this code requires the previous patch?
>
>> @@ -5615,35 +5593,51 @@ struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr,
>>   	err = wl_init_priv(cfg);
>>   	if (err) {
>>   		brcmf_err("Failed to init iwm_priv (%d)\n", err);
>> -		goto cfg80211_attach_out;
>> +		brcmf_free_vif(vif);
>> +		goto wiphy_out;
>>   	}
>>   	ifp->vif = vif;
>>
>> +	if (wiphy_is_40mhz_24ghz_disabled(wiphy) == false) {
>> +		err = brcmf_enable_bw40_2g(ifp);
>> +		if (!err)
>> +			err = brcmf_fil_iovar_int_set(ifp, "obss_coex",
>> +						      BRCMF_OBSS_COEX_AUTO);
>> +	}
>
> This seems to have an effect only on the device, so why not do it after
> wiphy_register()?

It does affect channels list in brcmf_update_wiphybands().

>> +	/* determine d11 io type before updating bands */
>> +	err = brcmf_fil_cmd_int_get(ifp, BRCMF_C_GET_VERSION, &io_type);
>> +	if (err) {
>> +		brcmf_err("Failed to get D11 version (%d)\n", err);
>> +		goto priv_out;
>> +	}
>> +	cfg->d11inf.io_type = (u8)io_type;
>> +	brcmu_d11_attach(&cfg->d11inf);
>> +
>> +	err = brcmf_update_wiphybands(cfg);
>> +	if (err)
>> +		goto priv_out;
>> +
>> +	brcmf_dbg(INFO, "Registering custom regulatory\n");
>> +	wiphy->regulatory_flags |= REGULATORY_CUSTOM_REG;
>> +	wiphy_apply_custom_regulatory(wiphy, &brcmf_regdom);
>
> The regdomain is also static const so not affected by it.

Digging further in this code, I found that the brcmf_update_wiphybands() 
queries the device to get the actual list of channels, which 
subsequently is determined by the 40MHz setting in the 2G band. Not so 
much for the channel itself as for the flags. If that is all done after 
wiphy_apply_custom_regulatory() it does not show up when doing 'iw phy 
info'. And according to the documentation that call needs to be done 
before wiphy_register().

In the current code the wiphy_apply_custom_regulatory() is called twice 
(before and after wiphy_register()) which seems to do the trick, but in 
that it might be exploiting unintended behaviour. Hence the attempt to 
change the order of things.

Regards,
Arend

Re: [RFC 3/3] brcmfmac: update band and regulatory info before wiphy_register()

[Johannes Berg]

On Tue, 2014-06-24 at 19:11 +0200, Arend van Spriel wrote:

> >> +	if (wiphy_is_40mhz_24ghz_disabled(wiphy) == false) {
> >> +		err = brcmf_enable_bw40_2g(ifp);
> >> +		if (!err)
> >> +			err = brcmf_fil_iovar_int_set(ifp, "obss_coex",
> >> +						      BRCMF_OBSS_COEX_AUTO);
> >> +	}
> >
> > This seems to have an effect only on the device, so why not do it after
> > wiphy_register()?
> 
> It does affect channels list in brcmf_update_wiphybands().

Ah, ok, I guess I missed that.

> Digging further in this code, I found that the brcmf_update_wiphybands() 
> queries the device to get the actual list of channels, which 
> subsequently is determined by the 40MHz setting in the 2G band. Not so 
> much for the channel itself as for the flags. If that is all done after 
> wiphy_apply_custom_regulatory() it does not show up when doing 'iw phy 
> info'. And according to the documentation that call needs to be done 
> before wiphy_register().
> 
> In the current code the wiphy_apply_custom_regulatory() is called twice 
> (before and after wiphy_register()) which seems to do the trick, but in 
> that it might be exploiting unintended behaviour. Hence the attempt to 
> change the order of things.

Ok :)

johannes