[PATCH] hwmon: da9052: Synchronize access with mfd

[PATCH] hwmon: da9052: Synchronize access with mfd

[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

Re: [PATCH] hwmon: da9052: Synchronize access with mfd

[Guenter Roeck]

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

Re: [PATCH] hwmon: da9052: Synchronize access with mfd

[Sebastian Reichel]

[-- 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 --]

Re: [PATCH] hwmon: da9052: Synchronize access with mfd

[Guenter Roeck]

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

[PATCH] hwmon: (da9052) Synchronize access with mfd

[Samu Nuutamo]

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

Re: [PATCH] hwmon: (da9052) Synchronize access with mfd

[Guenter Roeck]

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

Re: [PATCH] hwmon: (da9052) Synchronize access with mfd

[Samu Nuutamo]

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