Hi Tony,
On 04/21/2015 07:08 PM, Tony Lindgren wrote:
> 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.
In general, this approach is ok for me - I've had not able to test this patch, but
I'm going to take a deeper look on it on Friday or at the beginning of next week.
But, honestly, there is one thing I really don't like :( see below pls.
>
> Cc: Felipe Balbi <balbi@xxxxxx>
> Cc: Grygorii Strashko <grygorii.strashko@xxxxxxxxxx>
> Cc: Javier Martinez Canillas <jmartinez@xxxxxxxxxxxxxx>
> Cc: Nishanth Menon <nm@xxxxxx>
> Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
> ---
> drivers/gpio/gpio-omap.c | 111 +++++++++++++++++------------------------------
> 1 file changed, 40 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index d44e617..39a6312 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -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);
> 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?
> + 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)
>
--
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
