linux-kernel - Re: [GIT pull] irq updates for 4.13

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 11 Jul 2017 10:39:34 -0500
From:   Grygorii Strashko <grygorii.strashko@...com>
To:     Thomas Gleixner <tglx@...utronix.de>,
        Tony Lindgren <tony@...mide.com>
CC:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Sebastian Reichel <sebastian.reichel@...labora.co.uk>,
        LKML <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Ingo Molnar <mingo@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>, Pavel Machek <pavel@....cz>,
        Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [GIT pull] irq updates for 4.13



On 07/11/2017 09:41 AM, Thomas Gleixner wrote:
> On Tue, 11 Jul 2017, Tony Lindgren wrote:
>> * Thomas Gleixner <tglx@...utronix.de> [170711 02:48]:
>> And "external abort on non-linefetch" means something is not clocked
>> in this case. The following alone makes things boot for me again, but I don't
>> quite follow what has now changed with the ordering.. Thomas, any ideas?
> 
> Ah. Now that makes sense.
> 
> Unpatched the ordering is:
> 
> 	  chip_bus_lock(desc);
> 	  irq_request_resources(desc);
> 
> Now the offending change reordered the calls. OMAP gpio has:
> 
>      omap_gpio_irq_bus_lock()
>         pm_runtime_get_sync(bank->chip.parent);
> 
> So that at least explains the error. So that omap gpio irq chip (ab)uses
> the bus_lock() callback to do runtime power management. Sigh, I did not
> expect that. Let me have a deeper look if that's OMAP only or whether this
> happens in other places as well.

It was the only one way to power on GPIO bank when the first GPIO IRQ is requested,
as all other irqchip callbacks are under raw_lock while pm_runtime uses spinlock, as
result on -RT it was not possible to use PM runtime in other irqchip callbacks.

Now, I think, It might be possible to use irq_chip_pm_get(), but there is one problem -
OMAP Power management platform code can call omap2_gpio_prepare_for_idle()/omap2_gpio_resume_after_idle()
which expected to disable GPIO banks using PM runtime and current driver implementation
expect to have PM runtime usage_count = 1.

Tony, Potentially we can use pm_runtime_force_suspend()/resume() there, but they are not compatible with
irqoff context (CPUIdle late stages).

In other words, below patch should fix this issue, but will break CPUIdle on OMAP :(

--
>From dfca1c806f03ad6bdd72b634d71c96d39bda2046 Mon Sep 17 00:00:00 2001
From: Grygorii Strashko <grygorii.strashko@...com>
Date: Tue, 11 Jul 2017 10:36:23 -0500
Subject: [PATCH] gpio: omap: switch to use irq_chip_pm_get/put()

Signed-off-by: Grygorii Strashko <grygorii.strashko@...com>
---
 drivers/gpio/gpio-omap.c | 23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index ba58c8b..b614475 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -787,26 +787,6 @@ static void omap_gpio_irq_shutdown(struct irq_data *d)
 	raw_spin_unlock_irqrestore(&bank->lock, flags);
 }
 
-static void omap_gpio_irq_bus_lock(struct irq_data *data)
-{
-	struct gpio_bank *bank = omap_irq_data_get_bank(data);
-
-	if (!BANK_USED(bank))
-		pm_runtime_get_sync(bank->chip.parent);
-}
-
-static void gpio_irq_bus_sync_unlock(struct irq_data *data)
-{
-	struct gpio_bank *bank = omap_irq_data_get_bank(data);
-
-	/*
-	 * If this is the last IRQ to be freed in the bank,
-	 * disable the bank module.
-	 */
-	if (!BANK_USED(bank))
-		pm_runtime_put(bank->chip.parent);
-}
-
 static void omap_gpio_ack_irq(struct irq_data *d)
 {
 	struct gpio_bank *bank = omap_irq_data_get_bank(d);
@@ -1168,10 +1148,9 @@ static int omap_gpio_probe(struct platform_device *pdev)
 	irqc->irq_unmask = omap_gpio_unmask_irq,
 	irqc->irq_set_type = omap_gpio_irq_type,
 	irqc->irq_set_wake = omap_gpio_wake_enable,
-	irqc->irq_bus_lock = omap_gpio_irq_bus_lock,
-	irqc->irq_bus_sync_unlock = gpio_irq_bus_sync_unlock,
 	irqc->name = dev_name(&pdev->dev);
 	irqc->flags = IRQCHIP_MASK_ON_SUSPEND;
+	irqc->parent_device = dev;
 
 	bank->irq = platform_get_irq(pdev, 0);
 	if (bank->irq <= 0) {
-- 
2.10.1


-- 
regards,
-grygorii