From: Andrei Emeltchenko <[email protected]>
RFC for Software / Virtual _DRAFT_ implementation of Bluetooth High Speed
by using wireless driver.
Currently it can be used for testing Bluetooth A2MP protocol.
Please comment is this right way of implementing things like: using virtual
interface to enable AMP. Shall I use module instead? Shall I put files to
mac80211 or use other location? Is name "vamp" OK? Other variants are:
softamp, soft_amp, soft_amp80211, etc.
Main ideas:
- Use existing wireless infrastructure nl80211, mac80211.
- Use virtual interface of type NL80211_IFTYPE_VIRTUAL_AMP80211. Then all
drivers supporting virtual interface should work.
After modifying iw it will be enabled by adding new virtual interface.
# iw phy <physical device> interface add vamp type vamp
# iw dev
phy#0
Interface vamp
ifindex 6
type Virtual AMP
...
# hciconfig
hci1: Type: AMP Bus: VIRTUAL
BD Address: 00:00:00:00:00:00 ACL MTU: 0:0 SCO MTU: 0:0
DOWN
RX bytes:0 acl:0 sco:0 events:0 errors:0
TX bytes:0 acl:0 sco:0 commands:0 errors:0
...
Andrei Emeltchenko (1):
mac80211: Adds Software / Virtual AMP 80211
drivers/net/wireless/mac80211_hwsim.c | 3 +-
include/linux/nl80211.h | 1 +
net/mac80211/Kconfig | 8 ++
net/mac80211/Makefile | 2 +
net/mac80211/ieee80211_i.h | 4 +
net/mac80211/iface.c | 25 ++++
net/mac80211/util.c | 1 +
net/mac80211/virtual_amp.c | 205 +++++++++++++++++++++++++++++++++
net/mac80211/virtual_amp.h | 29 +++++
9 files changed, 277 insertions(+), 1 deletions(-)
create mode 100644 net/mac80211/virtual_amp.c
create mode 100644 net/mac80211/virtual_amp.h
--
1.7.9.1
On 4/19/2012 6:39 AM, Andrei Emeltchenko wrote:
>> You cannot use more than one channel today. We are changing that. To
>> do good channel management, rather than hard-coding it in your
>> config, you want to run a single wpa_s controlling all interfaces.
>
> Can wpa_supplicant be an AP? I though that functionality is split to
> hostap and wpa_supplicant.
Yes, it can.
johannes
Hi Johannes,
On Thu, Apr 19, 2012 at 06:28:09AM -0700, Johannes Berg wrote:
> On 4/18/2012 11:36 PM, Andrei Emeltchenko wrote:
> >>>>No, this is incorrect. If one device wants to connect on one
> >>>>channel, the other typically has to use the same channel. If one
> >>>>device wants to scan, the other will be affected. Some hardware may
> >>>>support switching around between two channels, but might also
> >>>>support more than 2 virtual interfaces, so again they won't be
> >>>>independent.
> >>>
> >>>BTW: which devices can switch channels?
> >>
> >>None today, I'm working on it.
> >
> >If this is not possible how people setup wireless hotspot using virtual
> >interfaces and run wpa_supplicant and hostapd on each separately.
> >Like here:
> >
> >http://linuxalfi.wordpress.com/2011/11/08/connectify-for-linux-with-single-wireless-interface/
>
> Of course it's possible. Are you deliberately misunderstanding me?
>
> You cannot use more than one channel today. We are changing that. To
> do good channel management, rather than hard-coding it in your
> config, you want to run a single wpa_s controlling all interfaces.
Can wpa_supplicant be an AP? I though that functionality is split to
hostap and wpa_supplicant.
Best regards
Andrei Emeltchenko
On 4/18/2012 11:36 PM, Andrei Emeltchenko wrote:
>>>> No, this is incorrect. If one device wants to connect on one
>>>> channel, the other typically has to use the same channel. If one
>>>> device wants to scan, the other will be affected. Some hardware may
>>>> support switching around between two channels, but might also
>>>> support more than 2 virtual interfaces, so again they won't be
>>>> independent.
>>>
>>> BTW: which devices can switch channels?
>>
>> None today, I'm working on it.
>
> If this is not possible how people setup wireless hotspot using virtual
> interfaces and run wpa_supplicant and hostapd on each separately.
> Like here:
>
> http://linuxalfi.wordpress.com/2011/11/08/connectify-for-linux-with-single-wireless-interface/
Of course it's possible. Are you deliberately misunderstanding me?
You cannot use more than one channel today. We are changing that. To do
good channel management, rather than hard-coding it in your config, you
want to run a single wpa_s controlling all interfaces.
johannes
Hi Johannes,
On Wed, Apr 18, 2012 at 08:09:07AM -0700, Johannes Berg wrote:
> Hi Andrei,
>
> >>No, this is incorrect. If one device wants to connect on one
> >>channel, the other typically has to use the same channel. If one
> >>device wants to scan, the other will be affected. Some hardware may
> >>support switching around between two channels, but might also
> >>support more than 2 virtual interfaces, so again they won't be
> >>independent.
> >
> >BTW: which devices can switch channels?
>
> None today, I'm working on it.
If this is not possible how people setup wireless hotspot using virtual
interfaces and run wpa_supplicant and hostapd on each separately.
Like here:
http://linuxalfi.wordpress.com/2011/11/08/connectify-for-linux-with-single-wireless-interface/
Best regards
Andrei Emeltchenko
Andrei and Johannes -
On Wed, 18 Apr 2012, Johannes Berg wrote:
> Hi Andrei,
>
>>> No, this is incorrect. If one device wants to connect on one
>>> channel, the other typically has to use the same channel. If one
>>> device wants to scan, the other will be affected. Some hardware may
>>> support switching around between two channels, but might also
>>> support more than 2 virtual interfaces, so again they won't be
>>> independent.
>>
>> BTW: which devices can switch channels?
>
> None today, I'm working on it.
>
>>> Therefore, you need something managing all this concurrency. This is
>>> in a small part the driver which will enforce restrictions (it will
>>> reject new impossible things), but mostly the supplicant which can
>>> make policy decisions about which usage should win.
>>
>> This doesn't sound like a rocket science to me. IMO this might be done in
>> drivers. Those drivers which can switch channels why do they need
>> wpa_supplicant involved making this decision?
>
> I don't think you understand.
>
> Say our device can do 3 virtual interfaces, on 2 channels. Then if the user
> is connected to some managed network (say office network), one interface &
> channel is used up. Now the user has AMP running, another channel might be
> used up. Now the user wants to do P2P negotiation. Now the supplicant, which
> is doing the negotiation, needs to know that it can negotiate only one of
> those two channels, not any other. Or maybe P2P should win, then it might
> disconnect the AMP or the managed connection. But all those are policy
> decisions, so the driver can't really handle them.
I concur with Johannes...
All of the AMP PALs I've seen are restricted to using the current
wireless channel if a wireless interface is active. The consequence
is that if you have two BT3.0+HS devices, and their wireless
interfaces are associated with different APs on different channels,
then they are unable to use AMP.
Trying to manage two active channels with a single-radio wireless
devices is non-trivial -- there may be simultaneous beacons on
different channels, for example. I suppose it could be a different
situation when a wireless device has independent radios for 2.4GHz and
5GHz bands, but there's still a need to make decisions about which
channel(s) to use and how they are to be shared.
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi Andrei,
>> No, this is incorrect. If one device wants to connect on one
>> channel, the other typically has to use the same channel. If one
>> device wants to scan, the other will be affected. Some hardware may
>> support switching around between two channels, but might also
>> support more than 2 virtual interfaces, so again they won't be
>> independent.
>
> BTW: which devices can switch channels?
None today, I'm working on it.
>> Therefore, you need something managing all this concurrency. This is
>> in a small part the driver which will enforce restrictions (it will
>> reject new impossible things), but mostly the supplicant which can
>> make policy decisions about which usage should win.
>
> This doesn't sound like a rocket science to me. IMO this might be done in
> drivers. Those drivers which can switch channels why do they need
> wpa_supplicant involved making this decision?
I don't think you understand.
Say our device can do 3 virtual interfaces, on 2 channels. Then if the
user is connected to some managed network (say office network), one
interface & channel is used up. Now the user has AMP running, another
channel might be used up. Now the user wants to do P2P negotiation. Now
the supplicant, which is doing the negotiation, needs to know that it
can negotiate only one of those two channels, not any other. Or maybe
P2P should win, then it might disconnect the AMP or the managed
connection. But all those are policy decisions, so the driver can't
really handle them.
johannes
Hi Marcel,
On Wed, Apr 18, 2012 at 04:29:55PM +0200, Marcel Holtmann wrote:
> > > > > > If we create virtual interface then netdev is allocated and we can handle
> > > > > > it with common tools.
> > > > >
> > > > > what common tools do you wanna use here. We actually do not wanna have
> > > > > them show up as netdev devices at all. AMP connections don't have IP
> > > > > addresses or anything else for that matter.
> > > >
> > > > I might want to set up MAC address. Otherwise I might not need netdev. I
> > > > feel that I need it for interface separation.
> > >
> > > what MAC address? And where do you get this MAC address from? AMP
> > > controllers do not have MAC addresses.
> >
> > I assume I need some address to send/receive data over wireless.
>
> from a Bluetooth point, we do not have to deal with the MAC address.
> When looking at the mac80211 part, I do not know. However even then,
> that address should come from the driver and not from userspace.
The MAC address coming from driver might be already used in default
wireless interface. I am not very familiar with wireless devices, maybe
they have pool of MAC addresses we might choose from ...
My point here: we create virtual interface and separate it from other
wireless interfaces => hence we do not need to worry (much) about
concurrency between wireless interfaces. Those interfaces need to have
separate MAC addresses assigned if I understand virtual interfaces concept
right.
Best regards
Andrei Emeltchenko
Hi Johannes,
> >> Why do we need netlink commands like NL80211_CMD_HCI_AMP_ADD and
> >> NL80211_CMD_HCI_AMP_DELETE if what we need is to create/delete virtual
> >> interface which can be done with standard tools with a several lines
> >> patch to iw:
> >
> > if we put the crypto piece aside for a minute, then we need to decide
> > who is creating the actual AMP virtual interface on mac80211.
> >
> > And here the problem starts. That part is actually not triggered from
> > userspace via wpa_supplicant. It is triggered over the A2MP (AMP manager
> > protocol) that runs inside the Bluetooth stack in the kernel.
> >
> > Or is the idea to always keep the AMP virtual interface around? Meaning
> > that as soon as we have a mac80211 card, we have an AMP virtual
> > interface if the driver supports it?
> >
> > This is also a little bit of confusing since FullMac cards will not
> > create an AMP virtual interface. With them you just see the HCI AMP
> > controller on the Bluetooth side. Can an AMP virtual interface be just
> > virtual inside mac80211 or does it have to have a netdev attached to it?
>
> It shouldn't have a netdev, hence the separate commands (though I'll
> admit it's also possible to use the same commands, if a bit confusing).
> However, wpa_s would probably keep it around for use by BT whenever it
> didn't conflict with other wifi usage, e.g. when a device like ours has
> a limited number of MAC contexts you can create and use.
actually we can keep it around all the time. With A2MP we can signal
availability of the AMP controller or not. So Bluetooth can have
multiple AMP controllers available, but they can signal the other side
(over Bluetooth BR/EDR) that they don't have resources right now.
So I don't know why wpa_supplicant should create the AMP controller in
the first place. We can just always create it. And then have a command
for wpa_supplicant to signal if one has free resources or not.
> >> I see that all commands by default are sent via netlink to wpa_supplicant.
> >> I think that we can send those command which cannot be handled by us
> >> directly but I believe most command might be handled directly.
> >
> > The crypto itself is done either in hardware or with the kernel crypto
> > framework. Just the EAP part is done inside wpa_supplicant.
> >
> > With the changes that we did for Bluetooth and its management interface,
> > all the link keys are present in the kernel. And these are used as the
> > WPA2 PSK. I don't think it is a good idea to push these around into
> > userspace to wpa_supplicant one more time. And we would need to do that
> > since bluetoothd has no option to hand them out.
>
> I don't see that as a problem, since they're just HCI commands and the
> kernel just has to sort them into "for management" and "for data path",
> the former going to userspace.
We do not wanna do HCI emulation inside wpa_supplicant. That sounds like
a pretty bad idea. We wanna do that inside the kernel. And if we end up
creating an LMP layer (SoftMAC for Bluetooth) inside the kernel, I wanna
share the code there.
> > I still think that WPA2 PSK only EAP implementation for Bluetooth AMP
> > controllers would be a good idea in the kernel. It has nothing to do
> > with policy or user input in this specific case. The roundtrip into
> > userspace for doing EAP 4-way handshake and some HMAC-SHA1 where the PSK
> > is already present in kernel space seems really pointless.
>
> I disagree -- I think rewriting crypto code is almost always a bad idea,
> and code reuse is good.
It is an EAP 4-way handshake. That is not even a handful of messages and
it is not really crypto code.
Anyhow for the sake of argument, say we would push the link key via
netlink into wpa_supplicant. How do we secure it?
> Beside this though, we need wpa_s managing the concurrency aspect
> anyway, so even if we had it in the kernel it wouldn't really change much.
See above. That can be easily done with an extra nl80211 command that
wpa_supplicant gives to mac80211 and then handles A2MP to manage this.
In the worst case we can always be drastic and just move the Bluetooth
link back onto BR/EDR. AMP logical links can come and go as we please.
Regards
Marcel
Hi Johannes,
On Wed, Apr 18, 2012 at 07:38:08AM -0700, Johannes Berg wrote:
> On 4/18/2012 5:15 AM, Andrei Emeltchenko wrote:
> >>It's never in your best interest to run one supplicant per virtual
> >>interface, you want to run one per piece of hardware so it can
> >>coordinate between virtual interfaces.
> >
> >Do I understand it right that virtual interfaces are like separate devices from
> >user point of view? If I create virtual interface and assign MAC I can
> >transfer data through it without affecting other interfaces? Then
> >wpa_supplicant does not need to know about it at all.
>
> No, this is incorrect. If one device wants to connect on one
> channel, the other typically has to use the same channel. If one
> device wants to scan, the other will be affected. Some hardware may
> support switching around between two channels, but might also
> support more than 2 virtual interfaces, so again they won't be
> independent.
BTW: which devices can switch channels?
> Therefore, you need something managing all this concurrency. This is
> in a small part the driver which will enforce restrictions (it will
> reject new impossible things), but mostly the supplicant which can
> make policy decisions about which usage should win.
This doesn't sound like a rocket science to me. IMO this might be done in
drivers. Those drivers which can switch channels why do they need
wpa_supplicant involved making this decision?
Best regards
Andrei Emeltchenko
On 4/18/2012 5:15 AM, Andrei Emeltchenko wrote:
>> It's never in your best interest to run one supplicant per virtual
>> interface, you want to run one per piece of hardware so it can
>> coordinate between virtual interfaces.
>
> Do I understand it right that virtual interfaces are like separate devices from
> user point of view? If I create virtual interface and assign MAC I can
> transfer data through it without affecting other interfaces? Then
> wpa_supplicant does not need to know about it at all.
No, this is incorrect. If one device wants to connect on one channel,
the other typically has to use the same channel. If one device wants to
scan, the other will be affected. Some hardware may support switching
around between two channels, but might also support more than 2 virtual
interfaces, so again they won't be independent.
Therefore, you need something managing all this concurrency. This is in
a small part the driver which will enforce restrictions (it will reject
new impossible things), but mostly the supplicant which can make policy
decisions about which usage should win.
johannes
On 4/18/2012 4:51 AM, Marcel Holtmann wrote:
>> Why do we need netlink commands like NL80211_CMD_HCI_AMP_ADD and
>> NL80211_CMD_HCI_AMP_DELETE if what we need is to create/delete virtual
>> interface which can be done with standard tools with a several lines
>> patch to iw:
>
> if we put the crypto piece aside for a minute, then we need to decide
> who is creating the actual AMP virtual interface on mac80211.
>
> And here the problem starts. That part is actually not triggered from
> userspace via wpa_supplicant. It is triggered over the A2MP (AMP manager
> protocol) that runs inside the Bluetooth stack in the kernel.
>
> Or is the idea to always keep the AMP virtual interface around? Meaning
> that as soon as we have a mac80211 card, we have an AMP virtual
> interface if the driver supports it?
>
> This is also a little bit of confusing since FullMac cards will not
> create an AMP virtual interface. With them you just see the HCI AMP
> controller on the Bluetooth side. Can an AMP virtual interface be just
> virtual inside mac80211 or does it have to have a netdev attached to it?
It shouldn't have a netdev, hence the separate commands (though I'll
admit it's also possible to use the same commands, if a bit confusing).
However, wpa_s would probably keep it around for use by BT whenever it
didn't conflict with other wifi usage, e.g. when a device like ours has
a limited number of MAC contexts you can create and use.
>> I see that all commands by default are sent via netlink to wpa_supplicant.
>> I think that we can send those command which cannot be handled by us
>> directly but I believe most command might be handled directly.
>
> The crypto itself is done either in hardware or with the kernel crypto
> framework. Just the EAP part is done inside wpa_supplicant.
>
> With the changes that we did for Bluetooth and its management interface,
> all the link keys are present in the kernel. And these are used as the
> WPA2 PSK. I don't think it is a good idea to push these around into
> userspace to wpa_supplicant one more time. And we would need to do that
> since bluetoothd has no option to hand them out.
I don't see that as a problem, since they're just HCI commands and the
kernel just has to sort them into "for management" and "for data path",
the former going to userspace.
> I still think that WPA2 PSK only EAP implementation for Bluetooth AMP
> controllers would be a good idea in the kernel. It has nothing to do
> with policy or user input in this specific case. The roundtrip into
> userspace for doing EAP 4-way handshake and some HMAC-SHA1 where the PSK
> is already present in kernel space seems really pointless.
I disagree -- I think rewriting crypto code is almost always a bad idea,
and code reuse is good.
Beside this though, we need wpa_s managing the concurrency aspect
anyway, so even if we had it in the kernel it wouldn't really change much.
johannes
On 4/18/2012 4:20 AM, Andrei Emeltchenko wrote:
> Hi Johannes,
>
> On Tue, Apr 17, 2012 at 07:07:21PM -0700, Johannes Berg wrote:
>>>>>>> I don't get this patch at all. Why am I reviewing some very very basic
>>>>>>> skeleton code when we should be discussing userspace APIs (we have
>>>>>>> already discussed them with a few people years ago), how the AMP is
>>>>>>> going to be managed, how the security handshake is going to work, etc.
>>>
>>> Do we have some outcome from that discussion?
>>
>> This API-defining patch is probably the best we have:
>> http://johannes.sipsolutions.net/patches/kernel/all/2010-10-13-15%
>> 3a24/035-bt3-amp.patch
>
> Thanks for the link. After looking to the patches I think that there are
> some similarities with respect to interface type. As I understood the
> basic idea is the same: create virtual interface. But in your case the
> implementation is really difficult.
>
> Why do we need netlink commands like NL80211_CMD_HCI_AMP_ADD and
> NL80211_CMD_HCI_AMP_DELETE if what we need is to create/delete virtual
> interface which can be done with standard tools with a several lines
> patch to iw:
[...]
That would work, but the plan was to have separate commands because
those commands create a virtual netdev, which we don't want in this case.
And no, given the lack of userspace tools we never tested these patches.
I was more of a "write down API thoughts in code" thing.
johannes
Hi Andrei,
> > > > > > This is also a little bit of confusing since FullMac cards will not
> > > > > > create an AMP virtual interface. With them you just see the HCI AMP
> > > > > > controller on the Bluetooth side. Can an AMP virtual interface be just
> > > > > > virtual inside mac80211 or does it have to have a netdev attached to it?
> > > > >
> > > > > If we create virtual interface then netdev is allocated and we can handle
> > > > > it with common tools.
> > > >
> > > > what common tools do you wanna use here. We actually do not wanna have
> > > > them show up as netdev devices at all. AMP connections don't have IP
> > > > addresses or anything else for that matter.
> > >
> > > I might want to set up MAC address. Otherwise I might not need netdev. I
> > > feel that I need it for interface separation.
> >
> > what MAC address? And where do you get this MAC address from? AMP
> > controllers do not have MAC addresses.
>
> I assume I need some address to send/receive data over wireless.
from a Bluetooth point, we do not have to deal with the MAC address.
When looking at the mac80211 part, I do not know. However even then,
that address should come from the driver and not from userspace.
Regards
Marcel
Hi Marcel,
On Wed, Apr 18, 2012 at 03:11:57PM +0200, Marcel Holtmann wrote:
> > > > > This is also a little bit of confusing since FullMac cards will not
> > > > > create an AMP virtual interface. With them you just see the HCI AMP
> > > > > controller on the Bluetooth side. Can an AMP virtual interface be just
> > > > > virtual inside mac80211 or does it have to have a netdev attached to it?
> > > >
> > > > If we create virtual interface then netdev is allocated and we can handle
> > > > it with common tools.
> > >
> > > what common tools do you wanna use here. We actually do not wanna have
> > > them show up as netdev devices at all. AMP connections don't have IP
> > > addresses or anything else for that matter.
> >
> > I might want to set up MAC address. Otherwise I might not need netdev. I
> > feel that I need it for interface separation.
>
> what MAC address? And where do you get this MAC address from? AMP
> controllers do not have MAC addresses.
I assume I need some address to send/receive data over wireless.
Best regards
Andrei Emeltchenko
Hi Andrei,
> > > > This is also a little bit of confusing since FullMac cards will not
> > > > create an AMP virtual interface. With them you just see the HCI AMP
> > > > controller on the Bluetooth side. Can an AMP virtual interface be just
> > > > virtual inside mac80211 or does it have to have a netdev attached to it?
> > >
> > > If we create virtual interface then netdev is allocated and we can handle
> > > it with common tools.
> >
> > what common tools do you wanna use here. We actually do not wanna have
> > them show up as netdev devices at all. AMP connections don't have IP
> > addresses or anything else for that matter.
>
> I might want to set up MAC address. Otherwise I might not need netdev. I
> feel that I need it for interface separation.
what MAC address? And where do you get this MAC address from? AMP
controllers do not have MAC addresses.
Regards
Marcel
Hi Marcel,
On Wed, Apr 18, 2012 at 02:15:59PM +0200, Marcel Holtmann wrote:
> > > This is also a little bit of confusing since FullMac cards will not
> > > create an AMP virtual interface. With them you just see the HCI AMP
> > > controller on the Bluetooth side. Can an AMP virtual interface be just
> > > virtual inside mac80211 or does it have to have a netdev attached to it?
> >
> > If we create virtual interface then netdev is allocated and we can handle
> > it with common tools.
>
> what common tools do you wanna use here. We actually do not wanna have
> them show up as netdev devices at all. AMP connections don't have IP
> addresses or anything else for that matter.
I might want to set up MAC address. Otherwise I might not need netdev. I
feel that I need it for interface separation.
Best regards
Andrei Emeltchenko
Hi Andrei,
<snip>
> > This is also a little bit of confusing since FullMac cards will not
> > create an AMP virtual interface. With them you just see the HCI AMP
> > controller on the Bluetooth side. Can an AMP virtual interface be just
> > virtual inside mac80211 or does it have to have a netdev attached to it?
>
> If we create virtual interface then netdev is allocated and we can handle
> it with common tools.
what common tools do you wanna use here. We actually do not wanna have
them show up as netdev devices at all. AMP connections don't have IP
addresses or anything else for that matter. We do not even need the
statistics here since they will be counted within HCI AMP controller in
the Bluetooth subsystem.
If you wanna do IP over AMP, it would be via BNEP. And the netdev you
are getting would come as bnep0 from the Bluetooth stack and not via the
mac80211 stack.
Regards
Marcel
Hi Johannes,
On Tue, Apr 17, 2012 at 07:03:31PM -0700, Johannes Berg wrote:
> > I am new to the wireless code but can new virtual interface separate those
> > interfaces? They can have different MAC addresses and you can run
> > wpa_supplicant for each virtual interface separately.
>
> It's never in your best interest to run one supplicant per virtual
> interface, you want to run one per piece of hardware so it can
> coordinate between virtual interfaces.
Do I understand it right that virtual interfaces are like separate devices from
user point of view? If I create virtual interface and assign MAC I can
transfer data through it without affecting other interfaces? Then
wpa_supplicant does not need to know about it at all.
Best regards
Andrei Emeltchenko
Hi Marcel,
On Wed, Apr 18, 2012 at 01:51:19PM +0200, Marcel Holtmann wrote:
> Hi Andrei,
>
> > > > > > >> I don't get this patch at all. Why am I reviewing some very very basic
> > > > > > >> skeleton code when we should be discussing userspace APIs (we have
> > > > > > >> already discussed them with a few people years ago), how the AMP is
> > > > > > >> going to be managed, how the security handshake is going to work, etc.
> > > >
> > > > Do we have some outcome from that discussion?
> > >
> > > This API-defining patch is probably the best we have:
> > > http://johannes.sipsolutions.net/patches/kernel/all/2010-10-13-15%
> > > 3a24/035-bt3-amp.patch
> >
> > Thanks for the link. After looking to the patches I think that there are
> > some similarities with respect to interface type. As I understood the
> > basic idea is the same: create virtual interface. But in your case the
> > implementation is really difficult.
> >
> > Why do we need netlink commands like NL80211_CMD_HCI_AMP_ADD and
> > NL80211_CMD_HCI_AMP_DELETE if what we need is to create/delete virtual
> > interface which can be done with standard tools with a several lines
> > patch to iw:
>
> if we put the crypto piece aside for a minute, then we need to decide
> who is creating the actual AMP virtual interface on mac80211.
I think we can start with creating softAMP in advance.
>
> And here the problem starts. That part is actually not triggered from
> userspace via wpa_supplicant. It is triggered over the A2MP (AMP manager
> protocol) that runs inside the Bluetooth stack in the kernel.
A2MP might work without AMP present on the system. Do we need to create
AMP after "Discover AMP" requests? I believe we might be too smart here.
but this is possibly.
> Or is the idea to always keep the AMP virtual interface around? Meaning
> that as soon as we have a mac80211 card, we have an AMP virtual
> interface if the driver supports it?
IMO this shall be the case.
> This is also a little bit of confusing since FullMac cards will not
> create an AMP virtual interface. With them you just see the HCI AMP
> controller on the Bluetooth side. Can an AMP virtual interface be just
> virtual inside mac80211 or does it have to have a netdev attached to it?
If we create virtual interface then netdev is allocated and we can handle
it with common tools.
> <snip>
>
> > > > > The whole AMP control goes via A2MP and L2CAP and both are fully
> > > > > implemented inside the kernel. In theory we do not even need to expose
> > > > > HCI AMP interfaces to userspace.
> > > >
> > > > Johannes, you can think of SoftAMP as analog of SoftMAC (vs FullMAC).
> > > > SoftMAC is also possible to implement in user space but only
> > > > authentication is done this way.
> > >
> > > Yeah, and we also implement roaming and crypto stuff in userspace, for
> > > softmac. Heck, we implement crypto in userspace even for fullmac, so
> > > really ...
> >
> > Does crypto stuff mean getting symmetric key?
> >
> > I see that all commands by default are sent via netlink to wpa_supplicant.
> > I think that we can send those command which cannot be handled by us
> > directly but I believe most command might be handled directly.
>
> The crypto itself is done either in hardware or with the kernel crypto
> framework. Just the EAP part is done inside wpa_supplicant.
>
> With the changes that we did for Bluetooth and its management interface,
> all the link keys are present in the kernel. And these are used as the
> WPA2 PSK. I don't think it is a good idea to push these around into
> userspace to wpa_supplicant one more time. And we would need to do that
> since bluetoothd has no option to hand them out.
>
> I still think that WPA2 PSK only EAP implementation for Bluetooth AMP
> controllers would be a good idea in the kernel. It has nothing to do
> with policy or user input in this specific case. The roundtrip into
> userspace for doing EAP 4-way handshake and some HMAC-SHA1 where the PSK
> is already present in kernel space seems really pointless.
I do agree here with Marcel.
Best regards
Andrei Emeltchenko
Hi Andrei,
> > > > > >> I don't get this patch at all. Why am I reviewing some very very basic
> > > > > >> skeleton code when we should be discussing userspace APIs (we have
> > > > > >> already discussed them with a few people years ago), how the AMP is
> > > > > >> going to be managed, how the security handshake is going to work, etc.
> > >
> > > Do we have some outcome from that discussion?
> >
> > This API-defining patch is probably the best we have:
> > http://johannes.sipsolutions.net/patches/kernel/all/2010-10-13-15%
> > 3a24/035-bt3-amp.patch
>
> Thanks for the link. After looking to the patches I think that there are
> some similarities with respect to interface type. As I understood the
> basic idea is the same: create virtual interface. But in your case the
> implementation is really difficult.
>
> Why do we need netlink commands like NL80211_CMD_HCI_AMP_ADD and
> NL80211_CMD_HCI_AMP_DELETE if what we need is to create/delete virtual
> interface which can be done with standard tools with a several lines
> patch to iw:
if we put the crypto piece aside for a minute, then we need to decide
who is creating the actual AMP virtual interface on mac80211.
And here the problem starts. That part is actually not triggered from
userspace via wpa_supplicant. It is triggered over the A2MP (AMP manager
protocol) that runs inside the Bluetooth stack in the kernel.
Or is the idea to always keep the AMP virtual interface around? Meaning
that as soon as we have a mac80211 card, we have an AMP virtual
interface if the driver supports it?
This is also a little bit of confusing since FullMac cards will not
create an AMP virtual interface. With them you just see the HCI AMP
controller on the Bluetooth side. Can an AMP virtual interface be just
virtual inside mac80211 or does it have to have a netdev attached to it?
<snip>
> > > > The whole AMP control goes via A2MP and L2CAP and both are fully
> > > > implemented inside the kernel. In theory we do not even need to expose
> > > > HCI AMP interfaces to userspace.
> > >
> > > Johannes, you can think of SoftAMP as analog of SoftMAC (vs FullMAC).
> > > SoftMAC is also possible to implement in user space but only
> > > authentication is done this way.
> >
> > Yeah, and we also implement roaming and crypto stuff in userspace, for
> > softmac. Heck, we implement crypto in userspace even for fullmac, so
> > really ...
>
> Does crypto stuff mean getting symmetric key?
>
> I see that all commands by default are sent via netlink to wpa_supplicant.
> I think that we can send those command which cannot be handled by us
> directly but I believe most command might be handled directly.
The crypto itself is done either in hardware or with the kernel crypto
framework. Just the EAP part is done inside wpa_supplicant.
With the changes that we did for Bluetooth and its management interface,
all the link keys are present in the kernel. And these are used as the
WPA2 PSK. I don't think it is a good idea to push these around into
userspace to wpa_supplicant one more time. And we would need to do that
since bluetoothd has no option to hand them out.
I still think that WPA2 PSK only EAP implementation for Bluetooth AMP
controllers would be a good idea in the kernel. It has nothing to do
with policy or user input in this specific case. The roundtrip into
userspace for doing EAP 4-way handshake and some HMAC-SHA1 where the PSK
is already present in kernel space seems really pointless.
Regards
Marcel
Hi Johannes,
On Tue, Apr 17, 2012 at 07:07:21PM -0700, Johannes Berg wrote:
> > > > >> I don't get this patch at all. Why am I reviewing some very very basic
> > > > >> skeleton code when we should be discussing userspace APIs (we have
> > > > >> already discussed them with a few people years ago), how the AMP is
> > > > >> going to be managed, how the security handshake is going to work, etc.
> >
> > Do we have some outcome from that discussion?
>
> This API-defining patch is probably the best we have:
> http://johannes.sipsolutions.net/patches/kernel/all/2010-10-13-15%
> 3a24/035-bt3-amp.patch
Thanks for the link. After looking to the patches I think that there are
some similarities with respect to interface type. As I understood the
basic idea is the same: create virtual interface. But in your case the
implementation is really difficult.
Why do we need netlink commands like NL80211_CMD_HCI_AMP_ADD and
NL80211_CMD_HCI_AMP_DELETE if what we need is to create/delete virtual
interface which can be done with standard tools with a several lines
patch to iw:
<------8<---------------------------------------------------------------------
| diff --git a/interface.c b/interface.c
| index 6c90f9d..49227ce 100644
| --- a/interface.c
| +++ b/interface.c
| @@ -136,6 +136,9 @@ static int get_if_type(int *argc, char ***argv, enum
| nl80211_iftype *type,
| } else if (strcmp(tpstr, "__p2pgo") == 0) {
| *type = NL80211_IFTYPE_P2P_GO;
| return 0;
| + } else if (strcmp(tpstr, "vamp") == 0) {
| + *type = NL80211_IFTYPE_VIRTUAL_AMP;
| + return 0;
| }
|
| fprintf(stderr, "invalid interface type %s\n", tpstr);
| diff --git a/nl80211.h b/nl80211.h
| index e474f6e..ad4a252 100644
| --- a/nl80211.h
| +++ b/nl80211.h
| @@ -1546,6 +1546,7 @@ enum nl80211_iftype {
| NL80211_IFTYPE_MESH_POINT,
| NL80211_IFTYPE_P2P_CLIENT,
| NL80211_IFTYPE_P2P_GO,
| + NL80211_IFTYPE_VIRTUAL_AMP,
|
| /* keep last */
| NUM_NL80211_IFTYPES,
| diff --git a/util.c b/util.c
| index 103ded9..bf2fefe 100644
| --- a/util.c
| +++ b/util.c
| @@ -132,6 +132,7 @@ static const char *ifmodes[NL80211_IFTYPE_MAX + 1] = {
| "mesh point",
| "P2P-client",
| "P2P-GO",
| + "Virtual AMP",
| };
|
| static char modebuf[100];
|
<------8<---------------------------------------------------------------------
Anyway I think that this is not an issue since those commands can be
easily added.
> > > The whole AMP control goes via A2MP and L2CAP and both are fully
> > > implemented inside the kernel. In theory we do not even need to expose
> > > HCI AMP interfaces to userspace.
> >
> > Johannes, you can think of SoftAMP as analog of SoftMAC (vs FullMAC).
> > SoftMAC is also possible to implement in user space but only
> > authentication is done this way.
>
> Yeah, and we also implement roaming and crypto stuff in userspace, for
> softmac. Heck, we implement crypto in userspace even for fullmac, so
> really ...
Does crypto stuff mean getting symmetric key?
I see that all commands by default are sent via netlink to wpa_supplicant.
I think that we can send those command which cannot be handled by us
directly but I believe most command might be handled directly.
> > Consider use case when user sends data over Bluetooth High Speed. Data
> > go from obex user space to kernel L2CAP. Then you just need to add
> > MAC header and send to wireless device. But you are proposing to copy data
> > to user space for processing; then user space needs to copy data again to
> > kernel and then to wireless device.
>
> I never said data should be copied. If you thought I did, you
> misunderstood me.
I see now that data are sent directly through ieee80211_tx_skb and
received through ieee80211_deliver_skb. BTW: Have you tested those patches?
Best regards
Andrei Emeltchenko
Hi,
> > > >> I don't get this patch at all. Why am I reviewing some very very basic
> > > >> skeleton code when we should be discussing userspace APIs (we have
> > > >> already discussed them with a few people years ago), how the AMP is
> > > >> going to be managed, how the security handshake is going to work, etc.
>
> Do we have some outcome from that discussion?
This API-defining patch is probably the best we have:
http://johannes.sipsolutions.net/patches/kernel/all/2010-10-13-15%
3a24/035-bt3-amp.patch
> > The whole AMP control goes via A2MP and L2CAP and both are fully
> > implemented inside the kernel. In theory we do not even need to expose
> > HCI AMP interfaces to userspace.
>
> Johannes, you can think of SoftAMP as analog of SoftMAC (vs FullMAC).
> SoftMAC is also possible to implement in user space but only
> authentication is done this way.
Yeah, and we also implement roaming and crypto stuff in userspace, for
softmac. Heck, we implement crypto in userspace even for fullmac, so
really ...
> Consider use case when user sends data over Bluetooth High Speed. Data
> go from obex user space to kernel L2CAP. Then you just need to add
> MAC header and send to wireless device. But you are proposing to copy data
> to user space for processing; then user space needs to copy data again to
> kernel and then to wireless device.
I never said data should be copied. If you thought I did, you
misunderstood me.
> I think that user does not need to know that it uses High Speed, it just
> notice that speed is better :). Do you require any special API for
> latest and greatest wireless standard? Why user shall care about it?
Well, actually, yes, most new wireless standards do require new API for
the supplicant to be able to use them. The *user* never needs to know
about it of course -- consider the supplicant part of the wireless
stack.
johannes
Hi Andrei,
Sorry for the delay. I wrote answers to a lot of email on Friday morning
but they all disappeared into the void -- not really sure what happened.
> I am new to the wireless code but can new virtual interface separate those
> interfaces? They can have different MAC addresses and you can run
> wpa_supplicant for each virtual interface separately.
It's never in your best interest to run one supplicant per virtual
interface, you want to run one per piece of hardware so it can
coordinate between virtual interfaces.
johannes
Hi Johannes,
On Tue, Apr 10, 2012 at 02:24:58PM -0700, Johannes Berg wrote:
> On 4/10/2012 2:20 PM, Johannes Berg wrote:
> >>>I don't get this patch at all. Why am I reviewing some very very basic
> >>>skeleton code when we should be discussing userspace APIs (we have
> >>>already discussed them with a few people years ago), how the AMP is
> >>>going to be managed, how the security handshake is going to work, etc.
> >>
> >>adding AMP (meaning Bluetooth Alternate MAC/PHY in case anybody cares)
> >>for SoftMac WiFi cards should be done solely in kernel space between
> >>Bluetooth core and mac80211. All the FullMac cards will expose the HCI
> >>AMP directly via the Bluetooth core. See Marvell solution for example.
> >>
> >>If we require a userspace interaction, I think we are doing something
> >>wrong here. And as far as I can tell, the only tricky part is the WPA2
> >>PSK 4-way handshake. We would need a kernel implementation for that.
> >
> >You already know I disagree, I don't want this code re-implemented in
> >kernel space when adding a few tightly controlled APIs is all it needs
> >to use an existing implementation of the relevant mechanisms.
>
> There are also additional complexities like wpa_supplicant having to
> know whether to set up/tear down a BT AMP interface for P2P
> concurrency etc.,
I am new to the wireless code but can new virtual interface separate those
interfaces? They can have different MAC addresses and you can run
wpa_supplicant for each virtual interface separately.
Best regards
Andrei Emeltchenko
Hi Johannes and Marcel,
On Tue, Apr 10, 2012 at 11:29:38PM +0200, Marcel Holtmann wrote:
> Hi Johannes,
>
> > >> I don't get this patch at all. Why am I reviewing some very very basic
> > >> skeleton code when we should be discussing userspace APIs (we have
> > >> already discussed them with a few people years ago), how the AMP is
> > >> going to be managed, how the security handshake is going to work, etc.
Do we have some outcome from that discussion?
> > > adding AMP (meaning Bluetooth Alternate MAC/PHY in case anybody cares)
> > > for SoftMac WiFi cards should be done solely in kernel space between
> > > Bluetooth core and mac80211. All the FullMac cards will expose the HCI
> > > AMP directly via the Bluetooth core. See Marvell solution for example.
Also Qualcomm.
> > > If we require a userspace interaction, I think we are doing something
> > > wrong here. And as far as I can tell, the only tricky part is the WPA2
> > > PSK 4-way handshake. We would need a kernel implementation for that.
> >
> > You already know I disagree, I don't want this code re-implemented in
> > kernel space when adding a few tightly controlled APIs is all it needs
> > to use an existing implementation of the relevant mechanisms.
>
> I know that, but I still think it is the right approach here. It might
> take me a bit longer to convince you ;)
>
> The whole AMP control goes via A2MP and L2CAP and both are fully
> implemented inside the kernel. In theory we do not even need to expose
> HCI AMP interfaces to userspace.
Johannes, you can think of SoftAMP as analog of SoftMAC (vs FullMAC).
SoftMAC is also possible to implement in user space but only
authentication is done this way.
Consider use case when user sends data over Bluetooth High Speed. Data
go from obex user space to kernel L2CAP. Then you just need to add
MAC header and send to wireless device. But you are proposing to copy data
to user space for processing; then user space needs to copy data again to
kernel and then to wireless device.
I think that user does not need to know that it uses High Speed, it just
notice that speed is better :). Do you require any special API for
latest and greatest wireless standard? Why user shall care about it?
Best regards
Andrei Emeltchenko
> We just do it for convince right now so
> we can sniff the transfers, but even that is no longer needed with the
> addition of the Bluetooth monitor socket.
>
> Anyway, my real point is that we should not need any extra userspace API
> to add support for Bluetooth HS in mac80211.
>
> Regards
>
> Marcel
>
>
Hi Johannes,
> >> I don't get this patch at all. Why am I reviewing some very very basic
> >> skeleton code when we should be discussing userspace APIs (we have
> >> already discussed them with a few people years ago), how the AMP is
> >> going to be managed, how the security handshake is going to work, etc.
> >
> > adding AMP (meaning Bluetooth Alternate MAC/PHY in case anybody cares)
> > for SoftMac WiFi cards should be done solely in kernel space between
> > Bluetooth core and mac80211. All the FullMac cards will expose the HCI
> > AMP directly via the Bluetooth core. See Marvell solution for example.
> >
> > If we require a userspace interaction, I think we are doing something
> > wrong here. And as far as I can tell, the only tricky part is the WPA2
> > PSK 4-way handshake. We would need a kernel implementation for that.
>
> You already know I disagree, I don't want this code re-implemented in
> kernel space when adding a few tightly controlled APIs is all it needs
> to use an existing implementation of the relevant mechanisms.
I know that, but I still think it is the right approach here. It might
take me a bit longer to convince you ;)
The whole AMP control goes via A2MP and L2CAP and both are fully
implemented inside the kernel. In theory we do not even need to expose
HCI AMP interfaces to userspace. We just do it for convince right now so
we can sniff the transfers, but even that is no longer needed with the
addition of the Bluetooth monitor socket.
Anyway, my real point is that we should not need any extra userspace API
to add support for Bluetooth HS in mac80211.
Regards
Marcel
On 4/10/2012 2:20 PM, Johannes Berg wrote:
>>> I don't get this patch at all. Why am I reviewing some very very basic
>>> skeleton code when we should be discussing userspace APIs (we have
>>> already discussed them with a few people years ago), how the AMP is
>>> going to be managed, how the security handshake is going to work, etc.
>>
>> adding AMP (meaning Bluetooth Alternate MAC/PHY in case anybody cares)
>> for SoftMac WiFi cards should be done solely in kernel space between
>> Bluetooth core and mac80211. All the FullMac cards will expose the HCI
>> AMP directly via the Bluetooth core. See Marvell solution for example.
>>
>> If we require a userspace interaction, I think we are doing something
>> wrong here. And as far as I can tell, the only tricky part is the WPA2
>> PSK 4-way handshake. We would need a kernel implementation for that.
>
> You already know I disagree, I don't want this code re-implemented in
> kernel space when adding a few tightly controlled APIs is all it needs
> to use an existing implementation of the relevant mechanisms.
There are also additional complexities like wpa_supplicant having to
know whether to set up/tear down a BT AMP interface for P2P concurrency
etc., so it's not really a good idea anyway to implement it purely in
kernel space and hope it'll all work out.
(My flight is boarding now so I can't elaborate or discuss right now.)
johannes
>> I don't get this patch at all. Why am I reviewing some very very basic
>> skeleton code when we should be discussing userspace APIs (we have
>> already discussed them with a few people years ago), how the AMP is
>> going to be managed, how the security handshake is going to work, etc.
>
> adding AMP (meaning Bluetooth Alternate MAC/PHY in case anybody cares)
> for SoftMac WiFi cards should be done solely in kernel space between
> Bluetooth core and mac80211. All the FullMac cards will expose the HCI
> AMP directly via the Bluetooth core. See Marvell solution for example.
>
> If we require a userspace interaction, I think we are doing something
> wrong here. And as far as I can tell, the only tricky part is the WPA2
> PSK 4-way handshake. We would need a kernel implementation for that.
You already know I disagree, I don't want this code re-implemented in
kernel space when adding a few tightly controlled APIs is all it needs
to use an existing implementation of the relevant mechanisms.
johannes
Hi Johannes,
<snip>
> Anyway.
>
> I don't get this patch at all. Why am I reviewing some very very basic
> skeleton code when we should be discussing userspace APIs (we have
> already discussed them with a few people years ago), how the AMP is
> going to be managed, how the security handshake is going to work, etc.
adding AMP (meaning Bluetooth Alternate MAC/PHY in case anybody cares)
for SoftMac WiFi cards should be done solely in kernel space between
Bluetooth core and mac80211. All the FullMac cards will expose the HCI
AMP directly via the Bluetooth core. See Marvell solution for example.
If we require a userspace interaction, I think we are doing something
wrong here. And as far as I can tell, the only tricky part is the WPA2
PSK 4-way handshake. We would need a kernel implementation for that.
Especially since the key material is actually derived from the Bluetooth
link key that we have available in kernel space already.
Regards
Marcel
Hi Andrei,
> Add new interface type VIRTUAL_AMP80211 which emulates Bluetooth AMP
> Controller. AMP is Alternate MAC/PHYs Controller for Bluetooth sybsystem.
> When an AMP is common between the two devices, the Bluetooth system
> provides mechanisms for moving data traffic from BR/EDR Controller to
> an AMP Controller.
Interesting work. I would have expected to see more work in cfg80211
though, this is a bit unexpected. How is it controlled at all?
> create mode 100644 net/mac80211/virtual_amp.c
Also, why is it "virtual"? I would rather you name the file btamp.c or
such -- to you, AMP may mean something, but to us wifi people it really
doesn't mean much. I suspect most wouldn't even know where to look for a
definition of it.
> --- a/drivers/net/wireless/mac80211_hwsim.c
> +++ b/drivers/net/wireless/mac80211_hwsim.c
> @@ -1783,7 +1783,8 @@ static int __init init_mac80211_hwsim(void)
> BIT(NL80211_IFTYPE_P2P_CLIENT) |
> BIT(NL80211_IFTYPE_P2P_GO) |
> BIT(NL80211_IFTYPE_ADHOC) |
> - BIT(NL80211_IFTYPE_MESH_POINT);
> + BIT(NL80211_IFTYPE_MESH_POINT) |
> + BIT(NL80211_IFTYPE_VIRTUAL_AMP80211);
This is insufficient, it should also beacon.
> +++ b/include/linux/nl80211.h
> @@ -1546,6 +1546,7 @@ enum nl80211_iftype {
> NL80211_IFTYPE_MESH_POINT,
> NL80211_IFTYPE_P2P_CLIENT,
> NL80211_IFTYPE_P2P_GO,
> + NL80211_IFTYPE_VIRTUAL_AMP80211,
I believe that this may need additional checks in cfg80211 so that you
can't simply add this interface type.
> +config MAC80211_VIRTUAL_AMP
> + bool "Virtual AMP80211 device"
> + ---help---
> + Enable Bluetooth Virtual / Software AMP 80211 controller.
> + When AMP is common between two devices data may be routed
> + through fast 80211 connection from standard Bluetooth BR/EDR
802.11, and the whole virtual vs. Bluetooth thing again.
Also, it seems this really needs a dependency on BT.
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index d9798a3..0e39ed7 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -709,6 +709,10 @@ struct ieee80211_sub_if_data {
> u32 rc_rateidx_mask[IEEE80211_NUM_BANDS];
> u8 rc_rateidx_mcs_mask[IEEE80211_NUM_BANDS][IEEE80211_HT_MCS_MASK_LEN];
>
> +#ifdef CONFIG_MAC80211_VIRTUAL_AMP
> + struct hci_dev *hdev;
> +#endif
That should be inside the union:
> union {
> struct ieee80211_if_ap ap;
> struct ieee80211_if_wds wds;
> @@ -898,6 +900,11 @@ static void ieee80211_setup_sdata(struct ieee80211_sub_if_data *sdata,
> case NL80211_IFTYPE_WDS:
> case NL80211_IFTYPE_AP_VLAN:
> break;
> + case NL80211_IFTYPE_VIRTUAL_AMP80211:
> +#ifdef CONFIG_MAC80211_VIRTUAL_AMP
> + ieee80211_vamp_setup_sdata(sdata);
I also really don't like the abbrevation "vamp". It means even less than
"virtual AMP", particularly for people not familiar with BT terminology.
Please also use btamp or so.
> +static void ieee80211_clean_sdata(struct ieee80211_sub_if_data *sdata)
> +{
> + switch (sdata->vif.type) {
> + case NL80211_IFTYPE_VIRTUAL_AMP80211:
> +#ifdef CONFIG_MAC80211_VIRTUAL_AMP
> + ieee80211_vamp_clean_sdata(sdata);
> +#endif
That's really bad form. Please hide the ifdefs better, or use an if
construct with something like vif_is_mesh() below that will not need ifdefs:
> @@ -1230,6 +1250,9 @@ void ieee80211_if_remove(struct ieee80211_sub_if_data *sdata)
> if (ieee80211_vif_is_mesh(&sdata->vif))
> mesh_path_flush_by_iface(sdata);
>
> + /* clean up type-dependent data */
> + ieee80211_clean_sdata(sdata);
Seems you should move the mesh pieces into the new function ... Possibly
as a previous refactor patch.
> --- a/net/mac80211/util.c
> +++ b/net/mac80211/util.c
> @@ -1299,6 +1299,7 @@ int ieee80211_reconfig(struct ieee80211_local *local)
> case NUM_NL80211_IFTYPES:
> case NL80211_IFTYPE_P2P_CLIENT:
> case NL80211_IFTYPE_P2P_GO:
> + case NL80211_IFTYPE_VIRTUAL_AMP80211:
> WARN_ON(1);
Umm. This can happen, I'd say.
> +#include<net/bluetooth/bluetooth.h>
I'm not sure I fully trust thunderbird, but there seem to be missing spaces.
> +static int virtual_amp_init(struct ieee80211_sub_if_data *sdata)
> +{
> + struct hci_dev *hdev;
> + struct vamp_data *data;
> +
> + data = kzalloc(sizeof(struct vamp_data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
Why can't you embed the data in the union inside the sdata? it's
probably smaller than managed anyway.
> + hdev->bus = HCI_VIRTUAL;
Should that really be that way?
> + hci_set_drvdata(hdev, data);
> +
> + hdev->dev_type = HCI_AMP;
> +
> + hdev->open = vamp_open_dev;
> + hdev->close = vamp_close_dev;
> + hdev->flush = vamp_flush;
> + hdev->send = vamp_send_frame;
> +
> + if (hci_register_dev(hdev)< 0) {
> + BT_ERR("Can't register HCI device");
> + kfree(data);
> + hci_free_dev(hdev);
> + return -EBUSY;
> + }
Why go have return values when you don't use them:
> +void ieee80211_vamp_setup_sdata(struct ieee80211_sub_if_data *sdata)
> +{
> + pr_info("Create virtual AMP device");
> +
> + virtual_amp_init(sdata);
> +
> +}
Anyway.
I don't get this patch at all. Why am I reviewing some very very basic
skeleton code when we should be discussing userspace APIs (we have
already discussed them with a few people years ago), how the AMP is
going to be managed, how the security handshake is going to work, etc.
johannes
Hi Julian,
Thanks for review,
On Tue, Apr 10, 2012 at 10:26:11PM +1000, Julian Calaby wrote:
> > --- a/net/mac80211/iface.c
> > +++ b/net/mac80211/iface.c
> > @@ -898,6 +900,11 @@ static void ieee80211_setup_sdata(struct ieee80211_sub_if_data *sdata,
> > ? ? ? ?case NL80211_IFTYPE_WDS:
> > ? ? ? ?case NL80211_IFTYPE_AP_VLAN:
> > ? ? ? ? ? ? ? ?break;
> > + ? ? ? case NL80211_IFTYPE_VIRTUAL_AMP80211:
> > +#ifdef CONFIG_MAC80211_VIRTUAL_AMP
> > + ? ? ? ? ? ? ? ieee80211_vamp_setup_sdata(sdata);
> > + ? ? ? ? ? ? ? break;
> > +#endif
> > ? ? ? ?case NL80211_IFTYPE_UNSPECIFIED:
> > ? ? ? ?case NUM_NL80211_IFTYPES:
> > ? ? ? ? ? ? ? ?BUG();
>
> Should this really BUG() if VAMP is not enabled? Maybe print a warning
> and return an error instead.
I will make it empty inline function.
> > diff --git a/net/mac80211/virtual_amp.h b/net/mac80211/virtual_amp.h
> > new file mode 100644
> > index 0000000..5b7d219
> > --- /dev/null
> > +++ b/net/mac80211/virtual_amp.h
> > @@ -0,0 +1,29 @@
> > +/*
> > + * Virtual / Software AMP 80211 BT Controller header
> > + *
> > + * Copyright 2012 Intel Corp.
> > + *
> > + * Written by [email protected]
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include "ieee80211_i.h"
> > +
> > +#ifdef CONFIG_MAC80211_VIRTUAL_AMP
> > +
> > +void ieee80211_vamp_setup_sdata(struct ieee80211_sub_if_data *sdata);
> > +void ieee80211_vamp_clean_sdata(struct ieee80211_sub_if_data *sdata);
> > +
> > +struct vamp_data {
> > + ? ? ? struct hci_dev *hdev;
> > +
> > + ? ? ? unsigned long flags;
> > +
> > + ? ? ? struct work_struct work;
> > + ? ? ? struct sk_buff_head txq;
> > +};
> > +
> > +#endif /* CONFIG_MAC80211_VIRTUAL_AMP */
>
> A cleaner way of doing this is to have the function prototypes
> specified as empty inline functions when CONFIG_MAC80211_VIRTUAL_AMP
> is not defined and remove the #ifdefs in the main code. GCC's smart
> enough to compile away all the empty functions.
Will do this way.
Best regards
Andrei Emeltchenko
Hi Andrei,
A couple of minor nits:
On Tue, Apr 10, 2012 at 22:11, Andrei Emeltchenko
<[email protected]> wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> Add new interface type VIRTUAL_AMP80211 which emulates Bluetooth AMP
> Controller. AMP is Alternate MAC/PHYs Controller for Bluetooth sybsystem.
> When an AMP is common between the two devices, the Bluetooth system
> provides mechanisms for moving data traffic from BR/EDR Controller to
> an AMP Controller.
>
> Signed-off-by: Andrei Emeltchenko <[email protected]>
> ---
> diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
> index 401c01f..0718cc4 100644
> --- a/net/mac80211/iface.c
> +++ b/net/mac80211/iface.c
> @@ -898,6 +900,11 @@ static void ieee80211_setup_sdata(struct ieee80211_sub_if_data *sdata,
> ? ? ? ?case NL80211_IFTYPE_WDS:
> ? ? ? ?case NL80211_IFTYPE_AP_VLAN:
> ? ? ? ? ? ? ? ?break;
> + ? ? ? case NL80211_IFTYPE_VIRTUAL_AMP80211:
> +#ifdef CONFIG_MAC80211_VIRTUAL_AMP
> + ? ? ? ? ? ? ? ieee80211_vamp_setup_sdata(sdata);
> + ? ? ? ? ? ? ? break;
> +#endif
> ? ? ? ?case NL80211_IFTYPE_UNSPECIFIED:
> ? ? ? ?case NUM_NL80211_IFTYPES:
> ? ? ? ? ? ? ? ?BUG();
Should this really BUG() if VAMP is not enabled? Maybe print a warning
and return an error instead.
> diff --git a/net/mac80211/virtual_amp.h b/net/mac80211/virtual_amp.h
> new file mode 100644
> index 0000000..5b7d219
> --- /dev/null
> +++ b/net/mac80211/virtual_amp.h
> @@ -0,0 +1,29 @@
> +/*
> + * Virtual / Software AMP 80211 BT Controller header
> + *
> + * Copyright 2012 Intel Corp.
> + *
> + * Written by [email protected]
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include "ieee80211_i.h"
> +
> +#ifdef CONFIG_MAC80211_VIRTUAL_AMP
> +
> +void ieee80211_vamp_setup_sdata(struct ieee80211_sub_if_data *sdata);
> +void ieee80211_vamp_clean_sdata(struct ieee80211_sub_if_data *sdata);
> +
> +struct vamp_data {
> + ? ? ? struct hci_dev *hdev;
> +
> + ? ? ? unsigned long flags;
> +
> + ? ? ? struct work_struct work;
> + ? ? ? struct sk_buff_head txq;
> +};
> +
> +#endif /* CONFIG_MAC80211_VIRTUAL_AMP */
A cleaner way of doing this is to have the function prototypes
specified as empty inline functions when CONFIG_MAC80211_VIRTUAL_AMP
is not defined and remove the #ifdefs in the main code. GCC's smart
enough to compile away all the empty functions.
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/
From: Andrei Emeltchenko <[email protected]>
Add new interface type VIRTUAL_AMP80211 which emulates Bluetooth AMP
Controller. AMP is Alternate MAC/PHYs Controller for Bluetooth sybsystem.
When an AMP is common between the two devices, the Bluetooth system
provides mechanisms for moving data traffic from BR/EDR Controller to
an AMP Controller.
Signed-off-by: Andrei Emeltchenko <[email protected]>
---
drivers/net/wireless/mac80211_hwsim.c | 3 +-
include/linux/nl80211.h | 1 +
net/mac80211/Kconfig | 8 ++
net/mac80211/Makefile | 2 +
net/mac80211/ieee80211_i.h | 4 +
net/mac80211/iface.c | 25 ++++
net/mac80211/util.c | 1 +
net/mac80211/virtual_amp.c | 205 +++++++++++++++++++++++++++++++++
net/mac80211/virtual_amp.h | 29 +++++
9 files changed, 277 insertions(+), 1 deletions(-)
create mode 100644 net/mac80211/virtual_amp.c
create mode 100644 net/mac80211/virtual_amp.h
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index b7ce6a6..c98b775 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -1783,7 +1783,8 @@ static int __init init_mac80211_hwsim(void)
BIT(NL80211_IFTYPE_P2P_CLIENT) |
BIT(NL80211_IFTYPE_P2P_GO) |
BIT(NL80211_IFTYPE_ADHOC) |
- BIT(NL80211_IFTYPE_MESH_POINT);
+ BIT(NL80211_IFTYPE_MESH_POINT) |
+ BIT(NL80211_IFTYPE_VIRTUAL_AMP80211);
hw->flags = IEEE80211_HW_MFP_CAPABLE |
IEEE80211_HW_SIGNAL_DBM |
diff --git a/include/linux/nl80211.h b/include/linux/nl80211.h
index e474f6e..5f2b84b 100644
--- a/include/linux/nl80211.h
+++ b/include/linux/nl80211.h
@@ -1546,6 +1546,7 @@ enum nl80211_iftype {
NL80211_IFTYPE_MESH_POINT,
NL80211_IFTYPE_P2P_CLIENT,
NL80211_IFTYPE_P2P_GO,
+ NL80211_IFTYPE_VIRTUAL_AMP80211,
/* keep last */
NUM_NL80211_IFTYPES,
diff --git a/net/mac80211/Kconfig b/net/mac80211/Kconfig
index 96ddb72..6658721 100644
--- a/net/mac80211/Kconfig
+++ b/net/mac80211/Kconfig
@@ -67,6 +67,14 @@ config MAC80211_RC_DEFAULT_MINSTREL
endchoice
+config MAC80211_VIRTUAL_AMP
+ bool "Virtual AMP80211 device"
+ ---help---
+ Enable Bluetooth Virtual / Software AMP 80211 controller.
+ When AMP is common between two devices data may be routed
+ through fast 80211 connection from standard Bluetooth BR/EDR
+ connection.
+
config MAC80211_RC_DEFAULT
string
default "minstrel_ht" if MAC80211_RC_DEFAULT_MINSTREL && MAC80211_RC_MINSTREL_HT
diff --git a/net/mac80211/Makefile b/net/mac80211/Makefile
index 1be7a45..cda3c08 100644
--- a/net/mac80211/Makefile
+++ b/net/mac80211/Makefile
@@ -40,6 +40,8 @@ mac80211-$(CONFIG_MAC80211_MESH) += \
mesh_plink.o \
mesh_hwmp.o
+mac80211-$(CONFIG_MAC80211_VIRTUAL_AMP) += virtual_amp.o
+
mac80211-$(CONFIG_PM) += pm.o
CFLAGS_driver-trace.o := -I$(src)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index d9798a3..0e39ed7 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -709,6 +709,10 @@ struct ieee80211_sub_if_data {
u32 rc_rateidx_mask[IEEE80211_NUM_BANDS];
u8 rc_rateidx_mcs_mask[IEEE80211_NUM_BANDS][IEEE80211_HT_MCS_MASK_LEN];
+#ifdef CONFIG_MAC80211_VIRTUAL_AMP
+ struct hci_dev *hdev;
+#endif
+
union {
struct ieee80211_if_ap ap;
struct ieee80211_if_wds wds;
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 401c01f..0718cc4 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -25,6 +25,7 @@
#include "driver-ops.h"
#include "wme.h"
#include "rate.h"
+#include "virtual_amp.h"
/**
* DOC: Interface list locking
@@ -217,6 +218,7 @@ static int ieee80211_do_open(struct net_device *dev, bool coming_up)
case NUM_NL80211_IFTYPES:
case NL80211_IFTYPE_P2P_CLIENT:
case NL80211_IFTYPE_P2P_GO:
+ case NL80211_IFTYPE_VIRTUAL_AMP80211:
/* cannot happen */
WARN_ON(1);
break;
@@ -898,6 +900,11 @@ static void ieee80211_setup_sdata(struct ieee80211_sub_if_data *sdata,
case NL80211_IFTYPE_WDS:
case NL80211_IFTYPE_AP_VLAN:
break;
+ case NL80211_IFTYPE_VIRTUAL_AMP80211:
+#ifdef CONFIG_MAC80211_VIRTUAL_AMP
+ ieee80211_vamp_setup_sdata(sdata);
+ break;
+#endif
case NL80211_IFTYPE_UNSPECIFIED:
case NUM_NL80211_IFTYPES:
BUG();
@@ -907,6 +914,19 @@ static void ieee80211_setup_sdata(struct ieee80211_sub_if_data *sdata,
ieee80211_debugfs_add_netdev(sdata);
}
+static void ieee80211_clean_sdata(struct ieee80211_sub_if_data *sdata)
+{
+ switch (sdata->vif.type) {
+ case NL80211_IFTYPE_VIRTUAL_AMP80211:
+#ifdef CONFIG_MAC80211_VIRTUAL_AMP
+ ieee80211_vamp_clean_sdata(sdata);
+#endif
+ break;
+ default:
+ break;
+ }
+}
+
static int ieee80211_runtime_change_iftype(struct ieee80211_sub_if_data *sdata,
enum nl80211_iftype type)
{
@@ -1230,6 +1250,9 @@ void ieee80211_if_remove(struct ieee80211_sub_if_data *sdata)
if (ieee80211_vif_is_mesh(&sdata->vif))
mesh_path_flush_by_iface(sdata);
+ /* clean up type-dependent data */
+ ieee80211_clean_sdata(sdata);
+
synchronize_rcu();
unregister_netdevice(sdata->dev);
}
@@ -1252,6 +1275,8 @@ void ieee80211_remove_interfaces(struct ieee80211_local *local)
if (ieee80211_vif_is_mesh(&sdata->vif))
mesh_path_flush_by_iface(sdata);
+ ieee80211_clean_sdata(sdata);
+
unregister_netdevice_queue(sdata->dev, &unreg_list);
}
mutex_unlock(&local->iflist_mtx);
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 32f7a3b..2c235c7 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -1299,6 +1299,7 @@ int ieee80211_reconfig(struct ieee80211_local *local)
case NUM_NL80211_IFTYPES:
case NL80211_IFTYPE_P2P_CLIENT:
case NL80211_IFTYPE_P2P_GO:
+ case NL80211_IFTYPE_VIRTUAL_AMP80211:
WARN_ON(1);
break;
}
diff --git a/net/mac80211/virtual_amp.c b/net/mac80211/virtual_amp.c
new file mode 100644
index 0000000..beccf15
--- /dev/null
+++ b/net/mac80211/virtual_amp.c
@@ -0,0 +1,205 @@
+/*
+ * Virtual/Software AMP 80211 BT Controller. AMP is Alternate MAC/PHYs
+ * Controller for Bluetooth sybsystem. When an AMP is common between the
+ * two devices, the Bluetooth system provides mechanisms for moving data
+ * traffic from BR/EDR Controller to an AMP Controller.
+ *
+ * Copyright 2012 Intel Corp.
+ *
+ * Written by [email protected]
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+#include "virtual_amp.h"
+
+static int vamp_open_dev(struct hci_dev *hdev)
+{
+ BT_DBG("%s", hdev->name);
+
+ set_bit(HCI_RUNNING, &hdev->flags);
+
+ return 0;
+}
+
+static int vamp_close_dev(struct hci_dev *hdev)
+{
+ struct vamp_data *data = hci_get_drvdata(hdev);
+
+ BT_DBG("%s", hdev->name);
+
+ if (!test_and_clear_bit(HCI_RUNNING, &hdev->flags))
+ return 0;
+
+ skb_queue_purge(&data->txq);
+
+ return 0;
+}
+
+static int vamp_send_frame(struct sk_buff *skb)
+{
+ struct hci_dev *hdev = (struct hci_dev *) skb->dev;
+ struct vamp_data *data;
+
+ BT_DBG("%s", hdev->name);
+
+ if (!hdev) {
+ BT_ERR("Frame for unknown HCI device (hdev=NULL)");
+ return -ENODEV;
+ }
+
+ if (!test_bit(HCI_RUNNING, &hdev->flags))
+ return -EBUSY;
+
+ data = hci_get_drvdata(hdev);
+
+ skb_queue_tail(&data->txq, skb);
+
+ schedule_work(&data->work);
+
+ return 0;
+}
+
+static int vamp_flush(struct hci_dev *hdev)
+{
+ struct vamp_data *data = hci_get_drvdata(hdev);
+
+ BT_DBG("%s", hdev->name);
+
+ skb_queue_purge(&data->txq);
+
+ return 0;
+}
+
+static void vamp_command_packet(struct hci_dev *hdev, struct sk_buff *skb)
+{
+ struct hci_command_hdr *hdr = (void *) skb->data;
+ __u16 opcode = le16_to_cpu(hdr->opcode);
+
+ BT_DBG("%s", hdev->name);
+
+ skb_pull(skb, HCI_EVENT_HDR_SIZE);
+
+ switch (opcode) {
+ default:
+ BT_DBG("%s opcode 0x%x", hdev->name, opcode);
+ break;
+ }
+
+ kfree_skb(skb);
+}
+
+static void vamp_acldata_packet(struct hci_dev *hdev, struct sk_buff *skb)
+{
+ struct hci_acl_hdr *hdr = (void *) skb->data;
+ __u16 handle, flags;
+
+ skb_pull(skb, HCI_ACL_HDR_SIZE);
+
+ handle = __le16_to_cpu(hdr->handle);
+ flags = hci_flags(handle);
+ handle = hci_handle(handle);
+
+ BT_DBG("%s len %d handle 0x%x flags 0x%x", hdev->name, skb->len,
+ handle, flags);
+
+ /* Send data through WIFI */
+
+ kfree_skb(skb);
+}
+
+static void vamp_work(struct work_struct *work)
+{
+ struct vamp_data *data = container_of(work, struct vamp_data, work);
+ struct hci_dev *hdev = data->hdev;
+ struct sk_buff *skb;
+
+ BT_DBG("%s", data->hdev->name);
+
+ while ((skb = skb_dequeue(&data->txq))) {
+ /* Process frame */
+ switch (bt_cb(skb)->pkt_type) {
+ case HCI_COMMAND_PKT:
+ vamp_command_packet(hdev, skb);
+ break;
+
+ case HCI_ACLDATA_PKT:
+ BT_DBG("%s ACL data packet", hdev->name);
+ vamp_acldata_packet(hdev, skb);
+ break;
+
+ default:
+ BT_ERR("Unknown frame type %d", bt_cb(skb)->pkt_type);
+ kfree_skb(skb);
+ break;
+ }
+
+ }
+}
+
+static int virtual_amp_init(struct ieee80211_sub_if_data *sdata)
+{
+ struct hci_dev *hdev;
+ struct vamp_data *data;
+
+ data = kzalloc(sizeof(struct vamp_data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ skb_queue_head_init(&data->txq);
+
+ INIT_WORK(&data->work, vamp_work);
+
+ hdev = hci_alloc_dev();
+ if (!hdev) {
+ kfree(data);
+ return -ENOMEM;
+ }
+
+ data->hdev = hdev;
+
+ hdev->bus = HCI_VIRTUAL;
+ hci_set_drvdata(hdev, data);
+
+ hdev->dev_type = HCI_AMP;
+
+ hdev->open = vamp_open_dev;
+ hdev->close = vamp_close_dev;
+ hdev->flush = vamp_flush;
+ hdev->send = vamp_send_frame;
+
+ if (hci_register_dev(hdev) < 0) {
+ BT_ERR("Can't register HCI device");
+ kfree(data);
+ hci_free_dev(hdev);
+ return -EBUSY;
+ }
+
+ sdata->hdev = hdev;
+
+ return 0;
+}
+
+void ieee80211_vamp_setup_sdata(struct ieee80211_sub_if_data *sdata)
+{
+ pr_info("Create virtual AMP device");
+
+ virtual_amp_init(sdata);
+
+}
+
+void ieee80211_vamp_clean_sdata(struct ieee80211_sub_if_data *sdata)
+{
+ struct hci_dev *hdev = sdata->hdev;
+ struct vamp_data *data = hci_get_drvdata(hdev);
+
+ pr_info("Clean up virtual AMP device");
+
+ hci_unregister_dev(hdev);
+ hci_free_dev(hdev);
+ kfree(data);
+}
diff --git a/net/mac80211/virtual_amp.h b/net/mac80211/virtual_amp.h
new file mode 100644
index 0000000..5b7d219
--- /dev/null
+++ b/net/mac80211/virtual_amp.h
@@ -0,0 +1,29 @@
+/*
+ * Virtual / Software AMP 80211 BT Controller header
+ *
+ * Copyright 2012 Intel Corp.
+ *
+ * Written by [email protected]
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include "ieee80211_i.h"
+
+#ifdef CONFIG_MAC80211_VIRTUAL_AMP
+
+void ieee80211_vamp_setup_sdata(struct ieee80211_sub_if_data *sdata);
+void ieee80211_vamp_clean_sdata(struct ieee80211_sub_if_data *sdata);
+
+struct vamp_data {
+ struct hci_dev *hdev;
+
+ unsigned long flags;
+
+ struct work_struct work;
+ struct sk_buff_head txq;
+};
+
+#endif /* CONFIG_MAC80211_VIRTUAL_AMP */
--
1.7.9.1