| Message ID | 20180622151432.1566-7-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 22/06/18 16:14, Miquel Raynal wrote: > Before splitting the code to support multiple platform devices to > be probed (one for the ICU, one per interrupt group), let's switch to > regmap first by creating one in the ->probe(). What's the benefit of doing so? I assume that has to do with supporting multiple devices that share an MMIO range? > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/irqchip/irq-mvebu-icu.c | 45 +++++++++++++++++++++++++++-------------- > 1 file changed, 30 insertions(+), 15 deletions(-) > > diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c > index 0f2655d7f19e..3694c0d73c0d 100644 > --- a/drivers/irqchip/irq-mvebu-icu.c > +++ b/drivers/irqchip/irq-mvebu-icu.c > @@ -18,6 +18,8 @@ > #include <linux/of_irq.h> > #include <linux/of_platform.h> > #include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/mfd/syscon.h> > > #include <dt-bindings/interrupt-controller/mvebu-icu.h> > > @@ -38,7 +40,7 @@ > > struct mvebu_icu { > struct irq_chip irq_chip; > - void __iomem *base; > + struct regmap *regmap; > struct irq_domain *domain; > struct device *dev; > atomic_t initialized; > @@ -56,10 +58,10 @@ static void mvebu_icu_init(struct mvebu_icu *icu, struct msi_msg *msg) > return; > > /* Set Clear/Set ICU SPI message address in AP */ > - writel_relaxed(msg[0].address_hi, icu->base + ICU_SETSPI_NSR_AH); > - writel_relaxed(msg[0].address_lo, icu->base + ICU_SETSPI_NSR_AL); > - writel_relaxed(msg[1].address_hi, icu->base + ICU_CLRSPI_NSR_AH); > - writel_relaxed(msg[1].address_lo, icu->base + ICU_CLRSPI_NSR_AL); > + regmap_write(icu->regmap, ICU_SETSPI_NSR_AH, msg[0].address_hi); > + regmap_write(icu->regmap, ICU_SETSPI_NSR_AL, msg[0].address_lo); > + regmap_write(icu->regmap, ICU_CLRSPI_NSR_AH, msg[1].address_hi); > + regmap_write(icu->regmap, ICU_CLRSPI_NSR_AL, msg[1].address_lo); Isn't this a change in the way we write things to the MMIO registers? You're now trading a writel_relaxed for a writel, plus some locking... Talking about which: Are you always in a context where you can take that lock? The bit of documentation I've just read seems to imply that the default lock is a mutex. Is that always safe? My guess is that it isn't, and any callback that can end-up here after having taken something like the desc lock is going to blow in your face. Have you tried lockdep? > } > > static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg) > @@ -82,7 +84,7 @@ static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg) > icu_int = 0; > } > > - writel_relaxed(icu_int, icu->base + ICU_INT_CFG(d->hwirq)); > + regmap_write(icu->regmap, ICU_INT_CFG(d->hwirq), icu_int); > > /* > * The SATA unit has 2 ports, and a dedicated ICU entry per > @@ -94,10 +96,10 @@ static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg) > * configured (regardless of which port is actually in use). > */ > if (d->hwirq == ICU_SATA0_ICU_ID || d->hwirq == ICU_SATA1_ICU_ID) { > - writel_relaxed(icu_int, > - icu->base + ICU_INT_CFG(ICU_SATA0_ICU_ID)); > - writel_relaxed(icu_int, > - icu->base + ICU_INT_CFG(ICU_SATA1_ICU_ID)); > + regmap_write(icu->regmap, ICU_INT_CFG(ICU_SATA0_ICU_ID), > + icu_int); > + regmap_write(icu->regmap, ICU_INT_CFG(ICU_SATA1_ICU_ID), > + icu_int); > } > } > > @@ -204,12 +206,20 @@ static const struct irq_domain_ops mvebu_icu_domain_ops = { > .free = mvebu_icu_irq_domain_free, > }; > > +static struct regmap_config mvebu_icu_regmap_config = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > + .name = "mvebu_icu", > +}; > + > static int mvebu_icu_probe(struct platform_device *pdev) > { > struct mvebu_icu *icu; > struct device_node *node = pdev->dev.of_node; > struct device_node *gicp_dn; > struct resource *res; > + void __iomem *regs; > int i; > > icu = devm_kzalloc(&pdev->dev, sizeof(struct mvebu_icu), > @@ -220,12 +230,17 @@ static int mvebu_icu_probe(struct platform_device *pdev) > icu->dev = &pdev->dev; > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - icu->base = devm_ioremap_resource(&pdev->dev, res); > - if (IS_ERR(icu->base)) { > + regs = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(regs)) { > dev_err(&pdev->dev, "Failed to map icu base address.\n"); > - return PTR_ERR(icu->base); > + return PTR_ERR(regs); > } > > + icu->regmap = devm_regmap_init_mmio(icu->dev, regs, > + &mvebu_icu_regmap_config); > + if (IS_ERR(icu->regmap)) > + return PTR_ERR(icu->regmap); > + > icu->irq_chip.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, > "ICU.%x", > (unsigned int)res->start); > @@ -260,11 +275,11 @@ static int mvebu_icu_probe(struct platform_device *pdev) > for (i = 0 ; i < ICU_MAX_IRQS ; i++) { > u32 icu_int, icu_grp; > > - icu_int = readl_relaxed(icu->base + ICU_INT_CFG(i)); > + regmap_read(icu->regmap, ICU_INT_CFG(i), &icu_int); > icu_grp = icu_int >> ICU_GROUP_SHIFT; > > if (icu_grp == ICU_GRP_NSR) > - writel_relaxed(0x0, icu->base + ICU_INT_CFG(i)); > + regmap_write(icu->regmap, ICU_INT_CFG(i), 0); > } > > icu->domain = > Thanks, M.
Hi Marc, Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 28 Jun 2018 13:05:05 +0100: > On 22/06/18 16:14, Miquel Raynal wrote: > > Before splitting the code to support multiple platform devices to > > be probed (one for the ICU, one per interrupt group), let's switch to > > regmap first by creating one in the ->probe(). > > What's the benefit of doing so? I assume that has to do with supporting > multiple devices that share an MMIO range? Yes, the ICU subnodes will share the same MMIO range. > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/irqchip/irq-mvebu-icu.c | 45 +++++++++++++++++++++++++++-------------- > > 1 file changed, 30 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c > > index 0f2655d7f19e..3694c0d73c0d 100644 > > --- a/drivers/irqchip/irq-mvebu-icu.c > > +++ b/drivers/irqchip/irq-mvebu-icu.c > > @@ -18,6 +18,8 @@ > > #include <linux/of_irq.h> > > #include <linux/of_platform.h> > > #include <linux/platform_device.h> > > +#include <linux/regmap.h> > > +#include <linux/mfd/syscon.h> > > > > #include <dt-bindings/interrupt-controller/mvebu-icu.h> > > > > @@ -38,7 +40,7 @@ > > > > struct mvebu_icu { > > struct irq_chip irq_chip; > > - void __iomem *base; > > + struct regmap *regmap; > > struct irq_domain *domain; > > struct device *dev; > > atomic_t initialized; > > @@ -56,10 +58,10 @@ static void mvebu_icu_init(struct mvebu_icu *icu, struct msi_msg *msg) > > return; > > > > /* Set Clear/Set ICU SPI message address in AP */ > > - writel_relaxed(msg[0].address_hi, icu->base + ICU_SETSPI_NSR_AH); > > - writel_relaxed(msg[0].address_lo, icu->base + ICU_SETSPI_NSR_AL); > > - writel_relaxed(msg[1].address_hi, icu->base + ICU_CLRSPI_NSR_AH); > > - writel_relaxed(msg[1].address_lo, icu->base + ICU_CLRSPI_NSR_AL); > > + regmap_write(icu->regmap, ICU_SETSPI_NSR_AH, msg[0].address_hi); > > + regmap_write(icu->regmap, ICU_SETSPI_NSR_AL, msg[0].address_lo); > > + regmap_write(icu->regmap, ICU_CLRSPI_NSR_AH, msg[1].address_hi); > > + regmap_write(icu->regmap, ICU_CLRSPI_NSR_AL, msg[1].address_lo); > > Isn't this a change in the way we write things to the MMIO registers? > You're now trading a writel_relaxed for a writel, plus some locking... Is the "writel_relaxed" -> "writel" thing really an issue? > > Talking about which: Are you always in a context where you can take that > lock? The bit of documentation I've just read seems to imply that the > default lock is a mutex. Is that always safe? My guess is that it isn't, > and any callback that can end-up here after having taken something like > the desc lock is going to blow in your face. > > Have you tried lockdep? Just did -- thanks for pointing it, it failed once the overheat interrupt fired. I'm not sure if it is because of the regmap-locking mechanism. There is definitely something to fix there, but I don't know what for now; I'll come back on it. [for reference: logs below] Thanks, Miquèl --- [ 91.128615] [ 91.130120] ================================ [ 91.134408] WARNING: inconsistent lock state [ 91.138698] 4.18.0-rc1-00047-g7f7c805c2b69-dirty #1580 Not tainted [ 91.144905] -------------------------------- [ 91.149193] inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage. [ 91.155227] swapper/0/0 [HC1[1]:SC0[0]:HE0:SE1] takes: [ 91.160388] (____ptrval____) (&tz->lock){?.+.}, at: thermal_zone_get_temp+0x60/0x158 [ 91.168186] {HARDIRQ-ON-W} state was registered at: [ 91.173089] lock_acquire+0x44/0x60 [ 91.176683] __mutex_lock+0x74/0x880 [ 91.180362] mutex_lock_nested+0x1c/0x28 [ 91.184390] thermal_zone_of_sensor_register+0x198/0x250 [ 91.189812] devm_thermal_zone_of_sensor_register+0x5c/0xc0 [ 91.195498] armada_thermal_probe+0x220/0x5f0 [ 91.199963] platform_drv_probe+0x50/0xb0 [ 91.204078] driver_probe_device+0x224/0x308 [ 91.208454] __driver_attach+0xe8/0xf0 [ 91.212309] bus_for_each_dev+0x68/0xc8 [ 91.216248] driver_attach+0x20/0x28 [ 91.219926] bus_add_driver+0x108/0x228 [ 91.223866] driver_register+0x60/0x110 [ 91.227806] __platform_driver_register+0x44/0x50 [ 91.232622] armada_thermal_driver_init+0x18/0x20 [ 91.237436] do_one_initcall+0x58/0x170 [ 91.241378] kernel_init_freeable+0x1d4/0x27c [ 91.245843] kernel_init+0x10/0x108 [ 91.249434] ret_from_fork+0x10/0x18 [ 91.253111] irq event stamp: 61312 [ 91.256530] hardirqs last enabled at (61309): [<ffff000008086ab0>] arch_cpu_idle+0x10/0x20 [ 91.264918] hardirqs last disabled at (61310): [<ffff000008083134>] el1_irq+0x74/0x130 [ 91.272871] softirqs last enabled at (61312): [<ffff0000080b7fa0>] _local_bh_enable+0x20/0x40 [ 91.281520] softirqs last disabled at (61311): [<ffff0000080b8398>] irq_enter+0x58/0x78 [ 91.289559] [ 91.289559] other info that might help us debug this: [ 91.296114] Possible unsafe locking scenario: [ 91.296114] [ 91.302058] CPU0 [ 91.304514] ---- [ 91.306970] lock(&tz->lock); [ 91.310041] <Interrupt> [ 91.312670] lock(&tz->lock); [ 91.315916] [ 91.315916] *** DEADLOCK *** [ 91.315916] [ 91.321863] no locks held by swapper/0/0. [ 91.325890] [ 91.325890] stack backtrace: [ 91.330269] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.18.0-rc1-00047-g7f7c805c2b69-dirty #1580 [ 91.339091] Hardware name: Marvell Armada 7040 DB board (DT) [ 91.344775] Call trace: [ 91.347234] dump_backtrace+0x0/0x1a0 [ 91.350913] show_stack+0x14/0x20 [ 91.354244] dump_stack+0xb8/0xf4 [ 91.357574] print_usage_bug.part.24+0x264/0x27c [ 91.362211] mark_lock+0x150/0x6b0 [ 91.365628] __lock_acquire+0x6d0/0x18f8 [ 91.369568] lock_acquire+0x44/0x60 [ 91.373073] __mutex_lock+0x74/0x880 [ 91.376666] mutex_lock_nested+0x1c/0x28 [ 91.380606] thermal_zone_get_temp+0x60/0x158 [ 91.384982] thermal_zone_device_update.part.4+0x34/0xe0 [ 91.390318] thermal_zone_device_update+0x28/0x38 [ 91.395043] armada_overheat_isr+0xb0/0xb8 [ 91.399159] __handle_irq_event_percpu+0x9c/0x128 [ 91.403883] handle_irq_event_percpu+0x34/0x88 [ 91.408346] handle_irq_event+0x48/0x78 [ 91.412201] handle_fasteoi_irq+0xa0/0x180 [ 91.416316] generic_handle_irq+0x24/0x38 [ 91.420346] mvebu_sei_handle_cascade_irq+0xc8/0x180 [ 91.425332] generic_handle_irq+0x24/0x38 [ 91.429360] __handle_domain_irq+0x5c/0xb8 [ 91.433473] gic_handle_irq+0x58/0xb0 [ 91.437151] el1_irq+0xb4/0x130 [ 91.440306] arch_cpu_idle+0x14/0x20 [ 91.443898] default_idle_call+0x18/0x30 [ 91.447840] do_idle+0x1c4/0x278 [ 91.451082] cpu_startup_entry+0x24/0x28 [ 91.455023] rest_init+0x254/0x268 [ 91.458440] start_kernel+0x3f0/0x41c [ 91.462126] thermal thermal_zone5: critical temperature reached (54 C), shutting down
On Fri, 29 Jun 2018 16:27:51 +0100, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Marc, > > Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 28 Jun 2018 13:05:05 > +0100: > > > On 22/06/18 16:14, Miquel Raynal wrote: > > > Before splitting the code to support multiple platform devices to > > > be probed (one for the ICU, one per interrupt group), let's switch to > > > regmap first by creating one in the ->probe(). > > > > What's the benefit of doing so? I assume that has to do with supporting > > multiple devices that share an MMIO range? > > Yes, the ICU subnodes will share the same MMIO range. So, one MMIO range, shared by multiple devices managed by the same driver. Why the complexity? > > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > drivers/irqchip/irq-mvebu-icu.c | 45 +++++++++++++++++++++++++++-------------- > > > 1 file changed, 30 insertions(+), 15 deletions(-) > > > > > > diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c > > > index 0f2655d7f19e..3694c0d73c0d 100644 > > > --- a/drivers/irqchip/irq-mvebu-icu.c > > > +++ b/drivers/irqchip/irq-mvebu-icu.c > > > @@ -18,6 +18,8 @@ > > > #include <linux/of_irq.h> > > > #include <linux/of_platform.h> > > > #include <linux/platform_device.h> > > > +#include <linux/regmap.h> > > > +#include <linux/mfd/syscon.h> > > > > > > #include <dt-bindings/interrupt-controller/mvebu-icu.h> > > > > > > @@ -38,7 +40,7 @@ > > > > > > struct mvebu_icu { > > > struct irq_chip irq_chip; > > > - void __iomem *base; > > > + struct regmap *regmap; > > > struct irq_domain *domain; > > > struct device *dev; > > > atomic_t initialized; > > > @@ -56,10 +58,10 @@ static void mvebu_icu_init(struct mvebu_icu *icu, struct msi_msg *msg) > > > return; > > > > > > /* Set Clear/Set ICU SPI message address in AP */ > > > - writel_relaxed(msg[0].address_hi, icu->base + ICU_SETSPI_NSR_AH); > > > - writel_relaxed(msg[0].address_lo, icu->base + ICU_SETSPI_NSR_AL); > > > - writel_relaxed(msg[1].address_hi, icu->base + ICU_CLRSPI_NSR_AH); > > > - writel_relaxed(msg[1].address_lo, icu->base + ICU_CLRSPI_NSR_AL); > > > + regmap_write(icu->regmap, ICU_SETSPI_NSR_AH, msg[0].address_hi); > > > + regmap_write(icu->regmap, ICU_SETSPI_NSR_AL, msg[0].address_lo); > > > + regmap_write(icu->regmap, ICU_CLRSPI_NSR_AH, msg[1].address_hi); > > > + regmap_write(icu->regmap, ICU_CLRSPI_NSR_AL, msg[1].address_lo); > > > > Isn't this a change in the way we write things to the MMIO registers? > > You're now trading a writel_relaxed for a writel, plus some locking... > > Is the "writel_relaxed" -> "writel" thing really an issue? If you're happy with system-wide barriers (dsb sy) being issued, synchronising unrelated accesses, and generally slowing down the whole system in a completely unnecessary way, then there is absolutely no issue whatsoever. Performance is completely overrated anyway, let's embrace slow computing. > > > > > Talking about which: Are you always in a context where you can take that > > lock? The bit of documentation I've just read seems to imply that the > > default lock is a mutex. Is that always safe? My guess is that it isn't, > > and any callback that can end-up here after having taken something like > > the desc lock is going to blow in your face. > > > > Have you tried lockdep? > > Just did -- thanks for pointing it, it failed once the overheat > interrupt fired. I'm not sure if it is because of the regmap-locking > mechanism. There is definitely something to fix there, but I don't know > what for now; I'll come back on it. Well, that's interesting: > [ 91.376666] mutex_lock_nested+0x1c/0x28 > [ 91.380606] thermal_zone_get_temp+0x60/0x158 > [ 91.384982] thermal_zone_device_update.part.4+0x34/0xe0 > [ 91.390318] thermal_zone_device_update+0x28/0x38 > [ 91.395043] armada_overheat_isr+0xb0/0xb8 > [ 91.399159] __handle_irq_event_percpu+0x9c/0x128 > [ 91.403883] handle_irq_event_percpu+0x34/0x88 > [ 91.408346] handle_irq_event+0x48/0x78 > [ 91.412201] handle_fasteoi_irq+0xa0/0x180 > [ 91.416316] generic_handle_irq+0x24/0x38 > [ 91.420346] mvebu_sei_handle_cascade_irq+0xc8/0x180 > [ 91.425332] generic_handle_irq+0x24/0x38 > [ 91.429360] __handle_domain_irq+0x5c/0xb8 > [ 91.433473] gic_handle_irq+0x58/0xb0 > [ 91.437151] el1_irq+0xb4/0x130 Taking a mutex in an interrupt handler is... great! On the other hand, that armada_overheat_isr function doesn't seem to exist in 4.18-rc2, so that's absolutely fine. Nonetheless, regmap usage in this context is still suspect, and my gut feeling is that you really don't need it at all. Thanks, M.
Hi Marc, Marc Zyngier <marc.zyngier@arm.com> wrote on Fri, 29 Jun 2018 18:17:21 +0100: > On Fri, 29 Jun 2018 16:27:51 +0100, > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Marc, > > > > Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 28 Jun 2018 13:05:05 > > +0100: > > > > > On 22/06/18 16:14, Miquel Raynal wrote: > > > > Before splitting the code to support multiple platform devices to > > > > be probed (one for the ICU, one per interrupt group), let's switch to > > > > regmap first by creating one in the ->probe(). > > > > > > What's the benefit of doing so? I assume that has to do with supporting > > > multiple devices that share an MMIO range? > > > > Yes, the ICU subnodes will share the same MMIO range. > > So, one MMIO range, shared by multiple devices managed by the same > driver. Why the complexity? Mmmmh... one point :) > > > > > > > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > --- > > > > drivers/irqchip/irq-mvebu-icu.c | 45 +++++++++++++++++++++++++++-------------- > > > > 1 file changed, 30 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c > > > > index 0f2655d7f19e..3694c0d73c0d 100644 > > > > --- a/drivers/irqchip/irq-mvebu-icu.c > > > > +++ b/drivers/irqchip/irq-mvebu-icu.c > > > > @@ -18,6 +18,8 @@ > > > > #include <linux/of_irq.h> > > > > #include <linux/of_platform.h> > > > > #include <linux/platform_device.h> > > > > +#include <linux/regmap.h> > > > > +#include <linux/mfd/syscon.h> > > > > > > > > #include <dt-bindings/interrupt-controller/mvebu-icu.h> > > > > > > > > @@ -38,7 +40,7 @@ > > > > > > > > struct mvebu_icu { > > > > struct irq_chip irq_chip; > > > > - void __iomem *base; > > > > + struct regmap *regmap; > > > > struct irq_domain *domain; > > > > struct device *dev; > > > > atomic_t initialized; > > > > @@ -56,10 +58,10 @@ static void mvebu_icu_init(struct mvebu_icu *icu, struct msi_msg *msg) > > > > return; > > > > > > > > /* Set Clear/Set ICU SPI message address in AP */ > > > > - writel_relaxed(msg[0].address_hi, icu->base + ICU_SETSPI_NSR_AH); > > > > - writel_relaxed(msg[0].address_lo, icu->base + ICU_SETSPI_NSR_AL); > > > > - writel_relaxed(msg[1].address_hi, icu->base + ICU_CLRSPI_NSR_AH); > > > > - writel_relaxed(msg[1].address_lo, icu->base + ICU_CLRSPI_NSR_AL); > > > > + regmap_write(icu->regmap, ICU_SETSPI_NSR_AH, msg[0].address_hi); > > > > + regmap_write(icu->regmap, ICU_SETSPI_NSR_AL, msg[0].address_lo); > > > > + regmap_write(icu->regmap, ICU_CLRSPI_NSR_AH, msg[1].address_hi); > > > > + regmap_write(icu->regmap, ICU_CLRSPI_NSR_AL, msg[1].address_lo); > > > > > > Isn't this a change in the way we write things to the MMIO registers? > > > You're now trading a writel_relaxed for a writel, plus some locking... > > > > Is the "writel_relaxed" -> "writel" thing really an issue? > > If you're happy with system-wide barriers (dsb sy) being issued, > synchronising unrelated accesses, and generally slowing down the whole > system in a completely unnecessary way, then there is absolutely no > issue whatsoever. Performance is completely overrated anyway, let's > embrace slow computing. 8-D > > > > > > > > > Talking about which: Are you always in a context where you can take that > > > lock? The bit of documentation I've just read seems to imply that the > > > default lock is a mutex. Is that always safe? My guess is that it isn't, > > > and any callback that can end-up here after having taken something like > > > the desc lock is going to blow in your face. > > > > > > Have you tried lockdep? > > > > Just did -- thanks for pointing it, it failed once the overheat > > interrupt fired. I'm not sure if it is because of the regmap-locking > > mechanism. There is definitely something to fix there, but I don't know > > what for now; I'll come back on it. > > Well, that's interesting: > > > [ 91.376666] mutex_lock_nested+0x1c/0x28 > > [ 91.380606] thermal_zone_get_temp+0x60/0x158 > > [ 91.384982] thermal_zone_device_update.part.4+0x34/0xe0 > > [ 91.390318] thermal_zone_device_update+0x28/0x38 > > [ 91.395043] armada_overheat_isr+0xb0/0xb8 > > [ 91.399159] __handle_irq_event_percpu+0x9c/0x128 > > [ 91.403883] handle_irq_event_percpu+0x34/0x88 > > [ 91.408346] handle_irq_event+0x48/0x78 > > [ 91.412201] handle_fasteoi_irq+0xa0/0x180 > > [ 91.416316] generic_handle_irq+0x24/0x38 > > [ 91.420346] mvebu_sei_handle_cascade_irq+0xc8/0x180 > > [ 91.425332] generic_handle_irq+0x24/0x38 > > [ 91.429360] __handle_domain_irq+0x5c/0xb8 > > [ 91.433473] gic_handle_irq+0x58/0xb0 > > [ 91.437151] el1_irq+0xb4/0x130 > > Taking a mutex in an interrupt handler is... great! On the other hand, > that armada_overheat_isr function doesn't seem to exist in 4.18-rc2, > so that's absolutely fine. I had no troubles with NSRs so I wanted to check with SEIs too. The only device using SEIs for now is this thermal driver which I just contributed, it is still under review. The ISR is making a thermal-core call to update on an overheat situation. This call is synchronous and the core checks for the current temperature, but first locks its tz->lock mutex, which is kind of bad in interrupt context. I just realized it. Sad. > > Nonetheless, regmap usage in this context is still suspect, and my gut > feeling is that you really don't need it at all. You are right on this and I probably over-looked at the initial problem. I'm pretty sure I can get rid of it as long as only one driver touches this MMIO region. Thanks for all your thoughts, I'll resend a version next week. Thank you very much, Miquèl
diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c index 0f2655d7f19e..3694c0d73c0d 100644 --- a/drivers/irqchip/irq-mvebu-icu.c +++ b/drivers/irqchip/irq-mvebu-icu.c @@ -18,6 +18,8 @@ #include <linux/of_irq.h> #include <linux/of_platform.h> #include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/mfd/syscon.h> #include <dt-bindings/interrupt-controller/mvebu-icu.h> @@ -38,7 +40,7 @@ struct mvebu_icu { struct irq_chip irq_chip; - void __iomem *base; + struct regmap *regmap; struct irq_domain *domain; struct device *dev; atomic_t initialized; @@ -56,10 +58,10 @@ static void mvebu_icu_init(struct mvebu_icu *icu, struct msi_msg *msg) return; /* Set Clear/Set ICU SPI message address in AP */ - writel_relaxed(msg[0].address_hi, icu->base + ICU_SETSPI_NSR_AH); - writel_relaxed(msg[0].address_lo, icu->base + ICU_SETSPI_NSR_AL); - writel_relaxed(msg[1].address_hi, icu->base + ICU_CLRSPI_NSR_AH); - writel_relaxed(msg[1].address_lo, icu->base + ICU_CLRSPI_NSR_AL); + regmap_write(icu->regmap, ICU_SETSPI_NSR_AH, msg[0].address_hi); + regmap_write(icu->regmap, ICU_SETSPI_NSR_AL, msg[0].address_lo); + regmap_write(icu->regmap, ICU_CLRSPI_NSR_AH, msg[1].address_hi); + regmap_write(icu->regmap, ICU_CLRSPI_NSR_AL, msg[1].address_lo); } static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg) @@ -82,7 +84,7 @@ static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg) icu_int = 0; } - writel_relaxed(icu_int, icu->base + ICU_INT_CFG(d->hwirq)); + regmap_write(icu->regmap, ICU_INT_CFG(d->hwirq), icu_int); /* * The SATA unit has 2 ports, and a dedicated ICU entry per @@ -94,10 +96,10 @@ static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg) * configured (regardless of which port is actually in use). */ if (d->hwirq == ICU_SATA0_ICU_ID || d->hwirq == ICU_SATA1_ICU_ID) { - writel_relaxed(icu_int, - icu->base + ICU_INT_CFG(ICU_SATA0_ICU_ID)); - writel_relaxed(icu_int, - icu->base + ICU_INT_CFG(ICU_SATA1_ICU_ID)); + regmap_write(icu->regmap, ICU_INT_CFG(ICU_SATA0_ICU_ID), + icu_int); + regmap_write(icu->regmap, ICU_INT_CFG(ICU_SATA1_ICU_ID), + icu_int); } } @@ -204,12 +206,20 @@ static const struct irq_domain_ops mvebu_icu_domain_ops = { .free = mvebu_icu_irq_domain_free, }; +static struct regmap_config mvebu_icu_regmap_config = { + .reg_bits = 32, + .val_bits = 32, + .reg_stride = 4, + .name = "mvebu_icu", +}; + static int mvebu_icu_probe(struct platform_device *pdev) { struct mvebu_icu *icu; struct device_node *node = pdev->dev.of_node; struct device_node *gicp_dn; struct resource *res; + void __iomem *regs; int i; icu = devm_kzalloc(&pdev->dev, sizeof(struct mvebu_icu), @@ -220,12 +230,17 @@ static int mvebu_icu_probe(struct platform_device *pdev) icu->dev = &pdev->dev; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - icu->base = devm_ioremap_resource(&pdev->dev, res); - if (IS_ERR(icu->base)) { + regs = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(regs)) { dev_err(&pdev->dev, "Failed to map icu base address.\n"); - return PTR_ERR(icu->base); + return PTR_ERR(regs); } + icu->regmap = devm_regmap_init_mmio(icu->dev, regs, + &mvebu_icu_regmap_config); + if (IS_ERR(icu->regmap)) + return PTR_ERR(icu->regmap); + icu->irq_chip.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ICU.%x", (unsigned int)res->start); @@ -260,11 +275,11 @@ static int mvebu_icu_probe(struct platform_device *pdev) for (i = 0 ; i < ICU_MAX_IRQS ; i++) { u32 icu_int, icu_grp; - icu_int = readl_relaxed(icu->base + ICU_INT_CFG(i)); + regmap_read(icu->regmap, ICU_INT_CFG(i), &icu_int); icu_grp = icu_int >> ICU_GROUP_SHIFT; if (icu_grp == ICU_GRP_NSR) - writel_relaxed(0x0, icu->base + ICU_INT_CFG(i)); + regmap_write(icu->regmap, ICU_INT_CFG(i), 0); } icu->domain =
Before splitting the code to support multiple platform devices to be probed (one for the ICU, one per interrupt group), let's switch to regmap first by creating one in the ->probe(). Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/irqchip/irq-mvebu-icu.c | 45 +++++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 15 deletions(-)