linux-kernel - Re: [PATCH] staging: pi433: #define shift constants in rf69.c

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: <20171108115230.khywgqgypxrnkryv@mwanda>
Date:   Wed, 8 Nov 2017 14:52:30 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Joshua Abraham <j.abraham1776@...il.com>
Cc:     gregkh@...uxfoundation.org, devel@...verdev.osuosl.org,
        robsonde@...il.com, dudebrobro179@...il.com,
        linux-kernel@...r.kernel.org, marcin.s.ciupak@...il.com,
        linux@...f-Entwicklungen.de, colin.king@...onical.com
Subject: Re: [PATCH] staging: pi433: #define shift constants in rf69.c

On Wed, Nov 08, 2017 at 06:25:06AM -0500, Joshua Abraham wrote:
> This patch completes TODO improvements in rf69.c to change shift
> constants to a define.
> 
> Signed-off-by: Joshua Abraham <j.abraham1776@...il.com>
> ---
>  drivers/staging/pi433/rf69.c           | 4 ++--
>  drivers/staging/pi433/rf69_registers.h | 4 ++++
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index e69a2153c999..cfcace195be9 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -102,7 +102,7 @@ enum modulation rf69_get_modulation(struct spi_device *spi)
>  
>  	currentValue = READ_REG(REG_DATAMODUL);
>  
> -	switch (currentValue & MASK_DATAMODUL_MODULATION_TYPE >> 3) { // TODO improvement: change 3 to define
> +	switch (currentValue & MASK_DATAMODUL_MODULATION_TYPE >> SHIFT_DATAMODUL_MODE) {

You've send a few of mechanical patches without waiting for feedback and
you should probably slow down...

The first thing to notice is that the original code is probably buggy
and needs parenthesis.

	switch ((currentValue & MASK_DATAMODUL_MODULATION_TYPE) >> 3) {

But that still doesn't fix the problem that x18 >> 3 is never going to
equal to DATAMODUL_MODULATION_TYPE_OOK which is 0x8...  So there are a
couple bugs here.

The line is over 80 characters, so checkpatch will complain about your
patch.  Please run checkpatch.pl on all your patches.  Really, I hate
all the naming here...  Surely we can think of a better name than
MASK_DATAMODUL_MODULATION_TYPE?  Normally the "MASK" and "SHIFT" part of
the name go at the end instead of the start.

>  /* RegDataModul */
> +#define  SHIFT_DATAMODUL_MODE				0x03
> +
>  #define  MASK_DATAMODUL_MODE			0x06

Why did you add a blank line?  Don't use hex values for shifting, use
normal numbers.  The 0x3 is indented too far.

Anyway, take your time and really think about patches before you send
them.  Normally, I write a patch, then wait overnight, then review it
and again in the morning before I send it.

regards,
dan carpenter