lkml.org 
[lkml]   [2019]   [Feb]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v4] platform/mellanox: Add bootctl driver for Mellanox BlueField Soc
On Fri, Feb 1, 2019 at 10:47 PM Liming Sun <lsun@mellanox.com> 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)

First of all, I didn't find ABI documentation for interface this patch
introduces.
Must be added.

> +/* UUID used to probe ATF service. */
> +static const char * const mlxbf_bootctl_svc_uuid_str =
> + "89c036b4-e7d7-11e6-8797-001aca00bfc4";

static const char *..._str = ...;

> +/* 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.

> +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?

> +}

> +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?

> +}

> +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?

> + return sprintf(buf, "%s\n",
> + mlxbf_bootctl_reset_action_to_string(action));

Wouldn't be 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 ...)


> + 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;

> +
> + ret = (mlxbf_bootctl_smc_call1(MLXBF_BOOTCTL_SET_RESET_ACTION, action));

Redundant parens.

> + 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?

> + name = mlxbf_bootctl_reset_action_to_string(action);
> +
> + return sprintf(buf, "%s\n", name);

return sprintf(... _to_string(...));
?

> +}
> +
> +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 ...)

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

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

> +
> + /*
> + * 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.

> + 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() ?

> + int valid = sb_key_state &
> + (1 << (key + MLXBF_SB_KEY_NUM));

Ditto. And put to one line?

> + buf_len += sprintf(buf + buf_len, "%d:", key);

Why this can't be part of the below sprintf() call?

> + 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

> + } 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?

> + }
> + buf_len += sprintf(buf + buf_len, "\n");
> +
> + return buf_len;
> +}

> +static struct attribute_group mlxbf_bootctl_attr_group = {
> + .attrs = mlxbf_bootctl_dev_attrs

+ comma.

> +};

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

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

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.

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

> + }
> +};

--
With Best Regards,
Andy Shevchenko

\
 
 \ /
  Last update: 2019-02-05 18:22    [W:0.325 / U:1.544 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site