Re: [PATCH] i2c: hisi: Only handle the interrupt of the driver's transfer

[Andi Shyti <andi.shyti@kernel.org>]

Hi Yicong,

On Wed, Aug 02, 2023 at 10:39:04AM +0800, Yicong Yang wrote:
> On 2023/8/2 6:15, Andi Shyti wrote:
> > Hi Yicong,
> > 
> > On Tue, Aug 01, 2023 at 08:46:25PM +0800, Yicong Yang wrote:
> >> From: Yicong Yang <yangyicong@hisilicon.com>
> >>
> >> The controller may be shared with other port, for example the firmware.
> >> Handle the interrupt from other sources will cause crash since some
> >> data are not initialized. So only handle the interrupt of the driver's
> >> transfer and discard others.
> >>
> >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> > 
> > Is this a fix? Then, could you please add:
> > 
> > Fixes: d62fbdb99a85 ("i2c: add support for HiSilicon I2C controller")
> > Cc: <stable@vger.kernel.org> # v5.13+
> > 
> > What kind of crash is this? Is it a NULL pointer dereference?
> 
> I not quite sure this is a fix of the driver. On some use case the controller is
> shared between the firmware and the OS and we have no synchronization method
> from the hardware. A transfer started by the firmware cause the interrupt handled
> by the driver and cause a NULL pointer dereference.

So that the firmware is running on a controller and memory,
concurrently to the main CPU; which means that you are having
some kind of bus arbitration issue with two masters on the bus.

Anyway, if we are talking about avoiding a NULL pointer
dereference then this is a fix and you need to add the tags
above.

(No need to resend for this, I can do it for you if you convince
me on the part below.)

> >> ---
> >>  drivers/i2c/busses/i2c-hisi.c | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-hisi.c b/drivers/i2c/busses/i2c-hisi.c
> >> index e067671b3ce2..8328da4bc3ec 100644
> >> --- a/drivers/i2c/busses/i2c-hisi.c
> >> +++ b/drivers/i2c/busses/i2c-hisi.c
> >> @@ -330,6 +330,14 @@ static irqreturn_t hisi_i2c_irq(int irq, void *context)
> >>  	struct hisi_i2c_controller *ctlr = context;
> >>  	u32 int_stat;
> >>  
> >> +	/*
> >> +	 * Don't handle the interrupt if cltr->completion is NULL. We may
> >> +	 * reach here because the interrupt is spurious or the transfer is
> >> +	 * started by another port rather than us.
> >> +	 */
> >> +	if (!ctlr->completion)
> >> +		return IRQ_NONE;
> > 
> > Is this the place you should really check for completion being
> > NULL? By reading the code I don't exclude that completion at this
> > stage might be NULL.
> > 
> > Can it be that the real fix is this one instead:
> 
> Maybe not. If we handle the case as late as below, we'll operate the hardware
> which should be handled by the firmware which start the transfer. So we check
> it as early as possible.

But if i2c_master_xfer() is not called and we receive an irq,
most probably ctrl->completion is NULL. Right? Can this happen?

I can't really tell the sequence for enabling/disabling the
interrupt in this device. They might happen in
hisi_i2c_start_xfer() for enabling and in hisi_i2c_xfer_msg() for
desabling at the last message; which makes the scenario above a
bit difficult, indeed.

Thanks for the explanation,
Andi

> > @@ -352,7 +352,7 @@ static irqreturn_t hisi_i2c_irq(int irq, void *context)
> >          * Only use TRANS_CPLT to indicate the completion. On error cases we'll
> >          * get two interrupts, INT_ERR first then TRANS_CPLT.
> >          */
> > -       if (int_stat & HISI_I2C_INT_TRANS_CPLT) {
> > +       if (ctrl->completion && (int_stat & HISI_I2C_INT_TRANS_CPLT)) {
> >                 hisi_i2c_disable_int(ctlr, HISI_I2C_INT_ALL);
> >                 hisi_i2c_clear_int(ctlr, HISI_I2C_INT_ALL);
> >                 complete(ctlr->completion);
> > 
> > Anyway, this whole completion management smells a bit racy to me.
> > 
> > Andi
> > 
> >>  	int_stat = readl(ctlr->iobase + HISI_I2C_INT_MSTAT);
> >>  	hisi_i2c_clear_int(ctlr, int_stat);
> >>  	if (!(int_stat & HISI_I2C_INT_ALL))
> >> -- 
> >> 2.24.0
> >>
> > .
> >