1. Patchew
  2. QEMU
  3. View series

[PATCH v2] floppy: remove dead code related to formatting

Alexander Bulekov posted 1 patch 3 years ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210428022803.606814-1-alxndr@bu.edu
hw/block/fdc.c | 97 --------------------------------------------------
1 file changed, 97 deletions(-)
[PATCH v2] floppy: remove dead code related to formatting
Posted by Alexander Bulekov 3 years ago 
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
Re: [PATCH v2] floppy: remove dead code related to formatting
Posted by John Snow 2 years, 11 months ago 
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);
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.