Re: [RFCv1] mac80211: Adds Software / Virtual AMP 80211

[Johannes Berg <johannes@sipsolutions.net>]

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