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

[Vadim Pasternak]

[...]

Please, be consistent with naming convention.
All the above should have same prefix as others routines.

> 
> > +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.
> 
[...]