| Message ID | 564F2761.8000200@kamp.de |
|---|---|
| State | New |
| Headers | show |
Am 20.11.2015 um 15:00 hat Peter Lieven geschrieben: > Am 20.11.2015 um 14:53 schrieb Kevin Wolf: > > Am 20.11.2015 um 14:44 hat Peter Lieven geschrieben: > >> Am 20.11.2015 um 12:33 schrieb Peter Maydell: > >>> On 20 November 2015 at 09:38, Peter Lieven <pl@kamp.de> wrote: > >>>> I wonder if there is a glitch in the PIO implementation of test-ide.c. As far as I understand the specs > >>>> it is not allowed to read data while the BSY flag is set. With the following change to the test-ide script > >>>> the test does not race: > >>>> > >>>> diff --git a/tests/ide-test.c b/tests/ide-test.c > >>>> index d1014bb..ab0489e 100644 > >>>> --- a/tests/ide-test.c > >>>> +++ b/tests/ide-test.c > >>>> @@ -728,6 +728,7 @@ static void cdrom_pio_impl(int nblocks) > >>>> for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) { > >>>> size_t offset = i * (limit / 2); > >>>> size_t rem = (rxsize / 2) - offset; > >>>> + ide_wait_clear(BSY); > >>>> for (j = 0; j < MIN((limit / 2), rem); j++) { > >>>> rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data)); > >>>> } > >>>> > >>>> Note: in the old sync version of the ATAPI PIO implementation this could not happen. > >>> This certainly fixes the stalls for me, though I don't know enough > >>> IDE to say whether it is the correct fix. > >> Thanks for testing. > >> > >> I hope that John or Kevin can verify this fix? > > The fix looks correct to me. > > > > While you're improving the test, you could also add an assertion that > > DRQ is set after BSY has been cleared. > > I would actually move the check (which is already there) into the loop: > > diff --git a/tests/ide-test.c b/tests/ide-test.c > index d1014bb..d7ee376 100644 > --- a/tests/ide-test.c > +++ b/tests/ide-test.c > @@ -712,11 +712,6 @@ static void cdrom_pio_impl(int nblocks) > /* HPD3: INTRQ_Wait */ > ide_wait_intr(IDE_PRIMARY_IRQ); > > - /* HPD2: Check_Status_B */ > - data = ide_wait_clear(BSY); > - assert_bit_set(data, DRQ | DRDY); > - assert_bit_clear(data, ERR | DF | BSY); > - > /* Read data back: occurs in bursts of 'BYTE_COUNT_LIMIT' bytes. > * If BYTE_COUNT_LIMIT is odd, we transfer BYTE_COUNT_LIMIT - 1 bytes. > * We allow an odd limit only when the remaining transfer size is > @@ -728,6 +723,10 @@ static void cdrom_pio_impl(int nblocks) > for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) { > size_t offset = i * (limit / 2); > size_t rem = (rxsize / 2) - offset; > + /* HPD2: Check_Status_B */ > + data = ide_wait_clear(BSY); > + assert_bit_set(data, DRQ | DRDY); > + assert_bit_clear(data, ERR | DF | BSY); > for (j = 0; j < MIN((limit / 2), rem); j++) { > rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data)); > } > > Are you okay with that? @John, you also? Oh, yes, I missed that the check was already there, just in the wrong place. I agree that this is better. I also see that we have the state names from the ATA spec in a comment, so getting that part right might be nice, too. For a start, HPD* are the wrong states (they are for DMA transfers), it should be HP* everywhere. And for the part that your patch touches, the loop that actually transfers data is part of "HP4: Transfer_Data", so we might add a comment right before the (nested) for. Kevin
diff --git a/tests/ide-test.c b/tests/ide-test.c index d1014bb..d7ee376 100644 --- a/tests/ide-test.c +++ b/tests/ide-test.c @@ -712,11 +712,6 @@ static void cdrom_pio_impl(int nblocks) /* HPD3: INTRQ_Wait */ ide_wait_intr(IDE_PRIMARY_IRQ); - /* HPD2: Check_Status_B */ - data = ide_wait_clear(BSY); - assert_bit_set(data, DRQ | DRDY); - assert_bit_clear(data, ERR | DF | BSY); - /* Read data back: occurs in bursts of 'BYTE_COUNT_LIMIT' bytes. * If BYTE_COUNT_LIMIT is odd, we transfer BYTE_COUNT_LIMIT - 1 bytes. * We allow an odd limit only when the remaining transfer size is @@ -728,6 +723,10 @@ static void cdrom_pio_impl(int nblocks) for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) { size_t offset = i * (limit / 2); size_t rem = (rxsize / 2) - offset; + /* HPD2: Check_Status_B */ + data = ide_wait_clear(BSY); + assert_bit_set(data, DRQ | DRDY); + assert_bit_clear(data, ERR | DF | BSY); for (j = 0; j < MIN((limit / 2), rem); j++) { rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data)); }