Re: [PATCH] i2c: designware: Fix handling of real but unexpected device interrupts

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Sep 27, 2022 at 04:56:44PM +0300, Jarkko Nikula wrote:
> Commit c7b79a752871 ("mfd: intel-lpss: Add Intel Alder Lake PCH-S PCI
> IDs") caused a regression on certain Gigabyte motherboards for Intel
> Alder Lake-S where system crashes to NULL pointer dereference in
> i2c_dw_xfer_msg() when system resumes from S3 sleep state ("deep").
> 
> I was able to debug the issue on Gigabyte Z690 AORUS ELITE and made
> following notes:
> 
> - Issue happens when resuming from S3 but not when resuming from
>   "s2idle"
> - PCI device 00:15.0 == i2c_designware.0 is already in D0 state when
>   system enters into pci_pm_resume_noirq() while all other i2c_designware
>   PCI devices are in D3. Devices were runtime suspended and in D3 prior
>   entering into suspend
> - Interrupt comes after pci_pm_resume_noirq() when device interrupts are
>   re-enabled
> - According to register dump the interrupt really comes from the
>   i2c_designware.0. Controller is enabled, I2C target address register
>   points to a one detectable I2C device address 0x60 and the
>   DW_IC_RAW_INTR_STAT register START_DET, STOP_DET, ACTIVITY and
>   TX_EMPTY bits are set indicating completed I2C transaction.
> 
> My guess is that the firmware uses this controller to communicate with
> an on-board I2C device during resume but does not disable the controller
> before giving control to an operating system.
> 
> I was told the UEFI update fixes this but never the less it revealed the
> driver is not ready to handle TX_EMPTY (or RX_FULL) interrupt when device
> is supposed to be idle and state variables are not set (especially the
> dev->msgs pointer which may point to NULL or stale old data).
> 
> Introduce a new software status flag STATUS_ACTIVE indicating when the
> controller is active in driver point of view. Now treat all interrupts
> that occur when is not set as unexpected and mask all interrupts from
> the controller.

...

>  #define STATUS_IDLE			0x0

A side note: I think the clearer is to use STATUS_MASK and use
'&= ~STATUS_MASK' instead of '= STATUS_IDLE' in the affected pieces
of the code.

> -#define STATUS_WRITE_IN_PROGRESS	0x1
> -#define STATUS_READ_IN_PROGRESS		0x2
> +#define STATUS_ACTIVE			0x1
> +#define STATUS_WRITE_IN_PROGRESS	0x2
> +#define STATUS_READ_IN_PROGRESS		0x4

Can we at the same time replace them with BIT()?

...

Otherwise looks good to me,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko