linux-kernel - Re: [Outreachy kernel] [PATCH 5/7] staging: vt6655: Rewrite conditional in AL7320 initialization

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: <1683328.aCfAWUeHFl@localhost.localdomain>
Date:   Thu, 28 Oct 2021 14:36:59 +0200
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     outreachy-kernel@...glegroups.com
Cc:     gregkh@...uxfoundation.org, forest@...ttletooquiet.net,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        Karolina Drobnik <karolinadrobnik@...il.com>,
        Karolina Drobnik <karolinadrobnik@...il.com>
Subject: Re: [Outreachy kernel] [PATCH 5/7] staging: vt6655: Rewrite conditional in AL7320 initialization

On Thursday, October 28, 2021 12:35:35 PM CEST Karolina Drobnik wrote:
> Use conditional operator to determine which table for AL7320
> initialization should be used. Use `data` temporary value to
> store this value.
> 
> Signed-off-by: Karolina Drobnik <karolinadrobnik@...il.com>
> ---
>  drivers/staging/vt6655/rf.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
> index afd202ea3356..af4eb7eb8e7d 100644
> --- a/drivers/staging/vt6655/rf.c
> +++ b/drivers/staging/vt6655/rf.c
> @@ -716,13 +716,10 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, 
unsigned char rf_type,
>  		if (init_count > (MISCFIFO_SYNDATASIZE - sleep_count))
>  			return false;
>  
> -		if (channel <= CB_MAX_CHANNEL_24G) {
> -			for (i = 0; i < CB_AL7230_INIT_SEQ; i++)
> -				MACvSetMISCFifo(priv, idx++, 
al7230_init_table[i]);
> -		} else {
> -			for (i = 0; i < CB_AL7230_INIT_SEQ; i++)
> -				MACvSetMISCFifo(priv, idx++, 
al7230_init_table_a_mode[i]);
> -		}
> +		data = (channel <= CB_MAX_CHANNEL_24G) ?
> +			al7230_init_table : 
al7230_init_table_a_mode;

As far as I know by reading some Greg K-H's replies to other developers, this 
"<test> ? <true> : <false>" style is not well accepted here.

I'd prefer to see an explicit "if-else" statement because with that style you 
sacrifice readability and gain nothing here.

> +		for (i = 0; i < CB_AL7230_INIT_SEQ; i++)
> +			MACvSetMISCFifo(priv, idx++, *(data++));

Again, as Julia pointed out, "*data++" and "*(data++)" are syntactically 
correct instructions but are not required here. I'm also pretty sure that, by 
not reusing that index, you're adding additional unnecessary instructions to 
the resulting assembly code (unless the compiler is able to optimize it).

"*foo++" and the like are powerful and compact instructions, but you should 
use them in other, more suitable contexts.

Thanks,

Fabio


>  
>  		MACvSetMISCFifo(priv, idx++, 
al7230_channel_table0[channel - 1]);
>  		MACvSetMISCFifo(priv, idx++, 
al7230_channel_table1[channel - 1]);
> -- 
> 2.30.2
> 
> -- 
> You received this message because you are subscribed to the Google Groups 
"outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
email to outreachy-kernel+unsubscribe@...glegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/
outreachy-kernel/
948406a3e7d23f1cdf866aa4448d9428bdd32512.1635415820.git.karolinadrobnik%40gmail.com.
>