[PATCH] iio:core: In map_array_register() cleanup in case of error

[PATCH] iio:core: In map_array_register() cleanup in case of error

[Lino Sanfilippo]

In function map_array_register() properly rewind in case of error.
Furthermore save an extra label by using a break instead of goto to leave
the concerning loop.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/iio/inkern.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 5a8351c..0735cc4 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -28,6 +28,7 @@ int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
 {
 	int i = 0, ret = 0;
 	struct iio_map_internal *mapi;
+	struct list_head *pos, *tmp;

 	if (maps == NULL)
 		return 0;
@@ -37,14 +38,22 @@ int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
 		mapi = kzalloc(sizeof(*mapi), GFP_KERNEL);
 		if (mapi == NULL) {
 			ret = -ENOMEM;
-			goto error_ret;
+			break;
 		}
 		mapi->map = &maps[i];
 		mapi->indio_dev = indio_dev;
 		list_add_tail(&mapi->l, &iio_map_list);
 		i++;
 	}
-error_ret:
+
+	if (ret) { /* undo */
+		while (i--) {
+			mapi = list_last_entry(&iio_map_list,
+					       struct iio_map_internal, l);
+			list_del(&mapi->l);
+			kfree(mapi);
+		}
+	}
 	mutex_unlock(&iio_map_list_lock);

 	return ret;
--
2.7.4

Re: [PATCH] iio:core: In map_array_register() cleanup in case of error

[Jonathan Cameron]

On Sun, 11 Oct 2020 14:41:20 +0200
Lino Sanfilippo <LinoSanfilippo@gmx.de> wrote:

> In function map_array_register() properly rewind in case of error.
> Furthermore save an extra label by using a break instead of goto to leave
> the concerning loop.
> 
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

Good spot. I'd rather we went with a different code flow though.
See below.

Thanks,

Jonathan


> ---
>  drivers/iio/inkern.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 5a8351c..0735cc4 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -28,6 +28,7 @@ int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
>  {
>  	int i = 0, ret = 0;
>  	struct iio_map_internal *mapi;
> +	struct list_head *pos, *tmp;
> 
>  	if (maps == NULL)
>  		return 0;
> @@ -37,14 +38,22 @@ int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
>  		mapi = kzalloc(sizeof(*mapi), GFP_KERNEL);
>  		if (mapi == NULL) {
>  			ret = -ENOMEM;
> -			goto error_ret;
> +			break;
>  		}
>  		mapi->map = &maps[i];
>  		mapi->indio_dev = indio_dev;
>  		list_add_tail(&mapi->l, &iio_map_list);
>  		i++;
>  	}
> -error_ret:
> +
Please do this as a a separate error path rather than having
it as non trivial unwinding in the main code flow.

i.e. goto error_ret, but handle that path separately after
the return below.

> +	if (ret) { /* undo */
> +		while (i--) {
> +			mapi = list_last_entry(&iio_map_list,
> +					       struct iio_map_internal, l);
> +			list_del(&mapi->l);
> +			kfree(mapi);
> +		}
> +	}
>  	mutex_unlock(&iio_map_list_lock);
> 
>  	return ret;
> --
> 2.7.4
> 

Re: [PATCH] iio:core: In map_array_register() cleanup in case of error

[Lino Sanfilippo]

Hi,

Am 11.10.20 um 17:07 schrieb Jonathan Cameron:
>
> Good spot. I'd rather we went with a different code flow though.
> See below.
>

Ok, will adjust the code and send a V2 shortly.

Regards,
Lino

[PATCH v2] iio:core: In map_array_register() cleanup in case of error

[Lino Sanfilippo]

In function map_array_register() properly rewind in case of error.
Furthermore remove the now superfluous initialization of "ret" with 0.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/iio/inkern.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 5a8351c..9dedc30 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -26,7 +26,7 @@ static DEFINE_MUTEX(iio_map_list_lock);

 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:
+	/* undo */
+	while (i--) {
+		mapi = list_last_entry(&iio_map_list, struct iio_map_internal,
+				       l);
+		list_del(&mapi->l);
+		kfree(mapi);
+	}
 	mutex_unlock(&iio_map_list_lock);

 	return ret;
--
2.7.4

Re: [PATCH v2] iio:core: In map_array_register() cleanup in case of error

[Andy Shevchenko]

On Sun, Oct 11, 2020 at 9:24 PM Lino Sanfilippo <LinoSanfilippo@gmx.de> 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

Re: [PATCH v2] iio:core: In map_array_register() cleanup in case of error

[Lino Sanfilippo]

Hi Andy,

Thanks for you suggestion. I agree that it is nicer to reuse the cleanup in
iio_map_array_unregister() than to reimplement it. However I would like to
avoid dropping and regaining the mutex in case of error.
What about the attached approach:

Introduce an unlocked version of iio_map_array_unregister() and then use
this one to unwind in error case. Thus we only need to add two lines of
code, so that Jonathan may be ok with adding it to the main code flow...

Regards,
Lino

[PATCH 1/2] iio:core: Introduce unlocked version of iio_map_array_unregister()

[Lino Sanfilippo]

Introduce an unlocked version of iio_map_array_unregister(). This function
can help to unwind in case of error while the iio_map_list_lock mutex is
held.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/iio/inkern.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index ede99e0..39c1d63 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -24,6 +24,21 @@ struct iio_map_internal {
 static LIST_HEAD(iio_map_list);
 static DEFINE_MUTEX(iio_map_list_lock);
 
+static int iio_map_array_unregister_locked(struct iio_dev *indio_dev)
+{
+	int ret = -ENODEV;
+	struct iio_map_internal *mapi, *next;
+
+	list_for_each_entry_safe(mapi, next, &iio_map_list, l) {
+		if (indio_dev == mapi->indio_dev) {
+			list_del(&mapi->l);
+			kfree(mapi);
+			ret = 0;
+		}
+	}
+	return ret;
+}
+
 int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
 {
 	int i = 0, ret = 0;
@@ -57,18 +72,12 @@ EXPORT_SYMBOL_GPL(iio_map_array_register);
  */
 int iio_map_array_unregister(struct iio_dev *indio_dev)
 {
-	int ret = -ENODEV;
-	struct iio_map_internal *mapi, *next;
+	int ret;
 
 	mutex_lock(&iio_map_list_lock);
-	list_for_each_entry_safe(mapi, next, &iio_map_list, l) {
-		if (indio_dev == mapi->indio_dev) {
-			list_del(&mapi->l);
-			kfree(mapi);
-			ret = 0;
-		}
-	}
+	ret = iio_map_array_unregister_locked(indio_dev);
 	mutex_unlock(&iio_map_list_lock);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(iio_map_array_unregister);
-- 
2.7.4

Re: [PATCH 1/2] iio:core: Introduce unlocked version of iio_map_array_unregister()

[Andy Shevchenko]

On Sun, Oct 18, 2020 at 4:13 AM Lino Sanfilippo <LinoSanfilippo@gmx.de> wrote:
>
> Introduce an unlocked version of iio_map_array_unregister(). This function
> can help to unwind in case of error while the iio_map_list_lock mutex is
> held.

Both looks good to me, FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

if Jonathan is okay with them.

> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> ---
>  drivers/iio/inkern.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index ede99e0..39c1d63 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -24,6 +24,21 @@ struct iio_map_internal {
>  static LIST_HEAD(iio_map_list);
>  static DEFINE_MUTEX(iio_map_list_lock);
>
> +static int iio_map_array_unregister_locked(struct iio_dev *indio_dev)
> +{
> +       int ret = -ENODEV;
> +       struct iio_map_internal *mapi, *next;
> +
> +       list_for_each_entry_safe(mapi, next, &iio_map_list, l) {
> +               if (indio_dev == mapi->indio_dev) {
> +                       list_del(&mapi->l);
> +                       kfree(mapi);
> +                       ret = 0;
> +               }
> +       }
> +       return ret;
> +}
> +
>  int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
>  {
>         int i = 0, ret = 0;
> @@ -57,18 +72,12 @@ EXPORT_SYMBOL_GPL(iio_map_array_register);
>   */
>  int iio_map_array_unregister(struct iio_dev *indio_dev)
>  {
> -       int ret = -ENODEV;
> -       struct iio_map_internal *mapi, *next;
> +       int ret;
>
>         mutex_lock(&iio_map_list_lock);
> -       list_for_each_entry_safe(mapi, next, &iio_map_list, l) {
> -               if (indio_dev == mapi->indio_dev) {
> -                       list_del(&mapi->l);
> -                       kfree(mapi);
> -                       ret = 0;
> -               }
> -       }
> +       ret = iio_map_array_unregister_locked(indio_dev);
>         mutex_unlock(&iio_map_list_lock);
> +
>         return ret;
>  }
>  EXPORT_SYMBOL_GPL(iio_map_array_unregister);
> --
> 2.7.4
>


-- 
With Best Regards,
Andy Shevchenko

[PATCH 2/2] iio:core: In iio_map_array_register() cleanup in case of error.

[Lino Sanfilippo]

In function iio_map_array_register() properly rewind in case of error.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/iio/inkern.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 39c1d63..fe30bcb 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -60,6 +60,8 @@ int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
 		i++;
 	}
 error_ret:
+	if (ret)
+		iio_map_array_unregister_locked(indio_dev);
 	mutex_unlock(&iio_map_list_lock);
 
 	return ret;
-- 
2.7.4

Re: [PATCH v2] iio:core: In map_array_register() cleanup in case of error

[Jonathan Cameron]

On Fri, 16 Oct 2020 18:09:48 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Oct 11, 2020 at 9:24 PM Lino Sanfilippo <LinoSanfilippo@gmx.de> 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.

Better to cleanup and bail out I think.   It's fairly unlikely
a consumer is going to be ready to cope with getting a partial
set of the channels it's expecting.


> 
> > +       /* 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

I argued for the goto, but it is indeed less obviously the right
thing to do once we are using iio_map_array_unregister.

> 
> 
>         mutex_unlock(&iio_map_list_lock);
> if (ret)
>   iio_map_array_unregister();
> return ret;
> 
> Sounds like only a few LOCs are needed.
>