Re: [PATCH v2 11/34] media: iris: introduce packetization layer for creating HFI packets

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 12/18/23 12:32, Dikshita Agarwal wrote:
> Host firmware interface (HFI) is well defined set of interfaces
> for communication between host driver and firmware.
> The command and responses are exchanged in form of packets.
> One or multiple packets are grouped under packet header.
> Each packet has packet type which describes the specific HFI
> and payload which holds the corresponding value for that HFI.
> 
> Sys_init is the first packets sent to firmware, which initializes
> the firmware. Sys_image_version packet is to get the firmware
> version string.
> 
> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> ---
[...]

>   struct iris_core {
> @@ -65,6 +70,11 @@ struct iris_core {
>   	struct mem_desc				sfr;
>   	struct mutex				lock; /* lock for core structure */
>   	unsigned int				use_tz;
> +	u8					*packet;
> +	u32					packet_size;
I'm not sure it's necessary to always keep a reference to the last
packet in the core struct, especially since it needs to be allocated
first anyway

> +	u32					sys_init_id;
This looks like a hyper-defensive measure against some firmware
overtaking attacks.. Or a way to spot random/unwanted resets of
the firmware core..

Is it actually necessary, or does this just serve as a debug
feature?

> +	u32					header_id;
Similar to above..

> +	u32					packet_id;
And here.

I performed some quick CTRL-F-agge around the series and this is
never reset.. Can the firmware cope with this? What if I watch a
veeeery long youtube video that ends up creating more than
(1<<32)-1 HFI packets while playing?

> +
> +enum hfi_packet_host_flags {
> +	HFI_HOST_FLAGS_NONE			= 0x00000000,
> +	HFI_HOST_FLAGS_INTR_REQUIRED		= 0x00000001,
> +	HFI_HOST_FLAGS_RESPONSE_REQUIRED	= 0x00000002,
> +	HFI_HOST_FLAGS_NON_DISCARDABLE		= 0x00000004,
> +	HFI_HOST_FLAGS_GET_PROPERTY		= 0x00000008,
BIT(n)?

> +};
> +
> +enum hfi_packet_firmware_flags {
> +	HFI_FW_FLAGS_NONE		= 0x00000000,
> +	HFI_FW_FLAGS_SUCCESS		= 0x00000001,
> +	HFI_FW_FLAGS_INFORMATION	= 0x00000002,
> +	HFI_FW_FLAGS_SESSION_ERROR	= 0x00000004,
> +	HFI_FW_FLAGS_SYSTEM_ERROR	= 0x00000008,
BIT(n)?

Konrad