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

[Kevin Wolf <kwolf@redhat.com>]

Am 19.05.2015 um 22:40 hat John Snow geschrieben:
> 
> 
> 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.

Yes, that was redundant code.

> > -        /* 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.

I'm not too happy about it either, but I couldn't think of anything
better. Having two different switches almost immediately after each
other, with only the if line in between, would look really awkward and
be hard to read. And the old code isn't nice either.

If you have any idea for a better solution, let me know.

Kevin