Re: [Qemu-devel] [PATCH 5/8] fdc: Code cleanup in fdctrl_write_data()

[Kevin Wolf <kwolf@redhat.com>]

Am 19.05.2015 um 22:40 hat John Snow geschrieben:
> 
> 
> On 05/19/2015 11:35 AM, Kevin Wolf wrote:
> > Factor out a few common lines of code, reformat, improve comments.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  hw/block/fdc.c | 62 +++++++++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 38 insertions(+), 24 deletions(-)
> > 
> > diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> > index a13e0ce..cbf7abf 100644
> > --- a/hw/block/fdc.c
> > +++ b/hw/block/fdc.c
> > @@ -1942,14 +1942,16 @@ static void fdctrl_handle_relative_seek_out(FDCtrl *fdctrl, int direction)
> >  /*
> >   * Handlers for the execution phase of each command
> >   */
> > -static const struct {
> > +typedef struct FDCtrlCommand {
> >      uint8_t value;
> >      uint8_t mask;
> >      const char* name;
> >      int parameters;
> >      void (*handler)(FDCtrl *fdctrl, int direction);
> >      int direction;
> > -} handlers[] = {
> > +} FDCtrlCommand;
> > +
> > +static const FDCtrlCommand handlers[] = {
> >      { FD_CMD_READ, 0x1f, "READ", 8, fdctrl_start_transfer, FD_DIR_READ },
> >      { FD_CMD_WRITE, 0x3f, "WRITE", 8, fdctrl_start_transfer, FD_DIR_WRITE },
> >      { FD_CMD_SEEK, 0xff, "SEEK", 2, fdctrl_handle_seek },
> > @@ -1986,9 +1988,19 @@ static const struct {
> >  /* Associate command to an index in the 'handlers' array */
> >  static uint8_t command_to_handler[256];
> >  
> > +static const FDCtrlCommand *get_command(uint8_t cmd)
> > +{
> > +    int idx;
> > +
> > +    idx = command_to_handler[cmd];
> > +    FLOPPY_DPRINTF("%s command\n", handlers[idx].name);
> > +    return &handlers[idx];
> > +}
> > +
> >  static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
> >  {
> >      FDrive *cur_drv;
> > +    const FDCtrlCommand *cmd;
> >      uint32_t pos;
> >  
> >      /* Reset mode */
> > @@ -2002,13 +2014,20 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
> >      }
> >      fdctrl->dsr &= ~FD_DSR_PWRDOWN;
> >  
> > +    FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
> > +
> > +    /* 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;
> > +    fdctrl->fifo[pos] = value;
> > +
> >      switch (fdctrl->phase) {
> >      case FD_PHASE_EXECUTION:
> >          assert(fdctrl->msr & FD_MSR_NONDMA);
> > +
> >          /* FIFO data write */
> > -        pos = fdctrl->data_pos++;
> > -        pos %= FD_SECTOR_LEN;
> > -        fdctrl->fifo[pos] = value;
> >          if (pos == FD_SECTOR_LEN - 1 ||
> >              fdctrl->data_pos == fdctrl->data_len) {
> >              cur_drv = get_cur_drv(fdctrl);
> > @@ -2024,41 +2043,36 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
> >                  break;
> >              }
> >          }
> > -        /* Switch from transfer mode to status mode
> > -         * then from status mode to command mode
> > -         */
> > -        if (fdctrl->data_pos == fdctrl->data_len)
> > +
> > +        /* Switch to result phase when done with the transfer */
> > +        if (fdctrl->data_pos == fdctrl->data_len) {
> >              fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
> > +        }
> >          break;
> >  
> >      case FD_PHASE_COMMAND:
> >          assert(!(fdctrl->msr & FD_MSR_NONDMA));
> >  
> > -        if (fdctrl->data_pos == 0) {
> > -            /* Command */
> > -            pos = command_to_handler[value & 0xff];
> > -            FLOPPY_DPRINTF("%s command\n", handlers[pos].name);
> > -            fdctrl->data_len = handlers[pos].parameters + 1;
> > +        if (fdctrl->data_pos == 1) {
> 
> This reads more awkwardly than the previous ifz, but it's not
> like I have a better idea. (I just had a momentary pause of "Why 1?")

Hm, I think we can assert fdctrl->data_pos < FD_SECTOR_LEN (this is the
command phase and commands only have a few bytes of parameters) and
therefore check for pos == 0 here, if you think that's better.

Kevin