[PATCH] ataflop: unlock ataflop_probe_lock at atari_floppy_init()

[PATCH] ataflop: unlock ataflop_probe_lock at atari_floppy_init()

[Tetsuo Handa]

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. If
ataflop_probe() is called, it will deadlock on ataflop_probe_lock mutex.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Fixes: bf9c0538e485b591 ("ataflop: use a separate gendisk for each media format")
---
To m68k users

  This patch suggests that nobody is testing this module using a real hardware.
  Can somebody test this module?
  Is current m68k hardware still supporting Atari floppy?
  If Atari floppy is no longer supported, do we still need this module?

To Christoph Hellwig and Luis Chamberlain

  If we move __register_blkdev() in atari_floppy_init() to the end of
  atari_floppy_init() and move unregister_blkdev() in atari_floppy_exit() to
  the beginning of atari_floppy_exit(), we can remove unregister_blkdev() from
  atari_floppy_init(), and I think we can also remove ataflop_probe_lock mutex
  because probe function and __exit function are serialized by major_names_lock
  mutex.

 drivers/block/ataflop.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
index a093644ac39f..39b42cb8d173 100644
--- a/drivers/block/ataflop.c
+++ b/drivers/block/ataflop.c
@@ -2072,7 +2072,8 @@ static int __init atari_floppy_init (void)
 	       UseTrackbuffer ? "" : "no ");
 	config_types();
 
-	return 0;
+	ret = 0;
+	goto out_unlock;
 
 err:
 	while (--i >= 0) {
-- 
2.18.4

Re: [PATCH] ataflop: unlock ataflop_probe_lock at atari_floppy_init()

[Finn Thain]

On Sat, 16 Oct 2021, Tetsuo Handa wrote:

> 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. If
> ataflop_probe() is called, it will deadlock on ataflop_probe_lock mutex.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Fixes: bf9c0538e485b591 ("ataflop: use a separate gendisk for each media format")
> ---
> To m68k users
> 
>   This patch suggests that nobody is testing this module using a real hardware.
>   Can somebody test this module?
>   Is current m68k hardware still supporting Atari floppy?
>   If Atari floppy is no longer supported, do we still need this module?
> 

It is only to be expected that no-one would have reported this bug yet.

2 months ago, Debian 11 shipped with a 5.10 kernel, but the bug you found 
first appeared in Linux 5.11.

The existence of buggy drivers in mainline is undesirable but the real 
problem here is the rate at which new bugs get added.

So I wonder if it would have been possible to use Aranym to find the 
regression, or avoid it in the first place?

Re: [PATCH] ataflop: unlock ataflop_probe_lock at atari_floppy_init()

[Michael Schmitz]

Hi Tetsuo,

thank you for fixing this bug!

On 17/10/21 02:25, Tetsuo Handa wrote:
> 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. If
> ataflop_probe() is called, it will deadlock on ataflop_probe_lock mutex.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Fixes: bf9c0538e485b591 ("ataflop: use a separate gendisk for each media format")
> ---
> To m68k users
>
>   This patch suggests that nobody is testing this module using a real hardware.

Not as a module, no. I use the Atari floppy driver built-in. Latest 
kernel version I ran was 5.13.

Relevant kernel log excerpt:

calling  atari_floppy_init+0x0/0x4d4 @ 1
Atari floppy driver: max. HD, track buffering
Probing floppy drive(s):
fd0
initcall atari_floppy_init+0x0/0x4d4 returned 0 after 675082 usecs

Haven't tried to read from the drive in a while though... waiting for 
floppy I/O isn't my favourite spectator sport.

I take it a read attempt should fail, without your patch?

>   Can somebody test this module?

Yes.

>   Is current m68k hardware still supporting Atari floppy?

Yes.

Cheers,

	Michael Schmitz


>   If Atari floppy is no longer supported, do we still need this module?
>
> To Christoph Hellwig and Luis Chamberlain
>
>   If we move __register_blkdev() in atari_floppy_init() to the end of
>   atari_floppy_init() and move unregister_blkdev() in atari_floppy_exit() to
>   the beginning of atari_floppy_exit(), we can remove unregister_blkdev() from
>   atari_floppy_init(), and I think we can also remove ataflop_probe_lock mutex
>   because probe function and __exit function are serialized by major_names_lock
>   mutex.
>
>  drivers/block/ataflop.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
> index a093644ac39f..39b42cb8d173 100644
> --- a/drivers/block/ataflop.c
> +++ b/drivers/block/ataflop.c
> @@ -2072,7 +2072,8 @@ static int __init atari_floppy_init (void)
>  	       UseTrackbuffer ? "" : "no ");
>  	config_types();
>
> -	return 0;
> +	ret = 0;
> +	goto out_unlock;
>
>  err:
>  	while (--i >= 0) {
>

[PATCH v2] ataflop: remove ataflop_probe_lock mutex

[Tetsuo Handa]

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 <penguin-kernel@I-love.SAKURA.ne.jp>
Fixes: bf9c0538e485b591 ("ataflop: use a separate gendisk for each media format")
---
Changes in v2:
  Remove ataflop_probe_lock mutex than unlocking.

Finn Thain wrote:
> So I wonder if it would have been possible to use Aranym to find the 
> regression, or avoid it in the first place?

OK, there is an emulator for testing this module. But I'm not familiar
with m68k environment. Luis Chamberlain is proposing patchset for adding
add_disk() error handling. I think that an answer would be to include
m68k's mailing list into a patch for this module in order to notify of
changes and expect m68k developers to review/test the patch.

Michael Schmitz wrote:
> Not as a module, no. I use the Atari floppy driver built-in. Latest kernel version I ran was 5.13.

Great. Can you try this patch alone?

 drivers/block/ataflop.c | 55 ++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 28 deletions(-)

diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
index a093644ac39f..adfe198e4699 100644
--- a/drivers/block/ataflop.c
+++ b/drivers/block/ataflop.c
@@ -1986,8 +1986,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;
@@ -1998,12 +1996,30 @@ 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]);
 	}
-	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 int __init atari_floppy_init (void)
@@ -2015,11 +2031,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;
@@ -2072,7 +2083,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:
 	while (--i >= 0) {
@@ -2081,9 +2097,6 @@ static int __init atari_floppy_init (void)
 		blk_mq_free_tag_set(&unit[i].tag_set);
 	}
 
-	unregister_blkdev(FLOPPY_MAJOR, "fd");
-out_unlock:
-	mutex_unlock(&ataflop_probe_lock);
 	return ret;
 }
 
@@ -2128,22 +2141,8 @@ __setup("floppy=", atari_floppy_setup);
 
 static void __exit atari_floppy_exit(void)
 {
-	int i, 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);
-	}
 	unregister_blkdev(FLOPPY_MAJOR, "fd");
-
-	del_timer_sync(&fd_timer);
-	atari_stram_free( DMABuffer );
+	atari_floppy_cleanup();
 }
 
 module_init(atari_floppy_init)
-- 
2.18.4

Re: [PATCH v2] ataflop: remove ataflop_probe_lock mutex

[Michael Schmitz]

Hi Tetsuo,

On 17/10/21 15:09, Tetsuo Handa wrote:
> 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 <penguin-kernel@I-love.SAKURA.ne.jp>
> Fixes: bf9c0538e485b591 ("ataflop: use a separate gendisk for each media format")
> ---
> Changes in v2:
>   Remove ataflop_probe_lock mutex than unlocking.
>
> Finn Thain wrote:
>> So I wonder if it would have been possible to use Aranym to find the
>> regression, or avoid it in the first place?
>
> OK, there is an emulator for testing this module. But I'm not familiar
> with m68k environment. Luis Chamberlain is proposing patchset for adding
> add_disk() error handling. I think that an answer would be to include
> m68k's mailing list into a patch for this module in order to notify of
> changes and expect m68k developers to review/test the patch.
>
> Michael Schmitz wrote:
>> Not as a module, no. I use the Atari floppy driver built-in. Latest kernel version I ran was 5.13.
>
> Great. Can you try this patch alone?

Doesn't appear to work, sorry.

Regards,

	Michael Schmitz

>
>  drivers/block/ataflop.c | 55 ++++++++++++++++++++---------------------
>  1 file changed, 27 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
> index a093644ac39f..adfe198e4699 100644
> --- a/drivers/block/ataflop.c
> +++ b/drivers/block/ataflop.c
> @@ -1986,8 +1986,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;
> @@ -1998,12 +1996,30 @@ 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]);
>  	}
> -	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 int __init atari_floppy_init (void)
> @@ -2015,11 +2031,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;
> @@ -2072,7 +2083,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:
>  	while (--i >= 0) {
> @@ -2081,9 +2097,6 @@ static int __init atari_floppy_init (void)
>  		blk_mq_free_tag_set(&unit[i].tag_set);
>  	}
>
> -	unregister_blkdev(FLOPPY_MAJOR, "fd");
> -out_unlock:
> -	mutex_unlock(&ataflop_probe_lock);
>  	return ret;
>  }
>
> @@ -2128,22 +2141,8 @@ __setup("floppy=", atari_floppy_setup);
>
>  static void __exit atari_floppy_exit(void)
>  {
> -	int i, 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);
> -	}
>  	unregister_blkdev(FLOPPY_MAJOR, "fd");
> -
> -	del_timer_sync(&fd_timer);
> -	atari_stram_free( DMABuffer );
> +	atari_floppy_cleanup();
>  }
>
>  module_init(atari_floppy_init)
>

Re: [PATCH v2] ataflop: remove ataflop_probe_lock mutex

[Michael Schmitz]

Hi Tetsuo,

nevermind - stock 5.9 doesn't work either (mount hangs indefinitely).

Might have to do with format autoprobing - I'll try that next.

Cheers,

	Michael


On 18/10/21 08:05, Michael Schmitz wrote:
>> Michael Schmitz wrote:
>>> Not as a module, no. I use the Atari floppy driver built-in. Latest
>>> kernel version I ran was 5.13.
>>
>> Great. Can you try this patch alone?
>
> Doesn't appear to work, sorry.
>
> Regards,
>
>     Michael Schmitz
>
>>
>>  drivers/block/ataflop.c | 55 ++++++++++++++++++++---------------------
>>  1 file changed, 27 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
>> index a093644ac39f..adfe198e4699 100644
>> --- a/drivers/block/ataflop.c
>> +++ b/drivers/block/ataflop.c
>> @@ -1986,8 +1986,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;
>> @@ -1998,12 +1996,30 @@ 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]);
>>      }
>> -    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 int __init atari_floppy_init (void)
>> @@ -2015,11 +2031,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;
>> @@ -2072,7 +2083,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:
>>      while (--i >= 0) {
>> @@ -2081,9 +2097,6 @@ static int __init atari_floppy_init (void)
>>          blk_mq_free_tag_set(&unit[i].tag_set);
>>      }
>>
>> -    unregister_blkdev(FLOPPY_MAJOR, "fd");
>> -out_unlock:
>> -    mutex_unlock(&ataflop_probe_lock);
>>      return ret;
>>  }
>>
>> @@ -2128,22 +2141,8 @@ __setup("floppy=", atari_floppy_setup);
>>
>>  static void __exit atari_floppy_exit(void)
>>  {
>> -    int i, 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);
>> -    }
>>      unregister_blkdev(FLOPPY_MAJOR, "fd");
>> -
>> -    del_timer_sync(&fd_timer);
>> -    atari_stram_free( DMABuffer );
>> +    atari_floppy_cleanup();
>>  }
>>
>>  module_init(atari_floppy_init)
>>

Re: [PATCH v2] ataflop: remove ataflop_probe_lock mutex

[Michael Schmitz]

Hi Tetsuo,

the ataflop driver got broken in commit 6ec3938cff95f (ataflop: convert 
to blk-mq), three years ago. Can't remember seeing that one before.

Looking at a debug log, I get:

[ 7919.150000] fd_open: type=0
[ 7919.180000] Queue request: drive 0 type 0
[ 7919.200000] Request params: Si=0 Tr=0 Se=1 Data=00541000
[ 7919.200000] do_fd_action
[ 7919.200000] fd_rwsec(), Sec=1, Access=r
[ 7919.200000] finish_fdc: dummy seek started
[ 7920.550000] FDC irq, status = a0 handler = 001ea00a
[ 7920.550000] finish_fdc_done entered
[ 7920.550000] finish_fdc() finished

I would have expected an interrupt from fd_rwsec() (which sets 
fd_rwsec_done() as IRQ handler), but calling finish_fdc() from the 
request function while interrupts for the controller are disabled but a 
request is in-flight pretty much ensures we never see fd_rwsec_done()
called:

         ReqCnt = 0;
         ReqCmd = rq_data_dir(fd_request);
         ReqBlock = blk_rq_pos(fd_request);
         ReqBuffer = bio_data(fd_request->bio);
         setup_req_params( drive );
         do_fd_action( drive );

         if (bd->last)
                 finish_fdc();
         atari_enable_irq( IRQ_MFP_FDC );

The old driver used a wait queue to serialize requests, and only called 
finish_fdc() when no further requests were pending. I wonder how to make 
sure finish_fdc() only gets called in the same situation now ... 
bd->last clearly isn't the right hint here.

I suspect this isn't the only thing that broke...

Cheers,

	Michael


On 18/10/21 12:47, Michael Schmitz wrote:
> Hi Tetsuo,
>
> nevermind - stock 5.9 doesn't work either (mount hangs indefinitely).
>
> Might have to do with format autoprobing - I'll try that next.
>
> Cheers,
>
>     Michael
>
>
> On 18/10/21 08:05, Michael Schmitz wrote:
>>> Michael Schmitz wrote:
>>>> Not as a module, no. I use the Atari floppy driver built-in. Latest
>>>> kernel version I ran was 5.13.
>>>
>>> Great. Can you try this patch alone?
>>
>> Doesn't appear to work, sorry.
>>
>> Regards,
>>
>>     Michael Schmitz
>>
>>>
>>>  drivers/block/ataflop.c | 55 ++++++++++++++++++++---------------------
>>>  1 file changed, 27 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
>>> index a093644ac39f..adfe198e4699 100644
>>> --- a/drivers/block/ataflop.c
>>> +++ b/drivers/block/ataflop.c
>>> @@ -1986,8 +1986,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;
>>> @@ -1998,12 +1996,30 @@ 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]);
>>>      }
>>> -    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 int __init atari_floppy_init (void)
>>> @@ -2015,11 +2031,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;
>>> @@ -2072,7 +2083,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:
>>>      while (--i >= 0) {
>>> @@ -2081,9 +2097,6 @@ static int __init atari_floppy_init (void)
>>>          blk_mq_free_tag_set(&unit[i].tag_set);
>>>      }
>>>
>>> -    unregister_blkdev(FLOPPY_MAJOR, "fd");
>>> -out_unlock:
>>> -    mutex_unlock(&ataflop_probe_lock);
>>>      return ret;
>>>  }
>>>
>>> @@ -2128,22 +2141,8 @@ __setup("floppy=", atari_floppy_setup);
>>>
>>>  static void __exit atari_floppy_exit(void)
>>>  {
>>> -    int i, 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);
>>> -    }
>>>      unregister_blkdev(FLOPPY_MAJOR, "fd");
>>> -
>>> -    del_timer_sync(&fd_timer);
>>> -    atari_stram_free( DMABuffer );
>>> +    atari_floppy_cleanup();
>>>  }
>>>
>>>  module_init(atari_floppy_init)
>>>

Re: [PATCH v2] ataflop: remove ataflop_probe_lock mutex

[Michael Schmitz]

Hi Tetsu,

On 17/10/21 15:09, Tetsuo Handa wrote:
> 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 <penguin-kernel@I-love.SAKURA.ne.jp>
> Fixes: bf9c0538e485b591 ("ataflop: use a separate gendisk for each media format")
> ---
> Changes in v2:
>   Remove ataflop_probe_lock mutex than unlocking.
>
> Finn Thain wrote:
>> So I wonder if it would have been possible to use Aranym to find the
>> regression, or avoid it in the first place?
>
> OK, there is an emulator for testing this module. But I'm not familiar
> with m68k environment. Luis Chamberlain is proposing patchset for adding
> add_disk() error handling. I think that an answer would be to include
> m68k's mailing list into a patch for this module in order to notify of
> changes and expect m68k developers to review/test the patch.
>
> Michael Schmitz wrote:
>> Not as a module, no. I use the Atari floppy driver built-in. Latest kernel version I ran was 5.13.
>
> Great. Can you try this patch alone?

Works, after fixing the ataflop_queue_rq() breakage (separate patch sent).

Tested-by: Michael Schmitz <schmitzmic@gmail.com>

Thanks,

	Michael


>
>  drivers/block/ataflop.c | 55 ++++++++++++++++++++---------------------
>  1 file changed, 27 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
> index a093644ac39f..adfe198e4699 100644
> --- a/drivers/block/ataflop.c
> +++ b/drivers/block/ataflop.c
> @@ -1986,8 +1986,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;
> @@ -1998,12 +1996,30 @@ 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]);
>  	}
> -	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 int __init atari_floppy_init (void)
> @@ -2015,11 +2031,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;
> @@ -2072,7 +2083,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:
>  	while (--i >= 0) {
> @@ -2081,9 +2097,6 @@ static int __init atari_floppy_init (void)
>  		blk_mq_free_tag_set(&unit[i].tag_set);
>  	}
>
> -	unregister_blkdev(FLOPPY_MAJOR, "fd");
> -out_unlock:
> -	mutex_unlock(&ataflop_probe_lock);
>  	return ret;
>  }
>
> @@ -2128,22 +2141,8 @@ __setup("floppy=", atari_floppy_setup);
>
>  static void __exit atari_floppy_exit(void)
>  {
> -	int i, 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);
> -	}
>  	unregister_blkdev(FLOPPY_MAJOR, "fd");
> -
> -	del_timer_sync(&fd_timer);
> -	atari_stram_free( DMABuffer );
> +	atari_floppy_cleanup();
>  }
>
>  module_init(atari_floppy_init)
>

Re: [PATCH v2] ataflop: remove ataflop_probe_lock mutex

[Luis Chamberlain]

On Sun, Oct 17, 2021 at 11:09:47AM +0900, Tetsuo Handa wrote:
> 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 <penguin-kernel@I-love.SAKURA.ne.jp>
> Fixes: bf9c0538e485b591 ("ataflop: use a separate gendisk for each media format")

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH v2] ataflop: remove ataflop_probe_lock mutex

[Luis Chamberlain]

On Thu, Oct 21, 2021 at 09:20:06AM -0700, Luis Chamberlain wrote:
> On Sun, Oct 17, 2021 at 11:09:47AM +0900, Tetsuo Handa wrote:
> > 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 <penguin-kernel@I-love.SAKURA.ne.jp>
> > Fixes: bf9c0538e485b591 ("ataflop: use a separate gendisk for each media format")
> 
> Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

I should note though that this does not apply to linux-next, I rebased
it and will send a rebased version on my v3 series of my last patch set
for add_disk error handling work().

  Luis