Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3] iio: adc: exynos: do not rely on 'users' counter in ISR
@ 2020-10-06 21:55 dmitry.torokhov
  2020-10-10 16:50 ` Jonathan Cameron
  0 siblings, 1 reply; 2+ messages in thread
From: dmitry.torokhov @ 2020-10-06 21:55 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Kukjin Kim, Krzysztof Kozlowski, Andrzej Pietrasiewicz,
	Michał Mirosław, linux-iio, linux-samsung-soc,
	linux-kernel

The order in which 'users' counter is decremented vs calling drivers'
close() method is implementation specific, and we should not rely on
it. Let's introduce driver private flag and use it to signal ISR
to exit when device is being closed.

This has a side-effect of fixing issue of accessing inut->users
outside of input->mutex protection.

Reported-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Acked-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

v3: fixed typo in exynos_adc_ts_close() per Michał Mirosław
v2: switched from ordinary read/write to READ_ONCE/WRITE_ONCE per Michał
Mirosław

 drivers/iio/adc/exynos_adc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index 22131a677445..908df4b9b93c 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -7,6 +7,7 @@
  *  Copyright (C) 2013 Naveen Krishna Chatradhi <ch.naveen@samsung.com>
  */
 
+#include <linux/compiler.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/interrupt.h>
@@ -135,6 +136,8 @@ struct exynos_adc {
 	u32			value;
 	unsigned int            version;
 
+	bool			ts_enabled;
+
 	bool			read_ts;
 	u32			ts_x;
 	u32			ts_y;
@@ -633,7 +636,7 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
 	bool pressed;
 	int ret;
 
-	while (info->input->users) {
+	while (READ_ONCE(info->ts_enabled)) {
 		ret = exynos_read_s3c64xx_ts(dev, &x, &y);
 		if (ret == -ETIMEDOUT)
 			break;
@@ -712,6 +715,7 @@ static int exynos_adc_ts_open(struct input_dev *dev)
 {
 	struct exynos_adc *info = input_get_drvdata(dev);
 
+	WRITE_ONCE(info->ts_enabled, true);
 	enable_irq(info->tsirq);
 
 	return 0;
@@ -721,6 +725,7 @@ static void exynos_adc_ts_close(struct input_dev *dev)
 {
 	struct exynos_adc *info = input_get_drvdata(dev);
 
+	WRITE_ONCE(info->ts_enabled, false);
 	disable_irq(info->tsirq);
 }
 
-- 
2.28.0.806.g8561365e88-goog


-- 
Dmitry

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

* Re: [PATCH v3] iio: adc: exynos: do not rely on 'users' counter in ISR
  2020-10-06 21:55 [PATCH v3] iio: adc: exynos: do not rely on 'users' counter in ISR dmitry.torokhov
@ 2020-10-10 16:50 ` Jonathan Cameron
  0 siblings, 0 replies; 2+ messages in thread
From: Jonathan Cameron @ 2020-10-10 16:50 UTC (permalink / raw)
  To: dmitry.torokhov
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Kukjin Kim, Krzysztof Kozlowski, Andrzej Pietrasiewicz,
	Michał Mirosław, linux-iio, linux-samsung-soc,
	linux-kernel

On Tue, 6 Oct 2020 14:55:09 -0700
dmitry.torokhov@gmail.com wrote:

> The order in which 'users' counter is decremented vs calling drivers'
> close() method is implementation specific, and we should not rely on
> it. Let's introduce driver private flag and use it to signal ISR
> to exit when device is being closed.
> 
> This has a side-effect of fixing issue of accessing inut->users
> outside of input->mutex protection.
> 
> Reported-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> Acked-by: Krzysztof Kozlowski <krzk@kernel.org>
> Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to work their magic.

Given this doesn't have a fixes tag etc I'm assuming it isn't
high priority etc.  Let me know if it is!

Thanks,

Jonathan
> ---
> 
> v3: fixed typo in exynos_adc_ts_close() per Michał Mirosław
> v2: switched from ordinary read/write to READ_ONCE/WRITE_ONCE per Michał
> Mirosław
> 
>  drivers/iio/adc/exynos_adc.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> index 22131a677445..908df4b9b93c 100644
> --- a/drivers/iio/adc/exynos_adc.c
> +++ b/drivers/iio/adc/exynos_adc.c
> @@ -7,6 +7,7 @@
>   *  Copyright (C) 2013 Naveen Krishna Chatradhi <ch.naveen@samsung.com>
>   */
>  
> +#include <linux/compiler.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/interrupt.h>
> @@ -135,6 +136,8 @@ struct exynos_adc {
>  	u32			value;
>  	unsigned int            version;
>  
> +	bool			ts_enabled;
> +
>  	bool			read_ts;
>  	u32			ts_x;
>  	u32			ts_y;
> @@ -633,7 +636,7 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
>  	bool pressed;
>  	int ret;
>  
> -	while (info->input->users) {
> +	while (READ_ONCE(info->ts_enabled)) {
>  		ret = exynos_read_s3c64xx_ts(dev, &x, &y);
>  		if (ret == -ETIMEDOUT)
>  			break;
> @@ -712,6 +715,7 @@ static int exynos_adc_ts_open(struct input_dev *dev)
>  {
>  	struct exynos_adc *info = input_get_drvdata(dev);
>  
> +	WRITE_ONCE(info->ts_enabled, true);
>  	enable_irq(info->tsirq);
>  
>  	return 0;
> @@ -721,6 +725,7 @@ static void exynos_adc_ts_close(struct input_dev *dev)
>  {
>  	struct exynos_adc *info = input_get_drvdata(dev);
>  
> +	WRITE_ONCE(info->ts_enabled, false);
>  	disable_irq(info->tsirq);
>  }
>  


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

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-06 21:55 [PATCH v3] iio: adc: exynos: do not rely on 'users' counter in ISR dmitry.torokhov
2020-10-10 16:50 ` 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