[2/7] gpio: omap: fix error handling in omap_gpio_irq_type

The GPIO bank will be kept powered in case if input parameters
are invalid or error occurred in omap_gpio_irq_type.

Hence, fix it by ensuring that GPIO bank will be unpowered
in case of errors and add additional check of value returned
from omap_set_gpio_triggering().

Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
---
 drivers/gpio/gpio-omap.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

Hello Grygorii,

On Fri, May 22, 2015 at 4:35 PM, Grygorii Strashko
<grygorii.strashko@linaro.org> wrote:
> The GPIO bank will be kept powered in case if input parameters
> are invalid or error occurred in omap_gpio_irq_type.
>
> Hence, fix it by ensuring that GPIO bank will be unpowered
> in case of errors and add additional check of value returned
> from omap_set_gpio_triggering().
>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
> ---
>  drivers/gpio/gpio-omap.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index bb60cbc..f6cc638 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -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,12 +495,18 @@ 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);
> +       if (retval)

At this point the bank->lock spinlock is held so you need to add a
spin_unlock_irqrestore() in the error path.

> +               goto error;
>         omap_gpio_init_irq(bank, offset);
>         if (!omap_gpio_is_input(bank, offset)) {
>                 spin_unlock_irqrestore(&bank->lock, flags);
> -               return -EINVAL;
> +               retval = -EINVAL;
> +               goto error;
>         }
>         spin_unlock_irqrestore(&bank->lock, flags);
>
> @@ -512,6 +515,11 @@ 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);
>
> +       return 0;
> +
> +error:
> +       if (!BANK_USED(bank))
> +               pm_runtime_put(bank->dev);
>         return retval;
>  }
>
> --

But you are correct about the runtime PM bug so after addressing the
above comment, feel free to add:

Acked-by: Javier Martinez Canillas <javier@dowhile0.org>

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 06/02/2015 12:40 PM, Javier Martinez Canillas wrote:
> Hello Grygorii,
>
> On Fri, May 22, 2015 at 4:35 PM, Grygorii Strashko
> <grygorii.strashko@linaro.org> wrote:
>> The GPIO bank will be kept powered in case if input parameters
>> are invalid or error occurred in omap_gpio_irq_type.
>>
>> Hence, fix it by ensuring that GPIO bank will be unpowered
>> in case of errors and add additional check of value returned
>> from omap_set_gpio_triggering().
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
>> ---
>>   drivers/gpio/gpio-omap.c | 16 ++++++++++++----
>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index bb60cbc..f6cc638 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -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,12 +495,18 @@ 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);
>> +       if (retval)
>
> At this point the bank->lock spinlock is held so you need to add a
> spin_unlock_irqrestore() in the error path.

Ops. Thanks for catching it.
>
>> +               goto error;
>>          omap_gpio_init_irq(bank, offset);
>>          if (!omap_gpio_is_input(bank, offset)) {
>>                  spin_unlock_irqrestore(&bank->lock, flags);
>> -               return -EINVAL;
>> +               retval = -EINVAL;
>> +               goto error;
>>          }
>>          spin_unlock_irqrestore(&bank->lock, flags);
>>
>> @@ -512,6 +515,11 @@ 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);
>>
>> +       return 0;
>> +
>> +error:
>> +       if (!BANK_USED(bank))
>> +               pm_runtime_put(bank->dev);
>>          return retval;
>>   }
>>
>> --
>
> But you are correct about the runtime PM bug so after addressing the
> above comment, feel free to add:
>
> Acked-by: Javier Martinez Canillas <javier@dowhile0.org>
>

Linus, How do prefer me to fix it?
Resend whole patch or send additional fix on top of patch 5.