On Sun, Oct 11, 2020 at 9:24 PM Lino Sanfilippo <LinoSanfilippo@xxxxxx> wrote:
>
> In function map_array_register() properly rewind in case of error.
> Furthermore remove the now superfluous initialization of "ret" with 0.
> int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
> {
> - int i = 0, ret = 0;
> + int i = 0, ret;
> struct iio_map_internal *mapi;
>
> if (maps == NULL)
> @@ -44,7 +44,18 @@ int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
> list_add_tail(&mapi->l, &iio_map_list);
> i++;
> }
> + mutex_unlock(&iio_map_list_lock);
> +
> + return 0;
> +
> error_ret:
Wait a bit.
First of all we linked all successfully added items to the list.
>From this we have two ways to go:
- leave with as many maps as we registered
- clean up and bail out
I dunno which one would play better in IIO, but you seem to go with
the latter one.
> + /* undo */
> + while (i--) {
> + mapi = list_last_entry(&iio_map_list, struct iio_map_internal,
> + l);
> + list_del(&mapi->l);
> + kfree(mapi);
> + }
We have iio_map_array_unregister(). Why not use it?
> mutex_unlock(&iio_map_list_lock);
I would rather drop a label with replacement goto -> break inside the
loop and call the following
mutex_unlock(&iio_map_list_lock);
if (ret)
iio_map_array_unregister();
return ret;
Sounds like only a few LOCs are needed.
--
With Best Regards,
Andy Shevchenko
