Re: ATA Patch #7 (Re: ATA Patch #6)

[YONETANI Tomokazu <qhwt+dfly@xxxxxxxxxx>]

On Tue, Nov 30, 2004 at 10:45:43AM -0800, Matthew Dillon wrote:
> :Here's another patch to be applied on top of ATA Patch #7(attached).
> :I added ata_clear_interlock() in ata_dmastart() because it needs
> :device interrupt enabled anyway. Here I'm assuming that the critical
> :section can be nested(looking at thread2.h it can).
> :Command-start delay can be changed via hw.ata.command_delay(default: 10).
> :Negative value is ignored and silently reset to 0 (can we let sysctl(8)
> :to reject bogus values?).
> 
>     You can use SYSCTL_PROC to control allowed values, but it may be
>     easier just to leave it SYSCTL_INT and check at DELAY time, e.g.:
> 
>     delay = blahdelay;
>     if (delay < 1)
> 	delay = 1;
>     if (delay > 1000)
> 	delay = 1000;
>     DELAY(delay);

I took this way, but allowing the blahdelay to be zero(in which case
DELAY() isn't called). I also made selection delay tunable, and made
two delays tunable int's so that I can put it in the /boot/loader.conf
  hw.ata.command_delay=0
  hw.ata.selection_delay=0
(it works on all of my DragonFly machines, but not sure on other machines)

They both default to 10 for safety, but these DELAY()'s seem to have bigger
impact on performance on slower machines. Does command-start delay really
need to be 10us(=10000ns) which is far too larger than the required 400ns
and taking into account the number of instructions between ATA_OUTB()
and ATA_INB()?

>     We are going to have to clean up the ata_clear_interlock() calls...
>     we do not want to reenable the interlock in situations where no 
>     interrupt is required to complete the command because otherwise
>     if an interrupt does occur the system can livelock because we no
>     longer unconditionally read the status register if we dont think the
>     interrupt is valid.
> 
>     e.g. there are a bunch of places where you added ata_clear_interlock()
>     where you shouldn't have.  Most of those need to be backed out.  We
>     *ONLY* clear the interlock and enable the interrupt if the operation
>     is one which requires an interrupt to complete.

Yes, it turned out that ata_clear_interlock() I added in the previous
patch were unnecessary, and only ones following ata_dmastart() should
have been moved.

>     Here is a posit... if writing to ALTSTATUS (which controls IDS and 
>     other bits) is illegal during DMAC, is reading STATUS or ALTSTATUS
>     also illegal during DMAC?  The spec seems to indicate that it is
>     not illegal but I'm beginning to think that it may be.
>     If it is then we need to add more code to the interrupt handler, though
>     I'm not sure exactly what we would be able to do.. I guess we could
>     check the DMA controller to see if it indicates that DMA has completed,
>     but that doesn't help us with the ATA_S_ERROR case.

I don't know about that.. Alternate Status register and Device Control register
are just two separate registers sharing the same address, so I don't think
the same access restriction as Device Control register applies to Alternate
Status register. But it may depend on controllers.

>     I've synchronized with your patch-7-take-2 so lets form a new basis
>     relative to that.  Could you take another pass at the interlock calls
>     you added and remove the ones for situations where no completion
>     interrupt is expected?

Thanks, attached(a diff against CVS-HEAD).

Unfortunately, none of ATA Patch #7, #7 + patch-7-take2, or the patch
attached to this message worked on a desktop in my office. It's has
a PentiumIV 1.7GHz running SMP kernel(but HTT is unavailable). No timer
problem, and not using TIMER_USE_1.
It frequently timed out trying to identify the media in the CD-ROM drive,
or when writing to the harddrive. Reading from the harddrive also timed
out, but only once or so at the beginning.
Inserting DELAY()'s after the call to ata_clear_interlock() seemed to
reduce the number of timeouts, but it wasn't quite reproducible.
Increasing or decreaing command_delay or selection_delay seemed to have
little to do with the timeout. Sigh. I'll try a UP kernel later to see
if running an SMP kernel has something to do with this.

Attachment: ata-9.patch.gz
Description: application/gunzip