linux-kernel - Re: [PATCH] media: atomisp: pci: hive_isp_css_common: host: vmem: Replace SUBWORD macros with 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] [day] [month] [year] [list]

Message-ID: <5860bf02-6b55-a885-742b-cd1477c7b1aa@redhat.com>
Date:   Mon, 23 Jan 2023 13:28:02 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Brent Pappas <bpappas@...pasbrent.com>
Cc:     mchehab@...nel.org, sakari.ailus@...ux.intel.com,
        gregkh@...uxfoundation.org, linux-media@...r.kernel.org,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: atomisp: pci: hive_isp_css_common: host: vmem:
 Replace SUBWORD macros with functions

Hi,

On 1/20/23 19:26, Brent Pappas wrote:
> Replace the macros SUBWORD() and INV_SUBWORD() with functions to comply
> with Linux coding style standards.
> 
> Signed-off-by: Brent Pappas <bpappas@...pasbrent.com>


Thank you.

I have added this to my personal git tree now and I will include
this in the atomisp driver pull-req which I will send to the
media-subsystem maintainer in a couple of weeks.

Regards,

Hans



> ---
> I am not sure if it would better to inline SUBWORD() or turn it into
> a function.
> On the one hand, SUBWORD() is only invoked once, so it may be better to
> to inline it.
> On the other hand, the macro defined beside it, INV_SUBWORD() should be
> turned into to a function because it is invoked multiple times, so it
> may make sense to turn SUBWORD() into a function as well.
> I have opted to turn SUBWORD() into a function.
> 
>  .../pci/hive_isp_css_common/host/vmem.c       | 21 +++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/vmem.c b/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/vmem.c
> index 6620f091442f..316abfb72a83 100644
> --- a/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/vmem.c
> +++ b/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/vmem.c
> @@ -28,10 +28,19 @@ typedef hive_uedge *hive_wide;
>  /* Copied from SDK: sim_semantics.c */
>  
>  /* subword bits move like this:         MSB[____xxxx____]LSB -> MSB[00000000xxxx]LSB */
> -#define SUBWORD(w, start, end)     (((w) & (((1ULL << ((end) - 1)) - 1) << 1 | 1)) >> (start))
> +static inline hive_uedge
> +subword(hive_uedge w, unsigned int start, unsigned int end)
> +{
> +	return (w & (((1ULL << (end - 1)) - 1) << 1 | 1)) >> start;
> +}
>  
>  /* inverse subword bits move like this: MSB[xxxx____xxxx]LSB -> MSB[xxxx0000xxxx]LSB */
> -#define INV_SUBWORD(w, start, end) ((w) & (~(((1ULL << ((end) - 1)) - 1) << 1 | 1) | ((1ULL << (start)) - 1)))
> +static inline hive_uedge
> +inv_subword(hive_uedge w, unsigned int start, unsigned int end)
> +{
> +	return w & (~(((1ULL << (end - 1)) - 1) << 1 | 1) |
> +		    ((1ULL << start) - 1));
> +}
>  
>  #define uedge_bits (8 * sizeof(hive_uedge))
>  #define move_lower_bits(target, target_bit, src, src_bit) move_subword(target, target_bit, src, 0, src_bit)
> @@ -50,18 +59,18 @@ move_subword(
>  	unsigned int start_bit  = target_bit % uedge_bits;
>  	unsigned int subword_width = src_end - src_start;
>  
> -	hive_uedge src_subword = SUBWORD(src, src_start, src_end);
> +	hive_uedge src_subword = subword(src, src_start, src_end);
>  
>  	if (subword_width + start_bit > uedge_bits) { /* overlap */
>  		hive_uedge old_val1;
> -		hive_uedge old_val0 = INV_SUBWORD(target[start_elem], start_bit, uedge_bits);
> +		hive_uedge old_val0 = inv_subword(target[start_elem], start_bit, uedge_bits);
>  
>  		target[start_elem] = old_val0 | (src_subword << start_bit);
> -		old_val1 = INV_SUBWORD(target[start_elem + 1], 0,
> +		old_val1 = inv_subword(target[start_elem + 1], 0,
>  				       subword_width + start_bit - uedge_bits);
>  		target[start_elem + 1] = old_val1 | (src_subword >> (uedge_bits - start_bit));
>  	} else {
> -		hive_uedge old_val = INV_SUBWORD(target[start_elem], start_bit,
> +		hive_uedge old_val = inv_subword(target[start_elem], start_bit,
>  						 start_bit + subword_width);
>  
>  		target[start_elem] = old_val | (src_subword << start_bit);