Re: [Qemu-devel] [PATCH 6/8] fdc: Disentangle phases in fdctrl_read_data()

[John Snow <jsnow@redhat.com>]

On 05/19/2015 11:36 AM, Kevin Wolf wrote:
> This commit makes similar improvements as have already been made to the
> write function: Instead of relying on a flag in the MSR to distinguish
> controller phases, use the explicit phase that we store now. Assertions
> of the right MSR flags are added.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/block/fdc.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index cbf7abf..8d322e0 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -1533,9 +1533,16 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>          FLOPPY_DPRINTF("error: controller not ready for reading\n");
>          return 0;
>      }
> +
> +    /* If data_len spans multiple sectors, the current position in the FIFO
> +     * wraps around while fdctrl->data_pos is the real position in the whole
> +     * request. */
>      pos = fdctrl->data_pos;
>      pos %= FD_SECTOR_LEN;
> -    if (fdctrl->msr & FD_MSR_NONDMA) {
> +
> +    switch (fdctrl->phase) {
> +    case FD_PHASE_EXECUTION:
> +        assert(fdctrl->msr & FD_MSR_NONDMA);
>          if (pos == 0) {
>              if (fdctrl->data_pos != 0)
>                  if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
> @@ -1551,20 +1558,26 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>                  memset(fdctrl->fifo, 0, FD_SECTOR_LEN);
>              }
>          }
> -    }
> -    retval = fdctrl->fifo[pos];
> -    if (++fdctrl->data_pos == fdctrl->data_len) {
> -        fdctrl->data_pos = 0;

I suppose data_pos is now reset by either stop_transfer (via
to_result_phase) or to_command_phase, so this is OK.

> -        /* Switch from transfer mode to status mode
> -         * then from status mode to command mode
> -         */
> -        if (fdctrl->msr & FD_MSR_NONDMA) {
> +
> +        if (++fdctrl->data_pos == fdctrl->data_len) {
>              fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
> -        } else {
> +        }
> +        break;
> +
> +    case FD_PHASE_RESULT:
> +        assert(!(fdctrl->msr & FD_MSR_NONDMA));
> +        if (++fdctrl->data_pos == fdctrl->data_len) {

Not a terribly big fan of moving this pointer independently inside of
each case statement, but I guess the alternative does look a lot worse.
Having things separated by phases is a lot easier to follow.

>              fdctrl_to_command_phase(fdctrl);
>              fdctrl_reset_irq(fdctrl);
>          }
> +        break;
> +
> +    case FD_PHASE_COMMAND:
> +    default:
> +        abort();
>      }
> +
> +    retval = fdctrl->fifo[pos];
>      FLOPPY_DPRINTF("data register: 0x%02x\n", retval);
>  
>      return retval;
> 

Reviewed-by: John Snow <jsnow@redhat.com>