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
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
