* [PATCH v2] block - ataflop.c: fix breakage introduced at blk-mq refactoring
@ 2021-10-19 6:13 Michael Schmitz
2021-10-19 12:12 ` Jens Axboe
0 siblings, 1 reply; 3+ messages in thread
From: Michael Schmitz @ 2021-10-19 6:13 UTC (permalink / raw)
To: linux-m68k, geert; +Cc: schmitzmic, linux-block, Tetsuo Handa
Refactoring of the Atari floppy driver when converting to blk-mq
has broken the state machine in not-so-subtle ways:
finish_fdc() must be called when operations on the floppy device
have completed. This is crucial in order to relase the ST-DMA
lock, which protects against concurrent access to the ST-DMA
controller by other drivers (some DMA related, most just related
to device register access - broken beyond compare, I know).
When rewriting the driver's old do_request() function, the fact
that finish_fdc() was called only when all queued requests had
completed appears to have been overlooked. Instead, the new
request function calls finish_fdc() immediately after the last
request has been queued. finish_fdc() executes a dummy seek after
most requests, and this overwrites the state machine's interrupt
hander that was set up to wait for completion of the read/write
request just prior. To make matters worse, finish_fdc() is called
before device interrupts are re-enabled, making certain that the
read/write interupt is missed.
Shifting the finish_fdc() call into the read/write request
completion handler ensures the driver waits for the request to
actually complete. With a queue depth of 2, we won't see long
request sequences, so calling finish_fdc() unconditionally just
adds a little overhead for the dummy seeks, and keeps the code
simple.
While we're at it, kill ataflop_commit_rqs() which does nothing
but run finish_fdc() unconditionally, again likely wiping out an
in-flight request.
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
Fixes: 6ec3938cff95f (ataflop: convert to blk-mq)
CC: linux-block@vger.kernel.org
CC: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
--
Changes from RFC:
Jens Axboe:
- kill ataflop_commit_rqs()
- call finish_fdc() unconditionally in fd_rwsec_done1()
---
drivers/block/ataflop.c | 18 +++---------------
1 file changed, 3 insertions(+), 15 deletions(-)
diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
index 96e2abe34a72..a9d3b24cc8f5 100644
--- a/drivers/block/ataflop.c
+++ b/drivers/block/ataflop.c
@@ -653,9 +653,6 @@ static inline void copy_buffer(void *from, void *to)
*p2++ = *p1++;
}
-
-
-
/* General Interrupt Handling */
static void (*FloppyIRQHandler)( int status ) = NULL;
@@ -1228,6 +1225,7 @@ static void fd_rwsec_done1(int status)
}
else {
/* all sectors finished */
+ finish_fdc();
fd_end_request_cur(BLK_STS_OK);
}
return;
@@ -1475,15 +1473,6 @@ static void setup_req_params( int drive )
ReqTrack, ReqSector, (unsigned long)ReqData ));
}
-static void ataflop_commit_rqs(struct blk_mq_hw_ctx *hctx)
-{
- spin_lock_irq(&ataflop_lock);
- atari_disable_irq(IRQ_MFP_FDC);
- finish_fdc();
- atari_enable_irq(IRQ_MFP_FDC);
- spin_unlock_irq(&ataflop_lock);
-}
-
static blk_status_t ataflop_queue_rq(struct blk_mq_hw_ctx *hctx,
const struct blk_mq_queue_data *bd)
{
@@ -1491,6 +1480,8 @@ static blk_status_t ataflop_queue_rq(struct blk_mq_hw_ctx *hctx,
int drive = floppy - unit;
int type = floppy->type;
+ DPRINT(("Queue request: drive %d type %d last %d\n", drive, type, bd->last));
+
spin_lock_irq(&ataflop_lock);
if (fd_request) {
spin_unlock_irq(&ataflop_lock);
@@ -1550,8 +1541,6 @@ static blk_status_t ataflop_queue_rq(struct blk_mq_hw_ctx *hctx,
setup_req_params( drive );
do_fd_action( drive );
- if (bd->last)
- finish_fdc();
atari_enable_irq( IRQ_MFP_FDC );
out:
@@ -1962,7 +1951,6 @@ static const struct block_device_operations floppy_fops = {
static const struct blk_mq_ops ataflop_mq_ops = {
.queue_rq = ataflop_queue_rq,
- .commit_rqs = ataflop_commit_rqs,
};
static int ataflop_alloc_disk(unsigned int drive, unsigned int type)
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] block - ataflop.c: fix breakage introduced at blk-mq refactoring
2021-10-19 6:13 [PATCH v2] block - ataflop.c: fix breakage introduced at blk-mq refactoring Michael Schmitz
@ 2021-10-19 12:12 ` Jens Axboe
2021-10-19 21:30 ` Michael Schmitz
0 siblings, 1 reply; 3+ messages in thread
From: Jens Axboe @ 2021-10-19 12:12 UTC (permalink / raw)
To: geert, Michael Schmitz, linux-m68k; +Cc: Jens Axboe, Tetsuo Handa, linux-block
On Tue, 19 Oct 2021 19:13:21 +1300, Michael Schmitz wrote:
> Refactoring of the Atari floppy driver when converting to blk-mq
> has broken the state machine in not-so-subtle ways:
>
> finish_fdc() must be called when operations on the floppy device
> have completed. This is crucial in order to relase the ST-DMA
> lock, which protects against concurrent access to the ST-DMA
> controller by other drivers (some DMA related, most just related
> to device register access - broken beyond compare, I know).
>
> [...]
Applied, thanks!
[1/1] block - ataflop.c: fix breakage introduced at blk-mq refactoring
commit: 86d46fdaa12ae5befc16b8d73fc85a3ca0399ea6
Best regards,
--
Jens Axboe
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] block - ataflop.c: fix breakage introduced at blk-mq refactoring
2021-10-19 12:12 ` Jens Axboe
@ 2021-10-19 21:30 ` Michael Schmitz
0 siblings, 0 replies; 3+ messages in thread
From: Michael Schmitz @ 2021-10-19 21:30 UTC (permalink / raw)
To: Jens Axboe, geert, linux-m68k; +Cc: Tetsuo Handa, linux-block
Thanks Jens,
I'll pore over the code some more to see if there are any other problems...
Cheers,
Michael
On 20/10/21 01:12, Jens Axboe wrote:
> On Tue, 19 Oct 2021 19:13:21 +1300, Michael Schmitz wrote:
>> Refactoring of the Atari floppy driver when converting to blk-mq
>> has broken the state machine in not-so-subtle ways:
>>
>> finish_fdc() must be called when operations on the floppy device
>> have completed. This is crucial in order to relase the ST-DMA
>> lock, which protects against concurrent access to the ST-DMA
>> controller by other drivers (some DMA related, most just related
>> to device register access - broken beyond compare, I know).
>>
>> [...]
>
> Applied, thanks!
>
> [1/1] block - ataflop.c: fix breakage introduced at blk-mq refactoring
> commit: 86d46fdaa12ae5befc16b8d73fc85a3ca0399ea6
>
> Best regards,
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-19 21:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-19 6:13 [PATCH v2] block - ataflop.c: fix breakage introduced at blk-mq refactoring Michael Schmitz
2021-10-19 12:12 ` Jens Axboe
2021-10-19 21:30 ` Michael Schmitz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).