RE: [PATCH v1 1/1] platform/mellanox: Add bootctl driver for Mellanox BlueField Soc
From: Liming Sun
Date: Wed Jan 30 2019 - 15:56:44 EST
Thanks Vadim. Please see my response and questions below.
Regards,
Liming
> -----Original Message-----
> From: Vadim Pasternak
> Sent: Wednesday, January 30, 2019 1:24 AM
> To: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>; Liming Sun <lsun@xxxxxxxxxxxx>
> Cc: Andy Shevchenko <andy@xxxxxxxxxxxxx>; Darren Hart <dvhart@xxxxxxxxxxxxx>; David Woods <dwoods@xxxxxxxxxxxx>; Platform
> Driver <platform-driver-x86@xxxxxxxxxxxxxxx>; Linux Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx>
> Subject: RE: [PATCH v1 1/1] platform/mellanox: Add bootctl driver for Mellanox BlueField Soc
>
>
> [...]
>
> Please, be consistent with naming convention.
> All the above should have same prefix as others routines.
I fixed reset_action_to_val() and reset_action_to_string() and will update
them in v2.
Other static functions like xxx_show() and xxx_store() are actually defined
by kernel macro DRIVER_ATTR_RW(name) and DRIVER_ATTR_RO(name),
where the 'name' are attribute names which will be displayed in sysfs.
In order to use "mlx_bf_" prefix for the functions, the same prefix will need
to be added to the attribute name as well, which seems unusual in sysfs.
So probably I could leave those xxx_show() and xxx_store() attribute functions
as it is?
>
> >
> > > +static ssize_t post_reset_wdog_store(struct device_driver *drv,
> > > + const char *buf, size_t count) {
> > > + int err;
> > > + unsigned long watchdog;
> > > +
> > > + err = kstrtoul(buf, 10, &watchdog);
> > > + if (err)
> > > + return err;
> > > +
> >
> > > + 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.
> >
> > > +
> > > + return count;
> > > +}
> > > +
> > > +static ssize_t reset_action_show(struct device_driver *drv, char
> > > +*buf) {
> >
> > > + return sprintf(buf, "%s\n", reset_action_to_string(
> > > +
> > > + mlxbf_bootctl_smc_call0(MLXBF_BOOTCTL_GET_RESET_ACTION)));
> >
> > Wouldn't be easy to parse this as
> >
> > int action = ...call0();
> > return sprintf(...);
> >
> > ?
> >
> > (int is an arbitrary type here, choose one that suits)
> >
> > > +}
> > > +
> > > +static ssize_t reset_action_store(struct device_driver *drv,
> > > + const char *buf, size_t count) {
> > > + int action = reset_action_to_val(buf, count);
> > > +
> >
> > > + if (action < 0 || action == MLXBF_BOOTCTL_NONE)
> > > + return -EINVAL;
> >
> > Don't shadow an error.
> >
> > > +
> > > + if (mlxbf_bootctl_smc_call1(MLXBF_BOOTCTL_SET_RESET_ACTION,
> > action) < 0)
> > > + return -EINVAL;
> >
> > Same.
> >
> > > +
> > > + return count;
> > > +}
> > > +
> > > +static ssize_t second_reset_action_show(struct device_driver *drv,
> > > +char *buf) {
> >
> > > + return sprintf(buf, "%s\n", reset_action_to_string(
> > > + mlxbf_bootctl_smc_call0(
> > > + MLXBF_BOOTCTL_GET_SECOND_RESET_ACTION)));
> >
> > Use temp variable.
> >
> > > +}
> > > +
> > > +static ssize_t second_reset_action_store(struct device_driver *drv,
> > > + const char *buf, size_t
> > > +count) {
> > > + int action = reset_action_to_val(buf, count);
> > > +
> > > + if (action < 0)
> > > + return -EINVAL;
> >
> > Don't shadow an error.
> >
> > > +
> > > + if
> > (mlxbf_bootctl_smc_call1(MLXBF_BOOTCTL_SET_SECOND_RESET_ACTION,
> > > + action) < 0)
> > > + return -EINVAL;
> >
> > Same.
> >
> > > +
> > > + return count;
> > > +}
> > > +
> > > +static ssize_t lifecycle_state_show(struct device_driver *drv, char
> > > +*buf) {
> >
> > > + int lc_state = mlxbf_bootctl_smc_call1(
> > > + MLXBF_BOOTCTL_GET_TBB_FUSE_STATUS,
> > > + MLXBF_BOOTCTL_FUSE_STATUS_LIFECYCLE);
> >
> > Split it as
> >
> > int ...;
> >
> > ... = call1();
> > if (...)
> >
> > > +
> > > + if (lc_state < 0)
> > > + return -EINVAL;
> >
> > Don't shadow an error.
> >
> > > +
> > > + lc_state &= (MLXBF_BOOTCTL_SB_MODE_TEST_MASK |
> > > + MLXBF_BOOTCTL_SB_MODE_SECURE_MASK);
> >
> > Better to split like
> >
> > xxx =
> > (A | B);
> >
> > > + /*
> > > + * If the test bits are set, we specify that the current state may be
> > > + * due to using the test bits.
> > > + */
> >
> > > + if ((lc_state & MLXBF_BOOTCTL_SB_MODE_TEST_MASK) != 0) {
> >
> > ' != 0' is redundant.
> >
> > > +
> > > + lc_state &= MLXBF_BOOTCTL_SB_MODE_SECURE_MASK;
> > > +
> >
> > > + return sprintf(buf, "%s(test)\n",
> > > +
> > > + mlxbf_bootctl_lifecycle_states[lc_state]);
> >
> > One line?
> >
> > > + }
> > > +
> > > + return sprintf(buf, "%s\n",
> > > +mlxbf_bootctl_lifecycle_states[lc_state]);
> > > +}
> > > +
> > > +static ssize_t secure_boot_fuse_state_show(struct device_driver *drv,
> > > +char *buf) {
> > > + int key;
> > > + int buf_len = 0;
> > > + int upper_key_used = 0;
> > > + int sb_key_state = mlxbf_bootctl_smc_call1(
> > > + MLXBF_BOOTCTL_GET_TBB_FUSE_STATUS,
> > > + MLXBF_BOOTCTL_FUSE_STATUS_KEYS);
> > > +
> > > + if (sb_key_state < 0)
> > > + return -EINVAL;
> > > +
> >
> > > + for (key = MLXBF_SB_KEY_NUM - 1; key >= 0; key--) {
> >
> > I'm not sure it's a good idea to put several lines in one sysfs attribute.
> >
> > > + int burnt = ((sb_key_state & (1 << key)) != 0);
> >
> > Redundant ' != 0', redundant parens.
> >
> > > + int valid = ((sb_key_state &
> > > + (1 << (key + MLXBF_SB_KEY_NUM))) != 0);
> >
> > Same.
> >
> > > +
> > > + buf_len += sprintf(buf + buf_len, "Ver%d:", key);
> > > + if (upper_key_used) {
> > > + if (burnt) {
> > > + if (valid)
> > > + buf_len += sprintf(buf + buf_len,
> > > + "Used");
> >
> > Oh, why not just
> >
> > const char *status;
> >
> > if (...) {
> > ...
> > status = "1";
> > ...
> > status = "2";
> > ...
> > }
> > len = snprintf(buf + len, ..., status);
> >
> > ?
> >
> > > + else
> > > + buf_len += sprintf(buf + buf_len,
> > > + "Wasted");
> > > + } else {
> > > + if (valid)
> > > + buf_len += sprintf(buf + buf_len,
> > > + "Invalid");
> > > + else
> > > + buf_len += sprintf(buf + buf_len,
> > > + "Skipped");
> > > + }
> > > + } else {
> > > + if (burnt) {
> > > + if (valid) {
> > > + upper_key_used = 1;
> > > + buf_len += sprintf(buf + buf_len,
> > > + "In use");
> > > + } else
> > > + buf_len += sprintf(buf + buf_len,
> > > + "Burn incomplete");
> > > + } else {
> > > + if (valid)
> > > + buf_len += sprintf(buf + buf_len,
> > > + "Invalid");
> > > + else
> > > + buf_len += sprintf(buf + buf_len,
> > > + "Free");
> > > + }
> > > + }
> > > + buf_len += sprintf(buf + buf_len, "\n");
> > > + }
> > > +
> > > + return buf_len;
> > > +}
> > > +
> >
> > > +#define MLXBF_BOOTCTL_DRV_ATTR(_name) DRIVER_ATTR_RW(_name)
> >
> > What the point?
> >
> > > +static struct attribute_group mlxbf_bootctl_attr_group = {
> > > + .attrs = mlxbf_bootctl_dev_attrs
> >
> > + comma.
> >
> [...]