Re: ATA Patch #6

[YONETANI Tomokazu <qhwt+dfly@xxxxxxxxxx>]

I'm glad you're back :)

On Sat, Nov 27, 2004 at 12:45:40PM -0800, Matthew Dillon wrote:
> :2) adding DELAY() in ata_command() unconditionally was critical on performance.
> 
>     I can believe that... but by how much?  I don't think that delay is
>     optional or, at least, the interrupt code may have to add the delay
>     itself.  For the moment I would prefer to be conservative.
>
>     There are two unconditional delays.  One at the beginning of
>     ata_command() (in the 'The 10uS delay here could probably be reduced
>     to 1 uS' section), and one at the end (in the 'Start the command 
>     rolling...' section).
> 
>     The performance difference shouldn't be terrible, though, what are
>     you seeing?

I'm only talking about the report from dd command when I pressed ctrl+T,
but I believe reducing the rate from 5Mbytes/sec to 3.5Mbytes/sec has some
impact on the real-world performance.
On my laptop(DynaBook SS3500), If I removed or reduced only the first
DELAY(10), the read rate recovers while the write rate stays reduced. If
I reduced both, the write rate recovers too.

> :3) you removed the conditional from the following code in ata_command()
> :   and made it always control interrupt from the device:
> :  /* disable interrupt from device */
> :  if (atadev->channel->flags & ATA_QUEUED)
> :      ATA_OUTB(atadev->channel->r_altio, ATA_ALTSTAT, ATA_A_IDS | ATA_A_4BIT);
> :
> :   but it led to timeout or lock-up on two of my DragonFly machines.
> :   if I put the conditional back in, the timeout won't happen.
> 
>     This is kinda central to the changes.  I added the interrupt interlock
>     but in order to prevent an interrupt from freezing the system due to
>     livelock (because the interrupt just returns if the interlock is set and
>     doesn't try to clear anything) that means the interrupt bit must be
>     unconditionally disabled until the command has been completely set up.
> 
>     If there is an issue here with the interrupt disablement I think
>     it may be the key to solving the lockup issue.   The ATA spec is
>     fairly clear on how ATA_A_IDS is supposed to work so I thought it was
>     safe to manipulate.

I hope so(I'm talking without knowing the ATA spec, do you have a reference
to a documentation I can read?), but unfortunately, writing to that port
seems to have some side-effect so you can't turn the bit on and off at will,
at least on my DragonFly boxes. I even tried dropping ATA_A_4BIT, but it
still timed out or locked up(even with DELAY()'s you added unchanged).
To avoid lock-ups, I needed to backout all `ATA_A_IDS | ATA_A_4BIT' lines,
and remove ATA_OUTB() in ata_clear_interlock() (it may not work correctly,
but it's too scary to bring it out of single-user mode).

BTW I noticed that you changed
  DELAY(50000);
  ATA_OUTB(ch->r_altio, ATA_ALTSTAT, ATA_A_4BIT);
to
  DELAY(50000);
  ATA_OUTB(ch->r_altio, ATA_ALTSTAT, ATA_A_IDS | ATA_A_4BIT);

where is ATA_A_IDS supposed to be cleared?

> :4) in ata_command(), calls to crit_enter() and crit_exit() don't correspond.
> :
> 
>     Oops.  Yes, I see them.  Here's a new patch (we'll call it #7) with
>     the critical section handling fixed.  Though I think the mismatch
>     only occurs if an error/warning is also reported to the console.

I also noticed while trying #7, a call to ata_clear_interlock() is missing
after a few calls to ata_command() with ATA_WAIT_READY(which is conflicting
to the comment in ata_command() which says caller should clear the interlock).