linux-kernel - Re: [PATCH v2 5/6] memory: mtk-smi: Add sleep ctrl function

Open Source and information security mailing list archives

Date:   Thu, 13 Jan 2022 14:02:37 +0800
From:   Yong Wu <yong.wu@...iatek.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
        Rob Herring <robh+dt@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>
CC:     Krzysztof Kozlowski <krzk@...nel.org>,
        Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        "Tomasz Figa" <tfiga@...omium.org>,
        <linux-mediatek@...ts.infradead.org>,
        <srv_heupstream@...iatek.com>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <iommu@...ts.linux-foundation.org>, <youlin.pei@...iatek.com>,
        <anan.sun@...iatek.com>, <lc.kan@...iatek.com>,
        <yi.kuo@...iatek.com>, <anthony.huang@...iatek.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH v2 5/6] memory: mtk-smi: Add sleep ctrl function

On Wed, 2022-01-12 at 11:27 +0100, Krzysztof Kozlowski wrote:
> On 11/01/2022 07:39, Yong Wu wrote:
> > Sleep control means that when the larb goes to sleep, we should
> > wait a bit
> > until all the current commands are finished. Thus, when the larb
> > runtime
> > suspends, we need to enable this function to wait until all the
> > existed
> > commands are finished. When the larb resumes, just disable this
> > function.
> > This function only improves the safety of bus. Add a new flag for
> > this
> > function. Prepare for mt8186.
> > 
> > Signed-off-by: Anan Sun <anan.sun@...iatek.com>
> > Signed-off-by: Yong Wu <yong.wu@...iatek.com>
> > ---
> >  drivers/memory/mtk-smi.c | 35 ++++++++++++++++++++++++++++++++++-
> >  1 file changed, 34 insertions(+), 1 deletion(-)

[...]

> > @@ -492,9 +521,13 @@ static int __maybe_unused
> > mtk_smi_larb_resume(struct device *dev)
> >  static int __maybe_unused mtk_smi_larb_suspend(struct device *dev)
> >  {
> >  	struct mtk_smi_larb *larb = dev_get_drvdata(dev);
> > +	int ret = 0;
> > +
> > +	if (MTK_SMI_CAPS(larb->larb_gen->flags_general,
> > MTK_SMI_FLAG_SLEEP_CTL))
> > +		ret = mtk_smi_larb_sleep_ctrl_enable(larb);
> >  
> >  	clk_bulk_disable_unprepare(larb->smi.clk_num, larb->smi.clks);
> > -	return 0;
> > +	return ret;
> 
> I am wondering whether disabling clocks in error case is a proper
> step.
> On suspend error, the PM core won't run any further callbacks on this
> device. This means, it won't be resumed and your clocks stay
> disabled. I
> think you should return early and leave the device in active state in
> case of error.

oh, Yes. Thanks for pointing this out.

I will fix this in next version.

> 
> 
> Best regards,
> Krzysztof
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives