Messages in this thread | | | Date | Tue, 15 Feb 2022 10:10:57 +0100 | Subject | Re: [PATCH v22 5/7] soc: mediatek: SVS: add debug commands | From | 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@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. >
Thank you for this ack, very much appreciated. Eager to see v23!
>> >> Regards, >> Angelo >
| |