RE: [PATCH v1 1/1] platform/mellanox: Add bootctl driver for Mellanox BlueField Soc

[Liming Sun <lsun@mellanox.com>]

Thanks!  v2 has been posted trying to solve all the comments. Please also see response inline.

Regards,
Liming

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Wednesday, January 30, 2019 4:17 PM
> To: Liming Sun <lsun@mellanox.com>
> Cc: Andy Shevchenko <andy@infradead.org>; Darren Hart <dvhart@infradead.org>; Vadim Pasternak <vadimp@mellanox.com>; David
> Woods <dwoods@mellanox.com>; Platform Driver <platform-driver-x86@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>
> Subject: Re: [PATCH v1 1/1] platform/mellanox: Add bootctl driver for Mellanox BlueField Soc
> 
> On Wed, Jan 30, 2019 at 10:47 PM Liming Sun <lsun@mellanox.com> wrote:
> 
> > > First of all, is it a real watchdog with a driver? I think watchdog in
> > > that case should be set up through standard watchdog facilities.
> >
> > This is not a watchdog driver itself. Instead, it provides interface to
> > user-space and use ARM SMC calls to ATF to configure registers and
> > watchdog. I'll update the commit message in v2 to clarify it.
> 
> Hmm... For example Intel MID platforms have SCU (system controller
> unit) that provides a watchdog facility. In the kernel we have a
> watchdog driver for that.
> Can't you do similar for your case?

The process might be different than what's done on the BlueField SoC.

After the boot-partition swapping, the ARM watchdog is not started
right away. It is started during next rebooting by the ATF BL1 code 
(which is the Boot ROM code than couldn't change). The Boot-ROM 
checks some register value which is preserved during rebooting, and 
knows that a boot-partition swapping is in process, thus enable the 
watchdog timer if configured. At this time, no UEFI or Linux is running, 
just the boot ROM code at the very beginning.

I'll add the sequence of such use case in the commit message of v2.

> 
> > > > +       if (mlxbf_bootctl_smc_call1(MLXBF_BOOTCTL_SET_POST_RESET_WDOG,
> > > > +                                   watchdog) < 0)
> > > > +               return -EINVAL;
> > >
> > > If that call returns an error it shouldn't be shadowed here.
> >
> > Not sure I understand this comment correctly or not.
> > This function is defined by DRIVER_ATTR_RW(), which appears to expect
> > ssize_t or an error code as return value according to other examples I saw.
> 
> What is returned by mlx...call1() should be propagated to the actual caller.
> Same comment for all similar cases.

Make sense! Thanks for the explanation. Updated it in v2.

> 
> > > > +       lc_state &= (MLXBF_BOOTCTL_SB_MODE_TEST_MASK |
> > > > +                    MLXBF_BOOTCTL_SB_MODE_SECURE_MASK);
> > >
> > > Better to split like
> > >
> > > xxx =
> > >  (A | B);
> >
> > It seems hard to do "(A | B);" within 80 characters plus the indents.
> 
> Repeating myself, it's still better than your variant for readability.

Done. Managed to shorten the variable name a little bit.
Updated it in v2.

> 
> > > > +       if (res.a0 != 0x89c036b4 || res.a1 != 0x11e6e7d7 ||
> > > > +           res.a2 != 0x1a009787 || res.a3 != 0xc4bf00ca)
> > >
> > > What is this?!
> > >
> > > Can you use UUID API?
> >
> > Yes, it is UUID comparison. The SMC call returns four 'unsigned long' from ATF
> > to represent the UUID. There seems no existing APIs in uapi/linux/uuid.h to
> > compare such special format. How about replacing it with comment and MACROs
> > instead of the hardcoded values to make it more readable?
> 
> Should be no magic numbers involved inside the function at the end.
> Use descriptive definitions and I still recommend to give a look at
> UUID API how it can be utilized here.
> (hint: Thunderbolt hw is operating with integers, though driver uses
> UUID API at the end)
> 

Updated in v2. Now it should look better :)

> --
> With Best Regards,
> Andy Shevchenko