| Message ID | 20210108230137.8860-1-alxndr@bu.edu |
|---|---|
| State | New |
| Headers | show |
| Series | floppy: remove unused function fdctrl_format_sector | expand |
On 1/8/21 6:01 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. > Hm, was this code *ever* used? It's hard to tell when we go back into the old SVN history. Does this mean that fdctrl_handle_format_track is also basically an incomplete stub method? I'm in favor of deleting bitrotted code, but I wonder if we should take a bigger bite. --js > This removes fdctrl_format_sector and the unncessary setting/unsetting > of the FD_STATE_FORMAT flag. > > Signed-off-by: Alexander Bulekov <alxndr@bu.edu> > --- > hw/block/fdc.c | 68 -------------------------------------------------- > 1 file changed, 68 deletions(-) > > diff --git a/hw/block/fdc.c b/hw/block/fdc.c > index 3636874432..837dd819ea 100644 > --- a/hw/block/fdc.c > +++ b/hw/block/fdc.c > @@ -1952,67 +1952,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; > @@ -2126,7 +2065,6 @@ static void fdctrl_handle_format_track(FDCtrl *fdctrl, int direction) > > 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 > @@ -2144,7 +2082,6 @@ static void fdctrl_handle_format_track(FDCtrl *fdctrl, int direction) > * 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); > } > > @@ -2458,11 +2395,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); >
Le 12/03/2021 à 07:45, John Snow a écrit : > On 1/8/21 6:01 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. >> > > Hm, was this code *ever* used? It's hard to tell when we go back into the old SVN history. > > Does this mean that fdctrl_handle_format_track is also basically an incomplete stub method? > > I'm in favor of deleting bitrotted code, but I wonder if we should take a bigger bite. > > --js The fdctrl_format_sector has been added in SVN revision 671 (baca51faff03df59386c95d9478ede18b5be5ec8), along with FD_STATE_FORMAT/FD_FORMAT_CMD. As with current code, the only place where the FD_STATE_FORMAT flag was 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. I can however see the following comment: /* Bochs BIOS is buggy and don't send format informations * for each sector. So, pretend all's done right now... */ fdctrl->data_state &= ~FD_STATE_FORMAT; which was changed in SVN revision 2295 (b92090309e5ff7154e4c131438ee2d540e233955) to: /* 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 */ This probably means that code may have worked without DMA (to be confirmed), but was disabled since its introduction due to a problem with Bochs BIOS. Later, fdformat was also tested and not working. Since then, lots of work has also been done in DMA handling. I especially think at bb8f32c0318cb6c6e13e09ec0f35e21eff246413, which fixed a similar problem with floppy drives on IBM 40p machine. What happens when this flag unsetting is removed? Does fdformat now works? I think that we should either fix the code, or remove more code (everything related to fdctrl_format_sector, FD_STATE_FORMAT, FD_FORMAT_CMD, maybe even fdctrl_handle_format_track). Regards, Hervé > >> This removes fdctrl_format_sector and the unncessary setting/unsetting >> of the FD_STATE_FORMAT flag. >> >> Signed-off-by: Alexander Bulekov <alxndr@bu.edu> >> --- >> hw/block/fdc.c | 68 -------------------------------------------------- >> 1 file changed, 68 deletions(-) >> >> diff --git a/hw/block/fdc.c b/hw/block/fdc.c >> index 3636874432..837dd819ea 100644 >> --- a/hw/block/fdc.c >> +++ b/hw/block/fdc.c >> @@ -1952,67 +1952,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; >> @@ -2126,7 +2065,6 @@ static void fdctrl_handle_format_track(FDCtrl *fdctrl, int direction) >> 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 >> @@ -2144,7 +2082,6 @@ static void fdctrl_handle_format_track(FDCtrl *fdctrl, int direction) >> * 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); >> } >> @@ -2458,11 +2395,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); >> >
On 3/14/21 3:53 AM, Hervé Poussineau wrote: > Le 12/03/2021 à 07:45, John Snow a écrit : >> On 1/8/21 6:01 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. >>> >> >> Hm, was this code *ever* used? It's hard to tell when we go back into >> the old SVN history. >> >> Does this mean that fdctrl_handle_format_track is also basically an >> incomplete stub method? >> >> I'm in favor of deleting bitrotted code, but I wonder if we should >> take a bigger bite. >> >> --js > > The fdctrl_format_sector has been added in SVN revision 671 > (baca51faff03df59386c95d9478ede18b5be5ec8), along with > FD_STATE_FORMAT/FD_FORMAT_CMD. > As with current code, the only place where the FD_STATE_FORMAT flag was > 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. > > I can however see the following comment: > /* Bochs BIOS is buggy and don't send format informations > * for each sector. So, pretend all's done right now... > */ > fdctrl->data_state &= ~FD_STATE_FORMAT; > > which was changed in SVN revision 2295 > (b92090309e5ff7154e4c131438ee2d540e233955) to: > /* 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 > */ > > This probably means that code may have worked without DMA (to be > confirmed), but was disabled since its introduction due to a problem > with Bochs BIOS. > Later, fdformat was also tested and not working. > > Since then, lots of work has also been done in DMA handling. I > especially think at bb8f32c0318cb6c6e13e09ec0f35e21eff246413, which > fixed a similar problem with floppy drives on IBM 40p machine. > What happens when this flag unsetting is removed? Does fdformat now works? > > I think that we should either fix the code, or remove more code > (everything related to fdctrl_format_sector, FD_STATE_FORMAT, > FD_FORMAT_CMD, maybe even fdctrl_handle_format_track). > > Regards, > > Hervé > Alex, do you want to respin this following Hervé's suggestion for additional deletions? I doubt anyone has the time or interest to actually FIX this code, so we may as well remove misleading code. --js
diff --git a/hw/block/fdc.c b/hw/block/fdc.c index 3636874432..837dd819ea 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -1952,67 +1952,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; @@ -2126,7 +2065,6 @@ static void fdctrl_handle_format_track(FDCtrl *fdctrl, int direction) 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 @@ -2144,7 +2082,6 @@ static void fdctrl_handle_format_track(FDCtrl *fdctrl, int direction) * 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); } @@ -2458,11 +2395,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);
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 and the unncessary setting/unsetting of the FD_STATE_FORMAT flag. Signed-off-by: Alexander Bulekov <alxndr@bu.edu> --- hw/block/fdc.c | 68 -------------------------------------------------- 1 file changed, 68 deletions(-)