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

[Guenter Roeck <linux@xxxxxxxxxxxx>]

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