Re: pccbb crashes when detaching (unsafe interrupt handler)

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.

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.

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.

-- 
Brian Fundakowski Feldman                           \'[ FreeBSD ]''''''''''\
  <> green_at_FreeBSD.org                               \  The Power to Serve! \
 Opinions expressed are my own.                       \,,,,,,,,,,,,,,,,,,,,,,\