Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: "BUG: Invalid wait context" in ls_extirq_set_type
To:     Vladimir Oltean <olteanv@gmail.com>
Cc:     Mark Brown <broonie@kernel.org>, Lee Jones <lee.jones@linaro.org>,
        Arnd Bergmann <arnd@arndb.de>,
        Thomas Gleixner <tglx@linutronix.de>,
        Marc Zyngier <maz@kernel.org>, linux-kernel@vger.kernel.org,
        Esben Haabendal <esben@geanix.com>
References: <20210825135438.ubcuxm5vctt6ne2q@skbuf>
 <26de7b85-e466-e9af-077a-9d1dc087e061@rasmusvillemoes.dk>
 <20210826133355.lbb26bdf4ov5jjyp@skbuf>
From:   Rasmus Villemoes <linux@rasmusvillemoes.dk>
Message-ID: <42b39571-0c76-2cfe-20d2-6c67259117b5@rasmusvillemoes.dk>
Date:   Thu, 26 Aug 2021 15:56:47 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <20210826133355.lbb26bdf4ov5jjyp@skbuf>
Content-Type: text/plain; charset=windows-1252
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 26/08/2021 15.33, Vladimir Oltean wrote:
> On Thu, Aug 26, 2021 at 11:01:31AM +0200, Rasmus Villemoes wrote:

>> So, we've been using the irq-ls-extirq driver for years, on an RT
>> kernel, without seeing something like that. Does it require certain
>> debug knobs in .config? Or perhaps the sanity checks have been added
>> later, we've mostly been using it on 4.14.y and 4.19.y.
> 
> Grepping for "BUG: Invalid wait context" in the kernel yields a single
> hit, and answers both questions. It was added by commit
> 
> commit de8f5e4f2dc1f032b46afda0a78cab5456974f89
> Author: Peter Zijlstra <peterz@infradead.org>
> Date:   Sat Mar 21 12:26:01 2020 +0100
> 
>     lockdep: Introduce wait-type checks
> 
>     Extend lockdep to validate lock wait-type context.
> 
> and selectable via "config PROVE_RAW_LOCK_NESTING".

OK, thanks. Yes, that explains it.

>>
>> I don't know what the right fix is. Am I right when a say that for !RT,
>> spinlock==raw_spinlock? If so, switching regmap's spinlock to
>> raw_spinlock would be nop for !RT and fix this issue, but would of
>> course have quite far-reaching effects on RT kernels.
> 
> far-reaching? Explain?

I was "suggesting" (or more accurately just considering the implications
of) _switching_ the core regmap code to use a raw spinlock
unconditionally for its "fast io" case. A change that would affect all
such regmaps throughout the kernel. Thus far-reaching (though IIUC a nop
for !RT).

> It is _a_single_register_, accessed once per IRQ line, all at consumer driver probe time
> (typically boot time).
> 

I know, I wrote the driver, and our platform is also a ls1021a.

> We are not switching regmap from normal to raw spinlocks, we are just
> adding the option for raw spinlocks for this one register.
> 
>> Perhaps it's silly for the driver to use syscon's regmap to access that
>> single register, maybe it should just iomap it itself, and protect
>> access with a raw_spinlock of its own. While syscon's regmap would cover
>> that intpcr register, nobody else would ever access it, so that should
>> work fine.
> 
> I believe my competence ends here, I will re-read Arnd's email too, but
> I just don't see how the syscon API gives you something that is "not a
> regmap", something you can ioremap yourself as a consumer driver.

Surely the driver could be rewritten to be completely ignorant of the
containing scfg node and just iomap the register itself.

Yes, there'd probably also be a syscon driver with a regmap covering
                     reg = <0x0 0x1570000 0x0 0x10000>, but none of the
potential other consumers of that regmap would write to intpcr through
that regmap, since intpcr is solely of use to ls-extirq. So the
ls-extirq driver could just have its own raw spinlock in "struct
ls_extirq_data" next to a "void __iomem *" pointer, replacing the
"struct regmap *syscon;" member.

Rasmus