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

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

[Jarkko Nikula]

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.

Fixes: c7b79a752871 ("mfd: intel-lpss: Add Intel Alder Lake PCH-S PCI IDs")
Reported-by: Samuel Clark <slc2015@gmail.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=215907
Cc: stable@vger.kernel.org # v5.12+
Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
Hans: Are you able to test this on your Baytrail collection with shared
I2C controller and PUNIT so that patch doesn't break anything? I believe
even if the Linux interrupt for such shared I2C controller is shared e.g.
with the i2c-i801 the PUNIT access should not get affected by this
interrupt masking. I think PUNIT access won't use interrupts but you
never know... We have a MRD7 that has shared I2C controller with PUNIT
but Linux interrupt is not shared. I.e. unexpected interrupt case and
masking doesn't get hit.
i2c-designware-slave.c is not fully in sync with this new status flag since
STATUS_ACTIVE gets overwritten there and unexpected interrupt case is
needlessly hit in case of shared interrupt and this controller is
suspended but both of those can go to an another patchset.
---
 drivers/i2c/busses/i2c-designware-core.h   |  7 +++++--
 drivers/i2c/busses/i2c-designware-master.c | 13 +++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 70b80e710990..4d3a3b464ecd 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -126,8 +126,9 @@
  * status codes
  */
 #define STATUS_IDLE			0x0
-#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
 
 /*
  * operation modes
@@ -334,12 +335,14 @@ void i2c_dw_disable_int(struct dw_i2c_dev *dev);
 
 static inline void __i2c_dw_enable(struct dw_i2c_dev *dev)
 {
+	dev->status |= STATUS_ACTIVE;
 	regmap_write(dev->map, DW_IC_ENABLE, 1);
 }
 
 static inline void __i2c_dw_disable_nowait(struct dw_i2c_dev *dev)
 {
 	regmap_write(dev->map, DW_IC_ENABLE, 0);
+	dev->status &= ~STATUS_ACTIVE;
 }
 
 void __i2c_dw_disable(struct dw_i2c_dev *dev);
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index 44a94b225ed8..dc3c5a15a95b 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -716,6 +716,19 @@ static int i2c_dw_irq_handler_master(struct dw_i2c_dev *dev)
 	u32 stat;
 
 	stat = i2c_dw_read_clear_intrbits(dev);
+
+	if (!(dev->status & STATUS_ACTIVE)) {
+		/*
+		 * Unexpected interrupt in driver point of view. State
+		 * variables are either unset or stale so acknowledge and
+		 * disable interrupts for suppressing further interrupts if
+		 * interrupt really came from this HW (E.g. firmware has left
+		 * the HW active).
+		 */
+		regmap_write(dev->map, DW_IC_INTR_MASK, 0);
+		return 0;
+	}
+
 	if (stat & DW_IC_INTR_TX_ABRT) {
 		dev->cmd_err |= DW_IC_ERR_TX_ABRT;
 		dev->status = STATUS_IDLE;
-- 
2.35.1

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

[Andy Shevchenko]

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

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

[Jarkko Nikula]

+ Hans

I forgot to Cc you yesterday even especially had a question for you :-(

Patch here and my comment to Andy below.

https://patchwork.ozlabs.org/project/linux-i2c/patch/20220927135644.1656369-1-jarkko.nikula@linux.intel.com/

On 9/27/22 17:35, Andy Shevchenko wrote:
> On Tue, Sep 27, 2022 at 04:56:44PM +0300, Jarkko Nikula wrote:
>>   #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>
> 
Good points. I'll add these to follow up patches.

Jarkko

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

[Hans de Goede]

Hi Jarkko,

On 9/28/22 07:49, Jarkko Nikula wrote:
> + Hans
> 
> I forgot to Cc you yesterday even especially had a question for you :-(

Yes I can test this on BYT and CHT hw where one of the i2c-designware
busses is shared with the PUNIT. I have added this patch to me personal
tree which I regularly test on these kinda devices.

I will let you know if I hit any issues, if you don't hear anything from
me then you can assume I have not hit any issues :)

You also mention being especially interested on testing on hw where
the interrupt line is shared with other devices. I don't think the
i2c-designware interrupts are ever shared with other hw on the BYT/CHT
devices I have.

Regards,

Hans



> Patch here and my comment to Andy below.
> 
> https://patchwork.ozlabs.org/project/linux-i2c/patch/20220927135644.1656369-1-jarkko.nikula@linux.intel.com/
> 
> On 9/27/22 17:35, Andy Shevchenko wrote:
>> On Tue, Sep 27, 2022 at 04:56:44PM +0300, Jarkko Nikula wrote:
>>>   #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>
>>
> Good points. I'll add these to follow up patches.
> 
> Jarkko
> 

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

[Wolfram Sang]

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


> I will let you know if I hit any issues, if you don't hear anything from
> me then you can assume I have not hit any issues :)

Ehrm, how long should I wait before applying the patch?


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

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

[Hans de Goede]

Hi,

On 10/2/22 00:40, Wolfram Sang wrote:
> 
>> I will let you know if I hit any issues, if you don't hear anything from
>> me then you can assume I have not hit any issues :)
> 
> Ehrm, how long should I wait before applying the patch?

Just go for it, I don't expect any problems.

Also I'm doing some work on a Bay Trail device with a shared PMIC bus
today. So if there are any obvious problems then I should hit them today.

Regards,

Hans

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

[Wolfram Sang]

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

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.
> 
> Fixes: c7b79a752871 ("mfd: intel-lpss: Add Intel Alder Lake PCH-S PCI IDs")
> Reported-by: Samuel Clark <slc2015@gmail.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215907
> Cc: stable@vger.kernel.org # v5.12+
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Applied to for-current, thanks!


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