Re: [RFC v1 6/6] crypto/ccp: Implement SNP firmware live update

[Maxwell Doose]

On Thu Apr 30, 2026 at 11:07 AM CDT, Tycho Andersen wrote:
> From: "Tycho Andersen (AMD)" <tycho@xxxxxxxxxx>
>
> Put all the previous primitives together to implement SNP firmware
> live update via DOWNLOAD_FIRMWARE_EX.
>

[snip]

>
> [1]: https://lore.kernel.org/lkml/20241112232253.3379178-7-dionnaglaze@xxxxxxxxxx/
> Signed-off-by: Tycho Andersen (AMD) <tycho@xxxxxxxxxx>
> ---
>  drivers/crypto/ccp/sev-dev.c | 244 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 243 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index b4711bf823e8..e7fe6dbf69c2 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> +static int sev_download_firmware_ex(struct sev_device *sev, const u8 *data,
> +				    u32 size)
> +{
> +	struct sev_data_download_firmware_ex sev_data = {0};
> +	int ret, error = 0, order;
>

Why not assign across multiple lines? How about something like:

int ret, order;
int error = 0;

or:

int ret;
int order;
int error = 0;

Would be better for readability and git blame.

>  static enum fw_upload_err sev_fw_upload_write(struct fw_upload *fw_upload,
>  					      const u8 *data, u32 offset,
>  					      u32 size, u32 *written)
> {
>

[snip]

>
> +	old_major = sev->api_major;
> +	old_minor = sev->api_minor;
> +	old_build = sev->build;
> +
> +	mutex_lock(&sev_cmd_mutex);
>

Why not guard(mutex)()? You used it earlier in
sev_firmware_reinit_if_shutdown().

>
> +	*written = size;
> +	ret = FW_UPLOAD_ERR_NONE;
> +
> +unlock:
> +	mutex_unlock(&sev_cmd_mutex);
>

See above comment.

best regards,
maxwell