Re: [PATCH v2] floppy: remove dead code related to formatting
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 Signed-off-by: Alexander Bulekov --- 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) -
[PATCH v2] floppy: remove dead code related to formatting
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 Signed-off-by: Alexander Bulekov --- 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; -fdc