Re: brcmfmac and WIPHY_FLAG_NETNS_OK

Re: brcmfmac and WIPHY_FLAG_NETNS_OK

[Arend Van Spriel]

+ linux-wireless
+ Johannes

On 13-3-2017 19:15, Mark Asselstine wrote:
> Hello Arend,
> 
> I am thinking about proposing the following patch on linux-wireless but would 
> like to ask you your thoughts first. I have looked in detail at the kernel 
> code, the brcmfmac development pages, the mailing list archives etc. to make 
> sure that I haven't missed previous discussions on this subject, if you have 
> already ruled this functionality out I apologize for missing the relevant 
> info.
> 
> Like others folks (for example https://github.com/fgg89/docker-ap/issues/3) I 
> would like to have the ability to make the wifi interface available in a 
> container. The *80211 infrastructure should allow this but the brcmfmac driver 
> does not enable WIPHY_FLAG_NETNS_OK so attempts to move the phy and vifs to a 
> network NS results in -EOPNOTSUPP. Is there a reason for this not being 
> supported?

Hi Mark,

It never came up with any projects so far. I doubt that the patch below
is sufficient. I suspect something more is needed. Using git blame I
ended up finding these commits:

a272a72 mac80211: allow using network namespaces
463d018 cfg80211: make aware of net namespaces
5061b0c mac80211: cooperate more with network namespaces

I think what is required from brcmfmac is to set netns for each netdev
that we create to the same netns as the wiphy instance using
wiphy_net(). Not sure if there is more to consider, but hopefully
Johannes can comment on this although the mentioned commits have been
around for a while.

Regards,
Arend

> Thanks so much for taking the time to read this email and thanks in advance 
> for any feedback you are able to provide.
> 
> Regards,
> Mark Asselstine
> 
> ----
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index 944b83c..f38500b 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -6452,7 +6452,8 @@ static int brcmf_setup_wiphy(struct wiphy *wiphy, struct 
> brcmf_if *ifp)
>  
>         wiphy->flags |= WIPHY_FLAG_PS_ON_BY_DEFAULT |
>                         WIPHY_FLAG_OFFCHAN_TX |
> -                       WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL;
> +                       WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL |
> +                       WIPHY_FLAG_NETNS_OK;
>         if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_TDLS))
>                 wiphy->flags |= WIPHY_FLAG_SUPPORTS_TDLS;
>         if (!ifp->drvr->settings->roamoff)
> 

Re: brcmfmac and WIPHY_FLAG_NETNS_OK

[Johannes Berg]

> It never came up with any projects so far. I doubt that the patch
> below is sufficient. I suspect something more is needed. Using git
> blame I ended up finding these commits:
> 
> a272a72 mac80211: allow using network namespaces

This is needed in brcm drivers.

> 463d018 cfg80211: make aware of net namespaces

This has no impact on brcm drivers :)

> 5061b0c mac80211: cooperate more with network namespaces

This shouldn't be needed, you're not referring to init_net in brcm
drivers.

> I think what is required from brcmfmac is to set netns for each
> netdev that we create to the same netns as the wiphy instance using
> wiphy_net().

Yes, like the mac80211 patch above.

> Not sure if there is more to consider, but hopefully Johannes can
> comment on this although the mentioned commits have been around for a
> while.

I don't think there's anything else.

> >         wiphy->flags |= WIPHY_FLAG_PS_ON_BY_DEFAULT |
> >                         WIPHY_FLAG_OFFCHAN_TX |
> > -                       WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL;
> > +                       WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL |
> > +                       WIPHY_FLAG_NETNS_OK;

This is not sufficient, you still have to set the netns for newly
created netdevs.

johannes

Re: brcmfmac and WIPHY_FLAG_NETNS_OK

[Arend Van Spriel]

On 14-3-2017 12:28, Johannes Berg wrote:
> 
>> It never came up with any projects so far. I doubt that the patch
>> below is sufficient. I suspect something more is needed. Using git
>> blame I ended up finding these commits:
>>
>> a272a72 mac80211: allow using network namespaces
> 
> This is needed in brcm drivers.
> 
>> 463d018 cfg80211: make aware of net namespaces
> 
> This has no impact on brcm drivers :)
> 
>> 5061b0c mac80211: cooperate more with network namespaces
> 
> This shouldn't be needed, you're not referring to init_net in brcm
> drivers.
> 
>> I think what is required from brcmfmac is to set netns for each
>> netdev that we create to the same netns as the wiphy instance using
>> wiphy_net().
> 
> Yes, like the mac80211 patch above.
> 
>> Not sure if there is more to consider, but hopefully Johannes can
>> comment on this although the mentioned commits have been around for a
>> while.
> 
> I don't think there's anything else.
> 
>>>         wiphy->flags |= WIPHY_FLAG_PS_ON_BY_DEFAULT |
>>>                         WIPHY_FLAG_OFFCHAN_TX |
>>> -                       WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL;
>>> +                       WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL |
>>> +                       WIPHY_FLAG_NETNS_OK;
> 
> This is not sufficient, you still have to set the netns for newly
> created netdevs.

Thanks for confirming my suspicion.

Regards,
Arend

Re: brcmfmac and WIPHY_FLAG_NETNS_OK

[Mark Asselstine]

On Tuesday, March 14, 2017 1:41:02 PM EDT Arend Van Spriel wrote:
> On 14-3-2017 12:28, Johannes Berg wrote:
> >> It never came up with any projects so far. 

Myself and one of my colleagues had thought that this might be the case. On 
the bright side this matches my inability to find any discussions on the 
matter and that there is the possibility to get this functionality added.

> >> I doubt that the patch
> >> below is sufficient. I suspect something more is needed. Using git
> >> blame I ended up finding these commits:
> >> 
> >> a272a72 mac80211: allow using network namespaces
> > 
> > This is needed in brcm drivers.

I will have a closer look, thanks for the pointer to this patch.

> > 
> >> 463d018 cfg80211: make aware of net namespaces
> > 
> > This has no impact on brcm drivers :)
> > 
> >> 5061b0c mac80211: cooperate more with network namespaces
> > 
> > This shouldn't be needed, you're not referring to init_net in brcm
> > drivers.
> > 
> >> I think what is required from brcmfmac is to set netns for each
> >> netdev that we create to the same netns as the wiphy instance using
> >> wiphy_net().
> > 
> > Yes, like the mac80211 patch above.
> > 
> >> Not sure if there is more to consider, but hopefully Johannes can
> >> comment on this although the mentioned commits have been around for a
> >> while.
> > 
> > I don't think there's anything else.
> > 
> >>>         wiphy->flags |= WIPHY_FLAG_PS_ON_BY_DEFAULT |
> >>>         
> >>>                         WIPHY_FLAG_OFFCHAN_TX |
> >>> 
> >>> -                       WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL;
> >>> +                       WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL |
> >>> +                       WIPHY_FLAG_NETNS_OK;
> > 
> > This is not sufficient, you still have to set the netns for newly
> > created netdevs.

Ah, that is something I had not tested out. I moved an existing phy and its 
associated vif and things worked as expected and also destroying the NS had 
things move back as expected. I did not however create any new vifs

> 
> Thanks for confirming my suspicion.
> 
> Regards,
> Arend

Thanks for the quick pointers, not being all that familiar with the wireless 
code I appreciate the discussion. I will have a closer look and see if I can 
get a patch out for review.

Mark

Re: brcmfmac and WIPHY_FLAG_NETNS_OK

[Arend Van Spriel]

On 14-3-2017 14:21, Mark Asselstine wrote:
> On Tuesday, March 14, 2017 1:41:02 PM EDT Arend Van Spriel wrote:
>> On 14-3-2017 12:28, Johannes Berg wrote:
>>>> It never came up with any projects so far. 
> 
> Myself and one of my colleagues had thought that this might be the case. On 
> the bright side this matches my inability to find any discussions on the 
> matter and that there is the possibility to get this functionality added.
> 
>>>> I doubt that the patch
>>>> below is sufficient. I suspect something more is needed. Using git
>>>> blame I ended up finding these commits:
>>>>
>>>> a272a72 mac80211: allow using network namespaces
>>>
>>> This is needed in brcm drivers.
> 
> I will have a closer look, thanks for the pointer to this patch.
> 
>>>
>>>> 463d018 cfg80211: make aware of net namespaces
>>>
>>> This has no impact on brcm drivers :)
>>>
>>>> 5061b0c mac80211: cooperate more with network namespaces
>>>
>>> This shouldn't be needed, you're not referring to init_net in brcm
>>> drivers.
>>>
>>>> I think what is required from brcmfmac is to set netns for each
>>>> netdev that we create to the same netns as the wiphy instance using
>>>> wiphy_net().
>>>
>>> Yes, like the mac80211 patch above.
>>>
>>>> Not sure if there is more to consider, but hopefully Johannes can
>>>> comment on this although the mentioned commits have been around for a
>>>> while.
>>>
>>> I don't think there's anything else.
>>>
>>>>>         wiphy->flags |= WIPHY_FLAG_PS_ON_BY_DEFAULT |
>>>>>         
>>>>>                         WIPHY_FLAG_OFFCHAN_TX |
>>>>>
>>>>> -                       WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL;
>>>>> +                       WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL |
>>>>> +                       WIPHY_FLAG_NETNS_OK;
>>>
>>> This is not sufficient, you still have to set the netns for newly
>>> created netdevs.
> 
> Ah, that is something I had not tested out. I moved an existing phy and its 
> associated vif and things worked as expected and also destroying the NS had 
> things move back as expected. I did not however create any new vifs
> 
>>
>> Thanks for confirming my suspicion.
>>
>> Regards,
>> Arend
> 
> Thanks for the quick pointers, not being all that familiar with the wireless 
> code I appreciate the discussion. I will have a closer look and see if I can 
> get a patch out for review.

Hi Mark,

I looked at it and preparing a patch. Will send it out shortly to give
it a try.

Regards,
Arend

Re: brcmfmac and WIPHY_FLAG_NETNS_OK

[Mark Asselstine]

On Tuesday, March 14, 2017 2:27:53 PM EDT Arend Van Spriel wrote:
> On 14-3-2017 14:21, Mark Asselstine wrote:
> > On Tuesday, March 14, 2017 1:41:02 PM EDT Arend Van Spriel wrote:
> >> On 14-3-2017 12:28, Johannes Berg wrote:
> >>>> It never came up with any projects so far.
> > 
> > Myself and one of my colleagues had thought that this might be the case.
> > On
> > the bright side this matches my inability to find any discussions on the
> > matter and that there is the possibility to get this functionality added.
> > 
> >>>> I doubt that the patch
> >>>> below is sufficient. I suspect something more is needed. Using git
> >>>> blame I ended up finding these commits:
> >>>> 
> >>>> a272a72 mac80211: allow using network namespaces
> >>> 
> >>> This is needed in brcm drivers.
> > 
> > I will have a closer look, thanks for the pointer to this patch.
> > 
> >>>> 463d018 cfg80211: make aware of net namespaces
> >>> 
> >>> This has no impact on brcm drivers :)
> >>> 
> >>>> 5061b0c mac80211: cooperate more with network namespaces
> >>> 
> >>> This shouldn't be needed, you're not referring to init_net in brcm
> >>> drivers.
> >>> 
> >>>> I think what is required from brcmfmac is to set netns for each
> >>>> netdev that we create to the same netns as the wiphy instance using
> >>>> wiphy_net().
> >>> 
> >>> Yes, like the mac80211 patch above.
> >>> 
> >>>> Not sure if there is more to consider, but hopefully Johannes can
> >>>> comment on this although the mentioned commits have been around for a
> >>>> while.
> >>> 
> >>> I don't think there's anything else.
> >>> 
> >>>>>         wiphy->flags |= WIPHY_FLAG_PS_ON_BY_DEFAULT |
> >>>>>         
> >>>>>                         WIPHY_FLAG_OFFCHAN_TX |
> >>>>> 
> >>>>> -                       WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL;
> >>>>> +                       WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL |
> >>>>> +                       WIPHY_FLAG_NETNS_OK;
> >>> 
> >>> This is not sufficient, you still have to set the netns for newly
> >>> created netdevs.
> > 
> > Ah, that is something I had not tested out. I moved an existing phy and
> > its
> > associated vif and things worked as expected and also destroying the NS
> > had
> > things move back as expected. I did not however create any new vifs
> > 
> >> Thanks for confirming my suspicion.
> >> 
> >> Regards,
> >> Arend
> > 
> > Thanks for the quick pointers, not being all that familiar with the
> > wireless code I appreciate the discussion. I will have a closer look and
> > see if I can get a patch out for review.
> 
> Hi Mark,
> 
> I looked at it and preparing a patch. Will send it out shortly to give
> it a try.

Oh perfect thanks. I wasn't intending to create work for you but I will not 
protest. Your expert eye might catch things I would have missed, only to be 
caught in review.

Mark

> 
> Regards,
> Arend