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 <kw...@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 <js...@redhat.com>