From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752698AbaHMN5Z (ORCPT <rfc822;w@1wt.eu>);
	Wed, 13 Aug 2014 09:57:25 -0400
Received: from smtp.codeaurora.org ([198.145.11.231]:39774 "EHLO
	smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751145AbaHMN5Y (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 13 Aug 2014 09:57:24 -0400
From: Stephen Boyd <sboyd@codeaurora.org>
To: Thomas Gleixner <tglx@linutronix.de>, Jason Cooper <jason@lakedaemon.net>
Cc: linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, Nicolas Pitre <nico@linaro.org>
Subject: [PATCH v4] irqchip: gic: Allow gic_arch_extn hooks to call into scheduler
Date: Wed, 13 Aug 2014 06:57:18 -0700
Message-Id: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org>
X-Mailer: git-send-email 2.1.0.rc2.4.g1a517f0
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Commit 1a6b69b6548c (ARM: gic: add CPU migration support,
2012-04-12) introduced an acquisition of the irq_controller_lock
in gic_raise_softirq() which can lead to a spinlock recursion if
the gic_arch_extn hooks call into the scheduler (via complete()
or wake_up(), etc.). This happens because gic_arch_extn hooks are
normally called with the irq_controller_lock held and calling
into the scheduler may cause us to call smp_send_reschedule()
which will grab the irq_controller_lock again. Here's an example
from a vendor kernel (note that the gic_arch_extn hook code here
isn't actually in mainline):

BUG: spinlock recursion on CPU#0, swapper/0/1
 lock: irq_controller_lock+0x0/0x18, .magic: dead4ead, .owner: sw
er_cpu: 0
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.10-00430-g3d433c4e

Call trace:
[<ffffffc000087e1c>] dump_backtrace+0x0/0x140
[<ffffffc000087f6c>] show_stack+0x10/0x1c
[<ffffffc00064732c>] dump_stack+0x74/0xc4
[<ffffffc0006446c0>] spin_dump+0x78/0x88
[<ffffffc0006446f4>] spin_bug+0x24/0x34
[<ffffffc0000d47d0>] do_raw_spin_lock+0x58/0x148
[<ffffffc00064d398>] _raw_spin_lock_irqsave+0x24/0x38
[<ffffffc0002c9d7c>] gic_raise_softirq+0x2c/0xbc
[<ffffffc00008daa4>] smp_send_reschedule+0x34/0x40
[<ffffffc0000c1e94>] try_to_wake_up+0x224/0x288
[<ffffffc0000c1f4c>] default_wake_function+0xc/0x18
[<ffffffc0000ceef0>] __wake_up_common+0x50/0x8c
[<ffffffc0000cef3c>] __wake_up_locked+0x10/0x1c
[<ffffffc0000cf734>] complete+0x3c/0x5c
[<ffffffc0002f0e78>] msm_mpm_enable_irq_exclusive+0x1b8/0x1c8
[<ffffffc0002f0f58>] __msm_mpm_enable_irq+0x4c/0x7c
[<ffffffc0002f0f94>] msm_mpm_enable_irq+0xc/0x18
[<ffffffc0002c9bb0>] gic_unmask_irq+0x40/0x7c
[<ffffffc0000de5f4>] irq_enable+0x2c/0x48
[<ffffffc0000de65c>] irq_startup+0x4c/0x74
[<ffffffc0000dd2fc>] __setup_irq+0x264/0x3f0
[<ffffffc0000dd5e0>] request_threaded_irq+0xcc/0x11c
[<ffffffc0000df254>] devm_request_threaded_irq+0x68/0xb4
[<ffffffc000471520>] msm_iommu_ctx_probe+0x124/0x2d4
[<ffffffc000337374>] platform_drv_probe+0x20/0x54
[<ffffffc00033598c>] driver_probe_device+0x158/0x340
[<ffffffc000335c20>] __driver_attach+0x60/0x90
[<ffffffc000333c9c>] bus_for_each_dev+0x6c/0x8c
[<ffffffc000335304>] driver_attach+0x1c/0x28
[<ffffffc000334f14>] bus_add_driver+0x120/0x204
[<ffffffc0003362e4>] driver_register+0xbc/0x10c
[<ffffffc000337348>] __platform_driver_register+0x5c/0x68
[<ffffffc00094c478>] msm_iommu_driver_init+0x54/0x7c
[<ffffffc0000813ec>] do_one_initcall+0xa4/0x130
[<ffffffc00091d928>] kernel_init_freeable+0x138/0x1dc
[<ffffffc000642578>] kernel_init+0xc/0xd4

We really just want to synchronize the sending of an SGI with the
update of the gic_cpu_map[], so introduce a new SGI lock that we
can use to synchronize the two code paths. Three main events are
happening that we have to consider:

	1. We're updating the gic_cpu_mask to point to an
	incoming CPU

	2. We're (potentially) sending an SGI to the outgoing CPU

	3. We're redirecting any pending SGIs for the outgoing
	CPU to the incoming CPU.

Events 1 and 3 are already ordered within the same CPU by means
of program order and use of I/O accessors. Events 1 and 2 don't
need to be ordered, but events 2 and 3 do because any SGIs for
the outgoing CPU need to be pending before we can redirect them.
Synchronize by acquiring a new lock around event 2 and before
event 3. Use smp_mb__after_unlock_lock() before event 3 to ensure
that event 1 is seen before event 3 on other CPUs that may be
executing event 2. We put this all behind the b.L switcher config
option so that if we're not using this feature we don't have to
acquire any locks at all in the IPI path.

Cc: Nicolas Pitre <nico@linaro.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/irqchip/irq-gic.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 7c131cf7cc13..b9e669cb1c1e 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -81,6 +81,16 @@ static DEFINE_RAW_SPINLOCK(irq_controller_lock);
 #define NR_GIC_CPU_IF 8
 static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly;
 
+#ifdef CONFIG_BL_SWITCHER
+/* Synchronize switching CPU interface and sending SGIs */
+static DEFINE_RAW_SPINLOCK(gic_sgi_lock);
+#define sgi_map_lock(flags) raw_spin_lock_irqsave(&gic_sgi_lock, flags)
+#define sgi_map_unlock(flags) raw_spin_unlock_irqrestore(&gic_sgi_lock, flags)
+#else
+#define sgi_map_lock(flags) (void)(flags)
+#define sgi_map_unlock(flags) (void)(flags)
+#endif
+
 /*
  * Supported arch specific GIC irq extension.
  * Default make them NULL.
@@ -658,7 +668,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 	int cpu;
 	unsigned long flags, map = 0;
 
-	raw_spin_lock_irqsave(&irq_controller_lock, flags);
+	sgi_map_lock(flags);
 
 	/* Convert our logical CPU mask into a physical one. */
 	for_each_cpu(cpu, mask)
@@ -673,7 +683,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 	/* this always happens on GIC0 */
 	writel_relaxed(map << 16 | irq, gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
 
-	raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
+	sgi_map_unlock(flags);
 }
 #endif
 
@@ -764,6 +774,15 @@ void gic_migrate_target(unsigned int new_cpu_id)
 
 	raw_spin_unlock(&irq_controller_lock);
 
+	raw_spin_lock(&gic_sgi_lock);
+	/*
+	 * Ensure that the gic_cpu_map update above is seen in
+	 * gic_raise_softirq() before we redirect any pending SGIs that
+	 * may have been raised for the outgoing CPU (cur_cpu_id)
+	 */
+	smp_mb__after_unlock_lock();
+	raw_spin_unlock(&gic_sgi_lock);
+
 	/*
 	 * Now let's migrate and clear any potential SGIs that might be
 	 * pending for us (cur_cpu_id).  Since GIC_DIST_SGI_PENDING_SET
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation


From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753155AbaHMOXM (ORCPT <rfc822;w@1wt.eu>);
	Wed, 13 Aug 2014 10:23:12 -0400
Received: from gw-1.arm.linux.org.uk ([78.32.30.217]:49150 "EHLO
	pandora.arm.linux.org.uk" rhost-flags-OK-OK-OK-FAIL)
	by vger.kernel.org with ESMTP id S1751518AbaHMOXL (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 13 Aug 2014 10:23:11 -0400
Date: Wed, 13 Aug 2014 15:22:57 +0100
From: Russell King - ARM Linux <linux@arm.linux.org.uk>
To: Stephen Boyd <sboyd@codeaurora.org>, Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        Nicolas Pitre <nico@linaro.org>
Subject: Re: [PATCH v4] irqchip: gic: Allow gic_arch_extn hooks to call
	into scheduler
Message-ID: <20140813142257.GK30401@n2100.arm.linux.org.uk>
References: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org>
User-Agent: Mutt/1.5.19 (2009-01-05)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Aug 13, 2014 at 06:57:18AM -0700, Stephen Boyd wrote:
> Commit 1a6b69b6548c (ARM: gic: add CPU migration support,
> 2012-04-12) introduced an acquisition of the irq_controller_lock
> in gic_raise_softirq() which can lead to a spinlock recursion if
> the gic_arch_extn hooks call into the scheduler (via complete()
> or wake_up(), etc.). This happens because gic_arch_extn hooks are
> normally called with the irq_controller_lock held and calling
> into the scheduler may cause us to call smp_send_reschedule()
> which will grab the irq_controller_lock again. Here's an example
> from a vendor kernel (note that the gic_arch_extn hook code here
> isn't actually in mainline):

Here's a question: why would you want to call into the scheduler from
the gic_arch_extn code?

Oh.  My.  God.  Thomas, what have you done to the generic IRQ layer?
This is /totally/ unsafe:

void disable_irq(unsigned int irq)
{
        if (!__disable_irq_nosync(irq))
                synchronize_irq(irq);
}

static int __disable_irq_nosync(unsigned int irq)
{
        unsigned long flags;
        struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);

        if (!desc)
                return -EINVAL;
        __disable_irq(desc, irq, false);
        irq_put_desc_busunlock(desc, flags);
        return 0;
}

void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend)
{
        if (suspend) {
                if (!desc->action || (desc->action->flags & IRQF_NO_SUSPEND))
                        return;
                desc->istate |= IRQS_SUSPENDED;
        }

        if (!desc->depth++)
                irq_disable(desc);
}

You realise that disable_irq() and enable_irq() can be called by
concurrently by different drivers for the /same/ interrupt.  For
starters, that post-increment there is completely unprotected against
races.  Secondly, the above is completely racy against a concurrent
enable_irq() - what if we're in disable_irq(), we've incremented
depth, but have yet to call irq_disable().  The count now has a
value of 1.

We then preempt, and run another thread which calls enable_irq()
on it.  This results in the depth being decremented, and the IRQ
is now enabled.

We resume the original thread, and continue to call irq_disable(),
resulting in the interrupt being disabled.

That's not nice (the right answer is that it's strictly an unbalanced
enable_irq(), but that's no excuse here.)

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753136AbaHMOxz (ORCPT <rfc822;w@1wt.eu>);
	Wed, 13 Aug 2014 10:53:55 -0400
Received: from mail-wi0-f182.google.com ([209.85.212.182]:43505 "EHLO
	mail-wi0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751557AbaHMOxx (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 13 Aug 2014 10:53:53 -0400
Message-ID: <53EB7BF7.1080702@linaro.org>
Date: Wed, 13 Aug 2014 15:53:43 +0100
From: Daniel Thompson <daniel.thompson@linaro.org>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.7.0
MIME-Version: 1.0
To: Russell King - ARM Linux <linux@arm.linux.org.uk>,
        Stephen Boyd <sboyd@codeaurora.org>,
        Thomas Gleixner <tglx@linutronix.de>
CC: Jason Cooper <jason@lakedaemon.net>, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        Nicolas Pitre <nico@linaro.org>
Subject: Re: [PATCH v4] irqchip: gic: Allow gic_arch_extn hooks to call into
 scheduler
References: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org> <20140813142257.GK30401@n2100.arm.linux.org.uk>
In-Reply-To: <20140813142257.GK30401@n2100.arm.linux.org.uk>
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7BIT
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 13/08/14 15:22, Russell King - ARM Linux wrote:
> On Wed, Aug 13, 2014 at 06:57:18AM -0700, Stephen Boyd wrote:
>> Commit 1a6b69b6548c (ARM: gic: add CPU migration support,
>> 2012-04-12) introduced an acquisition of the irq_controller_lock
>> in gic_raise_softirq() which can lead to a spinlock recursion if
>> the gic_arch_extn hooks call into the scheduler (via complete()
>> or wake_up(), etc.). This happens because gic_arch_extn hooks are
>> normally called with the irq_controller_lock held and calling
>> into the scheduler may cause us to call smp_send_reschedule()
>> which will grab the irq_controller_lock again. Here's an example
>> from a vendor kernel (note that the gic_arch_extn hook code here
>> isn't actually in mainline):
> 
> Here's a question: why would you want to call into the scheduler from
> the gic_arch_extn code?
> 
> Oh.  My.  God.  Thomas, what have you done to the generic IRQ layer?
> This is /totally/ unsafe:
> 
> void disable_irq(unsigned int irq)
> {
>         if (!__disable_irq_nosync(irq))
>                 synchronize_irq(irq);
> }
> 
> static int __disable_irq_nosync(unsigned int irq)
> {
>         unsigned long flags;
>         struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);

irq_get_desc_buslock() results in us owning the descriptor's lock
(raw_spinlock_t).

> 
>         if (!desc)
>                 return -EINVAL;
>         __disable_irq(desc, irq, false);
>         irq_put_desc_busunlock(desc, flags);
>         return 0;
> }
> 
> void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend)
> {
>         if (suspend) {
>                 if (!desc->action || (desc->action->flags & IRQF_NO_SUSPEND))
>                         return;
>                 desc->istate |= IRQS_SUSPENDED;
>         }
> 
>         if (!desc->depth++)
>                 irq_disable(desc);
> }
> 
> You realise that disable_irq() and enable_irq() can be called by
> concurrently by different drivers for the /same/ interrupt.  For
> starters, that post-increment there is completely unprotected against
> races.  Secondly, the above is completely racy against a concurrent
> enable_irq() - what if we're in disable_irq(), we've incremented
> depth, but have yet to call irq_disable().  The count now has a
> value of 1.
> 
> We then preempt, and run another thread which calls enable_irq()
> on it.  This results in the depth being decremented, and the IRQ
> is now enabled.

We shouldn't get that far due to the spinlock taken during the disable.


> We resume the original thread, and continue to call irq_disable(),
> resulting in the interrupt being disabled.
> 
> That's not nice (the right answer is that it's strictly an unbalanced
> enable_irq(), but that's no excuse here.)
> 


From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753263AbaHMOza (ORCPT <rfc822;w@1wt.eu>);
	Wed, 13 Aug 2014 10:55:30 -0400
Received: from smtp.codeaurora.org ([198.145.11.231]:42528 "EHLO
	smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751393AbaHMOz2 (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 13 Aug 2014 10:55:28 -0400
Date: Wed, 13 Aug 2014 07:55:26 -0700
From: Stephen Boyd <sboyd@codeaurora.org>
To: Russell King - ARM Linux <linux@arm.linux.org.uk>
Cc: Thomas Gleixner <tglx@linutronix.de>, Jason Cooper <jason@lakedaemon.net>,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, Nicolas Pitre <nico@linaro.org>
Subject: Re: [PATCH v4] irqchip: gic: Allow gic_arch_extn hooks to call into
 scheduler
Message-ID: <20140813145526.GF32301@codeaurora.org>
References: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org>
 <20140813142257.GK30401@n2100.arm.linux.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20140813142257.GK30401@n2100.arm.linux.org.uk>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 08/13, Russell King - ARM Linux wrote:
> On Wed, Aug 13, 2014 at 06:57:18AM -0700, Stephen Boyd wrote:
> > Commit 1a6b69b6548c (ARM: gic: add CPU migration support,
> > 2012-04-12) introduced an acquisition of the irq_controller_lock
> > in gic_raise_softirq() which can lead to a spinlock recursion if
> > the gic_arch_extn hooks call into the scheduler (via complete()
> > or wake_up(), etc.). This happens because gic_arch_extn hooks are
> > normally called with the irq_controller_lock held and calling
> > into the scheduler may cause us to call smp_send_reschedule()
> > which will grab the irq_controller_lock again. Here's an example
> > from a vendor kernel (note that the gic_arch_extn hook code here
> > isn't actually in mainline):
> 
> Here's a question: why would you want to call into the scheduler from
> the gic_arch_extn code?

In this case we want to send a message to another processor when
an interrupt is enabled that's only a wakeup interrupt in certain
low power states. It's done sort of indirectly, but basically we
block that low power state from being entered so we can ensure
that the interrupt wakes us up from a lighter version of suspend.

> 
> Oh.  My.  God.  Thomas, what have you done to the generic IRQ layer?
> This is /totally/ unsafe:
> 
> void disable_irq(unsigned int irq)
> {
>         if (!__disable_irq_nosync(irq))
>                 synchronize_irq(irq);
> }
> 
> static int __disable_irq_nosync(unsigned int irq)
> {
>         unsigned long flags;
>         struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);

We got the lock here.

> 
>         if (!desc)
>                 return -EINVAL;
>         __disable_irq(desc, irq, false);
>         irq_put_desc_busunlock(desc, flags);
>         return 0;
> }
> 
> void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend)
> {
>         if (suspend) {
>                 if (!desc->action || (desc->action->flags & IRQF_NO_SUSPEND))
>                         return;
>                 desc->istate |= IRQS_SUSPENDED;
>         }
> 
>         if (!desc->depth++)

We're still holding the lock here right?

>                 irq_disable(desc);
> }
> 
> You realise that disable_irq() and enable_irq() can be called by
> concurrently by different drivers for the /same/ interrupt. 

Sure.

> For
> starters, that post-increment there is completely unprotected against
> races.  Secondly, the above is completely racy against a concurrent
> enable_irq() - what if we're in disable_irq(), we've incremented
> depth, but have yet to call irq_disable().  The count now has a
> value of 1.
> 
> We then preempt, and run another thread which calls enable_irq()
> on it.  This results in the depth being decremented, and the IRQ
> is now enabled.

How? Aren't we holding the descriptor lock?

> 
> We resume the original thread, and continue to call irq_disable(),
> resulting in the interrupt being disabled.
> 
> That's not nice (the right answer is that it's strictly an unbalanced
> enable_irq(), but that's no excuse here.)
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753149AbaHMPFx (ORCPT <rfc822;w@1wt.eu>);
	Wed, 13 Aug 2014 11:05:53 -0400
Received: from gw-1.arm.linux.org.uk ([78.32.30.217]:49177 "EHLO
	pandora.arm.linux.org.uk" rhost-flags-OK-OK-OK-FAIL)
	by vger.kernel.org with ESMTP id S1751965AbaHMPFv (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 13 Aug 2014 11:05:51 -0400
Date: Wed, 13 Aug 2014 16:05:40 +0100
From: Russell King - ARM Linux <linux@arm.linux.org.uk>
To: Stephen Boyd <sboyd@codeaurora.org>
Cc: Thomas Gleixner <tglx@linutronix.de>, Jason Cooper <jason@lakedaemon.net>,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, Nicolas Pitre <nico@linaro.org>
Subject: Re: [PATCH v4] irqchip: gic: Allow gic_arch_extn hooks to call
	into scheduler
Message-ID: <20140813150540.GL30401@n2100.arm.linux.org.uk>
References: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org> <20140813142257.GK30401@n2100.arm.linux.org.uk> <20140813145526.GF32301@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20140813145526.GF32301@codeaurora.org>
User-Agent: Mutt/1.5.19 (2009-01-05)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Aug 13, 2014 at 07:55:26AM -0700, Stephen Boyd wrote:
> On 08/13, Russell King - ARM Linux wrote:
> > On Wed, Aug 13, 2014 at 06:57:18AM -0700, Stephen Boyd wrote:
> > > Commit 1a6b69b6548c (ARM: gic: add CPU migration support,
> > > 2012-04-12) introduced an acquisition of the irq_controller_lock
> > > in gic_raise_softirq() which can lead to a spinlock recursion if
> > > the gic_arch_extn hooks call into the scheduler (via complete()
> > > or wake_up(), etc.). This happens because gic_arch_extn hooks are
> > > normally called with the irq_controller_lock held and calling
> > > into the scheduler may cause us to call smp_send_reschedule()
> > > which will grab the irq_controller_lock again. Here's an example
> > > from a vendor kernel (note that the gic_arch_extn hook code here
> > > isn't actually in mainline):
> > 
> > Here's a question: why would you want to call into the scheduler from
> > the gic_arch_extn code?
> 
> In this case we want to send a message to another processor when
> an interrupt is enabled that's only a wakeup interrupt in certain
> low power states. It's done sort of indirectly, but basically we
> block that low power state from being entered so we can ensure
> that the interrupt wakes us up from a lighter version of suspend.

No, that's not the correct answer for the question I asked.  I did not
ask "why would you want to call into the IRQ code from the scheduler".
I asked "why would you want to call into the scheduler from the
gic_arch_extn code?"

That's a completely different question.  Let me rephrase to try and get
an answer to my question: Why are you calling complete() or wake_up()
from the gic_arch_extn code?

> > static int __disable_irq_nosync(unsigned int irq)
> > {
> >         unsigned long flags;
> >         struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
> 
> We got the lock here.

Yes, Daniel pointed that out, which makes this fine of course.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753326AbaHMPbc (ORCPT <rfc822;w@1wt.eu>);
	Wed, 13 Aug 2014 11:31:32 -0400
Received: from smtp.codeaurora.org ([198.145.11.231]:44617 "EHLO
	smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752833AbaHMPba (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 13 Aug 2014 11:31:30 -0400
Date: Wed, 13 Aug 2014 08:31:29 -0700
From: Stephen Boyd <sboyd@codeaurora.org>
To: Russell King - ARM Linux <linux@arm.linux.org.uk>
Cc: Thomas Gleixner <tglx@linutronix.de>, Jason Cooper <jason@lakedaemon.net>,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, Nicolas Pitre <nico@linaro.org>
Subject: Re: [PATCH v4] irqchip: gic: Allow gic_arch_extn hooks to call into
 scheduler
Message-ID: <20140813153129.GH32301@codeaurora.org>
References: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org>
 <20140813142257.GK30401@n2100.arm.linux.org.uk>
 <20140813145526.GF32301@codeaurora.org>
 <20140813150540.GL30401@n2100.arm.linux.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20140813150540.GL30401@n2100.arm.linux.org.uk>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 08/13, Russell King - ARM Linux wrote:
> On Wed, Aug 13, 2014 at 07:55:26AM -0700, Stephen Boyd wrote:
> > On 08/13, Russell King - ARM Linux wrote:
> > > On Wed, Aug 13, 2014 at 06:57:18AM -0700, Stephen Boyd wrote:
> > > > Commit 1a6b69b6548c (ARM: gic: add CPU migration support,
> > > > 2012-04-12) introduced an acquisition of the irq_controller_lock
> > > > in gic_raise_softirq() which can lead to a spinlock recursion if
> > > > the gic_arch_extn hooks call into the scheduler (via complete()
> > > > or wake_up(), etc.). This happens because gic_arch_extn hooks are
> > > > normally called with the irq_controller_lock held and calling
> > > > into the scheduler may cause us to call smp_send_reschedule()
> > > > which will grab the irq_controller_lock again. Here's an example
> > > > from a vendor kernel (note that the gic_arch_extn hook code here
> > > > isn't actually in mainline):
> > > 
> > > Here's a question: why would you want to call into the scheduler from
> > > the gic_arch_extn code?
> > 
> > In this case we want to send a message to another processor when
> > an interrupt is enabled that's only a wakeup interrupt in certain
> > low power states. It's done sort of indirectly, but basically we
> > block that low power state from being entered so we can ensure
> > that the interrupt wakes us up from a lighter version of suspend.
> 
> No, that's not the correct answer for the question I asked.  I did not
> ask "why would you want to call into the IRQ code from the scheduler".
> I asked "why would you want to call into the scheduler from the
> gic_arch_extn code?"
> 
> That's a completely different question.  Let me rephrase to try and get
> an answer to my question: Why are you calling complete() or wake_up()
> from the gic_arch_extn code?

That's the answer. I guess you think "another processor" means
some other CPU running linux? That isn't the case. We aren't
calling into the IRQ code from the scheduler. The IRQ code is
calling the gic_arch_extn code which is calling into the
scheduler. Let me try to clarify. The path from the stacktrace I
posted is:

 request_irq()
  irq_enable()
   gic_unmask_irq()
    msm_mpm_enable_irq()
     ...
     complete()

In this case a driver is requesting an interrupt that is wakeup
capable even in the lowest power state, so we wakeup a thread
that's sitting around waiting on that completion to notify the
non-linux running remote processor that we can go into the lowest
power state. We can only communicate with the other processor via
sleeping APIs. Another way to do it would be to schedule a work
but we would get into the same situation as this because
scheduling works may call into the scheduler too.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753392AbaHMPox (ORCPT <rfc822;w@1wt.eu>);
	Wed, 13 Aug 2014 11:44:53 -0400
Received: from mail-qg0-f49.google.com ([209.85.192.49]:56521 "EHLO
	mail-qg0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1753342AbaHMPow (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 13 Aug 2014 11:44:52 -0400
Date: Wed, 13 Aug 2014 11:44:48 -0400 (EDT)
From: Nicolas Pitre <nicolas.pitre@linaro.org>
To: Stephen Boyd <sboyd@codeaurora.org>
cc: Thomas Gleixner <tglx@linutronix.de>, Jason Cooper <jason@lakedaemon.net>,
        linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4] irqchip: gic: Allow gic_arch_extn hooks to call into
 scheduler
In-Reply-To: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org>
Message-ID: <alpine.LFD.2.11.1408131139400.1133@knanqh.ubzr>
References: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org>
User-Agent: Alpine 2.11 (LFD 23 2013-08-11)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, 13 Aug 2014, Stephen Boyd wrote:

> Commit 1a6b69b6548c (ARM: gic: add CPU migration support,
> 2012-04-12) introduced an acquisition of the irq_controller_lock
> in gic_raise_softirq() which can lead to a spinlock recursion if
> the gic_arch_extn hooks call into the scheduler (via complete()
> or wake_up(), etc.). This happens because gic_arch_extn hooks are
> normally called with the irq_controller_lock held and calling
> into the scheduler may cause us to call smp_send_reschedule()
> which will grab the irq_controller_lock again. Here's an example
> from a vendor kernel (note that the gic_arch_extn hook code here
> isn't actually in mainline):
> 
> BUG: spinlock recursion on CPU#0, swapper/0/1
>  lock: irq_controller_lock+0x0/0x18, .magic: dead4ead, .owner: sw
> er_cpu: 0
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.10-00430-g3d433c4e
> 
> Call trace:
> [<ffffffc000087e1c>] dump_backtrace+0x0/0x140
> [<ffffffc000087f6c>] show_stack+0x10/0x1c
> [<ffffffc00064732c>] dump_stack+0x74/0xc4
> [<ffffffc0006446c0>] spin_dump+0x78/0x88
> [<ffffffc0006446f4>] spin_bug+0x24/0x34
> [<ffffffc0000d47d0>] do_raw_spin_lock+0x58/0x148
> [<ffffffc00064d398>] _raw_spin_lock_irqsave+0x24/0x38
> [<ffffffc0002c9d7c>] gic_raise_softirq+0x2c/0xbc
> [<ffffffc00008daa4>] smp_send_reschedule+0x34/0x40
> [<ffffffc0000c1e94>] try_to_wake_up+0x224/0x288
> [<ffffffc0000c1f4c>] default_wake_function+0xc/0x18
> [<ffffffc0000ceef0>] __wake_up_common+0x50/0x8c
> [<ffffffc0000cef3c>] __wake_up_locked+0x10/0x1c
> [<ffffffc0000cf734>] complete+0x3c/0x5c
> [<ffffffc0002f0e78>] msm_mpm_enable_irq_exclusive+0x1b8/0x1c8
> [<ffffffc0002f0f58>] __msm_mpm_enable_irq+0x4c/0x7c
> [<ffffffc0002f0f94>] msm_mpm_enable_irq+0xc/0x18
> [<ffffffc0002c9bb0>] gic_unmask_irq+0x40/0x7c
> [<ffffffc0000de5f4>] irq_enable+0x2c/0x48
> [<ffffffc0000de65c>] irq_startup+0x4c/0x74
> [<ffffffc0000dd2fc>] __setup_irq+0x264/0x3f0
> [<ffffffc0000dd5e0>] request_threaded_irq+0xcc/0x11c
> [<ffffffc0000df254>] devm_request_threaded_irq+0x68/0xb4
> [<ffffffc000471520>] msm_iommu_ctx_probe+0x124/0x2d4
> [<ffffffc000337374>] platform_drv_probe+0x20/0x54
> [<ffffffc00033598c>] driver_probe_device+0x158/0x340
> [<ffffffc000335c20>] __driver_attach+0x60/0x90
> [<ffffffc000333c9c>] bus_for_each_dev+0x6c/0x8c
> [<ffffffc000335304>] driver_attach+0x1c/0x28
> [<ffffffc000334f14>] bus_add_driver+0x120/0x204
> [<ffffffc0003362e4>] driver_register+0xbc/0x10c
> [<ffffffc000337348>] __platform_driver_register+0x5c/0x68
> [<ffffffc00094c478>] msm_iommu_driver_init+0x54/0x7c
> [<ffffffc0000813ec>] do_one_initcall+0xa4/0x130
> [<ffffffc00091d928>] kernel_init_freeable+0x138/0x1dc
> [<ffffffc000642578>] kernel_init+0xc/0xd4
> 
> We really just want to synchronize the sending of an SGI with the
> update of the gic_cpu_map[], so introduce a new SGI lock that we
> can use to synchronize the two code paths. Three main events are
> happening that we have to consider:
> 
> 	1. We're updating the gic_cpu_mask to point to an
> 	incoming CPU
> 
> 	2. We're (potentially) sending an SGI to the outgoing CPU
> 
> 	3. We're redirecting any pending SGIs for the outgoing
> 	CPU to the incoming CPU.
> 
> Events 1 and 3 are already ordered within the same CPU by means
> of program order and use of I/O accessors. Events 1 and 2 don't
> need to be ordered, but events 2 and 3 do because any SGIs for
> the outgoing CPU need to be pending before we can redirect them.
> Synchronize by acquiring a new lock around event 2 and before
> event 3. Use smp_mb__after_unlock_lock() before event 3 to ensure
> that event 1 is seen before event 3 on other CPUs that may be
> executing event 2. We put this all behind the b.L switcher config
> option so that if we're not using this feature we don't have to
> acquire any locks at all in the IPI path.
> 
> Cc: Nicolas Pitre <nico@linaro.org>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>

Russell's concerns notwithstanding, this is a worthwhile optimization 
and cleanup.

Reviewed-by: Nicolas Pitre <nico@linaro.org>

Of course it would be good to clarify things wrt Russell's remark 
independently from this patch.


> ---
>  drivers/irqchip/irq-gic.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 7c131cf7cc13..b9e669cb1c1e 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -81,6 +81,16 @@ static DEFINE_RAW_SPINLOCK(irq_controller_lock);
>  #define NR_GIC_CPU_IF 8
>  static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly;
>  
> +#ifdef CONFIG_BL_SWITCHER
> +/* Synchronize switching CPU interface and sending SGIs */
> +static DEFINE_RAW_SPINLOCK(gic_sgi_lock);
> +#define sgi_map_lock(flags) raw_spin_lock_irqsave(&gic_sgi_lock, flags)
> +#define sgi_map_unlock(flags) raw_spin_unlock_irqrestore(&gic_sgi_lock, flags)
> +#else
> +#define sgi_map_lock(flags) (void)(flags)
> +#define sgi_map_unlock(flags) (void)(flags)
> +#endif
> +
>  /*
>   * Supported arch specific GIC irq extension.
>   * Default make them NULL.
> @@ -658,7 +668,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
>  	int cpu;
>  	unsigned long flags, map = 0;
>  
> -	raw_spin_lock_irqsave(&irq_controller_lock, flags);
> +	sgi_map_lock(flags);
>  
>  	/* Convert our logical CPU mask into a physical one. */
>  	for_each_cpu(cpu, mask)
> @@ -673,7 +683,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
>  	/* this always happens on GIC0 */
>  	writel_relaxed(map << 16 | irq, gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
>  
> -	raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
> +	sgi_map_unlock(flags);
>  }
>  #endif
>  
> @@ -764,6 +774,15 @@ void gic_migrate_target(unsigned int new_cpu_id)
>  
>  	raw_spin_unlock(&irq_controller_lock);
>  
> +	raw_spin_lock(&gic_sgi_lock);
> +	/*
> +	 * Ensure that the gic_cpu_map update above is seen in
> +	 * gic_raise_softirq() before we redirect any pending SGIs that
> +	 * may have been raised for the outgoing CPU (cur_cpu_id)
> +	 */
> +	smp_mb__after_unlock_lock();
> +	raw_spin_unlock(&gic_sgi_lock);
> +
>  	/*
>  	 * Now let's migrate and clear any potential SGIs that might be
>  	 * pending for us (cur_cpu_id).  Since GIC_DIST_SGI_PENDING_SET
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 
> 

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752265AbaHQRcu (ORCPT <rfc822;w@1wt.eu>);
	Sun, 17 Aug 2014 13:32:50 -0400
Received: from mho-03-ewr.mailhop.org ([204.13.248.66]:63993 "EHLO
	mho-01-ewr.mailhop.org" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org
	with ESMTP id S1752243AbaHQRcr (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Sun, 17 Aug 2014 13:32:47 -0400
X-Mail-Handler: Dyn Standard SMTP by Dyn
X-Originating-IP: 96.249.243.124
X-Report-Abuse-To: abuse@dyndns.com (see http://www.dyndns.com/services/sendlabs/outbound_abuse.html for abuse reporting information)
X-MHO-User: U2FsdGVkX19gMZ6qKKmgxmTqchroMcT8MKV+OU8rnGI=
X-DKIM: OpenDKIM Filter v2.0.1 titan 7BC1C5C7DE3
Date: Sun, 17 Aug 2014 13:32:36 -0400
From: Jason Cooper <jason@lakedaemon.net>
To: Stephen Boyd <sboyd@codeaurora.org>
Cc: Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        Nicolas Pitre <nico@linaro.org>
Subject: Re: [PATCH v4] irqchip: gic: Allow gic_arch_extn hooks to call
 into scheduler
Message-ID: <20140817173236.GG12769@titan.lakedaemon.net>
References: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org>
User-Agent: Mutt/1.5.20 (2009-06-14)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Stephen,

On Wed, Aug 13, 2014 at 06:57:18AM -0700, Stephen Boyd wrote:
> Commit 1a6b69b6548c (ARM: gic: add CPU migration support,
> 2012-04-12) introduced an acquisition of the irq_controller_lock
> in gic_raise_softirq() which can lead to a spinlock recursion if
> the gic_arch_extn hooks call into the scheduler (via complete()
> or wake_up(), etc.). This happens because gic_arch_extn hooks are
> normally called with the irq_controller_lock held and calling
> into the scheduler may cause us to call smp_send_reschedule()
> which will grab the irq_controller_lock again. Here's an example
> from a vendor kernel (note that the gic_arch_extn hook code here
> isn't actually in mainline):
> 
> BUG: spinlock recursion on CPU#0, swapper/0/1
>  lock: irq_controller_lock+0x0/0x18, .magic: dead4ead, .owner: sw
> er_cpu: 0
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.10-00430-g3d433c4e
> 
> Call trace:
> [<ffffffc000087e1c>] dump_backtrace+0x0/0x140
> [<ffffffc000087f6c>] show_stack+0x10/0x1c
> [<ffffffc00064732c>] dump_stack+0x74/0xc4
> [<ffffffc0006446c0>] spin_dump+0x78/0x88
> [<ffffffc0006446f4>] spin_bug+0x24/0x34
> [<ffffffc0000d47d0>] do_raw_spin_lock+0x58/0x148
> [<ffffffc00064d398>] _raw_spin_lock_irqsave+0x24/0x38
> [<ffffffc0002c9d7c>] gic_raise_softirq+0x2c/0xbc
> [<ffffffc00008daa4>] smp_send_reschedule+0x34/0x40
> [<ffffffc0000c1e94>] try_to_wake_up+0x224/0x288
> [<ffffffc0000c1f4c>] default_wake_function+0xc/0x18
> [<ffffffc0000ceef0>] __wake_up_common+0x50/0x8c
> [<ffffffc0000cef3c>] __wake_up_locked+0x10/0x1c
> [<ffffffc0000cf734>] complete+0x3c/0x5c
> [<ffffffc0002f0e78>] msm_mpm_enable_irq_exclusive+0x1b8/0x1c8
> [<ffffffc0002f0f58>] __msm_mpm_enable_irq+0x4c/0x7c
> [<ffffffc0002f0f94>] msm_mpm_enable_irq+0xc/0x18
> [<ffffffc0002c9bb0>] gic_unmask_irq+0x40/0x7c
> [<ffffffc0000de5f4>] irq_enable+0x2c/0x48
> [<ffffffc0000de65c>] irq_startup+0x4c/0x74
> [<ffffffc0000dd2fc>] __setup_irq+0x264/0x3f0
> [<ffffffc0000dd5e0>] request_threaded_irq+0xcc/0x11c
> [<ffffffc0000df254>] devm_request_threaded_irq+0x68/0xb4
> [<ffffffc000471520>] msm_iommu_ctx_probe+0x124/0x2d4
> [<ffffffc000337374>] platform_drv_probe+0x20/0x54
> [<ffffffc00033598c>] driver_probe_device+0x158/0x340
> [<ffffffc000335c20>] __driver_attach+0x60/0x90
> [<ffffffc000333c9c>] bus_for_each_dev+0x6c/0x8c
> [<ffffffc000335304>] driver_attach+0x1c/0x28
> [<ffffffc000334f14>] bus_add_driver+0x120/0x204
> [<ffffffc0003362e4>] driver_register+0xbc/0x10c
> [<ffffffc000337348>] __platform_driver_register+0x5c/0x68
> [<ffffffc00094c478>] msm_iommu_driver_init+0x54/0x7c
> [<ffffffc0000813ec>] do_one_initcall+0xa4/0x130
> [<ffffffc00091d928>] kernel_init_freeable+0x138/0x1dc
> [<ffffffc000642578>] kernel_init+0xc/0xd4
> 
> We really just want to synchronize the sending of an SGI with the
> update of the gic_cpu_map[], so introduce a new SGI lock that we
> can use to synchronize the two code paths. Three main events are
> happening that we have to consider:
> 
> 	1. We're updating the gic_cpu_mask to point to an
> 	incoming CPU
> 
> 	2. We're (potentially) sending an SGI to the outgoing CPU
> 
> 	3. We're redirecting any pending SGIs for the outgoing
> 	CPU to the incoming CPU.
> 
> Events 1 and 3 are already ordered within the same CPU by means
> of program order and use of I/O accessors. Events 1 and 2 don't
> need to be ordered, but events 2 and 3 do because any SGIs for
> the outgoing CPU need to be pending before we can redirect them.
> Synchronize by acquiring a new lock around event 2 and before
> event 3. Use smp_mb__after_unlock_lock() before event 3 to ensure
> that event 1 is seen before event 3 on other CPUs that may be
> executing event 2. We put this all behind the b.L switcher config
> option so that if we're not using this feature we don't have to
> acquire any locks at all in the IPI path.
> 
> Cc: Nicolas Pitre <nico@linaro.org>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>  drivers/irqchip/irq-gic.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)

Applied to irqchip/urgent with Nico's Ack.

thx,

Jason.

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751512AbaHQSzl (ORCPT <rfc822;w@1wt.eu>);
	Sun, 17 Aug 2014 14:55:41 -0400
Received: from gw-1.arm.linux.org.uk ([78.32.30.217]:52427 "EHLO
	pandora.arm.linux.org.uk" rhost-flags-OK-OK-OK-FAIL)
	by vger.kernel.org with ESMTP id S1751368AbaHQSzj (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Sun, 17 Aug 2014 14:55:39 -0400
Date: Sun, 17 Aug 2014 19:55:23 +0100
From: Russell King - ARM Linux <linux@arm.linux.org.uk>
To: Jason Cooper <jason@lakedaemon.net>
Cc: Stephen Boyd <sboyd@codeaurora.org>, linux-arm-msm@vger.kernel.org,
        Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, Nicolas Pitre <nico@linaro.org>
Subject: Re: [PATCH v4] irqchip: gic: Allow gic_arch_extn hooks to call
	into scheduler
Message-ID: <20140817185522.GX30401@n2100.arm.linux.org.uk>
References: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org> <20140817173236.GG12769@titan.lakedaemon.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20140817173236.GG12769@titan.lakedaemon.net>
User-Agent: Mutt/1.5.19 (2009-01-05)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sun, Aug 17, 2014 at 01:32:36PM -0400, Jason Cooper wrote:
> Stephen,
> 
> On Wed, Aug 13, 2014 at 06:57:18AM -0700, Stephen Boyd wrote:
> > Commit 1a6b69b6548c (ARM: gic: add CPU migration support,
> > 2012-04-12) introduced an acquisition of the irq_controller_lock
> > in gic_raise_softirq() which can lead to a spinlock recursion if
> > the gic_arch_extn hooks call into the scheduler (via complete()
> > or wake_up(), etc.). This happens because gic_arch_extn hooks are
> > normally called with the irq_controller_lock held and calling
> > into the scheduler may cause us to call smp_send_reschedule()
> > which will grab the irq_controller_lock again. Here's an example
> > from a vendor kernel (note that the gic_arch_extn hook code here
> > isn't actually in mainline):
> > 
> > BUG: spinlock recursion on CPU#0, swapper/0/1
> >  lock: irq_controller_lock+0x0/0x18, .magic: dead4ead, .owner: sw
> > er_cpu: 0
> > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.10-00430-g3d433c4e
> > 
> > Call trace:
> > [<ffffffc000087e1c>] dump_backtrace+0x0/0x140
> > [<ffffffc000087f6c>] show_stack+0x10/0x1c
> > [<ffffffc00064732c>] dump_stack+0x74/0xc4
> > [<ffffffc0006446c0>] spin_dump+0x78/0x88
> > [<ffffffc0006446f4>] spin_bug+0x24/0x34
> > [<ffffffc0000d47d0>] do_raw_spin_lock+0x58/0x148
> > [<ffffffc00064d398>] _raw_spin_lock_irqsave+0x24/0x38
> > [<ffffffc0002c9d7c>] gic_raise_softirq+0x2c/0xbc
> > [<ffffffc00008daa4>] smp_send_reschedule+0x34/0x40
> > [<ffffffc0000c1e94>] try_to_wake_up+0x224/0x288
> > [<ffffffc0000c1f4c>] default_wake_function+0xc/0x18
> > [<ffffffc0000ceef0>] __wake_up_common+0x50/0x8c
> > [<ffffffc0000cef3c>] __wake_up_locked+0x10/0x1c
> > [<ffffffc0000cf734>] complete+0x3c/0x5c
> > [<ffffffc0002f0e78>] msm_mpm_enable_irq_exclusive+0x1b8/0x1c8
> > [<ffffffc0002f0f58>] __msm_mpm_enable_irq+0x4c/0x7c
> > [<ffffffc0002f0f94>] msm_mpm_enable_irq+0xc/0x18
> > [<ffffffc0002c9bb0>] gic_unmask_irq+0x40/0x7c
> > [<ffffffc0000de5f4>] irq_enable+0x2c/0x48
> > [<ffffffc0000de65c>] irq_startup+0x4c/0x74
> > [<ffffffc0000dd2fc>] __setup_irq+0x264/0x3f0
> > [<ffffffc0000dd5e0>] request_threaded_irq+0xcc/0x11c
> > [<ffffffc0000df254>] devm_request_threaded_irq+0x68/0xb4
> > [<ffffffc000471520>] msm_iommu_ctx_probe+0x124/0x2d4
> > [<ffffffc000337374>] platform_drv_probe+0x20/0x54
> > [<ffffffc00033598c>] driver_probe_device+0x158/0x340
> > [<ffffffc000335c20>] __driver_attach+0x60/0x90
> > [<ffffffc000333c9c>] bus_for_each_dev+0x6c/0x8c
> > [<ffffffc000335304>] driver_attach+0x1c/0x28
> > [<ffffffc000334f14>] bus_add_driver+0x120/0x204
> > [<ffffffc0003362e4>] driver_register+0xbc/0x10c
> > [<ffffffc000337348>] __platform_driver_register+0x5c/0x68
> > [<ffffffc00094c478>] msm_iommu_driver_init+0x54/0x7c
> > [<ffffffc0000813ec>] do_one_initcall+0xa4/0x130
> > [<ffffffc00091d928>] kernel_init_freeable+0x138/0x1dc
> > [<ffffffc000642578>] kernel_init+0xc/0xd4
> > 
> > We really just want to synchronize the sending of an SGI with the
> > update of the gic_cpu_map[], so introduce a new SGI lock that we
> > can use to synchronize the two code paths. Three main events are
> > happening that we have to consider:
> > 
> > 	1. We're updating the gic_cpu_mask to point to an
> > 	incoming CPU
> > 
> > 	2. We're (potentially) sending an SGI to the outgoing CPU
> > 
> > 	3. We're redirecting any pending SGIs for the outgoing
> > 	CPU to the incoming CPU.
> > 
> > Events 1 and 3 are already ordered within the same CPU by means
> > of program order and use of I/O accessors. Events 1 and 2 don't
> > need to be ordered, but events 2 and 3 do because any SGIs for
> > the outgoing CPU need to be pending before we can redirect them.
> > Synchronize by acquiring a new lock around event 2 and before
> > event 3. Use smp_mb__after_unlock_lock() before event 3 to ensure
> > that event 1 is seen before event 3 on other CPUs that may be
> > executing event 2. We put this all behind the b.L switcher config
> > option so that if we're not using this feature we don't have to
> > acquire any locks at all in the IPI path.
> > 
> > Cc: Nicolas Pitre <nico@linaro.org>
> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > ---
> >  drivers/irqchip/irq-gic.c | 23 +++++++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> Applied to irqchip/urgent with Nico's Ack.

Interesting, so I'm discussing this patch, and it gets applied anyway...
yes, that's great.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751547AbaHQTEv (ORCPT <rfc822;w@1wt.eu>);
	Sun, 17 Aug 2014 15:04:51 -0400
Received: from mho-03-ewr.mailhop.org ([204.13.248.66]:33659 "EHLO
	mho-01-ewr.mailhop.org" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org
	with ESMTP id S1751398AbaHQTEt (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Sun, 17 Aug 2014 15:04:49 -0400
X-Mail-Handler: Dyn Standard SMTP by Dyn
X-Originating-IP: 96.249.243.124
X-Report-Abuse-To: abuse@dyndns.com (see http://www.dyndns.com/services/sendlabs/outbound_abuse.html for abuse reporting information)
X-MHO-User: U2FsdGVkX1/I+932t8WDmLiDVQ7qS+JIDqvXNzvk9uU=
X-DKIM: OpenDKIM Filter v2.0.1 titan 0B9AD5C813C
Date: Sun, 17 Aug 2014 15:04:34 -0400
From: Jason Cooper <jason@lakedaemon.net>
To: Russell King - ARM Linux <linux@arm.linux.org.uk>
Cc: Stephen Boyd <sboyd@codeaurora.org>, linux-arm-msm@vger.kernel.org,
        Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, Nicolas Pitre <nico@linaro.org>
Subject: Re: [PATCH v4] irqchip: gic: Allow gic_arch_extn hooks to call
 into scheduler
Message-ID: <20140817190434.GJ12769@titan.lakedaemon.net>
References: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org>
 <20140817173236.GG12769@titan.lakedaemon.net>
 <20140817185522.GX30401@n2100.arm.linux.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20140817185522.GX30401@n2100.arm.linux.org.uk>
User-Agent: Mutt/1.5.20 (2009-06-14)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Russell,

On Sun, Aug 17, 2014 at 07:55:23PM +0100, Russell King - ARM Linux wrote:
> On Sun, Aug 17, 2014 at 01:32:36PM -0400, Jason Cooper wrote:
> > On Wed, Aug 13, 2014 at 06:57:18AM -0700, Stephen Boyd wrote:
> > > Commit 1a6b69b6548c (ARM: gic: add CPU migration support,
> > > 2012-04-12) introduced an acquisition of the irq_controller_lock
> > > in gic_raise_softirq() which can lead to a spinlock recursion if
> > > the gic_arch_extn hooks call into the scheduler (via complete()
> > > or wake_up(), etc.). This happens because gic_arch_extn hooks are
> > > normally called with the irq_controller_lock held and calling
> > > into the scheduler may cause us to call smp_send_reschedule()
> > > which will grab the irq_controller_lock again. Here's an example
> > > from a vendor kernel (note that the gic_arch_extn hook code here
> > > isn't actually in mainline):
> > > 
> > > BUG: spinlock recursion on CPU#0, swapper/0/1
> > >  lock: irq_controller_lock+0x0/0x18, .magic: dead4ead, .owner: sw
> > > er_cpu: 0
> > > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.10-00430-g3d433c4e
> > > 
> > > Call trace:
> > > [<ffffffc000087e1c>] dump_backtrace+0x0/0x140
> > > [<ffffffc000087f6c>] show_stack+0x10/0x1c
> > > [<ffffffc00064732c>] dump_stack+0x74/0xc4
> > > [<ffffffc0006446c0>] spin_dump+0x78/0x88
> > > [<ffffffc0006446f4>] spin_bug+0x24/0x34
> > > [<ffffffc0000d47d0>] do_raw_spin_lock+0x58/0x148
> > > [<ffffffc00064d398>] _raw_spin_lock_irqsave+0x24/0x38
> > > [<ffffffc0002c9d7c>] gic_raise_softirq+0x2c/0xbc
> > > [<ffffffc00008daa4>] smp_send_reschedule+0x34/0x40
> > > [<ffffffc0000c1e94>] try_to_wake_up+0x224/0x288
> > > [<ffffffc0000c1f4c>] default_wake_function+0xc/0x18
> > > [<ffffffc0000ceef0>] __wake_up_common+0x50/0x8c
> > > [<ffffffc0000cef3c>] __wake_up_locked+0x10/0x1c
> > > [<ffffffc0000cf734>] complete+0x3c/0x5c
> > > [<ffffffc0002f0e78>] msm_mpm_enable_irq_exclusive+0x1b8/0x1c8
> > > [<ffffffc0002f0f58>] __msm_mpm_enable_irq+0x4c/0x7c
> > > [<ffffffc0002f0f94>] msm_mpm_enable_irq+0xc/0x18
> > > [<ffffffc0002c9bb0>] gic_unmask_irq+0x40/0x7c
> > > [<ffffffc0000de5f4>] irq_enable+0x2c/0x48
> > > [<ffffffc0000de65c>] irq_startup+0x4c/0x74
> > > [<ffffffc0000dd2fc>] __setup_irq+0x264/0x3f0
> > > [<ffffffc0000dd5e0>] request_threaded_irq+0xcc/0x11c
> > > [<ffffffc0000df254>] devm_request_threaded_irq+0x68/0xb4
> > > [<ffffffc000471520>] msm_iommu_ctx_probe+0x124/0x2d4
> > > [<ffffffc000337374>] platform_drv_probe+0x20/0x54
> > > [<ffffffc00033598c>] driver_probe_device+0x158/0x340
> > > [<ffffffc000335c20>] __driver_attach+0x60/0x90
> > > [<ffffffc000333c9c>] bus_for_each_dev+0x6c/0x8c
> > > [<ffffffc000335304>] driver_attach+0x1c/0x28
> > > [<ffffffc000334f14>] bus_add_driver+0x120/0x204
> > > [<ffffffc0003362e4>] driver_register+0xbc/0x10c
> > > [<ffffffc000337348>] __platform_driver_register+0x5c/0x68
> > > [<ffffffc00094c478>] msm_iommu_driver_init+0x54/0x7c
> > > [<ffffffc0000813ec>] do_one_initcall+0xa4/0x130
> > > [<ffffffc00091d928>] kernel_init_freeable+0x138/0x1dc
> > > [<ffffffc000642578>] kernel_init+0xc/0xd4
> > > 
> > > We really just want to synchronize the sending of an SGI with the
> > > update of the gic_cpu_map[], so introduce a new SGI lock that we
> > > can use to synchronize the two code paths. Three main events are
> > > happening that we have to consider:
> > > 
> > > 	1. We're updating the gic_cpu_mask to point to an
> > > 	incoming CPU
> > > 
> > > 	2. We're (potentially) sending an SGI to the outgoing CPU
> > > 
> > > 	3. We're redirecting any pending SGIs for the outgoing
> > > 	CPU to the incoming CPU.
> > > 
> > > Events 1 and 3 are already ordered within the same CPU by means
> > > of program order and use of I/O accessors. Events 1 and 2 don't
> > > need to be ordered, but events 2 and 3 do because any SGIs for
> > > the outgoing CPU need to be pending before we can redirect them.
> > > Synchronize by acquiring a new lock around event 2 and before
> > > event 3. Use smp_mb__after_unlock_lock() before event 3 to ensure
> > > that event 1 is seen before event 3 on other CPUs that may be
> > > executing event 2. We put this all behind the b.L switcher config
> > > option so that if we're not using this feature we don't have to
> > > acquire any locks at all in the IPI path.
> > > 
> > > Cc: Nicolas Pitre <nico@linaro.org>
> > > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > > ---
> > >  drivers/irqchip/irq-gic.c | 23 +++++++++++++++++++++--
> > >  1 file changed, 21 insertions(+), 2 deletions(-)
> > 
> > Applied to irqchip/urgent with Nico's Ack.
> 
> Interesting, so I'm discussing this patch, and it gets applied anyway...
> yes, that's great.

Quoting Nico:

"Of course it would be good to clarify things wrt Russell's remark
independently from this patch."

I took 'independently' to mean "This patch is ok, *and* we need to
address Russell's concerns in a follow-up patch."

Nico's Reviewed-by with that comment was sent August 13th.  The most
recent activity on this thread was also August 13th.  After four days, I
reasoned there were no objections to his comment.

thx,

Jason.

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751574AbaHQVlh (ORCPT <rfc822;w@1wt.eu>);
	Sun, 17 Aug 2014 17:41:37 -0400
Received: from gw-1.arm.linux.org.uk ([78.32.30.217]:52472 "EHLO
	pandora.arm.linux.org.uk" rhost-flags-OK-OK-OK-FAIL)
	by vger.kernel.org with ESMTP id S1751368AbaHQVlg (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Sun, 17 Aug 2014 17:41:36 -0400
Date: Sun, 17 Aug 2014 22:41:23 +0100
From: Russell King - ARM Linux <linux@arm.linux.org.uk>
To: Jason Cooper <jason@lakedaemon.net>
Cc: Stephen Boyd <sboyd@codeaurora.org>, linux-arm-msm@vger.kernel.org,
        Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, Nicolas Pitre <nico@linaro.org>
Subject: Re: [PATCH v4] irqchip: gic: Allow gic_arch_extn hooks to call
	into scheduler
Message-ID: <20140817214123.GY30401@n2100.arm.linux.org.uk>
References: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org> <20140817173236.GG12769@titan.lakedaemon.net> <20140817185522.GX30401@n2100.arm.linux.org.uk> <20140817190434.GJ12769@titan.lakedaemon.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20140817190434.GJ12769@titan.lakedaemon.net>
User-Agent: Mutt/1.5.19 (2009-01-05)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sun, Aug 17, 2014 at 03:04:34PM -0400, Jason Cooper wrote:
> Quoting Nico:
> 
> "Of course it would be good to clarify things wrt Russell's remark
> independently from this patch."
> 
> I took 'independently' to mean "This patch is ok, *and* we need to
> address Russell's concerns in a follow-up patch."
> 
> Nico's Reviewed-by with that comment was sent August 13th.  The most
> recent activity on this thread was also August 13th.  After four days, I
> reasoned there were no objections to his comment.

Right, during the merge window, and during merge windows, I tend to
ignore almost all email now because people don't stop developing, and
they don't take any notice where the mainline cycle is.  In fact, I go
off and do non-kernel work during a merge window and only briefly scan
for bug fixes.

However, I have other concerns with this patch, which I've yet to air.
For example, I don't like this crappy conditional locking that people
keep dreaming up - that kind of stuff makes the kernel much harder to
statically check that everything is correct.  It's an anti-lockdep
strategy.

Secondly, I don't like this:

+       raw_spin_lock(&gic_sgi_lock);
+       /*
+        * Ensure that the gic_cpu_map update above is seen in
+        * gic_raise_softirq() before we redirect any pending SGIs that
+        * may have been raised for the outgoing CPU (cur_cpu_id)
+        */
+       smp_mb__after_unlock_lock();
+       raw_spin_unlock(&gic_sgi_lock);

That goes against the principle of locking, that you lock the data,
not the code.

I have no problem with changing gic_raise_softirq() to use a different
lock, which gic_migrate_target(), and gic_set_affinity() can also use.
There's no need for horrid locking here, because the only thing we're
protecting is gic_map[] and the write to the register to trigger an
IPI - and nothing using gic_arch_extn has any business knowing about
SGIs.

No need for these crappy sgi_map_lock() macros and all the ifdeffery.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751640AbaHRAEu (ORCPT <rfc822;w@1wt.eu>);
	Sun, 17 Aug 2014 20:04:50 -0400
Received: from mail-qc0-f172.google.com ([209.85.216.172]:57161 "EHLO
	mail-qc0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751552AbaHRAEs (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Sun, 17 Aug 2014 20:04:48 -0400
Date: Sun, 17 Aug 2014 20:04:45 -0400 (EDT)
From: Nicolas Pitre <nicolas.pitre@linaro.org>
To: Jason Cooper <jason@lakedaemon.net>
cc: Russell King - ARM Linux <linux@arm.linux.org.uk>,
        Stephen Boyd <sboyd@codeaurora.org>, linux-arm-msm@vger.kernel.org,
        Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4] irqchip: gic: Allow gic_arch_extn hooks to call into
 scheduler
In-Reply-To: <20140817190434.GJ12769@titan.lakedaemon.net>
Message-ID: <alpine.LFD.2.11.1408172000460.29347@knanqh.ubzr>
References: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org> <20140817173236.GG12769@titan.lakedaemon.net> <20140817185522.GX30401@n2100.arm.linux.org.uk> <20140817190434.GJ12769@titan.lakedaemon.net>
User-Agent: Alpine 2.11 (LFD 23 2013-08-11)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sun, 17 Aug 2014, Jason Cooper wrote:

> Russell,
> 
> On Sun, Aug 17, 2014 at 07:55:23PM +0100, Russell King - ARM Linux wrote:
> > On Sun, Aug 17, 2014 at 01:32:36PM -0400, Jason Cooper wrote:
> > > Applied to irqchip/urgent with Nico's Ack.
> > 
> > Interesting, so I'm discussing this patch, and it gets applied anyway...
> > yes, that's great.
> 
> Quoting Nico:
> 
> "Of course it would be good to clarify things wrt Russell's remark
> independently from this patch."
> 
> I took 'independently' to mean "This patch is ok, *and* we need to
> address Russell's concerns in a follow-up patch."
> 
> Nico's Reviewed-by with that comment was sent August 13th.  The most
> recent activity on this thread was also August 13th.  After four days, I
> reasoned there were no objections to his comment.

Well... I mentioned this patch is a nice cleanup independently of the 
reason why it was created in the first place.  Maybe that shouldn't be 
sorted as "urgent" in that case, especially when the code having problem 
with the current state of things is living out of mainline.


Nicolas

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751613AbaHRARL (ORCPT <rfc822;w@1wt.eu>);
	Sun, 17 Aug 2014 20:17:11 -0400
Received: from mail-qc0-f171.google.com ([209.85.216.171]:34481 "EHLO
	mail-qc0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1750725AbaHRARI (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Sun, 17 Aug 2014 20:17:08 -0400
Date: Sun, 17 Aug 2014 20:17:05 -0400 (EDT)
From: Nicolas Pitre <nicolas.pitre@linaro.org>
To: Russell King - ARM Linux <linux@arm.linux.org.uk>
cc: Jason Cooper <jason@lakedaemon.net>, Stephen Boyd <sboyd@codeaurora.org>,
        linux-arm-msm@vger.kernel.org, Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4] irqchip: gic: Allow gic_arch_extn hooks to call into
 scheduler
In-Reply-To: <20140817214123.GY30401@n2100.arm.linux.org.uk>
Message-ID: <alpine.LFD.2.11.1408172005070.29347@knanqh.ubzr>
References: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org> <20140817173236.GG12769@titan.lakedaemon.net> <20140817185522.GX30401@n2100.arm.linux.org.uk> <20140817190434.GJ12769@titan.lakedaemon.net> <20140817214123.GY30401@n2100.arm.linux.org.uk>
User-Agent: Alpine 2.11 (LFD 23 2013-08-11)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sun, 17 Aug 2014, Russell King - ARM Linux wrote:

> On Sun, Aug 17, 2014 at 03:04:34PM -0400, Jason Cooper wrote:
> > Quoting Nico:
> > 
> > "Of course it would be good to clarify things wrt Russell's remark
> > independently from this patch."
> > 
> > I took 'independently' to mean "This patch is ok, *and* we need to
> > address Russell's concerns in a follow-up patch."
> > 
> > Nico's Reviewed-by with that comment was sent August 13th.  The most
> > recent activity on this thread was also August 13th.  After four days, I
> > reasoned there were no objections to his comment.
> 
> Right, during the merge window, and during merge windows, I tend to
> ignore almost all email now because people don't stop developing, and
> they don't take any notice where the mainline cycle is.  In fact, I go
> off and do non-kernel work during a merge window and only briefly scan
> for bug fixes.
> 
> However, I have other concerns with this patch, which I've yet to air.
> For example, I don't like this crappy conditional locking that people
> keep dreaming up - that kind of stuff makes the kernel much harder to
> statically check that everything is correct.  It's an anti-lockdep
> strategy.
> 
> Secondly, I don't like this:
> 
> +       raw_spin_lock(&gic_sgi_lock);
> +       /*
> +        * Ensure that the gic_cpu_map update above is seen in
> +        * gic_raise_softirq() before we redirect any pending SGIs that
> +        * may have been raised for the outgoing CPU (cur_cpu_id)
> +        */
> +       smp_mb__after_unlock_lock();
> +       raw_spin_unlock(&gic_sgi_lock);
> 
> That goes against the principle of locking, that you lock the data,
> not the code.

I admit I didn't understand the point of that construct on the first 
read.  Maybe I wouldn't be the only one.  Using Stephen's initial 
version for that hunk would be preferable as it is straight forward and 
would mean locking the data instead.

> I have no problem with changing gic_raise_softirq() to use a different
> lock, which gic_migrate_target(), and gic_set_affinity() can also use.
> There's no need for horrid locking here, because the only thing we're
> protecting is gic_map[] and the write to the register to trigger an
> IPI - and nothing using gic_arch_extn has any business knowing about
> SGIs.
> 
> No need for these crappy sgi_map_lock() macros and all the ifdeffery.

Those macros are there only to conditionalize the locking in 
gic_raise_softirq() because no locking what so ever is needed there when 
gic_migrate_target() is configured out.  I suggested the macros to cut 
down on the #ifdefery in the code.


Nicolas

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751818AbaHRBZ1 (ORCPT <rfc822;w@1wt.eu>);
	Sun, 17 Aug 2014 21:25:27 -0400
Received: from mho-02-ewr.mailhop.org ([204.13.248.72]:38003 "EHLO
	mho-02-ewr.mailhop.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751548AbaHRBZZ (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Sun, 17 Aug 2014 21:25:25 -0400
X-Mail-Handler: Dyn Standard SMTP by Dyn
X-Originating-IP: 96.249.243.124
X-Report-Abuse-To: abuse@dyndns.com (see http://www.dyndns.com/services/sendlabs/outbound_abuse.html for abuse reporting information)
X-MHO-User: U2FsdGVkX188qJcQ9+G3E60hqs4RLcYgNxbzM/7ERvI=
X-DKIM: OpenDKIM Filter v2.0.1 titan 8FAB15C8370
Date: Sun, 17 Aug 2014 21:25:12 -0400
From: Jason Cooper <jason@lakedaemon.net>
To: Nicolas Pitre <nicolas.pitre@linaro.org>
Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>,
        Stephen Boyd <sboyd@codeaurora.org>, linux-arm-msm@vger.kernel.org,
        Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4] irqchip: gic: Allow gic_arch_extn hooks to call
 into scheduler
Message-ID: <20140818012512.GL12769@titan.lakedaemon.net>
References: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org>
 <20140817173236.GG12769@titan.lakedaemon.net>
 <20140817185522.GX30401@n2100.arm.linux.org.uk>
 <20140817190434.GJ12769@titan.lakedaemon.net>
 <alpine.LFD.2.11.1408172000460.29347@knanqh.ubzr>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.LFD.2.11.1408172000460.29347@knanqh.ubzr>
User-Agent: Mutt/1.5.20 (2009-06-14)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sun, Aug 17, 2014 at 08:04:45PM -0400, Nicolas Pitre wrote:
> On Sun, 17 Aug 2014, Jason Cooper wrote:
> > On Sun, Aug 17, 2014 at 07:55:23PM +0100, Russell King - ARM Linux wrote:
> > > On Sun, Aug 17, 2014 at 01:32:36PM -0400, Jason Cooper wrote:
> > > > Applied to irqchip/urgent with Nico's Ack.
> > > 
> > > Interesting, so I'm discussing this patch, and it gets applied anyway...
> > > yes, that's great.
> > 
> > Quoting Nico:
> > 
> > "Of course it would be good to clarify things wrt Russell's remark
> > independently from this patch."
> > 
> > I took 'independently' to mean "This patch is ok, *and* we need to
> > address Russell's concerns in a follow-up patch."
> > 
> > Nico's Reviewed-by with that comment was sent August 13th.  The most
> > recent activity on this thread was also August 13th.  After four days, I
> > reasoned there were no objections to his comment.
> 
> Well... I mentioned this patch is a nice cleanup independently of the 
> reason why it was created in the first place.

Ah, fair enough.

> Maybe that shouldn't be sorted as "urgent" in that case, especially
> when the code having problem with the current state of things is
> living out of mainline.

hmmm, yes.  I've been grappling with the semantics of '/urgent' vice
'/fixes'.  With mvebu, /fixes is the branch for all changes needing to go
into the current -rcX cycle.  For irqchip, Thomas suggested /urgent for
the equivalent branch.  To me, they serve the same purpose.
Unfortunately, I occasionally hear "Well, it's not _urgent_ ...".  I
suppose I'll put up with it for one more cycle and then change it to
/fixes. :)

wrt this patch, I need to drop it anyway.  I was a bit rusty (it's been
a few weeks) and forgot to add the Cc -stable and Fixes: tags.  I do
agree, though, it's certainly not urgent.

As Russell has raised more issues with this patch as well, I'll hold off
on re-applying until I see a new version.  Hopefully it'll meet with
everyones approval.

thx,

Jason.

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751805AbaHRBch (ORCPT <rfc822;w@1wt.eu>);
	Sun, 17 Aug 2014 21:32:37 -0400
Received: from mho-03-ewr.mailhop.org ([204.13.248.66]:10310 "EHLO
	mho-01-ewr.mailhop.org" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org
	with ESMTP id S1751372AbaHRBcg (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Sun, 17 Aug 2014 21:32:36 -0400
X-Mail-Handler: Dyn Standard SMTP by Dyn
X-Originating-IP: 96.249.243.124
X-Report-Abuse-To: abuse@dyndns.com (see http://www.dyndns.com/services/sendlabs/outbound_abuse.html for abuse reporting information)
X-MHO-User: U2FsdGVkX18mbGtZxesb6Q8y7uf6rm23Ql4olID3N54=
X-DKIM: OpenDKIM Filter v2.0.1 titan 1A21C5C838D
Date: Sun, 17 Aug 2014 21:32:24 -0400
From: Jason Cooper <jason@lakedaemon.net>
To: Russell King - ARM Linux <linux@arm.linux.org.uk>
Cc: Stephen Boyd <sboyd@codeaurora.org>, linux-arm-msm@vger.kernel.org,
        Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, Nicolas Pitre <nico@linaro.org>
Subject: Re: [PATCH v4] irqchip: gic: Allow gic_arch_extn hooks to call
 into scheduler
Message-ID: <20140818013224.GM12769@titan.lakedaemon.net>
References: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org>
 <20140817173236.GG12769@titan.lakedaemon.net>
 <20140817185522.GX30401@n2100.arm.linux.org.uk>
 <20140817190434.GJ12769@titan.lakedaemon.net>
 <20140817214123.GY30401@n2100.arm.linux.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20140817214123.GY30401@n2100.arm.linux.org.uk>
User-Agent: Mutt/1.5.20 (2009-06-14)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sun, Aug 17, 2014 at 10:41:23PM +0100, Russell King - ARM Linux wrote:
> On Sun, Aug 17, 2014 at 03:04:34PM -0400, Jason Cooper wrote:
> > Quoting Nico:
> > 
> > "Of course it would be good to clarify things wrt Russell's remark
> > independently from this patch."
> > 
> > I took 'independently' to mean "This patch is ok, *and* we need to
> > address Russell's concerns in a follow-up patch."
> > 
> > Nico's Reviewed-by with that comment was sent August 13th.  The most
> > recent activity on this thread was also August 13th.  After four days, I
> > reasoned there were no objections to his comment.
> 
> Right, during the merge window, and during merge windows, I tend to
> ignore almost all email now because people don't stop developing, and
> they don't take any notice where the mainline cycle is.  In fact, I go
> off and do non-kernel work during a merge window and only briefly scan
> for bug fixes.

Ok, now dropped.

thx,

Jason.

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751870AbaHRBfR (ORCPT <rfc822;w@1wt.eu>);
	Sun, 17 Aug 2014 21:35:17 -0400
Received: from mail-qg0-f46.google.com ([209.85.192.46]:32928 "EHLO
	mail-qg0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751636AbaHRBfP (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Sun, 17 Aug 2014 21:35:15 -0400
Date: Sun, 17 Aug 2014 21:35:11 -0400 (EDT)
From: Nicolas Pitre <nicolas.pitre@linaro.org>
To: Jason Cooper <jason@lakedaemon.net>
cc: Russell King - ARM Linux <linux@arm.linux.org.uk>,
        Stephen Boyd <sboyd@codeaurora.org>, linux-arm-msm@vger.kernel.org,
        Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4] irqchip: gic: Allow gic_arch_extn hooks to call into
 scheduler
In-Reply-To: <20140818012512.GL12769@titan.lakedaemon.net>
Message-ID: <alpine.LFD.2.11.1408172131550.29347@knanqh.ubzr>
References: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org> <20140817173236.GG12769@titan.lakedaemon.net> <20140817185522.GX30401@n2100.arm.linux.org.uk> <20140817190434.GJ12769@titan.lakedaemon.net> <alpine.LFD.2.11.1408172000460.29347@knanqh.ubzr>
 <20140818012512.GL12769@titan.lakedaemon.net>
User-Agent: Alpine 2.11 (LFD 23 2013-08-11)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sun, 17 Aug 2014, Jason Cooper wrote:

> On Sun, Aug 17, 2014 at 08:04:45PM -0400, Nicolas Pitre wrote:
> > On Sun, 17 Aug 2014, Jason Cooper wrote:
> > > On Sun, Aug 17, 2014 at 07:55:23PM +0100, Russell King - ARM Linux wrote:
> > > > On Sun, Aug 17, 2014 at 01:32:36PM -0400, Jason Cooper wrote:
> > > > > Applied to irqchip/urgent with Nico's Ack.
> > > > 
> > > > Interesting, so I'm discussing this patch, and it gets applied anyway...
> > > > yes, that's great.
> > > 
> > > Quoting Nico:
> > > 
> > > "Of course it would be good to clarify things wrt Russell's remark
> > > independently from this patch."
> > > 
> > > I took 'independently' to mean "This patch is ok, *and* we need to
> > > address Russell's concerns in a follow-up patch."
> > > 
> > > Nico's Reviewed-by with that comment was sent August 13th.  The most
> > > recent activity on this thread was also August 13th.  After four days, I
> > > reasoned there were no objections to his comment.
> > 
> > Well... I mentioned this patch is a nice cleanup independently of the 
> > reason why it was created in the first place.
> 
> Ah, fair enough.
> 
> > Maybe that shouldn't be sorted as "urgent" in that case, especially
> > when the code having problem with the current state of things is
> > living out of mainline.
> 
> hmmm, yes.  I've been grappling with the semantics of '/urgent' vice
> '/fixes'.  With mvebu, /fixes is the branch for all changes needing to go
> into the current -rcX cycle.  For irqchip, Thomas suggested /urgent for
> the equivalent branch.  To me, they serve the same purpose.
> Unfortunately, I occasionally hear "Well, it's not _urgent_ ...".  I
> suppose I'll put up with it for one more cycle and then change it to
> /fixes. :)
> 
> wrt this patch, I need to drop it anyway.  I was a bit rusty (it's been
> a few weeks) and forgot to add the Cc -stable and Fixes: tags.  I do
> agree, though, it's certainly not urgent.

Given the raised issue has to do with out-of-tree code, there is no need 
to CC stable in that case anyway.


Nicolas

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751671AbaHRByp (ORCPT <rfc822;w@1wt.eu>);
	Sun, 17 Aug 2014 21:54:45 -0400
Received: from mho-02-ewr.mailhop.org ([204.13.248.72]:26126 "EHLO
	mho-02-ewr.mailhop.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751269AbaHRByn (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Sun, 17 Aug 2014 21:54:43 -0400
X-Mail-Handler: Dyn Standard SMTP by Dyn
X-Originating-IP: 96.249.243.124
X-Report-Abuse-To: abuse@dyndns.com (see http://www.dyndns.com/services/sendlabs/outbound_abuse.html for abuse reporting information)
X-MHO-User: U2FsdGVkX1+239uF0yDXSl+gQj6WrPTQT7Yu5vOezlg=
X-DKIM: OpenDKIM Filter v2.0.1 titan CA8EC5C83C8
Date: Sun, 17 Aug 2014 21:54:30 -0400
From: Jason Cooper <jason@lakedaemon.net>
To: Nicolas Pitre <nicolas.pitre@linaro.org>
Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>,
        Stephen Boyd <sboyd@codeaurora.org>, linux-arm-msm@vger.kernel.org,
        Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4] irqchip: gic: Allow gic_arch_extn hooks to call
 into scheduler
Message-ID: <20140818015430.GN12769@titan.lakedaemon.net>
References: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org>
 <20140817173236.GG12769@titan.lakedaemon.net>
 <20140817185522.GX30401@n2100.arm.linux.org.uk>
 <20140817190434.GJ12769@titan.lakedaemon.net>
 <alpine.LFD.2.11.1408172000460.29347@knanqh.ubzr>
 <20140818012512.GL12769@titan.lakedaemon.net>
 <alpine.LFD.2.11.1408172131550.29347@knanqh.ubzr>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.LFD.2.11.1408172131550.29347@knanqh.ubzr>
User-Agent: Mutt/1.5.20 (2009-06-14)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sun, Aug 17, 2014 at 09:35:11PM -0400, Nicolas Pitre wrote:
> On Sun, 17 Aug 2014, Jason Cooper wrote:
> 
> > On Sun, Aug 17, 2014 at 08:04:45PM -0400, Nicolas Pitre wrote:
> > > On Sun, 17 Aug 2014, Jason Cooper wrote:
> > > > On Sun, Aug 17, 2014 at 07:55:23PM +0100, Russell King - ARM Linux wrote:
> > > > > On Sun, Aug 17, 2014 at 01:32:36PM -0400, Jason Cooper wrote:
> > > > > > Applied to irqchip/urgent with Nico's Ack.
> > > > > 
> > > > > Interesting, so I'm discussing this patch, and it gets applied anyway...
> > > > > yes, that's great.
> > > > 
> > > > Quoting Nico:
> > > > 
> > > > "Of course it would be good to clarify things wrt Russell's remark
> > > > independently from this patch."
> > > > 
> > > > I took 'independently' to mean "This patch is ok, *and* we need to
> > > > address Russell's concerns in a follow-up patch."
> > > > 
> > > > Nico's Reviewed-by with that comment was sent August 13th.  The most
> > > > recent activity on this thread was also August 13th.  After four days, I
> > > > reasoned there were no objections to his comment.
> > > 
> > > Well... I mentioned this patch is a nice cleanup independently of the 
> > > reason why it was created in the first place.
> > 
> > Ah, fair enough.
> > 
> > > Maybe that shouldn't be sorted as "urgent" in that case, especially
> > > when the code having problem with the current state of things is
> > > living out of mainline.
> > 
> > hmmm, yes.  I've been grappling with the semantics of '/urgent' vice
> > '/fixes'.  With mvebu, /fixes is the branch for all changes needing to go
> > into the current -rcX cycle.  For irqchip, Thomas suggested /urgent for
> > the equivalent branch.  To me, they serve the same purpose.
> > Unfortunately, I occasionally hear "Well, it's not _urgent_ ...".  I
> > suppose I'll put up with it for one more cycle and then change it to
> > /fixes. :)
> > 
> > wrt this patch, I need to drop it anyway.  I was a bit rusty (it's been
> > a few weeks) and forgot to add the Cc -stable and Fixes: tags.  I do
> > agree, though, it's certainly not urgent.
> 
> Given the raised issue has to do with out-of-tree code, there is no need 
> to CC stable in that case anyway.

I could go either way here.  On the one hand, a fix is a fix is a fix.
On the other, if it can't be triggered in mainline, we shouldn't accept
it at all.

Stephen, is the out of tree code that triggered this bound for mainline?

thx,

Jason.

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751835AbaHRCTF (ORCPT <rfc822;w@1wt.eu>);
	Sun, 17 Aug 2014 22:19:05 -0400
Received: from mail-qg0-f44.google.com ([209.85.192.44]:35128 "EHLO
	mail-qg0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751464AbaHRCTD (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Sun, 17 Aug 2014 22:19:03 -0400
Date: Sun, 17 Aug 2014 22:18:59 -0400 (EDT)
From: Nicolas Pitre <nicolas.pitre@linaro.org>
To: Jason Cooper <jason@lakedaemon.net>
cc: Russell King - ARM Linux <linux@arm.linux.org.uk>,
        Stephen Boyd <sboyd@codeaurora.org>, linux-arm-msm@vger.kernel.org,
        Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4] irqchip: gic: Allow gic_arch_extn hooks to call into
 scheduler
In-Reply-To: <20140818015430.GN12769@titan.lakedaemon.net>
Message-ID: <alpine.LFD.2.11.1408172208490.29347@knanqh.ubzr>
References: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org> <20140817173236.GG12769@titan.lakedaemon.net> <20140817185522.GX30401@n2100.arm.linux.org.uk> <20140817190434.GJ12769@titan.lakedaemon.net> <alpine.LFD.2.11.1408172000460.29347@knanqh.ubzr>
 <20140818012512.GL12769@titan.lakedaemon.net> <alpine.LFD.2.11.1408172131550.29347@knanqh.ubzr> <20140818015430.GN12769@titan.lakedaemon.net>
User-Agent: Alpine 2.11 (LFD 23 2013-08-11)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sun, 17 Aug 2014, Jason Cooper wrote:

> On Sun, Aug 17, 2014 at 09:35:11PM -0400, Nicolas Pitre wrote:
> > On Sun, 17 Aug 2014, Jason Cooper wrote:
> > 
> > > On Sun, Aug 17, 2014 at 08:04:45PM -0400, Nicolas Pitre wrote:
> > > > On Sun, 17 Aug 2014, Jason Cooper wrote:
> > > > > On Sun, Aug 17, 2014 at 07:55:23PM +0100, Russell King - ARM Linux wrote:
> > > > > > On Sun, Aug 17, 2014 at 01:32:36PM -0400, Jason Cooper wrote:
> > > > > > > Applied to irqchip/urgent with Nico's Ack.
> > > > > > 
> > > > > > Interesting, so I'm discussing this patch, and it gets applied anyway...
> > > > > > yes, that's great.
> > > > > 
> > > > > Quoting Nico:
> > > > > 
> > > > > "Of course it would be good to clarify things wrt Russell's remark
> > > > > independently from this patch."
> > > > > 
> > > > > I took 'independently' to mean "This patch is ok, *and* we need to
> > > > > address Russell's concerns in a follow-up patch."
> > > > > 
> > > > > Nico's Reviewed-by with that comment was sent August 13th.  The most
> > > > > recent activity on this thread was also August 13th.  After four days, I
> > > > > reasoned there were no objections to his comment.
> > > > 
> > > > Well... I mentioned this patch is a nice cleanup independently of the 
> > > > reason why it was created in the first place.
> > > 
> > > Ah, fair enough.
> > > 
> > > > Maybe that shouldn't be sorted as "urgent" in that case, especially
> > > > when the code having problem with the current state of things is
> > > > living out of mainline.
> > > 
> > > hmmm, yes.  I've been grappling with the semantics of '/urgent' vice
> > > '/fixes'.  With mvebu, /fixes is the branch for all changes needing to go
> > > into the current -rcX cycle.  For irqchip, Thomas suggested /urgent for
> > > the equivalent branch.  To me, they serve the same purpose.
> > > Unfortunately, I occasionally hear "Well, it's not _urgent_ ...".  I
> > > suppose I'll put up with it for one more cycle and then change it to
> > > /fixes. :)
> > > 
> > > wrt this patch, I need to drop it anyway.  I was a bit rusty (it's been
> > > a few weeks) and forgot to add the Cc -stable and Fixes: tags.  I do
> > > agree, though, it's certainly not urgent.
> > 
> > Given the raised issue has to do with out-of-tree code, there is no need 
> > to CC stable in that case anyway.
> 
> I could go either way here.  On the one hand, a fix is a fix is a fix.
> On the other, if it can't be triggered in mainline, we shouldn't accept
> it at all.

For mainline, it should be accepted as a cleanup and minor optimization 
since no mainline code is currently affected by the absence of this 
patch.

If there is a real bug being fixed by this patch, and whether the best 
way to fix it is by relying on this patch, is still up for debate.

> Stephen, is the out of tree code that triggered this bound for mainline?

Maybe "mainline", but certainly not "stable".


Nicolas

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752159AbaHTTLR (ORCPT <rfc822;w@1wt.eu>);
	Wed, 20 Aug 2014 15:11:17 -0400
Received: from smtp.codeaurora.org ([198.145.11.231]:44414 "EHLO
	smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1750734AbaHTTLP (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 20 Aug 2014 15:11:15 -0400
Message-ID: <53F4F2D2.2050600@codeaurora.org>
Date: Wed, 20 Aug 2014 12:11:14 -0700
From: Stephen Boyd <sboyd@codeaurora.org>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130329 Thunderbird/17.0.5
MIME-Version: 1.0
To: Nicolas Pitre <nicolas.pitre@linaro.org>
CC: Russell King - ARM Linux <linux@arm.linux.org.uk>,
        Jason Cooper <jason@lakedaemon.net>, linux-arm-msm@vger.kernel.org,
        Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4] irqchip: gic: Allow gic_arch_extn hooks to call into
 scheduler
References: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org> <20140817173236.GG12769@titan.lakedaemon.net> <20140817185522.GX30401@n2100.arm.linux.org.uk> <20140817190434.GJ12769@titan.lakedaemon.net> <20140817214123.GY30401@n2100.arm.linux.org.uk> <alpine.LFD.2.11.1408172005070.29347@knanqh.ubzr>
In-Reply-To: <alpine.LFD.2.11.1408172005070.29347@knanqh.ubzr>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 08/17/14 17:17, Nicolas Pitre wrote:
> On Sun, 17 Aug 2014, Russell King - ARM Linux wrote:
>
>> I have no problem with changing gic_raise_softirq() to use a different
>> lock, which gic_migrate_target(), and gic_set_affinity() can also use.
>> There's no need for horrid locking here, because the only thing we're
>> protecting is gic_map[] and the write to the register to trigger an
>> IPI - and nothing using gic_arch_extn has any business knowing about
>> SGIs.
>>
>> No need for these crappy sgi_map_lock() macros and all the ifdeffery.
> Those macros are there only to conditionalize the locking in 
> gic_raise_softirq() because no locking what so ever is needed there when 
> gic_migrate_target() is configured out.  I suggested the macros to cut 
> down on the #ifdefery in the code.

Ok I can resend with the sgi lock around the gic_cpu_map updating code.
Let's see how v5 goes.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation


From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752814AbaHTTQE (ORCPT <rfc822;w@1wt.eu>);
	Wed, 20 Aug 2014 15:16:04 -0400
Received: from smtp.codeaurora.org ([198.145.11.231]:45279 "EHLO
	smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751928AbaHTTQB (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 20 Aug 2014 15:16:01 -0400
Message-ID: <53F4F3F0.20107@codeaurora.org>
Date: Wed, 20 Aug 2014 12:16:00 -0700
From: Stephen Boyd <sboyd@codeaurora.org>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130329 Thunderbird/17.0.5
MIME-Version: 1.0
To: Jason Cooper <jason@lakedaemon.net>
CC: Nicolas Pitre <nicolas.pitre@linaro.org>,
        Russell King - ARM Linux <linux@arm.linux.org.uk>,
        linux-arm-msm@vger.kernel.org, Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4] irqchip: gic: Allow gic_arch_extn hooks to call into
 scheduler
References: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org> <20140817173236.GG12769@titan.lakedaemon.net> <20140817185522.GX30401@n2100.arm.linux.org.uk> <20140817190434.GJ12769@titan.lakedaemon.net> <alpine.LFD.2.11.1408172000460.29347@knanqh.ubzr> <20140818012512.GL12769@titan.lakedaemon.net> <alpine.LFD.2.11.1408172131550.29347@knanqh.ubzr> <20140818015430.GN12769@titan.lakedaemon.net>
In-Reply-To: <20140818015430.GN12769@titan.lakedaemon.net>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 08/17/14 18:54, Jason Cooper wrote:
>
> Stephen, is the out of tree code that triggered this bound for mainline?
>

It's bound for mainline eventually. We're actively working on enabling
more low power modes and when that happens we'll need this patch. We can
always carry this patch for now if you don't want to accept it, but I
figured getting it mainlined reduces our carrying burden and also makes
a minor improvement in sending IPIs when the BL switcher is used.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation


