Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
In-Reply-To: <CAD=FV=Wqu21ErJGwf24mkFcXTZx_vR1r++0cP68vr9FQDY8O-A@mail.gmail.com>
References: <20230329143317.RFC.v2.1.I4e9d433ea26360c06dd1381d091c82bb1a4ce843@changeid>
 <20230329143317.RFC.v2.2.I30d8e1ca10cfbe5403884cdd192253a2e063eb9e@changeid>
 <CAE-0n53Eb1BeDPmjBycXUaQAF4ppiAM6UDWje_jiB9GAmR8MMw@mail.gmail.com> <CAD=FV=Wqu21ErJGwf24mkFcXTZx_vR1r++0cP68vr9FQDY8O-A@mail.gmail.com>
From:   Stephen Boyd <swboyd@chromium.org>
User-Agent: alot/0.10
Date:   Fri, 28 Apr 2023 18:57:36 -0700
Message-ID: <CAE-0n50vofDO6dpEvnetfvXL41m55j7_OXF=JGZ9L34M0xXPDA@mail.gmail.com>
Subject: Re: [RFC PATCH v2 2/2] regulator: core: Avoid lockdep reports when
 resolving supplies
To:     Doug Anderson <dianders@chromium.org>
Cc:     Mark Brown <broonie@kernel.org>,
        Dmitry Osipenko <digetx@gmail.com>,
        Lucas Stach <l.stach@pengutronix.de>,
        David Collins <quic_collinsd@quicinc.com>,
        Liam Girdwood <lgirdwood@gmail.com>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Quoting Doug Anderson (2023-04-13 17:36:09)
> Hi,
>
> On Fri, Apr 7, 2023 at 2:46=E2=80=AFPM Stephen Boyd <swboyd@chromium.org>=
 wrote:
> >
> > Quoting Douglas Anderson (2023-03-29 14:33:54)
> > > @@ -2190,7 +2263,9 @@ struct regulator *_regulator_get(struct device =
*dev, const char *id,
> > >                 return regulator;
> > >         }
> > >
> > > +       regulator_lock(rdev);
> > >         regulator =3D create_regulator(rdev, dev, id);
> > > +       regulator_unlock(rdev);
> >
> > I'm sad that we're now locking the entire time create_regulator() is
> > called. Can that be avoided? I see that create_regulator() publishes th=
e
> > consumer on the consumer_list, but otherwise I don't think it needs to
> > hold the regulator lock. It goes on to call debugfs code after
> > allocating memory. After this patch, we're going to be holding the lock
> > for that regulator across debugfs APIs. I suspect that may lead to more
> > problems later on because the time we hold the lock is extremely wide
> > now.
> >
> > Of course, we were already holding the child regulator's lock for the
> > supply, because that's what this patch is fixing in
> > regulator_resolve_supply(). I'm just nervous that we're holding the loc=
k
> > for a much wider time now. Maybe we can have create_regulator() return
> > the regulator and add a new function like add_regulator_consumer() that
> > does the list modification? Then we can make create_regulator() do
> > everything without holding a lock and have a very short time where the
> > new function locks two regulator locks and does the linkage.
>
> While we could try to come up with something fancier like this, I'm
> not convinced it's worth the complexity. There are already cases where
> we hold multiple regulator locks for quite long periods of time.
> Specifically you can look at regulator_enable(). There, we'll grab the
> lock for the regulator and the locks for all of the regulators parents
> up the chain. Then we'll enable the regulator (and maybe the parents)
> which might even include a long delay/sleep while holding the mutexes
> for the whole chain.

Does that long period of time cover a large section of code? I'm not so
much concerned about holding the lock for a long time. I'm mostly
concerned about holding the lock across the debugfs APIs, and
potentially anything else that debugfs might lock against. If the lock
is only really needed to modify the list in a safe manner then it may be
better to make it a little more complex so that we don't exit the
regulator framework with some rdev locks held.

>
> Mark: do you have any opinion / intuition here? Is holding the rdev
> lock for this larger scope a problem?