On Fri, Oct 11, 2019 at 09:46:56PM +0000, Andrew Duggan wrote: > Hi Evan, > > On 10/8/19 3:36 PM, Evan Green wrote: > > rmi_process_interrupt_requests() calls handle_nested_irq() for > > each interrupt status bit it finds. If the irq domain mapping for > > this bit had not yet been set up, then it ends up calling > > handle_nested_irq(0), which causes a NULL pointer dereference. > > > > There's already code that masks the irq_status bits coming out of the > > hardware with current_irq_mask, presumably to avoid this situation. > > However current_irq_mask seems to more reflect the actual mask set > > in the hardware rather than the IRQs software has set up and registered > > for. For example, in rmi_driver_reset_handler(), the current_irq_mask > > is initialized based on what is read from the hardware. If the reset > > value of this mask enables IRQs that Linux has not set up yet, then > > we end up in this situation. > > > > There appears to be a third unused bitmask that used to serve this > > purpose, fn_irq_bits. Use that bitmask instead of current_irq_mask > > to avoid calling handle_nested_irq() on IRQs that have not yet been > > set up. > > Yes, it looks like the code which ensured that there was a function > handler to handle the IRQ was removed when the driver switched to using > an irq domain. Setting the fn_irq_bits and using them instead of > current_irq_mask in rmi_process_interrupt_requests() makes sense to me. > > Andrew > > > Signed-off-by: Evan Green <evgreen@xxxxxxxxxxxx> > Reviewed-by: Andrew Duggan <aduggan@xxxxxxxxxxxxx> Applied, thank you. > > --- > > > > drivers/input/rmi4/rmi_driver.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c > > index 772493b1f665..190b9974526b 100644 > > --- a/drivers/input/rmi4/rmi_driver.c > > +++ b/drivers/input/rmi4/rmi_driver.c > > @@ -146,7 +146,7 @@ static int rmi_process_interrupt_requests(struct rmi_device *rmi_dev) > > } > > > > mutex_lock(&data->irq_mutex); > > - bitmap_and(data->irq_status, data->irq_status, data->current_irq_mask, > > + bitmap_and(data->irq_status, data->irq_status, data->fn_irq_bits, > > data->irq_count); > > /* > > * At this point, irq_status has all bits that are set in the > > @@ -385,6 +385,8 @@ static int rmi_driver_set_irq_bits(struct rmi_device *rmi_dev, > > bitmap_copy(data->current_irq_mask, data->new_irq_mask, > > data->num_of_irq_regs); > > > > + bitmap_or(data->fn_irq_bits, data->fn_irq_bits, mask, data->irq_count); > > + > > error_unlock: > > mutex_unlock(&data->irq_mutex); > > return error; > > @@ -398,6 +400,8 @@ static int rmi_driver_clear_irq_bits(struct rmi_device *rmi_dev, > > struct device *dev = &rmi_dev->dev; > > > > mutex_lock(&data->irq_mutex); > > + bitmap_andnot(data->fn_irq_bits, > > + data->fn_irq_bits, mask, data->irq_count); > > bitmap_andnot(data->new_irq_mask, > > data->current_irq_mask, mask, data->irq_count); > > -- Dmitry
