block: 6th batch of add_disk() error handling conversions
Subsystems (2)
cover block: 6th batch of add_disk() error handling conversions
This is the 6th of 7 batch of driver conversions over to use the new add_disk() error handling. This series addresses the wonderful and extremely exciting world of floppy drivers. You can find the full set of my patches on my 20210901-for-axboe-add-disk-error-handling branch [0]. This is based on axboe/master.
Luis Chamberlain (14):
block/swim3: add error handling support for add_disk()
floppy: fix add_disk() assumption on exit due to new developments
floppy: use blk_cleanup_disk()
floppy: fix calling platform_device_unregister() on invalid drives
floppy: add error handling support for add_disk()
amiflop: add error handling support for add_disk()
swim: simplify using blk_cleanup_disk() on swim_remove()
swim: add helper for disk cleanup
swim: add a floppy registration bool which triggers del_gendisk()
swim: add error handling support for add_disk() block/ataflop: use the blk_cleanup_disk() helper block/ataflop: add registration bool before calling del_gendisk() block/ataflop: provide a helper for cleanup up an atari disk block/ataflop add error handling support for add_disk()
drivers/block/amiflop.c | 7 ++++-- drivers/block/ataflop.c | 47 +++++++++++++++++++++++++---------------- drivers/block/floppy.c | 34 +++++++++++------------------ drivers/block/swim.c | 35 ++++++++++++++++++------------ drivers/block/swim3.c | 4 +++- 5 files changed, 71 insertions(+), 56 deletions(-)
Signature · 1 line
2.30.2
PATCH 1/14 block/swim3: add error handling support for add_disk()
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.
--- drivers/block/swim3.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/block/swim3.c +3 -1
@@ -1229,7 +1229,9 @@ static int swim3_attach(struct macio_dev *mdev, disk->flags |=GENHD_FL_REMOVABLE ;sprintf (disk->disk_name,"fd%d" , floppy_count);set_capacity (disk,2880 ); -add_disk (disk); + rc =add_disk (disk); +if (rc) + goto out_cleanup_disk; disks[floppy_count++] = disk;return 0 ;
Signature · 1 line
2.30.2
PATCH 2/14 floppy: fix add_disk() assumption on exit due to new developments
In reply to Luis ChamberlainAfter the patch titled "floppy: use blk_mq_alloc_disk and blk_cleanup_disk" the floppy driver was modified to allocate the blk_mq_alloc_disk() which allocates the disk with the queue. This is further clarified later with the patch titled "block: remove alloc_disk and alloc_disk_node". This clarifies that:
Most drivers should use and have been converted to use blk_alloc_disk and blk_mq_alloc_disk. Only the scsi ULPs and dasd still allocate a disk separately from the request_queue so don't bother with convenience macros for something that should not see significant new users and remove these wrappers.
And then we have the patch titled, "block: hold a request_queue reference for the lifetime of struct gendisk" which ensures that a queue is *always* present for sure during the entire lifetime of a disk.
In the floppy driver's case then the disk always comes with the queue. So even if even if the queue was cleaned up on exit, putting the disk *is* still required, and likewise, blk_cleanup_queue() on a null queue should not happen now as disk->queue is valid from disk allocation time on.
Automatic backport code scrapers should hopefully not cherry pick this patch as a stable fix candidate without full due dilligence to ensure all the work done on the block layer to make this happen is merged first.
--- drivers/block/floppy.c | 13 ------------- 1 file changed, 13 deletions(-)
drivers/block/floppy.c +0 -13
@@ -4953,19 +4953,6 @@ static void __exit floppy_module_exit(void)blk_cleanup_queue (disks[drive][i]->queue); }blk_mq_free_tag_set (&tag_sets[drive]); - -/* -* 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 ) { -for (i =0 ; i <ARRAY_SIZE (floppy_type); i++) { -if (disks[drive][i]) - disks[drive][i]->queue =NULL ; - } - } -for (i =0 ; i <ARRAY_SIZE (floppy_type); i++) {if (disks[drive][i])put_disk (disks[drive][i]);
Signature · 1 line
2.30.2
PATCH 3/14 floppy: use blk_cleanup_disk()
In reply to Luis ChamberlainUse the blk_cleanup_queue() followed by put_disk() can be replaced with blk_cleanup_disk(). No need for two separate loops.
--- drivers/block/floppy.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
drivers/block/floppy.c +1 -5
@@ -4950,13 +4950,9 @@ static void __exit floppy_module_exit(void) }for (i =0 ; i <ARRAY_SIZE (floppy_type); i++) {if (disks[drive][i]) -blk_cleanup_queue (disks[drive][i]->queue); +blk_cleanup_disk (disks[drive][i]); }blk_mq_free_tag_set (&tag_sets[drive]); -for (i =0 ; i <ARRAY_SIZE (floppy_type); i++) { -if (disks[drive][i]) -put_disk (disks[drive][i]); - } }cancel_delayed_work_sync (&fd_timeout);
Signature · 1 line
2.30.2
PATCH 4/14 floppy: fix calling platform_device_unregister() on invalid drives
In reply to Luis Chamberlainplatform_device_unregister() should only be called when a respective platform_device_register() is called. However the floppy driver currently allows failures when registring a drive and a bail out could easily cause an invalid call to platform_device_unregister() where it was not intended.
Fix this by adding a bool to keep track of when the platform device was registered for a drive.
This does not fix any known panic / bug. This issue was found through code inspection while preparing the driver to use the up and coming support for device_add_disk() error handling. From what I can tell from code inspection, chances of this ever happening should be insanely small, perhaps OOM.
--- drivers/block/floppy.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
drivers/block/floppy.c +7 -2
@@ -4478,6 +4478,7 @@ static const struct blk_mq_ops floppy_mq_ops = { };static struct platform_device floppy_device[N_DRIVE ]; +static bool registered[N_DRIVE ];static bool floppy_available (int drive) {
@@ -4693,6 +4694,8 @@ static int __init do_floppy_init(void)if (err) goto out_remove_drives; + registered[drive] = true; +device_add_disk (&floppy_device[drive].dev, disks[drive][0 ],NULL ); }
@@ -4703,7 +4706,8 @@ static int __init do_floppy_init(void)while (drive--) {if (floppy_available (drive)) {del_gendisk (disks[drive][0 ]); -platform_device_unregister (&floppy_device[drive]); +if (registered[drive]) +platform_device_unregister (&floppy_device[drive]); } } out_release_dma:
@@ -4946,7 +4950,8 @@ static void __exit floppy_module_exit(void)if (disks[drive][i])del_gendisk (disks[drive][i]); } -platform_device_unregister (&floppy_device[drive]); +if (registered[drive]) +platform_device_unregister (&floppy_device[drive]); }for (i =0 ; i <ARRAY_SIZE (floppy_type); i++) {if (disks[drive][i])
Signature · 1 line
2.30.2
PATCH 5/14 floppy: add error handling support for add_disk()
In reply to Luis ChamberlainWe never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.
--- drivers/block/floppy.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/block/floppy.c +4 -2
@@ -4696,8 +4696,10 @@ static int __init do_floppy_init(void) registered[drive] = true; -device_add_disk (&floppy_device[drive].dev, disks[drive][0 ], -NULL ); + err =device_add_disk (&floppy_device[drive].dev, + disks[drive][0 ],NULL ); +if (err) + goto out_remove_drives; }return 0 ;
Signature · 1 line
2.30.2
PATCH 6/14 amiflop: add error handling support for add_disk()
In reply to Luis ChamberlainWe never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. The caller for fd_alloc_disk() deals with the rest of the cleanup like the tag.
--- drivers/block/amiflop.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/block/amiflop.c +5 -2
@@ -1780,6 +1780,7 @@ static const struct blk_mq_ops amiflop_mq_ops = {static int fd_alloc_disk (int drive,int system) {struct gendisk *disk; +int err; disk =blk_mq_alloc_disk (&unit[drive].tag_set,NULL );if (IS_ERR (disk))
@@ -1798,8 +1799,10 @@ static int fd_alloc_disk(int drive, int system)set_capacity (disk,880 *2 ); unit[drive].gendisk[system] = disk; -add_disk (disk); -return 0 ; + err =add_disk (disk); +if (err) +blk_cleanup_disk (disk); +return err; }static int fd_alloc_drive (int drive)
Signature · 1 line
2.30.2
PATCH 7/14 swim: simplify using blk_cleanup_disk() on swim_remove()
In reply to Luis ChamberlainWe can simplify swim_remove() by using one call instead of two, just as other drivers do. Use that pattern.
--- drivers/block/swim.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/block/swim.c +1 -2
@@ -911,9 +911,8 @@ static int swim_remove(struct platform_device *dev)for (drive =0 ; drive < swd->floppy_count; drive++) {del_gendisk (swd->unit[drive].disk); -blk_cleanup_queue (swd->unit[drive].disk->queue); +blk_cleanup_disk (swd->unit[drive].disk);blk_mq_free_tag_set (&swd->unit[drive].tag_set); -put_disk (swd->unit[drive].disk); }unregister_blkdev (FLOPPY_MAJOR ,"fd" );
Signature · 1 line
2.30.2
PATCH 8/14 swim: add helper for disk cleanup
In reply to Luis ChamberlainDisk cleanup can be shared between exit and bringup. Use a helper to do the work required. The only functional change at this point is we're being overly paraoid on exit to check for a null disk as well now, and this should be safe.
We'll later expand on this, this change just makes subsequent changes easier to read.
--- drivers/block/swim.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)
drivers/block/swim.c +13 -8
@@ -771,6 +771,17 @@ static const struct blk_mq_ops swim_mq_ops = { .queue_rq = swim_queue_rq, }; +static void swim_cleanup_floppy_disk (struct floppy_state *fs) +{ +struct gendisk *disk = fs->disk; + +if (!disk) +return ; + +blk_cleanup_disk (disk); +blk_mq_free_tag_set (&fs->tag_set); +} +static int swim_floppy_init (struct swim_priv *swd) {int err;
@@ -835,12 +846,7 @@ static int swim_floppy_init(struct swim_priv *swd) exit_put_disks:unregister_blkdev (FLOPPY_MAJOR ,"fd" );do { -struct gendisk *disk = swd->unit[drive].disk; - -if (!disk) -continue ; -blk_cleanup_disk (disk); -blk_mq_free_tag_set (&swd->unit[drive].tag_set); +swim_cleanup_floppy_disk (&swd->unit[drive]); }while (drive--);return err; }
@@ -911,8 +917,7 @@ static int swim_remove(struct platform_device *dev)for (drive =0 ; drive < swd->floppy_count; drive++) {del_gendisk (swd->unit[drive].disk); -blk_cleanup_disk (swd->unit[drive].disk); -blk_mq_free_tag_set (&swd->unit[drive].tag_set); +swim_cleanup_floppy_disk (&swd->unit[drive]); }unregister_blkdev (FLOPPY_MAJOR ,"fd" );
Signature · 1 line
2.30.2
PATCH 9/14 swim: add a floppy registration bool which triggers del_gendisk()
In reply to Luis ChamberlainInstead of calling del_gendisk() on exit alone, let's add a registration bool to the floppy disk state, this way this can be done on the shared caller, swim_cleanup_floppy_disk().
This will be more useful in subsequent patches. Right now, this just shuffles functionality out to a helper in a safe way.
--- drivers/block/swim.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/block/swim.c +6 -3
@@ -184,6 +184,7 @@ struct floppy_state {int track;int ref_count; +bool registered;struct gendisk *disk;struct blk_mq_tag_set tag_set;
@@ -778,6 +779,9 @@ static void swim_cleanup_floppy_disk(struct floppy_state *fs)if (!disk)return ; +if (fs->registered) +del_gendisk (fs->disk); +blk_cleanup_disk (disk);blk_mq_free_tag_set (&fs->tag_set); }
@@ -839,6 +843,7 @@ static int swim_floppy_init(struct swim_priv *swd) swd->unit[drive].disk->private_data = &swd->unit[drive];set_capacity (swd->unit[drive].disk,2880 );add_disk (swd->unit[drive].disk); + swd->unit[drive].registered = true; }return 0 ;
@@ -915,10 +920,8 @@ static int swim_remove(struct platform_device *dev)int drive;struct resource *res; -for (drive =0 ; drive < swd->floppy_count; drive++) { -del_gendisk (swd->unit[drive].disk); +for (drive =0 ; drive < swd->floppy_count; drive++)swim_cleanup_floppy_disk (&swd->unit[drive]); - }unregister_blkdev (FLOPPY_MAJOR ,"fd" );
Signature · 1 line
2.30.2
PATCH 10/14 swim: add error handling support for add_disk()
In reply to Luis ChamberlainWe never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.
Since we have a caller to do our unwinding for the disk, and this is already dealt with safely we can re-use our existing error path goto label which already deals with the cleanup.
--- drivers/block/swim.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/block/swim.c +3 -1
@@ -842,7 +842,9 @@ static int swim_floppy_init(struct swim_priv *swd) swd->unit[drive].disk->events =DISK_EVENT_MEDIA_CHANGE ; swd->unit[drive].disk->private_data = &swd->unit[drive];set_capacity (swd->unit[drive].disk,2880 ); -add_disk (swd->unit[drive].disk); + err =add_disk (swd->unit[drive].disk); +if (err) + goto exit_put_disks; swd->unit[drive].registered = true; }
Signature · 1 line
2.30.2
PATCH 11/14 block/ataflop: use the blk_cleanup_disk() helper
In reply to Luis ChamberlainUse the helper to replace two lines with one.
--- drivers/block/ataflop.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/block/ataflop.c +2 -4
@@ -2076,8 +2076,7 @@ static int __init atari_floppy_init (void) err:while (--i >=0 ) { -blk_cleanup_queue (unit[i].disk[0 ]->queue); -put_disk (unit[i].disk[0 ]); +blk_cleanup_disk (unit[i].disk[0 ]);blk_mq_free_tag_set (&unit[i].tag_set); }
@@ -2135,8 +2134,7 @@ static void __exit atari_floppy_exit(void)if (!unit[i].disk[type])continue ;del_gendisk (unit[i].disk[type]); -blk_cleanup_queue (unit[i].disk[type]->queue); -put_disk (unit[i].disk[type]); +blk_cleanup_disk (unit[i].disk[type]); }blk_mq_free_tag_set (&unit[i].tag_set); }
Signature · 1 line
2.30.2
PATCH 12/14 block/ataflop: add registration bool before calling del_gendisk()
In reply to Luis ChamberlainThe ataflop assumes del_gendisk() is safe to call, this is only true because add_disk() does not return a failure, but that will change soon. And so, before we get to adding error handling for that case, let's make sure we keep track of which disks actually get registered. Then we use this to only call del_gendisk for them.
--- drivers/block/ataflop.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
drivers/block/ataflop.c +7 -2
@@ -298,6 +298,7 @@ static struct atari_floppy_struct {disk change detection) */ int flags;/* flags */ struct gendisk *disk[NUM_DISK_MINORS ]; +bool registered[NUM_DISK_MINORS ];int ref;int type;struct blk_mq_tag_set tag_set;
@@ -2000,8 +2001,10 @@ static void ataflop_probe(dev_t dev)return ;mutex_lock (&ataflop_probe_lock);if (!unit[drive].disk[type]) { -if (ataflop_alloc_disk (drive, type) ==0 ) +if (ataflop_alloc_disk (drive, type) ==0 ) {add_disk (unit[drive].disk[type]); + unit[drive].registered[type] = true; + } }mutex_unlock (&ataflop_probe_lock); }
@@ -2065,6 +2068,7 @@ static int __init atari_floppy_init (void) unit[i].track =-1 ; unit[i].flags =0 ;add_disk (unit[i].disk[0 ]); + unit[i].registered[0 ] = true; } printk(KERN_INFO "Atari floppy driver: max. %cD, %strack buffering\n",
@@ -2133,7 +2137,8 @@ static void __exit atari_floppy_exit(void)for (type =0 ; type <NUM_DISK_MINORS ; type++) {if (!unit[i].disk[type])continue ; -del_gendisk (unit[i].disk[type]); +if (unit[i].registered[type]) +del_gendisk (unit[i].disk[type]);blk_cleanup_disk (unit[i].disk[type]); }blk_mq_free_tag_set (&unit[i].tag_set);
Signature · 1 line
2.30.2
PATCH 13/14 block/ataflop: provide a helper for cleanup up an atari disk
In reply to Luis ChamberlainInstead of using two separate code paths for cleaning up an atari disk, use one. We take the more careful approach to check for *all* disk types, as is done on exit. The init path didn't have that check as the alternative disk types are only probed for later, they are not initialized by default.
Yes, there is a shared tag for all disks.
--- drivers/block/ataflop.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-)
drivers/block/ataflop.c +19 -15
@@ -2009,6 +2009,20 @@ static void ataflop_probe(dev_t dev)mutex_unlock (&ataflop_probe_lock); } +static void atari_cleanup_floppy_disk (struct atari_floppy_struct *fs) +{ +int type; + +for (type =0 ; type <NUM_DISK_MINORS ; type++) { +if (!fs->disk[type]) +continue ; +if (fs->registered[type]) +del_gendisk (fs->disk[type]); +blk_cleanup_disk (fs->disk[type]); + } +blk_mq_free_tag_set (&fs->tag_set); +} +static int __init atari_floppy_init (void ) {int i;
@@ -2079,10 +2093,8 @@ static int __init atari_floppy_init (void)return 0 ; err: -while (--i >=0 ) { -blk_cleanup_disk (unit[i].disk[0 ]); -blk_mq_free_tag_set (&unit[i].tag_set); - } +while (--i >=0 ) +atari_cleanup_floppy_disk (&unit[i]);unregister_blkdev (FLOPPY_MAJOR ,"fd" ); out_unlock:
@@ -2131,18 +2143,10 @@ __setup("floppy=", atari_floppy_setup); static void __exit atari_floppy_exit(void) { -int i, type; +int i; -for (i =0 ; i <FD_MAX_UNITS ; i++) { -for (type =0 ; type <NUM_DISK_MINORS ; type++) { -if (!unit[i].disk[type]) -continue ; -if (unit[i].registered[type]) -del_gendisk (unit[i].disk[type]); -blk_cleanup_disk (unit[i].disk[type]); - } -blk_mq_free_tag_set (&unit[i].tag_set); - } +for (i =0 ; i <FD_MAX_UNITS ; i++) +atari_cleanup_floppy_disk (&unit[i]);unregister_blkdev (FLOPPY_MAJOR ,"fd" );del_timer_sync (&fd_timer);
Signature · 1 line
2.30.2
PATCH 14/14 block/ataflop add error handling support for add_disk()
In reply to Luis ChamberlainWe never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.
--- drivers/block/ataflop.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/block/ataflop.c +5 -1
@@ -2081,7 +2081,9 @@ static int __init atari_floppy_init (void)for (i =0 ; i <FD_MAX_UNITS ; i++) { unit[i].track =-1 ; unit[i].flags =0 ; -add_disk (unit[i].disk[0 ]); + ret =add_disk (unit[i].disk[0 ]); +if (ret) + goto err_out_dma; unit[i].registered[0 ] = true; }
@@ -2092,6 +2094,8 @@ static int __init atari_floppy_init (void)return 0 ; +err_out_dma: +atari_stram_free (DMABuffer); err:while (--i >=0 )atari_cleanup_floppy_disk (&unit[i]);
Signature · 1 line
2.30.2