Re: [PATCH] gpio: gpio-omap: take pm_runtime usage while IRQs are claimed

[Russell King - ARM Linux admin <linux@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxx>
> >>
> >> 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@xxxxxx>. 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@xxxxxx>
> >> Cc: Grygorii Strashko <grygorii.strashko@xxxxxx>
> >> Cc: Keerthy <j-keerthy@xxxxxx>
> >> Cc: Peter Ujfalusi <peter.ujfalusi@xxxxxx>
> >> Cc: Tero Kristo <t-kristo@xxxxxx>
> >> Fixes: b764a5863fd8 ("gpio: omap: Remove custom PM calls and use cpu_pm instead")
> >> Reported-by: Keerthy <j-keerthy@xxxxxx>
> >> Signed-off-by: Russell King <rmk+kernel@xxxxxxxxxxxxxxx>
> >> [tony@xxxxxxxxxxx: updated patch description for regression fix]
> >> Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
> >> ---
> >>
> >> 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@xxxxxx>
  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
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up