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

From: Roger Lu <roger.lu@mediatek.com>
To: AngeloGioacchino Del Regno 
	<angelogioacchino.delregno@collabora.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Enric Balletbo Serra <eballetbo@gmail.com>,
	Kevin Hilman <khilman@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Nicolas Boichat <drinkcat@google.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Philipp Zabel <p.zabel@pengutronix.de>
Cc: Fan Chen <fan.chen@mediatek.com>,
	HenryC Chen <HenryC.Chen@mediatek.com>,
	Xiaoqing Liu <Xiaoqing.Liu@mediatek.com>,
	Charles Yang <Charles.Yang@mediatek.com>,
	Angus Lin <Angus.Lin@mediatek.com>,
	Mark Rutland <mark.rutland@arm.com>, Nishanth Menon <nm@ti.com>,
	<devicetree@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-mediatek@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>, <linux-pm@vger.kernel.org>,
	<Project_Global_Chrome_Upstream_Group@mediatek.com>,
	Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH v22 5/7] soc: mediatek: SVS: add debug commands
Date: Tue, 15 Feb 2022 17:08:45 +0800	[thread overview]
Message-ID: <eb4a903b90020e8220768e9bb674b9de477006e3.camel@mediatek.com> (raw)
In-Reply-To: <0846872b-03da-ee5d-6a9d-e6c9fa754191@collabora.com>

Hi AngeloGioacchino,

Excuse me for the late reply.

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@mediatek.com>
> 
> 
> 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.

> 
> Regards,
> Angelo