hwmon: (da9052) Synchronize access with mfd
diff mbox series

Message ID 20190318142953.GA23692@samu-ThinkPad-T480s
State Changes Requested
Headers show
Series
  • hwmon: (da9052) Synchronize access with mfd
Related show

Commit Message

Samu Nuutamo March 18, 2019, 2:29 p.m. UTC
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(-)

Comments

Guenter Roeck March 18, 2019, 2:51 p.m. UTC | #1
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;
>
Samu Nuutamo March 19, 2019, 1:38 p.m. UTC | #2
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

Patch
diff mbox series

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;