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

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

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?

> ---
>  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:

@@ -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
>