Re: pccbb crashes when detaching (unsafe interrupt handler)

In message: <20040717191943.GU1626_at_green.homeunix.org>
            Brian Fundakowski Feldman <green_at_FreeBSD.ORG> writes:
: On Sat, Jul 17, 2004 at 12:48:03PM -0600, M. Warner Losh wrote:
: > In message: <20040717182841.GR1626_at_green.homeunix.org>
: > : FreeBSD 5.x is not based on the spl
: > : model anymore, and unless you want to ADD the ability to do something like
: > : that, which is certainly quite feasible (that is, add a "blocking" count
: > : to the interrupt handler, which the ithread has to ack before you can
: > : return with the interrupt "blocked") there will be a specific cost inside
: > : the pccbb driver to do things safely and it will involve some sort of
: > : synchronization.
: > 
: > I don't assume that things are spl based, I just had never seen the
: > race that you found.  I don't want to use sx locks to solve this
: > because I'd one day like to make the cbb isr a fast interrupt handler
: > so that we can restore fast interrupt handlers to pccard and cardbus
: > modems.  sx locks sleep, and can't be used in a fast interrupt
: > handler.
: 
: I'd love it if we could tone down the anger and incredulity.  From this
: point on, let's start off assuming each one of us knows what the other is
: talking about.  None of us likes feeling insulted or slighted, so I would
: like to publically apologize for being provoked and taking an angry tone
: when I am simply interested in fixing this matter (and many others) so
: that 5.3 Does Not Suck.

I'd also like to appologize for getting my nose out of joint.  I have
some personal issues hitting finality in my life right now, and I've
noticed I have a thinner skin of late as a result.

: Okay, using an sx_xlock() (instead of sx_tryxlock()) would certainly prevent
: that, but I can understand not wanting to add more weight to the fast
: path.  It's something no one wants, even if it's the cleanest solution.

One thing that I could do, if we *KNOW* that the bridge ISR is run
first of all the shared interrupts, is to use atomic ops to clear/set
the OK flag.  That would prevent me from calling the ISR, while
pushing down into kern_intr.c the details of dealing with the issues
of inserting/removing from the list.  This would also get the priority
right, which I currently don't do right in the pccbb code.  I'd then
register a wrapper for these interrupts that checks the OK flag and
neglects to call them if it isn't OK.  This does add a little
overhead, but not all that much, to the ISR.  And it would ensure that
pccard/cardbus interrupts have the same semantics as others in the
system.

The question is do we know this?  And are there plans that the ISRs
might run on different CPUs in parallel...

I also did some research.  I likely didn't see this because I grab
Giant when attaching/detaching cards and the ISR used to be marked as
needing Giant, so the race you've found wouldn't happened.  When I
removed the Giant flag from the bridge ISR, this race reared its ugly
head.

: So now that I've had more time to think about it, here's my idea on the
: feasibility of an ithread handler critical section.  It should cost no
: more in the fast path than it does now, other than increasing the amount
: of code jumped past by several instructions when it's skipped.
:
: >From currentish kern_intr.c:
:                                 if ((ih->ih_flags & IH_DEAD) != 0) {
:                                         mtx_lock(&ithd->it_lock);
:                                         TAILQ_REMOVE(&ithd->it_handlers, ih,
:                                             ih_next);
:                                         wakeup(ih);
:                                         mtx_unlock(&ithd->it_lock);
:                                         goto restart;
:                                 }
: We add a flag IH_PIN:
:                                 if ((ih->ih_flags & (IH_DEAD | IH_PIN)) != 0) {
: 					if ((ih->ih_flags & IH_DEAD) == 0) {
: 						wakeup(ih);
: 						continue;
: 					}
:                                         mtx_lock(&ithd->it_lock);
: 					TAILQ_REMOVE(&ithd->it_handlers,
: 					    ih, ih_next);
:                                         wakeup(ih);
:                                         mtx_unlock(&ithd->it_lock);
:                                         goto restart;
:                                 }
:
: The implementation for the caller would look almost the same as the
: ithread_remove_handler() function.  It would push essentially all of the
: cost out to the code actually modifying the specifics of the interrupt
: handler, and should provide a sufficient critical section for any such
: drivers that want to create their own interrupt sub-handlers.
: 
: So ithread_pin_handler()/ithread_unpin_handler() seem like a reasonable
: solution, if a little bit ugly to be calling ithread support code from
: drivers directly?  I don't know if there's any reason on FreeBSD the
: driver should not be able to know for certain that all interrupts are
: registered as an intrhand on on ithread.

I'm not sure what I think of this, so I'll think about it for a while
and reply later.

Warner