linux-kernel - Re: [PATCH v3 3/8] media: iris: stop copying r/o data

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <j5za2knx6qmv6c7j6tszwpzawu5iugcnbgkfxptfxorm5zkzh4@ih3ylae7fngs>
Date: Wed, 15 Oct 2025 03:13:44 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Vikash Garodia <vikash.garodia@....qualcomm.com>
Cc: Dikshita Agarwal <dikshita.agarwal@....qualcomm.com>,
        Abhinav Kumar <abhinav.kumar@...ux.dev>,
        Bryan O'Donoghue <bod@...nel.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
        linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/8] media: iris: stop copying r/o data

On Tue, Oct 14, 2025 at 01:34:59PM +0530, Vikash Garodia wrote:
> 
> On 10/13/2025 7:38 AM, Dmitry Baryshkov wrote:
> > Most of the platform_inst_caps data is read-only. In order to lower the
> > amount of memory consumed by the driver, store the value and the
> > corresponding index in the read-write data and use the rest via the
> > pointer to r/o capability data.
> > 
> > Keep all read-only flags inside platform_inst_fw_cap.flags and transform
> > CAP_FLAG_CLIENT_SET (which is being set per-instance once the client
> > changes any of the controls) into the boolean field inside struct
> > platform_inst_fw_cap_value.
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
> > ---
> >  drivers/media/platform/qcom/iris/iris_core.h       |   4 +-
> >  drivers/media/platform/qcom/iris/iris_ctrls.c      | 238 ++++++++++-----------
> >  drivers/media/platform/qcom/iris/iris_instance.h   |   3 +-
> >  .../platform/qcom/iris/iris_platform_common.h      |   8 +-
> >  drivers/media/platform/qcom/iris/iris_vdec.c       |   5 +-
> >  drivers/media/platform/qcom/iris/iris_venc.c       |   5 +-
> >  6 files changed, 135 insertions(+), 128 deletions(-)
> > 
> > @@ -312,14 +317,8 @@ void iris_session_init_caps(struct iris_core *core)
> >  		if (!iris_valid_cap_id(cap_id))
> >  			continue;
> >  
> > -		core->inst_fw_caps_dec[cap_id].cap_id = caps[i].cap_id;
> > -		core->inst_fw_caps_dec[cap_id].min = caps[i].min;
> > -		core->inst_fw_caps_dec[cap_id].max = caps[i].max;
> > -		core->inst_fw_caps_dec[cap_id].step_or_mask = caps[i].step_or_mask;
> > +		core->inst_fw_caps_dec[cap_id].idx = i;
> 
> Consider a case when one of the cap is not present for a target, then idx and
> cap_id would not match, and any further usage to map each other would go wrong.
> Please keep idx and cap_id in sync so that driver can use it as key to pull out
> from inst_fw_caps.
> 
> >  		core->inst_fw_caps_dec[cap_id].value = caps[i].value;
> > -		core->inst_fw_caps_dec[cap_id].flags = caps[i].flags;
> > -		core->inst_fw_caps_dec[cap_id].hfi_id = caps[i].hfi_id;
> > -		core->inst_fw_caps_dec[cap_id].set = caps[i].set;
> >  	}
> >  
> >  	caps = core->iris_platform_data->inst_fw_caps_enc;
> > @@ -330,29 +329,23 @@ void iris_session_init_caps(struct iris_core *core)
> >  		if (!iris_valid_cap_id(cap_id))
> >  			continue;
> >  
> > -		core->inst_fw_caps_enc[cap_id].cap_id = caps[i].cap_id;
> > -		core->inst_fw_caps_enc[cap_id].min = caps[i].min;
> > -		core->inst_fw_caps_enc[cap_id].max = caps[i].max;
> > -		core->inst_fw_caps_enc[cap_id].step_or_mask = caps[i].step_or_mask;
> > +		core->inst_fw_caps_enc[cap_id].idx = i;
> 
> The above comment is directly seen on encoder, where the cap_id in platform data
> is not available in serial order for the given SOC.
> 
> For SM8250, the cap_id 3 is not present in the platform data, so when idx 3 is
> used as cap_idx to pick the cap from inst_fw_caps, it returns 0 as the cap_id,
> which is incorrect as it points to first cap in inst_fw_caps. This happens for
> all cases when idx is used for cap_id which is not defined for that SOC.
> 
> [ 1234.157453] iris_session_init_caps: cap_id: 9 idx: 0
> [ 1234.167761] iris_session_init_caps: cap_id: 1 idx: 1
> [ 1234.178240] iris_session_init_caps: cap_id: 2 idx: 2
> 
> ===> cap_id: 3 would have idx:0

I see. And this is the reason why cap_ids starts enumeration from 1:
this way all missing entries have core->inst_fw_caps_*[id].cap_id = 0
and are ignored by the rest of the driver.

I will see what is the best way to handle it in the driver.

> 
> [ 1234.188498] iris_session_init_caps: cap_id: 4 idx: 3
> [ 1234.198734] iris_session_init_caps: cap_id: 5 idx: 4
> [ 1234.208954] iris_session_init_caps: cap_id: 16 idx: 5
> [ 1234.219354] iris_session_init_caps: cap_id: 18 idx: 6
> [ 1234.229735] iris_session_init_caps: cap_id: 20 idx: 7
> 
> >  		core->inst_fw_caps_enc[cap_id].value = caps[i].value;
> > -		core->inst_fw_caps_enc[cap_id].flags = caps[i].flags;
> > -		core->inst_fw_caps_enc[cap_id].hfi_id = caps[i].hfi_id;
> > -		core->inst_fw_caps_enc[cap_id].set = caps[i].set;
> >  	}
> >  }
> >  

-- 
With best wishes
Dmitry