* [PATCH v2] floppy: remove dead code related to formatting
@ 2021-04-28 2:28 Alexander Bulekov
2021-05-17 22:59 ` John Snow
0 siblings, 1 reply; 2+ messages in thread
From: Alexander Bulekov @ 2021-04-28 2:28 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, open list:Floppy, Max Reitz, Alexander Bulekov,
Hervé Poussineau, John Snow
fdctrl_format_sector was added in
baca51faff ("updated floppy driver: formatting code, disk geometry auto detect (Jocelyn Mayer)")
The single callsite is guarded by a check:
fdctrl->data_state & FD_STATE_FORMAT
However, the only place where the FD_STATE_FORMAT flag is set (in
fdctrl_handle_format_track) is closely followed by the same flag being
unset, with no possibility to call fdctrl_format_sector in between.
This removes fdctrl_format_sector, the unncessary setting/unsetting
of the FD_STATE_FORMAT flag, and the fdctrl_handle_format_track function
(which is just a stub).
Suggested-by: Hervé Poussineau <hpoussin@reactos.org>
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
I ran through tests/qtest/fdc-test, and ran fdformat on a dummy disk -
nothing exploded, but since I don't use floppies very often, more eyes
definitely won't hurt. In particular, I'm not sure about the
fdctrl_handle_format_track delete - that function has side-effects on
both FDrive and FDCtrl, and it is certainly reachable. If deleting the
whole thing seems wrong, I'll roll-back that change, and we can just
remove the unreachable code..
hw/block/fdc.c | 97 --------------------------------------------------
1 file changed, 97 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index a825c2acba..d851d23cc0 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -657,7 +657,6 @@ enum {
enum {
FD_STATE_MULTI = 0x01, /* multi track flag */
- FD_STATE_FORMAT = 0x02, /* format flag */
};
enum {
@@ -826,7 +825,6 @@ enum {
};
#define FD_MULTI_TRACK(state) ((state) & FD_STATE_MULTI)
-#define FD_FORMAT_CMD(state) ((state) & FD_STATE_FORMAT)
struct FDCtrl {
MemoryRegion iomem;
@@ -1942,67 +1940,6 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
return retval;
}
-static void fdctrl_format_sector(FDCtrl *fdctrl)
-{
- FDrive *cur_drv;
- uint8_t kh, kt, ks;
-
- SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);
- cur_drv = get_cur_drv(fdctrl);
- kt = fdctrl->fifo[6];
- kh = fdctrl->fifo[7];
- ks = fdctrl->fifo[8];
- FLOPPY_DPRINTF("format sector at %d %d %02x %02x (%d)\n",
- GET_CUR_DRV(fdctrl), kh, kt, ks,
- fd_sector_calc(kh, kt, ks, cur_drv->last_sect,
- NUM_SIDES(cur_drv)));
- switch (fd_seek(cur_drv, kh, kt, ks, fdctrl->config & FD_CONFIG_EIS)) {
- case 2:
- /* sect too big */
- fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, 0x00, 0x00);
- fdctrl->fifo[3] = kt;
- fdctrl->fifo[4] = kh;
- fdctrl->fifo[5] = ks;
- return;
- case 3:
- /* track too big */
- fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_EC, 0x00);
- fdctrl->fifo[3] = kt;
- fdctrl->fifo[4] = kh;
- fdctrl->fifo[5] = ks;
- return;
- case 4:
- /* No seek enabled */
- fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, 0x00, 0x00);
- fdctrl->fifo[3] = kt;
- fdctrl->fifo[4] = kh;
- fdctrl->fifo[5] = ks;
- return;
- case 1:
- fdctrl->status0 |= FD_SR0_SEEK;
- break;
- default:
- break;
- }
- memset(fdctrl->fifo, 0, FD_SECTOR_LEN);
- if (cur_drv->blk == NULL ||
- blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
- BDRV_SECTOR_SIZE, 0) < 0) {
- FLOPPY_DPRINTF("error formatting sector %d\n", fd_sector(cur_drv));
- fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
- } else {
- if (cur_drv->sect == cur_drv->last_sect) {
- fdctrl->data_state &= ~FD_STATE_FORMAT;
- /* Last sector done */
- fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
- } else {
- /* More to do */
- fdctrl->data_pos = 0;
- fdctrl->data_len = 4;
- }
- }
-}
-
static void fdctrl_handle_lock(FDCtrl *fdctrl, int direction)
{
fdctrl->lock = (fdctrl->fifo[0] & 0x80) ? 1 : 0;
@@ -2110,34 +2047,6 @@ static void fdctrl_handle_readid(FDCtrl *fdctrl, int direction)
(NANOSECONDS_PER_SECOND / 50));
}
-static void fdctrl_handle_format_track(FDCtrl *fdctrl, int direction)
-{
- FDrive *cur_drv;
-
- SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);
- cur_drv = get_cur_drv(fdctrl);
- fdctrl->data_state |= FD_STATE_FORMAT;
- if (fdctrl->fifo[0] & 0x80)
- fdctrl->data_state |= FD_STATE_MULTI;
- else
- fdctrl->data_state &= ~FD_STATE_MULTI;
- cur_drv->bps =
- fdctrl->fifo[2] > 7 ? 16384 : 128 << fdctrl->fifo[2];
-#if 0
- cur_drv->last_sect =
- cur_drv->flags & FDISK_DBL_SIDES ? fdctrl->fifo[3] :
- fdctrl->fifo[3] / 2;
-#else
- cur_drv->last_sect = fdctrl->fifo[3];
-#endif
- /* TODO: implement format using DMA expected by the Bochs BIOS
- * and Linux fdformat (read 3 bytes per sector via DMA and fill
- * the sector with the specified fill byte
- */
- fdctrl->data_state &= ~FD_STATE_FORMAT;
- fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
-}
-
static void fdctrl_handle_specify(FDCtrl *fdctrl, int direction)
{
fdctrl->timer0 = (fdctrl->fifo[1] >> 4) & 0xF;
@@ -2330,7 +2239,6 @@ static const FDCtrlCommand handlers[] = {
{ FD_CMD_SEEK, 0xff, "SEEK", 2, fdctrl_handle_seek },
{ FD_CMD_SENSE_INTERRUPT_STATUS, 0xff, "SENSE INTERRUPT STATUS", 0, fdctrl_handle_sense_interrupt_status },
{ FD_CMD_RECALIBRATE, 0xff, "RECALIBRATE", 1, fdctrl_handle_recalibrate },
- { FD_CMD_FORMAT_TRACK, 0xbf, "FORMAT TRACK", 5, fdctrl_handle_format_track },
{ FD_CMD_READ_TRACK, 0xbf, "READ TRACK", 8, fdctrl_start_transfer, FD_DIR_READ },
{ FD_CMD_RESTORE, 0xff, "RESTORE", 17, fdctrl_handle_restore }, /* part of READ DELETED DATA */
{ FD_CMD_SAVE, 0xff, "SAVE", 0, fdctrl_handle_save }, /* part of READ DELETED DATA */
@@ -2448,11 +2356,6 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
/* We have all parameters now, execute the command */
fdctrl->phase = FD_PHASE_EXECUTION;
- if (fdctrl->data_state & FD_STATE_FORMAT) {
- fdctrl_format_sector(fdctrl);
- break;
- }
-
cmd = get_command(fdctrl->fifo[0]);
FLOPPY_DPRINTF("Calling handler for '%s'\n", cmd->name);
cmd->handler(fdctrl, cmd->direction);
--
2.28.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v2] floppy: remove dead code related to formatting
2021-04-28 2:28 [PATCH v2] floppy: remove dead code related to formatting Alexander Bulekov
@ 2021-05-17 22:59 ` John Snow
0 siblings, 0 replies; 2+ messages in thread
From: John Snow @ 2021-05-17 22:59 UTC (permalink / raw)
To: Alexander Bulekov, qemu-devel, Hervé Poussineau
Cc: Kevin Wolf, Hervé Poussineau, open list:Floppy, Max Reitz
On 4/27/21 10:28 PM, Alexander Bulekov wrote:
> fdctrl_format_sector was added in
> baca51faff ("updated floppy driver: formatting code, disk geometry auto detect (Jocelyn Mayer)")
>
> The single callsite is guarded by a check:
> fdctrl->data_state & FD_STATE_FORMAT
>
> However, the only place where the FD_STATE_FORMAT flag is set (in
> fdctrl_handle_format_track) is closely followed by the same flag being
> unset, with no possibility to call fdctrl_format_sector in between.
>
> This removes fdctrl_format_sector, the unncessary setting/unsetting
> of the FD_STATE_FORMAT flag, and the fdctrl_handle_format_track function
> (which is just a stub).
>
> Suggested-by: Hervé Poussineau <hpoussin@reactos.org>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>
Herve, does it look good to you? I feel bad about deleting code out of a
device that badly needs attention, but it seems like this code was
probably not operating correctly to begin with and I don't have the time
to figure out how to implement it correctly.
> I ran through tests/qtest/fdc-test, and ran fdformat on a dummy disk -
> nothing exploded, but since I don't use floppies very often, more eyes
> definitely won't hurt. In particular, I'm not sure about the
> fdctrl_handle_format_track delete - that function has side-effects on
> both FDrive and FDCtrl, and it is certainly reachable. If deleting the
> whole thing seems wrong, I'll roll-back that change, and we can just
> remove the unreachable code..
>
Yeah, I just had some reservations about allowing a stub to persist that
touched state and didn't actually seem to invoke the routine it was
meant to.
It's hard to audit the impact either way, and I don't have a good test
suite to know what the ramifications are.
> hw/block/fdc.c | 97 --------------------------------------------------
> 1 file changed, 97 deletions(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index a825c2acba..d851d23cc0 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -657,7 +657,6 @@ enum {
>
> enum {
> FD_STATE_MULTI = 0x01, /* multi track flag */
> - FD_STATE_FORMAT = 0x02, /* format flag */
> };
>
> enum {
> @@ -826,7 +825,6 @@ enum {
> };
>
> #define FD_MULTI_TRACK(state) ((state) & FD_STATE_MULTI)
> -#define FD_FORMAT_CMD(state) ((state) & FD_STATE_FORMAT)
>
> struct FDCtrl {
> MemoryRegion iomem;
> @@ -1942,67 +1940,6 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
> return retval;
> }
>
> -static void fdctrl_format_sector(FDCtrl *fdctrl)
> -{
> - FDrive *cur_drv;
> - uint8_t kh, kt, ks;
> -
> - SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);
> - cur_drv = get_cur_drv(fdctrl);
> - kt = fdctrl->fifo[6];
> - kh = fdctrl->fifo[7];
> - ks = fdctrl->fifo[8];
> - FLOPPY_DPRINTF("format sector at %d %d %02x %02x (%d)\n",
> - GET_CUR_DRV(fdctrl), kh, kt, ks,
> - fd_sector_calc(kh, kt, ks, cur_drv->last_sect,
> - NUM_SIDES(cur_drv)));
> - switch (fd_seek(cur_drv, kh, kt, ks, fdctrl->config & FD_CONFIG_EIS)) {
> - case 2:
> - /* sect too big */
> - fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, 0x00, 0x00);
> - fdctrl->fifo[3] = kt;
> - fdctrl->fifo[4] = kh;
> - fdctrl->fifo[5] = ks;
> - return;
> - case 3:
> - /* track too big */
> - fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_EC, 0x00);
> - fdctrl->fifo[3] = kt;
> - fdctrl->fifo[4] = kh;
> - fdctrl->fifo[5] = ks;
> - return;
> - case 4:
> - /* No seek enabled */
> - fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, 0x00, 0x00);
> - fdctrl->fifo[3] = kt;
> - fdctrl->fifo[4] = kh;
> - fdctrl->fifo[5] = ks;
> - return;
> - case 1:
> - fdctrl->status0 |= FD_SR0_SEEK;
> - break;
> - default:
> - break;
> - }
> - memset(fdctrl->fifo, 0, FD_SECTOR_LEN);
> - if (cur_drv->blk == NULL ||
> - blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
> - BDRV_SECTOR_SIZE, 0) < 0) {
> - FLOPPY_DPRINTF("error formatting sector %d\n", fd_sector(cur_drv));
> - fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
> - } else {
> - if (cur_drv->sect == cur_drv->last_sect) {
> - fdctrl->data_state &= ~FD_STATE_FORMAT;
> - /* Last sector done */
> - fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
> - } else {
> - /* More to do */
> - fdctrl->data_pos = 0;
> - fdctrl->data_len = 4;
> - }
> - }
> -}
> -
> static void fdctrl_handle_lock(FDCtrl *fdctrl, int direction)
> {
> fdctrl->lock = (fdctrl->fifo[0] & 0x80) ? 1 : 0;
> @@ -2110,34 +2047,6 @@ static void fdctrl_handle_readid(FDCtrl *fdctrl, int direction)
> (NANOSECONDS_PER_SECOND / 50));
> }
>
> -static void fdctrl_handle_format_track(FDCtrl *fdctrl, int direction)
> -{
> - FDrive *cur_drv;
> -
> - SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);
> - cur_drv = get_cur_drv(fdctrl);
> - fdctrl->data_state |= FD_STATE_FORMAT;
> - if (fdctrl->fifo[0] & 0x80)
> - fdctrl->data_state |= FD_STATE_MULTI;
> - else
> - fdctrl->data_state &= ~FD_STATE_MULTI;
> - cur_drv->bps =
> - fdctrl->fifo[2] > 7 ? 16384 : 128 << fdctrl->fifo[2];
> -#if 0
> - cur_drv->last_sect =
> - cur_drv->flags & FDISK_DBL_SIDES ? fdctrl->fifo[3] :
> - fdctrl->fifo[3] / 2;
> -#else
> - cur_drv->last_sect = fdctrl->fifo[3];
> -#endif
> - /* TODO: implement format using DMA expected by the Bochs BIOS
> - * and Linux fdformat (read 3 bytes per sector via DMA and fill
> - * the sector with the specified fill byte
> - */
> - fdctrl->data_state &= ~FD_STATE_FORMAT;
> - fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
> -}
> -
> static void fdctrl_handle_specify(FDCtrl *fdctrl, int direction)
> {
> fdctrl->timer0 = (fdctrl->fifo[1] >> 4) & 0xF;
> @@ -2330,7 +2239,6 @@ static const FDCtrlCommand handlers[] = {
> { FD_CMD_SEEK, 0xff, "SEEK", 2, fdctrl_handle_seek },
> { FD_CMD_SENSE_INTERRUPT_STATUS, 0xff, "SENSE INTERRUPT STATUS", 0, fdctrl_handle_sense_interrupt_status },
> { FD_CMD_RECALIBRATE, 0xff, "RECALIBRATE", 1, fdctrl_handle_recalibrate },
> - { FD_CMD_FORMAT_TRACK, 0xbf, "FORMAT TRACK", 5, fdctrl_handle_format_track },
> { FD_CMD_READ_TRACK, 0xbf, "READ TRACK", 8, fdctrl_start_transfer, FD_DIR_READ },
> { FD_CMD_RESTORE, 0xff, "RESTORE", 17, fdctrl_handle_restore }, /* part of READ DELETED DATA */
> { FD_CMD_SAVE, 0xff, "SAVE", 0, fdctrl_handle_save }, /* part of READ DELETED DATA */
> @@ -2448,11 +2356,6 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
> /* We have all parameters now, execute the command */
> fdctrl->phase = FD_PHASE_EXECUTION;
>
> - if (fdctrl->data_state & FD_STATE_FORMAT) {
> - fdctrl_format_sector(fdctrl);
> - break;
> - }
> -
> cmd = get_command(fdctrl->fifo[0]);
> FLOPPY_DPRINTF("Calling handler for '%s'\n", cmd->name);
> cmd->handler(fdctrl, cmd->direction);
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-05-17 23:02 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-28 2:28 [PATCH v2] floppy: remove dead code related to formatting Alexander Bulekov
2021-05-17 22:59 ` John Snow
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.