[PATCH] net: Avoid problems with bonding and device rename

[PATCH] net: Avoid problems with bonding and device rename

[Stephen Hemminger]

See: http://bugzilla.kernel.org/show_bug.cgi?id=10698

The use of /sys/class/net/bonding_masters was a poor ABI choice
that now we have to live with. Best choice is to just block other
usage of that name.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/net/core/dev.c	2008-05-14 16:50:53.000000000 -0700
+++ b/net/core/dev.c	2008-05-14 16:52:25.000000000 -0700
@@ -758,6 +758,9 @@ int dev_valid_name(const char *name)
 		return 0;
 	if (!strcmp(name, ".") || !strcmp(name, ".."))
 		return 0;
+	/* Sigh. need better ABI discipline -- see bond_sysfs */
+	if (!strcmp(name, "bonding_masters"))
+		return 0;
 
 	while (*name) {
 		if (*name == '/' || isspace(*name))

Re: [PATCH] net: Avoid problems with bonding and device rename

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 14 May 2008 17:03:16 -0700

> See: http://bugzilla.kernel.org/show_bug.cgi?id=10698
> 
> The use of /sys/class/net/bonding_masters was a poor ABI choice
> that now we have to live with. Best choice is to just block other
> usage of that name.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

I think sysfs is way too noisy in this case and we should
handle the sysfs add failure just like any other -EEXIST case.

[PATCH 3/3] bonding: handle case of device named bonding_master

[Stephen Hemminger]

If device already exists named bonding_masters, then fail. This is a wierd
corner case only a QA group could love.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
---
Note: bonding is not yet network namespace aware, it will need lots of
work to get this to happen.

--- a/drivers/net/bonding/bond_sysfs.c	2008-05-14 17:49:29.000000000 -0700
+++ b/drivers/net/bonding/bond_sysfs.c	2008-05-14 18:02:36.000000000 -0700
@@ -1437,8 +1437,16 @@ int bond_create_sysfs(void)
 	 * configure multiple bonding devices.
 	 */
 	if (ret == -EEXIST) {
-		netdev_class = NULL;
-		return 0;
+		/* Is someone being kinky and naming a device bonding_master? */
+		if (__dev_get_by_name(&init_net,
+				      class_attr_bonding_masters.attr.name))
+			printk(KERN_ERR
+			       "network device named %s already exists in sysfs",
+			       class_attr_bonding_masters.attr.name);
+		else {
+			netdev_class = NULL;
+			return 0;
+		}
 	}
 
 	return ret;

[PATCH 1/3] net: handle errors from device_rename

[Stephen Hemminger]

device_rename can fail with -EEXIST or -ENOMEM, so handle any problems.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/net/core/dev.c	2008-05-14 17:49:37.000000000 -0700
+++ b/net/core/dev.c	2008-05-14 17:51:21.000000000 -0700
@@ -903,7 +903,11 @@ int dev_change_name(struct net_device *d
 		strlcpy(dev->name, newname, IFNAMSIZ);
 
 rollback:
-	device_rename(&dev->dev, dev->name);
+	err = device_rename(&dev->dev, dev->name);
+	if (err) {
+		memcpy(dev->name, oldname, IFNAMSIZ);
+		return err;
+	}
 
 	write_lock_bh(&dev_base_lock);
 	hlist_del(&dev->name_hlist);

[PATCH 2/3] sysfs: remove error messages for -EEXIST case

[Stephen Hemminger]

It is possible that the entry in sysfs already exists, one case of this is
when a network device is renamed to bonding_masters. Anyway, in this case
the proper error path is for device_rename to return an error code, not to
generate bogus backtrace and errors.

Also, to avoid possible races, the create link should be done before the
remove link. This makes a device rename atomic operation like other renames.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/base/core.c	2008-05-14 17:56:36.000000000 -0700
+++ b/drivers/base/core.c	2008-05-14 17:56:40.000000000 -0700
@@ -1218,13 +1218,11 @@ int device_rename(struct device *dev, ch
 	}
 #else
 	if (dev->class) {
-		sysfs_remove_link(&dev->class->subsys.kobj, old_device_name);
 		error = sysfs_create_link(&dev->class->subsys.kobj, &dev->kobj,
 					  dev->bus_id);
-		if (error) {
-			dev_err(dev, "%s: sysfs_create_symlink failed (%d)\n",
-				__func__, error);
-		}
+		if (error)
+			goto out;
+		sysfs_remove_link(&dev->class->subsys.kobj, old_device_name);
 	}
 #endif
 
--- a/fs/sysfs/dir.c	2008-05-14 17:56:36.000000000 -0700
+++ b/fs/sysfs/dir.c	2008-05-14 17:56:40.000000000 -0700
@@ -419,12 +419,8 @@ void sysfs_addrm_start(struct sysfs_addr
  */
 int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 {
-	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name)) {
-		printk(KERN_WARNING "sysfs: duplicate filename '%s' "
-		       "can not be created\n", sd->s_name);
-		WARN_ON(1);
+	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name))
 		return -EEXIST;
-	}
 
 	sd->s_parent = sysfs_get(acxt->parent_sd);
 

Re: [PATCH 2/3] sysfs: remove error messages for -EEXIST case

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 14 May 2008 18:16:03 -0700

> It is possible that the entry in sysfs already exists, one case of this is
> when a network device is renamed to bonding_masters. Anyway, in this case
> the proper error path is for device_rename to return an error code, not to
> generate bogus backtrace and errors.
> 
> Also, to avoid possible races, the create link should be done before the
> remove link. This makes a device rename atomic operation like other renames.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

I definitely agree with this change.

We have several cases where device names are user configurable,
yet the devices live in a directory which also has subdirectories
created by other subsystems.

It's pointless to require the top-level guy to look for any
purge out any subdirectory cases, that's none of it's business.

I realize the backtrace is useful for finding bugs, but in this
case it's definitely not appropriate.

Re: [PATCH 2/3] sysfs: remove error messages for -EEXIST case

[Greg KH]

On Wed, May 14, 2008 at 06:26:37PM -0700, David Miller wrote:
> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Wed, 14 May 2008 18:16:03 -0700
> 
> > It is possible that the entry in sysfs already exists, one case of this is
> > when a network device is renamed to bonding_masters. Anyway, in this case
> > the proper error path is for device_rename to return an error code, not to
> > generate bogus backtrace and errors.
> > 
> > Also, to avoid possible races, the create link should be done before the
> > remove link. This makes a device rename atomic operation like other renames.
> > 
> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> I definitely agree with this change.
> 
> We have several cases where device names are user configurable,
> yet the devices live in a directory which also has subdirectories
> created by other subsystems.
> 
> It's pointless to require the top-level guy to look for any
> purge out any subdirectory cases, that's none of it's business.
> 
> I realize the backtrace is useful for finding bugs, but in this
> case it's definitely not appropriate.

Fair enough, I have no objection to these.

David, do you want me to take them through my tree?  Or are they
dependant on the network core changes as well?  If so, I have no
problem you taking them.  Feel free to add a:
	Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
to them in that case.

thanks,

greg k-h

Re: [PATCH 2/3] sysfs: remove error messages for -EEXIST case

[David Miller]

From: Greg KH <greg@kroah.com>
Date: Wed, 14 May 2008 20:14:58 -0700

> Fair enough, I have no objection to these.
> 
> David, do you want me to take them through my tree?  Or are they
> dependant on the network core changes as well?  If so, I have no
> problem you taking them.  Feel free to add a:
> 	Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> to them in that case.

I'll take them, thanks for reviewing and the signoff Greg.

Re: [PATCH 1/3] net: handle errors from device_rename

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 14 May 2008 18:15:43 -0700

> device_rename can fail with -EEXIST or -ENOMEM, so handle any problems.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied, thanks.

Re: [PATCH 2/3] sysfs: remove error messages for -EEXIST case

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 14 May 2008 18:16:03 -0700

> It is possible that the entry in sysfs already exists, one case of this is
> when a network device is renamed to bonding_masters. Anyway, in this case
> the proper error path is for device_rename to return an error code, not to
> generate bogus backtrace and errors.
> 
> Also, to avoid possible races, the create link should be done before the
> remove link. This makes a device rename atomic operation like other renames.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied.

Re: [PATCH 3/3] bonding: handle case of device named bonding_master

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 14 May 2008 18:15:28 -0700

> If device already exists named bonding_masters, then fail. This is a wierd
> corner case only a QA group could love.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Also applied, thanks a lot Stephen.

Re: [PATCH 2/3] sysfs: remove error messages for -EEXIST case

[Cornelia Huck]

On Wed, 14 May 2008 18:16:03 -0700,
Stephen Hemminger <shemminger@vyatta.com> wrote:

> --- a/drivers/base/core.c	2008-05-14 17:56:36.000000000 -0700
> +++ b/drivers/base/core.c	2008-05-14 17:56:40.000000000 -0700
> @@ -1218,13 +1218,11 @@ int device_rename(struct device *dev, ch
>  	}
>  #else
>  	if (dev->class) {
> -		sysfs_remove_link(&dev->class->subsys.kobj, old_device_name);
>  		error = sysfs_create_link(&dev->class->subsys.kobj, &dev->kobj,
>  					  dev->bus_id);
> -		if (error) {
> -			dev_err(dev, "%s: sysfs_create_symlink failed (%d)\n",
> -				__func__, error);
> -		}
> +		if (error)
> +			goto out;
> +		sysfs_remove_link(&dev->class->subsys.kobj, old_device_name);
>  	}
>  #endif
> 

This looks reasonable...

> --- a/fs/sysfs/dir.c	2008-05-14 17:56:36.000000000 -0700
> +++ b/fs/sysfs/dir.c	2008-05-14 17:56:40.000000000 -0700
> @@ -419,12 +419,8 @@ void sysfs_addrm_start(struct sysfs_addr
>   */
>  int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
>  {
> -	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name)) {
> -		printk(KERN_WARNING "sysfs: duplicate filename '%s' "
> -		       "can not be created\n", sd->s_name);
> -		WARN_ON(1);
> +	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name))
>  		return -EEXIST;
> -	}
> 
>  	sd->s_parent = sysfs_get(acxt->parent_sd);

...but this will cause many useful warnings to disappear.

What to do here?

- Rely on people checking all __must_check stuff and printing a warning
when desired. Not everyone seems to like that (see, for example,
http://marc.info/?l=linux-kernel&m=121012176111154&w=2).
- Put the burden unto the callers of sysfs_add_one() (or maybe even
further up). They should hopefully know whether -EEXIST is "might
happen" or "argh, we've messed up" (and print a warning in the latter
case so that it gets reported).
- Make this a debugging thing. Unfortunately then we won't neccessarily
get reports when things are busted.

Re: [PATCH 2/3] sysfs: remove error messages for -EEXIST case

[David Miller]

From: Cornelia Huck <cornelia.huck@de.ibm.com>
Date: Thu, 15 May 2008 09:52:46 +0200

> >  int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
> >  {
> > -	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name)) {
> > -		printk(KERN_WARNING "sysfs: duplicate filename '%s' "
> > -		       "can not be created\n", sd->s_name);
> > -		WARN_ON(1);
> > +	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name))
> >  		return -EEXIST;
> > -	}
> > 
> >  	sd->s_parent = sysfs_get(acxt->parent_sd);
> 
> ...but this will cause many useful warnings to disappear.
> 
> What to do here?

Make a __sysfs_add_one() that doesn't warn.   Make
sysfs_add_one() be a wrapper around __sysfs_add_one()
that warns.

Change networking to call __sysfs_add_one().

Repeat and rinse up the call chain, as needed.

Re: [PATCH 1/3] net: handle errors from device_rename

[Wang Chen]

Stephen Hemminger said the following on 2008-5-15 9:15:
>  rollback:
> -	device_rename(&dev->dev, dev->name);
> +	err = device_rename(&dev->dev, dev->name);
> +	if (err) {
> +		memcpy(dev->name, oldname, IFNAMSIZ);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
no need, dev->name didn't change in device_rename().
 
> +		return err;
> +	}
>  

Re: [PATCH 2/3] sysfs: remove error messages for -EEXIST case

[Cornelia Huck]

On Thu, 15 May 2008 01:01:39 -0700 (PDT),
David Miller <davem@davemloft.net> wrote:

> From: Cornelia Huck <cornelia.huck@de.ibm.com>
> Date: Thu, 15 May 2008 09:52:46 +0200
> 
> > >  int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
> > >  {
> > > -	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name)) {
> > > -		printk(KERN_WARNING "sysfs: duplicate filename '%s' "
> > > -		       "can not be created\n", sd->s_name);
> > > -		WARN_ON(1);
> > > +	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name))
> > >  		return -EEXIST;
> > > -	}
> > > 
> > >  	sd->s_parent = sysfs_get(acxt->parent_sd);
> > 
> > ...but this will cause many useful warnings to disappear.
> > 
> > What to do here?
> 
> Make a __sysfs_add_one() that doesn't warn.   Make
> sysfs_add_one() be a wrapper around __sysfs_add_one()
> that warns.
> 
> Change networking to call __sysfs_add_one().

Like this (not even compile tested)? This depends on being able to
properly isolate the functions called though, or we need to get the
nowarn flag one level further up.

diff --git a/drivers/base/core.c b/drivers/base/core.c
index be288b5..a9058f4 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1209,8 +1209,8 @@ #ifdef CONFIG_SYSFS_DEPRECATED
 	if (old_class_name) {
 		new_class_name = make_class_name(dev->class->name, &dev->kobj);
 		if (new_class_name) {
-			error = sysfs_create_link(&dev->parent->kobj,
-						  &dev->kobj, new_class_name);
+			error = __sysfs_create_link(&dev->parent->kobj,
+						  &dev->kobj, new_class_name, 1);
 			if (error)
 				goto out;
 			sysfs_remove_link(&dev->parent->kobj, old_class_name);
@@ -1219,8 +1219,8 @@ #ifdef CONFIG_SYSFS_DEPRECATED
 #else
 	if (dev->class) {
 		sysfs_remove_link(&dev->class->subsys.kobj, old_device_name);
-		error = sysfs_create_link(&dev->class->subsys.kobj, &dev->kobj,
-					  dev->bus_id);
+		error = __sysfs_create_link(&dev->class->subsys.kobj, &dev->kobj,
+					  dev->bus_id, 1);
 		if (error) {
 			dev_err(dev, "%s: sysfs_create_symlink failed (%d)\n",
 				__func__, error);
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index a1c3a1f..b7914a0 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -397,6 +397,23 @@ void sysfs_addrm_start(struct sysfs_addr
 		iput(inode);
 }
 
+int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
+{
+	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name))
+		return -EEXIST;
+
+	sd->s_parent = sysfs_get(acxt->parent_sd);
+
+	if (sysfs_type(sd) == SYSFS_DIR && acxt->parent_inode)
+		inc_nlink(acxt->parent_inode);
+
+	acxt->cnt++;
+
+	sysfs_link_sibling(sd);
+
+	return 0;
+}
+
 /**
  *	sysfs_add_one - add sysfs_dirent to parent
  *	@acxt: addrm context to use
@@ -419,23 +436,14 @@ void sysfs_addrm_start(struct sysfs_addr
  */
 int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 {
-	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name)) {
+	int err = __sysfs_add_one(acxt, sd);
+
+	if (err == -EEXIST) {
 		printk(KERN_WARNING "sysfs: duplicate filename '%s' "
 		       "can not be created\n", sd->s_name);
 		WARN_ON(1);
-		return -EEXIST;
 	}
-
-	sd->s_parent = sysfs_get(acxt->parent_sd);
-
-	if (sysfs_type(sd) == SYSFS_DIR && acxt->parent_inode)
-		inc_nlink(acxt->parent_inode);
-
-	acxt->cnt++;
-
-	sysfs_link_sibling(sd);
-
-	return 0;
+	return err;
 }
 
 /**
diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index 817f596..c08184d 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -25,7 +25,8 @@ #include "sysfs.h"
  *	@target:	object we're pointing to.
  *	@name:		name of the symlink.
  */
-int sysfs_create_link(struct kobject * kobj, struct kobject * target, const char * name)
+int __sysfs_create_link(struct kobject * kobj, struct kobject * target,
+			const char * name, int nowarn)
 {
 	struct sysfs_dirent *parent_sd = NULL;
 	struct sysfs_dirent *target_sd = NULL;
@@ -65,7 +66,10 @@ int sysfs_create_link(struct kobject * k
 	target_sd = NULL;	/* reference is now owned by the symlink */
 
 	sysfs_addrm_start(&acxt, parent_sd);
-	error = sysfs_add_one(&acxt, sd);
+	if (nowarn)
+		error = __sysfs_add_one(&acxt, sd);
+	else
+		error = sysfs_add_one(&acxt, sd);
 	sysfs_addrm_finish(&acxt);
 
 	if (error)
@@ -79,6 +83,12 @@ int sysfs_create_link(struct kobject * k
 	return error;
 }
 
+int sysfs_create_link(struct kobject * kobj, struct kobject * target,
+		      const char * name)
+{
+	return sysfs_create_link(kobj, target, name, 0);
+}
+
 /**
  *	sysfs_remove_link - remove symlink in object's directory.
  *	@kobj:	object we're acting for.
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index ce4e15f..1d459f5 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -108,6 +108,7 @@ void sysfs_put_active_two(struct sysfs_d
 void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt,
 		       struct sysfs_dirent *parent_sd);
 int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
+int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
 void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
 void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt);
 
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 7858eac..3ef8ae7 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -101,6 +101,8 @@ void sysfs_remove_bin_file(struct kobjec
 
 int __must_check sysfs_create_link(struct kobject *kobj, struct kobject *target,
 				   const char *name);
+int __sysfs_create_link(struct kobject *kobj, struct kobject *target,
+			const char *name, int nowarn);
 void sysfs_remove_link(struct kobject *kobj, const char *name);
 
 int __must_check sysfs_create_group(struct kobject *kobj,

Re: [PATCH 2/3] sysfs: remove error messages for -EEXIST case

[David Miller]

From: Cornelia Huck <cornelia.huck@de.ibm.com>
Date: Thu, 15 May 2008 11:31:31 +0200

> On Thu, 15 May 2008 01:01:39 -0700 (PDT),
> David Miller <davem@davemloft.net> wrote:
> 
> > From: Cornelia Huck <cornelia.huck@de.ibm.com>
> > Date: Thu, 15 May 2008 09:52:46 +0200
> > 
> > > >  int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
> > > >  {
> > > > -	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name)) {
> > > > -		printk(KERN_WARNING "sysfs: duplicate filename '%s' "
> > > > -		       "can not be created\n", sd->s_name);
> > > > -		WARN_ON(1);
> > > > +	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name))
> > > >  		return -EEXIST;
> > > > -	}
> > > > 
> > > >  	sd->s_parent = sysfs_get(acxt->parent_sd);
> > > 
> > > ...but this will cause many useful warnings to disappear.
> > > 
> > > What to do here?
> > 
> > Make a __sysfs_add_one() that doesn't warn.   Make
> > sysfs_add_one() be a wrapper around __sysfs_add_one()
> > that warns.
> > 
> > Change networking to call __sysfs_add_one().
> 
> Like this (not even compile tested)?

No, the idea is that the __sysfs_add_one() wouldn't need a parameter
at all.  sysfs_add_one() just needs to check for -EEXIST from
__sysfs_add_one() to decide whether to warn or not.

Re: [PATCH 2/3] sysfs: remove error messages for -EEXIST case

[Cornelia Huck]

On Thu, 15 May 2008 03:00:54 -0700 (PDT),
David Miller <davem@davemloft.net> wrote:

> No, the idea is that the __sysfs_add_one() wouldn't need a parameter
> at all.  sysfs_add_one() just needs to check for -EEXIST from
> __sysfs_add_one() to decide whether to warn or not.

But that is what the patch does? Only the upper layers need to pass a
flag around.

Re: [PATCH 1/3] net: handle errors from device_rename

[Stephen Hemminger]

On Thu, 15 May 2008 16:41:40 +0800
Wang Chen <wangchen@cn.fujitsu.com> wrote:

> Stephen Hemminger said the following on 2008-5-15 9:15:
> >  rollback:
> > -	device_rename(&dev->dev, dev->name);
> > +	err = device_rename(&dev->dev, dev->name);
> > +	if (err) {
> > +		memcpy(dev->name, oldname, IFNAMSIZ);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> no need, dev->name didn't change in device_rename().
>  
> > +		return err;
> > +	}
> >  

dev->name has already been changed before the call to device_rename,
so it needs to be set back read the whole code context.

[PATCH] driver core: Suppress sysfs warnings for device_rename().

[Cornelia Huck]

OK, here is an actually-compiled patch with proper description and
s-o-b. Comments?

-----

driver core: Suppress sysfs warnings for device_rename().

Renaming network devices to an already existing name is not
something we want sysfs to print a scary warning for, since the
callers can deal with this correctly. So let's introduce
sysfs_create_link_nowarn() which gets rid of the common warning.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

---
 drivers/base/core.c   |   17 ++++++++---------
 fs/sysfs/dir.c        |   37 +++++++++++++++++++++++++++++++++++--
 fs/sysfs/symlink.c    |   41 +++++++++++++++++++++++++++++++++--------
 fs/sysfs/sysfs.h      |    1 +
 include/linux/sysfs.h |   10 ++++++++++
 5 files changed, 87 insertions(+), 19 deletions(-)

Index: linux-2.6/drivers/base/core.c
===================================================================
--- linux-2.6.orig/drivers/base/core.c
+++ linux-2.6/drivers/base/core.c
@@ -1287,8 +1287,9 @@ int device_rename(struct device *dev, ch
 	if (old_class_name) {
 		new_class_name = make_class_name(dev->class->name, &dev->kobj);
 		if (new_class_name) {
-			error = sysfs_create_link(&dev->parent->kobj,
-						  &dev->kobj, new_class_name);
+			error = sysfs_create_link_nowarn(&dev->parent->kobj,
+							 &dev->kobj,
+							 new_class_name);
 			if (error)
 				goto out;
 			sysfs_remove_link(&dev->parent->kobj, old_class_name);
@@ -1296,13 +1297,11 @@ int device_rename(struct device *dev, ch
 	}
 #else
 	if (dev->class) {
-		sysfs_remove_link(&dev->class->subsys.kobj, old_device_name);
-		error = sysfs_create_link(&dev->class->subsys.kobj, &dev->kobj,
-					  dev->bus_id);
-		if (error) {
-			dev_err(dev, "%s: sysfs_create_symlink failed (%d)\n",
-				__func__, error);
-		}
+		error = sysfs_create_link_nowarn(&dev->class->subsys.kobj,
+						 &dev->kobj, dev->bus_id);
+		if (!error)
+			sysfs_remove_link(&dev->class->subsys.kobj,
+					  old_device_name);
 	}
 #endif
 
Index: linux-2.6/fs/sysfs/dir.c
===================================================================
--- linux-2.6.orig/fs/sysfs/dir.c
+++ linux-2.6/fs/sysfs/dir.c
@@ -398,7 +398,7 @@ void sysfs_addrm_start(struct sysfs_addr
 }
 
 /**
- *	sysfs_add_one - add sysfs_dirent to parent
+ *	__sysfs_add_one - add sysfs_dirent to parent without warning
  *	@acxt: addrm context to use
  *	@sd: sysfs_dirent to be added
  *
@@ -417,7 +417,7 @@ void sysfs_addrm_start(struct sysfs_addr
  *	0 on success, -EEXIST if entry with the given name already
  *	exists.
  */
-int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
+int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 {
 	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name)) {
 		printk(KERN_WARNING "sysfs: duplicate filename '%s' "
@@ -439,6 +439,39 @@ int sysfs_add_one(struct sysfs_addrm_cxt
 }
 
 /**
+ *	sysfs_add_one - add sysfs_dirent to parent
+ *	@acxt: addrm context to use
+ *	@sd: sysfs_dirent to be added
+ *
+ *	Get @acxt->parent_sd and set sd->s_parent to it and increment
+ *	nlink of parent inode if @sd is a directory and link into the
+ *	children list of the parent.
+ *
+ *	This function should be called between calls to
+ *	sysfs_addrm_start() and sysfs_addrm_finish() and should be
+ *	passed the same @acxt as passed to sysfs_addrm_start().
+ *
+ *	LOCKING:
+ *	Determined by sysfs_addrm_start().
+ *
+ *	RETURNS:
+ *	0 on success, -EEXIST if entry with the given name already
+ *	exists.
+ */
+int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
+{
+	int ret;
+
+	ret = __sysfs_add_one(acxt, sd);
+	if (ret == -EEXIST) {
+		printk(KERN_WARNING "sysfs: duplicate filename '%s' "
+		       "can not be created\n", sd->s_name);
+		WARN_ON(1);
+	}
+	return ret;
+}
+
+/**
  *	sysfs_remove_one - remove sysfs_dirent from parent
  *	@acxt: addrm context to use
  *	@sd: sysfs_dirent to be removed
Index: linux-2.6/fs/sysfs/symlink.c
===================================================================
--- linux-2.6.orig/fs/sysfs/symlink.c
+++ linux-2.6/fs/sysfs/symlink.c
@@ -19,13 +19,8 @@
 
 #include "sysfs.h"
 
-/**
- *	sysfs_create_link - create symlink between two objects.
- *	@kobj:	object whose directory we're creating the link in.
- *	@target:	object we're pointing to.
- *	@name:		name of the symlink.
- */
-int sysfs_create_link(struct kobject * kobj, struct kobject * target, const char * name)
+static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,
+				const char *name, int warn)
 {
 	struct sysfs_dirent *parent_sd = NULL;
 	struct sysfs_dirent *target_sd = NULL;
@@ -65,7 +60,10 @@ int sysfs_create_link(struct kobject * k
 	target_sd = NULL;	/* reference is now owned by the symlink */
 
 	sysfs_addrm_start(&acxt, parent_sd);
-	error = sysfs_add_one(&acxt, sd);
+	if (warn)
+		error = sysfs_add_one(&acxt, sd);
+	else
+		error = __sysfs_add_one(&acxt, sd);
 	sysfs_addrm_finish(&acxt);
 
 	if (error)
@@ -80,6 +78,33 @@ int sysfs_create_link(struct kobject * k
 }
 
 /**
+ *	sysfs_create_link - create symlink between two objects.
+ *	@kobj:	object whose directory we're creating the link in.
+ *	@target:	object we're pointing to.
+ *	@name:		name of the symlink.
+ */
+int sysfs_create_link(struct kobject *kobj, struct kobject *target,
+		      const char *name)
+{
+	return sysfs_do_create_link(kobj, target, name, 1);
+}
+
+/**
+ *	sysfs_create_link_nowarn - create symlink between two objects.
+ *	@kobj:	object whose directory we're creating the link in.
+ *	@target:	object we're pointing to.
+ *	@name:		name of the symlink.
+ *
+ *	This function does the same as sysf_create_link(), but it
+ *	doesn't warn if the link already exists.
+ */
+int sysfs_create_link_nowarn(struct kobject *kobj, struct kobject *target,
+			     const char *name)
+{
+	return sysfs_do_create_link(kobj, target, name, 0);
+}
+
+/**
  *	sysfs_remove_link - remove symlink in object's directory.
  *	@kobj:	object we're acting for.
  *	@name:	name of the symlink to remove.
Index: linux-2.6/fs/sysfs/sysfs.h
===================================================================
--- linux-2.6.orig/fs/sysfs/sysfs.h
+++ linux-2.6/fs/sysfs/sysfs.h
@@ -107,6 +107,7 @@ struct sysfs_dirent *sysfs_get_active_tw
 void sysfs_put_active_two(struct sysfs_dirent *sd);
 void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt,
 		       struct sysfs_dirent *parent_sd);
+int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
 int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
 void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
 void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt);
Index: linux-2.6/include/linux/sysfs.h
===================================================================
--- linux-2.6.orig/include/linux/sysfs.h
+++ linux-2.6/include/linux/sysfs.h
@@ -103,6 +103,9 @@ void sysfs_remove_bin_file(struct kobjec
 
 int __must_check sysfs_create_link(struct kobject *kobj, struct kobject *target,
 				   const char *name);
+int __must_check sysfs_create_link_nowarn(struct kobject *kobj,
+					  struct kobject *target,
+					  const char *name);
 void sysfs_remove_link(struct kobject *kobj, const char *name);
 
 int __must_check sysfs_create_group(struct kobject *kobj,
@@ -182,6 +185,13 @@ static inline int sysfs_create_link(stru
 	return 0;
 }
 
+static inline int sysfs_create_link_nowarn(struct kobject *kobj,
+					   struct kobject *target,
+					   const char *name)
+{
+	return 0;
+}
+
 static inline void sysfs_remove_link(struct kobject *kobj, const char *name)
 {
 }

Re: [PATCH] driver core: Suppress sysfs warnings for device_rename().

[Stephen Hemminger]

On Tue, 20 May 2008 12:59:13 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> OK, here is an actually-compiled patch with proper description and
> s-o-b. Comments?
> 
> -----
> 
> driver core: Suppress sysfs warnings for device_rename().
> 
> Renaming network devices to an already existing name is not
> something we want sysfs to print a scary warning for, since the
> callers can deal with this correctly. So let's introduce
> sysfs_create_link_nowarn() which gets rid of the common warning.
> 
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>

This is still getting to be overkill. I prefer that the warnings
always are removed.

Re: [PATCH] driver core: Suppress sysfs warnings for device_rename().

[Greg KH]

On Tue, May 20, 2008 at 02:45:08PM -0700, Stephen Hemminger wrote:
> On Tue, 20 May 2008 12:59:13 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> > OK, here is an actually-compiled patch with proper description and
> > s-o-b. Comments?
> > 
> > -----
> > 
> > driver core: Suppress sysfs warnings for device_rename().
> > 
> > Renaming network devices to an already existing name is not
> > something we want sysfs to print a scary warning for, since the
> > callers can deal with this correctly. So let's introduce
> > sysfs_create_link_nowarn() which gets rid of the common warning.
> > 
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> >
> 
> This is still getting to be overkill. I prefer that the warnings
> always are removed.

No, I do not.  They have found a lot of real bugs that have been going
unnoticed for quite some time, and some new ones (like the current mess
in the pci hotplug subsystem where two different drivers are controlling
the same pci hotplug slots and not realizing it at all.)

So having the warning gone for rename() is fine, but I still want it
there for new files that are being added to the system.

thanks,

greg k-h

Re: [PATCH] driver core: Suppress sysfs warnings for device_rename().

[Greg KH]

On Tue, May 20, 2008 at 12:59:13PM +0200, Cornelia Huck wrote:
> OK, here is an actually-compiled patch with proper description and
> s-o-b. Comments?

Looks good to me, feel free to add an:
	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

to it.

David, are you going to take this through your tree?

thanks,

greg k-h

Re: [PATCH] driver core: Suppress sysfs warnings for device_rename().

[Cornelia Huck]

On Tue, 20 May 2008 15:52:44 -0700,
Greg KH <greg@kroah.com> wrote:

> On Tue, May 20, 2008 at 12:59:13PM +0200, Cornelia Huck wrote:
> > OK, here is an actually-compiled patch with proper description and
> > s-o-b. Comments?
> 
> Looks good to me, feel free to add an:
> 	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
> 
> to it.
> 
> David, are you going to take this through your tree?

Here it is again, respun against today's git:


driver core: Suppress sysfs warnings for device_rename().

Renaming network devices to an already existing name is not
something we want sysfs to print a scary warning for, since the
callers can deal with this correctly. So let's introduce
sysfs_create_link_nowarn() which gets rid of the common warning.

Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

---
 drivers/base/core.c   |    9 +++++----
 fs/sysfs/dir.c        |   37 +++++++++++++++++++++++++++++++++++--
 fs/sysfs/symlink.c    |   41 +++++++++++++++++++++++++++++++++--------
 fs/sysfs/sysfs.h      |    1 +
 include/linux/sysfs.h |   10 ++++++++++
 5 files changed, 84 insertions(+), 14 deletions(-)

--- linux-2.6.orig/drivers/base/core.c
+++ linux-2.6/drivers/base/core.c
@@ -1282,8 +1282,9 @@ int device_rename(struct device *dev, ch
 	if (old_class_name) {
 		new_class_name = make_class_name(dev->class->name, &dev->kobj);
 		if (new_class_name) {
-			error = sysfs_create_link(&dev->parent->kobj,
-						  &dev->kobj, new_class_name);
+			error = sysfs_create_link_nowarn(&dev->parent->kobj,
+							 &dev->kobj,
+							 new_class_name);
 			if (error)
 				goto out;
 			sysfs_remove_link(&dev->parent->kobj, old_class_name);
@@ -1291,8 +1292,8 @@ int device_rename(struct device *dev, ch
 	}
 #else
 	if (dev->class) {
-		error = sysfs_create_link(&dev->class->subsys.kobj, &dev->kobj,
-					  dev->bus_id);
+		error = sysfs_create_link_nowarn(&dev->class->subsys.kobj,
+						 &dev->kobj, dev->bus_id);
 		if (error)
 			goto out;
 		sysfs_remove_link(&dev->class->subsys.kobj, old_device_name);
--- linux-2.6.orig/fs/sysfs/dir.c
+++ linux-2.6/fs/sysfs/dir.c
@@ -398,7 +398,7 @@ void sysfs_addrm_start(struct sysfs_addr
 }
 
 /**
- *	sysfs_add_one - add sysfs_dirent to parent
+ *	__sysfs_add_one - add sysfs_dirent to parent without warning
  *	@acxt: addrm context to use
  *	@sd: sysfs_dirent to be added
  *
@@ -417,7 +417,7 @@ void sysfs_addrm_start(struct sysfs_addr
  *	0 on success, -EEXIST if entry with the given name already
  *	exists.
  */
-int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
+int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 {
 	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name))
 		return -EEXIST;
@@ -435,6 +435,39 @@ int sysfs_add_one(struct sysfs_addrm_cxt
 }
 
 /**
+ *	sysfs_add_one - add sysfs_dirent to parent
+ *	@acxt: addrm context to use
+ *	@sd: sysfs_dirent to be added
+ *
+ *	Get @acxt->parent_sd and set sd->s_parent to it and increment
+ *	nlink of parent inode if @sd is a directory and link into the
+ *	children list of the parent.
+ *
+ *	This function should be called between calls to
+ *	sysfs_addrm_start() and sysfs_addrm_finish() and should be
+ *	passed the same @acxt as passed to sysfs_addrm_start().
+ *
+ *	LOCKING:
+ *	Determined by sysfs_addrm_start().
+ *
+ *	RETURNS:
+ *	0 on success, -EEXIST if entry with the given name already
+ *	exists.
+ */
+int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
+{
+	int ret;
+
+	ret = __sysfs_add_one(acxt, sd);
+	if (ret == -EEXIST) {
+		printk(KERN_WARNING "sysfs: duplicate filename '%s' "
+		       "can not be created\n", sd->s_name);
+		WARN_ON(1);
+	}
+	return ret;
+}
+
+/**
  *	sysfs_remove_one - remove sysfs_dirent from parent
  *	@acxt: addrm context to use
  *	@sd: sysfs_dirent to be removed
--- linux-2.6.orig/fs/sysfs/symlink.c
+++ linux-2.6/fs/sysfs/symlink.c
@@ -19,13 +19,8 @@
 
 #include "sysfs.h"
 
-/**
- *	sysfs_create_link - create symlink between two objects.
- *	@kobj:	object whose directory we're creating the link in.
- *	@target:	object we're pointing to.
- *	@name:		name of the symlink.
- */
-int sysfs_create_link(struct kobject * kobj, struct kobject * target, const char * name)
+static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,
+				const char *name, int warn)
 {
 	struct sysfs_dirent *parent_sd = NULL;
 	struct sysfs_dirent *target_sd = NULL;
@@ -65,7 +60,10 @@ int sysfs_create_link(struct kobject * k
 	target_sd = NULL;	/* reference is now owned by the symlink */
 
 	sysfs_addrm_start(&acxt, parent_sd);
-	error = sysfs_add_one(&acxt, sd);
+	if (warn)
+		error = sysfs_add_one(&acxt, sd);
+	else
+		error = __sysfs_add_one(&acxt, sd);
 	sysfs_addrm_finish(&acxt);
 
 	if (error)
@@ -80,6 +78,33 @@ int sysfs_create_link(struct kobject * k
 }
 
 /**
+ *	sysfs_create_link - create symlink between two objects.
+ *	@kobj:	object whose directory we're creating the link in.
+ *	@target:	object we're pointing to.
+ *	@name:		name of the symlink.
+ */
+int sysfs_create_link(struct kobject *kobj, struct kobject *target,
+		      const char *name)
+{
+	return sysfs_do_create_link(kobj, target, name, 1);
+}
+
+/**
+ *	sysfs_create_link_nowarn - create symlink between two objects.
+ *	@kobj:	object whose directory we're creating the link in.
+ *	@target:	object we're pointing to.
+ *	@name:		name of the symlink.
+ *
+ *	This function does the same as sysf_create_link(), but it
+ *	doesn't warn if the link already exists.
+ */
+int sysfs_create_link_nowarn(struct kobject *kobj, struct kobject *target,
+			     const char *name)
+{
+	return sysfs_do_create_link(kobj, target, name, 0);
+}
+
+/**
  *	sysfs_remove_link - remove symlink in object's directory.
  *	@kobj:	object we're acting for.
  *	@name:	name of the symlink to remove.
--- linux-2.6.orig/fs/sysfs/sysfs.h
+++ linux-2.6/fs/sysfs/sysfs.h
@@ -107,6 +107,7 @@ struct sysfs_dirent *sysfs_get_active_tw
 void sysfs_put_active_two(struct sysfs_dirent *sd);
 void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt,
 		       struct sysfs_dirent *parent_sd);
+int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
 int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
 void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
 void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt);
--- linux-2.6.orig/include/linux/sysfs.h
+++ linux-2.6/include/linux/sysfs.h
@@ -101,6 +101,9 @@ void sysfs_remove_bin_file(struct kobjec
 
 int __must_check sysfs_create_link(struct kobject *kobj, struct kobject *target,
 				   const char *name);
+int __must_check sysfs_create_link_nowarn(struct kobject *kobj,
+					  struct kobject *target,
+					  const char *name);
 void sysfs_remove_link(struct kobject *kobj, const char *name);
 
 int __must_check sysfs_create_group(struct kobject *kobj,
@@ -180,6 +183,13 @@ static inline int sysfs_create_link(stru
 	return 0;
 }
 
+static inline int sysfs_create_link_nowarn(struct kobject *kobj,
+					   struct kobject *target,
+					   const char *name)
+{
+	return 0;
+}
+
 static inline void sysfs_remove_link(struct kobject *kobj, const char *name)
 {
 }

Re: [PATCH] driver core: Suppress sysfs warnings for device_rename().

[Cornelia Huck]

On Wed, 21 May 2008 10:05:56 +0200,
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

Just digging through my backlog: Is there any further interest in this
patch?

> On Tue, 20 May 2008 15:52:44 -0700,
> Greg KH <greg@kroah.com> wrote:
> 
> > On Tue, May 20, 2008 at 12:59:13PM +0200, Cornelia Huck wrote:
> > > OK, here is an actually-compiled patch with proper description and
> > > s-o-b. Comments?
> > 
> > Looks good to me, feel free to add an:
> > 	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
> > 
> > to it.
> > 
> > David, are you going to take this through your tree?
> 
> Here it is again, respun against today's git:
> 
> 
> driver core: Suppress sysfs warnings for device_rename().
> 
> Renaming network devices to an already existing name is not
> something we want sysfs to print a scary warning for, since the
> callers can deal with this correctly. So let's introduce
> sysfs_create_link_nowarn() which gets rid of the common warning.
> 
> Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> 
> ---
>  drivers/base/core.c   |    9 +++++----
>  fs/sysfs/dir.c        |   37 +++++++++++++++++++++++++++++++++++--
>  fs/sysfs/symlink.c    |   41 +++++++++++++++++++++++++++++++++--------
>  fs/sysfs/sysfs.h      |    1 +
>  include/linux/sysfs.h |   10 ++++++++++
>  5 files changed, 84 insertions(+), 14 deletions(-)
> 
> --- linux-2.6.orig/drivers/base/core.c
> +++ linux-2.6/drivers/base/core.c
> @@ -1282,8 +1282,9 @@ int device_rename(struct device *dev, ch
>  	if (old_class_name) {
>  		new_class_name = make_class_name(dev->class->name, &dev->kobj);
>  		if (new_class_name) {
> -			error = sysfs_create_link(&dev->parent->kobj,
> -						  &dev->kobj, new_class_name);
> +			error = sysfs_create_link_nowarn(&dev->parent->kobj,
> +							 &dev->kobj,
> +							 new_class_name);
>  			if (error)
>  				goto out;
>  			sysfs_remove_link(&dev->parent->kobj, old_class_name);
> @@ -1291,8 +1292,8 @@ int device_rename(struct device *dev, ch
>  	}
>  #else
>  	if (dev->class) {
> -		error = sysfs_create_link(&dev->class->subsys.kobj, &dev->kobj,
> -					  dev->bus_id);
> +		error = sysfs_create_link_nowarn(&dev->class->subsys.kobj,
> +						 &dev->kobj, dev->bus_id);
>  		if (error)
>  			goto out;
>  		sysfs_remove_link(&dev->class->subsys.kobj, old_device_name);
> --- linux-2.6.orig/fs/sysfs/dir.c
> +++ linux-2.6/fs/sysfs/dir.c
> @@ -398,7 +398,7 @@ void sysfs_addrm_start(struct sysfs_addr
>  }
>  
>  /**
> - *	sysfs_add_one - add sysfs_dirent to parent
> + *	__sysfs_add_one - add sysfs_dirent to parent without warning
>   *	@acxt: addrm context to use
>   *	@sd: sysfs_dirent to be added
>   *
> @@ -417,7 +417,7 @@ void sysfs_addrm_start(struct sysfs_addr
>   *	0 on success, -EEXIST if entry with the given name already
>   *	exists.
>   */
> -int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
> +int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
>  {
>  	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name))
>  		return -EEXIST;
> @@ -435,6 +435,39 @@ int sysfs_add_one(struct sysfs_addrm_cxt
>  }
>  
>  /**
> + *	sysfs_add_one - add sysfs_dirent to parent
> + *	@acxt: addrm context to use
> + *	@sd: sysfs_dirent to be added
> + *
> + *	Get @acxt->parent_sd and set sd->s_parent to it and increment
> + *	nlink of parent inode if @sd is a directory and link into the
> + *	children list of the parent.
> + *
> + *	This function should be called between calls to
> + *	sysfs_addrm_start() and sysfs_addrm_finish() and should be
> + *	passed the same @acxt as passed to sysfs_addrm_start().
> + *
> + *	LOCKING:
> + *	Determined by sysfs_addrm_start().
> + *
> + *	RETURNS:
> + *	0 on success, -EEXIST if entry with the given name already
> + *	exists.
> + */
> +int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
> +{
> +	int ret;
> +
> +	ret = __sysfs_add_one(acxt, sd);
> +	if (ret == -EEXIST) {
> +		printk(KERN_WARNING "sysfs: duplicate filename '%s' "
> +		       "can not be created\n", sd->s_name);
> +		WARN_ON(1);
> +	}
> +	return ret;
> +}
> +
> +/**
>   *	sysfs_remove_one - remove sysfs_dirent from parent
>   *	@acxt: addrm context to use
>   *	@sd: sysfs_dirent to be removed
> --- linux-2.6.orig/fs/sysfs/symlink.c
> +++ linux-2.6/fs/sysfs/symlink.c
> @@ -19,13 +19,8 @@
>  
>  #include "sysfs.h"
>  
> -/**
> - *	sysfs_create_link - create symlink between two objects.
> - *	@kobj:	object whose directory we're creating the link in.
> - *	@target:	object we're pointing to.
> - *	@name:		name of the symlink.
> - */
> -int sysfs_create_link(struct kobject * kobj, struct kobject * target, const char * name)
> +static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,
> +				const char *name, int warn)
>  {
>  	struct sysfs_dirent *parent_sd = NULL;
>  	struct sysfs_dirent *target_sd = NULL;
> @@ -65,7 +60,10 @@ int sysfs_create_link(struct kobject * k
>  	target_sd = NULL;	/* reference is now owned by the symlink */
>  
>  	sysfs_addrm_start(&acxt, parent_sd);
> -	error = sysfs_add_one(&acxt, sd);
> +	if (warn)
> +		error = sysfs_add_one(&acxt, sd);
> +	else
> +		error = __sysfs_add_one(&acxt, sd);
>  	sysfs_addrm_finish(&acxt);
>  
>  	if (error)
> @@ -80,6 +78,33 @@ int sysfs_create_link(struct kobject * k
>  }
>  
>  /**
> + *	sysfs_create_link - create symlink between two objects.
> + *	@kobj:	object whose directory we're creating the link in.
> + *	@target:	object we're pointing to.
> + *	@name:		name of the symlink.
> + */
> +int sysfs_create_link(struct kobject *kobj, struct kobject *target,
> +		      const char *name)
> +{
> +	return sysfs_do_create_link(kobj, target, name, 1);
> +}
> +
> +/**
> + *	sysfs_create_link_nowarn - create symlink between two objects.
> + *	@kobj:	object whose directory we're creating the link in.
> + *	@target:	object we're pointing to.
> + *	@name:		name of the symlink.
> + *
> + *	This function does the same as sysf_create_link(), but it
> + *	doesn't warn if the link already exists.
> + */
> +int sysfs_create_link_nowarn(struct kobject *kobj, struct kobject *target,
> +			     const char *name)
> +{
> +	return sysfs_do_create_link(kobj, target, name, 0);
> +}
> +
> +/**
>   *	sysfs_remove_link - remove symlink in object's directory.
>   *	@kobj:	object we're acting for.
>   *	@name:	name of the symlink to remove.
> --- linux-2.6.orig/fs/sysfs/sysfs.h
> +++ linux-2.6/fs/sysfs/sysfs.h
> @@ -107,6 +107,7 @@ struct sysfs_dirent *sysfs_get_active_tw
>  void sysfs_put_active_two(struct sysfs_dirent *sd);
>  void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt,
>  		       struct sysfs_dirent *parent_sd);
> +int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
>  int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
>  void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
>  void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt);
> --- linux-2.6.orig/include/linux/sysfs.h
> +++ linux-2.6/include/linux/sysfs.h
> @@ -101,6 +101,9 @@ void sysfs_remove_bin_file(struct kobjec
>  
>  int __must_check sysfs_create_link(struct kobject *kobj, struct kobject *target,
>  				   const char *name);
> +int __must_check sysfs_create_link_nowarn(struct kobject *kobj,
> +					  struct kobject *target,
> +					  const char *name);
>  void sysfs_remove_link(struct kobject *kobj, const char *name);
>  
>  int __must_check sysfs_create_group(struct kobject *kobj,
> @@ -180,6 +183,13 @@ static inline int sysfs_create_link(stru
>  	return 0;
>  }
>  
> +static inline int sysfs_create_link_nowarn(struct kobject *kobj,
> +					   struct kobject *target,
> +					   const char *name)
> +{
> +	return 0;
> +}
> +
>  static inline void sysfs_remove_link(struct kobject *kobj, const char *name)
>  {
>  }

Re: [PATCH] driver core: Suppress sysfs warnings for device_rename().

[Stephen Hemminger]

On Tue, 10 Jun 2008 11:09:08 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Wed, 21 May 2008 10:05:56 +0200,
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> Just digging through my backlog: Is there any further interest in this
> patch?
> 
> > On Tue, 20 May 2008 15:52:44 -0700,
> > Greg KH <greg@kroah.com> wrote:
> > 
> > > On Tue, May 20, 2008 at 12:59:13PM +0200, Cornelia Huck wrote:
> > > > OK, here is an actually-compiled patch with proper description and
> > > > s-o-b. Comments?
> > > 
> > > Looks good to me, feel free to add an:
> > > 	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
> > > 
> > > to it.
> > > 
> > > David, are you going to take this through your tree?
> > 
> > Here it is again, respun against today's git:
> > 
> > 
> > driver core: Suppress sysfs warnings for device_rename().
> > 
> > Renaming network devices to an already existing name is not
> > something we want sysfs to print a scary warning for, since the
> > callers can deal with this correctly. So let's introduce
> > sysfs_create_link_nowarn() which gets rid of the common warning.
> > 
> > Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > 

I you want to put the warnings back this is a reasonable way.