Re: [PATCH v1] iio: health: max30102: fix NULL dereference in interrupt handler

On Sun, Aug 02, 2026 at 03:26:09AM +0100, Jonathan Cameron wrote:
> > +	if (!indio_dev->active_scan_mask) {
> 
> That is racy as this could be going on in parallel with the buffer being disabled
> so we check here but it's gone before it is accessed.

I understand what you mean. I will remove this active_scan_mask check.

> Which leads me to suggest alternative fix - check INT_STATUS and if nothing
> set it isn't our interrupt.  That doesn't get into the potential buffer
> enabled / disabled races but should close your condition I think
> - looks like it is read in the fifo part below. You may need to refactor a
> little to not read it twice.

This makes a lot of sense. For the v2 patch, I will plan to read
INT_STATUS once at the top of the handler and pass that value into
max30102_fifo_count() so it is not read twice. Consequently I will
remove the INT_STATUS read from max30102_fifo_count(), too.

For checking INT_STATUS to see if it isn't our interrupt, instead of
checking if nothing is set, I was thinking to check the
MAX30102_REG_INT_STATUS_FIFO_RDY bit specifically because that is the 
only interrupt enabled in max30102_chip_init().

This will close the reproducer I was hitting earlier as you said. Since
we only ever reach bitmap_weight() when FIFO_RDY is set, the NULL dereference
is avoided. I think there is a theoretical window if the buffer is disabled
between the FIFO_RDY check and bitmap_weight() call, but as you said,
closing this would need the buffered mode claim. I plan to leave that
out of this fix.


On Sat, Aug 01, 2026 at 16:18:00 +0100, David Lechner wrote:

> This is one of the reasons why you are supposed to turn off power
> before moving wires. ;-)

Fair point, I was pretty careless.

> IRQ_NONE would cause the handler to just run again as soon as it
> exits, so it is almost never the right value to return. IRQ_HANDLED
> is correct.

Got it. I will keep IRQ_HANDLED for the v2.

> There isn't anything we could do other than log the error.

Understood. Since the INT_STATUS read at the top of the interrupt handler now
determines if it is our interrupt, I will check the return value of
regmap_read() and log the error on failure.