Re: [PATCH v2 06/11] staging: pi433: Split rf69_set_crc_enabled into two functions

[Greg KH]

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@xxxxxxxxxx>
> > > > ---
> > > >    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