lkml.org 
[lkml]   [2019]   [May]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v4] platform/mellanox: Add bootctl driver for Mellanox BlueField Soc
Date
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@vger.kernel.org <linux-kernel-owner@vger.kernel.org> On Behalf Of Andy Shevchenko
> Sent: Tuesday, February 5, 2019 12:22 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 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)

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
\
 
 \ /
  Last update: 2019-05-17 19:51    [W:0.063 / U:26.788 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site