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

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

[Yicong Yang]

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>
---
 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;
+
 	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

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

[Andi Shyti]

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
> 

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

[Yicong Yang]

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

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

[Andi Shyti]

Hi Yicong,

On Wed, Aug 02, 2023 at 10:39:04AM +0800, Yicong Yang wrote:
> 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.

So that the firmware is running on a controller and memory,
concurrently to the main CPU; which means that you are having
some kind of bus arbitration issue with two masters on the bus.

Anyway, if we are talking about avoiding a NULL pointer
dereference then this is a fix and you need to add the tags
above.

(No need to resend for this, I can do it for you if you convince
me on the part below.)

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

But if i2c_master_xfer() is not called and we receive an irq,
most probably ctrl->completion is NULL. Right? Can this happen?

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.

Thanks for the explanation,
Andi

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

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

[Yicong Yang]

On 2023/8/5 7:30, Andi Shyti wrote:
> Hi Yicong,
> 
> On Wed, Aug 02, 2023 at 10:39:04AM +0800, Yicong Yang wrote:
>> 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.
> 
> So that the firmware is running on a controller and memory,
> concurrently to the main CPU; which means that you are having
> some kind of bus arbitration issue with two masters on the bus.
> 

That is one case. Another case maybe the i2c rtc also locates on the same
bus and the efi-rtc will access the i2c bus.

> Anyway, if we are talking about avoiding a NULL pointer
> dereference then this is a fix and you need to add the tags
> above.
> 
> (No need to resend for this, I can do it for you if you convince
> me on the part below.)

Thanks!

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

Thanks,
Yicong

> Thanks for the explanation,
> Andi
> 
>>> @@ -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
>>>>
>>> .
>>>
> .
> 

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

[Andi Shyti]

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

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

[Andi Shyti]

Hi

On Tue, 01 Aug 2023 20:46:25 +0800, Yicong Yang wrote:
> 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.
> 
> 

With the Fixes tag added, applied to i2c/andi-for-current on

https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git

Please note that this patch may still undergo further evaluation
and the final decision will be made in collaboration with
Wolfram.

Thank you,
Andi

Patches applied
===============
[1/1] i2c: hisi: Only handle the interrupt of the driver's transfer
      commit: 9a5adaf694f5ae8ba8f8e6178d01b5950fc7222a

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 572 bytes --]

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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Geert Uytterhoeven]

 	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