All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] iio:trigger: Fix use_count race condition
@ 2013-07-16 14:28 Lars-Peter Clausen
  2013-07-16 15:34 ` Denis CIOCCA
  0 siblings, 1 reply; 3+ messages in thread
From: Lars-Peter Clausen @ 2013-07-16 14:28 UTC (permalink / raw)
  To: Jonathan Cameron; +Cc: Denis Ciocca, linux-iio, Lars-Peter Clausen

When using more than one trigger consumer it can happen that multiple threads
perform a read-modify-update cycle on 'use_count' concurrently. This can cause
updates to be lost and use_count can get stuck at non-zero value, in which case
the IIO core assumes that at least one thread is still running and will wait for
it to finish before running any trigger handlers again. This effectively renders
the trigger disabled and a reboot is necessary before it can be used again. To
fix this make use_count an atomic variable. Also set it to the number of
consumers before starting the first consumer, otherwise it might happen that
use_count drops to 0 even though not all consumers have been run yet.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
Changes since v1:
	* Set use_count to CONFIG_IIO_CONSUMERS_PER_TRIGGER and call
	  iio_trigger_notify_done for all subirqs that are not enabled to avoid a
	  race which could be triggered if the irq was enabled or disabled while
	  iio_trigger_poll() is running.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/industrialio-trigger.c | 34 ++++++++++++++++++++++------------
 include/linux/iio/trigger.h        |  3 ++-
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index 4d6c7d8..47769ae 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -127,12 +127,17 @@ static struct iio_trigger *iio_trigger_find_by_name(const char *name,
 void iio_trigger_poll(struct iio_trigger *trig, s64 time)
 {
 	int i;
-	if (!trig->use_count)
-		for (i = 0; i < CONFIG_IIO_CONSUMERS_PER_TRIGGER; i++)
-			if (trig->subirqs[i].enabled) {
-				trig->use_count++;
+
+	if (!atomic_read(&trig->use_count)) {
+		atomic_set(&trig->use_count, CONFIG_IIO_CONSUMERS_PER_TRIGGER);
+
+		for (i = 0; i < CONFIG_IIO_CONSUMERS_PER_TRIGGER; i++) {
+			if (trig->subirqs[i].enabled)
 				generic_handle_irq(trig->subirq_base + i);
-			}
+			else
+				iio_trigger_notify_done(trig);
+		}
+	}
 }
 EXPORT_SYMBOL(iio_trigger_poll);
 
@@ -146,19 +151,24 @@ EXPORT_SYMBOL(iio_trigger_generic_data_rdy_poll);
 void iio_trigger_poll_chained(struct iio_trigger *trig, s64 time)
 {
 	int i;
-	if (!trig->use_count)
-		for (i = 0; i < CONFIG_IIO_CONSUMERS_PER_TRIGGER; i++)
-			if (trig->subirqs[i].enabled) {
-				trig->use_count++;
+
+	if (!atomic_read(&trig->use_count)) {
+		atomic_set(&trig->use_count, CONFIG_IIO_CONSUMERS_PER_TRIGGER);
+
+		for (i = 0; i < CONFIG_IIO_CONSUMERS_PER_TRIGGER; i++) {
+			if (trig->subirqs[i].enabled)
 				handle_nested_irq(trig->subirq_base + i);
-			}
+			else
+				iio_trigger_notify_done(trig);
+		}
+	}
 }
 EXPORT_SYMBOL(iio_trigger_poll_chained);
 
 void iio_trigger_notify_done(struct iio_trigger *trig)
 {
-	trig->use_count--;
-	if (trig->use_count == 0 && trig->ops && trig->ops->try_reenable)
+	if (atomic_dec_and_test(&trig->use_count) && trig->ops &&
+		trig->ops->try_reenable)
 		if (trig->ops->try_reenable(trig))
 			/* Missed an interrupt so launch new poll now */
 			iio_trigger_poll(trig, 0);
diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h
index 3869c52..369cf2c 100644
--- a/include/linux/iio/trigger.h
+++ b/include/linux/iio/trigger.h
@@ -8,6 +8,7 @@
  */
 #include <linux/irq.h>
 #include <linux/module.h>
+#include <linux/atomic.h>
 
 #ifndef _IIO_TRIGGER_H_
 #define _IIO_TRIGGER_H_
@@ -61,7 +62,7 @@ struct iio_trigger {
 
 	struct list_head		list;
 	struct list_head		alloc_list;
-	int use_count;
+	atomic_t			use_count;
 
 	struct irq_chip			subirq_chip;
 	int				subirq_base;
-- 
1.8.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH v2] iio:trigger: Fix use_count race condition
  2013-07-16 14:28 [PATCH v2] iio:trigger: Fix use_count race condition Lars-Peter Clausen
@ 2013-07-16 15:34 ` Denis CIOCCA
  2013-07-20  9:19   ` Jonathan Cameron
  0 siblings, 1 reply; 3+ messages in thread
From: Denis CIOCCA @ 2013-07-16 15:34 UTC (permalink / raw)
  To: Lars-Peter Clausen; +Cc: Jonathan Cameron, linux-iio

Hi Lars,

works fine!
Tested-by: Denis Ciocca <denis.ciocca@st.com>

Denis

On Tuesday, July 16, 2013 04:28:41 PM Lars-Peter Clausen wrote:
> When using more than one trigger consumer it can happen that multiple
> threads perform a read-modify-update cycle on 'use_count' concurrently.
> This can cause updates to be lost and use_count can get stuck at non-zero
> value, in which case the IIO core assumes that at least one thread is still
> running and will wait for it to finish before running any trigger handlers
> again. This effectively renders the trigger disabled and a reboot is
> necessary before it can be used again. To fix this make use_count an atomic
> variable. Also set it to the number of consumers before starting the first
> consumer, otherwise it might happen that use_count drops to 0 even though
> not all consumers have been run yet.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
> Changes since v1:
> 	* Set use_count to CONFIG_IIO_CONSUMERS_PER_TRIGGER and call
> 	  iio_trigger_notify_done for all subirqs that are not enabled to avoid a
> 	  race which could be triggered if the irq was enabled or disabled while
> 	  iio_trigger_poll() is running.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>  drivers/iio/industrialio-trigger.c | 34 ++++++++++++++++++++++------------
>  include/linux/iio/trigger.h        |  3 ++-
>  2 files changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-trigger.c
> b/drivers/iio/industrialio-trigger.c index 4d6c7d8..47769ae 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -127,12 +127,17 @@ static struct iio_trigger
> *iio_trigger_find_by_name(const char *name, void iio_trigger_poll(struct
> iio_trigger *trig, s64 time)
>  {
>  	int i;
> -	if (!trig->use_count)
> -		for (i = 0; i < CONFIG_IIO_CONSUMERS_PER_TRIGGER; i++)
> -			if (trig->subirqs[i].enabled) {
> -				trig->use_count++;
> +
> +	if (!atomic_read(&trig->use_count)) {
> +		atomic_set(&trig->use_count, CONFIG_IIO_CONSUMERS_PER_TRIGGER);
> +
> +		for (i = 0; i < CONFIG_IIO_CONSUMERS_PER_TRIGGER; i++) {
> +			if (trig->subirqs[i].enabled)
>  				generic_handle_irq(trig->subirq_base + i);
> -			}
> +			else
> +				iio_trigger_notify_done(trig);
> +		}
> +	}
>  }
>  EXPORT_SYMBOL(iio_trigger_poll);
> 
> @@ -146,19 +151,24 @@ EXPORT_SYMBOL(iio_trigger_generic_data_rdy_poll);
>  void iio_trigger_poll_chained(struct iio_trigger *trig, s64 time)
>  {
>  	int i;
> -	if (!trig->use_count)
> -		for (i = 0; i < CONFIG_IIO_CONSUMERS_PER_TRIGGER; i++)
> -			if (trig->subirqs[i].enabled) {
> -				trig->use_count++;
> +
> +	if (!atomic_read(&trig->use_count)) {
> +		atomic_set(&trig->use_count, CONFIG_IIO_CONSUMERS_PER_TRIGGER);
> +
> +		for (i = 0; i < CONFIG_IIO_CONSUMERS_PER_TRIGGER; i++) {
> +			if (trig->subirqs[i].enabled)
>  				handle_nested_irq(trig->subirq_base + i);
> -			}
> +			else
> +				iio_trigger_notify_done(trig);
> +		}
> +	}
>  }
>  EXPORT_SYMBOL(iio_trigger_poll_chained);
> 
>  void iio_trigger_notify_done(struct iio_trigger *trig)
>  {
> -	trig->use_count--;
> -	if (trig->use_count == 0 && trig->ops && trig->ops->try_reenable)
> +	if (atomic_dec_and_test(&trig->use_count) && trig->ops &&
> +		trig->ops->try_reenable)
>  		if (trig->ops->try_reenable(trig))
>  			/* Missed an interrupt so launch new poll now */
>  			iio_trigger_poll(trig, 0);
> diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h
> index 3869c52..369cf2c 100644
> --- a/include/linux/iio/trigger.h
> +++ b/include/linux/iio/trigger.h
> @@ -8,6 +8,7 @@
>   */
>  #include <linux/irq.h>
>  #include <linux/module.h>
> +#include <linux/atomic.h>
> 
>  #ifndef _IIO_TRIGGER_H_
>  #define _IIO_TRIGGER_H_
> @@ -61,7 +62,7 @@ struct iio_trigger {
> 
>  	struct list_head		list;
>  	struct list_head		alloc_list;
> -	int use_count;
> +	atomic_t			use_count;
> 
>  	struct irq_chip			subirq_chip;
>  	int				subirq_base;

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH v2] iio:trigger: Fix use_count race condition
  2013-07-16 15:34 ` Denis CIOCCA
@ 2013-07-20  9:19   ` Jonathan Cameron
  0 siblings, 0 replies; 3+ messages in thread
From: Jonathan Cameron @ 2013-07-20  9:19 UTC (permalink / raw)
  To: Denis CIOCCA; +Cc: Lars-Peter Clausen, Jonathan Cameron, linux-iio

On 07/16/2013 04:34 PM, Denis CIOCCA wrote:
> Hi Lars,
> 
> works fine!
> Tested-by: Denis Ciocca <denis.ciocca@st.com>
> 
> Denis
> 
> On Tuesday, July 16, 2013 04:28:41 PM Lars-Peter Clausen wrote:
>> When using more than one trigger consumer it can happen that multiple
>> threads perform a read-modify-update cycle on 'use_count' concurrently.
>> This can cause updates to be lost and use_count can get stuck at non-zero
>> value, in which case the IIO core assumes that at least one thread is still
>> running and will wait for it to finish before running any trigger handlers
>> again. This effectively renders the trigger disabled and a reboot is
>> necessary before it can be used again. To fix this make use_count an atomic
>> variable. Also set it to the number of consumers before starting the first
>> consumer, otherwise it might happen that use_count drops to 0 even though
>> not all consumers have been run yet.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Nice clean solution. Thanks.

Applied to the fixes-togreg branch of iio.git.

>> ---
>> Changes since v1:
>> 	* Set use_count to CONFIG_IIO_CONSUMERS_PER_TRIGGER and call
>> 	  iio_trigger_notify_done for all subirqs that are not enabled to avoid a
>> 	  race which could be triggered if the irq was enabled or disabled while
>> 	  iio_trigger_poll() is running.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> ---
>>  drivers/iio/industrialio-trigger.c | 34 ++++++++++++++++++++++------------
>>  include/linux/iio/trigger.h        |  3 ++-
>>  2 files changed, 24 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/iio/industrialio-trigger.c
>> b/drivers/iio/industrialio-trigger.c index 4d6c7d8..47769ae 100644
>> --- a/drivers/iio/industrialio-trigger.c
>> +++ b/drivers/iio/industrialio-trigger.c
>> @@ -127,12 +127,17 @@ static struct iio_trigger
>> *iio_trigger_find_by_name(const char *name, void iio_trigger_poll(struct
>> iio_trigger *trig, s64 time)
>>  {
>>  	int i;
>> -	if (!trig->use_count)
>> -		for (i = 0; i < CONFIG_IIO_CONSUMERS_PER_TRIGGER; i++)
>> -			if (trig->subirqs[i].enabled) {
>> -				trig->use_count++;
>> +
>> +	if (!atomic_read(&trig->use_count)) {
>> +		atomic_set(&trig->use_count, CONFIG_IIO_CONSUMERS_PER_TRIGGER);
>> +
>> +		for (i = 0; i < CONFIG_IIO_CONSUMERS_PER_TRIGGER; i++) {
>> +			if (trig->subirqs[i].enabled)
>>  				generic_handle_irq(trig->subirq_base + i);
>> -			}
>> +			else
>> +				iio_trigger_notify_done(trig);
>> +		}
>> +	}
>>  }
>>  EXPORT_SYMBOL(iio_trigger_poll);
>>
>> @@ -146,19 +151,24 @@ EXPORT_SYMBOL(iio_trigger_generic_data_rdy_poll);
>>  void iio_trigger_poll_chained(struct iio_trigger *trig, s64 time)
>>  {
>>  	int i;
>> -	if (!trig->use_count)
>> -		for (i = 0; i < CONFIG_IIO_CONSUMERS_PER_TRIGGER; i++)
>> -			if (trig->subirqs[i].enabled) {
>> -				trig->use_count++;
>> +
>> +	if (!atomic_read(&trig->use_count)) {
>> +		atomic_set(&trig->use_count, CONFIG_IIO_CONSUMERS_PER_TRIGGER);
>> +
>> +		for (i = 0; i < CONFIG_IIO_CONSUMERS_PER_TRIGGER; i++) {
>> +			if (trig->subirqs[i].enabled)
>>  				handle_nested_irq(trig->subirq_base + i);
>> -			}
>> +			else
>> +				iio_trigger_notify_done(trig);
>> +		}
>> +	}
>>  }
>>  EXPORT_SYMBOL(iio_trigger_poll_chained);
>>
>>  void iio_trigger_notify_done(struct iio_trigger *trig)
>>  {
>> -	trig->use_count--;
>> -	if (trig->use_count == 0 && trig->ops && trig->ops->try_reenable)
>> +	if (atomic_dec_and_test(&trig->use_count) && trig->ops &&
>> +		trig->ops->try_reenable)
>>  		if (trig->ops->try_reenable(trig))
>>  			/* Missed an interrupt so launch new poll now */
>>  			iio_trigger_poll(trig, 0);
>> diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h
>> index 3869c52..369cf2c 100644
>> --- a/include/linux/iio/trigger.h
>> +++ b/include/linux/iio/trigger.h
>> @@ -8,6 +8,7 @@
>>   */
>>  #include <linux/irq.h>
>>  #include <linux/module.h>
>> +#include <linux/atomic.h>
>>
>>  #ifndef _IIO_TRIGGER_H_
>>  #define _IIO_TRIGGER_H_
>> @@ -61,7 +62,7 @@ struct iio_trigger {
>>
>>  	struct list_head		list;
>>  	struct list_head		alloc_list;
>> -	int use_count;
>> +	atomic_t			use_count;
>>
>>  	struct irq_chip			subirq_chip;
>>  	int				subirq_base;--
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-07-20  9:19 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-16 14:28 [PATCH v2] iio:trigger: Fix use_count race condition Lars-Peter Clausen
2013-07-16 15:34 ` Denis CIOCCA
2013-07-20  9:19   ` Jonathan Cameron

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.