linux-kernel - Re: [PATCH v2 2/8] staging: vt6655: Use incrementation in `idx`

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]

Date:   Thu, 4 Nov 2021 20:40:18 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Joe Perches <joe@...ches.com>
Cc:     Karolina Drobnik <karolinadrobnik@...il.com>,
        outreachy-kernel@...glegroups.com, gregkh@...uxfoundation.org,
        forest@...ttletooquiet.net, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/8] staging: vt6655: Use incrementation in `idx`

On Thu, Nov 04, 2021 at 07:44:22AM -0700, Joe Perches wrote:
> On Thu, 2021-11-04 at 16:00 +0300, Dan Carpenter wrote:
> > On Mon, Nov 01, 2021 at 02:32:00PM +0000, Karolina Drobnik wrote:
> > > Increment `idx` in a loop instead of adding the loop counter
> > > `i` to do so. Thanks to this change, the cast to unsigned short
> > > can be removed.
> > > 
> > > Signed-off-by: Karolina Drobnik <karolinadrobnik@...il.com>
> > > ---
> > >  drivers/staging/vt6655/rf.c | 16 ++++++++--------
> > >  1 file changed, 8 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
> []
> > > @@ -700,11 +700,11 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char rf_type,
> > >  			return false;
> > >  
> > >  		for (i = 0; i < CB_AL2230_INIT_SEQ; i++)
> > > -			MACvSetMISCFifo(priv, (unsigned short)(idx + i), al2230_init_table[i]);
> > > +			MACvSetMISCFifo(priv, idx++, al2230_init_table[i]);
> > 
> > Of course, idx is no longer a constant so declaring it as a variable
> > makes sense here.  But maybe just do it in the same patch because the
> > patch 1/1 doesn't make sense as a stand alone patch.
> > 
> > Also don't declare idx as an unsigned short.  It's better to declare it
> > as a int so it just works like a normal number and you don't have to
> > think about signedness bugs and wrapping and edge cases.
> 
> No, IMO it really should be a u16.
> Look at the iowrite16() within the call to MACvSetMISCFifo.

Obviously we all agree it has no effect on runtime at all.

I just think that people get into trouble getting too creative with
their types.  There was some discussion earlier about where the casting
was required (it's not required at all).  It seems like there was some
confusion about how types work.  Declaring it as "unsigned short" has
some documentation value, I guess, but you avoid signedness bugs when
you declare things as int.

I don't care too much, but I also try to tell people not to be fancy.

regards,
dan carpenter