On Mon, 20 Apr 2020 06:17:32 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@xxxxxxxxxx> wrote:
> On Sun, 2020-04-19 at 16:13 +0100, jic23@xxxxxxxxxx wrote:
> > [External]
> >
> > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > Cc: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> > ---
> > 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;
> > }
