Skip to contentSkip to conversation

sysdev: remove global sysdev drivers list

PATCH In review Akinobu Mita 2 participants 4 messages Aug 12, 2007 – Aug 20, 2007 Driver core

lore
Share & tools
lore

Akinobu Mita

author
#0 lore raw

PATCH 1/1 sysdev: remove global sysdev drivers list

No one uses sysdev_drivers. Because no one calls sysdev_driver_register with NULL class.

And it is difficult to imagine that someone want to implement a global sysdev driver which is called with all sys_device on any kind of sysdev_class.

So this patch removes global sysdev_drivers list.

Cc Tejun Heo <[email protected]>
Cc Cornelia Huck <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Signed-off-by Akinobu Mita <[email protected]>
---
 drivers/base/sys.c |   66 +++++++++--------------------------------------------
 1 file changed, 12 insertions(+), 54 deletions(-)
drivers/base/sys.c +12 -54
Index: 2.6-git/drivers/base/sys.c
===================================================================
@@ -153,25 +153,22 @@ void sysdev_class_unregister(struct sysd
 EXPORT_SYMBOL_GPL(sysdev_class_register);
 EXPORT_SYMBOL_GPL(sysdev_class_unregister);
 
-
-static LIST_HEAD(sysdev_drivers);
 static DEFINE_MUTEX(sysdev_drivers_lock);
 
 /**
  *	sysdev_driver_register - Register auxillary driver
- * 	@cls:	Device class driver belongs to.
+ *	@cls:	Device class driver belongs to.
  *	@drv:	Driver.
  *
- *	If @cls is valid, then @drv is inserted into @cls->drivers to be
+ *	@drv is inserted into @cls->drivers to be
  *	called on each operation on devices of that class. The refcount
  *	of @cls is incremented.
- *	Otherwise, @drv is inserted into sysdev_drivers, and called for
- *	each device.
  */
 
-int sysdev_driver_register(struct sysdev_class * cls,
-			   struct sysdev_driver * drv)
+int sysdev_driver_register(struct sysdev_class *cls, struct sysdev_driver *drv)
 {
+	int err = 0;
+
 	mutex_lock(&sysdev_drivers_lock);
 	if (cls && kset_get(&cls->kset)) {
 		list_add_tail(&drv->entry, &cls->drivers);

@@ -182,10 +179,12 @@ int sysdev_driver_register(struct sysdev
 			list_for_each_entry(dev, &cls->kset.list, kobj.entry)
 				drv->add(dev);
 		}
-	} else
-		list_add_tail(&drv->entry, &sysdev_drivers);
+	} else {
+		err = -EINVAL;
+		WARN_ON(1);
+	}
 	mutex_unlock(&sysdev_drivers_lock);
-	return 0;
+	return err;
 }
 
 

@@ -206,6 +205,8 @@ void sysdev_driver_unregister(struct sys
 				drv->remove(dev);
 		}
 		kset_put(&cls->kset);
+	} else {
+		WARN_ON(1);
 	}
 	mutex_unlock(&sysdev_drivers_lock);
 }

@@ -251,12 +252,6 @@ int sysdev_register(struct sys_device * 
 		 * code that should have called us.
 		 */
 
-		/* Notify global drivers */
-		list_for_each_entry(drv, &sysdev_drivers, entry) {
-			if (drv->add)
-				drv->add(sysdev);
-		}
-
 		/* Notify class auxillary drivers */
 		list_for_each_entry(drv, &cls->drivers, entry) {
 			if (drv->add)

@@ -272,11 +267,6 @@ void sysdev_unregister(struct sys_device
 	struct sysdev_driver * drv;
 
 	mutex_lock(&sysdev_drivers_lock);
-	list_for_each_entry(drv, &sysdev_drivers, entry) {
-		if (drv->remove)
-			drv->remove(sysdev);
-	}
-
 	list_for_each_entry(drv, &sysdev->cls->drivers, entry) {
 		if (drv->remove)
 			drv->remove(sysdev);

@@ -320,12 +310,6 @@ void sysdev_shutdown(void)
 			struct sysdev_driver * drv;
 			pr_debug(" %s\n", kobject_name(&sysdev->kobj));
 
-			/* Call global drivers first. */
-			list_for_each_entry(drv, &sysdev_drivers, entry) {
-				if (drv->shutdown)
-					drv->shutdown(sysdev);
-			}
-
 			/* Call auxillary drivers next. */
 			list_for_each_entry(drv, &cls->drivers, entry) {
 				if (drv->shutdown)

@@ -354,12 +338,6 @@ static void __sysdev_resume(struct sys_d
 		if (drv->resume)
 			drv->resume(dev);
 	}
-
-	/* Call global drivers. */
-	list_for_each_entry(drv, &sysdev_drivers, entry) {
-		if (drv->resume)
-			drv->resume(dev);
-	}
 }
 
 /**

@@ -393,15 +371,6 @@ int sysdev_suspend(pm_message_t state)
 		list_for_each_entry(sysdev, &cls->kset.list, kobj.entry) {
 			pr_debug(" %s\n", kobject_name(&sysdev->kobj));
 
-			/* Call global drivers first. */
-			list_for_each_entry(drv, &sysdev_drivers, entry) {
-				if (drv->suspend) {
-					ret = drv->suspend(sysdev, state);
-					if (ret)
-						goto gbl_driver;
-				}
-			}
-
 			/* Call auxillary drivers next. */
 			list_for_each_entry(drv, &cls->drivers, entry) {
 				if (drv->suspend) {

@@ -436,18 +405,7 @@ aux_driver:
 		if (err_drv->resume)
 			err_drv->resume(sysdev);
 	}
-	drv = NULL;
 
-gbl_driver:
-	if (drv)
-		printk(KERN_ERR "sysdev driver suspend failed for %s\n",
-				kobject_name(&sysdev->kobj));
-	list_for_each_entry(err_drv, &sysdev_drivers, entry) {
-		if (err_drv == drv)
-			break;
-		if (err_drv->resume)
-			err_drv->resume(sysdev);
-	}
 	/* resume other sysdevs in current class */
 	list_for_each_entry(err_dev, &cls->kset.list, kobj.entry) {
 		if (err_dev == sysdev)
#1 lore raw
Quoted 8 lines · Akinobu Mita

No one uses sysdev_drivers. Because no one calls sysdev_driver_register with NULL class.

And it is difficult to imagine that someone want to implement a global sysdev driver which is called with all sys_device on any kind of sysdev_class.

So this patch removes global sysdev_drivers list.

This makes sense, I guess, especially since it simplyfies the code.

Some minor comments below.

drivers/base/sys.c int sysdev_driver_register(struct sysdev · line 184

Quoted 9 lines
drivers/base/sys.c +3 -2
@@ -182,10 +179,12 @@ int sysdev_driver_register(struct sysdev
 			list_for_each_entry(dev, &cls->kset.list, kobj.entry)
 				drv->add(dev);
 		}
-	} else
-		list_add_tail(&drv->entry, &sysdev_drivers);
+	} else {
+		err = -EINVAL;
+		WARN_ON(1);

Maybe print what went wrong here?

drivers/base/sys.c void sysdev_driver_unregister(struct sys · line 209

Quoted 13 lines
drivers/base/sys.c +4 -1
+	}
 	mutex_unlock(&sysdev_drivers_lock);
-	return 0;
+	return err;
 }
 
 

@@ -206,6 +205,8 @@ void sysdev_driver_unregister(struct sys
 				drv->remove(dev);
 		}
 		kset_put(&cls->kset);
+	} else {
+		WARN_ON(1);

This one is redundant, since you already warned on register.

Quoted 5 lines
drivers/base/sys.c +0 -0
 	}
 	mutex_unlock(&sysdev_drivers_lock);
 }

drivers/base/sys.c void sysdev_shutdown(void) · line 313

Quoted 10 lines
drivers/base/sys.c +0 -6
@@ -320,12 +310,6 @@ void sysdev_shutdown(void)
 			struct sysdev_driver * drv;
 			pr_debug(" %s\n", kobject_name(&sysdev->kobj));
 
-			/* Call global drivers first. */
-			list_for_each_entry(drv, &sysdev_drivers, entry) {
-				if (drv->shutdown)
-					drv->shutdown(sysdev);
-			}
-
 			/* Call auxillary drivers next. */

This comment has become meaningless.

Quoted 7 lines
drivers/base/sys.c +0 -0
 			list_for_each_entry(drv, &cls->drivers, entry) {
 				if (drv->shutdown)

@@ -393,15 +371,6 @@ int sysdev_suspend(pm_message_t state)
 		list_for_each_entry(sysdev, &cls->kset.list, kobj.entry) {
 			pr_debug(" %s\n", kobject_name(&sysdev->kobj));

drivers/base/sys.c int sysdev_suspend(pm_message_t state) · line 374

Quoted 10 lines
drivers/base/sys.c +0 -9
-			/* Call global drivers first. */
-			list_for_each_entry(drv, &sysdev_drivers, entry) {
-				if (drv->suspend) {
-					ret = drv->suspend(sysdev, state);
-					if (ret)
-						goto gbl_driver;
-				}
-			}
-
 			/* Call auxillary drivers next. */

Dito here.

Quoted 2 lines
drivers/base/sys.c +0 -0
 			list_for_each_entry(drv, &cls->drivers, entry) {
 				if (drv->suspend) {
author
#2 lore raw
Quoted 10 lines · Cornelia Huck
Quoted 7 lines · Akinobu Mita

No one uses sysdev_drivers. Because no one calls sysdev_driver_register with NULL class.

And it is difficult to imagine that someone want to implement a global sysdev driver which is called with all sys_device on any kind of sysdev_class.

Quoted 5 lines
Quoted 1 line

So this patch removes global sysdev_drivers list.

This makes sense, I guess, especially since it simplyfies the code.

Some minor comments below.

Yes. All your comments are oviously correct.
Please check updated patch.

From: Akinobu Mita <[email protected]>
Subject: [PATCH] sysdev: remove global sysdev drivers list

No one uses sysdev_drivers. Because no one calls sysdev_driver_register with NULL class.

And it is difficult to imagine that someone want to implement a global sysdev driver which is called with all sys_device on any kind of sysdev_class.

So this patch removes global sysdev_drivers list and update comments for this change.

Cc Tejun Heo <[email protected]>
Cc Cornelia Huck <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Signed-off-by Akinobu Mita <[email protected]>
---
 drivers/base/sys.c |   71 ++++++++++-------------------------------------------
 1 file changed, 14 insertions(+), 57 deletions(-)
drivers/base/sys.c +14 -57
Index: 2.6-git/drivers/base/sys.c
===================================================================
@@ -153,25 +153,22 @@ void sysdev_class_unregister(struct sysd
 EXPORT_SYMBOL_GPL(sysdev_class_register);
 EXPORT_SYMBOL_GPL(sysdev_class_unregister);
 
-
-static LIST_HEAD(sysdev_drivers);
 static DEFINE_MUTEX(sysdev_drivers_lock);
 
 /**
  *	sysdev_driver_register - Register auxillary driver
- * 	@cls:	Device class driver belongs to.
+ *	@cls:	Device class driver belongs to.
  *	@drv:	Driver.
  *
- *	If @cls is valid, then @drv is inserted into @cls->drivers to be
+ *	@drv is inserted into @cls->drivers to be
  *	called on each operation on devices of that class. The refcount
  *	of @cls is incremented.
- *	Otherwise, @drv is inserted into sysdev_drivers, and called for
- *	each device.
  */
 
-int sysdev_driver_register(struct sysdev_class * cls,
-			   struct sysdev_driver * drv)
+int sysdev_driver_register(struct sysdev_class *cls, struct sysdev_driver *drv)
 {
+	int err = 0;
+
 	mutex_lock(&sysdev_drivers_lock);
 	if (cls && kset_get(&cls->kset)) {
 		list_add_tail(&drv->entry, &cls->drivers);

@@ -182,10 +179,13 @@ int sysdev_driver_register(struct sysdev
 			list_for_each_entry(dev, &cls->kset.list, kobj.entry)
 				drv->add(dev);
 		}
-	} else
-		list_add_tail(&drv->entry, &sysdev_drivers);
+	} else {
+		err = -EINVAL;
+		printk(KERN_ERR "%s: invalid device class\n", __FUNCTION__);
+		WARN_ON(1);
+	}
 	mutex_unlock(&sysdev_drivers_lock);
-	return 0;
+	return err;
 }
 
 

@@ -251,12 +251,6 @@ int sysdev_register(struct sys_device * 
 		 * code that should have called us.
 		 */
 
-		/* Notify global drivers */
-		list_for_each_entry(drv, &sysdev_drivers, entry) {
-			if (drv->add)
-				drv->add(sysdev);
-		}
-
 		/* Notify class auxillary drivers */
 		list_for_each_entry(drv, &cls->drivers, entry) {
 			if (drv->add)

@@ -272,11 +266,6 @@ void sysdev_unregister(struct sys_device
 	struct sysdev_driver * drv;
 
 	mutex_lock(&sysdev_drivers_lock);
-	list_for_each_entry(drv, &sysdev_drivers, entry) {
-		if (drv->remove)
-			drv->remove(sysdev);
-	}
-
 	list_for_each_entry(drv, &sysdev->cls->drivers, entry) {
 		if (drv->remove)
 			drv->remove(sysdev);

@@ -293,7 +282,7 @@ void sysdev_unregister(struct sys_device
  *
  *	Loop over each class of system devices, and the devices in each
  *	of those classes. For each device, we call the shutdown method for
- *	each driver registered for the device - the globals, the auxillaries,
+ *	each driver registered for the device - the auxillaries,
  *	and the class driver.
  *
  *	Note: The list is iterated in reverse order, so that we shut down

@@ -320,13 +309,7 @@ void sysdev_shutdown(void)
 			struct sysdev_driver * drv;
 			pr_debug(" %s\n", kobject_name(&sysdev->kobj));
 
-			/* Call global drivers first. */
-			list_for_each_entry(drv, &sysdev_drivers, entry) {
-				if (drv->shutdown)
-					drv->shutdown(sysdev);
-			}
-
-			/* Call auxillary drivers next. */
+			/* Call auxillary drivers first */
 			list_for_each_entry(drv, &cls->drivers, entry) {
 				if (drv->shutdown)
 					drv->shutdown(sysdev);

@@ -354,12 +337,6 @@ static void __sysdev_resume(struct sys_d
 		if (drv->resume)
 			drv->resume(dev);
 	}
-
-	/* Call global drivers. */
-	list_for_each_entry(drv, &sysdev_drivers, entry) {
-		if (drv->resume)
-			drv->resume(dev);
-	}
 }
 
 /**

@@ -393,16 +370,7 @@ int sysdev_suspend(pm_message_t state)
 		list_for_each_entry(sysdev, &cls->kset.list, kobj.entry) {
 			pr_debug(" %s\n", kobject_name(&sysdev->kobj));
 
-			/* Call global drivers first. */
-			list_for_each_entry(drv, &sysdev_drivers, entry) {
-				if (drv->suspend) {
-					ret = drv->suspend(sysdev, state);
-					if (ret)
-						goto gbl_driver;
-				}
-			}
-
-			/* Call auxillary drivers next. */
+			/* Call auxillary drivers first */
 			list_for_each_entry(drv, &cls->drivers, entry) {
 				if (drv->suspend) {
 					ret = drv->suspend(sysdev, state);

@@ -436,18 +404,7 @@ aux_driver:
 		if (err_drv->resume)
 			err_drv->resume(sysdev);
 	}
-	drv = NULL;
 
-gbl_driver:
-	if (drv)
-		printk(KERN_ERR "sysdev driver suspend failed for %s\n",
-				kobject_name(&sysdev->kobj));
-	list_for_each_entry(err_drv, &sysdev_drivers, entry) {
-		if (err_drv == drv)
-			break;
-		if (err_drv->resume)
-			err_drv->resume(sysdev);
-	}
 	/* resume other sysdevs in current class */
 	list_for_each_entry(err_dev, &cls->kset.list, kobj.entry) {
 		if (err_dev == sysdev)
Acked-by
#3 lore raw
Quoted 13 lines · Akinobu Mita

From: Akinobu Mita <[email protected]>
Subject: [PATCH] sysdev: remove global sysdev drivers list

No one uses sysdev_drivers. Because no one calls sysdev_driver_register with NULL class.

And it is difficult to imagine that someone want to implement a global sysdev driver which is called with all sys_device on any kind of sysdev_class.

So this patch removes global sysdev_drivers list and update comments for this change.

Quoted 8 lines
Cc Tejun Heo <[email protected]>
Cc Cornelia Huck <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Signed-off-by Akinobu Mita <[email protected]>
---
 drivers/base/sys.c |   71 ++++++++++-------------------------------------------
 1 file changed, 14 insertions(+), 57 deletions(-)
Acked-by Cornelia Huck <[email protected]>