Re: [RFC PATCH 0/3] Introduce SCMI Quirks framework

[Cristian Marussi]

On Fri, Apr 04, 2025 at 03:22:57PM +0200, Johan Hovold wrote:
> Hi Cristian,
> 
> On Tue, Apr 01, 2025 at 01:25:42PM +0100, Cristian Marussi wrote:
> 
> > with the increasing adoption of SCMI across arm64 ecosystems, we have to
> > start considering how to deal and take care of out-of-spec SCMI firmware
> > platforms that are actively deployed in the wild, in a consistent manner.
> > 
> > This small series introduces a simple framework, based on static_keys,
> > that allows a user to:
> > 
> > - define a quirk and its matching conditions; quirks can match based on:
> > 
> >   	compatible / Vendor_ID / Sub_Vendor_ID / ImplVersion
> > 
> >   from the longest matching sequence down to the shortest.
> > 
> >   When the SCMI core stack boots it will enable the matching quirks
> >   depending on the information gathered from the platform via Base
> >   protocol: any NULL/0 match condition is ignored during matching and is
> >   interpreted as ANY, so you can decide to match on a very specific
> >   combination of compatibles and FW versions OR simply on a compatible.
> > 
> > - define a quirk code-block: simply a block of code, meant to play the
> >   magic quirk trick, defined in the proximity of where it will be used
> >   and gated by an implicit quirk static-key associated with the defined
> >   quirk
> 
> > Any feedback and testing is very much welcome.
> 
> I'm hitting the below lockdep splat when running with this series and
> the FC quirk enabled.
> 
> Johan
> 
> 
> [    8.399581] ======================================================
> [    8.399582] WARNING: possible circular locking dependency detected
> [    8.399583] 6.14.0 #103 Not tainted
> [    8.399584] ------------------------------------------------------
> [    8.399584] kworker/u49:5/165 is trying to acquire lock:
> [    8.399586] ffff65d004d36320 (&info->protocols_mtx){+.+.}-{4:4}, at: scmi_get_protocol_instance+0x4c/0x5c0
> [    8.399596] 
>                but task is already holding lock:
> [    8.399596] ffff65d00a895348 (&ni->pending_mtx){+.+.}-{4:4}, at: __scmi_event_handler_get_ops+0x70/0x47c
> [    8.399600] 
>                which lock already depends on the new lock.
> 
> [    8.399601] 
>                the existing dependency chain (in reverse order) is:
> [    8.399601] 
>                -> #4 (&ni->pending_mtx){+.+.}-{4:4}:

Hi Johan,

I have reproduced this when running with LOCKDEP and quirks enabled:
it was due to the fact that static_branch_enable() takes a cpu_lock
internally and I was triggering all of this from Base protocol init
which takes a number of other mutexes...

I moved SCMI quirk initialiation after Base protocol initialization
in scmi_probe and it is gone...and indeed was not needed anyway so
early.

I'll post a new V1 for quirks in a short while including this change.

Thanks,
Cristian