> 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.
> > > 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
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:
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);
This broke the code since the SDT-5000 tape drive from Sony was fast
enough that between the 2nd and 3rd aic_outb instructions, it would
re-raise the REQ signal, and that would re-assert the REQINIT, which would
re-assert the SCSIINT, which we would then clear erroneously. We then
would miss one of our bytes.
So that gives an idea of how fast some devices are on the SCSI bus in
regards to REQ/ACK cycles. The average message that this driver handles
is 5 bytes long and only rarely sent. Now, the reason I have that loop is
so that we don't take 5 separate interrupts in order to handle these
conditions. I can't loop in the REQINIT handler proper for the next byte
since the reading of some of the registers is undefined until the INTSTAT
goes high again and I didn't want to loop there on INSTAT on the off
chance that the bus goes nuts and we don't ever get the next REQ signal.
So, basically, I allow a few outb instructions, followed by the exit from
aic7xxx_handle_reqinit(), followed by the exit from
aic7xxx_handle_scsiint(), followed by the exit from aic7xxx_isr() as a
sort of delay time. If the next byte is ready by then, the INTSTAT will
have gone high again, and we go back in and grab the next byte. If not,
tough shit to the device, it will have to live with the added latency of
re-grabbing the io_request_lock (or the global cli() in 2.0.x) plus a
bunch of other cycle eating stuff before we come back to re-service this
interrupt. That was my compromise between going one interrupt per
character or spinning in the interrupt handler waiting for the bus to be
ready for the next byte. If your opinion is that this victimizes the PCI
bus, then I'm sorry, I think you are insane. Doing it character by
character would victimize any bus far more than this current code does
simply because of the way it would end up consuming extra CPU cycles to
get the same job done.
> > FreeBSD can look at things where the code isn't dictated by hardware
> > ordering requirements and see I don't. Simply check the init code, the
> > reset code, the queue code, etc. In all of those areas it becomes obvious
> > that there actually is little code shared between FreeBSD and linux. Now,
> > having said that, I'm going to ignore the rest of your comment as uneducated
> > guessing.
>
> 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
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.
Doug Ledford <dledford@redhat.com>
Opinions expressed are my own, but
they should be everybody's.
-
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/