gpio: omap: Fix PM runtime issue and remove most BANK_USED macros

Looks like omap_gpio_irq_type can return early at several places leaving a GPIO bank enabled without doing pm_runtime_put if wrong GPIO arguments are passed.

Instead of adding more complicated BANK_USED macros, let's fix the issue properly. We can pass is_irq flag to omap_enable_gpio_module and omap_disble_gpio_module. And with that we can remove all the similar code elsewhere to get rid of most BANK_USED macros.

Note that the reason for the BANK_USED macro is that we need to manage PM runtime on per GPIO bank basis. In the long run we want to move to using PM runtime counts for each GPIO line to determine if a GPIO bank is used. Once we have a solution for omap_enable_gpio_module and omap_disable_gpio_module, we can remove the remaining BANK_USED macros.

Cc Felipe Balbi <[email protected]>

Cc Grygorii Strashko <[email protected]>

Cc Javier Martinez Canillas <[email protected]>

Cc Nishanth Menon <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

Looks like omap_gpio_irq_type can return early at several places leaving a GPIO bank enabled without doing pm_runtime_put if wrong GPIO arguments are passed.

Instead of adding more complicated BANK_USED macros, let's fix the issue properly. We can pass is_irq flag to omap_enable_gpio_module and omap_disble_gpio_module. And with that we can remove all the similar code elsewhere to get rid of most BANK_USED macros.

Note that the reason for the BANK_USED macro is that we need to manage PM runtime on per GPIO bank basis. In the long run we want to move to using PM runtime counts for each GPIO line to determine if a GPIO bank is used. Once we have a solution for omap_enable_gpio_module and omap_disable_gpio_module, we can remove the remaining BANK_USED macros.

Cc Felipe Balbi <[email protected]>

Cc Grygorii Strashko <[email protected]>

Cc Javier Martinez Canillas <[email protected]>

Cc Nishanth Menon <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
  drivers/gpio/gpio-omap.c | 111 +++++++++++++++++------------------------------
  1 file changed, 40 insertions(+), 71 deletions(-)

drivers/gpio/gpio-omap.c +10 -1

index d44e617..39a6312 100644
@@ -86,6 +86,7 @@ struct gpio_bank {
  #define BANK_USED(bank) (bank->mod_usage || bank->irq_usage)
  #define LINE_USED(line, offset) (line & (BIT(offset)))
 
+static void omap_reset_gpio(struct gpio_bank *bank, unsigned offset);
  static void omap_gpio_unmask_irq(struct irq_data *d);
 
  static inline struct gpio_bank *omap_irq_data_get_bank(struct irq_data *d)

@@ -419,8 +420,16 @@ static int omap_set_gpio_triggering(struct gpio_bank *bank, int gpio,
  	return 0;
  }
 
-static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset)
+static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset,
+				    bool is_irq)
  {
+	unsigned long flags;
+
+	/* PM runtime is per bank, not per GPIO line */
+	if (!BANK_USED(bank))
+		pm_runtime_get_sync(bank->dev);
+
+	spin_lock_irqsave(&bank->lock, flags);
  	if (bank->regs->pinctrl) {
  		void __iomem *reg = bank->base + bank->regs->pinctrl;
 

@@ -438,11 +447,30 @@ static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset)
  		writel_relaxed(ctrl, reg);

drivers/gpio/gpio-omap.c +22 -70

+	spin_unlock_irqrestore(&bank->lock, flags);
  }
 
-static void omap_disable_gpio_module(struct gpio_bank *bank, unsigned offset)
+static void omap_disable_gpio_module(struct gpio_bank *bank, unsigned offset,
+				     bool is_irq)
  {
  	void __iomem *base = bank->base;
+	unsigned long flags;
+
+	spin_lock_irqsave(&bank->lock, flags);
+	if (is_irq)
+		bank->irq_usage &= ~(BIT(offset));
+	else
+		bank->mod_usage &= ~(BIT(offset));
+
+	omap_reset_gpio(bank, offset);
 
  	if (bank->regs->wkup_en &&
  	    !LINE_USED(bank->mod_usage, offset) &&

@@ -463,6 +491,11 @@ static void omap_disable_gpio_module(struct gpio_bank *bank, unsigned offset)
  		writel_relaxed(ctrl, reg);
  		bank->context.ctrl = ctrl;
  	}
+	spin_unlock_irqrestore(&bank->lock, flags);
+
+	/* PM runtime is per bank, not per GPIO line */
+	if (!BANK_USED(bank))
+		pm_runtime_put(bank->dev);
  }
 
  static int omap_gpio_is_input(struct gpio_bank *bank, unsigned offset)

@@ -472,15 +505,6 @@ static int omap_gpio_is_input(struct gpio_bank *bank, unsigned offset)
  	return readl_relaxed(reg) & BIT(offset);
  }
 
-static void omap_gpio_init_irq(struct gpio_bank *bank, unsigned offset)
-{
-	if (!LINE_USED(bank->mod_usage, offset)) {
-		omap_enable_gpio_module(bank, offset);
-		omap_set_gpio_direction(bank, offset, 1);
-	}
-	bank->irq_usage |= BIT(offset);
-}
-
  static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
  {
  	struct gpio_bank *bank = omap_irq_data_get_bank(d);

@@ -488,9 +512,6 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
  	unsigned long flags;
  	unsigned offset = d->hwirq;
 
-	if (!BANK_USED(bank))
-		pm_runtime_get_sync(bank->dev);
-
  	if (type & ~IRQ_TYPE_SENSE_MASK)
  		return -EINVAL;
 

@@ -498,13 +519,9 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
  		(type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH)))
  		return -EINVAL;
 
+	omap_enable_gpio_module(bank, offset, true);
  	spin_lock_irqsave(&bank->lock, flags);
  	retval = omap_set_gpio_triggering(bank, offset, type);
-	omap_gpio_init_irq(bank, offset);
-	if (!omap_gpio_is_input(bank, offset)) {
-		spin_unlock_irqrestore(&bank->lock, flags);
-		return -EINVAL;
-	}
  	spin_unlock_irqrestore(&bank->lock, flags);
 
  	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))

@@ -659,26 +676,8 @@ static int omap_gpio_wake_enable(struct irq_data *d, unsigned int enable)
  static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
  {
  	struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
-	unsigned long flags;
 
-	/*
-	 * If this is the first gpio_request for the bank,
-	 * enable the bank module.
-	 */
-	if (!BANK_USED(bank))
-		pm_runtime_get_sync(bank->dev);
-
-	spin_lock_irqsave(&bank->lock, flags);
-	/* Set trigger to none. You need to enable the desired trigger with
-	 * request_irq() or set_irq_type(). Only do this if the IRQ line has
-	 * not already been requested.
-	 */
-	if (!LINE_USED(bank->irq_usage, offset)) {
-		omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
-		omap_enable_gpio_module(bank, offset);
-	}
-	bank->mod_usage |= BIT(offset);
-	spin_unlock_irqrestore(&bank->lock, flags);
+	omap_enable_gpio_module(bank, offset, false);
 
  	return 0;
  }

@@ -686,20 +685,8 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
  static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
  {
  	struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
-	unsigned long flags;
-
-	spin_lock_irqsave(&bank->lock, flags);
-	bank->mod_usage &= ~(BIT(offset));
-	omap_disable_gpio_module(bank, offset);
-	omap_reset_gpio(bank, offset);
-	spin_unlock_irqrestore(&bank->lock, flags);
 
-	/*
-	 * If this is the last gpio to be freed in the bank,
-	 * disable the bank module.
-	 */
-	if (!BANK_USED(bank))
-		pm_runtime_put(bank->dev);
+	omap_disable_gpio_module(bank, offset, false);
  }
 
  /*

@@ -788,15 +775,9 @@ exit:
  static unsigned int omap_gpio_irq_startup(struct irq_data *d)
  {
  	struct gpio_bank *bank = omap_irq_data_get_bank(d);
-	unsigned long flags;
  	unsigned offset = d->hwirq;
 
-	if (!BANK_USED(bank))
-		pm_runtime_get_sync(bank->dev);
-
-	spin_lock_irqsave(&bank->lock, flags);
-	omap_gpio_init_irq(bank, offset);
-	spin_unlock_irqrestore(&bank->lock, flags);
+	omap_enable_gpio_module(bank, offset, true);
  	omap_gpio_unmask_irq(d);
 
  	return 0;

@@ -805,21 +786,9 @@ static unsigned int omap_gpio_irq_startup(struct irq_data *d)
  static void omap_gpio_irq_shutdown(struct irq_data *d)
  {
  	struct gpio_bank *bank = omap_irq_data_get_bank(d);
-	unsigned long flags;
  	unsigned offset = d->hwirq;
 
-	spin_lock_irqsave(&bank->lock, flags);
-	bank->irq_usage &= ~(BIT(offset));
-	omap_disable_gpio_module(bank, offset);
-	omap_reset_gpio(bank, offset);
-	spin_unlock_irqrestore(&bank->lock, flags);
-
-	/*
-	 * If this is the last IRQ to be freed in the bank,
-	 * disable the bank module.
-	 */
-	if (!BANK_USED(bank))
-		pm_runtime_put(bank->dev);
+	omap_disable_gpio_module(bank, offset, true);
  }
 
  static void omap_gpio_ack_irq(struct irq_data *d)

drivers/gpio/gpio-omap.c static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset) · line 457

Quoted 12 lines · Tony Lindgren

drivers/gpio/gpio-omap.c +8 -0

@@ -438,11 +447,30 @@ static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset)
  		writel_relaxed(ctrl, reg);
  		bank->context.ctrl = ctrl;
  	}
+
+	if (is_irq) {
+		omap_set_gpio_direction(bank, offset, 1);
+		bank->irq_usage |= BIT(offset);
+	} else {
+		omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
+		bank->mod_usage |= BIT(offset);
+	}

The OMAP GPIO driver implements two Core interfaces IRQ-chip and GPIO-chip which, in general, more or less independent.

So, I don't think, that it's good to mix GPIO-IRQ-chip specific code with GPIO-chip code. And this even don't really correspond the purpose of omap_enable_gpio_module() :( and might introduce misunderstanding of code. The worst thing is that future fixes in IRQ-chip may affect on on GPIO-chip and vise versa :(

Could we keep omap_xxx_gpio_module() functions responsible only for GPIO bank PM and enabling/disabling?

Quoted 17 lines · Grygorii.Strashko at linaro.org

drivers/gpio/gpio-omap.c static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset) · line 457

Quoted 12 lines · Tony Lindgren

drivers/gpio/gpio-omap.c +8 -0

@@ -438,11 +447,30 @@ static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset)
   		writel_relaxed(ctrl, reg);
   		bank->context.ctrl = ctrl;
   	}
+
+	if (is_irq) {
+		omap_set_gpio_direction(bank, offset, 1);
+		bank->irq_usage |= BIT(offset);
+	} else {
+		omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
+		bank->mod_usage |= BIT(offset);
+	}

The OMAP GPIO driver implements two Core interfaces IRQ-chip and GPIO-chip which, in general, more or less independent.

Quoted 4 lines

So, I don't think, that it's good to mix GPIO-IRQ-chip specific code with GPIO-chip code. And this even don't really correspond the purpose of omap_enable_gpio_module() :( and might introduce misunderstanding of code. The worst thing is that future fixes in IRQ-chip may affect on on GPIO-chip and vise versa :(

Hmm I'm thinking omap_enable/disable_gpio_module() eventually becomes our runtime_resume/suspend(). Currently the enabling and disabling is buggy for GPIO for some corner cases.. AFAIK the only difference between

enabling GPIO vs GPIO-IRQ is the calling of omap_set_gpio_direction vs omap_set_gpio_triggering. Or at least that's the way it should be unless I'm missing something?

Quoted 3 lines

Could we keep omap_xxx_gpio_module() functions responsible only for GPIO bank PM and enabling/disabling?

If you're thinking about just thinking about having separate wrappers around it like this::

static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset,
bool is_irq)

{
	...
}

static void omap_enable_gpio((struct gpio_bank *bank, unsigned offset)

{
	omap_enable_gpio_module(bpio_bank, offset, 0);
}

static void omap_enable_gpio_irq((struct gpio_bank *bank, unsigned offset)

{
	omap_enable_gpio_module(bpio_bank, offset, 1);
}

Then yes makes sense to me. Or do you have something else in mind?

Hi Tony,

Sorry for delayed reply.

Quoted 23 lines · Tony Lindgren

Quoted 21 lines · Grygorii.Strashko at linaro.org

Review static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset) · line 457

Quoted 12 lines · Tony Lindgren

(quoted diff) +8 -0

@@ -438,11 +447,30 @@ static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset)
   		writel_relaxed(ctrl, reg);
   		bank->context.ctrl = ctrl;
   	}
+
+	if (is_irq) {
+		omap_set_gpio_direction(bank, offset, 1);
+		bank->irq_usage |= BIT(offset);
+	} else {
+		omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
+		bank->mod_usage |= BIT(offset);
+	}

The OMAP GPIO driver implements two Core interfaces IRQ-chip and GPIO-chip which, in general, more or less independent.

So, I don't think, that it's good to mix GPIO-IRQ-chip specific code with GPIO-chip code. And this even don't really correspond the purpose of omap_enable_gpio_module() :( and might introduce misunderstanding of code. The worst thing is that future fixes in IRQ-chip may affect on on GPIO-chip and vise versa :(

Quoted 3 lines

Hmm I'm thinking omap_enable/disable_gpio_module() eventually becomes our runtime_resume/suspend(). Currently the enabling and disabling is buggy for GPIO for some corner cases.. AFAIK the only difference between

It might be very helpful if you'd able to share additional info about any "corner cases" you know.

Quoted 3 lines

enabling GPIO vs GPIO-IRQ is the calling of omap_set_gpio_direction vs omap_set_gpio_triggering. Or at least that's the way it should be unless I'm missing something?

I think yes. what i'd like to say, first of all, is that it might be not good idea to mix too much functionality in omap_enable/disable_gpio_module() - especially GPIO-IRQ vs GPIO-chip ( Very easy to get lost ;)

For example (1) - your change of omap_gpio_request() looks like invalid:

  static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
  {
  	struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);

(quoted diff) +0 -16

-	unsigned long flags;
 
-	/*
-	 * If this is the first gpio_request for the bank,
-	 * enable the bank module.
-	 */
-	if (!BANK_USED(bank))
-		pm_runtime_get_sync(bank->dev);
-
-	spin_lock_irqsave(&bank->lock, flags);
-	/* Set trigger to none. You need to enable the desired trigger with
-	 * request_irq() or set_irq_type(). Only do this if the IRQ line has
-	 * not already been requested.
-	 */
-	if (!LINE_USED(bank->irq_usage, offset)) {
-		omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
-		omap_enable_gpio_module(bank, offset);

^^ above two line should be executed only if GPIO line was not requested as IRQ before

(quoted diff) +1 -3

-	}
 
 
-	bank->mod_usage |= BIT(offset);
-	spin_unlock_irqrestore(&bank->lock, flags);
+	omap_enable_gpio_module(bank, offset, false);

^^ after your change, it looks like omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE)
will be called always and GPIO triggering configuration might be lost

  	return 0;
  }

Example (2)
I've found commit 55b6019ae294 "OMAP: GPIO: clear/restore level/edge detect settings on mask/unmask" which does the following
gpio_mask_irq()
|- _set_gpio_triggering(bank, get_gpio_index(gpio), IRQ_TYPE_NONE);

  gpio_unmask_irq()
   |- u32 trigger = irqd_get_trigger_type(d);
      if (trigger)
             omap_set_gpio_triggering(bank, offset, trigger);

As result, it seems unreasonable to physically configure IRQ triggering type in GPIO bank registers inside omap_gpio_irq_type(). Of course, such assumption should be double checked taking into account that __irq_set_trigger() can be called any time even before request_irq().
Also, seems the same could be applied to omap_enable_gpio_module and omap_set_gpio_direction and they could be removed from omap_gpio_irq_type().

Quoted 17 lines

Quoted 2 lines

Could we keep omap_xxx_gpio_module() functions responsible only for GPIO bank PM and enabling/disabling?

If you're thinking about just thinking about having separate wrappers around it like this::

static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset,
bool is_irq)

{
	...
}

static void omap_enable_gpio((struct gpio_bank *bank, unsigned offset)

{
	omap_enable_gpio_module(bpio_bank, offset, 0);
}

Quoted 6 lines

static void omap_enable_gpio_irq((struct gpio_bank *bank, unsigned offset)

{
	omap_enable_gpio_module(bpio_bank, offset, 1);
}

Then yes makes sense to me. Or do you have something else in mind?

Yep. Commented above.

Also, it probably could work if we will set GPIO_CTRL.DISABLEMODULE=1 in omap_gpio_runtime_resume and GPIO_CTRL.DISABLEMODULE=0 in _runtime_suspend, but it may require from us to split CPUIdle and suspend code path (

Quoted 28 lines · Grygorii.Strashko at linaro.org

Hi Tony,

Sorry for delayed reply.

Quoted 23 lines · Tony Lindgren

Quoted 21 lines · Grygorii.Strashko at linaro.org

Review static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset) · line 457

Quoted 12 lines · Tony Lindgren

(quoted diff) +8 -0

@@ -438,11 +447,30 @@ static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset)
    		writel_relaxed(ctrl, reg);
    		bank->context.ctrl = ctrl;
    	}
+
+	if (is_irq) {
+		omap_set_gpio_direction(bank, offset, 1);
+		bank->irq_usage |= BIT(offset);
+	} else {
+		omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
+		bank->mod_usage |= BIT(offset);
+	}

The OMAP GPIO driver implements two Core interfaces IRQ-chip and GPIO-chip which, in general, more or less independent.

So, I don't think, that it's good to mix GPIO-IRQ-chip specific code with GPIO-chip code. And this even don't really correspond the purpose of omap_enable_gpio_module() :( and might introduce misunderstanding of code. The worst thing is that future fixes in IRQ-chip may affect on on GPIO-chip and vise versa :(

Quoted 6 lines

Quoted 3 lines

Hmm I'm thinking omap_enable/disable_gpio_module() eventually becomes our runtime_resume/suspend(). Currently the enabling and disabling is buggy for GPIO for some corner cases.. AFAIK the only difference between

It might be very helpful if you'd able to share additional info about any "corner cases" you know.

Well not much to share there. I started trying to figure out why your earlier patch causes PM regressions trying reduce the GPIO_BANK usage, but had not luck. Based on looking at the code, I then noticed that some resources are not freed for omap_gpio_irq_type() with the early returns. And then I noticed we have piles of duplicate code all over the place pretty much trying to do the same thing.

Quoted 4 lines

Quoted 3 lines

enabling GPIO vs GPIO-IRQ is the calling of omap_set_gpio_direction vs omap_set_gpio_triggering. Or at least that's the way it should be unless I'm missing something?

Quoted 3 lines

I think yes. what i'd like to say, first of all, is that it might be not good idea to mix too much functionality in omap_enable/disable_gpio_module() - especially GPIO-IRQ vs GPIO-chip ( Very easy to get lost ;)

In general removing 30 lines of duplicate code to fix freeing of resources makes sense :) I'd rather do that than add more similar code to fix it for sure. That will make further changes easier too.

Quoted 33 lines

For example (1) - your change of omap_gpio_request() looks like invalid:

   static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
   {
   	struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);

(quoted diff) +0 -16

-	unsigned long flags;
 
-	/*
-	 * If this is the first gpio_request for the bank,
-	 * enable the bank module.
-	 */
-	if (!BANK_USED(bank))
-		pm_runtime_get_sync(bank->dev);
-
-	spin_lock_irqsave(&bank->lock, flags);
-	/* Set trigger to none. You need to enable the desired trigger with
-	 * request_irq() or set_irq_type(). Only do this if the IRQ line has
-	 * not already been requested.
-	 */
-	if (!LINE_USED(bank->irq_usage, offset)) {
-		omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
-		omap_enable_gpio_module(bank, offset);

^^ above two line should be executed only if GPIO line was not requested as IRQ before

(quoted diff) +1 -3

-	}
 
 
-	bank->mod_usage |= BIT(offset);
-	spin_unlock_irqrestore(&bank->lock, flags);
+	omap_enable_gpio_module(bank, offset, false);

Quoted 2 lines

^^ after your change, it looks like omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE)
will be called always and GPIO triggering configuration might be lost

OK yeah that's a valid concern thanks. Let's keep that part to be sure we're not introducing regressions.

Quoted 14 lines

   	return 0;
   }

Example (2)
I've found commit 55b6019ae294 "OMAP: GPIO: clear/restore level/edge detect settings on mask/unmask" which does the following
gpio_mask_irq()

    |-  _set_gpio_triggering(bank, get_gpio_index(gpio), IRQ_TYPE_NONE);

   gpio_unmask_irq()
    |- u32 trigger = irqd_get_trigger_type(d);
       if (trigger)
              omap_set_gpio_triggering(bank, offset, trigger);

Quoted 5 lines

As result, it seems unreasonable to physically configure IRQ triggering type in GPIO bank registers inside omap_gpio_irq_type(). Of course, such assumption should be double checked taking into account that __irq_set_trigger() can be called any time even before request_irq().
Also, seems the same could be applied to omap_enable_gpio_module and omap_set_gpio_direction and they could be removed from omap_gpio_irq_type().

In omap_gpio_irq_type() we call omap_set_gpio_triggering() after we call omap_enable_gpio_module(), so I don't think we need to change anything there. Or am I missing something here?

Quoted 17 lines

Quoted 17 lines

Quoted 2 lines

Could we keep omap_xxx_gpio_module() functions responsible only for GPIO bank PM and enabling/disabling?

If you're thinking about just thinking about having separate wrappers around it like this::

static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset,
bool is_irq)

{
	...
}

static void omap_enable_gpio((struct gpio_bank *bank, unsigned offset)

{
	omap_enable_gpio_module(bpio_bank, offset, 0);
}

Quoted 8 lines

Quoted 6 lines

static void omap_enable_gpio_irq((struct gpio_bank *bank, unsigned offset)

{
	omap_enable_gpio_module(bpio_bank, offset, 1);
}

Then yes makes sense to me. Or do you have something else in mind?

Yep. Commented above.

Sounds like adding the wrappers is not needed as we're planning to replace these with line specific pm runtime calls anyways.

Quoted 3 lines

Also, it probably could work if we will set GPIO_CTRL.DISABLEMODULE=1 in omap_gpio_runtime_resume and GPIO_CTRL.DISABLEMODULE=0 in _runtime_suspend, but it may require from us to split CPUIdle and suspend code path (

What Felipe suggested a while back to me was we implement runtime_idle() that checks for GPIO lines used and then we can still suspend and resume for idle.

Anyways, updated patch below with the line check added. Do you see other issues?

Regards,

Tony

8< -------------------------
From: Tony Lindgren <[email protected]>
Date: Mon, 27 Apr 2015 10:18:17 -0700
Subject: [PATCH] gpio: omap: Fix PM runtime issue and remove most BANK_USED macros

Looks like omap_gpio_irq_type can return early at several places leaving a GPIO bank enabled without doing pm_runtime_put if wrong GPIO arguments are passed.

Instead of adding more complicated BANK_USED macros, let's fix the issue properly. We can pass is_irq flag to omap_enable_gpio_module and omap_disble_gpio_module. And with that we can remove all the similar code elsewhere to get rid of most BANK_USED macros.

Note that the reason for the BANK_USED macro is that we need to manage PM runtime on per GPIO bank basis. In the long run we want to move to using PM runtime counts for each GPIO line to determine if a GPIO bank is used. Once we have a solution for omap_enable_gpio_module and omap_disable_gpio_module, we can remove the remaining BANK_USED macros.

Cc Felipe Balbi <[email protected]>

Cc Javier Martinez Canillas <[email protected]>

Cc Grygorii Strashko <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Nishanth Menon <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

drivers/gpio/gpio-omap.c +27 -2

@@ -86,6 +86,7 @@ struct gpio_bank {
  #define BANK_USED(bank) (bank->mod_usage || bank->irq_usage)
  #define LINE_USED(line, offset) (line & (BIT(offset)))
 
+static void omap_reset_gpio(struct gpio_bank *bank, unsigned offset);
  static void omap_gpio_unmask_irq(struct irq_data *d);
 
  static inline struct gpio_bank *omap_irq_data_get_bank(struct irq_data *d)

@@ -419,8 +420,16 @@ static int omap_set_gpio_triggering(struct gpio_bank *bank, int gpio,
  	return 0;
  }
 
-static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset)
+static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset,
+				    bool is_irq)
  {
+	unsigned long flags;
+
+	/* PM runtime is per bank, not per GPIO line */
+	if (!BANK_USED(bank))
+		pm_runtime_get_sync(bank->dev);
+
+	spin_lock_irqsave(&bank->lock, flags);
  	if (bank->regs->pinctrl) {
  		void __iomem *reg = bank->base + bank->regs->pinctrl;
 

@@ -438,11 +447,36 @@ static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset)
  		writel_relaxed(ctrl, reg);
  		bank->context.ctrl = ctrl;
  	}
+
+	if (is_irq) {
+		omap_set_gpio_direction(bank, offset, 1);
+		bank->irq_usage |= BIT(offset);
+	} else {
+		/*
+		 * Set trigger to none. You need to enable the desired trigger
+		 * with request_irq() or set_irq_type(). Only do this if the
+		 * IRQ line has not already been requested.
+		 */
+		if (!LINE_USED(bank->irq_usage, offset))
+			omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
+		bank->mod_usage |= BIT(offset);
+	}
+	spin_unlock_irqrestore(&bank->lock, flags);
  }
 
-static void omap_disable_gpio_module(struct gpio_bank *bank, unsigned offset)
+static void omap_disable_gpio_module(struct gpio_bank *bank, unsigned offset,
+				     bool is_irq)
  {

drivers/gpio/gpio-omap.c +10 -69

  	if (bank->regs->wkup_en &&
  	    !LINE_USED(bank->mod_usage, offset) &&

@@ -463,6 +497,11 @@ static void omap_disable_gpio_module(struct gpio_bank *bank, unsigned offset)
  		writel_relaxed(ctrl, reg);
  		bank->context.ctrl = ctrl;
  	}
+	spin_unlock_irqrestore(&bank->lock, flags);
+
+	/* PM runtime is per bank, not per GPIO line */
+	if (!BANK_USED(bank))
+		pm_runtime_put(bank->dev);
  }
 
  static int omap_gpio_is_input(struct gpio_bank *bank, unsigned offset)

@@ -472,15 +511,6 @@ static int omap_gpio_is_input(struct gpio_bank *bank, unsigned offset)
  	return readl_relaxed(reg) & BIT(offset);
  }
 
-static void omap_gpio_init_irq(struct gpio_bank *bank, unsigned offset)
-{
-	if (!LINE_USED(bank->mod_usage, offset)) {
-		omap_enable_gpio_module(bank, offset);
-		omap_set_gpio_direction(bank, offset, 1);
-	}
-	bank->irq_usage |= BIT(offset);
-}
-
  static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
  {
  	struct gpio_bank *bank = omap_irq_data_get_bank(d);

@@ -488,9 +518,6 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
  	unsigned long flags;
  	unsigned offset = d->hwirq;
 
-	if (!BANK_USED(bank))
-		pm_runtime_get_sync(bank->dev);
-
  	if (type & ~IRQ_TYPE_SENSE_MASK)
  		return -EINVAL;
 

@@ -498,13 +525,9 @@ static int oomap_gpio_irq_startupmap_gpio_irq_type(struct irq_data *d, unsigned type)
  		(type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH)))
  		return -EINVAL;
 
+	omap_enable_gpio_module(bank, offset, true);
  	spin_lock_irqsave(&bank->lock, flags);
  	retval = omap_set_gpio_triggering(bank, offset, type);
-	omap_gpio_init_irq(bank, offset);
-	if (!omap_gpio_is_input(bank, offset)) {
-		spin_unlock_irqrestore(&bank->lock, flags);
-		return -EINVAL;
-	}
  	spin_unlock_irqrestore(&bank->lock, flags);
 
  	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))

@@ -659,26 +682,8 @@ static int omap_gpio_wake_enable(struct irq_data *d, unsigned int enable)
  static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
  {
  	struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
-	unsigned long flags;
 
-	/*
-	 * If this is the first gpio_request for the bank,
-	 * enable the bank module.
-	 */
-	if (!BANK_USED(bank))
-		pm_runtime_get_sync(bank->dev);
-
-	spin_lock_irqsave(&bank->lock, flags);
-	/* Set trigger to none. You need to enable the desired trigger with
-	 * request_irq() or set_irq_type(). Only do this if the IRQ line has
-	 * not already been requested.
-	 */
-	if (!LINE_USED(bank->irq_usage, offset)) {
-		omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
-		omap_enable_gpio_module(bank, offset);
-	}
-	bank->mod_usage |= BIT(offset);
-	spin_unlock_irqrestore(&bank->lock, flags);
+	omap_enable_gpio_module(bank, offset, false);
 
  	return 0;
  }

@@ -686,20 +691,8 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
  static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
  {
  	struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
-	unsigned long flags;
-
-	spin_lock_irqsave(&bank->lock, flags);
-	bank->mod_usage &= ~(BIT(offset));
-	omap_disable_gpio_module(bank, offset);
-	omap_reset_gpio(bank, offset);
-	spin_unlock_irqrestore(&bank->lock, flags);
 
-	/*
-	 * If this is the last gpio to be freed in the bank,
-	 * disable the bank module.
-	 */
-	if (!BANK_USED(bank))
-		pm_runtime_put(bank->dev);
+	omap_disable_gpio_module(bank, offset, false);
  }
 
  /*

@@ -788,15 +781,9 @@ exit:
  static unsigned int omap_gpio_irq_startup(struct irq_data *d)
  {
  	struct gpio_bank *bank = omap_irq_data_get_bank(d);
-	unsigned long flags;
  	unsigned offset = d->hwirq;
 
-	if (!BANK_USED(bank))
-		pm_runtime_get_sync(bank->dev);
-
-	spin_lock_irqsave(&bank->lock, flags);
-	omap_gpio_init_irq(bank, offset);
-	spin_unlock_irqrestore(&bank->lock, flags);
+	omap_enable_gpio_module(bank, offset, true);
  	omap_gpio_unmask_irq(d);
 
  	return 0;

@@ -805,21 +792,9 @@ static unsigned int omap_gpio_irq_startup(struct irq_data *d)
  static void omap_gpio_irq_shutdown(struct irq_data *d)
  {
  	struct gpio_bank *bank = omap_irq_data_get_bank(d);
-	unsigned long flags;
  	unsigned offset = d->hwirq;
 
-	spin_lock_irqsave(&bank->lock, flags);
-	bank->irq_usage &= ~(BIT(offset));
-	omap_disable_gpio_module(bank, offset);
-	omap_reset_gpio(bank, offset);
-	spin_unlock_irqrestore(&bank->lock, flags);
-
-	/*
-	 * If this is the last IRQ to be freed in the bank,
-	 * disable the bank module.
-	 */
-	if (!BANK_USED(bank))
-		pm_runtime_put(bank->dev);
+	omap_disable_gpio_module(bank, offset, true);
  }
 

Quoted 4 lines · Tony Lindgren

In general removing 30 lines of duplicate code to fix freeing of resources makes sense :) I'd rather do that than add more similar code to fix it for sure. That will make further changes easier too.

Unfortunately I'm not sure ( at least not for this driver.
Future fixes most probably will eat all these removed lines and
will just add anther "if else" but in different places (see description or corner case below).

In my opinion, It might be better to review each interface function first as for GPIO chip as for GPIOIRQ chip and ensure that they are doing exactly what need to be done (as part this job - get rid of over-optimized functions like omap_reset_gpio or omap_gpio_init_irq). And after that do further optimization.

drivers/gpio/gpio-omap.c +3 -8

@@ -488,9 +488,6 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
        unsigned long flags;
        unsigned offset = d->hwirq;
 
-       if (!BANK_USED(bank))
-               pm_runtime_get_sync(bank->dev);
-
        if (type & ~IRQ_TYPE_SENSE_MASK)
                return -EINVAL;
 

@@ -498,13 +495,11 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
                (type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH)))
                return -EINVAL;
 
+       if (!BANK_USED(bank))
+               pm_runtime_get_sync(bank->dev);
+
        spin_lock_irqsave(&bank->lock, flags);
        retval = omap_set_gpio_triggering(bank, offset, type);
-       omap_gpio_init_irq(bank, offset);
-       if (!omap_gpio_is_input(bank, offset)) {
-               spin_unlock_irqrestore(&bank->lock, flags);
-               return -EINVAL;
-       }
        spin_unlock_irqrestore(&bank->lock, flags);
 
        if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))

There is another corner case :) - easy to reproduce and it's present in old code.

GPIOn is used as IRQ by some dev:
- in my case PCF8575.INT -> gpio6.11
- PCFx driver knows nothing about type of IRQ line (GPIO or not) so it doesn't request gpio and just do request_irq()

If I'll export gpio6.11 through sysfs and then unxeport it then IRQs from PCFx will not be received any more.

Seems omap_reset_gpio has to be split for GPIO and GPIO-IRQ cases.

Quoted 6 lines · Grygorii.Strashko at linaro.org

Quoted 4 lines · Tony Lindgren

In general removing 30 lines of duplicate code to fix freeing of resources makes sense :) I'd rather do that than add more similar code to fix it for sure. That will make further changes easier too.

Quoted 8 lines

Unfortunately I'm not sure ( at least not for this driver.
Future fixes most probably will eat all these removed lines and
will just add anther "if else" but in different places (see description or corner case below).

In my opinion, It might be better to review each interface function first as for GPIO chip as for GPIOIRQ chip and ensure that they are doing exactly what need to be done (as part this job - get rid of over-optimized functions like omap_reset_gpio or omap_gpio_init_irq). And after that do further optimization.

In general I agree that a minimal fix is preferred. Unfortunately in this case "fixing" it gets us deeper into the mess with BANK_USED :)

Quoted 30 lines

drivers/gpio/gpio-omap.c +3 -8

@@ -488,9 +488,6 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
         unsigned long flags;
         unsigned offset = d->hwirq;
 
-       if (!BANK_USED(bank))
-               pm_runtime_get_sync(bank->dev);
-
         if (type & ~IRQ_TYPE_SENSE_MASK)
                 return -EINVAL;
 

@@ -498,13 +495,11 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
                 (type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH)))
                 return -EINVAL;
 
+       if (!BANK_USED(bank))
+               pm_runtime_get_sync(bank->dev);
+
         spin_lock_irqsave(&bank->lock, flags);
         retval = omap_set_gpio_triggering(bank, offset, type);
-       omap_gpio_init_irq(bank, offset);
-       if (!omap_gpio_is_input(bank, offset)) {
-               spin_unlock_irqrestore(&bank->lock, flags);
-               return -EINVAL;
-       }
         spin_unlock_irqrestore(&bank->lock, flags);
 
         if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))

drivers/gpio/gpio-omap.c static int omap_gpio_irq_type(struct irq_data *d, unsigned type) · line 514

Quoted 8 lines

drivers/gpio/gpio-omap.c +3 -0

@@ -512,6 +507,9 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
         else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
                 __irq_set_handler_locked(d->irq, handle_edge_irq);
 
+       if (!BANK_USED(bank))
+               pm_runtime_put(bank->dev);
+
         return retval;
  }

Hmm I don't think we can remove the call to omap_gpio_init_irq as with request_irq there's nothing ensuring the GPIO line is configured in the device tree case for example?

And then removing the check for omap_gpio_is_input is not good.. We should just call omap_set_gpio_direction to set it to input instead like I'm doing.

And looking at it more, omap_set_gpio_triggering can fail so we should check the return value (also with my patch). And if that fails, then we currently need a flag to keep track if we enabled the bank in omap_gpio_irq_type or not.. So piling up yet more BANK_USED tinkering.

I still think we're better of just removing it all. Below is my patch updated with the omap_set_gpio_triggering error handling. We can just make omap_enable_gpio_module return a value, it might provide us with more info where things go wrong :)

Quoted 8 lines

There is another corner case :) - easy to reproduce and it's present in old code.

GPIOn is used as IRQ by some dev:
- in my case PCF8575.INT -> gpio6.11
- PCFx driver knows nothing about type of IRQ line (GPIO or not) so it doesn't request gpio and just do request_irq()

Quoted 4 lines

If I'll export gpio6.11 through sysfs and then unxeport it then IRQs from PCFx will not be received any more.

Seems omap_reset_gpio has to be split for GPIO and GPIO-IRQ cases.

OK, that sounds like a separate fix though if I understand your right. We should not allow exporting GPIOs to sysfs if they are being used.

Regards,

Tony

8< -------------------------
From: Tony Lindgren <[email protected]>
Date: Mon, 27 Apr 2015 10:18:17 -0700
Subject: [PATCH] gpio: omap: Fix PM runtime issue and remove most BANK_USED
macros

Looks like omap_gpio_irq_type can return early at several places leaving a GPIO bank enabled without doing pm_runtime_put if wrong GPIO arguments are passed.

Instead of adding more complicated BANK_USED macros, let's fix the issue properly. We can pass is_irq flag to omap_enable_gpio_module and omap_disble_gpio_module. And with that we can remove all the similar code elsewhere to get rid of most BANK_USED macros.

Note that the reason for the BANK_USED macro is that we need to manage PM runtime on per GPIO bank basis. In the long run we want to move to using PM runtime counts for each GPIO line to determine if a GPIO bank is used. Once we have a solution for omap_enable_gpio_module and omap_disable_gpio_module, we can remove the remaining BANK_USED macros.

Cc Felipe Balbi <[email protected]>

Cc Javier Martinez Canillas <[email protected]>

Cc Grygorii Strashko <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Nishanth Menon <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

drivers/gpio/gpio-omap.c +11 -1

@@ -86,6 +86,7 @@ struct gpio_bank {
  #define BANK_USED(bank) (bank->mod_usage || bank->irq_usage)
  #define LINE_USED(line, offset) (line & (BIT(offset)))
 
+static void omap_reset_gpio(struct gpio_bank *bank, unsigned offset);
  static void omap_gpio_unmask_irq(struct irq_data *d);
 
  static inline struct gpio_bank *omap_irq_data_get_bank(struct irq_data *d)

@@ -419,8 +420,17 @@ static int omap_set_gpio_triggering(struct gpio_bank *bank, int gpio,
  	return 0;
  }
 
-static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset)
+static int omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset,
+				   bool is_irq)
  {
+	unsigned long flags;
+	int error;
+
+	/* PM runtime is per bank, not per GPIO line */
+	if (!BANK_USED(bank))
+		pm_runtime_get_sync(bank->dev);
+
+	spin_lock_irqsave(&bank->lock, flags);
  	if (bank->regs->pinctrl) {
  		void __iomem *reg = bank->base + bank->regs->pinctrl;
 

drivers/gpio/gpio-omap.c +13 -0

+	} else {
+		/*
+		 * Set trigger to none. You need to enable the desired trigger
+		 * with request_irq() or set_irq_type(). Only do this if the
+		 * IRQ line has not already been requested.
+		 */
+		if (!LINE_USED(bank->irq_usage, offset)) {
+			error = omap_set_gpio_triggering(bank, offset,
+							 IRQ_TYPE_NONE);
+			if (error)
+				goto out_err;
+		}
+		bank->mod_usage |= BIT(offset);

drivers/gpio/gpio-omap.c +19 -13

+
+	return error;
  }
 
-static void omap_disable_gpio_module(struct gpio_bank *bank, unsigned offset)
+static void omap_disable_gpio_module(struct gpio_bank *bank, unsigned offset,
+				     bool is_irq)
  {
  	void __iomem *base = bank->base;
+	unsigned long flags;
+
+	spin_lock_irqsave(&bank->lock, flags);
+	if (is_irq)
+		bank->irq_usage &= ~(BIT(offset));
+	else
+		bank->mod_usage &= ~(BIT(offset));
+
+	omap_reset_gpio(bank, offset);
 
  	if (bank->regs->wkup_en &&
  	    !LINE_USED(bank->mod_usage, offset) &&

@@ -463,6 +512,11 @@ static void omap_disable_gpio_module(struct gpio_bank *bank, unsigned offset)
  		writel_relaxed(ctrl, reg);
  		bank->context.ctrl = ctrl;
  	}
+	spin_unlock_irqrestore(&bank->lock, flags);
+
+	/* PM runtime is per bank, not per GPIO line */
+	if (!BANK_USED(bank))
+		pm_runtime_put(bank->dev);
  }
 
  static int omap_gpio_is_input(struct gpio_bank *bank, unsigned offset)

@@ -472,15 +526,6 @@ static int omap_gpio_is_input(struct gpio_bank *bank, unsigned offset)
  	return readl_relaxed(reg) & BIT(offset);
  }
 
-static void omap_gpio_init_irq(struct gpio_bank *bank, unsigned offset)
-{
-	if (!LINE_USED(bank->mod_usage, offset)) {
-		omap_enable_gpio_module(bank, offset);
-		omap_set_gpio_direction(bank, offset, 1);
-	}
-	bank->irq_usage |= BIT(offset);
-}
-
  static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
  {
  	struct gpio_bank *bank = omap_irq_data_get_bank(d);

@@ -488,9 +533,6 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
  	unsigned long flags;
  	unsigned offset = d->hwirq;
 
-	if (!BANK_USED(bank))
-		pm_runtime_get_sync(bank->dev);
-
  	if (type & ~IRQ_TYPE_SENSE_MASK)
  		return -EINVAL;
 

@@ -498,13 +540,11 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
  		(type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH)))
  		return -EINVAL;
 
+	retval = omap_enable_gpio_module(bank, offset, true);

drivers/gpio/gpio-omap.c +10 -52

  	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))

@@ -659,26 +699,11 @@ static int omap_gpio_wake_enable(struct irq_data *d, unsigned int enable)
  static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
  {
  	struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
-	unsigned long flags;
+	int error;
 
-	/*
-	 * If this is the first gpio_request for the bank,
-	 * enable the bank module.
-	 */
-	if (!BANK_USED(bank))
-		pm_runtime_get_sync(bank->dev);
-
-	spin_lock_irqsave(&bank->lock, flags);
-	/* Set trigger to none. You need to enable the desired trigger with
-	 * request_irq() or set_irq_type(). Only do this if the IRQ line has
-	 * not already been requested.
-	 */
-	if (!LINE_USED(bank->irq_usage, offset)) {
-		omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
-		omap_enable_gpio_module(bank, offset);
-	}
-	bank->mod_usage |= BIT(offset);
-	spin_unlock_irqrestore(&bank->lock, flags);
+	error = omap_enable_gpio_module(bank, offset, false);
+	if (error)
+		return error;
 
  	return 0;
  }

@@ -686,20 +711,8 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
  static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
  {
  	struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
-	unsigned long flags;
-
-	spin_lock_irqsave(&bank->lock, flags);
-	bank->mod_usage &= ~(BIT(offset));
-	omap_disable_gpio_module(bank, offset);
-	omap_reset_gpio(bank, offset);
-	spin_unlock_irqrestore(&bank->lock, flags);
 
-	/*
-	 * If this is the last gpio to be freed in the bank,
-	 * disable the bank module.
-	 */
-	if (!BANK_USED(bank))
-		pm_runtime_put(bank->dev);
+	omap_disable_gpio_module(bank, offset, false);
  }
 
  /*

@@ -788,15 +801,12 @@ exit:
  static unsigned int omap_gpio_irq_startup(struct irq_data *d)
  {
  	struct gpio_bank *bank = omap_irq_data_get_bank(d);
-	unsigned long flags;
  	unsigned offset = d->hwirq;
+	int error;
 
-	if (!BANK_USED(bank))
-		pm_runtime_get_sync(bank->dev);
-
-	spin_lock_irqsave(&bank->lock, flags);
-	omap_gpio_init_irq(bank, offset);
-	spin_unlock_irqrestore(&bank->lock, flags);
+	error = omap_enable_gpio_module(bank, offset, true);
+	if (error)
+		return error;
  	omap_gpio_unmask_irq(d);
 
  	return 0;

@@ -805,21 +815,9 @@ static unsigned int omap_gpio_irq_startup(struct irq_data *d)
  static void omap_gpio_irq_shutdown(struct irq_data *d)
  {
  	struct gpio_bank *bank = omap_irq_data_get_bank(d);
-	unsigned long flags;
  	unsigned offset = d->hwirq;
 
-	spin_lock_irqsave(&bank->lock, flags);
-	bank->irq_usage &= ~(BIT(offset));
-	omap_disable_gpio_module(bank, offset);
-	omap_reset_gpio(bank, offset);
-	spin_unlock_irqrestore(&bank->lock, flags);
-
-	/*
-	 * If this is the last IRQ to be freed in the bank,
-	 * disable the bank module.
-	 */
-	if (!BANK_USED(bank))
-		pm_runtime_put(bank->dev);
+	omap_disable_gpio_module(bank, offset, true);
  }

Hi Tony,

Quoted 62 lines · Tony Lindgren

Quoted 6 lines · Grygorii.Strashko at linaro.org

Quoted 4 lines · Tony Lindgren

In general removing 30 lines of duplicate code to fix freeing of resources makes sense :) I'd rather do that than add more similar code to fix it for sure. That will make further changes easier too.

Quoted 8 lines

Unfortunately I'm not sure ( at least not for this driver.
Future fixes most probably will eat all these removed lines and
will just add anther "if else" but in different places (see description or corner case below).

In my opinion, It might be better to review each interface function first as for GPIO chip as for GPIOIRQ chip and ensure that they are doing exactly what need to be done (as part this job - get rid of over-optimized functions like omap_reset_gpio or omap_gpio_init_irq). And after that do further optimization.

In general I agree that a minimal fix is preferred. Unfortunately in this case "fixing" it gets us deeper into the mess with BANK_USED :)

Quoted 30 lines

drivers/gpio/gpio-omap.c +3 -8

@@ -488,9 +488,6 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
         unsigned long flags;
         unsigned offset = d->hwirq;
 
-       if (!BANK_USED(bank))
-               pm_runtime_get_sync(bank->dev);
-
         if (type & ~IRQ_TYPE_SENSE_MASK)
                 return -EINVAL;
 

@@ -498,13 +495,11 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
                 (type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH)))
                 return -EINVAL;
 
+       if (!BANK_USED(bank))
+               pm_runtime_get_sync(bank->dev);
+
         spin_lock_irqsave(&bank->lock, flags);
         retval = omap_set_gpio_triggering(bank, offset, type);
-       omap_gpio_init_irq(bank, offset);
-       if (!omap_gpio_is_input(bank, offset)) {
-               spin_unlock_irqrestore(&bank->lock, flags);
-               return -EINVAL;
-       }
         spin_unlock_irqrestore(&bank->lock, flags);
 
         if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))

drivers/gpio/gpio-omap.c static int omap_gpio_irq_type(struct irq_data *d, unsigned type) · line 514

Quoted 8 lines

drivers/gpio/gpio-omap.c +3 -0

@@ -512,6 +507,9 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
         else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
                 __irq_set_handler_locked(d->irq, handle_edge_irq);
 
+       if (!BANK_USED(bank))
+               pm_runtime_put(bank->dev);
+
         return retval;
  }

Hmm I don't think we can remove the call to omap_gpio_init_irq as with request_irq there's nothing ensuring the GPIO line is configured in the device tree case for example?

Quoted 3 lines

And then removing the check for omap_gpio_is_input is not good.. We should just call omap_set_gpio_direction to set it to input instead like I'm doing.

I think, We can remove all these calls from omap_gpio_irq_type() because the following call sequence is executed when IRQ is requested:

request_threaded_irq() - or - __irq_set_handler(is_chained==true) __setup_irq()
irq_startup()
desc->irq_data.chip->irq_startup() omap_gpio_irq_startup()
omap_gpio_init_irq()
omap_gpio_unmask_irq()

The check for omap_gpio_is_input should be done in startup omap_gpio_irq_startup() in case if GPIO was requested before, and if not - GPIO should be configured as input. (we just need to continue reviewing process as for me)

Quoted 21 lines

And looking at it more, omap_set_gpio_triggering can fail so we should check the return value (also with my patch). And if that fails, then we currently need a flag to keep track if we enabled the bank in omap_gpio_irq_type or not.. So piling up yet more BANK_USED tinkering.

I still think we're better of just removing it all. Below is my patch updated with the omap_set_gpio_triggering error handling. We can just make omap_enable_gpio_module return a value, it might provide us with more info where things go wrong :)

Quoted 11 lines

There is another corner case :) - easy to reproduce and it's present in old code.

GPIOn is used as IRQ by some dev:
- in my case PCF8575.INT -> gpio6.11
- PCFx driver knows nothing about type of IRQ line (GPIO or not) so it doesn't request gpio and just do request_irq()

If I'll export gpio6.11 through sysfs and then unxeport it then IRQs from PCFx will not be received any more.

Quoted 5 lines

Quoted 1 line

Seems omap_reset_gpio has to be split for GPIO and GPIO-IRQ cases.

OK, that sounds like a separate fix though if I understand your right. We should not allow exporting GPIOs to sysfs if they are being used.

Why not? It's good for IRQ debugging at least, but changing of GPIO direction should be prohibited.

Looks like omap_gpio_irq_type can return early at several places leaving a GPIO bank enabled without doing pm_runtime_put if wrong GPIO arguments are passed.

Instead of adding more complicated BANK_USED macros, let's fix the issue properly. We can pass is_irq flag to omap_enable_gpio_module and omap_disble_gpio_module. And with that we can remove all the similar code elsewhere to get rid of most BANK_USED macros.

Note that the reason for the BANK_USED macro is that we need to manage PM runtime on per GPIO bank basis. In the long run we want to move to using PM runtime counts for each GPIO line to determine if a GPIO bank is used. Once we have a solution for omap_enable_gpio_module and omap_disable_gpio_module, we can remove the remaining BANK_USED macros.

Cc Felipe Balbi <[email protected]>

Cc Grygorii Strashko <[email protected]>

Cc Javier Martinez Canillas <[email protected]>

Cc Nishanth Menon <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

Quoted 16 lines · Tony Lindgren

Looks like omap_gpio_irq_type can return early at several places leaving a GPIO bank enabled without doing pm_runtime_put if wrong GPIO arguments are passed.

Instead of adding more complicated BANK_USED macros, let's fix the issue properly. We can pass is_irq flag to omap_enable_gpio_module and omap_disble_gpio_module. And with that we can remove all the similar code elsewhere to get rid of most BANK_USED macros.

Note that the reason for the BANK_USED macro is that we need to manage PM runtime on per GPIO bank basis. In the long run we want to move to using PM runtime counts for each GPIO line to determine if a GPIO bank is used. Once we have a solution for omap_enable_gpio_module and omap_disable_gpio_module, we can remove the remaining BANK_USED macros.

Quoted 5 lines

Cc Felipe Balbi <[email protected]>

Cc Grygorii Strashko <[email protected]>

Cc Javier Martinez Canillas <[email protected]>

Cc Nishanth Menon <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

There is so much discussion around this patch that I can't make up my mind whether I should apply this or some subsequent inline version of it.

Holding this off for now...

Quoted 28 lines · Linus Walleij

Quoted 16 lines · Tony Lindgren

Looks like omap_gpio_irq_type can return early at several places leaving a GPIO bank enabled without doing pm_runtime_put if wrong GPIO arguments are passed.

Instead of adding more complicated BANK_USED macros, let's fix the issue properly. We can pass is_irq flag to omap_enable_gpio_module and omap_disble_gpio_module. And with that we can remove all the similar code elsewhere to get rid of most BANK_USED macros.

Note that the reason for the BANK_USED macro is that we need to manage PM runtime on per GPIO bank basis. In the long run we want to move to using PM runtime counts for each GPIO line to determine if a GPIO bank is used. Once we have a solution for omap_enable_gpio_module and omap_disable_gpio_module, we can remove the remaining BANK_USED macros.

Quoted 5 lines

Cc Felipe Balbi <[email protected]>

Cc Grygorii Strashko <[email protected]>

Cc Javier Martinez Canillas <[email protected]>

Cc Nishanth Menon <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

There is so much discussion around this patch that I can't make up my mind whether I should apply this or some subsequent inline version of it.

Quoted 1 line

Holding this off for now...

Yeah let's see what Grygorii comes up, sounds like he may have a minimal fix coming up. Then we can do the rest as clean-up later on.