linux-kernel - Re: [PATCH v2 06/11] staging: pi433: Split rf69_set_crc

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 6 Dec 2017 16:11:11 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Marcus Wolf <marcus.wolf@...rthome-wolf.de>
Cc:     Dan Carpenter <dan.carpenter@...cle.com>,
        devel@...verdev.osuosl.org, linux@...f-Entwicklungen.de,
        linux-kernel@...r.kernel.org,
        Simon Sandström <simon@...anor.nu>
Subject: Re: [PATCH v2 06/11] staging: pi433: Split rf69_set_crc_enabled into
 two functions

On Wed, Dec 06, 2017 at 12:07:20PM +0200, Marcus Wolf wrote:
> 
> 
> Am 06.12.2017 um 11:37 schrieb Dan Carpenter:
> > On Wed, Dec 06, 2017 at 11:05:22AM +0200, Marcus Wolf wrote:
> > > 
> > > 
> > > Am 06.12.2017 um 00:08 schrieb Simon Sandström:
> > > > Splits rf69_set_crc_enabled(dev, enabled) into
> > > > rf69_enable_crc(dev) and rf69_disable_crc(dev).
> > > > 
> > > > Signed-off-by: Simon Sandström <simon@...anor.nu>
> > > > ---
> > > >    drivers/staging/pi433/pi433_if.c | 22 ++++++++++++++++++++--
> > > >    drivers/staging/pi433/rf69.c     | 18 ++++++------------
> > > >    drivers/staging/pi433/rf69.h     |  4 ++--
> > > >    3 files changed, 28 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> > > > index 2ae19ac565d1..614eec7dd904 100644
> > > > --- a/drivers/staging/pi433/pi433_if.c
> > > > +++ b/drivers/staging/pi433/pi433_if.c
> > > > @@ -216,7 +216,16 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
> > > >    			return ret;
> > > >    	}
> > > >    	SET_CHECKED(rf69_set_adressFiltering(dev->spi, rx_cfg->enable_address_filtering));
> > > > -	SET_CHECKED(rf69_set_crc_enable	    (dev->spi, rx_cfg->enable_crc));
> > > > +
> > > > +	if (rx_cfg->enable_crc == OPTION_ON) {
> > > > +		ret = rf69_enable_crc(dev->spi);
> > > > +		if (ret < 0)
> > > > +			return ret;
> > > > +	} else {
> > > > +		ret = rf69_disable_crc(dev->spi);
> > > > +		if (ret < 0)
> > > > +			return ret;
> > > > +	}
> > > 
> > > Why don't you use SET_CHECKED(...)?
> > > 
> > 
> > Marcus, please don't introduce new uses of SET_CHECKED().  It has a
> > hidden return in it which is against kernel style and introduces very
> > predictable and avoidable bugs.  For example, in probe().
> 
> Ah ok.
> 
> Thanks for clarifiytion!
> 
> What a pitty - another bunch of extra lines of code...
> 
> Or is there an other construction, allowing for one line per register
> change? Something like
> 
> 	ret = rf69_set_xyz(...); if (ret) return ret;
> 	ret = rf69_set_abc(...); if (ret) return ret;
> 
> is pretty ugly and voids the style guide...

Just spell it out:
	ret = rf69_set_xyz();
	if (ret)
		goto unwind_xyz;

Almost never do you want to instantly return.  You should clean up from
the error first.

But if you do just want to exit, that's fine too, just return.  That's
the normal way here, don't do funny things in macros (like return from a
function), that way lies madness...

thanks,

greg k-h