This v4 only updates the last 2 patches from my v3 series of stragglers
to account for Christoph's request to split the __register_blkdev()
probe call changes into 3 patches, one for documentation, the other 2
patches for each respective driver.
I also noticed I had a typo on the documentation, so fixed that.
Luis Chamberlain (4):
block: update __register_blkdev() probe documentation
ataflop: address add_disk() error handling on probe
floppy: address add_disk() error handling on probe
block: add __must_check for *add_disk*() callers
block/genhd.c | 11 +++++++----
drivers/block/ataflop.c | 16 +++++++++++++---
drivers/block/floppy.c | 15 +++++++++++++--
include/linux/genhd.h | 6 +++---
4 files changed, 36 insertions(+), 12 deletions(-)
--
2.33.0
We need to cleanup resources on the probe() callback registered
with __register_blkdev(), now that add_disk() error handling is
supported. Address this.
Signed-off-by: Luis Chamberlain <[email protected]>
---
drivers/block/ataflop.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
index 170dd193cef6..e981b351f37e 100644
--- a/drivers/block/ataflop.c
+++ b/drivers/block/ataflop.c
@@ -2012,6 +2012,7 @@ static void ataflop_probe(dev_t dev)
{
int drive = MINOR(dev) & 3;
int type = MINOR(dev) >> 2;
+ int err = 0;
if (type)
type--;
@@ -2019,11 +2020,20 @@ static void ataflop_probe(dev_t dev)
if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS)
return;
if (!unit[drive].disk[type]) {
- if (ataflop_alloc_disk(drive, type) == 0) {
- add_disk(unit[drive].disk[type]);
- unit[drive].registered[type] = true;
+ err = ataflop_alloc_disk(drive, type);
+ if (err == 0) {
+ err = add_disk(unit[drive].disk[type]);
+ if (err)
+ goto err_out;
+ else
+ unit[drive].registered[type] = true;
}
}
+ return;
+
+err_out:
+ blk_cleanup_disk(unit[drive].disk[type]);
+ unit[drive].disk[type] = NULL;
}
static void atari_floppy_cleanup(void)
--
2.33.0
Now that we have done a spring cleaning on all drivers and added
error checking / handling, let's keep it that way and ensure
no new drivers fail to stick with it.
Signed-off-by: Luis Chamberlain <[email protected]>
---
block/genhd.c | 6 +++---
include/linux/genhd.h | 6 +++---
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/block/genhd.c b/block/genhd.c
index 2f5b7e24e88a..2263f7862241 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -394,8 +394,8 @@ static void disk_scan_partitions(struct gendisk *disk)
* This function registers the partitioning information in @disk
* with the kernel.
*/
-int device_add_disk(struct device *parent, struct gendisk *disk,
- const struct attribute_group **groups)
+int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
+ const struct attribute_group **groups)
{
struct device *ddev = disk_to_dev(disk);
@@ -542,7 +542,7 @@ int device_add_disk(struct device *parent, struct gendisk *disk,
out_free_ext_minor:
if (disk->major == BLOCK_EXT_MAJOR)
blk_free_ext_minor(disk->first_minor);
- return WARN_ON_ONCE(ret); /* keep until all callers handle errors */
+ return ret;
}
EXPORT_SYMBOL(device_add_disk);
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 59eabbc3a36b..f7d6810e68b3 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -205,9 +205,9 @@ static inline dev_t disk_devt(struct gendisk *disk)
void disk_uevent(struct gendisk *disk, enum kobject_action action);
/* block/genhd.c */
-int device_add_disk(struct device *parent, struct gendisk *disk,
- const struct attribute_group **groups);
-static inline int add_disk(struct gendisk *disk)
+int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
+ const struct attribute_group **groups);
+static inline int __must_check add_disk(struct gendisk *disk)
{
return device_add_disk(NULL, disk, NULL);
}
--
2.33.0
On Wed, Nov 03, 2021 at 11:12:56AM -0700, Luis Chamberlain wrote:
> We need to cleanup resources on the probe() callback registered
> with __register_blkdev(), now that add_disk() error handling is
> supported. Address this.
>
> Signed-off-by: Luis Chamberlain <[email protected]>
> ---
> drivers/block/ataflop.c | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
> index 170dd193cef6..e981b351f37e 100644
> --- a/drivers/block/ataflop.c
> +++ b/drivers/block/ataflop.c
> @@ -2012,6 +2012,7 @@ static void ataflop_probe(dev_t dev)
> {
> int drive = MINOR(dev) & 3;
> int type = MINOR(dev) >> 2;
> + int err = 0;
>
> if (type)
> type--;
> @@ -2019,11 +2020,20 @@ static void ataflop_probe(dev_t dev)
> if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS)
> return;
> if (!unit[drive].disk[type]) {
> + err = ataflop_alloc_disk(drive, type);
> + if (err == 0) {
> + err = add_disk(unit[drive].disk[type]);
> + if (err)
> + goto err_out;
> + else
> + unit[drive].registered[type] = true;
This looks weird. Why not:
if (unit[drive].disk[type])
return;
if (ataflop_alloc_disk(drive, type))
return;
if (add_disk(unit[drive].disk[type]))
goto cleanup_disk;
unit[drive].registered[type] = true;
return;
cleanup_disk:
blk_cleanup_disk(unit[drive].disk[type]);
unit[drive].disk[type] = NULL;
Or maybe I should stop nitpicking for these cruft drivers.
I'm ok with this and the next patch:
Reviewed-by: Christoph Hellwig <[email protected]>
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
On 11/3/21 12:12 PM, Luis Chamberlain wrote:
> This v4 only updates the last 2 patches from my v3 series of stragglers
> to account for Christoph's request to split the __register_blkdev()
> probe call changes into 3 patches, one for documentation, the other 2
> patches for each respective driver.
Part of the reason why I think this has taken so long is that there's
a hundreds of series, and then you get partial updates, etc. It's really
super hard to keep track of...
Can we please just have one final series, not 1 and then another one
that turns the last two into more?
--
Jens Axboe