Re: [PATCH] floppy: remove unused function fdctrl_format_sector

[Hervé Poussineau]

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);