Re: [PATCH v1] mlxbf-bootctl: check the secure boot development mode status bit

[Ilpo Järvinen]

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.