Re: [PATCH] floppy: remove unused function fdctrl_format_sector

["Hervé Poussineau" <hpoussin@reactos.org>]

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