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

[Guenter Roeck <linux@roeck-us.net>]

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