Re: [PATCH] iio: Use an early return in iio_device_alloc to simplify code.

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 20 Apr 2020 06:17:32 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Sun, 2020-04-19 at 16:13 +0100, jic23@kernel.org wrote:
> > [External]
> > 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Noticed whilst reviewing Alexandru's patch to the same function.
> > If we simply flip the logic and return NULL immediately after memory
> > allocation failure we reduce the indent of the following block and
> > end up with more 'idiomatic' kernel code.
> >   
> 
> I also was tempted to do it, but was tempted [a bit more] by the initial change
> that I goofed.
> 
> A few thoughts on this [can be ignored].
> But, since doing this change, should 'dev' be renamed to 'indio_dev'?
> It shouldn't be a lot more code than the current change [I hope].
> When looking through IIO core, I got a minor/slight confusion on this alloc code
> about the name 'dev' [which is of type 'struct iio_dev' vs 'struct device', as
> is more customary].
> 
> If 'dev' was chosen to fit within any 80 col-width limit, that limit should be
> less likely to hit now.

A different type of cleanup, so I think worth a separate patch
(even though it's messing with the same block of code.)

Got to keep to the rules I pester everyone else into following :)

So I'll apply this as is and might get the dev->indio_dev one out
after I've caught up with rest of email queue. 

Thanks,

Jonathan

> 
> 1 more inline.
> 
> Well, even with/without these changes.
> 
> Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  drivers/iio/industrialio-core.c | 38 ++++++++++++++++-----------------
> >  1 file changed, 19 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index f4daf19f2a3b..96f6dacb206d 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1504,27 +1504,27 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
> >  	alloc_size += IIO_ALIGN - 1;
> >  
> >  	dev = kzalloc(alloc_size, GFP_KERNEL);
> > +	if (!dev)
> > +		return NULL;
> >  
> > -	if (dev) {
> > -		dev->dev.groups = dev->groups;
> > -		dev->dev.type = &iio_device_type;
> > -		dev->dev.bus = &iio_bus_type;
> > -		device_initialize(&dev->dev);
> > -		dev_set_drvdata(&dev->dev, (void *)dev);
> > -		mutex_init(&dev->mlock);
> > -		mutex_init(&dev->info_exist_lock);
> > -		INIT_LIST_HEAD(&dev->channel_attr_list);
> > -
> > -		dev->id = ida_simple_get(&iio_ida, 0, 0, GFP_KERNEL);
> > -		if (dev->id < 0) {
> > -			/* cannot use a dev_err as the name isn't available */
> > -			pr_err("failed to get device id\n");
> > -			kfree(dev);
> > -			return NULL;
> > -		}
> > -		dev_set_name(&dev->dev, "iio:device%d", dev->id);
> > -		INIT_LIST_HEAD(&dev->buffer_list);
> > +	dev->dev.groups = dev->groups;
> > +	dev->dev.type = &iio_device_type;
> > +	dev->dev.bus = &iio_bus_type;
> > +	device_initialize(&dev->dev);
> > +	dev_set_drvdata(&dev->dev, (void *)dev);
> > +	mutex_init(&dev->mlock);
> > +	mutex_init(&dev->info_exist_lock);
> > +	INIT_LIST_HEAD(&dev->channel_attr_list);
> > +
> > +	dev->id = ida_simple_get(&iio_ida, 0, 0, GFP_KERNEL);
> > +	if (dev->id < 0) {
> > +		/* cannot use a dev_err as the name isn't available */
> > +		pr_err("failed to get device id\n");
> > +		kfree(dev);
> > +		return NULL;  
> 
> would it be too much for this patch to move this right after the kzalloc()?
> no strong opinion from my side to do it or not;
> but it does save some init cycles, and compresses this init block a bit;

It doesn't really save any cycles because the chance of failure of ID allocation
is negligible...  Now I'd agree with you if writing from scratch, but as a
tidy up patch, it's good to keep things really simple.


> 
> >  	}
> > +	dev_set_name(&dev->dev, "iio:device%d", dev->id);
> > +	INIT_LIST_HEAD(&dev->buffer_list);
> >  
> >  	return dev;
> >  }