linux-kernel - Re: [PATCH 1/3] regulator: s2mps11: Refactor setting ramp delay

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]

Date:	Tue, 27 May 2014 08:57:18 +0200
From:	Krzysztof Kozlowski <k.kozlowski@...sung.com>
To:	Yadwinder Singh Brar <yadi.brar01@...il.com>
Cc:	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	Sangbeom Kim <sbkim73@...sung.com>,
	Lee Jones <lee.jones@...aro.org>,
	Sachin Kamat <sachin.kamat@...aro.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	linux-samsung-soc <linux-samsung-soc@...r.kernel.org>,
	Tomasz Figa <t.figa@...sung.com>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
	devicetree <devicetree@...r.kernel.org>
Subject: Re: [PATCH 1/3] regulator: s2mps11: Refactor setting ramp delay

On wto, 2014-05-27 at 11:56 +0530, Yadwinder Singh Brar wrote:
> Hi Krzysztof,
> 
> 
> On Mon, May 26, 2014 at 6:50 PM, Krzysztof Kozlowski
> <k.kozlowski@...sung.com> wrote:
> > Prepare for merging the s2mpa01 regulator driver into s2mps11 by:
> > 1. Adding common id for buck regulators.
> > 2. Splitting shared ramp delay settings to match S2MPA01.
> > 3. Adding a configuration of registers for setting ramp delay for each
> >    buck regulator.
> >
> > The functionality of the driver should not change as this patch only
> > prepares for supporting S2MPA01 device.
> >
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@...sung.com>
> > ---
> >  drivers/regulator/s2mps11.c | 210 ++++++++++++++++++++++++++++++--------------
> >  1 file changed, 144 insertions(+), 66 deletions(-)
> >
> 
> [snip]
> 
> >
> > -               if (ramp_delay > s2mps11->ramp_delay34)
> > -                       s2mps11->ramp_delay34 = ramp_delay;
> > +               if (ramp_delay > s2mps11->ramp_delay3)
> > +                       s2mps11->ramp_delay3 = ramp_delay;
> >                 else
> > -                       ramp_delay = s2mps11->ramp_delay34;
> > -
> > -               ramp_shift = S2MPS11_BUCK34_RAMP_SHIFT;
> > -               ramp_reg = S2MPS11_REG_RAMP;
> > +                       ramp_delay = s2mps11->ramp_delay3;
> >                 break;
> >         case S2MPS11_BUCK4:
> > -               enable_shift = S2MPS11_BUCK4_RAMP_EN_SHIFT;
> >                 if (!ramp_delay) {
> >                         ramp_enable = 0;
> >                         break;
> >                 }
> >
> > -               if (ramp_delay > s2mps11->ramp_delay34)
> > -                       s2mps11->ramp_delay34 = ramp_delay;
> > +               if (ramp_delay > s2mps11->ramp_delay4)
> > +                       s2mps11->ramp_delay4 = ramp_delay;
> >                 else
> > -                       ramp_delay = s2mps11->ramp_delay34;
> > -
> > -               ramp_shift = S2MPS11_BUCK34_RAMP_SHIFT;
> > -               ramp_reg = S2MPS11_REG_RAMP;
> > +                       ramp_delay = s2mps11->ramp_delay4;
> 
> Main rationale behind shared value is completely omitted here, in
> other cases also,
> after just giving a NOTE in documentation asking user to make sure to
> pass same value.
> It doesn't seem safe, simply leaving a scope of stability issue (in
> case ramp_delay3 > ramp_delay4).

The documentation already states that these bucks (e.g. 3 and 4) share
the ramp delay setting and 'regulator-ramp-delay' should be the same.
However you're right that patch is not safe against changing shared ramp
delays to different values. Previously the code was safe so this is not
entirely "non-functional" change. I'll fix it to retain the same
behavior.

Best regards,
Krzysztof



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/