Bug fixes/cleanups for floppy driver (v4)

Bug fixes/cleanups for floppy driver (v4)

[Herton Ronaldo Krzesinski]

Following this are fixes for bugs noticed on floppy driver, and some
extra cleanups. Version history below:

v2: separate fixes, and incorporate suggestions by Vivek Goyal.
    also I splitted the cleanups from fixes.
v3: remove dr, also this possibly makes error handling more readable
    (suggested by by Vivek Goyal), incorporate acks.
v4: re-shuffle series with patch from Ben Hutchings handling the queue
    reference in a better way inside genhd

Patches 1-2 and 4 are general bug fixes for problems noted in the floppy
driver initialization and its error handling. They are the minimal fix
required, so it's suitable and easy to pick for stable releases (so
are Cc'd to stable), and thus not contain cleanups or re-factoring.

Patch 3 is a patch from Ben Hutchings fixing in a better way the extra
queue reference issue that was introduced in commit 523e1d3 ('block:
make gendisk hold a reference to its queue'). Instead of making the
drivers worry about the extra reference, fix it and handle that in a
better way inside genhd. I'm not sure if this should go with the floppy
changes or picked up separate for the block tree. In any case, patch 6
will only apply after it is applied. His patch was proposed with the
previous series I submitted and handles the problem better so I took the
liberty to include now, I just fixed the changelog as was noticed that
time to reference the right commit, and included my ack.

Patch 5 and 6 implements earlier suggested cleanups and enhancements.

-- 
[]'s
Herton

[PATCH 1/6] floppy: don't call alloc_ordered_workqueue inside the alloc_disk loop

[Herton Ronaldo Krzesinski]

Since commit 070ad7e ("floppy: convert to delayed work and single-thread
wq"), we end up calling alloc_ordered_workqueue multiple times inside
the loop, which shouldn't be intended. Besides the leak, other side
effect in the current code is if blk_init_queue fails, we would end up
calling unregister_blkdev even if we didn't call yet register_blkdev.

Just moved the allocation of floppy_wq before the loop, and adjusted the
code accordingly.

Cc: stable@vger.kernel.org # 3.5+
Acked-by: Vivek Goyal <vgoyal@redhat.com>
Reviewed-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
---
 drivers/block/floppy.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index a7d6347..c8d9e68 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4138,6 +4138,10 @@ static int __init do_floppy_init(void)
 
 	raw_cmd = NULL;
 
+	floppy_wq = alloc_ordered_workqueue("floppy", 0);
+	if (!floppy_wq)
+		return -ENOMEM;
+
 	for (dr = 0; dr < N_DRIVE; dr++) {
 		disks[dr] = alloc_disk(1);
 		if (!disks[dr]) {
@@ -4145,16 +4149,10 @@ static int __init do_floppy_init(void)
 			goto out_put_disk;
 		}
 
-		floppy_wq = alloc_ordered_workqueue("floppy", 0);
-		if (!floppy_wq) {
-			err = -ENOMEM;
-			goto out_put_disk;
-		}
-
 		disks[dr]->queue = blk_init_queue(do_fd_request, &floppy_lock);
 		if (!disks[dr]->queue) {
 			err = -ENOMEM;
-			goto out_destroy_workq;
+			goto out_put_disk;
 		}
 
 		blk_queue_max_hw_sectors(disks[dr]->queue, 64);
@@ -4318,8 +4316,6 @@ out_release_dma:
 out_unreg_region:
 	blk_unregister_region(MKDEV(FLOPPY_MAJOR, 0), 256);
 	platform_driver_unregister(&floppy_driver);
-out_destroy_workq:
-	destroy_workqueue(floppy_wq);
 out_unreg_blkdev:
 	unregister_blkdev(FLOPPY_MAJOR, "fd");
 out_put_disk:
@@ -4335,6 +4331,7 @@ out_put_disk:
 		}
 		put_disk(disks[dr]);
 	}
+	destroy_workqueue(floppy_wq);
 	return err;
 }
 
-- 
1.7.9.5

[PATCH 2/6] floppy: do put_disk on current dr if blk_init_queue fails

[Herton Ronaldo Krzesinski]

If blk_init_queue fails, we do not call put_disk on the current dr
(dr is decremented first in the error handling loop).

Cc: stable@vger.kernel.org
Reviewed-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
---
 drivers/block/floppy.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index c8d9e68..1e09e99 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4151,6 +4151,7 @@ static int __init do_floppy_init(void)
 
 		disks[dr]->queue = blk_init_queue(do_fd_request, &floppy_lock);
 		if (!disks[dr]->queue) {
+			put_disk(disks[dr]);
 			err = -ENOMEM;
 			goto out_put_disk;
 		}
-- 
1.7.9.5

[PATCH 3/6] genhd: Make put_disk() safe for disks that have not been registered

[Herton Ronaldo Krzesinski]

From: Ben Hutchings <ben@decadent.org.uk>

Since commit 523e1d3 ('block: make gendisk hold a reference to its
queue'), add_disk() adds a reference to disk->queue, which is then
dropped by disk_release().  But if a disk is destroyed without being
registered through add_disk() (or if add_disk() fails at the first
hurdle) then we have a reference imbalance.

Use the GENHD_FL_UP flag to tell whether this extra reference has been
added.  Remove the incomplete workaround from the floppy driver.

Cc: stable@vger.kernel.org
Acked-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 block/genhd.c          |    6 +++---
 drivers/block/floppy.c |   16 +---------------
 2 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index d839723..633751d 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -587,8 +587,6 @@ void add_disk(struct gendisk *disk)
 	WARN_ON(disk->minors && !(disk->major || disk->first_minor));
 	WARN_ON(!disk->minors && !(disk->flags & GENHD_FL_EXT_DEVT));
 
-	disk->flags |= GENHD_FL_UP;
-
 	retval = blk_alloc_devt(&disk->part0, &devt);
 	if (retval) {
 		WARN_ON(1);
@@ -596,6 +594,8 @@ void add_disk(struct gendisk *disk)
 	}
 	disk_to_dev(disk)->devt = devt;
 
+	disk->flags |= GENHD_FL_UP;
+
 	/* ->major and ->first_minor aren't supposed to be
 	 * dereferenced from here on, but set them just in case.
 	 */
@@ -1105,7 +1105,7 @@ static void disk_release(struct device *dev)
 	disk_replace_part_tbl(disk, NULL);
 	free_part_stats(&disk->part0);
 	free_part_info(&disk->part0);
-	if (disk->queue)
+	if (disk->queue && disk->flags & GENHD_FL_UP)
 		blk_put_queue(disk->queue);
 	kfree(disk);
 }
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 1e09e99..32551f2 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4322,14 +4322,8 @@ out_unreg_blkdev:
 out_put_disk:
 	while (dr--) {
 		del_timer_sync(&motor_off_timer[dr]);
-		if (disks[dr]->queue) {
+		if (disks[dr]->queue)
 			blk_cleanup_queue(disks[dr]->queue);
-			/*
-			 * put_disk() is not paired with add_disk() and
-			 * will put queue reference one extra time. fix it.
-			 */
-			disks[dr]->queue = NULL;
-		}
 		put_disk(disks[dr]);
 	}
 	destroy_workqueue(floppy_wq);
@@ -4558,14 +4552,6 @@ static void __exit floppy_module_exit(void)
 		}
 		blk_cleanup_queue(disks[drive]->queue);
 
-		/*
-		 * These disks have not called add_disk().  Don't put down
-		 * queue reference in put_disk().
-		 */
-		if (!(allowed_drive_mask & (1 << drive)) ||
-		    fdc_state[FDC(drive)].version == FDC_NONE)
-			disks[drive]->queue = NULL;
-
 		put_disk(disks[drive]);
 	}
 
-- 
1.7.9.5

[PATCH 4/6] floppy: properly handle failure on add_disk loop

[Herton Ronaldo Krzesinski]

On floppy initialization, if something failed inside the loop we call
add_disk, there was no cleanup of previous iterations in the error
handling.

Cc: stable@vger.kernel.org
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
---
 drivers/block/floppy.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 32551f2..2197c88 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4293,7 +4293,7 @@ static int __init do_floppy_init(void)
 
 		err = platform_device_register(&floppy_device[drive]);
 		if (err)
-			goto out_release_dma;
+			goto out_remove_drives;
 
 		err = device_create_file(&floppy_device[drive].dev,
 					 &dev_attr_cmos);
@@ -4311,6 +4311,15 @@ static int __init do_floppy_init(void)
 
 out_unreg_platform_dev:
 	platform_device_unregister(&floppy_device[drive]);
+out_remove_drives:
+	while (drive--) {
+		if ((allowed_drive_mask & (1 << drive)) &&
+		    fdc_state[FDC(drive)].version != FDC_NONE) {
+			del_gendisk(disks[drive]);
+			device_remove_file(&floppy_device[drive].dev, &dev_attr_cmos);
+			platform_device_unregister(&floppy_device[drive]);
+		}
+	}
 out_release_dma:
 	if (atomic_read(&usage_count))
 		floppy_release_irq_and_dma();
-- 
1.7.9.5

[PATCH 5/6] floppy: use common function to check if floppies can be registered

[Herton Ronaldo Krzesinski]

The same checks to see if a drive can be or is registered are
repeated through the code, factor out the checks in a common function
and replace the repeated checks with it.

Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
---
 drivers/block/floppy.c |   23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 2197c88..c4e6152 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4110,12 +4110,19 @@ static struct platform_driver floppy_driver = {
 
 static struct platform_device floppy_device[N_DRIVE];
 
+static bool floppy_available(int drive)
+{
+	if (!(allowed_drive_mask & (1 << drive)))
+		return false;
+	if (fdc_state[FDC(drive)].version == FDC_NONE)
+		return false;
+	return true;
+}
+
 static struct kobject *floppy_find(dev_t dev, int *part, void *data)
 {
 	int drive = (*part & 3) | ((*part & 0x80) >> 5);
-	if (drive >= N_DRIVE ||
-	    !(allowed_drive_mask & (1 << drive)) ||
-	    fdc_state[FDC(drive)].version == FDC_NONE)
+	if (drive >= N_DRIVE || !floppy_available(drive))
 		return NULL;
 	if (((*part >> 2) & 0x1f) >= ARRAY_SIZE(floppy_type))
 		return NULL;
@@ -4282,9 +4289,7 @@ static int __init do_floppy_init(void)
 	}
 
 	for (drive = 0; drive < N_DRIVE; drive++) {
-		if (!(allowed_drive_mask & (1 << drive)))
-			continue;
-		if (fdc_state[FDC(drive)].version == FDC_NONE)
+		if (!floppy_available(drive))
 			continue;
 
 		floppy_device[drive].name = floppy_device_name;
@@ -4313,8 +4318,7 @@ out_unreg_platform_dev:
 	platform_device_unregister(&floppy_device[drive]);
 out_remove_drives:
 	while (drive--) {
-		if ((allowed_drive_mask & (1 << drive)) &&
-		    fdc_state[FDC(drive)].version != FDC_NONE) {
+		if (floppy_available(drive)) {
 			del_gendisk(disks[drive]);
 			device_remove_file(&floppy_device[drive].dev, &dev_attr_cmos);
 			platform_device_unregister(&floppy_device[drive]);
@@ -4553,8 +4557,7 @@ static void __exit floppy_module_exit(void)
 	for (drive = 0; drive < N_DRIVE; drive++) {
 		del_timer_sync(&motor_off_timer[drive]);
 
-		if ((allowed_drive_mask & (1 << drive)) &&
-		    fdc_state[FDC(drive)].version != FDC_NONE) {
+		if (floppy_available(drive)) {
 			del_gendisk(disks[drive]);
 			device_remove_file(&floppy_device[drive].dev, &dev_attr_cmos);
 			platform_device_unregister(&floppy_device[drive]);
-- 
1.7.9.5

[PATCH 6/6] floppy: remove dr, reuse drive on do_floppy_init

[Herton Ronaldo Krzesinski]

This is a small cleanup, that also may turn error handling of
unitialized disks more readable. We don't need a separate variable to
track allocated disks, remove dr and reuse drive variable instead.

Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
---
 drivers/block/floppy.c |   43 ++++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index c4e6152..95e52879 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4132,8 +4132,7 @@ static struct kobject *floppy_find(dev_t dev, int *part, void *data)
 
 static int __init do_floppy_init(void)
 {
-	int i, unit, drive;
-	int err, dr;
+	int i, unit, drive, err;
 
 	set_debugt();
 	interruptjiffies = resultjiffies = jiffies;
@@ -4149,29 +4148,28 @@ static int __init do_floppy_init(void)
 	if (!floppy_wq)
 		return -ENOMEM;
 
-	for (dr = 0; dr < N_DRIVE; dr++) {
-		disks[dr] = alloc_disk(1);
-		if (!disks[dr]) {
+	for (drive = 0; drive < N_DRIVE; drive++) {
+		disks[drive] = alloc_disk(1);
+		if (!disks[drive]) {
 			err = -ENOMEM;
 			goto out_put_disk;
 		}
 
-		disks[dr]->queue = blk_init_queue(do_fd_request, &floppy_lock);
-		if (!disks[dr]->queue) {
-			put_disk(disks[dr]);
+		disks[drive]->queue = blk_init_queue(do_fd_request, &floppy_lock);
+		if (!disks[drive]->queue) {
 			err = -ENOMEM;
 			goto out_put_disk;
 		}
 
-		blk_queue_max_hw_sectors(disks[dr]->queue, 64);
-		disks[dr]->major = FLOPPY_MAJOR;
-		disks[dr]->first_minor = TOMINOR(dr);
-		disks[dr]->fops = &floppy_fops;
-		sprintf(disks[dr]->disk_name, "fd%d", dr);
+		blk_queue_max_hw_sectors(disks[drive]->queue, 64);
+		disks[drive]->major = FLOPPY_MAJOR;
+		disks[drive]->first_minor = TOMINOR(drive);
+		disks[drive]->fops = &floppy_fops;
+		sprintf(disks[drive]->disk_name, "fd%d", drive);
 
-		init_timer(&motor_off_timer[dr]);
-		motor_off_timer[dr].data = dr;
-		motor_off_timer[dr].function = motor_off_callback;
+		init_timer(&motor_off_timer[drive]);
+		motor_off_timer[drive].data = drive;
+		motor_off_timer[drive].function = motor_off_callback;
 	}
 
 	err = register_blkdev(FLOPPY_MAJOR, "fd");
@@ -4333,11 +4331,14 @@ out_unreg_region:
 out_unreg_blkdev:
 	unregister_blkdev(FLOPPY_MAJOR, "fd");
 out_put_disk:
-	while (dr--) {
-		del_timer_sync(&motor_off_timer[dr]);
-		if (disks[dr]->queue)
-			blk_cleanup_queue(disks[dr]->queue);
-		put_disk(disks[dr]);
+	for (drive = 0; drive < N_DRIVE; drive++) {
+		if (!disks[drive])
+			break;
+		if (disks[drive]->queue) {
+			del_timer_sync(&motor_off_timer[drive]);
+			blk_cleanup_queue(disks[drive]->queue);
+		}
+		put_disk(disks[drive]);
 	}
 	destroy_workqueue(floppy_wq);
 	return err;
-- 
1.7.9.5

Re: Bug fixes/cleanups for floppy driver (v4)

[Jiri Kosina]

On Mon, 27 Aug 2012, Herton Ronaldo Krzesinski wrote:

> Following this are fixes for bugs noticed on floppy driver, and some
> extra cleanups. Version history below:
> 
> v2: separate fixes, and incorporate suggestions by Vivek Goyal.
>     also I splitted the cleanups from fixes.
> v3: remove dr, also this possibly makes error handling more readable
>     (suggested by by Vivek Goyal), incorporate acks.
> v4: re-shuffle series with patch from Ben Hutchings handling the queue
>     reference in a better way inside genhd
> 
> Patches 1-2 and 4 are general bug fixes for problems noted in the floppy
> driver initialization and its error handling. They are the minimal fix
> required, so it's suitable and easy to pick for stable releases (so
> are Cc'd to stable), and thus not contain cleanups or re-factoring.
> 
> Patch 3 is a patch from Ben Hutchings fixing in a better way the extra
> queue reference issue that was introduced in commit 523e1d3 ('block:
> make gendisk hold a reference to its queue'). Instead of making the
> drivers worry about the extra reference, fix it and handle that in a
> better way inside genhd. I'm not sure if this should go with the floppy
> changes or picked up separate for the block tree. In any case, patch 6
> will only apply after it is applied. His patch was proposed with the
> previous series I submitted and handles the problem better so I took the
> liberty to include now, I just fixed the changelog as was noticed that
> time to reference the right commit, and included my ack.
> 
> Patch 5 and 6 implements earlier suggested cleanups and enhancements.

I have now finally got to reviewing the series and all the previous 
Vivek's and Ben's comments and verifying this on real HW, sorry for the 
delay.

Applied, thanks Herton.

-- 
Jiri Kosina
SUSE Labs