| Message ID | 20220922233356.1712262-1-floridsleeves@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1] drivers/hwmon/adm9240: fix data race in adm9240_fan_read | expand |
On 9/22/22 16:33, Li Zhong wrote: > In > adm9240_read() > adm9240_fan_read() > adm9240_write_fan_div(), > > it assumes that the caller of adm9240_write_fan_div() must hold > data->update_lock. Otherwise, it may cause data races when data is > updated by other threads. > > Signed-off-by: Li Zhong <floridsleeves@gmail.com> > --- > drivers/hwmon/adm9240.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c > index 483cd757abd3..d93ae3147994 100644 > --- a/drivers/hwmon/adm9240.c > +++ b/drivers/hwmon/adm9240.c > @@ -501,6 +501,7 @@ static int adm9240_fan_read(struct device *dev, u32 attr, int channel, long *val > > switch (attr) { > case hwmon_fan_input: > + mutex_lock(&data->update_lock); > err = regmap_read(data->regmap, ADM9240_REG_FAN(channel), ®val); > if (err < 0) > return err; The mutex needs to be released here. > @@ -511,6 +512,7 @@ static int adm9240_fan_read(struct device *dev, u32 attr, int channel, long *val > if (err) > return err; > } > + mutex_unlock(&data->update_lock); > *val = FAN_FROM_REG(regval, BIT(data->fan_div[channel])); > break; > case hwmon_fan_div:
On Thu, Sep 22, 2022 at 4:45 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 9/22/22 16:33, Li Zhong wrote: > > In > > adm9240_read() > > adm9240_fan_read() > > adm9240_write_fan_div(), > > > > it assumes that the caller of adm9240_write_fan_div() must hold > > data->update_lock. Otherwise, it may cause data races when data is > > updated by other threads. > > > > Signed-off-by: Li Zhong <floridsleeves@gmail.com> > > --- > > drivers/hwmon/adm9240.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c > > index 483cd757abd3..d93ae3147994 100644 > > --- a/drivers/hwmon/adm9240.c > > +++ b/drivers/hwmon/adm9240.c > > @@ -501,6 +501,7 @@ static int adm9240_fan_read(struct device *dev, u32 attr, int channel, long *val > > > > switch (attr) { > > case hwmon_fan_input: > > + mutex_lock(&data->update_lock); > > err = regmap_read(data->regmap, ADM9240_REG_FAN(channel), ®val); > > if (err < 0) > > return err; > > The mutex needs to be released here. > > > @@ -511,6 +512,7 @@ static int adm9240_fan_read(struct device *dev, u32 attr, int channel, long *val > > if (err) > > return err; > > } > > + mutex_unlock(&data->update_lock); > > *val = FAN_FROM_REG(regval, BIT(data->fan_div[channel])); > > break; > > case hwmon_fan_div: > Thanks. Fixed in v2 patch.
On 9/22/22 16:33, Li Zhong wrote: > In > adm9240_read() > adm9240_fan_read() > adm9240_write_fan_div(), > > it assumes that the caller of adm9240_write_fan_div() must hold > data->update_lock. Otherwise, it may cause data races when data is > updated by other threads. > > Signed-off-by: Li Zhong <floridsleeves@gmail.com> > --- > drivers/hwmon/adm9240.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c > index 483cd757abd3..d93ae3147994 100644 > --- a/drivers/hwmon/adm9240.c > +++ b/drivers/hwmon/adm9240.c > @@ -501,6 +501,7 @@ static int adm9240_fan_read(struct device *dev, u32 attr, int channel, long *val > > switch (attr) { > case hwmon_fan_input: > + mutex_lock(&data->update_lock); > err = regmap_read(data->regmap, ADM9240_REG_FAN(channel), ®val); > if (err < 0) > return err; > @@ -511,6 +512,7 @@ static int adm9240_fan_read(struct device *dev, u32 attr, int channel, long *val > if (err) > return err; The mutex also needs to be released here (in the error handling path). > } > + mutex_unlock(&data->update_lock); > *val = FAN_FROM_REG(regval, BIT(data->fan_div[channel])); > break; > case hwmon_fan_div:
diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c index 483cd757abd3..d93ae3147994 100644 --- a/drivers/hwmon/adm9240.c +++ b/drivers/hwmon/adm9240.c @@ -501,6 +501,7 @@ static int adm9240_fan_read(struct device *dev, u32 attr, int channel, long *val switch (attr) { case hwmon_fan_input: + mutex_lock(&data->update_lock); err = regmap_read(data->regmap, ADM9240_REG_FAN(channel), ®val); if (err < 0) return err; @@ -511,6 +512,7 @@ static int adm9240_fan_read(struct device *dev, u32 attr, int channel, long *val if (err) return err; } + mutex_unlock(&data->update_lock); *val = FAN_FROM_REG(regval, BIT(data->fan_div[channel])); break; case hwmon_fan_div:
In adm9240_read() adm9240_fan_read() adm9240_write_fan_div(), it assumes that the caller of adm9240_write_fan_div() must hold data->update_lock. Otherwise, it may cause data races when data is updated by other threads. Signed-off-by: Li Zhong <floridsleeves@gmail.com> --- drivers/hwmon/adm9240.c | 2 ++ 1 file changed, 2 insertions(+)