From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752089AbdJ3IAP (ORCPT ); Mon, 30 Oct 2017 04:00:15 -0400 Received: from foss.arm.com ([217.140.101.70]:49022 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752046AbdJ3IAO (ORCPT ); Mon, 30 Oct 2017 04:00:14 -0400 From: Marc Zyngier To: Paul Burton Cc: Jason Cooper , Thomas Gleixner , , Subject: Re: [PATCH 2/8] irqchip: mips-gic: Use irq_cpu_online to (un)mask all-VP(E) IRQs In-Reply-To: <20171025233730.22225-3-paul.burton@mips.com> (Paul Burton's message of "Wed, 25 Oct 2017 16:37:24 -0700") Organization: ARM Ltd References: <20171025233730.22225-1-paul.burton@mips.com> <20171025233730.22225-3-paul.burton@mips.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) Date: Mon, 30 Oct 2017 08:00:08 +0000 Message-ID: <86mv495alz.fsf@arm.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Oct 25 2017 at 5:37:24 pm BST, Paul Burton wrote: > The gic_all_vpes_local_irq_controller chip currently attempts to operate > on all CPUs/VPs in the system when masking or unmasking an interrupt. > This has a few drawbacks: > > - In multi-cluster systems we may not always have access to all CPUs in > the system. When all CPUs in a cluster are powered down that > cluster's GIC may also power down, in which case we cannot configure > its state. > > - Relatedly, if we power down a cluster after having configured > interrupts for CPUs within it then the cluster's GIC may lose state & > we need to reconfigure it. The current approach doesn't take this > into account. > > - It's wasteful if we run Linux on fewer VPs than are present in the > system. For example if we run a uniprocessor kernel on CPU0 of a > system with 16 CPUs then there's no point in us configuring CPUs > 1-15. > > - The implementation is also lacking in that it expects the range > 0..gic_vpes-1 to represent valid Linux CPU numbers which may not > always be the case - for example if we run on a system with more VPs > than the kernel is configured to support. > > Fix all of these issues by only configuring the affected interrupts for > CPUs which are online at the time, and recording the configuration in a > new struct gic_all_vpes_chip_data for later use by CPUs being brought > online. We register a CPU hotplug state (reusing > CPUHP_AP_IRQ_GIC_STARTING which the ARM GIC driver uses, and which seems > suitably generic for reuse with the MIPS GIC) and execute > irq_cpu_online() in order to configure the interrupts on the newly > onlined CPU. > > Signed-off-by: Paul Burton > Cc: Jason Cooper > Cc: Marc Zyngier > Cc: Thomas Gleixner > Cc: linux-mips@linux-mips.org > --- > > drivers/irqchip/irq-mips-gic.c | 72 ++++++++++++++++++++++++++++++++---------- > 1 file changed, 56 insertions(+), 16 deletions(-) > > diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c > index 6fdcc1552fab..dd9da773db90 100644 > --- a/drivers/irqchip/irq-mips-gic.c > +++ b/drivers/irqchip/irq-mips-gic.c [...] > @@ -622,6 +653,13 @@ static const struct irq_domain_ops gic_ipi_domain_ops = { > .match = gic_ipi_domain_match, > }; > > +static int gic_cpu_startup(unsigned int cpu) > +{ > + /* Invoke irq_cpu_online callbacks to enable desired interrupts */ > + irq_cpu_online(); > + > + return 0; > +} > > static int __init gic_of_init(struct device_node *node, > struct device_node *parent) > @@ -768,6 +806,8 @@ static int __init gic_of_init(struct device_node *node, > } > } > > - return 0; > + return cpuhp_setup_state(CPUHP_AP_IRQ_GIC_STARTING, > + "irqchip/mips/gic:starting", > + gic_cpu_startup, NULL); I'm wondering about this. CPUHP_AP_IRQ_GIC_STARTING is a symbol that is used on ARM platforms. You're very welcome to use it (as long as nobody builds a system with both an ARM GIC and a MIPS GIC...), but I'm a bit worried that we could end-up breaking things if one of us decides to reorder it in enum cpuhp_state. The safest option would be for you to add your own state value, which would allow the two architecture to evolve independently. Thoughts? M. -- Jazz is not dead. It just smells funny. From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:53346 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S23990391AbdJ3IAVMdMyL (ORCPT ); Mon, 30 Oct 2017 09:00:21 +0100 From: Marc Zyngier Subject: Re: [PATCH 2/8] irqchip: mips-gic: Use irq_cpu_online to (un)mask all-VP(E) IRQs In-Reply-To: <20171025233730.22225-3-paul.burton@mips.com> (Paul Burton's message of "Wed, 25 Oct 2017 16:37:24 -0700") References: <20171025233730.22225-1-paul.burton@mips.com> <20171025233730.22225-3-paul.burton@mips.com> Date: Mon, 30 Oct 2017 08:00:08 +0000 Message-ID: <86mv495alz.fsf@arm.com> MIME-Version: 1.0 Content-Type: text/plain Return-Path: Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-subscribe: List-owner: List-post: List-archive: To: Paul Burton Cc: Jason Cooper , Thomas Gleixner , linux-mips@linux-mips.org, linux-kernel@vger.kernel.org Message-ID: <20171030080008.q8hEAWcfThTn-ExdtX174trxw522R5tJbn_TdWCACKY@z> On Wed, Oct 25 2017 at 5:37:24 pm BST, Paul Burton wrote: > The gic_all_vpes_local_irq_controller chip currently attempts to operate > on all CPUs/VPs in the system when masking or unmasking an interrupt. > This has a few drawbacks: > > - In multi-cluster systems we may not always have access to all CPUs in > the system. When all CPUs in a cluster are powered down that > cluster's GIC may also power down, in which case we cannot configure > its state. > > - Relatedly, if we power down a cluster after having configured > interrupts for CPUs within it then the cluster's GIC may lose state & > we need to reconfigure it. The current approach doesn't take this > into account. > > - It's wasteful if we run Linux on fewer VPs than are present in the > system. For example if we run a uniprocessor kernel on CPU0 of a > system with 16 CPUs then there's no point in us configuring CPUs > 1-15. > > - The implementation is also lacking in that it expects the range > 0..gic_vpes-1 to represent valid Linux CPU numbers which may not > always be the case - for example if we run on a system with more VPs > than the kernel is configured to support. > > Fix all of these issues by only configuring the affected interrupts for > CPUs which are online at the time, and recording the configuration in a > new struct gic_all_vpes_chip_data for later use by CPUs being brought > online. We register a CPU hotplug state (reusing > CPUHP_AP_IRQ_GIC_STARTING which the ARM GIC driver uses, and which seems > suitably generic for reuse with the MIPS GIC) and execute > irq_cpu_online() in order to configure the interrupts on the newly > onlined CPU. > > Signed-off-by: Paul Burton > Cc: Jason Cooper > Cc: Marc Zyngier > Cc: Thomas Gleixner > Cc: linux-mips@linux-mips.org > --- > > drivers/irqchip/irq-mips-gic.c | 72 ++++++++++++++++++++++++++++++++---------- > 1 file changed, 56 insertions(+), 16 deletions(-) > > diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c > index 6fdcc1552fab..dd9da773db90 100644 > --- a/drivers/irqchip/irq-mips-gic.c > +++ b/drivers/irqchip/irq-mips-gic.c [...] > @@ -622,6 +653,13 @@ static const struct irq_domain_ops gic_ipi_domain_ops = { > .match = gic_ipi_domain_match, > }; > > +static int gic_cpu_startup(unsigned int cpu) > +{ > + /* Invoke irq_cpu_online callbacks to enable desired interrupts */ > + irq_cpu_online(); > + > + return 0; > +} > > static int __init gic_of_init(struct device_node *node, > struct device_node *parent) > @@ -768,6 +806,8 @@ static int __init gic_of_init(struct device_node *node, > } > } > > - return 0; > + return cpuhp_setup_state(CPUHP_AP_IRQ_GIC_STARTING, > + "irqchip/mips/gic:starting", > + gic_cpu_startup, NULL); I'm wondering about this. CPUHP_AP_IRQ_GIC_STARTING is a symbol that is used on ARM platforms. You're very welcome to use it (as long as nobody builds a system with both an ARM GIC and a MIPS GIC...), but I'm a bit worried that we could end-up breaking things if one of us decides to reorder it in enum cpuhp_state. The safest option would be for you to add your own state value, which would allow the two architecture to evolve independently. Thoughts? M. -- Jazz is not dead. It just smells funny.