Search Linux Wireless

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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Wireless Personal Area Network]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Hiking]     [MIPS Linux]     [ARM Linux]     [Linux RAID]

  Powered by Linux