Re: [PATCH] gpio: omap: Silence lockdep "Invalid wait context"

["Sverdlin, Alexander" <alexander.sverdlin@xxxxxxxxxxx>]

Thanks Bartosz for looking into it!

On Wed, 2024-11-27 at 11:04 +0100, Bartosz Golaszewski wrote:
> > > > @@ -827,6 +846,13 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
> > > >           struct gpio_bank *bank = gpiochip_get_data(chip);
> > > >           unsigned long flags;
> > > > 
> > > > +       /*
> > > > +        * Enable the clock here so that the nested clk_disable() in the
> > > > +        * following omap_clear_gpio_debounce() is lockless
> > > > +        */
> > > > +       if (bank->dbck_flag)
> > > > +               clk_enable(bank->dbck);
> >                      ^^^^^^^^^^^^^^^^^^^^^^
> > And for this second enable....
> > 
> > > > +
> > > >           raw_spin_lock_irqsave(&bank->lock, flags);
> > > >           bank->mod_usage &= ~(BIT(offset));
> > > >           if (!LINE_USED(bank->irq_usage, offset)) {
> > > > @@ -836,6 +862,9 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
> > > >           omap_disable_gpio_module(bank, offset);
> > > >           raw_spin_unlock_irqrestore(&bank->lock, flags);
> > > > 
> > > > +       if (bank->dbck_flag)
> > > > +               clk_disable(bank->dbck);
> >                      ^^^^^^^^^^^^^^^^^^^^^^^
> > ... this would be the corresponding 2nd disable.
> > 
> > > > +
> > > >           pm_runtime_put(chip->parent);
> > > >    }
> > > > 
> > 
> > Aren't they paired from your PoV?
> > 
> 
> Ok, I needed to take a long look at this driver.
> 
> IIUC what happens now:
> 
> In omap_gpio_irq_shutdown() and omap_gpio_free() you now enable the
> clock (really just bumping its enable count) before entering
> omap_clear_gpio_debounce() so that its internal call do clk_disable()
> only decreases the reference count without actually disabling it and
> the clock is really disabled one level up the stack.
> 
> If that's correct, isn't it unnecessarily convoluted?
> omap_clear_gpio_debounce() is only called from these two functions so
> why not just move the clk_disable() out of it and into them?

Seems that I didn't notice the elephant in the room, so let me rework it ;-)

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com