Re: [RESEND RFC PATCH v1] arm64: kvm: flush tlbs by range in unmap_stage2_range function
Zhenyu,
On 2020-07-27 15:51, Zhenyu Ye wrote:
Hi Marc,
On 2020/7/26 1:40, Marc Zyngier wrote:
On 2020-07-24 14:43, Zhenyu Ye wrote:
Now in unmap_stage2_range(), we flush tlbs one by one just after the
corresponding pages cleared. However, this may cause some
performance
problems when the unmap range is very large (such as when the vm
migration rollback, this may cause vm downtime too loog).
You keep resending this patch, but you don't give any numbers
that would back your assertion.
I have tested the downtime of vm migration rollback on arm64, and found
the downtime could even take up to 7s. Then I traced the cost of
unmap_stage2_range() and found it could take a maximum of 1.2s. The
vm configuration is as follows (with high memory pressure, the dirty
rate is about 500MB/s):
192
48
This means nothing to me, I'm afraid.
After this patch applied, the cost of unmap_stage2_range() can reduce
to
16ms, and VM downtime can be less than 1s.
The following figure shows a clear comparison:
| vm downtime | cost of unmap_stage2_range()
--+--+--
before change | 7s | 1200 ms
after change | 1s |16 ms
--+--+--
I don't see how you turn a 1.184s reduction into a 6s gain.
Surely there is more to it than what you posted.
+
+ if ((end - start) >= 512 << (PAGE_SHIFT - 12)) {
+ __tlbi(vmalls12e1is);
And what is this magic value based on? You don't even mention in the
commit log that you are taking this shortcut.
If the page num is bigger than 512, flush all tlbs of this vm to avoid
soft lock-ups on large TLB flushing ranges. Just like what the
flush_tlb_range() does.
I'm not sure this is applicable here, and it doesn't mean
this is as good on other systems.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
[tip: irq/urgent] genirq/debugfs: Add missing irqchip flags
The following commit has been merged into the irq/urgent branch of tip:
Commit-ID: aa251fc5b936d3ddb4b4c4b36427eb9aa3347c82
Gitweb:
https://git.kernel.org/tip/aa251fc5b936d3ddb4b4c4b36427eb9aa3347c82
Author:Marc Zyngier
AuthorDate:Sat, 25 Jul 2020 13:30:55 +01:00
Committer: Thomas Gleixner
CommitterDate: Mon, 27 Jul 2020 16:20:40 +02:00
genirq/debugfs: Add missing irqchip flags
Recently introduced irqchip flags lack the corresponding printouts in
debugfs. Add them.
Signed-off-by: Marc Zyngier
Signed-off-by: Thomas Gleixner
Link: https://lkml.kernel.org/r/874kpvydxc.wl-...@kernel.org
---
kernel/irq/debugfs.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/kernel/irq/debugfs.c b/kernel/irq/debugfs.c
index 4f9f844..b95ff5d 100644
--- a/kernel/irq/debugfs.c
+++ b/kernel/irq/debugfs.c
@@ -112,6 +112,7 @@ static const struct irq_bit_descr irqdata_states[] = {
BIT_MASK_DESCR(IRQD_AFFINITY_SET),
BIT_MASK_DESCR(IRQD_SETAFFINITY_PENDING),
BIT_MASK_DESCR(IRQD_AFFINITY_MANAGED),
+ BIT_MASK_DESCR(IRQD_AFFINITY_ON_ACTIVATE),
BIT_MASK_DESCR(IRQD_MANAGED_SHUTDOWN),
BIT_MASK_DESCR(IRQD_CAN_RESERVE),
BIT_MASK_DESCR(IRQD_MSI_NOMASK_QUIRK),
@@ -120,6 +121,10 @@ static const struct irq_bit_descr irqdata_states[] = {
BIT_MASK_DESCR(IRQD_WAKEUP_STATE),
BIT_MASK_DESCR(IRQD_WAKEUP_ARMED),
+
+ BIT_MASK_DESCR(IRQD_DEFAULT_TRIGGER_SET),
+
+ BIT_MASK_DESCR(IRQD_HANDLE_ENFORCE_IRQCTX),
};
static const struct irq_bit_descr irqdesc_states[] = {
Re: [PATCH] irqchip/gic-v4.1: Use GFP_ATOMIC flag in allocate_vpe_l1_table()
On Tue, 30 Jun 2020 21:37:46 +0800, Zenghui Yu wrote: > Booting the latest kernel with DEBUG_ATOMIC_SLEEP=y on a GICv4.1 enabled > box, I get the following kernel splat: > > [0.053766] BUG: sleeping function called from invalid context at > mm/slab.h:567 > [0.053767] in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 0, > name: swapper/1 > [0.053769] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.8.0-rc3+ #23 > [0.053770] Call trace: > [0.053774] dump_backtrace+0x0/0x218 > [0.053775] show_stack+0x2c/0x38 > [0.053777] dump_stack+0xc4/0x10c > [0.053779] ___might_sleep+0xfc/0x140 > [0.053780] __might_sleep+0x58/0x90 > [0.053782] slab_pre_alloc_hook+0x7c/0x90 > [0.053783] kmem_cache_alloc_trace+0x60/0x2f0 > [0.053785] its_cpu_init+0x6f4/0xe40 > [0.053786] gic_starting_cpu+0x24/0x38 > [0.053788] cpuhp_invoke_callback+0xa0/0x710 > [0.053789] notify_cpu_starting+0xcc/0xd8 > [0.053790] secondary_start_kernel+0x148/0x200 > > [...] Applied to irq/irqchip-next, thanks! [1/1] irqchip/gic-v4.1: Use GFP_ATOMIC flag in allocate_vpe_l1_table() commit: d1bd7e0ba533a2a6f313579ec9b504f6614c35c4 Cheers, M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH V3 0/1] irqchip: intmux: implement intmux PM
On Mon, 27 Jul 2020 22:17:33 +0800, Joakim Zhang wrote: > This patch intends to implement intmux PM. > > ChangeLogs: > V2->V3: > 1. allocate u32 saved_reg for a per channel. > > V1->V2: > 1. add more detailed commit message. > 2. use u32 for 32bit HW registers. > 3. fix kbuild failures. > 4. move trivial functions into their respective callers. > 5. squash two patches together. > > [...] Applied to irq/irqchip-next, thanks! [1/1] irqchip/imx-intmux: Implement intmux runtime power management commit: bb403111e017a327737242eca40311921f833627 Cheers, M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH] irqchip/gic-v4.1: Use GFP_ATOMIC flag in allocate_vpe_l1_table()
Hi Zenghui,
On 2020-07-27 04:50, Zenghui Yu wrote:
Hi Marc,
On 2020/6/30 21:37, Zenghui Yu wrote:
Booting the latest kernel with DEBUG_ATOMIC_SLEEP=y on a GICv4.1
enabled
box, I get the following kernel splat:
[0.053766] BUG: sleeping function called from invalid context at
mm/slab.h:567
[0.053767] in_atomic(): 1, irqs_disabled(): 128, non_block: 0,
pid: 0, name: swapper/1
[0.053769] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.8.0-rc3+
#23
[0.053770] Call trace:
[0.053774] dump_backtrace+0x0/0x218
[0.053775] show_stack+0x2c/0x38
[0.053777] dump_stack+0xc4/0x10c
[0.053779] ___might_sleep+0xfc/0x140
[0.053780] __might_sleep+0x58/0x90
[0.053782] slab_pre_alloc_hook+0x7c/0x90
[0.053783] kmem_cache_alloc_trace+0x60/0x2f0
[0.053785] its_cpu_init+0x6f4/0xe40
[0.053786] gic_starting_cpu+0x24/0x38
[0.053788] cpuhp_invoke_callback+0xa0/0x710
[0.053789] notify_cpu_starting+0xcc/0xd8
[0.053790] secondary_start_kernel+0x148/0x200
# ./scripts/faddr2line vmlinux its_cpu_init+0x6f4/0xe40
its_cpu_init+0x6f4/0xe40:
allocate_vpe_l1_table at drivers/irqchip/irq-gic-v3-its.c:2818
(inlined by) its_cpu_init_lpis at
drivers/irqchip/irq-gic-v3-its.c:3138
(inlined by) its_cpu_init at drivers/irqchip/irq-gic-v3-its.c:5166
It turned out that we're allocating memory using GFP_KERNEL (may
sleep)
within the CPU hotplug notifier, which is indeed an atomic context.
Bad
thing may happen if we're playing on a system with more than a single
CommonLPIAff group. Avoid it by turning this into an atomic
allocation.
Fixes: 5e5168461c22 ("irqchip/gic-v4.1: VPE table (aka
GICR_VPROPBASER) allocation")
Signed-off-by: Zenghui Yu
---
drivers/irqchip/irq-gic-v3-its.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c
index 6a5a87fc4601..b66eeca442c4 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -2814,7 +2814,7 @@ static int allocate_vpe_l1_table(void)
if (val & GICR_VPROPBASER_4_1_VALID)
goto out;
- gic_data_rdist()->vpe_table_mask = kzalloc(sizeof(cpumask_t),
GFP_KERNEL);
+ gic_data_rdist()->vpe_table_mask = kzalloc(sizeof(cpumask_t),
GFP_ATOMIC);
if (!gic_data_rdist()->vpe_table_mask)
return -ENOMEM;
@@ -2881,7 +2881,7 @@ static int allocate_vpe_l1_table(void)
pr_debug("np = %d, npg = %lld, psz = %d, epp = %d, esz = %d\n",
np, npg, psz, epp, esz);
- page = alloc_pages(GFP_KERNEL | __GFP_ZERO, get_order(np *
PAGE_SIZE));
+ page = alloc_pages(GFP_ATOMIC | __GFP_ZERO, get_order(np *
PAGE_SIZE));
if (!page)
return -ENOMEM;
Do you mind taking this patch into v5.9? Or please let me know if you
still have any concerns on it?
Oops, I seem to have dropped this one on the floor.
I've picked it up now.
Thanks for the heads up,
M.
--
Jazz is not dead. It just smells funny...
Re: linux-next: Signed-off-by missing for commit in the irqchip tree
Hi Stephen,
On 2020-07-26 23:10, Stephen Rothwell wrote:
Hi all,
Commit
e5c19cf32b68 ("irqchip/stm32-exti: Use the
hwspin_lock_timeout_in_atomic() API")
is missing a Signed-off-by from its committer.
Looks like I fat-fingered a b4 invocation! Thanks for the heads up,
will be fixed in a minute.
M.
--
Jazz is not dead. It just smells funny...
Re: [RESEND RFC PATCH v1] arm64: kvm: flush tlbs by range in unmap_stage2_range function
On 2020-07-24 14:43, Zhenyu Ye wrote:
Now in unmap_stage2_range(), we flush tlbs one by one just after the
corresponding pages cleared. However, this may cause some performance
problems when the unmap range is very large (such as when the vm
migration rollback, this may cause vm downtime too loog).
You keep resending this patch, but you don't give any numbers
that would back your assertion.
This patch moves the kvm_tlb_flush_vmid_ipa() out of loop, and
flush tlbs by range after other operations completed. Because we
do not make new mapping for the pages here, so this doesn't violate
the Break-Before-Make rules.
Signed-off-by: Zhenyu Ye
---
arch/arm64/include/asm/kvm_asm.h | 2 ++
arch/arm64/kvm/hyp/tlb.c | 36
arch/arm64/kvm/mmu.c | 11 +++---
3 files changed, 46 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_asm.h
b/arch/arm64/include/asm/kvm_asm.h
index 352aaebf4198..ef8203d3ca45 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -61,6 +61,8 @@ extern char __kvm_hyp_vector[];
extern void __kvm_flush_vm_context(void);
extern void __kvm_tlb_flush_vmid_ipa(struct kvm *kvm, phys_addr_t
ipa);
+extern void __kvm_tlb_flush_vmid_range(struct kvm *kvm, phys_addr_t
start,
+ phys_addr_t end);
extern void __kvm_tlb_flush_vmid(struct kvm *kvm);
extern void __kvm_tlb_flush_local_vmid(struct kvm_vcpu *vcpu);
diff --git a/arch/arm64/kvm/hyp/tlb.c b/arch/arm64/kvm/hyp/tlb.c
index d063a576d511..4f4737a7e588 100644
--- a/arch/arm64/kvm/hyp/tlb.c
+++ b/arch/arm64/kvm/hyp/tlb.c
@@ -189,6 +189,42 @@ void __hyp_text __kvm_tlb_flush_vmid_ipa(struct
kvm *kvm, phys_addr_t ipa)
__tlb_switch_to_host(kvm, &cxt);
}
+void __hyp_text __kvm_tlb_flush_vmid_range(struct kvm *kvm,
phys_addr_t start,
+ phys_addr_t end)
+{
+ struct tlb_inv_context cxt;
+ unsigned long addr;
+
+ start = __TLBI_VADDR(start, 0);
+ end = __TLBI_VADDR(end, 0);
+
+ dsb(ishst);
+
+ /* Switch to requested VMID */
+ kvm = kern_hyp_va(kvm);
+ __tlb_switch_to_guest(kvm, &cxt);
+
+ if ((end - start) >= 512 << (PAGE_SHIFT - 12)) {
+ __tlbi(vmalls12e1is);
And what is this magic value based on? You don't even mention in the
commit log that you are taking this shortcut.
+ goto end;
+ }
+
+ for (addr = start; addr < end; addr += 1 << (PAGE_SHIFT - 12))
+ __tlbi(ipas2e1is, addr);
+
+ dsb(ish);
+ __tlbi(vmalle1is);
+
+end:
+ dsb(ish);
+ isb();
+
+ if (!has_vhe() && icache_is_vpipt())
+ __flush_icache_all();
+
+ __tlb_switch_to_host(kvm, &cxt);
+}
+
I'm sorry, but without numbers backing this approach for a number
of workloads and a representative set of platforms, this is
going nowhere.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Re: [PATCHv3 3/6] irqchip/irq-pruss-intc: Add support for shared and invalid interrupts
On 2020-07-25 16:57, Suman Anna wrote:
Suman,
Hi Marc,
[...]
@@ -244,8 +295,14 @@ static int pruss_intc_probe(struct
platform_device *pdev)
return -ENOMEM;
for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
+ if (intc->invalid_intr & BIT(i))
+ continue;
+
irq = platform_get_irq_byname(pdev, irq_names[i]);
if (irq <= 0) {
+ if (intc->shared_intr & BIT(i))
+ continue;
I don't really understand why you are treating these "shared"
interrupts
differently from the invalid ones. In all cases, they shouldn't be
used.
The behavior is the same in how we handle it, but the difference is
that an "invalid" one is never even connected to the ARM interrupt
controller, while the "shared" one is a choice. So, unless this
interrupt is being used/handled by a different processor/entity, you
would not see this skipped from the dts node.
And I'm saying that all that matters is that you are discarding these
interrupts. Whether they are flagged invalid or shared, they are not
available to Linux. So the difference in handling is pointless and
only makes it harder to understand what you are doing.
The primary reason for using two properties and this logic was to
accurately describe the h/w and usage of these in the DT bindings to
distinguish the "never connected" vs the "optionally can be skipped"
interrupts rather than go by how these are handled in the driver. I
feel we will loose this description and make it confusing for SoC
product integration developers.
This logic makes zero difference to Linux, and I do not see what
you gain by having two code paths with separate list of unusable
interrupts. And why on Earth would a "Soc product integration
developer" have any business to mess with this driver code?
They should very much stay away from it and deal with their
precious value add.
If you want two properties or even twenty, go for it, and have fun.
Just don't make this driver even more unreadable than it already is.
Merge all these interrupts in *one* list of unusable interrupts,
and be done with it.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Re: [PATCH v3 0/4] irqchip: Add IRQCHIP_PLATFORM_DRIVER helper macros
On Fri, 17 Jul 2020 17:06:33 -0700, Saravana Kannan wrote: > Made a series out of the previous v2 patch + some example uses of the > macros. > > Not sure if the MTK changes work (just compile tested), but saw that > Hanks was trying to make those drivers into tristate. So I assume > they'll work as platform drivers. Please wait for MTK Ack before picking > up patches 3 and 4. > > [...] Applied to irq/irqchip-5.9, thanks! [1/4] irqchip: Add IRQCHIP_PLATFORM_DRIVER_BEGIN/END and IRQCHIP_MATCH helper macros commit: f3b5e608ed6d17bdf04dacbf2374f10d51fe9b09 [2/4] irqchip/qcom-pdc: Switch to using IRQCHIP_PLATFORM_DRIVER helper macros commit: 04741740254cd83fb4f2b7747aeb35202104f8fe [3/4] irqchip/mtk-sysirq: Convert to a platform driver commit: 3ae3022690e6787839dafa8ea3496450248b53e1 [4/4] irqchip/mtk-cirq: Convert to a platform driver commit: 538b63351607960ff2249460089daa31337ddeba Cheers, M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH v2] irqchip/stm32-exti: map direct event to irq parent
On Fri, 17 Jul 2020 16:07:17 +0200, Alexandre Torgue wrote: > EXTI lines are mainly used to wake-up system from CStop low power mode. > Currently, if a device wants to use a EXTI (direct) line as wakeup line, > it has to declare 2 interrupts: > - one for EXTI used to wake-up system (with dedicated_wake_irq api). > - one for GIC used to get the wake up reason inside the concerned IP. > > This split is not really needed as each EXTI line is actually "linked " to > a GIC. So to avoid this useless double interrupt management in each > wake-up driver, this patch lets the STM32 EXTI driver abstract it by > mapping each EXTI line to his corresponding GIC. Applied to irq/irqchip-5.9, thanks! [1/1] irqchip/stm32-exti: map direct event to irq parent commit: 99e05524bc722c8d3c1ab9c817afcb6829dbded3 Cheers, M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH v2] irqchip/gic-v4.1: Ensure accessing the correct RD when writing INVALLR
On Mon, 20 Jul 2020 17:23:28 +0800, Zenghui Yu wrote:
> The GICv4.1 spec tells us that it's CONSTRAINED UNPREDICTABLE to issue a
> register-based invalidation operation for a vPEID not mapped to that RD,
> or another RD within the same CommonLPIAff group.
>
> To follow this rule, commit f3a059219bc7 ("irqchip/gic-v4.1: Ensure mutual
> exclusion between vPE affinity change and RD access") tried to address the
> race between the RD accesses and the vPE affinity change, but somehow
> forgot to take GICR_INVALLR into account. Let's take the vpe_lock before
> evaluating vpe->col_idx to fix it.
Applied to irq/irqchip-5.9, thanks!
[1/1] irqchip/gic-v4.1: Ensure accessing the correct RD when writing INVALLR
commit: fdccf1d9d10395abfe082f50694f374997c6e101
Cheers,
M.
--
Without deviation from the norm, progress is not possible.
Re: [PATCH] irqchip/irq-bcm7038-l1: Guard uses of cpu_logical_map
On Fri, 24 Jul 2020 11:41:56 -0700, Florian Fainelli wrote: > cpu_logical_map is only defined for CONFIG_SMP builds, when we are in an > UP configuration, the boot CPU is 0. Applied to irq/irqchip-5.9, thanks! [1/1] irqchip/irq-bcm7038-l1: Guard uses of cpu_logical_map commit: 29a190b6a2bd9ef4282e2f73e7abd76203740150 Cheers, M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH] irqchip: irq-bcm2836.h: drop a duplicated word
On Sat, 18 Jul 2020 17:28:53 -0700, Randy Dunlap wrote: > Drop the repeated word "the" in a comment. Applied to irq/irqchip-5.9, thanks! [1/1] irqchip: irq-bcm2836.h: drop a duplicated word commit: b48489d82fcf61f229a892c7e1b559be15c95916 Cheers, M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH V2 1/1] irqchip: imx-intmux: implement intmux PM
On Mon, 20 Jul 2020 11:42:37 +0100,
Joakim Zhang wrote:
>
> When system suspended, we could explicitly disable clock to save power.
> And we need save registers' state since it could be lost after power
> off.
>
> Implement PM which will:
> 1) Without CONFIG_PM, clock is always on after probe stage.
> 2) With CONFIG_PM, clock is off after probe stage.
> 3) Disable clock and save registers' state when do system suspend and
> enable clock and restore registers' state while system resume.
> 4) Make Power Domain framework be able to shutdown the corresponding
> power domain of this device.
>
> Signed-off-by: Joakim Zhang
> ---
> drivers/irqchip/irq-imx-intmux.c | 70 +++-
> 1 file changed, 68 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/irqchip/irq-imx-intmux.c
> b/drivers/irqchip/irq-imx-intmux.c
> index c27577c81126..5971603cc607 100644
> --- a/drivers/irqchip/irq-imx-intmux.c
> +++ b/drivers/irqchip/irq-imx-intmux.c
> @@ -53,6 +53,7 @@
> #include
> #include
> #include
> +#include
>
> #define CHANIER(n) (0x10 + (0x40 * n))
> #define CHANIPR(n) (0x20 + (0x40 * n))
> @@ -60,6 +61,7 @@
> #define CHAN_MAX_NUM 0x8
>
> struct intmux_irqchip_data {
> + struct irq_chip chip;
> int chanidx;
> int irq;
> struct irq_domain *domain;
> @@ -70,6 +72,7 @@ struct intmux_data {
> void __iomem*regs;
> struct clk *ipg_clk;
> int channum;
> + u32 *saved_reg;
> struct intmux_irqchip_data irqchip_data[];
> };
>
> @@ -120,8 +123,10 @@ static struct irq_chip imx_intmux_irq_chip = {
> static int imx_intmux_irq_map(struct irq_domain *h, unsigned int irq,
> irq_hw_number_t hwirq)
> {
> - irq_set_chip_data(irq, h->host_data);
> - irq_set_chip_and_handler(irq, &imx_intmux_irq_chip, handle_level_irq);
> + struct intmux_irqchip_data *data = h->host_data;
> +
> + irq_set_chip_data(irq, data);
> + irq_set_chip_and_handler(irq, &data->chip, handle_level_irq);
>
> return 0;
> }
> @@ -232,6 +237,19 @@ static int imx_intmux_probe(struct platform_device *pdev)
> data->channum = channum;
> raw_spin_lock_init(&data->lock);
>
> + if (IS_ENABLED(CONFIG_PM)) {
> + /* save CHANIER register */
> + data->saved_reg = devm_kzalloc(&pdev->dev,
> +sizeof(unsigned int) * channum,
This isn't consistent with the type of data->saved_reg. Consider using
sizeof(*data->saved_reg), which is guaranteed to be the right type.
It also begs the question: since this saved_reg array is allocated on
a per channel basis, why don't you have a per-channel additional u32
in the intmux_irqchip_data structure instead? This would sidestep this
extra allocation altogether.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
Re: [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
gt;* useable state so startup works.
>*/
> - if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) || irqd_is_activated(data))
> + if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) ||
> + irqd_is_activated(data) || !irqd_affinity_on_activate(data))
> return false;
>
> cpumask_copy(desc->irq_common_data.affinity, mask);
>
I have given this a go on two systems: one with a GICv2 that has its
PMUs wired in a similar way to John's system (each CPU PMU is on a
separate SPI), and another one that has an ITS. Both came up normally
and their interrupts are routed as expected:
* GICv2 PMU:
30: 1210000000 GICv2 39 Level arm-pmu
31:0 167000000 GICv2 40 Level arm-pmu
32:00 14500000 GICv2 41 Level arm-pmu
33:000 4000000 GICv2 42 Level arm-pmu
34:0000 97000 GICv2 43 Level arm-pmu
35:00000 46300 GICv2 44 Level arm-pmu
36:000000 740 GICv2 45 Level arm-pmu
37:00000008 GICv2 46 Level arm-pmu
* GICv3+ITS:
241: 21900000 ITS-MSI 524289 Edge nvme0q1
242:0 2510000 ITS-MSI 524290 Edge nvme0q2
243:00 197000 ITS-MSI 524291 Edge nvme0q3
244:000 38000 ITS-MSI 524292 Edge nvme0q4
245:0000 6750 ITS-MSI 524293 Edge nvme0q5
246:00000 436 ITS-MSI 524294 Edge nvme0q6
For a good measure, I've added this on top, adding the missing bits to
the debugfs entries:
diff --git a/kernel/irq/debugfs.c b/kernel/irq/debugfs.c
index 4f9f844074db..d44fc8a5dab2 100644
--- a/kernel/irq/debugfs.c
+++ b/kernel/irq/debugfs.c
@@ -120,6 +120,9 @@ static const struct irq_bit_descr irqdata_states[] = {
BIT_MASK_DESCR(IRQD_WAKEUP_STATE),
BIT_MASK_DESCR(IRQD_WAKEUP_ARMED),
+ BIT_MASK_DESCR(IRQD_DEFAULT_TRIGGER_SET),
+ BIT_MASK_DESCR(IRQD_HANDLE_ENFORCE_IRQCTX),
+ BIT_MASK_DESCR(IRQD_AFFINITY_ON_ACTIVATE),
};
static const struct irq_bit_descr irqdesc_states[] = {
FWIW:
Acked-by: Marc Zyngier
Tested-by: Marc Zyngier
M.
--
Without deviation from the norm, progress is not possible.
Re: [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
Hi both, On Fri, 24 Jul 2020 21:03:50 +0100, Thomas Gleixner wrote: > > John, > > John Keeping writes: > > On Fri, 17 Jul 2020 18:00:02 +0200 > > Thomas Gleixner wrote: > > It seems that this patch breaks perf events on RK3288 because the PMU > > interrupts that should be per-cpu are now all on CPU0 so no events are > > collected from CPUs 1-3 and those interrupts are killed as spurious > > after a few seconds. SPI-backed PMUs. Urgh... > > > > I'm seeing this on 4.19.134 and 5.4.53 but as far as I can tell the > > relevant code hasn't changed through to next-20200723. Reverting the > > backport of this change fixes the problem. > > Bah. > > > It looks like what happens is that because the interrupts are not > > per-CPU in the hardware, armpmu_request_irq() calls irq_force_affinity() > > while the interrupt is deactivated and then request_irq() with > > IRQF_PERCPU | IRQF_NOBALANCING. > > > > Now when irq_startup() runs with IRQ_STARTUP_NORMAL, it calls > > irq_setup_affinity() which returns early because IRQF_PERCPU and > > IRQF_NOBALANCING are set, leaving the interrupt on its original CPU. > > Right. My brain tricked me to believe that we made activation mandatory, > but that's not. > > I have some ideas for a trivial generic way to solve this without > undoing the commit in question and without going through all the irq > chip drivers. So far everything I came up with is butt ugly. Maybe Marc > has some brilliant idea. Not really. We have contradicting behaviours here, where some interrupts want to see the set_affinity early (the above case), and some cannot handle that (x86 vectors and the GICv3 ITS). We could key it on the presence of an activate callback, but it feels fragile. I'll follow up on your patch in the next email, which seems like a sensible approach. M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH RFC v2 02/18] irq/dev-msi: Add support for a new DEV_MSI irq domain
On 2020-07-22 20:59, Jason Gunthorpe wrote: On Wed, Jul 22, 2020 at 07:52:33PM +0100, Marc Zyngier wrote: Which is exactly what platform-MSI already does. Why do we need something else? It looks to me like all the code is around managing the dev->msi_domain of the devices. The intended use would have PCI drivers create children devices using mdev or virtbus and those devices wouldn't have a msi_domain from the platform. Looks like platform_msi_alloc_priv_data() fails immediately because dev->msi_domain will be NULL for these kinds of devices. Maybe that issue should be handled directly instead of wrappering platform_msi_*? For instance a trivial addition to the platform_msi API: platform_msi_assign_domain(struct_device *newly_created_virtual_device, struct device *physical_device); Which could set the msi_domain of new device using the topology of physical_device to deduce the correct domain? That would seem like a sensible course of action, as losing the topology information will likely result in problems down the line. Then the question is how to properly create a domain within the hardware topology of physical_device with the correct parameters for the platform. Why do we need a dummy msi_domain anyhow? Can this just use physical_device->msi_domain directly? (I'm at my limit here of how much of this I remember, sorry) The parent device would be a PCI device, if I follow you correctly. It would thus expect to be able to program the MSI the PCI way, which wouldn't work. So we end-up with this custom MSI domain that knows about *this* particular family of devices. If you solve that it should solve the remapping problem too, as the physical_device is already assigned by the platform to a remapping irq domain if that is what the platform wants. + parent = irq_get_default_host(); Really? How is it going to work once you have devices sending their MSIs to two different downstream blocks? This looks rather short-sighted. .. and fix this too, the parent domain should be derived from the topology of the physical_device which is originating the interrupt messages. On the other hand, masking an interrupt is an irqchip operation, and only concerns the irqchip level. Here, you seem to be making it an end-point operation, which doesn't really make sense to me. Or is this device its own interrupt controller as well? That would be extremely surprising, and I'd expect some block downstream of the device to be able to control the masking of the interrupt. These are message interrupts so they originate directly from the device and generally travel directly to the CPU APIC. On the wire there is no difference between a MSI, MSI-X and a device using the dev-msi approach. I understand that. IIRC on Intel/AMD at least once a MSI is launched it is not maskable. Really? So you can't shut a device with a screaming interrupt, for example, should it become otherwise unresponsive? So the model for MSI is always "mask at source". The closest mapping to the Linux IRQ model is to say the end device has a irqchip that encapsulates the ability of the device to generate the MSI in the first place. This is an x86'ism, I'm afraid. Systems I deal with can mask any interrupt at the interrupt controller level, MSI or not. It looks like existing platform_msi drivers deal with "masking" implicitly by halting the device interrupt generation before releasing the interrupt and have no way for the generic irqchip layer to mask the interrupt. No. As I said above, the interrupt controller is perfectly capable of masking interrupts on its own, without touching the device. I suppose the motivation to make it explicit is related to vfio using the generic mask/unmask functionality? Explicit seems better, IMHO. If masking at the source is the only way to shut the device up, and assuming that the device provides the expected semantics (a MSI raised by the device while the interrupt is masked isn't lost and gets sent when unmasked), that's fair enough. It's just ugly. Thanks, M. -- Jazz is not dead. It just smells funny...
Re: [PATCH RFC v2 02/18] irq/dev-msi: Add support for a new DEV_MSI irq domain
On Tue, 21 Jul 2020 17:02:28 +0100,
Dave Jiang wrote:
>
> From: Megha Dey
>
> Add support for the creation of a new DEV_MSI irq domain. It creates a
> new irq chip associated with the DEV_MSI domain and adds the necessary
> domain operations to it.
>
> Add a new config option DEV_MSI which must be enabled by any
> driver that wants to support device-specific message-signaled-interrupts
> outside of PCI-MSI(-X).
Which is exactly what platform-MSI already does. Why do we need
something else?
>
> Lastly, add device specific mask/unmask callbacks in addition to a write
> function to the platform_msi_ops.
>
> Reviewed-by: Dan Williams
> Signed-off-by: Megha Dey
> Signed-off-by: Dave Jiang
> ---
> arch/x86/include/asm/hw_irq.h |5 ++
> drivers/base/Kconfig |7 +++
> drivers/base/Makefile |1
> drivers/base/dev-msi.c| 95
> +
> drivers/base/platform-msi.c | 45 +--
> drivers/base/platform-msi.h | 23 ++
> include/linux/msi.h |8 +++
> 7 files changed, 168 insertions(+), 16 deletions(-)
> create mode 100644 drivers/base/dev-msi.c
> create mode 100644 drivers/base/platform-msi.h
>
> diff --git a/arch/x86/include/asm/hw_irq.h b/arch/x86/include/asm/hw_irq.h
> index 74c12437401e..8ecd7570589d 100644
> --- a/arch/x86/include/asm/hw_irq.h
> +++ b/arch/x86/include/asm/hw_irq.h
> @@ -61,6 +61,11 @@ struct irq_alloc_info {
> irq_hw_number_t msi_hwirq;
> };
> #endif
> +#ifdef CONFIG_DEV_MSI
> + struct {
> + irq_hw_number_t hwirq;
> + };
> +#endif
> #ifdef CONFIG_X86_IO_APIC
> struct {
> int ioapic_id;
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 8d7001712062..f00901bac056 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -210,4 +210,11 @@ config GENERIC_ARCH_TOPOLOGY
> appropriate scaling, sysfs interface for reading capacity values at
> runtime.
>
> +config DEV_MSI
> + bool "Device Specific Interrupt Messages"
> + select IRQ_DOMAIN_HIERARCHY
> + select GENERIC_MSI_IRQ_DOMAIN
> + help
> + Allow device drivers to generate device-specific interrupt messages
> + for devices independent of PCI MSI/-X.
> endmenu
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index 157452080f3d..ca1e4d39164e 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_REGMAP)+= regmap/
> obj-$(CONFIG_SOC_BUS) += soc.o
> obj-$(CONFIG_PINCTRL) += pinctrl.o
> obj-$(CONFIG_DEV_COREDUMP) += devcoredump.o
> +obj-$(CONFIG_DEV_MSI) += dev-msi.o
> obj-$(CONFIG_GENERIC_MSI_IRQ_DOMAIN) += platform-msi.o
> obj-$(CONFIG_GENERIC_ARCH_TOPOLOGY) += arch_topology.o
>
> diff --git a/drivers/base/dev-msi.c b/drivers/base/dev-msi.c
> new file mode 100644
> index ..240ccc353933
> --- /dev/null
> +++ b/drivers/base/dev-msi.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright © 2020 Intel Corporation.
> + *
> + * Author: Megha Dey
> + */
> +
> +#include
> +#include
> +#include
> +#include "platform-msi.h"
> +
> +struct irq_domain *dev_msi_default_domain;
> +
> +static irq_hw_number_t dev_msi_get_hwirq(struct msi_domain_info *info,
> + msi_alloc_info_t *arg)
> +{
> + return arg->hwirq;
> +}
> +
> +static irq_hw_number_t dev_msi_calc_hwirq(struct msi_desc *desc)
> +{
> + u32 devid;
> +
> + devid = desc->platform.msi_priv_data->devid;
> +
> + return (devid << (32 - DEV_ID_SHIFT)) | desc->platform.msi_index;
> +}
> +
> +static void dev_msi_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
> +{
> + arg->hwirq = dev_msi_calc_hwirq(desc);
> +}
> +
> +static int dev_msi_prepare(struct irq_domain *domain, struct device *dev,
> +int nvec, msi_alloc_info_t *arg)
> +{
> + memset(arg, 0, sizeof(*arg));
> +
> + return 0;
> +}
> +
> +static struct msi_domain_ops dev_msi_domain_ops = {
> + .get_hwirq = dev_msi_get_hwirq,
> + .set_desc = dev_msi_set_desc,
> + .msi_prepare= dev_msi_prepare,
> +};
> +
> +static struct irq_chip dev_msi_controller = {
> + .name = "DEV-MSI",
> + .irq_unmask = platform_msi_unmask_irq,
> + .irq_mask = platform_msi_mask_irq,
This seems pretty odd, see below.
> + .irq_write_msi_msg = platform_msi_write_msg,
> + .irq_ack= irq_chip_ack_parent,
> + .irq_retrigger = irq_chip_retrigger_hierarchy,
> + .flags = IRQCHIP_SKIP_SET_WAKE,
> +};
> +
> +static struct msi_domain_info dev_msi_domain_info = {
> + .flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS,
> + .ops= &dev_msi_domain_ops,
> + .chip = &
Re: [PATCHv3 2/6] irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts
On 2020-07-21 10:27, Grzegorz Jaszczyk wrote: Hi Marc, First of all thank you very much for your review. I apologize in advance if the description below is too verbose or not detailed enough. On Fri, 17 Jul 2020 at 14:36, Marc Zyngier wrote: Suman, Grzegorz, On Wed, 15 Jul 2020 14:38:05 +0100, Grzegorz Jaszczyk wrote: > > Hi Marc, > > > On 7/8/20 5:47 AM, Marc Zyngier wrote: > > > On 2020-07-08 08:04, Grzegorz Jaszczyk wrote: > > >> On Sun, 5 Jul 2020 at 22:45, Marc Zyngier wrote: > > >>> > > >>> On 2020-07-05 14:26, Grzegorz Jaszczyk wrote: > > >>> > On Sat, 4 Jul 2020 at 11:39, Marc Zyngier wrote: > > >>> >> > > >>> >> On 2020-07-03 15:28, Grzegorz Jaszczyk wrote: > > >>> > > >>> [...] > > >>> > > >>> >> It still begs the question: if the HW can support both edge and level > > >>> >> triggered interrupts, why isn't the driver supporting this diversity? > > >>> >> I appreciate that your HW may only have level interrupts so far, but > > >>> >> what guarantees that this will forever be true? It would imply a > > >>> >> change > > >>> >> in the DT binding, which isn't desirable. > > >>> > > > >>> > Ok, I've got your point. I will try to come up with something later > > >>> > on. Probably extending interrupt-cells by one and passing interrupt > > >>> > type will be enough for now. Extending this driver to actually support > > >>> > it can be handled later if needed. Hope it works for you. > > >>> > > >>> Writing a set_type callback to deal with this should be pretty easy. > > >>> Don't delay doing the right thing. > > >> > > >> Ok. > > > > Sorry for the typo in my comment causing this confusion. > > > > The h/w actually doesn't support the edge-interrupts. Likewise, the > > polarity is always high. The individual register bit descriptions > > mention what the bit values 0 and 1 mean, but there is additional > > description in the TRMs on all the SoCs that says > > "always write 1 to the bits of this register" for PRUSS_INTC_SIPR(x) and > > "always write 0 to the bits of this register" for PRUSS_INTC_SITR(x). > > FWIW, these are also the reset values. > > > > Eg: AM335x TRM - https://www.ti.com/lit/pdf/spruh73 > > Please see Section 4.4.2.5 and the register descriptions in 4.5.3.49, > > 4.5.3.51. Please also see Section 4.4.2.3 that explains the PRUSS INTC > > methodology. > > > > >> > > >>> > > >>> [...] > > >>> > > >>> >> >> > + hwirq = hipir & GENMASK(9, 0); > > >>> >> >> > + virq = irq_linear_revmap(intc->domain, hwirq); > > >>> >> >> > > >>> >> >> And this is where I worry. You seems to have a single irqdomain > > >>> >> >> for all the muxes. Are you guaranteed that you will have no > > >>> >> >> overlap between muxes? And please use irq_find_mapping(), as > > >>> >> >> I have top-secret plans to kill irq_linear_revmap(). > > >>> >> > > > >>> >> > Regarding irq_find_mapping - sure. > > >>> >> > > > >>> >> > Regarding irqdomains: > > >>> >> > It is a single irqdomain since the hwirq (system event) can be > > >>> mapped > > >>> >> > to different irq_host (muxes). Patch #6 > > >>> >> > https://lkml.org/lkml/2020/7/2/616 implements and describes how > > >>> input > > >>> >> > events can be mapped to some output host interrupts through 2 > > >>> levels > > >>> >> > of many-to-one mapping i.e. events to channel mapping and > > >>> channels to > > >>> >> > host interrupts. Mentioned implementation ensures that specific > > >>> system > > >>> >> > event (hwirq) can be mapped through PRUSS specific channel into a > > >>> >> > single host interrupt. > > >>> >> > > >>> >> Patch #6 is a nightmare of its own, and I haven't fully groked it > > >>> yet. > > >>> >> Also, this driver seems to totally ignore th
Re: [PATCH] irqchip/gic-v4.1: Ensure accessing the correct RD when writing INVALLR
Hi Zenghui,
On 2020-07-20 03:27, Zenghui Yu wrote:
Hi Marc,
On 2020/7/17 19:07, Marc Zyngier wrote:
On Thu, 09 Jul 2020 14:49:59 +0100,
Zenghui Yu wrote:
The GICv4.1 spec tells us that it's CONSTRAINED UNPREDICTABLE to
issue a
register-based invalidation operation for a vPEID not mapped to that
RD,
or another RD within the same CommonLPIAff group.
To follow this rule, commit f3a059219bc7 ("irqchip/gic-v4.1: Ensure
mutual
exclusion between vPE affinity change and RD access") tried to
address the
race between the RD accesses and the vPE affinity change, but somehow
forgot to take GICR_INVALLR into account. Let's take the vpe_lock
before
evaluating vpe->col_idx to fix it.
Signed-off-by: Zenghui Yu
Shouldn't this deserve a Fixes: tag?
Yes, I think a
Fixes: f3a059219bc7 ("irqchip/gic-v4.1: Ensure mutual exclusion
between vPE affinity change and RD access")
should be enough. Should I resend a version with the tag added?
Yes, please, together with a Cc: sta...@vger.kernel.org, as the
original patch is in 5.7 and I intend to take it via the 5.9
branch.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Re: [PATCH v2] irqchip: Add IRQCHIP_PLATFORM_DRIVER_BEGIN/END and IRQCHIP_MATCH helper macros
On 2020-07-17 18:50, Saravana Kannan wrote:
On Fri, Jul 17, 2020 at 3:49 AM Marc Zyngier wrote:
Hi Saravana,
Thanks for re-spinning this one.
On Fri, 17 Jul 2020 03:44:47 +0100,
Saravana Kannan wrote:
>
> Compiling an irqchip driver as a platform driver needs to bunch of
> things to be done right:
> - Making sure the parent domain is initialized first
> - Making sure the device can't be unbound from sysfs
> - Disallowing module unload if it's built as a module
> - Finding the parent node
> - Etc.
>
> Instead of trying to make sure all future irqchip platform drivers get
> this right, provide boilerplate macros that take care of all of this.
>
> An example use would look something like this. Where acme_foo_init and
> acme_bar_init are similar to what would be passed to IRQCHIP_DECLARE.
>
> IRQCHIP_PLATFORM_DRIVER_BEGIN
I think there is some value in having the BEGIN statement containing
the driver name, see below.
> IRQCHIP_MATCH(foo, "acme,foo", acme_foo_init)
> IRQCHIP_MATCH(bar, "acme,bar", acme_bar_init)
> IRQCHIP_PLATFORM_DRIVER_END(acme_irq)
>
> Signed-off-by: Saravana Kannan
> ---
> drivers/irqchip/irqchip.c | 22 ++
> include/linux/irqchip.h | 23 +++
> 2 files changed, 45 insertions(+)
>
> diff --git a/drivers/irqchip/irqchip.c b/drivers/irqchip/irqchip.c
> index 2b35e68bea82..236ea793f01c 100644
> --- a/drivers/irqchip/irqchip.c
> +++ b/drivers/irqchip/irqchip.c
> @@ -10,8 +10,10 @@
>
> #include
> #include
> +#include
> #include
> #include
> +#include
>
> /*
> * This special of_device_id is the sentinel at the end of the
> @@ -29,3 +31,23 @@ void __init irqchip_init(void)
> of_irq_init(__irqchip_of_table);
> acpi_probe_device_table(irqchip);
> }
> +
> +int platform_irqchip_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct device_node *par_np = of_irq_find_parent(np);
> + of_irq_init_cb_t irq_init_cb = of_device_get_match_data(&pdev->dev);
> +
> + if (!irq_init_cb)
> + return -EINVAL;
> +
> + if (par_np == np)
> + par_np = NULL;
> +
> + /* If there's a parent irqchip, make sure it has been initialized. */
> + if (par_np && !irq_find_matching_host(np, DOMAIN_BUS_ANY))
There is no guarantee that the calling driver wants BUS_ANY as a token
for its parent. It may work for now, if you only have dependencies to
drivers that only expose a single domain, but that's not a general
purpose check..
At least, please add a comment saying that the new driver may want to
check that the irqdomain it depends on may not be available.
This is just checking if the parent interrupt controller has been
initialized. It's just saying that if NONE of the parent irq domains
have been registered, it's not time for this interrupt controller to
initialize. And yes, as you said, the actual init code can do more
checks and defer probe too. Maybe I'll just put the 2nd sentence as
the comment.
Sure, go ahead.
> + return -EPROBE_DEFER;
> +
> + return irq_init_cb(np, par_np);
> +}
> +EXPORT_SYMBOL_GPL(platform_irqchip_probe);
> diff --git a/include/linux/irqchip.h b/include/linux/irqchip.h
> index 950e4b2458f0..6d5eba7cbbb7 100644
> --- a/include/linux/irqchip.h
> +++ b/include/linux/irqchip.h
> @@ -13,6 +13,7 @@
>
> #include
> #include
> +#include
>
> /*
> * This macro must be used by the different irqchip drivers to declare
> @@ -26,6 +27,28 @@
> */
> #define IRQCHIP_DECLARE(name, compat, fn) OF_DECLARE_2(irqchip, name,
compat, fn)
>
> +extern int platform_irqchip_probe(struct platform_device *pdev);
> +
> +#define IRQCHIP_PLATFORM_DRIVER_BEGIN \
> +static const struct of_device_id __irqchip_match_table[] = {
How about:
#define IRQCHIP_PLATFORM_DRIVER_BEGIN(drv_name) \
static const struct of_device_id __irqchip_match_table_##drv_name = {
which makes it easier to debug when you want to identify specific
structures in an object (otherwise, they all have the same
name...). it is also much more pleasing aesthetically ;-).
I totally agree. I wanted BEGIN to have the name and END to not have
to specify the name. But I couldn't figure out a way to do it. I
assumed you wouldn't want the names repeated in both BEGIN and END. If
you are okay with that, I prefer your suggestion too.
I'm perfectly fine having the name in both the BEGIN and END tags.
It has a nice LaTeX twist to it ;-).
> +
> +#define IRQCHIP_MATCH(compat, fn) { .compatible = compat, .data = fn },
> +
> +#define IRQCHIP_PLATFORM_DRIVER_END(drv
Re: [PATCH v4 8/8] dt-bindings: interrupt-controller: Fix typos in loongson,liointc.yaml
On Thu, 16 Jul 2020 20:08:20 +0100,
Rob Herring wrote:
>
> On Thu, 16 Jul 2020 11:16:30 +0800, Tiezhu Yang wrote:
> > Fix the following typos in loongson,liointc.yaml:
> > children -> child
> > fron -> from
> > connected -> connect
> > it's -> its
> >
> > Fixes: b6280c8bb6f5 ("dt-bindings: interrupt-controller: Add Loongson
> > LIOINTC")
> > Signed-off-by: Tiezhu Yang
> > Cc: Rob Herring
> > Cc: devicet...@vger.kernel.org
> > ---
> > .../devicetree/bindings/interrupt-controller/loongson,liointc.yaml| 4
> > ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
>
> Applied, thanks!
>
Ah! I had an older version queued already. I'll drop mine.
M.
--
Without deviation from the norm, progress is not possible.
Re: [PATCH] irqchip: ativic32: constify irq_domain_ops
On Wed, 15 Jul 2020 02:38:57 +0900, Masahiro Yamada wrote: > This is passed to irq_domain_add_linear(), which accepts a pointer > to a const structure. Applied to irq/irqchip-5.9, thanks! [1/1] irqchip/ativic32: Constify irq_domain_ops commit: 605a2cf566e130c77fc2cc77fac37fb901fc868a Cheers, M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH v3 0/8] irqchip: Fix some issues and do some code cleanups about Loongson
On Tue, 7 Jul 2020 10:12:44 +0800, Tiezhu Yang wrote: > Check the return value of irq_domain_translate_onecell() and > irq_domain_translate_twocell(), fix potential resource leak > and dead lock, do some code cleanups about Loongson to make > it more clean and readable. > > v2: > - In order to avoid git send-email failed, make the related patches > about Loongson into a new patch series and add "Fixes" tag > v3: > - Add a new patch "irqchip/loongson-liointc: Fix potential dead lock" > - Fix another typo in loongson,liointc.yaml > > [...] Applied to irq/irqchip-5.9, thanks! [1/8] irqchip/loongson-htpic: Remove redundant kfree operation commit: f90fafecf4880b9785da85feb9b3e6d85fbf2287 [2/8] irqchip/loongson-htpic: Remove unneeded select of I8259 commit: 85efd6059ae1a99e5e7205f37e910fd41dfa0ade [3/8] irqchip/loongson-htvec: Fix potential resource leak commit: 652d54e77a438cf38a5731d8f9983c81e72dc429 [4/8] irqchip/loongson-htvec: Check return value of irq_domain_translate_onecell() commit: dbec37048d27cee36e103e113b5f9b1852bfe997 [5/8] irqchip/loongson-pch-pic: Check return value of irq_domain_translate_twocell() commit: 66a535c495f72e1deacc37dfa34acca2a06e3578 [6/8] irqchip/loongson-pch-msi: Remove unneeded variable commit: b10cbca8f03dd10dc241395a639d488f4144e986 [7/8] irqchip/loongson-liointc: Fix potential dead lock commit: fa03587cad9bd32aa552377de4f05c50181a35a8 [8/8] dt-bindings: interrupt-controller: Fix typos in loongson,liointc.yaml commit: 1055df97676a31df0b46942cb8bf66eb37ae318f Cheers, M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH v3 0/3] Allow for qcom-pdc to be loadable as a module
On Fri, 10 Jul 2020 23:18:21 +, John Stultz wrote: > This patch series provides exports and config tweaks to allow > the qcom-pdc driver to be able to be configured as a permement > modules (particularlly useful for the Android Generic Kernel > Image efforts). > > This was part of a larger patch series, to enable qcom_scm > driver to be a module as well, but I've split it out as there > are some outstanding objections I still need to address with > the follow-on patches, and wanted to see if progress could be > made on this subset of the series in the meantime. > > [...] Applied to irq/irqchip-5.9, thanks! [1/3] irqdomain: Export irq_domain_update_bus_token commit: ce8cefa53c06cd98607125c52c91e74373a893a0 [2/3] genirq: Export irq_chip_retrigger_hierarchy and irq_chip_set_vcpu_affinity_parent commit: 96703f046c42f8ab15e735953cbfee572c717e2d [3/3] irqchip/qcom-pdc: Allow QCOM_PDC to be loadable as a permanent module commit: 5ef7f1bbf9f56c5380c4d876655920cac92008e5 Cheers, M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH -next] irqchip: mips-gic: Make some symbols static
On Tue, 14 Jul 2020 22:22:45 +0800, Wei Yongjun wrote: > The sparse tool complains as follows: > > drivers/irqchip/irq-mips-gic.c:49:1: warning: > symbol '__pcpu_scope_pcpu_masks' was not declared. Should it be static? > drivers/irqchip/irq-mips-gic.c:620:6: warning: > symbol 'gic_ipi_domain_free' was not declared. Should it be static? > drivers/irqchip/irq-mips-gic.c:634:5: warning: > symbol 'gic_ipi_domain_match' was not declared. Should it be static? > > [...] Applied to irq/irqchip-5.9, thanks! [1/1] irqchip/mips-gic: Make local symbols static commit: 63bf3444359c94d647c2afa79b5e732585469581 Cheers, M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH] genirq/irqdomain: Remove redundant NULL pointer check on fwnode
On Thu, 16 Jul 2020 16:39:05 +0800, Zenghui Yu wrote: > The is_fwnode_irqchip() helper will check if the fwnode_handle is empty. > There is no need to perform a redundant check outside of it. Applied to irq/irqchip-5.9, thanks! [1/1] genirq/irqdomain: Remove redundant NULL pointer check on fwnode commit: 47903428b0e9db7a6251aa696fd1b2fc5de98545 Cheers, M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH 0/6] irqchip: Broadcom STB interrupt controller updates
On Thu, 9 Jul 2020 15:30:10 -0700, Florian Fainelli wrote: > This patch series contains a number of updates for Broadcom STB L2 > interrupt controllers to enable them as wake-up interrupt controllers, > and add missing compatible strings that should be matched. > > Thanks! > > Florian Fainelli (3): > dt-bindings: interrupt-controller: Document Broadcom STB HIF L2 > dt-bindings: interrupt-controller: Document UPG auxiliary L2 > irqchip/brcmstb-l2: Match UPG_AUX_AON_INTR2 compatible > > [...] Applied to irq/irqchip-5.9, thanks! [1/6] irqchip/bcm7120-l2: Set controller as wake-up source commit: f4ccb74569aaf839c2830382e902dd50d564df55 [2/6] irqchip/brcmstb-l2: Set controller as wake-up source commit: c8d8d6fc478a30f3e8ea5372664dd2a808c4311e [3/6] dt-bindings: interrupt-controller: Document Broadcom STB HIF L2 commit: 90b06e2dc4d1e8e9311a5275d53f61d90b61efdc [4/6] irqchip/brcmstb-l2: Match HIF_SPI_INTR2 compatible commit: 9ac793dc5c97691152818305974299604c67e110 [5/6] dt-bindings: interrupt-controller: Document UPG auxiliary L2 commit: 03a7ac47c14c7ef50742a34b3cfba1a47a578a03 [6/6] irqchip/brcmstb-l2: Match UPG_AUX_AON_INTR2 compatible commit: 240e176a96187ee84e63626ca0d1aac92da503aa Cheers, M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH] irqchip/irq-bcm7038-l1: Allow building on ARM 32-bit
On Thu, 9 Jul 2020 16:41:41 -0700, Florian Fainelli wrote: > We need to have a definition for cpu_logical_map[] which on ARM > platforms is provided by asm/smp_plat.h. This header is not > automatically included from linux/smp.h and untangling it is a bit > difficult. Applied to irq/irqchip-5.9, thanks! [1/1] irqchip/irq-bcm7038-l1: Allow building on ARM 32-bit commit: 52b350cbc94721d87f087d1a5800f9531c2d682c Cheers, M. -- Without deviation from the norm, progress is not possible.
Re: [RESEND PATCH] irqchip/stm32-exti: Use the hwspin_lock_timeout_in_atomic() API
On Mon, 6 Jul 2020 10:11:15 +0200, Alexandre Torgue wrote: > Now that the hwspin_lock_timeout_in_atomic() API is available use it. > > Signed-off-by: Fabien Dessenne > Signed-off-by: Alexandre Torgue > > diff --git a/drivers/irqchip/irq-stm32-exti.c > b/drivers/irqchip/irq-stm32-exti.c > index faa8482c8246..c7ab69694931 100644 > --- a/drivers/irqchip/irq-stm32-exti.c > +++ b/drivers/irqchip/irq-stm32-exti.c > @@ -25,7 +25,6 @@ > #define IRQS_PER_BANK 32 > > [...] Applied to irq/irqchip-5.9, thanks! [1/1] irqchip/stm32-exti: Use the hwspin_lock_timeout_in_atomic() API commit: e5c19cf32b68d8c59cd3e94e257ab030f07db7d6 Cheers, M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH] irqchip/stm32-exti: map direct event to irq parent
On Fri, 10 Jul 2020 10:37:47 +0100,
Alexandre Torgue wrote:
>
> Hi Marc,
>
> On 7/10/20 11:31 AM, Marc Zyngier wrote:
> > Alexandre,
> >
> > On Wed, 08 Jul 2020 05:57:24 +0100,
> > kernel test robot wrote:
> >>
> >> [1 ]
> >> Hi Alexandre,
> >>
> >> I love your patch! Perhaps something to improve:
> >>
> >> [auto build test WARNING on stm32/stm32-next]
> >> [also build test WARNING on soc/for-next v5.8-rc4 next-20200707]
> >> [If your patch is applied to the wrong git tree, kindly drop us a note.
> >> And when submitting patch, we suggest to use as documented in
> >> https://git-scm.com/docs/git-format-patch]
> >>
> >> url:
> >> https://github.com/0day-ci/linux/commits/Alexandre-Torgue/irqchip-stm32-exti-map-direct-event-to-irq-parent/20200706-161327
> >> base: https://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git
> >> stm32-next
> >> config: arm-randconfig-s031-20200707 (attached as .config)
> >> compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
> >> reproduce:
> >> wget
> >> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
> >> -O ~/bin/make.cross
> >> chmod +x ~/bin/make.cross
> >> # apt-get install sparse
> >> # sparse version: v0.6.2-31-gabbfd661-dirty
> >> # save the attached .config to linux build tree
> >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross
> >> C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=arm
> >>
> >> If you fix the issue, kindly add following tag as appropriate
> >> Reported-by: kernel test robot
> >>
> >> All warnings (new ones prefixed by >>):
> >>
> >> In file included from include/linux/build_bug.h:5,
> >> from include/linux/bits.h:23,
> >> from include/linux/bitops.h:5,
> >> from drivers/irqchip/irq-stm32-exti.c:8:
> >> drivers/irqchip/irq-stm32-exti.c: In function
> >> 'stm32_exti_h_domain_alloc':
> >> drivers/irqchip/irq-stm32-exti.c:683:23: warning: comparison of
> >> unsigned expression >= 0 is always true [-Wtype-limits]
> >> 683 | if (desc->irq_parent >= 0) {
> >> | ^~
> >> include/linux/compiler.h:58:52: note: in definition of macro
> >> '__trace_if_var'
> >>58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ?
> >> (cond) : __trace_if_value(cond))
> >> |^~~~
> >>>> drivers/irqchip/irq-stm32-exti.c:683:2: note: in expansion of macro 'if'
> >> 683 | if (desc->irq_parent >= 0) {
> >
> > Do you plan to address this? Looks like an actual bug to me.
>
> I'll fix it in v2, I was just waiting for other comments before
> sending a v2. Do you prefer I send a v2 with this fix, and you'll do
> your review on this v2 ?
I'm certainly not going to review something that has such a glaring
issue.
M.
--
Without deviation from the norm, progress is not possible.
Re: [PATCHv3 2/6] irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts
Suman, Grzegorz, On Wed, 15 Jul 2020 14:38:05 +0100, Grzegorz Jaszczyk wrote: > > Hi Marc, > > > On 7/8/20 5:47 AM, Marc Zyngier wrote: > > > On 2020-07-08 08:04, Grzegorz Jaszczyk wrote: > > >> On Sun, 5 Jul 2020 at 22:45, Marc Zyngier wrote: > > >>> > > >>> On 2020-07-05 14:26, Grzegorz Jaszczyk wrote: > > >>> > On Sat, 4 Jul 2020 at 11:39, Marc Zyngier wrote: > > >>> >> > > >>> >> On 2020-07-03 15:28, Grzegorz Jaszczyk wrote: > > >>> > > >>> [...] > > >>> > > >>> >> It still begs the question: if the HW can support both edge and level > > >>> >> triggered interrupts, why isn't the driver supporting this diversity? > > >>> >> I appreciate that your HW may only have level interrupts so far, but > > >>> >> what guarantees that this will forever be true? It would imply a > > >>> >> change > > >>> >> in the DT binding, which isn't desirable. > > >>> > > > >>> > Ok, I've got your point. I will try to come up with something later > > >>> > on. Probably extending interrupt-cells by one and passing interrupt > > >>> > type will be enough for now. Extending this driver to actually support > > >>> > it can be handled later if needed. Hope it works for you. > > >>> > > >>> Writing a set_type callback to deal with this should be pretty easy. > > >>> Don't delay doing the right thing. > > >> > > >> Ok. > > > > Sorry for the typo in my comment causing this confusion. > > > > The h/w actually doesn't support the edge-interrupts. Likewise, the > > polarity is always high. The individual register bit descriptions > > mention what the bit values 0 and 1 mean, but there is additional > > description in the TRMs on all the SoCs that says > > "always write 1 to the bits of this register" for PRUSS_INTC_SIPR(x) and > > "always write 0 to the bits of this register" for PRUSS_INTC_SITR(x). > > FWIW, these are also the reset values. > > > > Eg: AM335x TRM - https://www.ti.com/lit/pdf/spruh73 > > Please see Section 4.4.2.5 and the register descriptions in 4.5.3.49, > > 4.5.3.51. Please also see Section 4.4.2.3 that explains the PRUSS INTC > > methodology. > > > > >> > > >>> > > >>> [...] > > >>> > > >>> >> >> > + hwirq = hipir & GENMASK(9, 0); > > >>> >> >> > + virq = irq_linear_revmap(intc->domain, hwirq); > > >>> >> >> > > >>> >> >> And this is where I worry. You seems to have a single irqdomain > > >>> >> >> for all the muxes. Are you guaranteed that you will have no > > >>> >> >> overlap between muxes? And please use irq_find_mapping(), as > > >>> >> >> I have top-secret plans to kill irq_linear_revmap(). > > >>> >> > > > >>> >> > Regarding irq_find_mapping - sure. > > >>> >> > > > >>> >> > Regarding irqdomains: > > >>> >> > It is a single irqdomain since the hwirq (system event) can be > > >>> mapped > > >>> >> > to different irq_host (muxes). Patch #6 > > >>> >> > https://lkml.org/lkml/2020/7/2/616 implements and describes how > > >>> input > > >>> >> > events can be mapped to some output host interrupts through 2 > > >>> levels > > >>> >> > of many-to-one mapping i.e. events to channel mapping and > > >>> channels to > > >>> >> > host interrupts. Mentioned implementation ensures that specific > > >>> system > > >>> >> > event (hwirq) can be mapped through PRUSS specific channel into a > > >>> >> > single host interrupt. > > >>> >> > > >>> >> Patch #6 is a nightmare of its own, and I haven't fully groked it > > >>> yet. > > >>> >> Also, this driver seems to totally ignore the 2-level routing. Where > > >>> >> is it set up? map/unmap in this driver do exactly *nothing*, so > > >>> >> something somewhere must set it up. > > >>> > > > >>> > Th
Re: [PATCH] irqchip/gic-v4.1: Ensure accessing the correct RD when writing INVALLR
On Thu, 09 Jul 2020 14:49:59 +0100,
Zenghui Yu wrote:
>
> The GICv4.1 spec tells us that it's CONSTRAINED UNPREDICTABLE to issue a
> register-based invalidation operation for a vPEID not mapped to that RD,
> or another RD within the same CommonLPIAff group.
>
> To follow this rule, commit f3a059219bc7 ("irqchip/gic-v4.1: Ensure mutual
> exclusion between vPE affinity change and RD access") tried to address the
> race between the RD accesses and the vPE affinity change, but somehow
> forgot to take GICR_INVALLR into account. Let's take the vpe_lock before
> evaluating vpe->col_idx to fix it.
>
> Signed-off-by: Zenghui Yu
Shouldn't this deserve a Fixes: tag?
Thanks,
M.
> ---
> drivers/irqchip/irq-gic-v3-its.c | 10 +++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c
> b/drivers/irqchip/irq-gic-v3-its.c
> index da44bfa48bc2..50a04cca8207 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -4087,18 +4087,22 @@ static void its_vpe_4_1_deschedule(struct its_vpe
> *vpe,
> static void its_vpe_4_1_invall(struct its_vpe *vpe)
> {
> void __iomem *rdbase;
> + unsigned long flags;
> u64 val;
> + int cpu;
>
> val = GICR_INVALLR_V;
> val |= FIELD_PREP(GICR_INVALLR_VPEID, vpe->vpe_id);
>
> /* Target the redistributor this vPE is currently known on */
> - raw_spin_lock(&gic_data_rdist_cpu(vpe->col_idx)->rd_lock);
> - rdbase = per_cpu_ptr(gic_rdists->rdist, vpe->col_idx)->rd_base;
> + cpu = vpe_to_cpuid_lock(vpe, &flags);
> + raw_spin_lock(&gic_data_rdist_cpu(cpu)->rd_lock);
> + rdbase = per_cpu_ptr(gic_rdists->rdist, cpu)->rd_base;
> gic_write_lpir(val, rdbase + GICR_INVALLR);
>
> wait_for_syncr(rdbase);
> - raw_spin_unlock(&gic_data_rdist_cpu(vpe->col_idx)->rd_lock);
> + raw_spin_unlock(&gic_data_rdist_cpu(cpu)->rd_lock);
> + vpe_to_cpuid_unlock(vpe, flags);
> }
>
> static int its_vpe_4_1_set_vcpu_affinity(struct irq_data *d, void *vcpu_info)
> --
> 2.19.1
>
>
--
Without deviation from the norm, progress is not possible.
Re: [PATCHv3 3/6] irqchip/irq-pruss-intc: Add support for shared and invalid interrupts
On Fri, 10 Jul 2020 21:59:17 +0100,
Suman Anna wrote:
Hi Suman,
[...]
>
> Hi Marc,
>
> On 7/2/20 12:44 PM, Marc Zyngier wrote:
> > On 2020-07-02 15:17, Grzegorz Jaszczyk wrote:
> >> From: Suman Anna
> >>
> >> The PRUSS INTC has a fixed number of output interrupt lines that are
> >> connected to a number of processors or other PRUSS instances or other
> >> devices (like DMA) on the SoC. The output interrupt lines 2 through 9
> >> are usually connected to the main Arm host processor and are referred
> >> to as host interrupts 0 through 7 from ARM/MPU perspective.
> >>
> >> All of these 8 host interrupts are not always exclusively connected
> >> to the Arm interrupt controller. Some SoCs have some interrupt lines
> >> not connected to the Arm interrupt controller at all, while a few others
> >> have the interrupt lines connected to multiple processors in which they
> >> need to be partitioned as per SoC integration needs. For example, AM437x
> >> and 66AK2G SoCs have 2 PRUSS instances each and have the host interrupt 5
> >> connected to the other PRUSS, while AM335x has host interrupt 0 shared
> >> between MPU and TSC_ADC and host interrupts 6 & 7 shared between MPU and
> >> a DMA controller.
> >>
> >> Add support to the PRUSS INTC driver to allow both these shared and
> >> invalid interrupts by not returning a failure if any of these interrupts
> >> are skipped from the corresponding INTC DT node.
> >
> > That's not exactly "adding support", is it? It really is "ignore these
> > interrupts because they are useless from the main CPU's perspective",
> > right?
>
> Correct. We can rephrase this to something like
> "Add logic to the PRUSS INTC driver to ignore.."
>
> >
> >>
> >> Signed-off-by: Suman Anna
> >> Signed-off-by: Grzegorz Jaszczyk
> >> ---
> >> v2->v3:
> >> - Extra checks for (intc->irqs[i]) in error/remove path was moved from
> >> "irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS
> >> interrupts" to this patch
> >> v1->v2:
> >> - https://patchwork.kernel.org/patch/11069757/
> >> ---
> >> drivers/irqchip/irq-pruss-intc.c | 73
> >> +---
> >> 1 file changed, 68 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/irqchip/irq-pruss-intc.c
> >> b/drivers/irqchip/irq-pruss-intc.c
> >> index fb3dda3..49c936f 100644
> >> --- a/drivers/irqchip/irq-pruss-intc.c
> >> +++ b/drivers/irqchip/irq-pruss-intc.c
> >> @@ -65,11 +65,15 @@
> >> * @irqs: kernel irq numbers corresponding to PRUSS host interrupts
> >> * @base: base virtual address of INTC register space
> >> * @domain: irq domain for this interrupt controller
> >> + * @shared_intr: bit-map denoting if the MPU host interrupt is shared
> >
> > nit: bitmap
>
> ok
>
> >
> >> + * @invalid_intr: bit-map denoting if host interrupt is not
> >> connected to MPU
> >> */
> >> struct pruss_intc {
> >> unsigned int irqs[MAX_NUM_HOST_IRQS];
> >> void __iomem *base;
> >> struct irq_domain *domain;
> >> + u16 shared_intr;
> >> + u16 invalid_intr;
> >
> > Please represent bitmaps as an unsigned long.
>
> ok. We have atmost 8 interrupts coming in, but agree on the change
> since we are using the BIT() macro below.
>
> >
> >> };
> >>
> >> static inline u32 pruss_intc_read_reg(struct pruss_intc *intc,
> >> unsigned int reg)
> >> @@ -222,7 +226,8 @@ static int pruss_intc_probe(struct
> >> platform_device *pdev)
> >> "host_intr4", "host_intr5", "host_intr6", "host_intr7", };
> >> struct device *dev = &pdev->dev;
> >> struct pruss_intc *intc;
> >> - int i, irq;
> >> + int i, irq, count;
> >> + u8 temp_intr[MAX_NUM_HOST_IRQS] = { 0 };
> >>
> >> intc = devm_kzalloc(dev, sizeof(*intc), GFP_KERNEL);
> >> if (!intc)
> >> @@ -235,6 +240,52 @@ static int pruss_intc_probe(struct
> >> platform_device *pdev)
> >> return PTR_ERR(intc->base);
> >> }
> >>
> >> + count = of_property_read_variable_u8_array(dev->of_node,
> >> + "ti,irqs-reserved",
> >> +
Re: [PATCH v2] irqchip: Add IRQCHIP_PLATFORM_DRIVER_BEGIN/END and IRQCHIP_MATCH helper macros
Hi Saravana,
Thanks for re-spinning this one.
On Fri, 17 Jul 2020 03:44:47 +0100,
Saravana Kannan wrote:
>
> Compiling an irqchip driver as a platform driver needs to bunch of
> things to be done right:
> - Making sure the parent domain is initialized first
> - Making sure the device can't be unbound from sysfs
> - Disallowing module unload if it's built as a module
> - Finding the parent node
> - Etc.
>
> Instead of trying to make sure all future irqchip platform drivers get
> this right, provide boilerplate macros that take care of all of this.
>
> An example use would look something like this. Where acme_foo_init and
> acme_bar_init are similar to what would be passed to IRQCHIP_DECLARE.
>
> IRQCHIP_PLATFORM_DRIVER_BEGIN
I think there is some value in having the BEGIN statement containing
the driver name, see below.
> IRQCHIP_MATCH(foo, "acme,foo", acme_foo_init)
> IRQCHIP_MATCH(bar, "acme,bar", acme_bar_init)
> IRQCHIP_PLATFORM_DRIVER_END(acme_irq)
>
> Signed-off-by: Saravana Kannan
> ---
> drivers/irqchip/irqchip.c | 22 ++
> include/linux/irqchip.h | 23 +++
> 2 files changed, 45 insertions(+)
>
> diff --git a/drivers/irqchip/irqchip.c b/drivers/irqchip/irqchip.c
> index 2b35e68bea82..236ea793f01c 100644
> --- a/drivers/irqchip/irqchip.c
> +++ b/drivers/irqchip/irqchip.c
> @@ -10,8 +10,10 @@
>
> #include
> #include
> +#include
> #include
> #include
> +#include
>
> /*
> * This special of_device_id is the sentinel at the end of the
> @@ -29,3 +31,23 @@ void __init irqchip_init(void)
> of_irq_init(__irqchip_of_table);
> acpi_probe_device_table(irqchip);
> }
> +
> +int platform_irqchip_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct device_node *par_np = of_irq_find_parent(np);
> + of_irq_init_cb_t irq_init_cb = of_device_get_match_data(&pdev->dev);
> +
> + if (!irq_init_cb)
> + return -EINVAL;
> +
> + if (par_np == np)
> + par_np = NULL;
> +
> + /* If there's a parent irqchip, make sure it has been initialized. */
> + if (par_np && !irq_find_matching_host(np, DOMAIN_BUS_ANY))
There is no guarantee that the calling driver wants BUS_ANY as a token
for its parent. It may work for now, if you only have dependencies to
drivers that only expose a single domain, but that's not a general
purpose check..
At least, please add a comment saying that the new driver may want to
check that the irqdomain it depends on may not be available.
> + return -EPROBE_DEFER;
> +
> + return irq_init_cb(np, par_np);
> +}
> +EXPORT_SYMBOL_GPL(platform_irqchip_probe);
> diff --git a/include/linux/irqchip.h b/include/linux/irqchip.h
> index 950e4b2458f0..6d5eba7cbbb7 100644
> --- a/include/linux/irqchip.h
> +++ b/include/linux/irqchip.h
> @@ -13,6 +13,7 @@
>
> #include
> #include
> +#include
>
> /*
> * This macro must be used by the different irqchip drivers to declare
> @@ -26,6 +27,28 @@
> */
> #define IRQCHIP_DECLARE(name, compat, fn) OF_DECLARE_2(irqchip, name,
> compat, fn)
>
> +extern int platform_irqchip_probe(struct platform_device *pdev);
> +
> +#define IRQCHIP_PLATFORM_DRIVER_BEGIN \
> +static const struct of_device_id __irqchip_match_table[] = {
How about:
#define IRQCHIP_PLATFORM_DRIVER_BEGIN(drv_name) \
static const struct of_device_id __irqchip_match_table_##drv_name = {
which makes it easier to debug when you want to identify specific
structures in an object (otherwise, they all have the same
name...). it is also much more pleasing aesthetically ;-).
> +
> +#define IRQCHIP_MATCH(compat, fn) { .compatible = compat, .data = fn },
> +
> +#define IRQCHIP_PLATFORM_DRIVER_END(drv_name)\
> + {}, \
> +}; \
> +MODULE_DEVICE_TABLE(of, __irqchip_match_table); \
> +static struct platform_driver drv_name##_driver = { \
const?
> + .probe = platform_irqchip_probe, \
> + .driver = { \
> + .name = #drv_name, \
> + .owner = THIS_MODULE, \
> + .of_match_table = __irqchip_match_table,\
> + .suppress_bind_attrs = true,\
> + }, \
> +}; \
> +builtin_platform_driver(drv_name##_driver)
> +
> /*
> * This macro must be used by the different irqchip drivers to declare
> * the association between their version and their initialization function.
> --
> 2.28.0.rc0.105.gf9edc3c819-goog
>
>
Otherwise looks good. When you respin it, it would be good to also
submit one user of this API by converting an existing driver, as I'd
hate to merge something that has no user.
Thanks,
M.
--
Without deviation from the norm, progress is no
Re: [PATCH 2/2] irqchip: imx-intmux: add runtime PM support
On 2020-07-16 20:32, Joakim Zhang wrote:
Add runtime PM support for intmux interrupt controller.
Same as the previous patch. The changes are significant enough
that you need to write a proper change log.
Signed-off-by: Joakim Zhang
---
drivers/irqchip/irq-imx-intmux.c | 31 +--
1 file changed, 25 insertions(+), 6 deletions(-)
diff --git a/drivers/irqchip/irq-imx-intmux.c
b/drivers/irqchip/irq-imx-intmux.c
index 6095f76c4f0d..a631815c7bb4 100644
--- a/drivers/irqchip/irq-imx-intmux.c
+++ b/drivers/irqchip/irq-imx-intmux.c
@@ -53,6 +53,7 @@
#include
#include
#include
+#include
#define CHANIER(n) (0x10 + (0x40 * n))
#define CHANIPR(n) (0x20 + (0x40 * n))
@@ -60,6 +61,7 @@
#define CHAN_MAX_NUM 0x8
struct intmux_irqchip_data {
+ struct irq_chip chip;
int chanidx;
int irq;
struct irq_domain *domain;
@@ -123,8 +125,10 @@ static struct irq_chip imx_intmux_irq_chip = {
static int imx_intmux_irq_map(struct irq_domain *h, unsigned int irq,
irq_hw_number_t hwirq)
{
- irq_set_chip_data(irq, h->host_data);
- irq_set_chip_and_handler(irq, &imx_intmux_irq_chip,
handle_level_irq);
+ struct intmux_irqchip_data *data = h->host_data;
+
+ irq_set_chip_data(irq, data);
+ irq_set_chip_and_handler(irq, &data->chip, handle_level_irq);
return 0;
}
@@ -244,6 +248,10 @@ static int imx_intmux_probe(struct platform_device
*pdev)
return -ENOMEM;
}
+ pm_runtime_get_noresume(&pdev->dev);
+ pm_runtime_set_active(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
+
ret = clk_prepare_enable(data->ipg_clk);
if (ret) {
dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
@@ -251,6 +259,8 @@ static int imx_intmux_probe(struct platform_device
*pdev)
}
for (i = 0; i < channum; i++) {
+ data->irqchip_data[i].chip = imx_intmux_irq_chip;
+ data->irqchip_data[i].chip.parent_device = &pdev->dev;
At some point, we will have to find a way to throw the parent_device
thing out of the irq_chip structure. This thing is a liability.
Nothing you can do about it for now though.
data->irqchip_data[i].chanidx = i;
data->irqchip_data[i].irq = irq_of_parse_and_map(np, i);
@@ -279,6 +289,12 @@ static int imx_intmux_probe(struct platform_device
*pdev)
platform_set_drvdata(pdev, data);
+ /*
+* Let pm_runtime_put() disable clock.
+* If CONFIG_PM is not enabled, the clock will stay powered.
+*/
+ pm_runtime_put(&pdev->dev);
+
return 0;
out:
clk_disable_unprepare(data->ipg_clk);
@@ -300,7 +316,7 @@ static int imx_intmux_remove(struct platform_device
*pdev)
irq_domain_remove(data->irqchip_data[i].domain);
}
- clk_disable_unprepare(data->ipg_clk);
+ pm_runtime_disable(&pdev->dev);
return 0;
}
@@ -322,7 +338,7 @@ static void imx_intmux_restore_regs(struct
intmux_data *data)
writel_relaxed(data->saved_reg[i], data->regs + CHANIER(i));
}
-static int imx_intmux_suspend(struct device *dev)
+static int imx_intmux_runtime_suspend(struct device *dev)
{
struct intmux_data *data = dev_get_drvdata(dev);
@@ -332,7 +348,7 @@ static int imx_intmux_suspend(struct device *dev)
return 0;
}
-static int imx_intmux_resume(struct device *dev)
+static int imx_intmux_runtime_resume(struct device *dev)
You just introduced these two functions, and rename them immediately?
{
struct intmux_data *data = dev_get_drvdata(dev);
int ret;
@@ -349,7 +365,10 @@ static int imx_intmux_resume(struct device *dev)
#endif
static const struct dev_pm_ops imx_intmux_pm_ops = {
- SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_intmux_suspend, imx_intmux_resume)
+ SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
+ SET_RUNTIME_PM_OPS(imx_intmux_runtime_suspend,
+ imx_intmux_runtime_resume, NULL)
};
static const struct of_device_id imx_intmux_id[] = {
I think you'd might as well squash the two patches together.
Splitting the two serves little purpose.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Re: [PATCH 1/2] irqchip: imx-intmux: add system PM support
On 2020-07-16 20:32, Joakim Zhang wrote:
Add system PM support for intmux interrupt controller.
Care to be a little more descriptive?
Signed-off-by: Joakim Zhang
---
drivers/irqchip/irq-imx-intmux.c | 59
1 file changed, 59 insertions(+)
diff --git a/drivers/irqchip/irq-imx-intmux.c
b/drivers/irqchip/irq-imx-intmux.c
index c27577c81126..6095f76c4f0d 100644
--- a/drivers/irqchip/irq-imx-intmux.c
+++ b/drivers/irqchip/irq-imx-intmux.c
@@ -70,6 +70,9 @@ struct intmux_data {
void __iomem*regs;
struct clk *ipg_clk;
int channum;
+#ifdef CONFIG_PM
+ unsigned int*saved_reg;
Please use u32 for 32bit HW registers.
+#endif
struct intmux_irqchip_data irqchip_data[];
};
@@ -232,6 +235,15 @@ static int imx_intmux_probe(struct platform_device
*pdev)
data->channum = channum;
raw_spin_lock_init(&data->lock);
+ if (IS_ENABLED(CONFIG_PM)) {
+ /* save CHANIER register */
+ data->saved_reg = devm_kzalloc(&pdev->dev,
+ sizeof(unsigned int) * channum,
+ GFP_KERNEL);
+ if (!data->saved_reg)
Which isn't defined when !CONFIG_PM. The compiler is allowed to
bail here, and does (see the two kbuild failures).
+ return -ENOMEM;
+ }
+
ret = clk_prepare_enable(data->ipg_clk);
if (ret) {
dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
@@ -293,6 +305,53 @@ static int imx_intmux_remove(struct
platform_device *pdev)
return 0;
}
+#ifdef CONFIG_PM
+static void imx_intmux_save_regs(struct intmux_data *data)
+{
+ int i;
+
+ for (i = 0; i < data->channum; i++)
+ data->saved_reg[i] = readl_relaxed(data->regs + CHANIER(i));
+}
+
+static void imx_intmux_restore_regs(struct intmux_data *data)
+{
+ int i;
+
+ for (i = 0; i < data->channum; i++)
+ writel_relaxed(data->saved_reg[i], data->regs + CHANIER(i));
+}
Please move these two trivial functions into their respective callers.
+
+static int imx_intmux_suspend(struct device *dev)
+{
+ struct intmux_data *data = dev_get_drvdata(dev);
+
+ imx_intmux_save_regs(data);
+ clk_disable_unprepare(data->ipg_clk);
+
+ return 0;
+}
+
+static int imx_intmux_resume(struct device *dev)
+{
+ struct intmux_data *data = dev_get_drvdata(dev);
+ int ret;
+
+ ret = clk_prepare_enable(data->ipg_clk);
+ if (ret) {
+ dev_err(dev, "failed to enable ipg clk: %d\n", ret);
+ return ret;
+ }
+ imx_intmux_restore_regs(data);
+
+ return 0;
+}
+#endif
+
+static const struct dev_pm_ops imx_intmux_pm_ops = {
+ SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_intmux_suspend, imx_intmux_resume)
+};
+
static const struct of_device_id imx_intmux_id[] = {
{ .compatible = "fsl,imx-intmux", },
{ /* sentinel */ },
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Re: [Stable-5.4][PATCH 0/3] arm64: Allow the compat vdso to be disabled at runtime
Hi Greg, On 2020-07-16 12:58, Greg KH wrote: On Wed, Jul 15, 2020 at 01:56:11PM +0100, Marc Zyngier wrote: This is a backport of the series that recently went into 5.8. Note that the first patch is more a complete rewriting than a backport, as the vdso implementation in 5.4 doesn't have much in common with mainline. This affects the 32bit arch code in a benign way. It has seen very little testing, as I don't have the HW that triggers this issue. I have run it in VMs by faking the CPU MIDR, and nothing caught fire. Famous last words. These are also needed in 5.7.y, right? If so, I need that series before I can take this one as we don't want people moving to a newer kernel and suffer regressions :( The original mainline changes: 4b661d6133c5 arm64: arch_timer: Disable the compat vdso for cores affected by ARM64_WORKAROUND_1418040 c1fbec4ac0d7 arm64: arch_timer: Allow an workaround descriptor to disable compat vdso 97884ca8c292 arm64: Introduce a way to disable the 32bit vdso do apply cleanly to stable-5.7. Do you want me to resend them separately, or will you pick the patches directly from mainline? Thanks, M. -- Jazz is not dead. It just smells funny...
Re: [REPORT] possible circular locking dependency when booting a VM on arm64 host
On 2020-07-16 01:58, Salil Mehta wrote:
From: Marc Zyngier [mailto:m...@kernel.org]
Sent: Wednesday, July 15, 2020 5:09 PM
To: yuzenghui
Hi Zenghui,
On 2020-07-09 11:41, Zenghui Yu wrote:
> Hi All,
>
> I had seen the following lockdep splat when booting a guest on my
> Kunpeng 920 with GICv4 enabled. I can also trigger the same splat
> on v5.5 so it should already exist in the kernel for a while. I'm
> not sure what the exact problem is and hope someone can have a look!
I can't manage to trigger this splat on my D05, despite running guests
with GICv4 enabled. A couple of questions below:
Sorry I forgot to update but I did try on Friday and I could not manage
to trigger it on D06/Kunpeng920 either. I used 5.8.0-rc4.
> Thanks,
> Zenghui
>
> [ 103.855511] ==
> [ 103.861664] WARNING: possible circular locking dependency detected
> [ 103.867817] 5.8.0-rc4+ #35 Tainted: GW
> [ 103.872932] --
> [ 103.879083] CPU 2/KVM/20515 is trying to acquire lock:
> [ 103.884200] 202fcd5865b0 (&irq_desc_lock_class){-.-.}-{2:2},
> at: __irq_get_desc_lock+0x60/0xa0
> [ 103.893127]
>but task is already holding lock:
> [ 103.898933] 202fcfd07f58 (&rq->lock){-.-.}-{2:2}, at:
> __schedule+0x114/0x8b8
> [ 103.906301]
>which lock already depends on the new lock.
>
> [ 103.914441]
>the existing dependency chain (in reverse order) is:
> [ 103.921888]
>-> #3 (&rq->lock){-.-.}-{2:2}:
> [ 103.927438]_raw_spin_lock+0x54/0x70
> [ 103.931605]task_fork_fair+0x48/0x150
> [ 103.935860]sched_fork+0x100/0x268
> [ 103.939856]copy_process+0x628/0x1868
> [ 103.944106]_do_fork+0x74/0x710
> [ 103.947840]kernel_thread+0x78/0xa0
> [ 103.951917]rest_init+0x30/0x270
> [ 103.955742]arch_call_rest_init+0x14/0x1c
> [ 103.960339]start_kernel+0x534/0x568
> [ 103.964503]
>-> #2 (&p->pi_lock){-.-.}-{2:2}:
> [ 103.970224]_raw_spin_lock_irqsave+0x70/0x98
> [ 103.975080]try_to_wake_up+0x5c/0x5b0
> [ 103.979330]wake_up_process+0x28/0x38
> [ 103.983581]create_worker+0x128/0x1b8
> [ 103.987834]workqueue_init+0x308/0x3bc
> [ 103.992172]kernel_init_freeable+0x180/0x33c
> [ 103.997027]kernel_init+0x18/0x118
> [ 104.001020]ret_from_fork+0x10/0x18
> [ 104.005097]
>-> #1 (&pool->lock){-.-.}-{2:2}:
> [ 104.010817]_raw_spin_lock+0x54/0x70
> [ 104.014983]__queue_work+0x120/0x6e8
> [ 104.019146]queue_work_on+0xa0/0xd8
> [ 104.023225]irq_set_affinity_locked+0xa8/0x178
> [ 104.028253]__irq_set_affinity+0x5c/0x90
> [ 104.032762]irq_set_affinity_hint+0x74/0xb0
> [ 104.037540]hns3_nic_init_irq+0xe0/0x210 [hns3]
> [ 104.042655]hns3_client_init+0x2d8/0x4e0 [hns3]
> [ 104.047779]hclge_init_client_instance+0xf0/0x3a8 [hclge]
> [ 104.053760]hnae3_init_client_instance.part.3+0x30/0x68
> [hnae3]
> [ 104.060257]hnae3_register_ae_dev+0x100/0x1f0 [hnae3]
> [ 104.065892]hns3_probe+0x60/0xa8 [hns3]
Are you performing some kind of PCIe hot-plug here? Or is that done
at boot only? It seems to help triggering the splat.
I am not sure how you can do that since HNS3 is integrated NIC so
physical hot-plug is definitely ruled out. local_pci_probe()
should also get called when we insert the hns3_enet module which
eventually initializes the driver.
> [ 104.070319]local_pci_probe+0x44/0x98
> [ 104.074573]work_for_cpu_fn+0x20/0x30
> [ 104.078823]process_one_work+0x258/0x618
> [ 104.08]worker_thread+0x1c0/0x438
> [ 104.087585]kthread+0x120/0x128
> [ 104.091318]ret_from_fork+0x10/0x18
> [ 104.095394]
>-> #0 (&irq_desc_lock_class){-.-.}-{2:2}:
> [ 104.101895]__lock_acquire+0x11bc/0x1530
> [ 104.106406]lock_acquire+0x100/0x3f8
> [ 104.110570]_raw_spin_lock_irqsave+0x70/0x98
> [ 104.115426]__irq_get_desc_lock+0x60/0xa0
> [ 104.120021]irq_set_vcpu_affinity+0x48/0xc8
> [ 104.124793]its_make_vpe_non_resident+0x6c/0xc0
> [ 104.129910]vgic_v4_put+0x64/0x70
> [ 104.133815]vgic_v3_put+0x28/0x100
> [ 104.137806]kvm_vgic_put+0x3c/0x60
> [ 104.141801]kvm_arch_vcpu_put+0x38/0x58
> [ 104.146228]kvm_sched_out+0x38/0x58
> [ 104.150306]__schedule+0x554/0x8b8
> [ 104.154298]schedule+0x50/0xe0
> [ 104.157946]
Re: [REPORT] possible circular locking dependency when booting a VM on arm64 host
Hi Zenghui,
On 2020-07-09 11:41, Zenghui Yu wrote:
Hi All,
I had seen the following lockdep splat when booting a guest on my
Kunpeng 920 with GICv4 enabled. I can also trigger the same splat
on v5.5 so it should already exist in the kernel for a while. I'm
not sure what the exact problem is and hope someone can have a look!
I can't manage to trigger this splat on my D05, despite running guests
with GICv4 enabled. A couple of questions below:
Thanks,
Zenghui
[ 103.855511] ==
[ 103.861664] WARNING: possible circular locking dependency detected
[ 103.867817] 5.8.0-rc4+ #35 Tainted: GW
[ 103.872932] --
[ 103.879083] CPU 2/KVM/20515 is trying to acquire lock:
[ 103.884200] 202fcd5865b0 (&irq_desc_lock_class){-.-.}-{2:2},
at: __irq_get_desc_lock+0x60/0xa0
[ 103.893127]
but task is already holding lock:
[ 103.898933] 202fcfd07f58 (&rq->lock){-.-.}-{2:2}, at:
__schedule+0x114/0x8b8
[ 103.906301]
which lock already depends on the new lock.
[ 103.914441]
the existing dependency chain (in reverse order) is:
[ 103.921888]
-> #3 (&rq->lock){-.-.}-{2:2}:
[ 103.927438]_raw_spin_lock+0x54/0x70
[ 103.931605]task_fork_fair+0x48/0x150
[ 103.935860]sched_fork+0x100/0x268
[ 103.939856]copy_process+0x628/0x1868
[ 103.944106]_do_fork+0x74/0x710
[ 103.947840]kernel_thread+0x78/0xa0
[ 103.951917]rest_init+0x30/0x270
[ 103.955742]arch_call_rest_init+0x14/0x1c
[ 103.960339]start_kernel+0x534/0x568
[ 103.964503]
-> #2 (&p->pi_lock){-.-.}-{2:2}:
[ 103.970224]_raw_spin_lock_irqsave+0x70/0x98
[ 103.975080]try_to_wake_up+0x5c/0x5b0
[ 103.979330]wake_up_process+0x28/0x38
[ 103.983581]create_worker+0x128/0x1b8
[ 103.987834]workqueue_init+0x308/0x3bc
[ 103.992172]kernel_init_freeable+0x180/0x33c
[ 103.997027]kernel_init+0x18/0x118
[ 104.001020]ret_from_fork+0x10/0x18
[ 104.005097]
-> #1 (&pool->lock){-.-.}-{2:2}:
[ 104.010817]_raw_spin_lock+0x54/0x70
[ 104.014983]__queue_work+0x120/0x6e8
[ 104.019146]queue_work_on+0xa0/0xd8
[ 104.023225]irq_set_affinity_locked+0xa8/0x178
[ 104.028253]__irq_set_affinity+0x5c/0x90
[ 104.032762]irq_set_affinity_hint+0x74/0xb0
[ 104.037540]hns3_nic_init_irq+0xe0/0x210 [hns3]
[ 104.042655]hns3_client_init+0x2d8/0x4e0 [hns3]
[ 104.047779]hclge_init_client_instance+0xf0/0x3a8 [hclge]
[ 104.053760]hnae3_init_client_instance.part.3+0x30/0x68
[hnae3]
[ 104.060257]hnae3_register_ae_dev+0x100/0x1f0 [hnae3]
[ 104.065892]hns3_probe+0x60/0xa8 [hns3]
Are you performing some kind of PCIe hot-plug here? Or is that done
at boot only? It seems to help triggering the splat.
[ 104.070319]local_pci_probe+0x44/0x98
[ 104.074573]work_for_cpu_fn+0x20/0x30
[ 104.078823]process_one_work+0x258/0x618
[ 104.08]worker_thread+0x1c0/0x438
[ 104.087585]kthread+0x120/0x128
[ 104.091318]ret_from_fork+0x10/0x18
[ 104.095394]
-> #0 (&irq_desc_lock_class){-.-.}-{2:2}:
[ 104.101895]__lock_acquire+0x11bc/0x1530
[ 104.106406]lock_acquire+0x100/0x3f8
[ 104.110570]_raw_spin_lock_irqsave+0x70/0x98
[ 104.115426]__irq_get_desc_lock+0x60/0xa0
[ 104.120021]irq_set_vcpu_affinity+0x48/0xc8
[ 104.124793]its_make_vpe_non_resident+0x6c/0xc0
[ 104.129910]vgic_v4_put+0x64/0x70
[ 104.133815]vgic_v3_put+0x28/0x100
[ 104.137806]kvm_vgic_put+0x3c/0x60
[ 104.141801]kvm_arch_vcpu_put+0x38/0x58
[ 104.146228]kvm_sched_out+0x38/0x58
[ 104.150306]__schedule+0x554/0x8b8
[ 104.154298]schedule+0x50/0xe0
[ 104.157946]kvm_arch_vcpu_ioctl_run+0x644/0x9e8
[ 104.163063]kvm_vcpu_ioctl+0x4b4/0x918
[ 104.167403]ksys_ioctl+0xb4/0xd0
[ 104.171222]__arm64_sys_ioctl+0x28/0xc8
[ 104.175647]el0_svc_common.constprop.2+0x74/0x138
[ 104.180935]do_el0_svc+0x34/0xa0
[ 104.184755]el0_sync_handler+0xec/0x128
[ 104.189179]el0_sync+0x140/0x180
[ 104.192997]
The kernel seems to scream at us because we have two paths
exercising the irq_desc_lock_class, one holding the rq->lock
because we are on the schedule out path.
These two locks are somehow unrelated (they just belong to the
same class), but the kernel cannot know that.
Not quite sure how to solve it though. The set_vcpu_affinity
call is necessary on the preemption path in order to make the
vpe non-resident. But it would help if I could reproduce it...
Thanks,
M.
--
Jazz is not dead. It just smells funny...
[Stable-5.4][PATCH 1/3] arm64: Introduce a way to disable the 32bit vdso
Commit 97884ca8c2925d14c32188e865069f21378b4b4f upstream.
[this is a redesign rather than a backport]
We have a class of errata (grouped under the ARM64_WORKAROUND_1418040
banner) that force the trapping of counter access from 32bit EL0.
We would normally disable the whole vdso for such defect, except that
it would disable it for 64bit userspace as well, which is a shame.
Instead, add a new vdso_clock_mode, which signals that the vdso
isn't usable for compat tasks. This gets checked in the
__arch_get_hw_counter() helper.
Signed-off-by: Marc Zyngier
Acked-by: Mark Rutland
Cc: sta...@vger.kernel.org
Link: https://lore.kernel.org/r/20200706163802.1836732-2-...@kernel.org
Signed-off-by: Will Deacon
Signed-off-by: Marc Zyngier
---
arch/arm/include/asm/clocksource.h| 11 ++-
arch/arm/kernel/vdso.c| 2 +-
arch/arm64/include/asm/clocksource.h | 5 -
arch/arm64/include/asm/vdso/clocksource.h | 14 ++
arch/arm64/include/asm/vdso/compat_gettimeofday.h | 5 +++--
arch/arm64/include/asm/vdso/gettimeofday.h| 6 --
arch/arm64/include/asm/vdso/vsyscall.h| 4 +---
drivers/clocksource/arm_arch_timer.c | 8
8 files changed, 41 insertions(+), 14 deletions(-)
create mode 100644 arch/arm64/include/asm/vdso/clocksource.h
diff --git a/arch/arm/include/asm/clocksource.h
b/arch/arm/include/asm/clocksource.h
index 0b350a7e26f3..afb7a59828fe 100644
--- a/arch/arm/include/asm/clocksource.h
+++ b/arch/arm/include/asm/clocksource.h
@@ -1,8 +1,17 @@
#ifndef _ASM_CLOCKSOURCE_H
#define _ASM_CLOCKSOURCE_H
+enum vdso_arch_clockmode {
+ /* vdso clocksource not usable */
+ VDSO_CLOCKMODE_NONE,
+ /* vdso clocksource usable */
+ VDSO_CLOCKMODE_ARCHTIMER,
+ VDSO_CLOCKMODE_ARCHTIMER_NOCOMPAT = VDSO_CLOCKMODE_ARCHTIMER,
+};
+
struct arch_clocksource_data {
- bool vdso_direct; /* Usable for direct VDSO access? */
+ /* Usable for direct VDSO access? */
+ enum vdso_arch_clockmode clock_mode;
};
#endif
diff --git a/arch/arm/kernel/vdso.c b/arch/arm/kernel/vdso.c
index f00e45fa62c4..6c69a5548ba2 100644
--- a/arch/arm/kernel/vdso.c
+++ b/arch/arm/kernel/vdso.c
@@ -281,7 +281,7 @@ static bool tk_is_cntvct(const struct timekeeper *tk)
if (!IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
return false;
- if (!tk->tkr_mono.clock->archdata.vdso_direct)
+ if (tk->tkr_mono.clock->archdata.clock_mode != VDSO_CLOCKMODE_ARCHTIMER)
return false;
return true;
diff --git a/arch/arm64/include/asm/clocksource.h
b/arch/arm64/include/asm/clocksource.h
index 0ece64a26c8c..0c7910447235 100644
--- a/arch/arm64/include/asm/clocksource.h
+++ b/arch/arm64/include/asm/clocksource.h
@@ -2,8 +2,11 @@
#ifndef _ASM_CLOCKSOURCE_H
#define _ASM_CLOCKSOURCE_H
+#include
+
struct arch_clocksource_data {
- bool vdso_direct; /* Usable for direct VDSO access? */
+ /* Usable for direct VDSO access? */
+ enum vdso_arch_clockmode clock_mode;
};
#endif
diff --git a/arch/arm64/include/asm/vdso/clocksource.h
b/arch/arm64/include/asm/vdso/clocksource.h
new file mode 100644
index ..8019f616e1f7
--- /dev/null
+++ b/arch/arm64/include/asm/vdso/clocksource.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __ASM_VDSOCLOCKSOURCE_H
+#define __ASM_VDSOCLOCKSOURCE_H
+
+enum vdso_arch_clockmode {
+ /* vdso clocksource not usable */
+ VDSO_CLOCKMODE_NONE,
+ /* vdso clocksource for both 32 and 64bit tasks */
+ VDSO_CLOCKMODE_ARCHTIMER,
+ /* vdso clocksource for 64bit tasks only */
+ VDSO_CLOCKMODE_ARCHTIMER_NOCOMPAT,
+};
+
+#endif
diff --git a/arch/arm64/include/asm/vdso/compat_gettimeofday.h
b/arch/arm64/include/asm/vdso/compat_gettimeofday.h
index c50ee1b7d5cd..413d42e197c7 100644
--- a/arch/arm64/include/asm/vdso/compat_gettimeofday.h
+++ b/arch/arm64/include/asm/vdso/compat_gettimeofday.h
@@ -10,6 +10,7 @@
#include
#include
+#include
#include
#define __VDSO_USE_SYSCALL ULLONG_MAX
@@ -117,10 +118,10 @@ static __always_inline u64 __arch_get_hw_counter(s32
clock_mode)
u64 res;
/*
-* clock_mode == 0 implies that vDSO are enabled otherwise
+* clock_mode == ARCHTIMER implies that vDSO are enabled otherwise
* fallback on syscall.
*/
- if (clock_mode)
+ if (clock_mode != VDSO_CLOCKMODE_ARCHTIMER)
return __VDSO_USE_SYSCALL;
/*
diff --git a/arch/arm64/include/asm/vdso/gettimeofday.h
b/arch/arm64/include/asm/vdso/gettimeofday.h
index b08f476b72b4..ff83b8b574fc 100644
--- a/arch/arm64/include/asm/vdso/gettimeofday.h
+++ b/arch/arm64/include/asm/vdso/gettimeofday.h
@@ -10,6 +10,8 @@
#include
#include
+#include
+
#define __VDSO_USE_SYSCALL ULLONG_MAX
#define VDSO_HAS_CLOCK_GETRES 1
@@ -71,10
[Stable-5.4][PATCH 0/3] arm64: Allow the compat vdso to be disabled at runtime
This is a backport of the series that recently went into 5.8. Note that the first patch is more a complete rewriting than a backport, as the vdso implementation in 5.4 doesn't have much in common with mainline. This affects the 32bit arch code in a benign way. It has seen very little testing, as I don't have the HW that triggers this issue. I have run it in VMs by faking the CPU MIDR, and nothing caught fire. Famous last words. The original cover letter follows. M. The relatively recent introduction of the compat vdso on arm64 has overlooked its interactions with some of the interesting errata workarounds, such as ARM64_ERRATUM_1418040 (and its older 1188873 incarnation). This erratum requires the 64bit kernel to trap 32bit accesses to the virtual counter and emulate it. When the workaround was introduced, the compat vdso simply wasn't a thing. Now that the patches have landed in mainline, we trap the CVTVCT accesses from the vdso. This can end-up in a nasty loop in the vdso, where the sequence number changes on each trap, never stabilising, and leaving userspace in a bit of a funny state (which is why we disable the vdso in most similar cases). This erratum mentionned above is a bit special in the sense that in only requires to trap AArch32 accesses, and 64bit tasks can be left alone. Consequently, the vdso is never disabled and AArch32 tasks are affected. Obviously, we really want to retain the 64bit vdso in this case. To that effect, this series offers a way to disable the 32bit view of the vdso without impacting its 64bit counterpart, by providing a "no-compat" vdso clock_mode, and plugging this feature into the 1418040 detection code. Marc Zyngier (3): arm64: Introduce a way to disable the 32bit vdso arm64: arch_timer: Allow an workaround descriptor to disable compat vdso arm64: arch_timer: Disable the compat vdso for cores affected by ARM64_WORKAROUND_1418040 arch/arm/include/asm/clocksource.h| 11 ++- arch/arm/kernel/vdso.c| 2 +- arch/arm64/include/asm/arch_timer.h | 1 + arch/arm64/include/asm/clocksource.h | 5 - arch/arm64/include/asm/vdso/clocksource.h | 14 ++ .../include/asm/vdso/compat_gettimeofday.h| 5 +++-- arch/arm64/include/asm/vdso/gettimeofday.h| 6 -- arch/arm64/include/asm/vdso/vsyscall.h| 4 +--- drivers/clocksource/arm_arch_timer.c | 19 +++ 9 files changed, 53 insertions(+), 14 deletions(-) create mode 100644 arch/arm64/include/asm/vdso/clocksource.h -- 2.27.0
[Stable-5.4][PATCH 3/3] arm64: arch_timer: Disable the compat vdso for cores affected by ARM64_WORKAROUND_1418040
Commit 4b661d6133c5d3a7c9aca0b4ee5a78c7766eff3f upstream.
ARM64_WORKAROUND_1418040 requires that AArch32 EL0 accesses to
the virtual counter register are trapped and emulated by the kernel.
This makes the vdso pretty pointless, and in some cases livelock
prone.
Provide a workaround entry that limits the vdso to 64bit tasks.
Signed-off-by: Marc Zyngier
Acked-by: Mark Rutland
Cc: sta...@vger.kernel.org
Link: https://lore.kernel.org/r/20200706163802.1836732-4-...@kernel.org
Signed-off-by: Will Deacon
Signed-off-by: Marc Zyngier
---
drivers/clocksource/arm_arch_timer.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/clocksource/arm_arch_timer.c
b/drivers/clocksource/arm_arch_timer.c
index fd2a75f0af77..4be83b4de2a0 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -476,6 +476,14 @@ static const struct arch_timer_erratum_workaround
ool_workarounds[] = {
.set_next_event_virt = erratum_set_next_event_tval_virt,
},
#endif
+#ifdef CONFIG_ARM64_ERRATUM_1418040
+ {
+ .match_type = ate_match_local_cap_id,
+ .id = (void *)ARM64_WORKAROUND_1418040,
+ .desc = "ARM erratum 1418040",
+ .disable_compat_vdso = true,
+ },
+#endif
};
typedef bool (*ate_match_fn_t)(const struct arch_timer_erratum_workaround *,
--
2.27.0
[Stable-5.4][PATCH 2/3] arm64: arch_timer: Allow an workaround descriptor to disable compat vdso
Commit c1fbec4ac0d701f350a581941d35643d5a9cd184 upstream.
As we are about to disable the vdso for compat tasks in some circumstances,
let's allow a workaround descriptor to express exactly that.
Signed-off-by: Marc Zyngier
Acked-by: Mark Rutland
Cc: sta...@vger.kernel.org
Link: https://lore.kernel.org/r/20200706163802.1836732-3-...@kernel.org
Signed-off-by: Will Deacon
Signed-off-by: Marc Zyngier
---
arch/arm64/include/asm/arch_timer.h | 1 +
drivers/clocksource/arm_arch_timer.c | 3 +++
2 files changed, 4 insertions(+)
diff --git a/arch/arm64/include/asm/arch_timer.h
b/arch/arm64/include/asm/arch_timer.h
index 7ae54d7d333a..9f0ec21d6327 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -58,6 +58,7 @@ struct arch_timer_erratum_workaround {
u64 (*read_cntvct_el0)(void);
int (*set_next_event_phys)(unsigned long, struct clock_event_device *);
int (*set_next_event_virt)(unsigned long, struct clock_event_device *);
+ bool disable_compat_vdso;
};
DECLARE_PER_CPU(const struct arch_timer_erratum_workaround *,
diff --git a/drivers/clocksource/arm_arch_timer.c
b/drivers/clocksource/arm_arch_timer.c
index 909fe093249e..fd2a75f0af77 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -562,6 +562,9 @@ void arch_timer_enable_workaround(const struct
arch_timer_erratum_workaround *wa
if (wa->read_cntvct_el0) {
clocksource_counter.archdata.clock_mode = VDSO_CLOCKMODE_NONE;
vdso_default = VDSO_CLOCKMODE_NONE;
+ } else if (wa->disable_compat_vdso && vdso_default !=
VDSO_CLOCKMODE_NONE) {
+ vdso_default = VDSO_CLOCKMODE_ARCHTIMER_NOCOMPAT;
+ clocksource_counter.archdata.clock_mode = vdso_default;
}
}
--
2.27.0
Re: [PATCH v3] pinctrl: qcom: Handle broken/missing PDC dual edge IRQs on sc7180
On 2020-07-14 16:04, Douglas Anderson wrote:
Depending on how you look at it, you can either say that:
a) There is a PDC hardware issue (with the specific IP rev that exists
on sc7180) that causes the PDC not to work properly when configured
to handle dual edges.
b) The dual edge feature of the PDC hardware was only added in later
HW revisions and thus isn't in all hardware.
Regardless of how you look at it, let's work around the lack of dual
edge support by only ever letting our parent see requests for single
edge interrupts on affected hardware.
NOTE: it's possible that a driver requesting a dual edge interrupt
might get several edges coalesced into a single IRQ. For instance if
a line starts low and then goes high and low again, the driver that
requested the IRQ is not guaranteed to be called twice. However, it
is guaranteed that once the driver's interrupt handler starts running
its first instruction that any new edges coming in will cause the
interrupt to fire again. This is relatively commonplace for dual-edge
gpio interrupts (many gpio controllers require software to emulate
dual edge with single edge) so client drivers should be setup to
handle it.
Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
Signed-off-by: Douglas Anderson
Reviewed-by: Marc Zyngier
Linus, I assume you will get this one via the pinctrl tree once you
are happy with it?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Re: [PATCH v2] pinctrl: qcom: Handle broken/missing PDC dual edge IRQs on sc7180
Hi Doug,
On 2020-07-13 16:26, Douglas Anderson wrote:
Depending on how you look at it, you can either say that:
a) There is a PDC hardware issue (with the specific IP rev that exists
on sc7180) that causes the PDC not to work properly when configured
to handle dual edges.
b) The dual edge feature of the PDC hardware was only added in later
HW revisions and thus isn't in all hardware.
Regardless of how you look at it, let's work around the lack of dual
edge support by only ever letting our parent see requests for single
edge interrupts on affected hardware.
NOTE: it's possible that a driver requesting a dual edge interrupt
might get several edges coalesced into a single IRQ. For instance if
a line starts low and then goes high and low again, the driver that
requested the IRQ is not guaranteed to be called twice. However, it
is guaranteed that once the driver's interrupt handler starts running
its first instruction that any new edges coming in will cause the
interrupt to fire again. This is relatively commonplace for dual-edge
gpio interrupts (many gpio controllers require software to emulate
dual edge with single edge) so client drivers should be setup to
handle it.
Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
Signed-off-by: Douglas Anderson
---
As far as I can tell everything here should work and the limited
testing I'm able to give it shows that, in fact, I can detect both
edges.
I specifically left off Reviewed-by and Tested-by tags from v2 becuase
I felt that the implementation had changed just enough to invalidate
previous reviews / testing. Hopefully it's not too much of a hassle
for folks to re-review and re-test.
Changes in v2:
- Use handle_fasteoi_ack_irq() and switch edges in the Ack now.
- If we change types, switch back to the normal handle_fasteoi_irq().
- No extra locking.
- Properly print an error if we hit 100 loops w/ no stability.
- Beefed up the commit message.
drivers/pinctrl/qcom/Kconfig | 2 +
drivers/pinctrl/qcom/pinctrl-msm.c| 74 ++-
drivers/pinctrl/qcom/pinctrl-msm.h| 4 ++
drivers/pinctrl/qcom/pinctrl-sc7180.c | 1 +
4 files changed, 79 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/qcom/Kconfig
b/drivers/pinctrl/qcom/Kconfig
index ff1ee159dca2..f8ff30cdafa6 100644
--- a/drivers/pinctrl/qcom/Kconfig
+++ b/drivers/pinctrl/qcom/Kconfig
@@ -7,6 +7,8 @@ config PINCTRL_MSM
select PINCONF
select GENERIC_PINCONF
select GPIOLIB_IRQCHIP
+ select IRQ_DOMAIN_HIERARCHY
+ select IRQ_FASTEOI_HIERARCHY_HANDLERS
config PINCTRL_APQ8064
tristate "Qualcomm APQ8064 pin controller driver"
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c
b/drivers/pinctrl/qcom/pinctrl-msm.c
index 83b7d64bc4c1..eae8f421ff63 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -832,6 +832,52 @@ static void msm_gpio_irq_unmask(struct irq_data
*d)
msm_gpio_irq_clear_unmask(d, false);
}
+/**
+ * msm_gpio_update_dual_edge_parent() - Prime next edge for IRQs
handled by parent.
+ * @d: The irq dta.
+ *
+ * This is much like msm_gpio_update_dual_edge_pos() but for IRQs that
are
+ * normally handled by the parent irqchip. The logic here is slightly
+ * different due to what's easy to do with our parent, but in
principle it's
+ * the same.
+ */
+static void msm_gpio_update_dual_edge_parent(struct irq_data *d)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+ struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+ const struct msm_pingroup *g = &pctrl->soc->groups[d->hwirq];
+ int loop_limit = 100;
+ unsigned int val;
+ unsigned int type;
+
+ /* Read the value and make a guess about what edge we need to catch
*/
+ val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
+ type = val ? IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
+
+ do {
+ /* Set the parent to catch the next edge */
+ irq_chip_set_type_parent(d, type);
+
+ /*
+* Possibly the line changed between when we last read "val"
+* (and decided what edge we needed) and when set the edge.
+* If the value didn't change (or changed and then changed
+* back) then we're done.
+*/
+ val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
+ if (type == IRQ_TYPE_EDGE_RISING) {
+ if (!val)
+ return;
+ type = IRQ_TYPE_EDGE_FALLING;
+ } else if (type == IRQ_TYPE_EDGE_FALLING) {
+ if (val)
+ return;
+ type = IRQ_TYPE_EDGE_RISING;
+ }
+ } while (loop_limit-- > 0);
+ dev_err(pctrl->dev, "dual-edge irq failed to stabilize\n");
I'd tone this down to a dev_warn_once(), if at all possible, or
some other rate-lim
Re: [PATCH v2 3/5] irqchip: Allow QCOM_PDC to be loadable as a permanent module
On Sat, 11 Jul 2020 00:27:45 +0100,
Stephen Boyd wrote:
>
> Quoting John Stultz (2020-07-10 15:44:18)
> > On Thu, Jul 9, 2020 at 11:02 PM Stephen Boyd wrote:
> > >
> > > Does it work? I haven't looked in detail but I worry that the child
> > > irqdomain (i.e. pinctrl-msm) would need to delay probing until this
> > > parent irqdomain is registered. Or has the hierarchical irqdomain code
> > > been updated to handle the parent child relationship and wait for things
> > > to probe or be loaded?
> >
> > So I can't say I know the underlying hardware particularly well, but
> > I've been using this successfully on the Dragonboard 845c with both
> > static builds as well as module enabled builds.
> > And the same patch has been in the android-mainline and android-5.4
> > kernels for a while without objections from QCOM.
> >
> > As to the probe ordering question, Saravana can maybe speak in more
> > detail if it's involved in this case but the fw_devlink code has
> > addressed many of these sorts of ordering issues.
> > However, I'm not sure if I'm lucking into the right probe order, as we
> > have been able to boot android-mainline w/ both fw_devlink=on and
> > fw_devlink=off (though in the =off case, we need
> > deferred_probe_timeout=30 to give us a bit more time for modules to
> > load after init starts).
> >
>
> Ok I looked at the code (sorry for not checking earlier) and I see this in
> msm_gpio_init()
>
> np = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0);
> if (np) {
> chip->irq.parent_domain = irq_find_matching_host(np,
> DOMAIN_BUS_WAKEUP);
> of_node_put(np);
> if (!chip->irq.parent_domain)
> return -EPROBE_DEFER;
>
> so it looks like we'll probe defer the pinctrl driver until the pdc module
> loads. Meaning it should work to have pinctrl builtin and pdc as a module.
What I hope is that eventually fw_devlink will become the norm (on by
default), and that probe deferral will become a thing of the past.
M.
--
Without deviation from the norm, progress is not possible.
Re: [PATCH] pinctrl: qcom: Handle broken PDC dual edge case on sc7180
On Fri, 10 Jul 2020 17:10:55 +0100,
Doug Anderson wrote:
>
> Hi,
>
> On Fri, Jul 10, 2020 at 2:03 AM Marc Zyngier wrote:
> >
> > Hi Doug,
[...]
> >
> > > + type = val ? IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
> > > +
> > > + raw_spin_lock_irqsave(&pctrl->lock, flags);
> >
> > What is this lock protecting you against? In both cases, you are
> > already under the irq_desc lock, with interrupts disabled.
>
> We are? I put a breakpoint when the IRQ hits and did a bt. I see
> this (I happen to be on 5.4 at the moment, so hopefully the same as
> mainline):
>
> kgdb_breakpoint+0x3c/0x74
> msm_gpio_update_dual_edge_parent+0x58/0x17c
> msm_gpio_handle_dual_edge_parent_irq+0x1c/0x30
> __handle_domain_irq+0x84/0xc4
> gic_handle_irq+0x170/0x220
> el1_irq+0xd0/0x180
>
> I think the stack is missing a few things due to aggressive inlining
> from my compiler, so the true backtrace would be:
>
> msm_gpio_handle_dual_edge_parent_irq()
> generic_handle_irq_desc()
> generic_handle_irq()
> __handle_domain_irq()
> handle_domain_irq()
> gic_handle_irq()
>
> The first place that got the "desc" was generic_handle_irq() and it
> got it via irq_to_desc(). That doesn't seem to do any locking. Then
> generic_handle_irq_desc() just calls a function pointer so no locking
> there either.
>
> ...ah, but maybe what you're saying is that
> msm_gpio_handle_dual_edge_parent_irq() should be holding "desc->lock"
> around the call to msm_gpio_update_dual_edge_parent()? I can do that.
No, I mentally did a fast-forward to moving this hack into the irq
flow, rather than doing before entering the flow. handle_fasteoi_irq
will take the lock, but obviously not with the current state of this
patch.
>
>
> > > + do {
> > > + /* Set the parent to catch the next edge */
> > > + irq_chip_set_type_parent(d, type);
> > > +
> > > + /*
> > > + * Possibly the line changed between when we last read "val"
> > > + * (and decided what edge we needed) and when set the edge.
> > > + * If the value didn't change (or changed and then changed
> > > + * back) then we're done.
> > > + */
> >
> > If the line changed, shouldn't you actually inject a new interrupt
> > altogether? By changing the polarity more than once, you are
> > effectively loosing edges that should have triggered an interrupt.
>
> Are you sure this is needed? My understanding of edge triggered
> interrupts is that until the interrupt handler is called that all
> edges can be coalesced into a single interrupt.
It really depends on whether the edges are semantically different, and
I'm not sure you can decide this at the interrupt controller
level. The core IRQ code doesn't give you a way to discriminate
between those, but endpoint drivers could, and could get terminally
confused if the see two rising edges without a falling edge in
between.
> It's only after the
> interrupt handler is called that it's important to capture new edges.
> So if you have this:
>
> a) Be busy processing another unrelated interrupt
> b) 5 edges happen on the line
> c) Other interrupt finishes
> d) Edge interrupt is acked and handler is called
>
> You'll only get one call to the interrupt handler even though there
> were 5 edges, right? It's only important that you queue another
> interrupt if that interrupt happens after the true interrupt handler
> (the one acting on the edge) has started.
>
> ...actually, in theory you'll get _either_ one or two calls to the
> interrupt handler depending on timing, since the above could also
> happen as:
>
> a) Be busy processing another unrelated interrupt
> b) 4 edges happen on the line
> c) Other interrupt finishes
> d) Edge interrupt is acked and ...
> e) 1 more edge happens on the line
> f) ...handler is called
> g) Edge interrupt is acked and handler is called
>
>
> As long as msm_gpio_update_dual_edge_parent() is called _before_ the
> true interrupt handler is called then what I have should be fine,
> right?
I don't disagree with any of that, except that being fine at the
irqchip level doesn't necessarily mean being fine at the endpoint
driver level. On the other hand, the HW looks terminally broken, so
maybe it doesn't matter as the drivers will have to be written with
this limitation in mind...
>
> > > + val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
> > > + if (
Re: [RFC PATCH] irqchip/gic: Implement irq_chip->irq_retrigger()
On Fri, 10 Jul 2020 15:56:42 +0100, Valentin Schneider wrote: > > While digging around IRQCHIP_EOI_IF_HANDLED and irq/resend.c, it has come > to my attention that the IRQ resend situation seems a bit precarious for > the GIC(s). Below is a (somewhat structured) dump of my notes/thoughts > about that. > > IRQCHIP_EOI_IF_HANDLED > == > > If the irqchip doesn't have this flag set, we may issue an irq_eoi() > despite not having handled the IRQ in the following cases: > > o !irq_may_run() > - IRQ may be in progress (handle_irq_event() ongoing) > - IRQ is an armed wakeup source (also sets it pending) > > o !action or IRQ disabled; in this case it is set as pending. > > irq_resend() > > > For the above cases where the IRQ is marked as pending, it means that when > we'll go through check_irq_resend() (e.g. when we re-enable the IRQ) we > will end up in resend_irqs() which goes through the flow handler again, IOW > will issue *another* EOI on the same IRQ. > > Now, this is all done via a tasklet, so AFAICT cannot happen in irq > context (as defined by in_interrupt() - it can happen at the tail of > handling an IRQ if it wasn't nesting). > > GIC woes > > > The TL;DR for IRQ handling on the GIC is that we have 3 steps: > o Acknowledgement (Ack) > o priority drop (PD) > o deactivation (D) > > The GIC also has an "eoimode" knob that says whether PD and D are split, IOW: > o eoimode=0: irq_eoi() does PD + D > o eoimode=1: irq_eoi() does D > > Regardless of the mode, it is paramount that any PD is > o issued on the same CPU that Ack'd the IRQ > o issued in reverse order of the Acks > > IHI0069D, 4.1.1 Physical CPU interface, Priority drop > """ > A priority drop must be performed by the same PE that activated the > interrupt. > > [...] > > For each CPU interface, the GIC architecture requires the order of the > valid writes to ICC_EOIR0_EL1 and ICC_EOIR1_EL1 to be the exact reverse of > the order of the reads from ICC_IAR0_EL1 and ICC_IAR1_EL1 > """ > > IHI0069D, 8.2.9 ICC_EOIR0_EL1, Interrupt Controller End Of Interrupt Register > 0 > """ > A write to this register must correspond to the most recent valid read by > this PE from an Interrupt Acknowledge Register, and must correspond to the > INTID that was read from ICC_IAR0_EL1, otherwise the system behavior is > UNPREDICTABLE. > """ > > For eoimode=1, the PD is hidden from genirq and is contained within the GIC > driver. This means that issuing another irq_eoi() will only be re-issuing a > D, which I think the GIC can live with (even if issued from a different CPU). > > For eoimode=0, it is more troubling, as we break the aforementioned > restrictions. That said, IIUC this cannot cause e.g. a bogus running > priority reduction due to the tasklet context mentioned above (running > priority ought to be idle priority). > > Linux hosts will almost always use eoimode=1, so that leaves us with > guests running with eoimode=0, and I don't know how common it is (if at all > possible) for those to use pm / wakeup IRQs. I suppose that is a reason > why this hasn't cropped up before, that or I miserably misunderstood the > whole thing. > > In any case, the virtual interface follows the same restrictions wrt > PD ordering: > > IHI0069D 8.3.7 ICV_EOIR0_EL1, Interrupt Controller Virtual End Of Interrupt > Register 0 > """ > A write to this register must correspond to the most recent valid read by > this vPE from a Virtual Interrupt Acknowledge Register, and must correspond > to the INTID that was read from ICV_IAR0_EL1, otherwise the system behavior > is UNPREDICTABLE. > """ > > Change > == > > One way to ensure we only get one PD per interrupt activation and maintain > the expected ordering is to add the IRQCHIP_EOI_IF_HANDLED flag to all > irq-gic chips, but that only really works for eoimode=1; for eoimode=0 that > would mean leaving the flow handler without having issued a PD at all. > > At the same time, this whole IRQS_PENDING & resend thing feels like > something we can handle in hardware: we can leverage > irq_chip.irq_retrigger() and use this to mark the interrupt as pending in > the GIC itself, in which case we *don't* want to have > IRQCHIP_EOI_IF_HANDLED (as the retrigger will lead to another ack+eoi > pair). > > Implement irq_chip.irq_retrigger() for both GICs. Although I am very grateful for the whole documentation, I'd rather have a slightly more condensed changelog that documents the implementation of the retrigger callback! ;-) > > Signed-off-by: Valentin Schneider > --- > drivers/irqchip/irq-gic-v3.c | 7 +++ > drivers/irqchip/irq-gic.c| 6 ++ > 2 files changed, 13 insertions(+) > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > index cc46bc2d634b..c025e8b51464 100644 > --- a/drivers/irqchip/irq-gic-v3.c > +++ b/drivers/irqchip/irq-gic-v3.c > @@ -1207,6 +1207,11 @@ static int gic_set_affinity(struct irq_data *d, const > struct cpumask *mask_va
Re: [PATCH] irqchip/stm32-exti: map direct event to irq parent
Alexandre,
On Wed, 08 Jul 2020 05:57:24 +0100,
kernel test robot wrote:
>
> [1 ]
> Hi Alexandre,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on stm32/stm32-next]
> [also build test WARNING on soc/for-next v5.8-rc4 next-20200707]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url:
> https://github.com/0day-ci/linux/commits/Alexandre-Torgue/irqchip-stm32-exti-map-direct-event-to-irq-parent/20200706-161327
> base: https://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git
> stm32-next
> config: arm-randconfig-s031-20200707 (attached as .config)
> compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
> reproduce:
> wget
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # apt-get install sparse
> # sparse version: v0.6.2-31-gabbfd661-dirty
> # save the attached .config to linux build tree
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1
> CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=arm
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot
>
> All warnings (new ones prefixed by >>):
>
>In file included from include/linux/build_bug.h:5,
> from include/linux/bits.h:23,
> from include/linux/bitops.h:5,
> from drivers/irqchip/irq-stm32-exti.c:8:
>drivers/irqchip/irq-stm32-exti.c: In function 'stm32_exti_h_domain_alloc':
>drivers/irqchip/irq-stm32-exti.c:683:23: warning: comparison of unsigned
> expression >= 0 is always true [-Wtype-limits]
> 683 | if (desc->irq_parent >= 0) {
> | ^~
>include/linux/compiler.h:58:52: note: in definition of macro
> '__trace_if_var'
> 58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond)
> : __trace_if_value(cond))
> |^~~~
> >> drivers/irqchip/irq-stm32-exti.c:683:2: note: in expansion of macro 'if'
> 683 | if (desc->irq_parent >= 0) {
Do you plan to address this? Looks like an actual bug to me.
M.
--
Without deviation from the norm, progress is not possible.
Re: [PATCH] pinctrl: qcom: Handle broken PDC dual edge case on sc7180
Hi Doug,
On Wed, 08 Jul 2020 22:16:25 +0100,
Douglas Anderson wrote:
>
> As per Qualcomm, there is a PDC hardware issue (with the specific IP
> rev that exists on sc7180) that causes the PDC not to work properly
> when configured to handle dual edges.
>
> Let's work around this by emulating only ever letting our parent see
> requests for single edge interrupts on affected hardware.
>
> Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
> Signed-off-by: Douglas Anderson
> ---
> As far as I can tell everything here should work and the limited
> testing I'm able to give it shows that, in fact, I can detect both
> edges.
>
> Please give this an extra thorough review since it's trying to find
> the exact right place to insert this code and I'm not massively
> familiar with all the frameworks.
>
> If someone has hardware where it's easy to stress test this that'd be
> wonderful too. The board I happen to have in front of me doesn't have
> any easy-to-toggle GPIOs where I can just poke a button or a switch to
> generate edges. My testing was done by hacking the "write protect"
> GPIO on my board into gpio-keys as a dual-edge interrupt and then
> sending commands to our security chip to toggle it--not exactly great
> for testing to make sure there are no race conditions if the interrupt
> bounces a lot.
This looks positively awful (the erratum, not the patch). Is there an
actual description of the problem, outlining the circumstances that
triggers this issue? The PDC really never fails to disappoint...
>
> drivers/pinctrl/qcom/pinctrl-msm.c| 80 +++
> drivers/pinctrl/qcom/pinctrl-msm.h| 4 ++
> drivers/pinctrl/qcom/pinctrl-sc7180.c | 1 +
> 3 files changed, 85 insertions(+)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c
> b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 83b7d64bc4c1..45ca09ebb7b3 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -860,6 +860,79 @@ static void msm_gpio_irq_ack(struct irq_data *d)
> raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> }
>
> +/**
> + * msm_gpio_update_dual_edge_parent() - Prime next edge for IRQs handled by
> parent.
> + * @d: The irq dta.
> + *
> + * This is much like msm_gpio_update_dual_edge_pos() but for IRQs that are
> + * normally handled by the parent irqchip. The logic here is slightly
> + * different due to what's easy to do with our parent, but in principle it's
> + * the same.
> + */
> +static void msm_gpio_update_dual_edge_parent(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> + const struct msm_pingroup *g = &pctrl->soc->groups[d->hwirq];
> + unsigned long flags;
> + int loop_limit = 100;
I guess this is a "finger up in the air" type of limit?
> + unsigned int val;
> + unsigned int type;
> +
> + /* Read the value and make a guess about what edge we need to catch */
> + val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
What does this value represent? The input line value?
> + type = val ? IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
> +
> + raw_spin_lock_irqsave(&pctrl->lock, flags);
What is this lock protecting you against? In both cases, you are
already under the irq_desc lock, with interrupts disabled.
> + do {
> + /* Set the parent to catch the next edge */
> + irq_chip_set_type_parent(d, type);
> +
> + /*
> + * Possibly the line changed between when we last read "val"
> + * (and decided what edge we needed) and when set the edge.
> + * If the value didn't change (or changed and then changed
> + * back) then we're done.
> + */
If the line changed, shouldn't you actually inject a new interrupt
altogether? By changing the polarity more than once, you are
effectively loosing edges that should have triggered an interrupt.
> + val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
> + if (type == IRQ_TYPE_EDGE_RISING) {
> + if (!val)
> + break;
> + type = IRQ_TYPE_EDGE_FALLING;
> + } else if (type == IRQ_TYPE_EDGE_FALLING) {
> + if (val)
> + break;
> + type = IRQ_TYPE_EDGE_RISING;
> + }
> + } while (loop_limit-- > 0);
> + raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> + if (!loop_limit)
> + dev_err(pctrl->dev, "dual-edge irq failed to stabilize\n");
> +}
> +
> +void msm_gpio_handle_dual_edge_parent_irq(struct irq_desc *desc)
> +{
> + struct irq_data *d = &desc->irq_data;
> +
> + /* Make sure we're primed for the next edge */
> + msm_gpio_update_dual_edge_parent(d);
I would have expected this to happen on EOI or ACK, rather than before
the flow is actually handled, once you have
Re: [PATCH] irqdomain/treewide: Keep firmware node unconditionally allocated
Hi Thomas,
Catching up on email...
On 2020-07-09 10:53, Thomas Gleixner wrote:
Quite some non OF/ACPI users of irqdomains allocate firmware nodes of
type
IRQCHIP_FWNODE_NAMED or IRQCHIP_FWNODE_NAMED_ID and free them right
after
creating the irqdomain. The only purpose of these FW nodes is to convey
name information. When this was introduced the core code did not store
the
pointer to the node in the irqdomain. A recent change stored the
firmware
node pointer in irqdomain for other reasons and missed to notice that
the
usage sites which do the alloc_fwnode/create_domain/free_fwnode
sequence
are broken by this. Storing a dangling pointer is dangerous itself, but
in
case that the domain is destroyed later on this leads to a double free.
Remove the freeing of the firmware node after creating the irqdomain
from
all affected call sites to cure this.
Fixes: 711419e504eb ("irqdomain: Add the missing assignment of
domain->fwnode for named fwnode")
Reported-by: Andy Shevchenko
Signed-off-by: Thomas Gleixner
Cc: sta...@vger.kernel.org
Urgh, that's pretty disastrous. My bad. Thanks a lot for having
put this patch together.
Acked-by: Marc Zyngier
If you can take it directly into Linus' tree, that'd be greatly
appreciated.
Thanks again,
M.
--
Jazz is not dead. It just smells funny...
Re: [PATCH 6/8] arm64: dts: renesas: Initial r8a774e1 SoC device tree
On 2020-07-08 18:48, Lad Prabhakar wrote:
From: Marian-Cristian Rotariu
Basic support for the RZ/G2H SoC.
Signed-off-by: Marian-Cristian Rotariu
Signed-off-by: Lad Prabhakar
---
arch/arm64/boot/dts/renesas/r8a774e1.dtsi | 652 ++
1 file changed, 652 insertions(+)
create mode 100644 arch/arm64/boot/dts/renesas/r8a774e1.dtsi
diff --git a/arch/arm64/boot/dts/renesas/r8a774e1.dtsi
b/arch/arm64/boot/dts/renesas/r8a774e1.dtsi
new file mode 100644
index ..6637e157ffcd
--- /dev/null
+++ b/arch/arm64/boot/dts/renesas/r8a774e1.dtsi
[...]
+ gic: interrupt-controller@f101 {
+ compatible = "arm,gic-400";
+ #interrupt-cells = <3>;
+ #address-cells = <0>;
+ interrupt-controller;
+ reg = <0x0 0xf101 0 0x1000>,
+ <0x0 0xf102 0 0x2>,
+ <0x0 0xf104 0 0x2>,
+ <0x0 0xf106 0 0x2>;
+ interrupts = ;
You seem to have a bit more than only 2 CPUs in this system.
+ clocks = <&cpg CPG_MOD 408>;
+ clock-names = "clk";
+ power-domains = <&sysc R8A774E1_PD_ALWAYS_ON>;
+ resets = <&cpg 408>;
+ };
M.
--
Jazz is not dead. It just smells funny...
Re: [PATCHv3 2/6] irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts
On 2020-07-08 08:04, Grzegorz Jaszczyk wrote: On Sun, 5 Jul 2020 at 22:45, Marc Zyngier wrote: On 2020-07-05 14:26, Grzegorz Jaszczyk wrote: > On Sat, 4 Jul 2020 at 11:39, Marc Zyngier wrote: >> >> On 2020-07-03 15:28, Grzegorz Jaszczyk wrote: [...] >> It still begs the question: if the HW can support both edge and level >> triggered interrupts, why isn't the driver supporting this diversity? >> I appreciate that your HW may only have level interrupts so far, but >> what guarantees that this will forever be true? It would imply a >> change >> in the DT binding, which isn't desirable. > > Ok, I've got your point. I will try to come up with something later > on. Probably extending interrupt-cells by one and passing interrupt > type will be enough for now. Extending this driver to actually support > it can be handled later if needed. Hope it works for you. Writing a set_type callback to deal with this should be pretty easy. Don't delay doing the right thing. Ok. [...] >> >> > + hwirq = hipir & GENMASK(9, 0); >> >> > + virq = irq_linear_revmap(intc->domain, hwirq); >> >> >> >> And this is where I worry. You seems to have a single irqdomain >> >> for all the muxes. Are you guaranteed that you will have no >> >> overlap between muxes? And please use irq_find_mapping(), as >> >> I have top-secret plans to kill irq_linear_revmap(). >> > >> > Regarding irq_find_mapping - sure. >> > >> > Regarding irqdomains: >> > It is a single irqdomain since the hwirq (system event) can be mapped >> > to different irq_host (muxes). Patch #6 >> > https://lkml.org/lkml/2020/7/2/616 implements and describes how input >> > events can be mapped to some output host interrupts through 2 levels >> > of many-to-one mapping i.e. events to channel mapping and channels to >> > host interrupts. Mentioned implementation ensures that specific system >> > event (hwirq) can be mapped through PRUSS specific channel into a >> > single host interrupt. >> >> Patch #6 is a nightmare of its own, and I haven't fully groked it yet. >> Also, this driver seems to totally ignore the 2-level routing. Where >> is it set up? map/unmap in this driver do exactly *nothing*, so >> something somewhere must set it up. > > The map/unmap is updated in patch #6 and it deals with those 2-level > routing setup. Map is responsible for programming the Channel Map > Registers (CMRx) and Host-Interrupt Map Registers (HMRx) basing on > provided configuration from the one parsed in the xlate function. > Unmap undo whatever was done on the map. More details can be found in > patch #6. > > Maybe it would be better to squash patch #6 with this one so it would > be less confusing. What is your advice? So am I right in understanding that without patch #6, this driver does exactly nothing? If so, it has been a waste of review time. Please split patch #6 so that this driver does something useful for Linux, without any of the PRU interrupt routing stuff. I want to see a Linux-only driver that works and doesn't rely on any other exotic feature. Patch #6 provides PRU specific 2-level routing setup. This step is required and it is part of the entire patch-set. Theoretically routing setup could be done by other platform driver (not irq one) or e.g. by PRU firmware. In such case this driver would be functional without patch #6 but I do not think it would be proper. Then this whole driver is non-functional until the last patch that comes with the PRU-specific "value-add". [...] I am open to any suggestion if there is a better way of handling 2-level routing. I will also appreciate if you could elaborate about issues that you see with patch #6. The two level routing has to be part of this (or another) irqchip driver (specially given that it appears to me like another set of crossbar). There should only be a *single* binding for all interrupts, including those targeting the PRU (you seem to have two). And the non-CPU interrupt code has to be in its own patch, because it is pretty borderline anyway (I'm still not completely convinced this is Linux's job). N, -- Jazz is not dead. It just smells funny...
Re: [PATCH v5 05/13] irqchip: add sl28cpld interrupt controller support
On 2020-07-06 18:53, Michael Walle wrote: Add support for the interrupt controller inside the sl28 CPLD management controller. The interrupt controller can handle at most 8 interrupts and is really simplistic and consists only of an interrupt mask and an interrupt pending register. Signed-off-by: Michael Walle --- Changes since v4: - update copyright year - don't use "int irq" instead of "unsigne int irq", because platform_get_irq() might return a negative error code. Found by "kernel test robot - remove comma in terminator line of the compatible strings list, suggested by Andy - use newer devm_regmap_add_irq_chip_fwnode() - don't use KBUID_MODNAME, suggested by Andy - remove the platform device table Changes since v3: - see cover letter drivers/irqchip/Kconfig| 8 +++ drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-sl28cpld.c | 96 ++ 3 files changed, 105 insertions(+) create mode 100644 drivers/irqchip/irq-sl28cpld.c Acked-by: Marc Zyngier Given the dependency on the MFD patches, I assume this will be routed to that subsystem. Please let me know if you want it to be handled differently. Thanks, M. -- Jazz is not dead. It just smells funny...
Re: [RFC PATCH v4 2/2] arm64: tlb: Use the TLBI RANGE feature in arm64
On 2020-07-07 18:36, Catalin Marinas wrote: On Mon, Jun 01, 2020 at 10:47:13PM +0800, Zhenyu Ye wrote: @@ -59,6 +69,47 @@ __ta; \ }) +/* + * __TG defines translation granule of the system, which is decided by + * PAGE_SHIFT. Used by TTL. + * - 4KB : 1 + * - 16KB : 2 + * - 64KB : 3 + */ +#define __TG ((PAGE_SHIFT - 12) / 2 + 1) Nitpick: maybe something like __TLBI_TG to avoid clashes in case someone else defines a __TG macro. I have commented on this in the past, and still maintain that this would be better served by a switch statement similar to what is used for TTL already (I don't think this magic formula exists in the ARM ARM). M. -- Jazz is not dead. It just smells funny...
[PATCH v2 0/4] arm64: Allow the compat vdso to be disabled at runtime
The relatively recent introduction of the compat vdso on arm64 has overlooked its interactions with some of the interesting errata workarounds, such as ARM64_ERRATUM_1418040 (and its older 1188873 incarnation). This erratum requires the 64bit kernel to trap 32bit accesses to the virtual counter and emulate it. When the workaround was introduced, the compat vdso simply wasn't a thing. Now that the patches have landed in mainline, we trap the CVTVCT accesses from the vdso. This can end-up in a nasty loop in the vdso, where the sequence number changes on each trap, never stabilising, and leaving userspace in a bit of a funny state (which is why we disable the vdso in most similar cases). This erratum mentionned above is a bit special in the sense that in only requires to trap AArch32 accesses, and 64bit tasks can be left alone. Consequently, the vdso is never disabled and AArch32 tasks are affected. Obviously, we really want to retain the 64bit vdso in this case. To that effect, this series offers a way to disable the 32bit view of the vdso without impacting its 64bit counterpart, by providing a "no-compat" vdso clock_mode, and plugging this feature into the 1418040 detection code. Lastly, I've tagged a rework of the 1414080 workaround (which had been posted separately) at the end of the series so that it limits its effect to 32bit tasks exclusively (so far, it forces the userspace access bit on 64bit tasks, and we may need to leave it disabled in the future...). * From v1: - Reworked following Mark's feedback (patches #2 and #3) - Reworked patch #4 after Will's comments - patches #1 to #3 are now cc stable - Applied Mark's AB to patch #1 Marc Zyngier (4): arm64: Introduce a way to disable the 32bit vdso arm64: arch_timer: Allow an workaround descriptor to disable compat vdso arm64: arch_timer: Disable the compat vdso for cores affected by ARM64_WORKAROUND_1418040 arm64: Rework ARM_ERRATUM_1414080 handling arch/arm64/include/asm/arch_timer.h | 1 + arch/arm64/include/asm/vdso/clocksource.h | 7 +++- .../include/asm/vdso/compat_gettimeofday.h| 8 +++- arch/arm64/kernel/entry.S | 40 +++ drivers/clocksource/arm_arch_timer.c | 11 + 5 files changed, 48 insertions(+), 19 deletions(-) -- 2.27.0
[PATCH v2 1/4] arm64: Introduce a way to disable the 32bit vdso
We have a class of errata (grouped under the ARM64_WORKAROUND_1418040
banner) that force the trapping of counter access from 32bit EL0.
We would normally disable the whole vdso for such defect, except that
it would disable it for 64bit userspace as well, which is a shame.
Instead, add a new vdso_clock_mode, which signals that the vdso
isn't usable for compat tasks. This gets checked in the new
vdso_clocksource_ok() helper, now provided for the 32bit vdso.
Cc: sta...@vger.kernel.org
Acked-by: Mark Rutland
Signed-off-by: Marc Zyngier
---
arch/arm64/include/asm/vdso/clocksource.h | 7 +--
arch/arm64/include/asm/vdso/compat_gettimeofday.h | 8 +++-
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/include/asm/vdso/clocksource.h
b/arch/arm64/include/asm/vdso/clocksource.h
index df6ea65c1dec..b054d9febfb5 100644
--- a/arch/arm64/include/asm/vdso/clocksource.h
+++ b/arch/arm64/include/asm/vdso/clocksource.h
@@ -2,7 +2,10 @@
#ifndef __ASM_VDSOCLOCKSOURCE_H
#define __ASM_VDSOCLOCKSOURCE_H
-#define VDSO_ARCH_CLOCKMODES \
- VDSO_CLOCKMODE_ARCHTIMER
+#define VDSO_ARCH_CLOCKMODES \
+ /* vdso clocksource for both 32 and 64bit tasks */ \
+ VDSO_CLOCKMODE_ARCHTIMER, \
+ /* vdso clocksource for 64bit tasks only */ \
+ VDSO_CLOCKMODE_ARCHTIMER_NOCOMPAT
#endif
diff --git a/arch/arm64/include/asm/vdso/compat_gettimeofday.h
b/arch/arm64/include/asm/vdso/compat_gettimeofday.h
index b6907ae78e53..9a625e8947ff 100644
--- a/arch/arm64/include/asm/vdso/compat_gettimeofday.h
+++ b/arch/arm64/include/asm/vdso/compat_gettimeofday.h
@@ -111,7 +111,7 @@ static __always_inline u64 __arch_get_hw_counter(s32
clock_mode)
* update. Return something. Core will do another round and then
* see the mode change and fallback to the syscall.
*/
- if (clock_mode == VDSO_CLOCKMODE_NONE)
+ if (clock_mode != VDSO_CLOCKMODE_ARCHTIMER)
return 0;
/*
@@ -152,6 +152,12 @@ static __always_inline const struct vdso_data
*__arch_get_vdso_data(void)
return ret;
}
+static inline bool vdso_clocksource_ok(const struct vdso_data *vd)
+{
+ return vd->clock_mode == VDSO_CLOCKMODE_ARCHTIMER;
+}
+#define vdso_clocksource_okvdso_clocksource_ok
+
#endif /* !__ASSEMBLY__ */
#endif /* __ASM_VDSO_GETTIMEOFDAY_H */
--
2.27.0
[PATCH v2 4/4] arm64: Rework ARM_ERRATUM_1414080 handling
The current handling of erratum 1414080 has the side effect that cntkctl_el1 can get changed for both 32 and 64bit tasks. This isn't a problem so far, but if we ever need to mitigate another of these errata on the 64bit side, we'd better keep the messing with cntkctl_el1 local to 32bit tasks. For that, make sure that on entering the kernel from a 32bit tasks, userspace access to cntvct gets enabled, and disabled returning to userspace, while it never gets changed for 64bit tasks. Signed-off-by: Marc Zyngier --- arch/arm64/kernel/entry.S | 40 +++ 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index 5304d193c79d..8f51f3273bc7 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -167,6 +167,19 @@ alternative_cb_end stp x28, x29, [sp, #16 * 14] .if \el == 0 + .if \regsize == 32 + // If we come back from a 32bit task on a system affected by + // 1418040, let's reenable userspace access to the virtual counter. +#ifdef CONFIG_ARM64_ERRATUM_1418040 +alternative_if_not ARM64_WORKAROUND_1418040 + b .L__entry_skip_wa_1418040\@ +alternative_else_nop_endif + mrs x0, cntkctl_el1 + orr x0, x0, #2 // ARCH_TIMER_USR_VCT_ACCESS_EN + msr cntkctl_el1, x0 +.L__entry_skip_wa_1418040\@: +#endif + .endif clear_gp_regs mrs x21, sp_el0 ldr_this_cputsk, __entry_task, x20 @@ -318,7 +331,17 @@ alternative_else_nop_endif ldr x23, [sp, #S_SP]// load return stack pointer msr sp_el0, x23 tst x22, #PSR_MODE32_BIT// native task? - b.eq3f + b.eq4f + +#ifdef CONFIG_ARM64_ERRATUM_1418040 +alternative_if_not ARM64_WORKAROUND_1418040 + b 3f +alternative_else_nop_endif + mrs x1, cntkctl_el1 + bic x1, x1, #2 // ARCH_TIMER_USR_VCT_ACCESS_EN + msr cntkctl_el1, x1 +3: +#endif #ifdef CONFIG_ARM64_ERRATUM_845719 alternative_if ARM64_WORKAROUND_845719 @@ -330,22 +353,7 @@ alternative_if ARM64_WORKAROUND_845719 #endif alternative_else_nop_endif #endif -3: -#ifdef CONFIG_ARM64_ERRATUM_1418040 -alternative_if_not ARM64_WORKAROUND_1418040 - b 4f -alternative_else_nop_endif - /* -* if (x22.mode32 == cntkctl_el1.el0vcten) -* cntkctl_el1.el0vcten = ~cntkctl_el1.el0vcten -*/ - mrs x1, cntkctl_el1 - eon x0, x1, x22, lsr #3 - tbz x0, #1, 4f - eor x1, x1, #2 // ARCH_TIMER_USR_VCT_ACCESS_EN - msr cntkctl_el1, x1 4: -#endif scs_save tsk, x0 /* No kernel C function calls after this as user keys are set. */ -- 2.27.0
[PATCH v2 3/4] arm64: arch_timer: Disable the compat vdso for cores affected by ARM64_WORKAROUND_1418040
ARM64_WORKAROUND_1418040 requires that AArch32 EL0 accesses to
the virtual counter register are trapped and emulated by the kernel.
This makes the vdso pretty pointless, and in some cases livelock
prone.
Provide a workaround entry that limits the vdso to 64bit tasks.
Cc: sta...@vger.kernel.org
Signed-off-by: Marc Zyngier
---
drivers/clocksource/arm_arch_timer.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/clocksource/arm_arch_timer.c
b/drivers/clocksource/arm_arch_timer.c
index a8e4fb429f52..6c3e84180146 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -480,6 +480,14 @@ static const struct arch_timer_erratum_workaround
ool_workarounds[] = {
.set_next_event_virt = erratum_set_next_event_tval_virt,
},
#endif
+#ifdef CONFIG_ARM64_ERRATUM_1418040
+ {
+ .match_type = ate_match_local_cap_id,
+ .id = (void *)ARM64_WORKAROUND_1418040,
+ .desc = "ARM erratum 1418040",
+ .disable_compat_vdso = true,
+ },
+#endif
};
typedef bool (*ate_match_fn_t)(const struct arch_timer_erratum_workaround *,
--
2.27.0
[PATCH v2 2/4] arm64: arch_timer: Allow an workaround descriptor to disable compat vdso
As we are about to disable the vdso for compat tasks in some circumstances,
let's allow a workaround descriptor to express exactly that.
Cc: sta...@vger.kernel.org
Signed-off-by: Marc Zyngier
---
arch/arm64/include/asm/arch_timer.h | 1 +
drivers/clocksource/arm_arch_timer.c | 3 +++
2 files changed, 4 insertions(+)
diff --git a/arch/arm64/include/asm/arch_timer.h
b/arch/arm64/include/asm/arch_timer.h
index 7ae54d7d333a..9f0ec21d6327 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -58,6 +58,7 @@ struct arch_timer_erratum_workaround {
u64 (*read_cntvct_el0)(void);
int (*set_next_event_phys)(unsigned long, struct clock_event_device *);
int (*set_next_event_virt)(unsigned long, struct clock_event_device *);
+ bool disable_compat_vdso;
};
DECLARE_PER_CPU(const struct arch_timer_erratum_workaround *,
diff --git a/drivers/clocksource/arm_arch_timer.c
b/drivers/clocksource/arm_arch_timer.c
index ecf7b7db2d05..a8e4fb429f52 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -566,6 +566,9 @@ void arch_timer_enable_workaround(const struct
arch_timer_erratum_workaround *wa
if (wa->read_cntvct_el0) {
clocksource_counter.vdso_clock_mode = VDSO_CLOCKMODE_NONE;
vdso_default = VDSO_CLOCKMODE_NONE;
+ } else if (wa->disable_compat_vdso && vdso_default !=
VDSO_CLOCKMODE_NONE) {
+ vdso_default = VDSO_CLOCKMODE_ARCHTIMER_NOCOMPAT;
+ clocksource_counter.vdso_clock_mode = vdso_default;
}
}
--
2.27.0
Re: [PATCH v5 00/14] irqchip: Fix potential resource leaks
On 2020-07-06 02:19, Tiezhu Yang wrote: When I test the irqchip code of Loongson, I read the related code of other chips in drivers/irqchip and I find some potential resource leaks in the error path, I think it is better to fix them. v2: - Split the first patch into a new patch series which includes small patches and add "Fixes" tag - Use "goto" label to handle error path in some patches v3: - Add missed variable "ret" in the patch #5 and #13 v4: - Modify the commit message of each patch suggested by Markus Elfring - Make "irq_domain_remove(root_domain)" under CONFIG_SMP in patch #3 - Add a return statement before goto label in patch #4 v5: - Modify the commit messages and do some code cleanups Please stop replying to Markus Elfring, and give people who actually care a chance to review this code. Elfring will keep asking you to make absolutely pointless changes until you are blue in the face Thanks, M. -- Jazz is not dead. It just smells funny...
Re: [PATCHv3 2/6] irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts
On 2020-07-05 14:26, Grzegorz Jaszczyk wrote: On Sat, 4 Jul 2020 at 11:39, Marc Zyngier wrote: On 2020-07-03 15:28, Grzegorz Jaszczyk wrote: [...] It still begs the question: if the HW can support both edge and level triggered interrupts, why isn't the driver supporting this diversity? I appreciate that your HW may only have level interrupts so far, but what guarantees that this will forever be true? It would imply a change in the DT binding, which isn't desirable. Ok, I've got your point. I will try to come up with something later on. Probably extending interrupt-cells by one and passing interrupt type will be enough for now. Extending this driver to actually support it can be handled later if needed. Hope it works for you. Writing a set_type callback to deal with this should be pretty easy. Don't delay doing the right thing. [...] >> > + hwirq = hipir & GENMASK(9, 0); >> > + virq = irq_linear_revmap(intc->domain, hwirq); >> >> And this is where I worry. You seems to have a single irqdomain >> for all the muxes. Are you guaranteed that you will have no >> overlap between muxes? And please use irq_find_mapping(), as >> I have top-secret plans to kill irq_linear_revmap(). > > Regarding irq_find_mapping - sure. > > Regarding irqdomains: > It is a single irqdomain since the hwirq (system event) can be mapped > to different irq_host (muxes). Patch #6 > https://lkml.org/lkml/2020/7/2/616 implements and describes how input > events can be mapped to some output host interrupts through 2 levels > of many-to-one mapping i.e. events to channel mapping and channels to > host interrupts. Mentioned implementation ensures that specific system > event (hwirq) can be mapped through PRUSS specific channel into a > single host interrupt. Patch #6 is a nightmare of its own, and I haven't fully groked it yet. Also, this driver seems to totally ignore the 2-level routing. Where is it set up? map/unmap in this driver do exactly *nothing*, so something somewhere must set it up. The map/unmap is updated in patch #6 and it deals with those 2-level routing setup. Map is responsible for programming the Channel Map Registers (CMRx) and Host-Interrupt Map Registers (HMRx) basing on provided configuration from the one parsed in the xlate function. Unmap undo whatever was done on the map. More details can be found in patch #6. Maybe it would be better to squash patch #6 with this one so it would be less confusing. What is your advice? So am I right in understanding that without patch #6, this driver does exactly nothing? If so, it has been a waste of review time. Please split patch #6 so that this driver does something useful for Linux, without any of the PRU interrupt routing stuff. I want to see a Linux-only driver that works and doesn't rely on any other exotic feature. M. -- Jazz is not dead. It just smells funny...
Re: [PATCH] kvm/arm64: Correct incorrect function parameter specification
On Wed, 1 Jul 2020 08:07:09 -0400, Peng Hao wrote: > update_vmid() just has one parameter "vmid".The other parameter > "kvm" is no longer used. Applied to kvm-arm64/next-5.9, thanks! [1/1] KVM: arm64: Drop long gone function parameter documentation commit: 95fa0ba83e66dea0d3af48ad69842ae8c1dd9af2 Cheers, M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH] KVM: arm64: Change default caching mode for {PEND, PROP}BASER.outer
On Wed, 1 Jul 2020 16:02:06 +0200, Alexander Graf wrote:
> PENDBASER and PROPBASER define the outer caching mode for LPI tables.
> The memory backing them may not be outer sharable, so we mark them as nC
> by default. This however, breaks Windows on ARM which only accepts
> SameAsInner or RaWaWb as values for outer cachability.
>
> We do today already allow the outer mode to be set to SameAsInner
> explicitly, so the easy fix is to default to that instead of nC for
> situations when an OS asks for a not fulfillable cachability request.
>
> [...]
Applied to kvm-arm64/next-5.9, thanks!
[1/1] KVM: arm64: vgic-its: Change default outer cacheability for {PEND,
PROP}BASER
commit: 731532176716e2775a5d21115bb9c5c61e0cb704
Cheers,
M.
--
Without deviation from the norm, progress is not possible.
Re: [PATCH 0/5] KVM: arm64: Remove the target table
On Mon, 22 Jun 2020 11:33:12 +, James Morse wrote: > KVM's target_table indirection is a relic from 32bit where different > CPUs had different reset values for ACTLR. All 64bit CPUs have the > same behaviour here, but we support different targets, that all map > to the same behaviour. > > This series removes the indirection and the fiddly handling of two > tables. > > [...] Applied to kvm-arm64/next-5.9, thanks! [1/5] KVM: arm64: Drop the target_table[] indirection commit: 6b33e0d64f8501b51d32069e08d3ed68c58c25b4 [2/5] KVM: arm64: Tolerate an empty target_table list commit: 04343ae312ef0d80d601ea1b784b6b039ae9c82a [3/5] KVM: arm64: Move ACTLR_EL1 emulation to the sys_reg_descs array commit: af4738290d9dfe3787f60e40f709a4f78a115943 [4/5] KVM: arm64: Remove target_table from exit handlers commit: dcaffa7bf911578a6d69165d712501996c587fbe [5/5] KVM: arm64: Remove the target table commit: 750ed56693803e992ed09ac9c46e07226dd4d350 Cheers, M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH v4 00/15] Split off nVHE hyp code
On Thu, 25 Jun 2020 14:14:05 +0100, David Brazdil wrote: > Refactor files in arch/arm64/kvm/hyp to compile all code which runs in EL2 > under nVHE into separate object files from the rest of KVM. This is done in > preparation for being able to unmap hyp code from EL1 and kernel code/data > from EL2 but has other benefits too, notably: > * safe use of KASAN/UBSAN/GCOV instrumentation on VHE code, > * no need for __hyp_text annotations. > > [...] Applied to kvm-arm64/next-5.9, thanks! [01/15] KVM: arm64: Fix symbol dependency in __hyp_call_panic_nvhe commit: b38b298aa4397e2dc74a89b4dd3eac9e59b64c96 [02/15] KVM: arm64: Move __smccc_workaround_1_smc to .rodata commit: 7b2399ea5640b2e5f576af08b91091a26f240ea4 [03/15] KVM: arm64: Add build rules for separate VHE/nVHE object files commit: 7621712918ad4f5e6356193d9058debf657a6254 [04/15] KVM: arm64: Use build-time defines in has_vhe() commit: 53b671128bd7f9ea41ae1a06106d88eb4cf66623 [05/15] KVM: arm64: Handle calls to prefixed hyp functions commit: f50b6f6ae131b6ee7d5dd738961eda0c00b7f027 [06/15] KVM: arm64: Build hyp-entry.S separately for VHE/nVHE commit: b877e9849d41e7d2100d2933e0a3971d0ddec011 [07/15] KVM: arm64: Move hyp-init.S to nVHE commit: 208243c752a7eeef4236f7b7d67e806ee356e3f8 [08/15] KVM: arm64: Duplicate hyp/tlb.c for VHE/nVHE commit: e03fa29164ec1db1a81dc0168d0017a9e0366c7c [09/15] KVM: arm64: Split hyp/switch.c to VHE/nVHE commit: 09cf57eba304246141367b95c89801fd2047ac96 [10/15] KVM: arm64: Split hyp/debug-sr.c to VHE/nVHE commit: d400c5b2025c9aeca76213d6bd4138ec39da5cef [11/15] KVM: arm64: Split hyp/sysreg-sr.c to VHE/nVHE commit: 13aeb9b400c5d7c5e979fdbbf994c787487f7889 [12/15] KVM: arm64: Duplicate hyp/timer-sr.c for VHE/nVHE commit: 9aebdea494b5d2d5fe0ba54d71e9d6c5acfe76b4 [13/15] KVM: arm64: Compile remaining hyp/ files for both VHE/nVHE commit: c04dd455eb311d2d289c9d81d080eaf11a06cebf [14/15] KVM: arm64: Remove __hyp_text macro, use build rules instead commit: c50cb04303cb88c517715b078e3e010c024af1a5 [15/15] KVM: arm64: Lift instrumentation restrictions on VHE commit: f9a026e3d38ba81cd274725f1caaf64322a86aa5 Cheers, M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH] tty: serial: meson_uart: Init port lock early
On 2020-07-05 12:22, Andy Shevchenko wrote: On Sun, Jul 05, 2020 at 11:28:56AM +0100, Marc Zyngier wrote: On 2020-07-05 11:07, Andy Shevchenko wrote: > On Sun, Jul 5, 2020 at 12:32 PM Marc Zyngier wrote: > > > > The meson UART driver triggers a lockdep splat at boot time, due > > to the new expectation that the driver has to initialize the > > per-port spinlock itself. > > > > It remains unclear why a double initialization of the port > > spinlock is a desirable outcome, but in the meantime let's > > fix the splat. > > > > Thanks! > > Can you test patch from [1] if it helps and doesn't break anything in > your case? > > [1]: > https://lore.kernel.org/linux-serial/20200217114016.49856-1-andriy.shevche...@linux.intel.com/T/#m9255e2a7474b160e66c7060fca5323ca3df49cfd On its own, this patch doesn't seem to cure the issue (and it adds a compile-time warning due to unused flags). Ah, sorry, I didn't compile it. And after second though I think we simple need to initialise spin lock there. Can you try below (compile-tested only): From ed4c882e7dc3fdfcea706ada0678c060c36163b3 Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Sat, 4 Jul 2020 19:30:39 +0300 Subject: [PATCH 1/1] serial: core: Initialise spin lock before use in uart_configure_port() In case of the port to be used as a console we must initialise a spin lock before use. Signed-off-by: Andy Shevchenko --- drivers/tty/serial/serial_core.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index 3cc183acf7ba..a81b4900eb60 100644 --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -2371,6 +2371,13 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state, /* Power up port for set_mctrl() */ uart_change_pm(state, UART_PM_STATE_ON); + /* +* If this driver supports console, and it hasn't been +* successfully registered yet, initialise spin lock for it. +*/ + if (port->cons && !(port->cons->flags & CON_ENABLED)) + spin_lock_init(&port->lock); + /* * Ensure that the modem control lines are de-activated. * keep the DTR setting that is set in uart_set_options() -- 2.27.0 Or did you mean to test it in complement of my patch? No, the idea to avoid "fixing" driver as you rightfully noticed a double init issue. This certainly keeps lockdep quiet on my system. You probably want to set the lockdep class whilst you're at it (keeping that code common with uart_port_spin_lock_init). Thanks, M. -- Jazz is not dead. It just smells funny...
Re: [PATCH v4 08/15] arm64: kvm: Duplicate hyp/tlb.c for VHE/nVHE
On Thu, 25 Jun 2020 14:14:13 +0100,
David Brazdil wrote:
>
> tlb.c contains code for flushing the TLB, with code shared between VHE/nVHE.
> Because common code is small, duplicate tlb.c and specialize each copy for
> VHE/nVHE.
>
> Signed-off-by: David Brazdil
> ---
> arch/arm64/kernel/image-vars.h | 14 +--
> arch/arm64/kvm/hyp/Makefile | 2 +-
> arch/arm64/kvm/hyp/nvhe/Makefile| 2 +-
> arch/arm64/kvm/hyp/{ => nvhe}/tlb.c | 94 +---
> arch/arm64/kvm/hyp/vhe/Makefile | 2 +-
> arch/arm64/kvm/hyp/vhe/tlb.c| 162
> 6 files changed, 178 insertions(+), 98 deletions(-)
> rename arch/arm64/kvm/hyp/{ => nvhe}/tlb.c (62%)
> create mode 100644 arch/arm64/kvm/hyp/vhe/tlb.c
>
[...]
> diff --git a/arch/arm64/kvm/hyp/vhe/tlb.c b/arch/arm64/kvm/hyp/vhe/tlb.c
> new file mode 100644
> index ..35e8e112ba28
> --- /dev/null
> +++ b/arch/arm64/kvm/hyp/vhe/tlb.c
[...]
> +void __kvm_tlb_flush_vmid_ipa(struct kvm *kvm, phys_addr_t ipa)
> +{
> + struct tlb_inv_context cxt;
> +
> + dsb(ishst);
> +
> + /* Switch to requested VMID */
> + kvm = kern_hyp_va(kvm);
nit: this is now superfluous. I'll drop it locally.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
Re: [PATCH v4 08/15] arm64: kvm: Duplicate hyp/tlb.c for VHE/nVHE
On Thu, 25 Jun 2020 14:14:13 +0100,
David Brazdil wrote:
>
> tlb.c contains code for flushing the TLB, with code shared between VHE/nVHE.
> Because common code is small, duplicate tlb.c and specialize each copy for
> VHE/nVHE.
>
> Signed-off-by: David Brazdil
> ---
> arch/arm64/kernel/image-vars.h | 14 +--
> arch/arm64/kvm/hyp/Makefile | 2 +-
> arch/arm64/kvm/hyp/nvhe/Makefile| 2 +-
> arch/arm64/kvm/hyp/{ => nvhe}/tlb.c | 94 +---
> arch/arm64/kvm/hyp/vhe/Makefile | 2 +-
> arch/arm64/kvm/hyp/vhe/tlb.c| 162
> 6 files changed, 178 insertions(+), 98 deletions(-)
> rename arch/arm64/kvm/hyp/{ => nvhe}/tlb.c (62%)
> create mode 100644 arch/arm64/kvm/hyp/vhe/tlb.c
[...]
> diff --git a/arch/arm64/kvm/hyp/tlb.c b/arch/arm64/kvm/hyp/nvhe/tlb.c
> similarity index 62%
> rename from arch/arm64/kvm/hyp/tlb.c
> rename to arch/arm64/kvm/hyp/nvhe/tlb.c
> index d063a576d511..9513ad41db9a 100644
> --- a/arch/arm64/kvm/hyp/tlb.c
> +++ b/arch/arm64/kvm/hyp/nvhe/tlb.c
> @@ -4,8 +4,6 @@
> * Author: Marc Zyngier
> */
>
> -#include
> -
> #include
> #include
> #include
> @@ -16,52 +14,8 @@ struct tlb_inv_context {
> u64 sctlr;
> };
nit: You seem to have overlooked that some of the tlb_inv_context
fields are now unused. I plan to squash the following patch in:
diff --git a/arch/arm64/kvm/hyp/nvhe/tlb.c b/arch/arm64/kvm/hyp/nvhe/tlb.c
index 6fe190bb930a..d4475f8340c4 100644
--- a/arch/arm64/kvm/hyp/nvhe/tlb.c
+++ b/arch/arm64/kvm/hyp/nvhe/tlb.c
@@ -9,9 +9,7 @@
#include
struct tlb_inv_context {
- unsigned long flags;
u64 tcr;
- u64 sctlr;
};
static void __tlb_switch_to_guest(struct kvm *kvm, struct tlb_inv_context *cxt)
Otherwise, this looks good to me.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
Re: [PATCH v4 07/15] arm64: kvm: Move hyp-init.S to nVHE
Hi David, On Thu, 25 Jun 2020 14:14:12 +0100, David Brazdil wrote: > > From: Andrew Scull > > hyp-init.S contains the identity mapped initialisation code for the > non-VHE code that runs at EL2. It is only used for non-VHE. > > Adjust code that calls into this to use the prefixed symbol name. > > Signed-off-by: Andrew Scull > > [David: pass idmap_t0sz as an argument] It is unclear to me why moving the way idmap_t0sz is passed is required at this stage. I understand that you want to minimise the amount of shared data between EL1 and EL2, but it hardly seems relevant here. Or is it, as I expect, to avoid yet another symbol renaming issue? If so, it would be preferable to have the symbol alias, keep the setup hypercall as is, and have a later, separate patch that deals with the the idmap. And I am pretty sure that, as we move to a more autonomous EL2, we won't have to deal with it at all and we'll simply delete this code. I'm planning to squash the following diff into this patch, effectively reverting the idmap_t0sz related changes. Let me know if you're OK with it. diff --git a/arch/arm64/kernel/image-vars.h b/arch/arm64/kernel/image-vars.h index 8ba32bff7bb2..9e897c500237 100644 --- a/arch/arm64/kernel/image-vars.h +++ b/arch/arm64/kernel/image-vars.h @@ -83,6 +83,9 @@ KVM_NVHE_ALIAS(panic); /* Vectors installed by hyp-init on reset HVC. */ KVM_NVHE_ALIAS(__hyp_stub_vectors); +/* IDMAP TCR_EL1.T0SZ as computed by the EL1 init code */ +KVM_NVHE_ALIAS(idmap_t0sz); + /* Kernel symbol used by icache_is_vpipt(). */ KVM_NVHE_ALIAS(__icache_flags); diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 8ca2c111cec2..0bf2cf5614c6 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -1296,7 +1296,7 @@ static void cpu_init_hyp_mode(void) * cpus_have_const_cap() wrapper. */ BUG_ON(!system_capabilities_finalized()); - __kvm_call_hyp((void *)pgd_ptr, hyp_stack_ptr, vector_ptr, tpidr_el2, idmap_t0sz); + __kvm_call_hyp((void *)pgd_ptr, hyp_stack_ptr, vector_ptr, tpidr_el2); /* * Disabling SSBD on a non-VHE system requires us to enable SSBS diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-init.S b/arch/arm64/kvm/hyp/nvhe/hyp-init.S index 7bb75acbede0..6e6ed5581eed 100644 --- a/arch/arm64/kvm/hyp/nvhe/hyp-init.S +++ b/arch/arm64/kvm/hyp/nvhe/hyp-init.S @@ -47,24 +47,23 @@ __invalid: * x1: HYP stack * x2: HYP vectors * x3: per-CPU offset -* x4: idmap_t0sz */ __do_hyp_init: /* Check for a stub HVC call */ cmp x0, #HVC_STUB_HCALL_NR b.lo__kvm_handle_stub_hvc - phys_to_ttbr x5, x0 + phys_to_ttbr x4, x0 alternative_if ARM64_HAS_CNP - orr x5, x5, #TTBR_CNP_BIT + orr x4, x4, #TTBR_CNP_BIT alternative_else_nop_endif - msr ttbr0_el2, x5 + msr ttbr0_el2, x4 - mrs x5, tcr_el1 - mov_q x6, TCR_EL2_MASK - and x5, x5, x6 - mov x6, #TCR_EL2_RES1 - orr x5, x5, x6 + mrs x4, tcr_el1 + mov_q x5, TCR_EL2_MASK + and x4, x4, x5 + mov x5, #TCR_EL2_RES1 + orr x4, x4, x5 /* * The ID map may be configured to use an extended virtual address @@ -80,14 +79,15 @@ alternative_else_nop_endif * * So use the same T0SZ value we use for the ID map. */ - bfi x5, x4, TCR_T0SZ_OFFSET, TCR_TxSZ_WIDTH + ldr_l x5, idmap_t0sz + bfi x4, x5, TCR_T0SZ_OFFSET, TCR_TxSZ_WIDTH /* * Set the PS bits in TCR_EL2. */ - tcr_compute_pa_size x5, #TCR_EL2_PS_SHIFT, x4, x6 + tcr_compute_pa_size x4, #TCR_EL2_PS_SHIFT, x5, x6 - msr tcr_el2, x5 + msr tcr_el2, x4 mrs x4, mair_el1 msr mair_el2, x4 Thanks, M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH] tty: serial: meson_uart: Init port lock early
On 2020-07-05 11:07, Andy Shevchenko wrote: On Sun, Jul 5, 2020 at 12:32 PM Marc Zyngier wrote: The meson UART driver triggers a lockdep splat at boot time, due to the new expectation that the driver has to initialize the per-port spinlock itself. It remains unclear why a double initialization of the port spinlock is a desirable outcome, but in the meantime let's fix the splat. Thanks! Can you test patch from [1] if it helps and doesn't break anything in your case? [1]: https://lore.kernel.org/linux-serial/20200217114016.49856-1-andriy.shevche...@linux.intel.com/T/#m9255e2a7474b160e66c7060fca5323ca3df49cfd On its own, this patch doesn't seem to cure the issue (and it adds a compile-time warning due to unused flags). Or did you mean to test it in complement of my patch? M. -- Jazz is not dead. It just smells funny...
[PATCH] tty: serial: meson_uart: Init port lock early
The meson UART driver triggers a lockdep splat at boot time, due
to the new expectation that the driver has to initialize the
per-port spinlock itself.
It remains unclear why a double initialization of the port
spinlock is a desirable outcome, but in the meantime let's
fix the splat.
Fixes: a3cb39d258ef ("serial: core: Allow detach and attach serial device for
console")
Cc: Andy Shevchenko
Cc: Greg Kroah-Hartman
Signed-off-by: Marc Zyngier
---
drivers/tty/serial/meson_uart.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index d2c08b760f83..386e39c90628 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -759,6 +759,9 @@ static int meson_uart_probe(struct platform_device *pdev)
if (ret)
return ret;
+ /* Init the spinlock early in case this is the console */
+ spin_lock_init(&port->lock);
+
port->iotype = UPIO_MEM;
port->mapbase = res_mem->start;
port->mapsize = resource_size(res_mem);
--
2.26.2
Re: [PATCH 0/2] genirq: Kill preflow handlers
Hi Valentin, On 2020-07-03 16:56, Valentin Schneider wrote: Hi, while strolling around the different flow handlers, I tried to make sense of what preflow_handler() was about. Turns out no one uses those anymore, but the genirq support has remained in place. If we needed to reintroduce some form of preflow handler, we'd try and do it using hierarchical irqchips, if at all possible. Unless we can see another user of those in the near future, this seems like as good a time as any for a little housecleaning. - Patch 1 simply deselects the (unexploited) preflow Kconfig for sparc64 - Patch 2 is the actual cleanup Cheers, Valentin Valentin Schneider (2): sparc64: Deselect IRQ_PREFLOW_FASTEOI genirq: Remove preflow handler support arch/sparc/Kconfig | 1 - include/linux/irqdesc.h| 15 --- include/linux/irqhandler.h | 1 - kernel/irq/Kconfig | 4 kernel/irq/chip.c | 13 - 5 files changed, 34 deletions(-) For the whole series, and assuming that there is no regression (can't imagine any for unused code): Reviewed-by: Marc Zyngier Thanks, M. -- Jazz is not dead. It just smells funny...
Re: [PATCHv3 2/6] irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts
On 2020-07-03 15:28, Grzegorz Jaszczyk wrote:
On Thu, 2 Jul 2020 at 19:24, Marc Zyngier wrote:
On 2020-07-02 15:17, Grzegorz Jaszczyk wrote:
> From: Suman Anna
>
> The Programmable Real-Time Unit Subsystem (PRUSS) contains a local
> interrupt controller (INTC) that can handle various system input events
> and post interrupts back to the device-level initiators. The INTC can
> support upto 64 input events with individual control configuration and
> hardware prioritization. These events are mapped onto 10 output
> interrupt
> lines through two levels of many-to-one mapping support. Different
> interrupt lines are routed to the individual PRU cores or to the host
> CPU, or to other devices on the SoC. Some of these events are sourced
> from peripherals or other sub-modules within that PRUSS, while a few
> others are sourced from SoC-level peripherals/devices.
>
> The PRUSS INTC platform driver manages this PRUSS interrupt controller
> and implements an irqchip driver to provide a Linux standard way for
> the PRU client users to enable/disable/ack/re-trigger a PRUSS system
> event. The system events to interrupt channels and output interrupts
> relies on the mapping configuration provided either through the PRU
> firmware blob or via the PRU application's device tree node. The
> mappings will be programmed during the boot/shutdown of a PRU core.
>
> The PRUSS INTC module is reference counted during the interrupt
> setup phase through the irqchip's irq_request_resources() and
> irq_release_resources() ops. This restricts the module from being
> removed as long as there are active interrupt users.
>
> The driver currently supports and can be built for OMAP architecture
> based AM335x, AM437x and AM57xx SoCs; Keystone2 architecture based
> 66AK2G SoCs and Davinci architecture based OMAP-L13x/AM18x/DA850 SoCs.
> All of these SoCs support 64 system events, 10 interrupt channels and
> 10 output interrupt lines per PRUSS INTC with a few SoC integration
> differences.
>
> NOTE:
> Each PRU-ICSS's INTC on AM57xx SoCs is preceded by a Crossbar that
> enables multiple external events to be routed to a specific number
> of input interrupt events. Any non-default external interrupt event
> directed towards PRUSS needs this crossbar to be setup properly.
>
> Signed-off-by: Suman Anna
> Signed-off-by: Andrew F. Davis
> Signed-off-by: Roger Quadros
> Signed-off-by: Grzegorz Jaszczyk
> Reviewed-by: Lee Jones
> ---
> v2->v3:
> - use single irqchip description instead of separately allocating it
> for
> each pruss_intc
> - get rid of unused mutex
> - improve error handling
> v1->v2:
> - https://patchwork.kernel.org/patch/11069771/
> +static void pruss_intc_init(struct pruss_intc *intc)
> +{
> + int i;
> +
> + /* configure polarity to active high for all system interrupts */
> + pruss_intc_write_reg(intc, PRU_INTC_SIPR0, 0x);
> + pruss_intc_write_reg(intc, PRU_INTC_SIPR1, 0x);
> +
> + /* configure type to pulse interrupt for all system interrupts */
> + pruss_intc_write_reg(intc, PRU_INTC_SITR0, 0);
> + pruss_intc_write_reg(intc, PRU_INTC_SITR1, 0);
So the default is to configure everything as edge...
Sorry, the description is wrong - '0' indicates level and '1' edge. So
the default configuration is level - I will fix the comment.
> +
> + /* clear all 16 interrupt channel map registers */
> + for (i = 0; i < 16; i++)
> + pruss_intc_write_reg(intc, PRU_INTC_CMR(i), 0);
> +
> + /* clear all 3 host interrupt map registers */
> + for (i = 0; i < 3; i++)
> + pruss_intc_write_reg(intc, PRU_INTC_HMR(i), 0);
> +}
> +
> +static void pruss_intc_irq_ack(struct irq_data *data)
> +{
> + struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
> + unsigned int hwirq = data->hwirq;
> +
> + pruss_intc_write_reg(intc, PRU_INTC_SICR, hwirq);
> +}
> +
> +static void pruss_intc_irq_mask(struct irq_data *data)
> +{
> + struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
> + unsigned int hwirq = data->hwirq;
> +
> + pruss_intc_write_reg(intc, PRU_INTC_EICR, hwirq);
> +}
> +
> +static void pruss_intc_irq_unmask(struct irq_data *data)
> +{
> + struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
> + unsigned int hwirq = data->hwirq;
> +
> + pruss_intc_write_reg(intc, PRU_INTC_EISR, hwirq);
> +}
> +
> +static int pruss_intc_irq_reqres(struct irq_data *data)
> +{
> + if (!try_module_get(THIS_MODULE))
> + return -ENODEV;
> +
> + return 0;
> +}
> +
> +static void pruss_intc_irq_relres(struct irq_data *dat
Re: [PATCHv3 5/6] irqchip/irq-pruss-intc: Add support for ICSSG INTC on K3 SoCs
On 2020-07-02 15:17, Grzegorz Jaszczyk wrote:
From: Suman Anna
The K3 AM65x and J721E SoCs have the next generation of the PRU-ICSS
IP,
commonly called ICSSG. The PRUSS INTC present within the ICSSG supports
more System Events (160 vs 64), more Interrupt Channels and Host
Interrupts
(20 vs 10) compared to the previous generation PRUSS INTC instances.
The
first 2 and the last 10 of these host interrupt lines are used by the
PRU and other auxiliary cores and sub-modules within the ICSSG, with 8
host interrupts connected to MPU. The host interrupts 5, 6, 7 are also
connected to the other ICSSG instances within the SoC and can be
partitioned as per system integration through the board dts files.
Enhance the PRUSS INTC driver to add support for this ICSSG INTC
instance. This support is added using specific compatible and match
data and updating the code to use this data instead of the current
hard-coded macros. The INTC config structure is updated to use the
higher events and channels on all SoCs, while limiting the actual
processing to only the relevant number of events/channels/interrupts.
Signed-off-by: Suman Anna
Signed-off-by: Grzegorz Jaszczyk
---
v2->v3:
- Change patch order: use it directly after "irqchip/irq-pruss-intc:
Implement irq_{get,set}_irqchip_state ops" and before new
"irqchip/irq-pruss-intc: Add event mapping support" in order to
reduce
diff.
The diff would be even smaller if you introduced a variable number of
inputs the first place, i.e. in patch #2. Most if this patch just
retrofits it. Please squash these changes into that initial patch,
and only add the platform stuff here.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Re: [PATCHv3 4/6] irqchip/irq-pruss-intc: Implement irq_{get,set}_irqchip_state ops
On 2020-07-02 15:17, Grzegorz Jaszczyk wrote:
From: David Lechner
This implements the irq_get_irqchip_state and irq_set_irqchip_state
callbacks for the TI PRUSS INTC driver. The set callback can be used
by drivers to "kick" a PRU by enabling a PRU system event.
"enabling"? That'd be unmasking an interrupt, which isn't what this
does. "injecting", maybe?
Example:
irq_set_irqchip_state(irq, IRQCHIP_STATE_PENDING, true);
Nice example.
What this example does explain is how you are actually going to kick
a PRU via this interface. For that to happen, you'd have to have on
the Linux side an interrupt that is actually routed to a PRU.
And from what I have understood of the previous patches, this can't
be the case. What didi I miss?
Signed-off-by: David Lechner
Signed-off-by: Suman Anna
Signed-off-by: Grzegorz Jaszczyk
Reviewed-by: Lee Jones
---
v2->v3:
- Get rid of unnecessary pruss_intc_check_write() and use
pruss_intc_write_reg directly.
v1->v2:
- https://patchwork.kernel.org/patch/11069769/
---
drivers/irqchip/irq-pruss-intc.c | 43
++--
1 file changed, 41 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-pruss-intc.c
b/drivers/irqchip/irq-pruss-intc.c
index 49c936f..19b3d38 100644
--- a/drivers/irqchip/irq-pruss-intc.c
+++ b/drivers/irqchip/irq-pruss-intc.c
@@ -7,6 +7,7 @@
* Suman Anna
*/
+#include
#include
#include
#include
@@ -39,8 +40,7 @@
#define PRU_INTC_HIEISR0x0034
#define PRU_INTC_HIDISR0x0038
#define PRU_INTC_GPIR 0x0080
-#define PRU_INTC_SRSR0 0x0200
-#define PRU_INTC_SRSR1 0x0204
+#define PRU_INTC_SRSR(x) (0x0200 + (x) * 4)
#define PRU_INTC_SECR0 0x0280
#define PRU_INTC_SECR1 0x0284
#define PRU_INTC_ESR0 0x0300
@@ -145,6 +145,43 @@ static void pruss_intc_irq_relres(struct irq_data
*data)
module_put(THIS_MODULE);
}
+static int pruss_intc_irq_get_irqchip_state(struct irq_data *data,
+ enum irqchip_irq_state which,
+ bool *state)
+{
+ struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
+ u32 reg, mask, srsr;
+
+ if (which != IRQCHIP_STATE_PENDING)
+ return -EINVAL;
+
+ reg = PRU_INTC_SRSR(data->hwirq / 32);
I assume the register file scales as more interrupts are added in the
subsequent patch?
+ mask = BIT(data->hwirq % 32);
+
+ srsr = pruss_intc_read_reg(intc, reg);
+
+ *state = !!(srsr & mask);
+
+ return 0;
+}
+
+static int pruss_intc_irq_set_irqchip_state(struct irq_data *data,
+ enum irqchip_irq_state which,
+ bool state)
+{
+ struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
+
+ if (which != IRQCHIP_STATE_PENDING)
+ return -EINVAL;
+
+ if (state)
+ pruss_intc_write_reg(intc, PRU_INTC_SISR, data->hwirq);
+ else
+ pruss_intc_write_reg(intc, PRU_INTC_SICR, data->hwirq);
+
+ return 0;
+}
+
static struct irq_chip pruss_irqchip = {
.name = "pruss-intc",
.irq_ack = pruss_intc_irq_ack,
@@ -152,6 +189,8 @@ static struct irq_chip pruss_irqchip = {
.irq_unmask = pruss_intc_irq_unmask,
.irq_request_resources = pruss_intc_irq_reqres,
.irq_release_resources = pruss_intc_irq_relres,
+ .irq_get_irqchip_state = pruss_intc_irq_get_irqchip_state,
+ .irq_set_irqchip_state = pruss_intc_irq_set_irqchip_state,
};
static int pruss_intc_irq_domain_map(struct irq_domain *d, unsigned
int virq,
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Re: [PATCHv3 3/6] irqchip/irq-pruss-intc: Add support for shared and invalid interrupts
On 2020-07-02 15:17, Grzegorz Jaszczyk wrote:
From: Suman Anna
The PRUSS INTC has a fixed number of output interrupt lines that are
connected to a number of processors or other PRUSS instances or other
devices (like DMA) on the SoC. The output interrupt lines 2 through 9
are usually connected to the main Arm host processor and are referred
to as host interrupts 0 through 7 from ARM/MPU perspective.
All of these 8 host interrupts are not always exclusively connected
to the Arm interrupt controller. Some SoCs have some interrupt lines
not connected to the Arm interrupt controller at all, while a few
others
have the interrupt lines connected to multiple processors in which they
need to be partitioned as per SoC integration needs. For example,
AM437x
and 66AK2G SoCs have 2 PRUSS instances each and have the host interrupt
5
connected to the other PRUSS, while AM335x has host interrupt 0 shared
between MPU and TSC_ADC and host interrupts 6 & 7 shared between MPU
and
a DMA controller.
Add support to the PRUSS INTC driver to allow both these shared and
invalid interrupts by not returning a failure if any of these
interrupts
are skipped from the corresponding INTC DT node.
That's not exactly "adding support", is it? It really is "ignore these
interrupts because they are useless from the main CPU's perspective",
right?
Signed-off-by: Suman Anna
Signed-off-by: Grzegorz Jaszczyk
---
v2->v3:
- Extra checks for (intc->irqs[i]) in error/remove path was moved from
"irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS
interrupts" to this patch
v1->v2:
- https://patchwork.kernel.org/patch/11069757/
---
drivers/irqchip/irq-pruss-intc.c | 73
+---
1 file changed, 68 insertions(+), 5 deletions(-)
diff --git a/drivers/irqchip/irq-pruss-intc.c
b/drivers/irqchip/irq-pruss-intc.c
index fb3dda3..49c936f 100644
--- a/drivers/irqchip/irq-pruss-intc.c
+++ b/drivers/irqchip/irq-pruss-intc.c
@@ -65,11 +65,15 @@
* @irqs: kernel irq numbers corresponding to PRUSS host interrupts
* @base: base virtual address of INTC register space
* @domain: irq domain for this interrupt controller
+ * @shared_intr: bit-map denoting if the MPU host interrupt is shared
nit: bitmap
+ * @invalid_intr: bit-map denoting if host interrupt is not connected
to MPU
*/
struct pruss_intc {
unsigned int irqs[MAX_NUM_HOST_IRQS];
void __iomem *base;
struct irq_domain *domain;
+ u16 shared_intr;
+ u16 invalid_intr;
Please represent bitmaps as an unsigned long.
};
static inline u32 pruss_intc_read_reg(struct pruss_intc *intc,
unsigned int reg)
@@ -222,7 +226,8 @@ static int pruss_intc_probe(struct platform_device
*pdev)
"host_intr4", "host_intr5", "host_intr6", "host_intr7", };
struct device *dev = &pdev->dev;
struct pruss_intc *intc;
- int i, irq;
+ int i, irq, count;
+ u8 temp_intr[MAX_NUM_HOST_IRQS] = { 0 };
intc = devm_kzalloc(dev, sizeof(*intc), GFP_KERNEL);
if (!intc)
@@ -235,6 +240,52 @@ static int pruss_intc_probe(struct platform_device
*pdev)
return PTR_ERR(intc->base);
}
+ count = of_property_read_variable_u8_array(dev->of_node,
+ "ti,irqs-reserved",
+ temp_intr, 0,
+ MAX_NUM_HOST_IRQS);
+ /*
+* The irqs-reserved is used only for some SoC's therefore not having
+* this property is still valid
+*/
+ if (count == -EINVAL)
+ count = 0;
+ if (count < 0)
+ return count;
+
+ for (i = 0; i < count; i++) {
+ if (temp_intr[i] >= MAX_NUM_HOST_IRQS) {
+ dev_warn(dev, "ignoring invalid reserved irq %d\n",
+temp_intr[i]);
+ continue;
+ }
+
+ intc->invalid_intr |= BIT(temp_intr[i]);
+ }
+
+ count = of_property_read_variable_u8_array(dev->of_node,
+ "ti,irqs-shared",
+ temp_intr, 0,
+ MAX_NUM_HOST_IRQS);
+ /*
+* The irqs-shared is used only for some SoC's therefore not having
+* this property is still valid
+*/
+ if (count == -EINVAL)
+ count = 0;
+ if (count < 0)
+ return count;
+
+ for (i = 0; i < count; i++) {
+ if (temp_intr[i] >= MAX_NUM_HOST_IRQS) {
+ dev_warn(dev, "ignoring invalid shared irq %d\n",
+temp_intr[i]);
+ continue;
+ }
+
+ intc->shared_intr |= BIT(temp_intr[i]);
+ }
+
You probably want to move this in a separate function, since you
popul
Re: [PATCHv3 2/6] irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts
On 2020-07-02 15:17, Grzegorz Jaszczyk wrote: From: Suman Anna The Programmable Real-Time Unit Subsystem (PRUSS) contains a local interrupt controller (INTC) that can handle various system input events and post interrupts back to the device-level initiators. The INTC can support upto 64 input events with individual control configuration and hardware prioritization. These events are mapped onto 10 output interrupt lines through two levels of many-to-one mapping support. Different interrupt lines are routed to the individual PRU cores or to the host CPU, or to other devices on the SoC. Some of these events are sourced from peripherals or other sub-modules within that PRUSS, while a few others are sourced from SoC-level peripherals/devices. The PRUSS INTC platform driver manages this PRUSS interrupt controller and implements an irqchip driver to provide a Linux standard way for the PRU client users to enable/disable/ack/re-trigger a PRUSS system event. The system events to interrupt channels and output interrupts relies on the mapping configuration provided either through the PRU firmware blob or via the PRU application's device tree node. The mappings will be programmed during the boot/shutdown of a PRU core. The PRUSS INTC module is reference counted during the interrupt setup phase through the irqchip's irq_request_resources() and irq_release_resources() ops. This restricts the module from being removed as long as there are active interrupt users. The driver currently supports and can be built for OMAP architecture based AM335x, AM437x and AM57xx SoCs; Keystone2 architecture based 66AK2G SoCs and Davinci architecture based OMAP-L13x/AM18x/DA850 SoCs. All of these SoCs support 64 system events, 10 interrupt channels and 10 output interrupt lines per PRUSS INTC with a few SoC integration differences. NOTE: Each PRU-ICSS's INTC on AM57xx SoCs is preceded by a Crossbar that enables multiple external events to be routed to a specific number of input interrupt events. Any non-default external interrupt event directed towards PRUSS needs this crossbar to be setup properly. Signed-off-by: Suman Anna Signed-off-by: Andrew F. Davis Signed-off-by: Roger Quadros Signed-off-by: Grzegorz Jaszczyk Reviewed-by: Lee Jones --- v2->v3: - use single irqchip description instead of separately allocating it for each pruss_intc - get rid of unused mutex - improve error handling v1->v2: - https://patchwork.kernel.org/patch/11069771/ --- drivers/irqchip/Kconfig | 10 ++ drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-pruss-intc.c | 307 +++ 3 files changed, 318 insertions(+) create mode 100644 drivers/irqchip/irq-pruss-intc.c diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index 29fead2..733d7ec 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -493,6 +493,16 @@ config TI_SCI_INTA_IRQCHIP If you wish to use interrupt aggregator irq resources managed by the TI System Controller, say Y here. Otherwise, say N. +config TI_PRUSS_INTC + tristate "TI PRU-ICSS Interrupt Controller" + depends on ARCH_DAVINCI || SOC_AM33XX || SOC_AM43XX || SOC_DRA7XX || ARCH_KEYSTONE + select IRQ_DOMAIN + help + This enables support for the PRU-ICSS Local Interrupt Controller + present within a PRU-ICSS subsystem present on various TI SoCs. + The PRUSS INTC enables various interrupts to be routed to multiple + different processors within the SoC. + config RISCV_INTC bool "RISC-V Local Interrupt Controller" depends on RISCV diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index 133f9c4..990a106 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -106,6 +106,7 @@ obj-$(CONFIG_MADERA_IRQ)+= irq-madera.o obj-$(CONFIG_LS1X_IRQ) += irq-ls1x.o obj-$(CONFIG_TI_SCI_INTR_IRQCHIP) += irq-ti-sci-intr.o obj-$(CONFIG_TI_SCI_INTA_IRQCHIP) += irq-ti-sci-inta.o +obj-$(CONFIG_TI_PRUSS_INTC)+= irq-pruss-intc.o obj-$(CONFIG_LOONGSON_LIOINTC) += irq-loongson-liointc.o obj-$(CONFIG_LOONGSON_HTPIC) += irq-loongson-htpic.o obj-$(CONFIG_LOONGSON_HTVEC) += irq-loongson-htvec.o diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c new file mode 100644 index 000..fb3dda3 --- /dev/null +++ b/drivers/irqchip/irq-pruss-intc.c @@ -0,0 +1,307 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * PRU-ICSS INTC IRQChip driver for various TI SoCs + * + * Copyright (C) 2016-2020 Texas Instruments Incorporated - http://www.ti.com/ + * Andrew F. Davis + * Suman Anna + */ + +#include +#include +#include +#include +#include +#include + +/* + * Number of host interrupts reaching the main MPU sub-system. Note that this + * is not the same as the total number of host interrupts supported by the PRUSS + * INTC instance + */
Re: [PATCH 2/3] arm64: arch_timer: Allow an workaround descriptor to provide vdso_clock_mode
On 2020-07-02 11:28, Mark Rutland wrote:
On Wed, Jul 01, 2020 at 05:18:23PM +0100, Marc Zyngier wrote:
As we are about to disable the vdso for compat tasks in some
circumstances,
let's allow a workaround descriptor to provide the vdso_clock_mode
that
matches the platform.
Signed-off-by: Marc Zyngier
---
arch/arm64/include/asm/arch_timer.h | 3 +++
drivers/clocksource/arm_arch_timer.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/arch/arm64/include/asm/arch_timer.h
b/arch/arm64/include/asm/arch_timer.h
index 7ae54d7d333a..fb8dfcbf9c01 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -18,6 +18,8 @@
#include
#include
+#include
+
#include
#if IS_ENABLED(CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND)
@@ -58,6 +60,7 @@ struct arch_timer_erratum_workaround {
u64 (*read_cntvct_el0)(void);
int (*set_next_event_phys)(unsigned long, struct clock_event_device
*);
int (*set_next_event_virt)(unsigned long, struct clock_event_device
*);
+ enum vdso_clock_mode vdso_clock_mode;
};
DECLARE_PER_CPU(const struct arch_timer_erratum_workaround *,
diff --git a/drivers/clocksource/arm_arch_timer.c
b/drivers/clocksource/arm_arch_timer.c
index ecf7b7db2d05..f828835c568f 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -566,6 +566,9 @@ void arch_timer_enable_workaround(const struct
arch_timer_erratum_workaround *wa
if (wa->read_cntvct_el0) {
clocksource_counter.vdso_clock_mode = VDSO_CLOCKMODE_NONE;
vdso_default = VDSO_CLOCKMODE_NONE;
+ } else {
+ clocksource_counter.vdso_clock_mode = wa->vdso_clock_mode;
+ vdso_default = wa->vdso_clock_mode;
}
I fear that we're liable to forget to set vdso_clock_mode on new errata
that don't need a read_cntvct_el0 hook, and if so we'll happen to set
these to 0 (i.e. VDSO_CLOCKMODE_NONE).
For now could we instead have a boolan disable_compat_vdso, with this
being:
| if (wa->read_cntvct_el0) {
| clocksource_counter.vdso_clock_mode = VDSO_CLOCKMODE_NONE;
| vdso_default = VDSO_CLOCKMODE_NONE;
| } else if (wa->disable_compat_vdso) {
| clocksource_counter.vdso_clock_mode =
VDSO_CLOCKMODE_ARCHTIMER_NOCOMPAT;
| vdso_default = wa->vdso_clock_mode =
VDSO_CLOCKMODE_ARCHTIMER_NOCOMPAT;
| }
Sure, that'd work too.
Do we need to handle the comination of a workaround seeting NONE and
one
setting ARCHTIMER_NOCOMPAT?
Not yet. Probably coming any time now. Which is why we may need to
look into per-CPU vdso data pages (Broonie has a series that could
be of some use, apparently).
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Re: [PATCH v2 06/17] irqchip/gic-v3: Configure SGIs as standard interrupts
On 2020-07-02 14:23, Valentin Schneider wrote:
On 30/06/20 11:15, Marc Zyngier wrote:
On 2020-06-25 19:25, Valentin Schneider wrote:
Also, while staring at this it dawned on me that IPI's don't need the
eoimode=0 isb(): due to how the IPI flow-handler is structured, we'll
get a
gic_eoi_irq() just before calling into the irqaction. Dunno how much
we
care about it.
That's interesting. This ISB is a leftover from the loop we had before
the pseudo-NMI code, where we had to make sure the write to EOIR was
ordered with the read from IAR.
Given that we have an exception return right after the interrupt
handling, I *think* we could get rid of it (but that would need
mode checking on broken systems such as TX1...). I don't think
this is specific to IPIs though.
If I got this one right:
39a06b67c2c1 ("irqchip/gic: Ensure we have an ISB between ack and
->handle_irq")
you're describing case 2, which is indeed gone on gic-v3. However IIUC
we
also want an ISB between poking IAR and calling into the irqaction
(case 1)
- we get just that with IPIs due to the early gic_eoi_irq(), but we
don't
for the other flows.
You just made me realise something amazing: I've started to forget about
all this crap. Which is wonderful! ;-)
More seriously, you are absolutely right. If we wanted to address this,
we'd probably have to give IPIs their own irqchip so that they get their
own eoi callback. Not sure that's worth it.
M.
--
Jazz is not dead. It just smells funny...
Re: [PATCH v2 14/17] arm64: Kill __smp_cross_call and co
On 2020-06-25 19:25, Valentin Schneider wrote:
On 24/06/20 20:58, Marc Zyngier wrote:
@@ -852,8 +841,7 @@ void arch_send_wakeup_ipi_mask(const struct
cpumask *mask)
#ifdef CONFIG_IRQ_WORK
void arch_irq_work_raise(void)
{
- if (__smp_cross_call)
- smp_cross_call(cpumask_of(smp_processor_id()), IPI_IRQ_WORK);
+ smp_cross_call(cpumask_of(smp_processor_id()), IPI_IRQ_WORK);
AIU the following commit:
eb631bb5bf5b ("arm64: Support arch_irq_work_raise() via self IPIs")
It seems arm64 hasn't needed that check since
4b3dc9679cf7 ("arm64: force CONFIG_SMP=y and remove redundant
#ifdefs")
Did I get that right?
Indeed. SMP implies being able to IPI, and thus the check has been
irrelevant for some time now.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
[PATCH 3/3] arm64: timers: Disable the compat vdso for cores affected by ARM64_WORKAROUND_1418040
ARM64_WORKAROUND_1418040 requires that AArch32 EL0 accesses to
the virtual counter register are trapped and emulated by the kernel.
This makes the vdso pretty pointless, and in some cases livelock
prone.
Provide a workaround entry that limits the vdso to 64bit tasks.
Signed-off-by: Marc Zyngier
---
drivers/clocksource/arm_arch_timer.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/clocksource/arm_arch_timer.c
b/drivers/clocksource/arm_arch_timer.c
index f828835c568f..9c61252e7d99 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -480,6 +480,14 @@ static const struct arch_timer_erratum_workaround
ool_workarounds[] = {
.set_next_event_virt = erratum_set_next_event_tval_virt,
},
#endif
+#ifdef CONFIG_ARM64_ERRATUM_1418040
+ {
+ .match_type = ate_match_local_cap_id,
+ .id = (void *)ARM64_WORKAROUND_1418040,
+ .desc = "ARM erratum 1418040",
+ .vdso_clock_mode = VDSO_CLOCKMODE_ARCHTIMER_NOCOMPAT,
+ },
+#endif
};
typedef bool (*ate_match_fn_t)(const struct arch_timer_erratum_workaround *,
--
2.27.0
[PATCH 1/3] arm64: Introduce a way to disable the 32bit vdso
We have a class of errata (grouped under the ARM64_WORKAROUND_1418040
banner) that force the trapping of counter access from 32bit EL0.
We would normally disable the whole vdso for such defect, except that
it would disable it for 64bit userspace as well, which is a shame.
Instead, add a new vdso_clock_mode, which signals that the vdso
isn't usable for compat tasks. This gets checked in the new
vdso_clocksource_ok() helper, now provided for the 32bit vdso.
Signed-off-by: Marc Zyngier
---
arch/arm64/include/asm/vdso/clocksource.h | 7 +--
arch/arm64/include/asm/vdso/compat_gettimeofday.h | 8 +++-
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/include/asm/vdso/clocksource.h
b/arch/arm64/include/asm/vdso/clocksource.h
index df6ea65c1dec..b054d9febfb5 100644
--- a/arch/arm64/include/asm/vdso/clocksource.h
+++ b/arch/arm64/include/asm/vdso/clocksource.h
@@ -2,7 +2,10 @@
#ifndef __ASM_VDSOCLOCKSOURCE_H
#define __ASM_VDSOCLOCKSOURCE_H
-#define VDSO_ARCH_CLOCKMODES \
- VDSO_CLOCKMODE_ARCHTIMER
+#define VDSO_ARCH_CLOCKMODES \
+ /* vdso clocksource for both 32 and 64bit tasks */ \
+ VDSO_CLOCKMODE_ARCHTIMER, \
+ /* vdso clocksource for 64bit tasks only */ \
+ VDSO_CLOCKMODE_ARCHTIMER_NOCOMPAT
#endif
diff --git a/arch/arm64/include/asm/vdso/compat_gettimeofday.h
b/arch/arm64/include/asm/vdso/compat_gettimeofday.h
index b6907ae78e53..9a625e8947ff 100644
--- a/arch/arm64/include/asm/vdso/compat_gettimeofday.h
+++ b/arch/arm64/include/asm/vdso/compat_gettimeofday.h
@@ -111,7 +111,7 @@ static __always_inline u64 __arch_get_hw_counter(s32
clock_mode)
* update. Return something. Core will do another round and then
* see the mode change and fallback to the syscall.
*/
- if (clock_mode == VDSO_CLOCKMODE_NONE)
+ if (clock_mode != VDSO_CLOCKMODE_ARCHTIMER)
return 0;
/*
@@ -152,6 +152,12 @@ static __always_inline const struct vdso_data
*__arch_get_vdso_data(void)
return ret;
}
+static inline bool vdso_clocksource_ok(const struct vdso_data *vd)
+{
+ return vd->clock_mode == VDSO_CLOCKMODE_ARCHTIMER;
+}
+#define vdso_clocksource_okvdso_clocksource_ok
+
#endif /* !__ASSEMBLY__ */
#endif /* __ASM_VDSO_GETTIMEOFDAY_H */
--
2.27.0
[PATCH 2/3] arm64: arch_timer: Allow an workaround descriptor to provide vdso_clock_mode
As we are about to disable the vdso for compat tasks in some circumstances,
let's allow a workaround descriptor to provide the vdso_clock_mode that
matches the platform.
Signed-off-by: Marc Zyngier
---
arch/arm64/include/asm/arch_timer.h | 3 +++
drivers/clocksource/arm_arch_timer.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/arch/arm64/include/asm/arch_timer.h
b/arch/arm64/include/asm/arch_timer.h
index 7ae54d7d333a..fb8dfcbf9c01 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -18,6 +18,8 @@
#include
#include
+#include
+
#include
#if IS_ENABLED(CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND)
@@ -58,6 +60,7 @@ struct arch_timer_erratum_workaround {
u64 (*read_cntvct_el0)(void);
int (*set_next_event_phys)(unsigned long, struct clock_event_device *);
int (*set_next_event_virt)(unsigned long, struct clock_event_device *);
+ enum vdso_clock_mode vdso_clock_mode;
};
DECLARE_PER_CPU(const struct arch_timer_erratum_workaround *,
diff --git a/drivers/clocksource/arm_arch_timer.c
b/drivers/clocksource/arm_arch_timer.c
index ecf7b7db2d05..f828835c568f 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -566,6 +566,9 @@ void arch_timer_enable_workaround(const struct
arch_timer_erratum_workaround *wa
if (wa->read_cntvct_el0) {
clocksource_counter.vdso_clock_mode = VDSO_CLOCKMODE_NONE;
vdso_default = VDSO_CLOCKMODE_NONE;
+ } else {
+ clocksource_counter.vdso_clock_mode = wa->vdso_clock_mode;
+ vdso_default = wa->vdso_clock_mode;
}
}
--
2.27.0
[PATCH 0/3] arm64: Allow the compat vdso to be disabled at runtime
The relatively recent introduction of the compat vdso on arm64 has overlooked its interactions with some of the interesting errata workarounds, such as ARM64_ERRATUM_1418040 (and its older 1188873 incarnation). This erratum requires the 64bit kernel to trap 32bit accesses to the virtual counter and emulate it. When the workaround was introduced, the compat vdso simply wasn't a thing. Now that the patches have landed in mainline, we trap the CVTVCT accesses from the vdso. This can end-up in a nasty loop in the vdso, where the sequence number changes on each trap, never stabilising, and leaving userspace in a bit of a funny state (which is why we disable the vdso in most similar cases). This erratum mentionned above is a bit special in the sense that in only requires to trap AArch32 accesses, and 64bit tasks can be left alone. Consequently, the vdso is never disabled and AArch32 tasks are affected. Obviously, we really want to retain the 64bit vdso in this case. To that effect, this series offers a way to disable the 32bit view of the vdso without impacting its 64bit counterpart, by providing a "no-compat" vdso clock_mode, and plugging this feature into the 1418040 detection code. I haven't put any Cc stable for now, but I believe we need to do something for older kernels. Marc Zyngier (3): arm64: Introduce a way to disable the 32bit vdso arm64: arch_timer: Allow an workaround descriptor to provide vdso_clock_mode arm64: timers: Disable the compat vdso for cores affected by ARM64_WORKAROUND_1418040 arch/arm64/include/asm/arch_timer.h | 3 +++ arch/arm64/include/asm/vdso/clocksource.h | 7 +-- arch/arm64/include/asm/vdso/compat_gettimeofday.h | 8 +++- drivers/clocksource/arm_arch_timer.c | 11 +++ 4 files changed, 26 insertions(+), 3 deletions(-) -- 2.27.0
Re: [PATCH] irqchip/gic-v3: Remove the unused register definition
On Tue, 30 Jun 2020 21:41:26 +0800, Zenghui Yu wrote:
> As per the GICv3 specification, GIC{D,R}_SEIR are not assigned and the
> locations (0x0068) are actually Reserved. GICR_MOV{LPI,ALL}R are two IMP
> DEF registers and might be defined by some specific micro-architecture,
> Linux doesn't use them.
>
> As they're not used anywhere in the kernel, just drop all of them.
Applied to irq/irqchip-5.9, thanks!
[1/1] irqchip/gic-v3: Remove unused register definition
commit: c15c35a83c01fb25a995fbc4f58fe499f0493813
Cheers,
M.
--
Without deviation from the norm, progress is not possible.
Re: [PATCH] irqchip/gic-v3: Remove the unused register definition
On 2020-06-30 14:41, Zenghui Yu wrote:
As per the GICv3 specification, GIC{D,R}_SEIR are not assigned and the
locations (0x0068) are actually Reserved. GICR_MOV{LPI,ALL}R are two
IMP
DEF registers and might be defined by some specific micro-architecture,
Linux doesn't use them.
As they're not used anywhere in the kernel, just drop all of them.
Signed-off-by: Zenghui Yu
To be clear, those were actually present in the spec back in the days
(in the 2013 time frame). It just shows how the architecture evolved...
I'll queue this for 5.9.
Thanks,
M.
---
Compile tested on top of mainline.
include/linux/irqchip/arm-gic-v3.h | 4
1 file changed, 4 deletions(-)
diff --git a/include/linux/irqchip/arm-gic-v3.h
b/include/linux/irqchip/arm-gic-v3.h
index 6c36b6cc3edf..f6d092fdb93d 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -19,7 +19,6 @@
#define GICD_CLRSPI_NSR0x0048
#define GICD_SETSPI_SR 0x0050
#define GICD_CLRSPI_SR 0x0058
-#define GICD_SEIR 0x0068
#define GICD_IGROUPR 0x0080
#define GICD_ISENABLER 0x0100
#define GICD_ICENABLER 0x0180
@@ -119,14 +118,11 @@
#define GICR_WAKER 0x0014
#define GICR_SETLPIR 0x0040
#define GICR_CLRLPIR 0x0048
-#define GICR_SEIR GICD_SEIR
#define GICR_PROPBASER 0x0070
#define GICR_PENDBASER 0x0078
#define GICR_INVLPIR 0x00A0
#define GICR_INVALLR 0x00B0
#define GICR_SYNCR 0x00C0
-#define GICR_MOVLPIR 0x0100
-#define GICR_MOVALLR 0x0110
#define GICR_IDREGSGICD_IDREGS
#define GICR_PIDR2 GICD_PIDR2
--
Jazz is not dead. It just smells funny...
Re: [PATCH v5 2/6] PCI: uniphier: Add misc interrupt handler to invoke PME and AER
On 2020-06-29 10:49, Kunihiko Hayashi wrote:
Hi Marc,
On 2020/06/27 18:48, Marc Zyngier wrote:
On Thu, 18 Jun 2020 09:38:09 +0100,
Kunihiko Hayashi wrote:
The misc interrupts consisting of PME, AER, and Link event, is
handled
by INTx handler, however, these interrupts should be also handled by
MSI handler.
This adds the function uniphier_pcie_misc_isr() that handles misc
interrupts, which is called from both INTx and MSI handlers.
This function detects PME and AER interrupts with the status
register,
and invoke PME and AER drivers related to MSI.
And this sets the mask for misc interrupts from INTx if MSI is
enabled
and sets the mask for misc interrupts from MSI if MSI is disabled.
Cc: Marc Zyngier
Cc: Jingoo Han
Cc: Gustavo Pimentel
Signed-off-by: Kunihiko Hayashi
---
drivers/pci/controller/dwc/pcie-uniphier.c | 57
--
1 file changed, 46 insertions(+), 11 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c
b/drivers/pci/controller/dwc/pcie-uniphier.c
index a5401a0..5ce2479 100644
--- a/drivers/pci/controller/dwc/pcie-uniphier.c
+++ b/drivers/pci/controller/dwc/pcie-uniphier.c
@@ -44,7 +44,9 @@
#define PCL_SYS_AUX_PWR_DET BIT(8)
#define PCL_RCV_INT 0x8108
+#define PCL_RCV_INT_ALL_INT_MASK GENMASK(28, 25)
#define PCL_RCV_INT_ALL_ENABLEGENMASK(20, 17)
+#define PCL_RCV_INT_ALL_MSI_MASK GENMASK(12, 9)
#define PCL_CFG_BW_MGT_STATUS BIT(4)
#define PCL_CFG_LINK_AUTO_BW_STATUS BIT(3)
#define PCL_CFG_AER_RC_ERR_MSI_STATUS BIT(2)
@@ -167,7 +169,15 @@ static void uniphier_pcie_stop_link(struct
dw_pcie *pci)
static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv
*priv)
{
- writel(PCL_RCV_INT_ALL_ENABLE, priv->base + PCL_RCV_INT);
+ u32 val;
+
+ val = PCL_RCV_INT_ALL_ENABLE;
+ if (pci_msi_enabled())
+ val |= PCL_RCV_INT_ALL_INT_MASK;
+ else
+ val |= PCL_RCV_INT_ALL_MSI_MASK;
Does this affect endpoints? Or just the RC itself?
These interrupts are asserted by RC itself, so this part affects only
RC.
+
+ writel(val, priv->base + PCL_RCV_INT);
writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
}
@@ -231,32 +241,56 @@ static const struct irq_domain_ops
uniphier_intx_domain_ops = {
.map = uniphier_pcie_intx_map,
};
-static void uniphier_pcie_irq_handler(struct irq_desc *desc)
+static void uniphier_pcie_misc_isr(struct pcie_port *pp, bool
is_msi)
{
- struct pcie_port *pp = irq_desc_get_handler_data(desc);
struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
- struct irq_chip *chip = irq_desc_get_chip(desc);
- unsigned long reg;
- u32 val, bit, virq;
+ u32 val, virq;
- /* INT for debug */
val = readl(priv->base + PCL_RCV_INT);
if (val & PCL_CFG_BW_MGT_STATUS)
dev_dbg(pci->dev, "Link Bandwidth Management Event\n");
+
if (val & PCL_CFG_LINK_AUTO_BW_STATUS)
dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n");
- if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
- dev_dbg(pci->dev, "Root Error\n");
- if (val & PCL_CFG_PME_MSI_STATUS)
- dev_dbg(pci->dev, "PME Interrupt\n");
+
+ if (is_msi) {
+ if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
+ dev_dbg(pci->dev, "Root Error Status\n");
+
+ if (val & PCL_CFG_PME_MSI_STATUS)
+ dev_dbg(pci->dev, "PME Interrupt\n");
+
+ if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS |
+ PCL_CFG_PME_MSI_STATUS)) {
+ virq = irq_linear_revmap(pp->irq_domain, 0);
+ generic_handle_irq(virq);
+ }
+ }
Please have two handlers: one for interrupts that are from the RC,
another for interrupts coming from the endpoints.
I assume that this handler treats interrupts from the RC only and
this is set on the member ".msi_host_isr" added in the patch 1/6.
I think that the handler for interrupts coming from endpoints should be
treated as a normal case (after calling .msi_host_isr in
dw_handle_msi_irq()).
It looks pretty odd that you end-up dealing with both from the
same "parent" interrupt. I guess this is in keeping with the
rest of the DW PCIe hacks... :-/
It is for Lorenzo to make up his mind about this anyway.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Re: [PATCH v2 06/17] irqchip/gic-v3: Configure SGIs as standard interrupts
On 2020-06-25 19:25, Valentin Schneider wrote:
On 24/06/20 20:58, Marc Zyngier wrote:
Change the way we deal with GICv3 SGIs by turning them into proper
IRQs, and calling into the arch code to register the interrupt range
instead of a callback.
Signed-off-by: Marc Zyngier
---
drivers/irqchip/irq-gic-v3.c | 81
+++-
1 file changed, 43 insertions(+), 38 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3.c
b/drivers/irqchip/irq-gic-v3.c
index 19b294ed48ba..d275e9b9533d 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -36,6 +36,8 @@
#define FLAGS_WORKAROUND_GICR_WAKER_MSM8996(1ULL << 0)
#define FLAGS_WORKAROUND_CAVIUM_ERRATUM_38539 (1ULL << 1)
+#define GIC_IRQ_TYPE_PARTITION (GIC_IRQ_TYPE_LPI + 1)
+
Nit: this piqued my interest but ended up being just a define shuffle;
As a
member of the git speleologists' guild, I'd be overjoyed with having a
small notion of that in the changelog.
Fair enough.
struct redist_region {
void __iomem *redist_base;
phys_addr_t phys_base;
@@ -657,38 +659,14 @@ static asmlinkage void __exception_irq_entry
gic_handle_irq(struct pt_regs *regs
if ((irqnr >= 1020 && irqnr <= 1023))
return;
- /* Treat anything but SGIs in a uniform way */
- if (likely(irqnr > 15)) {
- int err;
-
- if (static_branch_likely(&supports_deactivate_key))
- gic_write_eoir(irqnr);
- else
- isb();
-
- err = handle_domain_irq(gic_data.domain, irqnr, regs);
- if (err) {
- WARN_ONCE(true, "Unexpected interrupt received!\n");
- gic_deactivate_unhandled(irqnr);
- }
- return;
- }
- if (irqnr < 16) {
+ if (static_branch_likely(&supports_deactivate_key))
gic_write_eoir(irqnr);
- if (static_branch_likely(&supports_deactivate_key))
- gic_write_dir(irqnr);
-#ifdef CONFIG_SMP
- /*
-* Unlike GICv2, we don't need an smp_rmb() here.
-* The control dependency from gic_read_iar to
-* the ISB in gic_write_eoir is enough to ensure
-* that any shared data read by handle_IPI will
-* be read after the ACK.
-*/
Isn't that still relevant?
It is. It is just that there is no really good place to put it.
I may end-up just leaving it where it is.
Also, while staring at this it dawned on me that IPI's don't need the
eoimode=0 isb(): due to how the IPI flow-handler is structured, we'll
get a
gic_eoi_irq() just before calling into the irqaction. Dunno how much we
care about it.
That's interesting. This ISB is a leftover from the loop we had before
the pseudo-NMI code, where we had to make sure the write to EOIR was
ordered with the read from IAR.
Given that we have an exception return right after the interrupt
handling, I *think* we could get rid of it (but that would need
mode checking on broken systems such as TX1...). I don't think
this is specific to IPIs though.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
[tip: irq/urgent] irqchip/gic: Atomically update affinity
The following commit has been merged into the irq/urgent branch of tip:
Commit-ID: 005c34ae4b44f085120d7f371121ec7ded677761
Gitweb:
https://git.kernel.org/tip/005c34ae4b44f085120d7f371121ec7ded677761
Author:Marc Zyngier
AuthorDate:Sun, 21 Jun 2020 14:43:15 +01:00
Committer: Marc Zyngier
CommitterDate: Sun, 21 Jun 2020 15:24:46 +01:00
irqchip/gic: Atomically update affinity
The GIC driver uses a RMW sequence to update the affinity, and
relies on the gic_lock_irqsave/gic_unlock_irqrestore sequences
to update it atomically.
But these sequences only expand into anything meaningful if
the BL_SWITCHER option is selected, which almost never happens.
It also turns out that using a RMW and locks is just as silly,
as the GIC distributor supports byte accesses for the GICD_TARGETRn
registers, which when used make the update atomic by definition.
Drop the terminally broken code and replace it by a byte write.
Fixes: 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only feature")
Cc: sta...@vger.kernel.org
Signed-off-by: Marc Zyngier
---
drivers/irqchip/irq-gic.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 00de05a..c17fabd 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -329,10 +329,8 @@ static int gic_irq_set_vcpu_affinity(struct irq_data *d,
void *vcpu)
static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
bool force)
{
- void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) &
~3);
- unsigned int cpu, shift = (gic_irq(d) % 4) * 8;
- u32 val, mask, bit;
- unsigned long flags;
+ void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + gic_irq(d);
+ unsigned int cpu;
if (!force)
cpu = cpumask_any_and(mask_val, cpu_online_mask);
@@ -342,13 +340,7 @@ static int gic_set_affinity(struct irq_data *d, const
struct cpumask *mask_val,
if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids)
return -EINVAL;
- gic_lock_irqsave(flags);
- mask = 0xff << shift;
- bit = gic_cpu_map[cpu] << shift;
- val = readl_relaxed(reg) & ~mask;
- writel_relaxed(val | bit, reg);
- gic_unlock_irqrestore(flags);
-
+ writeb_relaxed(gic_cpu_map[cpu], reg);
irq_data_update_effective_affinity(d, cpumask_of(cpu));
return IRQ_SET_MASK_OK_DONE;
[PATCH v3] PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent
Booting a recent kernel on a rk3399-based system (nanopc-t4),
equipped with a recent u-boot and ATF results in an Oops due
to a NULL pointer dereference.
This turns out to be due to the rk3399-dmc driver looking for
an *undocumented* property (rockchip,pmu), and happily using
a NULL pointer when the property isn't there.
Instead, make most of what was brought in with 9173c5ceb035
("PM / devfreq: rk3399_dmc: Pass ODT and auto power down parameters
to TF-A.") conditioned on finding this property in the device-tree,
preventing the driver from exploding.
Cc: sta...@vger.kernel.org
Fixes: 9173c5ceb035 ("PM / devfreq: rk3399_dmc: Pass ODT and auto power down
parameters to TF-A.")
Signed-off-by: Marc Zyngier
---
* From v2:
- Trimmed down commit message
- Cc stable
drivers/devfreq/rk3399_dmc.c | 42
1 file changed, 23 insertions(+), 19 deletions(-)
diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
index 24f04f78285b..027769e39f9b 100644
--- a/drivers/devfreq/rk3399_dmc.c
+++ b/drivers/devfreq/rk3399_dmc.c
@@ -95,18 +95,20 @@ static int rk3399_dmcfreq_target(struct device *dev,
unsigned long *freq,
mutex_lock(&dmcfreq->lock);
- if (target_rate >= dmcfreq->odt_dis_freq)
- odt_enable = true;
-
- /*
-* This makes a SMC call to the TF-A to set the DDR PD (power-down)
-* timings and to enable or disable the ODT (on-die termination)
-* resistors.
-*/
- arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, dmcfreq->odt_pd_arg0,
- dmcfreq->odt_pd_arg1,
- ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD,
- odt_enable, 0, 0, 0, &res);
+ if (dmcfreq->regmap_pmu) {
+ if (target_rate >= dmcfreq->odt_dis_freq)
+ odt_enable = true;
+
+ /*
+* This makes a SMC call to the TF-A to set the DDR PD
+* (power-down) timings and to enable or disable the
+* ODT (on-die termination) resistors.
+*/
+ arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, dmcfreq->odt_pd_arg0,
+ dmcfreq->odt_pd_arg1,
+ ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD,
+ odt_enable, 0, 0, 0, &res);
+ }
/*
* If frequency scaling from low to high, adjust voltage first.
@@ -371,13 +373,14 @@ static int rk3399_dmcfreq_probe(struct platform_device
*pdev)
}
node = of_parse_phandle(np, "rockchip,pmu", 0);
- if (node) {
- data->regmap_pmu = syscon_node_to_regmap(node);
- of_node_put(node);
- if (IS_ERR(data->regmap_pmu)) {
- ret = PTR_ERR(data->regmap_pmu);
- goto err_edev;
- }
+ if (!node)
+ goto no_pmu;
+
+ data->regmap_pmu = syscon_node_to_regmap(node);
+ of_node_put(node);
+ if (IS_ERR(data->regmap_pmu)) {
+ ret = PTR_ERR(data->regmap_pmu);
+ goto err_edev;
}
regmap_read(data->regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
@@ -399,6 +402,7 @@ static int rk3399_dmcfreq_probe(struct platform_device
*pdev)
goto err_edev;
};
+no_pmu:
arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, 0, 0,
ROCKCHIP_SIP_CONFIG_DRAM_INIT,
0, 0, 0, 0, &res);
--
2.27.0
Re: [PATCH v2 04/17] ARM: Allow IPIs to be handled as normal interrupts
On 2020-06-25 19:25, Valentin Schneider wrote:
On 24/06/20 20:57, Marc Zyngier wrote:
@@ -696,9 +696,76 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
if ((unsigned)ipinr < NR_IPI)
trace_ipi_exit_rcuidle(ipi_types[ipinr]);
+}
+
+/* Legacy version, should go away once all irqchips have been
converted */
+void handle_IPI(int ipinr, struct pt_regs *regs)
+{
+ struct pt_regs *old_regs = set_irq_regs(regs);
+
+ irq_enter();
+ do_handle_IPI(ipinr);
+ irq_exit();
+
set_irq_regs(old_regs);
}
+static irqreturn_t ipi_handler(int irq, void *data)
+{
+ do_handle_IPI(irq - ipi_irq_base);
+ return IRQ_HANDLED;
+}
+
+static void ipi_send(const struct cpumask *target, unsigned int ipi)
+{
+ __ipi_send_mask(ipi_desc[ipi], target);
+}
+
+static void ipi_setup(int cpu)
+{
+ if (ipi_irq_base) {
+ int i;
+
+ for (i = 0; i < nr_ipi; i++)
+ enable_percpu_irq(ipi_irq_base + i, 0);
+ }
+}
+
+static void ipi_teardown(int cpu)
+{
+ if (ipi_irq_base) {
+ int i;
+
+ for (i = 0; i < nr_ipi; i++)
+ disable_percpu_irq(ipi_irq_base + i);
+ }
+}
+
+void __init set_smp_ipi_range(int ipi_base, int n)
+{
+ int i;
+
+ WARN_ON(n < MAX_IPI);
+ nr_ipi = min(n, MAX_IPI);
I got confused by that backtrace thing and NR_IPI vs MAX_IPI.
I think I got it now : we don't want to call trace_ipi_raise() for
IPI_CPU_BACKTRACE *but* we still need to alloc the desc and route it
through the generic IPI layers.
Indeed, and I didn't want to have a bizarre "+ 1" hanging about.
The only difference I can tell is that now we will get some trace
events
for it via the handler entry/exit tracepoints - that shouldn't cause
any
issue.
I hope so. I don't see how you can avoid all tracepoints anyway (if that
was the intention).
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Re: [PATCH v2] PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent
Hi Chanwoo,
On Mon, 29 Jun 2020 03:43:37 +0100,
Chanwoo Choi wrote:
>
> Hi Marc,
>
> On 6/23/20 12:28 AM, Marc Zyngier wrote:
[...]
> It looks good to me. But, I think that it is not necessary
> fully kernel panic log about NULL pointer. It is enoughspsp
> just mentioning the NULL pointer issue without full kernel panic log.
I personally find the backtrace useful as it allows people with the
same issue to trawl the kernel log and find whether it has already be
fixed upstream. But it's only me, and I'm not attached to it.
> So, how about editing the patch description as following or others simply?
> and we need to add 'sta...@vger.kernel.org' to Cc list for applying it
> to stable branch.
Looks good to me.
>
>
> PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent
>
> Booting a recent kernel on a rk3399-based system (nanopc-t4),
> equipped with a recent u-boot and ATF results in the kernel panic
> about NULL pointer issue.
nit: "results in a kernel panic on dereferencing a NULL pointer".
>
> This turns out to be due to the rk3399-dmc driver looking for
> an *undocumented* property (rockchip,pmu), and happily using
> a NULL pointer when the property isn't there.
>
> Instead, make most of what was brought in with 9173c5ceb035
> ("PM / devfreq: rk3399_dmc: Pass ODT and auto power down parameters
> to TF-A.") conditioned on finding this property in the device-tree,
> preventing the driver from exploding.
>
> Fixes: 9173c5ceb035 ("PM / devfreq: rk3399_dmc: Pass ODT and auto power
> down parameters to TF-A.")
> Signed-off-by: Marc Zyngier
> Signed-off-by: Chanwoo Choi
Note that the biggest issue is still there: the driver is using an
undocumented property, and this patch is just papering over it.
Since I expect this property to be useful for something, it would be
good for whoever knows what it does to document it.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
Re: [BUG] irqchip/gic-v4.1: sleeping function called from invalid context
Hi Zenghui, On 2020-06-29 10:39, Zenghui Yu wrote: Hi All, Booting the latest kernel with DEBUG_ATOMIC_SLEEP=y on a GICv4.1 enabled box, I get the following kernel splat: [0.053766] BUG: sleeping function called from invalid context at mm/slab.h:567 [0.053767] in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 0, name: swapper/1 [0.053769] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.8.0-rc3+ #23 [0.053770] Call trace: [0.053774] dump_backtrace+0x0/0x218 [0.053775] show_stack+0x2c/0x38 [0.053777] dump_stack+0xc4/0x10c [0.053779] ___might_sleep+0xfc/0x140 [0.053780] __might_sleep+0x58/0x90 [0.053782] slab_pre_alloc_hook+0x7c/0x90 [0.053783] kmem_cache_alloc_trace+0x60/0x2f0 [0.053785] its_cpu_init+0x6f4/0xe40 [0.053786] gic_starting_cpu+0x24/0x38 [0.053788] cpuhp_invoke_callback+0xa0/0x710 [0.053789] notify_cpu_starting+0xcc/0xd8 [0.053790] secondary_start_kernel+0x148/0x200 # ./scripts/faddr2line vmlinux its_cpu_init+0x6f4/0xe40 its_cpu_init+0x6f4/0xe40: allocate_vpe_l1_table at drivers/irqchip/irq-gic-v3-its.c:2818 (inlined by) its_cpu_init_lpis at drivers/irqchip/irq-gic-v3-its.c:3138 (inlined by) its_cpu_init at drivers/irqchip/irq-gic-v3-its.c:5166 Let me guess: a system with more than a single CommonLPIAff group? I've tried to replace GFP_KERNEL flag with GFP_ATOMIC to allocate memory in this atomic context, and the splat disappears. But after a quick look at [*], it seems not a good idea to allocate memory within the CPU hotplug notifier. I really don't know much about it, please have a look. [*] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=11e37d357f6ba7a9af850a872396082cc0a0001f The allocation of the cpumask is pretty benign, and could either be allocated upfront for all RDs (and freed on detecting that we share the same CommonLPIAff group) or made atomic. The much bigger issue is the alloc_pages call just after. Allocating this upfront probably is the wrong thing to do, as you are likely to allocate way too much memory, even if you free it quickly afterwards. At this stage, I'd rather we turn this into an atomic allocation. A notifier is just another atomic context, and if this fails at such an early stage, then the CPU is unlikely to continue booting... Would you like to write a patch for this? Given that you have tested something, it probably already exists. Or do you want me to do it? Thanks, M. -- Jazz is not dead. It just smells funny...
Re: [PATCH v2] PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent
On 2020-06-29 12:29, Chanwoo Choi wrote: Hi Enric and Mark, On 6/29/20 8:05 PM, Enric Balletbo i Serra wrote: Hi Chanwoo and Marc, On 29/6/20 13:09, Chanwoo Choi wrote: Hi Enric, Could you check this issue? Your patch[1] causes this issue. As Marc mentioned, although rk3399-dmc.c handled 'rockchip,pmu' as the mandatory property, your patch[1] didn't add the 'rockchip,pmu' property to the documentation. I think the problem is that the DT binding patch, for some reason, was missed and didn't land. The patch seems to have all the required reviews and acks. https://patchwork.kernel.org/patch/10901593/ Sorry because I didn't notice this issue when 9173c5ceb035 landed. And thanks for fixing the issue. If the 'rockchip,pmu' propery is mandatory, instead of Mark's patch, we better to require the merge of patch[1] to DT maintainer. It is way too late. Firmware exists (mainline u-boot, for one) that do not expose the new property, and you can't demand that people upgrade. This is an ABI bug, and we now have to live with it. So, yes to fixing the DT, and no to *only* fixing the DT. Thanks, M. -- Jazz is not dead. It just smells funny...
Re: [PATCH v3 00/14 RESEND] irqchip: Fix potential resource leaks
On 2020-06-28 13:48, Aleksandar Markovic wrote: нед, 28. јун 2020. у 14:06 Marc Zyngier је написао/ла: On 2020-06-28 12:25, Aleksandar Markovic wrote: > сре, 24. јун 2020. у 10:40 Marc Zyngier је написао/ла: >> >> On 2020-06-24 08:44, Tiezhu Yang wrote: >> > [git send-email failed due to too many commands, >> > so only cc the major related email and resend it, >> > sorry for that] >> >> This is becoming majorly annoying. > > I don't think this is the right vocabulary to welcome newcomers, > however "terrible" thinks they did. > > Rather, patience, calmness and clear and detailed explanations would > work much better - and certainly be much more helpful and useful to > the community in the long run. Pray tell how you would have handled this. I expressed *my* personal frustration at a SNR hovering below the 25% mark. I have only indicated that the current course of action was becoming a problem. And instead of taking the moral high ground, maybe you could actually share your wisdom with Teizhu and help him becoming a better contributor? He will improve. This initial clumsiness is normal. It could have been avoided, true - for example, if he had had someone more experienced at hand, preferably a co-worker. He obviously acts alone. It will be better. I thank you for imparting your insight on us. You clearly have helped things moving forward, and I am sure Teizhu now knows a lot more about what he should have done. M. -- Jazz is not dead. It just smells funny...
Re: [PATCH v3 00/14 RESEND] irqchip: Fix potential resource leaks
On 2020-06-28 12:25, Aleksandar Markovic wrote: сре, 24. јун 2020. у 10:40 Marc Zyngier је написао/ла: On 2020-06-24 08:44, Tiezhu Yang wrote: > [git send-email failed due to too many commands, > so only cc the major related email and resend it, > sorry for that] This is becoming majorly annoying. I don't think this is the right vocabulary to welcome newcomers, however "terrible" thinks they did. Rather, patience, calmness and clear and detailed explanations would work much better - and certainly be much more helpful and useful to the community in the long run. Pray tell how you would have handled this. I expressed *my* personal frustration at a SNR hovering below the 25% mark. I have only indicated that the current course of action was becoming a problem. And instead of taking the moral high ground, maybe you could actually share your wisdom with Teizhu and help him becoming a better contributor? Thanks, M. -- Jazz is not dead. It just smells funny...
Re: [PATCH v2 15/17] arm64: Remove custom IRQ stat accounting
On 2020-06-27 00:15, Valentin Schneider wrote:
On 26/06/20 12:58, Marc Zyngier wrote:
On 2020-06-25 19:26, Valentin Schneider wrote:
On 24/06/20 20:58, Marc Zyngier wrote:
@@ -801,26 +802,15 @@ void show_ipi_list(struct seq_file *p, int
prec)
unsigned int cpu, i;
for (i = 0; i < NR_IPI; i++) {
+ unsigned int irq = irq_desc_get_irq(ipi_desc[i]);
seq_printf(p, "%*s%u:%s", prec - 1, "IPI", i,
prec >= 4 ? " " : "");
for_each_online_cpu(cpu)
- seq_printf(p, "%10u ",
- __get_irq_stat(cpu, ipi_irqs[i]));
+ seq_printf(p, "%10u ", kstat_irqs_cpu(irq, cpu));
seq_printf(p, " %s\n", ipi_types[i]);
How attached are we to that custom IPI printout? AIUI we *could* give
them
a "prettier" name in request_percpu_irq() and let the standard procfs
printout take the wheel.
I wish. Unfortunately, /proc/interrupts is likely to be considered
ABI,
and I'm really worried to change this (see what happened last time we
tried to update /proc/cpuinfo to be less braindead...).
Hmph, it's borderline here I think: we're not introducing a new field
or
format in the file, so any tool that can parse /proc/interrupts can
parse
the IPIs if they are formatted like the other "regular" interrupts. But
then said tool could die in flames when not seeing the special IPI
fields
because sturdiness is overrated :(
Which is exactly what I'm worried about. People do stupid things,
and stupidity becomes ABI. I hate luserspace.
M.
--
Jazz is not dead. It just smells funny...
Re: [PATCH v5 0/3] drivers/acpi: Remove function callback casts
On Sat, 30 May 2020 16:34:27 +0200, Oscar Carter wrote: > In an effort to enable -Wcast-function-type in the top-level Makefile to > support Control Flow Integrity builds, there are the need to remove all > the function callback casts in the acpi driver. > > The first patch creates a macro called ACPI_DECLARE_SUBTABLE_PROBE_ENTRY > to initialize the acpi_probe_entry struct using the probe_subtbl field > instead of the probe_table field to avoid function cast mismatches. > > [...] Applied to irq/irqchip-5.9: [1/3] drivers/acpi: Add new macro ACPI_DECLARE_SUBTABLE_PROBE_ENTRY commit: 89778093d38d547cd80f6097659d1cf1c2dd4d9d [2/3] drivers/irqchip: Use new macro ACPI_DECLARE_SUBTABLE_PROBE_ENTRY commit: aba3c7ed3fcf74524b7072615028827d5e5750d7 [3/3] drivers/acpi: Remove function cast commit: 8ebf642f3d809b59f57d0d408189a2218294e269 Thanks, M. -- Without deviation from the norm, progress is not possible.
