| Message ID | 20230801124625.63587-1-yangyicong@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: hisi: Only handle the interrupt of the driver's transfer | expand |
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 >
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. > >> --- >> 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. > > @@ -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 >> > . >
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
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> Applied to for-current, thanks! I updated the comment to mention that another port is likely the firmware. Similar like in the above text.
Hi Wolfram, Andi, On Mon, 14 Aug 2023, Wolfram Sang wrote: > 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> > > Applied to for-current, thanks! I updated the comment to mention that > another port is likely the firmware. Similar like in the above text. Today's renesas-drivers merge of i2c-host/i2c/andi-for-current got a conflict in: drivers/i2c/busses/i2c-hisi.c between commit fff67c1b17ee0939 ("i2c: hisi: Only handle the interrupt of the driver's transfer") in i2c/i2c/for-next and commit 9a5adaf694f5ae8b ("i2c: hisi: Only handle the interrupt of the driver's transfer") in i2c-host/i2c/andi-for-current. I took the version from i2c/i2c/for-next, as that contained the extra comment. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
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; + int_stat = readl(ctlr->iobase + HISI_I2C_INT_MSTAT); hisi_i2c_clear_int(ctlr, int_stat); if (!(int_stat & HISI_I2C_INT_ALL))