possible defect and fix for HAL SMBUS interrupt processing

[bdolan]

Hello,

I am using the HAL for multi-master SMBUS operations on an STM32H753. I ran into a scenario in which the event handler interrupt function (I2C4_EV_IRQHandler() in my case) was repeatedly firing such that there was no CPU available for other code to run. 
It seems that the logic within HAL_SMBUS_EV_IRQHandler(), which the above function calls, may not be correctly identifying the source of the interrupt, and not clearing it. 
This code:

  /* SMBUS in mode Transmitter ---------------------------------------------------*/
  if ((SMBUS_CHECK_IT_SOURCE(tmpcr1value, (SMBUS_IT_TCI | SMBUS_IT_STOPI |
                                           SMBUS_IT_NACKI | SMBUS_IT_TXI)) != RESET) &&
      ((SMBUS_CHECK_FLAG(tmpisrvalue, SMBUS_FLAG_TXIS) != RESET) ||
       (SMBUS_CHECK_FLAG(tmpisrvalue, SMBUS_FLAG_TCR) != RESET) ||
       (SMBUS_CHECK_FLAG(tmpisrvalue, SMBUS_FLAG_TC) != RESET) ||
       (SMBUS_CHECK_FLAG(tmpisrvalue, SMBUS_FLAG_STOPF) != RESET) ||
       (SMBUS_CHECK_FLAG(tmpisrvalue, SMBUS_FLAG_AF) != RESET)))
  {

calls SMBUS_CHECK_IT_SOURCE() to see if certain interrupts are enabled in the control register, before checking the associated statuses in the status register. However, looking at the definition of this macro, this code is not checking whether ANY of the four interrupts are enabled - it is instead checking if ALL of the four interrupts are enabled:

#define SMBUS_CHECK_IT_SOURCE(__CR1__, __IT__)          ((((__CR1__) & (__IT__)) == (__IT__)) ? SET : RESET)

I believe the intent was probably to check whether ANY of these interrupts are enabled (?).
The same possibly-incorrect pattern is repeated again below under "SMBUS in mode Receiver", but is done correctly under "SMBUS in mode Listener Only".
E.g., in my particular case, only the SMBUS_IT_TCI/TCIE interrupt enable bit was set in CR1 (the other three were clear). And the TC bit was set in the ISR register, needing to be cleared, but the if condition above was not passing and not allowing SMBUS_Master_ISR() to execute and potentially clear the interrupt.
I compared this SMBUS HAL code to the general I2C HAL code, and saw that the I2C HAL code does not follow this pattern - it checks only one bit at a time with I2C_CHECK_IT_SOURCE().
Similar HAL SMBUS code seems to also appear for processors other than the H7 that I am using. 

Thanks,

Brian

[Saket_Om]

Hello @bdolan 

In the standard HAL usage, the situation you describe cannot occur. When calling functions such as `HAL_SMBUS_Master_Transmit_IT()`, the HAL always enables the full group of related interrupts through `SMBUS_Enable_IRQ()` (i.e. `SMBUS_IT_ERRI | SMBUS_IT_TCI | SMBUS_IT_STOPI | SMBUS_IT_NACKI | SMBUS_IT_TXI` for transmit). Therefore, in normal operation there is no case where only one of these bits (like `SMBUS_IT_TCI`) is enabled on its own, and the grouped check in `HAL_SMBUS_EV_IRQHandler()` is consistent with this configuration. The issue you observed could only happen if the interrupt enables are modified manually outside the HAL flow.

[bdolan]

Hi Saket_Om, thanks for looking into this and your reply. I had noticed that in most places the HAL enables and disables all of these flags together, via the SMBUS_IT_TX macro, as you mentioned. However, I somehow got into the state that I mentioned, even though I am not sure what led to this - attached are screenshots of the CR1 and ISR registers at the time of the original issue. You can see that the RXIE and TCIE were set in the CR1 register.

The code I am running does not manipulate these registers directly - it exclusively uses HAL_SMBUS calls. However, it handles a multi-master bus, and makes both master and slave calls into the HAL. I assume that somehow this switching of modes triggered the problem. The following HAL_SMBUS functions are called:
HAL_SMBUS_Master_Transmit_IT()
HAL_SMBUS_Master_Receive_IT()
HAL_SMBUS_Slave_Receive_IT()
HAL_SMBUS_EnableListen_IT()
HAL_SMBUS_DisableListen_IT()

Thanks,

Brian

[TDK]

Can you show your code? Are you calling SMBUS functions from both the main thread and an interrupt? These calls are not thread-safe. In particular, are you calling HAL_SMBUS_DisableListen_IT from an interrupt while a call is ongoing in the main thread?

I traced through the HAL logic, seems okay to me.

[bdolan]

Thank you, TDK. Yes, this code is in fact making calls into the HAL from both the thread context and interrupt context, and perhaps that is the problem. The calls look like this:

called from thread context only:

HAL_SMBUS_Master_Transmit_IT()

HAL_SMBUS_Master_Receive_IT()

HAL_SMBUS_DisableListen_IT()

HAL_SMBUS_EnableListen_IT() called from:

thread context,

interrupt context: HAL_SMBUS_ErrorCallback(), HAL_SMBUS_ListenCpltCallback()

HAL_SMBUS_Slave_Receive_IT() called from:

interrupt context: HAL_SMBUS_AddrCallback()

A simplified version of the code is attached.

The code sets up the STM32 in a type of bus snooping listener/slave mode by default, and every ~1 second switches out of listener/slave mode into master mode to poll information from a smart battery and smart charger.

FYI the design was partially based on the tutorials at:

https://controllerstech.com/stm32-hal/i2c-slave-tutorials/

Thanks,

Brian