| 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
| ||
|
Message-ID: <CANMq1KDxVLo=JAAq-gjybke1WiX03COwNX7sHsZDMF9USzSECw@mail.gmail.com>
Date: Mon, 4 Jan 2021 17:27:49 +0800
From: Nicolas Boichat <drinkcat@...omium.org>
To: Roger Lu <roger.lu@...iatek.com>
Cc: 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>,
Mark Rutland <mark.rutland@....com>,
Nishanth Menon <nm@...com>, Angus Lin <Angus.Lin@...iatek.com>,
Devicetree List <devicetree@...r.kernel.org>,
"open list:THERMAL" <linux-pm@...r.kernel.org>,
lkml <linux-kernel@...r.kernel.org>,
Xiaoqing Liu <Xiaoqing.Liu@...iatek.com>,
YT Lee <yt.lee@...iatek.com>, Fan Chen <fan.chen@...iatek.com>,
"moderated list:ARM/Mediatek SoC support"
<linux-mediatek@...ts.infradead.org>,
HenryC Chen <HenryC.Chen@...iatek.com>,
Charles Yang <Charles.Yang@...iatek.com>,
linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v10 3/7] [v10, 3/7]: soc: mediatek: SVS: introduce MTK SVS engine
On Mon, Jan 4, 2021 at 4:51 PM Roger Lu <roger.lu@...iatek.com> wrote:
>
>
> Hi Nicolas,
>
> Thanks for all the advices.
>
> On Thu, 2020-12-31 at 10:10 +0800, Nicolas Boichat wrote:
> > On Sun, Dec 27, 2020 at 6:55 PM Roger Lu <roger.lu@...iatek.com> wrote:
[snip]
> > > +static int svs_adjust_pm_opp_volts(struct svs_bank *svsb, bool force_update)
> > > +{
> > > + int tzone_temp, ret = -EPERM;
> >
> > No need to initialize ret.
>
> Oh, excuse me, some coding check tool warn that this `ret` might return
> without being uninitialized. Therefore, I'll keep the initialization.
Oh, you're right, there is a possible path where ret is not set. sgtm then.
>
> >
> > > + u32 i, svsb_volt, opp_volt, temp_offset = 0;
> > > +
> > > + mutex_lock(&svsb->lock);
> > > +
> > > + /*
> > > + * If svs bank is suspended, it means signed-off voltages are applied.
> > > + * Don't need to update opp voltage anymore.
> > > + */
> > > + if (svsb->suspended && !force_update) {
> > > + dev_notice(svsb->dev, "bank is suspended\n");
> > > + ret = -EPERM;
> > > + goto unlock_mutex;
> > > + }
> > > +
> > > + /* Get thermal effect */
> > > + if (svsb->phase == SVSB_PHASE_MON) {
> > > + if (svsb->temp > svsb->temp_upper_bound &&
> > > + svsb->temp < svsb->temp_lower_bound) {
> > > + dev_warn(svsb->dev, "svsb temp = 0x%x?\n", svsb->temp);
> > > + ret = -EINVAL;
> > > + goto unlock_mutex;
> > > + }
> > > +
> > > + ret = svs_get_bank_zone_temperature(svsb->tzone_name,
> > > + &tzone_temp);
> > > + if (ret) {
> > > + dev_err(svsb->dev, "no \"%s\"?(%d)?\n",
> > > + svsb->tzone_name, ret);
> > > + dev_err(svsb->dev, "set signed-off voltage\n");
> >
> > Please merge the error message in one line (I'm not sure what "set
> > signed-off voltage" means here).
>
> 1. Ok, I'll merge them. Thanks.
> 2. signed-off voltages means CPU DVFS default voltages
So just write "default voltages" then? ,-)
>
> >
[snip]
> > > +static irqreturn_t svs_isr(int irq, void *data)
> > > +{
> > > + struct svs_platform *svsp = (struct svs_platform *)data;
> >
> > cast not needed.
>
> Ok, I'll remove it. Thanks.
>
> >
> > > + struct svs_bank *svsb = NULL;
> > > + unsigned long flags;
> > > + u32 idx, int_sts, svs_en;
> > > +
> > > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > > + svsb = &svsp->banks[idx];
> > > +
> > > + spin_lock_irqsave(&mtk_svs_lock, flags);
> > > + svsp->pbank = svsb;
> > > +
> > > + /* Find out which svs bank fires interrupt */
> > > + if (svsb->int_st & svs_readl(svsp, INTST)) {
> > > + spin_unlock_irqrestore(&mtk_svs_lock, flags);
> > > + continue;
> > > + }
> > > +
> > > + if (!svsb->suspended) {
> > > + svs_switch_bank(svsp);
> > > + int_sts = svs_readl(svsp, INTSTS);
> > > + svs_en = svs_readl(svsp, SVSEN);
> > > +
> > > + if (int_sts == SVSB_INTSTS_COMPLETE &&
> > > + ((svs_en & SVSB_EN_MASK) == SVSB_EN_INIT01))
> > > + svs_init01_isr_handler(svsp);
> > > + else if ((int_sts == SVSB_INTSTS_COMPLETE) &&
> > > + ((svs_en & SVSB_EN_MASK) == SVSB_EN_INIT02))
> > > + svs_init02_isr_handler(svsp);
> > > + else if (!!(int_sts & SVSB_INTSTS_MONVOP))
> >
> > !! is not required.
>
> Ok, I'll remove it. Thanks.
>
> >
> > > + svs_mon_mode_isr_handler(svsp);
> > > + else
> > > + svs_error_isr_handler(svsp);
> > > + }
> > > +
> > > + spin_unlock_irqrestore(&mtk_svs_lock, flags);
> > > + break;
> > > + }
> >
> > This will panic if svsb is NULL, is that ok or do you want to catch that?
>
> Oh, it is fine. Thanks for the heads-up.
I should have been stronger in my statement, I think you want to add a
BUG_ON(!svsb) to crash in a slightly more predictable manner.
[snip]
> > > +
> > > + svsp->tefuse = (u32 *)nvmem_cell_read(cell, &svsp->tefuse_num);
> >
> > Cast not needed.
>
> Ok, I'll remove it if build/test ok. Because nvmem_cell_read returns
> (void *).
>
> >
> > Also, this need to be freed somewhere in remove code (kfree(svsp->tefuse)).
> >
> > And it seems like svsp->tefuse is only used in this function, can you
> > just allocate it here?
>
> Oh, svsp->tefuse will be used in SVS debug patch for debug purpose. So,
> I need to save it as struct member.
Oh I missed that, sgtm then. Thanks.
>
[snip]
Powered by blists - more mailing lists