Page MenuHomeFreeBSD

MIPS: Fix cpu_establish_[hard|soft]intr() post r304459
ClosedPublic

Authored by sgalabov on Aug 29 2016, 9:05 AM.

Details

Summary

MIPS code still uses cpu_establish_[hard|soft]intr() in places to set up interrupts, even in the case of INTRNG (e.g., for MIPS ticker). Post- r304459 this functionality was broken, so MIPS targets using INTRNG couldn't properly attach their clock sources, resulting in panic.

The attached diff, although a bit hacky, allows MIPS to boot properly.

Test Plan

Tested on MT7621, where both ticker and GIC use cpu_establish_hardintr() to set up their interrupt handlers.
Before the patch the board couldn't finish booting at all, while after it it boots fine.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

sgalabov updated this revision to Diff 19787.Aug 29 2016, 9:05 AM
sgalabov retitled this revision from to MIPS: Fix cpu_establish_[hard|soft]intr() post r304459.
sgalabov updated this object.
sgalabov edited the test plan for this revision. (Show Details)
sgalabov added reviewers: adrian, mmel, MIPS.
sgalabov set the repository for this revision to rS FreeBSD src repository.
sgalabov added a project: MIPS.
adrian accepted this revision.Aug 29 2016, 4:03 PM
adrian edited edge metadata.
This revision is now accepted and ready to land.Aug 29 2016, 4:03 PM
sgalabov updated this revision to Diff 19832.Aug 30 2016, 6:45 AM
sgalabov edited edge metadata.

Previous diff was not compiling for BCM kernel config, as BCM only uses INTRNG but not FDT.
Rework the fix, so that cpu_establish_[hard|soft]intr uses a different intr mapping strategy, which doesn't require FDT-specific data to be exposed to non-FDT kernels.

This revision now requires review to proceed.Aug 30 2016, 6:45 AM
adrian accepted this revision.Aug 31 2016, 5:52 AM
adrian edited edge metadata.

Hm, can you get someone who does FDT stuff on #bsdmips to look at this?

This revision is now accepted and ready to land.Aug 31 2016, 5:52 AM

Hm, can you get someone who does FDT stuff on #bsdmips to look at this?

I could try, but this diff doesn't change anything with regards to how interrupts described via FDT are handled - it only takes care of interrupts that are directly established by cpu_establish_[hard|soft]intr().

nathan, what do you think about this? it's fallout from the last intrng change.

sgalabov updated this revision to Diff 19863.Aug 31 2016, 6:36 AM
sgalabov edited edge metadata.

Provide more diff context

This revision now requires review to proceed.Aug 31 2016, 6:36 AM
mizhka added a subscriber: mizhka.Aug 31 2016, 4:13 PM

Hi,

Quick check on Broadcom 4716.

Before it crashed on:

clock0: <Generic MIPS32 ticker> on nexus0
panic: Unable to add hard IRQ 7 handler
KDB: enter: panic
[ thread pid 0 tid 100000 ]
Stopped at      kdb_enter+0x4c: lui     at,0x8049
db> 
db> bt
Tracing pid 0 tid 100000 td 0x80483980
db_trace_thread+30 (?,?,?,?) ra 8000f48c sp 80589820 sz 24
db_stack_trace+114 (0,?,ffffffff,?) ra 8000e9ac sp 80589838 sz 32
db_command+388 (?,?,?,?) ra 8000eb30 sp 80589858 sz 168
db_command_loop+70 (?,?,?,?) ra 800119dc sp 80589900 sz 24
db_trap+f4 (?,?,?,?) ra 801bd970 sp 80589918 sz 424
kdb_trap+100 (?,?,?,?) ra 8036991c sp 80589ac0 sz 48
trap+cec (?,?,?,?) ra 80357a18 sp 80589af0 sz 200
MipsKernGenException+ec (0,4,803c05c8,16b) ra 801bd698 sp 80589bb8 sz 200
kdb_enter+4c (?,?,?,?) ra 8017827c sp 80589c80 sz 24
vpanic+ec (?,?,?,?) ra 801782bc sp 80589c98 sz 32
panic+20 (?,7,803c05c8,118) ra 803739f4 sp 80589cb8 sz 32
cpu_establish_hardintr+cc (?,?,?,?) ra 80372aa0 sp 80589cd8 sz 56
clock_attach+74 (?,?,?,?) ra 801b3d38 sp 80589d10 sz 48
device_attach+480 (?,?,?,?) ra 801b3f30 sp 80589d40 sz 96
device_probe_and_attach+5c (?,?,?,?) ra 801b49bc sp 80589da0 sz 24
bus_generic_new_pass+10c (?,?,?,?) ra 801b49a4 sp 80589db8 sz 40
bus_generic_new_pass+f4 (?,?,?,?) ra 801b1158 sp 80589de0 sz 40
bus_set_pass+c0 (?,?,?,?) ra 801b11cc sp 80589e08 sz 40
root_bus_configure+14 (?,?,?,?) ra 8034ed9c sp 80589e30 sz 24
configure+10 (?,?,?,?) ra 80121684 sp 80589e48 sz 24
mi_startup+138 (?,?,?,?) ra 80001170 sp 80589e60 sz 32
_start+70 (?,?,?,?) ra 0 sp 80589e80 sz 0
pid 0
db> reboot
bcm::platform_reset()

After patch: http://pastebin.com/PttFWDsT

clock0: <Generic MIPS32 ticker> on nexus0
Timecounter "MIPS32" frequency 240000000 Hz quality 800
Event timer "MIPS32" frequency 240000000 Hz quality 800
random: harvesting attach, 8 bytes (4 bits) from clock0
uart0: <16750 or compatible> mem 0x18000300-0x180003ff irq 2 on bhnd_chipc0
uart0: console (115200,n,8,1)
panic: Attempt to copy invalid resource id: 2

KDB: enter: panic
[ thread pid 0 tid 100000 ]
Stopped at      kdb_enter+0x4c: lui     at,0x8049
db> bt
Tracing pid 0 tid 100000 td 0x80485980
db_trace_thread+30 (?,?,?,?) ra 8000f4ac sp 8058b478 sz 24
db_stack_trace+114 (0,?,ffffffff,?) ra 8000e9cc sp 8058b490 sz 32
db_command+388 (?,?,?,?) ra 8000eb50 sp 8058b4b0 sz 168
db_command_loop+70 (?,?,?,?) ra 800119fc sp 8058b558 sz 24
db_trap+f4 (?,?,?,?) ra 801bd990 sp 8058b570 sz 424
kdb_trap+100 (?,?,?,?) ra 8036993c sp 8058b718 sz 48
trap+cec (?,?,?,?) ra 80357a38 sp 8058b748 sz 200
MipsKernGenException+ec (0,4,803c0758,16b) ra 801bd6b8 sp 8058b810 sz 200
kdb_enter+4c (?,?,?,?) ra 8017829c sp 8058b8d8 sz 24
vpanic+ec (?,?,?,?) ra 801782dc sp 8058b8f0 sz 32
panic+20 (?,2,803f0c58,5f6) ra 80374118 sp 8058b910 sz 32
intr_map_copy_map_data+84 (?,?,?,?) ra 803755a0 sp 8058b930 sz 40
intr_activate_irq+98 (?,?,?,?) ra 8035d43c sp 8058b958 sz 56
nexus_activate_resource+d8 (?,?,?,?) ra 801b29c8 sp 8058b990 sz 64
bus_generic_activate_resource+88 (?,?,?,?) ra 801b29c8 sp 8058b9d0 sz 48
bus_generic_activate_resource+88 (?,?,?,?) ra 80020dd4 sp 8058ba00 sz 48
chipc_activate_resource+78 (?,?,?,?) ra 801af940 sp 8058ba30 sz 56
bus_activate_resource+8c (?,?,?,?) ra 8035d9d4 sp 8058ba68 sz 48
nexus_alloc_resource+1c8 (?,?,?,?) ra 801b5910 sp 8058ba98 sz 80
bus_generic_rl_alloc_resource+c8 (?,?,?,?) ra 801b4c84 sp 8058bae8 sz 80
resource_list_alloc+284 (?,?,?,1) ra 801b59b0 sp 8058bb38 sz 88
bus_generic_rl_alloc_resource+168 (?,?,?,?) ra 80021594 sp 8058bb90 sz 80
chipc_alloc_resource+e0 (8082ce00,8082bc80,1,8080904c) ra 801b4eac sp 8058bbe0 sz 96
bus_alloc_resource+b8 (?,?,?,?) ra 8006d03c sp 8058bc40 sz 72
uart_bus_attach+3f4 (?,?,?,?) ra 801b3d58 sp 8058bc88 sz 72
device_attach+480 (?,?,?,?) ra 801b3f50 sp 8058bcd0 sz 96
device_probe_and_attach+5c (?,?,?,?) ra 801b49dc sp 8058bd30 sz 24
bus_generic_new_pass+10c (?,?,?,?) ra 801b49c4 sp 8058bd48 sz 40
bus_generic_new_pass+f4 (?,?,?,?) ra 80019320 sp 8058bd70 sz 40
bhnd_new_pass+24 (?,?,?,?) ra 801b49c4 sp 8058bd98 sz 32
bus_generic_new_pass+f4 (?,?,?,?) ra 801b49c4 sp 8058bdb8 sz 40
bus_generic_new_pass+f4 (?,?,?,?) ra 801b1178 sp 8058bde0 sz 40
bus_set_pass+c0 (?,?,?,?) ra 801b11ec sp 8058be08 sz 40
root_bus_configure+14 (?,?,?,?) ra 8034edbc sp 8058be30 sz 24
configure+10 (?,?,?,?) ra 801216a4 sp 8058be48 sz 24
mi_startup+138 (?,?,?,?) ra 80001190 sp 8058be60 sz 32
_start+70 (?,?,?,?) ra 0 sp 8058be80 sz 0
pid 0
db> show irq
irq0   <sint0>: cpu 00 cnt 0
irq1   <sint1>: cpu 00 cnt 0
irq2   <int0>: cpu 00 cnt 0
irq3   <int1>: cpu 00 cnt 0
irq4   <int2>: cpu 00 cnt 0
irq5   <int3>: cpu 00 cnt 0
irq6   <int4>: cpu 00 cnt 0
irq7   <int5>: cpu 00 cnt 0
irq total 0
db> show intr
swi1: netisr 0 (pid 11) {SOFT}
swi4: clock (0) (pid 11) {SOFT}
swi3: vm (pid 11) {SOFT}
swi5: fast taskq (pid 11) {SOFT}
swi6: Giant taskq (pid 11) {SOFT}
swi6: task queue (pid 11) {SOFT}
int5: cpupic0 (no thread)
db> show rman
db> show rmans
rman 0x804a21b0: Hardware IRQs (0x0-0x5 full range)
rman 0x804a21e0: Memory addresses (0x0-0xffffffff full range)
rman 0x80828250: ChipCommon Device Memory (0x0-0xffffffff full range)
rman 0x80839de8: MIPS PIC IRQs (0x0-0xffffffffffffffff full range)
db> show allrmans
No such command
db> show allrman 
rman 0x804a21b0: Hardware IRQs (0x0-0x5 full range)
    0x0-0x1 (RID=0) ----
    0x2-0x2 (RID=0) (uart0)
    0x3-0x5 (RID=0) ----
rman 0x804a21e0: Memory addresses (0x0-0xffffffff full range)
    0x0-0x17ffffff (RID=0) ----
    0x18000000-0x18000fff (RID=0) (bhnd_chipc0)
    0x18001000-0x18002fff (RID=0) ----
    0x18003000-0x18003fff (RID=0) (bhnd_mips0)
    0x18004000-0x180fffff (RID=0) ----
    0x18100000-0x18100fff (RID=1) (bhnd0)
    0x18101000-0x18101fff (RID=2) (bhnd0)
    0x18102000-0x18102fff (RID=3) (bhnd0)
    0x18103000-0x18103fff (RID=4) (bhnd0)
    0x18104000-0x18104fff (RID=5) (bhnd0)
    0x18105000-0x18105fff (RID=6) (bhnd0)
    0x18106000-0x18107fff (RID=0) ----
    0x18108000-0x18108fff (RID=9) (bhnd0)
    0x18109000-0x1810afff (RID=0) ----
    0x1810b000-0x1810bfff (RID=10) (bhnd0)
    0x1810c000-0x1810cfff (RID=11) (bhnd0)
    0x1810d000-0x1810efff (RID=0) ----
    0x1810f000-0x1810ffff (RID=15) (bhnd0)
    0x18110000-0xffffffff (RID=0) ----
rman 0x80828250: ChipCommon Device Memory (0x0-0xffffffff full range)
    0x18000000-0x180002ff (RID=0) ----
    0x18000300-0x180003ff (RID=0) (uart0)
    0x18000400-0x18000fff (RID=0) ----
    0x1a000000-0x1bffffff (RID=0) ----
    0x1c000000-0x1fbfffff (RID=0) ----
    0x1fc00000-0x1fffffff (RID=0) ----
rman 0x80839de8: MIPS PIC IRQs (0x0-0xffffffffffffffff full range)
    0x0-0x0 (RID=0) (cpupic0)
    0x1-0x1 (RID=0) (cpupic0)
    0x2-0x2 (RID=0) (cpupic0)
    0x3-0x3 (RID=0) (cpupic0)
    0x4-0x4 (RID=0) (cpupic0)
    0x5-0x5 (RID=0) (cpupic0)
    0x6-0x6 (RID=0) (cpupic0)
    0x0-0x0 (RID=0) (cpupic0)
db>
nwhitehorn edited edge metadata.Sep 1 2016, 12:08 AM

This looks good to me.

sys/mips/mips/mips_pic.c
329 ↗(On Diff #19863)

This identifier is a little ugly, but I don't see an alternative.

nwhitehorn accepted this revision.Sep 1 2016, 12:09 AM
nwhitehorn edited edge metadata.
This revision is now accepted and ready to land.Sep 1 2016, 12:09 AM
sgalabov updated this revision to Diff 19921.Sep 1 2016, 8:53 AM
sgalabov edited edge metadata.

The updated diff also fixes additional problems seen on Broadcom MIPS (INTRNG without FDT) as a result of the r304459 changes. The new diff was boot tested by mizhka.

This revision now requires review to proceed.Sep 1 2016, 8:53 AM
adrian accepted this revision.Sep 1 2016, 2:54 PM
adrian edited edge metadata.

ok, cool. commit it and we'll shake out any other issues afterwards. :)

(Eg, it needs a bunch more testing on non-FDT, non-INTRNG platforms, but I bet that's going to be easy..)

This revision is now accepted and ready to land.Sep 1 2016, 2:54 PM

(Eg, it needs a bunch more testing on non-FDT, non-INTRNG platforms, but I bet that's going to be easy..)

Well, the non-FDT, non-INTRNG platforms shouldn't have been affected by this issue in the first place.
The changes to nexus are localized within INTRNG ifdefs (and now within FDT sub-ifdefs as well) and the rest of the files I touched in the above diffs should only be used by INTRNG platforms anyway.

But I agree that more testing should be performed in general :-)

mmel edited edge metadata.Sep 4 2016, 7:21 AM

I’m fine with this patch unless you consider it as quick recovery from damage caused by r304459.
But, current usage of INTRNG on MIPS platform is still little ‘non-standard’ and I thing that MIPS needs more love in this area.

From my point of view, I think that:

  • cpu_establish_softintr() is not used and can be deleted.
  • better place for establish interrupt mapping is nexus_alloc_resource();
  • having separated interrupt mappings creation and interrupt activation/deactivation is more conform with current INTRNG design.

I meant something like:
in mips_pic.c

static u_int
cpu_create_intr_map(int irq)
{
        struct intr_map_data_mips_pic *mips_pic_data;
        intptr_t iparent;

        len = sizeof(*mips_pic_data);
        iparent = pic_xref(pic_sc->pic_dev);

        /* Allocate mips_pic data and fill it in */
        mips_pic_data = (struct intr_map_data_mips_pic *)intr_alloc_map_data(
            INTR_MAP_DATA_PLAT_1, len, M_WAITOK | M_ZERO);
        mips_pic_data->irq = irq;

        /* Get the new irq number */
        return (intr_map_irq(pic_sc->pic_dev, iparent,
            (struct intr_map_data *)mips_pic_data));
}

void
cpu_establish_hardintr(const char *name, driver_filter_t *filt,
    void (*handler)(void*), void *arg, int irq, int flags, void **cookiep)
{
	int res;
	u_int new_irq;

	/*
	 * We have 6 levels, but thats 0 - 5 (not including 6)
	 */
	if (irq < 0 || irq >= NHARD_IRQS)
		panic("%s called for unknown hard intr %d", __func__, irq);

	KASSERT(pic_sc != NULL, ("%s: no pic", __func__));

	irq += NSOFT_IRQS;
	new_irq = cpu_create_intr_map(irq);
	
        /* Adjust the resource accordingly */
        rman_set_start(pic_sc->pic_irqs[irq].res, new_irq);
        rman_set_end(pic_sc->pic_irqs[irq].res, new_irq);
        res = intr_activate_irq(pic_sc->pic_dev, pic_sc->pic_irqs[irq].res));
	if (res != 0)
		panic("Unable to create map for soft IRQ %d", irq);
	
	res = intr_setup_irq(pic_sc->pic_dev, pic_sc->pic_irqs[new_irq].res, filt,
	    handler, arg, flags, cookiep);
	if (res != 0) panic("Unable to add hard IRQ %d handler", irq);
}

and in nexus.c

nexus_alloc_resource(…)
{
	…
	if (isdefault) {
		…
	} else  if (type == SYS_RES_IRQ)) {
		if (start != end || count != 1)
 			panic(“Cannot allocate multiple IRQs”);
		start = cpu_create_intr_map(start);
		end = start;
	}
}

and leave nexus_setup_intr() unchanged.

adrian added a comment.Sep 5 2016, 6:44 PM

does this still need committing?

Yes, I was planning to commit it on Wednesday if I get a chance, but wouldn't mind if someone else does it in the meantime...