Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 17 Oct 2019 11:04:03 -0700
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
To:     Andrew Duggan <aduggan@synaptics.com>
Cc:     Evan Green <evgreen@chromium.org>, Pan Bian <bianpan2016@163.com>,
        Allison Randal <allison@lohutok.net>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Kate Stewart <kstewart@linuxfoundation.org>,
        "linux-input@vger.kernel.org" <linux-input@vger.kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Nick Dyer <nick@shmanahar.org>,
        Christopher Heiny <Cheiny@synaptics.com>
Subject: Re: [PATCH] Input: synaptics-rmi4 - Avoid processing unknown IRQs
Message-ID: <20191017180403.GK35946@dtor-ws>
References: <20191008223657.163366-1-evgreen@chromium.org>
 <a6e517a2-51ca-431a-88f8-8bfa0a867dc6@synaptics.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a6e517a2-51ca-431a-88f8-8bfa0a867dc6@synaptics.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@chromium.org>
> Reviewed-by: Andrew Duggan <aduggan@synaptics.com>

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