Re: [PATCH RFC] block - ataflop.c: fix breakage introduced at blk-mq refactoring

[Jens Axboe <axboe@kernel.dk>]

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