Re: [Patch v4 7/7] crypto/ccp: Implement SNP Download Firmware EX

[Shantanu Sinha <shansinha@google.com>]

On Thu, Oct 15, 2026 at 05:09:38PM -0400, Pratik R. Sampat wrote:
> On 10/15/26 3:38 PM, Tycho Andersen wrote:
> >> +	/*
> >> +	 * Firmware either just requested a rollback or a rollback attempt
> >> +	 * failed. Keep the PSP dead for all other commands, only another
> >> +	 * DLFW_EX attempt is allowed through.
> >> +	 */
> >> +	if (sev->fwl_rollback_required) {
> >> +		psp_dead = true;
> >> +		if (fwl_ret != FW_UPLOAD_ERR_HW_ERROR)
> >> +			fwl_ret = FW_UPLOAD_ERR_RW_ERROR;
> >
> > I think this error switcharoo could use a comment (or maybe just be
> > dropped?). e.g. if the user does something to set
> > ->fwl_rollback_required and then tries another firmware upload that is
> > the wrong size, ERR_INVALID_SIZE -> ERR_RW_ERROR, which seems
> > misleading to me.
> >
> > I saw some discussion about all this:
> > https://lore.kernel.org/lkml/20260924213737.3833625-1-shansinha@google.com/
> > but it's not clear to me why you need to reset the error code here.
> >
>
> My understanding from that discussion was that userspace errors should
> fall into clean buckets. read-write-error would mean "roll back",
> whether it came from the first failure or a later rollback attempt,
> since a failed rollback still leaves the PSP dead and needing a
> rollback. dmesg would still log the exact reason for the failure.
>
> That said, I see how it can be misleading, and I'm fine with dropping
> the override or putting a comment and re-spinning this.

I do see how it can be misleading.

I think dropping it makes sense. When I suggested the changes for v2,
psp_ret was only logged via dev_dbg(), so the sysfs error was the only
clean signal userspace had. Since the error code is reported in dmesg
now, I think it's fine to keep the original error and have the sysfs
error reflect what happened on the upload rather than the action
tooling should take.

Thanks,
Shantanu