Details

Reviewers

nwhitehorn
mmel
adrian

Group Reviewers

MIPS

Commits

rS305527: Fix MIPS INTRNG (both FDT and non-FDT) behaviour broken by r304459

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 - subversion

Lint

Lint Not Applicable

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 - subversion.

sgalabov added a project: MIPS.

Herald added a subscriber: imp. · View Herald TranscriptAug 29 2016, 9:05 AM

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

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

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

In D7692#160205, @adrian wrote:

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.

Provide more diff context

This revision now requires review to proceed.Aug 31 2016, 6:36 AM

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>

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

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

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

In D7692#160754, @adrian wrote:

(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 :-)

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.

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...

Closed by commit rS305527: Fix MIPS INTRNG (both FDT and non-FDT) behaviour broken by r304459 (authored by sgalabov). · Explain WhySep 7 2016, 9:31 AM

This revision was automatically updated to reflect the committed changes.

sgalabov mentioned this in rS305527: Fix MIPS INTRNG (both FDT and non-FDT) behaviour broken by r304459.

Thank you! Already on github: https://github.com/freebsd/freebsd/commit/2a1c75d8f2f60bf398ad401e6cde49562f938564. Time to rebase ;)

landonf mentioned this in D12385: Preemptively perform intr_map_irq() on non-FDT MIPS targets..Sep 15 2017, 7:50 PM

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 20133

head/sys/mips/include/intr.h

head/sys/mips/mips/mips_pic.c

head/sys/mips/mips/nexus.c

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

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 20133

head/sys/mips/include/intr.h

head/sys/mips/mips/mips_pic.c

head/sys/mips/mips/nexus.c

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

Revision Contents
Changeset List