Re: [Qemu-devel] [PATCH v6 5/6] fdc_test: update media_change test

[Pavel Hrdina]

On 06/25/2012 11:50 AM, Kevin Wolf wrote:
> Am 22.06.2012 12:33, schrieb Pavel Hrdina:
> > After rewrite DSKCHG bit handling the test has to be updated. Now
> > is needed to seek to different track to clear DSKCHG bit.
> >
> > Signed-off-by: Pavel Hrdina<address@hidden>
> > ---
> >   tests/fdc-test.c |   29 +++++++++++++++++++++--------
> >   1 files changed, 21 insertions(+), 8 deletions(-)
> >
> > diff --git a/tests/fdc-test.c b/tests/fdc-test.c
> > index 610e2f1..5280eff 100644
> > --- a/tests/fdc-test.c
> > +++ b/tests/fdc-test.c
> > @@ -156,19 +156,20 @@ static uint8_t send_read_command(void)
> >       return ret;
> >   }
> >
> > -static void send_step_pulse(void)
> > +static void send_step_pulse(bool chg_cyl)
> >   {
> >       int drive = 0;
> >       int head = 0;
> > -    static int cyl = 0;
> > +    int cyl = 0;
> > +
> > +    if (chg_cyl)
> > +        cyl = (cyl + 1) % 4;
> Why do you remove the static? Previously, the function would cycle
> through cylinders 1-4. Now it's always 0 if !chg_cyl and 1 if chg_cyl. I
> think you need to fix this.
>
> I also don't quite understand why you move the increment to here instead
> of leaving it as the bottom. It doesn't look wrong, but pointless.
Now the functionality is different. I need seek to cylinder 0 or 1. 
After every change of media an actual cylinder is set to 0 and I need to 
try seek to cylinder 0 to test, that after this seek the DSKCHG bit is 
still set. Then I need to seek to different cylinder to reset the DSKCHG 
and for that seek to 1 is enough.

I'll chagne the

cyl = (cyl + 1) % 4; to cyl = 1;

> >       floppy_send(CMD_SEEK);
> >       floppy_send(head<<  2 | drive);
> >       g_assert(!get_irq(FLOPPY_IRQ));
> >       floppy_send(cyl);
> >       ack_irq();
> > -
> > -    cyl = (cyl + 1) % 4;
> >   }
> >
> >   static uint8_t cmos_read(uint8_t reg)
> > @@ -195,8 +196,7 @@ static void test_no_media_on_start(void)
> >       assert_bit_set(dir, DSKCHG);
> >       dir = inb(FLOPPY_BASE + reg_dir);
> >       assert_bit_set(dir, DSKCHG);
> > -    send_step_pulse();
> > -    send_step_pulse();
> > +    send_step_pulse(1);
> This is a bool parameter, so you should pass true/false instead of 0/1.
>
> Other than that, the test case additions look good and I'll accept it as
> sufficient for the DSKCHG handling change. It would be great, though, to
> add some more cases that test the implicit seeks during commands like
> READ (ideally one for each command that can seek).
>
> Kevin
I'll fix the bool parameter and then I'll create new patches for more 
test cases.

Pavel