linux-kernel - Re: [PATCH 2/2] iio: mpl3115: add threshold events support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aQMtUUp_QoR4l3nd@smile.fi.intel.com>
Date: Thu, 30 Oct 2025 11:18:09 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Antoni Pokusinski <apokusinski01@...il.com>
Cc: jic23@...nel.org, dlechner@...libre.com, nuno.sa@...log.com,
	andy@...nel.org, marcelo.schmitt1@...il.com,
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: mpl3115: add threshold events support

On Wed, Oct 29, 2025 at 11:46:05PM +0100, Antoni Pokusinski wrote:
> On Wed, Oct 29, 2025 at 10:24:49AM +0200, Andy Shevchenko wrote:
> > On Tue, Oct 28, 2025 at 10:33:52PM +0100, Antoni Pokusinski wrote:

Please, remove context you are agree with!
Otherwise raise your point(s).

...


> > > -	u8 ctrl_reg1 = data->ctrl_reg1;
> > > -	u8 ctrl_reg4 = data->ctrl_reg4;
> > > +	u8 ctrl_reg1, ctrl_reg4;
> > 
> > > +	guard(mutex)(&data->lock);
> > 
> > Why this is moved? Before the access to the data->ctrl* was done without
> > locking. Is it an existing bug?
> > 
> Since this patchset adds `write_event_config()` in which CTRL_REG1.ACTIVE
> and CTRL_REG4 are modified, the lock now needs to guard the read of
> data->ctrl_regX as well. Otherwise, we could have e.g. 2 concurrent
> threads executing `set_trigger_state()` and `write_event_config()` that
> would read data->ctrl_regX at the same time and then one would overwrite
> the other's values in `config_interrupt()`.
> 
> In the current driver I don't think there is any bug in here. The only
> place (except probe) where the data->ctrl_regX is modified is
> `config_interrupt()`, called from `set_trigger_state()`. If we had
> concurrent calls to this function, then the final values of CTRL_REG1
> and CTRL_REG4 would simply depend on which thread is scheduled as the last one.
> With the `guard(mutex)` before accessing data->ctrl_reg1, the situation
> would be exactly the same.

I see, can you summarize this in the commit message as well?
And/or in the code near to the lock description.

> > > +	ctrl_reg1 = data->ctrl_reg1;
> > > +	ctrl_reg4 = data->ctrl_reg4;
> > >  
> > >  	if (state) {
> > >  		ctrl_reg1 |= MPL3115_CTRL1_ACTIVE;
> > >  		ctrl_reg4 |= MPL3115_CTRL4_INT_EN_DRDY;
> > >  	} else {
> > > -		ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
> > >  		ctrl_reg4 &= ~MPL3115_CTRL4_INT_EN_DRDY;
> > > -	}
> > >  
> > > -	guard(mutex)(&data->lock);
> > > +		if (!ctrl_reg4)
> > > +			ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
> > > +	}

-- 
With Best Regards,
Andy Shevchenko