Re: [PATCH v4 05/23] interconnect: icc-clk: add support for scaling using OPP

[Stephan Gerhold <stephan@gerhold.net>]

On Tue, Oct 03, 2023 at 04:36:11PM +0300, Dmitry Baryshkov wrote:
> On Tue, 3 Oct 2023 at 16:02, Stephan Gerhold <stephan@gerhold.net> wrote:
> >
> > On Tue, Oct 03, 2023 at 11:30:28AM +0300, Dmitry Baryshkov wrote:
> > > On 28/08/2023 21:09, Stephen Boyd wrote:
> > > > Quoting Dmitry Baryshkov (2023-08-27 04:50:15)
> > > > > diff --git a/drivers/interconnect/icc-clk.c b/drivers/interconnect/icc-clk.c
> > > > > index d787f2ea36d9..45ffb068979d 100644
> > > > > --- a/drivers/interconnect/icc-clk.c
> > > > > +++ b/drivers/interconnect/icc-clk.c
> > > > > @@ -25,12 +28,16 @@ struct icc_clk_provider {
> > > > >   static int icc_clk_set(struct icc_node *src, struct icc_node *dst)
> > > > >   {
> > > > >          struct icc_clk_node *qn = src->data;
> > > > > +       unsigned long rate = icc_units_to_bps(src->peak_bw);
> > > > >          int ret;
> > > > >          if (!qn || !qn->clk)
> > > > >                  return 0;
> > > > > -       if (!src->peak_bw) {
> > > > > +       if (qn->opp)
> > > > > +               return dev_pm_opp_set_rate(qn->dev, rate);
> > > >
> > > > Just curious how does lockdep do with this? Doesn't OPP call into
> > > > interconnect code, so lockdep will complain about ABBA?
> > >
> > > Unfortunately it does. It seems, the icc-clk is not a proper way to go here.
> > > I will take a look at reusing set_required_opps for this case.
> > >
> >
> > Could you elaborate a bit which locks exactly cause trouble here?
> > I'm probably missing something here.
> >
> > From a quick look at the OPP code I don't see a global lock taken there
> > for the entire OPP switch sequence, so I'm not sure how this could cause
> > an ABBA deadlock.
> 
> For example:
> 
> [    7.680041] Chain exists of:
> [    7.680041]   icc_bw_lock --> regulator_ww_class_acquire --> fs_reclaim
> [    7.680041]
> [    7.687955]  Possible unsafe locking scenario:
> [    7.687955]
> [    7.699039]        CPU0                    CPU1
> [    7.704752]        ----                    ----
> [    7.709266]   lock(fs_reclaim);
> [    7.713779]                                lock(regulator_ww_class_acquire);
> [    7.716919]                                lock(fs_reclaim);
> [    7.724204]   lock(icc_bw_lock);
> 

Hm, I'm not entirely sure how to interpret this. Is there really a
connection between OPP and ICC here? It looks more like ICC <->
regulator and somehow related to memory allocations (the fs_reclaim).

Was there more output around this?

In general, I would expect that adjusting a regulator from an
interconnect driver should be made possible somehow. Just because the
RPM firmware or similar typically handles this internally on Qualcomm
platforms doesn't mean no one else will ever need to do this. If you
have a higher bandwidth requests, need to increase the clock, which
again depends on a higher voltage, then you need to be able to do the
regulator_set_voltage() from the ICC driver somehow.

Thanks,
Stephan