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

[John Snow <jsnow@redhat.com>]

On 05/20/2015 04:25 AM, Kevin Wolf wrote:
> 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
> 

I'm all complaints and no solutions. I believe I gave you my R-b anyway. :)