> On Tue, 10 Nov 1998, Gerard Roudier wrote:
>
> > The mb() macros works at processor level and for example allows to flush
> > write buffers. The concern is PCI ordering rules and especially rules
> > that applies to posted memory writes that are handle by the bridge and not
> > the processor.
>
> The mb() macro does a memory locked operation to force ordering. If that
> doesn't do the PCI flush, then that was a mis-understanding on my part
> from some time ago. I'm not real concerned about it anyway. Re-read the
> code section you pointed out. In there, we outb the CLRCMDINT once, and
> only once, and after that we *enter* the loop that grabs commands. We
> then run through the commands clearing all of them out, without ever
> clearing the INTSTAT register again. We won't ever have to worry about
> the problem you suggested. What I do have to worry about is the rare
> possibility that I'll complete a command when I haven't cleared the
> INTSTAT register for that command and we'll get re-called with nothing to
> do.
Reread my initial mail. The PCI memory write transactions performed by the
device (sequencer) and the PCI memory writes performed by the CPU have no
ordering requirements. Only PCI memory writes in the same direction follow
ordering. But a PCI memory read may be used to flush them and so to
ensure some ordering.
So, when PCI writes are posted in both directions, the write to INSTAT may
clear the flag at the device and the last command completion may not yet
be flushed to memory.
In such a situation, if the write from the sequencer occurs after your
read of the INTSTAT and before your write to the INTSTAT, you may just
miss the last completion and your reread of the INSTAT after having
scanned the completion queue will not see any flag set in the INSTAT for
it. You may be fortunate if another command will complete, but if there
are no active commands, the lost command completion will be timed out.
When interrupts aren't shared, MMIO may seem to work because the damned
bridge flushes posted buffers prior to deliver the interrupt, but when
IRQs are shared, buffers may have been flushed for a previous device that
shares the same IRQ but not for the other device since the interrupt for
it may have been raised while it is already raised for the other device.
> > > > do
> > > > {
> > > > aic7xxx_isr(irq, dev_id, regs);
> > > > } while ( (aic_inb(p, INTSTAT) & INT_PEND) );
> > > >
> > > > I often noticed this in BSD stuff. If the purpose is not to lose PCI
> > > > interrupt, let me tell you that such a construct is, in my opinion, kind
> > > > of bad quality band-aid.
> > >
> > > This is the comment that made me let this letter set for a month. Gerard,
> > > if you seriously think I am so lame as to use such a construct as a
> > > "band-aid" to avoid loosing PCI interrupts, then we have little if anything
> > > else further to discuss. You and I are obviously in two different worlds..
> >
> > It is also executed for PCI. It is useless and may somtimes add too much
> > latency for other devices that are sharing the IRQ with your driver.
>
> That's stupid Gerard. Read the code in question, understand the flow of
> execution and where things are done, and then think about it. The entire
> aic7xxx_isr() routine is a fairly short code path. In general, the code
> rarely calls into the scsiint, seqint, and brkadrint handlers. For the
The code is question is stupid in my opinion.
It performs a mostly useless read to PCI that flushes for nothing posted
transactions if any. On the other hand it may increase uselessly the
latency for devices that will be called for the same IRQ after your code.
It is not a significant penalty, indeed, but it seems horribly stupid
to me.
> most part, multiple invocations of that code run the cmdcmplt int section
> and that's it. The REQINIT case is a special example, but one that
> deserves the loop in question especially considering how fast it is. If I
> handled each REQINIT as an individual interrupt, message handling speed in
> that driver would fall like a rock. Doing it this was is much more
> efficient. Multiple executions of that code path are relatively painless
> compared to the overhead of isr entry and exit. If I had the scsi
> completion stuff in that same loop, then your complaint might be valid,
> but I don't so it isn't. The scsi completion is done later when we call
> aic7xxx_done_cmds_complete(p);. That part isn't in the loop.
>
> > Handling EISA/VLB the same way as PCI is not the way to go if PCI has to
> > be victimized, IMO.
>
> Again, this is stupid Gerard. The point wasn't that I'm doing this
> because of VLB or EISA, my point in that commentary was that the VLB cards
> allowed me to determine whether I was or was not loosing interrupts. The
> aic7xxx_isr() code does not loose interrupts as you suggested. The fact
> that I loop is an optimization thing. Consider the following:
You are not optimizing anything by trying to catch an event that has less
han 1/1000 chance to occur. But you just are doing 1000 times a stupid
thing for about nothing.
> When we outb the data onto the bus (or inb the data from the bus) on the
> aic7xxx cards, that inb/outb operation automatically pulls the ack signal
> so the device can send the next byte. I found a race condition in Justin
> Gibb's original interrupt code in this area. We used to do the following:
>
> aic_outb(data, SCSI_DATL); /* This put the data out and sent the ACK */
> aic_outb(CLRSCSIINT, CLRINT); /* This clears the SCSI interrupt */
> return();
>
> This worked fine. The outb onto the SCSI bus would automatically reset
> the REQINIT bit in SSTAT1 and then we could clear the SCSIINT bit in
> INTSTAT. However, I wanted to be more exact and so I did the following:
>
> aic_outb(data, SCSI_DATL);
> aic_outb(CLRREQINIT, CLRSSTAT1); /* explicity clear the REQINIT bit
> instead of relying on an auto clear */
> aic_outb(CLRSCSIINT, CLRINT);
My concern is not specific aic7xxx stuff, but PCI and IRQ ones.
This stuff is beyond my fields.
(BTW, tampering SCSI REQ/ACK signals using programmed I/O can be totally
avoided with NCR/SYMBIOS/LSI chips).
[ ... ]
> > My mail was indeed to harsh and I apologize to you for that.
> > I have been nervous to see that the QUEUE stuff of FreeBSD was in the
> > kernel. Not because I dislike it, but because it is Linus to decide on
> > this point and I thought you just shoe-horned the thing in your patch.
> >
> > But I stick with my remarks on the technical issues regarding ordering and
> > you driver (and perhaps Justin's one too, but I haven't time to look into
> > that one).
>
> Jesus Gerard, first you complain that I import too much stuff from FreeBSD
> like you've actually looked at the code, then you say you'll look at it
Without any doc, I haven't been able to understand the code within the
time I had for.
> later. As for the queue stuff, that was one header file that was being
> used as a static definition and that didn't even get used in any
> functional way in my code. When I removed it (after Jes Sorenson pointed
> out the fact that it still used the BSD license, which I had missed when I
> put it in there), the entire queue file was replaced by one struct def in
> sequencer.h.
I noticed that.
> Doug Ledford <dledford@redhat.com>
> Opinions expressed are my own, but
> they should be everybody's.
They aren't.
Gerard.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/