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

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

[jic23]

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.

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;
 	}
+	dev_set_name(&dev->dev, "iio:device%d", dev->id);
+	INIT_LIST_HEAD(&dev->buffer_list);
 
 	return dev;
 }
-- 
2.26.1

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

[Ardelean, Alexandru]

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.

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;

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

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

[Jonathan Cameron]

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;
> >  }  

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

[Ardelean, Alexandru]

On Sat, 2020-04-25 at 16:47 +0100, Jonathan Cameron wrote:
> 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 :)

Fair enough.
I was assuming there would be a preference to keep the patch semantic as simple
as possible.

I thought I'd point them out, since I also looked over this bit a while back.

> 
> 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;
> > >  }