Re: [PATCH v2 3/4] watchdog: add meson secure watchdog driver

From: Guenter Roeck
Date: Mon Oct 21 2019 - 09:38:07 EST


On 10/21/19 1:03 AM, Xingyu Chen wrote:
Hi, Guenter

On 2019/10/21 0:56, Guenter Roeck wrote:
On 10/18/19 1:33 AM, Xingyu Chen wrote:
The watchdog controller on the Meson-A/C series SoCs is moved to secure
world, watchdog operation needs to be done in secure EL3 mode via ATF,
Non-secure world can call SMC instruction to trap to AFT for watchdog
operation.

Signed-off-by: Xingyu Chen <xingyu.chen@xxxxxxxxxxx>
---
 drivers/watchdog/Kconfig | 17 ++++
 drivers/watchdog/Makefile | 1 +
 drivers/watchdog/meson_sec_wdt.c | 187 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 205 insertions(+)
 create mode 100644 drivers/watchdog/meson_sec_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 58e7c10..e84be42 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -826,6 +826,23 @@ config MESON_GXBB_WATCHDOG
ÂÂÂÂÂÂÂ To compile this driver as a module, choose M here: the
ÂÂÂÂÂÂÂ module will be called meson_gxbb_wdt.
+config MESON_SEC_WATCHDOG
+ÂÂÂ tristate "Amlogic Meson Secure watchdog support"
+ÂÂÂ depends on MESON_SM
+ÂÂÂ depends on ARCH_MESON || COMPILE_TEST

This dependency is pointless. MESON_SM already depends on ARCH_MESON,
thus specifying "COMPILE_TEST" here adds no value but only
creates confusion.
Thanks for your analysis, perhaps i should remove the line below.
- depends on ARCH_MESON || COMPILE_TEST

Is it ok to modify code above like this ?

Yes.
[ ... ]

+static unsigned int meson_sec_wdt_get_timeleft(struct watchdog_device *wdt_dev)
+{
+ÂÂÂ int ret;
+ÂÂÂ unsigned int timeleft;
+ÂÂÂ struct meson_sec_wdt *data = watchdog_get_drvdata(wdt_dev);
+
+ÂÂÂ ret = meson_sm_call(data->fw, SM_WATCHDOG_OPS, &timeleft,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ MESON_SIP_WDT_GETTIMELEFT, 0, 0, 0, 0);
+
+ÂÂÂ if (ret)
+ÂÂÂÂÂÂÂ return ret;

Meh, that doesn't work. I just realized that the return type is unsigned,
so returning a negative error code is pointless. Guess we'll have to
live with returning 0 in this case after all. I wonder if we should
fix the API and return an integer (with negative error code), but that
is a different question.
Thanks for your review.

IMO, if returning an integer, and the value which copy to user buf should be formatted with %d instead of %u (see timeleft_show), it will cause the max value of timeleft is reduced from 4294967295 to 2147483647. but i'am not sure whether it will bring risk.

Not that it matters right now, but I don't think that limiting 'timeleft'
reporting to 2147483647 seconds, or ~68 years, would cause any risk.
It would just be a large patch changing several drivers all at once,
that is all.


So i also think returning 0 may be better in this case.

Yes, please do that.

Thanks,
Guenter