From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47711) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YuoJg-0005wa-8L for qemu-devel@nongnu.org; Tue, 19 May 2015 16:40:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YuoJf-0001aG-AE for qemu-devel@nongnu.org; Tue, 19 May 2015 16:40:44 -0400 Message-ID: <555B9FC7.9070008@redhat.com> Date: Tue, 19 May 2015 16:40:39 -0400 From: John Snow MIME-Version: 1.0 References: <1432049762-2184-1-git-send-email-kwolf@redhat.com> <1432049762-2184-7-git-send-email-kwolf@redhat.com> In-Reply-To: <1432049762-2184-7-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 6/8] fdc: Disentangle phases in fdctrl_read_data() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-block@nongnu.org Cc: qemu-devel@nongnu.org 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 > --- > 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