Re: Bluetooth and HAL (Was Re: PPTP Plugin Updated -- Ver. 0.6.9 Generally PPP capable?)

[Marcel Holtmann <marcel holtmann org>]

Hi David,

> > > > > So the missing pieces are to link other Bluetooth kernel services like
> > > > > RFCOMM, HID, BNEP or CAPI with the Bluetooth adapter. I haven't come up
> > > > > the best approach to do this right. The obvious problem is if you have
> > > > > a /dev/rfcomm0 device for example. How do you tell which Bluetooth
> > > > > adapter it is assigned to. And of course RFCOMM is capable of picking
> > > > > any available adapter if you give it the wildcard source address.
> > > > 
> > > > What's wrong with having the actual Bluetooth devices (not the adapter)
> > > > live in sysfs below the adapter
> > > > 
> > > >  http://people.freedesktop.org/~david/bt.txt
> > > > 
> > > > as described in that link?
> > > 
> > > I spent a lot time argumenting about this with myself and I haven't come
> > > to final conclusion yet. In general you have to create a new device for
> > > every device you discover or connect to. Which is actually not a bad
> > > idea after all and I saw that the UWB subsystem is actually doing such
> > > thing everytime they get a beacon.
> > > 
> > > If we gonna go into this direction then it needs massive changes in the
> > > Bluetooth core itself. At the moment we only have adapters presented
> > > through hci_dev and connection (ACL and SCO) presented through hci_conn.
> > > And the assumption that you only have one ACL link per connection might
> > > not be longer true in the future. The inclusion of UWB as transport for
> > > Bluetooth will change some things.
> > > 
> > > However it might be worth to define an empty bt_dev structure and
> > > actually link it with inquiry results and hci_conn to create a bus alike
> > > device tree. This also means we need to present hci_dev as bt_host,
> > > because otherwise we can't differentiate between devices seen from two
> > > different adapters.
> > > 
> > > I am open for suggestions here. It is not easy to tell if Bluetooth
> > > better fits into the class device model or be better a simple device on
> > > a bus.
> > 
> > Right. It's also interesting how things like Wireless USB is going to
> > fit in here.
> > 
> > For me it comes down to this: since you seem to provide kernel drivers
> > for e.g. input and ALSA functionality from Bluetooth devices don't you
> > think it would be nice with the device-> symlink so it's easy to figure
> > out the relationship between class devices and bus devices? I mean, the
> > kernel _already_ knows about the remote Bluetooth devices, all I'm
> > asking for is to put them in sysfs. 
> > 
> > My crappy patch here
> > 
> >  http://people.freedesktop.org/~david/bt.txt
> >  http://people.freedesktop.org/~david/davidz-bluetooth.patch
> > 
> > did exactly that. I really don't know how we'd integrate Bluetooth input
> > and audio devices if HAL doesn't know about this.
> > 
> > Going forward it would be super useful to also have e.g. a 'pair' file
> > you could read from ("is the device paired?") and write to ("attempt to
> > pair a device"), e.g. have parts of the Bluetooth control functionality
> > through sysfs. I don't know the details and if you say there are good
> > reasons to go through a user space Bluez D-BUS interface instead of just
> > sysfs that's fine with me. But I thought I'd raise this anyway :-)
> 
> I didn't know that the driver model can now link class devices to actual
> real devices. However it can do that and so I converted all Bluetooth
> host controller into real devices with a pseudo platform device as
> parent for virtual and serial devices. The patch for this has been
> tested since a week and I sent it to David Miller for inclusion.
> 
> I also tried to represent every remote device as child, but this failed
> so far. It triggers a BUG_ON() in the mutex slowpath and I need more
> time to fully understand it. A possible solution is schedule_work() like
> David did, but I have to check some possible race conditions first.
> 
> So this email is to inform you that I am on my way to fully integrate
> local and remote Bluetooth devices into the driver model.

and attached is a patch against the latest kernel that represents every
Bluetooth low-level connection (ACL and SCO) as device under the parent
device that represents the Bluetooth host controller.

/sys/class/bluetooth/hci0
|-- acl001122334455
|   |-- address
|   |-- power
|   |   |-- state
|   |   `-- wakeup
|   |-- type
|   `-- uevent
|-- address
|-- device -> ../../../../../../../devices/pci0000:00/0000:00:1d.2/usb4/4-1/4-1:1.0
|-- power
|   |-- state
|   `-- wakeup
|-- subsystem -> ../../../../../../../class/bluetooth
|-- type
`-- uevent

Regards

Marcel

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index d84855f..263e42b 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -165,6 +165,10 @@ struct hci_conn {
 	struct timer_list disc_timer;
 	struct timer_list idle_timer;
 
+	struct work_struct work;
+
+	struct device	dev;
+
 	struct hci_dev	*hdev;
 	void		*l2cap_data;
 	void		*sco_data;
@@ -412,6 +416,8 @@ static inline int hci_recv_frame(struct 
 
 int hci_register_sysfs(struct hci_dev *hdev);
 void hci_unregister_sysfs(struct hci_dev *hdev);
+void hci_conn_add_sysfs(struct hci_conn *conn);
+void hci_conn_del_sysfs(struct hci_conn *conn);
 
 #define SET_HCIDEV_DEV(hdev, pdev) ((hdev)->parent = (pdev))
 
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 420ed4d..7e9515b 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -179,6 +179,8 @@ struct hci_conn *hci_conn_add(struct hci
 	if (hdev->notify)
 		hdev->notify(hdev, HCI_NOTIFY_CONN_ADD);
 
+	hci_conn_add_sysfs(conn);
+
 	tasklet_enable(&hdev->tx_task);
 
 	return conn;
@@ -211,6 +213,8 @@ int hci_conn_del(struct hci_conn *conn)
 
 	tasklet_disable(&hdev->tx_task);
 
+	hci_conn_del_sysfs(conn);
+
 	hci_conn_hash_del(hdev, conn);
 	if (hdev->notify)
 		hdev->notify(hdev, HCI_NOTIFY_CONN_DEL);
@@ -221,7 +225,9 @@ int hci_conn_del(struct hci_conn *conn)
 
 	hci_dev_put(hdev);
 
-	kfree(conn);
+	/* will free via device release */
+	put_device(&conn->dev);
+
 	return 0;
 }
 
diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 3987d16..87c1df3 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -166,6 +186,32 @@ static struct device_attribute *bt_attrs
 	NULL
 };
 
+static ssize_t show_conn_type(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct hci_conn *conn = dev_get_drvdata(dev);
+	return sprintf(buf, "%s\n", conn->type == ACL_LINK ? "ACL" : "SCO");
+}
+
+static ssize_t show_conn_address(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct hci_conn *conn = dev_get_drvdata(dev);
+	bdaddr_t bdaddr;
+	baswap(&bdaddr, &conn->dst);
+	return sprintf(buf, "%s\n", batostr(&bdaddr));
+}
+
+#define CONN_ATTR(_name,_mode,_show,_store) \
+struct device_attribute conn_attr_##_name = __ATTR(_name,_mode,_show,_store)
+
+static CONN_ATTR(type, S_IRUGO, show_conn_type, NULL);
+static CONN_ATTR(address, S_IRUGO, show_conn_address, NULL);
+
+static struct device_attribute *conn_attrs[] = {
+	&conn_attr_type,
+	&conn_attr_address,
+	NULL
+};
+
 struct class *bt_class = NULL;
 EXPORT_SYMBOL_GPL(bt_class);
 
@@ -177,8 +223,57 @@ static struct platform_device *bt_platfo
 
 static void bt_release(struct device *dev)
 {
-	struct hci_dev *hdev = dev_get_drvdata(dev);
-	kfree(hdev);
+	void *data = dev_get_drvdata(dev);
+	kfree(data);
+}
+
+static void add_conn(void *data)
+{
+	struct hci_conn *conn = data;
+	int i;
+
+	device_register(&conn->dev);
+
+	for (i = 0; conn_attrs[i]; i++)
+		device_create_file(&conn->dev, conn_attrs[i]);
+}
+
+void hci_conn_add_sysfs(struct hci_conn *conn)
+{
+	struct hci_dev *hdev = conn->hdev;
+	bdaddr_t *ba = &conn->dst;
+
+	BT_DBG("conn %p", conn);
+
+	conn->dev.parent  = &hdev->dev;
+	conn->dev.release = bt_release;
+
+	snprintf(conn->dev.bus_id, BUS_ID_SIZE,
+			"%s%2.2X%2.2X%2.2X%2.2X%2.2X%2.2X",
+			conn->type == ACL_LINK ? "acl" : "sco",
+			ba->b[5], ba->b[4], ba->b[3],
+			ba->b[2], ba->b[1], ba->b[0]);
+
+	dev_set_drvdata(&conn->dev, conn);
+
+	INIT_WORK(&conn->work, add_conn, (void *) conn);
+
+	schedule_work(&conn->work);
+}
+
+static void del_conn(void *data)
+{
+	struct hci_conn *conn = data;
+	device_del(&conn->dev);
+}
+
+void hci_conn_del_sysfs(struct hci_conn *conn)
+{
+	BT_DBG("conn %p", conn);
+
+	INIT_WORK(&conn->work, del_conn, (void *) conn);
+
+	schedule_work(&conn->work);
 }
 
 int hci_register_sysfs(struct hci_dev *hdev)