* [PATCH 1/6] floppy: don't call alloc_ordered_workqueue inside the alloc_disk loop
2012-08-09 19:59 Bug fixes/cleanups for floppy driver (v2) Herton Ronaldo Krzesinski
@ 2012-08-09 19:59 ` Herton Ronaldo Krzesinski
2012-08-10 17:11 ` Vivek Goyal
2012-08-09 19:59 ` [PATCH 2/6] floppy: do put_disk on current dr if blk_init_queue fails Herton Ronaldo Krzesinski
` (4 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Herton Ronaldo Krzesinski @ 2012-08-09 19:59 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.
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
Cc: stable@vger.kernel.org # 3.5+
---
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] 15+ messages in thread* Re: [PATCH 1/6] floppy: don't call alloc_ordered_workqueue inside the alloc_disk loop
2012-08-09 19:59 ` [PATCH 1/6] floppy: don't call alloc_ordered_workqueue inside the alloc_disk loop Herton Ronaldo Krzesinski
@ 2012-08-10 17:11 ` Vivek Goyal
0 siblings, 0 replies; 15+ messages in thread
From: Vivek Goyal @ 2012-08-10 17:11 UTC (permalink / raw)
To: Herton Ronaldo Krzesinski
Cc: Jiri Kosina, Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel,
Ben Hutchings
On Thu, Aug 09, 2012 at 04:59:46PM -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.
>
> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
> Cc: stable@vger.kernel.org # 3.5+
Looks good to me.
Acked-by: Vivek Goyal <vgoyal@redhat.com>
Vivek
> ---
> 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 [flat|nested] 15+ messages in thread
* [PATCH 2/6] floppy: do put_disk on current dr if blk_init_queue fails
2012-08-09 19:59 Bug fixes/cleanups for floppy driver (v2) Herton Ronaldo Krzesinski
2012-08-09 19:59 ` [PATCH 1/6] floppy: don't call alloc_ordered_workqueue inside the alloc_disk loop Herton Ronaldo Krzesinski
@ 2012-08-09 19:59 ` Herton Ronaldo Krzesinski
2012-08-10 17:24 ` Vivek Goyal
2012-08-09 19:59 ` [PATCH 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling Herton Ronaldo Krzesinski
` (3 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Herton Ronaldo Krzesinski @ 2012-08-09 19:59 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).
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
Cc: stable@vger.kernel.org
---
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] 15+ messages in thread* Re: [PATCH 2/6] floppy: do put_disk on current dr if blk_init_queue fails
2012-08-09 19:59 ` [PATCH 2/6] floppy: do put_disk on current dr if blk_init_queue fails Herton Ronaldo Krzesinski
@ 2012-08-10 17:24 ` Vivek Goyal
0 siblings, 0 replies; 15+ messages in thread
From: Vivek Goyal @ 2012-08-10 17:24 UTC (permalink / raw)
To: Herton Ronaldo Krzesinski
Cc: Jiri Kosina, Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel,
Ben Hutchings
On Thu, Aug 09, 2012 at 04:59:47PM -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).
>
> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
> Cc: stable@vger.kernel.org
Hi,
So for the current drive we do put_disk() here and for rest of the drives
we do it in out_put_disk:.
How about if we go through all the N_DRIVE always and do put disk as need be.
for(i = 0, i < N_DRIVE, i++) {
if (!disks[i])
continue;
if (disks[i]->queue)
blk_cleanup_queue();
if (!disk_registered[i])
disks[i]->queue = NULL;
put_disk();
}
It is little more lines of code but personally I find it easier to understand
and less error prone as future modifications take place.
Thanks
Vivek
> ---
> 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 [flat|nested] 15+ messages in thread
* [PATCH 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling
2012-08-09 19:59 Bug fixes/cleanups for floppy driver (v2) Herton Ronaldo Krzesinski
2012-08-09 19:59 ` [PATCH 1/6] floppy: don't call alloc_ordered_workqueue inside the alloc_disk loop Herton Ronaldo Krzesinski
2012-08-09 19:59 ` [PATCH 2/6] floppy: do put_disk on current dr if blk_init_queue fails Herton Ronaldo Krzesinski
@ 2012-08-09 19:59 ` Herton Ronaldo Krzesinski
2012-08-10 17:25 ` Vivek Goyal
2012-08-09 19:59 ` [PATCH 4/6] floppy: properly handle failure on add_disk loop Herton Ronaldo Krzesinski
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Herton Ronaldo Krzesinski @ 2012-08-09 19:59 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.
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
Cc: stable@vger.kernel.org
---
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] 15+ messages in thread* Re: [PATCH 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling
2012-08-09 19:59 ` [PATCH 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling Herton Ronaldo Krzesinski
@ 2012-08-10 17:25 ` Vivek Goyal
0 siblings, 0 replies; 15+ messages in thread
From: Vivek Goyal @ 2012-08-10 17:25 UTC (permalink / raw)
To: Herton Ronaldo Krzesinski
Cc: Jiri Kosina, Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel,
Ben Hutchings
On Thu, Aug 09, 2012 at 04:59:48PM -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.
>
> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
> Cc: stable@vger.kernel.org
Looks good to me.
Acked-by: Vivek Goyal <vgoyal@redhat.com>
Vivek
> ---
> 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 [flat|nested] 15+ messages in thread
* [PATCH 4/6] floppy: properly handle failure on add_disk loop
2012-08-09 19:59 Bug fixes/cleanups for floppy driver (v2) Herton Ronaldo Krzesinski
` (2 preceding siblings ...)
2012-08-09 19:59 ` [PATCH 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling Herton Ronaldo Krzesinski
@ 2012-08-09 19:59 ` Herton Ronaldo Krzesinski
2012-08-10 17:34 ` Vivek Goyal
2012-08-09 19:59 ` [PATCH 5/6] floppy: remove check for allocated queue on do_floppy_init error handling Herton Ronaldo Krzesinski
2012-08-09 19:59 ` [PATCH 6/6] floppy: use disk_registered for checking if a drive is present Herton Ronaldo Krzesinski
5 siblings, 1 reply; 15+ messages in thread
From: Herton Ronaldo Krzesinski @ 2012-08-09 19:59 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.
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
Cc: stable@vger.kernel.org
---
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] 15+ messages in thread* Re: [PATCH 4/6] floppy: properly handle failure on add_disk loop
2012-08-09 19:59 ` [PATCH 4/6] floppy: properly handle failure on add_disk loop Herton Ronaldo Krzesinski
@ 2012-08-10 17:34 ` Vivek Goyal
0 siblings, 0 replies; 15+ messages in thread
From: Vivek Goyal @ 2012-08-10 17:34 UTC (permalink / raw)
To: Herton Ronaldo Krzesinski
Cc: Jiri Kosina, Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel,
Ben Hutchings
On Thu, Aug 09, 2012 at 04:59:49PM -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.
>
> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
> Cc: stable@vger.kernel.org
> ---
Looks good to me.
Acked-by: Vivek Goyal <vgoyal@redhat.com>
Vivek
> 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 [flat|nested] 15+ messages in thread
* [PATCH 5/6] floppy: remove check for allocated queue on do_floppy_init error handling
2012-08-09 19:59 Bug fixes/cleanups for floppy driver (v2) Herton Ronaldo Krzesinski
` (3 preceding siblings ...)
2012-08-09 19:59 ` [PATCH 4/6] floppy: properly handle failure on add_disk loop Herton Ronaldo Krzesinski
@ 2012-08-09 19:59 ` Herton Ronaldo Krzesinski
2012-08-10 17:36 ` Vivek Goyal
2012-08-09 19:59 ` [PATCH 6/6] floppy: use disk_registered for checking if a drive is present Herton Ronaldo Krzesinski
5 siblings, 1 reply; 15+ messages in thread
From: Herton Ronaldo Krzesinski @ 2012-08-09 19:59 UTC (permalink / raw)
To: Jiri Kosina
Cc: Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel, Vivek Goyal,
Ben Hutchings
The check "if (disks[dr]->queue)" check is bogus, if we reach there
for each dr should exist an queue allocated (note that we decrement dr
first on entering the loop).
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
---
drivers/block/floppy.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 3eafe93..438ffc9 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4332,15 +4332,13 @@ out_unreg_blkdev:
out_put_disk:
while (dr--) {
del_timer_sync(&motor_off_timer[dr]);
- 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.
- */
- if (!disk_registered[dr])
- disks[dr]->queue = NULL;
- }
+ blk_cleanup_queue(disks[dr]->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;
put_disk(disks[dr]);
}
destroy_workqueue(floppy_wq);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 5/6] floppy: remove check for allocated queue on do_floppy_init error handling
2012-08-09 19:59 ` [PATCH 5/6] floppy: remove check for allocated queue on do_floppy_init error handling Herton Ronaldo Krzesinski
@ 2012-08-10 17:36 ` Vivek Goyal
2012-08-13 16:22 ` Herton Ronaldo Krzesinski
0 siblings, 1 reply; 15+ messages in thread
From: Vivek Goyal @ 2012-08-10 17:36 UTC (permalink / raw)
To: Herton Ronaldo Krzesinski
Cc: Jiri Kosina, Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel,
Ben Hutchings
On Thu, Aug 09, 2012 at 04:59:50PM -0300, Herton Ronaldo Krzesinski wrote:
> The check "if (disks[dr]->queue)" check is bogus, if we reach there
> for each dr should exist an queue allocated (note that we decrement dr
> first on entering the loop).
>
> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
As mentioned in second patch, I like going trhough full array of drives
and do cleanup as needed instead of relying on "dr" variable.
But if you don't like that, then I am not as such against this approach.
Was just trying to keep all put_disk() at one place.
Thanks
Vivek
> ---
> drivers/block/floppy.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index 3eafe93..438ffc9 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -4332,15 +4332,13 @@ out_unreg_blkdev:
> out_put_disk:
> while (dr--) {
> del_timer_sync(&motor_off_timer[dr]);
> - 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.
> - */
> - if (!disk_registered[dr])
> - disks[dr]->queue = NULL;
> - }
> + blk_cleanup_queue(disks[dr]->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;
> put_disk(disks[dr]);
> }
> destroy_workqueue(floppy_wq);
> --
> 1.7.9.5
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 5/6] floppy: remove check for allocated queue on do_floppy_init error handling
2012-08-10 17:36 ` Vivek Goyal
@ 2012-08-13 16:22 ` Herton Ronaldo Krzesinski
0 siblings, 0 replies; 15+ messages in thread
From: Herton Ronaldo Krzesinski @ 2012-08-13 16:22 UTC (permalink / raw)
To: Vivek Goyal
Cc: Jiri Kosina, Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel,
Ben Hutchings
On Fri, Aug 10, 2012 at 01:36:53PM -0400, Vivek Goyal wrote:
> On Thu, Aug 09, 2012 at 04:59:50PM -0300, Herton Ronaldo Krzesinski wrote:
> > The check "if (disks[dr]->queue)" check is bogus, if we reach there
> > for each dr should exist an queue allocated (note that we decrement dr
> > first on entering the loop).
> >
> > Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
>
> As mentioned in second patch, I like going trhough full array of drives
> and do cleanup as needed instead of relying on "dr" variable.
>
> But if you don't like that, then I am not as such against this approach.
> Was just trying to keep all put_disk() at one place.
Ok, I'll drop this, and do a full cleanup. I just ended up doing the
minimal work, but I'm not against cleaning this. I still want to keep
the second patch, as being a simple/appropriate bug fix for stable. I'll
submit a new series soon. Thanks for looking at this series.
>
> Thanks
> Vivek
>
--
[]'s
Herton
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 6/6] floppy: use disk_registered for checking if a drive is present
2012-08-09 19:59 Bug fixes/cleanups for floppy driver (v2) Herton Ronaldo Krzesinski
` (4 preceding siblings ...)
2012-08-09 19:59 ` [PATCH 5/6] floppy: remove check for allocated queue on do_floppy_init error handling Herton Ronaldo Krzesinski
@ 2012-08-09 19:59 ` Herton Ronaldo Krzesinski
2012-08-10 17:35 ` Vivek Goyal
5 siblings, 1 reply; 15+ messages in thread
From: Herton Ronaldo Krzesinski @ 2012-08-09 19:59 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.
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 438ffc9..5fcc2a1 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;
@@ -4559,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 (disk_registered[drive]) {
del_gendisk(disks[drive]);
device_remove_file(&floppy_device[drive].dev, &dev_attr_cmos);
platform_device_unregister(&floppy_device[drive]);
@@ -4571,8 +4568,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] 15+ messages in thread* Re: [PATCH 6/6] floppy: use disk_registered for checking if a drive is present
2012-08-09 19:59 ` [PATCH 6/6] floppy: use disk_registered for checking if a drive is present Herton Ronaldo Krzesinski
@ 2012-08-10 17:35 ` Vivek Goyal
0 siblings, 0 replies; 15+ messages in thread
From: Vivek Goyal @ 2012-08-10 17:35 UTC (permalink / raw)
To: Herton Ronaldo Krzesinski
Cc: Jiri Kosina, Andrew Morton, Jens Axboe, Tejun Heo, linux-kernel,
Ben Hutchings
On Thu, Aug 09, 2012 at 04:59:51PM -0300, Herton Ronaldo Krzesinski wrote:
> Simplify/cleanup code, replacing remaining checks for drives present
> using disk_registered array.
>
> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
> ---
Looks good to me.
Acked-by: Vivek Goyal <vgoyal@redhat.com>
Vivek
> 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 438ffc9..5fcc2a1 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;
> @@ -4559,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 (disk_registered[drive]) {
> del_gendisk(disks[drive]);
> device_remove_file(&floppy_device[drive].dev, &dev_attr_cmos);
> platform_device_unregister(&floppy_device[drive]);
> @@ -4571,8 +4568,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 [flat|nested] 15+ messages in thread