* [PATCH v3 1/6] floppy: don't call alloc_ordered_workqueue inside the alloc_disk loop
2012-08-13 18:16 Bug fixes/cleanups for floppy driver (v2) Herton Ronaldo Krzesinski
@ 2012-08-13 18:16 ` Herton Ronaldo Krzesinski
2012-08-14 2:01 ` Ben Hutchings
2012-08-13 18:16 ` [PATCH v3 2/6] floppy: do put_disk on current dr if blk_init_queue fails Herton Ronaldo Krzesinski
` (5 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Herton Ronaldo Krzesinski @ 2012-08-13 18:16 UTC (permalink / raw)
To: Jiri Kosina
Cc: Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel, Vivek Goyal,
Ben Hutchings
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>
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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3 1/6] floppy: don't call alloc_ordered_workqueue inside the alloc_disk loop
2012-08-13 18:16 ` [PATCH v3 1/6] floppy: don't call alloc_ordered_workqueue inside the alloc_disk loop Herton Ronaldo Krzesinski
@ 2012-08-14 2:01 ` Ben Hutchings
0 siblings, 0 replies; 22+ messages in thread
From: Ben Hutchings @ 2012-08-14 2:01 UTC (permalink / raw)
To: Herton Ronaldo Krzesinski
Cc: Jiri Kosina, Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel,
Vivek Goyal
[-- Attachment #1: Type: text/plain, Size: 2373 bytes --]
On Mon, 2012-08-13 at 15:16 -0300, Herton Ronaldo Krzesinski wrote:
> 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>
> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
Reviewed-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> 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;
> }
>
--
Ben Hutchings
I say we take off; nuke the site from orbit. It's the only way to be sure.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 2/6] floppy: do put_disk on current dr if blk_init_queue fails
2012-08-13 18:16 Bug fixes/cleanups for floppy driver (v2) Herton Ronaldo Krzesinski
2012-08-13 18:16 ` [PATCH v3 1/6] floppy: don't call alloc_ordered_workqueue inside the alloc_disk loop Herton Ronaldo Krzesinski
@ 2012-08-13 18:16 ` Herton Ronaldo Krzesinski
2012-08-14 2:04 ` Ben Hutchings
2012-08-14 19:48 ` Vivek Goyal
2012-08-13 18:16 ` [PATCH v3 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling Herton Ronaldo Krzesinski
` (4 subsequent siblings)
6 siblings, 2 replies; 22+ messages in thread
From: Herton Ronaldo Krzesinski @ 2012-08-13 18:16 UTC (permalink / raw)
To: Jiri Kosina
Cc: Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel, Vivek Goyal,
Ben Hutchings
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
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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/6] floppy: do put_disk on current dr if blk_init_queue fails
2012-08-13 18:16 ` [PATCH v3 2/6] floppy: do put_disk on current dr if blk_init_queue fails Herton Ronaldo Krzesinski
@ 2012-08-14 2:04 ` Ben Hutchings
2012-08-14 19:48 ` Vivek Goyal
1 sibling, 0 replies; 22+ messages in thread
From: Ben Hutchings @ 2012-08-14 2:04 UTC (permalink / raw)
To: Herton Ronaldo Krzesinski
Cc: Jiri Kosina, Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel,
Vivek Goyal
[-- Attachment #1: Type: text/plain, Size: 958 bytes --]
On Mon, 2012-08-13 at 15:16 -0300, Herton Ronaldo Krzesinski wrote:
> 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
> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
Reviewed-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> 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;
> }
--
Ben Hutchings
I say we take off; nuke the site from orbit. It's the only way to be sure.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/6] floppy: do put_disk on current dr if blk_init_queue fails
2012-08-13 18:16 ` [PATCH v3 2/6] floppy: do put_disk on current dr if blk_init_queue fails Herton Ronaldo Krzesinski
2012-08-14 2:04 ` Ben Hutchings
@ 2012-08-14 19:48 ` Vivek Goyal
2012-08-14 20:08 ` Herton Ronaldo Krzesinski
1 sibling, 1 reply; 22+ messages in thread
From: Vivek Goyal @ 2012-08-14 19:48 UTC (permalink / raw)
To: Herton Ronaldo Krzesinski
Cc: Jiri Kosina, Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel,
Ben Hutchings
On Mon, Aug 13, 2012 at 03:16:23PM -0300, Herton Ronaldo Krzesinski wrote:
> 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
> 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;
> }
I think it will create conflict with patch 6. Will it not call put_disk()
twice on disks[dr].
Why are we retaining this patch given the fact that we will loop through
all the drives and queues in out_put_disk.
Thanks
Vivek
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/6] floppy: do put_disk on current dr if blk_init_queue fails
2012-08-14 19:48 ` Vivek Goyal
@ 2012-08-14 20:08 ` Herton Ronaldo Krzesinski
0 siblings, 0 replies; 22+ messages in thread
From: Herton Ronaldo Krzesinski @ 2012-08-14 20:08 UTC (permalink / raw)
To: Vivek Goyal
Cc: Jiri Kosina, Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel,
Ben Hutchings
On Tue, Aug 14, 2012 at 03:48:35PM -0400, Vivek Goyal wrote:
> On Mon, Aug 13, 2012 at 03:16:23PM -0300, Herton Ronaldo Krzesinski wrote:
> > 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
> > 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;
> > }
>
> I think it will create conflict with patch 6. Will it not call put_disk()
> twice on disks[dr].
It'll not conflict, because I took care of removing it on patch 6.
> Why are we retaining this patch given the fact that we will loop through
> all the drives and queues in out_put_disk.
Because I wanted a minimal bug fix for stable, didn't want to bring
entire code shuffling/cleanup in patch 6 for stable, and thus didn't
mark that patch for stable.
>
> Thanks
> Vivek
>
--
[]'s
Herton
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling
2012-08-13 18:16 Bug fixes/cleanups for floppy driver (v2) Herton Ronaldo Krzesinski
2012-08-13 18:16 ` [PATCH v3 1/6] floppy: don't call alloc_ordered_workqueue inside the alloc_disk loop Herton Ronaldo Krzesinski
2012-08-13 18:16 ` [PATCH v3 2/6] floppy: do put_disk on current dr if blk_init_queue fails Herton Ronaldo Krzesinski
@ 2012-08-13 18:16 ` Herton Ronaldo Krzesinski
2012-08-14 3:20 ` Ben Hutchings
2012-08-13 18:16 ` [PATCH v3 4/6] floppy: properly handle failure on add_disk loop Herton Ronaldo Krzesinski
` (3 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Herton Ronaldo Krzesinski @ 2012-08-13 18:16 UTC (permalink / raw)
To: Jiri Kosina
Cc: Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel, Vivek Goyal,
Ben Hutchings
After commit 3f9a5aa ("floppy: Cleanup disk->queue before caling
put_disk() if add_disk() was never called"), if something fails in the
add_disk loop, we unconditionally set disks[dr]->queue to NULL. But
that's wrong, since we may have succesfully done an add_disk on some of
the drives previously in the loop, and in this case we would end up with
an extra reference to the disks[dr]->queue.
Add a new global array to mark "registered" disks, and use that to check
if we did an add_disk on one of the disks already. Using an array to
track added disks also will help to simplify/cleanup code later, as
suggested by Vivek Goyal.
Cc: stable@vger.kernel.org
Acked-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
---
drivers/block/floppy.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 1e09e99..9272203 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -409,6 +409,7 @@ static struct floppy_drive_struct drive_state[N_DRIVE];
static struct floppy_write_errors write_errors[N_DRIVE];
static struct timer_list motor_off_timer[N_DRIVE];
static struct gendisk *disks[N_DRIVE];
+static bool disk_registered[N_DRIVE];
static struct block_device *opened_bdev[N_DRIVE];
static DEFINE_MUTEX(open_lock);
static struct floppy_raw_cmd *raw_cmd, default_raw_cmd;
@@ -4305,6 +4306,7 @@ static int __init do_floppy_init(void)
disks[drive]->flags |= GENHD_FL_REMOVABLE;
disks[drive]->driverfs_dev = &floppy_device[drive].dev;
add_disk(disks[drive]);
+ disk_registered[drive] = true;
}
return 0;
@@ -4328,7 +4330,8 @@ out_put_disk:
* put_disk() is not paired with add_disk() and
* will put queue reference one extra time. fix it.
*/
- disks[dr]->queue = NULL;
+ if (!disk_registered[dr])
+ disks[dr]->queue = NULL;
}
put_disk(disks[dr]);
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling
2012-08-13 18:16 ` [PATCH v3 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling Herton Ronaldo Krzesinski
@ 2012-08-14 3:20 ` Ben Hutchings
2012-08-14 9:03 ` Stanislaw Gruszka
` (2 more replies)
0 siblings, 3 replies; 22+ messages in thread
From: Ben Hutchings @ 2012-08-14 3:20 UTC (permalink / raw)
To: Herton Ronaldo Krzesinski, Jens Axboe
Cc: Jiri Kosina, Andrew Morton, Tejun Heo, linux-kernel, Vivek Goyal,
Stanislaw Gruszka
[-- Attachment #1: Type: text/plain, Size: 3865 bytes --]
On Mon, 2012-08-13 at 15:16 -0300, Herton Ronaldo Krzesinski wrote:
> After commit 3f9a5aa ("floppy: Cleanup disk->queue before caling
> put_disk() if add_disk() was never called"), if something fails in the
> add_disk loop, we unconditionally set disks[dr]->queue to NULL. But
> that's wrong, since we may have succesfully done an add_disk on some of
> the drives previously in the loop, and in this case we would end up with
> an extra reference to the disks[dr]->queue.
>
> Add a new global array to mark "registered" disks, and use that to check
> if we did an add_disk on one of the disks already. Using an array to
> track added disks also will help to simplify/cleanup code later, as
> suggested by Vivek Goyal.
[...]
It's totally ridiculous that a driver should have to do this. Any
registered disk should have the GENHD_FL_UP flag set... so why can't
genhd check it? It doesn't look like floppy is the only driver affected
by this problem, either. So I suggest the following general fix
(untested):
---
Subject: genhd: Make put_disk() safe for disks that have not been registered
Since commit 9f53d2f ('block: fix __blkdev_get and add_disk race
condition'), 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.
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Cc: stable@vger.kernel.org
---
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 cac7366..6201212 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 a7d6347..cf27922 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4325,14 +4325,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]);
}
return err;
@@ -4560,14 +4554,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]);
}
--
Ben Hutchings
I say we take off; nuke the site from orbit. It's the only way to be sure.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling
2012-08-14 3:20 ` Ben Hutchings
@ 2012-08-14 9:03 ` Stanislaw Gruszka
2012-08-14 14:26 ` Herton Ronaldo Krzesinski
2012-08-14 14:28 ` Ben Hutchings
2012-08-14 14:33 ` Herton Ronaldo Krzesinski
2012-08-14 20:00 ` Vivek Goyal
2 siblings, 2 replies; 22+ messages in thread
From: Stanislaw Gruszka @ 2012-08-14 9:03 UTC (permalink / raw)
To: Ben Hutchings
Cc: Herton Ronaldo Krzesinski, Jens Axboe, Jiri Kosina,
Andrew Morton, Tejun Heo, linux-kernel, Vivek Goyal
On Tue, Aug 14, 2012 at 04:20:39AM +0100, Ben Hutchings wrote:
> On Mon, 2012-08-13 at 15:16 -0300, Herton Ronaldo Krzesinski wrote:
> > After commit 3f9a5aa ("floppy: Cleanup disk->queue before caling
> > put_disk() if add_disk() was never called"), if something fails in the
> > add_disk loop, we unconditionally set disks[dr]->queue to NULL. But
> > that's wrong, since we may have succesfully done an add_disk on some of
> > the drives previously in the loop, and in this case we would end up with
> > an extra reference to the disks[dr]->queue.
> >
> > Add a new global array to mark "registered" disks, and use that to check
> > if we did an add_disk on one of the disks already. Using an array to
> > track added disks also will help to simplify/cleanup code later, as
> > suggested by Vivek Goyal.
> [...]
>
> It's totally ridiculous that a driver should have to do this. Any
> registered disk should have the GENHD_FL_UP flag set... so why can't
> genhd check it? It doesn't look like floppy is the only driver affected
> by this problem, either. So I suggest the following general fix
> (untested):
>
> ---
> Subject: genhd: Make put_disk() safe for disks that have not been registered
>
> Since commit 9f53d2f ('block: fix __blkdev_get and add_disk race
> condition'), add_disk() adds a reference to disk->queue,
I do not see this? Commit 9f53d2fe insert disk_alloc_events() to add_disk(),
but disk_alloc_events() function does not get any reference to disk->queue,
I missed something?
Stanislaw
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling
2012-08-14 9:03 ` Stanislaw Gruszka
@ 2012-08-14 14:26 ` Herton Ronaldo Krzesinski
2012-08-14 14:28 ` Ben Hutchings
1 sibling, 0 replies; 22+ messages in thread
From: Herton Ronaldo Krzesinski @ 2012-08-14 14:26 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: Ben Hutchings, Jens Axboe, Jiri Kosina, Andrew Morton, Tejun Heo,
linux-kernel, Vivek Goyal
On Tue, Aug 14, 2012 at 11:03:30AM +0200, Stanislaw Gruszka wrote:
> On Tue, Aug 14, 2012 at 04:20:39AM +0100, Ben Hutchings wrote:
> > It's totally ridiculous that a driver should have to do this. Any
> > registered disk should have the GENHD_FL_UP flag set... so why can't
> > genhd check it? It doesn't look like floppy is the only driver affected
> > by this problem, either. So I suggest the following general fix
> > (untested):
> >
> > ---
> > Subject: genhd: Make put_disk() safe for disks that have not been registered
> >
> > Since commit 9f53d2f ('block: fix __blkdev_get and add_disk race
> > condition'), add_disk() adds a reference to disk->queue,
>
> I do not see this? Commit 9f53d2fe insert disk_alloc_events() to add_disk(),
> but disk_alloc_events() function does not get any reference to disk->queue,
> I missed something?
I think he meant commit 523e1d3 ("block: make gendisk hold a reference
to its queue") instead.
>
> Stanislaw
>
--
[]'s
Herton
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling
2012-08-14 9:03 ` Stanislaw Gruszka
2012-08-14 14:26 ` Herton Ronaldo Krzesinski
@ 2012-08-14 14:28 ` Ben Hutchings
1 sibling, 0 replies; 22+ messages in thread
From: Ben Hutchings @ 2012-08-14 14:28 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: Herton Ronaldo Krzesinski, Jens Axboe, Jiri Kosina,
Andrew Morton, Tejun Heo, linux-kernel, Vivek Goyal
[-- Attachment #1: Type: text/plain, Size: 2032 bytes --]
On Tue, 2012-08-14 at 11:03 +0200, Stanislaw Gruszka wrote:
> On Tue, Aug 14, 2012 at 04:20:39AM +0100, Ben Hutchings wrote:
> > On Mon, 2012-08-13 at 15:16 -0300, Herton Ronaldo Krzesinski wrote:
> > > After commit 3f9a5aa ("floppy: Cleanup disk->queue before caling
> > > put_disk() if add_disk() was never called"), if something fails in the
> > > add_disk loop, we unconditionally set disks[dr]->queue to NULL. But
> > > that's wrong, since we may have succesfully done an add_disk on some of
> > > the drives previously in the loop, and in this case we would end up with
> > > an extra reference to the disks[dr]->queue.
> > >
> > > Add a new global array to mark "registered" disks, and use that to check
> > > if we did an add_disk on one of the disks already. Using an array to
> > > track added disks also will help to simplify/cleanup code later, as
> > > suggested by Vivek Goyal.
> > [...]
> >
> > It's totally ridiculous that a driver should have to do this. Any
> > registered disk should have the GENHD_FL_UP flag set... so why can't
> > genhd check it? It doesn't look like floppy is the only driver affected
> > by this problem, either. So I suggest the following general fix
> > (untested):
> >
> > ---
> > Subject: genhd: Make put_disk() safe for disks that have not been registered
> >
> > Since commit 9f53d2f ('block: fix __blkdev_get and add_disk race
> > condition'), add_disk() adds a reference to disk->queue,
>
> I do not see this? Commit 9f53d2fe insert disk_alloc_events() to add_disk(),
> but disk_alloc_events() function does not get any reference to disk->queue,
> I missed something?
Sorry, not sure why I pointed to that one. The reference should of
course be to:
commit 523e1d399ce0e23bec562abe2b2f8d297af81161
Author: Tejun Heo <tj@kernel.org>
Date: Wed Oct 19 14:31:07 2011 +0200
block: make gendisk hold a reference to its queue
Ben.
--
Ben Hutchings
I say we take off; nuke the site from orbit. It's the only way to be sure.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling
2012-08-14 3:20 ` Ben Hutchings
2012-08-14 9:03 ` Stanislaw Gruszka
@ 2012-08-14 14:33 ` Herton Ronaldo Krzesinski
2012-08-14 20:00 ` Vivek Goyal
2 siblings, 0 replies; 22+ messages in thread
From: Herton Ronaldo Krzesinski @ 2012-08-14 14:33 UTC (permalink / raw)
To: Ben Hutchings
Cc: Jens Axboe, Jiri Kosina, Andrew Morton, Tejun Heo, linux-kernel,
Vivek Goyal, Stanislaw Gruszka
On Tue, Aug 14, 2012 at 04:20:39AM +0100, Ben Hutchings wrote:
> It's totally ridiculous that a driver should have to do this. Any
> registered disk should have the GENHD_FL_UP flag set... so why can't
> genhd check it? It doesn't look like floppy is the only driver affected
> by this problem, either. So I suggest the following general fix
> (untested):
>
> ---
> Subject: genhd: Make put_disk() safe for disks that have not been registered
>
> Since commit 9f53d2f ('block: fix __blkdev_get and add_disk race
> condition'), 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.
Indeed this is a more sane/right approach.
Acked-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
Just the changelog is pointing to the wrong commit as already noted by
Stanislaw.
>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> Cc: stable@vger.kernel.org
--
[]'s
Herton
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling
2012-08-14 3:20 ` Ben Hutchings
2012-08-14 9:03 ` Stanislaw Gruszka
2012-08-14 14:33 ` Herton Ronaldo Krzesinski
@ 2012-08-14 20:00 ` Vivek Goyal
2 siblings, 0 replies; 22+ messages in thread
From: Vivek Goyal @ 2012-08-14 20:00 UTC (permalink / raw)
To: Ben Hutchings
Cc: Herton Ronaldo Krzesinski, Jens Axboe, Jiri Kosina,
Andrew Morton, Tejun Heo, linux-kernel, Stanislaw Gruszka
On Tue, Aug 14, 2012 at 04:20:39AM +0100, Ben Hutchings wrote:
> On Mon, 2012-08-13 at 15:16 -0300, Herton Ronaldo Krzesinski wrote:
> > After commit 3f9a5aa ("floppy: Cleanup disk->queue before caling
> > put_disk() if add_disk() was never called"), if something fails in the
> > add_disk loop, we unconditionally set disks[dr]->queue to NULL. But
> > that's wrong, since we may have succesfully done an add_disk on some of
> > the drives previously in the loop, and in this case we would end up with
> > an extra reference to the disks[dr]->queue.
> >
> > Add a new global array to mark "registered" disks, and use that to check
> > if we did an add_disk on one of the disks already. Using an array to
> > track added disks also will help to simplify/cleanup code later, as
> > suggested by Vivek Goyal.
> [...]
>
> It's totally ridiculous that a driver should have to do this. Any
> registered disk should have the GENHD_FL_UP flag set... so why can't
> genhd check it? It doesn't look like floppy is the only driver affected
> by this problem, either. So I suggest the following general fix
> (untested):
>
> ---
> Subject: genhd: Make put_disk() safe for disks that have not been registered
>
> Since commit 9f53d2f ('block: fix __blkdev_get and add_disk race
> condition'), 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.
>
Checking for GENHD_FL_UP to represent whether we took a reference on the
disk->queue or not sounds reasonable.
Thanks
Vivek
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 4/6] floppy: properly handle failure on add_disk loop
2012-08-13 18:16 Bug fixes/cleanups for floppy driver (v2) Herton Ronaldo Krzesinski
` (2 preceding siblings ...)
2012-08-13 18:16 ` [PATCH v3 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling Herton Ronaldo Krzesinski
@ 2012-08-13 18:16 ` Herton Ronaldo Krzesinski
2012-08-14 3:31 ` Ben Hutchings
2012-08-13 18:16 ` [PATCH v3 5/6] floppy: use disk_registered for checking if a drive is present Herton Ronaldo Krzesinski
` (2 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Herton Ronaldo Krzesinski @ 2012-08-13 18:16 UTC (permalink / raw)
To: Jiri Kosina
Cc: Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel, Vivek Goyal,
Ben Hutchings
On do_floppy_init, 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
Acked-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
---
drivers/block/floppy.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 9272203..3eafe93 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4294,7 +4294,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);
@@ -4313,6 +4313,14 @@ static int __init do_floppy_init(void)
out_unreg_platform_dev:
platform_device_unregister(&floppy_device[drive]);
+out_remove_drives:
+ while (drive--) {
+ if (disk_registered[drive]) {
+ 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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3 4/6] floppy: properly handle failure on add_disk loop
2012-08-13 18:16 ` [PATCH v3 4/6] floppy: properly handle failure on add_disk loop Herton Ronaldo Krzesinski
@ 2012-08-14 3:31 ` Ben Hutchings
2012-08-14 14:43 ` Herton Ronaldo Krzesinski
0 siblings, 1 reply; 22+ messages in thread
From: Ben Hutchings @ 2012-08-14 3:31 UTC (permalink / raw)
To: Herton Ronaldo Krzesinski
Cc: Jiri Kosina, Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel,
Vivek Goyal
[-- Attachment #1: Type: text/plain, Size: 1832 bytes --]
On Mon, 2012-08-13 at 15:16 -0300, Herton Ronaldo Krzesinski wrote:
> On do_floppy_init, 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
> Acked-by: Vivek Goyal <vgoyal@redhat.com>
> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
This depends on 3/6. If that's replaced by my proposed fix, then:
> ---
> drivers/block/floppy.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index 9272203..3eafe93 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -4294,7 +4294,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);
> @@ -4313,6 +4313,14 @@ static int __init do_floppy_init(void)
>
> out_unreg_platform_dev:
> platform_device_unregister(&floppy_device[drive]);
> +out_remove_drives:
> + while (drive--) {
> + if (disk_registered[drive]) {
I think the test of allowed_drive_mask and FDC version before
registering each driver should be factored out into a function that you
can use here. There would then no need for the disk_registered array.
Ben.
> + 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();
--
Ben Hutchings
I say we take off; nuke the site from orbit. It's the only way to be sure.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 4/6] floppy: properly handle failure on add_disk loop
2012-08-14 3:31 ` Ben Hutchings
@ 2012-08-14 14:43 ` Herton Ronaldo Krzesinski
2012-08-27 23:53 ` Herton Ronaldo Krzesinski
0 siblings, 1 reply; 22+ messages in thread
From: Herton Ronaldo Krzesinski @ 2012-08-14 14:43 UTC (permalink / raw)
To: Ben Hutchings
Cc: Jiri Kosina, Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel,
Vivek Goyal
On Tue, Aug 14, 2012 at 04:31:23AM +0100, Ben Hutchings wrote:
> On Mon, 2012-08-13 at 15:16 -0300, Herton Ronaldo Krzesinski wrote:
> > On do_floppy_init, 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
> > Acked-by: Vivek Goyal <vgoyal@redhat.com>
> > Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
>
> This depends on 3/6. If that's replaced by my proposed fix, then:
>
> > ---
> > drivers/block/floppy.c | 10 +++++++++-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> > index 9272203..3eafe93 100644
> > --- a/drivers/block/floppy.c
> > +++ b/drivers/block/floppy.c
> > @@ -4294,7 +4294,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);
> > @@ -4313,6 +4313,14 @@ static int __init do_floppy_init(void)
> >
> > out_unreg_platform_dev:
> > platform_device_unregister(&floppy_device[drive]);
> > +out_remove_drives:
> > + while (drive--) {
> > + if (disk_registered[drive]) {
>
> I think the test of allowed_drive_mask and FDC version before
> registering each driver should be factored out into a function that you
> can use here. There would then no need for the disk_registered array.
Seems a better approach. I'll redo the patches, but I'm not sure how to
proceed now, since it depends on your change. Patches 4-6 that I did
needs to be dropped/redone. At first my patches 0001/0002 could be
applied with yours, then I would submit another series for the rest. Or
I could take your change and submit your patch along with my series.
>
> Ben.
>
--
[]'s
Herton
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 4/6] floppy: properly handle failure on add_disk loop
2012-08-14 14:43 ` Herton Ronaldo Krzesinski
@ 2012-08-27 23:53 ` Herton Ronaldo Krzesinski
0 siblings, 0 replies; 22+ messages in thread
From: Herton Ronaldo Krzesinski @ 2012-08-27 23:53 UTC (permalink / raw)
To: Ben Hutchings
Cc: Jiri Kosina, Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel,
Vivek Goyal
On Tue, Aug 14, 2012 at 11:43:11AM -0300, Herton Ronaldo Krzesinski wrote:
> On Tue, Aug 14, 2012 at 04:31:23AM +0100, Ben Hutchings wrote:
> > On Mon, 2012-08-13 at 15:16 -0300, Herton Ronaldo Krzesinski wrote:
> > > On do_floppy_init, 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
> > > Acked-by: Vivek Goyal <vgoyal@redhat.com>
> > > Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
> >
> > This depends on 3/6. If that's replaced by my proposed fix, then:
I redid the patch series with your proposed fix, and I'm going to submit it
again in some minutes, so I hope finally it goes forward.
> >
> > > ---
> > > drivers/block/floppy.c | 10 +++++++++-
> > > 1 file changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> > > index 9272203..3eafe93 100644
> > > --- a/drivers/block/floppy.c
> > > +++ b/drivers/block/floppy.c
> > > @@ -4294,7 +4294,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);
> > > @@ -4313,6 +4313,14 @@ static int __init do_floppy_init(void)
> > >
> > > out_unreg_platform_dev:
> > > platform_device_unregister(&floppy_device[drive]);
> > > +out_remove_drives:
> > > + while (drive--) {
> > > + if (disk_registered[drive]) {
> >
> > I think the test of allowed_drive_mask and FDC version before
> > registering each driver should be factored out into a function that you
> > can use here. There would then no need for the disk_registered array.
>
> Seems a better approach. I'll redo the patches, but I'm not sure how to
> proceed now, since it depends on your change. Patches 4-6 that I did
> needs to be dropped/redone. At first my patches 0001/0002 could be
> applied with yours, then I would submit another series for the rest. Or
> I could take your change and submit your patch along with my series.
>
> >
> > Ben.
> >
--
[]'s
Herton
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 5/6] floppy: use disk_registered for checking if a drive is present
2012-08-13 18:16 Bug fixes/cleanups for floppy driver (v2) Herton Ronaldo Krzesinski
` (3 preceding siblings ...)
2012-08-13 18:16 ` [PATCH v3 4/6] floppy: properly handle failure on add_disk loop Herton Ronaldo Krzesinski
@ 2012-08-13 18:16 ` Herton Ronaldo Krzesinski
2012-08-14 3:36 ` Ben Hutchings
2012-08-13 18:16 ` [PATCH v3 6/6] floppy: remove dr, reuse drive on do_floppy_init Herton Ronaldo Krzesinski
2012-08-13 18:21 ` Bug fixes/cleanups for floppy driver (v2) Herton Ronaldo Krzesinski
6 siblings, 1 reply; 22+ messages in thread
From: Herton Ronaldo Krzesinski @ 2012-08-13 18:16 UTC (permalink / raw)
To: Jiri Kosina
Cc: Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel, Vivek Goyal,
Ben Hutchings
Simplify/cleanup code, replacing remaining checks for drives present
using disk_registered array.
Acked-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
---
drivers/block/floppy.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 3eafe93..381b9c1 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4114,9 +4114,7 @@ static struct platform_device floppy_device[N_DRIVE];
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 || !disk_registered[drive])
return NULL;
if (((*part >> 2) & 0x1f) >= ARRAY_SIZE(floppy_type))
return NULL;
@@ -4561,8 +4559,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 (disk_registered[drive]) {
del_gendisk(disks[drive]);
device_remove_file(&floppy_device[drive].dev, &dev_attr_cmos);
platform_device_unregister(&floppy_device[drive]);
@@ -4573,8 +4570,7 @@ static void __exit floppy_module_exit(void)
* 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)
+ if (!disk_registered[drive])
disks[drive]->queue = NULL;
put_disk(disks[drive]);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3 5/6] floppy: use disk_registered for checking if a drive is present
2012-08-13 18:16 ` [PATCH v3 5/6] floppy: use disk_registered for checking if a drive is present Herton Ronaldo Krzesinski
@ 2012-08-14 3:36 ` Ben Hutchings
0 siblings, 0 replies; 22+ messages in thread
From: Ben Hutchings @ 2012-08-14 3:36 UTC (permalink / raw)
To: Herton Ronaldo Krzesinski
Cc: Jiri Kosina, Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel,
Vivek Goyal
[-- Attachment #1: Type: text/plain, Size: 2246 bytes --]
On Mon, 2012-08-13 at 15:16 -0300, Herton Ronaldo Krzesinski wrote:
> Simplify/cleanup code, replacing remaining checks for drives present
> using disk_registered array.
>
> Acked-by: Vivek Goyal <vgoyal@redhat.com>
> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
> ---
> drivers/block/floppy.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index 3eafe93..381b9c1 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -4114,9 +4114,7 @@ static struct platform_device floppy_device[N_DRIVE];
> 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 || !disk_registered[drive])
> return NULL;
> if (((*part >> 2) & 0x1f) >= ARRAY_SIZE(floppy_type))
> return NULL;
> @@ -4561,8 +4559,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 (disk_registered[drive]) {
> del_gendisk(disks[drive]);
> device_remove_file(&floppy_device[drive].dev, &dev_attr_cmos);
> platform_device_unregister(&floppy_device[drive]);
The same function I suggested for 4/6 would also be used here. A little
neater than yet another static array, I think. But I have nothing to do
with this code so you could just ignore my preferences. :-)
Ben.
> @@ -4573,8 +4570,7 @@ static void __exit floppy_module_exit(void)
> * 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)
> + if (!disk_registered[drive])
> disks[drive]->queue = NULL;
>
> put_disk(disks[drive]);
--
Ben Hutchings
I say we take off; nuke the site from orbit. It's the only way to be sure.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 6/6] floppy: remove dr, reuse drive on do_floppy_init
2012-08-13 18:16 Bug fixes/cleanups for floppy driver (v2) Herton Ronaldo Krzesinski
` (4 preceding siblings ...)
2012-08-13 18:16 ` [PATCH v3 5/6] floppy: use disk_registered for checking if a drive is present Herton Ronaldo Krzesinski
@ 2012-08-13 18:16 ` Herton Ronaldo Krzesinski
2012-08-13 18:21 ` Bug fixes/cleanups for floppy driver (v2) Herton Ronaldo Krzesinski
6 siblings, 0 replies; 22+ messages in thread
From: Herton Ronaldo Krzesinski @ 2012-08-13 18:16 UTC (permalink / raw)
To: Jiri Kosina
Cc: Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel, Vivek Goyal,
Ben Hutchings
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 | 46 +++++++++++++++++++++++-----------------------
1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 381b9c1..0312ba4 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4124,8 +4124,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;
@@ -4141,29 +4140,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");
@@ -4328,18 +4326,20 @@ 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);
+ 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() is not paired with add_disk() and
* will put queue reference one extra time. fix it.
*/
- if (!disk_registered[dr])
- disks[dr]->queue = NULL;
+ if (!disk_registered[drive])
+ disks[drive]->queue = NULL;
}
- put_disk(disks[dr]);
+ put_disk(disks[drive]);
}
destroy_workqueue(floppy_wq);
return err;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: Bug fixes/cleanups for floppy driver (v2)
2012-08-13 18:16 Bug fixes/cleanups for floppy driver (v2) Herton Ronaldo Krzesinski
` (5 preceding siblings ...)
2012-08-13 18:16 ` [PATCH v3 6/6] floppy: remove dr, reuse drive on do_floppy_init Herton Ronaldo Krzesinski
@ 2012-08-13 18:21 ` Herton Ronaldo Krzesinski
6 siblings, 0 replies; 22+ messages in thread
From: Herton Ronaldo Krzesinski @ 2012-08-13 18:21 UTC (permalink / raw)
To: Jiri Kosina
Cc: Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel, Vivek Goyal,
Ben Hutchings
On Mon, Aug 13, 2012 at 03:16:21PM -0300, Herton Ronaldo Krzesinski wrote:
> Following this are fixes for bugs I noticed on floppy driver, and some
> extra cleanups.
>
> 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.
Subject obviously should have been "... (v3)"
--
[]'s
Herton
^ permalink raw reply [flat|nested] 22+ messages in thread