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
