Re: [PATCH v9 9/9] drm/xe/xe_late_bind_fw: Extract and print version info

["Nilawar, Badal" <badal.nilawar@intel.com>]

On 25-09-2025 02:14, Kees Bakker wrote:
> Op 05-09-2025 om 17:49 schreef Badal Nilawar:
>> Extract and print version info of the late binding binary.
>>
>> v2: Some refinements (Daniele)
>>
>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_late_bind_fw.c       | 124 +++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_late_bind_fw_types.h |   3 +
>>   drivers/gpu/drm/xe/xe_uc_fw_abi.h          |  66 +++++++++++
>>   3 files changed, 193 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_late_bind_fw.c 
>> b/drivers/gpu/drm/xe/xe_late_bind_fw.c
>> index 0f062008ca83..38f3feb2aecd 100644
>> --- a/drivers/gpu/drm/xe/xe_late_bind_fw.c
>> +++ b/drivers/gpu/drm/xe/xe_late_bind_fw.c
>> @@ -45,6 +45,121 @@ late_bind_to_xe(struct xe_late_bind *late_bind)
>>       return container_of(late_bind, struct xe_device, late_bind);
>>   }
>>   +static struct xe_device *
>> +late_bind_fw_to_xe(struct xe_late_bind_fw *lb_fw)
>> +{
>> +    return container_of(lb_fw, struct xe_device, 
>> late_bind.late_bind_fw[lb_fw->id]);
>> +}
>> +
>> +/* Refer to the "Late Bind based Firmware Layout" documentation 
>> entry for details */
>> +static int parse_cpd_header(struct xe_late_bind_fw *lb_fw,
>> +                const void *data, size_t size, const char 
>> *manifest_entry)
>> +{
>> +    struct xe_device *xe = late_bind_fw_to_xe(lb_fw);
>> +    const struct gsc_cpd_header_v2 *header = data;
>> +    const struct gsc_manifest_header *manifest;
>> +    const struct gsc_cpd_entry *entry;
>> +    size_t min_size = sizeof(*header);
>> +    u32 offset;
>> +    int i;
>> +
>> +    /* manifest_entry is mandatory */
>> +    xe_assert(xe, manifest_entry);
>> +
>> +    if (size < min_size || header->header_marker != 
>> GSC_CPD_HEADER_MARKER)
>> +        return -ENOENT;
>> +
>> +    if (header->header_length < sizeof(struct gsc_cpd_header_v2)) {
>> +        drm_err(&xe->drm, "%s late binding fw: Invalid CPD header 
>> length %u!\n",
>> +            fw_id_to_name[lb_fw->id], header->header_length);
>> +        return -EINVAL;
>> +    }
>> +
>> +    min_size = header->header_length + sizeof(struct gsc_cpd_entry) 
>> * header->num_of_entries;
>> +    if (size < min_size) {
>> +        drm_err(&xe->drm, "%s late binding fw: too small! %zu < %zu\n",
>> +            fw_id_to_name[lb_fw->id], size, min_size);
>> +        return -ENODATA;
>> +    }
>> +
>> +    /* Look for the manifest first */
>> +    entry = (void *)header + header->header_length;
>> +    for (i = 0; i < header->num_of_entries; i++, entry++)
>> +        if (strcmp(entry->name, manifest_entry) == 0)
>> +            offset = entry->offset & GSC_CPD_ENTRY_OFFSET_MASK;
>> +
>> +    if (!offset) {
> This for loop looks suspicious. Do you continue the loop on purpose
> after finding the first match? Or should there be a break?
> Also, if there is no match then offset is uninitialized. Isn't it better
> to initialize offset at the start?

Thanks for highlighting. This has been addressed in this patch 
https://lore.kernel.org/intel-xe/20250924102208.9216-1-colin.i.king@gmail.com/.

Regards,
Badal