Patchwork [v6,5/6] fdc_test: update media_change test

login
register
mail settings
Submitter Pavel Hrdina
Date June 22, 2012, 10:33 a.m.
Message ID <b64802460ceadf02d96aef6b5d76173b0d98f8bb.1340360906.git.phrdina@redhat.com>
Download mbox | patch
Permalink /patch/166579/
State New
Headers show

Comments

Pavel Hrdina - June 22, 2012, 10:33 a.m.
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 <phrdina@redhat.com>
---
 tests/fdc-test.c |   29 +++++++++++++++++++++--------
 1 files changed, 21 insertions(+), 8 deletions(-)
Blue Swirl - June 24, 2012, 6:16 a.m.
On Fri, Jun 22, 2012 at 10:33 AM, Pavel Hrdina <phrdina@redhat.com> wrote:
> 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 <phrdina@redhat.com>
> ---
>  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;

Missing braces, please use checkpatch.pl to avoid these issues.

% 4 could be turned into & 3, maybe with a separate patch.

>
>     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);
>     dir = inb(FLOPPY_BASE + reg_dir);
>     assert_bit_set(dir, DSKCHG);
>     dir = inb(FLOPPY_BASE + reg_dir);
> @@ -227,7 +227,14 @@ static void test_media_change(void)
>     dir = inb(FLOPPY_BASE + reg_dir);
>     assert_bit_set(dir, DSKCHG);
>
> -    send_step_pulse();
> +    send_step_pulse(0);
> +    dir = inb(FLOPPY_BASE + reg_dir);
> +    assert_bit_set(dir, DSKCHG);
> +    dir = inb(FLOPPY_BASE + reg_dir);
> +    assert_bit_set(dir, DSKCHG);
> +
> +    /* Step to next track should clear DSKCHG bit. */
> +    send_step_pulse(1);
>     dir = inb(FLOPPY_BASE + reg_dir);
>     assert_bit_clear(dir, DSKCHG);
>     dir = inb(FLOPPY_BASE + reg_dir);
> @@ -243,7 +250,13 @@ static void test_media_change(void)
>     dir = inb(FLOPPY_BASE + reg_dir);
>     assert_bit_set(dir, DSKCHG);
>
> -    send_step_pulse();
> +    send_step_pulse(0);
> +    dir = inb(FLOPPY_BASE + reg_dir);
> +    assert_bit_set(dir, DSKCHG);
> +    dir = inb(FLOPPY_BASE + reg_dir);
> +    assert_bit_set(dir, DSKCHG);
> +
> +    send_step_pulse(1);
>     dir = inb(FLOPPY_BASE + reg_dir);
>     assert_bit_set(dir, DSKCHG);
>     dir = inb(FLOPPY_BASE + reg_dir);
> --
> 1.7.7.6
>
>
Kevin Wolf - June 25, 2012, 8:49 a.m.
Am 24.06.2012 08:16, schrieb Blue Swirl:
> On Fri, Jun 22, 2012 at 10:33 AM, Pavel Hrdina <phrdina@redhat.com> wrote:
>> 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 <phrdina@redhat.com>
>> ---
>>  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;
> 
> Missing braces, please use checkpatch.pl to avoid these issues.
> 
> % 4 could be turned into & 3, maybe with a separate patch.

I wouldn't do that. It's not something like a mask for a bitfield, but
just a way to let counting start from the beginning after a while. If
you have two way to express the same, choose the one that expresses the
clearest what the real intention is. The compiler will optimise it (not
that this optimisation in a test case like this really mattered...)

Kevin
Kevin Wolf - June 25, 2012, 9:50 a.m.
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 <phrdina@redhat.com>
> ---
>  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.

>  
>      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
Pavel Hrdina - July 3, 2012, 2:43 p.m.
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<phrdina@redhat.com>
>> ---
>>   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

Patch

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;
 
     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);
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
     dir = inb(FLOPPY_BASE + reg_dir);
@@ -227,7 +227,14 @@  static void test_media_change(void)
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
 
-    send_step_pulse();
+    send_step_pulse(0);
+    dir = inb(FLOPPY_BASE + reg_dir);
+    assert_bit_set(dir, DSKCHG);
+    dir = inb(FLOPPY_BASE + reg_dir);
+    assert_bit_set(dir, DSKCHG);
+
+    /* Step to next track should clear DSKCHG bit. */
+    send_step_pulse(1);
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_clear(dir, DSKCHG);
     dir = inb(FLOPPY_BASE + reg_dir);
@@ -243,7 +250,13 @@  static void test_media_change(void)
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
 
-    send_step_pulse();
+    send_step_pulse(0);
+    dir = inb(FLOPPY_BASE + reg_dir);
+    assert_bit_set(dir, DSKCHG);
+    dir = inb(FLOPPY_BASE + reg_dir);
+    assert_bit_set(dir, DSKCHG);
+
+    send_step_pulse(1);
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
     dir = inb(FLOPPY_BASE + reg_dir);