This is v3 series of the last patch set which should be considered
once nvdimm/blk driver is removed, as Dan Williams noted it would be
gone and once *all* add_disk() error handling patches have been merged.
I rebased Tetsuo Handa's patch onto the latest linux-next as this
series depends on it, and so I am sending it part of this series as
without it, this won't apply. Tetsuo, does the rebase of your patch
look OK?
If it is not too much trouble, I'd like to ask for testing for the
ataflop changes from Michael Schmitz, if possible, that is he'd just
have to merge Tetsuo's rebased patch and the 2nd patch in this series.
This is all rebased on linux-next tag 20211020.
Changes in this v3:
- we don't set ataflop registered to true when we fail, an issue
spotted during review by Tetsuo
- rebased to take into consideration Tetsuo's changes, both his old
and the latest patch carried in this series
- sets the floppy to null on failure from add_disk(), an issue spotted
by Tetsuo during patch review
- removes out label from ataflop as suggested by Finn Thain
- we return the failure from floppy_alloc_disk as suggested by Finn Thain
Luis Chamberlain (2):
block: make __register_blkdev() return an error
block: add __must_check for *add_disk*() callers
Tetsuo Handa (1):
ataflop: remove ataflop_probe_lock mutex
block/bdev.c | 5 +++-
block/genhd.c | 27 +++++++++++------
drivers/block/ataflop.c | 66 +++++++++++++++++++++++++----------------
drivers/block/brd.c | 7 +++--
drivers/block/floppy.c | 17 ++++++++---
drivers/block/loop.c | 11 +++++--
drivers/md/md.c | 12 ++++++--
drivers/scsi/sd.c | 3 +-
include/linux/genhd.h | 10 +++----
9 files changed, 105 insertions(+), 53 deletions(-)
--
2.30.2
From: Tetsuo Handa <[email protected]>
Commit bf9c0538e485b591 ("ataflop: use a separate gendisk for each media
format") introduced ataflop_probe_lock mutex, but forgot to unlock the
mutex when atari_floppy_init() (i.e. module loading) succeeded. This will
result in double lock deadlock if ataflop_probe() is called. Also,
unregister_blkdev() must not be called from atari_floppy_init() with
ataflop_probe_lock held when atari_floppy_init() failed, for
ataflop_probe() waits for ataflop_probe_lock with major_names_lock held
(i.e. AB-BA deadlock).
__register_blkdev() needs to be called last in order to avoid calling
ataflop_probe() when atari_floppy_init() is about to fail, for memory for
completing already-started ataflop_probe() safely will be released as soon
as atari_floppy_init() released ataflop_probe_lock mutex.
As with commit 8b52d8be86d72308 ("loop: reorder loop_exit"),
unregister_blkdev() needs to be called first in order to avoid calling
ataflop_alloc_disk() from ataflop_probe() after del_gendisk() from
atari_floppy_exit().
By relocating __register_blkdev() / unregister_blkdev() as explained above,
we can remove ataflop_probe_lock mutex, for probe function and __exit
function are serialized by major_names_lock mutex.
Signed-off-by: Tetsuo Handa <[email protected]>
Fixes: bf9c0538e485b591 ("ataflop: use a separate gendisk for each media format")
Reviewed-by: Luis Chamberlain <[email protected]>
Tested-by: Michael Schmitz <[email protected]>
---
drivers/block/ataflop.c | 47 +++++++++++++++++++++++------------------
1 file changed, 27 insertions(+), 20 deletions(-)
diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
index 3d217f1d7440..9aecd37ea8a5 100644
--- a/drivers/block/ataflop.c
+++ b/drivers/block/ataflop.c
@@ -1976,8 +1976,6 @@ static int ataflop_alloc_disk(unsigned int drive, unsigned int type)
return 0;
}
-static DEFINE_MUTEX(ataflop_probe_lock);
-
static void ataflop_probe(dev_t dev)
{
int drive = MINOR(dev) & 3;
@@ -1988,14 +1986,32 @@ static void ataflop_probe(dev_t dev)
if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS)
return;
- mutex_lock(&ataflop_probe_lock);
if (!unit[drive].disk[type]) {
if (ataflop_alloc_disk(drive, type) == 0) {
add_disk(unit[drive].disk[type]);
unit[drive].registered[type] = true;
}
}
- mutex_unlock(&ataflop_probe_lock);
+}
+
+static void atari_floppy_cleanup(void)
+{
+ int i;
+ int type;
+
+ for (i = 0; i < FD_MAX_UNITS; i++) {
+ for (type = 0; type < NUM_DISK_MINORS; type++) {
+ 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_mq_free_tag_set(&unit[i].tag_set);
+ }
+
+ del_timer_sync(&fd_timer);
+ atari_stram_free(DMABuffer);
}
static void atari_cleanup_floppy_disk(struct atari_floppy_struct *fs)
@@ -2021,11 +2037,6 @@ static int __init atari_floppy_init (void)
/* Amiga, Mac, ... don't have Atari-compatible floppy :-) */
return -ENODEV;
- mutex_lock(&ataflop_probe_lock);
- ret = __register_blkdev(FLOPPY_MAJOR, "fd", ataflop_probe);
- if (ret)
- goto out_unlock;
-
for (i = 0; i < FD_MAX_UNITS; i++) {
memset(&unit[i].tag_set, 0, sizeof(unit[i].tag_set));
unit[i].tag_set.ops = &ataflop_mq_ops;
@@ -2081,7 +2092,12 @@ static int __init atari_floppy_init (void)
UseTrackbuffer ? "" : "no ");
config_types();
- return 0;
+ ret = __register_blkdev(FLOPPY_MAJOR, "fd", ataflop_probe);
+ if (ret) {
+ printk(KERN_ERR "atari_floppy_init: cannot register block device\n");
+ atari_floppy_cleanup();
+ }
+ return ret;
err_out_dma:
atari_stram_free(DMABuffer);
@@ -2089,9 +2105,6 @@ static int __init atari_floppy_init (void)
while (--i >= 0)
atari_cleanup_floppy_disk(&unit[i]);
- unregister_blkdev(FLOPPY_MAJOR, "fd");
-out_unlock:
- mutex_unlock(&ataflop_probe_lock);
return ret;
}
@@ -2136,14 +2149,8 @@ __setup("floppy=", atari_floppy_setup);
static void __exit atari_floppy_exit(void)
{
- int i;
-
- for (i = 0; i < FD_MAX_UNITS; i++)
- atari_cleanup_floppy_disk(&unit[i]);
unregister_blkdev(FLOPPY_MAJOR, "fd");
-
- del_timer_sync(&fd_timer);
- atari_stram_free( DMABuffer );
+ atari_floppy_cleanup();
}
module_init(atari_floppy_init)
--
2.30.2
This makes __register_blkdev() return an error, and also changes the
probe() call to return an error as well.
We expand documentation for the probe call to ensure that if the block
device already exists we don't return on error on that condition. We do
this as otherwise we loose ability to handle concurrent requests if the
block device already existed.
Cc: Tetsuo Handa <[email protected]>
Signed-off-by: Luis Chamberlain <[email protected]>
---
block/bdev.c | 5 ++++-
block/genhd.c | 21 +++++++++++++++------
drivers/block/ataflop.c | 19 ++++++++++++++-----
drivers/block/brd.c | 7 +++++--
drivers/block/floppy.c | 17 +++++++++++++----
drivers/block/loop.c | 11 ++++++++---
drivers/md/md.c | 12 +++++++++---
drivers/scsi/sd.c | 3 ++-
include/linux/genhd.h | 4 ++--
9 files changed, 72 insertions(+), 27 deletions(-)
diff --git a/block/bdev.c b/block/bdev.c
index cff0bb3a4578..ec6982f7c4a6 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -735,10 +735,13 @@ struct block_device *blkdev_get_no_open(dev_t dev)
{
struct block_device *bdev;
struct inode *inode;
+ int ret;
inode = ilookup(blockdev_superblock, dev);
if (!inode) {
- blk_request_module(dev);
+ ret = blk_request_module(dev);
+ if (ret)
+ return NULL;
inode = ilookup(blockdev_superblock, dev);
if (!inode)
return NULL;
diff --git a/block/genhd.c b/block/genhd.c
index 000e344265ca..404429e6f06c 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -183,7 +183,7 @@ static struct blk_major_name {
struct blk_major_name *next;
int major;
char name[16];
- void (*probe)(dev_t devt);
+ int (*probe)(dev_t devt);
} *major_names[BLKDEV_MAJOR_HASH_SIZE];
static DEFINE_MUTEX(major_names_lock);
static DEFINE_SPINLOCK(major_names_spinlock);
@@ -213,7 +213,13 @@ void blkdev_show(struct seq_file *seqf, off_t offset)
* @major: the requested major device number [1..BLKDEV_MAJOR_MAX-1]. If
* @major = 0, try to allocate any unused major number.
* @name: the name of the new block device as a zero terminated string
- * @probe: allback that is called on access to any minor number of @major
+ * @probe: callback that is called on access to any minor number of @major.
+ * This will return 0 if the block device is already present or was
+ * not present and it succcessfully added a new one. If the block device
+ * was not already present but it was a valid request an error reflecting
+ * the reason why adding the block device is returned. An error should not
+ * be returned if the block device already exists as otherwise concurrent
+ * requests to open an existing block device would fail.
*
* The @name must be unique within the system.
*
@@ -231,7 +237,7 @@ void blkdev_show(struct seq_file *seqf, off_t offset)
* Use register_blkdev instead for any new code.
*/
int __register_blkdev(unsigned int major, const char *name,
- void (*probe)(dev_t devt))
+ int (*probe)(dev_t devt))
{
struct blk_major_name **n, *p;
int index, ret = 0;
@@ -650,17 +656,18 @@ static ssize_t disk_badblocks_store(struct device *dev,
return badblocks_store(disk->bb, page, len, 0);
}
-void blk_request_module(dev_t devt)
+int blk_request_module(dev_t devt)
{
unsigned int major = MAJOR(devt);
struct blk_major_name **n;
+ int err;
mutex_lock(&major_names_lock);
for (n = &major_names[major_to_index(major)]; *n; n = &(*n)->next) {
if ((*n)->major == major && (*n)->probe) {
- (*n)->probe(devt);
+ err = (*n)->probe(devt);
mutex_unlock(&major_names_lock);
- return;
+ return err;
}
}
mutex_unlock(&major_names_lock);
@@ -668,6 +675,8 @@ void blk_request_module(dev_t devt)
if (request_module("block-major-%d-%d", MAJOR(devt), MINOR(devt)) > 0)
/* Make old-style 2.4 aliases work */
request_module("block-major-%d", MAJOR(devt));
+
+ return 0;
}
/*
diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
index 9aecd37ea8a5..c58750dcc685 100644
--- a/drivers/block/ataflop.c
+++ b/drivers/block/ataflop.c
@@ -1976,22 +1976,31 @@ static int ataflop_alloc_disk(unsigned int drive, unsigned int type)
return 0;
}
-static void ataflop_probe(dev_t dev)
+static int ataflop_probe(dev_t dev)
{
int drive = MINOR(dev) & 3;
int type = MINOR(dev) >> 2;
+ int err = 0;
if (type)
type--;
if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS)
- return;
+ return -EINVAL;
+
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) {
+ blk_cleanup_disk(unit[drive].disk[type]);
+ unit[drive].disk[type] = NULL;
+ } else
+ unit[drive].registered[type] = true;
}
}
+
+ return err;
}
static void atari_floppy_cleanup(void)
diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index a896ee175d86..fa6f532035fc 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -437,9 +437,12 @@ static int brd_alloc(int i)
return err;
}
-static void brd_probe(dev_t dev)
+static int brd_probe(dev_t dev)
{
- brd_alloc(MINOR(dev) / max_part);
+ int ret = brd_alloc(MINOR(dev) / max_part);
+ if (ret == -EEXIST)
+ return 0;
+ return ret;
}
static void brd_del_one(struct brd_device *brd)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 3873e789478e..ff3422f517a6 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4518,21 +4518,30 @@ static int floppy_alloc_disk(unsigned int drive, unsigned int type)
static DEFINE_MUTEX(floppy_probe_lock);
-static void floppy_probe(dev_t dev)
+static int floppy_probe(dev_t dev)
{
unsigned int drive = (MINOR(dev) & 3) | ((MINOR(dev) & 0x80) >> 5);
unsigned int type = (MINOR(dev) >> 2) & 0x1f;
+ int err = 0;
if (drive >= N_DRIVE || !floppy_available(drive) ||
type >= ARRAY_SIZE(floppy_type))
- return;
+ return -EINVAL;
mutex_lock(&floppy_probe_lock);
if (!disks[drive][type]) {
- if (floppy_alloc_disk(drive, type) == 0)
- add_disk(disks[drive][type]);
+ err = floppy_alloc_disk(drive, type);
+ if (err == 0) {
+ err = add_disk(disks[drive][type]);
+ if (err) {
+ blk_cleanup_disk(disks[drive][type]);
+ disks[drive][type] = NULL;
+ }
+ }
}
mutex_unlock(&floppy_probe_lock);
+
+ return err;
}
static int __init do_floppy_init(void)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 00ee365ed5e0..5ffb1900baa9 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2433,13 +2433,18 @@ static void loop_remove(struct loop_device *lo)
kfree(lo);
}
-static void loop_probe(dev_t dev)
+static int loop_probe(dev_t dev)
{
int idx = MINOR(dev) >> part_shift;
+ int ret;
if (max_loop && idx >= max_loop)
- return;
- loop_add(idx);
+ return -EINVAL;
+ ret = loop_add(idx);
+ if (ret == -EEXIST)
+ return 0;
+
+ return ret;
}
static int loop_control_remove(int idx)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 5111ed966947..cdfabb90acb5 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5737,12 +5737,18 @@ static int md_alloc(dev_t dev, char *name)
return error;
}
-static void md_probe(dev_t dev)
+static int md_probe(dev_t dev)
{
+ int error = 0;
+
if (MAJOR(dev) == MD_MAJOR && MINOR(dev) >= 512)
- return;
+ return -EINVAL;
if (create_on_open)
- md_alloc(dev, NULL);
+ error = md_alloc(dev, NULL);
+ if (error == -EEXIST)
+ return 0;
+
+ return error;
}
static int add_named_array(const char *val, const struct kernel_param *kp)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 665e79d3f06c..5f5e4a4cbe28 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -632,8 +632,9 @@ static struct scsi_driver sd_template = {
* Don't request a new module, as that could deadlock in multipath
* environment.
*/
-static void sd_default_probe(dev_t devt)
+static int sd_default_probe(dev_t devt)
{
+ return 0;
}
/*
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 8d98ff613aae..f805173de312 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -288,7 +288,7 @@ struct gendisk *__blk_alloc_disk(int node, struct lock_class_key *lkclass);
void blk_cleanup_disk(struct gendisk *disk);
int __register_blkdev(unsigned int major, const char *name,
- void (*probe)(dev_t devt));
+ int (*probe)(dev_t devt));
#define register_blkdev(major, name) \
__register_blkdev(major, name, NULL)
void unregister_blkdev(unsigned int major, const char *name);
@@ -320,7 +320,7 @@ static inline int bd_register_pending_holders(struct gendisk *disk)
dev_t part_devt(struct gendisk *disk, u8 partno);
void inc_diskseq(struct gendisk *disk);
dev_t blk_lookup_devt(const char *name, int partno);
-void blk_request_module(dev_t devt);
+int blk_request_module(dev_t devt);
#ifdef CONFIG_BLOCK
void printk_all_partitions(void);
#else /* CONFIG_BLOCK */
--
2.30.2
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 404429e6f06c..51ceb084135a 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -397,8 +397,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);
@@ -543,7 +543,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 f805173de312..bddcb30d94c1 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.30.2
On 2021/10/22 1:38, Luis Chamberlain wrote:
> I rebased Tetsuo Handa's patch onto the latest linux-next as this
> series depends on it, and so I am sending it part of this series as
> without it, this won't apply. Tetsuo, does the rebase of your patch
> look OK?
OK, though I wanted my fix to be sent to upstream and stable before this series.
>
> If it is not too much trouble, I'd like to ask for testing for the
> ataflop changes from Michael Schmitz, if possible, that is he'd just
> have to merge Tetsuo's rebased patch and the 2nd patch in this series.
> This is all rebased on linux-next tag 20211020.
Yes, please.
After this series, I guess we can remove "bool registered[NUM_DISK_MINORS];" like below
due to (unit[drive].disk[type] != NULL) == (unit[drive].registered[type] == true).
Regarding this series, setting unit[drive].registered[type] = true in ataflop_probe() is
pointless because atari_floppy_cleanup() checks unit[i].disk[type] != NULL for calling
del_gendisk(). And we need to fix __register_blkdev() in driver/block/floppy.c because
floppy_probe_lock is pointless.
drivers/block/ataflop.c | 75 +++++++++++++++--------------------------
1 file changed, 28 insertions(+), 47 deletions(-)
diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
index c58750dcc685..7fedf8506335 100644
--- a/drivers/block/ataflop.c
+++ b/drivers/block/ataflop.c
@@ -299,7 +299,6 @@ 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;
@@ -1988,41 +1987,20 @@ static int ataflop_probe(dev_t dev)
if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS)
return -EINVAL;
- if (!unit[drive].disk[type]) {
- err = ataflop_alloc_disk(drive, type);
- if (err == 0) {
- err = add_disk(unit[drive].disk[type]);
- if (err) {
- blk_cleanup_disk(unit[drive].disk[type]);
- unit[drive].disk[type] = NULL;
- } else
- unit[drive].registered[type] = true;
+ if (unit[drive].disk[type])
+ return 0;
+ err = ataflop_alloc_disk(drive, type);
+ if (err == 0) {
+ err = add_disk(unit[drive].disk[type]);
+ if (err) {
+ blk_cleanup_disk(unit[drive].disk[type]);
+ unit[drive].disk[type] = NULL;
}
}
return err;
}
-static void atari_floppy_cleanup(void)
-{
- int i;
- int type;
-
- for (i = 0; i < FD_MAX_UNITS; i++) {
- for (type = 0; type < NUM_DISK_MINORS; type++) {
- 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_mq_free_tag_set(&unit[i].tag_set);
- }
-
- del_timer_sync(&fd_timer);
- atari_stram_free(DMABuffer);
-}
-
static void atari_cleanup_floppy_disk(struct atari_floppy_struct *fs)
{
int type;
@@ -2030,13 +2008,24 @@ static void atari_cleanup_floppy_disk(struct atari_floppy_struct *fs)
for (type = 0; type < NUM_DISK_MINORS; type++) {
if (!fs->disk[type])
continue;
- if (fs->registered[type])
- del_gendisk(fs->disk[type]);
+ del_gendisk(fs->disk[type]);
blk_cleanup_disk(fs->disk[type]);
}
blk_mq_free_tag_set(&fs->tag_set);
}
+static void atari_floppy_cleanup(void)
+{
+ int i;
+
+ for (i = 0; i < FD_MAX_UNITS; i++)
+ atari_cleanup_floppy_disk(&unit[i]);
+
+ del_timer_sync(&fd_timer);
+ if (DMABuffer)
+ atari_stram_free(DMABuffer);
+}
+
static int __init atari_floppy_init (void)
{
int i;
@@ -2055,13 +2044,10 @@ static int __init atari_floppy_init (void)
unit[i].tag_set.numa_node = NUMA_NO_NODE;
unit[i].tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
ret = blk_mq_alloc_tag_set(&unit[i].tag_set);
- if (ret)
- goto err;
-
- ret = ataflop_alloc_disk(i, 0);
if (ret) {
- blk_mq_free_tag_set(&unit[i].tag_set);
- goto err;
+ while (--i >= 0)
+ blk_mq_free_tag_set(&unit[i].tag_set);
+ return ret;
}
}
@@ -2090,10 +2076,9 @@ static int __init atari_floppy_init (void)
for (i = 0; i < FD_MAX_UNITS; i++) {
unit[i].track = -1;
unit[i].flags = 0;
- ret = add_disk(unit[i].disk[0]);
- if (ret)
- goto err_out_dma;
- unit[i].registered[0] = true;
+ ret = ataflop_probe(MKDEV(0, 1 << 2));
+ if (err)
+ goto err;
}
printk(KERN_INFO "Atari floppy driver: max. %cD, %strack buffering\n",
@@ -2108,12 +2093,8 @@ static int __init atari_floppy_init (void)
}
return ret;
-err_out_dma:
- atari_stram_free(DMABuffer);
err:
- while (--i >= 0)
- atari_cleanup_floppy_disk(&unit[i]);
-
+ atari_floppy_cleanup();
return ret;
}
--
2.18.4
Luis, Tetsuo,
I'll try to test this - still running 5.13 (need the old IDE driver for
now), so not sure this will all apply cleanly.
Cheers,
Michael
On 22/10/21 14:06, Tetsuo Handa wrote:
> On 2021/10/22 1:38, Luis Chamberlain wrote:
>> I rebased Tetsuo Handa's patch onto the latest linux-next as this
>> series depends on it, and so I am sending it part of this series as
>> without it, this won't apply. Tetsuo, does the rebase of your patch
>> look OK?
>
> OK, though I wanted my fix to be sent to upstream and stable before this series.
>
>>
>> If it is not too much trouble, I'd like to ask for testing for the
>> ataflop changes from Michael Schmitz, if possible, that is he'd just
>> have to merge Tetsuo's rebased patch and the 2nd patch in this series.
>> This is all rebased on linux-next tag 20211020.
>
> Yes, please.
>
> After this series, I guess we can remove "bool registered[NUM_DISK_MINORS];" like below
> due to (unit[drive].disk[type] != NULL) == (unit[drive].registered[type] == true).
> Regarding this series, setting unit[drive].registered[type] = true in ataflop_probe() is
> pointless because atari_floppy_cleanup() checks unit[i].disk[type] != NULL for calling
> del_gendisk(). And we need to fix __register_blkdev() in driver/block/floppy.c because
> floppy_probe_lock is pointless.
>
> drivers/block/ataflop.c | 75 +++++++++++++++--------------------------
> 1 file changed, 28 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
> index c58750dcc685..7fedf8506335 100644
> --- a/drivers/block/ataflop.c
> +++ b/drivers/block/ataflop.c
> @@ -299,7 +299,6 @@ 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;
> @@ -1988,41 +1987,20 @@ static int ataflop_probe(dev_t dev)
> if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS)
> return -EINVAL;
>
> - if (!unit[drive].disk[type]) {
> - err = ataflop_alloc_disk(drive, type);
> - if (err == 0) {
> - err = add_disk(unit[drive].disk[type]);
> - if (err) {
> - blk_cleanup_disk(unit[drive].disk[type]);
> - unit[drive].disk[type] = NULL;
> - } else
> - unit[drive].registered[type] = true;
> + if (unit[drive].disk[type])
> + return 0;
> + err = ataflop_alloc_disk(drive, type);
> + if (err == 0) {
> + err = add_disk(unit[drive].disk[type]);
> + if (err) {
> + blk_cleanup_disk(unit[drive].disk[type]);
> + unit[drive].disk[type] = NULL;
> }
> }
>
> return err;
> }
>
> -static void atari_floppy_cleanup(void)
> -{
> - int i;
> - int type;
> -
> - for (i = 0; i < FD_MAX_UNITS; i++) {
> - for (type = 0; type < NUM_DISK_MINORS; type++) {
> - 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_mq_free_tag_set(&unit[i].tag_set);
> - }
> -
> - del_timer_sync(&fd_timer);
> - atari_stram_free(DMABuffer);
> -}
> -
> static void atari_cleanup_floppy_disk(struct atari_floppy_struct *fs)
> {
> int type;
> @@ -2030,13 +2008,24 @@ static void atari_cleanup_floppy_disk(struct atari_floppy_struct *fs)
> for (type = 0; type < NUM_DISK_MINORS; type++) {
> if (!fs->disk[type])
> continue;
> - if (fs->registered[type])
> - del_gendisk(fs->disk[type]);
> + del_gendisk(fs->disk[type]);
> blk_cleanup_disk(fs->disk[type]);
> }
> blk_mq_free_tag_set(&fs->tag_set);
> }
>
> +static void atari_floppy_cleanup(void)
> +{
> + int i;
> +
> + for (i = 0; i < FD_MAX_UNITS; i++)
> + atari_cleanup_floppy_disk(&unit[i]);
> +
> + del_timer_sync(&fd_timer);
> + if (DMABuffer)
> + atari_stram_free(DMABuffer);
> +}
> +
> static int __init atari_floppy_init (void)
> {
> int i;
> @@ -2055,13 +2044,10 @@ static int __init atari_floppy_init (void)
> unit[i].tag_set.numa_node = NUMA_NO_NODE;
> unit[i].tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
> ret = blk_mq_alloc_tag_set(&unit[i].tag_set);
> - if (ret)
> - goto err;
> -
> - ret = ataflop_alloc_disk(i, 0);
> if (ret) {
> - blk_mq_free_tag_set(&unit[i].tag_set);
> - goto err;
> + while (--i >= 0)
> + blk_mq_free_tag_set(&unit[i].tag_set);
> + return ret;
> }
> }
>
> @@ -2090,10 +2076,9 @@ static int __init atari_floppy_init (void)
> for (i = 0; i < FD_MAX_UNITS; i++) {
> unit[i].track = -1;
> unit[i].flags = 0;
> - ret = add_disk(unit[i].disk[0]);
> - if (ret)
> - goto err_out_dma;
> - unit[i].registered[0] = true;
> + ret = ataflop_probe(MKDEV(0, 1 << 2));
> + if (err)
> + goto err;
> }
>
> printk(KERN_INFO "Atari floppy driver: max. %cD, %strack buffering\n",
> @@ -2108,12 +2093,8 @@ static int __init atari_floppy_init (void)
> }
> return ret;
>
> -err_out_dma:
> - atari_stram_free(DMABuffer);
> err:
> - while (--i >= 0)
> - atari_cleanup_floppy_disk(&unit[i]);
> -
> + atari_floppy_cleanup();
> return ret;
> }
>
>
Turns out both patches don't apply cleanly to 5.13 (probably no surprise
to you). I'll update my system to work with the pata IDE driver and
update to the latest version in Geert's m68k tree (meant to do that
anyway...), might take a few days.
Cheers,
Michael
On 22/10/21 15:33, Michael Schmitz wrote:
> Luis, Tetsuo,
>
> I'll try to test this - still running 5.13 (need the old IDE driver for
> now), so not sure this will all apply cleanly.
>
> Cheers,
>
> Michael
>
>
> On 22/10/21 14:06, Tetsuo Handa wrote:
>> On 2021/10/22 1:38, Luis Chamberlain wrote:
>>> I rebased Tetsuo Handa's patch onto the latest linux-next as this
>>> series depends on it, and so I am sending it part of this series as
>>> without it, this won't apply. Tetsuo, does the rebase of your patch
>>> look OK?
>>
>> OK, though I wanted my fix to be sent to upstream and stable before
>> this series.
>>
>>>
>>> If it is not too much trouble, I'd like to ask for testing for the
>>> ataflop changes from Michael Schmitz, if possible, that is he'd just
>>> have to merge Tetsuo's rebased patch and the 2nd patch in this series.
>>> This is all rebased on linux-next tag 20211020.
>>
>> Yes, please.
>>
>> After this series, I guess we can remove "bool
>> registered[NUM_DISK_MINORS];" like below
>> due to (unit[drive].disk[type] != NULL) ==
>> (unit[drive].registered[type] == true).
>> Regarding this series, setting unit[drive].registered[type] = true in
>> ataflop_probe() is
>> pointless because atari_floppy_cleanup() checks unit[i].disk[type] !=
>> NULL for calling
>> del_gendisk(). And we need to fix __register_blkdev() in
>> driver/block/floppy.c because
>> floppy_probe_lock is pointless.
>>
>> drivers/block/ataflop.c | 75 +++++++++++++++--------------------------
>> 1 file changed, 28 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
>> index c58750dcc685..7fedf8506335 100644
>> --- a/drivers/block/ataflop.c
>> +++ b/drivers/block/ataflop.c
>> @@ -299,7 +299,6 @@ 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;
>> @@ -1988,41 +1987,20 @@ static int ataflop_probe(dev_t dev)
>> if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS)
>> return -EINVAL;
>>
>> - if (!unit[drive].disk[type]) {
>> - err = ataflop_alloc_disk(drive, type);
>> - if (err == 0) {
>> - err = add_disk(unit[drive].disk[type]);
>> - if (err) {
>> - blk_cleanup_disk(unit[drive].disk[type]);
>> - unit[drive].disk[type] = NULL;
>> - } else
>> - unit[drive].registered[type] = true;
>> + if (unit[drive].disk[type])
>> + return 0;
>> + err = ataflop_alloc_disk(drive, type);
>> + if (err == 0) {
>> + err = add_disk(unit[drive].disk[type]);
>> + if (err) {
>> + blk_cleanup_disk(unit[drive].disk[type]);
>> + unit[drive].disk[type] = NULL;
>> }
>> }
>>
>> return err;
>> }
>>
>> -static void atari_floppy_cleanup(void)
>> -{
>> - int i;
>> - int type;
>> -
>> - for (i = 0; i < FD_MAX_UNITS; i++) {
>> - for (type = 0; type < NUM_DISK_MINORS; type++) {
>> - 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_mq_free_tag_set(&unit[i].tag_set);
>> - }
>> -
>> - del_timer_sync(&fd_timer);
>> - atari_stram_free(DMABuffer);
>> -}
>> -
>> static void atari_cleanup_floppy_disk(struct atari_floppy_struct *fs)
>> {
>> int type;
>> @@ -2030,13 +2008,24 @@ static void atari_cleanup_floppy_disk(struct
>> atari_floppy_struct *fs)
>> for (type = 0; type < NUM_DISK_MINORS; type++) {
>> if (!fs->disk[type])
>> continue;
>> - if (fs->registered[type])
>> - del_gendisk(fs->disk[type]);
>> + del_gendisk(fs->disk[type]);
>> blk_cleanup_disk(fs->disk[type]);
>> }
>> blk_mq_free_tag_set(&fs->tag_set);
>> }
>>
>> +static void atari_floppy_cleanup(void)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < FD_MAX_UNITS; i++)
>> + atari_cleanup_floppy_disk(&unit[i]);
>> +
>> + del_timer_sync(&fd_timer);
>> + if (DMABuffer)
>> + atari_stram_free(DMABuffer);
>> +}
>> +
>> static int __init atari_floppy_init (void)
>> {
>> int i;
>> @@ -2055,13 +2044,10 @@ static int __init atari_floppy_init (void)
>> unit[i].tag_set.numa_node = NUMA_NO_NODE;
>> unit[i].tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
>> ret = blk_mq_alloc_tag_set(&unit[i].tag_set);
>> - if (ret)
>> - goto err;
>> -
>> - ret = ataflop_alloc_disk(i, 0);
>> if (ret) {
>> - blk_mq_free_tag_set(&unit[i].tag_set);
>> - goto err;
>> + while (--i >= 0)
>> + blk_mq_free_tag_set(&unit[i].tag_set);
>> + return ret;
>> }
>> }
>>
>> @@ -2090,10 +2076,9 @@ static int __init atari_floppy_init (void)
>> for (i = 0; i < FD_MAX_UNITS; i++) {
>> unit[i].track = -1;
>> unit[i].flags = 0;
>> - ret = add_disk(unit[i].disk[0]);
>> - if (ret)
>> - goto err_out_dma;
>> - unit[i].registered[0] = true;
>> + ret = ataflop_probe(MKDEV(0, 1 << 2));
>> + if (err)
>> + goto err;
>> }
>>
>> printk(KERN_INFO "Atari floppy driver: max. %cD, %strack
>> buffering\n",
>> @@ -2108,12 +2093,8 @@ static int __init atari_floppy_init (void)
>> }
>> return ret;
>>
>> -err_out_dma:
>> - atari_stram_free(DMABuffer);
>> err:
>> - while (--i >= 0)
>> - atari_cleanup_floppy_disk(&unit[i]);
>> -
>> + atari_floppy_cleanup();
>> return ret;
>> }
>>
>>
On Fri, Oct 22, 2021 at 10:06:07AM +0900, Tetsuo Handa wrote:
> On 2021/10/22 1:38, Luis Chamberlain wrote:
> > I rebased Tetsuo Handa's patch onto the latest linux-next as this
> > series depends on it, and so I am sending it part of this series as
> > without it, this won't apply. Tetsuo, does the rebase of your patch
> > look OK?
>
> OK, though I wanted my fix to be sent to upstream and stable before this series.
Sure, absolutely, your patch is certainly separate and is needed as a
fix downstream to stable it would seem.
> > If it is not too much trouble, I'd like to ask for testing for the
> > ataflop changes from Michael Schmitz, if possible, that is he'd just
> > have to merge Tetsuo's rebased patch and the 2nd patch in this series.
> > This is all rebased on linux-next tag 20211020.
>
> Yes, please.
>
> After this series, I guess we can remove "bool registered[NUM_DISK_MINORS];" like below
> due to (unit[drive].disk[type] != NULL) == (unit[drive].registered[type] == true).
Sounds good.
> Regarding this series, setting unit[drive].registered[type] = true in ataflop_probe() is
> pointless because atari_floppy_cleanup() checks unit[i].disk[type] != NULL for calling
> del_gendisk().
I see, will fix.
> And we need to fix __register_blkdev() in driver/block/floppy.c because
> floppy_probe_lock is pointless.
Sure, I take it you're going to work on a patch?
So much excitement with the floppy world, who would have thought? :)
Luis
On Fri, Oct 22, 2021 at 10:08:39AM -0700, Luis Chamberlain wrote:
> On Fri, Oct 22, 2021 at 10:06:07AM +0900, Tetsuo Handa wrote:
> > On 2021/10/22 1:38, Luis Chamberlain wrote:
> > > I rebased Tetsuo Handa's patch onto the latest linux-next as this
> > > series depends on it, and so I am sending it part of this series as
> > > without it, this won't apply. Tetsuo, does the rebase of your patch
> > > look OK?
> >
> > OK, though I wanted my fix to be sent to upstream and stable before this series.
>
> Sure, absolutely, your patch is certainly separate and is needed as a
> fix downstream to stable it would seem.
>
> > > If it is not too much trouble, I'd like to ask for testing for the
> > > ataflop changes from Michael Schmitz, if possible, that is he'd just
> > > have to merge Tetsuo's rebased patch and the 2nd patch in this series.
> > > This is all rebased on linux-next tag 20211020.
> >
> > Yes, please.
> >
> > After this series, I guess we can remove "bool registered[NUM_DISK_MINORS];" like below
> > due to (unit[drive].disk[type] != NULL) == (unit[drive].registered[type] == true).
>
> Sounds good.
>
> > Regarding this series, setting unit[drive].registered[type] = true in ataflop_probe() is
> > pointless because atari_floppy_cleanup() checks unit[i].disk[type] != NULL for calling
> > del_gendisk().
>
> I see, will fix.
Actually just not doing it for that case seems odd, so I would prefer
the removal of the bool registered to be a separate patch to make it
clearer.
Luis
Hi Tetsuo,
On 22/10/21 14:06, Tetsuo Handa wrote:
> On 2021/10/22 1:38, Luis Chamberlain wrote:
>> I rebased Tetsuo Handa's patch onto the latest linux-next as this
>> series depends on it, and so I am sending it part of this series as
>> without it, this won't apply. Tetsuo, does the rebase of your patch
>> look OK?
>
> OK, though I wanted my fix to be sent to upstream and stable before this series.
>
>>
>> If it is not too much trouble, I'd like to ask for testing for the
>> ataflop changes from Michael Schmitz, if possible, that is he'd just
>> have to merge Tetsuo's rebased patch and the 2nd patch in this series.
>> This is all rebased on linux-next tag 20211020.
>
> Yes, please.
Took a little convincing (patch 2 didn't apply cleanly by 'git am' on
yesterday's top of linux-next), but works just fine, thanks.
I'll submit another patch with ataflop fixes that were used in my tests,
but nothing in that interacts with your patches at all.
Tested-by: Michael Schmitz <[email protected]>
> After this series, I guess we can remove "bool registered[NUM_DISK_MINORS];" like below
> due to (unit[drive].disk[type] != NULL) == (unit[drive].registered[type] == true).
> Regarding this series, setting unit[drive].registered[type] = true in ataflop_probe() is
> pointless because atari_floppy_cleanup() checks unit[i].disk[type] != NULL for calling
> del_gendisk(). And we need to fix __register_blkdev() in driver/block/floppy.c because
> floppy_probe_lock is pointless.
>
> drivers/block/ataflop.c | 75 +++++++++++++++--------------------------
> 1 file changed, 28 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
> index c58750dcc685..7fedf8506335 100644
> --- a/drivers/block/ataflop.c
> +++ b/drivers/block/ataflop.c
> @@ -299,7 +299,6 @@ 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;
> @@ -1988,41 +1987,20 @@ static int ataflop_probe(dev_t dev)
> if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS)
> return -EINVAL;
>
> - if (!unit[drive].disk[type]) {
> - err = ataflop_alloc_disk(drive, type);
> - if (err == 0) {
> - err = add_disk(unit[drive].disk[type]);
> - if (err) {
> - blk_cleanup_disk(unit[drive].disk[type]);
> - unit[drive].disk[type] = NULL;
> - } else
> - unit[drive].registered[type] = true;
> + if (unit[drive].disk[type])
> + return 0;
> + err = ataflop_alloc_disk(drive, type);
> + if (err == 0) {
> + err = add_disk(unit[drive].disk[type]);
> + if (err) {
> + blk_cleanup_disk(unit[drive].disk[type]);
> + unit[drive].disk[type] = NULL;
> }
> }
>
> return err;
> }
>
> -static void atari_floppy_cleanup(void)
> -{
> - int i;
> - int type;
> -
> - for (i = 0; i < FD_MAX_UNITS; i++) {
> - for (type = 0; type < NUM_DISK_MINORS; type++) {
> - 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_mq_free_tag_set(&unit[i].tag_set);
> - }
> -
> - del_timer_sync(&fd_timer);
> - atari_stram_free(DMABuffer);
> -}
> -
> static void atari_cleanup_floppy_disk(struct atari_floppy_struct *fs)
> {
> int type;
> @@ -2030,13 +2008,24 @@ static void atari_cleanup_floppy_disk(struct atari_floppy_struct *fs)
> for (type = 0; type < NUM_DISK_MINORS; type++) {
> if (!fs->disk[type])
> continue;
> - if (fs->registered[type])
> - del_gendisk(fs->disk[type]);
> + del_gendisk(fs->disk[type]);
> blk_cleanup_disk(fs->disk[type]);
> }
> blk_mq_free_tag_set(&fs->tag_set);
> }
>
> +static void atari_floppy_cleanup(void)
> +{
> + int i;
> +
> + for (i = 0; i < FD_MAX_UNITS; i++)
> + atari_cleanup_floppy_disk(&unit[i]);
> +
> + del_timer_sync(&fd_timer);
> + if (DMABuffer)
> + atari_stram_free(DMABuffer);
> +}
> +
> static int __init atari_floppy_init (void)
> {
> int i;
> @@ -2055,13 +2044,10 @@ static int __init atari_floppy_init (void)
> unit[i].tag_set.numa_node = NUMA_NO_NODE;
> unit[i].tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
> ret = blk_mq_alloc_tag_set(&unit[i].tag_set);
> - if (ret)
> - goto err;
> -
> - ret = ataflop_alloc_disk(i, 0);
> if (ret) {
> - blk_mq_free_tag_set(&unit[i].tag_set);
> - goto err;
> + while (--i >= 0)
> + blk_mq_free_tag_set(&unit[i].tag_set);
> + return ret;
> }
> }
>
> @@ -2090,10 +2076,9 @@ static int __init atari_floppy_init (void)
> for (i = 0; i < FD_MAX_UNITS; i++) {
> unit[i].track = -1;
> unit[i].flags = 0;
> - ret = add_disk(unit[i].disk[0]);
> - if (ret)
> - goto err_out_dma;
> - unit[i].registered[0] = true;
> + ret = ataflop_probe(MKDEV(0, 1 << 2));
> + if (err)
> + goto err;
> }
>
> printk(KERN_INFO "Atari floppy driver: max. %cD, %strack buffering\n",
> @@ -2108,12 +2093,8 @@ static int __init atari_floppy_init (void)
> }
> return ret;
>
> -err_out_dma:
> - atari_stram_free(DMABuffer);
> err:
> - while (--i >= 0)
> - atari_cleanup_floppy_disk(&unit[i]);
> -
> + atari_floppy_cleanup();
> return ret;
> }
>
>