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

[Tzung-Bi Shih <tzungbi@xxxxxxxxxx>]

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@xxxxxxxxxx>