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@xxxxxxxxxxxxxxx>
--
With Best Regards,
Andy Shevchenko
