Re: [PATCH v5 3/5] drm/xe/xe_late_bind_fw: Add support to load Ocode firmware

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] drm/xe: unreachable INTEL_LB_STATUS_IP_ERROR_START and missing status in xe_late_bind_parse_status()
--

commit 24b78786ca211ddb93b253dbb3bc2c006a8f78c6
Author: Badal Nilawar <badal.nilawar@intel.com>

drm/xe/xe_late_bind_fw: Add support to load Ocode firmware

This commit enables loading the Ocode firmware through the late binding
flow by querying the SDK API version from the System Controller mailbox
and verifying it against the firmware manifest. It also removes the 4K
payload size limit and expands the status string table in
xe_late_bind_parse_status().

> diff --git a/drivers/gpu/drm/xe/xe_late_bind_fw.c b/drivers/gpu/drm/xe/xe_late_bind_fw.c
> index 881a46abc4f51..39c47b743edc9 100644
> --- a/drivers/gpu/drm/xe/xe_late_bind_fw.c
> +++ b/drivers/gpu/drm/xe/xe_late_bind_fw.c
[ ... ]
> @@ -179,6 +188,34 @@ static const char *xe_late_bind_parse_status(uint32_t status)
>  		return "Invalid Payload";
>  	case INTEL_LB_STATUS_TIMEOUT:
>  		return "Timeout";
> +	case INTEL_LB_STATUS_INTERNAL_ERROR:
> +		return "Internal Error";
> +	case INTEL_LB_STATUS_INVALID_FPT_TABLE:
> +		return "Invalid FPT Table";
> +	case INTEL_LB_STATUS_SIGNED_PAYLOAD_VERIFICATION_ERROR:
> +		return "Signed Payload Verification Error";
> +	case INTEL_LB_STATUS_SIGNED_PAYLOAD_INVALID_CPD:
> +		return "Signed Payload Invalid CPD";
> +	case INTEL_LB_STATUS_SIGNED_PAYLOAD_FW_VERSION_MISMATCH:
> +		return "Signed Payload FW Version Mismatch";
> +	case INTEL_LB_STATUS_SIGNED_PAYLOAD_INVALID_MANIFEST:
> +		return "Signed Payload Invalid Manifest";
> +	case INTEL_LB_STATUS_SIGNED_PAYLOAD_INVALID_HASH:
> +		return "Signed Payload Invalid Hash";
> +	case INTEL_LB_STATUS_SIGNED_PAYLOAD_BINDING_TYPE_MISMATCH:
> +		return "Signed Payload Binding type Mismatch";
> +	case INTEL_LB_STATUS_SIGNED_PAYLOAD_HANDLE_SVN_FAILED:
> +		return "Signed Payload Handle SVN Failed";
> +	case INTEL_LB_STATUS_DESTINATION_MBOX_FAILURE:
> +		return "Destination MBOX Failure";
> +	case INTEL_LB_STATUS_MISSING_LOADING_PATCH:
> +		return "Missing Loading Patch";
> +	case INTEL_LB_STATUS_INVALID_COMMAND:
> +		return "Invalid Command";
> +	case INTEL_LB_STATUS_INVALID_HECI_HEADER:
> +		return "Invalid HECI Header";
> +	case INTEL_LB_STATUS_IP_ERROR_START:
> +		return "IP Error Start";

[Severity: Low]
Can the INTEL_LB_STATUS_IP_ERROR_START case in xe_late_bind_parse_status()
ever be reached?

In include/drm/intel/intel_lb_mei_interface.h, enum intel_lb_status defines
INTEL_LB_STATUS_IP_ERROR_START as BIT(31):

	INTEL_LB_STATUS_TIMEOUT                              = 7,
	INTEL_LB_STATUS_BUFFER_TOO_SMALL                     = 8,
	INTEL_LB_STATUS_INTERNAL_ERROR                       = 9,
	...
	INTEL_LB_STATUS_IP_ERROR_START                       = BIT(31),

However, in drivers/gpu/drm/xe/xe_late_bind_fw.c, the sole caller
xe_late_bind_work() stores the return value of push_payload() in a signed
int and only invokes xe_late_bind_parse_status() when ret > 0:

	if (ret > 0)
		drm_err(&xe->drm, "Load %s firmware failed with err %d, %s\n",
			fw_id_to_name[lbfw->id], ret, xe_late_bind_parse_status(ret));
	else
		drm_err(&xe->drm, "Load %s firmware failed with err %d",
			fw_id_to_name[lbfw->id], ret);

Because BIT(31) is negative when stored in a signed 32-bit int, a failure
returning INTEL_LB_STATUS_IP_ERROR_START will take the else branch instead
of calling xe_late_bind_parse_status().

Also, should INTEL_LB_STATUS_BUFFER_TOO_SMALL (8), which sits between
INTEL_LB_STATUS_TIMEOUT (7) and INTEL_LB_STATUS_INTERNAL_ERROR (9) in
enum intel_lb_status, be included in this switch statement so it does not
fall through to "Unknown error"?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028181951.1093283-7-badal.nilawar@intel.com?part=3