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

[Yicong Yang <yangyicong@huawei.com>]

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