* [PATCH] floppy: remove unused function fdctrl_format_sector
@ 2021-01-08 23:01 Alexander Bulekov
2021-03-12 6:45 ` John Snow
0 siblings, 1 reply; 4+ messages in thread
From: Alexander Bulekov @ 2021-01-08 23:01 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Alexander Bulekov, John Snow, open list:Floppy,
Max Reitz
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(-)
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);
--
2.27.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] floppy: remove unused function fdctrl_format_sector
2021-01-08 23:01 [PATCH] floppy: remove unused function fdctrl_format_sector Alexander Bulekov
@ 2021-03-12 6:45 ` John Snow
2021-03-14 7:53 ` Hervé Poussineau
0 siblings, 1 reply; 4+ messages in thread
From: John Snow @ 2021-03-12 6:45 UTC (permalink / raw)
To: Alexander Bulekov, qemu-devel
Cc: Kevin Wolf, Hervé Poussineau, open list:Floppy, Max Reitz
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);
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] floppy: remove unused function fdctrl_format_sector
2021-03-12 6:45 ` John Snow
@ 2021-03-14 7:53 ` Hervé Poussineau
2021-04-27 18:13 ` John Snow
0 siblings, 1 reply; 4+ messages in thread
From: Hervé Poussineau @ 2021-03-14 7:53 UTC (permalink / raw)
To: John Snow, Alexander Bulekov, qemu-devel
Cc: Kevin Wolf, open list:Floppy, Max Reitz
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);
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] floppy: remove unused function fdctrl_format_sector
2021-03-14 7:53 ` Hervé Poussineau
@ 2021-04-27 18:13 ` John Snow
0 siblings, 0 replies; 4+ messages in thread
From: John Snow @ 2021-04-27 18:13 UTC (permalink / raw)
To: Hervé Poussineau, Alexander Bulekov, qemu-devel
Cc: Kevin Wolf, open list:Floppy, Max Reitz
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-04-27 18:16 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-01-08 23:01 [PATCH] floppy: remove unused function fdctrl_format_sector Alexander Bulekov
2021-03-12 6:45 ` John Snow
2021-03-14 7:53 ` Hervé Poussineau
2021-04-27 18:13 ` John Snow
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).