linux-kernel - Re: [PATCH] staging: vt6655: Fix line wrapping in rf.c file

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:   Tue, 19 Oct 2021 05:26:05 -0700
From:   Joe Perches <joe@...ches.com>
To:     Karolina Drobnik <karolinadrobnik@...il.com>,
        Greg KH <gregkh@...uxfoundation.org>
Cc:     outreachy-kernel@...glegroups.com, forest@...ttletooquiet.net,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: vt6655: Fix line wrapping in rf.c file

On Tue, 2021-10-19 at 11:59 +0100, Karolina Drobnik wrote:
> Hi,
> 
> Thank you very much for your comments.
> 
> On Mon, 2021-10-18 at 17:12 +0200, Greg KH wrote:
> > Also, these are all just fine as-is for now.  A better way to make
> > these lines smaller is to use better variable and function names 
> > that are shorter and make sense :)
> 
> I have v2 ready but I'm not sure, given the Joe's patch, if my solution
> is a satisfactory one. I didn't jump on such refactoring as I'm still
> learning about the codebase/process and didn't want to muddle the
> waters (...more than I do already).
> 
> Greg, what would you prefer? Should I back up with my patch, pick
> something else and let Joe's patch be merged?

What I suggested is not a patch it's just an example.

There's quite a lot of code in that driver that _could_
be updated/refined/refactored (none of which _I_ will
submit), but it's up to you do whatever _you_ want.

You could:

o remove the Hungarian notation
o convert the mixed case variables to snake case
o remove unnecessary function definitions and make them static
o refactor various functions

Generally, I prefer refactoring code to make it simpler or
more like the generally preferred kernel styles.

Another option would be to submit a completely new driver for
this device based on this existing driver as what's there isn't
particularly great IMO, but read the vt6655 TODO file and see if
there's something you actually want to do there.

> Also, I have a question about the patch if that's ok :)

It's OK to ask.

> On Mon, 2021-10-18 at 22:56 -0700, Joe Perches wrote:
> > Maybe some refactoring like:
> > ---
[]
> > diff --git a/drivers/staging/vt6655/rf.c
[]
> > +       uChannel--;
> 
> I see that you introduced `uChannel--` to further tidy up the lines
> with `[uChannel - 1]`. In general, is there anything wrong with
> indexing like `i - 1`?

Depends on how often it's used and if it's ever missed accidentally.

> What's the preference here? DRY things up as much as possible?

Up to you

> I'm asking because when I was reading this line, at first, it wasn't
> clear to me why we could decrement it (example though: "Was this
> modified earlier? Do we need to "correct" it?").

Generally, just try to make code clear for a reader.

When you do that, the compiler will also do a better job
at what it does.

If you look at the callers of the function, see if it's better
to decrement the argument instead.