Date: Thu, 29 Jan 1998 13:22:05 +1030 From: Mike Smith <mike@smith.net.au> To: Bruce Evans <bde@zeta.org.au> Cc: mike@smith.net.au, cvs-committers@FreeBSD.ORG, msmith@FreeBSD.ORG Subject: Re: cvs commit: src/sys/i386/isa atapi.c Message-ID: <199801290252.NAA00885@word.smith.net.au> In-Reply-To: Your message of "Thu, 29 Jan 1998 04:43:48 %2B1100." <199801281743.EAA15033@godzilla.zeta.org.au>
next in thread | previous in thread | raw e-mail | index | archive | help
> >The Zip is, as I said, slow off the bus. At least on the P6 I was
> >using, it goes like this (PC98 junk elided):
> >
> > /* Wait for controller not busy. */
> > outb (port + AR_DRIVE, unit ? ARD_DRIVE1 : ARD_DRIVE0);
> > if (atapi_wait (port, 0) < 0) {
>
> I complain about some other points while I'm here:
> - There is no wait before the first outb. It is invalid to access the
> "Command block" registers while BSY is set.
> - The comment doesn't apply to the code that immediately follows it.
> The PC98 ifdef moves it further from the code that it applies to.
Agreed on both. I wasn't rewriting the driver, just patching around
undesirable behaviour with minimal side-effects.
> >## Zip is still on the bus here; atapi_wait() returns happily thinking
> >## there is a device present and ready. A possibly better fix would be
> >## to have the 10us delay earlier in the loop in atapi_wait().
>
> Does atapi_wait() really return early here? You didn't change anything
> here.
No, I didn't. atapi_wait() obviously returns happily or I wouldn't
have cared. At this point the problem is that we are assuming that
there is a drive responding to selection.
> > /* Issue ATAPI IDENTIFY command. */
> > outb (port + AR_DRIVE, unit ? ARD_DRIVE1 : ARD_DRIVE0);
> > outb (port + AR_COMMAND, ATAPIC_IDENTIFY);
>
> - OK if Zip follows spec, since we waited. Since we reselect the drive
> here, not waiting earlier is probably harmless. Selecting it earlier
> may be to hide a bug somehere.
Indeed. Removing the first selection would count as a side-effect in
my view.
> - The PC98 code waits for DRQ to be deasserted between the above 2 outb's
> this may be to hide the bug related to stale DRQs in atapi_wait() (if
> BSY and DRQ are both stale (and wrong), then atapi_wait() returns
> early).
If this is the case, then the PC98 define is misplaced. I am not happy
with the amount of PC98 crap blasted through various drivers; it tends
to chop the flow up *extremely* badly.
> > /* Check that device is present. */
> > if (inb (port + AR_STATUS) == 0xff) {
>
> - Bogus. The device had better be there since we have written a lot to
> its registers. atapi_wait() has just succeeded, so its status was
> not 0xff when it was read before the command was issued (ARS_BSY was
> clear). The value just read is invalid since we didn't wait 400 nsec.
Not necessarily bogus, because if you assume the first drive selection
may potentially fail and the second may be the first honoured, then we
have not yet checked for a device responding. We should check for a
response to selection before issuing a command, however writing a
command to a drive that's not present is relatively harmless.
> >## Zip is going off the bus at this point, but not gone.
> >## Read returns 0xa1.
>
> I see, you think 0xa1 is an echo of the value just read.
I don't actually know *what* the 0xa1 value is. I do know that a
subsequent call to atapi_wait(, DRQ) returns as though DRQ had been
validly asserted, but that after the return there is nothing actually
selected.
> It happens to
> be valid but not reasonable as a status value, since it is ARS_BSY |
> ARS_DF | ARS_CHECK, and a drive fault or error probably won't occur this
> fast. The drive is probably within spec if it gets off the bus within
> 400 nsec. A non-buggy driver couldn't tell the difference, because the
> ARS_BSY bit is invalid until 400 nsec after the command is issued.
Yes. A complete reimplementation of atapi_probe() could bring it
within spec but would lose the implicit (and unrecoverable) bug/feature
support that the current version has.
> The bogus inb() probably helps because it delays for >= 400 nsec. What
> does the next inb() return?
Which next one? ie. a subsequent read from the status port?
> > /* Wait for data ready. */
> > if (atapi_wait (port, ARS_DRQ) != 0) {
> >
> >## Zip eventually goes off the bus, and thus DRQ reads high.
>
> I don't think this has anything to do with the bus. The Zip has been
> assembling the reply somewhere. After finishing of up to 700 nsec before,
> it sets DRQ, which atapi_wait() reads a little later, up to 700 nsec
> before the Zip clears BSY to indicate that it has finished, not to mention
> that DRQ is valid.
The Zip isn't, or shouldn't be, assembling a reply. It's not
*selected*, which is the whole issue here. The Zip in this case is the
master, and we are trying to probe a *slave*.
I haven't had the time to try the case where there is a slave; I am in
the throes of packing my life into the smallest set of cardboard boxes
that will contain it and putting it in the hands of an unscrupulous
freight forwarder.
> >At this point the probe thinks it has a device that's responded to the
> >IDENTIFY. The Zip is off having a beer, and the result is a garbage
> >probe. The easiest fix at the time was to simply catch the obviously
> >nonsense case where the status register reads 0xff *after* waiting for
> >DRQ.
>
> Did it actually read as 0xff? This is possible, since BSY is not checked
> for.
Obviously it did. Why else would I have bothered with the change?
> >> Perhaps the problem is earlier. According to an old draft version of
> >> the ATA spec, the timing for issueing a polled-mode input command is:
>
> >0.0 Select the target
> >0.5 Wait XXX<units) for drive to respond to selection, or poll
> > (some register) to indicate drive is selected.
>
> I think it's actually the polling of DRQ without checking BSY (step 9
> or 10), which is most likely to cause problems if the drive is smart
> enough to set DRQ 700 nsec in advance of becoming ready, to give drivers
> advance notice.
I think you are still under the mistaken assumption that we are trying
to talk to the Zip here. The whole point of the change was to deal
with the case where we aren't.
--
\\ Sometimes you're ahead, \\ Mike Smith
\\ sometimes you're behind. \\ mike@smith.net.au
\\ The race is long, and in the \\ msmith@freebsd.org
\\ end it's only with yourself. \\
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199801290252.NAA00885>