Re: clk: qcom: genpd lockdep warning in gdsc

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 27/10/2022 01:18, Stephen Boyd wrote:
> Reviving this old thread because this commit has lead to a couple bugs
> now.
> 
> Quoting Ulf Hansson (2022-06-22 03:26:52)
>> On Fri, 17 Jun 2022 at 21:58, Stephen Boyd <swboyd@chromium.org> wrote:
>>>
>>> Hi Bjorn and Dmitry,
>>>
>>> Yu reported a lockdep warning coming from the gdsc driver. It looks like
>>> the runtime PM usage in gdsc.c is causing lockdep to see an AA deadlock
>>> possibility with 'genpd->mlock'. I suspect this is because we have
>>> commit 1b771839de05 ("clk: qcom: gdsc: enable optional power domain
>>> support"), and that is now calling runtime PM code from within the genpd
>>> code.
> 
> This commit has caused a deadlock at boot for Doug[1] and I see that the
> camera driver on Google CoachZ and Wormdingler devices doesn't work
> after resuming from suspend once this commit is applied. I'm leaning
> towards sending a revert, because it seems to cause 3 issues while
> removing the regulator hack that was in place to enable MMCX. This patch
> is cleaning up the hack, but trading the hack for three more problems.
> 
>> I think genpd already has nested lock support, so the only
>>> solution is to not use runtime PM from within genpd code and start
>>> expressing genpd parent relationships in genpd itself?
>>
>> Not sure exactly what you mean here, but yes, expressing the
>> parent/child domain relationship is always needed.
>>
>> Having gdsc_disable() to do runtime PM calls (gdsc_pm_runtime_put())
>> seems awkward to me. Why is that needed, more exactly?
> 
> It seems like this is needed so that the gdsc_enable() and
> gdsc_disable() calls can read/write the registers for the genpd, while
> those registers live in some clk controller that needs either a
> different clk (probably some AHB clk) or another genpd to be enabled. It
> looks like for qcom,sm8250-dispcc it relies on MMCX gdsc (which is a
> genpd). From a hardware view, the MDSS_GDSC provided by the display clk
> controller is probably not a sub-domain of MMCX. Instead, we need to
> have MMCX enabled so that we can access the registers for the MDSS GDSC.

Yes, exactly.

> 
> My question is if it makes sense to simply describe that the GDSCs
> provided by a device are sub-domains of whatever power domains are
> listed in DT for that device? I think if we did that here for sm8250
> dispcc, we wouldn't need to use runtime PM within the genpd code
> assuming that the MMCX parent genpd is enabled before we attempt to
> read/write the dispcc gdsc registers. Hopefully that is also done, i.e.
> enabling parent domains before enabling child domains if the parent is
> disabled.

I will check this tomorrow. It should be possible to handle the 
MMCX/MDSS_GDSC relationship in this way.

> Is this already being done with pm_genpd_add_subdomain() in
> gdsc_register()? I see that we're attaching that to dispcc's struct
> device::pm_domain, but I assume that is different from the MMCX genpd.

No, I think the only domain there is the MMCX domain, so this call s

> Maybe that is the problem here. Dmitry can you further describe the
> problem being solved?

I must admit, I don't remember what caused me to add this call. May be 
it was added before me getting the pm_genpd_add_subdomain() call in place.

> 
>>
>>> Or maybe genpd
>>> needs to drop locks while calling down into gdsc_disable() and reacquire
>>> them after that?
>>
>> Nope, that doesn't work. This internals of genpd relies on this
>> behaviour, as it allows it to properly deal with power-on|off for
>> parent/child domains, for example.
> 
> Ok.
> 
> [1] https://lore.kernel.org/r/20220922154354.2486595-1-dianders@chromium.org

-- 
With best wishes
Dmitry