[RFT] brcmfmac: add support to move wiphy instance into network namespace

[RFT] 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.

Reported-by: Mark Asselstine <mark.asselstine@windriver.com>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
Hi Mark,

Please check this patch. I can not say I am an expert when it comes
to using namespaces. So let me know if it works and I can change
Reported-by into Tested-by.

Regards,
Arend
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 3 ++-
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c     | 5 ++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 3856de6..e0d65df 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -6450,7 +6450,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 22b4883..74ede27 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -463,6 +463,7 @@ static int brcmf_netdev_open(struct net_device *ndev)
 int brcmf_net_attach(struct brcmf_if *ifp, bool rtnl_locked)
 {
 	struct brcmf_pub *drvr = ifp->drvr;
+	struct wiphy *wiphy;
 	struct net_device *ndev;
 	s32 err;
 
@@ -476,8 +477,10 @@ 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);
+	wiphy = cfg_to_wiphy(drvr->config);
+	dev_net_set(ndev, wiphy_net(wiphy));
 
 	INIT_WORK(&ifp->multicast_work, _brcmf_set_multicast_list);
 	INIT_WORK(&ifp->ndoffload_work, _brcmf_update_ndtable);
-- 
1.9.1

Re: [RFT] brcmfmac: add support to move wiphy instance into network namespace

[Johannes Berg]

On Tue, 2017-03-14 at 21:51 +0000, Arend van Spriel wrote:
> To support network namespace the driver must assure all created
> network interfaces are in the same namespace as the wiphy instance.

FWIW, looks fine to me.

>  	memcpy(ndev->dev_addr, ifp->mac_addr, ETH_ALEN);
> +	wiphy = cfg_to_wiphy(drvr->config);
> +	dev_net_set(ndev, wiphy_net(wiphy));

You (almost?) don't need the wiphy variable :)

johannes

Re: [RFT] brcmfmac: add support to move wiphy instance into network namespace

[Arend Van Spriel]

On 14-3-2017 23:19, Johannes Berg wrote:
> On Tue, 2017-03-14 at 21:51 +0000, Arend van Spriel wrote:
>> To support network namespace the driver must assure all created
>> network interfaces are in the same namespace as the wiphy instance.
> 
> FWIW, looks fine to me.

Thanks. Any feedback is worth something ;-)

>>  	memcpy(ndev->dev_addr, ifp->mac_addr, ETH_ALEN);
>> +	wiphy = cfg_to_wiphy(drvr->config);
>> +	dev_net_set(ndev, wiphy_net(wiphy));
> 
> You (almost?) don't need the wiphy variable :)

Yeah. I could wrap that into wiphy_net() call as it is the only place I
need it. Will do that if I stay clear from column #80.

Regards,
Arend

Re: [RFT] brcmfmac: add support to move wiphy instance into network namespace

[Mark Asselstine]

On Tuesday, March 14, 2017 9:51:52 PM EDT Arend van Spriel wrote:
> To support network namespace the driver must assure all created
> network interfaces are in the same namespace as the wiphy instance.
> 
> Reported-by: Mark Asselstine <mark.asselstine@windriver.com>
> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> ---
> Hi Mark,
> 
> Please check this patch. I can not say I am an expert when it comes
> to using namespaces. So let me know if it works and I can change
> Reported-by into Tested-by.

Seems to pass the tests I threw at it. Seems happy with namespaces.

Mark

> 
> Regards,
> Arend
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 3 ++-
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c     | 5 ++++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c index
> 3856de6..e0d65df 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -6450,7 +6450,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
> 22b4883..74ede27 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -463,6 +463,7 @@ static int brcmf_netdev_open(struct net_device *ndev)
>  int brcmf_net_attach(struct brcmf_if *ifp, bool rtnl_locked)
>  {
>  	struct brcmf_pub *drvr = ifp->drvr;
> +	struct wiphy *wiphy;
>  	struct net_device *ndev;
>  	s32 err;
> 
> @@ -476,8 +477,10 @@ 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);
> +	wiphy = cfg_to_wiphy(drvr->config);
> +	dev_net_set(ndev, wiphy_net(wiphy));
> 
>  	INIT_WORK(&ifp->multicast_work, _brcmf_set_multicast_list);
>  	INIT_WORK(&ifp->ndoffload_work, _brcmf_update_ndtable);

Re: [RFT] brcmfmac: add support to move wiphy instance into network namespace

[Arend Van Spriel]

On 16-3-2017 22:51, Mark Asselstine wrote:
> On Tuesday, March 14, 2017 9:51:52 PM EDT Arend van Spriel wrote:
>> To support network namespace the driver must assure all created
>> network interfaces are in the same namespace as the wiphy instance.
>>
>> Reported-by: Mark Asselstine <mark.asselstine@windriver.com>
>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>> ---
>> Hi Mark,
>>
>> Please check this patch. I can not say I am an expert when it comes
>> to using namespaces. So let me know if it works and I can change
>> Reported-by into Tested-by.
> 
> Seems to pass the tests I threw at it. Seems happy with namespaces.

Thanks. Will queue it for submission and add Tested-by: tag.

Regards,
Arend

> Mark
> 
>>
>> Regards,
>> Arend
>> ---
>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 3 ++-
>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c     | 5 ++++-
>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c index
>> 3856de6..e0d65df 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> @@ -6450,7 +6450,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
>> 22b4883..74ede27 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> @@ -463,6 +463,7 @@ static int brcmf_netdev_open(struct net_device *ndev)
>>  int brcmf_net_attach(struct brcmf_if *ifp, bool rtnl_locked)
>>  {
>>  	struct brcmf_pub *drvr = ifp->drvr;
>> +	struct wiphy *wiphy;
>>  	struct net_device *ndev;
>>  	s32 err;
>>
>> @@ -476,8 +477,10 @@ 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);
>> +	wiphy = cfg_to_wiphy(drvr->config);
>> +	dev_net_set(ndev, wiphy_net(wiphy));
>>
>>  	INIT_WORK(&ifp->multicast_work, _brcmf_set_multicast_list);
>>  	INIT_WORK(&ifp->ndoffload_work, _brcmf_update_ndtable);
> 
> 

Re: [RFT] brcmfmac: add support to move wiphy instance into network namespace

[Arend Van Spriel]

On 16-3-2017 22:51, Mark Asselstine wrote:
> On Tuesday, March 14, 2017 9:51:52 PM EDT Arend van Spriel wrote:
>> To support network namespace the driver must assure all created
>> network interfaces are in the same namespace as the wiphy instance.
>>
>> Reported-by: Mark Asselstine <mark.asselstine@windriver.com>
>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>> ---
>> Hi Mark,
>>
>> Please check this patch. I can not say I am an expert when it comes
>> to using namespaces. So let me know if it works and I can change
>> Reported-by into Tested-by.
> 
> Seems to pass the tests I threw at it. Seems happy with namespaces.

I tested it myself and noticed something unexpected. Upon changing from
&init_net to brcm-wifi the wdev id has changed from 0x1 to 0x2 (see
below) and upon going from brcm-wifi to &init_net both interface change
their wdev id.

Regards,
Arend

     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 12
                                                *wdev 0x5*
                                        Interface wlan3
                                                ifindex 11
                                                *wdev 0x4*

> Mark
> 
>>
>> Regards,
>> Arend
>> ---
>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 3 ++-
>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c     | 5 ++++-
>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c index
>> 3856de6..e0d65df 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> @@ -6450,7 +6450,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
>> 22b4883..74ede27 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> @@ -463,6 +463,7 @@ static int brcmf_netdev_open(struct net_device *ndev)
>>  int brcmf_net_attach(struct brcmf_if *ifp, bool rtnl_locked)
>>  {
>>  	struct brcmf_pub *drvr = ifp->drvr;
>> +	struct wiphy *wiphy;
>>  	struct net_device *ndev;
>>  	s32 err;
>>
>> @@ -476,8 +477,10 @@ 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);
>> +	wiphy = cfg_to_wiphy(drvr->config);
>> +	dev_net_set(ndev, wiphy_net(wiphy));
>>
>>  	INIT_WORK(&ifp->multicast_work, _brcmf_set_multicast_list);
>>  	INIT_WORK(&ifp->ndoffload_work, _brcmf_update_ndtable);
> 
> 

Re: [RFT] brcmfmac: add support to move wiphy instance into network namespace

[Johannes Berg]

On Fri, 2017-03-17 at 10:06 +0100, Arend Van Spriel wrote:
> On 16-3-2017 22:51, Mark Asselstine wrote:
> > On Tuesday, March 14, 2017 9:51:52 PM EDT Arend van Spriel wrote:
> > > To support network namespace the driver must assure all created
> > > network interfaces are in the same namespace as the wiphy
> > > instance.
> > > 
> > > Reported-by: Mark Asselstine <mark.asselstine@windriver.com>
> > > Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> > > ---
> > > Hi Mark,
> > > 
> > > Please check this patch. I can not say I am an expert when it
> > > comes
> > > to using namespaces. So let me know if it works and I can change
> > > Reported-by into Tested-by.
> > 
> > Seems to pass the tests I threw at it. Seems happy with namespaces.
> 
> I tested it myself and noticed something unexpected. Upon changing
> from &init_net to brcm-wifi the wdev id has changed from 0x1 to 0x2
> (see below) and upon going from brcm-wifi to &init_net both interface
> change their wdev id.

Interesting. That's clearly a cfg80211 bug, does this help?

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 2e1740c7a8bf..d71d5e90229f 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -1183,7 +1183,15 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 		INIT_LIST_HEAD(&wdev->mgmt_registrations);
 		spin_lock_init(&wdev->mgmt_registrations_lock);
 
-		wdev->identifier = ++rdev->wdev_id;
+		/*
+		 * We get here also when the interface changes network namespaces,
+		 * as it's registered into the new one, but we don't want it to
+		 * change ID in that case. Checking if the ID is already assigned
+		 * works, because 0 isn't considered a valid ID and the memory is
+		 * 0-initialized.
+		 */
+		if (!wdev->identifier)
+			wdev->identifier = ++rdev->wdev_id;
 		list_add_rcu(&wdev->list, &rdev->wiphy.wdev_list);
 		rdev->devlist_generation++;
 		/* can only change netns with wiphy */

johannes

Re: [RFT] brcmfmac: add support to move wiphy instance into network namespace

[Arend Van Spriel]

On 17-3-2017 10:06, Arend Van Spriel wrote:
> On 16-3-2017 22:51, Mark Asselstine wrote:
>> On Tuesday, March 14, 2017 9:51:52 PM EDT Arend van Spriel wrote:
>>> To support network namespace the driver must assure all created
>>> network interfaces are in the same namespace as the wiphy instance.
>>>
>>> Reported-by: Mark Asselstine <mark.asselstine@windriver.com>
>>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>>> ---
>>> Hi Mark,
>>>
>>> Please check this patch. I can not say I am an expert when it comes
>>> to using namespaces. So let me know if it works and I can change
>>> Reported-by into Tested-by.
>>
>> Seems to pass the tests I threw at it. Seems happy with namespaces.
> 
> I tested it myself and noticed something unexpected. Upon changing from
> &init_net to brcm-wifi the wdev id has changed from 0x1 to 0x2 (see
> below) and upon going from brcm-wifi to &init_net both interface change
> their wdev id.

Hi Johannes,

Seems we get a NETDEV_REGISTER notification when changing to other
namespace thus increasing the struct wireless_dev::identifier.

# insmod brcmfmac
[253715.650567] brcmfmac: brcmf_c_preinit_dcmds: Firmware version = wl0:
Feb 28 2017 12:44:14 version 7.112.21 (TOB) (r588403) FWID 01-3439bdc5
[253715.669388] brcmfmac: brcmf_cfg80211_reg_notifier: not a ISO3166
code (0x30 0x30)
[253715.698289] ieee80211 phy0: wdev id changed: 0 -> 1
[253715.704032] brcmfmac 0000:02:00.0 wlan3: renamed from wlan0
[253715.732362] systemd-udevd[21461]: renamed network interface wlan0 to
wlan3
[253715.779859] IPv6: ADDRCONF(NETDEV_UP): wlan3: link is not ready
[253721.326349] IPv6: ADDRCONF(NETDEV_CHANGE): wlan3: link becomes ready
# iw phy0 set netns 21499
[253788.877846] ieee80211 phy0: wdev id changed: 1 -> 2
[253788.886710] brcmfmac: brcmf_cfg80211_reg_notifier: not a ISO3166
code (0x30 0x30)

I guess this is intended behavior?

Regards,
Arend

> Regards,
> Arend
> 
>      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 12
>                                                 *wdev 0x5*
>                                         Interface wlan3
>                                                 ifindex 11
>                                                 *wdev 0x4*
> 
>> Mark
>>
>>>
>>> Regards,
>>> Arend
>>> ---
>>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 3 ++-
>>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c     | 5 ++++-
>>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c index
>>> 3856de6..e0d65df 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>> @@ -6450,7 +6450,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
>>> 22b4883..74ede27 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>> @@ -463,6 +463,7 @@ static int brcmf_netdev_open(struct net_device *ndev)
>>>  int brcmf_net_attach(struct brcmf_if *ifp, bool rtnl_locked)
>>>  {
>>>  	struct brcmf_pub *drvr = ifp->drvr;
>>> +	struct wiphy *wiphy;
>>>  	struct net_device *ndev;
>>>  	s32 err;
>>>
>>> @@ -476,8 +477,10 @@ 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);
>>> +	wiphy = cfg_to_wiphy(drvr->config);
>>> +	dev_net_set(ndev, wiphy_net(wiphy));
>>>
>>>  	INIT_WORK(&ifp->multicast_work, _brcmf_set_multicast_list);
>>>  	INIT_WORK(&ifp->ndoffload_work, _brcmf_update_ndtable);
>>
>>

Re: [RFT] brcmfmac: add support to move wiphy instance into network namespace

[Arend Van Spriel]

On 17-3-2017 10:24, Johannes Berg wrote:
> On Fri, 2017-03-17 at 10:06 +0100, Arend Van Spriel wrote:
>> On 16-3-2017 22:51, Mark Asselstine wrote:
>>> On Tuesday, March 14, 2017 9:51:52 PM EDT Arend van Spriel wrote:
>>>> To support network namespace the driver must assure all created
>>>> network interfaces are in the same namespace as the wiphy
>>>> instance.
>>>>
>>>> Reported-by: Mark Asselstine <mark.asselstine@windriver.com>
>>>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>>>> ---
>>>> Hi Mark,
>>>>
>>>> Please check this patch. I can not say I am an expert when it
>>>> comes
>>>> to using namespaces. So let me know if it works and I can change
>>>> Reported-by into Tested-by.
>>>
>>> Seems to pass the tests I threw at it. Seems happy with namespaces.
>>
>> I tested it myself and noticed something unexpected. Upon changing
>> from &init_net to brcm-wifi the wdev id has changed from 0x1 to 0x2
>> (see below) and upon going from brcm-wifi to &init_net both interface
>> change their wdev id.
> 
> Interesting. That's clearly a cfg80211 bug, does this help?
> 
> diff --git a/net/wireless/core.c b/net/wireless/core.c
> index 2e1740c7a8bf..d71d5e90229f 100644
> --- a/net/wireless/core.c
> +++ b/net/wireless/core.c
> @@ -1183,7 +1183,15 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
>  		INIT_LIST_HEAD(&wdev->mgmt_registrations);
>  		spin_lock_init(&wdev->mgmt_registrations_lock);
>  
> -		wdev->identifier = ++rdev->wdev_id;
> +		/*
> +		 * We get here also when the interface changes network namespaces,
> +		 * as it's registered into the new one, but we don't want it to
> +		 * change ID in that case. Checking if the ID is already assigned
> +		 * works, because 0 isn't considered a valid ID and the memory is
> +		 * 0-initialized.
> +		 */
> +		if (!wdev->identifier)
> +			wdev->identifier = ++rdev->wdev_id;

Given my debug session just a minute ago I would say yes ;-) as this is
where I put wiphy_err() statement. Will give it a quick spin.

Regards,
Arend

>  		list_add_rcu(&wdev->list, &rdev->wiphy.wdev_list);
>  		rdev->devlist_generation++;
>  		/* can only change netns with wiphy */
> 
> johannes
> 

Re: [RFT] brcmfmac: add support to move wiphy instance into network namespace

[Arend Van Spriel]

On 17-3-2017 10:24, Johannes Berg wrote:
> On Fri, 2017-03-17 at 10:06 +0100, Arend Van Spriel wrote:
>> On 16-3-2017 22:51, Mark Asselstine wrote:
>>> On Tuesday, March 14, 2017 9:51:52 PM EDT Arend van Spriel wrote:
>>>> To support network namespace the driver must assure all created
>>>> network interfaces are in the same namespace as the wiphy
>>>> instance.
>>>>
>>>> Reported-by: Mark Asselstine <mark.asselstine@windriver.com>
>>>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>>>> ---
>>>> Hi Mark,
>>>>
>>>> Please check this patch. I can not say I am an expert when it
>>>> comes
>>>> to using namespaces. So let me know if it works and I can change
>>>> Reported-by into Tested-by.
>>>
>>> Seems to pass the tests I threw at it. Seems happy with namespaces.
>>
>> I tested it myself and noticed something unexpected. Upon changing
>> from &init_net to brcm-wifi the wdev id has changed from 0x1 to 0x2
>> (see below) and upon going from brcm-wifi to &init_net both interface
>> change their wdev id.
> 
> Interesting. That's clearly a cfg80211 bug, does this help?

Yep. Tested and verified.

Regards,
Arend

> diff --git a/net/wireless/core.c b/net/wireless/core.c
> index 2e1740c7a8bf..d71d5e90229f 100644
> --- a/net/wireless/core.c
> +++ b/net/wireless/core.c
> @@ -1183,7 +1183,15 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
>  		INIT_LIST_HEAD(&wdev->mgmt_registrations);
>  		spin_lock_init(&wdev->mgmt_registrations_lock);
>  
> -		wdev->identifier = ++rdev->wdev_id;
> +		/*
> +		 * We get here also when the interface changes network namespaces,
> +		 * as it's registered into the new one, but we don't want it to
> +		 * change ID in that case. Checking if the ID is already assigned
> +		 * works, because 0 isn't considered a valid ID and the memory is
> +		 * 0-initialized.
> +		 */
> +		if (!wdev->identifier)
> +			wdev->identifier = ++rdev->wdev_id;
>  		list_add_rcu(&wdev->list, &rdev->wiphy.wdev_list);
>  		rdev->devlist_generation++;
>  		/* can only change netns with wiphy */
> 
> johannes
> 

Re: [RFT] brcmfmac: add support to move wiphy instance into network namespace

[Mark Asselstine]

On Friday, March 17, 2017 10:30:24 AM EDT Arend Van Spriel wrote:
> On 17-3-2017 10:06, Arend Van Spriel wrote:
> > On 16-3-2017 22:51, Mark Asselstine wrote:
> >> On Tuesday, March 14, 2017 9:51:52 PM EDT Arend van Spriel wrote:
> >>> To support network namespace the driver must assure all created
> >>> network interfaces are in the same namespace as the wiphy instance.
> >>> 
> >>> Reported-by: Mark Asselstine <mark.asselstine@windriver.com>
> >>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> >>> ---
> >>> Hi Mark,
> >>> 
> >>> Please check this patch. I can not say I am an expert when it comes
> >>> to using namespaces. So let me know if it works and I can change
> >>> Reported-by into Tested-by.
> >> 
> >> Seems to pass the tests I threw at it. Seems happy with namespaces.
> > 
> > I tested it myself and noticed something unexpected. Upon changing from
> > &init_net to brcm-wifi the wdev id has changed from 0x1 to 0x2 (see
> > below) and upon going from brcm-wifi to &init_net both interface change
> > their wdev id.
> 
> Hi Johannes,
> 
> Seems we get a NETDEV_REGISTER notification when changing to other
> namespace thus increasing the struct wireless_dev::identifier.
> 
> # insmod brcmfmac
> [253715.650567] brcmfmac: brcmf_c_preinit_dcmds: Firmware version = wl0:
> Feb 28 2017 12:44:14 version 7.112.21 (TOB) (r588403) FWID 01-3439bdc5
> [253715.669388] brcmfmac: brcmf_cfg80211_reg_notifier: not a ISO3166
> code (0x30 0x30)
> [253715.698289] ieee80211 phy0: wdev id changed: 0 -> 1
> [253715.704032] brcmfmac 0000:02:00.0 wlan3: renamed from wlan0
> [253715.732362] systemd-udevd[21461]: renamed network interface wlan0 to
> wlan3
> [253715.779859] IPv6: ADDRCONF(NETDEV_UP): wlan3: link is not ready
> [253721.326349] IPv6: ADDRCONF(NETDEV_CHANGE): wlan3: link becomes ready
> # iw phy0 set netns 21499
> [253788.877846] ieee80211 phy0: wdev id changed: 1 -> 2
> [253788.886710] brcmfmac: brcmf_cfg80211_reg_notifier: not a ISO3166
> code (0x30 0x30)
> 
> I guess this is intended behavior?

I had thought this was intended behavior as well but I see that a patch is 
already prepped and tested to make this not happen. At any rate it wasn't 
appearing to affect my usecase.

Mark

> 
> Regards,
> Arend
> 
> > Regards,
> > Arend
> > 
> >      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 12
> >                                                 *wdev 0x5*
> >                                         
> >                                         Interface wlan3
> >                                         
> >                                                 ifindex 11
> >                                                 *wdev 0x4*
> >> 
> >> Mark
> >> 
> >>> Regards,
> >>> Arend
> >>> ---
> >>> 
> >>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 3 ++-
> >>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c     | 5 ++++-
> >>>  2 files changed, 6 insertions(+), 2 deletions(-)
> >>> 
> >>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> >>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c index
> >>> 3856de6..e0d65df 100644
> >>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> >>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> >>> @@ -6450,7 +6450,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
> >>> 22b4883..74ede27 100644
> >>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> >>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> >>> @@ -463,6 +463,7 @@ static int brcmf_netdev_open(struct net_device
> >>> *ndev)
> >>> 
> >>>  int brcmf_net_attach(struct brcmf_if *ifp, bool rtnl_locked)
> >>>  {
> >>>  
> >>>  	struct brcmf_pub *drvr = ifp->drvr;
> >>> 
> >>> +	struct wiphy *wiphy;
> >>> 
> >>>  	struct net_device *ndev;
> >>>  	s32 err;
> >>> 
> >>> @@ -476,8 +477,10 @@ 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);
> >>> 
> >>> +	wiphy = cfg_to_wiphy(drvr->config);
> >>> +	dev_net_set(ndev, wiphy_net(wiphy));
> >>> 
> >>>  	INIT_WORK(&ifp->multicast_work, _brcmf_set_multicast_list);
> >>>  	INIT_WORK(&ifp->ndoffload_work, _brcmf_update_ndtable);

Re: [RFT] brcmfmac: add support to move wiphy instance into network namespace

[Johannes Berg]

> > I guess this is intended behavior?
> 
> I had thought this was intended behavior as well but I see that a
> patch is already prepped and tested to make this not happen. At any
> rate it wasn't appearing to affect my usecase.

I can't actually see how it'd affect any usecase, since you really need
to check inside the new netns what's going on etc. anyway, and you
don't really want to pass such identifiers across the boundaries. But
preserving it makes more sense, if only for debugging and making sure
we won't run out of numbers :)

johannes

Re: [RFT] brcmfmac: add support to move wiphy instance into network namespace

[Mark Asselstine]

On Friday, March 17, 2017 1:58:58 PM EDT Johannes Berg wrote:
> > > I guess this is intended behavior?
> > 
> > I had thought this was intended behavior as well but I see that a
> > patch is already prepped and tested to make this not happen. At any
> > rate it wasn't appearing to affect my usecase.
> 
> I can't actually see how it'd affect any usecase, since you really need
> to check inside the new netns what's going on etc. anyway, and you
> don't really want to pass such identifiers across the boundaries. But
> preserving it makes more sense, if only for debugging and making sure
> we won't run out of numbers :)

OK, I can see how preserving this makes sense for debugging. Understood and 
thanks for getting this namespace support in.

Mark

> 
> johannes