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

[Xingyu Chen]

Hi, Guenter

On 2019/10/21 21:38, Guenter Roeck wrote:
> 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.
Thanks, fix it in next version.

> [ ... ]
>
> > > > +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, Thanks&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, fix it in next version.
> Thanks,
> Guenter
>
> .