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 <p...@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
