Linux-Hwmon Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] hwmon: da9052: Synchronize access with mfd
@ 2020-05-11 11:02 Sebastian Reichel
  2020-05-11 16:51 ` Guenter Roeck
  0 siblings, 1 reply; 7+ messages in thread
From: Sebastian Reichel @ 2020-05-11 11:02 UTC (permalink / raw)
  To: Support Opensource, Jean Delvare, Guenter Roeck
  Cc: Lee Jones, linux-hwmon, linux-kernel, kernel, Samu Nuutamo,
	Sebastian Reichel

From: Samu Nuutamo <samu.nuutamo@vincit.fi>

When tsi-as-adc is configured it is possible for in7[0123]_input read to
return an incorrect value if a concurrent read to in[456]_input is
performed. This is caused by a concurrent manipulation of the mux
channel without proper locking as hwmon and mfd use different locks for
synchronization.

Switch hwmon to use the same lock as mfd when accessing the TSI channel.

Fixes: 4f16cab19a3d5 ("hwmon: da9052: Add support for TSI channel")
Signed-off-by: Samu Nuutamo <samu.nuutamo@vincit.fi>
[rebase to current master, reword commit message slightly]
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/hwmon/da9052-hwmon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/da9052-hwmon.c b/drivers/hwmon/da9052-hwmon.c
index 53b517dbe7e6..4af2fc309c28 100644
--- a/drivers/hwmon/da9052-hwmon.c
+++ b/drivers/hwmon/da9052-hwmon.c
@@ -244,9 +244,9 @@ static ssize_t da9052_tsi_show(struct device *dev,
 	int channel = to_sensor_dev_attr(devattr)->index;
 	int ret;
 
-	mutex_lock(&hwmon->hwmon_lock);
+	mutex_lock(&hwmon->da9052->auxadc_lock);
 	ret = __da9052_read_tsi(dev, channel);
-	mutex_unlock(&hwmon->hwmon_lock);
+	mutex_unlock(&hwmon->da9052->auxadc_lock);
 
 	if (ret < 0)
 		return ret;
-- 
2.26.2


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

* Re: [PATCH] hwmon: da9052: Synchronize access with mfd
  2020-05-11 11:02 [PATCH] hwmon: da9052: Synchronize access with mfd Sebastian Reichel
@ 2020-05-11 16:51 ` Guenter Roeck
  2020-05-12 11:31   ` Sebastian Reichel
  0 siblings, 1 reply; 7+ messages in thread
From: Guenter Roeck @ 2020-05-11 16:51 UTC (permalink / raw)
  To: Sebastian Reichel, Support Opensource, Jean Delvare
  Cc: Lee Jones, linux-hwmon, linux-kernel, kernel, Samu Nuutamo

On 5/11/20 4:02 AM, Sebastian Reichel wrote:
> From: Samu Nuutamo <samu.nuutamo@vincit.fi>
> 
> When tsi-as-adc is configured it is possible for in7[0123]_input read to
> return an incorrect value if a concurrent read to in[456]_input is
> performed. This is caused by a concurrent manipulation of the mux
> channel without proper locking as hwmon and mfd use different locks for
> synchronization.
> 
> Switch hwmon to use the same lock as mfd when accessing the TSI channel.
> 
> Fixes: 4f16cab19a3d5 ("hwmon: da9052: Add support for TSI channel")
> Signed-off-by: Samu Nuutamo <samu.nuutamo@vincit.fi>
> [rebase to current master, reword commit message slightly]
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Have you explored calling da9052_adc_manual_read() instead ?
At the very least we should have a comment explaining why that
isn't feasible.

Thanks,
Guenter

> ---
>  drivers/hwmon/da9052-hwmon.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/da9052-hwmon.c b/drivers/hwmon/da9052-hwmon.c
> index 53b517dbe7e6..4af2fc309c28 100644
> --- a/drivers/hwmon/da9052-hwmon.c
> +++ b/drivers/hwmon/da9052-hwmon.c
> @@ -244,9 +244,9 @@ static ssize_t da9052_tsi_show(struct device *dev,
>  	int channel = to_sensor_dev_attr(devattr)->index;
>  	int ret;
>  
> -	mutex_lock(&hwmon->hwmon_lock);
> +	mutex_lock(&hwmon->da9052->auxadc_lock);
>  	ret = __da9052_read_tsi(dev, channel);
> -	mutex_unlock(&hwmon->hwmon_lock);
> +	mutex_unlock(&hwmon->da9052->auxadc_lock);
>  
>  	if (ret < 0)
>  		return ret;
> 


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

* Re: [PATCH] hwmon: da9052: Synchronize access with mfd
  2020-05-11 16:51 ` Guenter Roeck
@ 2020-05-12 11:31   ` Sebastian Reichel
  0 siblings, 0 replies; 7+ messages in thread
From: Sebastian Reichel @ 2020-05-12 11:31 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Support Opensource, Jean Delvare, Lee Jones, linux-hwmon,
	linux-kernel, kernel, Samu Nuutamo


[-- Attachment #1: Type: text/plain, Size: 2431 bytes --]

Hi Guenter,

On Mon, May 11, 2020 at 09:51:25AM -0700, Guenter Roeck wrote:
> On 5/11/20 4:02 AM, Sebastian Reichel wrote:
> > From: Samu Nuutamo <samu.nuutamo@vincit.fi>
> > 
> > When tsi-as-adc is configured it is possible for in7[0123]_input read to
> > return an incorrect value if a concurrent read to in[456]_input is
> > performed. This is caused by a concurrent manipulation of the mux
> > channel without proper locking as hwmon and mfd use different locks for
> > synchronization.
> > 
> > Switch hwmon to use the same lock as mfd when accessing the TSI channel.
> > 
> > Fixes: 4f16cab19a3d5 ("hwmon: da9052: Add support for TSI channel")
> > Signed-off-by: Samu Nuutamo <samu.nuutamo@vincit.fi>
> > [rebase to current master, reword commit message slightly]
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> 
> Have you explored calling da9052_adc_manual_read() instead ?
> At the very least we should have a comment explaining why that
> isn't feasible.

da9052_adc_manual_read() writes to DA9052_ADC_MAN_REG, waits for an
IRQ and then reads DA9052_ADC_RES_H_REG/DA9052_ADC_RES_L_REG. The
function called here works with the TSI registers instead. So
calling da9052_adc_manual_read() is not an option.

The reason for the locking problem is, that the same ADC is used
internally by the devices and muxed to different pins in the
background. In continuous touchscreen mode, the device is supposed
to schedule the ADC slots automatically and I assumed this would
also work here when I wrote the original support. Turns out the
device is not smart enough for that :(

-- Sebastian

> Thanks,
> Guenter
> 
> > ---
> >  drivers/hwmon/da9052-hwmon.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/hwmon/da9052-hwmon.c b/drivers/hwmon/da9052-hwmon.c
> > index 53b517dbe7e6..4af2fc309c28 100644
> > --- a/drivers/hwmon/da9052-hwmon.c
> > +++ b/drivers/hwmon/da9052-hwmon.c
> > @@ -244,9 +244,9 @@ static ssize_t da9052_tsi_show(struct device *dev,
> >  	int channel = to_sensor_dev_attr(devattr)->index;
> >  	int ret;
> >  
> > -	mutex_lock(&hwmon->hwmon_lock);
> > +	mutex_lock(&hwmon->da9052->auxadc_lock);
> >  	ret = __da9052_read_tsi(dev, channel);
> > -	mutex_unlock(&hwmon->hwmon_lock);
> > +	mutex_unlock(&hwmon->da9052->auxadc_lock);
> >  
> >  	if (ret < 0)
> >  		return ret;
> > 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

* Re: [PATCH] hwmon: da9052: Synchronize access with mfd
@ 2020-05-13 17:07 Guenter Roeck
  0 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2020-05-13 17:07 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Support Opensource, Jean Delvare, Lee Jones, linux-hwmon,
	linux-kernel, kernel, Samu Nuutamo

On Mon, May 11, 2020 at 01:02:19PM +0200, Sebastian Reichel wrote:
> From: Samu Nuutamo <samu.nuutamo@vincit.fi>
> 
> When tsi-as-adc is configured it is possible for in7[0123]_input read to
> return an incorrect value if a concurrent read to in[456]_input is
> performed. This is caused by a concurrent manipulation of the mux
> channel without proper locking as hwmon and mfd use different locks for
> synchronization.
> 
> Switch hwmon to use the same lock as mfd when accessing the TSI channel.
> 
> Fixes: 4f16cab19a3d5 ("hwmon: da9052: Add support for TSI channel")
> Signed-off-by: Samu Nuutamo <samu.nuutamo@vincit.fi>
> [rebase to current master, reword commit message slightly]
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Applied.

Thanks,
Guenter

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

* Re: [PATCH] hwmon: (da9052) Synchronize access with mfd
  2019-03-18 14:51 ` Guenter Roeck
@ 2019-03-19 13:38   ` Samu Nuutamo
  0 siblings, 0 replies; 7+ messages in thread
From: Samu Nuutamo @ 2019-03-19 13:38 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: linux-hwmon, Support Opensource, Jean Delvare

On Mon, Mar 18, 2019 at 07:51:55AM -0700, Guenter Roeck wrote:
> On 3/18/19 7:29 AM, Samu Nuutamo wrote:
> > When tsi-as-adc is configured it is possible for in7[0123]_input read to
> 
> in{70,71,72,73}_input ?
> 

Yes, and by that I meant the files inside /sys/class/hwmon/hwmon0. The 
race can be triggered by reading both tsi and adc values (in70-73, 
in4-6) concurrently, while verifying that the values are as expected. 

Using a simple test script that read the values continuously I got 
incorrect readings once every few minutes.


> > return an incorrect value if a concurrent read to in[456]_input is
> > performed. This is likely caused by a concurrent manipulation of the mux
> > channel without proper locking as hwmon and mfd use different locks for
> > synchronization.
> > 
> > Switch hwmon to use the same lock as mfd when accessing the TSI channel.
> > 
> > Fixes: 4f16cab19a3d5 ("hwmon: da9052: Add support for TSI channel")
> > Signed-off-by: Samu Nuutamo <samu.nuutamo@vincit.fi>
> > ---
> >   drivers/hwmon/da9052-hwmon.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/hwmon/da9052-hwmon.c b/drivers/hwmon/da9052-hwmon.c
> > index 8ec5bf4ce392..af136a638065 100644
> > --- a/drivers/hwmon/da9052-hwmon.c
> > +++ b/drivers/hwmon/da9052-hwmon.c
> > @@ -249,9 +249,9 @@ static ssize_t da9052_tsi_show(struct device *dev,
> >   	int channel = to_sensor_dev_attr(devattr)->index;
> >   	int ret;
> > -	mutex_lock(&hwmon->hwmon_lock);
> > +	mutex_lock(&hwmon->da9052->auxadc_lock);
> 
> This is, at best, a kludge.
> 
> The hwmon driver doesn't own struct da9052 and should not really access any of its
> structure members. The problem suggests that __da9052_read_tsi() should have
> been implemented in the mfd core, similar to da9052_adc_manual_read().
> 

Not sure if I have resources to do a proper fix at this point.

- Samu

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

* Re: [PATCH] hwmon: (da9052) Synchronize access with mfd
  2019-03-18 14:29 [PATCH] hwmon: (da9052) " Samu Nuutamo
@ 2019-03-18 14:51 ` Guenter Roeck
  2019-03-19 13:38   ` Samu Nuutamo
  0 siblings, 1 reply; 7+ messages in thread
From: Guenter Roeck @ 2019-03-18 14:51 UTC (permalink / raw)
  To: Samu Nuutamo, linux-hwmon; +Cc: Support Opensource, Jean Delvare

On 3/18/19 7:29 AM, Samu Nuutamo wrote:
> When tsi-as-adc is configured it is possible for in7[0123]_input read to

in{70,71,72,73}_input ?

> return an incorrect value if a concurrent read to in[456]_input is
> performed. This is likely caused by a concurrent manipulation of the mux
> channel without proper locking as hwmon and mfd use different locks for
> synchronization.
> 
> Switch hwmon to use the same lock as mfd when accessing the TSI channel.
> 
> Fixes: 4f16cab19a3d5 ("hwmon: da9052: Add support for TSI channel")
> Signed-off-by: Samu Nuutamo <samu.nuutamo@vincit.fi>
> ---
>   drivers/hwmon/da9052-hwmon.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/da9052-hwmon.c b/drivers/hwmon/da9052-hwmon.c
> index 8ec5bf4ce392..af136a638065 100644
> --- a/drivers/hwmon/da9052-hwmon.c
> +++ b/drivers/hwmon/da9052-hwmon.c
> @@ -249,9 +249,9 @@ static ssize_t da9052_tsi_show(struct device *dev,
>   	int channel = to_sensor_dev_attr(devattr)->index;
>   	int ret;
>   
> -	mutex_lock(&hwmon->hwmon_lock);
> +	mutex_lock(&hwmon->da9052->auxadc_lock);

This is, at best, a kludge.

The hwmon driver doesn't own struct da9052 and should not really access any of its
structure members. The problem suggests that __da9052_read_tsi() should have
been implemented in the mfd core, similar to da9052_adc_manual_read().

Guenter

>   	ret = __da9052_read_tsi(dev, channel);
> -	mutex_unlock(&hwmon->hwmon_lock);
> +	mutex_unlock(&hwmon->da9052->auxadc_lock);
>   
>   	if (ret < 0)
>   		return ret;
> 


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

* [PATCH] hwmon: (da9052) Synchronize access with mfd
@ 2019-03-18 14:29 Samu Nuutamo
  2019-03-18 14:51 ` Guenter Roeck
  0 siblings, 1 reply; 7+ messages in thread
From: Samu Nuutamo @ 2019-03-18 14:29 UTC (permalink / raw)
  To: linux-hwmon; +Cc: Support Opensource, Jean Delvare, Guenter Roeck

When tsi-as-adc is configured it is possible for in7[0123]_input read to
return an incorrect value if a concurrent read to in[456]_input is
performed. This is likely caused by a concurrent manipulation of the mux
channel without proper locking as hwmon and mfd use different locks for
synchronization.

Switch hwmon to use the same lock as mfd when accessing the TSI channel.

Fixes: 4f16cab19a3d5 ("hwmon: da9052: Add support for TSI channel")
Signed-off-by: Samu Nuutamo <samu.nuutamo@vincit.fi>
---
 drivers/hwmon/da9052-hwmon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/da9052-hwmon.c b/drivers/hwmon/da9052-hwmon.c
index 8ec5bf4ce392..af136a638065 100644
--- a/drivers/hwmon/da9052-hwmon.c
+++ b/drivers/hwmon/da9052-hwmon.c
@@ -249,9 +249,9 @@ static ssize_t da9052_tsi_show(struct device *dev,
 	int channel = to_sensor_dev_attr(devattr)->index;
 	int ret;
 
-	mutex_lock(&hwmon->hwmon_lock);
+	mutex_lock(&hwmon->da9052->auxadc_lock);
 	ret = __da9052_read_tsi(dev, channel);
-	mutex_unlock(&hwmon->hwmon_lock);
+	mutex_unlock(&hwmon->da9052->auxadc_lock);
 
 	if (ret < 0)
 		return ret;
-- 
2.17.1


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

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-11 11:02 [PATCH] hwmon: da9052: Synchronize access with mfd Sebastian Reichel
2020-05-11 16:51 ` Guenter Roeck
2020-05-12 11:31   ` Sebastian Reichel
  -- strict thread matches above, loose matches on Subject: below --
2020-05-13 17:07 Guenter Roeck
2019-03-18 14:29 [PATCH] hwmon: (da9052) " Samu Nuutamo
2019-03-18 14:51 ` Guenter Roeck
2019-03-19 13:38   ` Samu Nuutamo

Linux-Hwmon Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hwmon/0 linux-hwmon/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-hwmon linux-hwmon/ https://lore.kernel.org/linux-hwmon \
		linux-hwmon@vger.kernel.org
	public-inbox-index linux-hwmon

Example config snippet for mirrors

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


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