Re: [PATCH v3 20/29] media: iris: subscribe parameters and properties to firmware for hfi_gen2

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 27/08/2024 11:05, Dikshita Agarwal via B4 Relay wrote:
> From: Vedang Nagar <quic_vnagar@quicinc.com>
> 
> For hfi_gen2, subscribe for different bitstream parameters to
> firmware to get notified for change in any of the subscribed
> parameters.
> 
> Signed-off-by: Vedang Nagar <quic_vnagar@quicinc.com>
> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> ---
>   drivers/media/platform/qcom/iris/iris_hfi_gen2.h   |   6 +
>   .../platform/qcom/iris/iris_hfi_gen2_command.c     | 179 +++++++++++++++++++++
>   .../platform/qcom/iris/iris_hfi_gen2_defines.h     |   9 ++
>   .../platform/qcom/iris/iris_platform_common.h      |   4 +
>   .../platform/qcom/iris/iris_platform_sm8550.c      |  13 ++
>   5 files changed, 211 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2.h b/drivers/media/platform/qcom/iris/iris_hfi_gen2.h
> index 8170c1fef569..5fbbab844025 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen2.h
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2.h
> @@ -18,12 +18,18 @@ struct iris_core;
>    *
>    * @inst: pointer to iris_instance structure
>    * @packet: HFI packet
> + * @ipsc_properties_set: boolean to set ipsc properties to fw
> + * @opsc_properties_set: boolean to set opsc properties to fw
>    * @src_subcr_params: subscription params to fw on input port
> + * @dst_subcr_params: subscription params to fw on output port
>    */
>   struct iris_inst_hfi_gen2 {
>   	struct iris_inst		inst;
>   	struct iris_hfi_header		*packet;
> +	bool				ipsc_properties_set;
> +	bool				opsc_properties_set;
>   	struct hfi_subscription_params	src_subcr_params;
> +	struct hfi_subscription_params	dst_subcr_params;
>   };
>   
>   void iris_hfi_gen2_command_ops_init(struct iris_core *core);
> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c b/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
> index e50f00021f6d..791b535a3584 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
> @@ -472,6 +472,9 @@ static int iris_hfi_gen2_session_open(struct iris_inst *inst)
>   	if (inst->state != IRIS_INST_DEINIT)
>   		return -EALREADY;
>   
> +	inst_hfi_gen2->ipsc_properties_set = false;
> +	inst_hfi_gen2->opsc_properties_set = false;
> +
>   	inst_hfi_gen2->packet = kzalloc(4096, GFP_KERNEL);
>   	if (!inst_hfi_gen2->packet)
>   		return -ENOMEM;
> @@ -536,9 +539,185 @@ static int iris_hfi_gen2_session_close(struct iris_inst *inst)
>   	return ret;
>   }
>   
> +static int iris_hfi_gen2_session_subscribe_mode(struct iris_inst *inst,
> +						u32 cmd, u32 plane, u32 payload_type,
> +						void *payload, u32 payload_size)
> +{
> +	struct iris_inst_hfi_gen2 *inst_hfi_gen2 = to_iris_inst_hfi_gen2(inst);
> +
> +	iris_hfi_gen2_packet_session_command(inst,
> +					     cmd,
> +					     (HFI_HOST_FLAGS_RESPONSE_REQUIRED |
> +					     HFI_HOST_FLAGS_INTR_REQUIRED),
> +					     iris_hfi_gen2_get_port(plane),
> +					     inst->session_id,
> +					     payload_type,
> +					     payload,
> +					     payload_size);
> +
> +	return iris_hfi_queue_cmd_write(inst->core, inst_hfi_gen2->packet,
> +					inst_hfi_gen2->packet->size);
> +}
> +
> +static int iris_hfi_gen2_subscribe_change_param(struct iris_inst *inst, u32 plane)
> +{
> +	struct iris_inst_hfi_gen2 *inst_hfi_gen2 = to_iris_inst_hfi_gen2(inst);
> +	struct hfi_subscription_params subsc_params;
> +	u32 prop_type, payload_size, payload_type;
> +	struct iris_core *core = inst->core;
> +	const u32 *change_param = NULL;
> +	u32 change_param_size = 0;
> +	u32 payload[32] = {0};
> +	u32 hfi_port = 0;
> +	int ret;
> +	u32 i;
> +
> +	if ((V4L2_TYPE_IS_OUTPUT(plane) && inst_hfi_gen2->ipsc_properties_set) ||
> +	    (V4L2_TYPE_IS_CAPTURE(plane) && inst_hfi_gen2->opsc_properties_set)) {
> +		dev_err(core->dev, "invalid plane\n");
> +		return 0;
> +	}
> +
> +	change_param = core->iris_platform_data->input_config_params;
> +	change_param_size = core->iris_platform_data->input_config_params_size;
> +
> +	if (!change_param || !change_param_size)
> +		return -EINVAL;

That's an odd one, checking for zero but _not_ bounds checking 
chanage_param_size < (sizeof(payload)/sizeof(u32)) - 1

I'm not sure where inpug_config_param_size gets populated but I'd rather 
check that type of parameter - for exactly that reason - than the 
defensive coding done on your inputs elsewhere.

TL;DR why do you trust change_param_size < your array size but not 
change_param_size >= 1 ?

---
bod