| Message ID | 20200419151337.43293-1-jic23@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: Use an early return in iio_device_alloc to simplify code. | expand |
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; > }
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; > > }
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; > > > }
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; }