linux-kernel - Re: [PATCH v2] staging: pi433: move get version func to where all other functions are

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: <20220107192438.GB2693@mail.google.com>
Date:   Sat, 8 Jan 2022 08:24:38 +1300
From:   Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     gregkh@...uxfoundation.org, realwakka@...il.com,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] staging: pi433: move get version func to where all
 other functions are

On Fri, Jan 07, 2022 at 11:53:44AM +0300, Dan Carpenter wrote:
> On Fri, Jan 07, 2022 at 10:33:25AM +1300, Paulo Miguel Almeida wrote:
> > As a convention for the pi433 driver, all routines that deals with the
> > rf69 chip are defined in the rf69.c file.
> 
> That's some EnterpriseQuality[tm] style guidelines.  It's an over fussy
> rule that just makes the code harder to read for no reason.

EnterpriseQuality[tm] was witty :)

> >  
> >  	/* Ping the chip by reading the version register */
> 
> This comment doesn't make sense now.

you are right, I will change this.

> > -	retval = spi_w8r8(spi, 0x10);
> > -	if (retval < 0)
> > -		return retval;
> > +	retval = rf69_get_version(spi);
> 
> Just say:
> 
> 	retval = rf69_read_reg(spi, REG_VERSION);

atm rf69_read_reg is a static function in rf69.c.

I do agree that this is technically possible to write the code
exactly as you suggested but on the other hand, that would be the only
exception to the rule when considering all other higher-level functions
provided in the rf69.c

are you strongly set on the rf69_read_reg approach or would you 
be open to keep the original approach? (rf69_get_version)

> 	if (retval < 0)
> 		return retval;
> 
> Deleting the error handling was a bad style choice.  Also preserve the
> error code.
>

I just want to double-check if this suggestion is taking into
consideration what was mentioned here:
https://lore.kernel.org/lkml/20220106210134.GB3416@mail.google.com/

If it is, I'm happy to add it back but I just wanted to confirm it
first.

thanks,

Paulo A.