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;
Message-ID: <d9e56d6fb6a31ecbd024cfe81e331817fa492331.camel@mediatek.com>
Subject: Re: [PATCH] cpufreq: mediatek: Fix KP and lockups on proc/sram
 regulators error
From:   Jia-Wei Chang <jia-wei.chang@mediatek.com>
To:     Viresh Kumar <viresh.kumar@linaro.org>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>
CC:     <rafael@kernel.org>, <lgirdwood@gmail.com>, <broonie@kernel.org>,
        <matthias.bgg@gmail.com>, <andrew-sh.cheng@mediatek.com>,
        <rex-bc.chen@mediatek.com>, <nfraprado@collabora.com>,
        <linux-pm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-mediatek@lists.infradead.org>
Date:   Fri, 23 Sep 2022 14:38:43 +0800
In-Reply-To: <20220921071913.p7kwsjnnuad2jgvk@vireshk-i7>
References: <20220909093724.40078-1-angelogioacchino.delregno@collabora.com>
         <20220921071913.p7kwsjnnuad2jgvk@vireshk-i7>
Content-Type: text/plain; charset="UTF-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 2022-09-21 at 12:49 +0530, Viresh Kumar wrote:
> Jia, do you want to reply to this thread as the Fixes patch was added
> by you ?
> 
> On 09-09-22, 11:37, AngeloGioacchino Del Regno wrote:
> > Function regulator_get_optional() returns a negative error number
> > on
> > any kind of regulator_get() failure: failing to check for that in
> > the
> > teardown path will lead to a kernel panic due to a call to function
> > regulator_disable().
> 
> I don't see how this can happen. The code does check if the
> regulators
> are enabled earlier or not.
> 
Hi Angelo,

Could you help provide more details, like the call stack of kernel
panic? and how to reproduce this failure?

> > Besides that, the "proc" regulator does actually provide power to
> > the
> > CPU cluster(s): disabling it will produce a lockup on at least some
> > SoCs, such as MT8173.
> 
> We are just dropping the count that we increased earlier, how will
> that disable the regulator which was already enabled ?
> 
> > That consideration is also valid for the "sram" regulator,
> > providing
> > power to the CPU caches instead, present on some other SoCs, such
> > as
> > MT8183, MT8186 (and others).
> > 
> > Resolve both situations and by simply removing the entire faulty
> > branches responsible for disabling the aforementioned regulators if
> > enabled, keeping in mind that these are enabled (and left enabled)
> > by the bootloader before booting the kernel.
> 
> This looks fishy, we just keep on increasing the ref count of the
> regulator but never take it down.
> 
Angelo,

Do you mean the ref count of the regulator in the kernel will be
affected if that regulator is enabled earlier in the bootloader?

Thanks.