Re: [PATCH v22 5/7] soc: mediatek: SVS: add debug commands

[AngeloGioacchino Del Regno]

Il 15/02/22 10:08, Roger Lu ha scritto:
> Hi AngeloGioacchino,
>
> Excuse me for the late reply.

Hi Roger,
no worries about that.

> On Mon, 2022-01-31 at 12:11 +0100, AngeloGioacchino Del Regno wrote:
> > Il 27/01/22 04:39, 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@xxxxxxxxxxxx>
> >
> >
> > Hello Roger,
> > I was thinking about what this patch is adding... and I have a few
> > considerations.
> >
> > It's nice to have a debugging mechanism to read the status and dump registers,
> > as
> > that's very helpful when doing heavy debugging of the IP... but adding the
> > possibility to write a voltage offset may be very dangerous: think about the
> > case
> > in which, either for misconfiguration, or for any other reason, the debugfs
> > entry
> > that allows writing voffset becomes user-writable, or a user writes an
> > impossibly
> > high voffset.
> > In case a very low (negative) voffset is entered, the platform would crash
> > (denial
> > of service); if a very high voffset is entered, hardware damage may occur.
> >
> > For this reason, there are two proposals:
> > 1. If you want to keep the debugfs voffset write, please constrain the
> > permissible
> >      voffset to an acceptable range that at least makes it unlikely to damage
> > the HW;
> >      Moreover, since voffset write is a feature that would be used in very
> > limited
> >      debugging cases, I think that this should be implemented over a build-time
> >      configuration barrier... something like CONFIG_MTK_SVS_DEBUG_ALLOW_WRITE,
> > or
> >      similar;
> > 2. Since it's very unlikely for someone to really play that much with a
> > voltage
> >      offset during runtime, and since this looks like something very machine
> > specific
> >      (perhaps addressing board-specific quirks?), I would suggest to add this
> > as a
> >      device-tree parameter instead, such as "mediatek,svs-voffset", as it is
> > indeed
> >      possible to specify both positive or negative values in DT.
> >
> > I would prefer proposal 2, as it looks generally cleaner and way less risky.
>
> Thanks for raising the considerations and give these great suggestions for us to
> think about. Since these voffset read/write commands are used seldomly, we
> decide to remove them for better system security.

Thank you for this ack, very much appreciated.
Eager to see v23!


> > Regards,
> > Angelo