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

[Nilawar, Badal]

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@xxxxxxxxx>
> > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > ---
> >   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@xxxxxxxxx/.

Regards,
Badal