Re: [PATCH] fdc: support READ command with VERIFY DMA mode

[John Snow]

On 10/20/19 2:38 AM, Sven Schnelle wrote:
> While working on the Tulip driver i tried to write some Teledisk images to
> a floppy image which didn't work. Turned out that Teledisk checks the written
> data by issuing a READ command to the FDC but running the DMA controller
> in VERIFY mode. As we ignored the DMA request in that case, the DMA transfer
> never finished, and Teledisk reported an error.
> 

CC address@hidden, who sometimes submits patches here too.

> Signed-off-by: Sven Schnelle <address@hidden>
> ---
>  hw/block/fdc.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index ac5d31e8c1..8a1228df78 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -1733,7 +1733,8 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int 
> direction)
>              dma_mode_ok = (dma_mode == ISADMA_TRANSFER_WRITE);
>              break;
>          case FD_DIR_READ:
> -            dma_mode_ok = (dma_mode == ISADMA_TRANSFER_READ);
> +            dma_mode_ok = (dma_mode == ISADMA_TRANSFER_READ) ||
> +                          (dma_mode == ISADMA_TRANSFER_VERIFY);

So we're enabling DMA when the command is an FD_DIR_READ command and the
dma_mode is VERIFY. Those read commands are:

READ
READ TRACK
READ DELETED DATA

>              break;
>          case FD_DIR_VERIFY:
>              dma_mode_ok = true;
> @@ -1835,8 +1836,11 @@ static int fdctrl_transfer_handler (void *opaque, int 
> nchan,
>          switch (fdctrl->data_dir) {
>          case FD_DIR_READ:
>              /* READ commands */
> -            k->write_memory(fdctrl->dma, nchan, fdctrl->fifo + rel_pos,
> -                            fdctrl->data_pos, len);
> +            if (k->get_transfer_mode(fdctrl->dma, fdctrl->dma_chann) !=
> +                ISADMA_TRANSFER_VERIFY) {
> +                k->write_memory(fdctrl->dma, nchan, fdctrl->fifo + rel_pos,
> +                        fdctrl->data_pos, len);
> +            }

Would it horrify you to know I don't know how the VERIFY mode should
work? It's always nice when you google i8257 to look for information and
the top page of results are all QEMU patches.

The i8257 spec says this:

(3) DMA verify, which does not actually involve the
transfer of data. When an 8257 channel is in the DMA verify
mode, it will respond the same as described for transfer
operations, except that no memory or I/O read/write
control signals will be generated,

Alright, looks good to me -- my question is if there aren't other
commands where we want to give this same treatment, but then again...
we've made it to 2019 without them, so...

>              break;
>          case FD_DIR_WRITE:
>              /* WRITE commands */
> 

Reviewed-by: John Snow <address@hidden>