Re: [Bluez-devel] Oops involving RFCOMM and sysfs
flat view
From: Dave Young <hidden>
Date: 2008-01-10 01:08:12
Also in:
lkml
On Wed, Jan 09, 2008 at 06:16:02PM +0900, Tejun Heo wrote:
Hello, My laptop and cell finally decided to talk to each other and I could reproduce the bug here. The attached patch should remove the oops. The bug is two folded. I just skimmed through the bluetooth code and am very likely to wrong at places. Please correct me where I'm wrong. 1. It's introduced by class device migration and the bug will go away with CONFIG_SYSFS_DEPRECATED turned on. With CONFIG_SYSFS_DEPRECATED turned off, what used to be class devices live under actual devices. For rfcomm, this means the rfcommN tty device now lives under the aclXXXX node (probably represents an active connection?) instead of the class directory. It seems rfcommN devices are supposed to survive over disconnects so the rfcommN device moves under the live connection while connection is alive and retreats back to a default directory when the connection is lost. This is all fine and dandy as long as the rfcommN device lives under class directory as class directory never goes away. However, with recent sysfs updates, rfcommN now lives directly under the aclXXXX node. If the connection goes away while rfcommN device is under it, it gets deleted but rfcommN is still treated as alive. This isn't supported. sysfs doesn't allow parents to die first and the dangling children to be salvaged using sysfs_move(). 2. Which in turn exposes three bugs in sysfs - sysfs_lookup() returning NULL on negative lookup. sysfs code requires that all negative dentries are shot down. lookup should return -ENOENT instead of NULL. - in move and rename, error handling is wrong. It ends up passing ERR_PTR() values to dput(). - there was an extra dput() in sysfs_move_dir(). The attached patch fixes all sysfs bugs and removes the oops. However, rfcommX moving is still broken. The rfcommX device won't be visible from sysfs tree after the initial move failure and all following moves will fail. Please confirm the attached patch fixes the oops. I'll separate it into two patches and forward them to Greg. But bluetooth code also needs to be updated such that it moves the refcommX device before killing the connection node.
For bluetooth device_move, the only child device of hci_conn dev is the rfcomm tty_dev. How about the following patch, please verify : --- net/bluetooth/hci_sysfs.c | 7 +++++++ 1 file changed, 7 insertions(+) diff -upr linux/net/bluetooth/hci_sysfs.c linux.new/net/bluetooth/hci_sysfs.c
--- linux/net/bluetooth/hci_sysfs.c 2008-01-10 09:01:51.000000000 +0800
+++ linux.new/net/bluetooth/hci_sysfs.c 2008-01-10 09:01:22.000000000 +0800@@ -316,9 +316,16 @@ void hci_conn_add_sysfs(struct hci_conn schedule_work(&conn->work); } +static int hci_conn_move_child(struct device *dev, void *data) +{ + device_move(dev, NULL); + return 0; +} + static void del_conn(struct work_struct *work) { struct hci_conn *conn = container_of(work, struct hci_conn, work); + device_for_each_child(&conn->dev, NULL, hci_conn_move_child); device_del(&conn->dev); put_device(&conn->dev); }
Thanks. -- tejun
quoted hunk ↗ jump to hunk
diff --git a/drivers/ata/libata-pmp.c b/drivers/ata/libata-pmp.c diff --git a/fs/dcache.c b/fs/dcache.c diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c index 3371629..f281cc6 100644 --- a/fs/sysfs/dir.c +++ b/fs/sysfs/dir.c@@ -678,8 +678,10 @@ static struct dentry * sysfs_lookup(struct inode *dir, struct dentry *dentry, sd = sysfs_find_dirent(parent_sd, dentry->d_name.name); /* no such entry */ - if (!sd) + if (!sd) { + ret = ERR_PTR(-ENOENT); goto out_unlock; + } /* attach dentry and inode */ inode = sysfs_get_inode(sd);@@ -781,6 +783,7 @@ int sysfs_rename_dir(struct kobject * kobj, const char *new_name) old_dentry = sysfs_get_dentry(sd); if (IS_ERR(old_dentry)) { error = PTR_ERR(old_dentry); + old_dentry = NULL; goto out; }@@ -848,6 +851,7 @@ int sysfs_move_dir(struct kobject *kobj, struct kobject *new_parent_kobj) old_dentry = sysfs_get_dentry(sd); if (IS_ERR(old_dentry)) { error = PTR_ERR(old_dentry); + old_dentry = NULL; goto out; } old_parent = old_dentry->d_parent;@@ -855,6 +859,7 @@ int sysfs_move_dir(struct kobject *kobj, struct kobject *new_parent_kobj) new_parent = sysfs_get_dentry(new_parent_sd); if (IS_ERR(new_parent)) { error = PTR_ERR(new_parent); + new_parent = NULL; goto out; }@@ -878,7 +883,6 @@ again: error = 0; d_add(new_dentry, NULL); d_move(old_dentry, new_dentry); - dput(new_dentry); /* Remove from old parent's list and insert into new parent's list. */ sysfs_unlink_sibling(sd);diff --git a/include/linux/dcache.h b/include/linux/dcache.h
------------------------------------------------------------------------- Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace _______________________________________________ Bluez-devel mailing list Bluez-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/bluez-devel