Hi Yicong,
On Tue, Aug 01, 2023 at 08:46:25PM +0800, Yicong Yang wrote:
> From: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
Is this a fix? Then, could you please add:
Fixes: d62fbdb99a85 ("i2c: add support for HiSilicon I2C controller")
Cc: <stable@xxxxxxxxxxxxxxx> # 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
>
