diff mbox series

[4/6] hwmon: (tmp464) Use multi-byte regmap operations

Message ID 20240716230050.2049534-5-linux@roeck-us.net (mailing list archive)
State Accepted
Headers show
Series hwmon: Use multi-byte regmap operations | expand

Commit Message

Guenter Roeck July 16, 2024, 11 p.m. UTC 
Use multi-byte regmap operations where possible to reduce code size
and the need for mutex protection.

No functional changes.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/tmp464.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

Comments

Tzung-Bi Shih July 17, 2024, 1:35 p.m. UTC | #1 
On Tue, Jul 16, 2024 at 04:00:48PM -0700, Guenter Roeck wrote:
> @@ -147,11 +147,11 @@ static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val
>  {
[...]
> -	mutex_lock(&data->update_lock);
> -
>  	switch (attr) {
>  	case hwmon_temp_max_alarm:
>  		err = regmap_read(regmap, TMP464_THERM_STATUS_REG, &regval);
> @@ -172,26 +172,27 @@ static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val
>  		 * complete. That means we have to cache the value internally
>  		 * for one measurement cycle and report the cached value.
>  		 */
> +		mutex_lock(&data->update_lock);
>  		if (!data->valid || time_after(jiffies, data->last_updated +
>  					       msecs_to_jiffies(data->update_interval))) {
>  			err = regmap_read(regmap, TMP464_REMOTE_OPEN_REG, &regval);
>  			if (err < 0)
> -				break;
> +				goto unlock;
>  			data->open_reg = regval;
>  			data->last_updated = jiffies;
>  			data->valid = true;
>  		}
>  		*val = !!(data->open_reg & BIT(channel + 7));
> +unlock:
> +		mutex_unlock(&data->update_lock);
>  		break;

I think the function can entirely drop the mutex.  Only [1] needs it.

[1]: https://elixir.bootlin.com/linux/v6.10/source/drivers/hwmon/tmp464.c#L313
Guenter Roeck July 17, 2024, 2:37 p.m. UTC | #2 
On 7/17/24 06:35, Tzung-Bi Shih wrote:
> On Tue, Jul 16, 2024 at 04:00:48PM -0700, Guenter Roeck wrote:
>> @@ -147,11 +147,11 @@ static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val
>>   {
> [...]
>> -	mutex_lock(&data->update_lock);
>> -
>>   	switch (attr) {
>>   	case hwmon_temp_max_alarm:
>>   		err = regmap_read(regmap, TMP464_THERM_STATUS_REG, &regval);
>> @@ -172,26 +172,27 @@ static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val
>>   		 * complete. That means we have to cache the value internally
>>   		 * for one measurement cycle and report the cached value.
>>   		 */
>> +		mutex_lock(&data->update_lock);
>>   		if (!data->valid || time_after(jiffies, data->last_updated +
>>   					       msecs_to_jiffies(data->update_interval))) {
>>   			err = regmap_read(regmap, TMP464_REMOTE_OPEN_REG, &regval);
>>   			if (err < 0)
>> -				break;
>> +				goto unlock;
>>   			data->open_reg = regval;
>>   			data->last_updated = jiffies;
>>   			data->valid = true;
>>   		}
>>   		*val = !!(data->open_reg & BIT(channel + 7));
>> +unlock:
>> +		mutex_unlock(&data->update_lock);
>>   		break;
> 
> I think the function can entirely drop the mutex.  Only [1] needs it.
> 

It is needed to protect updating open_reg. Otherwise a second process
could enter the code and read the register again, which would then return
different (cleared) values. As result open_reg might contain the temporarily
"cleared" values.

Process 1			Process 2
err = regmap_read();
data->open_reg = regval;
				err = regmap_read();
				data->open_reg = regval;
data->last_updated = jiffies;
...

Thanks,
Guenter
Tzung-Bi Shih July 18, 2024, 1:29 a.m. UTC | #3 
On Wed, Jul 17, 2024 at 07:37:10AM -0700, Guenter Roeck wrote:
> On 7/17/24 06:35, Tzung-Bi Shih wrote:
> > On Tue, Jul 16, 2024 at 04:00:48PM -0700, Guenter Roeck wrote:
> > > @@ -147,11 +147,11 @@ static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val
> > >   {
> > [...]
> > > -	mutex_lock(&data->update_lock);
> > > -
> > >   	switch (attr) {
> > >   	case hwmon_temp_max_alarm:
> > >   		err = regmap_read(regmap, TMP464_THERM_STATUS_REG, &regval);
> > > @@ -172,26 +172,27 @@ static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val
> > >   		 * complete. That means we have to cache the value internally
> > >   		 * for one measurement cycle and report the cached value.
> > >   		 */
> > > +		mutex_lock(&data->update_lock);
> > >   		if (!data->valid || time_after(jiffies, data->last_updated +
> > >   					       msecs_to_jiffies(data->update_interval))) {
> > >   			err = regmap_read(regmap, TMP464_REMOTE_OPEN_REG, &regval);
> > >   			if (err < 0)
> > > -				break;
> > > +				goto unlock;
> > >   			data->open_reg = regval;
> > >   			data->last_updated = jiffies;
> > >   			data->valid = true;
> > >   		}
> > >   		*val = !!(data->open_reg & BIT(channel + 7));
> > > +unlock:
> > > +		mutex_unlock(&data->update_lock);
> > >   		break;
> > 
> > I think the function can entirely drop the mutex.  Only [1] needs it.
> > 
> 
> It is needed to protect updating open_reg. Otherwise a second process
> could enter the code and read the register again, which would then return
> different (cleared) values. As result open_reg might contain the temporarily
> "cleared" values.
> 
> Process 1			Process 2
> err = regmap_read();
> data->open_reg = regval;
> 				err = regmap_read();
> 				data->open_reg = regval;
> data->last_updated = jiffies;
> ...

Ack.

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
diff mbox series

Patch

diff --git a/drivers/hwmon/tmp464.c b/drivers/hwmon/tmp464.c
index 3ee1137533d6..0a7c0448835b 100644
--- a/drivers/hwmon/tmp464.c
+++ b/drivers/hwmon/tmp464.c
@@ -147,11 +147,11 @@  static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val
 {
 	struct tmp464_data *data = dev_get_drvdata(dev);
 	struct regmap *regmap = data->regmap;
-	unsigned int regval, regval2;
+	unsigned int regs[2];
+	unsigned int regval;
+	u16 regvals[2];
 	int err = 0;
 
-	mutex_lock(&data->update_lock);
-
 	switch (attr) {
 	case hwmon_temp_max_alarm:
 		err = regmap_read(regmap, TMP464_THERM_STATUS_REG, &regval);
@@ -172,26 +172,27 @@  static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val
 		 * complete. That means we have to cache the value internally
 		 * for one measurement cycle and report the cached value.
 		 */
+		mutex_lock(&data->update_lock);
 		if (!data->valid || time_after(jiffies, data->last_updated +
 					       msecs_to_jiffies(data->update_interval))) {
 			err = regmap_read(regmap, TMP464_REMOTE_OPEN_REG, &regval);
 			if (err < 0)
-				break;
+				goto unlock;
 			data->open_reg = regval;
 			data->last_updated = jiffies;
 			data->valid = true;
 		}
 		*val = !!(data->open_reg & BIT(channel + 7));
+unlock:
+		mutex_unlock(&data->update_lock);
 		break;
 	case hwmon_temp_max_hyst:
-		err = regmap_read(regmap, TMP464_THERM_LIMIT[channel], &regval);
+		regs[0] = TMP464_THERM_LIMIT[channel];
+		regs[1] = TMP464_TEMP_HYST_REG;
+		err = regmap_multi_reg_read(regmap, regs, regvals, 2);
 		if (err < 0)
 			break;
-		err = regmap_read(regmap, TMP464_TEMP_HYST_REG, &regval2);
-		if (err < 0)
-			break;
-		regval -= regval2;
-		*val = temp_from_reg(regval);
+		*val = temp_from_reg(regvals[0] - regvals[1]);
 		break;
 	case hwmon_temp_max:
 		err = regmap_read(regmap, TMP464_THERM_LIMIT[channel], &regval);
@@ -200,14 +201,12 @@  static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val
 		*val = temp_from_reg(regval);
 		break;
 	case hwmon_temp_crit_hyst:
-		err = regmap_read(regmap, TMP464_THERM2_LIMIT[channel], &regval);
+		regs[0] = TMP464_THERM2_LIMIT[channel];
+		regs[1] = TMP464_TEMP_HYST_REG;
+		err = regmap_multi_reg_read(regmap, regs, regvals, 2);
 		if (err < 0)
 			break;
-		err = regmap_read(regmap, TMP464_TEMP_HYST_REG, &regval2);
-		if (err < 0)
-			break;
-		regval -= regval2;
-		*val = temp_from_reg(regval);
+		*val = temp_from_reg(regvals[0] - regvals[1]);
 		break;
 	case hwmon_temp_crit:
 		err = regmap_read(regmap, TMP464_THERM2_LIMIT[channel], &regval);
@@ -239,8 +238,6 @@  static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val
 		break;
 	}
 
-	mutex_unlock(&data->update_lock);
-
 	return err;
 }