On Mon, 20 Nov 2023, David Thompson wrote:
> If the secure boot is enabled with the development key, then print
> it to the output buffer when lifecycle_state_show() is invoked.
>
> Fixes: 79e29cb8fbc5c ("platform/mellanox: Add bootctl driver for Mellanox BlueField Soc")
The commit message says nothing that warrants a Fixes tag.
Also, the commit message doesn't tell why you need to do this, that is, it
doesn't tell what's the current situation and how it's wrong/unwanted.
Please amend.
> Reviewed-by: Khalil Blaiech <kblaiech@xxxxxxxxxx>
> Signed-off-by: David Thompson <davthompson@xxxxxxxxxx>
> ---
> drivers/platform/mellanox/mlxbf-bootctl.c | 24 +++++++++++++++++------
> 1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/mellanox/mlxbf-bootctl.c b/drivers/platform/mellanox/mlxbf-bootctl.c
> index 1ac7dab22c63..ed22908d79b9 100644
> --- a/drivers/platform/mellanox/mlxbf-bootctl.c
> +++ b/drivers/platform/mellanox/mlxbf-bootctl.c
> @@ -20,6 +20,7 @@
>
> #define MLXBF_BOOTCTL_SB_SECURE_MASK 0x03
> #define MLXBF_BOOTCTL_SB_TEST_MASK 0x0c
> +#define MLXBF_BOOTCTL_SB_DEV_MASK 0x10
BIT(4)
(Those other too could be converted to GENMASK() but not in this patch.)
> #define MLXBF_SB_KEY_NUM 4
>
> @@ -40,11 +41,18 @@ static struct mlxbf_bootctl_name boot_names[] = {
> { MLXBF_BOOTCTL_NONE, "none" },
> };
>
> +enum {
> + MLXBF_BOOTCTL_SB_LIFECYCLE_PRODUCTION = 0,
> + MLXBF_BOOTCTL_SB_LIFECYCLE_GA_SECURE = 1,
> + MLXBF_BOOTCTL_SB_LIFECYCLE_GA_NON_SECURE = 2,
> + MLXBF_BOOTCTL_SB_LIFECYCLE_RMA = 3
> +};
> +
> static const char * const mlxbf_bootctl_lifecycle_states[] = {
> - [0] = "Production",
> - [1] = "GA Secured",
> - [2] = "GA Non-Secured",
> - [3] = "RMA",
> + [MLXBF_BOOTCTL_SB_LIFECYCLE_PRODUCTION] = "Production",
> + [MLXBF_BOOTCTL_SB_LIFECYCLE_GA_SECURE] = "GA Secured",
> + [MLXBF_BOOTCTL_SB_LIFECYCLE_GA_NON_SECURE] = "GA Non-Secured",
> + [MLXBF_BOOTCTL_SB_LIFECYCLE_RMA] = "RMA",
> };
>
> /* Log header format. */
> @@ -254,8 +262,9 @@ static ssize_t lifecycle_state_show(struct device *dev,
> if (lc_state < 0)
> return lc_state;
>
> - lc_state &=
> - MLXBF_BOOTCTL_SB_TEST_MASK | MLXBF_BOOTCTL_SB_SECURE_MASK;
> + lc_state &= (MLXBF_BOOTCTL_SB_TEST_MASK |
> + MLXBF_BOOTCTL_SB_SECURE_MASK |
> + MLXBF_BOOTCTL_SB_DEV_MASK);
>
> /*
> * If the test bits are set, we specify that the current state may be
> @@ -266,6 +275,9 @@ static ssize_t lifecycle_state_show(struct device *dev,
>
> return sprintf(buf, "%s(test)\n",
> mlxbf_bootctl_lifecycle_states[lc_state]);
> + } else if ((lc_state & MLXBF_BOOTCTL_SB_SECURE_MASK) == MLXBF_BOOTCTL_SB_LIFECYCLE_GA_SECURE
> + && (lc_state & MLXBF_BOOTCTL_SB_DEV_MASK)) {
I cannot review this line until you amend the commit message with the
above mentioned details. To be more precise, I'm interested in
understanding if you've precedences right here so your commit message
should have enough details to support me in that decision, thank you.
> + return sprintf(buf, "Secured (development)\n");
> }
>
> return sprintf(buf, "%s\n", mlxbf_bootctl_lifecycle_states[lc_state]);
>
--
i.
