Thanks Andy!
v5 has been posted to address all the comments. You could also see the detailed response below.
Regards,
Liming
> -----Original Message-----
> From: linux-kernel-owner@xxxxxxxxxxxxxxx <linux-kernel-owner@xxxxxxxxxxxxxxx> On Behalf Of Andy Shevchenko
> Sent: Tuesday, February 5, 2019 12:22 PM
> To: Liming Sun <lsun@xxxxxxxxxxxx>
> Cc: Andy Shevchenko <andy@xxxxxxxxxxxxx>; Darren Hart <dvhart@xxxxxxxxxxxxx>; Vadim Pasternak <vadimp@xxxxxxxxxxxx>; David
> Woods <dwoods@xxxxxxxxxxxx>; Platform Driver <platform-driver-x86@xxxxxxxxxxxxxxx>; Linux Kernel Mailing List <linux-
> kernel@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v4] platform/mellanox: Add bootctl driver for Mellanox BlueField Soc
>
> On Fri, Feb 1, 2019 at 10:47 PM Liming Sun <lsun@xxxxxxxxxxxx> wrote:
>
> Thanks for an update, my comments below.
>
> (To Mellanox kernel developer team: guys, perhaps you need to
> establish a few rounds of internal review before sending the stuff
> outside)
Yes, we did internal review on v5 and updated the 'Reviewed-by'.
>
> First of all, I didn't find ABI documentation for interface this patch
> introduces.
> Must be added.
Added in v5.
>
> > +/* UUID used to probe ATF service. */
> > +static const char * const mlxbf_bootctl_svc_uuid_str =
> > + "89c036b4-e7d7-11e6-8797-001aca00bfc4";
>
> static const char *..._str = ...;
Updated in v5.
>
> > +/* Syntactic sugar to avoid having to specify an unused argument. */
> > +#define mlxbf_bootctl_smc_call0(smc_op) mlxbf_bootctl_smc_call1(smc_op, 0)
>
> No.
> Please, do it explicitly.
Updated in v5. Renamed mlxbf_bootctl_smc_call1() to mlxbf_bootctl_smc().
>
> > +static const char *mlxbf_bootctl_reset_action_to_string(int action)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(boot_names); i++)
> > + if (boot_names[i].value == action)
> > + return boot_names[i].name;
> > +
>
> > + return "";
>
> Hmm...
> Shouldn't be more descriptive?
Updated in v5 to return "invalid action" in such case.
>
> > +}
>
> > +static ssize_t post_reset_wdog_show(struct device_driver *drv, char *buf)
> > +{
> > + return sprintf(buf, "%d\n",
> > + mlxbf_bootctl_smc_call0(MLXBF_BOOTCTL_GET_POST_RESET_WDOG));
>
> What if call return negative error?
Fixed in v5 to check this return value.
>
> > +}
>
> > +static ssize_t reset_action_show(struct device_driver *drv, char *buf)
> > +{
> > + int action;
> > +
> > + action = mlxbf_bootctl_smc_call0(MLXBF_BOOTCTL_GET_RESET_ACTION);
>
> What if action goes negative?
Fixed in v5 to check this return value.
>
> > + return sprintf(buf, "%s\n",
> > + mlxbf_bootctl_reset_action_to_string(action));
>
> Wouldn't be one line?
Updated in v5 (one line).
>
> > +}
>
> > +static ssize_t reset_action_store(struct device_driver *drv,
> > + const char *buf, size_t count)
> > +{
>
> > + int ret, action = mlxbf_bootctl_reset_action_to_val(buf);
>
> This should be like
> int action;
> int ret;
>
> action = ...;
> if (action ...)
Updated in v5.
>
>
> > + if (action == MLXBF_BOOTCTL_INVALID || action == MLXBF_BOOTCTL_NONE)
> > + return -EINVAL;
>
> The mlxbf_bootctl_reset_action_to_val() has to return a proper Linux error code.
> After this code should be modified to something like
>
> if (action < 0)
> return action;
Updated in v5.
>
> > +
> > + ret = (mlxbf_bootctl_smc_call1(MLXBF_BOOTCTL_SET_RESET_ACTION, action));
>
> Redundant parens.
Fixed in v5.
>
> > + if (ret < 0)
> > + return ret;
> > +
> > + return count;
> > +}
> > +
> > +static ssize_t second_reset_action_show(struct device_driver *drv, char *buf)
> > +{
> > + int action;
> > + const char *name;
> > +
> > + action = mlxbf_bootctl_smc_call0(MLXBF_BOOTCTL_GET_SECOND_RESET_ACTION);
>
> What if action is negative?
Updated in v5.
>
> > + name = mlxbf_bootctl_reset_action_to_string(action);
> > +
> > + return sprintf(buf, "%s\n", name);
>
> return sprintf(... _to_string(...));
> ?
Updated in v5 as suggested.
>
> > +}
> > +
> > +static ssize_t second_reset_action_store(struct device_driver *drv,
> > + const char *buf, size_t count)
> > +{
>
> > + int ret, action = mlxbf_bootctl_reset_action_to_val(buf);
>
> int action;
> int ret;
>
> action = ...
> if (action ...)
Updated in v5.
>
> > +
> > + if (action < 0)
> > + return action;
> > +
> > + ret = mlxbf_bootctl_smc_call1(MLXBF_BOOTCTL_SET_SECOND_RESET_ACTION,
> > + action);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return count;
> > +}
> > +
> > +static ssize_t lifecycle_state_show(struct device_driver *drv, char *buf)
> > +{
> > + int lc_state;
> > +
> > + lc_state = mlxbf_bootctl_smc_call1(
> > + MLXBF_BOOTCTL_GET_TBB_FUSE_STATUS,
> > + MLXBF_BOOTCTL_FUSE_STATUS_LIFECYCLE);
>
> > +
>
> Redundant blank line.
Updated in v5.
>
> > + if (lc_state < 0)
> > + return lc_state;
> > +
> > + lc_state &=
> > + (MLXBF_BOOTCTL_SB_TEST_MASK | MLXBF_BOOTCTL_SB_SECURE_MASK);
>
> Actually parens are not needed. Sorry, I forgot to point to this earlier.
Updated in v5.
>
> > +
> > + /*
> > + * 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_TEST_MASK) {
> > +
> > + lc_state &= MLXBF_BOOTCTL_SB_SECURE_MASK;
> > +
> > + return sprintf(buf, "%s(test)\n",
> > + mlxbf_bootctl_lifecycle_states[lc_state]);
> > + }
> > +
> > + 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 sb_key_state = mlxbf_bootctl_smc_call1(
> > + MLXBF_BOOTCTL_GET_TBB_FUSE_STATUS,
> > + MLXBF_BOOTCTL_FUSE_STATUS_KEYS);
>
> Split it to declaration and assignment.
Updated in v5.
>
> > + int upper_key_used = 0;
> > + int buf_len = 0;
> > + const char *status;
> > + int key;
> > +
> > + if (sb_key_state < 0)
> > + return sb_key_state;
> > +
> > + for (key = MLXBF_SB_KEY_NUM - 1; key >= 0; key--) {
>
> > + int burnt = sb_key_state & (1 << key);
>
> BIT() ?
Updated in v5.
>
> > + int valid = sb_key_state &
> > + (1 << (key + MLXBF_SB_KEY_NUM));
>
> Ditto. And put to one line?
Updated in v5.
>
> > + buf_len += sprintf(buf + buf_len, "%d:", key);
>
> Why this can't be part of the below sprintf() call?
Updated in v5.
>
> > + if (upper_key_used) {
> > + if (burnt)
> > + status = valid ? "Used" : "Wasted";
> > + else
> > + status = valid ? "Invalid" : "Skipped";
> > + } else {
> > + if (burnt) {
> > + status = valid ? "In use" : "Burn incomplete";
>
> > + if (valid)
> > + upper_key_used = 1;
>
> Move this out of this if-else-if block. The rationale is to split two
> logical parts:
> 1) message choice
> 2) flag flip
Updated the logic in v5 as suggested.
>
> > + } else
> > + status = valid ? "Invalid" : "Free";
> > + }
> > + buf_len += sprintf(buf + buf_len, status);
>
> > + if (key != 0)
> > + buf_len += sprintf(buf + buf_len, "; ");
>
> Why do you need to check this?
Updated in v5 to remove the check and merge it with the line above.
>
> > + }
> > + buf_len += sprintf(buf + buf_len, "\n");
> > +
> > + return buf_len;
> > +}
>
> > +static struct attribute_group mlxbf_bootctl_attr_group = {
> > + .attrs = mlxbf_bootctl_dev_attrs
>
> + comma.
Updated in v5.
>
> > +};
>
> > +static bool mlxbf_bootctl_guid_match(const guid_t *guid,
> > + const struct arm_smccc_res *res)
> > +{
>
> > + guid_t id = GUID_INIT(res->a0, res->a1 & 0xFFFF,
> > + (res->a1 >> 16) & 0xFFFF,
> > + res->a2 & 0xff, (res->a2 >> 8) & 0xFF,
> > + (res->a2 >> 16) & 0xFF, (res->a2 >> 24) & 0xFF,
> > + res->a3 & 0xff, (res->a3 >> 8) & 0xFF,
> > + (res->a3 >> 16) & 0xFF, (res->a3 >> 24) & 0xFF);
>
> All & 0xFF* are done inside the macro, no need to duplicate.
Updated in v5.
>
> > +
> > + return guid_equal(guid, &id);
> > +}
>
> > +static int mlxbf_bootctl_probe(struct platform_device *pdev)
> > +{
> > + struct arm_smccc_res res;
> > + guid_t guid;
> > +
> > + /* Ensure we have the UUID we expect for this service. */
> > + arm_smccc_smc(MLXBF_BOOTCTL_SIP_SVC_UID, 0, 0, 0, 0, 0, 0, 0, &res);
> > + guid_parse(mlxbf_bootctl_svc_uuid_str, &guid);
> > + if (!mlxbf_bootctl_guid_match(&guid, &res))
> > + return -ENODEV;
> > +
> > + /*
> > + * When watchdog is used, it sets boot mode to MLXBF_BOOTCTL_SWAP_EMMC
> > + * in case of boot failures. However it doesn't clear the state if there
> > + * is no failure. Restore the default boot mode here to avoid any
> > + * unnecessary boot partition swapping.
> > + */
>
> > + if (mlxbf_bootctl_smc_call1(MLXBF_BOOTCTL_SET_RESET_ACTION,
> > + MLXBF_BOOTCTL_EMMC) < 0)
>
> Use temporary variable here.
Updated in v5.
>
> int ret;> +
>
> ...
> ret = ..._call1(...);
> if (ret < 0)
>
> > + dev_err(&pdev->dev, "Unable to reset the EMMC boot mode\n");
>
> If it's error, why we return 0?
> Otherwise, use warn level here.
Updated to use dev_warn() to avoid module probe failure since other
functionalities can still be used.
>
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver mlxbf_bootctl_driver = {
> > + .probe = mlxbf_bootctl_probe,
> > + .driver = {
> > + .name = "mlxbf-bootctl",
> > + .groups = mlxbf_bootctl_attr_groups,
>
> > + .acpi_match_table = ACPI_PTR(mlxbf_bootctl_acpi_ids),
>
> ACPI_PTR is redundant for the ACPI dependent driver.
Updated in v5.
>
> > + }
> > +};
>
> --
> With Best Regards,
> Andy Shevchenko
