Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] iio:core: In map_array_register() cleanup in case of error
@ 2020-10-11 12:41 Lino Sanfilippo
  2020-10-11 15:07 ` Jonathan Cameron
  0 siblings, 1 reply; 10+ messages in thread
From: Lino Sanfilippo @ 2020-10-11 12:41 UTC (permalink / raw)
  To: jic23; +Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel, 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iio:core: In map_array_register() cleanup in case of error
  2020-10-11 12:41 [PATCH] iio:core: In map_array_register() cleanup in case of error Lino Sanfilippo
@ 2020-10-11 15:07 ` Jonathan Cameron
  2020-10-11 18:05   ` Lino Sanfilippo
  2020-10-11 18:22   ` [PATCH v2] " Lino Sanfilippo
  0 siblings, 2 replies; 10+ messages in thread
From: Jonathan Cameron @ 2020-10-11 15:07 UTC (permalink / raw)
  To: Lino Sanfilippo; +Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel

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
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iio:core: In map_array_register() cleanup in case of error
  2020-10-11 15:07 ` Jonathan Cameron
@ 2020-10-11 18:05   ` Lino Sanfilippo
  2020-10-11 18:22   ` [PATCH v2] " Lino Sanfilippo
  1 sibling, 0 replies; 10+ messages in thread
From: Lino Sanfilippo @ 2020-10-11 18:05 UTC (permalink / raw)
  To: Jonathan Cameron; +Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2] iio:core: In map_array_register() cleanup in case of error
  2020-10-11 15:07 ` Jonathan Cameron
  2020-10-11 18:05   ` Lino Sanfilippo
@ 2020-10-11 18:22   ` Lino Sanfilippo
  2020-10-16 15:09     ` Andy Shevchenko
  1 sibling, 1 reply; 10+ messages in thread
From: Lino Sanfilippo @ 2020-10-11 18:22 UTC (permalink / raw)
  To: jic23; +Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel, 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] iio:core: In map_array_register() cleanup in case of error
  2020-10-11 18:22   ` [PATCH v2] " Lino Sanfilippo
@ 2020-10-16 15:09     ` Andy Shevchenko
  2020-10-18  1:11       ` Lino Sanfilippo
  2020-10-18  9:56       ` [PATCH v2] iio:core: In map_array_register() " Jonathan Cameron
  0 siblings, 2 replies; 10+ messages in thread
From: Andy Shevchenko @ 2020-10-16 15:09 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald, linux-iio, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] iio:core: In map_array_register() cleanup in case of error
  2020-10-16 15:09     ` Andy Shevchenko
@ 2020-10-18  1:11       ` Lino Sanfilippo
  2020-10-18  1:11         ` [PATCH 1/2] iio:core: Introduce unlocked version of iio_map_array_unregister() Lino Sanfilippo
  2020-10-18  1:11         ` [PATCH 2/2] iio:core: In iio_map_array_register() cleanup in case of error Lino Sanfilippo
  2020-10-18  9:56       ` [PATCH v2] iio:core: In map_array_register() " Jonathan Cameron
  1 sibling, 2 replies; 10+ messages in thread
From: Lino Sanfilippo @ 2020-10-18  1:11 UTC (permalink / raw)
  To: andy.shevchenko; +Cc: jic23, knaack.h, lars, pmeerw, linux-iio, linux-kernel

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] iio:core: Introduce unlocked version of iio_map_array_unregister()
  2020-10-18  1:11       ` Lino Sanfilippo
@ 2020-10-18  1:11         ` Lino Sanfilippo
  2020-10-18 18:21           ` Andy Shevchenko
  2020-10-18  1:11         ` [PATCH 2/2] iio:core: In iio_map_array_register() cleanup in case of error Lino Sanfilippo
  1 sibling, 1 reply; 10+ messages in thread
From: Lino Sanfilippo @ 2020-10-18  1:11 UTC (permalink / raw)
  To: andy.shevchenko
  Cc: jic23, knaack.h, lars, pmeerw, linux-iio, linux-kernel, 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] iio:core: In iio_map_array_register() cleanup in case of error.
  2020-10-18  1:11       ` Lino Sanfilippo
  2020-10-18  1:11         ` [PATCH 1/2] iio:core: Introduce unlocked version of iio_map_array_unregister() Lino Sanfilippo
@ 2020-10-18  1:11         ` Lino Sanfilippo
  1 sibling, 0 replies; 10+ messages in thread
From: Lino Sanfilippo @ 2020-10-18  1:11 UTC (permalink / raw)
  To: andy.shevchenko
  Cc: jic23, knaack.h, lars, pmeerw, linux-iio, linux-kernel, 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] iio:core: In map_array_register() cleanup in case of error
  2020-10-16 15:09     ` Andy Shevchenko
  2020-10-18  1:11       ` Lino Sanfilippo
@ 2020-10-18  9:56       ` Jonathan Cameron
  1 sibling, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2020-10-18  9:56 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Lino Sanfilippo, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald, linux-iio, Linux Kernel Mailing List

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.
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] iio:core: Introduce unlocked version of iio_map_array_unregister()
  2020-10-18  1:11         ` [PATCH 1/2] iio:core: Introduce unlocked version of iio_map_array_unregister() Lino Sanfilippo
@ 2020-10-18 18:21           ` Andy Shevchenko
  0 siblings, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2020-10-18 18:21 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald, linux-iio, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-11 12:41 [PATCH] iio:core: In map_array_register() cleanup in case of error Lino Sanfilippo
2020-10-11 15:07 ` Jonathan Cameron
2020-10-11 18:05   ` Lino Sanfilippo
2020-10-11 18:22   ` [PATCH v2] " Lino Sanfilippo
2020-10-16 15:09     ` Andy Shevchenko
2020-10-18  1:11       ` Lino Sanfilippo
2020-10-18  1:11         ` [PATCH 1/2] iio:core: Introduce unlocked version of iio_map_array_unregister() Lino Sanfilippo
2020-10-18 18:21           ` Andy Shevchenko
2020-10-18  1:11         ` [PATCH 2/2] iio:core: In iio_map_array_register() cleanup in case of error Lino Sanfilippo
2020-10-18  9:56       ` [PATCH v2] iio:core: In map_array_register() " Jonathan Cameron

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org
	public-inbox-index linux-iio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git