Date: Wed, 24 Sep 1997 14:47:44 +0200 From: Stefan Esser <se@FreeBSD.ORG> To: Nate Williams <nate@mt.sri.com> Cc: mobile@FreeBSD.ORG, current@FreeBSD.ORG Subject: Re: PCCARD in -current broken Message-ID: <19970924144744.47293@mi.uni-koeln.de> In-Reply-To: <199709232246.QAA09189@rocky.mt.sri.com>; from Nate Williams on Tue, Sep 23, 1997 at 04:46:56PM -0600 References: <199709231929.NAA08312@rocky.mt.sri.com> <19970923230018.00034@mi.uni-koeln.de> <199709232246.QAA09189@rocky.mt.sri.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sep 23, Nate Williams <nate@mt.sri.com> wrote:
> > There should not be a problem. ISA does not
> > (should not, I didn't check the sources
> > recently) register the handler as shared,
> > and this will prevent another handler (both
> > shared or exclusive) to be registered.
>
> register_intr() sets the INTR_EXCL flag just before it calls
> intr_create() (so far so good), but in intr_connect(), the code to check
> for that flag is ifdef's out:
Well, I have implemented experimental resource
registration and check code, but come to the
conclusion, that it should be done quite
differently ...
> int
> intr_connect(intrec *idesc)
> {
> ....
> #ifdef RESOURCE_CHECK
> resflag = (idesc->flags & INTR_EXCL) ? RESF_NONE : RESF_SHARED;
> if (resource_claim(idesc->devdata, REST_INT, resflag, irq, irq) == 0)
> #endif /* RESOURCE_CHECK */
> {
>
> So, we don't even check to see if INTR_EXCL is used.
Well, you don't. I do, but I don't rely on that
check :)
> How does the new code realize that the interrupt is exclusive then?
I have implemented a very simple set of functions
to claim and check resource usage, and you see the
resource_claim() call above. (The code does know
resource types (e.g. REST_INT is interrupt), the
minimum and maximum value (both passed as "irq"
in the example), and a flag that currently only
takes the value RESF_SHARED, if a resource may
be shared with other uses, as long as all of them
have RESF_SHARED set.
> > Well, ISA interrupts should be registered in
> > a way that guarantees they are not shared.
>
> How?
But the code in -current does not use this function,
and to make sure, that no other interrupt is added
in combination with an exclusive interrupt, there is
the test in add_intrdesc(), which I outlined in my
previous reply.
> So, can I rely on register_intr() return a negative # for failure?
Well, if it fails for you, then there definitely is
a bug in the check I added to add_intrdesc(), which
is there to avoid just this situation.
I assume you are sure that the PCCARD code is only
called after the ISA devices are attached ?
> (Here's the code in question.)
> static u_int
> build_freelist(u_int pcic_mask)
> {
> inthand2_t *nullfunc;
> int irq;
> u_int mask, freemask;
>
> /* No free IRQs (yet). */
> freemask = 0;
>
> /* Walk through all of the IRQ's and find any that aren't allocated. */
> for (irq = 0; irq < ICU_LEN; irq++) {
> /*
> * If the PCIC controller can't generate it, don't
> * bother checking to see if it it's free.
> */
> mask = 1 << irq;
> if (!(mask & pcic_mask)) continue;
>
> /* See if the IRQ is free. */
> if (register_intr(irq, 0, 0, nullfunc, NULL, irq) == 0) {
> /* Give it back, but add it to the mask */
> INTRMASK(freemask, mask);
> unregister_intr(irq, nullfunc);
> }
> }
> #ifdef PCIC_DEBUG
> printf("Freelist of IRQ's <0x%x>\n", freemask);
> #endif
> return freemask;
> }
Looks fine to me. Lets see what happens after you call
register_intr():
[ Much simplified extract from /sys/kern/kern_intr.c ! ]
/** INTR_EXCL is added to flags and passed to intr_create(). **/
int
register_intr(int intr, int device_id, u_int flags,
inthand2_t handler, u_int *maskptr, int unit)
{
flags |= INTR_EXCL;
idesc = intr_create((void *)device_id, intr, handler,
(void*)unit, maskptr, flags);
return (intr_connect(idesc));
}
/** The flags parameter is put into the idesc structure. **/
intrec *
intr_create(void *dev_instance, int irq, inthand2_t handler, void *arg,
intrmask_t *maskptr, int flags)
{
idesc = malloc(sizeof *idesc, M_DEVBUF, M_WAITOK);
if (idesc) {
idesc->flags = flags;
}
return (idesc);
}
/** Return the error code from add_intrdesc (rest of code not shown). **/
int
intr_connect(intrec *idesc)
{
errcode = add_intrdesc(idesc);
return (errcode);
}
/** If this is the first handler for this irq, just register it. **/
/** If another handler has been registered before, then make sure, **/
/** that both the old and new handler are for a device that supports **/
/** shared interrupts (does not have INTR_EXCL set). Only the first **/
/** handler can have INTR_EXCL set, since no second one could have **/
/** been added ... **/
static int
add_intrdesc(intrec *idesc)
{
if (head == NULL) {
/* first handler for this irq, just install it */
if (icu_setup(irq, idesc->handler, idesc->argument,
idesc->maskptr, idesc->flags) != 0)
return (-1);
} else {
if ((idesc->flags & INTR_EXCL) != 0
|| (head->flags & INTR_EXCL) != 0) {
/*
* can't append new handler, if either list head or
* new handler do not allow interrupts to be shared
*/
printf("\tdevice combination doesn't support shared irq%d\n",
irq);
return (-1);
}
/* DO REAL WORK */
}
return (0);
}
Well, in fact you should see the "device combination doesn't support ..."
message printed, when you call register_intr() to check for the interrupt
to be available. If you don't then there is a good chance, that you perform
this test, before the ISA devices are attached, and there is no collision
reported until the PCCARD attach tries to register a handler for an assumed
free IRQ, which has been claimed by an ISA driver in between ...
Regards, Stefan
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?19970924144744.47293>