On 10/18/21 4:21 PM, 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). When rewriting the drivers' 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.
Was going to ask if this driver was used by anyone, since it's taken 3 years for the breakage to be spotted... In all fairness, it was pretty horribly broken before the change too (like waiting in request_fn, under a lock). So I'm curious, are you actively using it, or was it just an exercise in curiosity?
Testing this change, I've only ever seen single sector requests with the 'last' flag set. If there is a way to send requests to the driver without that flag set, I'd appreciate a hint. As it now stands, the driver won't release the ST-DMA lock on requests that don't have this flag set, but won't accept further requests because the attempt to acquire the already-held lock once more will fail.
'last' is set if it's the last of a sequence of ->queue_rq() calls. If you just do sync IO, then last is always set, as there is no sequence. It's not hard to generate sequences, but on a floppy with basically no queue depth the most you'd ever get is 2. You could try and set: /sys/block/<dev>/queue/max_sectors_kb to 4 for example, and then do something that generates a larger than 4k write or read. Ideally that should give you more than 1. -- Jens Axboe
