Re: [PATCH v3 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

On 20/06/17 14:46, Thomas Petazzoni wrote:

Hello,

On Mon, 19 Jun 2017 18:40:53 +0100, Marc Zyngier wrote:

quoted

quoted

+	if (msg->address_lo) {  

This should probably test both _lo and _hi.

Not sure what test you want to do on _hi. Since the physical address
I'm using is below the 4 GB boundary, the high bits are all zeroes,
even for a valid address. So to distinguish whether we're configuring
or de-configuring the MSI, I don't see how the address_hi value is
useful.

Am I missing something obvious here?

There is a few things: this driver could (mostly) work with a GICv3
distributor (located way above 4GB) instead of the GICP, and I'd rather
make no assumption of where GICP is located in the memory map.

So I'd rather see:

	if (msg->address_lo || msg->address_hi) {
		[...]
	} else {
		/* deconfiguration case */
	}

[...]

quoted

I think you may want to issue a irq_set_type here, because it is not
completely clear to me if the core code will be doing it by default for
you...

It's not needed I believe. I've added some trace in gic_set_type(), and
it's really called for every ICU interrupt as expected, as soon as the
interrupt is configured. And indeed, if you look at __setup_irq(), it
calls __irq_set_trigger(), see
http://elixir.free-electrons.com/linux/latest/source/kernel/irq/manage.c#L1309.
I've added a dump_stack() in git_set_type() to make sure when I was
getting called for the SPI interrupts corresponding to the GICP/ICU
stuff. Here is one example, from the XHCI driver:

[    1.815712] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 4.12.0-rc1 #613
[    1.822180] Hardware name: Marvell Armada 8040 DB board (DT)
[    1.827863] Call trace:
[    1.830329] [<ffff000008088528>] dump_backtrace+0x0/0x228
[    1.835752] [<ffff00000808881c>] show_stack+0x14/0x20
[    1.840828] [<ffff00000838fd80>] dump_stack+0x90/0xb0
[    1.845903] [<ffff0000083bf13c>] gic_set_type+0x94/0x98
[    1.851154] [<ffff00000810e734>] irq_chip_set_type_parent+0x1c/0x30
[    1.857449] [<ffff00000810e734>] irq_chip_set_type_parent+0x1c/0x30
[    1.863743] [<ffff00000810e734>] irq_chip_set_type_parent+0x1c/0x30
[    1.870037] [<ffff00000810d0a0>] __irq_set_trigger+0x60/0x178
[    1.875808] [<ffff00000810d764>] __setup_irq+0x5ac/0x690
[    1.881143] [<ffff00000810da1c>] request_threaded_irq+0xec/0x1c0
[    1.887177] [<ffff0000086a84dc>] usb_add_hcd+0x50c/0x800
[    1.892513] [<ffff0000087052ec>] xhci_plat_probe+0x584/0x768
[    1.898199] [<ffff00000854cc28>] platform_drv_probe+0x58/0xc0
[    1.903969] [<ffff00000854ad74>] driver_probe_device+0x214/0x2d0
[    1.910002] [<ffff00000854aedc>] __driver_attach+0xac/0xb0
[    1.915511] [<ffff000008548ef8>] bus_for_each_dev+0x60/0xa0
[    1.921107] [<ffff00000854a690>] driver_attach+0x20/0x28
[    1.926442] [<ffff00000854a1e0>] bus_add_driver+0x110/0x230
[    1.932038] [<ffff00000854b858>] driver_register+0x60/0xf8
[    1.937547] [<ffff00000854cb5c>] __platform_driver_register+0x44/0x50
[    1.944019] [<ffff000008d41a60>] xhci_plat_init+0x2c/0x34
[    1.949441] [<ffff0000080830f8>] do_one_initcall+0x38/0x120
[    1.955038] [<ffff000008d00ce8>] kernel_init_freeable+0x198/0x238
[    1.961159] [<ffff0000088fe470>] kernel_init+0x10/0x100
[    1.966406] [<ffff000008082ec0>] ret_from_fork+0x10/0x50

So, whenever you do the request_irq(), __setup_irq() calls
__irq_set_trigger(), which ends in the ICU ->irq_set_type(), calling
the GICP MSI domain ->irq_set_type(), calling the GICP inner domain
->irq_set_type(), itself calling the GIC ->irq_set_type().

Fair enough.

quoted

quoted

+	icu->gicp = platform_get_drvdata(gicp_pdev);
+
+	/* Set Clear/Set ICU SPI message address in AP */
+	setspi = mvebu_gicp_setspi_phys_addr(icu->gicp);  

I must say that I find this quite horrible. The idea of digging into the
internals of another driver and forcing it to blindly dereference a
pointer feels just wrong.

Instead, why don't you directly pass the device node, and kindly ask the
GICP driver to give you the two addresses? Something along the lines of:

	err = mvebu_gicp_get_doorbells(gicp_dn, &setspi, &clrspi);
	if (err)
		[...]

which at least gives a the GICP driver chance to check that this is
something it knows about. And you can then drop the icu->gicp field.

ACK, fixed for the next version.

quoted

quoted

+	/*
+	 * Clean all ICU interrupts with type SPI_NSR, required to
+	 * avoid unpredictable SPI assignments done by firmware.
+	 */
+	for (i = 0 ; i < ICU_MAX_IRQS ; i++) {
+		icu_int = readl(icu->base + ICU_INT_CFG(i));
+		if ((icu_int >> ICU_GROUP_SHIFT) == ICU_GRP_NSR)
+			writel_relaxed(0x0, icu->base + ICU_INT_CFG(i));
+	}  

I had questions about the safety of this in a previous review. Do you
have any update? Also, shouldn't you check that same thing in the
translate callback (so that you detect clashes between firmware and DT)?

I'm still waiting for feedback from Hannah and Yehuda in Cc on this
question. They should answer soon, hopefully.

quoted

Otherwise looking pretty neat.

Thanks again for the review. You can expect v4 today.

OK, thanks.

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html