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

Andi Shyti
2023-08-09T20:08:57Z

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