linux-kernel - Re: [PATCH v2 08/24] media: iris: Allow stop on firmware only if start was issued.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4fe5d966-f788-4fd7-9e74-6d63ecc8dcb3@linaro.org>
Date: Sat, 16 Aug 2025 12:07:15 +0100
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Dikshita Agarwal <quic_dikshita@...cinc.com>,
 Vikash Garodia <quic_vgarodia@...cinc.com>,
 Abhinav Kumar <abhinav.kumar@...ux.dev>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Hans Verkuil
 <hverkuil@...all.nl>, Stefan Schmidt <stefan.schmidt@...aro.org>,
 Vedang Nagar <quic_vnagar@...cinc.com>
Cc: linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org,
 linux-kernel@...r.kernel.org, Renjiang Han <quic_renjiang@...cinc.com>,
 Wangao Wang <quic_wangaow@...cinc.com>
Subject: Re: [PATCH v2 08/24] media: iris: Allow stop on firmware only if
 start was issued.

On 13/08/2025 10:37, Dikshita Agarwal wrote:
> For HFI Gen1, the instances substate is changed to LOAD_RESOURCES only
> when a START command is issues to the firmware. If STOP is called
> without a prior START, the firmware may reject the command and throw
> some erros.
> Handle this by adding a substate check before issuing STOP command to
> the firmware.
> 
> Fixes: 11712ce70f8e ("media: iris: implement vb2 streaming ops")
> Reviewed-by: Vikash Garodia <quic_vgarodia@...cinc.com>
> Tested-by: Vikash Garodia <quic_vgarodia@...cinc.com> # X1E80100
> Signed-off-by: Dikshita Agarwal <quic_dikshita@...cinc.com>
> ---
>   drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> index 5fc30d54af4dc34616cfd08813940aa0b7044a20..5f1748ab80f88393215fc2d82c5c6b4af1266090 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> @@ -184,11 +184,12 @@ static int iris_hfi_gen1_session_stop(struct iris_inst *inst, u32 plane)
>   	u32 flush_type = 0;
>   	int ret = 0;
>   
> -	if ((V4L2_TYPE_IS_OUTPUT(plane) &&
> -	     inst->state == IRIS_INST_INPUT_STREAMING) ||
> +	if (((V4L2_TYPE_IS_OUTPUT(plane) &&
> +	      inst->state == IRIS_INST_INPUT_STREAMING) ||

this is becoming a highly complex clause

         if (((V4L2_TYPE_IS_OUTPUT(plane) &&
               inst->state == IRIS_INST_INPUT_STREAMING) ||
             (V4L2_TYPE_IS_CAPTURE(plane) &&
              inst->state == IRIS_INST_OUTPUT_STREAMING) ||
             inst->state == IRIS_INST_ERROR) &&
                 inst->sub_state & IRIS_INST_SUB_LOAD_RESOURCES) {

can we not reduce down the number of conjunctions and dis-junctions here ?

Its getting hard to follow.

For example pivot on if (inst->sub_state & IRIS_INST_SUB_LOAD_RESOURCES)

or make it into a switch for inst->state... no that wouldn't work

Either way the complexity of this clause is indicating to me we need to 
do some decomposition.

Please consider if you can rationalise the logic here and make the code 
more readable.
>   	    (V4L2_TYPE_IS_CAPTURE(plane) &&
>   	     inst->state == IRIS_INST_OUTPUT_STREAMING) ||
> -	    inst->state == IRIS_INST_ERROR) {
> +	    inst->state == IRIS_INST_ERROR) &&
> +		inst->sub_state & IRIS_INST_SUB_LOAD_RESOURCES) {
>   		reinit_completion(&inst->completion);
>   		iris_hfi_gen1_packet_session_cmd(inst, &pkt, HFI_CMD_SESSION_STOP);
>   		ret = iris_hfi_queue_cmd_write(core, &pkt, pkt.shdr.hdr.size);
> 

---
bod