| Message ID | 20211019061321.26425-1-schmitzmic@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] block - ataflop.c: fix breakage introduced at blk-mq refactoring | expand |
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,
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, >
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)
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(-)