| Message ID | 20190408194506.25821-1-tony@atomide.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: gpio-omap: take pm_runtime usage while IRQs are claimed | expand |
On 09/04/19 1:15 AM, Tony Lindgren wrote: > From: Russell King <rmk+kernel@armlinux.org.uk> > > Commit b764a5863fd8 ("gpio: omap: Remove custom PM calls and use cpu_pm > instead") moved interrupt using GPIO banks to idle with cpu_pm in order > to drop the use of pm_runtime_irq_safe() in a later patch. The GPIO > banks with no interrupts claimed are still being idled based on PM > runtime calls. However this caused a regression for am437x suspend for > rtc+ddr idle mode as reported by Keerthy <j-keerthy@ti.com>. The fix > to this is: > > Bump the pm_runtime usage count while interrupts are in use, rather > than failing the pm_runtime callbacks. The logic here is a little > non-obvious - the calling order will be: > > omap_gpio_irq_bus_lock() > (optionally) raw_spin_lock_irqsave(desc->lock) > omap_gpio_irq_startup() > (optionally) raw_spin_unlock_irqrestore(desc->lock) > gpio_irq_bus_sync_unlock() > > As the irq_startup method may be called with interrupts disabled, we > must not use pm_runtime_get() here, so as the bus lock takes an initial > pm reference count us, use pm_runtime_get_if_in_use() which will merely > increment the use count. Tony, Applied cleanly on linux-next. I had to manually apply this on your for-next branch. Tested for AM4 RTC+DDR and DS0 back and forth. Regards, Keerthy > > Cc: Aaro Koskinen <aaro.koskinen@iki.fi> > Cc: Grygorii Strashko <grygorii.strashko@ti.com> > Cc: Keerthy <j-keerthy@ti.com> > Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> > Cc: Tero Kristo <t-kristo@ti.com> > Fixes: b764a5863fd8 ("gpio: omap: Remove custom PM calls and use cpu_pm instead") > Reported-by: Keerthy <j-keerthy@ti.com> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > [tony@atomide.com: updated patch description for regression fix] > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > > AFAIK this is only needed with patches heading to v5.2, so this is based on > Linux next. We can always backport as needed. > > --- > drivers/gpio/gpio-omap.c | 23 +++++++---------------- > 1 file changed, 7 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -811,6 +811,9 @@ static unsigned int omap_gpio_irq_startup(struct irq_data *d) > unsigned long flags; > unsigned offset = d->hwirq; > > + /* Take a reference on the runtime PM to prevent RPM suspends */ > + WARN_ON(pm_runtime_get_if_in_use(bank->chip.parent) == 0); > + > raw_spin_lock_irqsave(&bank->lock, flags); > > if (!LINE_USED(bank->mod_usage, offset)) > @@ -844,6 +847,8 @@ static void omap_gpio_irq_shutdown(struct irq_data *d) > omap_clear_gpio_debounce(bank, offset); > omap_disable_gpio_module(bank, offset); > raw_spin_unlock_irqrestore(&bank->lock, flags); > + > + pm_runtime_put(bank->chip.parent); > } > > static void omap_gpio_irq_bus_lock(struct irq_data *data) > @@ -1719,40 +1724,26 @@ static int __maybe_unused omap_gpio_runtime_suspend(struct device *dev) > { > struct gpio_bank *bank = dev_get_drvdata(dev); > unsigned long flags; > - int error = 0; > > raw_spin_lock_irqsave(&bank->lock, flags); > - /* Must be idled only by CPU_CLUSTER_PM_ENTER? */ > - if (bank->irq_usage) { > - error = -EBUSY; > - goto unlock; > - } > omap_gpio_idle(bank, true); > bank->is_suspended = true; > -unlock: > raw_spin_unlock_irqrestore(&bank->lock, flags); > > - return error; > + return 0; > } > > static int __maybe_unused omap_gpio_runtime_resume(struct device *dev) > { > struct gpio_bank *bank = dev_get_drvdata(dev); > unsigned long flags; > - int error = 0; > > raw_spin_lock_irqsave(&bank->lock, flags); > - /* Must be unidled only by CPU_CLUSTER_PM_ENTER? */ > - if (bank->irq_usage) { > - error = -EBUSY; > - goto unlock; > - } > omap_gpio_unidle(bank); > bank->is_suspended = false; > -unlock: > raw_spin_unlock_irqrestore(&bank->lock, flags); > > - return error; > + return 0; > } > > static const struct dev_pm_ops gpio_pm_ops = { >
On 09.04.19 07:36, Keerthy wrote: > > > On 09/04/19 1:15 AM, Tony Lindgren wrote: >> From: Russell King <rmk+kernel@armlinux.org.uk> >> >> Commit b764a5863fd8 ("gpio: omap: Remove custom PM calls and use cpu_pm >> instead") moved interrupt using GPIO banks to idle with cpu_pm in order >> to drop the use of pm_runtime_irq_safe() in a later patch. The GPIO >> banks with no interrupts claimed are still being idled based on PM >> runtime calls. However this caused a regression for am437x suspend for >> rtc+ddr idle mode as reported by Keerthy <j-keerthy@ti.com>. The fix >> to this is: >> >> Bump the pm_runtime usage count while interrupts are in use, rather >> than failing the pm_runtime callbacks. The logic here is a little >> non-obvious - the calling order will be: >> >> omap_gpio_irq_bus_lock() >> (optionally) raw_spin_lock_irqsave(desc->lock) >> omap_gpio_irq_startup() >> (optionally) raw_spin_unlock_irqrestore(desc->lock) >> gpio_irq_bus_sync_unlock() >> >> As the irq_startup method may be called with interrupts disabled, we >> must not use pm_runtime_get() here, so as the bus lock takes an initial >> pm reference count us, use pm_runtime_get_if_in_use() which will merely >> increment the use count. > > Tony, > > Applied cleanly on linux-next. I had to manually apply this on your for-next branch. Tested for AM4 RTC+DDR and DS0 back and forth. I'm very sorry, but what was the regression exactly? Can't enter RTC+DDR state? some crash? > > Regards, > Keerthy >> >> Cc: Aaro Koskinen <aaro.koskinen@iki.fi> >> Cc: Grygorii Strashko <grygorii.strashko@ti.com> >> Cc: Keerthy <j-keerthy@ti.com> >> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> >> Cc: Tero Kristo <t-kristo@ti.com> >> Fixes: b764a5863fd8 ("gpio: omap: Remove custom PM calls and use cpu_pm instead") >> Reported-by: Keerthy <j-keerthy@ti.com> >> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> >> [tony@atomide.com: updated patch description for regression fix] >> Signed-off-by: Tony Lindgren <tony@atomide.com> >> --- >> >> AFAIK this is only needed with patches heading to v5.2, so this is based on >> Linux next. We can always backport as needed. >> >> --- >> drivers/gpio/gpio-omap.c | 23 +++++++---------------- >> 1 file changed, 7 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c >> --- a/drivers/gpio/gpio-omap.c >> +++ b/drivers/gpio/gpio-omap.c >> @@ -811,6 +811,9 @@ static unsigned int omap_gpio_irq_startup(struct irq_data *d) >> unsigned long flags; >> unsigned offset = d->hwirq; >> + /* Take a reference on the runtime PM to prevent RPM suspends */ >> + WARN_ON(pm_runtime_get_if_in_use(bank->chip.parent) == 0); >> + We have in driver: irqc->parent_device = dev; which means: request_threaded_irq() irq_chip_pm_get() if (IS_ENABLED(CONFIG_PM) && data->chip->parent_device) { retval = pm_runtime_get_sync(data->chip->parent_device); and power.usage_count will be incremented every time GPIO irq is requested. only in free_irq() (or in case of error) power.usage_count is decremented. Now above change will introduce just another incrementation of power.usage_count How is it helping? >> raw_spin_lock_irqsave(&bank->lock, flags); >> if (!LINE_USED(bank->mod_usage, offset)) >> @@ -844,6 +847,8 @@ static void omap_gpio_irq_shutdown(struct irq_data *d) >> omap_clear_gpio_debounce(bank, offset); >> omap_disable_gpio_module(bank, offset); >> raw_spin_unlock_irqrestore(&bank->lock, flags); >> + >> + pm_runtime_put(bank->chip.parent); >> } >> static void omap_gpio_irq_bus_lock(struct irq_data *data) >> @@ -1719,40 +1724,26 @@ static int __maybe_unused omap_gpio_runtime_suspend(struct device *dev) >> { >> struct gpio_bank *bank = dev_get_drvdata(dev); >> unsigned long flags; >> - int error = 0; >> raw_spin_lock_irqsave(&bank->lock, flags); >> - /* Must be idled only by CPU_CLUSTER_PM_ENTER? */ >> - if (bank->irq_usage) { >> - error = -EBUSY; >> - goto unlock; >> - } >> omap_gpio_idle(bank, true); >> bank->is_suspended = true; >> -unlock: >> raw_spin_unlock_irqrestore(&bank->lock, flags); >> - return error; >> + return 0; >> } >> static int __maybe_unused omap_gpio_runtime_resume(struct device *dev) >> { >> struct gpio_bank *bank = dev_get_drvdata(dev); >> unsigned long flags; >> - int error = 0; >> raw_spin_lock_irqsave(&bank->lock, flags); >> - /* Must be unidled only by CPU_CLUSTER_PM_ENTER? */ >> - if (bank->irq_usage) { >> - error = -EBUSY; >> - goto unlock; >> - } >> omap_gpio_unidle(bank); >> bank->is_suspended = false; >> -unlock: >> raw_spin_unlock_irqrestore(&bank->lock, flags); >> - return error; >> + return 0; >> } >> static const struct dev_pm_ops gpio_pm_ops = { >>
* Grygorii Strashko <grygorii.strashko@ti.com> [190410 18:17]: > I'm very sorry, but what was the regression exactly? > Can't enter RTC+DDR state? some crash? AFAIK fails to enter RTC+DDR suspend because of conditional PM runtime handling. > We have in driver: > irqc->parent_device = dev; > > which means: > request_threaded_irq() > irq_chip_pm_get() > if (IS_ENABLED(CONFIG_PM) && data->chip->parent_device) { > retval = pm_runtime_get_sync(data->chip->parent_device); > > and power.usage_count will be incremented every time GPIO irq is requested. > > only in free_irq() (or in case of error) power.usage_count is decremented. > > Now above change will introduce just another incrementation of power.usage_count > How is it helping? Oh OK, that means we can simplify things further and drop those changes then. I'll post v2 version shortly with updated subject and description. Regards, Tony
On Wed, Apr 10, 2019 at 09:16:28PM +0300, Grygorii Strashko wrote: > > > On 09.04.19 07:36, Keerthy wrote: > > > > > > On 09/04/19 1:15 AM, Tony Lindgren wrote: > >> From: Russell King <rmk+kernel@armlinux.org.uk> > >> > >> Commit b764a5863fd8 ("gpio: omap: Remove custom PM calls and use cpu_pm > >> instead") moved interrupt using GPIO banks to idle with cpu_pm in order > >> to drop the use of pm_runtime_irq_safe() in a later patch. The GPIO > >> banks with no interrupts claimed are still being idled based on PM > >> runtime calls. However this caused a regression for am437x suspend for > >> rtc+ddr idle mode as reported by Keerthy <j-keerthy@ti.com>. The fix > >> to this is: > >> > >> Bump the pm_runtime usage count while interrupts are in use, rather > >> than failing the pm_runtime callbacks. The logic here is a little > >> non-obvious - the calling order will be: > >> > >> omap_gpio_irq_bus_lock() > >> (optionally) raw_spin_lock_irqsave(desc->lock) > >> omap_gpio_irq_startup() > >> (optionally) raw_spin_unlock_irqrestore(desc->lock) > >> gpio_irq_bus_sync_unlock() > >> > >> As the irq_startup method may be called with interrupts disabled, we > >> must not use pm_runtime_get() here, so as the bus lock takes an initial > >> pm reference count us, use pm_runtime_get_if_in_use() which will merely > >> increment the use count. > > > > Tony, > > > > Applied cleanly on linux-next. I had to manually apply this on your for-next branch. Tested for AM4 RTC+DDR and DS0 back and forth. > > I'm very sorry, but what was the regression exactly? > Can't enter RTC+DDR state? some crash? > > > > > > Regards, > > Keerthy > >> > >> Cc: Aaro Koskinen <aaro.koskinen@iki.fi> > >> Cc: Grygorii Strashko <grygorii.strashko@ti.com> > >> Cc: Keerthy <j-keerthy@ti.com> > >> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> > >> Cc: Tero Kristo <t-kristo@ti.com> > >> Fixes: b764a5863fd8 ("gpio: omap: Remove custom PM calls and use cpu_pm instead") > >> Reported-by: Keerthy <j-keerthy@ti.com> > >> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > >> [tony@atomide.com: updated patch description for regression fix] > >> Signed-off-by: Tony Lindgren <tony@atomide.com> > >> --- > >> > >> AFAIK this is only needed with patches heading to v5.2, so this is based on > >> Linux next. We can always backport as needed. > >> > >> --- > >> drivers/gpio/gpio-omap.c | 23 +++++++---------------- > >> 1 file changed, 7 insertions(+), 16 deletions(-) > >> > >> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > >> --- a/drivers/gpio/gpio-omap.c > >> +++ b/drivers/gpio/gpio-omap.c > >> @@ -811,6 +811,9 @@ static unsigned int omap_gpio_irq_startup(struct irq_data *d) > >> unsigned long flags; > >> unsigned offset = d->hwirq; > >> + /* Take a reference on the runtime PM to prevent RPM suspends */ > >> + WARN_ON(pm_runtime_get_if_in_use(bank->chip.parent) == 0); > >> + > > > > We have in driver: > irqc->parent_device = dev; > > which means: > request_threaded_irq() > irq_chip_pm_get() > if (IS_ENABLED(CONFIG_PM) && data->chip->parent_device) { > retval = pm_runtime_get_sync(data->chip->parent_device); > > and power.usage_count will be incremented every time GPIO irq is requested. > > only in free_irq() (or in case of error) power.usage_count is decremented. > > Now above change will introduce just another incrementation of power.usage_count > How is it helping? I really don't remember - too long ago, but I guess there _was_ a reason for the patch. It seems that the patch was created with your: commit 467480738d0b33335032652b29776d82200db41a Author: Grygorii Strashko <grygorii.strashko@ti.com> Date: Fri Sep 28 16:39:50 2018 -0500 change which added the setting of parent_device. So, in short, I've no idea about the background behind the patch now. > > >> raw_spin_lock_irqsave(&bank->lock, flags); > >> if (!LINE_USED(bank->mod_usage, offset)) > >> @@ -844,6 +847,8 @@ static void omap_gpio_irq_shutdown(struct irq_data *d) > >> omap_clear_gpio_debounce(bank, offset); > >> omap_disable_gpio_module(bank, offset); > >> raw_spin_unlock_irqrestore(&bank->lock, flags); > >> + > >> + pm_runtime_put(bank->chip.parent); > >> } > >> static void omap_gpio_irq_bus_lock(struct irq_data *data) > >> @@ -1719,40 +1724,26 @@ static int __maybe_unused omap_gpio_runtime_suspend(struct device *dev) > >> { > >> struct gpio_bank *bank = dev_get_drvdata(dev); > >> unsigned long flags; > >> - int error = 0; > >> raw_spin_lock_irqsave(&bank->lock, flags); > >> - /* Must be idled only by CPU_CLUSTER_PM_ENTER? */ > >> - if (bank->irq_usage) { > >> - error = -EBUSY; > >> - goto unlock; > >> - } > >> omap_gpio_idle(bank, true); > >> bank->is_suspended = true; > >> -unlock: > >> raw_spin_unlock_irqrestore(&bank->lock, flags); > >> - return error; > >> + return 0; > >> } > >> static int __maybe_unused omap_gpio_runtime_resume(struct device *dev) > >> { > >> struct gpio_bank *bank = dev_get_drvdata(dev); > >> unsigned long flags; > >> - int error = 0; > >> raw_spin_lock_irqsave(&bank->lock, flags); > >> - /* Must be unidled only by CPU_CLUSTER_PM_ENTER? */ > >> - if (bank->irq_usage) { > >> - error = -EBUSY; > >> - goto unlock; > >> - } > >> omap_gpio_unidle(bank); > >> bank->is_suspended = false; > >> -unlock: > >> raw_spin_unlock_irqrestore(&bank->lock, flags); > >> - return error; > >> + return 0; > >> } > >> static const struct dev_pm_ops gpio_pm_ops = { > >> > > -- > Best regards, > grygorii >
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -811,6 +811,9 @@ static unsigned int omap_gpio_irq_startup(struct irq_data *d) unsigned long flags; unsigned offset = d->hwirq; + /* Take a reference on the runtime PM to prevent RPM suspends */ + WARN_ON(pm_runtime_get_if_in_use(bank->chip.parent) == 0); + raw_spin_lock_irqsave(&bank->lock, flags); if (!LINE_USED(bank->mod_usage, offset)) @@ -844,6 +847,8 @@ static void omap_gpio_irq_shutdown(struct irq_data *d) omap_clear_gpio_debounce(bank, offset); omap_disable_gpio_module(bank, offset); raw_spin_unlock_irqrestore(&bank->lock, flags); + + pm_runtime_put(bank->chip.parent); } static void omap_gpio_irq_bus_lock(struct irq_data *data) @@ -1719,40 +1724,26 @@ static int __maybe_unused omap_gpio_runtime_suspend(struct device *dev) { struct gpio_bank *bank = dev_get_drvdata(dev); unsigned long flags; - int error = 0; raw_spin_lock_irqsave(&bank->lock, flags); - /* Must be idled only by CPU_CLUSTER_PM_ENTER? */ - if (bank->irq_usage) { - error = -EBUSY; - goto unlock; - } omap_gpio_idle(bank, true); bank->is_suspended = true; -unlock: raw_spin_unlock_irqrestore(&bank->lock, flags); - return error; + return 0; } static int __maybe_unused omap_gpio_runtime_resume(struct device *dev) { struct gpio_bank *bank = dev_get_drvdata(dev); unsigned long flags; - int error = 0; raw_spin_lock_irqsave(&bank->lock, flags); - /* Must be unidled only by CPU_CLUSTER_PM_ENTER? */ - if (bank->irq_usage) { - error = -EBUSY; - goto unlock; - } omap_gpio_unidle(bank); bank->is_suspended = false; -unlock: raw_spin_unlock_irqrestore(&bank->lock, flags); - return error; + return 0; } static const struct dev_pm_ops gpio_pm_ops = {