From: Andy Shevchenko <andy.shevchenko@gmail.com>
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
Date: Tue, 5 Feb 2019 19:21:31 +0200 [thread overview]
Message-ID: <CAHp75Vd2Rk=+CK6sR87R-2VvW8EPz7aSH8XF1_unBo6_Ww5+TQ@mail.gmail.com> (raw)
In-Reply-To: <1549054016-167128-1-git-send-email-lsun@mellanox.com>
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
next prev parent reply other threads:[~2019-02-05 17:21 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-29 20:59 [PATCH v1 1/1] platform/mellanox: Add bootctl driver for Mellanox BlueField Soc Liming Sun
2019-01-29 22:03 ` Andy Shevchenko
2019-01-30 6:24 ` Vadim Pasternak
2019-01-30 20:56 ` Liming Sun
2019-01-30 20:47 ` Liming Sun
2019-01-30 21:17 ` Andy Shevchenko
2019-01-31 16:53 ` Liming Sun
2019-01-31 16:53 ` [PATCH v2] " Liming Sun
2019-01-31 17:02 ` Andy Shevchenko
2019-01-31 17:18 ` Liming Sun
2019-01-31 19:20 ` Liming Sun
2019-01-31 19:19 ` [PATCH v3] " Liming Sun
2019-02-01 5:16 ` [PATCH v1 1/1] " kbuild test robot
2019-02-01 20:48 ` Liming Sun
2019-02-01 20:46 ` [PATCH v4] " Liming Sun
2019-02-05 17:21 ` Andy Shevchenko [this message]
2019-05-17 17:49 ` Liming Sun
2019-05-17 17:49 ` [PATCH v5 1/2] " Liming Sun
2019-05-20 15:56 ` Greg KH
2019-05-20 18:07 ` Liming Sun
2019-05-20 19:12 ` Greg KH
2019-05-20 20:43 ` Liming Sun
2019-05-21 7:58 ` Arnd Bergmann
2019-05-22 15:12 ` Liming Sun
2019-05-22 13:34 ` Mark Rutland
2019-05-22 14:51 ` Liming Sun
2019-05-17 17:49 ` [PATCH v5 2/2] platform/mellanox/mlxbf-bootctl: Add the ABI definitions Liming Sun
2019-05-17 17:59 ` Greg Kroah-Hartman
2019-05-17 20:36 ` Liming Sun
2019-05-18 6:35 ` Greg Kroah-Hartman
2019-05-20 15:20 ` Liming Sun
2019-05-20 15:53 ` Greg Kroah-Hartman
2019-10-07 15:48 ` [PATCH v6 1/2] platform/mellanox: Add bootctl driver for Mellanox BlueField Soc Liming Sun
2019-10-07 15:48 ` [PATCH v6 2/2] platform/mellanox/mlxbf-bootctl: Add the ABI definitions Liming Sun
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHp75Vd2Rk=+CK6sR87R-2VvW8EPz7aSH8XF1_unBo6_Ww5+TQ@mail.gmail.com' \
--to=andy.shevchenko@gmail.com \
--cc=andy@infradead.org \
--cc=dvhart@infradead.org \
--cc=dwoods@mellanox.com \
--cc=linux-kernel@vger.kernel.org \
--cc=lsun@mellanox.com \
--cc=platform-driver-x86@vger.kernel.org \
--cc=vadimp@mellanox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).