Re: [PATCH] drm/imagination: Support for querying Firmware information

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/imagination: missing 64-bit padding in struct drm_pvr_dev_query_fw_info
--

commit 1360941b5bdd5b35c5e7870e3bae3cb8d37c65fd
Author: Brajesh Gupta <brajesh.gupta@imgtec.com>

drm/imagination: Support for querying Firmware information

This commit adds a new dev query subtype to query firmware version
information from the driver.

> diff --git a/drivers/gpu/drm/imagination/pvr_drv.h b/drivers/gpu/drm/imagination/pvr_drv.h
> index 7fa147312dd11..fd01feeeda1d4 100644
> --- a/drivers/gpu/drm/imagination/pvr_drv.h
> +++ b/drivers/gpu/drm/imagination/pvr_drv.h
[ ... ]
> @@ -68,6 +69,7 @@ int pvr_set_uobj_array(const struct drm_pvr_obj_array *out, u32 min_stride, u32
>  	PVR_UOBJ_DECL(struct drm_pvr_dev_query_heap_info, heaps) \
>  	PVR_UOBJ_DECL(struct drm_pvr_static_data_area, offset) \
>  	PVR_UOBJ_DECL(struct drm_pvr_dev_query_static_data_areas, static_data_areas) \
> +	PVR_UOBJ_DECL(struct drm_pvr_dev_query_fw_info, minor_version) \
>  	)
>  
>  /**
[ ... ]
> diff --git a/include/uapi/drm/pvr_drm.h b/include/uapi/drm/pvr_drm.h
> index ccf6c2112468f..94281af974c78 100644
> --- a/include/uapi/drm/pvr_drm.h
> +++ b/include/uapi/drm/pvr_drm.h
> @@ -434,6 +434,18 @@ struct drm_pvr_dev_query_static_data_areas {
>  	struct drm_pvr_obj_array static_data_areas;
>  };
>  
> +/**
> + * struct drm_pvr_dev_query_fw_info - Container used to fetch information
> + * about the GPU Firmware like its version.
> + */
> +struct drm_pvr_dev_query_fw_info {
> +	/** @major_version: Major version of the Firmware. */
> +	__u16 major_version;
> +
> +	/** @minor_version: Minor version of the Firmware. */
> +	__u16 minor_version;
> +};

[Severity: Medium]
Should struct drm_pvr_dev_query_fw_info include an explicit __u32 _padding_4
field to pad the structure to 64-bit alignment, and pass _padding_4 as the
last member in the pvr_drv.h declaration macro?

The documentation at the top of include/uapi/drm/pvr_drm.h states that all
ioctl and dev query structs must be padded to 64-bit alignment using explicit
_padding_[x] fields:

include/uapi/drm/pvr_drm.h:
 *  - All members must be type-aligned.
 *  - The overall struct must be padded to 64-bit alignment.
 *  - Explicit padding is almost always required. This takes the form of
 *    ``_padding_[x]`` members of sufficient size to pad to the next power-of-two
 *    alignment, where [x] is the offset into the struct in hexadecimal. Arrays
 *    are never used for alignment. Padding fields must be zeroed; this is
 *    always checked.
 ...
 * In addition to the IOCTL argument structs, the PowerVR UAPI makes use of
 * DEV_QUERY argument structs. These are used to fetch information about the
 * device and runtime. These structs are subject to the same rules set out
 * above.

As defined, struct drm_pvr_dev_query_fw_info is only 4 bytes, so
pvr_ioctl_dev_query() -> pvr_fw_info_get() reports a size of 4 when
args->pointer is zero and copies out a 4-byte structure.

If a 64-bit field is appended to struct drm_pvr_dev_query_fw_info in the
future, leaving it at 4 bytes now would introduce implicit compiler padding at
offset 4 on 64-bit architectures and cause a 32-bit versus 64-bit layout
mismatch.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-fwversion-v1-1-3ed17c65bf4c@imgtec.com?part=1