Latest OpenPic changes

Latest OpenPic changes

[Geert Uytterhoeven]

	Hi Ben,

After two days of hacking I finally found out what went wrong. Your changes to
the OpenPIC code cause my machine to hang in openpic_enable_irq(). I added a
timeout to the do { ... } while (...) loop:

void openpic_enable_irq(u_int irq)
{
        int timeout = 1000000;
        check_arg_irq(irq);
        openpic_clearfield(&ISU[irq - open_pic_irq_offset].Vector_Priority, OPENPIC_MASK);
        /* make sure mask gets to controller before we return to user */
        do {
                mb(); /* sync is probably useless here */
        } while(openpic_readfield(&OpenPIC->Source[irq].Vector_Priority,
                        OPENPIC_MASK) && --timeout);
        if (!timeout)
            printk("openpic_enable_irq %d timeout status 0x%08x\n", irq, openpic_readfield(&OpenPIC->Source[irq].Vector_Priority,OPENPIC_MASK));
}

and OPENPIC_MASK is still set when the loop times out on enabling
IRQ_8259_CASCADE in chrp_init_IRQ(). How is this possible? There's an eieio()
after each register write and the mb() does sync.

Gr{oetje,eeting}s,
--
Geert Uytterhoeven -- Linux/{m68k~Amiga,PPC~CHRP} -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: Latest OpenPic changes

[Benjamin Herrenschmidt]

On Fri, Feb 11, 2000, Geert Uytterhoeven <geert@linux-m68k.org> wrote:

>and OPENPIC_MASK is still set when the loop times out on enabling
>IRQ_8259_CASCADE in chrp_init_IRQ(). How is this possible? There's an eieio()
>after each register write and the mb() does sync.

Is this the only interrupt causing this behaviour ? I beleive that means
either the mask register is not implemented for the cascade or a hw bug.
I'll try to have a look at my docs this week-end.


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: Latest OpenPic changes

[Geert Uytterhoeven]

On Fri, 11 Feb 2000, Benjamin Herrenschmidt wrote:
> On Fri, Feb 11, 2000, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >and OPENPIC_MASK is still set when the loop times out on enabling
> >IRQ_8259_CASCADE in chrp_init_IRQ(). How is this possible? There's an eieio()
> >after each register write and the mb() does sync.
>
> Is this the only interrupt causing this behaviour ? I beleive that means
> either the mask register is not implemented for the cascade or a hw bug.
> I'll try to have a look at my docs this week-end.

Yes it is. It works fine for all other interrupts (MESH, sym53c8xx).

Gr{oetje,eeting}s,
--
Geert Uytterhoeven -- Linux/{m68k~Amiga,PPC~CHRP} -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: Latest OpenPic changes

[Gabriel Paubert]

On Fri, 11 Feb 2000, Geert Uytterhoeven wrote:

>
> 	Hi Ben,
>
> After two days of hacking I finally found out what went wrong. Your changes to
> the OpenPIC code cause my machine to hang in openpic_enable_irq(). I added a
> timeout to the do { ... } while (...) loop:

Please use ISU[irq - open_pic_irq_offset] everywhere. You are not reading
the interrupt you are modifying probably. On the other hand I'm using a
completely different solution, and I don't read back when enabling the
IRQ. I can't see why you could need it, this will go to the bus sooner or
later, why wait one or several microseconds while the interrupts are being
acttually enabled in the controller ?



>
> void openpic_enable_irq(u_int irq)
> {
>         int timeout = 1000000;
>         check_arg_irq(irq);
>         openpic_clearfield(&ISU[irq - open_pic_irq_offset].Vector_Priority, OPENPIC_MASK);
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>         /* make sure mask gets to controller before we return to user */
>         do {
>                 mb(); /* sync is probably useless here */
>         } while(openpic_readfield(&OpenPIC->Source[irq].Vector_Priority,
				    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>                         OPENPIC_MASK) && --timeout);
>         if (!timeout)
>             printk("openpic_enable_irq %d timeout status 0x%08x\n", irq, openpic_readfield(&OpenPIC->Source[irq].Vector_Priority,OPENPIC_MASK));
> }
>

	Gabriel.


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: Latest OpenPic changes

[Geert Uytterhoeven]

On Fri, 11 Feb 2000, Gabriel Paubert wrote:
> On Fri, 11 Feb 2000, Geert Uytterhoeven wrote:
> > After two days of hacking I finally found out what went wrong. Your changes to
> > the OpenPIC code cause my machine to hang in openpic_enable_irq(). I added a
> > timeout to the do { ... } while (...) loop:
>
> Please use ISU[irq - open_pic_irq_offset] everywhere. You are not reading

Doh! I overlooked that one...

> the interrupt you are modifying probably. On the other hand I'm using a
> completely different solution, and I don't read back when enabling the
> IRQ. I can't see why you could need it, this will go to the bus sooner or
> later, why wait one or several microseconds while the interrupts are being
> acttually enabled in the controller ?
>
>
>
> >
> > void openpic_enable_irq(u_int irq)
> > {
> >         int timeout = 1000000;
> >         check_arg_irq(irq);
> >         openpic_clearfield(&ISU[irq - open_pic_irq_offset].Vector_Priority, OPENPIC_MASK);
>                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >         /* make sure mask gets to controller before we return to user */
> >         do {
> >                 mb(); /* sync is probably useless here */
> >         } while(openpic_readfield(&OpenPIC->Source[irq].Vector_Priority,
> 				    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >                         OPENPIC_MASK) && --timeout);
> >         if (!timeout)
> >             printk("openpic_enable_irq %d timeout status 0x%08x\n", irq, openpic_readfield(&OpenPIC->Source[irq].Vector_Priority,OPENPIC_MASK));
> > }

Gr{oetje,eeting}s,
--
Geert Uytterhoeven -- Linux/{m68k~Amiga,PPC~CHRP} -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: Latest OpenPic changes

[Gabriel Paubert]

On Fri, 11 Feb 2000, Geert Uytterhoeven wrote:

> On Fri, 11 Feb 2000, Gabriel Paubert wrote:
> > On Fri, 11 Feb 2000, Geert Uytterhoeven wrote:
> > > After two days of hacking I finally found out what went wrong. Your changes to
> > > the OpenPIC code cause my machine to hang in openpic_enable_irq(). I added a
> > > timeout to the do { ... } while (...) loop:
> >
> > Please use ISU[irq - open_pic_irq_offset] everywhere. You are not reading
>
> Doh! I overlooked that one...

Note that there are still problems, some functions use check_arg_irq(irq)
and then use irq while othre use irq-open_pic_irq_offset after the same
check. I'm still trying to understand how it should work since I suspect
something is wrong given the behaviour of my nfsroot machines :-(

Oh and BTW there is a typo in the kernel.org sources:

		if ( !(_machine && (_MACH_gemini|_MACH_Pmac)) )

				^^
should be & otherwise the cascade interrupt will never be requested.

	Gabriel.


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: Latest OpenPic changes

[Cort Dougan]

} Note that there are still problems, some functions use check_arg_irq(irq)
} and then use irq while othre use irq-open_pic_irq_offset after the same
} check. I'm still trying to understand how it should work since I suspect
} something is wrong given the behaviour of my nfsroot machines :-(
}
} Oh and BTW there is a typo in the kernel.org sources:
}
} 		if ( !(_machine && (_MACH_gemini|_MACH_Pmac)) )
}
} 				^^
} should be & otherwise the cascade interrupt will never be requested.

I think we should drop the check_irq_arg() calls everywhere.  They will
always fail for IPI's and since we remap the irqs anyway it doesn't matter.

I'll fix the typo... Thanks :)


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: Latest OpenPic changes

[Benjamin Herrenschmidt]

On Fri, Feb 11, 2000, Gabriel Paubert <paubert@iram.es> wrote:

>Please use ISU[irq - open_pic_irq_offset] everywhere. You are not reading
>the interrupt you are modifying probably. On the other hand I'm using a
>completely different solution, and I don't read back when enabling the
>IRQ. I can't see why you could need it, this will go to the bus sooner or
>later, why wait one or several microseconds while the interrupts are being
>acttually enabled in the controller ?

Hum... I must have broken that ISU stuff when moving my 2.2.x stuff to
2.3.x. I'll look at fixing this in Cort's tree this week-end. There's
nothing like that in 2.2.x, all offseting is made outside of openpic.c.

I read back the interrupt until it's actually changed in the controller
for the simple reason that if I don't do that, I sometimes get bogus
interrupts. It looks like the calling path from driver code calling
disable_irq() and the actual disabling of the irq is short enough to
cause the interrupt to fall when unexpected in some cases, or to arrive
after beeing masked (more frequent with IDE). I've seen this behaviour
with the old pmac pic (which was known to be slow). I used to have tons
of bogus interrupts on the ibook & G4 (OpenPIC based), and I got rid of
most of them by adding this code and by implementing an ack_and_mask
function that actually masks the interrupt (which is, I think, not done
on CHRP). I still get some when PMU/ADB doing synchronous requests however.


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: Latest OpenPic changes

[Gabriel Paubert]

On Fri, 11 Feb 2000, Benjamin Herrenschmidt wrote:

> I read back the interrupt until it's actually changed in the controller
> for the simple reason that if I don't do that, I sometimes get bogus
> interrupts. It looks like the calling path from driver code calling

I agree for the disable case, although I'm testing a different solution
with only an isync right now, no sync, not even an eieio. But for the
enable case it should not be necessary (Geert's snippet was
enable_irq).

Admittedly we have different machines since I have never put a single sync
on the OpenPIC accesses in my tree and I have a 3 months uptime on 2.2.12
with one spurious interrupt systematically just after initialization on
Raven chipset (but not on the followup chip called Hawk with exactly the
same code) which I never bothered to track down (it's perhaps even a HW
bug, obviously I don't care).

> disable_irq() and the actual disabling of the irq is short enough to
> cause the interrupt to fall when unexpected in some cases, or to arrive
> after beeing masked (more frequent with IDE). I've seen this behaviour

If it's truly the case we should consider a different solution. Masking
interrupts in a device or controller for a very short lapse is dominated
by the HW access overhead, and this is bad. sync instructions are also
extremely bad.

> with the old pmac pic (which was known to be slow). I used to have tons
> of bogus interrupts on the ibook & G4 (OpenPIC based), and I got rid of
> most of them by adding this code and by implementing an ack_and_mask
> function that actually masks the interrupt (which is, I think, not done
> on CHRP). I still get some when PMU/ADB doing synchronous requests however.
>
>
>
>
>
>


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: Latest OpenPic changes

[Benjamin Herrenschmidt]

On Fri, Feb 11, 2000, Gabriel Paubert <paubert@iram.es> wrote:

>If it's truly the case we should consider a different solution. Masking
>interrupts in a device or controller for a very short lapse is dominated
>by the HW access overhead, and this is bad. sync instructions are also
>extremely bad.

I implemented it in both enable and disable for "correctness" so that the
status of the controller is guaranteed when exiting any of those 2
functions. I agree that the unmask case may not be important.

I beleive the sync here can also be completely removed too. A single
eieio is plenty enough because of the read. Isn't it ?

Ben.


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: Latest OpenPic changes

[Gabriel Paubert]

On Fri, 11 Feb 2000, Benjamin Herrenschmidt wrote:

> I implemented it in both enable and disable for "correctness" so that the
> status of the controller is guaranteed when exiting any of those 2
> functions. I agree that the unmask case may not be important.

Well, try and report. In my case I have never seen a spurious interrupt
but the OpenPIC is inside the host bridge, which means that the accesses
do not necessarily have to wait for the PCI bus to be free. But in the
enable case it should not matter.

There is some confusion right now in the openpic code, the places where
we use vectors and the places where we use interrupt source numbers should
be clarified, although we could also say that vector is always source+16
to leave room for a cascaded 8259 pair just in case. Adding a global
variable for this as it is right now seems overkill, not to say bloat.

> I beleive the sync here can also be completely removed too. A single
> eieio is plenty enough because of the read. Isn't it ?

It should but you should wait for the read to finish when you mask the
irq source, which is not obvious with longer pipelines and memory queues
like the 7400. eieio does not guarantee this, isync neither, but I think
that you can force isync to actually wait for the read by inserting some
instruction which uses the read value.

Actually what I have now in my tree is an additioonal function called
openpic_flush(register) which is a poor name admittedly and
openpic_disable_irq does the following:

void openpic_disable_irq(u_int irq)
{
	check_arg_irq(irq);
	openpic_setfield(&ISU[irq - open_pic_irq_offset].Vector_Priority,
			 OPENPIC_MASK);
	/* make sure mask gets to controller before we return to user */
	openpic_flush(&ISU[irq - open_pic_irq_offset].Vector_Priority);
}

and openpic_flush  is:

static void inline openpic_flush(volatile u_int *addr)
{
	unsigned junk;
	asm __volatile__ ("lwz%U1%X1 %0,%1; twllti %0,0; isync" :
			  "=r" (junk) : "m" (*addr));
}

Some background on why I was experimenting with this from the programming
environment manual:

4.1.5.1 Context Synchronizing Instructions

The System Call (sc), Return from Interrupt (rfi), Return from Interrupt
Double Word (rfid), and Instruction Synchronize (isync) instructions
perform context synchronization by allowing previously issued instructions
to complete before performing a context switch. Execution of one of these
instructions ensures the following:

1. No higher priority exception exists (sc) and instruction dispatching is
halted.

2. All previous instructions have completed to a point where they can no
longer cause an exception. [...]
^^^^^^^^^^^^^^^^^^^^^^^^^

That's the point: actually a read followed by an isync is insufficient
since it does not guarantee that the access is performed on the bus (and
could still cause a machine check for example): the only thing it
guarantees is that the translation is valid (context synchronization just
in case you change a mapping or the MSR DR bit just after).

Then the conditional trap uses the result of the load (and actually never
traps since I check for unsigned less than zero ;-)), but it has to wait
for the memory access to be performed before the instructions past the
isync can be (re-)fetched and initiated because it has to determine
whether the connditional trap will actually cause an exception or not.

I consider higly unlikely the chip designers add a short circuit for this
special (apparently useless) conditional trap case since any compiler
should remove such a check in the first place :-). But if they would a
slightly more complex trick would force the check to be performed anyhow.

There is still something wrong in the boot logs:
Memory: 30420k available (196k kernel code, 168k data, 44k init)
                          ^^^
and [root@vdevel linux-test]# cat /proc/meminfo
        total:    used:    free:  shared: buffers:  cached:
Mem:  31338496 26034176  5304320  5382144        0 19341312
Swap:        0        0        0
MemTotal:     30604 kB
MemFree:       5180 kB
MemShared:     5256 kB
Buffers:          0 kB
Cached:       18888 kB
HighTotal: 100533296 kB
HighFree:  101974016 kB
LowTotal:  4194464604 kB
LowFree:   4192998460 kB
SwapTotal:        0 kB
SwapFree:         0 kB

but it does not seem to harm (I checked twice and no, I don't have 4 Tb of
RAM).

I've just recompiled the whole 2.3.43 on an NFSRooted machine running with
this interrupt handling and it works fine right now. See how long it
lasts, oh and I can't load any module (perhaps I have to upgrade my
modutils).

	Regards,
	Gabriel.

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: Latest OpenPic changes

[BenH]

On Fri, Feb 11, 2000, Gabriel Paubert <paubert@iram.es> wrote:

>There is some confusion right now in the openpic code, the places where
>we use vectors and the places where we use interrupt source numbers should
>be clarified, although we could also say that vector is always source+16
>to leave room for a cascaded 8259 pair just in case. Adding a global
>variable for this as it is right now seems overkill, not to say bloat.

The problem is that PowerMacs with openpic have vectors starting at 0. So
changing that would require fixup of both the PCI interrupts and OF
device tree.

>It should but you should wait for the read to finish when you mask the
>irq source, which is not obvious with longer pipelines and memory queues
>like the 7400. eieio does not guarantee this, isync neither, but I think
>that you can force isync to actually wait for the read by inserting some
>instruction which uses the read value.

But is it really necessary ? We do use the returned value of the read
since we loop. So we are safe. The only thing that could bite us is
eventual speculative accesses done just after the read, but since we have
eieio, we should be protected. isync would be necessary only if the code
itself is changed (or the MMU context), which is not the case.

>That's the point: actually a read followed by an isync is insufficient
>since it does not guarantee that the access is performed on the bus (and
>could still cause a machine check for example): the only thing it
>guarantees is that the translation is valid (context synchronization just
>in case you change a mapping or the MSR DR bit just after).

But since we _use_ the result from the read for the loop in my code, we
know that the read was actually done and the result fetched. We don't
care about cache since we are on non-cachable & guarded space. I think my
code without the mb() should be enough. I see no cases were we would use
disable_irq or enable_irq and change MSR_DR/IR just after, and the places
were we change them are usually protected with sync/isync already.


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/