LIST_ENTRY and DDK serial ISR

Hi,

Following is code from DDK serial ISR. I’m seeing a bug in it’s
implemenation but I’d like to get your opinion because this thing is a bit
confusing to me.

My concern is with “while (interruptEntry != firstInterruptEntry);”, so when
they are same then interruptEntry points to LIST_ENTRY structure itself. So
if “while (thisPassServiced);” is true then CONTAINING_RECORD won’t return
a valid SERIAL_DEVICE_EXTENSION. This is what I’m seeing with our multiport
serial board. If do " interruptEntry = interruptEntry->Flink;" before “while
(thisPassServiced);” then I see that extension starts from port 1. So the
fix is as following:

>>

interruptEntry = interruptEntry->Flink;

} while (interruptEntry != firstInterruptEntry);

interruptEntry = interruptEntry->Flink;

} while (thisPassServiced);

>>

I may be wrong, any thought?

Thanks,
Hakim

BOOLEAN
SerialSharerIsr(
IN PKINTERRUPT InterruptObject,
IN PVOID Context
)

/*++

Routine Description:

This is the isr that the system will call if there are any
serial devices sharing the same interrupt and they aren’t
all confined to one multiport card. This routine traverses
a linked list structure that contains a pointer to a more
refined isr and context that will indicate whether one of
the ports on this interrupt actually was interrupting.

Arguments:

InterruptObject - Points to the interrupt object declared for this
device. We *do not* use this parameter.

Context - Pointer to a linked list of contextes and isrs.
device.

Return Value:

This function will return TRUE if a serial port using this
interrupt was the source of this interrupt, FALSE otherwise.

–*/

{

BOOLEAN servicedAnInterrupt = FALSE;
BOOLEAN thisPassServiced;
PLIST_ENTRY interruptEntry = ((PLIST_ENTRY)Context)->Flink;
PLIST_ENTRY firstInterruptEntry = Context;

if (IsListEmpty(firstInterruptEntry)) {
return FALSE;
}

do {

thisPassServiced = FALSE;
do {

PSERIAL_DEVICE_EXTENSION extension = CONTAINING_RECORD(
interruptEntry,
SERIAL_DEVICE_EXTENSION,
TopLevelSharers
);

thisPassServiced |= extension->TopLevelOurIsr(
InterruptObject,
extension->TopLevelOurIsrContext
);

servicedAnInterrupt |= thisPassServiced;
interruptEntry = interruptEntry->Flink;

} while (interruptEntry != firstInterruptEntry);

} while (thisPassServiced);

return servicedAnInterrupt;

}

Hakim wrote:

Following is code from DDK serial ISR. I’m seeing a bug in it’s
implemenation but I’d like to get your opinion because this thing is a bit
confusing to me.

My concern is with “while (interruptEntry != firstInterruptEntry);”, so when
they are same then interruptEntry points to LIST_ENTRY structure itself. So
if “while (thisPassServiced);” is true then CONTAINING_RECORD won’t return
a valid SERIAL_DEVICE_EXTENSION. This is what I’m seeing with our multiport
serial board. If do " interruptEntry = interruptEntry->Flink;" before “while
(thisPassServiced);” then I see that extension starts from port 1. So the
fix is as following:

interruptEntry = interruptEntry->Flink;

} while (interruptEntry != firstInterruptEntry);

interruptEntry = interruptEntry->Flink;

} while (thisPassServiced);

I may be wrong, any thought?

Interesting; I’d say your analysis was correct. An alternative
solution, which avoids duplicating the code, is to move the declaration
for interruptEntry into the outer loop:

PLIST_ENTRY firstInterruptEntry = Context;

if (IsListEmpty(firstInterruptEntry)) {
return FALSE;
}

do {
PLIST_ENTRY interruptEntry = firstInterruptEntry->Flink;

thisPassServiced = FALSE;
do {

// …

servicedAnInterrupt |= thisPassServiced;
interruptEntry = interruptEntry->Flink;

} while (interruptEntry != firstInterruptEntry);

} while (thisPassServiced);

Or this, which I think reads even better:

PLIST_ENTRY firstInterruptEntry = Context;

if (IsListEmpty(firstInterruptEntry)) {
return FALSE;
}

do {
PLIST_ENTRY interruptEntry;

thisPassServiced = FALSE;
for(
interruptEntry = firstInterruptEntry->Flink,
interruptEntry != firstInterruptEntry;
interruptEntry = interruptEntry->Flink
) {

// …

servicedAnInterrupt |= thisPassServiced;

}

} while (thisPassServiced);


Tim Roberts, xxxxx@probo.com
Providenza & Boekelheide, Inc.

Thanks.
Hakim.

“Tim Roberts” wrote in message news:xxxxx@ntdev…
> Hakim wrote:
>
>>Following is code from DDK serial ISR. I’m seeing a bug in it’s
>>implemenation but I’d like to get your opinion because this thing is a bit
>>confusing to me.
>>
>>My concern is with “while (interruptEntry != firstInterruptEntry);”, so
>>when
>>they are same then interruptEntry points to LIST_ENTRY structure itself.
>>So
>>if “while (thisPassServiced);” is true then CONTAINING_RECORD won’t
>>return
>>a valid SERIAL_DEVICE_EXTENSION. This is what I’m seeing with our
>>multiport
>>serial board. If do " interruptEntry = interruptEntry->Flink;" before
>>“while
>>(thisPassServiced);” then I see that extension starts from port 1. So the
>>fix is as following:
>>
>>
>>
>> interruptEntry = interruptEntry->Flink;
>>
>> } while (interruptEntry != firstInterruptEntry);
>>
>> interruptEntry = interruptEntry->Flink;
>>
>> } while (thisPassServiced);
>>
>>
>>
>>I may be wrong, any thought?
>>
>>
>
> Interesting; I’d say your analysis was correct. An alternative
> solution, which avoids duplicating the code, is to move the declaration
> for interruptEntry into the outer loop:
>
> PLIST_ENTRY firstInterruptEntry = Context;
>
> if (IsListEmpty(firstInterruptEntry)) {
> return FALSE;
> }
>
> do {
> PLIST_ENTRY interruptEntry = firstInterruptEntry->Flink;
>
> thisPassServiced = FALSE;
> do {
>
> // …
>
> servicedAnInterrupt |= thisPassServiced;
> interruptEntry = interruptEntry->Flink;
>
> } while (interruptEntry != firstInterruptEntry);
>
> } while (thisPassServiced);
>
> Or this, which I think reads even better:
>
> PLIST_ENTRY firstInterruptEntry = Context;
>
> if (IsListEmpty(firstInterruptEntry)) {
> return FALSE;
> }
>
> do {
> PLIST_ENTRY interruptEntry;
>
> thisPassServiced = FALSE;
> for(
> interruptEntry = firstInterruptEntry->Flink,
> interruptEntry != firstInterruptEntry;
> interruptEntry = interruptEntry->Flink
> ) {
>
> // …
>
> servicedAnInterrupt |= thisPassServiced;
>
> }
>
> } while (thisPassServiced);
>
> –
> Tim Roberts, xxxxx@probo.com
> Providenza & Boekelheide, Inc.
>
>