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

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

Hi Yicong,

[...]

> >>>> @@ -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?
> > 
> 
> Yes, this is the case.
> 
> > 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.
> > 
> 
> The driver will keep the interrupt disabled if no transfer in progress.
> But since the transfer is driven by the interrupt so if the firmware
> start the transfer it will enable the interrupt. In such case the driver
> will receive an interrupt on the Tx FIFO empty, etc and since the
> transfer is not started by the driver ctlr->completion is not
> initialized.

OK... makes sense...

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Thanks!
Andi