Messages in this thread |  | | Date | Wed, 29 Apr 2026 11:41:54 +0100 | | From | Jonathan Cameron <> | | Subject | Re: [PATCH v3] iio: magnetometer: rm3100: Modernize locking and refactor control flow |
| |
On Tue, 28 Apr 2026 17:26:00 -0500 Maxwell Doose <m32285159@gmail.com> wrote:
> Replace mutex_lock() and mutex_unlock() calls in rm3100-core.c with > the more modern guard(mutex)() family. This will help modernize the > driver and bring it up-to-date with modern available macros/functions. > > While replacing mutex_lock() and mutex_unlock(), the critical sections > of rm3100_read_mag() and rm3100_get_samp_freq() have been extended to > include negligible operations for cleaner logic. > > Add new helper-wrapper function rm3100_guarded_regmap_bulk_read() to > help keep rm3100_trigger_handler() switch-cases clean while maintaining > mutex locking and avoiding re-entrancy risks from potential callbacks. > > While at it, remove redundant gotos where applicable, and use direct > returns instead. > > Suggested-by: Jonathan Cameron <jic23@kernel.org> > Signed-off-by: Maxwell Doose <m32285159@gmail.com> Hi Maxwell.
The use of the helper worked out well but it also made me look at some existing code and wonder why it is so complex!
> static irqreturn_t rm3100_trigger_handler(int irq, void *p) > { > struct iio_poll_func *pf = p; > @@ -468,11 +481,10 @@ static irqreturn_t rm3100_trigger_handler(int irq, void *p) > struct regmap *regmap = data->regmap; > int ret, i, bit; > > - mutex_lock(&data->lock); > switch (scan_mask) {
> default: > for_each_set_bit(bit, &scan_mask, mask_len) { > - ret = regmap_bulk_read(regmap, RM3100_REG_MX2 + 3 * bit, > - data->buffer, 3); > - if (ret < 0) { > - mutex_unlock(&data->lock); > + ret = rm3100_guarded_regmap_bulk_read(&data->lock, regmap, > + RM3100_REG_MX2 + 3 * bit, > + data->buffer, 3);
Hmm. Not one for this patch, but the only reason this doesn't hammer the lock is the bitmap is one hot. Which is why the read is always to the same location in the buffer. That could have been a lot easier to spot if instead of a loop find_first_bit() was used.
I don't think we can ever get here with no bits set but maybe check that as well (it's not supposed to be possible as doesn't make any sense to measure nothing ;)
> + if (ret < 0) > goto done; > - } > } > - mutex_unlock(&data->lock); > } > /* > * Always using the same buffer so that we wouldn't need to set the
|  |