diff mbox series

gpio: aspeed: Lock GPIO pin used as IRQ

Message ID 20201210065013.29348-1-troy_lee@aspeedtech.com
State New
Headers show
Series gpio: aspeed: Lock GPIO pin used as IRQ | expand

Commit Message

Troy Lee Dec. 10, 2020, 6:50 a.m. UTC 
GPIO pins can be used as IRQ indicators. When they do,
those pins should be flaged with locks to avoid kernel
warning message.

Signed-off-by: Chia-Wei, Wang <chiawei_wang@aspeedtech.com>
Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
---
 drivers/gpio/gpio-aspeed.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Andy Shevchenko Dec. 10, 2020, 2:14 p.m. UTC | #1 
On Thu, Dec 10, 2020 at 9:36 AM Troy Lee <troy_lee@aspeedtech.com> wrote:
>
> GPIO pins can be used as IRQ indicators. When they do,
> those pins should be flaged with locks to avoid kernel

flagged

> warning message.

...

> @@ -651,6 +651,13 @@ static int aspeed_gpio_set_type(struct irq_data *d, unsigned int type)

> +       rc = gpiochip_lock_as_irq(&gpio->chip, d->hwirq);
> +       if (rc) {
> +               dev_err(gpio->chip.parent, "unable to lock GPIO %lu as IRQ\n",
> +                       d->hwirq);
> +               return rc;
> +       }

It's a copy'n'paste of generic code. Why do you need it in an unusual
place, i.e. ->irq_set_type() IIUC?
Can you elaborate about an issue, because this seems to be a hack?
Andrew Jeffery Dec. 13, 2020, 11:38 p.m. UTC | #2 
On Fri, 11 Dec 2020, at 00:44, Andy Shevchenko wrote:
> On Thu, Dec 10, 2020 at 9:36 AM Troy Lee <troy_lee@aspeedtech.com> wrote:
> >
> > GPIO pins can be used as IRQ indicators. When they do,
> > those pins should be flaged with locks to avoid kernel
> 
> flagged
> 
> > warning message.
> 
> ...
> 
> > @@ -651,6 +651,13 @@ static int aspeed_gpio_set_type(struct irq_data *d, unsigned int type)
> 
> > +       rc = gpiochip_lock_as_irq(&gpio->chip, d->hwirq);
> > +       if (rc) {
> > +               dev_err(gpio->chip.parent, "unable to lock GPIO %lu as IRQ\n",
> > +                       d->hwirq);
> > +               return rc;
> > +       }
> 
> It's a copy'n'paste of generic code. Why do you need it in an unusual
> place, i.e. ->irq_set_type() IIUC?
> Can you elaborate about an issue, because this seems to be a hack?

Yep - Troy please provide more information. What was the warning you saw? How
were the GPIOs allocated on the system that triggered the warning? What did you
do to trigger the warning?

Andrew
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index b966f5e28ebf..f5b3e1d89fbf 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -651,6 +651,13 @@  static int aspeed_gpio_set_type(struct irq_data *d, unsigned int type)
 		aspeed_gpio_copro_release(gpio, offset);
 	spin_unlock_irqrestore(&gpio->lock, flags);
 
+	rc = gpiochip_lock_as_irq(&gpio->chip, d->hwirq);
+	if (rc) {
+		dev_err(gpio->chip.parent, "unable to lock GPIO %lu as IRQ\n",
+			d->hwirq);
+		return rc;
+	}
+
 	irq_set_handler_locked(d, handler);
 
 	return 0;