Re: [Qemu-block] [Qemu-devel] [PATCH 6/8] fdc: Disentangle phases in fdctrl_read_data()
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 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.
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. :)
Re: [Qemu-block] [Qemu-devel] [PATCH 6/8] fdc: Disentangle phases in fdctrl_read_data()
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 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.
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
Re: [Qemu-block] [Qemu-devel] [PATCH 6/8] fdc: Disentangle phases in fdctrl_read_data()
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
