linux-kernel - Re: [PATCH 10/33] iris: vidc: add helper functions

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <8d5b117e-4743-c006-7e7b-a15bd3866e6d@quicinc.com>
Date:   Tue, 15 Aug 2023 00:45:49 +0530
From:   Dikshita Agarwal <quic_dikshita@...cinc.com>
To:     Konrad Dybcio <konrad.dybcio@...aro.org>,
        Vikash Garodia <quic_vgarodia@...cinc.com>,
        <stanimir.k.varbanov@...il.com>, <agross@...nel.org>,
        <andersson@...nel.org>, <mchehab@...nel.org>,
        <hans.verkuil@...co.com>, <linux-kernel@...r.kernel.org>,
        <linux-media@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH 10/33] iris: vidc: add helper functions



On 7/28/2023 11:11 PM, Konrad Dybcio wrote:
> On 28.07.2023 15:23, Vikash Garodia wrote:
>> This implements common helper functions for v4l2 to vidc and
>> vice versa conversion for different enums.
>> Add helpers for state checks, buffer management, locks etc.
>>
>> Signed-off-by: Dikshita Agarwal <quic_dikshita@...cinc.com>
>> Signed-off-by: Vikash Garodia <quic_vgarodia@...cinc.com>
>> ---
> [...]
> 
>> +
>> +#define is_odd(val) ((val) % 2 == 1)
>> +#define in_range(val, min, max) (((min) <= (val)) && ((val) <= (max)))
>> +#define COUNT_BITS(a, out) {       \
> hweight.* functions?
> 
> [...]
> 
sure, will replace with hweight.
>> +
>> +const char *cap_name(enum msm_vidc_inst_capability_type cap_id)
>> +{
>> +	const char *name = "UNKNOWN CAP";
> Perhaps it'd be worth to include the unknown cap id here
> 
could you please elaborate more on this.
>> +
>> +	if (cap_id >= ARRAY_SIZE(cap_name_arr))
>> +		goto exit;
>> +
>> +	name = cap_name_arr[cap_id];
>> +
>> +exit:
>> +	return name;
>> +}
> [...]
> 
>> +
>> +const char *buf_name(enum msm_vidc_buffer_type type)
>> +{
>> +	const char *name = "UNKNOWN BUF";
> Similarly here
> 
could you please elaborate more on this.
>> +
>> +	if (type >= ARRAY_SIZE(buf_type_name_arr))
>> +		goto exit;
>> +
>> +	name = buf_type_name_arr[type];
>> +
>> +exit:
>> +	return name;
>> +}
> [...]
> 
>> +const char *v4l2_type_name(u32 port)
>> +{
>> +	switch (port) {
> switch-case seems a bit excessive here.
> 
>> +	case INPUT_MPLANE:      return "INPUT";
>> +	case OUTPUT_MPLANE:     return "OUTPUT";
>> +	}
>> +
>> +	return "UNKNOWN";
>> +}
that's right, will fix in next version
> [...]
> 
> There's some more stuff I'd comment on, but 4500 lines in a single patch
> is way too much to logically follow.
> 
> Couple more style suggestions:
> - use Reverse-Christmas-tree sorting for variable declarations
> - some oneliner functions could possibly become preprocessor macros
> - when printing giant debug messages, you may want to use loops
> - make sure your indentation is in order, 100 chars per line is
>   totally fine
> - generally inline magic hex values are discouraged, but if they're
>   necessary, the hex should be lowercase
Nice suggestions! will take care of these comments in next version.

Thanks,
Dikshita
> 
> Konrad