Re: Incorrect uses of get_driver()/put_driver()

[Sebastian Ott]

On Tue, 10 Jan 2012, Martin Schwidefsky wrote:

> On Mon, 9 Jan 2012 12:35:09 -0500 (EST)
> Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> 
> > drivers/s390/cio/device.c:1681:       drv = get_driver(&cdrv->driver);
> > drivers/s390/cio/device.c:1687:       put_driver(drv);
> > 
> > Martin, these calls seem to be useless.  The calls in ccwgroup.c are 
> > definitely useless; there's no reason to take a reference to a driver 
> > while it's being unregistered, since it can't go away until the 
> > unregistration is finished.
> 
> The get_driver/put_driver in ccwgroup.c are obviously useless, the caller
> passed ccwgroup_driver_unregister a ccwgroup_driver reference.
> I am not so sure about the code in device.c. get_ccwdev_by_busid() gets
> used e.g. by vmur like this:
> 
> static struct ccw_driver ur_driver = { .. };
> 
> static struct urdev *urdev_get_from_devno(u16 devno)
> {
> 	..
> 	sprintf(bus_id, "0.0.%04x", devno);
>         cdev = get_ccwdev_by_busid(&ur_driver, bus_id);
> 	..
> }
> 
> static int ur_open(struct inode *inode, struct file *file)
> {
> 	..
> 	urd = urdev_get_from_devno(devno);
> 	..
> }
> 
> For vmur we should be safe by the fact that ur_open is only possible if
> a try_module_get has been successful and the ccw_driver is unregistered
> only in the module exit function. But we have to check if this is true
> for all users of the get_ccwdev_by_busid() function.

Done.
[PATCH] cio: remove {get,put}_driver

Remove useless {get,put}_driver - the caller of the functions
has to ensure valid driver pointers.

Signed-off-by: Sebastian Ott <sebott@xxxxxxxxxxxxxxxxxx>
---
 drivers/s390/cio/ccwgroup.c |    2 --
 drivers/s390/cio/device.c   |    8 +-------
 2 files changed, 1 insertion(+), 9 deletions(-)

--- a/drivers/s390/cio/ccwgroup.c
+++ b/drivers/s390/cio/ccwgroup.c
@@ -580,7 +580,6 @@ void ccwgroup_driver_unregister(struct c
 	struct device *dev;
 
 	/* We don't want ccwgroup devices to live longer than their driver. */
-	get_driver(&cdriver->driver);
 	while ((dev = driver_find_device(&cdriver->driver, NULL, NULL,
 					 __ccwgroup_match_all))) {
 		struct ccwgroup_device *gdev = to_ccwgroupdev(dev);
@@ -592,7 +591,6 @@ void ccwgroup_driver_unregister(struct c
 		mutex_unlock(&gdev->reg_mutex);
 		put_device(dev);
 	}
-	put_driver(&cdriver->driver);
 	driver_unregister(&cdriver->driver);
 }
 EXPORT_SYMBOL(ccwgroup_driver_unregister);
--- a/drivers/s390/cio/device.c
+++ b/drivers/s390/cio/device.c
@@ -1676,15 +1676,9 @@ struct ccw_device *get_ccwdev_by_busid(s
 				       const char *bus_id)
 {
 	struct device *dev;
-	struct device_driver *drv;
 
-	drv = get_driver(&cdrv->driver);
-	if (!drv)
-		return NULL;
-
-	dev = driver_find_device(drv, NULL, (void *)bus_id,
+	dev = driver_find_device(&cdrv->driver, NULL, (void *)bus_id,
 				 __ccwdev_check_busid);
-	put_driver(drv);
 
 	return dev ? to_ccwdev(dev) : NULL;
 }



> 
> -- 
> blue skies,
>    Martin.
> 
> "Reality continues to ruin my life." - Calvin.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/