linux-kernel - Re: [PATCH v21 5/8] soc: mediatek: SVS: add debug commands

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2404cea17479df35b5a5d55a923a96b10ebae909.camel@mediatek.com>
Date:   Mon, 24 Jan 2022 18:40:15 +0800
From:   Roger Lu <roger.lu@...iatek.com>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Enric Balletbo Serra <eballetbo@...il.com>,
        Kevin Hilman <khilman@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Nicolas Boichat <drinkcat@...gle.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>
CC:     Fan Chen <fan.chen@...iatek.com>,
        HenryC Chen <HenryC.Chen@...iatek.com>,
        YT Lee <yt.lee@...iatek.com>,
        Xiaoqing Liu <Xiaoqing.Liu@...iatek.com>,
        Charles Yang <Charles.Yang@...iatek.com>,
        Angus Lin <Angus.Lin@...iatek.com>,
        Mark Rutland <mark.rutland@....com>,
        Nishanth Menon <nm@...com>, <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-mediatek@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <linux-pm@...r.kernel.org>,
        <Project_Global_Chrome_Upstream_Group@...iatek.com>,
        Guenter Roeck <linux@...ck-us.net>
Subject: Re: [PATCH v21 5/8] soc: mediatek: SVS: add debug commands

Hi AngeloGioacchino,

Sorry for the late reply and thanks for the advice.

On Fri, 2022-01-07 at 15:34 +0100, AngeloGioacchino Del Regno wrote:
> Il 07/01/22 10:51, Roger Lu ha scritto:
> > The purpose of SVS is to help find the suitable voltages
> > for DVFS. Therefore, if SVS bank voltages are concerned
> > to be wrong, we can adjust SVS bank voltages by this patch.
> > 
> > Signed-off-by: Roger Lu <roger.lu@...iatek.com>
> > ---
> >   drivers/soc/mediatek/mtk-svs.c | 321 ++++++++++++++++++++++++++++++++-
> >   1 file changed, 318 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> > index 042c6e8e9069..93cdaecadd6d 100644
> > --- a/drivers/soc/mediatek/mtk-svs.c
> > +++ b/drivers/soc/mediatek/mtk-svs.c
> 
> ..snip..
> 
> > @@ -605,6 +896,16 @@ static void svs_set_bank_phase(struct svs_platform
> > *svsp,
> >   	}
> >   }
> >   
> > +static inline void svs_save_bank_register_data(struct svs_platform *svsp,
> > +					       enum svsb_phase phase)
> > +{
> > +	struct svs_bank *svsb = svsp->pbank;
> > +	enum svs_reg_index rg_i;
> > +
> 
> I think that it'd be a good idea to add an `enable` parameter, so that we
> don't always do a register dump; after all, this is a debugging feature and
> it's going to be completely irrelevant to the user, so keeping this disabled
> by default would ensure to get no performance degradation (even if small)
> unless really wanted.
> 
> So, in this case, here we'd have
> 
> 	if (!svsp->debug_enabled)
> 		return;

Thanks for pointing out the concern. Excuse us, we really need this to be
enabled by default. If we add a enable flag here, we'll face below problems and
make debug more difficult.

1. If we enable it afterward by cmd, init0[1~2] registers' data cannot be
recorded expectedly because the init flow has been finished already and won't be
run anymore. So, it doesn't work by using cmd to enable the flag.
2. If we add a enable flag here, it means we have to re-build the kernel load in
order to enable this flag. However, we cannot re-build the kernel load and needs
to debug directly sometimes. It's a sad situation... :(

> 
> > +	for (rg_i = DESCHAR; rg_i < SVS_REG_MAX; rg_i++)
> > +		svsb->reg_data[phase][rg_i] = svs_readl_relaxed(svsp, rg_i);
> > +}
> > +
> 
> Of course, this implies adding a new debugfs entry to enable/disable the
> debugging.
> Everything else looks good :)

Oh, excuse us, we have to keep the old design for better instant support and
thanks for the understanding.

> >   static inline void svs_error_isr_handler(struct svs_platform *svsp)
> >   {
> >   	struct svs_bank *svsb = svsp->pbank;
> > @@ -619,6 +920,8 @@ static inline void svs_error_isr_handler(struct
> > svs_platform *svsp)
> >   		svs_readl_relaxed(svsp, SMSTATE1));
> >   	dev_err(svsb->dev, "TEMP = 0x%08x\n", svs_readl_relaxed(svsp, TEMP));
> >   
> > +	svs_save_bank_register_data(svsp, SVSB_PHASE_ERROR);
> > +
> >   	svsb->mode_support = SVSB_MODE_ALL_DISABLE;
> >   	svsb->phase = SVSB_PHASE_ERROR;

[snip]