Messages in this thread |  | | | Date | Fri, 20 Nov 1998 17:55:43 -0500 (EST) | | From | Doug Ledford <> | | Subject | Re: 2.1.125 - Problem. & Aic7xxx 5.1.0 |
| |
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.
> > > 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/
|  |