| Message ID | 20190318142953.GA23692@samu-ThinkPad-T480s (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (da9052) Synchronize access with mfd | expand |
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; >
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
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;
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(-)